| Message ID | 1498557807-10810-4-git-send-email-al1img@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 27, 2017 at 01:03:19PM +0300, Oleksandr Grytsov wrote: > From: Oleksandr Grytsov <oleksandr_grytsov@epam.com> > > Add libxl__device_list, libxl__device_list_free. > Device list is created from libxl xen store entries. > In order to fill libxl device structure from xen store, > the device handling framework extended with from_xenstore callback. > On this callback libxl_device shall be filled with data from > be xen store directory. > > Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com> > --- > tools/libxl/libxl_device.c | 76 ++++++++++++++++++++++++++++++++++++++++++++ > tools/libxl/libxl_internal.h | 8 +++++ > tools/libxl/libxl_vdispl.c | 17 ++++++++-- > 3 files changed, 98 insertions(+), 3 deletions(-) > > diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c > index 00356af..8bcfa2b 100644 > --- a/tools/libxl/libxl_device.c > +++ b/tools/libxl/libxl_device.c > @@ -1793,6 +1793,82 @@ out: > return AO_CREATE_FAIL(rc); > } > > +void* libxl__device_list(const struct libxl_device_type *dt, > + libxl_ctx *ctx, uint32_t domid, int *num) void *libxl_... > +{ > + GC_INIT(ctx); > + > + void *r = NULL; > + void *list = NULL; > + void *item = NULL; > + char *libxl_path; > + char *be_path; > + char** dir = NULL; char **dir > + unsigned int ndirs = 0; > + int rc; > + > + *num = 0; > + > + libxl_path = GCSPRINTF("%s/device/%s", > + libxl__xs_libxl_path(gc, domid), dt->type); > + > + dir = libxl__xs_directory(gc, XBT_NULL, libxl_path, &ndirs); > + > + if (dir && ndirs) { > + list = malloc(dt->dev_elem_size * ndirs); > + void *end = (uint8_t*)list + ndirs * dt->dev_elem_size; (uint8_t *) > + item = list; > + > + while(item < end) { > + be_path = libxl__xs_read(gc, XBT_NULL, > + GCSPRINTF("%s/%s/backend", > + libxl_path, *dir)); > + > + dt->init(item); > + > + if (dt->from_xenstore) > + { Move { to previous line. > + rc = dt->from_xenstore(gc, be_path, atoi(*dir), item); > + if (rc) goto out; > + } > + > + item = (uint8_t*)item + dt->dev_elem_size; > + ++dir; > + } > + } > + > + *num = ndirs; > + r = list; > + list = NULL; > + > +out: > + > + if (list) { > + *num = 0; > + while(item >= list) { Space after while. > + item = (uint8_t*)item - dt->dev_elem_size; > + dt->dispose(item); > + } > + free(list); > + } > + > + GC_FREE; > + > + return r; > +} > + > +void libxl__device_list_free(const struct libxl_device_type *dt, > + void *list, int num) > +{ > + int i; > + > + for (i = 0; i < num; i++) { > + dt->dispose((uint8_t*)list + i * dt->dev_elem_size); > + } > + No need to have {}.
Thanks, I will fix it. On Thu, Jun 29, 2017 at 8:36 PM, Wei Liu <wei.liu2@citrix.com> wrote: > On Tue, Jun 27, 2017 at 01:03:19PM +0300, Oleksandr Grytsov wrote: >> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com> >> >> Add libxl__device_list, libxl__device_list_free. >> Device list is created from libxl xen store entries. >> In order to fill libxl device structure from xen store, >> the device handling framework extended with from_xenstore callback. >> On this callback libxl_device shall be filled with data from >> be xen store directory. >> >> Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com> >> --- >> tools/libxl/libxl_device.c | 76 ++++++++++++++++++++++++++++++++++++++++++++ >> tools/libxl/libxl_internal.h | 8 +++++ >> tools/libxl/libxl_vdispl.c | 17 ++++++++-- >> 3 files changed, 98 insertions(+), 3 deletions(-) >> >> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c >> index 00356af..8bcfa2b 100644 >> --- a/tools/libxl/libxl_device.c >> +++ b/tools/libxl/libxl_device.c >> @@ -1793,6 +1793,82 @@ out: >> return AO_CREATE_FAIL(rc); >> } >> >> +void* libxl__device_list(const struct libxl_device_type *dt, >> + libxl_ctx *ctx, uint32_t domid, int *num) > > void *libxl_... > >> +{ >> + GC_INIT(ctx); >> + >> + void *r = NULL; >> + void *list = NULL; >> + void *item = NULL; >> + char *libxl_path; >> + char *be_path; >> + char** dir = NULL; > > char **dir > >> + unsigned int ndirs = 0; >> + int rc; >> + >> + *num = 0; >> + >> + libxl_path = GCSPRINTF("%s/device/%s", >> + libxl__xs_libxl_path(gc, domid), dt->type); >> + >> + dir = libxl__xs_directory(gc, XBT_NULL, libxl_path, &ndirs); >> + >> + if (dir && ndirs) { >> + list = malloc(dt->dev_elem_size * ndirs); >> + void *end = (uint8_t*)list + ndirs * dt->dev_elem_size; > > (uint8_t *) > >> + item = list; >> + >> + while(item < end) { >> + be_path = libxl__xs_read(gc, XBT_NULL, >> + GCSPRINTF("%s/%s/backend", >> + libxl_path, *dir)); >> + >> + dt->init(item); >> + >> + if (dt->from_xenstore) >> + { > > Move { to previous line. > >> + rc = dt->from_xenstore(gc, be_path, atoi(*dir), item); >> + if (rc) goto out; >> + } >> + >> + item = (uint8_t*)item + dt->dev_elem_size; >> + ++dir; >> + } >> + } >> + >> + *num = ndirs; >> + r = list; >> + list = NULL; >> + >> +out: >> + >> + if (list) { >> + *num = 0; >> + while(item >= list) { > > Space after while. > >> + item = (uint8_t*)item - dt->dev_elem_size; >> + dt->dispose(item); >> + } >> + free(list); >> + } >> + >> + GC_FREE; >> + >> + return r; >> +} >> + >> +void libxl__device_list_free(const struct libxl_device_type *dt, >> + void *list, int num) >> +{ >> + int i; >> + >> + for (i = 0; i < num; i++) { >> + dt->dispose((uint8_t*)list + i * dt->dev_elem_size); >> + } >> + > > No need to have {}.
On Tue, Jun 27, 2017 at 01:03:19PM +0300, Oleksandr Grytsov wrote: > From: Oleksandr Grytsov <oleksandr_grytsov@epam.com> > > Add libxl__device_list, libxl__device_list_free. > Device list is created from libxl xen store entries. > In order to fill libxl device structure from xen store, > the device handling framework extended with from_xenstore callback. > On this callback libxl_device shall be filled with data from > be xen store directory. > > Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com> > --- > tools/libxl/libxl_device.c | 76 ++++++++++++++++++++++++++++++++++++++++++++ > tools/libxl/libxl_internal.h | 8 +++++ > tools/libxl/libxl_vdispl.c | 17 ++++++++-- > 3 files changed, 98 insertions(+), 3 deletions(-) > > diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c > index 00356af..8bcfa2b 100644 > --- a/tools/libxl/libxl_device.c > +++ b/tools/libxl/libxl_device.c > @@ -1793,6 +1793,82 @@ out: > return AO_CREATE_FAIL(rc); > } > > +void* libxl__device_list(const struct libxl_device_type *dt, > + libxl_ctx *ctx, uint32_t domid, int *num) It should probably take a libxl__gc *gc here. > +{ > + GC_INIT(ctx); > + And omit the GC_INIT and GC_FREE. > + void *r = NULL; > + void *list = NULL; > + void *item = NULL; > + char *libxl_path; > + char *be_path; > + char** dir = NULL; > + unsigned int ndirs = 0; > + int rc; > + > + *num = 0; > + > + libxl_path = GCSPRINTF("%s/device/%s", > + libxl__xs_libxl_path(gc, domid), dt->type); > + > + dir = libxl__xs_directory(gc, XBT_NULL, libxl_path, &ndirs); > + > + if (dir && ndirs) { > + list = malloc(dt->dev_elem_size * ndirs); Also please use libxl__malloc here.
On Thu, Jul 6, 2017 at 6:29 PM, Wei Liu <wei.liu2@citrix.com> wrote: > On Tue, Jun 27, 2017 at 01:03:19PM +0300, Oleksandr Grytsov wrote: >> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com> >> >> Add libxl__device_list, libxl__device_list_free. >> Device list is created from libxl xen store entries. >> In order to fill libxl device structure from xen store, >> the device handling framework extended with from_xenstore callback. >> On this callback libxl_device shall be filled with data from >> be xen store directory. >> >> Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com> >> --- >> tools/libxl/libxl_device.c | 76 ++++++++++++++++++++++++++++++++++++++++++++ >> tools/libxl/libxl_internal.h | 8 +++++ >> tools/libxl/libxl_vdispl.c | 17 ++++++++-- >> 3 files changed, 98 insertions(+), 3 deletions(-) >> >> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c >> index 00356af..8bcfa2b 100644 >> --- a/tools/libxl/libxl_device.c >> +++ b/tools/libxl/libxl_device.c >> @@ -1793,6 +1793,82 @@ out: >> return AO_CREATE_FAIL(rc); >> } >> >> +void* libxl__device_list(const struct libxl_device_type *dt, >> + libxl_ctx *ctx, uint32_t domid, int *num) > > It should probably take a libxl__gc *gc here. > >> +{ >> + GC_INIT(ctx); >> + > > And omit the GC_INIT and GC_FREE. > In this case I should move GC_INIT and GC_FREE to above function: libxl_device_vdispl_list(libxl_ctx *ctx, uint32_t domid, int *num) { GC_INIT(ctx); } >> + void *r = NULL; >> + void *list = NULL; >> + void *item = NULL; >> + char *libxl_path; >> + char *be_path; >> + char** dir = NULL; >> + unsigned int ndirs = 0; >> + int rc; >> + >> + *num = 0; >> + >> + libxl_path = GCSPRINTF("%s/device/%s", >> + libxl__xs_libxl_path(gc, domid), dt->type); >> + >> + dir = libxl__xs_directory(gc, XBT_NULL, libxl_path, &ndirs); >> + >> + if (dir && ndirs) { >> + list = malloc(dt->dev_elem_size * ndirs); > > Also please use libxl__malloc here.
On Mon, Jul 10, 2017 at 3:22 PM, Oleksandr Grytsov <al1img@gmail.com> wrote: > On Thu, Jul 6, 2017 at 6:29 PM, Wei Liu <wei.liu2@citrix.com> wrote: >> On Tue, Jun 27, 2017 at 01:03:19PM +0300, Oleksandr Grytsov wrote: >>> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com> >>> >>> Add libxl__device_list, libxl__device_list_free. >>> Device list is created from libxl xen store entries. >>> In order to fill libxl device structure from xen store, >>> the device handling framework extended with from_xenstore callback. >>> On this callback libxl_device shall be filled with data from >>> be xen store directory. >>> >>> Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com> >>> --- >>> tools/libxl/libxl_device.c | 76 ++++++++++++++++++++++++++++++++++++++++++++ >>> tools/libxl/libxl_internal.h | 8 +++++ >>> tools/libxl/libxl_vdispl.c | 17 ++++++++-- >>> 3 files changed, 98 insertions(+), 3 deletions(-) >>> >>> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c >>> index 00356af..8bcfa2b 100644 >>> --- a/tools/libxl/libxl_device.c >>> +++ b/tools/libxl/libxl_device.c >>> @@ -1793,6 +1793,82 @@ out: >>> return AO_CREATE_FAIL(rc); >>> } >>> >>> +void* libxl__device_list(const struct libxl_device_type *dt, >>> + libxl_ctx *ctx, uint32_t domid, int *num) >> >> It should probably take a libxl__gc *gc here. >> >>> +{ >>> + GC_INIT(ctx); >>> + >> >> And omit the GC_INIT and GC_FREE. >> > > In this case I should move GC_INIT and GC_FREE to above function: > > libxl_device_vdispl_list(libxl_ctx *ctx, uint32_t domid, int *num) > { > GC_INIT(ctx); > > > } > It means for each device where getting device list is required there will be GC_INIT(ctc) libxl__device_list(gc, ...) GC_FREE instead of just: libxl__device_list(ctx, ...); >>> + void *r = NULL; >>> + void *list = NULL; >>> + void *item = NULL; >>> + char *libxl_path; >>> + char *be_path; >>> + char** dir = NULL; >>> + unsigned int ndirs = 0; >>> + int rc; >>> + >>> + *num = 0; >>> + >>> + libxl_path = GCSPRINTF("%s/device/%s", >>> + libxl__xs_libxl_path(gc, domid), dt->type); >>> + >>> + dir = libxl__xs_directory(gc, XBT_NULL, libxl_path, &ndirs); >>> + >>> + if (dir && ndirs) { >>> + list = malloc(dt->dev_elem_size * ndirs); >> >> Also please use libxl__malloc here. > > > > -- > Best Regards, > Oleksandr Grytsov.
On Mon, Jul 10, 2017 at 03:22:19PM +0300, Oleksandr Grytsov wrote: > On Thu, Jul 6, 2017 at 6:29 PM, Wei Liu <wei.liu2@citrix.com> wrote: > > On Tue, Jun 27, 2017 at 01:03:19PM +0300, Oleksandr Grytsov wrote: > >> From: Oleksandr Grytsov <oleksandr_grytsov@epam.com> > >> > >> Add libxl__device_list, libxl__device_list_free. > >> Device list is created from libxl xen store entries. > >> In order to fill libxl device structure from xen store, > >> the device handling framework extended with from_xenstore callback. > >> On this callback libxl_device shall be filled with data from > >> be xen store directory. > >> > >> Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@epam.com> > >> --- > >> tools/libxl/libxl_device.c | 76 ++++++++++++++++++++++++++++++++++++++++++++ > >> tools/libxl/libxl_internal.h | 8 +++++ > >> tools/libxl/libxl_vdispl.c | 17 ++++++++-- > >> 3 files changed, 98 insertions(+), 3 deletions(-) > >> > >> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c > >> index 00356af..8bcfa2b 100644 > >> --- a/tools/libxl/libxl_device.c > >> +++ b/tools/libxl/libxl_device.c > >> @@ -1793,6 +1793,82 @@ out: > >> return AO_CREATE_FAIL(rc); > >> } > >> > >> +void* libxl__device_list(const struct libxl_device_type *dt, > >> + libxl_ctx *ctx, uint32_t domid, int *num) > > > > It should probably take a libxl__gc *gc here. > > > >> +{ > >> + GC_INIT(ctx); > >> + > > > > And omit the GC_INIT and GC_FREE. > > > > In this case I should move GC_INIT and GC_FREE to above function: > > libxl_device_vdispl_list(libxl_ctx *ctx, uint32_t domid, int *num) > { > GC_INIT(ctx); > Yes that's what I meant.
On Mon, Jul 10, 2017 at 03:26:12PM +0300, Oleksandr Grytsov wrote: > It means for each device where getting device list is required there will be > GC_INIT(ctc) > > libxl__device_list(gc, ...) > > GC_FREE > > instead of just: > > libxl__device_list(ctx, ...); I think this is worth it because we might need to use the libl__device_list function internally.
On Wed, Jul 12, 2017 at 12:51 PM, Wei Liu <wei.liu2@citrix.com> wrote: > On Mon, Jul 10, 2017 at 03:26:12PM +0300, Oleksandr Grytsov wrote: >> It means for each device where getting device list is required there will be >> GC_INIT(ctc) >> >> libxl__device_list(gc, ...) >> >> GC_FREE >> >> instead of just: >> >> libxl__device_list(ctx, ...); > > I think this is worth it because we might need to use the > libl__device_list function internally. I've reworked the patch series and done it in following way: libxl__device_list takes gc and interface function init CTX: libxl_device_disk *libxl_device_disk_list(libxl_ctx *ctx, uint32_t domid, int *num) { libxl_device_disk *r; GC_INIT(ctx); r = libxl__device_list(gc, &libxl__disk_devtype, domid, "disk", num); GC_FREE; return r; } There was comment to use libxl_malloc instead of malloc in libxl__device_list. But it can't be used because calling GC_FREE frees the list. So I've left malloc and free the list in libxl__device_list_free.
On Wed, Jul 12, 2017 at 04:43:23PM +0300, Oleksandr Grytsov wrote: > On Wed, Jul 12, 2017 at 12:51 PM, Wei Liu <wei.liu2@citrix.com> wrote: > > On Mon, Jul 10, 2017 at 03:26:12PM +0300, Oleksandr Grytsov wrote: > >> It means for each device where getting device list is required there will be > >> GC_INIT(ctc) > >> > >> libxl__device_list(gc, ...) > >> > >> GC_FREE > >> > >> instead of just: > >> > >> libxl__device_list(ctx, ...); > > > > I think this is worth it because we might need to use the > > libl__device_list function internally. > > I've reworked the patch series and done it in following way: > > libxl__device_list takes gc and interface function init CTX: > > libxl_device_disk *libxl_device_disk_list(libxl_ctx *ctx, uint32_t > domid, int *num) > { > libxl_device_disk *r; > > GC_INIT(ctx); > > r = libxl__device_list(gc, &libxl__disk_devtype, domid, "disk", num); > > GC_FREE; > > return r; > } > > There was comment to use libxl_malloc instead of malloc in libxl__device_list. > But it can't be used because calling GC_FREE frees the list. > So I've left malloc and free the list in libxl__device_list_free. > You can pass in NO_GC (NOGC) to libxl__malloc.
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c index 00356af..8bcfa2b 100644 --- a/tools/libxl/libxl_device.c +++ b/tools/libxl/libxl_device.c @@ -1793,6 +1793,82 @@ out: return AO_CREATE_FAIL(rc); } +void* libxl__device_list(const struct libxl_device_type *dt, + libxl_ctx *ctx, uint32_t domid, int *num) +{ + GC_INIT(ctx); + + void *r = NULL; + void *list = NULL; + void *item = NULL; + char *libxl_path; + char *be_path; + char** dir = NULL; + unsigned int ndirs = 0; + int rc; + + *num = 0; + + libxl_path = GCSPRINTF("%s/device/%s", + libxl__xs_libxl_path(gc, domid), dt->type); + + dir = libxl__xs_directory(gc, XBT_NULL, libxl_path, &ndirs); + + if (dir && ndirs) { + list = malloc(dt->dev_elem_size * ndirs); + void *end = (uint8_t*)list + ndirs * dt->dev_elem_size; + item = list; + + while(item < end) { + be_path = libxl__xs_read(gc, XBT_NULL, + GCSPRINTF("%s/%s/backend", + libxl_path, *dir)); + + dt->init(item); + + if (dt->from_xenstore) + { + rc = dt->from_xenstore(gc, be_path, atoi(*dir), item); + if (rc) goto out; + } + + item = (uint8_t*)item + dt->dev_elem_size; + ++dir; + } + } + + *num = ndirs; + r = list; + list = NULL; + +out: + + if (list) { + *num = 0; + while(item >= list) { + item = (uint8_t*)item - dt->dev_elem_size; + dt->dispose(item); + } + free(list); + } + + GC_FREE; + + return r; +} + +void libxl__device_list_free(const struct libxl_device_type *dt, + void *list, int num) +{ + int i; + + for (i = 0; i < num; i++) { + dt->dispose((uint8_t*)list + i * dt->dev_elem_size); + } + + free(list); +} + /* * Local variables: * mode: C diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index afe6652..c192559 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -3484,11 +3484,13 @@ struct libxl_device_type { void (*add)(libxl__egc *, libxl__ao *, uint32_t, libxl_domain_config *, libxl__multidev *); void *(*list)(libxl_ctx *, uint32_t, int *); + void (*init)(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 *); }; #define DEFINE_DEVICE_TYPE_STRUCT_X(name, sname, ...) \ @@ -3500,6 +3502,7 @@ struct libxl_device_type { .add = libxl__add_ ## name ## s, \ .list = (void *(*)(libxl_ctx *, uint32_t, int *)) \ libxl_device_ ## sname ## _list, \ + .init = (void (*)(void *))libxl_device_ ## sname ## _init, \ .dispose = (void (*)(void *))libxl_device_ ## sname ## _dispose, \ .compare = (int (*)(void *, void *)) \ libxl_device_ ## sname ## _compare, \ @@ -3534,6 +3537,7 @@ extern const struct libxl_device_type libxl__vtpm_devtype; extern const struct libxl_device_type libxl__usbctrl_devtype; extern const struct libxl_device_type libxl__usbdev_devtype; extern const struct libxl_device_type libxl__pcidev_devtype; +extern const struct libxl_device_type libxl__vdispl_devtype; extern const struct libxl_device_type *device_type_tbl[]; @@ -4350,6 +4354,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_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, + void *list, int num); #endif /* diff --git a/tools/libxl/libxl_vdispl.c b/tools/libxl/libxl_vdispl.c index 2658e25..a628adc 100644 --- a/tools/libxl/libxl_vdispl.c +++ b/tools/libxl/libxl_vdispl.c @@ -21,6 +21,15 @@ static int libxl__device_from_vdispl(libxl__gc *gc, uint32_t domid, return 0; } +static int libxl__from_xenstore_vdispl(libxl__gc *gc, const char *be_path, + uint32_t devid, + libxl_device_vdispl *vdispl) +{ + vdispl->devid = devid; + + return libxl__backendpath_parse_domid(gc, be_path, &vdispl->backend_domid); +} + static void libxl__update_config_vdispl(libxl__gc *gc, libxl_device_vdispl *dst, libxl_device_vdispl *src) @@ -44,12 +53,12 @@ static void libxl__device_vdispl_add(libxl__egc *egc, uint32_t domid, libxl_device_vdispl *libxl_device_vdispl_list(libxl_ctx *ctx, uint32_t domid, int *num) { - return NULL; + return libxl__device_list(&libxl__vdispl_devtype, ctx, domid, num); } void libxl_device_vdispl_list_free(libxl_device_vdispl* list, int num) { - + libxl__device_list_free(&libxl__vdispl_devtype, list, num); } int libxl_device_vdispl_getinfo(libxl_ctx *ctx, uint32_t domid, @@ -65,7 +74,9 @@ LIBXL_DEFINE_DEVICE_REMOVE(vdispl) DEFINE_DEVICE_TYPE_STRUCT(vdispl, .update_config = (void (*)(libxl__gc *, void *, void *)) - libxl__update_config_vdispl + libxl__update_config_vdispl, + .from_xenstore = (int (*)(libxl__gc *, const char *, uint32_t, void *)) + libxl__from_xenstore_vdispl ); /*