| Message ID | 1500387930-16317-2-git-send-email-al1img@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 18, 2017 at 05:25:18PM +0300, Oleksandr Grytsov wrote: > From: Oleksandr Grytsov <oleksandr_grytsov@epam.com> > > Add libxl__device_add to simple write XenStore device conifg > and libxl__device_add_async to update domain configuration > and write XenStore device config asynchroniously. > Almost all devices have similar libxl__device_xxxx_add function. > This generic functions implement same functionality but > using the device handling framework. Th device specific > part such as setting xen store configurationis moved > to set_xenstore_config callback of the device framework. > The two add functions look correct. Some comments below. > Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com> > --- > tools/libxl/libxl_create.c | 3 + > tools/libxl/libxl_device.c | 198 +++++++++++++++++++++++++++++++++++++++++++ > tools/libxl/libxl_disk.c | 2 + > tools/libxl/libxl_internal.h | 36 ++++++++ > tools/libxl/libxl_nic.c | 2 + > tools/libxl/libxl_pci.c | 2 + > tools/libxl/libxl_usb.c | 6 ++ > tools/libxl/libxl_vtpm.c | 2 + > 8 files changed, 251 insertions(+) > > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c > index bffbc45..b2163cd 100644 > --- a/tools/libxl/libxl_create.c > +++ b/tools/libxl/libxl_create.c > @@ -1430,6 +1430,9 @@ 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 > +#define libxl__device_dtdev_update_devid 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 00356af..07165f0 100644 > --- a/tools/libxl/libxl_device.c > +++ b/tools/libxl/libxl_device.c > @@ -1793,6 +1793,204 @@ out: > return AO_CREATE_FAIL(rc); > } > > +static void device_add_domain_config(libxl__gc *gc, > + libxl_domain_config *d_config, > + const struct libxl_device_type *dt, > + void *type) > +{ > + int *num_dev; > + int i; unsigned int please. > + void *item = NULL; > + > + 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); Space after devs. > + *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); > +} > + > +void libxl__device_add_async(libxl__egc *egc, uint32_t domid, > + const struct libxl_device_type *dt, void *type, > + libxl__ao_device *aodev) > +{ > + STATE_AO_GC(aodev->ao); > + flexarray_t *back; > + flexarray_t *front, *ro_front; > + libxl__device *device; > + xs_transaction_t t = XBT_NULL; > + libxl_domain_config d_config; > + void *type_saved; > + libxl__domain_userdata_lock *lock = NULL; > + int rc; > + > + libxl_domain_config_init(&d_config); > + > + type_saved = libxl__malloc(gc, dt->dev_elem_size); > + > + dt->init(type_saved); > + dt->copy(CTX, type_saved, type); > + > + if (dt->set_default) { > + rc = dt->set_default(gc, domid, type, aodev->update_json); > + if (rc) goto out; > + } > + > + if (dt->update_devid) { > + rc = dt->update_devid(gc, domid, type); > + if (rc) goto out; > + } > + > + if (dt->update_config) > + dt->update_config(gc, type_saved, type); > + > + GCNEW(device); > + rc = dt->to_device(gc, domid, type, device); > + if (rc) goto out; > + > + if (aodev->update_json) { > + Extraneous empty line here. > + 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; > + > + device_add_domain_config(gc, &d_config, dt, type_saved); > + > + rc = libxl__dm_check_start(gc, &d_config, domid); > + if (rc) goto out; > + } > + > + back = flexarray_make(gc, 16, 1); > + front = flexarray_make(gc, 16, 1); > + ro_front = flexarray_make(gc, 16, 1); > + > + flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid)); > + flexarray_append_pair(back, "online", "1"); > + flexarray_append_pair(back, "state", > + GCSPRINTF("%d", XenbusStateInitialising)); > + > + flexarray_append_pair(front, "backend-id", > + GCSPRINTF("%d", device->backend_domid)); > + flexarray_append_pair(front, "state", > + GCSPRINTF("%d", XenbusStateInitialising)); > + > + if (dt->set_xenstore_config) > + dt->set_xenstore_config(gc, domid, type, back, front, ro_front); > + > + for (;;) { > + rc = libxl__xs_transaction_start(gc, &t); > + if (rc) goto out; > + > + rc = libxl__device_exists(gc, t, 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 = libxl__set_domain_configuration(gc, domid, &d_config); > + if (rc) goto out; > + } > + > + libxl__device_generic_add(gc, t, device, > + libxl__xs_kvs_of_flexarray(gc, back), > + libxl__xs_kvs_of_flexarray(gc, front), > + libxl__xs_kvs_of_flexarray(gc, ro_front)); > + > + rc = libxl__xs_transaction_commit(gc, &t); > + if (!rc) break; > + if (rc < 0) goto out; > + } > + > + aodev->dev = device; > + aodev->action = LIBXL__DEVICE_ACTION_ADD; > + libxl__wait_device_connection(egc, aodev); > + > + rc = 0; > + > +out: > + libxl__xs_transaction_abort(gc, &t); > + if (lock) libxl__unlock_domain_userdata(lock); > + dt->dispose(type_saved); > + libxl_domain_config_dispose(&d_config); > + aodev->rc = rc; > + if (rc) aodev->callback(egc, aodev); > + return; > +} > + > +int libxl__device_add(libxl__gc *gc, uint32_t domid, > + const struct libxl_device_type *dt, void *type) > +{ > + flexarray_t *back; > + flexarray_t *front, *ro_front; > + libxl__device *device; > + int rc; > + > + if (dt->set_default) { > + rc = dt->set_default(gc, domid, type, false); > + if (rc) goto out; > + } > + > + if (dt->update_devid) { > + rc = dt->update_devid(gc, domid, type); > + if (rc) goto out; > + } > + > + GCNEW(device); > + rc = dt->to_device(gc, domid, type, device); > + if (rc) goto out; > + > + back = flexarray_make(gc, 16, 1); > + front = flexarray_make(gc, 16, 1); > + ro_front = flexarray_make(gc, 16, 1); > + > + flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid)); > + flexarray_append_pair(back, "online", "1"); > + flexarray_append_pair(back, "state", > + GCSPRINTF("%d", XenbusStateInitialising)); > + flexarray_append_pair(front, "backend-id", > + libxl__sprintf(gc, "%d", device->backend_domid)); > + flexarray_append_pair(front, "state", > + GCSPRINTF("%d", XenbusStateInitialising)); > + > + if (dt->set_xenstore_config) > + dt->set_xenstore_config(gc, domid, type, back, front, ro_front); > + > + rc = libxl__device_generic_add(gc, XBT_NULL, device, > + libxl__xs_kvs_of_flexarray(gc, back), > + libxl__xs_kvs_of_flexarray(gc, front), > + libxl__xs_kvs_of_flexarray(gc, ro_front)); > + if (rc) goto out; > + > + rc = 0; > + > +out: > + return rc; > +} > + > /* > * Local variables: > * mode: C > diff --git a/tools/libxl/libxl_disk.c b/tools/libxl/libxl_disk.c > index 63de75c..f2f3635 100644 > --- a/tools/libxl/libxl_disk.c > +++ b/tools/libxl/libxl_disk.c > @@ -1244,6 +1244,8 @@ static int libxl_device_disk_dm_needed(void *e, unsigned domid) > elem->backend_domid == domid; > } > > +#define libxl__device_disk_update_devid NULL > + Is this correct for disk (and other device types as well)? Since you've defined LIBXL_DEFINE_UPDATE_DEVID, you should be able to use that immediately?
On Tue, Sep 5, 2017 at 2:47 PM, Wei Liu <wei.liu2@citrix.com> wrote: > On Tue, Jul 18, 2017 at 05:25:18PM +0300, Oleksandr Grytsov wrote: >> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com> >> >> Add libxl__device_add to simple write XenStore device conifg >> and libxl__device_add_async to update domain configuration >> and write XenStore device config asynchroniously. >> Almost all devices have similar libxl__device_xxxx_add function. >> This generic functions implement same functionality but >> using the device handling framework. Th device specific >> part such as setting xen store configurationis moved >> to set_xenstore_config callback of the device framework. >> > > The two add functions look correct. > > Some comments below. > >> Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com> >> --- >> tools/libxl/libxl_create.c | 3 + >> tools/libxl/libxl_device.c | 198 +++++++++++++++++++++++++++++++++++++++++++ >> tools/libxl/libxl_disk.c | 2 + >> tools/libxl/libxl_internal.h | 36 ++++++++ >> tools/libxl/libxl_nic.c | 2 + >> tools/libxl/libxl_pci.c | 2 + >> tools/libxl/libxl_usb.c | 6 ++ >> tools/libxl/libxl_vtpm.c | 2 + >> 8 files changed, 251 insertions(+) >> >> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c >> index bffbc45..b2163cd 100644 >> --- a/tools/libxl/libxl_create.c >> +++ b/tools/libxl/libxl_create.c >> @@ -1430,6 +1430,9 @@ 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 >> +#define libxl__device_dtdev_update_devid 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 00356af..07165f0 100644 >> --- a/tools/libxl/libxl_device.c >> +++ b/tools/libxl/libxl_device.c >> @@ -1793,6 +1793,204 @@ out: >> return AO_CREATE_FAIL(rc); >> } >> >> +static void device_add_domain_config(libxl__gc *gc, >> + libxl_domain_config *d_config, >> + const struct libxl_device_type *dt, >> + void *type) >> +{ >> + int *num_dev; >> + int i; > > unsigned int please. For "i" counter only or for num_dev as well? For "i" is ok but num_dev better to keep int. > >> + void *item = NULL; >> + >> + 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); > > Space after devs. > >> + *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); >> +} >> + >> +void libxl__device_add_async(libxl__egc *egc, uint32_t domid, >> + const struct libxl_device_type *dt, void *type, >> + libxl__ao_device *aodev) >> +{ >> + STATE_AO_GC(aodev->ao); >> + flexarray_t *back; >> + flexarray_t *front, *ro_front; >> + libxl__device *device; >> + xs_transaction_t t = XBT_NULL; >> + libxl_domain_config d_config; >> + void *type_saved; >> + libxl__domain_userdata_lock *lock = NULL; >> + int rc; >> + >> + libxl_domain_config_init(&d_config); >> + >> + type_saved = libxl__malloc(gc, dt->dev_elem_size); >> + >> + dt->init(type_saved); >> + dt->copy(CTX, type_saved, type); >> + >> + if (dt->set_default) { >> + rc = dt->set_default(gc, domid, type, aodev->update_json); >> + if (rc) goto out; >> + } >> + >> + if (dt->update_devid) { >> + rc = dt->update_devid(gc, domid, type); >> + if (rc) goto out; >> + } >> + >> + if (dt->update_config) >> + dt->update_config(gc, type_saved, type); >> + >> + GCNEW(device); >> + rc = dt->to_device(gc, domid, type, device); >> + if (rc) goto out; >> + >> + if (aodev->update_json) { >> + > > Extraneous empty line here. > >> + 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; >> + >> + device_add_domain_config(gc, &d_config, dt, type_saved); >> + >> + rc = libxl__dm_check_start(gc, &d_config, domid); >> + if (rc) goto out; >> + } >> + >> + back = flexarray_make(gc, 16, 1); >> + front = flexarray_make(gc, 16, 1); >> + ro_front = flexarray_make(gc, 16, 1); >> + >> + flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid)); >> + flexarray_append_pair(back, "online", "1"); >> + flexarray_append_pair(back, "state", >> + GCSPRINTF("%d", XenbusStateInitialising)); >> + >> + flexarray_append_pair(front, "backend-id", >> + GCSPRINTF("%d", device->backend_domid)); >> + flexarray_append_pair(front, "state", >> + GCSPRINTF("%d", XenbusStateInitialising)); >> + >> + if (dt->set_xenstore_config) >> + dt->set_xenstore_config(gc, domid, type, back, front, ro_front); >> + >> + for (;;) { >> + rc = libxl__xs_transaction_start(gc, &t); >> + if (rc) goto out; >> + >> + rc = libxl__device_exists(gc, t, 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 = libxl__set_domain_configuration(gc, domid, &d_config); >> + if (rc) goto out; >> + } >> + >> + libxl__device_generic_add(gc, t, device, >> + libxl__xs_kvs_of_flexarray(gc, back), >> + libxl__xs_kvs_of_flexarray(gc, front), >> + libxl__xs_kvs_of_flexarray(gc, ro_front)); >> + >> + rc = libxl__xs_transaction_commit(gc, &t); >> + if (!rc) break; >> + if (rc < 0) goto out; >> + } >> + >> + aodev->dev = device; >> + aodev->action = LIBXL__DEVICE_ACTION_ADD; >> + libxl__wait_device_connection(egc, aodev); >> + >> + rc = 0; >> + >> +out: >> + libxl__xs_transaction_abort(gc, &t); >> + if (lock) libxl__unlock_domain_userdata(lock); >> + dt->dispose(type_saved); >> + libxl_domain_config_dispose(&d_config); >> + aodev->rc = rc; >> + if (rc) aodev->callback(egc, aodev); >> + return; >> +} >> + >> +int libxl__device_add(libxl__gc *gc, uint32_t domid, >> + const struct libxl_device_type *dt, void *type) >> +{ >> + flexarray_t *back; >> + flexarray_t *front, *ro_front; >> + libxl__device *device; >> + int rc; >> + >> + if (dt->set_default) { >> + rc = dt->set_default(gc, domid, type, false); >> + if (rc) goto out; >> + } >> + >> + if (dt->update_devid) { >> + rc = dt->update_devid(gc, domid, type); >> + if (rc) goto out; >> + } >> + >> + GCNEW(device); >> + rc = dt->to_device(gc, domid, type, device); >> + if (rc) goto out; >> + >> + back = flexarray_make(gc, 16, 1); >> + front = flexarray_make(gc, 16, 1); >> + ro_front = flexarray_make(gc, 16, 1); >> + >> + flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid)); >> + flexarray_append_pair(back, "online", "1"); >> + flexarray_append_pair(back, "state", >> + GCSPRINTF("%d", XenbusStateInitialising)); >> + flexarray_append_pair(front, "backend-id", >> + libxl__sprintf(gc, "%d", device->backend_domid)); >> + flexarray_append_pair(front, "state", >> + GCSPRINTF("%d", XenbusStateInitialising)); >> + >> + if (dt->set_xenstore_config) >> + dt->set_xenstore_config(gc, domid, type, back, front, ro_front); >> + >> + rc = libxl__device_generic_add(gc, XBT_NULL, device, >> + libxl__xs_kvs_of_flexarray(gc, back), >> + libxl__xs_kvs_of_flexarray(gc, front), >> + libxl__xs_kvs_of_flexarray(gc, ro_front)); >> + if (rc) goto out; >> + >> + rc = 0; >> + >> +out: >> + return rc; >> +} >> + >> /* >> * Local variables: >> * mode: C >> diff --git a/tools/libxl/libxl_disk.c b/tools/libxl/libxl_disk.c >> index 63de75c..f2f3635 100644 >> --- a/tools/libxl/libxl_disk.c >> +++ b/tools/libxl/libxl_disk.c >> @@ -1244,6 +1244,8 @@ static int libxl_device_disk_dm_needed(void *e, unsigned domid) >> elem->backend_domid == domid; >> } >> >> +#define libxl__device_disk_update_devid NULL >> + > > Is this correct for disk (and other device types as well)? What exactly is correct? libxl__device_disk_update_devid NULL or libxl__device_add_async function? > > Since you've defined LIBXL_DEFINE_UPDATE_DEVID, you should be able to > use that immediately? Actually disk doesn't call update dev ID. So assigning it to NULL I guess is ok here.
On Tue, Sep 05, 2017 at 07:44:34PM +0300, Oleksandr Grytsov wrote: > On Tue, Sep 5, 2017 at 2:47 PM, Wei Liu <wei.liu2@citrix.com> wrote: > > On Tue, Jul 18, 2017 at 05:25:18PM +0300, Oleksandr Grytsov wrote: > >> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com> > >> > >> Add libxl__device_add to simple write XenStore device conifg > >> and libxl__device_add_async to update domain configuration > >> and write XenStore device config asynchroniously. > >> Almost all devices have similar libxl__device_xxxx_add function. > >> This generic functions implement same functionality but > >> using the device handling framework. Th device specific > >> part such as setting xen store configurationis moved > >> to set_xenstore_config callback of the device framework. > >> > > > > The two add functions look correct. > > > > Some comments below. > > > >> Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com> > >> --- > >> tools/libxl/libxl_create.c | 3 + > >> tools/libxl/libxl_device.c | 198 +++++++++++++++++++++++++++++++++++++++++++ > >> tools/libxl/libxl_disk.c | 2 + > >> tools/libxl/libxl_internal.h | 36 ++++++++ > >> tools/libxl/libxl_nic.c | 2 + > >> tools/libxl/libxl_pci.c | 2 + > >> tools/libxl/libxl_usb.c | 6 ++ > >> tools/libxl/libxl_vtpm.c | 2 + > >> 8 files changed, 251 insertions(+) > >> > >> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c > >> index bffbc45..b2163cd 100644 > >> --- a/tools/libxl/libxl_create.c > >> +++ b/tools/libxl/libxl_create.c > >> @@ -1430,6 +1430,9 @@ 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 > >> +#define libxl__device_dtdev_update_devid 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 00356af..07165f0 100644 > >> --- a/tools/libxl/libxl_device.c > >> +++ b/tools/libxl/libxl_device.c > >> @@ -1793,6 +1793,204 @@ out: > >> return AO_CREATE_FAIL(rc); > >> } > >> > >> +static void device_add_domain_config(libxl__gc *gc, > >> + libxl_domain_config *d_config, > >> + const struct libxl_device_type *dt, > >> + void *type) > >> +{ > >> + int *num_dev; > >> + int i; > > > > unsigned int please. > > For "i" counter only or for num_dev as well? > For "i" is ok but num_dev better to keep int. > For i only. > >> * mode: C > >> diff --git a/tools/libxl/libxl_disk.c b/tools/libxl/libxl_disk.c > >> index 63de75c..f2f3635 100644 > >> --- a/tools/libxl/libxl_disk.c > >> +++ b/tools/libxl/libxl_disk.c > >> @@ -1244,6 +1244,8 @@ static int libxl_device_disk_dm_needed(void *e, unsigned domid) > >> elem->backend_domid == domid; > >> } > >> > >> +#define libxl__device_disk_update_devid NULL > >> + > > > > Is this correct for disk (and other device types as well)? > > What exactly is correct? libxl__device_disk_update_devid NULL or > libxl__device_add_async function? > Defining all the update_devid functions to be NULL. They should be defined with the macros now, right? I notice in later patches they are changed, so I'm not too fuss either way. If you want to keep them to be defined as NULL please say so in commit message. > > > > Since you've defined LIBXL_DEFINE_UPDATE_DEVID, you should be able to > > use that immediately? > > Actually disk doesn't call update dev ID. So assigning it to NULL I > guess is ok here. > Yes. I think so. Please consider other device types then. > -- > Best Regards, > Oleksandr Grytsov.
On Wed, Sep 6, 2017 at 12:36 PM, Wei Liu <wei.liu2@citrix.com> wrote: > On Tue, Sep 05, 2017 at 07:44:34PM +0300, Oleksandr Grytsov wrote: >> On Tue, Sep 5, 2017 at 2:47 PM, Wei Liu <wei.liu2@citrix.com> wrote: >> > On Tue, Jul 18, 2017 at 05:25:18PM +0300, Oleksandr Grytsov wrote: >> >> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com> >> >> >> >> Add libxl__device_add to simple write XenStore device conifg >> >> and libxl__device_add_async to update domain configuration >> >> and write XenStore device config asynchroniously. >> >> Almost all devices have similar libxl__device_xxxx_add function. >> >> This generic functions implement same functionality but >> >> using the device handling framework. Th device specific >> >> part such as setting xen store configurationis moved >> >> to set_xenstore_config callback of the device framework. >> >> >> > >> > The two add functions look correct. >> > >> > Some comments below. >> > >> >> Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com> >> >> --- >> >> tools/libxl/libxl_create.c | 3 + >> >> tools/libxl/libxl_device.c | 198 +++++++++++++++++++++++++++++++++++++++++++ >> >> tools/libxl/libxl_disk.c | 2 + >> >> tools/libxl/libxl_internal.h | 36 ++++++++ >> >> tools/libxl/libxl_nic.c | 2 + >> >> tools/libxl/libxl_pci.c | 2 + >> >> tools/libxl/libxl_usb.c | 6 ++ >> >> tools/libxl/libxl_vtpm.c | 2 + >> >> 8 files changed, 251 insertions(+) >> >> >> >> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c >> >> index bffbc45..b2163cd 100644 >> >> --- a/tools/libxl/libxl_create.c >> >> +++ b/tools/libxl/libxl_create.c >> >> @@ -1430,6 +1430,9 @@ 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 >> >> +#define libxl__device_dtdev_update_devid 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 00356af..07165f0 100644 >> >> --- a/tools/libxl/libxl_device.c >> >> +++ b/tools/libxl/libxl_device.c >> >> @@ -1793,6 +1793,204 @@ out: >> >> return AO_CREATE_FAIL(rc); >> >> } >> >> >> >> +static void device_add_domain_config(libxl__gc *gc, >> >> + libxl_domain_config *d_config, >> >> + const struct libxl_device_type *dt, >> >> + void *type) >> >> +{ >> >> + int *num_dev; >> >> + int i; >> > >> > unsigned int please. >> >> For "i" counter only or for num_dev as well? >> For "i" is ok but num_dev better to keep int. >> > > For i only. > >> >> * mode: C >> >> diff --git a/tools/libxl/libxl_disk.c b/tools/libxl/libxl_disk.c >> >> index 63de75c..f2f3635 100644 >> >> --- a/tools/libxl/libxl_disk.c >> >> +++ b/tools/libxl/libxl_disk.c >> >> @@ -1244,6 +1244,8 @@ static int libxl_device_disk_dm_needed(void *e, unsigned domid) >> >> elem->backend_domid == domid; >> >> } >> >> >> >> +#define libxl__device_disk_update_devid NULL >> >> + >> > >> > Is this correct for disk (and other device types as well)? >> >> What exactly is correct? libxl__device_disk_update_devid NULL or >> libxl__device_add_async function? >> > > Defining all the update_devid functions to be NULL. They should be > defined with the macros now, right? > > I notice in later patches they are changed, so I'm not too fuss either > way. If you want to keep them to be defined as NULL please say so in > commit message. > >> > >> > Since you've defined LIBXL_DEFINE_UPDATE_DEVID, you should be able to >> > use that immediately? >> >> Actually disk doesn't call update dev ID. So assigning it to NULL I >> guess is ok here. >> > > Yes. I think so. Please consider other device types then. Ok, I will use the macro in every device which need update devid in this patch. Also I will call this function in add device function.
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c index bffbc45..b2163cd 100644 --- a/tools/libxl/libxl_create.c +++ b/tools/libxl/libxl_create.c @@ -1430,6 +1430,9 @@ 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 +#define libxl__device_dtdev_update_devid 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 00356af..07165f0 100644 --- a/tools/libxl/libxl_device.c +++ b/tools/libxl/libxl_device.c @@ -1793,6 +1793,204 @@ out: return AO_CREATE_FAIL(rc); } +static void device_add_domain_config(libxl__gc *gc, + libxl_domain_config *d_config, + const struct libxl_device_type *dt, + void *type) +{ + int *num_dev; + int i; + void *item = NULL; + + 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); +} + +void libxl__device_add_async(libxl__egc *egc, uint32_t domid, + const struct libxl_device_type *dt, void *type, + libxl__ao_device *aodev) +{ + STATE_AO_GC(aodev->ao); + flexarray_t *back; + flexarray_t *front, *ro_front; + libxl__device *device; + xs_transaction_t t = XBT_NULL; + libxl_domain_config d_config; + void *type_saved; + libxl__domain_userdata_lock *lock = NULL; + int rc; + + libxl_domain_config_init(&d_config); + + type_saved = libxl__malloc(gc, dt->dev_elem_size); + + dt->init(type_saved); + dt->copy(CTX, type_saved, type); + + if (dt->set_default) { + rc = dt->set_default(gc, domid, type, aodev->update_json); + if (rc) goto out; + } + + if (dt->update_devid) { + rc = dt->update_devid(gc, domid, type); + if (rc) goto out; + } + + if (dt->update_config) + dt->update_config(gc, type_saved, type); + + GCNEW(device); + rc = dt->to_device(gc, domid, type, device); + if (rc) goto out; + + if (aodev->update_json) { + + 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; + + device_add_domain_config(gc, &d_config, dt, type_saved); + + rc = libxl__dm_check_start(gc, &d_config, domid); + if (rc) goto out; + } + + back = flexarray_make(gc, 16, 1); + front = flexarray_make(gc, 16, 1); + ro_front = flexarray_make(gc, 16, 1); + + flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid)); + flexarray_append_pair(back, "online", "1"); + flexarray_append_pair(back, "state", + GCSPRINTF("%d", XenbusStateInitialising)); + + flexarray_append_pair(front, "backend-id", + GCSPRINTF("%d", device->backend_domid)); + flexarray_append_pair(front, "state", + GCSPRINTF("%d", XenbusStateInitialising)); + + if (dt->set_xenstore_config) + dt->set_xenstore_config(gc, domid, type, back, front, ro_front); + + for (;;) { + rc = libxl__xs_transaction_start(gc, &t); + if (rc) goto out; + + rc = libxl__device_exists(gc, t, 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 = libxl__set_domain_configuration(gc, domid, &d_config); + if (rc) goto out; + } + + libxl__device_generic_add(gc, t, device, + libxl__xs_kvs_of_flexarray(gc, back), + libxl__xs_kvs_of_flexarray(gc, front), + libxl__xs_kvs_of_flexarray(gc, ro_front)); + + rc = libxl__xs_transaction_commit(gc, &t); + if (!rc) break; + if (rc < 0) goto out; + } + + aodev->dev = device; + aodev->action = LIBXL__DEVICE_ACTION_ADD; + libxl__wait_device_connection(egc, aodev); + + rc = 0; + +out: + libxl__xs_transaction_abort(gc, &t); + if (lock) libxl__unlock_domain_userdata(lock); + dt->dispose(type_saved); + libxl_domain_config_dispose(&d_config); + aodev->rc = rc; + if (rc) aodev->callback(egc, aodev); + return; +} + +int libxl__device_add(libxl__gc *gc, uint32_t domid, + const struct libxl_device_type *dt, void *type) +{ + flexarray_t *back; + flexarray_t *front, *ro_front; + libxl__device *device; + int rc; + + if (dt->set_default) { + rc = dt->set_default(gc, domid, type, false); + if (rc) goto out; + } + + if (dt->update_devid) { + rc = dt->update_devid(gc, domid, type); + if (rc) goto out; + } + + GCNEW(device); + rc = dt->to_device(gc, domid, type, device); + if (rc) goto out; + + back = flexarray_make(gc, 16, 1); + front = flexarray_make(gc, 16, 1); + ro_front = flexarray_make(gc, 16, 1); + + flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid)); + flexarray_append_pair(back, "online", "1"); + flexarray_append_pair(back, "state", + GCSPRINTF("%d", XenbusStateInitialising)); + flexarray_append_pair(front, "backend-id", + libxl__sprintf(gc, "%d", device->backend_domid)); + flexarray_append_pair(front, "state", + GCSPRINTF("%d", XenbusStateInitialising)); + + if (dt->set_xenstore_config) + dt->set_xenstore_config(gc, domid, type, back, front, ro_front); + + rc = libxl__device_generic_add(gc, XBT_NULL, device, + libxl__xs_kvs_of_flexarray(gc, back), + libxl__xs_kvs_of_flexarray(gc, front), + libxl__xs_kvs_of_flexarray(gc, ro_front)); + if (rc) goto out; + + rc = 0; + +out: + return rc; +} + /* * Local variables: * mode: C diff --git a/tools/libxl/libxl_disk.c b/tools/libxl/libxl_disk.c index 63de75c..f2f3635 100644 --- a/tools/libxl/libxl_disk.c +++ b/tools/libxl/libxl_disk.c @@ -1244,6 +1244,8 @@ static int libxl_device_disk_dm_needed(void *e, unsigned domid) elem->backend_domid == domid; } +#define libxl__device_disk_update_devid NULL + DEFINE_DEVICE_TYPE_STRUCT(disk, .merge = libxl_device_disk_merge, .dm_needed = libxl_device_disk_dm_needed, diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index afe6652..075dfe3 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -3467,6 +3467,18 @@ _hidden void libxl__bootloader_run(libxl__egc*, libxl__bootloader_state *st); return AO_INPROGRESS; \ } +#define LIBXL_DEFINE_UPDATE_DEVID(type, name) \ + int libxl__device_##type##_update_devid(libxl__gc *gc, \ + uint32_t domid, \ + libxl_device_##type *type) \ + { \ + if (type->devid == -1) \ + type->devid = libxl__device_nextid(gc, domid, name); \ + if (type->devid < 0) \ + return ERROR_FAIL; \ + return 0; \ + } + #define LIBXL_DEFINE_DEVICE_REMOVE(type) \ LIBXL_DEFINE_DEVICE_REMOVE_EXT(type, generic, remove, 0) \ LIBXL_DEFINE_DEVICE_REMOVE_EXT(type, generic, destroy, 1) @@ -3484,11 +3496,18 @@ 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 *, bool); + 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 (*update_devid)(libxl__gc *, uint32_t, void *); + int (*set_xenstore_config)(libxl__gc *, uint32_t, void *, flexarray_t *, + flexarray_t *, flexarray_t *); }; #define DEFINE_DEVICE_TYPE_STRUCT_X(name, sname, ...) \ @@ -3500,9 +3519,19 @@ 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 *, bool hotplug))\ + 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, \ + .update_devid = (int (*)(libxl__gc *, uint32_t, void *)) \ + libxl__device_ ## sname ## _update_devid, \ __VA_ARGS__ \ } @@ -4350,6 +4379,13 @@ 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_async(libxl__egc *egc, uint32_t domid, + const struct libxl_device_type *dt, void *type, + libxl__ao_device *aodev); +int libxl__device_add(libxl__gc *gc, uint32_t domid, + const struct libxl_device_type *dt, void *type); + #endif /* diff --git a/tools/libxl/libxl_nic.c b/tools/libxl/libxl_nic.c index 4b6e8c0..dd07a6c 100644 --- a/tools/libxl/libxl_nic.c +++ b/tools/libxl/libxl_nic.c @@ -669,6 +669,8 @@ LIBXL_DEFINE_DEVICE_ADD(nic) LIBXL_DEFINE_DEVICES_ADD(nic) LIBXL_DEFINE_DEVICE_REMOVE(nic) +#define libxl__device_nic_update_devid NULL + DEFINE_DEVICE_TYPE_STRUCT(nic, .update_config = libxl_device_nic_update_config ); diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c index b14df16..c3f1e5c 100644 --- a/tools/libxl/libxl_pci.c +++ b/tools/libxl/libxl_pci.c @@ -1708,6 +1708,8 @@ static int libxl_device_pci_compare(libxl_device_pci *d1, return COMPARE_PCI(d1, d2); } +#define libxl__device_pci_update_devid NULL + DEFINE_DEVICE_TYPE_STRUCT_X(pcidev, pci); /* diff --git a/tools/libxl/libxl_usb.c b/tools/libxl/libxl_usb.c index d8948d5..07fb202 100644 --- a/tools/libxl/libxl_usb.c +++ b/tools/libxl/libxl_usb.c @@ -1965,9 +1965,15 @@ void libxl_device_usbdev_list_free(libxl_device_usbdev *list, int nr) free(list); } +#define libxl__device_usbctrl_update_devid NULL + DEFINE_DEVICE_TYPE_STRUCT(usbctrl, .dm_needed = libxl_device_usbctrl_dm_needed ); + +#define libxl__device_from_usbdev NULL +#define libxl__device_usbdev_update_devid NULL + DEFINE_DEVICE_TYPE_STRUCT(usbdev); /* diff --git a/tools/libxl/libxl_vtpm.c b/tools/libxl/libxl_vtpm.c index 9ee8cce..cbd5461 100644 --- a/tools/libxl/libxl_vtpm.c +++ b/tools/libxl/libxl_vtpm.c @@ -364,6 +364,8 @@ LIBXL_DEFINE_DEVICE_ADD(vtpm) static LIBXL_DEFINE_DEVICES_ADD(vtpm) LIBXL_DEFINE_DEVICE_REMOVE(vtpm) +#define libxl__device_vtpm_update_devid NULL + DEFINE_DEVICE_TYPE_STRUCT(vtpm, .update_config = libxl_device_vtpm_update_config );