Re: [Qemu-devel] [PATCH] netdev: fix socket backend implementation and docs

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Nov 07, 2018 at 12:57:30PM +0600, Artem Pisarenko wrote:
> Changes:
> - user documentation and QAPI 'NetdevSocketOptions' comments updated
> to match current implementation ('udp' type description added, 'fd'
> option separated to exclusive type and described, 'localaddr'-related
> description for 'mcast' type fixed, hostname parts in "[host]:port"
> options updated to match optional/required syntax, various fixes and
> improvements in options breakdown and wording);
> - 'fd' type backend: requirement for socket handle being already
> binded and connected made explicit and documented;
> - 'fd' type backend: fix broken SOCK_DGRAM support;
> - 'fd' type backend: removed multicast support and cleaned up broken
> workaround for it (never called);
> - fix possible broken multicasting in win32 platform;
> - fix parsing of "[host]:port" options (added error handling for cases
> where "host" part is documented as required but isn't provided);
> - some error messages improved;
> - other small fixes and refactoring in code.
> 
> Signed-off-by: Artem Pisarenko <artem.k.pisarenko@gmail.com>
> ---
> 
> Notes:
>     (Since these changes are closely related, I've combined them in one patch.)

This is really not a desirable thing todo. While you are changing 
one area of code, but you've got a number of independent bugs
or improvements you are making. These should be done as a patch
series, one distinct fix/change per patch. Refactoring should
especially always be done separately from any functional changes
to reviewers can clearly see no accidental behaviour changes are
introduced by the refactoring.


> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
> index 8140fea..3fad004 100644
> --- a/include/qemu/sockets.h
> +++ b/include/qemu/sockets.h
> @@ -46,7 +46,7 @@ void socket_listen_cleanup(int fd, Error **errp);
>  int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp);
>  
>  /* Old, ipv4 only bits.  Don't use for new code. */
> -int parse_host_port(struct sockaddr_in *saddr, const char *str,
> +int parse_host_port(struct sockaddr_in *saddr, const char *str, bool h_addr_opt,
>                      Error **errp);

Incidentally this method should not even be part of this header
files.  qemu/sockets.h is for code that lives in util/qemu-sockets.c

The parse_host_port declaration and impl should better live in
net/util.{c,h}, so I'd recommend moving that as the first patch
in a series.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|