[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v4 01/13] libxl: add generic function to add device



On Tue, Sep 5, 2017 at 2:47 PM, Wei Liu <wei.liu2@xxxxxxxxxx> wrote:
> On Tue, Jul 18, 2017 at 05:25:18PM +0300, Oleksandr Grytsov wrote:
>> From: Oleksandr Grytsov <oleksandr_grytsov@xxxxxxxx>
>>
>> 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@xxxxxxxx>
>> ---
>>  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.

-- 
Best Regards,
Oleksandr Grytsov.

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.