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

[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


> thanks
> -- PMM