Re: [libvirt] [PATCH 6/8 v8] qemu: Check if the shared disk's cdbfilter conflicts with others
On 2012年12月15日 05:49, Eric Blake wrote:
On 12/14/2012 08:44 AM, Osier Yang wrote:
> This prevents the domain starting if the shared disk's setting
> conflicts with other active domain(s), E.g. A domain with
> "cdbfilter" set as "yes", however, another active domain is
using
> it set as "no".
> ---
> src/qemu/qemu_process.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_process.h | 2 +
> 2 files changed, 53 insertions(+), 0 deletions(-)
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index bcea0ff..1e2baac 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3385,6 +3385,54 @@ qemuProcessReconnectAll(virConnectPtr conn, virQEMUDriverPtr
driver)
> virHashForEach(driver->domains.objs, qemuProcessReconnectHelper,&data);
> }
>
> +/* Check if a shared disk's setting conflicts with the conf
> + * used by other domain(s). Currently only checks the cdbfilter
> + * setting.
Probably worth documenting that this function should only be called on
disks with a block device 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;
since you error out on regular files. (You correctly obey that
restriction for now, but without documentation, it might be possible to
reuse this function from the wrong context in a future patch.)
Okay.
> +
> + /* 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;
> +
> + if (virGetDeviceUnprivSGIO(disk->src, NULL,&val)< 0) {
> + ret = -1;
> + goto cleanup;
> + }
> +
> + if ((val == 0&&
> + disk->cdbfilter == VIR_DOMAIN_DISK_CDB_FILTER_YES) ||
Does this do the right thing if disk->cdbfilter ==
VIR_DOMAIN_DISK_CDB_FILTER_DEFAULT (that is, the user didn't specify the
cdbfilter attribute)?
Oh, right, there is problem here. It should be like:
if (!disk->cdbfilter)
return 0
in the early beginnng of the function.
As VIR_DOMAIN_DISK_CDB_FILTER_DEFAULT implies it doesn't care
about the SG_IO setting. I.E, It lives with what current setting
is.
Again, this area of code may need tweaks if you
take my suggestion of making rawio a three-value enum.
> + (val == 1&&
> + disk->cdbfilter == VIR_DOMAIN_DISK_CDB_FILTER_NO))
> + goto cleanup;
> +
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("cdbfilter of shared disk '%s' conflicts with
other "
> + "active domains"), disk->src);
INTERNAL_ERROR is wrong. I think VIR_ERR_OPERATION_INVALID fits better.
Okay, the error number is always fuzzy to use. :-)