Re: [pve-devel] [PATCH pve-storage v2 5/5] vdisk_alloc: add optional assertion for volume's content type

[Daniel Kral <d.kral@proxmox.com>]

On 2/20/25 11:48, Fiona Ebner wrote:
> Am 20.02.25 um 11:40 schrieb Fabian Grünbichler:
>> Quoting Fiona Ebner (2025-02-20 10:03:09)
>>> Hmm, vdisk_alloc() is only for allocating guest and import images. So I
>>> think the other content types should be prohibited here (can still be
>>> extended later if it ever comes up).
>>>
>>> We plan to better distinguish between 'rootdir' and 'images' in the
>>> future, so I think we should aim to make the $vtype parameter even
>>> required here. Not quite possible yet, because that would require
>>> breaking 'pvesm alloc', but we can postpone that part for PVE 9 and have
>>> all other callers already use it.
>>>
>>> So my question is if we should rather make it a required parameter
>>> already rather than putting it into $opts? I mean while supporting
>>> passing in undef too, until we are ready to require it for 'pvesm alloc'
>>> too.
>>>
>>> @Fabian sounds good to you too?
>>
>> we could also make it required for real in vdisk_alloc, and make `pvesm alloc`
>> auto-select one of the allowed ones based on the storage config and error out
>> if the storage doesn't support either rootdir or images? or use a different
>> "magic" placeholder value like "any" - using undef is a bit opaque and could
>> happen by accident, although it is not very likely for this hash ;) then when
>> we introduce properly scoped volume names, we can replace that fallback logic
>> in `pvesm alloc` with properly setting *either* rootdir or images, depending on
>> what gets allocated?
> 
> Sounds good to me. I'm in favor of making it required already, since we
> already need versioned breaks for the series.

Sounds like a good plan, then I'll move the $vtype parameter into the 
signature itself in a v3 and implement the behavior suggested by 
@Fabian, thanks!

Thinking about the changes for PVE 9, I'd best retrieve the type of the 
resource behind $vmid with `PVE::Cluster::get_vmlist()` to select 
between the two content types, right?

> 
>>
>> OTOH, vdisk_alloc doesn't have too call sites anyway, so doing $opts now, and
>> then updating those when it becomes a regular argument would also work fine I
>> think.. the only downside to that approach is that we might miss setting the
>> option for newly introduce callers in the meantime..



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