Re: [Xen-devel] [PATCH] libxl: Documentation about the domain configuration on disk

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Dec 06, 2018 at 02:57:33PM +0000, Anthony PERARD wrote:
> On Thu, Dec 06, 2018 at 12:16:40PM +0000, Wei Liu wrote:
> > On Thu, Dec 06, 2018 at 10:43:32AM +0000, Anthony PERARD wrote:
> > > +UPDATE OF DOMAIN CONFIGURATION
> > > +------------------------------
> > > +
> > > +Also known as "libxl-json" userdata or `libxl_domain_config'.
> > > +
> > > +Whenever a running domain have its configuration updated, like changing
> > > +media in a cdrom drive, the domain configuration in libxl private data
> > > +store needs to be updated as well. The domain configuration should
> > > +contain *more* information about the domain rather than less, stale data
> > > +are easier to spot that missing data.
> > > +
> > > +Here is an example of how to update the domain configuration:
> > > + * Remove current media from cdrom drive
> > > + * Update domain configuration with media removed
> > 
> > We may not even need this because the primary source in this case is
> > QEMU. See below.
> > 
> > > + ( we could stop here)
> > > + * Update domain configuration to add media we are about to insert
> > > + * Insert media into cdrom drive
> > 
> > In essence we need a primary reference while using libxl-json file as a
> > secondary source.
> > 
> > When doing device hotplug, the primary source is xenstore. It may become
> > QEMU in the future if we move to a model where everything is
> > communicated via QMP.
> > 
> > When doing CDROM insertion and rejection, the primary source is QEMU
> > state.
> 
> I'm not trying to figure out what primary source should be here, I'm
> trying to find out how the secondary source, namely "libxl-json", should
> behave, what it should contain, when to update it compare the primary
> source (what a guest ultimately see).
> 
> > All in all I think your description is not wrong but it failed to
> > capture the high-level intent -- always update libxl-json before
> > updating the primary source.
> 
> That isn't what Ian said IRL, I don't think. From what I understand,
> when removing a media/disk, first remove the media, then update
> libxl-json; when adding a media/disk, first update libxl-json, then add
> the media.

OK I should have been clearer on this.

When removing a CD, you only need to update the primary source -- QEMU
in this case, you can leave libxl-json untouched. It is allowed to have
stale entries in libxl-json. This is implied in "We may not even need
this ..." further above.

When inserting a CD, always update libxl-json first, then add the media
to QEMU. My previous reply was for this part.

Yet I think CDROM has its own quirks. IIRC it is more an exception than
the norm. The existing code doesn't match what you wrote either.

> 
> > > +
> > > +Retrieve / store domain configuration from / to libxl private data store
> > > +are done with `libxl__get_domain_configuration' and
> > > +`libxl__set_domain_configuration'. Consult libxl_internal.h for more
> > > +information.
> > > +
> > 
> > What do you think about the text around libxl_internal.h:L2598?
> 
> If only I knew this comment existed :-(. It is burried, don't mention
> "libxl-json" or "userdata" or "domain config" but only the not very
> helpful term "json config"... Hmm, ... it actualy have "domain
> configuration" once.
> 
> Anyway, that comment block isn't very helpful because it basically says
> that we can't depriv QEMU, I mean do hotplug with a deprived QEMU. It
> assumes that we can keep a lock on the userdata while updating the
> guest, but we can't keep the lock while talking with QEMU (or more
> generaly: we can't keep the lock while doing any async operation).
> 
> But there is one useful piece of information:
>     Here we maintain one invariant: every device in xenstore must have
>     an entry in JSON file.
> (xenstore is describe as "primary reference" just before that sentence).
> 

Yes. That.

> This is what I would like my past self to be able to find out more
> easly, and having the information in CODING_STYLE would make sense I
> think.
> 
> > Maybe we should extend that comment block?
> 
> I still think it would be helpful to have pointers in CODING_STYLE, as
> there isn't a single place in libxl_internal.h where the information I
> was looking for could be added.
> 

Anyway, I'm not overly opposed to adding some easy to grep pointers, but
CODING_STYLE looks wrong to me.  Maybe README.dev?

Wei.

> Thanks,
> 
> -- 
> Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel