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

Re: [libvirt] [PATCH v3 04/13] Merge virDomainObjListIsDuplicate into virDomainObjListAdd



On Tue, Feb 05, 2013 at 14:48:01 +0000, Daniel P. Berrange wrote:
> On Mon, Feb 04, 2013 at 04:58:25PM +0100, Jiri Denemark wrote:
> > On Fri, Feb 01, 2013 at 11:18:26 +0000, Daniel P. Berrange wrote:
> > > From: "Daniel P. Berrange" <berrange redhat com>
> > > 
> > > The duplicate VM checking should be done atomically with
> > > virDomainObjListAdd, so shoud not be a separate function.
> > > Instead just use flags to indicate what kind of checks are
> > > required.
> > > 
> > ...
> > > This pair, used in virDomainDefinXML:
> > > @@ -12623,7 +12601,10 @@ static virDomainPtr qemuDomainAttach(virConnectPtr conn,
> > >  
> > >      if (!(vm = virDomainObjListAdd(driver->domains,
> > >                                     driver->caps,
> > > -                                   def, false)))
> > > +                                   def,
> > > +                                   VIR_DOMAIN_OBJ_LIST_ADD_LIVE |
> > > +                                   VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE,
> > > +                                   NULL)))
> > 
> > Although this could be correct, it doesn't match current code. To match
> > it, you'd need to use just VIR_DOMAIN_OBJ_LIST_ADD_CHECK_LIVE flag.
> 
> Yep, I believe this new code is correct
> 
> > 
> > Looking forward to v2 :-)
> 
> Here is the diff from v1 to v2
> 
> Daniel
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 1771668..06dbe1f 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1853,21 +1853,33 @@ error:
>  
>  void virDomainObjAssignDef(virDomainObjPtr domain,
>                             const virDomainDefPtr def,
> -                           bool live)
> +                           bool live,
> +                           virDomainDefPtr *oldDef)
>  {
> -    if (!virDomainObjIsActive(domain)) {
> +    *oldDef = NULL;

Oops, oldDef is optional; the above should be

  +    if (oldDef)
  +        *oldDef = NULL;

> +    if (virDomainObjIsActive(domain)) {
> +        if (oldDef)
> +            *oldDef = domain->newDef;
> +        else
> +            virDomainDefFree(domain->newDef);
> +        domain->newDef = def;
> +    } else {
>          if (live) {
> -            /* save current configuration to be restored on domain shutdown */
> -            if (!domain->newDef)
> -                domain->newDef = domain->def;
> +            if (domain->def) {

This test is not really needed since if both def and newDef are NULL,
newDef = def does nothing and if def is NULL but newDef is not,
virDomainFree should be able to handle NULL argument. Anyway, it's not a
big deal, just that the diff could have been smaller :-)

> +                /* save current configuration to be restored on domain shutdown */
> +                if (!domain->newDef)
> +                    domain->newDef = domain->def;
> +                else
> +                    virDomainDefFree(domain->def);
> +            }
>              domain->def = def;
>          } else {
...

ACK to the original patch with these changes (after fixing the possible
NULL dereference) squashed in.

Jirka


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