Re: [Xen-devel] [PATCH v3 04/11] libxl: add generic function to add device

[Oleksandr Grytsov <al1img@xxxxxxxxx>]

On Fri, Jul 7, 2017 at 12:49 PM, Oleksandr Grytsov <al1img@xxxxxxxxx> wrote:
> On Thu, Jul 6, 2017 at 6:51 PM, Wei Liu <wei.liu2@xxxxxxxxxx> wrote:
>> On Tue, Jun 27, 2017 at 01:03:20PM +0300, Oleksandr Grytsov wrote:
>>> From: Oleksandr Grytsov <oleksandr_grytsov@xxxxxxxx>
>>>
>>> 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@xxxxxxxx>
>> [...]
>>> +
>>> +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.



-- 
Best Regards,
Oleksandr Grytsov.

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