[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 2013年02月09日 05:09, John Ferlan wrote:
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...

Agreed.


      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;

Okay.



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

Agreed, freeing after call is more clear.



      /* 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).

Don't agree. As it's only used by qemuProcessStart. And it's per-VM
basis, not worth to maintain it in the driver life cycle.

Osier


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