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@xxxxxxxxxx>
> > >
> > > 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list