Re: [Xen-devel] [PATCH v2 16/18] libxl: introduce libxl_userdata_unlink

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Aug 28, 2014 at 07:20:31PM +0100, Ian Campbell wrote:
> On Thu, 2014-08-28 at 12:50 +0100, Wei Liu wrote:
> > On Wed, Aug 27, 2014 at 03:16:59AM +0100, Ian Campbell wrote:
> > > On Wed, 2014-07-30 at 19:23 +0100, Wei Liu wrote:
> > > > +    CTX_LOCK;
> > > > +    lock = libxl__lock_domain_data(gc, domid);
> > > > +    if (!lock) {
> > > 
> > > This seems like a quirk which deserves commenting on since this lock
> > > only protects a specific type of userdata, specifically the libxl-json
> > > userdata.
> > > 
> > 
> > The original intent for that lock is to protect all application defined
> > data -- the name in registry is "libxl-lock" not "libxl-json-lock".
> 
> libxl-lock suggests to me that it locks things internal to libxl, not
> "application userdata lock".
> 

Yes it's internal.

> > Though through out this series it's mostly used to protect libxl-json
> > data, it doesn't preclude us from using it to protect other type of
> > userdata.
> > 
> 
> Except the locking functions aren't public, are they? and since the
> interface uses carefd's I don't think it can be (easily) made public.
> 

Correct.

> I guess what I'm saying is that if an app wants to lock accesses to its
> userdata then it has to do that itself, it can't be done internally to
> libxl.
> 

Application locking is one thing, but we still need to serialise libxl
access to those files, don't we? Any access to userdata store via libxl
API should be serialised. The reason is stated in previous patch "libxl:
properly lock user data store".

> > During review last round we discussed how we should deal with "xl
> > config-udpate" command. The conclusion is that we still honour user
> > supplied config file and it has higher priority than libxl-json. We
> > would like to transform the config file supplied by user to libxl-json,
> > then remove that user supplied file, so that next time domain is
> > rebooted it always has the config tracked by libxl. Without this patch
> > xl has no way to unlink that file and it will still take effect during
> > next reboot, which is not what we want.
> 
> OK, so this is about removing the existing xl config, not the
> libxl-json. I don't think this should take the libxl lock then -- that
> lock doesn't protect the xl cfg userdata in any meaningful way AFAICT.
> 

libxl-lock won't lock that file on application level but it is used to
prevent libxl threads from messing things up.

Does my explanation make sense? And, what do you have in mind for
designing this API?

Wei.

> Ian.

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