tools: virsh: metadata: do not report error on missing metadata

[libvirt PATCH] tools: virsh: metadata: do not report error on missing metadata

Posted by Ján Tomko 10 months ago

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

Re: [libvirt PATCH] tools: virsh: metadata: do not report error on missing metadata

Posted by Michal Prívozník via Devel 8 months, 2 weeks ago

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

Re: [libvirt PATCH] tools: virsh: metadata: do not report error on missing metadata

Posted by Michal Prívozník 10 months ago

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

Re: [libvirt PATCH] tools: virsh: metadata: do not report error on missing metadata

Posted by Peter Krempa via Devel 8 months, 3 weeks ago

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.

Re: [libvirt PATCH] tools: virsh: metadata: do not report error on missing metadata

Posted by Daniel P. Berrangé via Devel 8 months, 2 weeks ago

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 :|