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

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