Re: [Xen-devel] [PATCH v1 03/10] libxl: store a copy of vanilla domain configuration when creating domain

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Wei Liu writes ("Re: [PATCH v1 03/10] libxl: store a copy of vanilla domain 
configuration when creating domain"):
> On Thu, Jul 24, 2014 at 07:52:46PM +0100, Ian Jackson wrote:
> > This race actually exists in libxl_userdata_store so it is my fault.
> > Sorry!
> 
> No worries. I can fix this.

Thanks.

> > I think this should be fixed as follows:
> > 
> >   * domain destruction should take a lock across deleting the
> >     userdata and destroying the domain in Xen
> > 
> >   * libxl_userdata_store should take take the lock,
> >     check domain exists, store userdata, unlock lock.
> > 
> > This would be a separate lock to the one you introduce, I think.
> > Your lock covers a lot of xenstore processing.  But perhaps we can
> > reuse some of the fcntl and lockfile massaging stuff.
> > 
> 
> Why do we need a second lock? The one I introduced in previous patch is
> used to serialise access to user data. Isn't that just what we want in
> this case?

I think you could use the same lock.  I suggested otherwise because
the xenstore manipulations might hold the lock for a bit longer than
ideal but today that doesn't seem so important.

(Sorry if I seem rather vague.  I have a head cold at the moment and
am quite confused.)

> > > +    /* At this point we've got domid and UUID, store configuration */
> > > +    ret = libxl__set_domain_configuration(gc, domid, &d_config_saved);
> > 
> > This libxl__set_domain_configuration is the only thing which creates
> > the libxl-json userdata ?  If so I think there is no 
> 
> -EPARSE. :-(

You can forget about that comment.  I was writing about the race I
mentioned further up, and forgot to come back and delete this after I
had added the other text to my mail.

> > > +    /* update network interface information */
...
> > Does creating the config early, and then updating it, not mean that
> > there will be a window during domain creation when configuration
> > exists but lacks these updates ?
> > 
> > I think that might be a (theoretically application-visible) bug.
> 
> Yes, there's such windows.
> 
> Ian C suggested I add lock during creation, which I think should fix
> this problem.

I don't think that's a particularly good idea.  Such a lock would
presumably cover most of the domain creation work (hotplug scripts
etc) which might take a long time.

During that time various operations would block unreasonably.

Also, I think you mustn't use an fcntl lock across ao operation
callback chains.  fcntl locks do not exclude other threads in the same
process.

> > If they do you can replace this whole block with a one-line
> >        if (rc) goto out;
> > 
> > > +    callback(gc, &d_config_saved, src);
> > 
> > Callback should probably have the ability to fail.
> 
> I guess you mean I should write
>    if (callback(XXX))
> 
> This of course can be done.

I was thinking more

     rc = callback(...);
     if (rc) goto out

:-).

(Unless you are sure that none of these callbacks will ever want to
fail...)

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel