Re: [PULL V2 16/20] qapi: net: Add query-netdev command

[Jason Wang <jasowang@redhat.com>]

在 2021/3/17 上午11:34, Jason Wang 写道:
>
> 在 2021/3/17 上午5:37, Peter Maydell 写道:
>> On Mon, 15 Mar 2021 at 09:15, Jason Wang <jasowang@redhat.com> wrote:
>>> From: Alexey Kirillov <lekiravi@yandex-team.ru>
>>>
>>> The query-netdev command is used to get the configuration of the 
>>> current
>>> network device backends (netdevs).
>>> This is the QMP analog of the HMP command "info network" but only for
>>> netdevs (i.e. excluding NIC and hubports).
>>>
>>> The query-netdev command returns an array of objects of the NetdevInfo
>>> type, which are an extension of Netdev type. It means that response can
>>> be used for netdev-add after small modification. This can be useful for
>>> recreate the same netdev configuration.
>>>
>>> Information about the network device is filled in when it is created or
>>> modified and is available through the NetClientState->stored_config.
>>>
>>> Signed-off-by: Alexey Kirillov <lekiravi@yandex-team.ru>
>>> Acked-by: Markus Armbruster <armbru@redhat.com>
>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>> ---
>> Hi; Coverity complains about a memory leak in this code
>> (CID 1450842):
>>
>>> @@ -581,15 +693,25 @@ static int net_slirp_init(NetClientState 
>>> *peer, const char *model,
>>>       s->poll_notifier.notify = net_slirp_poll_notify;
>>>       main_loop_poll_add_notifier(&s->poll_notifier);
>>>
>>> +    stored_hostfwd = &stored->hostfwd;
>>> +    stored_guestfwd = &stored->guestfwd;
>>> +
>>>       for (config = slirp_configs; config; config = config->next) {
>>> +        String *element = g_new0(String, 1);
>> Here we allocate memory...
>>
>>> +
>>> +        element->str = g_strdup(config->str);
>>>           if (config->flags & SLIRP_CFG_HOSTFWD) {
>>>               if (slirp_hostfwd(s, config->str, errp) < 0) {
>>>                   goto error;
>> ...but if we take this error-exit path we have neither freed nor
>> kept a pointer to that memory.
>
>
> Yes.
>
>
>>
>>>               }
>>> +            stored->has_hostfwd = true;
>>> +            QAPI_LIST_APPEND(stored_hostfwd, element);
>>>           } else {
>>>               if (slirp_guestfwd(s, config->str, errp) < 0) {
>>>                   goto error;
>> Similarly here.
>>
>>>               }
>>> +            stored->has_guestfwd = true;
>>> +            QAPI_LIST_APPEND(stored_guestfwd, element);
>>>           }
>>>       }
>>>   #ifndef _WIN32
>> More generally, what state is the net backend init function
>> supposed to leave 'stored' in if it fails? Is it the backend's
>> responsibility to free everything that it might have allocated
>> and left a pointer to? eg if we did
>>     stored->hostname = g_strdup(vhostname);
>> do we need to go back and free(stored->hostname) ? Or is the caller
>> guaranteeing to clean up 'stored' somehow ? Or is the backend
>> supposed to not touch 'stored' until it's sure it's going to
>> succeed ? (presumably not, as the current code does not do this...)
>
>
> Clean and free in the function that do the allocation seems better 
> (self-conatined).
>
>
>>
>> This commit has no comments describing or documenting the
>> API requirements the new functionality imposes on a net backend:
>> could we have a followup patch which adds some documentation,
>> please, so that authors of future backends know what they have to
>> implement ?
>
>
> Alexey, plase send patches to fix the above issues and document the API.
>
> Thanks


Alexey, any update on this. If it takes time I tend to revert this and 
let's re-try for 6.1?

Thanks


>
>
>>
>> thanks
>> -- PMM
>>
>
>