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

[Fiona Ebner]

Am 20.02.25 um 13:33 schrieb Daniel Kral:
> 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?

For PVE 9, we might already have encoded the type in the name.

For now, you could do as Fabian suggested, i.e. either just look at the
storage config or at support a placeholder value. The storage layer
should not be concerned with guests. The single existing get_vmlist()
call in the storage modules is already frowned upon :P and the guest
might not exist yet.