Re: [pve-devel] [PATCH qemu-server 02/14] blockdev: cmdline: convert drive to blockdev syntax

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

Maybe we can add the new code to a dedicated PVE/QemuServer/Blockdev.pm
module? Drive.pm is also not the smallest anymore.

Am 22.04.25 um 13:51 schrieb Alexandre Derumier via pve-devel:
> @@ -87,7 +89,7 @@ sub get_cdrom_path {
>  }
>  
>  sub get_iso_path {
> -    my ($storecfg, $vmid, $cdrom) = @_;
> +    my ($storecfg, $cdrom) = @_;
>  
>      if ($cdrom eq 'cdrom') {
>  	return get_cdrom_path();

Applied this hunk already and adapted the single caller (that your patch
would move):
https://lore.proxmox.com/pve-devel/20250506125631.64310-1-f.ebner@proxmox.com/T/#u

> +sub print_drive_throttle_group {
> +    my ($drive) = @_;
> +
> +    return if drive_is_cdrom($drive) && $drive->{file} eq 'none';
> +
> +    my $group = generate_throttle_group($drive);
> +    $group->{'qom-type'} = "throttle-group";
> +    return JSON->new->canonical->allow_nonref->encode($group)

Style nit: missing semicolon.
Can you also use to_json($group, { canonical => 1, ... }); here like we
usually do?

> +}
> +
> +sub generate_file_blockdev {
> +    my ($storecfg, $drive, $snap, $nodename) = @_;
> +
> +    my $volid = $drive->{file};
> +    my $blockdev = {};
> +
> +    my $scfg = undef;
> +    my $path = $volid;
> +    my $storeid = undef;
> +
> +    if($path !~ m/^nbd:(\S+)$/) {

What if there is a storage called "nbd"? Maybe the first part here for
obtaining the path should be separate (and should not be used for the
call-site with NBD). generate_file_blockdev() can receive the path and
either the relevant drive options only or still the whole drive, but
should not look at the drive->{file} anymore.

Or considering my proposal below, generate_file_blockdev() should
receive a hash with blockdev options associated to the path/volume, e.g.
{ driver => 'rbd', conf => ... , id => ..., server => ..., pool => ...}.
But not yet cache/aio/etc. that should be handled here. The NBD
call-site will pass in a hash with { driver 'nbd', ... }, the other call
sites, will ask the storage layer and pass in the result from that.

> +
> +	($storeid) = PVE::Storage::parse_volume_id($volid, 1);
> +	my $vtype = $storeid ? (PVE::Storage::parse_volname($storecfg, $drive->{file}))[0] : undef;
> +	die "$driveid: explicit media parameter is required for iso images\n"
> +	    if !defined($drive->{media}) && defined($vtype) && $vtype eq 'iso';
> +
> +	if (drive_is_cdrom($drive)) {
> +	    $path = get_iso_path($storecfg, $volid);
> +	} elsif ($storeid) {
> +	    $path = PVE::Storage::path($storecfg, $volid, $snap);
> +	    $scfg = PVE::Storage::storage_config($storecfg, $storeid);
> +	}
> +    }
> +
> +    if ($path =~ m/^rbd:(\S+)$/) {
> +
> +        my $rbd_options = $1;
> +        $blockdev->{driver} = 'rbd';
> +
> +	#map options to key=value pair (if not key is provided, this is the image)
> +	#options are seprated with : but we need to exclude \: used for ipv6 address
> +	my $options = {
> +	    map {
> +		s/\\:/:/g; /^(.*?)=(.*)/ ? ($1=>$2) : (image=>$_)
> +	    } $rbd_options =~ /(?:\\:|\[[^\]]*\]|[^:\\])+/g
> +	};

Maybe we should add a new method to the storage plugin API to give us a
hash with the necessary QEMU blockdev options? Because right now, we
construct an implicit QEMU-path just to deconstruct it again, which is
not nice at all. We can implement that for all our plugins and, for
backwards-compatibility handling with third-party plugins, the default
implementation in Plugin.pm could have the code you put here.

Just not sure if the RBD plugin should put the generated ceph config in
/run/qemu-server/${storeid}.ceph.conf then, or use some kind of
/run/pve-storage path?

CC @Fabian, opinions?


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