https://bugzilla.redhat.com/show_bug.cgi?id=1447169

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_driver.c                             |  4 +-
 src/qemu/qemu_hotplug.c                            | 61 ++++++++++++++++++++++
 src/qemu/qemu_hotplug.h                            |  3 ++
 tests/qemuhotplugtest.c                            |  7 ++-
 .../qemuhotplug-watchdog-full.xml                  |  3 ++
 5 files changed, 76 insertions(+), 2 deletions(-)
 create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-watchdog-full.xml

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 626148dba..4c636b956 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7692,12 +7692,14 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
     case VIR_DOMAIN_DEVICE_SHMEM:
         ret = qemuDomainDetachShmemDevice(driver, vm, dev->data.shmem);
         break;
+    case VIR_DOMAIN_DEVICE_WATCHDOG:
+        ret = qemuDomainDetachWatchdog(driver, vm, dev->data.watchdog);
+        break;
 
     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 989146eb9..44472ce9f 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -4360,6 +4360,25 @@ qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver,
 }
 
 
+static int
+qemuDomainRemoveWatchdog(virQEMUDriverPtr driver,
+                         virDomainObjPtr vm,
+                         virDomainWatchdogDefPtr watchdog)
+{
+    virObjectEventPtr event = NULL;
+
+    VIR_DEBUG("Removing watchdog %s from domain %p %s",
+              watchdog->info.alias, vm, vm->def->name);
+
+    event = virDomainEventDeviceRemovedNewFromObj(vm, watchdog->info.alias);
+    qemuDomainEventQueue(driver, event);
+    qemuDomainReleaseDeviceAddress(vm, &watchdog->info, NULL);
+    virDomainWatchdogDefFree(vm->def->watchdog);
+    vm->def->watchdog = NULL;
+    return 0;
+}
+
+
 int
 qemuDomainRemoveDevice(virQEMUDriverPtr driver,
                        virDomainObjPtr vm,
@@ -5076,6 +5095,48 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr driver,
 }
 
 
+int
+qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
+                         virDomainObjPtr vm,
+                         virDomainWatchdogDefPtr dev)
+{
+    int ret = -1;
+    virDomainWatchdogDefPtr watchdog;
+    qemuDomainObjPrivatePtr priv = vm->privateData;
+
+    /* While domains can have up to one watchdog, the one supplied by user
+     * doesn't necessarily match the one domain has. Refuse to detach in such
+     * case. */
+    if (!(vm->def->watchdog &&
+          STREQ_NULLABLE(dev->info.alias,
+                         vm->def->watchdog->info.alias))) {
+        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+                       _("device not present in domain configuration"));
+        return -1;
+    }
+
+    watchdog = vm->def->watchdog;
+
+    qemuDomainMarkDeviceForRemoval(vm, &watchdog->info);
+    qemuDomainObjEnterMonitor(driver, vm);
+
+    ret = qemuMonitorDelDevice(priv->mon, watchdog->info.alias);
+
+    if (qemuDomainObjExitMonitor(driver, vm) < 0)
+        ret = -1;
+
+    if (ret == 0) {
+        if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) {
+            qemuDomainReleaseDeviceAddress(vm, &watchdog->info, NULL);
+            ret = qemuDomainRemoveWatchdog(driver, vm, watchdog);
+        }
+    }
+    qemuDomainResetDeviceRemoval(vm);
+
+    return ret;
+}
+
+
 int
 qemuDomainDetachNetDevice(virQEMUDriverPtr driver,
                           virDomainObjPtr vm,
diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
index a9dfd8f7a..3455832d6 100644
--- a/src/qemu/qemu_hotplug.h
+++ b/src/qemu/qemu_hotplug.h
@@ -122,6 +122,9 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver,
 int qemuDomainDetachShmemDevice(virQEMUDriverPtr driver,
                                 virDomainObjPtr vm,
                                 virDomainShmemDefPtr dev);
+int qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
+                             virDomainObjPtr vm,
+                             virDomainWatchdogDefPtr watchdog);
 int qemuDomainAttachLease(virQEMUDriverPtr driver,
                           virDomainObjPtr vm,
                           virDomainLeaseDefPtr lease);
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index b02ae8034..df28a7fbd 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -155,6 +155,9 @@ testQemuHotplugDetach(virDomainObjPtr vm,
     case VIR_DOMAIN_DEVICE_SHMEM:
         ret = qemuDomainDetachShmemDevice(&driver, vm, dev->data.shmem);
         break;
+    case VIR_DOMAIN_DEVICE_WATCHDOG:
+        ret = qemuDomainDetachWatchdog(&driver, vm, dev->data.watchdog);
+        break;
     default:
         VIR_TEST_VERBOSE("device type '%s' cannot be detached\n",
                 virDomainDeviceTypeToString(dev->type));
@@ -818,9 +821,11 @@ mymain(void)
                    "human-monitor-command", HMP("OK\\r\\n"),
                    "device_add", QMP_OK);
 
-    DO_TEST_ATTACH("base-live", "watchdog", false, false,
+    DO_TEST_ATTACH("base-live", "watchdog", false, true,
                    "watchdog-set-action", QMP_OK,
                    "device_add", QMP_OK);
+    DO_TEST_DETACH("base-live", "watchdog-full", false, false,
+                   "device_del", QMP_OK);
 
 #define DO_TEST_CPU_GROUP(prefix, vcpus, modernhp, expectfail)                 \
     do {                                                                       \
diff --git a/tests/qemuhotplugtestdevices/qemuhotplug-watchdog-full.xml b/tests/qemuhotplugtestdevices/qemuhotplug-watchdog-full.xml
new file mode 100644
index 000000000..28827b58a
--- /dev/null
+++ b/tests/qemuhotplugtestdevices/qemuhotplug-watchdog-full.xml
@@ -0,0 +1,3 @@
+<watchdog model='ib700' action='poweroff'>
+  <alias name='watchdog0'/>
+</watchdog>
-- 
2.13.5

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

On 09/05/2017 07:45 AM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1447169
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_driver.c                             |  4 +-
>  src/qemu/qemu_hotplug.c                            | 61 ++++++++++++++++++++++
>  src/qemu/qemu_hotplug.h                            |  3 ++
>  tests/qemuhotplugtest.c                            |  7 ++-
>  .../qemuhotplug-watchdog-full.xml                  |  3 ++
>  5 files changed, 76 insertions(+), 2 deletions(-)
>  create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-watchdog-full.xml
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 626148dba..4c636b956 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7692,12 +7692,14 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
>      case VIR_DOMAIN_DEVICE_SHMEM:
>          ret = qemuDomainDetachShmemDevice(driver, vm, dev->data.shmem);
>          break;
> +    case VIR_DOMAIN_DEVICE_WATCHDOG:
> +        ret = qemuDomainDetachWatchdog(driver, vm, dev->data.watchdog);
> +        break;
>  
>      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 989146eb9..44472ce9f 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -4360,6 +4360,25 @@ qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver,
>  }
>  
>  
> +static int
> +qemuDomainRemoveWatchdog(virQEMUDriverPtr driver,
> +                         virDomainObjPtr vm,
> +                         virDomainWatchdogDefPtr watchdog)
> +{
> +    virObjectEventPtr event = NULL;
> +
> +    VIR_DEBUG("Removing watchdog %s from domain %p %s",

Is "Removing watchdog watchdog0 from ..." a bit superfluous?

Perhaps just "Removing '%s' from ..."

> +              watchdog->info.alias, vm, vm->def->name);
> +

This would/could be the place for the virDomainAuditWatchdog for
"detach" too.

> +    event = virDomainEventDeviceRemovedNewFromObj(vm, watchdog->info.alias);
> +    qemuDomainEventQueue(driver, event);
> +    qemuDomainReleaseDeviceAddress(vm, &watchdog->info, NULL);
> +    virDomainWatchdogDefFree(vm->def->watchdog);
> +    vm->def->watchdog = NULL;
> +    return 0;
> +}
> +
> +
>  int
>  qemuDomainRemoveDevice(virQEMUDriverPtr driver,
>                         virDomainObjPtr vm,
> @@ -5076,6 +5095,48 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr driver,
>  }
>  
>  
> +int
> +qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
> +                         virDomainObjPtr vm,
> +                         virDomainWatchdogDefPtr dev)
> +{
> +    int ret = -1;
> +    virDomainWatchdogDefPtr watchdog;

Why not watchdog = vm->def->watchdog; here?

> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +
> +    /* While domains can have up to one watchdog, the one supplied by user

s/by/by the/

> +     * doesn't necessarily match the one domain has. Refuse to detach in such
> +     * case. */
> +    if (!(vm->def->watchdog &&
> +          STREQ_NULLABLE(dev->info.alias,
> +                         vm->def->watchdog->info.alias))) {

if (!watchdog || STRNEQ_NULLABLE(dev->info.alias, watchdog->info.alias))

Trying to think why NULLABLE would be necessary... So it seems it's
required that that input XML has the alias - something not quite right
with that...

I would think for unplug there'd be something that would compare the
input "model" and "action" values to whatever watchdog is currently
present and if they don't match, then fail. Not sure why comparing the
alias is "right". It's not something we require of other unplugs, is it?
We just need to make sure the same "something" is being removed. Since
the alias is essentially static for the single available watchdog
device, then as long as the model and action are the same, we can
remove; otherwise, someone has to fix the input XML to have the matching
model and action.

> +        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +                       _("device not present in domain configuration"));

s/device/watchdog device/

> +        return -1;
> +    }

> +
> +    watchdog = vm->def->watchdog;

Now unnecessary...

> +
> +    qemuDomainMarkDeviceForRemoval(vm, &watchdog->info);
> +    qemuDomainObjEnterMonitor(driver, vm);
> +
> +    ret = qemuMonitorDelDevice(priv->mon, watchdog->info.alias);

So if this succeeds and the following fails, then we have no watchdog in
the domain, but then again we probably have no domain if the following
fails... Of course this is similar to what the shmem code is doing, so

> +
> +    if (qemuDomainObjExitMonitor(driver, vm) < 0)
> +        ret = -1;
> +
> +    if (ret == 0) {
> +        if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) {
> +            qemuDomainReleaseDeviceAddress(vm, &watchdog->info, NULL);
> +            ret = qemuDomainRemoveWatchdog(driver, vm, watchdog);
> +        }
> +    }
> +    qemuDomainResetDeviceRemoval(vm);
> +
> +    return ret;
> +}
> +
> +
>  int
>  qemuDomainDetachNetDevice(virQEMUDriverPtr driver,
>                            virDomainObjPtr vm,
> diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
> index a9dfd8f7a..3455832d6 100644
> --- a/src/qemu/qemu_hotplug.h
> +++ b/src/qemu/qemu_hotplug.h
> @@ -122,6 +122,9 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver,
>  int qemuDomainDetachShmemDevice(virQEMUDriverPtr driver,
>                                  virDomainObjPtr vm,
>                                  virDomainShmemDefPtr dev);
> +int qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
> +                             virDomainObjPtr vm,
> +                             virDomainWatchdogDefPtr watchdog);
>  int qemuDomainAttachLease(virQEMUDriverPtr driver,
>                            virDomainObjPtr vm,
>                            virDomainLeaseDefPtr lease);
> diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
> index b02ae8034..df28a7fbd 100644
> --- a/tests/qemuhotplugtest.c
> +++ b/tests/qemuhotplugtest.c
> @@ -155,6 +155,9 @@ testQemuHotplugDetach(virDomainObjPtr vm,
>      case VIR_DOMAIN_DEVICE_SHMEM:
>          ret = qemuDomainDetachShmemDevice(&driver, vm, dev->data.shmem);
>          break;
> +    case VIR_DOMAIN_DEVICE_WATCHDOG:
> +        ret = qemuDomainDetachWatchdog(&driver, vm, dev->data.watchdog);
> +        break;
>      default:
>          VIR_TEST_VERBOSE("device type '%s' cannot be detached\n",
>                  virDomainDeviceTypeToString(dev->type));
> @@ -818,9 +821,11 @@ mymain(void)
>                     "human-monitor-command", HMP("OK\\r\\n"),
>                     "device_add", QMP_OK);
>  
> -    DO_TEST_ATTACH("base-live", "watchdog", false, false,
> +    DO_TEST_ATTACH("base-live", "watchdog", false, true,
>                     "watchdog-set-action", QMP_OK,
>                     "device_add", QMP_OK);
> +    DO_TEST_DETACH("base-live", "watchdog-full", false, false,
> +                   "device_del", QMP_OK);
>  
>  #define DO_TEST_CPU_GROUP(prefix, vcpus, modernhp, expectfail)                 \
>      do {                                                                       \
> diff --git a/tests/qemuhotplugtestdevices/qemuhotplug-watchdog-full.xml b/tests/qemuhotplugtestdevices/qemuhotplug-watchdog-full.xml
> new file mode 100644
> index 000000000..28827b58a
> --- /dev/null
> +++ b/tests/qemuhotplugtestdevices/qemuhotplug-watchdog-full.xml
> @@ -0,0 +1,3 @@
> +<watchdog model='ib700' action='poweroff'>
> +  <alias name='watchdog0'/>
> +</watchdog>
> 

Like I noted above, I don't think supplying the alias is correct or
necessary.


John

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

On 09/12/2017 04:29 PM, John Ferlan wrote:
> 
> 
> On 09/05/2017 07:45 AM, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1447169
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/qemu/qemu_driver.c                             |  4 +-
>>  src/qemu/qemu_hotplug.c                            | 61 ++++++++++++++++++++++
>>  src/qemu/qemu_hotplug.h                            |  3 ++
>>  tests/qemuhotplugtest.c                            |  7 ++-
>>  .../qemuhotplug-watchdog-full.xml                  |  3 ++
>>  5 files changed, 76 insertions(+), 2 deletions(-)
>>  create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-watchdog-full.xml
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 626148dba..4c636b956 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -7692,12 +7692,14 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
>>      case VIR_DOMAIN_DEVICE_SHMEM:
>>          ret = qemuDomainDetachShmemDevice(driver, vm, dev->data.shmem);
>>          break;
>> +    case VIR_DOMAIN_DEVICE_WATCHDOG:
>> +        ret = qemuDomainDetachWatchdog(driver, vm, dev->data.watchdog);
>> +        break;
>>  
>>      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 989146eb9..44472ce9f 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -4360,6 +4360,25 @@ qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver,
>>  }
>>  
>>  
>> +static int
>> +qemuDomainRemoveWatchdog(virQEMUDriverPtr driver,
>> +                         virDomainObjPtr vm,
>> +                         virDomainWatchdogDefPtr watchdog)
>> +{
>> +    virObjectEventPtr event = NULL;
>> +
>> +    VIR_DEBUG("Removing watchdog %s from domain %p %s",
> 
> Is "Removing watchdog watchdog0 from ..." a bit superfluous?
> 
> Perhaps just "Removing '%s' from ..."
> 
>> +              watchdog->info.alias, vm, vm->def->name);
>> +
> 
> This would/could be the place for the virDomainAuditWatchdog for
> "detach" too.
> 
>> +    event = virDomainEventDeviceRemovedNewFromObj(vm, watchdog->info.alias);
>> +    qemuDomainEventQueue(driver, event);
>> +    qemuDomainReleaseDeviceAddress(vm, &watchdog->info, NULL);
>> +    virDomainWatchdogDefFree(vm->def->watchdog);
>> +    vm->def->watchdog = NULL;
>> +    return 0;
>> +}
>> +
>> +
>>  int
>>  qemuDomainRemoveDevice(virQEMUDriverPtr driver,
>>                         virDomainObjPtr vm,
>> @@ -5076,6 +5095,48 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr driver,
>>  }
>>  
>>  
>> +int
>> +qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
>> +                         virDomainObjPtr vm,
>> +                         virDomainWatchdogDefPtr dev)
>> +{
>> +    int ret = -1;
>> +    virDomainWatchdogDefPtr watchdog;
> 
> Why not watchdog = vm->def->watchdog; here?
> 
>> +    qemuDomainObjPrivatePtr priv = vm->privateData;
>> +
>> +    /* While domains can have up to one watchdog, the one supplied by user
> 
> s/by/by the/
> 
>> +     * doesn't necessarily match the one domain has. Refuse to detach in such
>> +     * case. */
>> +    if (!(vm->def->watchdog &&
>> +          STREQ_NULLABLE(dev->info.alias,
>> +                         vm->def->watchdog->info.alias))) {
> 
> if (!watchdog || STRNEQ_NULLABLE(dev->info.alias, watchdog->info.alias))
> 
> Trying to think why NULLABLE would be necessary... So it seems it's
> required that that input XML has the alias - something not quite right
> with that...

Is that so? We match devices based on their alias in a lot of places.
Users are expected to pass the full device XML anyway. So it's up to us
how we pick the right device to be detached. For instance, when
detaching a vNIC we match MAC addresses and ignore the rest, since MAC
identifies the device uniquely. So for instance, if the detach XML
provided by user has <bandwidth/> set but the one in domain doesn't have
it, we detach the device anyway. One can argue this is a buggy
behaviour. But I just don't think we should care. There's a line we have
to draw between protecting users from themselves and too complex and
mostly useless code. So we've documented that users are supposed to pass
the device XML as is in the domain XML.

> 
> I would think for unplug there'd be something that would compare the
> input "model" and "action" values to whatever watchdog is currently
> present and if they don't match, then fail. Not sure why comparing the
> alias is "right". It's not something we require of other unplugs, is it?

Sort of. For instance when detaching disks we only care about the
target. And for a lot of other devices we require alias too.

> We just need to make sure the same "something" is being removed. Since
> the alias is essentially static for the single available watchdog
> device, then as long as the model and action are the same, we can
> remove; otherwise, someone has to fix the input XML to have the matching
> model and action.

Okay, I'll add these two checks.

Michal

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

On Tue, Sep 26, 2017 at 11:38:26AM +0200, Michal Privoznik wrote:
> On 09/12/2017 04:29 PM, John Ferlan wrote:
> > 
> > 
> > On 09/05/2017 07:45 AM, Michal Privoznik wrote:
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1447169
> >>
> >> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> >> ---
> >>  src/qemu/qemu_driver.c                             |  4 +-
> >>  src/qemu/qemu_hotplug.c                            | 61 ++++++++++++++++++++++
> >>  src/qemu/qemu_hotplug.h                            |  3 ++
> >>  tests/qemuhotplugtest.c                            |  7 ++-
> >>  .../qemuhotplug-watchdog-full.xml                  |  3 ++
> >>  5 files changed, 76 insertions(+), 2 deletions(-)
> >>  create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-watchdog-full.xml
> >>
> >> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> >> index 626148dba..4c636b956 100644
> >> --- a/src/qemu/qemu_driver.c
> >> +++ b/src/qemu/qemu_driver.c
> >> @@ -7692,12 +7692,14 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
> >>      case VIR_DOMAIN_DEVICE_SHMEM:
> >>          ret = qemuDomainDetachShmemDevice(driver, vm, dev->data.shmem);
> >>          break;
> >> +    case VIR_DOMAIN_DEVICE_WATCHDOG:
> >> +        ret = qemuDomainDetachWatchdog(driver, vm, dev->data.watchdog);
> >> +        break;
> >>  
> >>      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 989146eb9..44472ce9f 100644
> >> --- a/src/qemu/qemu_hotplug.c
> >> +++ b/src/qemu/qemu_hotplug.c
> >> @@ -4360,6 +4360,25 @@ qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver,
> >>  }
> >>  
> >>  
> >> +static int
> >> +qemuDomainRemoveWatchdog(virQEMUDriverPtr driver,
> >> +                         virDomainObjPtr vm,
> >> +                         virDomainWatchdogDefPtr watchdog)
> >> +{
> >> +    virObjectEventPtr event = NULL;
> >> +
> >> +    VIR_DEBUG("Removing watchdog %s from domain %p %s",
> > 
> > Is "Removing watchdog watchdog0 from ..." a bit superfluous?
> > 
> > Perhaps just "Removing '%s' from ..."
> > 
> >> +              watchdog->info.alias, vm, vm->def->name);
> >> +
> > 
> > This would/could be the place for the virDomainAuditWatchdog for
> > "detach" too.
> > 
> >> +    event = virDomainEventDeviceRemovedNewFromObj(vm, watchdog->info.alias);
> >> +    qemuDomainEventQueue(driver, event);
> >> +    qemuDomainReleaseDeviceAddress(vm, &watchdog->info, NULL);
> >> +    virDomainWatchdogDefFree(vm->def->watchdog);
> >> +    vm->def->watchdog = NULL;
> >> +    return 0;
> >> +}
> >> +
> >> +
> >>  int
> >>  qemuDomainRemoveDevice(virQEMUDriverPtr driver,
> >>                         virDomainObjPtr vm,
> >> @@ -5076,6 +5095,48 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr driver,
> >>  }
> >>  
> >>  
> >> +int
> >> +qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
> >> +                         virDomainObjPtr vm,
> >> +                         virDomainWatchdogDefPtr dev)
> >> +{
> >> +    int ret = -1;
> >> +    virDomainWatchdogDefPtr watchdog;
> > 
> > Why not watchdog = vm->def->watchdog; here?
> > 
> >> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> >> +
> >> +    /* While domains can have up to one watchdog, the one supplied by user
> > 
> > s/by/by the/
> > 
> >> +     * doesn't necessarily match the one domain has. Refuse to detach in such
> >> +     * case. */
> >> +    if (!(vm->def->watchdog &&
> >> +          STREQ_NULLABLE(dev->info.alias,
> >> +                         vm->def->watchdog->info.alias))) {
> > 
> > if (!watchdog || STRNEQ_NULLABLE(dev->info.alias, watchdog->info.alias))
> > 
> > Trying to think why NULLABLE would be necessary... So it seems it's
> > required that that input XML has the alias - something not quite right
> > with that...
> 
> Is that so? We match devices based on their alias in a lot of places.
> Users are expected to pass the full device XML anyway. So it's up to us
> how we pick the right device to be detached. For instance, when
> detaching a vNIC we match MAC addresses and ignore the rest, since MAC
> identifies the device uniquely. So for instance, if the detach XML
> provided by user has <bandwidth/> set but the one in domain doesn't have
> it, we detach the device anyway. One can argue this is a buggy
> behaviour. But I just don't think we should care. There's a line we have
> to draw between protecting users from themselves and too complex and
> mostly useless code. So we've documented that users are supposed to pass
> the device XML as is in the domain XML.

We can protect ourselves from the danger of user passing incomplete device
XML by not using the passed in device XML directly.

IOW, once we parse the XML to create our virDomainDevicePtr instance,
lets say we want the dev->disk virDomainDiskDefPtr to unplug a disk.
Don't use the the dev->disk pointer in the unplug code. Instead use
the dev->disk to lookup the def->disk[N] we want from virDomaniDefPtr
and then use that virDomainDiskDefPtr instead. That way our code doesn't
have to be paranoid about incompletely specified device configs. This
is safer because we'll inevitably forget to be paranoid enough in places
which leaves us vulnerable to crashes / bugs.


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

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

On 09/26/2017 12:30 PM, Daniel P. Berrange wrote:
> On Tue, Sep 26, 2017 at 11:38:26AM +0200, Michal Privoznik wrote:
>> On 09/12/2017 04:29 PM, John Ferlan wrote:
>>>
>>>
>>> On 09/05/2017 07:45 AM, Michal Privoznik wrote:
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1447169
>>>>
>>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>>> ---
>>>>  src/qemu/qemu_driver.c                             |  4 +-
>>>>  src/qemu/qemu_hotplug.c                            | 61 ++++++++++++++++++++++
>>>>  src/qemu/qemu_hotplug.h                            |  3 ++
>>>>  tests/qemuhotplugtest.c                            |  7 ++-
>>>>  .../qemuhotplug-watchdog-full.xml                  |  3 ++
>>>>  5 files changed, 76 insertions(+), 2 deletions(-)
>>>>  create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-watchdog-full.xml
>>>>
>>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>>> index 626148dba..4c636b956 100644
>>>> --- a/src/qemu/qemu_driver.c
>>>> +++ b/src/qemu/qemu_driver.c
>>>> @@ -7692,12 +7692,14 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
>>>>      case VIR_DOMAIN_DEVICE_SHMEM:
>>>>          ret = qemuDomainDetachShmemDevice(driver, vm, dev->data.shmem);
>>>>          break;
>>>> +    case VIR_DOMAIN_DEVICE_WATCHDOG:
>>>> +        ret = qemuDomainDetachWatchdog(driver, vm, dev->data.watchdog);
>>>> +        break;
>>>>  
>>>>      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 989146eb9..44472ce9f 100644
>>>> --- a/src/qemu/qemu_hotplug.c
>>>> +++ b/src/qemu/qemu_hotplug.c
>>>> @@ -4360,6 +4360,25 @@ qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver,
>>>>  }
>>>>  
>>>>  
>>>> +static int
>>>> +qemuDomainRemoveWatchdog(virQEMUDriverPtr driver,
>>>> +                         virDomainObjPtr vm,
>>>> +                         virDomainWatchdogDefPtr watchdog)
>>>> +{
>>>> +    virObjectEventPtr event = NULL;
>>>> +
>>>> +    VIR_DEBUG("Removing watchdog %s from domain %p %s",
>>>
>>> Is "Removing watchdog watchdog0 from ..." a bit superfluous?
>>>
>>> Perhaps just "Removing '%s' from ..."
>>>
>>>> +              watchdog->info.alias, vm, vm->def->name);
>>>> +
>>>
>>> This would/could be the place for the virDomainAuditWatchdog for
>>> "detach" too.
>>>
>>>> +    event = virDomainEventDeviceRemovedNewFromObj(vm, watchdog->info.alias);
>>>> +    qemuDomainEventQueue(driver, event);
>>>> +    qemuDomainReleaseDeviceAddress(vm, &watchdog->info, NULL);
>>>> +    virDomainWatchdogDefFree(vm->def->watchdog);
>>>> +    vm->def->watchdog = NULL;
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +
>>>>  int
>>>>  qemuDomainRemoveDevice(virQEMUDriverPtr driver,
>>>>                         virDomainObjPtr vm,
>>>> @@ -5076,6 +5095,48 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr driver,
>>>>  }
>>>>  
>>>>  
>>>> +int
>>>> +qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
>>>> +                         virDomainObjPtr vm,
>>>> +                         virDomainWatchdogDefPtr dev)
>>>> +{
>>>> +    int ret = -1;
>>>> +    virDomainWatchdogDefPtr watchdog;
>>>
>>> Why not watchdog = vm->def->watchdog; here?
>>>
>>>> +    qemuDomainObjPrivatePtr priv = vm->privateData;
>>>> +
>>>> +    /* While domains can have up to one watchdog, the one supplied by user
>>>
>>> s/by/by the/
>>>
>>>> +     * doesn't necessarily match the one domain has. Refuse to detach in such
>>>> +     * case. */
>>>> +    if (!(vm->def->watchdog &&
>>>> +          STREQ_NULLABLE(dev->info.alias,
>>>> +                         vm->def->watchdog->info.alias))) {
>>>
>>> if (!watchdog || STRNEQ_NULLABLE(dev->info.alias, watchdog->info.alias))
>>>
>>> Trying to think why NULLABLE would be necessary... So it seems it's
>>> required that that input XML has the alias - something not quite right
>>> with that...
>>
>> Is that so? We match devices based on their alias in a lot of places.
>> Users are expected to pass the full device XML anyway. So it's up to us
>> how we pick the right device to be detached. For instance, when
>> detaching a vNIC we match MAC addresses and ignore the rest, since MAC
>> identifies the device uniquely. So for instance, if the detach XML
>> provided by user has <bandwidth/> set but the one in domain doesn't have
>> it, we detach the device anyway. One can argue this is a buggy
>> behaviour. But I just don't think we should care. There's a line we have
>> to draw between protecting users from themselves and too complex and
>> mostly useless code. So we've documented that users are supposed to pass
>> the device XML as is in the domain XML.
> 
> We can protect ourselves from the danger of user passing incomplete device
> XML by not using the passed in device XML directly.
> 
> IOW, once we parse the XML to create our virDomainDevicePtr instance,
> lets say we want the dev->disk virDomainDiskDefPtr to unplug a disk.
> Don't use the the dev->disk pointer in the unplug code. Instead use
> the dev->disk to lookup the def->disk[N] we want from virDomaniDefPtr
> and then use that virDomainDiskDefPtr instead. That way our code doesn't
> have to be paranoid about incompletely specified device configs. This
> is safer because we'll inevitably forget to be paranoid enough in places
> which leaves us vulnerable to crashes / bugs.
> 

Yup. That's exactly what we do. And while in fact using our own
definition of the device, the one provided by user is not taken fully
into consideration. Which is good. I'm just saying that to explain why I
think that alias is just enough for watchdog detach. Look at my code,
I'm using alias just to lookup the config stored in domain def.

Michal

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

On 09/26/2017 05:38 AM, Michal Privoznik wrote:
> On 09/12/2017 04:29 PM, John Ferlan wrote:
>>
>>
>> On 09/05/2017 07:45 AM, Michal Privoznik wrote:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1447169
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>> ---
>>>  src/qemu/qemu_driver.c                             |  4 +-
>>>  src/qemu/qemu_hotplug.c                            | 61 ++++++++++++++++++++++
>>>  src/qemu/qemu_hotplug.h                            |  3 ++
>>>  tests/qemuhotplugtest.c                            |  7 ++-
>>>  .../qemuhotplug-watchdog-full.xml                  |  3 ++
>>>  5 files changed, 76 insertions(+), 2 deletions(-)
>>>  create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-watchdog-full.xml
>>>
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index 626148dba..4c636b956 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -7692,12 +7692,14 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
>>>      case VIR_DOMAIN_DEVICE_SHMEM:
>>>          ret = qemuDomainDetachShmemDevice(driver, vm, dev->data.shmem);
>>>          break;
>>> +    case VIR_DOMAIN_DEVICE_WATCHDOG:
>>> +        ret = qemuDomainDetachWatchdog(driver, vm, dev->data.watchdog);
>>> +        break;
>>>  
>>>      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 989146eb9..44472ce9f 100644
>>> --- a/src/qemu/qemu_hotplug.c
>>> +++ b/src/qemu/qemu_hotplug.c
>>> @@ -4360,6 +4360,25 @@ qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver,
>>>  }
>>>  
>>>  
>>> +static int
>>> +qemuDomainRemoveWatchdog(virQEMUDriverPtr driver,
>>> +                         virDomainObjPtr vm,
>>> +                         virDomainWatchdogDefPtr watchdog)
>>> +{
>>> +    virObjectEventPtr event = NULL;
>>> +
>>> +    VIR_DEBUG("Removing watchdog %s from domain %p %s",
>>
>> Is "Removing watchdog watchdog0 from ..." a bit superfluous?
>>
>> Perhaps just "Removing '%s' from ..."
>>
>>> +              watchdog->info.alias, vm, vm->def->name);
>>> +
>>
>> This would/could be the place for the virDomainAuditWatchdog for
>> "detach" too.
>>
>>> +    event = virDomainEventDeviceRemovedNewFromObj(vm, watchdog->info.alias);
>>> +    qemuDomainEventQueue(driver, event);
>>> +    qemuDomainReleaseDeviceAddress(vm, &watchdog->info, NULL);
>>> +    virDomainWatchdogDefFree(vm->def->watchdog);
>>> +    vm->def->watchdog = NULL;
>>> +    return 0;
>>> +}
>>> +
>>> +
>>>  int
>>>  qemuDomainRemoveDevice(virQEMUDriverPtr driver,
>>>                         virDomainObjPtr vm,
>>> @@ -5076,6 +5095,48 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr driver,
>>>  }
>>>  
>>>  
>>> +int
>>> +qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
>>> +                         virDomainObjPtr vm,
>>> +                         virDomainWatchdogDefPtr dev)
>>> +{
>>> +    int ret = -1;
>>> +    virDomainWatchdogDefPtr watchdog;
>>
>> Why not watchdog = vm->def->watchdog; here?
>>
>>> +    qemuDomainObjPrivatePtr priv = vm->privateData;
>>> +
>>> +    /* While domains can have up to one watchdog, the one supplied by user
>>
>> s/by/by the/
>>
>>> +     * doesn't necessarily match the one domain has. Refuse to detach in such
>>> +     * case. */
>>> +    if (!(vm->def->watchdog &&
>>> +          STREQ_NULLABLE(dev->info.alias,
>>> +                         vm->def->watchdog->info.alias))) {
>>
>> if (!watchdog || STRNEQ_NULLABLE(dev->info.alias, watchdog->info.alias))
>>
>> Trying to think why NULLABLE would be necessary... So it seems it's
>> required that that input XML has the alias - something not quite right
>> with that...
> 
> Is that so? We match devices based on their alias in a lot of places.
> Users are expected to pass the full device XML anyway. So it's up to us
> how we pick the right device to be detached. For instance, when
> detaching a vNIC we match MAC addresses and ignore the rest, since MAC
> identifies the device uniquely. So for instance, if the detach XML
> provided by user has <bandwidth/> set but the one in domain doesn't have
> it, we detach the device anyway. One can argue this is a buggy
> behaviour. But I just don't think we should care. There's a line we have
> to draw between protecting users from themselves and too complex and
> mostly useless code. So we've documented that users are supposed to pass
> the device XML as is in the domain XML.
> 

Can the alias be user supplied and not be overwritten on attach?  IOW:
If someone supplies watchdog5 on hot plug, would supplying the same XML
work on hot unplug?

Typically I pass/use the same XML for my disk/hostdev hot plug/unplug.
Some of those aliases could be tricky unless of course someone's looked
at the process arguments.

And of course I now see the NULLABLE is being used because even the
input XML doesn't require it, you're checking that it is provided and
using that as the sole determination of matching the input XML against
what's been loaded into QEMU either via cold or hot plug.

>>
>> I would think for unplug there'd be something that would compare the
>> input "model" and "action" values to whatever watchdog is currently
>> present and if they don't match, then fail. Not sure why comparing the
>> alias is "right". It's not something we require of other unplugs, is it?
> 
> Sort of. For instance when detaching disks we only care about the
> target. And for a lot of other devices we require alias too.
> 

But we also seem to make use of whatever alias exists for the device in
many places... I looked at a few and I couldn't find a case where the
alias was required on input for the detach. Yes it's checked, but
there's various ways of ensuring it's either present in whatever was
used for the unplug XML or just building up the alias given other
matching criteria has been met.

John


>> We just need to make sure the same "something" is being removed. Since
>> the alias is essentially static for the single available watchdog
>> device, then as long as the model and action are the same, we can
>> remove; otherwise, someone has to fix the input XML to have the matching
>> model and action.
> 
> Okay, I'll add these two checks.
> 
> Michal
> 

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

On 09/26/2017 01:34 PM, John Ferlan wrote:
> 
> 
> On 09/26/2017 05:38 AM, Michal Privoznik wrote:
>> On 09/12/2017 04:29 PM, John Ferlan wrote:
>>>
>>>
>>> On 09/05/2017 07:45 AM, Michal Privoznik wrote:
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1447169
>>>>
>>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>>> ---
>>>>  src/qemu/qemu_driver.c                             |  4 +-
>>>>  src/qemu/qemu_hotplug.c                            | 61 ++++++++++++++++++++++
>>>>  src/qemu/qemu_hotplug.h                            |  3 ++
>>>>  tests/qemuhotplugtest.c                            |  7 ++-
>>>>  .../qemuhotplug-watchdog-full.xml                  |  3 ++
>>>>  5 files changed, 76 insertions(+), 2 deletions(-)
>>>>  create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-watchdog-full.xml
>>>>
>>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>>> index 626148dba..4c636b956 100644
>>>> --- a/src/qemu/qemu_driver.c
>>>> +++ b/src/qemu/qemu_driver.c
>>>> @@ -7692,12 +7692,14 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
>>>>      case VIR_DOMAIN_DEVICE_SHMEM:
>>>>          ret = qemuDomainDetachShmemDevice(driver, vm, dev->data.shmem);
>>>>          break;
>>>> +    case VIR_DOMAIN_DEVICE_WATCHDOG:
>>>> +        ret = qemuDomainDetachWatchdog(driver, vm, dev->data.watchdog);
>>>> +        break;
>>>>  
>>>>      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 989146eb9..44472ce9f 100644
>>>> --- a/src/qemu/qemu_hotplug.c
>>>> +++ b/src/qemu/qemu_hotplug.c
>>>> @@ -4360,6 +4360,25 @@ qemuDomainRemoveShmemDevice(virQEMUDriverPtr driver,
>>>>  }
>>>>  
>>>>  
>>>> +static int
>>>> +qemuDomainRemoveWatchdog(virQEMUDriverPtr driver,
>>>> +                         virDomainObjPtr vm,
>>>> +                         virDomainWatchdogDefPtr watchdog)
>>>> +{
>>>> +    virObjectEventPtr event = NULL;
>>>> +
>>>> +    VIR_DEBUG("Removing watchdog %s from domain %p %s",
>>>
>>> Is "Removing watchdog watchdog0 from ..." a bit superfluous?
>>>
>>> Perhaps just "Removing '%s' from ..."
>>>
>>>> +              watchdog->info.alias, vm, vm->def->name);
>>>> +
>>>
>>> This would/could be the place for the virDomainAuditWatchdog for
>>> "detach" too.
>>>
>>>> +    event = virDomainEventDeviceRemovedNewFromObj(vm, watchdog->info.alias);
>>>> +    qemuDomainEventQueue(driver, event);
>>>> +    qemuDomainReleaseDeviceAddress(vm, &watchdog->info, NULL);
>>>> +    virDomainWatchdogDefFree(vm->def->watchdog);
>>>> +    vm->def->watchdog = NULL;
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +
>>>>  int
>>>>  qemuDomainRemoveDevice(virQEMUDriverPtr driver,
>>>>                         virDomainObjPtr vm,
>>>> @@ -5076,6 +5095,48 @@ qemuDomainDetachShmemDevice(virQEMUDriverPtr driver,
>>>>  }
>>>>  
>>>>  
>>>> +int
>>>> +qemuDomainDetachWatchdog(virQEMUDriverPtr driver,
>>>> +                         virDomainObjPtr vm,
>>>> +                         virDomainWatchdogDefPtr dev)
>>>> +{
>>>> +    int ret = -1;
>>>> +    virDomainWatchdogDefPtr watchdog;
>>>
>>> Why not watchdog = vm->def->watchdog; here?
>>>
>>>> +    qemuDomainObjPrivatePtr priv = vm->privateData;
>>>> +
>>>> +    /* While domains can have up to one watchdog, the one supplied by user
>>>
>>> s/by/by the/
>>>
>>>> +     * doesn't necessarily match the one domain has. Refuse to detach in such
>>>> +     * case. */
>>>> +    if (!(vm->def->watchdog &&
>>>> +          STREQ_NULLABLE(dev->info.alias,
>>>> +                         vm->def->watchdog->info.alias))) {
>>>
>>> if (!watchdog || STRNEQ_NULLABLE(dev->info.alias, watchdog->info.alias))
>>>
>>> Trying to think why NULLABLE would be necessary... So it seems it's
>>> required that that input XML has the alias - something not quite right
>>> with that...
>>
>> Is that so? We match devices based on their alias in a lot of places.
>> Users are expected to pass the full device XML anyway. So it's up to us
>> how we pick the right device to be detached. For instance, when
>> detaching a vNIC we match MAC addresses and ignore the rest, since MAC
>> identifies the device uniquely. So for instance, if the detach XML
>> provided by user has <bandwidth/> set but the one in domain doesn't have
>> it, we detach the device anyway. One can argue this is a buggy
>> behaviour. But I just don't think we should care. There's a line we have
>> to draw between protecting users from themselves and too complex and
>> mostly useless code. So we've documented that users are supposed to pass
>> the device XML as is in the domain XML.
>>
> 
> Can the alias be user supplied and not be overwritten on attach?  IOW:
> If someone supplies watchdog5 on hot plug, would supplying the same XML
> work on hot unplug?

No, it wouldn't work. Because there's no watchdog5. However, what you're
doing is flawed according to the documentation. You're misusing the
APIs, because you know how they work internally and that's okay for the
testing we do when developing new feature. But in production, users are
required to:

1) write the device XML
2) hotplug it
3) dump the domain XML
4) find the device they want to detach
5) provide full device XML

Note, that after step 2, there are going to be differences between user
supplied XML and the device XML in domain XML. For instance, user are
not required to provide PCI address they want to attach the device at.
Libvirt calculates its own.

> 
> Typically I pass/use the same XML for my disk/hostdev hot plug/unplug.
> Some of those aliases could be tricky unless of course someone's looked
> at the process arguments.

Since there can be one watchdog per domain at most, passing "watchdog5"
as device alias would be unwise. But as I say, you're kind of misusing
the attach/detach APIs. I'm doing the same thing, because we both know
the internals. BTW: This approach does not work with all the devices -
just try to unplug a dimm module with such 'generic' XML. Dimm's alias
is checked explicitly.

Anyway, to raise chances of me getting this in, I've already added
checks for model & action.

> 
> And of course I now see the NULLABLE is being used because even the
> input XML doesn't require it, you're checking that it is provided and
> using that as the sole determination of matching the input XML against
> what's been loaded into QEMU either via cold or hot plug.

Yes.

> 
>>>
>>> I would think for unplug there'd be something that would compare the
>>> input "model" and "action" values to whatever watchdog is currently
>>> present and if they don't match, then fail. Not sure why comparing the
>>> alias is "right". It's not something we require of other unplugs, is it?
>>
>> Sort of. For instance when detaching disks we only care about the
>> target. And for a lot of other devices we require alias too.
>>
> 
> But we also seem to make use of whatever alias exists for the device in
> many places... I looked at a few and I couldn't find a case where the
> alias was required on input for the detach.

/**
 * virDomainDetachDeviceFlags:
 * @domain: pointer to domain object
 * @xml: pointer to XML description of one device
 * @flags: bitwise-OR of virDomainDeviceModifyFlags
 *
 * ..
 *
 * The supplied XML description of the device should be as specific
 * as its definition in the domain XML. The set of attributes used
 * to match the device are internal to the drivers. Using a partial
definition,
 * or attempting to detach a device that is not present in the domain XML,
 * but shares some specific attributes with one that is present,
 * may lead to unexpected results.
 */

I'm open for discussion though.

Michal

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