On 02/13/2013 09:57 AM, Osier Yang wrote:
This moves the various 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 | 14 +++++--------- src/qemu/qemu_process.c | 22 ++++++++++++---------- 4 files changed, 35 insertions(+), 25 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 2bd1931..d0ee80b 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -826,13 +826,21 @@ qemuGetSharedDiskKey(const char *disk_path) */ int qemuAddSharedDisk(virQEMUDriverPtr driver, - const char *disk_path) + virDomainDiskDefPtr disk) { size_t *ref = NULL; char *key = NULL; int ret = -1;
- 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))) goto cleanup;
if ((ref = virHashLookup(driver->sharedDisks, key))) { @@ -854,13 +862,17 @@ cleanup: */ int qemuRemoveSharedDisk(virQEMUDriverPtr driver, - const char *disk_path) + virDomainDiskDefPtr disk) { size_t *ref = NULL; char *key = NULL; int ret = -1;
- if (!(key = qemuGetSharedDiskKey(disk_path))) + if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK || + !disk->shared || !disk->src) + return 0; + + if (!(key = qemuGetSharedDiskKey(disk->src))) goto cleanup;
if (!(ref = virHashLookup(driver->sharedDisks, key))) diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 09eacce..1685cf6 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -276,11 +276,11 @@ void qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver, virConnectPtr conn);
int qemuAddSharedDisk(virQEMUDriverPtr driver, - const char *disk_path) + virDomainDiskDefPtr disk) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
int qemuRemoveSharedDisk(virQEMUDriverPtr driver, - 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 a4fc0c9..f98bd9b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5882,11 +5882,9 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, }
if (ret == 0) { - if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK && disk->shared) { - if (qemuAddSharedDisk(driver, disk->src) < 0) - VIR_WARN("Failed to add disk '%s' to shared disk table", - disk->src); - } + if (qemuAddSharedDisk(driver, disk) < 0) + VIR_WARN("Failed to add disk '%s' to shared disk table", + disk->src);
Are we assuming disk->src is not NULL here? IOW, do we need a NULLSTR() around this? I cannot remember from prior review, but I do see in the code just above this set of lines a NULLSTR(disk->src).
if (qemuSetUnprivSGIO(disk) < 0) VIR_WARN("Failed to set unpriv_sgio of disk '%s'", disk->src); @@ -6007,10 +6005,8 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver, break; }
- if (ret == 0 && - disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK && - disk->shared) { - if (qemuRemoveSharedDisk(driver, disk->src) < 0) + if (ret == 0) { + if (qemuRemoveSharedDisk(driver, disk) < 0) VIR_WARN("Failed to remove disk '%s' from shared disk table", disk->src);
Same comment
} diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 9759332..8425cbb 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3526,6 +3526,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; + if (disk->sgio) val = (disk->sgio == VIR_DOMAIN_DISK_SGIO_UNFILTERED);
@@ -3952,13 +3959,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, disk->src) < 0) - goto cleanup; + if (qemuAddSharedDisk(driver, disk) < 0) + goto cleanup;
- if (qemuCheckSharedDisk(driver, disk) < 0) - goto cleanup; - } + if (qemuCheckSharedDisk(driver, disk) < 0) + goto cleanup;
if (qemuSetUnprivSGIO(disk) < 0) goto cleanup; @@ -4366,10 +4371,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, disk->src)); - } + ignore_value(qemuRemoveSharedDisk(driver, disk)); }
/* Clear out dynamically assigned labels */