| Message ID | b107bc13f30e0b4a8ca8de1db178aef3fade9164.1498213152.git-series.knut.omang@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jun 23, 2017 at 12:31:06PM +0200, Knut Omang wrote: > First refactoring step to prepare for fixing the problem > exposed with the test-listen test in the previous commit > > Signed-off-by: Knut Omang <knut.omang@oracle.com> > --- > util/qemu-sockets.c | 24 +++++++++++++++++------- > 1 file changed, 17 insertions(+), 7 deletions(-) > > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c > index 852773d..699e36c 100644 > --- a/util/qemu-sockets.c > +++ b/util/qemu-sockets.c > @@ -149,6 +149,20 @@ int inet_ai_family_from_address(InetSocketAddress *addr, > return PF_UNSPEC; > } > > +static int create_fast_reuse_socket(struct addrinfo *e, Error **errp) > +{ > + int slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol); > + if (slisten < 0) { > + if (!e->ai_next) { > + error_setg_errno(errp, errno, "Failed to create socket"); > + } I think that having this method sometimes report an error message, and sometimes not report an error message, depending on state of a variable used by the caller is rather unpleasant. I'd much rather see this error message reporting remain in the caller. > + return -1; > + } > + > + socket_set_fast_reuse(slisten); > + return slisten; > +} > + > static int inet_listen_saddr(InetSocketAddress *saddr, > int port_offset, > bool update_addr, > @@ -210,21 +224,17 @@ static int inet_listen_saddr(InetSocketAddress *saddr, > return -1; > } > > - /* create socket + bind */ > + /* create socket + bind/listen */ > for (e = res; e != NULL; e = e->ai_next) { > getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen, > uaddr,INET6_ADDRSTRLEN,uport,32, > NI_NUMERICHOST | NI_NUMERICSERV); > - slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol); > + > + slisten = create_fast_reuse_socket(e, &err); > if (slisten < 0) { > - if (!e->ai_next) { > - error_setg_errno(errp, errno, "Failed to create socket"); > - } > continue; It isn't shown in this diff context, but at the end of the outer loop we have error_setg_errno(errp, errno, "Failed to find available port"); so IIUC, even this pre-existing code is wrong. If 'e->ai_next' is NULL, we report an error message here. Then, we continue to the next loop iteration, which causes use to terminate the loop entirely. At which point we'll report another error message over the top of the one we already have. So I think the error reporting does still need refactoring, but not in the way it is done here. > } > > - socket_set_fast_reuse(slisten); > - > port_min = inet_getport(e); > port_max = saddr->has_to ? saddr->to + port_offset : port_min; > for (p = port_min; p <= port_max; p++) { Regards, Daniel
On Mon, 2017-06-26 at 11:28 +0100, Daniel P. Berrange wrote: > On Fri, Jun 23, 2017 at 12:31:06PM +0200, Knut Omang wrote: > > First refactoring step to prepare for fixing the problem > > exposed with the test-listen test in the previous commit > > > > Signed-off-by: Knut Omang <knut.omang@oracle.com> > > --- > > util/qemu-sockets.c | 24 +++++++++++++++++------- > > 1 file changed, 17 insertions(+), 7 deletions(-) > > > > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c > > index 852773d..699e36c 100644 > > --- a/util/qemu-sockets.c > > +++ b/util/qemu-sockets.c > > @@ -149,6 +149,20 @@ int inet_ai_family_from_address(InetSocketAddress *addr, > > return PF_UNSPEC; > > } > > > > +static int create_fast_reuse_socket(struct addrinfo *e, Error **errp) > > +{ > > + int slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol); > > + if (slisten < 0) { > > + if (!e->ai_next) { > > + error_setg_errno(errp, errno, "Failed to create socket"); > > + } > > I think that having this method sometimes report an error message, and > sometimes not report an error message, depending on state of a variable > used by the caller is rather unpleasant. I'd much rather see this > error message reporting remain in the caller. In principle I agree with you, but I think we do want to keep the details of what the failure cause was by also propagating information about the system call that failed. I considered this an acceptable trade-off in the name of performance as well as readability at the next level. This is a fairly unlikely case that one really does not have to worry too much about at the next level. Setting an error that does not get used for that special, unlikely case is not that bad. Doing it for all failures would be a lot more unnecessary work. > > > + return -1; > > + } > > + > > + socket_set_fast_reuse(slisten); > > + return slisten; > > +} > > + > > static int inet_listen_saddr(InetSocketAddress *saddr, > > int port_offset, > > bool update_addr, > > @@ -210,21 +224,17 @@ static int inet_listen_saddr(InetSocketAddress *saddr, > > return -1; > > } > > > > - /* create socket + bind */ > > + /* create socket + bind/listen */ > > for (e = res; e != NULL; e = e->ai_next) { > > getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen, > > uaddr,INET6_ADDRSTRLEN,uport,32, > > NI_NUMERICHOST | NI_NUMERICSERV); > > - slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol); > > + > > + slisten = create_fast_reuse_socket(e, &err); > > if (slisten < 0) { > > - if (!e->ai_next) { > > - error_setg_errno(errp, errno, "Failed to create socket"); > > - } > > continue; > > It isn't shown in this diff context, but at the end of the outer > loop we have > > error_setg_errno(errp, errno, "Failed to find available port"); > > so IIUC, even this pre-existing code is wrong. If 'e->ai_next' is > NULL, we report an error message here. Then, we continue to the > next loop iteration, which causes use to terminate the loop > entirely. At which point we'll report another error message > over the top of the one we already have. > > So I think the error > reporting does still need refactoring, but not in the way it > is done here. I agree, a simple way to solve it would be to only set errp if no error has already been set. Thanks, Knut > > } > > > > - socket_set_fast_reuse(slisten); > > - > > port_min = inet_getport(e); > > port_max = saddr->has_to ? saddr->to + port_offset : port_min; > > for (p = port_min; p <= port_max; p++) { > > Regards, > Daniel
On Mon, Jun 26, 2017 at 01:56:28PM +0200, Knut Omang wrote: > On Mon, 2017-06-26 at 11:28 +0100, Daniel P. Berrange wrote: > > On Fri, Jun 23, 2017 at 12:31:06PM +0200, Knut Omang wrote: > > > First refactoring step to prepare for fixing the problem > > > exposed with the test-listen test in the previous commit > > > > > > Signed-off-by: Knut Omang <knut.omang@oracle.com> > > > --- > > > util/qemu-sockets.c | 24 +++++++++++++++++------- > > > 1 file changed, 17 insertions(+), 7 deletions(-) > > > > > > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c > > > index 852773d..699e36c 100644 > > > --- a/util/qemu-sockets.c > > > +++ b/util/qemu-sockets.c > > > @@ -149,6 +149,20 @@ int inet_ai_family_from_address(InetSocketAddress *addr, > > > return PF_UNSPEC; > > > } > > > > > > +static int create_fast_reuse_socket(struct addrinfo *e, Error **errp) > > > +{ > > > + int slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol); > > > + if (slisten < 0) { > > > + if (!e->ai_next) { > > > + error_setg_errno(errp, errno, "Failed to create socket"); > > > + } > > > > I think that having this method sometimes report an error message, and > > sometimes not report an error message, depending on state of a variable > > used by the caller is rather unpleasant. I'd much rather see this > > error message reporting remain in the caller. > > In principle I agree with you, but I think we do want to keep the details > of what the failure cause was by also propagating information about the system > call that failed. > > I considered this an acceptable trade-off in the name of performance as well as > readability at the next level. This is a fairly unlikely case that one really does not > have to worry too much about at the next level. Setting an error that does not get used > for that special, unlikely case is not that bad. Doing it for all failures would be > a lot more unnecessary work. > > > > > > + return -1; > > > + } > > > + > > > + socket_set_fast_reuse(slisten); > > > + return slisten; > > > +} > > > + > > > static int inet_listen_saddr(InetSocketAddress *saddr, > > > int port_offset, > > > bool update_addr, > > > @@ -210,21 +224,17 @@ static int inet_listen_saddr(InetSocketAddress *saddr, > > > return -1; > > > } > > > > > > - /* create socket + bind */ > > > + /* create socket + bind/listen */ > > > for (e = res; e != NULL; e = e->ai_next) { > > > getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen, > > > uaddr,INET6_ADDRSTRLEN,uport,32, > > > NI_NUMERICHOST | NI_NUMERICSERV); > > > - slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol); > > > + > > > + slisten = create_fast_reuse_socket(e, &err); > > > if (slisten < 0) { > > > - if (!e->ai_next) { > > > - error_setg_errno(errp, errno, "Failed to create socket"); > > > - } > > > continue; > > > > It isn't shown in this diff context, but at the end of the outer > > loop we have > > > > error_setg_errno(errp, errno, "Failed to find available port"); > > > > so IIUC, even this pre-existing code is wrong. If 'e->ai_next' is > > NULL, we report an error message here. Then, we continue to the > > next loop iteration, which causes use to terminate the loop > > entirely. At which point we'll report another error message > > over the top of the one we already have. > > > > So I think the error > > reporting does still need refactoring, but not in the way it > > is done here. > > I agree, a simple way to solve it would be to only set errp if no > error has already been set. I'd like to see each of the methods set the error unconditionally. In the inet_socket_listen() method, we can use an then intermediate Error variable on each iteration of the loop, and propagate to the global variable at the end or discard it. Regards, Daniel
On Mon, 2017-06-26 at 11:28 +0100, Daniel P. Berrange wrote: > On Fri, Jun 23, 2017 at 12:31:06PM +0200, Knut Omang wrote: > > > > First refactoring step to prepare for fixing the problem > > exposed with the test-listen test in the previous commit > > > > Signed-off-by: Knut Omang <knut.omang@oracle.com> > > --- > > util/qemu-sockets.c | 24 +++++++++++++++++------- > > 1 file changed, 17 insertions(+), 7 deletions(-) > > > > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c > > index 852773d..699e36c 100644 > > --- a/util/qemu-sockets.c > > +++ b/util/qemu-sockets.c > > @@ -149,6 +149,20 @@ int inet_ai_family_from_address(InetSocketAddress > > *addr, > > return PF_UNSPEC; > > } > > > > +static int create_fast_reuse_socket(struct addrinfo *e, Error **errp) > > +{ > > + int slisten = qemu_socket(e->ai_family, e->ai_socktype, e- > > >ai_protocol); > > + if (slisten < 0) { > > + if (!e->ai_next) { > > + error_setg_errno(errp, errno, "Failed to create socket"); > > + } > > I think that having this method sometimes report an error message, and > sometimes not report an error message, depending on state of a variable > used by the caller is rather unpleasant. I'd much rather see this > error message reporting remain in the caller. > > > > > + return -1; > > + } > > + > > + socket_set_fast_reuse(slisten); > > + return slisten; > > +} > > + > > static int inet_listen_saddr(InetSocketAddress *saddr, > > int port_offset, > > bool update_addr, > > @@ -210,21 +224,17 @@ static int inet_listen_saddr(InetSocketAddress *saddr, > > return -1; > > } > > > > - /* create socket + bind */ > > + /* create socket + bind/listen */ > > for (e = res; e != NULL; e = e->ai_next) { > > getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen, > > uaddr,INET6_ADDRSTRLEN,uport,32, > > NI_NUMERICHOST | NI_NUMERICSERV); > > - slisten = qemu_socket(e->ai_family, e->ai_socktype, e- > > >ai_protocol); > > + > > + slisten = create_fast_reuse_socket(e, &err); > > if (slisten < 0) { > > - if (!e->ai_next) { > > - error_setg_errno(errp, errno, "Failed to create socket"); > > - } > > continue; > > It isn't shown in this diff context, but at the end of the outer > loop we have > > error_setg_errno(errp, errno, "Failed to find available port"); > > so IIUC, even this pre-existing code is wrong. If 'e->ai_next' is > NULL, we report an error message here. Then, we continue to the > next loop iteration, which causes use to terminate the loop > entirely. At which point we'll report another error message > over the top of the one we already have. So I think the error > reporting does still need refactoring, but not in the way it > is done here. Yes, I did scratch my head about this but I tried to keep the original semantics to avoid mixing unrelated changes. With the split into separate refactoring commits we are beyond that anyway. I'll have a second look at it.. Thanks, Knut > > > > > } > > > > - socket_set_fast_reuse(slisten); > > - > > port_min = inet_getport(e); > > port_max = saddr->has_to ? saddr->to + port_offset : port_min; > > for (p = port_min; p <= port_max; p++) { > > Regards, > Daniel
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c index 852773d..699e36c 100644 --- a/util/qemu-sockets.c +++ b/util/qemu-sockets.c @@ -149,6 +149,20 @@ int inet_ai_family_from_address(InetSocketAddress *addr, return PF_UNSPEC; } +static int create_fast_reuse_socket(struct addrinfo *e, Error **errp) +{ + int slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol); + if (slisten < 0) { + if (!e->ai_next) { + error_setg_errno(errp, errno, "Failed to create socket"); + } + return -1; + } + + socket_set_fast_reuse(slisten); + return slisten; +} + static int inet_listen_saddr(InetSocketAddress *saddr, int port_offset, bool update_addr, @@ -210,21 +224,17 @@ static int inet_listen_saddr(InetSocketAddress *saddr, return -1; } - /* create socket + bind */ + /* create socket + bind/listen */ for (e = res; e != NULL; e = e->ai_next) { getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen, uaddr,INET6_ADDRSTRLEN,uport,32, NI_NUMERICHOST | NI_NUMERICSERV); - slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol); + + slisten = create_fast_reuse_socket(e, &err); if (slisten < 0) { - if (!e->ai_next) { - error_setg_errno(errp, errno, "Failed to create socket"); - } continue; } - socket_set_fast_reuse(slisten); - port_min = inet_getport(e); port_max = saddr->has_to ? saddr->to + port_offset : port_min; for (p = port_min; p <= port_max; p++) {
First refactoring step to prepare for fixing the problem exposed with the test-listen test in the previous commit Signed-off-by: Knut Omang <knut.omang@oracle.com> --- util/qemu-sockets.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)