[PATCH 1/5] virsh: Make cmdVersion() work with split daemon

[Michal Prívozník]

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 at redhat.com>
>> ---
>>  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 at redhat.com>
> 

Thanks,

Michal