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

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



On Mon, Feb 11, 2013 at 07:09:29PM +0800, Osier Yang wrote:
> On 2013年02月11日 18:48, Daniel P. Berrange wrote:
> >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.
> 
> There is checking in this version:
> 
>     /* "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;
> 
> So for a network disk, it has no chance to de-reference disk->src
> if the return value < 0.

Hmm, that is rather unclear, and looking at the code is also just
something we don't need. These methods are doing virRaiseError,
so we shouldn't also be doing VIR_WARN - just remove these VIR_WARN
lines from all this code.

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


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