Re: [pve-devel] [PATCH qemu-server 20/22] blockdev: add helpers to generate blockdev commandline

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

Am 13.06.25 um 13:35 schrieb Fabian Grünbichler:
> On June 12, 2025 4:02 pm, Fiona Ebner wrote:
>> The drive device and node structure is:
>> front-end device {ide-hd,scsi-hd,virtio-blk-pci} (id=$drive_id)
>>  - throttle node (node-name=$drive_id)
> 
> should we prefix this is well to not "use" the basic name (e.g., in case
> in the future a new "topmost" node comes along)?

We could, but what would be the advantage?

We can just rename it when the time comes and call the new top-most node
$drive_id then. We reference the node from the front-end device, so if
we do it now, the name would also need to be adapted there and again in
the future. But I'd rather like keeping the front-end device referring
to $drive_id and making sure that is the one we want also in the future.

We want to operate below the throttle node, because the throttling would
also apply to block jobs, not just guest reads, which we do not want. So
it's very likely that we want to insert any future node in the chain
still below the throttling node.

>> +my sub read_only_json_option {
>> +    my ($drive, $options) = @_;
>> +
>> +    return JSON::true if $drive->{ro} || drive_is_cdrom($drive) || $options->{'read-only'};
>> +    return JSON::false;
> 
> should we maybe have a generic jsonify-helper instead?

For bool, I'll add one to pve-common since I need to touch that anyways.
But in general, we cannot know if a string that looks like a number
should be a number or not without additional info.

> this is only
> called twice, but we have (counting this as well) three call sites here
> that basically do
> 
> foo ? JSON::true : JSON::false
> 
> which could become `json_bool(foo)`
> 
> with a few more in PVE::VZDump::QemuServer and other qemu-server
> modules..
> 
> we could still have a drive_is_ro helper in any case ;)

>> diff --git a/PVE/QemuServer/Makefile b/PVE/QemuServer/Makefile
>> index 8054a93b..e3671e5b 100644
>> --- a/PVE/QemuServer/Makefile
>> +++ b/PVE/QemuServer/Makefile
>> @@ -12,6 +12,7 @@ SOURCES=PCI.pm		\
>>  	MetaInfo.pm	\
>>  	CPUConfig.pm	\
>>  	CGroup.pm	\
>> +	Blockdev.pm	\
> 
> already added by the previous patch ;)

Right, too much rebasing :P

> should we sort this list?

Will add a patch in v2.


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