Re: [Xen-devel] [PATCH v5 10/12] libxl: change nic to use generec add function

[Oleksandr Grytsov <al1img@xxxxxxxxx>]

On Tue, Sep 12, 2017 at 2:35 PM, Oleksandr Grytsov <al1img@xxxxxxxxx> wrote:
> On Tue, Sep 12, 2017 at 2:08 PM, Wei Liu <wei.liu2@xxxxxxxxxx> wrote:
>> On Tue, Sep 12, 2017 at 11:52:17AM +0100, Wei Liu wrote:
>>> On Mon, Sep 11, 2017 at 08:15:00PM +0300, Oleksandr Grytsov wrote:
>>> > From: Oleksandr Grytsov <oleksandr_grytsov@xxxxxxxx>
>>> >
>>> > Signed-off-by: Oleksandr Grytsov <oleksandr_grytsov@xxxxxxxx>
>>> > ---
>>> >  tools/libxl/libxl.h                   |   9 +-
>>> >  tools/libxl/libxl_checkpoint_device.c |   9 +-
>>> >  tools/libxl/libxl_colo_save.c         |   4 +-
>>> >  tools/libxl/libxl_dm.c                |   4 +-
>>> >  tools/libxl/libxl_internal.h          |   2 -
>>> >  tools/libxl/libxl_nic.c               | 191 
>>> > +++++++---------------------------
>>> >  tools/ocaml/libs/xl/xenlight_stubs.c  |   3 +-
>>> >  tools/xl/xl_nic.c                     |   3 +-
>>> >  8 files changed, 52 insertions(+), 173 deletions(-)
>>> >
>>> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
>>> > index d5a3ab7..abe129e 100644
>>> > --- a/tools/libxl/libxl.h
>>> > +++ b/tools/libxl/libxl.h
>>> > @@ -1850,9 +1850,14 @@ int libxl_device_nic_destroy(libxl_ctx *ctx, 
>>> > uint32_t domid,
>>> >                               const libxl_asyncop_how *ao_how)
>>> >                               LIBXL_EXTERNAL_CALLERS_ONLY;
>>> >
>>> > -libxl_device_nic *libxl_device_nic_list(libxl_ctx *ctx, uint32_t domid, 
>>> > int *num);
>>> > +libxl_device_nic *libxl_device_nic_list(libxl_ctx *ctx,
>>> > +                                        uint32_t domid, int *num)
>>> > +                                        LIBXL_EXTERNAL_CALLERS_ONLY;
>>> > +void libxl_device_nic_list_free(libxl_device_nic* list, int num)
>>> > +                                LIBXL_EXTERNAL_CALLERS_ONLY;
>>> >  int libxl_device_nic_getinfo(libxl_ctx *ctx, uint32_t domid,
>>> > -                              libxl_device_nic *nic, libxl_nicinfo 
>>> > *nicinfo);
>>> > +                             libxl_device_nic *nic, libxl_nicinfo 
>>> > *nicinfo)
>>> > +                             LIBXL_EXTERNAL_CALLERS_ONLY;
>>>
>>> The annotation has caused the following failures:
>>>
>>> libxl_colo_save.c: In function ‘libxl__colo_save_setup’:
>>> libxl_colo_save.c:126:14: error: call to ‘libxl_device_nic_list’ declared 
>>> with attribute warning: may not be called from within libxl [-Werror]
>>> nics = libxl_device_nic_list(CTX, cds->domid, &nb);
>>>          ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> libxl_colo_save.c:129:9: error: call to ‘libxl_device_nic_list_free’ 
>>> declared with attribute warning: may not be called from within libxl 
>>> [-Werror]
>>> libxl_device_nic_list_free(nics, nb);
>>>          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> However, I think that's a bug in colo.
>>
>> Thinking about this a bit more: the list_free functions (here and in
>> other patches) shouldn't be annotated as external callers only.
>>
>> And you can replace the nic_list function here to call the framework
>> directly.
>
> The idea was that libxl_device_xxx_list and free are for external callers only
> because they don't aware about device framework. Internal callers shall use
> device framework instead. I wonder why I don't have error here on my
> build environment. I will put device framework get/free list here.
>
> I will create new patch set.
>

Also in this particular place I've tried to fix memory leak by adding
libxl_device_nic_list_free.
But this is not right fix and out of this patch scope.
So I will revert this changes. Probably libxl__strdup should be here.
I will move the fix to the separate patch in this patch series.

-- 
Best Regards,
Oleksandr Grytsov.

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