[PATCH 0/6] qemu: Add infrastructure to prevent accidental disk shrink via 'virDomainBlockResize'
Shrinking a disk accidentally equals data loss and thus very bad time for anyone mis-typing. This series adds a new flag to virDomainBlockResize to prevent such a thing if the user only intends to extend the disk. In virsh I've went a bit further and actually change the behaviour of 'virsh blockresize' to use the new feature if available to prevent mishaps. Peter Krempa (6): virDomainBlockResizeFlags: Convert to prefix-style docs API/qemu: Introduce 'VIR_DOMAIN_BLOCK_RESIZE_EXTEND' for 'virDomainBlockResize' virsh: blockresize: Introduce '--extend' flag API: Introduce 'VIR_DOMAIN_BLOCK_RESIZE_PROBE_FLAGS' for 'virDomainBlockResize' virsh/virt-admin: Add possibility to probe for new arguments virsh: blockresize: Use VIR_DOMAIN_BLOCK_RESIZE_EXTEND when available and introduce --allow-shrink docs/manpages/virsh.rst | 31 +++++++++++++++++++++++++- include/libvirt/libvirt-domain.h | 19 ++++++++++++++-- src/libvirt-domain.c | 17 +++++++++++++++ src/qemu/qemu_driver.c | 37 +++++++++++++++++++++++++++++++- src/vz/vz_driver.c | 7 +++++- tools/virsh-domain.c | 23 ++++++++++++++++++++ tools/virsh.c | 4 ++++ tools/virt-admin.c | 9 ++++++++ tools/vsh.c | 3 +++ tools/vsh.h | 1 + 10 files changed, 146 insertions(+), 5 deletions(-) -- 2.53.0
From: Peter Krempa <pkrempa@redhat.com> Upcoming patches will want to add more extensive docs for one of the new flags so this format will make it more readable. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- include/libvirt/libvirt-domain.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 09a797cab6..084debbf21 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -2346,8 +2346,11 @@ int virDomainBlockPeek (virDomainPtr dom, * Since: 0.9.11 */ typedef enum { - VIR_DOMAIN_BLOCK_RESIZE_BYTES = 1 << 0, /* size in bytes instead of KiB (Since: 0.9.11) */ - VIR_DOMAIN_BLOCK_RESIZE_CAPACITY = 1 << 1, /* resize to full the capacity of the source (Since: 10.0.0) */ + /* size in bytes instead of KiB (Since: 0.9.11) */ + VIR_DOMAIN_BLOCK_RESIZE_BYTES = 1 << 0, + + /* resize to full the capacity of the source (Since: 10.0.0) */ + VIR_DOMAIN_BLOCK_RESIZE_CAPACITY = 1 << 1, } virDomainBlockResizeFlags; int virDomainBlockResize (virDomainPtr dom, -- 2.53.0
From: Peter Krempa <pkrempa@redhat.com> Introduce a new flag VIR_DOMAIN_BLOCK_RESIZE_EXTEND which will prevent accidental shrinking of the block device. Warn callers that they ought to use it. While this won't prevent any old uses without the flag (which we couldn't change due to our API guarantees) it will give the users tools to handle the resizing of devices more safely. Implement it in the qemu driver. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- include/libvirt/libvirt-domain.h | 3 +++ src/libvirt-domain.c | 5 +++++ src/qemu/qemu_driver.c | 32 +++++++++++++++++++++++++++++++- 3 files changed, 39 insertions(+), 1 deletion(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 084debbf21..4a8e3114b3 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -2351,6 +2351,9 @@ typedef enum { /* resize to full the capacity of the source (Since: 10.0.0) */ VIR_DOMAIN_BLOCK_RESIZE_CAPACITY = 1 << 1, + + /* Disallow shrinking (Since: 12.3.0) */ + VIR_DOMAIN_BLOCK_RESIZE_EXTEND = 1 << 2, } virDomainBlockResizeFlags; int virDomainBlockResize (virDomainPtr dom, diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index ed51180c91..db9eea5774 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -6451,6 +6451,11 @@ virDomainBlockPeek(virDomainPtr dom, * size. Depending on the file format, the hypervisor may round up * to the next alignment boundary. * + * *BEWARE*: The block device may be shrunk if @size is less than the current + * guest visible size. Callers who wish to only extend @disk should always pass + * VIR_DOMAIN_BLOCK_RESIZE_EXTEND (since 12.3.0) in @flags which instructs the + * hypervisor to deny an attempt to shrink the device. + * * If @flag contains VIR_DOMAIN_BLOCK_RESIZE_CAPACITY (since 10.0.0) the * hypervisor will resize the guest block device to fully fill the source. * @size must be either set to zero, or to the exact size of the block diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 861795724a..551553df14 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9448,7 +9448,8 @@ qemuDomainBlockResize(virDomainPtr dom, virDomainDiskDef *persistDisk = NULL; virCheckFlags(VIR_DOMAIN_BLOCK_RESIZE_BYTES | - VIR_DOMAIN_BLOCK_RESIZE_CAPACITY, -1); + VIR_DOMAIN_BLOCK_RESIZE_CAPACITY | + VIR_DOMAIN_BLOCK_RESIZE_EXTEND, -1); /* We prefer operating on bytes. */ if ((flags & VIR_DOMAIN_BLOCK_RESIZE_BYTES) == 0) { @@ -9587,11 +9588,40 @@ qemuDomainBlockResize(virDomainPtr dom, if (!qemuDiskBusIsSD(disk->bus)) { nodename = qemuBlockStorageSourceGetEffectiveNodename(disk->src); } else { + if (flags & VIR_DOMAIN_BLOCK_RESIZE_EXTEND) { + /* technically possible, just not implemented */ + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("cannot prevent shrink on SD devices")); + goto endjob; + } + if (!(device = qemuAliasDiskDriveFromDisk(disk))) goto endjob; } qemuDomainObjEnterMonitor(vm); + + if (flags & VIR_DOMAIN_BLOCK_RESIZE_EXTEND) { + g_autoptr(GHashTable) blockNamedNodeData = qemuMonitorBlockGetNamedNodeData(priv->mon); + qemuBlockNamedNodeData *entry; + + if (!(entry = virHashLookup(blockNamedNodeData, nodename))) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("failed to determine current size of '%1$s'"), + path); + qemuDomainObjExitMonitor(vm); + goto endjob; + } + + if (entry->capacity > size) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("resize of '%1$s' would shrink it from '%2$llu' to '%3$llu' bytes"), + path, entry->capacity, size); + qemuDomainObjExitMonitor(vm); + goto endjob; + } + } + if (qemuMonitorBlockResize(priv->mon, device, nodename, size) < 0) { qemuDomainObjExitMonitor(vm); goto endjob; -- 2.53.0
From: Peter Krempa <pkrempa@redhat.com> Use the new VIR_DOMAIN_BLOCK_RESIZE_EXTEND to prevent accidentally shrinking a disk and thus destroying data. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/manpages/virsh.rst | 7 ++++++- tools/virsh-domain.c | 7 +++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 4b8bbec7d6..80b0ea14a8 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -1642,7 +1642,7 @@ blockresize :: - blockresize domain path ([size] | [--capacity]) + blockresize domain path ([size] | [--capacity]) [--extend] Resize a block device of domain while the domain is running, *path* specifies the absolute path of the block device; it corresponds @@ -1650,6 +1650,11 @@ to a unique target name (<target dev='name'/>) or source file (<source file='name'/>) for one of the disk devices attached to *domain* (see also ``domblklist`` for listing these names). +The *--extend* flag instructs the hypervisor to ensure that the new size isn't +smaller than the old size, thus prevents (accidental) shrinking of the block +device which may lead to data loss. Users are encouraged to always use this +flag unless communicating with an older hypervisor. + For image formats without metadata (raw) stored inside fixed-size storage (e.g. block devices) the --capacity flag can be used to resize the device to the full size of the backing device. diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index a17c8b42e2..08a1ce3953 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -3339,6 +3339,10 @@ static const vshCmdOptDef opts_blockresize[] = { .type = VSH_OT_BOOL, .help = N_("resize to capacity of source (block device)") }, + {.name = "extend", + .type = VSH_OT_BOOL, + .help = N_("ensure that the new size is larger than actual capacity (prevent shrink)") + }, {.name = NULL} }; @@ -3352,6 +3356,9 @@ cmdBlockresize(vshControl *ctl, const vshCmd *cmd) VSH_ALTERNATIVE_OPTIONS("size", "capacity"); + if (vshCommandOptBool(cmd, "extend")) + flags |= VIR_DOMAIN_BLOCK_RESIZE_EXTEND; + if (vshCommandOptString(ctl, cmd, "path", (const char **) &path) < 0) return false; -- 2.53.0
From: Peter Krempa <pkrempa@redhat.com> 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 <pkrempa@redhat.com> --- 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(); + * * Note that this call may fail if the underlying virtualization hypervisor * does not support it; this call requires privileged access to the * hypervisor. diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 551553df14..aa064ca7a9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9449,7 +9449,12 @@ qemuDomainBlockResize(virDomainPtr dom, virCheckFlags(VIR_DOMAIN_BLOCK_RESIZE_BYTES | VIR_DOMAIN_BLOCK_RESIZE_CAPACITY | - VIR_DOMAIN_BLOCK_RESIZE_EXTEND, -1); + VIR_DOMAIN_BLOCK_RESIZE_EXTEND | + VIR_DOMAIN_BLOCK_RESIZE_PROBE_FLAGS, -1); + + /* Assume success if flag probing is requested */ + if (flags & VIR_DOMAIN_BLOCK_RESIZE_PROBE_FLAGS) + return 0; /* We prefer operating on bytes. */ if ((flags & VIR_DOMAIN_BLOCK_RESIZE_BYTES) == 0) { diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 2d8878fe7f..efb0144093 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -3901,7 +3901,12 @@ vzDomainBlockResize(virDomainPtr domain, int ret = -1; bool job = false; - virCheckFlags(VIR_DOMAIN_BLOCK_RESIZE_BYTES, -1); + virCheckFlags(VIR_DOMAIN_BLOCK_RESIZE_BYTES | + VIR_DOMAIN_BLOCK_RESIZE_PROBE_FLAGS, -1); + + /* Assume success if flag probing is requested */ + if (flags & VIR_DOMAIN_BLOCK_RESIZE_PROBE_FLAGS) + return 0; if (!(dom = vzDomObjFromDomain(domain))) goto cleanup; -- 2.53.0
On Wed, Apr 01, 2026 at 03:27:15PM +0200, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
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.
This looks like a good design to be used for all APIs with flags, but we need to change how virCheckFlags reports error and ensure that in case of probing no error is reported so callers don't have to reset last error and logs are not spammend with errors whem management applications start using this feature. Pavel
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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(); + * * Note that this call may fail if the underlying virtualization hypervisor * does not support it; this call requires privileged access to the * hypervisor. diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 551553df14..aa064ca7a9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9449,7 +9449,12 @@ qemuDomainBlockResize(virDomainPtr dom,
virCheckFlags(VIR_DOMAIN_BLOCK_RESIZE_BYTES | VIR_DOMAIN_BLOCK_RESIZE_CAPACITY | - VIR_DOMAIN_BLOCK_RESIZE_EXTEND, -1); + VIR_DOMAIN_BLOCK_RESIZE_EXTEND | + VIR_DOMAIN_BLOCK_RESIZE_PROBE_FLAGS, -1); + + /* Assume success if flag probing is requested */ + if (flags & VIR_DOMAIN_BLOCK_RESIZE_PROBE_FLAGS) + return 0;
/* We prefer operating on bytes. */ if ((flags & VIR_DOMAIN_BLOCK_RESIZE_BYTES) == 0) { diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 2d8878fe7f..efb0144093 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -3901,7 +3901,12 @@ vzDomainBlockResize(virDomainPtr domain, int ret = -1; bool job = false;
- virCheckFlags(VIR_DOMAIN_BLOCK_RESIZE_BYTES, -1); + virCheckFlags(VIR_DOMAIN_BLOCK_RESIZE_BYTES | + VIR_DOMAIN_BLOCK_RESIZE_PROBE_FLAGS, -1); + + /* Assume success if flag probing is requested */ + if (flags & VIR_DOMAIN_BLOCK_RESIZE_PROBE_FLAGS) + return 0;
if (!(dom = vzDomObjFromDomain(domain))) goto cleanup; -- 2.53.0
On Wed, Apr 01, 2026 at 03:27:15PM +0200, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
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 <pkrempa@redhat.com> --- 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. 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. 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 ?
+ * * Note that this call may fail if the underlying virtualization hypervisor * does not support it; this call requires privileged access to the * hypervisor. diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 551553df14..aa064ca7a9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9449,7 +9449,12 @@ qemuDomainBlockResize(virDomainPtr dom,
virCheckFlags(VIR_DOMAIN_BLOCK_RESIZE_BYTES | VIR_DOMAIN_BLOCK_RESIZE_CAPACITY | - VIR_DOMAIN_BLOCK_RESIZE_EXTEND, -1); + VIR_DOMAIN_BLOCK_RESIZE_EXTEND | + VIR_DOMAIN_BLOCK_RESIZE_PROBE_FLAGS, -1); + + /* Assume success if flag probing is requested */ + if (flags & VIR_DOMAIN_BLOCK_RESIZE_PROBE_FLAGS) + return 0;
/* We prefer operating on bytes. */ if ((flags & VIR_DOMAIN_BLOCK_RESIZE_BYTES) == 0) { diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 2d8878fe7f..efb0144093 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -3901,7 +3901,12 @@ vzDomainBlockResize(virDomainPtr domain, int ret = -1; bool job = false;
- virCheckFlags(VIR_DOMAIN_BLOCK_RESIZE_BYTES, -1); + virCheckFlags(VIR_DOMAIN_BLOCK_RESIZE_BYTES | + VIR_DOMAIN_BLOCK_RESIZE_PROBE_FLAGS, -1); + + /* Assume success if flag probing is requested */ + if (flags & VIR_DOMAIN_BLOCK_RESIZE_PROBE_FLAGS) + return 0;
if (!(dom = vzDomObjFromDomain(domain))) goto cleanup; -- 2.53.0
With regards, Daniel -- |: https://berrange.com ~~ https://hachyderm.io/@berrange :| |: https://libvirt.org ~~ https://entangle-photo.org :| |: https://pixelfed.art/berrange ~~ https://fstop138.berrange.com :|
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 <pkrempa@redhat.com>
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 <pkrempa@redhat.com> --- 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.
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 <pkrempa@redhat.com>
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 <pkrempa@redhat.com> --- 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. With regards, Daniel -- |: https://berrange.com ~~ https://hachyderm.io/@berrange :| |: https://libvirt.org ~~ https://entangle-photo.org :| |: https://pixelfed.art/berrange ~~ https://fstop138.berrange.com :|
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 <pkrempa@redhat.com>
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 <pkrempa@redhat.com> --- 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.
On Mon, Jun 22, 2026 at 07:24:10PM +0200, Peter Krempa wrote:
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 <pkrempa@redhat.com>
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 <pkrempa@redhat.com> --- 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.
Yes, it only helps probing for flags added from this point onwards, not probing our pre-existing set. IOW, it would solve the immediate problem for block resize but not a broader set. With regards, Daniel -- |: https://berrange.com ~~ https://hachyderm.io/@berrange :| |: https://libvirt.org ~~ https://entangle-photo.org :| |: https://pixelfed.art/berrange ~~ https://fstop138.berrange.com :|
From: Peter Krempa <pkrempa@redhat.com> Add users possibility to discover presence of arguments safely. The new global argument '--probe-arguments' which runs the argument parser but doesn't run the actual function and returning success if it gets to the point of actually trying to runt he parser. This will allow users to probe for new arguments (which were added after this patch: - virsh prior to this patch: $ virsh --probe-arguments blockresize cd vda 1 --extend ; echo $? error: unsupported option '--probe-arguments'. See --help. 1 - virsh with this patch but checking for something that doesn't yet exist: $ virsh --probe-arguments blockresize cd vda 1 --allow-shrink ; echo $? error: command 'blockresize' doesn't support option --allow-shrink 1 - virsh with this patch with the argument present: $ virsh --probe-arguments blockresize cd vda 1 --allow-shrink ; echo $? 0 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/manpages/virsh.rst | 17 ++++++++++++++++- tools/virsh.c | 4 ++++ tools/virt-admin.c | 9 +++++++++ tools/vsh.c | 3 +++ tools/vsh.h | 1 + 5 files changed, 33 insertions(+), 1 deletion(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 80b0ea14a8..9ede859aaf 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -161,7 +161,22 @@ Ignore all other arguments, and prints the version of the libvirt library virsh is coming from and which options and driver are compiled in. - +- ``--probe-arguments`` + +Parse all incomming commands and their arguments. Return without actually +executing the command. This gives the caller the possibility to probe if given +command or argument exists in this version of virsh without executing the +command. Mandatory arguments need to be provided albeit with dummy values if +checking existence of optional features. +(Introduced in libvirt-12.3.0) + +Example:: + + if virsh --probe-arguments blockresize --domain dummy --path dummy --size 1 --allow-shrink 2>/dev/null; then + echo "supported" + else + echo "unsupported" + fi NOTES ===== diff --git a/tools/virsh.c b/tools/virsh.c index fdce3220b3..57e2b9c1a9 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -646,6 +646,7 @@ virshParseArgv(vshControl *ctl, int argc, char **argv) { "timing", no_argument, NULL, 't' }, { "no-pkttyagent", no_argument, NULL, 0 }, { "version", optional_argument, NULL, 'v' }, + { "probe-arguments", no_argument, NULL, 0 }, { NULL, 0, NULL, 0 }, }; @@ -746,6 +747,9 @@ virshParseArgv(vshControl *ctl, int argc, char **argv) if (STREQ(opt[longindex].name, "no-pkttyagent")) { ctl->no_pkttyagent = true; break; + } else if (STREQ(opt[longindex].name, "probe-arguments")) { + ctl->probe_arguments = true; + break; } else { vshError(ctl, "%s", _("unknown option")); exit(EXIT_FAILURE); diff --git a/tools/virt-admin.c b/tools/virt-admin.c index ac8f3202b6..ff6f3856aa 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -1292,6 +1292,7 @@ vshAdmParseArgv(vshControl *ctl, int argc, char **argv) { "log", required_argument, NULL, 'l' }, { "quiet", no_argument, NULL, 'q' }, { "version", optional_argument, NULL, 'v' }, + { "probe-arguments", no_argument, NULL, 0 }, { NULL, 0, NULL, 0 }, }; @@ -1337,6 +1338,14 @@ vshAdmParseArgv(vshControl *ctl, int argc, char **argv) case 'V': vshAdmShowVersion(ctl); exit(EXIT_SUCCESS); + case 0: + if (STREQ(opt[longindex].name, "probe-arguments")) { + ctl->probe_arguments = true; + break; + } else { + vshError(ctl, "%s", _("unknown option")); + exit(EXIT_FAILURE); + } case ':': for (i = 0; opt[i].name != NULL; i++) { if (opt[i].val == optopt) diff --git a/tools/vsh.c b/tools/vsh.c index 74016c0043..e85d8b5001 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -1366,6 +1366,9 @@ vshCommandRun(vshControl *ctl, const vshClientHooks *hooks = ctl->hooks; int ret = EXIT_SUCCESS; + if (ctl->probe_arguments) + return EXIT_SUCCESS; + while (cmd) { gint64 before, after; bool enable_timing = ctl->timing; diff --git a/tools/vsh.h b/tools/vsh.h index bd2494e899..18fa7ad594 100644 --- a/tools/vsh.h +++ b/tools/vsh.h @@ -203,6 +203,7 @@ struct _vshControl { bool timing; /* print timing info? */ bool no_pkttyagent; /* suppress registration of pkttyagent? */ int debug; /* print debug messages? */ + bool probe_arguments; /* process arguments but don't run command */ char *logfile; /* log file name */ int log_fd; /* log file descriptor */ char *historydir; /* readline history directory name */ -- 2.53.0
On Wed, Apr 01, 2026 at 03:27:16PM +0200, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
Add users possibility to discover presence of arguments safely.
The new global argument '--probe-arguments' which runs the argument parser but doesn't run the actual function and returning success if it gets to the point of actually trying to runt he parser.
s/runt he/run the/
This will allow users to probe for new arguments (which were added after this patch:
- virsh prior to this patch:
$ virsh --probe-arguments blockresize cd vda 1 --extend ; echo $? error: unsupported option '--probe-arguments'. See --help. 1
- virsh with this patch but checking for something that doesn't yet exist:
$ virsh --probe-arguments blockresize cd vda 1 --allow-shrink ; echo $? error: command 'blockresize' doesn't support option --allow-shrink 1
Same here as for the API, do we want to print error in this case? I would say this will be mainly used in scripts and it would help callers as they would most likely discard the error message in case the script is probing virsh if something is supported or not. I wanted to suggest that the script can run `virsh help | grep --probe-arguments` to figure out if it should even try using it but at the same time it can run `virsh CMD --help | grep --some-argument` so now I'm not sure if we even need this in virsh. Pavel
- virsh with this patch with the argument present:
$ virsh --probe-arguments blockresize cd vda 1 --allow-shrink ; echo $? 0
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/manpages/virsh.rst | 17 ++++++++++++++++- tools/virsh.c | 4 ++++ tools/virt-admin.c | 9 +++++++++ tools/vsh.c | 3 +++ tools/vsh.h | 1 + 5 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 80b0ea14a8..9ede859aaf 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -161,7 +161,22 @@ Ignore all other arguments, and prints the version of the libvirt library virsh is coming from and which options and driver are compiled in.
- +- ``--probe-arguments`` + +Parse all incomming commands and their arguments. Return without actually +executing the command. This gives the caller the possibility to probe if given +command or argument exists in this version of virsh without executing the +command. Mandatory arguments need to be provided albeit with dummy values if +checking existence of optional features. +(Introduced in libvirt-12.3.0) + +Example:: + + if virsh --probe-arguments blockresize --domain dummy --path dummy --size 1 --allow-shrink 2>/dev/null; then + echo "supported" + else + echo "unsupported" + fi
NOTES ===== diff --git a/tools/virsh.c b/tools/virsh.c index fdce3220b3..57e2b9c1a9 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -646,6 +646,7 @@ virshParseArgv(vshControl *ctl, int argc, char **argv) { "timing", no_argument, NULL, 't' }, { "no-pkttyagent", no_argument, NULL, 0 }, { "version", optional_argument, NULL, 'v' }, + { "probe-arguments", no_argument, NULL, 0 }, { NULL, 0, NULL, 0 }, };
@@ -746,6 +747,9 @@ virshParseArgv(vshControl *ctl, int argc, char **argv) if (STREQ(opt[longindex].name, "no-pkttyagent")) { ctl->no_pkttyagent = true; break; + } else if (STREQ(opt[longindex].name, "probe-arguments")) { + ctl->probe_arguments = true; + break; } else { vshError(ctl, "%s", _("unknown option")); exit(EXIT_FAILURE); diff --git a/tools/virt-admin.c b/tools/virt-admin.c index ac8f3202b6..ff6f3856aa 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -1292,6 +1292,7 @@ vshAdmParseArgv(vshControl *ctl, int argc, char **argv) { "log", required_argument, NULL, 'l' }, { "quiet", no_argument, NULL, 'q' }, { "version", optional_argument, NULL, 'v' }, + { "probe-arguments", no_argument, NULL, 0 }, { NULL, 0, NULL, 0 }, };
@@ -1337,6 +1338,14 @@ vshAdmParseArgv(vshControl *ctl, int argc, char **argv) case 'V': vshAdmShowVersion(ctl); exit(EXIT_SUCCESS); + case 0: + if (STREQ(opt[longindex].name, "probe-arguments")) { + ctl->probe_arguments = true; + break; + } else { + vshError(ctl, "%s", _("unknown option")); + exit(EXIT_FAILURE); + } case ':': for (i = 0; opt[i].name != NULL; i++) { if (opt[i].val == optopt) diff --git a/tools/vsh.c b/tools/vsh.c index 74016c0043..e85d8b5001 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -1366,6 +1366,9 @@ vshCommandRun(vshControl *ctl, const vshClientHooks *hooks = ctl->hooks; int ret = EXIT_SUCCESS;
+ if (ctl->probe_arguments) + return EXIT_SUCCESS; + while (cmd) { gint64 before, after; bool enable_timing = ctl->timing; diff --git a/tools/vsh.h b/tools/vsh.h index bd2494e899..18fa7ad594 100644 --- a/tools/vsh.h +++ b/tools/vsh.h @@ -203,6 +203,7 @@ struct _vshControl { bool timing; /* print timing info? */ bool no_pkttyagent; /* suppress registration of pkttyagent? */ int debug; /* print debug messages? */ + bool probe_arguments; /* process arguments but don't run command */ char *logfile; /* log file name */ int log_fd; /* log file descriptor */ char *historydir; /* readline history directory name */ -- 2.53.0
From: Peter Krempa <pkrempa@redhat.com> Use the new VIR_DOMAIN_BLOCK_RESIZE_PROBE_FLAGS flag to probe for VIR_DOMAIN_BLOCK_RESIZE_EXTEND and use it if available. This will give users the same protection as with --extend in the default case. IMPORTANT: This patch changes behaviour for users who want to shrink their VM's block device knowingly. Such users, when using a newer daemon will be required to pass --allow-shrink now. I've contemplated either renaming the whole command to preserve functionality for the existing one and deprecating 'virsh blockresize' or adding just the '--extend', but unfortunately neither of those in the end looked appealing to me. As shrinking filesystems is much more invloved and much less common the benefit of preventing accidental data loss should outweigh the breakage from the change. To aleviate issues with scripts virsh now also provides the '--probe-arguments' option to allow safe probe if virsh supports this new argument. Closes: https://gitlab.com/libvirt/libvirt/-/work_items/789 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/manpages/virsh.rst | 11 ++++++++++- tools/virsh-domain.c | 16 ++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 9ede859aaf..5138591595 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -1657,7 +1657,7 @@ blockresize :: - blockresize domain path ([size] | [--capacity]) [--extend] + blockresize domain path ([size] | [--capacity]) [--extend] [--allow-shrink] Resize a block device of domain while the domain is running, *path* specifies the absolute path of the block device; it corresponds @@ -1670,6 +1670,15 @@ smaller than the old size, thus prevents (accidental) shrinking of the block device which may lead to data loss. Users are encouraged to always use this flag unless communicating with an older hypervisor. +As of libvirt 12.3.0 the 'virsh blockresize' command now probes whether the +hypervisor supports enforcing that the device is not shrunk accidentally and +if it is supported this feature will be used as a precaution from accidental +data loss even at the cost of change of behaviour. With older hypervisors +which do not support the feature the protection will not be avaliable. +Users wanting to shrink the block device must use the *--allow-shrink* +flag. In scripts it's possible to use the *--probe-arguments* flag to determine +if the flag is supported. + For image formats without metadata (raw) stored inside fixed-size storage (e.g. block devices) the --capacity flag can be used to resize the device to the full size of the backing device. diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 08a1ce3953..95ce4dc845 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -3343,6 +3343,10 @@ static const vshCmdOptDef opts_blockresize[] = { .type = VSH_OT_BOOL, .help = N_("ensure that the new size is larger than actual capacity (prevent shrink)") }, + {.name = "allow-shrink", + .type = VSH_OT_BOOL, + .help = N_("disable size checks so that device can be shrunk") + }, {.name = NULL} }; @@ -3378,6 +3382,18 @@ cmdBlockresize(vshControl *ctl, const vshCmd *cmd) if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; + /* probe for support of VIR_DOMAIN_BLOCK_RESIZE_EXTEND and use it if it's + * supported to prevent mishaps even at the cost of usability change */ + if (!vshCommandOptBool(cmd, "allow-shrink")) { + if (virDomainBlockResize(dom, "", 0, + VIR_DOMAIN_BLOCK_RESIZE_EXTEND | + VIR_DOMAIN_BLOCK_RESIZE_PROBE_FLAGS) == 0) { + flags |= VIR_DOMAIN_BLOCK_RESIZE_EXTEND; + } else { + vshResetLibvirtError(); + } + } + if (virDomainBlockResize(dom, path, size, flags) < 0) { vshError(ctl, _("Failed to resize block device '%1$s'"), path); return false; -- 2.53.0
participants (3)
-
Daniel P. Berrangé -
Pavel Hrdina -
Peter Krempa