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

Re: [libvirt] [PATCH 1/8] Fix up the locking in the snapshot code.



On Fri, Apr 23, 2010 at 01:27:45PM -0400, Chris Lalancette wrote:
> In particular I was forgetting to take the qemuMonitorPrivatePtr
> lock (via qemuDomainObjBeginJob), which would cause problems
> if two users tried to access the same domain at the same time.
> This patch also fixes a problem where I was forgetting to remove
> a transient domain from the list of domains.
> 
> Thanks to Stephen Shaw for pointing out the problem and testing
> out the initial patch.
> 
> Signed-off-by: Chris Lalancette <clalance redhat com>
> ---
>  src/qemu/qemu_driver.c |   45 +++++++++++++++++++++++++++++++--------------
>  1 files changed, 31 insertions(+), 14 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 1ef15dd..2f889d9 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -10690,7 +10690,6 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain,
>      virDomainSnapshotPtr snapshot = NULL;
>      char uuidstr[VIR_UUID_STRING_BUFLEN];
>      virDomainSnapshotDefPtr def;
> -    qemuDomainObjPrivatePtr priv;
>      const char *qemuimgarg[] = { NULL, "snapshot", "-c", NULL, NULL, NULL };
>      int i;
>  
> @@ -10757,14 +10756,19 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain,
>          }
>      }
>      else {
> +        qemuDomainObjPrivatePtr priv;
> +        int ret;
> +
> +        if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
> +            goto cleanup;
>          priv = vm->privateData;
>          qemuDomainObjEnterMonitorWithDriver(driver, vm);
> -        if (qemuMonitorCreateSnapshot(priv->mon, def->name) < 0) {
> -            qemuDomainObjExitMonitorWithDriver(driver, vm);
> -            goto cleanup;
> -        }
> +        ret = qemuMonitorCreateSnapshot(priv->mon, def->name);
>          qemuDomainObjExitMonitorWithDriver(driver, vm);
> -
> +        if (qemuDomainObjEndJob(vm) == 0)
> +            vm = NULL;
> +        if (ret < 0)
> +            goto cleanup;
>      }
>  
>      snap->def->state = vm->state;
> @@ -11037,18 +11041,18 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>              rc = qemuMonitorLoadSnapshot(priv->mon, snap->def->name);
>              qemuDomainObjExitMonitorWithDriver(driver, vm);
>              if (rc < 0)
> -                goto cleanup;
> +                goto endjob;
>          }
>          else {
>              if (qemuDomainSnapshotSetActive(vm, driver->snapshotDir) < 0)
> -                goto cleanup;
> +                goto endjob;
>  
>              rc = qemudStartVMDaemon(snapshot->domain->conn, driver, vm, NULL,
>                                      -1);
>              if (qemuDomainSnapshotSetInactive(vm, driver->snapshotDir) < 0)
> -                goto cleanup;
> +                goto endjob;
>              if (rc < 0)
> -                goto cleanup;
> +                goto endjob;
>          }
>  
>          if (snap->def->state == VIR_DOMAIN_PAUSED) {
> @@ -11060,7 +11064,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>              rc = qemuMonitorStopCPUs(priv->mon);
>              qemuDomainObjExitMonitorWithDriver(driver, vm);
>              if (rc < 0)
> -                goto cleanup;
> +                goto endjob;
>          }
>  
>          event = virDomainEventNewFromObj(vm,
> @@ -11083,20 +11087,26 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>              event = virDomainEventNewFromObj(vm,
>                                               VIR_DOMAIN_EVENT_STOPPED,
>                                               VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT);
> +            if (!vm->persistent) {
> +                if (qemuDomainObjEndJob(vm) > 0)
> +                    virDomainRemoveInactive(&driver->domains, vm);
> +                vm = NULL;
> +            }
>          }
>  
>          if (qemuDomainSnapshotSetActive(vm, driver->snapshotDir) < 0)
> -            goto cleanup;
> +            goto endjob;
>      }
>  
>      vm->state = snap->def->state;
>  
>      ret = 0;
>  
> -cleanup:
> +endjob:
>      if (vm && qemuDomainObjEndJob(vm) == 0)
>          vm = NULL;
>  
> +cleanup:
>      if (event)
>          qemuDomainEventQueue(driver, event);
>      if (vm)
> @@ -11250,6 +11260,9 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
>          goto cleanup;
>      }
>  
> +    if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
> +        goto cleanup;
> +
>      if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN) {
>          rem.driver = driver;
>          rem.vm = vm;
> @@ -11258,11 +11271,15 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
>          virHashForEach(vm->snapshots.objs, qemuDomainSnapshotDiscardChildren,
>                         &rem);
>          if (rem.err < 0)
> -            goto cleanup;
> +            goto endjob;
>      }
>  
>      ret = qemuDomainSnapshotDiscard(driver, vm, snap);
>  
> +endjob:
> +    if (qemuDomainObjEndJob(vm) == 0)
> +        vm = NULL;
> +
>  cleanup:
>      if (vm)
>          virDomainObjUnlock(vm);

  ACK, looks fine to me,

Daniel

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


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