Re: [pve-devel] [PATCH qemu-server 01/13] blockdev: cmdline: add blockdev syntax support

From: Fiona Ebner <f.ebner@proxmox.com>
To: "DERUMIER, Alexandre" <alexandre.derumier@groupe-cyllene.com>,
	"pve-devel@lists.proxmox.com" <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH qemu-server 01/13] blockdev: cmdline: add blockdev syntax support
Date: Fri, 6 Jun 2025 09:50:39 +0200	[thread overview]
Message-ID: <6ed8b9a3-e706-42b1-84c6-d4078782c393@proxmox.com> (raw)
In-Reply-To: <ee4940c99e9866910405b492dad15c68718c49ea.camel@groupe-cyllene.com>

Am 05.06.25 um 16:39 schrieb DERUMIER, Alexandre:
> Am 03.06.25 um 09:55 schrieb Alexandre Derumier via pve-devel:
>> +sub encode_nodename {
>> +    my ($type, $volid, $snap) = @_;
>> +
>> +    my $nodename = "$volid";
>> +    $nodename .= "-$snap" if $snap;
> 
> This will lead to clashes in some cases:
>>> 1. Currently, we allow attaching the same volume multiple times to a
>>> single guest.
> do you mean, manually with editing the vm configuration ?  What is the
> usecase ? (I mean, without breaking the fs because the drive could be
> mounted twice )
> (the vm will not start if node-name is duplicated, so it's look more
> secure now ^_^)

No manual editing needed, just use "qm set" twice with the same volume
;) Sure, those are most likely quite exotic use cases. If we want to, we
could go ahead an prohibit this for PVE 9. There always is the -args
escape hatch for people that really need it. Would also avoid cases
where it's done accidentally, so I'm not opposed to this. We'd need to
document it as a breaking change in the upgrade guide and add a check to
the pve8to9 script.

>>> 2. You can end up with the same name for
>>> volname = vm-1234-disk-0-foo
>>> and for
>>> volname = vm-1234-disk-0, snap = foo
>>>
>>> The latter can be rather easily fixed by just using a character we
>>> don't
>>> usually support for volume names, but not the former.
> we could also generate the full path= , and encode it

Volume ID + snapshot should already be unique, we just need to
separate/map it in such a way that it cannot be confused as a volume ID
when there is a snapshot. E.g. could also be the encoding of the string
"snap=Y,volid=X". Using the full QEMU path seems overly involved.

>>> I think we can even rely on auto-generated-by-QEMU node names at
>>> first.
> 
> The main blocking problem is that auto-generated #block name are not
> working with some block action. (if I remember, the drive-reopen wasn't
> working correctly, so this was a blocker for snapshots).
> I need to retest because in early patches, I was reopening the file
> node, and now the format-node, so maybe the behaviour is different now
> 
> 
> They are also different behaviour is node-name are defined or notto the
> command-line on blockdev-remove.  (for example, sometime you remove the
> top-node, the format+file node can be autoremove or not, also the
> backing chain)
> 
> I really don't known if it's qemu bug or not. (I don't have digged the
> qemu code)

Hmm, will have to look into this. For top-level nodes, it's required to
specify an explicit node-name in any case.

>>> I'll try to work out a series that focuses just on the switch to
>>> "-blockdev" based on your patches during the next week or so. Maybe
>>> not
>>> much else needs to be changed :) The work is certainly greatly
>>> appreciated!
> 
> Great! Tell me if it needed testing. (my patches should cover all basic
> actions excluding backup)

Sure, testing is always appreciated! I'll focus on this and hope to get
a version out by the end of next week.

> I'm currently working to add tests for all live action
> (hotplug/unplug,cdeject,mirror,snap,...but until it's done, I can help
> with manually testing.

Such automatic tests would be very nice to have indeed! Even better if
we have the tests before the switch to blockdev is applied, so we can
check that the behavior is still the same :)

Best Regards,
Fiona

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