4:50 a.m.
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