Re: [Qemu-devel] [PATCH 3/5] net/dump: Rework net-dump init functions

[Markus Armbruster]

Thomas Huth <address@hidden> writes:

> Moved the creation of the dump client from net_dump_init() into
> net_init_dump(), renamed net_dump_init() to net_dump_state_init()
> and make it available for other parts of the source code, too,
> so we can use it for the -netdev option in a later patch.

The usual commit message style uses present tense: Move the creation...

>
> Signed-off-by: Thomas Huth <address@hidden>
> ---
>  net/clients.h |  2 ++
>  net/dump.c    | 24 +++++++++++++-----------
>  2 files changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/net/clients.h b/net/clients.h
> index 5092f3d..fc76503 100644
> --- a/net/clients.h
> +++ b/net/clients.h
> @@ -29,6 +29,8 @@
>  
>  int net_init_dump(const NetClientOptions *opts, const char *name,
>                    NetClientState *peer, Error **errp);
> +int net_dump_state_init(NetClientState *nc, const char *filename, int len,
> +                        Error **errp);
>  ssize_t net_dump_receive(NetClientState *nc, const uint8_t *buf, size_t 
> size);
>  ssize_t net_dump_receive_iov(NetClientState *nc, const struct iovec *iov,
>                               int cnt);
> diff --git a/net/dump.c b/net/dump.c
> index b7a8b30..48a8087 100644
> --- a/net/dump.c
> +++ b/net/dump.c
> @@ -125,12 +125,10 @@ static NetClientInfo net_dump_info = {
>      .cleanup = net_dump_cleanup,
>  };
>  
> -static int net_dump_init(NetClientState *peer, const char *device,
> -                         const char *name, const char *filename, int len,
> -                         Error **errp)
> +int net_dump_state_init(NetClientState *nc, const char *filename, int len,
> +                        Error **errp)
>  {
>      struct pcap_file_hdr hdr;
> -    NetClientState *nc;
>      NetDumpState *s;
>      struct tm tm;
>      int fd;
> @@ -155,11 +153,6 @@ static int net_dump_init(NetClientState *peer, const 
> char *device,
>          return -1;
>      }
>  
> -    nc = qemu_new_net_client(&net_dump_info, peer, device, name);
> -
> -    snprintf(nc->info_str, sizeof(nc->info_str),
> -             "dump to %s (len=%d)", filename, len);
> -
>      s = &nc->nds;
>  
>      s->fd = fd;
> @@ -174,10 +167,11 @@ static int net_dump_init(NetClientState *peer, const 
> char *device,
>  int net_init_dump(const NetClientOptions *opts, const char *name,
>                    NetClientState *peer, Error **errp)
>  {
> -    int len;
> +    int len, rc;
>      const char *file;
>      char def_file[128];
>      const NetdevDumpOptions *dump;
> +    NetClientState *nc;
>  
>      assert(opts->kind == NET_CLIENT_OPTIONS_KIND_DUMP);
>      dump = opts->dump;
> @@ -207,5 +201,13 @@ int net_init_dump(const NetClientOptions *opts, const 
> char *name,
>          len = 65536;
>      }
>  
> -    return net_dump_init(peer, "dump", name, file, len, errp);
> +    nc = qemu_new_net_client(&net_dump_info, peer, "dump", name);
> +    snprintf(nc->info_str, sizeof(nc->info_str),
> +             "dump to %s (len=%d)", file, len);
> +
> +    rc = net_dump_state_init(nc, file, len, errp);
> +    if (rc) {
> +        qemu_del_net_client(nc);

My first impulse was "Is this a bug fix?"  But then I realized that old
code allocated nc after its last error return, whule the new code does
it before some.  Good.

> +    }
> +    return rc;
>  }