[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] applied: [PATCH v1 pve-common 01/18] pbsclient: rename 'sdir' parameter of constructor to 'secret_dir'

[Thomas Lamprecht]

Am 02.08.24 um 15:26 schrieb 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(-)
> 
>

applied, thanks!


_______________________________________________
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] applied: [PATCH v1 pve-common 02/18] pbsclient: use parentheses when calling most inbuilts

[Thomas Lamprecht]

Am 02.08.24 um 15:26 schrieb 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(-)
> 
>

applied, thanks!


_______________________________________________
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] applied: [PATCH v1 pve-common 03/18] pbsclient: use post-if definedness checks instead of '//=' operator

[Thomas Lamprecht]

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

a frequent pattern we have and IMO a lateral move, but meh, borderline
good enough so:

applied, thanks!


_______________________________________________
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] applied: [PATCH v1 pve-common 04/18] pbsclient: pull variable out of long post-if definedness check

[Thomas Lamprecht]

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

applied, thanks!


_______________________________________________
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

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

[Thomas Lamprecht]

Am 02.08.24 um 15:26 schrieb 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";

we have this pattern quite often and for unlink this is IMO really fine to do,
especially with replacing `$! == ENOENT` with `$!{ENOENT}` to avoid the need
for loading the POSIX module.

> +    if (!unlink($pwfile)) {
> +	return if $! == ENOENT;
> +	die "deleting password file failed - $!\n";
> +    }
>  };
>  
>  sub get_password {



_______________________________________________
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] applied: [PATCH v1 pve-common 06/18] pbsclient: use spaces around list braces and parens around ternaries

[Thomas Lamprecht]

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

applied, thanks!


_______________________________________________
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] applied: [PATCH v1 pve-common 07/18] pbsclient: s/foreach/for

[Thomas Lamprecht]

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

applied, thanks!


_______________________________________________
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

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

[Thomas Lamprecht]

Am 02.08.24 um 15:26 schrieb 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.

great works and thanks for improving the documentation of perl code, but I
think making this a bit more concise, e.g. shorting some examples or formats
might give us a better balance and ROI here, to avoid losing the important
things in the noise, so to say.

E.g. the 5 extra lines setting some $Data::Dumper modules key seem rather
out of place to me.


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

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

[Max Carrara]

On Mon Nov 11, 2024 at 8:14 PM CET, Thomas Lamprecht wrote:
> Am 02.08.24 um 15:26 schrieb 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.
>
> great works and thanks for improving the documentation of perl code, but I
> think making this a bit more concise, e.g. shorting some examples or formats
> might give us a better balance and ROI here, to avoid losing the important
> things in the noise, so to say.
>
> E.g. the 5 extra lines setting some $Data::Dumper modules key seem rather
> out of place to me.

Thanks a lot for your feedback! I agree with you here, shortening some
stuff would make things a little bit more readable.

For now I'm going to remove the extra $Data::Dumper lines and shorten
some of the data structure examples; in the latter case, I initially
tried to show multiple possible values in things like returned file
lists, e.g.:

[
  {
      '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"
  },
  ...
]

In this case, I'd shorten it to just the following:

[
  {
      'crypt-mode' => "encrypt",
      filename => "qemu-server.conf.blob",
      size => 428
  },
  ...
]

So, examples like that will be a little bit smaller in the next
revision.

Are there any other cases where you'd prefer to shorten things? I
already tried to be as concise / clear as possible my wording, but maybe
I'm overlooking something.



_______________________________________________
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

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

[Thomas Lamprecht]

Am 02.08.24 um 15:26 schrieb 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.

this is already enforced by pmxcfs for our case though?

And not sure about making the whole base path 0700, might be better
to create only the last directory as such? but this is generally something
where IMO lots can go wrong with little benefit, so not so sure about
this one.



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

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

[Max Carrara]

On Mon Nov 11, 2024 at 8:16 PM CET, Thomas Lamprecht wrote:
> Am 02.08.24 um 15:26 schrieb 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.
>
> this is already enforced by pmxcfs for our case though?

Even though pmxcfs enforces this, the library may still be used in a
context outside of pmxcfs. For example, I could write a simple script
with `use PVE::PBSClient;` for my own backup management purposes and use
a separate directory to store secrets.

This is obviously theoretical, *but* because the possibility exists that
*someone* out there might use the library that way, I decided to be a
bit more defensive here.

To give a more elaborate example, let's say I have a regular user
account on a PVE host and I use this library to do ... some stuff
regarding backups of my VMs the admin allows me to run on that host.
Because I don't have permissions to access `/etc/pve/priv/storage` (I
don't have rights to `root`) I set the secret dir to something like
"$HOME/.config/pve/priv/storage" or whatever.

Because the admin forgot to configure `/etc/skel`, the permissions of my
home directory are 0755 -- and because I forgot to change them myself,
my home directory is accessible to other users.

If we didn't set the secret dir to 0700 here, other users could access
my credentials.

>
> And not sure about making the whole base path 0700, might be better
> to create only the last directory as such? but this is generally something
> where IMO lots can go wrong with little benefit, so not so sure about
> this one.

Setting just the last dir 0700 seems better actually, I agree.

I'm still in favour to be a little restrictive here. Perhaps I'm a
little overly cautious; I'm not sure if anyone would ever find
themselves in the scenario I described above. Usually I don't want to
implement things upfront because they're merely "possible" or because
there's some magical, potential case it will be used either (YAGNI etc.)
but in this case it does have obvious implications regarding security
IMO.

As an alternative, we could also just not allow specifying a custom
secret dir and hardcode it to `/etc/pve/storage/priv`, then all of the
above isn't possible in the first place and doesn't need to be dealt
with.


_______________________________________________
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

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

[Thomas Lamprecht]

a commit message explaining why/benefits would be good – especially as IIRC
we currently do not use this module at all?


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

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

[Max Carrara]

On Mon Nov 11, 2024 at 8:16 PM CET, Thomas Lamprecht wrote:
> a commit message explaining why/benefits would be good – especially as IIRC
> we currently do not use this module at all?

Sure, I'll add one!

I've opted to using `File::Spec` here because I've been using it a lot
for other things (that haven't landed on the mailing list yet). The
reasoning is mostly something like "oh there's a core module that
handles concatenating paths and file names, I will use that instead of
doing it by hand".

... granted, this is maybe a little pedantic here; I mainly just thought
it wouldn't hurt. But yes, I could've specified that in the commit
message.

I can add a message or just drop the commit in the next revision, if
you'd like.



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

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

[Thomas Lamprecht]

Am 21.11.24 um 17:17 schrieb Max Carrara:
> On Mon Nov 11, 2024 at 8:16 PM CET, Thomas Lamprecht wrote:
>> a commit message explaining why/benefits would be good – especially as IIRC
>> we currently do not use this module at all?
> 
> Sure, I'll add one!
> 
> I've opted to using `File::Spec` here because I've been using it a lot
> for other things (that haven't landed on the mailing list yet). The
> reasoning is mostly something like "oh there's a core module that
> handles concatenating paths and file names, I will use that instead of
> doing it by hand".
> 
> ... granted, this is maybe a little pedantic here; I mainly just thought
> it wouldn't hurt. But yes, I could've specified that in the commit
> message.

For perl this does not gives one much, but hides a bit what's happening,
IMO a bit of a lateral move, but it can be OK I guess.

And FWIW, File::Spec->join is an alias to catfile, and IMO would be a
bit easier to read, so if, I'd go for that variant.


_______________________________________________
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

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

[Thomas Lamprecht]

Am 02.08.24 um 15:26 schrieb Max Carrara:
> Instead of returning an (arguably awkward) array with four elements,

can be fine, but FWIW, this is how our storage plugin system expects the
status, but currently that calls run_client_cmd itself, i.e. is basically a
copy of the original code here.

> 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(-)
> 


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

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

[Max Carrara]

On Mon Nov 11, 2024 at 8:17 PM CET, Thomas Lamprecht wrote:
> Am 02.08.24 um 15:26 schrieb Max Carrara:
> > Instead of returning an (arguably awkward) array with four elements,
>
> can be fine, but FWIW, this is how our storage plugin system expects the
> status, but currently that calls run_client_cmd itself, i.e. is basically a
> copy of the original code here.

Ah, thanks for pointing that out, I hadn't considered this, as the
`status` sub isn't in use currently.

Wouldn't it make sense to design the API of PVE::PBSClient independent
of other components, though? Of course it should be able to do what
other system components need it to do, but in this case, returning an
array is much less extendable (and IMO readable) than returning a hash.

To elaborate on my reasoning here: Since I had assumed that this is a
more general purpose PBS client lib, I didn't want its return types,
signatures, etc. to be too restrictively designed.

>
> > 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(-)
> > 



_______________________________________________
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