Am 20.02.25 um 11:40 schrieb Fabian Grünbichler: > Quoting Fiona Ebner (2025-02-20 10:03:09) >> Am 11.02.25 um 17:07 schrieb Daniel Kral: >>> Allow a caller to specify the volume's intended content type and assert >>> whether the specified content type may be stored on the specified >>> storage before allocating any volume. >>> >>> Signed-off-by: Daniel Kral <d.k...@proxmox.com> >>> --- >>> changes since v1: >>> - add assertion at `vdisk_alloc` instead of wrapper `alloc_volume_disk` >>> >>> src/PVE/Storage.pm | 10 ++++++++++ >>> 1 file changed, 10 insertions(+) >>> >>> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm >>> index 3776565..96d4e41 100755 >>> --- a/src/PVE/Storage.pm >>> +++ b/src/PVE/Storage.pm >>> @@ -1059,6 +1059,13 @@ Specifies the name of the new volume. >>> >>> If undefined, the name will be generated with >>> C<PVE::Storage::Plugin::find_free_diskname>. >>> >>> +=item * C<< $vtype => $string >> >>> + >>> +Specifies the content type of the new volume, which can be one of >>> C<'images'>, C<'rootdir'>, >>> +C<'vztmpl'>, C<'iso'>, C<'backup'>, C<'snippets'> or C<'import'>. >> >> 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. > > 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
