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

[Osier Yang]

On 2012年12月04日 21:17, 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/conf/domain_conf.h: (Declare helper virDomainDiskFindByPath)
> * src/conf/domain_conf.c: (Implement virDomainDiskFindByPath)
> * src/libvirt_private.syms (export virDomainDiskFindByPath)
> * src/qemu/qemu_process.c: (Error out if the shared disk's cdbfilter
>                              conflicts with others)
> ---
>   src/conf/domain_conf.c   |   13 ++++++++++++
>   src/conf/domain_conf.h   |    2 +
>   src/libvirt_private.syms |    1 +
>   src/qemu/qemu_process.c  |   50 ++++++++++++++++++++++++++++++++++++++++-----
>   4 files changed, 60 insertions(+), 6 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 50d853e..1d6bb1f 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3062,6 +3062,19 @@ virDomainDiskFindControllerModel(virDomainDefPtr def,
>       return model;
>   }
>
> +virDomainDiskDefPtr
> +virDomainDiskFindByPath(virDomainDefPtr def,
> +                        const char *path)
> +{
> +    int i;
> +
> +    for (i = 0; i<  def->ndisks; i++)
> +        if (STREQ_NULLABLE(def->disks[i]->src, path))
> +            return def->disks[i];
> +
> +    return NULL;
> +}
> +
>   int
>   virDomainDiskDefAssignAddress(virCapsPtr caps, virDomainDiskDefPtr def)
>   {
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index d7c9b6b..2f24a3f 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1897,6 +1897,8 @@ void virDomainDiskHostDefFree(virDomainDiskHostDefPtr def);
>   int virDomainDiskFindControllerModel(virDomainDefPtr def,
>                                        virDomainDiskDefPtr disk,
>                                        int controllerType);
> +virDomainDiskDefPtr virDomainDiskFindByPath(virDomainDefPtr def,
> +                                            const char *path);
>   void virDomainControllerDefFree(virDomainControllerDefPtr def);
>   void virDomainFSDefFree(virDomainFSDefPtr def);
>   void virDomainActualNetDefFree(virDomainActualNetDefPtr def);
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index c2b01b9..41e4143 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -352,6 +352,7 @@ virDomainDiskDefFree;
>   virDomainDiskDeviceTypeToString;
>   virDomainDiskErrorPolicyTypeFromString;
>   virDomainDiskErrorPolicyTypeToString;
> +virDomainDiskFindByPath;
>   virDomainDiskFindControllerModel;
>   virDomainDiskGeometryTransTypeFromString;
>   virDomainDiskGeometryTransTypeToString;
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 5cb068e..fac7d57 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3704,12 +3704,50 @@ 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))) {
> +                for (i = 0; i<  entry->ndomains; i++) {
> +                    virDomainObjPtr domobj = NULL;
> +                    virDomainDiskDefPtr diskdef = NULL;
> +
> +                    if (!(domobj = virDomainFindByName(&driver->domains,
> +                                                       entry->domains[i])))
> +                        goto cleanup;
> +
> +                    if (!(diskdef = virDomainDiskFindByPath(domobj->def,
> +                                                            disk->src))) {
> +                        virDomainObjUnlock(domobj);
> +                        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);
> +                        goto cleanup;
> +                    }
> +                    virDomainObjUnlock(domobj);
> +                }
> +            }
> +
> +            /* Add to qemud_driver->sharedDisks list if the disk is shared */
> +            if (qemuSharedDiskListAdd(driver->sharedDisks,
> +                                      disk->src,
> +                                      vm->def->name)<  0)
> +                goto cleanup;
>           }
>
>           if (!disk->cdbfilter)

With the following patch squashed in (to avoid the deadlock,
it's not that comfortable to have nest lock/unlock, but I don't
have better idea temporally).

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index fac7d57..5284c74 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3714,17 +3714,21 @@ int qemuProcessStart(virConnectPtr conn,
                                                  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])))
+ 
entry->domains[i]))) {
+                        virDomainObjLock(vm);
                          goto cleanup;
+                    }

                      if (!(diskdef = virDomainDiskFindByPath(domobj->def,
                                                              disk->src))) {
                          virDomainObjUnlock(domobj);
+                        virDomainObjLock(vm);
                          goto cleanup;
                      }

@@ -3737,10 +3741,12 @@ int qemuProcessStart(virConnectPtr conn,
                                           "conflicts with other active "
                                           "domains"), disk->src);
                          virDomainObjUnlock(domobj);
+                        virDomainObjLock(vm);
                          goto cleanup;
                      }
                      virDomainObjUnlock(domobj);
                  }
+                virDomainObjLock(vm);
              }

              /* Add to qemud_driver->sharedDisks list if the disk is 
shared */