[libvirt] [PATCHv2] qemu: check for vm after starting a job

[Daniel Veillard]

On Wed, Oct 27, 2010 at 02:28:51PM -0600, Eric Blake wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=638285 - when migrating
> a guest, it was very easy to provoke a race where an application
> could query block information on a VM that had just been migrated
> away.  Any time qemu code obtains a job lock, it must also check
> that the VM was not taken down in the time where it was waiting
> for the lock.
> 
> * src/qemu/qemu_driver.c (qemudDomainSetMemory)
> (qemudDomainGetInfo, qemuDomainGetBlockInfo): Check that vm still
> exists after obtaining job lock, before starting monitor action.
> ---
> 
> v2: incorporate danpb's suggestions, to minimize virDomainObjIsActive
> calls and avoid use of goto for mid-function labels.
> 
>  src/qemu/qemu_driver.c |   60 ++++++++++++++++++++++++-----------------------
>  1 files changed, 31 insertions(+), 29 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index b119ca1..f1f8cdf 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -466,7 +466,8 @@ static int ATTRIBUTE_RETURN_CHECK qemuDomainObjEndJob(virDomainObjPtr obj)
>   * obj must be locked before calling, qemud_driver must be unlocked
>   *
>   * To be called immediately before any QEMU monitor API call
> - * Must have alrady called qemuDomainObjBeginJob().
> + * Must have already called qemuDomainObjBeginJob(), and checked
> + * that the VM is still active.
>   *
>   * To be followed with qemuDomainObjExitMonitor() once complete
>   */
> @@ -507,7 +508,7 @@ static void qemuDomainObjExitMonitor(virDomainObjPtr obj)
>   * obj must be locked before calling, qemud_driver must be locked
>   *
>   * To be called immediately before any QEMU monitor API call
> - * Must have alrady called qemuDomainObjBeginJob().
> + * Must have already called qemuDomainObjBeginJob().
>   *
>   * To be followed with qemuDomainObjExitMonitorWithDriver() once complete
>   */
> @@ -525,7 +526,7 @@ static void qemuDomainObjEnterMonitorWithDriver(struct qemud_driver *driver, vir
>  /* obj must NOT be locked before calling, qemud_driver must be unlocked,
>   * and will be locked after returning
>   *
> - * Should be paired with an earlier  qemuDomainObjEnterMonitor() call
> + * Should be paired with an earlier qemuDomainObjEnterMonitorWithDriver() call
>   */
>  static void qemuDomainObjExitMonitorWithDriver(struct qemud_driver *driver, virDomainObjPtr obj)
>  {
> @@ -5077,12 +5078,6 @@ static int qemudDomainSetMemory(virDomainPtr dom, unsigned long newmem) {
>          goto cleanup;
>      }
> 
> -    if (!virDomainObjIsActive(vm)) {
> -        qemuReportError(VIR_ERR_OPERATION_INVALID,
> -                        "%s", _("domain is not running"));
> -        goto cleanup;
> -    }
> -
>      if (newmem > vm->def->mem.max_balloon) {
>          qemuReportError(VIR_ERR_INVALID_ARG,
>                          "%s", _("cannot set memory higher than max memory"));
> @@ -5092,6 +5087,12 @@ static int qemudDomainSetMemory(virDomainPtr dom, unsigned long newmem) {
>      if (qemuDomainObjBeginJob(vm) < 0)
>          goto cleanup;
> 
> +    if (!virDomainObjIsActive(vm)) {
> +        qemuReportError(VIR_ERR_OPERATION_INVALID,
> +                        "%s", _("domain is not running"));
> +        goto endjob;
> +    }
> +
>      priv = vm->privateData;
>      qemuDomainObjEnterMonitor(vm);
>      r = qemuMonitorSetBalloon(priv->mon, newmem);
> @@ -5158,26 +5159,25 @@ static int qemudDomainGetInfo(virDomainPtr dom,
>          } else if (!priv->jobActive) {
>              if (qemuDomainObjBeginJob(vm) < 0)
>                  goto cleanup;
> -
> -            qemuDomainObjEnterMonitor(vm);
> -            err = qemuMonitorGetBalloonInfo(priv->mon, &balloon);
> -            qemuDomainObjExitMonitor(vm);
> -            if (err < 0) {
> -                if (qemuDomainObjEndJob(vm) == 0)
> -                    vm = NULL;
> +            if (!virDomainObjIsActive(vm))
> +                err = 0;
> +            else {
> +                qemuDomainObjEnterMonitor(vm);
> +                err = qemuMonitorGetBalloonInfo(priv->mon, &balloon);
> +                qemuDomainObjExitMonitor(vm);
> +            }
> +            if (qemuDomainObjEndJob(vm) == 0) {
> +                vm = NULL;
>                  goto cleanup;
>              }
> 
> +            if (err < 0)
> +                goto cleanup;
>              if (err == 0)
>                  /* Balloon not supported, so maxmem is always the allocation */
>                  info->memory = vm->def->mem.max_balloon;
>              else
>                  info->memory = balloon;
> -
> -            if (qemuDomainObjEndJob(vm) == 0) {
> -                vm = NULL;
> -                goto cleanup;
> -            }
>          } else {
>              info->memory = vm->def->mem.cur_balloon;
>          }
> @@ -10510,19 +10510,21 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom,
>      /* ..but if guest is running & not using raw
>         disk format and on a block device, then query
>         highest allocated extent from QEMU */
> -    if (virDomainObjIsActive(vm) &&
> -        disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK &&
> +    if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK &&
>          format != VIR_STORAGE_FILE_RAW &&
>          S_ISBLK(sb.st_mode)) {
>          qemuDomainObjPrivatePtr priv = vm->privateData;
>          if (qemuDomainObjBeginJob(vm) < 0)
>              goto cleanup;
> -
> -        qemuDomainObjEnterMonitor(vm);
> -        ret = qemuMonitorGetBlockExtent(priv->mon,
> -                                        disk->info.alias,
> -                                        &info->allocation);
> -        qemuDomainObjExitMonitor(vm);
> +        if (!virDomainObjIsActive(vm))
> +            ret = 0;
> +        else {
> +            qemuDomainObjEnterMonitor(vm);
> +            ret = qemuMonitorGetBlockExtent(priv->mon,
> +                                            disk->info.alias,
> +                                            &info->allocation);
> +            qemuDomainObjExitMonitor(vm);
> +        }
> 
>          if (qemuDomainObjEndJob(vm) == 0)
>              vm = NULL;

  ACK, it's true it makes for a nicer patch than v1

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/