On 7/18/23 17:47, Peter Krempa wrote:
> On Tue, Jul 18, 2023 at 17:27:33 +0200, Michal Privoznik wrote:
>> When virsh connects to a non-hypervisor daemon directly (e.g.
>> "nodedev:///system") and user executes 'version' they are met
>> with an error message. This is because cmdVersion() calls
>> virConnectGetVersion() which fails, hence the error.
>>
>> The reason for virConnectGetVersion() fail is simple - it's
>> documented as:
>>
>> Get the version level of the Hypervisor running.
>>
>> Well, there's no hypervisor in non-hypervisor daemons and thus it
>> doesn't make sense to provide an implementation in each driver's
>> virConnectDriver.hypervisorDriver table (just like we do for
>> other APIs, e.g. nodeConnectIsSecure()).
>>
>> Given all of this, just make cmdVersion() deal with the error in
>> a non-fatal fashion.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
>> ---
>> tools/virsh-host.c | 26 ++++++++++++--------------
>> 1 file changed, 12 insertions(+), 14 deletions(-)
>>
>> diff --git a/tools/virsh-host.c b/tools/virsh-host.c
>> index 0bda327cae..35e6a2eb98 100644
>> --- a/tools/virsh-host.c
>> +++ b/tools/virsh-host.c
>> @@ -1447,21 +1447,19 @@ cmdVersion(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED)
>> vshPrint(ctl, _("Using API: %1$s %2$d.%3$d.%4$d\n"), hvType,
>> major, minor, rel);
>>
>> - if (virConnectGetVersion(priv->conn, &hvVersion) < 0) {
>> - vshError(ctl, "%s", _("failed to get the hypervisor version"));
>> - return false;
>> - }
>> - if (hvVersion == 0) {
>> - vshPrint(ctl,
>> - _("Cannot extract running %1$s hypervisor version\n"), hvType);
>> - } else {
>> - major = hvVersion / 1000000;
>> - hvVersion %= 1000000;
>> - minor = hvVersion / 1000;
>> - rel = hvVersion % 1000;
>> + if (virConnectGetVersion(priv->conn, &hvVersion) >= 0) {
>> + if (hvVersion == 0) {
>> + vshPrint(ctl,
>> + _("Cannot extract running %1$s hypervisor version\n"), hvType);
>> + } else {
>> + major = hvVersion / 1000000;
>> + hvVersion %= 1000000;
>> + minor = hvVersion / 1000;
>> + rel = hvVersion % 1000;
>>
>> - vshPrint(ctl, _("Running hypervisor: %1$s %2$d.%3$d.%4$d\n"),
>> - hvType, major, minor, rel);
>> + vshPrint(ctl, _("Running hypervisor: %1$s %2$d.%3$d.%4$d\n"),
>> + hvType, major, minor, rel);
>> + }
>> }
>
> Ideally you'd also add an else branch with 'vshResetLibvirtError(); but
> the other call to virConnectGetLibVersion() doesn't do that so ...
> whatever ... I guess :)
Oh, you're right. In fact we should also ignore VIR_ERR_NO_SUPPORT.
Other errors might be actually a good reason to return early. Consider
this squashed in:
diff --git i/tools/virsh-host.c w/tools/virsh-host.c
index 35e6a2eb98..ad440d5123 100644
--- i/tools/virsh-host.c
+++ w/tools/virsh-host.c
@@ -1447,7 +1447,14 @@ cmdVersion(vshControl *ctl, const vshCmd *cmd G_GNUC_UNUSED)
vshPrint(ctl, _("Using API: %1$s %2$d.%3$d.%4$d\n"), hvType,
major, minor, rel);
- if (virConnectGetVersion(priv->conn, &hvVersion) >= 0) {
+ if (virConnectGetVersion(priv->conn, &hvVersion) < 0) {
+ if (last_error->code == VIR_ERR_NO_SUPPORT) {
+ vshResetLibvirtError();
+ } else {
+ vshError(ctl, "%s", _("failed to get the hypervisor version"));
+ return false;
+ }
+ } else {
>
>>
>> if (vshCommandOptBool(cmd, "daemon")) {
>> --
>> 2.41.0
>>
>
> Reviewed-by: Peter Krempa <pkrempa@xxxxxxxxxx>
>
Thanks,
Michal
