Re: [PATCH v3 04/11] libxl: add generic function to add device

[Wei Liu <wei.liu2@citrix.com>]

On Mon, Jul 10, 2017 at 03:41:28PM +0300, Oleksandr Grytsov wrote:
> On Fri, Jul 7, 2017 at 1:56 PM, Oleksandr Grytsov <al1img@gmail.com> wrote:
> > On Fri, Jul 7, 2017 at 1:32 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> >> On Fri, Jul 07, 2017 at 01:29:39PM +0300, Oleksandr Grytsov wrote:
> >>  > Actually my the first patch probably was done on the old codebase
> >>> > which doesn't have locking in add function. So new approach is
> >>> > definitely wrong and I will use former one.
> >>>
> >>> Please ignore my above comment. Actually it looks like my new approach
> >>> changes former behavior. I will rework this function to match former one.
> >>>
> >>> Actually new approach
> >>
> >> Hit "Send" too soon?
> >
> > Just forgot to remove this line. So, I will rework this part.
> >
> 
> Few questions about former implementation (I address vtpm as reference
> but questions are related to all devices):
> 
> 1. Using of libxl_device_vtpm vtpm_saved variable. It is unclear why
> we need this additional variable.
> There is no any rollback or cancellation with this variable.
> It is used to be added to the domain config but vtpm from input
> parameter can be used for this reason as well.

The vtpm_saved variable is a copy of the structure passed in by the caller.

We then pass vtpm to the _setdefault function which touches some of the
fields inside.

But then not all the fields changed by the _setdefault function need to
be written to our persistent domain configuration on disk. The fields we
care about are copied back  to vtpm_saved by libxl__update_config_vtpm.
Then we save vtpm_saved.

For your particular device, you should provide a similar update_config
function.

> 
> 2. Why libxl__update_config_vtpm(gc, &vtpm_saved, vtpm); is called if
> just before we copied
> vtpm_saved from vtpm?
> 
>     libxl_device_vtpm_init(&vtpm_saved);
>     libxl_device_vtpm_copy(CTX, &vtpm_saved, vtpm);
> 
> I see that dev id is updated but it could be done before copy operation.
> 

But for different device type there are different things to save. Vtpm
is a bit more of a simplistic one. See also other update_config
function.

The code structure is deliberated. It is a pattern applicable for all
devices --  the implementer can easily follow the pattern.

> 3. What is reason to call libxl__set_domain_configuration(gc, domid,
> &d_config); in each
> xen store transaction attempt?
> 

That would ensure the eventual consistency of the file written on disk
and the content in xenstore.

Keep in mind that there could be several threads competing with each
other to manipulate both the file on disk and xenstore.

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