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

[Jason Wang]

在 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