[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 4/5] virDomainObjListFindByName: Return referenced object



On Thu, Apr 23, 2015 at 19:14:57 +0200, Michal Privoznik wrote:
> Every domain that grabs a domain object to work over should
> reference it to make sure it won't disappear meanwhile.
> 
> Signed-off-by: Michal Privoznik <mprivozn redhat com>
> ---
>  src/bhyve/bhyve_driver.c         |  3 +-
>  src/conf/domain_conf.c           |  1 +
>  src/libxl/libxl_driver.c         | 10 ++---
>  src/lxc/lxc_driver.c             |  3 +-
>  src/openvz/openvz_driver.c       | 11 +++--
>  src/parallels/parallels_driver.c |  3 +-
>  src/qemu/qemu_driver.c           | 14 ++----
>  src/test/test_driver.c           | 93 +++++++++++++---------------------------
>  src/uml/uml_driver.c             | 15 +++----
>  9 files changed, 51 insertions(+), 102 deletions(-)
> 

...

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 686c614..6666d03 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1157,6 +1157,7 @@ virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms,
>      virDomainObjPtr obj;
>      virObjectLock(doms);
>      obj = virHashSearch(doms->objs, virDomainObjListSearchName, name);
> +    virObjectRef(obj);
>      if (obj) {
>          virObjectLock(obj);
>          if (obj->removing) {

Here this code that is below in the context checks if the @removing flag
is set and if so the function returns NULL. With your addition the
reference you've added wouldn't be removed.

> diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
> index 1bb8973..10d94ff 100644
> --- a/src/openvz/openvz_driver.c
> +++ b/src/openvz/openvz_driver.c

...

> @@ -1007,6 +1006,7 @@ openvzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int fla
>          virReportError(VIR_ERR_OPERATION_FAILED,
>                         _("Already an OPENVZ VM active with the id '%s'"),
>                         vmdef->name);
> +        virDomainObjEndAPI(&vm);
>          goto cleanup;
>      }
>      if (!(vm = virDomainObjListAdd(driver->domains, vmdef,

The whole piece of code that looks up the domain and reports the error
above could be removed as virDomainObjListAdd does the same check.

> @@ -1103,6 +1103,7 @@ openvzDomainCreateXML(virConnectPtr conn, const char *xml,
>          virReportError(VIR_ERR_OPERATION_FAILED,
>                         _("Already an OPENVZ VM defined with the id '%s'"),
>                         vmdef->name);
> +        virDomainObjEndAPI(&vm);
>          goto cleanup;
>      }
>      if (!(vm = virDomainObjListAdd(driver->domains,

Same here. Not worth changing though probably.

...



> diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
> index 9cee541..f8a84e4 100644
> --- a/src/uml/uml_driver.c
> +++ b/src/uml/uml_driver.c
> @@ -339,7 +339,7 @@ umlInotifyEvent(int watch,
>          if (e.mask & IN_DELETE) {
>              VIR_DEBUG("Got inotify domain shutdown '%s'", name);
>              if (!virDomainObjIsActive(dom)) {
> -                virObjectUnlock(dom);
> +                virDomainObjEndAPI(&dom);
>                  continue;
>              }
>  
> @@ -351,17 +351,16 @@ umlInotifyEvent(int watch,
>              if (!dom->persistent) {
>                  virDomainObjListRemove(driver->domains,
>                                         dom);
> -                dom = NULL;
>              }

This now leaves a single statement in an if with braces.

>          } else if (e.mask & (IN_CREATE | IN_MODIFY)) {
>              VIR_DEBUG("Got inotify domain startup '%s'", name);
>              if (virDomainObjIsActive(dom)) {
> -                virObjectUnlock(dom);
> +                virDomainObjEndAPI(&dom);
>                  continue;
>              }
>  
>              if (umlReadPidFile(driver, dom) < 0) {
> -                virObjectUnlock(dom);
> +                virDomainObjEndAPI(&dom);
>                  continue;
>              }
>  
> @@ -385,7 +384,6 @@ umlInotifyEvent(int watch,
>                  if (!dom->persistent) {
>                      virDomainObjListRemove(driver->domains,
>                                             dom);
> -                    dom = NULL;

Again, single statement in an if with braces

>                  }
>              } else if (umlIdentifyChrPTY(driver, dom) < 0) {
>                  VIR_WARN("Could not identify character devices for new domain");
> @@ -398,12 +396,10 @@ umlInotifyEvent(int watch,
>                  if (!dom->persistent) {
>                      virDomainObjListRemove(driver->domains,
>                                             dom);
> -                    dom = NULL;

Here too

>                  }
>              }
>          }
> -        if (dom)
> -            virObjectUnlock(dom);
> +        virDomainObjEndAPI(&dom);
>          if (event) {
>              umlDomainEventQueue(driver, event);
>              event = NULL;

ACK, if you fix the broken reference counting in case the domain is
being removed.

Peter

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]