On Mon, Jun 22, 2026 at 18:06:18 +0100, Daniel P. Berrangé wrote: > On Wed, Apr 01, 2026 at 04:31:22PM +0200, Peter Krempa wrote: > > On Wed, Apr 01, 2026 at 15:17:29 +0100, Daniel P. Berrangé wrote: > > > On Wed, Apr 01, 2026 at 03:27:15PM +0200, Peter Krempa via Devel wrote: > > > > From: Peter Krempa <[email protected]> > > > > > > > > In certain cases it's useful for the caller to be able to determine if > > > > given API supports some flags. > > > > > > > > To do this with 'virDomainBlockResize', if > > > > 'VIR_DOMAIN_BLOCK_RESIZE_PROBE_FLAGS' is specified, the API will return > > > > success right after flag verification, regardless of other input > > > > arguments and without modifying anything. > > > > > > > > Signed-off-by: Peter Krempa <[email protected]> > > > > --- > > > > include/libvirt/libvirt-domain.h | 9 +++++++++ > > > > src/libvirt-domain.c | 12 ++++++++++++ > > > > src/qemu/qemu_driver.c | 7 ++++++- > > > > src/vz/vz_driver.c | 7 ++++++- > > > > 4 files changed, 33 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/include/libvirt/libvirt-domain.h > > > > b/include/libvirt/libvirt-domain.h > > > > index 4a8e3114b3..113c7eefe6 100644 > > > > --- a/include/libvirt/libvirt-domain.h > > > > +++ b/include/libvirt/libvirt-domain.h > > > > @@ -2354,6 +2354,15 @@ typedef enum { > > > > > > > > /* Disallow shrinking (Since: 12.3.0) */ > > > > VIR_DOMAIN_BLOCK_RESIZE_EXTEND = 1 << 2, > > > > + > > > > + /* Ensures that the 'virDomainBlockResize' API returns success > > > > after > > > > + * checking support of 'flags' regardless of other arguments > > > > without > > > > + * actually modifying any aspect of the domain. > > > > + * > > > > + * Use of the flag thus allows probing for support of other flags > > > > of this > > > > + * API. (Since: 12.3.0) */ > > > > + VIR_DOMAIN_BLOCK_RESIZE_PROBE_FLAGS = 1 << 3, > > > > + > > > > } virDomainBlockResizeFlags; > > > > > > > > int virDomainBlockResize (virDomainPtr dom, > > > > diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c > > > > index db9eea5774..1b49d2f7e5 100644 > > > > --- a/src/libvirt-domain.c > > > > +++ b/src/libvirt-domain.c > > > > @@ -6469,6 +6469,18 @@ virDomainBlockPeek(virDomainPtr dom, > > > > * can be found by calling virDomainGetXMLDesc() and inspecting > > > > * elements within //domain/devices/disk. > > > > * > > > > + * If @flags contains VIR_DOMAIN_BLOCK_RESIZE_PROBE_FLAGS (since > > > > 12.3.0) the > > > > + * API will return success right after checking flags for support > > > > without > > > > + * modifying the disk in any way, thus allowing probing of flags with > > > > the > > > > + * following algorithm > > > > + * > > > > + * supp = virDomainBlockResize(dom, "", 0, > > > > VIR_DOMAIN_BLOCK_RESIZE_PROBE_FLAGS | VIR_DOMAIN_BLOCK_RESIZE_EXTEND); > > > > + * > > > > + * if (supp == 0) > > > > + * //flag supported > > > > + * else > > > > + * virResetLastError(); > > > > > > This suggests "flag" (singular), but if you pass many flags at once, > > > then the error only tells you that at least 1 flag is invalid, but > > > not which flag is invalid. So we need O(n) calls with PROBE_FLAGS > > > if we want to check n different flags as distinct things. > > > > I thought about this but I don't expect that this would be too widely > > used, so the inability to query flags in bulk shouldn't be a problem. > > > > I don't think there's a reasonable possibility to do this within the API > > itself, and would thus require a new API. > > > > > Also the "else" branch can also still be an error for non-probe > > > related issues. eg there are many errors that the remote driver > > > can raise from a transport POV. > > > > I didn't think about this but if used in close proximity with the real > > call I don't think there are many options where the probing call would > > error out but then the further real call would succeed. > > > > > How much better is this than simply trying the RESIZE_EXTEND usage > > > unconditionally, and then falling back to a different code path if > > > seeing INVALID_ARG error ? > > > > The unfortunate thing and the reason I've picked this approach is that > > the code that follows here liberally uses VIR_ERR_INVALID_ARG. Even on > > code paths where it makes sense to report error: > > > > if (!(disk = virDomainDiskByName(vm->def, path, false))) { > > virReportError(VIR_ERR_INVALID_ARG, > > _("disk '%1$s' was not found in the domain config"), > > path); > > goto endjob; > > } > > > > The caller (visible later in series) wouldn't be able to determine if > > this is a case where the user messed up the disk and needs to see error > > or is a case where the failure is from the real think we want to prevent > > (accidental shrinking of the disk). > > > > It would be possible to add a new error code for the 'unallowed shrink' > > though to sidestep that problem although it would really be specific to > > this scenario. > > This isn't the first time where "INVALID ARG" has not been as helpful > as it should have been. It is pretty hard to do control flow off > very generic errors codes like this, especially when we use it in > examples like the one above that are arguably inappropriate. > > Is there mileage in adding a highly tailored error code > "UNSUPPORTED_FLAGS" ? We are lucky in that we use virCheckFlags as a > macro everywhere which would make changing the code fairly easy.
This doesn't really help detecting if the daemon is too old to support the new UNSUPPORTED_FLAGS code. A caller who needs to know if the daemon is too old would need to try an existing API with a nonsense flag first risking that the flag does make sense. Nevertheless I think that it makes sense to add this code and fix virCheckFlags for the future. I can do that as a follow up, but that doesn't in any way detract from the problems that I've tried to fix either by using the extre 'PROBE_FLAGS' per this patch or the capability probing API.
