Re: [pve-devel] [PATCH] Check only first part of script parameter for executability

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Hi Phillipp,

On 21/10/2021 20:15, Phillipp Röll wrote:
> I'll skip the legal stuff for now, until we figure out if the patch has a chance
> to be merged, ok?

sounds fine to me.

> 
>> That would break existing configuration that use a script with whitespace in its
>> path name.
> 
> There could be two checks, check if either the full script name is executable OR
> just the first part (split by whitespaces).

Hmm, that could be an option, for splitting I'd recommend using the `split_args` helper
from the PVE::Tools module, it's made for this case and should handle some subtleties
better than a plain split on any whitespace.

> 
>> Can you please provide your usecase where the arguments Proxmox VE sets for the
>> script aren't enough? So that we've a better picture about possible options.
> 
> We have a backup & restore management system, where the state, retention period etc.
> of a backup is stored. All proxmox backup states are updated by a hook script calling
> a REST API. We need to pass the ID of the backup from the management system to the
> hook script, which is no longer possible since the check was introduced.

Is the ID related to the VMID? As in, could it be possible to save a mapping from
VMID to current backup ID somewhere (node local or cluster shared on /etc/pve through
pmxcfs), e.g., as JSON file, read that file in the hook script and get the BACKUP ID
then from that mapping using the VMID as key?

Could be slightly more robust and could potentially reduce the amount of REST calls
and pmxcfs updates, I mean those resources are not _that_ scarce or performance
critical, but with many guests and big clusters the pmxcfs load may get noticeable.

But, that said, I'm open into introducing your implementation, that formerly rather
worked by accident, as actual and documented possible behaviour.

> 
> We comment/patch these checks out automatically on all systems, but that's a bit
> annoying.
> 
> So actually the (much) earlier introduction of the check already broke our use case,
> but before today the pain to report it/change something simply wasn't great enough.
> 

It could help leading with that in the commit message or the like, as having an
existing use case broken should get you more attention in your favour compared to
people thinking you want add a new behaviour as we do not want to break existing
use cases if possible, but that side effect was not recognized as being used :)