1:26 a.m.
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(a)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
4:36 p.m.
New subject: [libvirt PATCH] tools: virsh: metadata: do not report error on
missing metadata
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(a)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
3:49 p.m.
New subject: [libvirt PATCH] tools: virsh: metadata: do not report error on
missing metadata
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(a)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.
3:43 p.m.
New subject: [libvirt PATCH] tools: virsh: metadata: do not report error on
missing metadata
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(a)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 :|
5:46 p.m.
New subject: [libvirt PATCH] tools: virsh: metadata: do not report error on
missing metadata
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(a)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(a)redhat.com>
Michal
13
days inactive
54
days old
4 comments
4 participants
participants (4)
-
Daniel P. Berrangé -
Ján Tomko -
Michal Prívozník -
Peter Krempa