Re: [PATCH 1/4] qemu: Add checking in helpers for sgio setting

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

 



On 2013年02月09日 04:21, John Ferlan wrote:
On 02/08/2013 08:07 AM, Osier Yang wrote:
This moves the checking into the helpers, to avoid the
callers missing the checking.
---
  src/qemu/qemu_conf.c    |   20 ++++++++++++++++----
  src/qemu/qemu_conf.h    |    4 ++--
  src/qemu/qemu_driver.c  |   18 +++++++-----------
  src/qemu/qemu_process.c |   21 ++++++++++++---------
  4 files changed, 37 insertions(+), 26 deletions(-)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 17f7d45..69ba710 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -726,12 +726,20 @@ qemuGetSharedDiskKey(const char *disk_path)
   */
  int
  qemuAddSharedDisk(virHashTablePtr sharedDisks,
-                  const char *disk_path)
+                  virDomainDiskDefPtr disk)
  {
      size_t *ref = NULL;
      char *key = NULL;

-    if (!(key = qemuGetSharedDiskKey(disk_path)))
+    /* Currently the only conflicts we have to care about
+     * for the shared disk is "sgio" setting, which is only
+     * valid for block disk.
+     */
+    if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK ||
+        !disk->shared || !disk->src)
+        return 0;
+
+    if (!(key = qemuGetSharedDiskKey(disk->src)))
          return -1;

      if ((ref = virHashLookup(sharedDisks, key))) {
@@ -755,12 +763,16 @@ qemuAddSharedDisk(virHashTablePtr sharedDisks,
   */
  int
  qemuRemoveSharedDisk(virHashTablePtr sharedDisks,
-                     const char *disk_path)
+                     virDomainDiskDefPtr disk)
  {
      size_t *ref = NULL;
      char *key = NULL;

-    if (!(key = qemuGetSharedDiskKey(disk_path)))
+    if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK ||
+        !disk->shared || !disk->src)
+        return 0;

[2]

+
+    if (!(key = qemuGetSharedDiskKey(disk->src)))
          return -1;

      if (!(ref = virHashLookup(sharedDisks, key))) {
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index 60c4109..8e84bcf 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -270,11 +270,11 @@ void qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,
                                     virConnectPtr conn);

  int qemuAddSharedDisk(virHashTablePtr sharedDisks,
-                      const char *disk_path)
+                      virDomainDiskDefPtr disk)
      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

  int qemuRemoveSharedDisk(virHashTablePtr sharedDisks,
-                         const char *disk_path)
+                         virDomainDiskDefPtr disk)
      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
  char * qemuGetSharedDiskKey(const char *disk_path)
      ATTRIBUTE_NONNULL(1);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 979a027..043efd3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -5855,11 +5855,9 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
      }

      if (ret == 0) {
-        if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK&&  disk->shared) {
-            if (qemuAddSharedDisk(driver->sharedDisks, disk->src)<  0)
-                VIR_WARN("Failed to add disk '%s' to shared disk table",
-                         disk->src);
-        }
+        if (qemuAddSharedDisk(driver->sharedDisks, disk)<  0)
+            VIR_WARN("Failed to add disk '%s' to shared disk table",
+                     disk->src);

          if (qemuSetUnprivSGIO(disk)<  0)
              VIR_WARN("Failed to set unpriv_sgio of disk '%s'", disk->src);

Does there need to be a NULLSTR(disk->src)?  Doesn't seem so, but just
checking.  I only note this because the qemuAddSharedDisk() has a
!disk->src check prior to returning zero.

qemuSetUnprivSGIO returns 0 as long as disk->src is NULL too. See [1].


@@ -5980,12 +5978,10 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver,
          break;
      }

-    if (ret == 0&&
-        disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK&&
-        disk->shared) {
-        if (qemuRemoveSharedDisk(driver->sharedDisks, disk->src)<  0)
-             VIR_WARN("Failed to remove disk '%s' from shared disk table",
-                      disk->src);
+    if (ret == 0) {
+        if (qemuRemoveSharedDisk(driver->sharedDisks, disk)<  0)
+            VIR_WARN("Failed to remove disk '%s' from shared disk table",
+                     disk->src);

Similar comment here w/r/t NULLSTR and checks in Remove code

Likewise. See [2].


      }

      return ret;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 30f923a..bc171a4 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3453,6 +3453,13 @@ qemuSetUnprivSGIO(virDomainDiskDefPtr disk)
  {
      int val = -1;

+    /* "sgio" is only valid for block disk; cdrom
+     * and floopy disk can have empty source.
+     */
+    if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK ||
+        !disk->src)
+        return 0;

[1]

+
      if (disk->sgio)
          val = (disk->sgio == VIR_DOMAIN_DISK_SGIO_UNFILTERED);

@@ -3875,13 +3882,11 @@ int qemuProcessStart(virConnectPtr conn,
                             _("Raw I/O is not supported on this platform"));
  #endif

-        if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK&&  disk->shared) {
-            if (qemuAddSharedDisk(driver->sharedDisks, disk->src)<  0)
-                goto cleanup;
+        if (qemuAddSharedDisk(driver->sharedDisks, disk)<  0)
+            goto cleanup;

-            if (qemuCheckSharedDisk(driver->sharedDisks, disk)<  0)
-                goto cleanup;
-        }
+        if (qemuCheckSharedDisk(driver->sharedDisks, disk)<  0)
+            goto cleanup;

          if (qemuSetUnprivSGIO(disk)<  0)
              goto cleanup;
@@ -4288,9 +4293,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,
      for (i = 0; i<  vm->def->ndisks; i++) {
          virDomainDiskDefPtr disk = vm->def->disks[i];

-        if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK&&  disk->shared) {
-            ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk->src));
-        }
+        ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk));
      }

      /* Clear out dynamically assigned labels */


ACK - everything seems straightforward to me.

John

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

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