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

[Artem Pisarenko]

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

Ok.

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

I understand that and I done it intentionally for this specific patch.
These changes are really very and very related to each other (not only
because they mostly live in one source file).
Yes, few specific changes are easily may be represented by separate
patches, but it also means that it doesn't bring much convinience for
review. Converting this to patch series just introduce more complexity and
effort (mostly for me, but for reviewers too enough). Not also it requires
95% of effort already spent, but for single reviewer understanding of
validity of each separate patch will be mostly invalidated by following
patch. Furthermore, if some patch will require rework, then it also most
probably will require rework whole chain of next dependent patches, thus
invalidating their 'reviewed' status if any. I beleive this patch is worth
of exception from principles you mentioned. Whole net/socket.c requires
revision - separating changes wil not simplify it. Just consider it as one
large fix of something hardly broken and given new fresh look and view.

If you still insist on patch series, I'll do it, but later, much later
(didn't planned such large efforts on this).


пн, 12 нояб. 2018 г. в 21:31, Daniel P. Berrangé <address@hidden>:

> 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 <address@hidden>
> > ---
> >
> > 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 :|
>