Re: [pve-devel] [PATCH qemu-server v3 1/6] fix #4225: qemuserver: drive: add parameter to mark drive required

[Fiona Ebner <f.ebner@proxmox.com>]

The 'qemuserver' prefix in the commit title doesn't add any information
and should not be there. Commit title prefixes are not for file names.
This also doesn't fix the issue yet, so I'd also drop that prefix too.

Am 30.01.25 um 12:31 schrieb Daniel Herzig:
> This commit add the parameter `essential` to mark a drive as required
> for booting the VM.
> 
> Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
> ---
>  PVE/QemuServer/Drive.pm | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
> index 1041c1dd..38136787 100644
> --- a/PVE/QemuServer/Drive.pm
> +++ b/PVE/QemuServer/Drive.pm
> @@ -266,7 +266,14 @@ my %drivedesc_base = (
>  	verbose_description => "Mark this locally-managed volume as available on all nodes.\n\nWARNING: This option does not share the volume automatically, it assumes it is shared already!",
>  	optional => 1,
>  	default => 0,
> -    }
> +    },
> +    essential => {
> +	type => 'boolean',
> +	description => 'Mark this iso volume as required for booting the VM.',

Nit: Since the 'media=cdrom' option is used to decide this, I'd state
"CD-ROM" here to be more precise. I'd also say "for starting the VM"
rather than "for booting the VM". The device isn't necessarily involved
into booting, but can still be considered essential to allow starting
the VM.

> +	verbose_description => 'If unset or set to 1, and the iso file is unavailable, the VM will not start.\nThis parameter is considered for cdrom iso drives only.',
> +	optional => 1,
> +	default => 1,
> +    },
>  );
>  
>  my %iothread_fmt = ( iothread => {

There should be some checking/handling in the API endpoints and/or
parse_drive() for making sure this can only be set in combination with
'media=cdrom'. There are already such checks in parse_drive() which will
return undef in those cases, but please also issue a warning for a new
such check so that it will be clear what went wrong ;)

Nit: Usually, it's nicer to have booleans be 0 if not present. So can we
invert this e.g. "ignore-if-missing" (or "detach-if-missing" or
"eject-if-missing")? Otherwise, maybe "essential-for-start" to be more
descriptive?


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