Re: [pve-devel] [PATCH pve-storage v2 2/5] introduce helpers for content type assertions of storages and volumes

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

Am 20.02.25 um 10:14 schrieb Daniel Kral:
> On 2/19/25 15:54, Fiona Ebner wrote:
>> Am 11.02.25 um 17:07 schrieb Daniel Kral:
>>> +sub assert_content_type_supported : prototype($$$;$) {
>>> +    my ($cfg, $storeid, $content_type, $node) = @_;
>>> +
>>> +    my $scfg = storage_config($cfg, $storeid, $node);
>>
>> The storage_config() function does not have a $node parameter, but a
>> $noerr parameter. I guess you want to use storage_check_enabled() since
>> the documentation talks about "storage being unavailable"?
> 
> Uff sorry, that's right that was an oversight and doesn't make sense...
> 
> Yes you're correct, I had the `storage_check_enabled` here before, but
> replaced it in the end but forgot to remove the parameter.
> 
> There are AFAIK 5 instances where I used this helper where there wasn't
> a `storage_check_enabled` before and I was worried about breaking any
> existing checks at these locations and another helper (with just an
> added `storage_check_enabled`) felt like bloat.
> 
> I checked the locations again where there wasn't a
> `storage_check_enabled` before:
> - qemu-server patch #9 (in `config_to_command`),
> - qemu-server patch #11 (in `check_storage_access`),
> - qemu-server patch #13 (in `parse_backup_hints`),
> - container patch #10 (in `update_pct_config`), and
> - container patch #11 (in `__mountpoint_mount`).
> 
> If we could use the `storage_check_enabled` in all of those, I'd move
> the `storage_check_enabled` in the helper method and add to each patch
> message where there was a `storage_check_enabled` before with "No
> functional changes intended" and those where it wasn't with a reason why
> it makes sense to add one now. If I didn't miss anything:
> 
> - qemu-server #9 - `config_to_command` obviously fails at another
> location if the storage is not currently enabled anyway

Not necessarily? What about qm showcmd <ID>? Question is, do we want to
do storage-related checks there or not? If we already check for the
content type, I don't see much reason not to check for storage being
enabled either.

It could be seen as surprising, because it's just building the command,
why would it do storage checks? But if we follow that rationale, it
shouldn't do either of the checks.

I'm fine with keeping/adding the checks, because we already do that for
other things while building the command too. Otherwise, we'd need some
nocheck flag or similar. I have no strong opinion against that either.
Just nobody complained yet about this I guess and there's nothing really
wrong with having the semantics be getting a "checked" command.

> - qemu-server #11 - `check_storage_access` is called in the create_vm
> and update_vm API handler... where it checks whether any new disk can be
> put on the storage (also fails when the storage is not enabled)
> - qemu-server #13 - `parse_backup_hints` is used in
> `restore_vma_archive` and `restore_proxmox_backup_archive` to parse and
> check whether the devices can be allocated (with
> `$restore_allocate_devices` afterwards)
> - container #10 - `update_pct_config` asserts whether new or changed
> mountpoints can be allocated... and fails if the storage is not enabled
> - container #11 - `__mountpoint_mount` is used in more places, but all
> look to need the volume on the storage immediately afterwards anyway
> (`snapshot`, `archive`, resize_vm API handler, `copy_volume`)... and I
> guess it would fail in any of those with
> `PVE::Storage::activate_volumes` calling `activate_storage` before the
> helper anyway
> 
> So if I didn't miss anything, I'd do this in a v3.

It does seem natural that callers that check for a content type being
supported actually want to do something with that content type too and
actually care about the content type being "ready"/"available". The only
cases I would imagine is checks about the storage config whether it
would be supported in principle, but if we ever have one of those, we
just don't need to use the helper.


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