[libvirt PATCH] tools: virsh: metadata: do not report error on missing metadata
Similarly to `desc` and `net-desc`, return an empty string if there is no metadata to be returned. https://issues.redhat.com/browse/RHEL-27172 Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-domain.c | 10 ++++++++-- tools/virsh-network.c | 10 ++++++++-- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index f3da2f903f..e104aa909a 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8480,8 +8480,14 @@ cmdMetadata(vshControl *ctl, const vshCmd *cmd) g_autofree char *data = NULL; /* get */ if (!(data = virDomainGetMetadata(dom, VIR_DOMAIN_METADATA_ELEMENT, - uri, flags))) - return false; + uri, flags))) { + if (virGetLastErrorCode() == VIR_ERR_NO_DOMAIN_METADATA) { + virResetLastError(); + data = g_strdup(""); + } else { + return false; + } + } vshPrint(ctl, "%s\n", data); } diff --git a/tools/virsh-network.c b/tools/virsh-network.c index 6fcc7fd8ee..bcdb76ae36 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -604,8 +604,14 @@ cmdNetworkMetadata(vshControl *ctl, const vshCmd *cmd) /* get */ if (!(data = virNetworkGetMetadata(net, VIR_NETWORK_METADATA_ELEMENT, - uri, flags))) - return false; + uri, flags))) { + if (virGetLastErrorCode() == VIR_ERR_NO_NETWORK_METADATA) { + virResetLastError(); + data = g_strdup(""); + } else { + return false; + } + } vshPrint(ctl, "%s\n", data); } -- 2.48.1
On 2/20/25 23:26, Ján Tomko wrote:
Similarly to `desc` and `net-desc`, return an empty string if there is no metadata to be returned.
https://issues.redhat.com/browse/RHEL-27172
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-domain.c | 10 ++++++++-- tools/virsh-network.c | 10 ++++++++-- 2 files changed, 16 insertions(+), 4 deletions(-)
Code-wise this is okay. What I worry about is change in error behavior. I mean, if somebody checks for exit value they'll be surprised why their script stopped working. Michal
On Fri, Feb 21, 2025 at 14:36:50 +0100, Michal Prívozník wrote:
On 2/20/25 23:26, Ján Tomko wrote:
Similarly to `desc` and `net-desc`, return an empty string if there is no metadata to be returned.
https://issues.redhat.com/browse/RHEL-27172
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-domain.c | 10 ++++++++-- tools/virsh-network.c | 10 ++++++++-- 2 files changed, 16 insertions(+), 4 deletions(-)
Code-wise this is okay. What I worry about is change in error behavior. I mean, if somebody checks for exit value they'll be surprised why their script stopped working.
While I wouldn't care for changing the command for network metadata, domain metadata is likely to actually be used. An argument against current behaviour is that virsh returns failure (return code 1) on any failure. So for scripts using virsh it's actually impossible to distinguish the non-existance of the metadata from any other error: $ virsh metadata asdfasdf asdfasdfa error: failed to get domain 'asdfasdf' $ echo $? 1 $ virsh metadata cd asdfasdfa error: metadata not found: Requested metadata element is not present $ echo $? 1 The above could lead to someone attempting to parse the error string. Changing behaviour e.g. based on 'isatty()' would IMO be wrong too. Given that you need to actually pass a full XML element as metadata (as in; empty string is not permissible): $ virsh metadata cd test.asdf <a/> I'm borderlinefor changing the behaviour because it will actually allow scripts seeing that there's no metadata rather than a failure which is not really distinguishable from other problems.
On Tue, Apr 01, 2025 at 02:49:11PM +0200, Peter Krempa via Devel wrote:
On Fri, Feb 21, 2025 at 14:36:50 +0100, Michal Prívozník wrote:
On 2/20/25 23:26, Ján Tomko wrote:
Similarly to `desc` and `net-desc`, return an empty string if there is no metadata to be returned.
https://issues.redhat.com/browse/RHEL-27172
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-domain.c | 10 ++++++++-- tools/virsh-network.c | 10 ++++++++-- 2 files changed, 16 insertions(+), 4 deletions(-)
Code-wise this is okay. What I worry about is change in error behavior. I mean, if somebody checks for exit value they'll be surprised why their script stopped working.
While I wouldn't care for changing the command for network metadata, domain metadata is likely to actually be used.
An argument against current behaviour is that virsh returns failure (return code 1) on any failure. So for scripts using virsh it's actually impossible to distinguish the non-existance of the metadata from any other error:
$ virsh metadata asdfasdf asdfasdfa error: failed to get domain 'asdfasdf'
$ echo $? 1 $ virsh metadata cd asdfasdfa error: metadata not found: Requested metadata element is not present
$ echo $? 1
The above could lead to someone attempting to parse the error string.
Changing behaviour e.g. based on 'isatty()' would IMO be wrong too.
Given that you need to actually pass a full XML element as metadata (as in; empty string is not permissible):
$ virsh metadata cd test.asdf <a/>
I'm borderlinefor changing the behaviour because it will actually allow scripts seeing that there's no metadata rather than a failure which is not really distinguishable from other problems.
Agreed, given that callers can distinguish "empty string" from a full (empty) XML element, I think the change is acceptable on balance. It enables a scenario that was not possible in the past, and if apps adapt to check for 'empty string', they'll be no worse off if doing this for old libvirt where everything was an error. Overall a net win. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 2/20/25 23:26, Ján Tomko wrote:
Similarly to `desc` and `net-desc`, return an empty string if there is no metadata to be returned.
https://issues.redhat.com/browse/RHEL-27172
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-domain.c | 10 ++++++++-- tools/virsh-network.c | 10 ++++++++-- 2 files changed, 16 insertions(+), 4 deletions(-)
Based on discussion: Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (4)
-
Daniel P. Berrangé -
Ján Tomko -
Michal Prívozník -
Peter Krempa