Message ID | 1498557807-10810-5-git-send-email-al1img@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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 >
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 >>
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.
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.
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?
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.
>> >> > > 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.
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.
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.
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.
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?
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.
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.
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.
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
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 --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,