[pve-devel] [PATCH v1 pve-common 00/18] Introduction of libproxmox-backup-client-perl

[pve-devel] [PATCH v1 pve-common 00/18] Introduction of libproxmox-backup-client-perl

[Max Carrara]

Introduction of libproxmox-backup-client-perl - v1
==================================================

Description
-----------

This series' main goal is to split the `PVE::PBSClient` Perl module from
the `libpve-common-perl` Debian package while improving and documenting
its code. The new `libproxmox-backup-client-perl` Debian package is
introduced at the end of this series.

While this looks like a lot, most patches, except patch 08 which
documents the `PVE::PBSClient` module, are rather small (tiny, even) and
rather localized (for the most part).

Code Style Changes: Patches 01 - 07
***********************************

The patches 01 - 07 contain a handful of smaller changes regarding code
style and readability.

Most of them, except patch 01, should be able to be dropped if they're
not desired or deemed unnecessary / too pedantic (though I haven't
tested this).

If desired, those patches can also be squashed into one or more larger
commits.

Documentation: Patch 08
***********************

Patch 08 provides some basic documentation on the `PVE::PBSClient`
module overall and adds docstrings for each public subroutine / method.

This is done mostly because a lot of the module's capabilities /
functionalities / invariants aren't really obvious - at best, some of
the behaviour is mentioned in a comment here or there, or has to be
deciphered from the private method's code itself.

Consumers of this module, in my opinion, shouldn't have to figure all of
these things out themselves, so patch 08 was made to address this.

After all, a couple minutes of reading documentation can save hours of
debugging and discovering things through trial-and-error.

Functional Changes: Patches 09 - 16
***********************************

The patches 09 - 16 contain various improvements regarding the module's
functionalities themselves, aiming to make *some* things here and there
a little more ergonimic.

For example, the password and encryption key file setter methods
currently don't create the directory in which the files are stored in
recursively (`mkdir` vs `mkdir -p`), which is addressed in patch 09.

Methods that shouldn't actually return anything (and yet return `0` if
the underlying `run_command` call was successful) are also made to
explicitly not return anything. While changes like these might also seem
somewhat pedantic, they prevent consumers of the module from
accidentally / inadvertedly rely on behaviour that isn't documented or
even intended, especially in the case of Perl's implicit returns.

Note that because none of our code uses any of the methods (only the
`get_repository` function is used), some of these changes deliberately
"break API" here - figured it would be a good opportunity to while we
still have the chance. Any recommendations in that regard are therefore
welcome.

Package Split: Patches 17 and 18
********************************

Finally, patches 17 and 18 make it possible to move `PVE::PBSClient`
into its own Debian package.

Patch 17 introduces the underlying support for building multiple Debian
packages, updating the root `Makefile` to support / be aware of multiple
binary packages.

Patch 18 then updates `debian/control` and adds a `.install` file for
both the old and the new package.

Note Regarding Version Bumps
----------------------------

The very last patch, patch 18, contains a placeholder version `X.Y.Z`
in the changes added to the `debian/control` file. This placeholder must
be substituted with the desired version when applying the patch or
bumping the package.

Note that the same version must be used for all `X.Y.Z` placeholders,
though I assume that this is known already.

There is only one package that uses `PVE::PBSClient`, which is
`qemu-server`. This package requires `libproxmox-backup-client-perl` as
dependency, should this series be applied.

Note Regarding API Breakage
---------------------------

As mentioned in the section regarding patches 09 - 16 above, some
methods' behaviours are deliberately changed in order to be more
developer-friendly.

Because almost none of our code depends on the `PVE::PBSClient` module,
this package split is a good opportunity for further changes in that
regard. A lot of the module's functionality also exists in similar (or
even duplicate) helpers in `libpve-storage` [1], so a lot of changes and
additional features could be implemented before replacing said helpers
with this module.

That way we'd have a mostly convenient (for lack of a better term) way
to decouple other packages' dependency on "internals" of the
`PVE::Storage::PBSPlugin` module. ("Internals" that have now become part
of an implicit or perhaps inadvertedly created public API.)

Because all this also directly ties into #3186 [2], I can also provide
any such changes in a separate series or incorporate them all in another
version of this series - I'm happy for any feedback or suggestions in
that regard.

Testing
-------

Module Methods
**************

The module's methods were smoke-tested with a local script that I
executed on my dev VM. I created a new user on my PBS VM that the
script uses.

What was tested:
  * Creating new password and encryption key files in a separate secret
    directory (in order to not mess with my VM's installation)
  * Querying the datastore's status
  * Listing snapshots
  * Creating and uploading a new PXAR backup from a random directory
    I had laying around
  * Querying the snapshots again and filtering out the newly created
    snapshot of the PXAR backup, then extracting the backed up archive
    to a new directory again
  * Forgetting the newly snapshot after its extraction
  * Downloading files from specific backups via the
   `file_restore_extract_prepare` and `file_restore_extract` methods

This does however not cover all methods / cases, so I'd be grateful for
anyone testing this series. The docs should hopefully be enough to get
you started - if they're not, please let me know, because that means
they need improvements ;)

Package Split
*************

The package split was tested by substituting the `X.Y.Z` placeholder in
`debian/control` with a higher minor version and adding a corresponding
bump in `debian/changelog` with `dch -i`. The two packages built
successfully and were installed on my dev VM.

* Trying to install only the `libproxmox-backup-client-perl` package
  results in `apt` refusing to perform the installation, as expected.
* Installing both packages succeeds, as expected, and did not caus any
  noticable issues.
* Downgrading to the previous version of `libpve-common-perl` also
  causes `libproxmox-backup-client-perl` to be uninstalled, as expected.

For good measure I also double-checked if the Perl module was correctly
moved to the new package by comparing both `.deb` files via `dpkg -c`
and running `dpkg -S /usr/share/perl5/PVE/PBSClient.pm` on my dev VM.

References
----------

[1]: https://git.proxmox.com/?p=pve-storage.git;a=blob;f=src/PVE/Storage/PBSPlugin.pm;h=0808bccd79a75a6ace1c7194b30e894f5db54659;hb=refs/heads/master#l75
[2]: https://bugzilla.proxmox.com/show_bug.cgi?id=3186

Summary of Changes
------------------

Max Carrara (18):
  pbsclient: rename 'sdir' parameter of constructor to 'secret_dir'
  pbsclient: use parentheses when calling most inbuilts
  pbsclient: use post-if definedness checks instead of '//=' operator
  pbsclient: pull variable out of long post-if definedness check
  pbsclient: use cond. statements instead of chained 'or' operators
  pbsclient: use spaces around list braces and parens around ternaries
  pbsclient: s/foreach/for
  pbsclient: document package and its public functions & methods
  pbsclient: create secret dir with `mkdir -p` and mode `700`
  pbsclient: use `File::Spec->catfile` to concatenate file paths
  pbsclient: let `status` method return a hash instead of an array
  pbsclient: throw exception if username of client has no realm
  pbsclient: make method `password_file_name` public
  pbsclient: prohibit implicit return
  pbsclient: don't return anything in PXAR methods
  pbsclient: don't return anything in `forget_snapshot`
  make: support building multiple packages from the same source
  deb: split PBSClient.pm into new package libproxmox-backup-client-perl

 Makefile                                     |  27 +-
 debian/control                               |  12 +
 debian/libproxmox-backup-client-perl.install |   1 +
 debian/libpve-common-perl.install            |  28 +
 src/PVE/PBSClient.pm                         | 670 ++++++++++++++++---
 5 files changed, 651 insertions(+), 87 deletions(-)
 create mode 100644 debian/libproxmox-backup-client-perl.install
 create mode 100644 debian/libpve-common-perl.install

-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v1 pve-common 01/18] pbsclient: rename 'sdir' parameter of constructor to 'secret_dir'

[Max Carrara]

.. so that it's less ambiguous for what the parameter stands for at a
glance.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/PBSClient.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index e63af03..a1536e6 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -42,12 +42,12 @@ sub get_repository {
 }
 
 sub new {
-    my ($class, $scfg, $storeid, $sdir) = @_;
+    my ($class, $scfg, $storeid, $secret_dir) = @_;
 
     die "no section config provided\n" if ref($scfg) eq '';
     die "undefined store id\n" if !defined($storeid);
 
-    my $secret_dir = $sdir // '/etc/pve/priv/storage';
+    $secret_dir = '/etc/pve/priv/storage' if !defined($secret_dir);
 
     my $self = bless {
 	scfg => $scfg,
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v1 pve-common 02/18] pbsclient: use parentheses when calling most inbuilts

[Max Carrara]

.. except for really common cases like `die`, `warn`, `keys`.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/PBSClient.pm | 46 ++++++++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index a1536e6..3e98dd1 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -49,11 +49,11 @@ sub new {
 
     $secret_dir = '/etc/pve/priv/storage' if !defined($secret_dir);
 
-    my $self = bless {
+    my $self = bless({
 	scfg => $scfg,
 	storeid => $storeid,
 	secret_dir => $secret_dir
-    }, $class;
+    }, $class);
     return $self;
 }
 
@@ -67,7 +67,7 @@ sub set_password {
     my ($self, $password) = @_;
 
     my $pwfile = password_file_name($self);
-    mkdir $self->{secret_dir};
+    mkdir($self->{secret_dir});
 
     PVE::Tools::file_set_contents($pwfile, "$password\n", 0600);
 };
@@ -98,7 +98,7 @@ sub set_encryption_key {
     my ($self, $key) = @_;
 
     my $encfile = $self->encryption_key_file_name();
-    mkdir $self->{secret_dir};
+    mkdir($self->{secret_dir});
 
     PVE::Tools::file_set_contents($encfile, "$key\n", 0600);
 };
@@ -108,7 +108,7 @@ sub delete_encryption_key {
 
     my $encfile = $self->encryption_key_file_name();
 
-    if (!unlink $encfile) {
+    if (!unlink($encfile)) {
 	return if $! == ENOENT;
 	die "failed to delete encryption key! $!\n";
     }
@@ -144,7 +144,7 @@ my $USE_CRYPT_PARAMS = {
 my sub do_raw_client_cmd {
     my ($self, $client_cmd, $param, %opts) = @_;
 
-    my $client_bin = (delete $opts{binary}) || 'proxmox-backup-client';
+    my $client_bin = delete($opts{binary}) || 'proxmox-backup-client';
     my $use_crypto = $USE_CRYPT_PARAMS->{$client_bin}->{$client_cmd} // 0;
 
     my $client_exe = "/usr/bin/$client_bin";
@@ -153,13 +153,13 @@ my sub do_raw_client_cmd {
     my $scfg = $self->{scfg};
     my $repo = get_repository($scfg);
 
-    my $userns_cmd = delete $opts{userns_cmd};
+    my $userns_cmd = delete($opts{userns_cmd});
 
     my $cmd = [];
 
-    push @$cmd, @$userns_cmd if defined($userns_cmd);
+    push(@$cmd, @$userns_cmd) if defined($userns_cmd);
 
-    push @$cmd, $client_exe, $client_cmd;
+    push(@$cmd, $client_exe, $client_cmd);
 
     # This must live in the top scope to not get closed before the `run_command`
     my $keyfd;
@@ -169,17 +169,17 @@ my sub do_raw_client_cmd {
 		// die "failed to get file descriptor flags: $!\n";
 	    fcntl($keyfd, F_SETFD, $flags & ~FD_CLOEXEC)
 		or die "failed to remove FD_CLOEXEC from encryption key file descriptor\n";
-	    push @$cmd, '--crypt-mode=encrypt', '--keyfd='.fileno($keyfd);
+	    push(@$cmd, '--crypt-mode=encrypt', '--keyfd='.fileno($keyfd));
 	} else {
-	    push @$cmd, '--crypt-mode=none';
+	    push(@$cmd, '--crypt-mode=none');
 	}
     }
 
-    push @$cmd, @$param if defined($param);
+    push(@$cmd, @$param) if defined($param);
 
-    push @$cmd, "--repository", $repo;
+    push(@$cmd, "--repository", $repo);
     if (defined(my $ns = delete($opts{namespace}))) {
-	push @$cmd, '--ns', $ns;
+	push(@$cmd, '--ns', $ns);
     }
 
     local $ENV{PBS_PASSWORD} = $self->get_password();
@@ -266,7 +266,7 @@ sub get_snapshots {
     }
 
     my $param = [];
-    push @$param, $group if defined($group);
+    push(@$param, $group) if defined($group);
 
     return run_client_cmd($self, "snapshots", $param, undef, undef, $namespace);
 };
@@ -337,16 +337,16 @@ sub prune_group {
 
     my $param = [];
 
-    push @$param, "--quiet";
+    push(@$param, "--quiet");
 
     if (defined($opts->{'dry-run'}) && $opts->{'dry-run'}) {
-	push @$param, "--dry-run", $opts->{'dry-run'};
+	push(@$param, "--dry-run", $opts->{'dry-run'});
     }
 
     foreach my $keep_opt (keys %$prune_opts) {
-	push @$param, "--$keep_opt", $prune_opts->{$keep_opt};
+	push(@$param, "--$keep_opt", $prune_opts->{$keep_opt});
     }
-    push @$param, "$group";
+    push(@$param, "$group");
 
     return run_client_cmd($self, 'prune', $param, undef, undef, $namespace);
 };
@@ -381,7 +381,7 @@ sub file_restore_list {
     my $cmd = [ $snapshot, $filepath, "--base64", $base64 ? 1 : 0];
 
     if (my $timeout = $extra_params->{timeout}) {
-	push $cmd->@*, '--timeout', $timeout;
+	push($cmd->@*, '--timeout', $timeout);
     }
 
     return run_client_cmd(
@@ -406,9 +406,9 @@ sub file_restore_extract_prepare {
     # allow reading data for proxy user
     my $wwwid = getpwnam('www-data') ||
 	die "getpwnam failed";
-    chown $wwwid, -1, "$tmpdir"
+    chown($wwwid, -1, "$tmpdir")
 	or die "changing permission on fifo dir '$tmpdir' failed: $!\n";
-    chown $wwwid, -1, "$tmpdir/fifo"
+    chown($wwwid, -1, "$tmpdir/fifo")
 	or die "changing permission on fifo '$tmpdir/fifo' failed: $!\n";
 
     return "$tmpdir/fifo";
@@ -432,7 +432,7 @@ sub file_restore_extract {
 
 	my $cmd = [ $snapshot, $filepath, "-", "--base64", $base64 ? 1 : 0];
 	if ($tar) {
-	    push @$cmd, '--format', 'tar', '--zstd', 1;
+	    push(@$cmd, '--format', 'tar', '--zstd', 1);
 	}
 
 	return run_raw_client_cmd(
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v1 pve-common 03/18] pbsclient: use post-if definedness checks instead of '//=' operator

[Max Carrara]

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/PBSClient.pm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index 3e98dd1..525b37f 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -208,7 +208,7 @@ my sub run_client_cmd : prototype($$;$$$$) {
     my $json_str = '';
     my $outfunc = sub { $json_str .= "$_[0]\n" };
 
-    $binary //= 'proxmox-backup-client';
+    $binary = 'proxmox-backup-client' if !defined($binary);
 
     $param = [] if !defined($param);
     $param = [ $param ] if !ref($param);
@@ -286,7 +286,7 @@ sub backup_fs_tree {
 	'--backup-id', $id,
     ];
 
-    $cmd_opts //= {};
+    $cmd_opts = {} if !defined($cmd_opts);
 
     $cmd_opts->{namespace} = $self->{scfg}->{namespace} if defined($self->{scfg}->{namespace});
 
@@ -308,7 +308,7 @@ sub restore_pxar {
 	"$target",
 	"--allow-existing-dirs", 0,
     ];
-    $cmd_opts //= {};
+    $cmd_opts = {} if !defined($cmd_opts);
 
     $cmd_opts->{namespace} = $namespace;
 
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v1 pve-common 04/18] pbsclient: pull variable out of long post-if definedness check

[Max Carrara]

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/PBSClient.pm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index 525b37f..ab1fa62 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -288,7 +288,9 @@ sub backup_fs_tree {
 
     $cmd_opts = {} if !defined($cmd_opts);
 
-    $cmd_opts->{namespace} = $self->{scfg}->{namespace} if defined($self->{scfg}->{namespace});
+    if (defined(my $namespace = $self->{scfg}->{namespace})) {
+	$cmd_opts->{namespace} = $namespace;
+    }
 
     return run_raw_client_cmd($self, 'backup', $param, %$cmd_opts);
 };
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v1 pve-common 05/18] pbsclient: use cond. statements instead of chained 'or' operators

[Max Carrara]

.. like in the `delete_encryption_key` subroutine below, as it's more
readable at a glance.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/PBSClient.pm | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index ab1fa62..d707971 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -77,7 +77,10 @@ sub delete_password {
 
     my $pwfile = password_file_name($self);
 
-    unlink $pwfile or $! == ENOENT or die "deleting password file failed - $!\n";
+    if (!unlink($pwfile)) {
+	return if $! == ENOENT;
+	die "deleting password file failed - $!\n";
+    }
 };
 
 sub get_password {
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v1 pve-common 06/18] pbsclient: use spaces around list braces and parens around ternaries

[Max Carrara]

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/PBSClient.pm | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index d707971..d3daf41 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -216,7 +216,7 @@ my sub run_client_cmd : prototype($$;$$$$) {
     $param = [] if !defined($param);
     $param = [ $param ] if !ref($param);
 
-    $param = [@$param, '--output-format=json'] if !$no_output;
+    $param = [ @$param, '--output-format=json' ] if !$no_output;
 
     do_raw_client_cmd(
 	$self,
@@ -239,7 +239,7 @@ sub autogen_encryption_key {
     my ($self) = @_;
     my $encfile = $self->encryption_key_file_name();
     run_command(
-        ['proxmox-backup-client', 'key', 'create', '--kdf', 'none', $encfile],
+        [ 'proxmox-backup-client', 'key', 'create', '--kdf', 'none', $encfile ],
         errmsg => 'failed to create encryption key'
     );
     return file_get_contents($encfile);
@@ -327,7 +327,7 @@ sub forget_snapshot {
 
     (my $namespace, $snapshot) = split_namespaced_parameter($self, $snapshot);
 
-    return run_client_cmd($self, 'forget', ["$snapshot"], 1, undef, $namespace)
+    return run_client_cmd($self, 'forget', [ "$snapshot" ], 1, undef, $namespace)
 };
 
 sub prune_group {
@@ -383,7 +383,7 @@ sub file_restore_list {
     my ($self, $snapshot, $filepath, $base64, $extra_params) = @_;
 
     (my $namespace, $snapshot) = split_namespaced_parameter($self, $snapshot);
-    my $cmd = [ $snapshot, $filepath, "--base64", $base64 ? 1 : 0];
+    my $cmd = [ $snapshot, $filepath, "--base64", ($base64 ? 1 : 0) ];
 
     if (my $timeout = $extra_params->{timeout}) {
 	push($cmd->@*, '--timeout', $timeout);
@@ -435,7 +435,7 @@ sub file_restore_extract {
 	my $fn = fileno($fh);
 	my $errfunc = sub { print $_[0], "\n"; };
 
-	my $cmd = [ $snapshot, $filepath, "-", "--base64", $base64 ? 1 : 0];
+	my $cmd = [ $snapshot, $filepath, "-", "--base64", ($base64 ? 1 : 0) ];
 	if ($tar) {
 	    push(@$cmd, '--format', 'tar', '--zstd', 1);
 	}
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v1 pve-common 07/18] pbsclient: s/foreach/for

[Max Carrara]

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/PBSClient.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index d3daf41..231406a 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -348,7 +348,7 @@ sub prune_group {
 	push(@$param, "--dry-run", $opts->{'dry-run'});
     }
 
-    foreach my $keep_opt (keys %$prune_opts) {
+    for my $keep_opt (keys %$prune_opts) {
 	push(@$param, "--$keep_opt", $prune_opts->{$keep_opt});
     }
     push(@$param, "$group");
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v1 pve-common 08/18] pbsclient: document package and its public functions & methods

[Max Carrara]

This commit adds a brief overview for the `PVE::PBSClient` package and
documents its public functions and methods. Examples are added where
deemed appropriate.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/PBSClient.pm | 526 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 511 insertions(+), 15 deletions(-)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index 231406a..e0468d3 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -1,5 +1,4 @@
 package PVE::PBSClient;
-# utility functions for interaction with Proxmox Backup client CLI executable
 
 use strict;
 use warnings;
@@ -13,14 +12,83 @@ use POSIX qw(mkfifo strftime ENOENT);
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::Tools qw(run_command file_set_contents file_get_contents file_read_firstline $IPV6RE);
 
-# returns a repository string suitable for proxmox-backup-client, pbs-restore, etc.
-# $scfg must have the following structure:
-# {
-#     datastore
-#     server
-#     port        (optional defaults to 8007)
-#     username    (optional defaults to 'root@pam')
-# }
+=pod
+
+=head1 NAME
+
+PVE::PBSClient - Proxmox Backup Client Library
+
+=head1 DESCRIPTION
+
+This package contains utilities that wrap common Proxmox Backup client CLI
+operations.
+
+=head2 THE CLIENT OBJECT
+
+While the C<L<PVE::PBSClient>> package contains regular L<functions|/FUNCTIONS>,
+the majority is done via the C<L<PVE::PBSClient>> object. This object represents
+a client that is used to connect to a Proxmox Backup Server:
+
+    use strict;
+    use warnings;
+
+    use Data::Dumper;
+
+    $Data::Dumper::Indent = 1;
+    $Data::Dumper::Quotekeys = 0;
+    $Data::Dumper::Sortkeys = 1;
+    $Data::Dumper::Terse = 1;
+
+    use PVE::PBSClient;
+
+    my $scfg = {
+	server => 'example.tld',
+	username => 'alice@pam',
+	datastore => 'foo-store',
+	fingerprint => '...',
+    };
+
+    my $client = PVE::PBSClient->new($scfg, "pbs-main");
+
+    my ($total, $free, $used) = $client->status();
+
+    print "Datastore has a total capacity of $total bytes, of which $used bytes are used"
+	. "and $free bytes are still available.\n";
+
+    my $snapshot = "vm/1337/2024-07-30T10:01:57Z";
+    my $filepath = "/";
+
+    my $file_list = $client->file_restore_list($snapshot, $filepath);
+
+    print "The snapshot '$snapshot' contains the following restorable files:\n";
+    print Dumper($file_list);
+    print "\n";
+
+
+=head1 FUNCTIONS
+
+=cut
+
+=pod
+
+=head3 get_repository
+
+    $repository = get_repository($scfg)
+
+Returns a repository string suitable for the C<proxmox-backup-client> and
+C<proxmox-file-restore> executables.
+
+The C<$scfg> hash must have the following structure:
+
+    {
+	datastore => 'my-datastore-name',
+	server => 'example.tld',
+	port => 8007,             # optional, defaults to 8007
+	username => 'user@realm', # optional, defaults to 'root@pam'
+    }
+
+=cut
+
 sub get_repository {
     my ($scfg) = @_;
 
@@ -41,6 +109,58 @@ sub get_repository {
     return "$username\@$server:$datastore";
 }
 
+=pod
+
+=head1 METHODS
+
+=cut
+
+=pod
+
+=head3 new
+
+    $client = PVE::PBSClient->new($scfg, $storeid)
+    $client = PVE::PBSClient->new($scfg, $storeid, $secret_dir)
+
+Creates a new instance of a C<L<PVE::PBSClient>>.
+
+Throws an exception if no C<$scfg> hash is provided or if C<$storeid> is C<undef>.
+
+=over
+
+=item C<$scfg>
+
+The I<storage config> hash that the client should use.
+
+This hash is expected to have the following structure:
+
+    {
+	datastore => 'my-datastore-name',
+	namespace => 'my-namespace',
+	server => 'example.tld',
+	fingerprint => '...',
+	port => 8007,             # optional, defaults to 8007
+	username => 'user@realm', # optional, defaults to 'root@pam'
+    }
+
+=item C<$storeid>
+
+The I<ID> of the storage corresponding to C<$scfg>. This ID is used for operations
+concerning the I<password> and I<encryption key>, such as C<L</get_password>> and
+C<L</set_encryption_key>>.
+
+=item C<$secret_dir> (optional)
+
+The name of the I<secret directory> in which the I<password> and I<encryption key>
+files are stored. Defaults to C</etc/pve/priv/storage>.
+
+Note that the I<password> and I<encryption key> files are expected to be named
+C<foo.pw> and C<foo.enc> respectively, if, for example, C<$storeid> is C<"foo">.
+
+=back
+
+=cut
+
 sub new {
     my ($class, $scfg, $storeid, $secret_dir) = @_;
 
@@ -63,6 +183,21 @@ my sub password_file_name {
     return "$self->{secret_dir}/$self->{storeid}.pw";
 }
 
+=pod
+
+=head3 set_password
+
+    $client->set_password($password)
+
+Updates or creates the I<password> file, storing the given C<$password>.
+
+If the I<secret directory> does not exist, it is created beforehand.
+
+If the I<password> file does not exist, a new one with the permissions C<600>
+is created.
+
+=cut
+
 sub set_password {
     my ($self, $password) = @_;
 
@@ -72,6 +207,19 @@ sub set_password {
     PVE::Tools::file_set_contents($pwfile, "$password\n", 0600);
 };
 
+=pod
+
+=head3 delete_password
+
+    $client->delete_password()
+
+Deletes the I<password> file inside the I<secret directory>.
+
+Will throw an exception if deleting the I<password> file fails, but not
+if the file doesn't exist.
+
+=cut
+
 sub delete_password {
     my ($self) = @_;
 
@@ -83,6 +231,16 @@ sub delete_password {
     }
 };
 
+=pod
+
+=head3 get_password
+
+    $password = $client->get_password()
+
+Reads and returns the I<password> from its file inside the I<secret directory>.
+
+=cut
+
 sub get_password {
     my ($self) = @_;
 
@@ -91,12 +249,38 @@ sub get_password {
     return PVE::Tools::file_read_firstline($pwfile);
 }
 
+=pod
+
+=head3 encryption_key_file_name
+
+    $file_name = $self->encryption_key_file_name()
+
+Returns the full name of the I<encryption key> file, including the path of the
+I<secret directory> it is located in.
+
+=cut
+
 sub encryption_key_file_name {
     my ($self) = @_;
 
     return "$self->{secret_dir}/$self->{storeid}.enc";
 };
 
+=pod
+
+=head3 set_encryption_key
+
+    $client->set_encryption_key($key)
+
+Updates or creates the I<encryption key> file, storing the given C<$key>.
+
+If the I<secret directory> does not exist, it is created beforehand.
+
+If the I<encryption key> file does not exist, a new one with the permissions C<600>
+is created.
+
+=cut
+
 sub set_encryption_key {
     my ($self, $key) = @_;
 
@@ -106,6 +290,19 @@ sub set_encryption_key {
     PVE::Tools::file_set_contents($encfile, "$key\n", 0600);
 };
 
+=pod
+
+=head3 delete_encryption_key
+
+    $client->delete_encryption_key()
+
+Deletes the I<encryption key> file inside the I<secret directory>.
+
+Will throw an exception if deleting the I<encryption key> file fails, but not
+if the file doesn't exist.
+
+=cut
+
 sub delete_encryption_key {
     my ($self) = @_;
 
@@ -235,6 +432,21 @@ my sub run_client_cmd : prototype($$;$$$$) {
     return $res;
 }
 
+=pod
+
+=head3 autogen_encryption_key
+
+    $new_key = $client->autogen_encryption_key()
+
+Generates a new I<encryption key> and stores it as a file inside the I<secret directory>.
+The raw contents of the key file, B<which are encoded as JSON string>, are
+returned afterwards.
+
+If an I<encryption key> file already exists at its expected location, an
+exception is thrown.
+
+=cut
+
 sub autogen_encryption_key {
     my ($self) = @_;
     my $encfile = $self->encryption_key_file_name();
@@ -257,7 +469,61 @@ my sub split_namespaced_parameter : prototype($$) {
     return ($namespace, $snapshot);
 }
 
-# lists all snapshots, optionally limited to a specific group
+=pod
+
+=head3 get_snapshots
+
+    $snapshots = $client->get_snapshots()
+    $snapshots = $client->get_snapshots($group)
+
+Returns all snapshots of the current client instance as a list of nesteded hashes.
+
+Optionally, the snapshots may be filtered by their C<$group>, such as C<"vm/100">
+or C<"ct/2000">, for example.
+
+The returned list has the following structure:
+
+    [
+	{
+	    'backup-id' => "100",
+	    'backup-time' => 1721901601,
+	    'backup-type' => "vm",
+	    comment => "standalone node/example-host/100-example-vm",
+	    files: [
+		{
+		    'crypt-mode' => "encrypt",
+		    filename => "qemu-server.conf.blob",
+		    size => 428
+		},
+		{
+		    'crypt-mode' => "encrypt",
+		    filename => "drive-scsi0.img.fidx",
+		    size => 17179869184
+		},
+		{
+		    'crypt-mode' => "sign-only",
+		    filename => "index.json.blob",
+		    size => 651
+		},
+		{
+		    filename => "client.log.blob"
+		},
+		...
+	    ],
+	    fingerprint => "...",
+	    owner => "root@pam",
+	    protected => false,
+	    size => 17179870263,
+	    verification => {
+		state => "ok",
+		upid => "..."
+	    }
+	},
+	...
+    ]
+
+=cut
+
 sub get_snapshots {
     my ($self, $group) = @_;
 
@@ -274,8 +540,28 @@ sub get_snapshots {
     return run_client_cmd($self, "snapshots", $param, undef, undef, $namespace);
 };
 
-# create a new PXAR backup of a FS directory tree - doesn't cross FS boundary
-# by default.
+=pod
+
+=head3 backup_fs_tree
+
+    $client->backup_fs_tree($root, $id, $pxarname)
+    $client->backup_fs_tree($root, $id, $pxarname, $cmd_opts)
+
+Create a new PXAR backup of a directory tree starting at C<$root>.
+
+C<$id> is the I<ID> of the stored backup and C<$pxarname> is the name of the
+uploaded PXAR archive.
+
+Optionally, the C<$cmd_opts> hash may be supplied, which should contain
+additional parameters to pass to C<L<PVE::Tools::run_command>> that is used
+under the hood.
+
+Raises an exception if either C<$root>, C<$id> or C<$pxarname> is C<undef>.
+
+B<NOTE:> This does B<not> cross filesystem boundaries.
+
+=cut
+
 sub backup_fs_tree {
     my ($self, $root, $id, $pxarname, $cmd_opts) = @_;
 
@@ -298,6 +584,32 @@ sub backup_fs_tree {
     return run_raw_client_cmd($self, 'backup', $param, %$cmd_opts);
 };
 
+=pod
+
+=head3 restore_pxar
+
+    $client->restore_pxar($snapshot, $pxarname, $target)
+    $client->restore_pxar($snapshot, $pxarname, $target, $cmd_opts)
+
+Restore a PXAR backup of a directory tree from the given C<$snapshot>.
+
+C<$pxarname> is the name of the previously uploaded PXAR archive to restore and
+C<$target> the directory to which the backed up tree will be restored to.
+
+Note that C<$snapshot> must be the snapshot's complete name in the format
+C<TYPE/ID/BACKUP_TIME> - for example C<"vm/100/2023-07-31T16:00:00Z"> or
+C<"ct/2000/2024-08-01T09:54:08Z"> (like it's displayed in the PBS UI).
+
+Optionally, the C<$cmd_opts> hash may be supplied, which should contain
+additional parameters to pass to C<L<PVE::Tools::run_command>> that is used
+under the hood.
+
+Raises an exception if either C<$snapshot>, C<$pxarname> or C<$target> is C<undef>,
+or if a filesystem entry to be restored already exists inside the C<$target>
+directory.
+
+=cut
+
 sub restore_pxar {
     my ($self, $snapshot, $pxarname, $target, $cmd_opts) = @_;
 
@@ -320,6 +632,22 @@ sub restore_pxar {
     return run_raw_client_cmd($self, 'restore', $param, %$cmd_opts);
 };
 
+=pod
+
+=head3 forget_snapshot
+
+    $client->forget_snapshot($snapshot)
+
+Forgets the given C<$snapshot>.
+
+Note that C<$snapshot> must be the snapshot's complete name in the format
+C<TYPE/ID/BACKUP_TIME> - for example C<"vm/100/2023-07-31T16:00:00Z"> or
+C<"ct/2000/2024-08-01T09:54:08Z"> (as displayed in the PBS UI).
+
+Raises an exception if C<$snapshot> is C<undef>.
+
+=cut
+
 sub forget_snapshot {
     my ($self, $snapshot) = @_;
 
@@ -330,6 +658,41 @@ sub forget_snapshot {
     return run_client_cmd($self, 'forget', [ "$snapshot" ], 1, undef, $namespace)
 };
 
+=pod
+
+=head3 prune_group
+
+    $client->prune_group($opts, $prune_opts, $group)
+
+Prunes a backup C<$group>. The exact behaviour can be controlled using the
+C<$opts> and C<$prune_opts> hashes.
+
+C<$group> must be in the format of C<TYPE/ID>, like C<"vm/100"> or C<"ct/2000">,
+for example (as displayed in the PBS UI).
+
+The C<$opts> hash supports the following options and may be left empty:
+
+    {
+	'dry-run' => 1,  # perform a dry run
+    }
+
+The C<$prune_opts> hash supports the following options:
+
+    {
+	'keep-last' => 1,
+	'keep-hourly' => 1,
+	'keep-daily' => 1,
+	'keep-weekly' => 1,
+	'keep-monthly' => 1,
+	'keep-yearly' => 1,
+    }
+
+Will do nothing if no C<$prune_opts> are supplied.
+
+Raises an exception if C<$group> is C<undef>.
+
+=cut
+
 sub prune_group {
     my ($self, $opts, $prune_opts, $group) = @_;
 
@@ -356,6 +719,19 @@ sub prune_group {
     return run_client_cmd($self, 'prune', $param, undef, undef, $namespace);
 };
 
+=pod
+
+=head3 status
+
+    ($total, $free, $used, $active) = $client->status()
+
+Return the I<status> of the client's repository as an array.
+
+The array contains the C<$total>, C<$free> and C<$used> size of the repository
+as bytes, as well as whether the repository is C<$active> or not.
+
+=cut
+
 sub status {
     my ($self) = @_;
 
@@ -379,6 +755,59 @@ sub status {
     return ($total, $free, $used, $active);
 };
 
+=pod
+
+=head3 file_restore_list
+
+    $restore_list = $client->($snapshot, $filepath)
+    $restore_list = $client->($snapshot, $filepath, $base64)
+    $restore_list = $client->($snapshot, $filepath, $base64, $extra_params)
+
+Return the list of entries from a directory C<$filepath> of a backup C<$snapshot>.
+
+Note that C<$snapshot> must be the snapshot's complete name in the format
+C<TYPE/ID/BACKUP_TIME> - for example C<"vm/100/2023-07-31T16:00:00Z"> or
+C<"ct/2000/2024-08-01T09:54:08Z"> (as displayed in the PBS UI).
+
+C<$base64> may optionally be set to C<1> if the C<$filepath> is base64-encoded.
+
+The C<$extra_params> hash supports the following options and may be left empty:
+
+    {
+	timeout => 5,  # in seconds
+    }
+
+If successful, the returned list of hashes has the following structure:
+
+    [
+	{
+	    filepath => "L2RyaXZlLXNjc2kwLmltZy5maWR4",
+	    leaf => 0,
+	    size => 34359738368,
+	    text => "drive-scsi0.img.fidx",
+	    type => "v"
+	},
+	{
+	    filepath => "L2RyaXZlLXNjc2kxLmltZy5maWR4",
+	    leaf => 0,
+	    size => 68719476736,
+	    text => "drive-scsi1.img.fidx",
+	    type => "v"
+	},
+	...
+    ]
+
+On error, the list of hashes will contain an error message, for example:
+
+    [
+	{
+	    message => "wrong key - unable to verify signature since manifest's key [...] does not match provided key [...]"
+	},
+	...
+    ]
+
+=cut
+
 sub file_restore_list {
     my ($self, $snapshot, $filepath, $base64, $extra_params) = @_;
 
@@ -399,8 +828,32 @@ sub file_restore_list {
     );
 }
 
-# call sync from API, returns a fifo path for streaming data to clients,
-# pass it to file_restore_extract to start transfering data
+=pod
+
+=head3 file_restore_extract_prepare
+
+    $fifo = $client->file_restore_extract_prepare()
+
+Create a I<named pipe> (FIFO) for streaming data and return its path.
+
+The returned path is usually passed to C<L</file_restore_extract>> which will
+stream the data to the I<named pipe>. A different process may then read from the
+same path, receiving the streamed data.
+
+Raises an exception if:
+
+=over
+
+=item creating the I<named pipe> fails
+
+=item the call to C<getpwnam> for the C<www-data> user fails
+
+=item changing the permissions for the I<named pipe> or its directory fails
+
+=back
+
+=cut
+
 sub file_restore_extract_prepare {
     my ($self) = @_;
 
@@ -419,7 +872,50 @@ sub file_restore_extract_prepare {
     return "$tmpdir/fifo";
 }
 
-# this blocks while data is transfered, call this from a background worker
+=pod
+
+=head3 file_restore_extract
+
+    $client->file_restore_extract($output_file, $snapshot, $filepath, $base64, $tar)
+
+Restores and extracts a C<$filepath> from a C<$snapshot> to the given C<$output_file>.
+By default, the C<$output_file> will be a ZIP archive.
+
+Because this method is mostly used for streaming purposes in conjunction with
+C<L</file_restore_extract_prepare>>, the C<$output_file> will be automatically
+I<unlinked> once the extraction is complete. See below for an example on how to
+use this method.
+
+C<$base64> may optionally be set to C<1> if the C<$filepath> is base64-encoded.
+
+C<$tar> may optionally be set to C<1> if the output written to C<$output_file>
+should be a ZSTD-compressed TAR archive. B<Otherwise, the file will be saved as
+a ZIP archive.>
+
+Usually used in conjunction with C<L</file_restore_extract_prepare>>.
+
+B<NOTE:> This method B<blocks> while data is being transferred to C<$output_file>.
+It is therefore best to call this within C<L<PVE::RESTEnvironment::fork_worker>>
+or C<L<PVE::RPCEnvironment::fork_worker>>, for example:
+
+    # [...]
+    my $fifo = $client->file_restore_extract_prepare();
+
+    $rpcenv->fork_worker('backup-download', undef, $user, sub {
+	print "Starting download of file: $filename\n";
+	$client->file_restore_extract($fifo, $snapshot, $filepath, 0, $tar);
+    });
+
+    return {
+	download => {
+	    path => $fifo,
+	    stream => 1,
+	    'content-type' => 'application/octet-stream',
+	},
+    };
+
+=cut
+
 sub file_restore_extract {
     my ($self, $output_file, $snapshot, $filepath, $base64, $tar) = @_;
 
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v1 pve-common 09/18] pbsclient: create secret dir with `mkdir -p` and mode `700`

[Max Carrara]

.. instead of using a regular `mkdir` call.

The `File::Path::make_path` subroutine is used for this purpose, which
recursively creates all directories if they didn't exist before. Upon
creation of those directories, the mode is also set to `700`.

This means that (like before), directory permissions are left
untouched if the directory existed already.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/PBSClient.pm | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index e0468d3..2084bb5 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -4,6 +4,7 @@ use strict;
 use warnings;
 
 use Fcntl qw(F_GETFD F_SETFD FD_CLOEXEC);
+use File::Path;
 use File::Temp qw(tempdir);
 use IO::File;
 use JSON;
@@ -191,7 +192,8 @@ my sub password_file_name {
 
 Updates or creates the I<password> file, storing the given C<$password>.
 
-If the I<secret directory> does not exist, it is created beforehand.
+If the I<secret directory> does not exist, it is recursively created with the
+permissions C<700> beforehand.
 
 If the I<password> file does not exist, a new one with the permissions C<600>
 is created.
@@ -202,7 +204,9 @@ sub set_password {
     my ($self, $password) = @_;
 
     my $pwfile = password_file_name($self);
-    mkdir($self->{secret_dir});
+    File::Path::make_path($self->{secret_dir}, {
+	mode => 0700,
+    });
 
     PVE::Tools::file_set_contents($pwfile, "$password\n", 0600);
 };
@@ -274,7 +278,8 @@ sub encryption_key_file_name {
 
 Updates or creates the I<encryption key> file, storing the given C<$key>.
 
-If the I<secret directory> does not exist, it is created beforehand.
+If the I<secret directory> does not exist, it is recursively created with the
+permissions C<700> beforehand.
 
 If the I<encryption key> file does not exist, a new one with the permissions C<600>
 is created.
@@ -285,7 +290,9 @@ sub set_encryption_key {
     my ($self, $key) = @_;
 
     my $encfile = $self->encryption_key_file_name();
-    mkdir($self->{secret_dir});
+    File::Path::make_path($self->{secret_dir}, {
+	mode => 0700,
+    });
 
     PVE::Tools::file_set_contents($encfile, "$key\n", 0600);
 };
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v1 pve-common 10/18] pbsclient: use `File::Spec->catfile` to concatenate file paths

[Max Carrara]

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/PBSClient.pm | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index 2084bb5..69b4e40 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -5,6 +5,7 @@ use warnings;
 
 use Fcntl qw(F_GETFD F_SETFD FD_CLOEXEC);
 use File::Path;
+use File::Spec;
 use File::Temp qw(tempdir);
 use IO::File;
 use JSON;
@@ -181,7 +182,7 @@ sub new {
 my sub password_file_name {
     my ($self) = @_;
 
-    return "$self->{secret_dir}/$self->{storeid}.pw";
+    return File::Spec->catfile($self->{secret_dir}, "$self->{storeid}.pw");
 }
 
 =pod
@@ -267,7 +268,7 @@ I<secret directory> it is located in.
 sub encryption_key_file_name {
     my ($self) = @_;
 
-    return "$self->{secret_dir}/$self->{storeid}.enc";
+    return File::Spec->catfile($self->{secret_dir}, "$self->{storeid}.enc");
 };
 
 =pod
@@ -354,7 +355,7 @@ my sub do_raw_client_cmd {
     my $client_bin = delete($opts{binary}) || 'proxmox-backup-client';
     my $use_crypto = $USE_CRYPT_PARAMS->{$client_bin}->{$client_cmd} // 0;
 
-    my $client_exe = "/usr/bin/$client_bin";
+    my $client_exe = File::Spec->catfile("/usr/bin", $client_bin);
     die "executable not found '$client_exe'! $client_bin not installed?\n" if ! -x $client_exe;
 
     my $scfg = $self->{scfg};
@@ -865,18 +866,20 @@ sub file_restore_extract_prepare {
     my ($self) = @_;
 
     my $tmpdir = tempdir();
-    mkfifo("$tmpdir/fifo", 0600)
-	or die "creating file download fifo '$tmpdir/fifo' failed: $!\n";
+    my $fifo_path = File::Spec->catfile($tmpdir, "fifo");
+
+    mkfifo($fifo_path, 0600)
+	or die "creating file download fifo '$fifo_path' failed: $!\n";
 
     # allow reading data for proxy user
     my $wwwid = getpwnam('www-data') ||
 	die "getpwnam failed";
     chown($wwwid, -1, "$tmpdir")
 	or die "changing permission on fifo dir '$tmpdir' failed: $!\n";
-    chown($wwwid, -1, "$tmpdir/fifo")
-	or die "changing permission on fifo '$tmpdir/fifo' failed: $!\n";
+    chown($wwwid, -1, $fifo_path)
+	or die "changing permission on fifo '$fifo_path' failed: $!\n";
 
-    return "$tmpdir/fifo";
+    return $fifo_path;
 }
 
 =pod
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v1 pve-common 11/18] pbsclient: let `status` method return a hash instead of an array

[Max Carrara]

Instead of returning an (arguably awkward) array with four elements,
where each element has a special meaning, return the hash that is
constructed from the `proxmox-backup-client status` command's JSON
output.

The documentation is updated accordingly.

This method isn't used anywhere in our code base at the moment, so I
assume it is safe to change it. Checked with ripgrep.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/PBSClient.pm | 32 ++++++++++----------------------
 1 file changed, 10 insertions(+), 22 deletions(-)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index 69b4e40..26f73ef 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -731,36 +731,24 @@ sub prune_group {
 
 =head3 status
 
-    ($total, $free, $used, $active) = $client->status()
+    $status = $client->status()
 
-Return the I<status> of the client's repository as an array.
+Return the I<status> of the client's repository as a hash.
 
-The array contains the C<$total>, C<$free> and C<$used> size of the repository
-as bytes, as well as whether the repository is C<$active> or not.
+The returned hash has the following structure:
+
+    {
+	avail => 41735159808,  # sizes are in bytes!
+	total => 264240103424,
+	used => 41735159808,
+    }
 
 =cut
 
 sub status {
     my ($self) = @_;
 
-    my $total = 0;
-    my $free = 0;
-    my $used = 0;
-    my $active = 0;
-
-    eval {
-	my $res = run_client_cmd($self, "status");
-
-	$active = 1;
-	$total = $res->{total};
-	$used = $res->{used};
-	$free = $res->{avail};
-    };
-    if (my $err = $@) {
-	warn $err;
-    }
-
-    return ($total, $free, $used, $active);
+    return run_client_cmd($self, "status");
 };
 
 =pod
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v1 pve-common 12/18] pbsclient: throw exception if username of client has no realm

[Max Carrara]

.. in order to catch that error earlier, rather than having
`proxmox-backup-client` err and complain that the "repository value
doesn't match the regex pattern".

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/PBSClient.pm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index 26f73ef..e7e9a79 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -108,6 +108,8 @@ sub get_repository {
 
     my $username = $scfg->{username} // 'root@pam';
 
+    die "no realm given for username '$username'" if $username !~ m/@/;
+
     return "$username\@$server:$datastore";
 }
 
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v1 pve-common 13/18] pbsclient: make method `password_file_name` public

[Max Carrara]

.. in order to allow users to only store a password if the password
file doesn't exist yet, for example.

Also adds corresponding documentation.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/PBSClient.pm | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index e7e9a79..a701542 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -181,7 +181,18 @@ sub new {
     return $self;
 }
 
-my sub password_file_name {
+=pod
+
+=head3 password_file_name
+
+    $file_name = $client->password_file_name()
+
+Returns the full name of the I<password> file, including the path of the
+I<secret directory> it is located in.
+
+=cut
+
+sub password_file_name {
     my ($self) = @_;
 
     return File::Spec->catfile($self->{secret_dir}, "$self->{storeid}.pw");
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v1 pve-common 14/18] pbsclient: prohibit implicit return

[Max Carrara]

.. for the `set_password`, `set_encryption_key` and
`do_raw_client_cmd` methods.

This makes it very clear that these methods aren't supposed to return
anything. In the case of `do_raw_client_cmd` in particular, the
implicit return of `0` on success (would be `undef` if the command
failed) was never depended on either, so prevent future code from
doing so.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/PBSClient.pm | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index a701542..4ffdd04 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -223,6 +223,7 @@ sub set_password {
     });
 
     PVE::Tools::file_set_contents($pwfile, "$password\n", 0600);
+    return;
 };
 
 =pod
@@ -309,6 +310,7 @@ sub set_encryption_key {
     });
 
     PVE::Tools::file_set_contents($encfile, "$key\n", 0600);
+    return;
 };
 
 =pod
@@ -416,6 +418,7 @@ my sub do_raw_client_cmd {
     }
 
     run_command($cmd, %opts);
+    return;
 }
 
 my sub run_raw_client_cmd : prototype($$$%) {
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v1 pve-common 15/18] pbsclient: don't return anything in PXAR methods

[Max Carrara]

Due to the previously removed implicit return in `do_raw_client_cmd`,
the `run_raw_client_cmd` and consequently also the `backup_fs_tree`
and `restore_pxar` methods would return `0` on success if they didn't
`die` otherwise.

Therefore, because now nothing is actually returned, explicitly don't
return anything in order to prevent returning something implicitly in
the future (if e.g. `run_raw_client_cmd` is made to return something).

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/PBSClient.pm | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index 4ffdd04..4adb04c 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -605,7 +605,8 @@ sub backup_fs_tree {
 	$cmd_opts->{namespace} = $namespace;
     }
 
-    return run_raw_client_cmd($self, 'backup', $param, %$cmd_opts);
+    run_raw_client_cmd($self, 'backup', $param, %$cmd_opts);
+    return;
 };
 
 =pod
@@ -653,7 +654,8 @@ sub restore_pxar {
 
     $cmd_opts->{namespace} = $namespace;
 
-    return run_raw_client_cmd($self, 'restore', $param, %$cmd_opts);
+    run_raw_client_cmd($self, 'restore', $param, %$cmd_opts);
+    return;
 };
 
 =pod
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v1 pve-common 16/18] pbsclient: don't return anything in `forget_snapshot`

[Max Carrara]

The actual `proxmox-backup-client forget` command doesn't return
anything (despite the `--output-format json` flag), which is why this
method ends up returning `undef` on successful calls (due to the JSON
parse in `run_client_cmd` returning `undef`).

Therefore, make it explicit that nothing is returned to make it more
clear what the code does.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 src/PVE/PBSClient.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index 4adb04c..aeceed3 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -681,7 +681,8 @@ sub forget_snapshot {
 
     (my $namespace, $snapshot) = split_namespaced_parameter($self, $snapshot);
 
-    return run_client_cmd($self, 'forget', [ "$snapshot" ], 1, undef, $namespace)
+    run_client_cmd($self, 'forget', [ "$snapshot" ], 1, undef, $namespace);
+    return;
 };
 
 =pod
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v1 pve-common 17/18] make: support building multiple packages from the same source

[Max Carrara]

This also makes sure that `lintian` actually lints any new packages.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 Makefile | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/Makefile b/Makefile
index 637cd49..c2969aa 100644
--- a/Makefile
+++ b/Makefile
@@ -1,20 +1,24 @@
 include /usr/share/dpkg/pkg-info.mk
 
-PACKAGE=libpve-common-perl
+SOURCE_PACKAGE=libpve-common-perl
+
+PACKAGES = \
+	$(SOURCE_PACKAGE) \
+
 
 ARCH=all
 
-BUILDDIR ?= $(PACKAGE)-$(DEB_VERSION_UPSTREAM)
+BUILDDIR ?= $(SOURCE_PACKAGE)-$(DEB_VERSION_UPSTREAM)
 
-DEB=$(PACKAGE)_$(DEB_VERSION_UPSTREAM_REVISION)_$(ARCH).deb
-DSC=$(PACKAGE)_$(DEB_VERSION_UPSTREAM_REVISION).dsc
+DEBS = $(addsuffix _$(DEB_VERSION_UPSTREAM_REVISION)_$(ARCH).deb,$(PACKAGES))
+DSC=$(SOURCE_PACKAGE)_$(DEB_VERSION_UPSTREAM_REVISION).dsc
 
 all:
 	$(MAKE) -C src
 
 .PHONY: dinstall
 dinstall: deb
-	dpkg -i $(DEB)
+	dpkg -i $(DEBS)
 
 $(BUILDDIR): src debian test
 	rm -rf $(BUILDDIR) $(BUILDDIR).tmp; mkdir $(BUILDDIR).tmp
@@ -23,10 +27,10 @@ $(BUILDDIR): src debian test
 	mv $(BUILDDIR).tmp $(BUILDDIR)
 
 .PHONY: deb
-deb: $(DEB)
-$(DEB): $(BUILDDIR)
+deb: $(DEBS)
+$(DEBS): $(BUILDDIR)
 	cd $(BUILDDIR); dpkg-buildpackage -b -us -uc
-	lintian $(DEB)
+	lintian $(DEBS)
 
 .PHONY: dsc
 dsc: $(DSC)
@@ -40,7 +44,7 @@ sbuild: $(DSC)
 .PHONY: clean distclean
 distclean: clean
 clean:
-	rm -rf *~ *.deb *.changes $(PACKAGE)-[0-9]*/ *.buildinfo *.build *.dsc *.tar.?z
+	rm -rf *~ *.deb *.changes $(SOURCE_PACKAGE)-[0-9]*/ *.buildinfo *.build *.dsc *.tar.?z
 
 .PHONY: check
 check:
@@ -52,5 +56,5 @@ install:
 
 .PHONY: upload
 upload: UPLOAD_DIST ?= $(DEB_DISTRIBUTION)
-upload: $(DEB)
-	tar cf - $(DEB)|ssh -X repoman@repo.proxmox.com -- upload --product pve,pmg --dist $(UPLOAD_DIST)
+upload: $(DEBS)
+	tar cf - $(DEBS)|ssh -X repoman@repo.proxmox.com -- upload --product pve,pmg --dist $(UPLOAD_DIST)
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH v1 pve-common 18/18] deb: split PBSClient.pm into new package libproxmox-backup-client-perl

[Max Carrara]

.. and also add corresponding 'debian/*.install' files for
'libpve-common-perl' and the new package.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
 Makefile                                     |  1 +
 debian/control                               | 12 +++++++++
 debian/libproxmox-backup-client-perl.install |  1 +
 debian/libpve-common-perl.install            | 28 ++++++++++++++++++++
 4 files changed, 42 insertions(+)
 create mode 100644 debian/libproxmox-backup-client-perl.install
 create mode 100644 debian/libpve-common-perl.install

diff --git a/Makefile b/Makefile
index c2969aa..28d7214 100644
--- a/Makefile
+++ b/Makefile
@@ -4,6 +4,7 @@ SOURCE_PACKAGE=libpve-common-perl
 
 PACKAGES = \
 	$(SOURCE_PACKAGE) \
+	libproxmox-backup-client-perl \
 
 
 ARCH=all
diff --git a/debian/control b/debian/control
index ac4cd66..11a1e17 100644
--- a/debian/control
+++ b/debian/control
@@ -52,3 +52,15 @@ Breaks: ifupdown2 (<< 2.0.1-1+pve5),
         qemu-server (<< 8.0.1),
 Description: Proxmox VE base library
  This package contains the base library used by other Proxmox VE components.
+
+Package: libproxmox-backup-client-perl
+Architecture: all
+Depends: libpve-common-perl (>= X.Y.Z),
+        proxmox-backup-client (>= 2.1.10~),
+        proxmox-backup-file-restore,
+        ${perl:Depends}
+Replaces: libpve-common-perl (<< X.Y.Z)
+Breaks: libpve-common-perl (<< X.Y.Z)
+Description: Proxmox Backup Client Perl Library
+ This package contains utilities that wrap common Proxmox Backup client CLI
+ operations.
diff --git a/debian/libproxmox-backup-client-perl.install b/debian/libproxmox-backup-client-perl.install
new file mode 100644
index 0000000..a54f59b
--- /dev/null
+++ b/debian/libproxmox-backup-client-perl.install
@@ -0,0 +1 @@
+/usr/share/perl5/PVE/PBSClient.pm
diff --git a/debian/libpve-common-perl.install b/debian/libpve-common-perl.install
new file mode 100644
index 0000000..bf42f42
--- /dev/null
+++ b/debian/libpve-common-perl.install
@@ -0,0 +1,28 @@
+/usr/share/perl5/PVE/AtomicFile.pm
+/usr/share/perl5/PVE/CalendarEvent.pm
+/usr/share/perl5/PVE/Certificate.pm
+/usr/share/perl5/PVE/CGroup.pm
+/usr/share/perl5/PVE/CLIFormatter.pm
+/usr/share/perl5/PVE/CLIHandler.pm
+/usr/share/perl5/PVE/CpuSet.pm
+/usr/share/perl5/PVE/Daemon.pm
+/usr/share/perl5/PVE/Exception.pm
+/usr/share/perl5/PVE/Format.pm
+/usr/share/perl5/PVE/INotify.pm
+/usr/share/perl5/PVE/Job/
+/usr/share/perl5/PVE/Job/Registry.pm
+/usr/share/perl5/PVE/JSONSchema.pm
+/usr/share/perl5/PVE/LDAP.pm
+/usr/share/perl5/PVE/Network.pm
+/usr/share/perl5/PVE/OTP.pm
+/usr/share/perl5/PVE/ProcFSTools.pm
+/usr/share/perl5/PVE/PTY.pm
+/usr/share/perl5/PVE/RESTEnvironment.pm
+/usr/share/perl5/PVE/RESTHandler.pm
+/usr/share/perl5/PVE/SafeSyslog.pm
+/usr/share/perl5/PVE/SectionConfig.pm
+/usr/share/perl5/PVE/Syscall.pm
+/usr/share/perl5/PVE/SysFSTools.pm
+/usr/share/perl5/PVE/Systemd.pm
+/usr/share/perl5/PVE/Ticket.pm
+/usr/share/perl5/PVE/Tools.pm
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel