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

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.