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 :|
[Qemu-devel] [PATCH] netdev: fix socket backend implementation and docs
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 patch addresses all issues I've pointed earlier
(http://lists.nongnu.org/archive/html/qemu-devel/2018-05/msg06695.html), except
internal protocol, and plus additional ones.
'fd' transport and its special 'af_inet multicast' case deserve special
attention.
Firstly, it already wasn't working as expected at least in current qemu
version, so it isn't supposed to break any compabitibility if someone cares.
The only question is a way of fixing it.
It depends on concept behind 'fd' transport, which is unknown to me. Seems
like initially it was just optional parameter to any transport allowing to
replace newly created socket descriptor with user supplied one, but at some
time someone changed it to be separate transport and not accounted 'af_inet
multicast' case (code analysis shows that condition "is_connected && mcast !=
NULL" in 'net_socket_fd_init_dgram' function never can be true and
"s->dgram_dst" value is left unassigned).
I would prefer concept of separate transport when qemu doesn't depend on
underlying domain, type and protocol of socket (almost). To make it
universal/flexible, qemu shouldn't handle their specifics, such as 'af_inet
multicast' one. So I fixed things accordingly. For example, if user needs
multicasting, it should already know that it cannot share one socket between
its app and qemu instances, so user should use 'mcast' transport of qemu which
will create separate socket for qemu instance. And there are maybe other socket
types (possibly not existing at present moment yet) with their own specifics.
Since this concept restricts usage of sockets which cannot work in
connected state (such as af_inet multicast), I've prepared and ready to submit
another version of patch which solves this restriction by checking socket type
and handling it accordingly. Currently it supports only 'af_inet multicast'
case by preserving existed hack (slightly modified): it extracts multicast
address from socket and duplicates socket in proper unconnected state. It also
requires synchronization with user who will unconnect original socket back. All
of this is very hacky, but I'm open for discussion...
include/qemu-common.h | 3 +
include/qemu/sockets.h | 2 +-
net/net.c | 8 ++-
net/socket.c | 148 ++---
qapi/net.json | 12 ++--
qemu-options.hx| 85 +---
6 files changed, 147 insertions(+), 111 deletions(-)
diff --git a/include/qemu-common.h b/include/qemu-common.h
index ed60ba2..d4e4121 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -67,6 +67,8 @@ int qemu_openpty_raw(int *aslave, char *pty_name);
#define qemu_setsockopt(sockfd, level, optname, optval, optlen) \
setsockopt(sockfd, level, optname, (const void *)optval, optlen)
#define qemu_recv(sockfd, buf, len, flags) recv(sockfd, (void *)buf, len,
flags)
+#define qemu_send(sockfd, buf, len, flags) \
+send(sockfd, (const void *)buf, len, flags)
#define qemu_sendto(sockfd, buf, len, flags, destaddr, addrlen) \
sendto(sockfd, (const void *)buf, len, flags, destaddr, addrlen)
#else
@@ -75,6 +77,7 @@ int qemu_openpty_raw(int *aslave, char *pty_name);
#define qemu_setsockopt(sockfd, level, optname, optval, optlen) \
setsockopt(sockfd, level, optname, optval, optlen)
#define qemu_recv(sockfd, buf, len, flags) recv(sockfd, buf, len, flags)
+#define qemu_send(sockfd, buf, len, flags) send(sockfd, buf, len, flags)
#define qemu_sendto(sockfd, buf, len, flags, destaddr, addrlen) \
sendto(sockfd, buf, len, flags, destaddr, addrlen)
#endif
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,
