Re: [PATCH 3/4] qemu: hot-plug of watchdog

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 09/12/2017 04:23 PM, John Ferlan wrote:
> 
> 
> On 09/05/2017 07:45 AM, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1447169
>>
>> Once again, since domain can have at most one watchdog it
>> simplifies things a bit. However, since we must be able to set
>> the watchdog action as well, new monitor command needs to be
>> used.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
>> ---
>>  src/qemu/qemu_alias.c                              | 15 ++++-
>>  src/qemu/qemu_alias.h                              |  2 +
>>  src/qemu/qemu_command.c                            |  2 +-
>>  src/qemu/qemu_command.h                            |  3 +
>>  src/qemu/qemu_driver.c                             | 10 +++-
>>  src/qemu/qemu_hotplug.c                            | 64 ++++++++++++++++++++++
>>  src/qemu/qemu_hotplug.h                            |  3 +
>>  src/qemu/qemu_monitor.c                            | 12 ++++
>>  src/qemu/qemu_monitor.h                            |  2 +
>>  src/qemu/qemu_monitor_json.c                       | 28 ++++++++++
>>  src/qemu/qemu_monitor_json.h                       |  3 +
>>  tests/qemuhotplugtest.c                            |  9 ++-
>>  .../qemuhotplug-watchdog.xml                       |  1 +
>>  .../qemuhotplug-base-live+watchdog.xml             | 55 +++++++++++++++++++
>>  14 files changed, 205 insertions(+), 4 deletions(-)
>>  create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-watchdog.xml
>>  create mode 100644 tests/qemuhotplugtestdomains/qemuhotplug-base-live+watchdog.xml
>>
>> diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c
>> index 914b2b94d..63f69e80f 100644
>> --- a/src/qemu/qemu_alias.c
>> +++ b/src/qemu/qemu_alias.c
>> @@ -405,6 +405,19 @@ qemuAssignDeviceShmemAlias(virDomainDefPtr def,
>>  }
>>  
>>  
>> +int
>> +qemuAssignDeviceWatchdogAlias(virDomainWatchdogDefPtr watchdog)
>> +{
>> +    const int idx = 0;
>> +
>> +    /* Currently, there's just one watchdog per domain */
>> +
>> +    if (virAsprintf(&watchdog->info.alias, "watchdog%d", idx) < 0)
> 
> There's really no need for @idx... s/idx/0/ works fine.
> 
> Although if multiple watchdogs were ever to be implemented, then using
> the watchdog->action could perhaps be a mechanism to have a watchdog for
> each type of action. Of course that breaks down for an existing domain
> on an old version where a new libvirtd gets added and hot unplug is
> attempted (since the running domain would always have watchdog0). Oh
> well.... It was a thought.
> 
>> +        return -1;
>> +    return 0;
>> +}
>> +
>> +
>>  int
>>  qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps)
>>  {
>> @@ -482,7 +495,7 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps)
>>              return -1;
>>      }
>>      if (def->watchdog) {
>> -        if (virAsprintf(&def->watchdog->info.alias, "watchdog%d", 0) < 0)
>> +        if (qemuAssignDeviceWatchdogAlias(def->watchdog) < 0)
>>              return -1;
>>      }
>>      if (def->memballoon) {
>> diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h
>> index 300fd4de5..652ffea0c 100644
>> --- a/src/qemu/qemu_alias.h
>> +++ b/src/qemu/qemu_alias.h
>> @@ -65,6 +65,8 @@ int qemuAssignDeviceShmemAlias(virDomainDefPtr def,
>>                                 virDomainShmemDefPtr shmem,
>>                                 int idx);
>>  
>> +int qemuAssignDeviceWatchdogAlias(virDomainWatchdogDefPtr watchdog);
>> +
>>  int qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps);
>>  
>>  int qemuDomainDeviceAliasIndex(const virDomainDeviceInfo *info,
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 9a27987d4..eb9df044b 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -3921,7 +3921,7 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
>>  }
>>  
>>  
>> -static char *
>> +char *
>>  qemuBuildWatchdogDevStr(const virDomainDef *def,
>>                          virDomainWatchdogDefPtr dev,
>>                          virQEMUCapsPtr qemuCaps)
>> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
>> index 6fbfb3e5f..4b504f685 100644
>> --- a/src/qemu/qemu_command.h
>> +++ b/src/qemu/qemu_command.h
>> @@ -206,5 +206,8 @@ char *qemuBuildShmemDevStr(virDomainDefPtr def,
>>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
>>  
> 
> Only one space between...  Although, I see commit id '06524fd5' a few
> extra blank spaces. We can use this opportunity to clean them up
> though...  Just be sure there's an extra blank line before the #endif
> though (helps w/ readability at least for me).
> 
>>  
>> +char * qemuBuildWatchdogDevStr(const virDomainDef *def,
>> +                               virDomainWatchdogDefPtr dev,
>> +                               virQEMUCapsPtr qemuCaps);
> 
> You have an extra space between * and qemu... Of course affects spacing
> of subsequent lines too.  Could always go with the same format as .c:
> 
> char *
> qemuBuildWatchdogDevStr(const virDomainDef *def,
>                         virDomainWatchdogDefPtr dev,
>                         virQEMUCapsPtr qemuCaps);
> 
> 
> 
> My only other note is I really wish qemu_command.h was ordered the same
> way as qemu_command.c, but that's a different problem. IOW: Why add to
> the end, but that's one of those type-A type problems.
> 
>>  
>>  #endif /* __QEMU_COMMAND_H__*/
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 85b4be360..626148dba 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -7593,12 +7593,20 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm,
>>          }
>>          break;
>>  
>> +    case VIR_DOMAIN_DEVICE_WATCHDOG:
>> +        ret = qemuDomainAttachWatchdog(driver, vm,
>> +                                       dev->data.watchdog);
>> +        if (!ret) {
> 
> I wish all the callers used "if (ret == 0)", but I won't complain ;-)
> 
>> +            alias = dev->data.watchdog->info.alias;
>> +            dev->data.watchdog = NULL;
>> +        }
>> +        break;
>> +
>>      case VIR_DOMAIN_DEVICE_NONE:
>>      case VIR_DOMAIN_DEVICE_FS:
>>      case VIR_DOMAIN_DEVICE_INPUT:
>>      case VIR_DOMAIN_DEVICE_SOUND:
>>      case VIR_DOMAIN_DEVICE_VIDEO:
>> -    case VIR_DOMAIN_DEVICE_WATCHDOG:
>>      case VIR_DOMAIN_DEVICE_GRAPHICS:
>>      case VIR_DOMAIN_DEVICE_HUB:
>>      case VIR_DOMAIN_DEVICE_SMARTCARD:
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index b365078ec..989146eb9 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -2866,6 +2866,70 @@ qemuDomainAttachShmemDevice(virQEMUDriverPtr driver,
>>  }
>>  
>>  
>> +int
>> +qemuDomainAttachWatchdog(virQEMUDriverPtr driver,
>> +                         virDomainObjPtr vm,
>> +                         virDomainWatchdogDefPtr watchdog)
>> +{
>> +    int ret = -1;
>> +    qemuDomainObjPrivatePtr priv = vm->privateData;
>> +    virDomainDeviceDef dev = { VIR_DOMAIN_DEVICE_WATCHDOG, { .watchdog = watchdog } };
>> +    virDomainWatchdogAction actualAction = watchdog->action;
>> +    char *watchdogstr = NULL;
>> +    bool releaseAddress = false;
>> +    int rv;
>> +
>> +    if (vm->def->watchdog) {
>> +        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
>> +                       _("domain already has a watchdog"));
>> +        return -1;
>> +    }
>> +
>> +    if (qemuAssignDeviceWatchdogAlias(watchdog) < 0)
>> +        return -1;
>> +
>> +    if (!(watchdogstr = qemuBuildWatchdogDevStr(vm->def, watchdog, priv->qemuCaps)))
>> +        return -1;
>> +
>> +    if (watchdog->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
>> +        if (qemuDomainEnsurePCIAddress(vm, &dev, driver) < 0)
>> +            goto cleanup;
>> +        releaseAddress = true;
>> +    }
>> +
>> +    /* QEMU doesn't have a 'dump' action; we tell qemu to 'pause', then
>> +       libvirt listens for the watchdog event, and we perform the dump
>> +       ourselves. so convert 'dump' to 'pause' for the qemu cli */
>> +    if (actualAction == VIR_DOMAIN_WATCHDOG_ACTION_DUMP)
>> +        actualAction = VIR_DOMAIN_WATCHDOG_ACTION_PAUSE;
>> +
> 
> I see the command line building has similar code, but also has a check
> that actualAction actually *TypeToString properly.

That is really useless. How can we successfully parse action but fail to
format it back?

> 
>> +    qemuDomainObjEnterMonitor(driver, vm);
>> +
>> +    rv = qemuMonitorSetWatchdogAction(priv->mon, actualAction);
> 
> Any reason to not pass a const char *action here instead?  Allowing this
> code to check validity rather than failing lower in the stack.

We can fail lower in the stack but not because of bad conversion from
enum to string. But okay, I can change this to const char *action. We
use both approaches, so one can argue either way.

> 
> I also think perhaps there could be a "common" API that both
> qemu_command and qemu_hotplug could call in order to return the action
> string so that you remove the duplicated code.

Well the only duplicated part is the check where action is overwritten.
I don't think it's that bad.

> 
> My only other comment is that it seems a bit strange to adjust the
> action of something that isn't yet added to the domain. That is the
> ordering of altering the action, then adding the watchdog doesn't seem
> right. The command line has the "-device" first, then the "action"
> second, that is I see from the *.args file:
> 
> ...
> -device ib700,id=watchdog0 \
> -watchdog-action pause \
> ...
> 
> which if qemu is reading the order would seem to imply the device is
> added, then the action is applied.

That's just coincidence IMO. The true reason was that I don't have to
try unplug the watchdog if setting action fails.

> 
>> +
>> +    if (rv >= 0)
> 
> This only ever returns -1 or 0, right?

Yep. But in general, 0 or a positive value are considered as success.

> 
>> +        rv = qemuMonitorAddDevice(priv->mon, watchdogstr);
>> +
>> +    if (qemuDomainObjExitMonitor(driver, vm) < 0) {
>> +        releaseAddress = false;
>> +        goto cleanup;
>> +    }
>> +
>> +    if (rv < 0)
>> +        goto cleanup;
> 
> Why no virDomainAuditWatchdog type code?

Ah, good point. There's no virDomainAuditWatchdog(), but there should
be! I'll post another version where I'll introduce the audit function as
the first patch.

> 
>> +
>> +    releaseAddress = false;
>> +    vm->def->watchdog = watchdog;
>> +    ret = 0;
>> +
>> + cleanup:
>> +    if (releaseAddress)
>> +        qemuDomainReleaseDeviceAddress(vm, &watchdog->info, NULL);
>> +    VIR_FREE(watchdogstr);
>> +    return ret;
>> +}
>> +
>> +
>>  static int
>>  qemuDomainChangeNetBridge(virDomainObjPtr vm,
>>                            virDomainNetDefPtr olddev,
>> diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
>> index 985c6733f..a9dfd8f7a 100644
>> --- a/src/qemu/qemu_hotplug.h
>> +++ b/src/qemu/qemu_hotplug.h
>> @@ -80,6 +80,9 @@ int qemuDomainAttachHostDevice(virConnectPtr conn,
>>  int qemuDomainAttachShmemDevice(virQEMUDriverPtr driver,
>>                                  virDomainObjPtr vm,
>>                                  virDomainShmemDefPtr shmem);
>> +int qemuDomainAttachWatchdog(virQEMUDriverPtr driver,
>> +                             virDomainObjPtr vm,
>> +                             virDomainWatchdogDefPtr watchdog);
>>  int qemuDomainFindGraphicsIndex(virDomainDefPtr def,
>>                                  virDomainGraphicsDefPtr dev);
>>  int qemuDomainAttachMemory(virQEMUDriverPtr driver,
>> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
>> index 19082d8bf..5d3f9df47 100644
>> --- a/src/qemu/qemu_monitor.c
>> +++ b/src/qemu/qemu_monitor.c
>> @@ -4343,3 +4343,15 @@ qemuMonitorEventPanicInfoFree(qemuMonitorEventPanicInfoPtr info)
>>  
>>      VIR_FREE(info);
>>  }
>> +
>> +
>> +int
>> +qemuMonitorSetWatchdogAction(qemuMonitorPtr mon,
>> +                             virDomainWatchdogAction watchdogAction)
>> +{
>> +    VIR_DEBUG("watchdogAction=%d", watchdogAction);
>> +
>> +    QEMU_CHECK_MONITOR_JSON(mon);
>> +
>> +    return qemuMonitorJSONSetWatchdogAction(mon, watchdogAction);
>> +}
>> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
>> index 9805a3390..b8c9409e5 100644
>> --- a/src/qemu/qemu_monitor.h
>> +++ b/src/qemu/qemu_monitor.h
>> @@ -1132,4 +1132,6 @@ int qemuMonitorSetBlockThreshold(qemuMonitorPtr mon,
>>  
>>  virJSONValuePtr qemuMonitorQueryNamedBlockNodes(qemuMonitorPtr mon);
>>  
>> +int qemuMonitorSetWatchdogAction(qemuMonitorPtr mon,
>> +                                 virDomainWatchdogAction watchdogAction);
> 
> Prefer to see that extra blank line here...
> 
>>  #endif /* QEMU_MONITOR_H */
>> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
>> index df5fb7c8f..02ba701e6 100644
>> --- a/src/qemu/qemu_monitor_json.c
>> +++ b/src/qemu/qemu_monitor_json.c
>> @@ -7679,3 +7679,31 @@ qemuMonitorJSONQueryNamedBlockNodes(qemuMonitorPtr mon)
>>  
>>      return ret;
>>  }
>> +
>> +
>> +int
>> +qemuMonitorJSONSetWatchdogAction(qemuMonitorPtr mon,
>> +                                 virDomainWatchdogAction watchdogAction)
>> +{
>> +    virJSONValuePtr cmd;
>> +    virJSONValuePtr reply = NULL;
>> +    const char *action = virDomainWatchdogActionTypeToString(watchdogAction);
>> +    int ret = -1;
> 
> Command line building checks !action....
> 
> I would think this function should take a "const char *action" and the
> caller handle the error though.
> 
>> +
>> +    if (!(cmd = qemuMonitorJSONMakeCommand("watchdog-set-action",
>> +                                           "s:action", action,
>> +                                           NULL)))
>> +        return -1;
>> +    if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
>> +        goto cleanup;
>> +
>> +    if (qemuMonitorJSONCheckError(cmd, reply) < 0)
>> +        goto cleanup;
>> +
>> +    ret = 0;
>> +
>> + cleanup:
>> +    virJSONValueFree(cmd);
>> +    virJSONValueFree(reply);
>> +    return ret;
>> +}
>> diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
>> index 7462967b5..a7a43d7b8 100644
>> --- a/src/qemu/qemu_monitor_json.h
>> +++ b/src/qemu/qemu_monitor_json.h
>> @@ -524,4 +524,7 @@ int qemuMonitorJSONSetBlockThreshold(qemuMonitorPtr mon,
>>  virJSONValuePtr qemuMonitorJSONQueryNamedBlockNodes(qemuMonitorPtr mon)
>>      ATTRIBUTE_NONNULL(1);
>>  
>> +int qemuMonitorJSONSetWatchdogAction(qemuMonitorPtr mon,
>> +                                     virDomainWatchdogAction watchdogAction)
>> +    ATTRIBUTE_NONNULL(1);
> 
> Likewise with the extra blank line.
> 
>>  #endif /* QEMU_MONITOR_JSON_H */
>> diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
>> index 23a498617..b02ae8034 100644
>> --- a/tests/qemuhotplugtest.c
>> +++ b/tests/qemuhotplugtest.c
>> @@ -127,6 +127,9 @@ testQemuHotplugAttach(virDomainObjPtr vm,
>>      case VIR_DOMAIN_DEVICE_SHMEM:
>>          ret = qemuDomainAttachShmemDevice(&driver, vm, dev->data.shmem);
>>          break;
>> +    case VIR_DOMAIN_DEVICE_WATCHDOG:
>> +        ret = qemuDomainAttachWatchdog(&driver, vm, dev->data.watchdog);
>> +        break;
>>      default:
>>          VIR_TEST_VERBOSE("device type '%s' cannot be attached\n",
>>                  virDomainDeviceTypeToString(dev->type));
>> @@ -811,10 +814,14 @@ mymain(void)
>>                     "device_del", QMP_OK,
>>                     "object-del", QMP_OK);
>>      DO_TEST_ATTACH("base-live+disk-scsi-wwn",
>> -                   "disk-scsi-duplicate-wwn", false, true,
>> +                   "disk-scsi-duplicate-wwn", false, false,
> 
> This would seem to be unrelated... and should be separate.

Not really. We don't want to keep the device (=disk) in the domain
because then the test case I'm adding would need to look different (with
that disk in, which would be insane).

Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux