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

[Jason Wang <jasowang@redhat.com>]

在 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


>
> thanks
> -- PMM
>