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

["DERUMIER, Alexandre via pve-devel" <pve-devel@lists.proxmox.com>]

[-- Attachment #1: Type: message/rfc822, Size: 16092 bytes --]

From: "DERUMIER, Alexandre" <alexandre.derumier@groupe-cyllene.com>
To: "pve-devel@lists.proxmox.com" <pve-devel@lists.proxmox.com>, "f.ebner@proxmox.com" <f.ebner@proxmox.com>
Subject: Re: [pve-devel] [PATCH qemu-server 02/14] blockdev: cmdline: convert drive to blockdev syntax
Date: Tue, 6 May 2025 14:48:11 +0000
Message-ID: <62a84f38e357d17eec4102660dfaa684ff3eb35b.camel@groupe-cyllene.com>

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

yes, sure !

> +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?

will 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"?
Didn't have thinked about this, but you are right, if user is currently
allowed to create an "nbd" storage, I'm pretty sure that some of them
have done it  ^_^


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

Do we need to handle old client (for example, a pve8 doing a live
migrate + lcoal storage migration  sending a nbd:// uri to a pve9 with
blockdev support ) ?




> + #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. 
yes, it was to avoid to touch other storage plugins for now, but it
could be better to have a clean method indeed sending a hash (I had an
error with ipv6 parsing of rbd for example)
(I think that old path syntax need to be keeped for qemu-img too)






[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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