Re: [PATCHv2 09/14] Exit early after domain crash in monitor on device hotplug

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

 




On 01/07/2015 10:42 AM, Ján Tomko wrote:
> Error out if the domain has disappeared in the meantime.
> 
> This prevents writing the persistent definition as the domain
> status and calling qemuDomainRemoveDevice on devices
> that already have been dealt with in qemuProcessStop.
> ---
>  src/qemu/qemu_domain.c  |  9 +++++----
>  src/qemu/qemu_driver.c  |  4 ++--
>  src/qemu/qemu_hotplug.c | 38 +++++++++++++++++++-------------------
>  3 files changed, 26 insertions(+), 25 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index c942b02..82d0c91 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2748,17 +2748,18 @@ qemuDomainUpdateDeviceList(virQEMUDriverPtr driver,
>  {
>      qemuDomainObjPrivatePtr priv = vm->privateData;
>      char **aliases;
> +    int rc;
>  
>      if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT))
>          return 0;
>  
>      if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
>          return -1;
> -    if (qemuMonitorGetDeviceAliases(priv->mon, &aliases) < 0) {
> -        qemuDomainObjExitMonitor(driver, vm);
> +    rc = qemuMonitorGetDeviceAliases(priv->mon, &aliases);
> +    if (qemuDomainObjExitMonitor(driver, vm) < 0)
> +        return -1;
> +    if (rc < 0)
>          return -1;
> -    }
> -    qemuDomainObjExitMonitor(driver, vm);
>  
>      virStringFreeList(priv->qemuDevices);
>      priv->qemuDevices = aliases;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 30c9017..1781ea9 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6982,7 +6982,7 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm,
>      }
>  
>      if (ret == 0)
> -        qemuDomainUpdateDeviceList(driver, vm, QEMU_ASYNC_JOB_NONE);
> +        ret = qemuDomainUpdateDeviceList(driver, vm, QEMU_ASYNC_JOB_NONE);
>  
>      return ret;
>  }
> @@ -7058,7 +7058,7 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
>      }
>  
>      if (ret == 0)
> -        qemuDomainUpdateDeviceList(driver, vm, QEMU_ASYNC_JOB_NONE);
> +        ret = qemuDomainUpdateDeviceList(driver, vm, QEMU_ASYNC_JOB_NONE);
>  
>      return ret;
>  }
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index a4e4d6b..6b108bf 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -193,7 +193,8 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
>  
>      qemuDomainObjEnterMonitor(driver, vm);
>      ret = qemuMonitorEjectMedia(priv->mon, driveAlias, force);
> -    qemuDomainObjExitMonitor(driver, vm);
> +    if (qemuDomainObjExitMonitor(driver, vm) < 0)
> +        goto cleanup;

if "ret == 0" we get to cleanup... with ejected media, no Audit and the
rest of this not done...
>  
>      if (ret < 0)
>          goto error;
> @@ -236,7 +237,8 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
>                                       driveAlias,
>                                       sourcestr,
>                                       format);
> -        qemuDomainObjExitMonitor(driver, vm);
> +        if (qemuDomainObjExitMonitor(driver, vm) < 0)
> +            goto cleanup;

Same here - get to cleanup with ret == 0 and of course no audit.

>      }
>  
>      virDomainAuditDisk(vm, disk->src, newsrc, "update", ret >= 0);
> @@ -275,7 +277,8 @@ qemuDomainCheckEjectableMedia(virQEMUDriverPtr driver,
>  
>      if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) == 0) {
>          table = qemuMonitorGetBlockInfo(priv->mon);
> -        qemuDomainObjExitMonitor(driver, vm);
> +        if (qemuDomainObjExitMonitor(driver, vm) < 0)
> +            goto cleanup;
>      }
>  
>      if (!table)
> @@ -1017,7 +1020,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
>          if (qemuMonitorAddNetdev(priv->mon, netstr,
>                                   tapfd, tapfdName, tapfdSize,
>                                   vhostfd, vhostfdName, vhostfdSize) < 0) {
> -            qemuDomainObjExitMonitor(driver, vm);
> +            ignore_value(qemuDomainObjExitMonitor(driver, vm));

Here we are with the audit thing again

>              virDomainAuditNet(vm, NULL, net, "attach", false);
>              goto cleanup;
>          }
> @@ -1025,24 +1028,19 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
>          if (qemuMonitorAddHostNetwork(priv->mon, netstr,
>                                        tapfd, tapfdName, tapfdSize,
>                                        vhostfd, vhostfdName, vhostfdSize) < 0) {
> -            qemuDomainObjExitMonitor(driver, vm);
> +            ignore_value(qemuDomainObjExitMonitor(driver, vm));

again

>              virDomainAuditNet(vm, NULL, net, "attach", false);
>              goto cleanup;
>          }
>      }
> -    qemuDomainObjExitMonitor(driver, vm);
> +    if (qemuDomainObjExitMonitor(driver, vm) < 0)
> +        goto cleanup;
>  
>      for (i = 0; i < tapfdSize; i++)
>          VIR_FORCE_CLOSE(tapfd[i]);
>      for (i = 0; i < vhostfdSize; i++)
>          VIR_FORCE_CLOSE(vhostfd[i]);
>  
> -    if (!virDomainObjIsActive(vm)) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                       _("guest unexpectedly quit"));
> -        goto cleanup;
> -    }
> -
>      if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
>          if (!(nicstr = qemuBuildNicDevStr(vm->def, net, vlan, 0,
>                                            vhostfdSize, priv->qemuCaps)))
> @@ -1055,7 +1053,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
>      qemuDomainObjEnterMonitor(driver, vm);
>      if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
>          if (qemuMonitorAddDevice(priv->mon, nicstr) < 0) {
> -            qemuDomainObjExitMonitor(driver, vm);
> +            ignore_value(qemuDomainObjExitMonitor(driver, vm));

Audit ...

>              virDomainAuditNet(vm, NULL, net, "attach", false);
>              goto try_remove;
>          }
> @@ -1063,14 +1061,15 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
>          guestAddr = net->info.addr.pci;
>          if (qemuMonitorAddPCINetwork(priv->mon, nicstr,
>                                       &guestAddr) < 0) {
> -            qemuDomainObjExitMonitor(driver, vm);
> +            ignore_value(qemuDomainObjExitMonitor(driver, vm));

Audit...

>              virDomainAuditNet(vm, NULL, net, "attach", false);
>              goto try_remove;
>          }
>          net->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI;
>          memcpy(&net->info.addr.pci, &guestAddr, sizeof(guestAddr));
>      }
> -    qemuDomainObjExitMonitor(driver, vm);
> +    if (qemuDomainObjExitMonitor(driver, vm) < 0)
> +        goto cleanup;

Audit - after the following if is only necessary if we are successful,
so I guess OK to not audit here; however, up through this point we could
have succeeded only to see the guest die...

>  
>      /* set link state */
>      if (net->linkstate == VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN) {
> @@ -1082,7 +1081,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
>  
>              if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV)) {
>                  if (qemuMonitorSetLink(priv->mon, net->info.alias, VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN) < 0) {
> -                    qemuDomainObjExitMonitor(driver, vm);
> +                    ignore_value(qemuDomainObjExitMonitor(driver, vm));

Audit...

>                      virDomainAuditNet(vm, NULL, net, "attach", false);
>                      goto try_remove;
>                  }
> @@ -1091,7 +1090,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
>                                 _("setting of link state not supported: Link is up"));
>              }
>  
> -            qemuDomainObjExitMonitor(driver, vm);
> +            if (qemuDomainObjExitMonitor(driver, vm) < 0)
> +                goto cleanup;

Audit...
>          }
>          /* link set to down */
>      }

FWIW: Successful Audit call is right here....

Not Auditing beyond here here seems fine and it's all error/exit path
anyway.

ACK once we clear up what the right thing to do with audit and of course
the place or two where we've returned without setting ret = -1 when
ExitMonitor fails.

John

> @@ -1165,7 +1165,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
>              if (qemuMonitorRemoveNetdev(priv->mon, netdev_name) < 0)
>                  VIR_WARN("Failed to remove network backend for netdev %s",
>                           netdev_name);
> -            qemuDomainObjExitMonitor(driver, vm);
> +            ignore_value(qemuDomainObjExitMonitor(driver, vm));
>              VIR_FREE(netdev_name);
>          } else {
>              VIR_WARN("Unable to remove network backend");
> @@ -1178,7 +1178,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
>          if (qemuMonitorRemoveHostNetwork(priv->mon, vlan, hostnet_name) < 0)
>              VIR_WARN("Failed to remove network backend for vlan %d, net %s",
>                       vlan, hostnet_name);
> -        qemuDomainObjExitMonitor(driver, vm);
> +        ignore_value(qemuDomainObjExitMonitor(driver, vm));
>          VIR_FREE(hostnet_name);
>      }
>      goto cleanup;
> 

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