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

[Daniel P. Berrange]

On Mon, Feb 11, 2013 at 06:35:42PM +0800, Osier Yang wrote:
> 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].

If disk->type == NETWORK, then de-referencing disk->src has potential to
SEGV the entire process, since that field is not valid.

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

Again you must *not* de-reference disk->src without first validating
the disk->type value.


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|