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

Re: [libvirt] [PATCH 3/4] qemu: Don't remove hash entry of other domains



On 02/08/2013 08:08 AM, Osier Yang wrote:
> qemuProcessStart invokes qemuProcessStop when fails, to avoid
> removing hash entry which belongs to other domain(s), this introduces
> a new virBitmapPtr argument for qemuProcessStop. And qemuProcessStart
> sets the bit for the disk only if it's successfully added into the
> hash table. Thus if the argument is provided for qemuProcessStop, it
> can't remove the hash entry belongs to other domain(s).
> ---
>  src/qemu/qemu_conf.c      |    5 ++++-
>  src/qemu/qemu_conf.h      |    3 ++-
>  src/qemu/qemu_driver.c    |   21 +++++++++++----------
>  src/qemu/qemu_migration.c |   14 +++++++-------
>  src/qemu/qemu_process.c   |   37 +++++++++++++++++++++++++++++--------
>  src/qemu/qemu_process.h   |    3 ++-
>  6 files changed, 55 insertions(+), 28 deletions(-)
> 
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 3eeea4b..a6162b6 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -773,7 +773,8 @@ cleanup:
>   */
>  int
>  qemuAddSharedDisk(virHashTablePtr sharedDisks,
> -                  virDomainDiskDefPtr disk)
> +                  virDomainDiskDefPtr disk,
> +                  int *added)
>  {
>      size_t *ref = NULL;
>      char *key = NULL;
> @@ -804,6 +805,8 @@ qemuAddSharedDisk(virHashTablePtr sharedDisks,
>          }
>      }
>  
> +    if (added)
> +        *added = 1;
>      VIR_FREE(key);
>      return 0;
>  }
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index 8e84bcf..b8b5275 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -270,7 +270,8 @@ void qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,
>                                     virConnectPtr conn);
>  
>  int qemuAddSharedDisk(virHashTablePtr sharedDisks,
> -                      virDomainDiskDefPtr disk)
> +                      virDomainDiskDefPtr disk,
> +                      int *added)
>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>  
>  int qemuRemoveSharedDisk(virHashTablePtr sharedDisks,
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 1dc9789..03fe526 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -2098,7 +2098,7 @@ qemuDomainDestroyFlags(virDomainPtr dom,
>          goto endjob;
>      }
>  
> -    qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED, 0);
> +    qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED, 0, NULL);
>      event = virDomainEventNewFromObj(vm,
>                                       VIR_DOMAIN_EVENT_STOPPED,
>                                       VIR_DOMAIN_EVENT_STOPPED_DESTROYED);
> @@ -2944,7 +2944,7 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom,
>          goto endjob;
>  
>      /* Shut it down */
> -    qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SAVED, 0);
> +    qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SAVED, 0, NULL);
>      virDomainAuditStop(vm, "saved");
>      event = virDomainEventNewFromObj(vm,
>                                       VIR_DOMAIN_EVENT_STOPPED,
> @@ -3393,7 +3393,7 @@ static int qemuDomainCoreDump(virDomainPtr dom,
>  
>  endjob:
>      if ((ret == 0) && (flags & VIR_DUMP_CRASH)) {
> -        qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_CRASHED, 0);
> +        qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_CRASHED, 0, NULL);
>          virDomainAuditStop(vm, "crashed");
>          event = virDomainEventNewFromObj(vm,
>                                           VIR_DOMAIN_EVENT_STOPPED,
> @@ -4902,7 +4902,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
>          }
>  
>          if (virCommandWait(cmd, NULL) < 0) {
> -            qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0);
> +            qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0, NULL);
>              ret = -1;
>          }
>          VIR_DEBUG("Decompression binary stderr: %s", NULLSTR(errbuf));
> @@ -5850,7 +5850,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
>      }
>  
>      if (ret == 0) {
> -        if (qemuAddSharedDisk(driver->sharedDisks, disk) < 0)
> +        if (qemuAddSharedDisk(driver->sharedDisks, disk, NULL) < 0)
>              VIR_WARN("Failed to add disk '%s' to shared disk table",
>                       disk->src);
>  
> @@ -10677,7 +10677,7 @@ qemuDomainSnapshotCreateActiveInternal(virConnectPtr conn,
>      if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT) {
>          event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
>                                           VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT);
> -        qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0);
> +        qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0, NULL);
>          virDomainAuditStop(vm, "from-snapshot");
>          /* We already filtered the _HALT flag for persistent domains
>           * only, so this end job never drops the last reference.  */
> @@ -11232,7 +11232,7 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn,
>  
>          event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
>                                           VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT);
> -        qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0);
> +        qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0, NULL);
>          virDomainAuditStop(vm, "from-snapshot");
>          /* We already filtered the _HALT flag for persistent domains
>           * only, so this end job never drops the last reference.  */
> @@ -12151,8 +12151,8 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>                      goto endjob;
>                  }
>                  virResetError(err);
> -                qemuProcessStop(driver, vm,
> -                                VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0);
> +                qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT,
> +                                0, NULL);
>                  virDomainAuditStop(vm, "from-snapshot");
>                  detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT;
>                  event = virDomainEventNewFromObj(vm,
> @@ -12265,7 +12265,8 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>  
>          if (virDomainObjIsActive(vm)) {
>              /* Transitions 4, 7 */
> -            qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0);
> +            qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT,
> +                            0, NULL);
>              virDomainAuditStop(vm, "from-snapshot");
>              detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT;
>              event = virDomainEventNewFromObj(vm,
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index a75fb4e..365afe3 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -1692,7 +1692,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
>              virReportSystemError(errno, "%s",
>                                   _("cannot pass pipe for tunnelled migration"));
>              virDomainAuditStart(vm, "migrated", false);
> -            qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0);
> +            qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0, NULL);
>              goto endjob;
>          }
>          dataFD[1] = -1; /* 'st' owns the FD now & will close it */
> @@ -3057,7 +3057,7 @@ qemuMigrationPerformJob(virQEMUDriverPtr driver,
>       */
>      if (!v3proto) {
>          qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_MIGRATED,
> -                        VIR_QEMU_PROCESS_STOP_MIGRATED);
> +                        VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
>          virDomainAuditStop(vm, "migrated");
>          event = virDomainEventNewFromObj(vm,
>                                           VIR_DOMAIN_EVENT_STOPPED,
> @@ -3359,7 +3359,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
>          if (!(flags & VIR_MIGRATE_OFFLINE)) {
>              if (qemuMigrationVPAssociatePortProfiles(vm->def) < 0) {
>                  qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
> -                                VIR_QEMU_PROCESS_STOP_MIGRATED);
> +                                VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
>                  virDomainAuditStop(vm, "failed");
>                  event = virDomainEventNewFromObj(vm,
>                                                   VIR_DOMAIN_EVENT_STOPPED,
> @@ -3398,7 +3398,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
>                  if (v3proto) {
>                      if (!(flags & VIR_MIGRATE_OFFLINE)) {
>                          qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
> -                                        VIR_QEMU_PROCESS_STOP_MIGRATED);
> +                                        VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
>                          virDomainAuditStop(vm, "failed");
>                      }
>                      if (newVM)
> @@ -3446,7 +3446,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
>                   */
>                  if (v3proto) {
>                      qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
> -                                    VIR_QEMU_PROCESS_STOP_MIGRATED);
> +                                    VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
>                      virDomainAuditStop(vm, "failed");
>                      event = virDomainEventNewFromObj(vm,
>                                                       VIR_DOMAIN_EVENT_STOPPED,
> @@ -3483,7 +3483,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
>          qemuProcessAutoDestroyRemove(driver, vm);
>      } else if (!(flags & VIR_MIGRATE_OFFLINE)) {
>          qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
> -                        VIR_QEMU_PROCESS_STOP_MIGRATED);
> +                        VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
>          virDomainAuditStop(vm, "failed");
>          event = virDomainEventNewFromObj(vm,
>                                           VIR_DOMAIN_EVENT_STOPPED,
> @@ -3554,7 +3554,7 @@ int qemuMigrationConfirm(virQEMUDriverPtr driver,
>       */
>      if (retcode == 0) {
>          qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_MIGRATED,
> -                        VIR_QEMU_PROCESS_STOP_MIGRATED);
> +                        VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
>          virDomainAuditStop(vm, "migrated");
>  
>          event = virDomainEventNewFromObj(vm,
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 1e0876c..f820c57 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -333,7 +333,7 @@ qemuProcessHandleMonitorEOF(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
>      event = virDomainEventNewFromObj(vm,
>                                       VIR_DOMAIN_EVENT_STOPPED,
>                                       eventReason);
> -    qemuProcessStop(driver, vm, stopReason, 0);
> +    qemuProcessStop(driver, vm, stopReason, 0, NULL);
>      virDomainAuditStop(vm, auditReason);
>  
>      if (!vm->persistent) {
> @@ -3340,7 +3340,7 @@ error:
>                   * really is and FAILED means "failed to start" */
>                  state = VIR_DOMAIN_SHUTOFF_UNKNOWN;
>              }
> -            qemuProcessStop(driver, obj, state, 0);
> +            qemuProcessStop(driver, obj, state, 0, NULL);
>              if (!obj->persistent)
>                  qemuDomainRemoveInactive(driver, obj);
>              else
> @@ -3417,7 +3417,8 @@ qemuProcessReconnectHelper(virDomainObjPtr obj,
>          } else if (virObjectUnref(obj)) {
>             /* We can't spawn a thread and thus connect to monitor.
>              * Kill qemu */
> -            qemuProcessStop(src->driver, obj, VIR_DOMAIN_SHUTOFF_FAILED, 0);
> +            qemuProcessStop(src->driver, obj,
> +                            VIR_DOMAIN_SHUTOFF_FAILED, 0, NULL);
>              if (!obj->persistent)
>                  qemuDomainRemoveInactive(src->driver, obj);
>              else
> @@ -3507,6 +3508,7 @@ int qemuProcessStart(virConnectPtr conn,
>      virBitmapPtr nodemask = NULL;
>      unsigned int stop_flags;
>      virQEMUDriverConfigPtr cfg;
> +    virBitmapPtr added_disks = NULL;
>  
>      /* Okay, these are just internal flags,
>       * but doesn't hurt to check */
> @@ -3820,9 +3822,15 @@ int qemuProcessStart(virConnectPtr conn,
>      if (cfg->clearEmulatorCapabilities)
>          virCommandClearCaps(cmd);
>  
> +    if (!(added_disks = virBitmapNew(vm->def->ndisks))) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }
> +
>      /* in case a certain disk is desirous of CAP_SYS_RAWIO, add this */
>      for (i = 0; i < vm->def->ndisks; i++) {
>          virDomainDiskDefPtr disk = vm->def->disks[i];
> +        int added;
>  
>          if (vm->def->disks[i]->rawio == 1)
>  #ifdef CAP_SYS_RAWIO
> @@ -3832,8 +3840,10 @@ int qemuProcessStart(virConnectPtr conn,
>                             _("Raw I/O is not supported on this platform"));
>  #endif
>  
> -        if (qemuAddSharedDisk(driver->sharedDisks, disk) < 0)
> +        if (qemuAddSharedDisk(driver->sharedDisks, disk, &added) < 0)
>              goto cleanup;
> +        if (added)
> +            ignore_value(virBitmapSetBit(added_disks, i));
>  
>          if (qemuSetUnprivSGIO(disk) < 0)
>              goto cleanup;
> @@ -4049,6 +4059,7 @@ int qemuProcessStart(virConnectPtr conn,
>      }
>  
>      virCommandFree(cmd);
> +    virBitmapFree(added_disks);
>      VIR_FORCE_CLOSE(logfile);
>      virObjectUnref(cfg);
>  
> @@ -4062,7 +4073,8 @@ cleanup:
>      virBitmapFree(nodemask);
>      virCommandFree(cmd);
>      VIR_FORCE_CLOSE(logfile);
> -    qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, stop_flags);
> +    qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
> +                    stop_flags, added_disks);

Do your virBitmapFree(added_disks) here not in called function...

>      virObjectUnref(cfg);
>  
>      return -1;
> @@ -4113,7 +4125,8 @@ qemuProcessKill(virQEMUDriverPtr driver,
>  void qemuProcessStop(virQEMUDriverPtr driver,
>                       virDomainObjPtr vm,
>                       virDomainShutoffReason reason,
> -                     unsigned int flags)
> +                     unsigned int flags,
> +                     virBitmapPtr added_disks)
>  {
>      int ret;
>      int retries = 0;
> @@ -4239,9 +4252,17 @@ void qemuProcessStop(virQEMUDriverPtr driver,
>  
>      for (i = 0; i < vm->def->ndisks; i++) {
>          virDomainDiskDefPtr disk = vm->def->disks[i];
> +        bool is_added;

Initialize to false, since result can be unchanged if :

int virBitmapGetBit(virBitmapPtr bitmap, size_t b, bool *result)
{
    if (bitmap->max_bit <= b)
        return -1;

>  
> -        ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk));
> +        if (added_disks) {
> +            ignore_value(virBitmapGetBit(added_disks, i, &is_added));
> +            if (is_added)
> +                ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk));
> +        } else {
> +            ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk));
> +        }
>      }
> +    virBitmapFree(added_disks);

Let the caller do this.  Think of it this way, if you make this call -
the local added_disks ends up NULL, but that is not reflected back in
the caller since this was passed by value and not reference.  For some
this is a coding style - caller did the alloc and it should do the free.

>  
>      /* Clear out dynamically assigned labels */
>      for (i = 0; i < vm->def->nseclabels; i++) {
> @@ -4594,7 +4615,7 @@ qemuProcessAutoDestroy(virQEMUDriverPtr driver,
>  
>      VIR_DEBUG("Killing domain");
>      qemuProcessStop(driver, dom, VIR_DOMAIN_SHUTOFF_DESTROYED,
> -                    VIR_QEMU_PROCESS_STOP_MIGRATED);
> +                    VIR_QEMU_PROCESS_STOP_MIGRATED, NULL);
>      virDomainAuditStop(dom, "destroyed");
>      event = virDomainEventNewFromObj(dom,
>                                       VIR_DOMAIN_EVENT_STOPPED,
> diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
> index 7c620d4..c4f8a6a 100644
> --- a/src/qemu/qemu_process.h
> +++ b/src/qemu/qemu_process.h
> @@ -68,7 +68,8 @@ typedef enum {
>  void qemuProcessStop(virQEMUDriverPtr driver,
>                       virDomainObjPtr vm,
>                       virDomainShutoffReason reason,
> -                     unsigned int flags);
> +                     unsigned int flags,
> +                     virBitmapPtr added_disks);
>  
>  int qemuProcessAttach(virConnectPtr conn,
>                        virQEMUDriverPtr driver,
> 

This is quite a bit of churn for the one place you needed the bitmap.
Would there be need/cause to add a field to the driver struct with the
shared disk bitmap rather than a parameter to qemuProcessStop(). Perhaps
there'd be other uses for it too (just thinking out loud late on snowy
Friday afternoon).

John


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