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.eb...@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
