Re: [Qemu-devel] [PATCH] netdev: fix socket backend implementation and docs
On Tue, Nov 13, 2018 at 04:13:04PM +0600, Artem Pisarenko wrote:
> > 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.
I still rather disagree with this. Looking at the list of bullet
points of things you have changed and the code I still believe
this patch is better split up. It is too hard to identify which
parts of the code change correspond to which bullet point in the
commit message, making it hard to see that the patch actually
achieves what it claims to. The fact that the current code is a
big mess & needs broader revision in fact makes it more important
that the patches are done incrementally to maximise clarity.
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 :|
Re: [Qemu-devel] [PATCH] netdev: fix socket backend implementation and docs
> 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é :
> 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
> > ---
> >
> > 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 :|
>
Re: [Qemu-devel] [PATCH] netdev: fix socket backend implementation and docs
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
> ---
>
> 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 :|
