Re: [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.