[pve-devel] [PATCH common 1/1] PBSClient: add option for extra parameter to file_restore_list

[Wolfgang Bumiller]

On Wed, Feb 09, 2022 at 06:35:33PM +0100, Thomas Lamprecht wrote:
> On 27.01.22 11:55, Dominik Csapak wrote:
> > we will need some extra parameters here, and instead of hardcoding them,
> > have the option to set a list of arbitrary parameters
> > 
> > Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> > ---
> >  src/PVE/PBSClient.pm | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
> > index 21dc363..dfb9f27 100644
> > --- a/src/PVE/PBSClient.pm
> > +++ b/src/PVE/PBSClient.pm
> > @@ -342,11 +342,15 @@ sub status {
> >  };
> >  
> >  sub file_restore_list {
> > -    my ($self, $snapshot, $filepath, $base64) = @_;
> > +    my ($self, $snapshot, $filepath, $base64, $extraParams) = @_;
> > +
> > +    my $params = [ $snapshot, $filepath, "--base64", $base64 ? 1 : 0 ];
> > +    push @$params, @$extraParams;
> > +
> >      return run_client_cmd(
> >  	$self,
> >  	"list",
> > -	[ $snapshot, $filepath, "--base64", $base64 ? 1 : 0 ],
> > +	$params,
> >  	0,
> >  	"proxmox-file-restore",
> >      );
> 
> CC'ing Wolfgang, as IIRC he does not like passing "take anything" variables
> especially in such dynamic languages like perl.
> 
> FWICT we only call file_restore_list once, and you set the options fixed
> there now, so why not just avoid the new method param and pass it directly
> fixed here?

Indeed, I'm not against having a hash there where we can specify
possible extra options, but not in the form of command line parameters
that are passed as-is. The question is rather how many more parameters
will be required in the future and will you just add a single `$timeout`
or a `%options` containing a `->{timeout}` field ;-)

And `--json-error` seems to be generally a good idea to be always set
for a CLI-tool-based perl api so it can just `die` with the right
errors, no? Wouldn't it make sense to add this directly to
`run_client_cmd`?