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

[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.

>              }
> +            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...)

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 ?

thanks
-- PMM