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].
} return ret; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 30f923a..bc171a4 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3453,6 +3453,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;
[1]
+ if (disk->sgio) val = (disk->sgio == VIR_DOMAIN_DISK_SGIO_UNFILTERED); @@ -3875,13 +3882,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->sharedDisks, disk->src)< 0) - goto cleanup; + if (qemuAddSharedDisk(driver->sharedDisks, disk)< 0) + goto cleanup; - if (qemuCheckSharedDisk(driver->sharedDisks, disk)< 0) - goto cleanup; - } + if (qemuCheckSharedDisk(driver->sharedDisks, disk)< 0) + goto cleanup; if (qemuSetUnprivSGIO(disk)< 0) goto cleanup; @@ -4288,9 +4293,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->sharedDisks, disk->src)); - } + ignore_value(qemuRemoveSharedDisk(driver->sharedDisks, disk)); } /* Clear out dynamically assigned labels */ACK - everything seems straightforward to me. John -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list