On 2013年02月09日 04:46, John Ferlan wrote:
On 02/08/2013 08:08 AM, Osier Yang wrote:Based on moving various checking into qemuAddSharedDisk, this avoids the caller using it in wrong ways. --- src/qemu/qemu_conf.c | 50 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_driver.c | 5 ---- src/qemu/qemu_process.c | 53 ----------------------------------------------- src/qemu/qemu_process.h | 3 -- 4 files changed, 50 insertions(+), 61 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 69ba710..3eeea4b 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -721,6 +721,53 @@ qemuGetSharedDiskKey(const char *disk_path) return key; } +/* Check if a shared disk's setting conflicts with the conf + * used by other domain(s). Currently only checks the sgio + * setting. Note that this should only be called for disk with + * block source. + * + * Returns 0 if no conflicts, otherwise returns -1. + */ +static int +qemuCheckSharedDisk(virHashTablePtr sharedDisks, + virDomainDiskDefPtr disk) +{ + int val; + size_t *ref = NULL; + char *key = NULL; + int ret = 0; + + if (!(key = qemuGetSharedDiskKey(disk->src))) + return -1; + + /* It can't be conflict if no other domain is + * is sharing it. + */ + if (!(ref = virHashLookup(sharedDisks, key))) + goto cleanup; + + if (virGetDeviceUnprivSGIO(disk->src, NULL,&val)< 0) { + ret = -1; + goto cleanup; + } + + if ((val == 0&& + (disk->sgio == VIR_DOMAIN_DISK_SGIO_FILTERED || + disk->sgio == VIR_DOMAIN_DISK_SGIO_DEFAULT)) || + (val == 1&& + disk->sgio == VIR_DOMAIN_DISK_SGIO_UNFILTERED)) + goto cleanup; + + virReportError(VIR_ERR_OPERATION_INVALID, + _("sgio of shared disk '%s' conflicts with other " + "active domains"), disk->src); + ret = -1; + +cleanup: + VIR_FREE(key); + return ret; +} + /* Increase ref count if the entry already exists, otherwise * add a new entry. */ @@ -739,6 +786,9 @@ qemuAddSharedDisk(virHashTablePtr sharedDisks, !disk->shared || !disk->src) return 0; + if (qemuCheckSharedDisk(sharedDisks, disk)< 0) + return -1; +I take the order previously in qemuProcessStart() was wrong then - it was adding it first, then checking if it was shared. The DiskLive code checked shared first, then added, which does seem more correct.
See PATCH 3/4, the reason for adding it first, and then checking, in qemuProcessStart, is to avoid removing the hash entry which belongs to other domain(s) when doing cleanup for failing on starting. But it has flaws, because it still could removes the hash entry belongs to other domain(s) if it fails before the block to add the hash entry && set unpriv_sgio. The reason for checking first, and then adding in DiskLive is that there is no risk to remove hash entry of other domains. But it also has flaws for [1], which works for qemuProcessStart, but not for DiskLive. I have ever added a new argument to the checking method so that it knowns whether should return 0 when the "ref" count is 1 in a previous version. But it's not that nice, and I changed it by using a bitmap for qemuProcessStop PATCH 3/4 of this version.
However, the change is subtle enough to make me wonder about the reason for the ordering of calls in the DiskLive code that would check shared before calls to qemuDomainDetermineDiskChain() and qemuSetupDiskCgroup()
There is no special reason, we can also put the checking after these calls. Just I think checking first is more effective in case of there is error when checking.
Seems as though now the code in DiskLive may need some extra backout code if the add now fails because of the check
It's done in PATCH 4/4.
if (!(key = qemuGetSharedDiskKey(disk->src))) return -1; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 043efd3..1dc9789 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5798,11 +5798,6 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, goto end; } - if (disk->type == VIR_DOMAIN_DISK_TYPE_BLOCK&& - disk->shared&& - (qemuCheckSharedDisk(driver->sharedDisks, disk)< 0)) - goto end; - if (qemuDomainDetermineDiskChain(driver, disk, false)< 0) goto end; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index bc171a4..1e0876c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3483,56 +3483,6 @@ qemuSetUnprivSGIO(virDomainDiskDefPtr disk) return 0; } -/* Check if a shared disk's setting conflicts with the conf - * used by other domain(s). Currently only checks the sgio - * setting. Note that this should only be called for disk with - * block source. - * - * Returns 0 if no conflicts, otherwise returns -1. - */ -int -qemuCheckSharedDisk(virHashTablePtr sharedDisks, - virDomainDiskDefPtr disk) -{ - int val; - size_t *ref = NULL; - char *key = NULL; - int ret = 0; - - if (!(key = qemuGetSharedDiskKey(disk->src))) - return -1; - - /* It can't be conflict if no other domain is - * is sharing it. - */ - if (!(ref = virHashLookup(sharedDisks, key))) - goto cleanup; - - if (ref == (void *)0x1) - goto cleanup;
[1]
- - if (virGetDeviceUnprivSGIO(disk->src, NULL,&val)< 0) { - ret = -1; - goto cleanup; - } - - if ((val == 0&& - (disk->sgio == VIR_DOMAIN_DISK_SGIO_FILTERED || - disk->sgio == VIR_DOMAIN_DISK_SGIO_DEFAULT)) || - (val == 1&& - disk->sgio == VIR_DOMAIN_DISK_SGIO_UNFILTERED)) - goto cleanup; - - virReportError(VIR_ERR_OPERATION_INVALID, - _("sgio of shared disk '%s' conflicts with other " - "active domains"), disk->src); - ret = -1; - -cleanup: - VIR_FREE(key); - return ret; -} - int qemuProcessStart(virConnectPtr conn, virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -3885,9 +3835,6 @@ int qemuProcessStart(virConnectPtr conn, if (qemuAddSharedDisk(driver->sharedDisks, disk)< 0) goto cleanup; - if (qemuCheckSharedDisk(driver->sharedDisks, disk)< 0) - goto cleanup; - if (qemuSetUnprivSGIO(disk)< 0) goto cleanup; } diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 2dc8041..7c620d4 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -100,7 +100,4 @@ virBitmapPtr qemuPrepareCpumap(virQEMUDriverPtr driver, virBitmapPtr nodemask); int qemuSetUnprivSGIO(virDomainDiskDefPtr disk); -int qemuCheckSharedDisk(virHashTablePtr sharedDisks, - virDomainDiskDefPtr disk); - #endif /* __QEMU_PROCESS_H__ */Need to understand ramifications of change before ACK I think. Unless someone else comes in after me with a deeper understanding and says the order is fine.
Thanks for the careful reviewing, the details is harry. Osier -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list