diff mbox

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

Message ID 1498557807-10810-5-git-send-email-al1img@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Oleksandr Grytsov June 27, 2017, 10:03 a.m. UTC
From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>

Add libxl__device_add functio.
Almost all devices have similar libxl__device_xxxx_add function.
This generic function implements same functionality but
using the device handling framework. The device specific
part this is setting xen store configuration. This part
is moved to set_xenstore_config callback of the device framework.

Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
---
 tools/libxl/libxl_create.c   |   2 +
 tools/libxl/libxl_device.c   | 103 +++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_internal.h |  15 +++++++
 tools/libxl/libxl_usb.c      |   2 +
 tools/libxl/libxl_vdispl.c   |  17 ++++++-
 5 files changed, 138 insertions(+), 1 deletion(-)

Comments

Wei Liu June 29, 2017, 5:36 p.m. UTC | #1
On Tue, Jun 27, 2017 at 01:03:20PM +0300, Oleksandr Grytsov wrote:
> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
> 
> Add libxl__device_add functio.

function

> Almost all devices have similar libxl__device_xxxx_add function.
> This generic function implements same functionality but
> using the device handling framework. The device specific
> part this is setting xen store configuration. This part
> is moved to set_xenstore_config callback of the device framework.
> 

Right. I think this is a good idea in general.

I don't see exiting device ported to the new framework, why?

We really don't want two sets of code that does the same thing in libxl.
That's a recipe for bugs.

> Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
[...]
>  /*
> diff --git a/tools/libxl/libxl_vdispl.c b/tools/libxl/libxl_vdispl.c
> index a628adc..c79bcda 100644
> --- a/tools/libxl/libxl_vdispl.c
> +++ b/tools/libxl/libxl_vdispl.c
> @@ -14,6 +14,21 @@
>  
>  #include "libxl_internal.h"
>  
> +static int libxl__device_vdispl_setdefault(libxl__gc *gc, uint32_t domid,
> +                                           libxl_device_vdispl *vdispl)
> +{
> +    int rc;
> +
> +    rc = libxl__resolve_domid(gc, vdispl->backend_domname,
> +                              &vdispl->backend_domid);
> +
> +    if (vdispl->devid == -1) {
> +        vdispl->devid = libxl__device_nextid(gc, domid, "vdispl");
> +    }
> +

No need to have {}.

> +    return rc;
> +}
> +
>  static int libxl__device_from_vdispl(libxl__gc *gc, uint32_t domid,
>                                       libxl_device_vdispl *vdispl,
>                                       libxl__device *device)
> @@ -47,7 +62,7 @@ static void libxl__device_vdispl_add(libxl__egc *egc, uint32_t domid,
>                                       libxl_device_vdispl *vdispl,
>                                       libxl__ao_device *aodev)
>  {
> -
> +    libxl__device_add(egc, domid, &libxl__vdispl_devtype, vdispl, aodev);
>  }
>  
>  libxl_device_vdispl *libxl_device_vdispl_list(libxl_ctx *ctx, uint32_t domid,
> -- 
> 2.7.4
>
Oleksandr Grytsov June 30, 2017, 1:24 p.m. UTC | #2
On Thu, Jun 29, 2017 at 8:36 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> On Tue, Jun 27, 2017 at 01:03:20PM +0300, Oleksandr Grytsov wrote:
>> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
>>
>> Add libxl__device_add functio.
>
> function
>
>> Almost all devices have similar libxl__device_xxxx_add function.
>> This generic function implements same functionality but
>> using the device handling framework. The device specific
>> part this is setting xen store configuration. This part
>> is moved to set_xenstore_config callback of the device framework.
>>
>
> Right. I think this is a good idea in general.
>
> I don't see exiting device ported to the new framework, why?

Good question. I think it is a little dangerous and may introduce regression.
But definitely it should be done. I can do these changes but I don't have
visibility how to check each device.

>
> We really don't want two sets of code that does the same thing in libxl.
> That's a recipe for bugs.



>
>> Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
> [...]
>>  /*
>> diff --git a/tools/libxl/libxl_vdispl.c b/tools/libxl/libxl_vdispl.c
>> index a628adc..c79bcda 100644
>> --- a/tools/libxl/libxl_vdispl.c
>> +++ b/tools/libxl/libxl_vdispl.c
>> @@ -14,6 +14,21 @@
>>
>>  #include "libxl_internal.h"
>>
>> +static int libxl__device_vdispl_setdefault(libxl__gc *gc, uint32_t domid,
>> +                                           libxl_device_vdispl *vdispl)
>> +{
>> +    int rc;
>> +
>> +    rc = libxl__resolve_domid(gc, vdispl->backend_domname,
>> +                              &vdispl->backend_domid);
>> +
>> +    if (vdispl->devid == -1) {
>> +        vdispl->devid = libxl__device_nextid(gc, domid, "vdispl");
>> +    }
>> +
>
> No need to have {}.
>
>> +    return rc;
>> +}
>> +
>>  static int libxl__device_from_vdispl(libxl__gc *gc, uint32_t domid,
>>                                       libxl_device_vdispl *vdispl,
>>                                       libxl__device *device)
>> @@ -47,7 +62,7 @@ static void libxl__device_vdispl_add(libxl__egc *egc, uint32_t domid,
>>                                       libxl_device_vdispl *vdispl,
>>                                       libxl__ao_device *aodev)
>>  {
>> -
>> +    libxl__device_add(egc, domid, &libxl__vdispl_devtype, vdispl, aodev);
>>  }
>>
>>  libxl_device_vdispl *libxl_device_vdispl_list(libxl_ctx *ctx, uint32_t domid,
>> --
>> 2.7.4
>>
Wei Liu June 30, 2017, 2:16 p.m. UTC | #3
On Fri, Jun 30, 2017 at 04:24:23PM +0300, Oleksandr Grytsov wrote:
> On Thu, Jun 29, 2017 at 8:36 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> > On Tue, Jun 27, 2017 at 01:03:20PM +0300, Oleksandr Grytsov wrote:
> >> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
> >>
> >> Add libxl__device_add functio.
> >
> > function
> >
> >> Almost all devices have similar libxl__device_xxxx_add function.
> >> This generic function implements same functionality but
> >> using the device handling framework. The device specific
> >> part this is setting xen store configuration. This part
> >> is moved to set_xenstore_config callback of the device framework.
> >>
> >
> > Right. I think this is a good idea in general.
> >
> > I don't see exiting device ported to the new framework, why?
> 
> Good question. I think it is a little dangerous and may introduce regression.
> But definitely it should be done. I can do these changes but I don't have
> visibility how to check each device.

Please just do it. We have a lot of time during development and RC
period for people to test your changes.
Wei Liu June 30, 2017, 2:18 p.m. UTC | #4
On Fri, Jun 30, 2017 at 03:16:38PM +0100, Wei Liu wrote:
> On Fri, Jun 30, 2017 at 04:24:23PM +0300, Oleksandr Grytsov wrote:
> > On Thu, Jun 29, 2017 at 8:36 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> > > On Tue, Jun 27, 2017 at 01:03:20PM +0300, Oleksandr Grytsov wrote:
> > >> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
> > >>
> > >> Add libxl__device_add functio.
> > >
> > > function
> > >
> > >> Almost all devices have similar libxl__device_xxxx_add function.
> > >> This generic function implements same functionality but
> > >> using the device handling framework. The device specific
> > >> part this is setting xen store configuration. This part
> > >> is moved to set_xenstore_config callback of the device framework.
> > >>
> > >
> > > Right. I think this is a good idea in general.
> > >
> > > I don't see exiting device ported to the new framework, why?
> > 
> > Good question. I think it is a little dangerous and may introduce regression.
> > But definitely it should be done. I can do these changes but I don't have
> > visibility how to check each device.
> 
> Please just do it. We have a lot of time during development and RC
> period for people to test your changes.

And I forget to say, please use one patch for one device type.
Oleksandr Grytsov July 3, 2017, 12:53 p.m. UTC | #5
On Fri, Jun 30, 2017 at 5:18 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> On Fri, Jun 30, 2017 at 03:16:38PM +0100, Wei Liu wrote:
>> On Fri, Jun 30, 2017 at 04:24:23PM +0300, Oleksandr Grytsov wrote:
>> > On Thu, Jun 29, 2017 at 8:36 PM, Wei Liu <wei.liu2@citrix.com> wrote:
>> > > On Tue, Jun 27, 2017 at 01:03:20PM +0300, Oleksandr Grytsov wrote:
>> > >> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
>> > >>
>> > >> Add libxl__device_add functio.
>> > >
>> > > function
>> > >
>> > >> Almost all devices have similar libxl__device_xxxx_add function.
>> > >> This generic function implements same functionality but
>> > >> using the device handling framework. The device specific
>> > >> part this is setting xen store configuration. This part
>> > >> is moved to set_xenstore_config callback of the device framework.
>> > >>
>> > >
>> > > Right. I think this is a good idea in general.
>> > >
>> > > I don't see exiting device ported to the new framework, why?
>> >
>> > Good question. I think it is a little dangerous and may introduce regression.
>> > But definitely it should be done. I can do these changes but I don't have
>> > visibility how to check each device.
>>
>> Please just do it. We have a lot of time during development and RC
>> period for people to test your changes.
>
> And I forget to say, please use one patch for one device type.

Should it be in this patch set or better to create new one for each device?
Wei Liu July 3, 2017, 12:57 p.m. UTC | #6
On Mon, Jul 03, 2017 at 03:53:08PM +0300, Oleksandr Grytsov wrote:
> On Fri, Jun 30, 2017 at 5:18 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> > On Fri, Jun 30, 2017 at 03:16:38PM +0100, Wei Liu wrote:
> >> On Fri, Jun 30, 2017 at 04:24:23PM +0300, Oleksandr Grytsov wrote:
> >> > On Thu, Jun 29, 2017 at 8:36 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> >> > > On Tue, Jun 27, 2017 at 01:03:20PM +0300, Oleksandr Grytsov wrote:
> >> > >> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
> >> > >>
> >> > >> Add libxl__device_add functio.
> >> > >
> >> > > function
> >> > >
> >> > >> Almost all devices have similar libxl__device_xxxx_add function.
> >> > >> This generic function implements same functionality but
> >> > >> using the device handling framework. The device specific
> >> > >> part this is setting xen store configuration. This part
> >> > >> is moved to set_xenstore_config callback of the device framework.
> >> > >>
> >> > >
> >> > > Right. I think this is a good idea in general.
> >> > >
> >> > > I don't see exiting device ported to the new framework, why?
> >> >
> >> > Good question. I think it is a little dangerous and may introduce regression.
> >> > But definitely it should be done. I can do these changes but I don't have
> >> > visibility how to check each device.
> >>
> >> Please just do it. We have a lot of time during development and RC
> >> period for people to test your changes.
> >
> > And I forget to say, please use one patch for one device type.
> 
> Should it be in this patch set or better to create new one for each device?
> 

Those patches should be in this series.  One for each device for ease of
review please, and arrange it a way such that I can partially apply this
series.
Oleksandr Grytsov July 4, 2017, 9:41 a.m. UTC | #7
>> >> > > I don't see exiting device ported to the new framework, why?
>> >> >
>> >> > Good question. I think it is a little dangerous and may introduce regression.
>> >> > But definitely it should be done. I can do these changes but I don't have
>> >> > visibility how to check each device.
>> >>
>> >> Please just do it. We have a lot of time during development and RC
>> >> period for people to test your changes.
>> >
>> > And I forget to say, please use one patch for one device type.
>>
>> Should it be in this patch set or better to create new one for each device?
>>
>
> Those patches should be in this series.  One for each device for ease of
> review please, and arrange it a way such that I can partially apply this
> series.

Ok. I will wait for your feedback about this series and will prepare v4 with
fixes and changes for other devices.

Thanks.
Wei Liu July 6, 2017, 3:51 p.m. UTC | #8
On Tue, Jun 27, 2017 at 01:03:20PM +0300, Oleksandr Grytsov wrote:
> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
> 
> Add libxl__device_add functio.
> Almost all devices have similar libxl__device_xxxx_add function.
> This generic function implements same functionality but
> using the device handling framework. The device specific
> part this is setting xen store configuration. This part
> is moved to set_xenstore_config callback of the device framework.
> 
> Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
[...]
> +
> +void libxl__device_add(libxl__egc *egc, uint32_t domid,
> +                       const struct libxl_device_type *dt, void *type,
> +                       libxl__ao_device *aodev)
> +{
> +    STATE_AO_GC(aodev->ao);
> +    libxl__device *device;
> +    int rc;
> +
> +    rc = dt->set_default(gc, domid, type);
> +    if (rc) goto out;
> +
> +    GCNEW(device);
> +    rc = dt->to_device(gc, domid, type, device);
> +    if ( rc != 0 ) goto out;
> +
> +    rc = libxl__device_exists(gc, XBT_NULL, device);
> +    if (rc < 0) goto out;
> +    if (rc == 1) {              /* already exists in xenstore */
> +        LOGD(ERROR, domid, "device already exists in xenstore");
> +        aodev->action = LIBXL__DEVICE_ACTION_ADD; /* for error message */
> +        rc = ERROR_DEVICE_EXISTS;
> +        goto out;
> +    }
> +
> +    if (aodev->update_json) {
> +        rc = device_add_domain_config(gc, domid, dt, type);
> +        if (rc) goto out;
> +    }
> +
> +    if (dt->set_xenstore_config) {
> +        rc = dt->set_xenstore_config(gc, domid, type);
> +        if (rc) goto out;
> +    }
> +

This has changed the locking hierarchy we define in libxl_internal.h.
See libxl_internal.h:L2592.

Either you need to preserve the hierarchy or you need to prove the
correctness of the new approach. The former is probably easier.
Oleksandr Grytsov July 7, 2017, 9:49 a.m. UTC | #9
On Thu, Jul 6, 2017 at 6:51 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> On Tue, Jun 27, 2017 at 01:03:20PM +0300, Oleksandr Grytsov wrote:
>> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
>>
>> Add libxl__device_add functio.
>> Almost all devices have similar libxl__device_xxxx_add function.
>> This generic function implements same functionality but
>> using the device handling framework. The device specific
>> part this is setting xen store configuration. This part
>> is moved to set_xenstore_config callback of the device framework.
>>
>> Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
> [...]
>> +
>> +void libxl__device_add(libxl__egc *egc, uint32_t domid,
>> +                       const struct libxl_device_type *dt, void *type,
>> +                       libxl__ao_device *aodev)
>> +{
>> +    STATE_AO_GC(aodev->ao);
>> +    libxl__device *device;
>> +    int rc;
>> +
>> +    rc = dt->set_default(gc, domid, type);
>> +    if (rc) goto out;
>> +
>> +    GCNEW(device);
>> +    rc = dt->to_device(gc, domid, type, device);
>> +    if ( rc != 0 ) goto out;
>> +
>> +    rc = libxl__device_exists(gc, XBT_NULL, device);
>> +    if (rc < 0) goto out;
>> +    if (rc == 1) {              /* already exists in xenstore */
>> +        LOGD(ERROR, domid, "device already exists in xenstore");
>> +        aodev->action = LIBXL__DEVICE_ACTION_ADD; /* for error message */
>> +        rc = ERROR_DEVICE_EXISTS;
>> +        goto out;
>> +    }
>> +
>> +    if (aodev->update_json) {
>> +        rc = device_add_domain_config(gc, domid, dt, type);
>> +        if (rc) goto out;
>> +    }
>> +
>> +    if (dt->set_xenstore_config) {
>> +        rc = dt->set_xenstore_config(gc, domid, type);
>> +        if (rc) goto out;
>> +    }
>> +
>
> This has changed the locking hierarchy we define in libxl_internal.h.
> See libxl_internal.h:L2592.
>
> Either you need to preserve the hierarchy or you need to prove the
> correctness of the new approach. The former is probably easier.

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.
Oleksandr Grytsov July 7, 2017, 10:29 a.m. UTC | #10
On Fri, Jul 7, 2017 at 12:49 PM, Oleksandr Grytsov <al1img@gmail.com> wrote:
> On Thu, Jul 6, 2017 at 6:51 PM, Wei Liu <wei.liu2@citrix.com> wrote:
>> On Tue, Jun 27, 2017 at 01:03:20PM +0300, Oleksandr Grytsov wrote:
>>> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
>>>
>>> Add libxl__device_add functio.
>>> Almost all devices have similar libxl__device_xxxx_add function.
>>> This generic function implements same functionality but
>>> using the device handling framework. The device specific
>>> part this is setting xen store configuration. This part
>>> is moved to set_xenstore_config callback of the device framework.
>>>
>>> Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com>
>> [...]
>>> +
>>> +void libxl__device_add(libxl__egc *egc, uint32_t domid,
>>> +                       const struct libxl_device_type *dt, void *type,
>>> +                       libxl__ao_device *aodev)
>>> +{
>>> +    STATE_AO_GC(aodev->ao);
>>> +    libxl__device *device;
>>> +    int rc;
>>> +
>>> +    rc = dt->set_default(gc, domid, type);
>>> +    if (rc) goto out;
>>> +
>>> +    GCNEW(device);
>>> +    rc = dt->to_device(gc, domid, type, device);
>>> +    if ( rc != 0 ) goto out;
>>> +
>>> +    rc = libxl__device_exists(gc, XBT_NULL, device);
>>> +    if (rc < 0) goto out;
>>> +    if (rc == 1) {              /* already exists in xenstore */
>>> +        LOGD(ERROR, domid, "device already exists in xenstore");
>>> +        aodev->action = LIBXL__DEVICE_ACTION_ADD; /* for error message */
>>> +        rc = ERROR_DEVICE_EXISTS;
>>> +        goto out;
>>> +    }
>>> +
>>> +    if (aodev->update_json) {
>>> +        rc = device_add_domain_config(gc, domid, dt, type);
>>> +        if (rc) goto out;
>>> +    }
>>> +
>>> +    if (dt->set_xenstore_config) {
>>> +        rc = dt->set_xenstore_config(gc, domid, type);
>>> +        if (rc) goto out;
>>> +    }
>>> +
>>
>> This has changed the locking hierarchy we define in libxl_internal.h.
>> See libxl_internal.h:L2592.
>>
>> Either you need to preserve the hierarchy or you need to prove the
>> correctness of the new approach. The former is probably easier.
>
> 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

>
> --
> Best Regards,
> Oleksandr Grytsov.
Wei Liu July 7, 2017, 10:32 a.m. UTC | #11
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?
Oleksandr Grytsov July 7, 2017, 10:56 a.m. UTC | #12
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.
Oleksandr Grytsov July 10, 2017, 12:41 p.m. UTC | #13
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.

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.

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

Thanks.

> --
> Best Regards,
> Oleksandr Grytsov.
Wei Liu July 12, 2017, 10:12 a.m. UTC | #14
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.
Oleksandr Grytsov July 12, 2017, 4:13 p.m. UTC | #15
On Tue, Jul 4, 2017 at 12:41 PM, Oleksandr Grytsov <al1img@gmail.com> wrote:
>>> >> > > I don't see exiting device ported to the new framework, why?
>>> >> >
>>> >> > Good question. I think it is a little dangerous and may introduce regression.
>>> >> > But definitely it should be done. I can do these changes but I don't have
>>> >> > visibility how to check each device.
>>> >>
>>> >> Please just do it. We have a lot of time during development and RC
>>> >> period for people to test your changes.
>>> >
>>> > And I forget to say, please use one patch for one device type.
>>>
>>> Should it be in this patch set or better to create new one for each device?
>>>
>>
>> Those patches should be in this series.  One for each device for ease of
>> review please, and arrange it a way such that I can partially apply this
>> series.
>
> Ok. I will wait for your feedback about this series and will prepare v4 with
> fixes and changes for other devices.
>
> Thanks.

Hi Wei,

I've prepared new patch set. It is on my github [1].
I would appreciate if you review it before I send it.

The main changes are:
* libxl__device_add renamed to libxl__device_add_async and reworked
  to match the former design;
* libxl__device_add used for devices which don't require updating domain
  config but simple write to Xen Store (9pfs, vkb, vfb);
* following devices are changed to use the libxl__device_add:
  9pfs, vkb, vfb, nic, vtpm. Other device (console, pci, usb, disk) have
  very different adding pattern and require to unreasonable extend
  libxl__device_add_async and its parameters;
* disk device list changed to use libxl__device_list;
* small previous comments are applied.

[1] https://github.com/al1img/xen/tree/xl-vdispl-v4
Wei Liu July 18, 2017, 1:35 p.m. UTC | #16
On Wed, Jul 12, 2017 at 07:13:34PM +0300, Oleksandr Grytsov wrote:
> On Tue, Jul 4, 2017 at 12:41 PM, Oleksandr Grytsov <al1img@gmail.com> wrote:
> >>> >> > > I don't see exiting device ported to the new framework, why?
> >>> >> >
> >>> >> > Good question. I think it is a little dangerous and may introduce regression.
> >>> >> > But definitely it should be done. I can do these changes but I don't have
> >>> >> > visibility how to check each device.
> >>> >>
> >>> >> Please just do it. We have a lot of time during development and RC
> >>> >> period for people to test your changes.
> >>> >
> >>> > And I forget to say, please use one patch for one device type.
> >>>
> >>> Should it be in this patch set or better to create new one for each device?
> >>>
> >>
> >> Those patches should be in this series.  One for each device for ease of
> >> review please, and arrange it a way such that I can partially apply this
> >> series.
> >
> > Ok. I will wait for your feedback about this series and will prepare v4 with
> > fixes and changes for other devices.
> >
> > Thanks.
> 
> Hi Wei,
> 
> I've prepared new patch set. It is on my github [1].
> I would appreciate if you review it before I send it.
> 
> The main changes are:
> * libxl__device_add renamed to libxl__device_add_async and reworked
>   to match the former design;
> * libxl__device_add used for devices which don't require updating domain
>   config but simple write to Xen Store (9pfs, vkb, vfb);
> * following devices are changed to use the libxl__device_add:
>   9pfs, vkb, vfb, nic, vtpm. Other device (console, pci, usb, disk) have
>   very different adding pattern and require to unreasonable extend
>   libxl__device_add_async and its parameters;
> * disk device list changed to use libxl__device_list;
> * small previous comments are applied.
> 
> [1] https://github.com/al1img/xen/tree/xl-vdispl-v4
> 

Please just send the patches. It would be easier for me to make comments
via emails.
diff mbox

Patch

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index bffbc45..4e5ba29 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1430,6 +1430,8 @@  out:
 
 #define libxl_device_dtdev_list NULL
 #define libxl_device_dtdev_compare NULL
+#define libxl__device_from_dtdev NULL
+#define libxl__device_dtdev_setdefault NULL
 static DEFINE_DEVICE_TYPE_STRUCT(dtdev);
 
 const struct libxl_device_type *device_type_tbl[] = {
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 8bcfa2b..e202503 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -1793,6 +1793,109 @@  out:
     return AO_CREATE_FAIL(rc);
 }
 
+static int device_add_domain_config(libxl__gc *gc, uint32_t domid,
+                                    const struct libxl_device_type *dt,
+                                    void *type)
+{
+    int rc;
+    libxl_domain_config d_config;
+    libxl__domain_userdata_lock *lock = NULL;
+    int *num_dev;
+    int i;
+    void *item = NULL;
+
+    libxl_domain_config_init(&d_config);
+
+    lock = libxl__lock_domain_userdata(gc, domid);
+    if (!lock) {
+        rc = ERROR_LOCK_FAIL; goto out;
+    }
+
+    rc = libxl__get_domain_configuration(gc, domid, &d_config);
+    if (rc) goto out;
+
+    num_dev = libxl__device_type_get_num(dt, &d_config);
+
+    /* Check for existing device */
+    for (i = 0; i < *num_dev; i++) {
+        if (dt->compare(libxl__device_type_get_elem(dt, &d_config, i), type)) {
+            item = libxl__device_type_get_elem(dt, &d_config, i);
+        }
+    }
+
+    if (!item) {
+        void **devs= libxl__device_type_get_ptr(dt, &d_config);
+        *devs = libxl__realloc(NOGC, *devs,
+                               dt->dev_elem_size * (*num_dev + 1));
+        item = libxl__device_type_get_elem(dt, &d_config, *num_dev);
+        (*num_dev)++;
+    } else {
+        dt->dispose(item);
+    }
+
+    dt->init(item);
+    dt->copy(CTX, item, type);
+
+    rc = libxl__dm_check_start(gc, &d_config, domid);
+    if (rc) goto out;
+
+    rc = libxl__set_domain_configuration(gc, domid, &d_config);
+    if (rc) goto out;
+
+    rc = 0;
+
+out:
+    if (lock) libxl__unlock_domain_userdata(lock);
+    libxl_domain_config_dispose(&d_config);
+    return rc;
+}
+
+void libxl__device_add(libxl__egc *egc, uint32_t domid,
+                       const struct libxl_device_type *dt, void *type,
+                       libxl__ao_device *aodev)
+{
+    STATE_AO_GC(aodev->ao);
+    libxl__device *device;
+    int rc;
+
+    rc = dt->set_default(gc, domid, type);
+    if (rc) goto out;
+
+    GCNEW(device);
+    rc = dt->to_device(gc, domid, type, device);
+    if ( rc != 0 ) goto out;
+
+    rc = libxl__device_exists(gc, XBT_NULL, device);
+    if (rc < 0) goto out;
+    if (rc == 1) {              /* already exists in xenstore */
+        LOGD(ERROR, domid, "device already exists in xenstore");
+        aodev->action = LIBXL__DEVICE_ACTION_ADD; /* for error message */
+        rc = ERROR_DEVICE_EXISTS;
+        goto out;
+    }
+
+    if (aodev->update_json) {
+        rc = device_add_domain_config(gc, domid, dt, type);
+        if (rc) goto out;
+    }
+
+    if (dt->set_xenstore_config) {
+        rc = dt->set_xenstore_config(gc, domid, type);
+        if (rc) goto out;
+    }
+
+    aodev->dev = device;
+    aodev->action = LIBXL__DEVICE_ACTION_ADD;
+    libxl__wait_device_connection(egc, aodev);
+
+    rc = 0;
+
+out:
+    aodev->rc = rc;
+    if(rc) aodev->callback(egc, aodev);
+    return;
+}
+
 void* libxl__device_list(const struct libxl_device_type *dt,
                          libxl_ctx *ctx, uint32_t domid, int *num)
 {
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index c192559..3397655 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3484,13 +3484,17 @@  struct libxl_device_type {
     void (*add)(libxl__egc *, libxl__ao *, uint32_t, libxl_domain_config *,
                 libxl__multidev *);
     void *(*list)(libxl_ctx *, uint32_t, int *);
+    int (*set_default)(libxl__gc *, uint32_t, void *);
+    int (*to_device)(libxl__gc *, uint32_t, void *, libxl__device *);
     void (*init)(void *);
+    void (*copy)(libxl_ctx *, void *, void *);
     void (*dispose)(void *);
     int (*compare)(void *, void *);
     void (*merge)(libxl_ctx *, void *, void *);
     int (*dm_needed)(void *, unsigned);
     void (*update_config)(libxl__gc *, void *, void *);
     int (*from_xenstore)(libxl__gc *, const char *, uint32_t, void *);
+    int (*set_xenstore_config)(libxl__gc *, uint32_t, void *);
 };
 
 #define DEFINE_DEVICE_TYPE_STRUCT_X(name, sname, ...)                          \
@@ -3502,7 +3506,14 @@  struct libxl_device_type {
         .add           = libxl__add_ ## name ## s,                             \
         .list          = (void *(*)(libxl_ctx *, uint32_t, int *))             \
                          libxl_device_ ## sname ## _list,                      \
+        .set_default   = (int (*)(libxl__gc *, uint32_t, void *))              \
+                         libxl__device_ ## sname ## _setdefault,               \
+        .to_device     = (int (*)(libxl__gc *, uint32_t,                       \
+                                  void *, libxl__device *))                    \
+                         libxl__device_from_ ## name,                          \
         .init          = (void (*)(void *))libxl_device_ ## sname ## _init,    \
+        .copy          = (void (*)(libxl_ctx *, void *, void *))               \
+                         libxl_device_ ## sname ## _copy,                      \
         .dispose       = (void (*)(void *))libxl_device_ ## sname ## _dispose, \
         .compare       = (int (*)(void *, void *))                             \
                          libxl_device_ ## sname ## _compare,                   \
@@ -4354,6 +4365,10 @@  static inline bool libxl__acpi_defbool_val(const libxl_domain_build_info *b_info
     return libxl_defbool_val(b_info->acpi) &&
            libxl_defbool_val(b_info->u.hvm.acpi);
 }
+
+void libxl__device_add(libxl__egc *egc, uint32_t domid,
+                       const struct libxl_device_type *dt, void *type,
+                       libxl__ao_device *aodev);
 void* libxl__device_list(const struct libxl_device_type *dt,
                          libxl_ctx *ctx, uint32_t domid, int *num);
 void libxl__device_list_free(const struct libxl_device_type *dt,
diff --git a/tools/libxl/libxl_usb.c b/tools/libxl/libxl_usb.c
index d8948d5..d1ec28f 100644
--- a/tools/libxl/libxl_usb.c
+++ b/tools/libxl/libxl_usb.c
@@ -1968,6 +1968,8 @@  void libxl_device_usbdev_list_free(libxl_device_usbdev *list, int nr)
 DEFINE_DEVICE_TYPE_STRUCT(usbctrl,
     .dm_needed = libxl_device_usbctrl_dm_needed
 );
+
+#define libxl__device_from_usbdev NULL
 DEFINE_DEVICE_TYPE_STRUCT(usbdev);
 
 /*
diff --git a/tools/libxl/libxl_vdispl.c b/tools/libxl/libxl_vdispl.c
index a628adc..c79bcda 100644
--- a/tools/libxl/libxl_vdispl.c
+++ b/tools/libxl/libxl_vdispl.c
@@ -14,6 +14,21 @@ 
 
 #include "libxl_internal.h"
 
+static int libxl__device_vdispl_setdefault(libxl__gc *gc, uint32_t domid,
+                                           libxl_device_vdispl *vdispl)
+{
+    int rc;
+
+    rc = libxl__resolve_domid(gc, vdispl->backend_domname,
+                              &vdispl->backend_domid);
+
+    if (vdispl->devid == -1) {
+        vdispl->devid = libxl__device_nextid(gc, domid, "vdispl");
+    }
+
+    return rc;
+}
+
 static int libxl__device_from_vdispl(libxl__gc *gc, uint32_t domid,
                                      libxl_device_vdispl *vdispl,
                                      libxl__device *device)
@@ -47,7 +62,7 @@  static void libxl__device_vdispl_add(libxl__egc *egc, uint32_t domid,
                                      libxl_device_vdispl *vdispl,
                                      libxl__ao_device *aodev)
 {
-
+    libxl__device_add(egc, domid, &libxl__vdispl_devtype, vdispl, aodev);
 }
 
 libxl_device_vdispl *libxl_device_vdispl_list(libxl_ctx *ctx, uint32_t domid,