[libvirt] [PATCH 10/15] qemu: Error out when domain starting if the cdbfilter setting conflicts

[Jiri Denemark]

On Wed, Dec 05, 2012 at 17:25:11 +0800, 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".
> 
...
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 05f8622..2938a65 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3712,12 +3712,56 @@ int qemuProcessStart(virConnectPtr conn,
>          if (disk->rawio == 1)
>              virCommandAllowCap(cmd, CAP_SYS_RAWIO);
>  
> -        /* Add to qemud_driver->sharedDisks list if the disk is shared */
> -        if (disk->shared &&
> -            (qemuSharedDiskListAdd(driver->sharedDisks,
> -                                   disk->src,
> -                                   vm->def->name) < 0)) {
> -            goto cleanup;
> +        if (disk->shared) {
> +            /* Error out if the cdbfilter setting is different with what
> +             * other domain(s) uses.
> +             */
> +            qemuSharedDiskPtr entry = NULL;
> +
> +            if ((entry = qemuSharedDiskListFind(driver->sharedDisks,
> +                                                disk->src,
> +                                                NULL,
> +                                                NULL))) {
> +                virDomainObjUnlock(vm);
> +                for (i = 0; i < entry->ndomains; i++) {
> +                    virDomainObjPtr domobj = NULL;
> +                    virDomainDiskDefPtr diskdef = NULL;
> +
> +                    if (!(domobj = virDomainFindByName(&driver->domains,
> +                                                       entry->domains[i]))) {
> +                        virDomainObjLock(vm);
> +                        goto cleanup;
> +                    }
> +
> +                    if (!(diskdef = virDomainDiskFindByPath(domobj->def,
> +                                                            disk->src))) {
> +                        virDomainObjUnlock(domobj);
> +                        virDomainObjLock(vm);
> +                        goto cleanup;
> +                    }
> +
> +                    /* XXX: Can be abstracted into a function when there
> +                     * are more stuffs to check in future.
> +                     */
> +                    if (diskdef->cdbfilter != disk->cdbfilter) {
> +                        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                                       _("cdbfilter of shared disk '%s' "
> +                                         "conflicts with other active "
> +                                         "domains"), disk->src);
> +                        virDomainObjUnlock(domobj);
> +                        virDomainObjLock(vm);
> +                        goto cleanup;
> +                    }
> +                    virDomainObjUnlock(domobj);
> +                }
> +                virDomainObjLock(vm);
> +            }

NACK. This is still doing what Daniel complained about in v1. Although it does
not go through all domains it is still locking some other domains. Not to
mention that it is absolute overkill to do. IIUC, all domains in entry-domains
has to have exactly the same cdbfilter otherwise libvirt would refuse to start
them. And why do we even need to maintain a list of domains sharing the same
disk? Wouldn't just storing the value of cdbfilter there be enough? It seems
like it would, at least for this purpose of checking that it matches cdbfilter
specified in current domain.

Jirka