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

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

On 2/20/25 10:36, Fiona Ebner wrote:
> 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.

Right, there's also `qm showcmd`, I only thought about the 
vm_start_nolock case here! I guess the best thing would be to move it 
there, or would an extra iteration through the disks like in cfg2cmd be 
too expensive there? Otherwise I'll let it be in cfg2cmd.

On 2/20/25 10:36, Fiona Ebner wrote:
> 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.

Yes, I'd double check that when working on the v3, but as far as I've 
seen how they current have been used and going through the cases above 
it'd sound like they were used like this before anyway.


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