| Message ID | 1347562697-15411-2-git-send-email-owasserm@redhat.com |
|---|---|
| State | New |
| Headers | show |
Orit Wasserman <owasserm@redhat.com> wrote: > From: "Michael S. Tsirkin" <mst@redhat.com> > > refactor address resolution code to fix nonblocking connect > remove getnameinfo call > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Amos Kong <akong@redhat.com> > Signed-off-by: Orit Wasserman <owasserm@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com> > -int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp) > +#ifdef _WIN32 > +#define QEMU_SOCKET_RC_INPROGRESS(rc) \ > + ((rc) == -EINPROGRESS || rc == -EWOULDBLOCK || rc == -WSAEALREADY) > +#else > +#define QEMU_SOCKET_RC_INPROGRESS(rc) \ > + ((rc) == -EINPROGRESS) > +#endif Not specific to this series, but this is used all around in qemu. Should we put it into osdep.h and refactor?
On 14/09/12 02:58, Orit Wasserman wrote: > From: "Michael S. Tsirkin" <mst@redhat.com> > > refactor address resolution code to fix nonblocking connect > remove getnameinfo call > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Amos Kong <akong@redhat.com> > Signed-off-by: Orit Wasserman <owasserm@redhat.com> Hi Orit, Happy new year! > --- > qemu-sockets.c | 144 +++++++++++++++++++++++++++++++------------------------ > 1 files changed, 81 insertions(+), 63 deletions(-) > > diff --git a/qemu-sockets.c b/qemu-sockets.c > index 361d890..939a453 100644 ... > + > +int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp) > +{ > + struct addrinfo *res, *e; > + int sock = -1; > + bool block = qemu_opt_get_bool(opts, "block", 0); > + > + res = inet_parse_connect_opts(opts, errp); > + if (!res) { > + return -1; > + } > + > + if (in_progress) { > + *in_progress = false; > } trivial comment: You moved this block to head of inet_connect_addr() in [patch 3/3], why not do this in this patch? I know if *in_progress becomes "true", sock is always >= 0, for loop will exits. But moving this block to inet_connect_addr() is clearer. > for (e = res; e != NULL; e = e->ai_next) { > - if (getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen, > - uaddr,INET6_ADDRSTRLEN,uport,32, > - NI_NUMERICHOST | NI_NUMERICSERV) != 0) { > - fprintf(stderr,"%s: getnameinfo: oops\n", __FUNCTION__); > - continue; > - } > - sock = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol); > - if (sock < 0) { > - fprintf(stderr,"%s: socket(%s): %s\n", __FUNCTION__, > - inet_strfamily(e->ai_family), strerror(errno)); > - continue; > - } > - setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on)); > - if (!block) { > - socket_set_nonblock(sock); > + sock = inet_connect_addr(e, block, in_progress); > + if (sock >= 0) { > + break; > } > - /* connect to peer */ > - do { > - rc = 0; > - if (connect(sock, e->ai_addr, e->ai_addrlen) < 0) { > - rc = -socket_error(); > - } > - } while (rc == -EINTR); > - > - #ifdef _WIN32 > - if (!block && (rc == -EINPROGRESS || rc == -EWOULDBLOCK > - || rc == -WSAEALREADY)) { > - #else > - if (!block && (rc == -EINPROGRESS)) { > - #endif > - if (in_progress) { > - *in_progress = true; > - } > - } else if (rc < 0) { > - if (NULL == e->ai_next) > - fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__, > - inet_strfamily(e->ai_family), > - e->ai_canonname, uaddr, uport, strerror(errno)); > - closesocket(sock); > - continue; > - } > - freeaddrinfo(res); > - return sock; > } > - error_set(errp, QERR_SOCKET_CONNECT_FAILED); > + if (sock < 0) { > + error_set(errp, QERR_SOCKET_CONNECT_FAILED); > + } > freeaddrinfo(res); > - return -1; > + return sock; > } > > int inet_dgram_opts(QemuOpts *opts) >
On 19/09/12 16:33, Amos Kong wrote: > On 14/09/12 02:58, Orit Wasserman wrote: >> From: "Michael S. Tsirkin" <mst@redhat.com> >> >> refactor address resolution code to fix nonblocking connect >> remove getnameinfo call >> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> Signed-off-by: Amos Kong <akong@redhat.com> >> Signed-off-by: Orit Wasserman <owasserm@redhat.com> > > Hi Orit, > > Happy new year! > >> --- >> qemu-sockets.c | 144 >> +++++++++++++++++++++++++++++++------------------------ >> 1 files changed, 81 insertions(+), 63 deletions(-) >> >> diff --git a/qemu-sockets.c b/qemu-sockets.c >> index 361d890..939a453 100644 >> --- a/qemu-sockets.c >> +++ b/qemu-sockets.c >> @@ -209,95 +209,113 @@ listen: >> return slisten; >> } >> >> -int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp) >> +#ifdef _WIN32 >> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \ >> + ((rc) == -EINPROGRESS || rc == -EWOULDBLOCK || rc == -WSAEALREADY) ^^ Brackets are only be used for first 'rc' >> +#else >> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \ >> + ((rc) == -EINPROGRESS) >> +#endif > >> + >> +int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp) >> +{ >> + struct addrinfo *res, *e; >> + int sock = -1; >> + bool block = qemu_opt_get_bool(opts, "block", 0); >> + >> + res = inet_parse_connect_opts(opts, errp); >> + if (!res) { >> + return -1; In this error path, in_progress is not assigned, but it's used in tcp_start_outgoing_migration() Let also set the default value of in_progress to 'false' in tcp_start_outgoing_migration() >> + } >> + > >> + if (in_progress) { >> + *in_progress = false; >> } > > trivial comment: > > You moved this block to head of inet_connect_addr() in [patch 3/3], > why not do this in this patch? > > I know if *in_progress becomes "true", sock is always >= 0, for loop > will exits. > But moving this block to inet_connect_addr() is clearer. > > >> for (e = res; e != NULL; e = e->ai_next) { >> - if (getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen, >> - uaddr,INET6_ADDRSTRLEN,uport,32, >> - NI_NUMERICHOST | NI_NUMERICSERV) != 0) { >> - fprintf(stderr,"%s: getnameinfo: oops\n", __FUNCTION__); >> - continue; >> - } >> - sock = qemu_socket(e->ai_family, e->ai_socktype, >> e->ai_protocol); >> - if (sock < 0) { >> - fprintf(stderr,"%s: socket(%s): %s\n", __FUNCTION__, >> - inet_strfamily(e->ai_family), strerror(errno)); >> - continue; >> - } >> - setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on)); >> - if (!block) { >> - socket_set_nonblock(sock); >> + sock = inet_connect_addr(e, block, in_progress); >> + if (sock >= 0) { >> + break; >> } ...
On 09/19/2012 11:33 AM, Amos Kong wrote: > On 14/09/12 02:58, Orit Wasserman wrote: >> From: "Michael S. Tsirkin" <mst@redhat.com> >> >> refactor address resolution code to fix nonblocking connect >> remove getnameinfo call >> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> Signed-off-by: Amos Kong <akong@redhat.com> >> Signed-off-by: Orit Wasserman <owasserm@redhat.com> > > Hi Orit, > > Happy new year! > >> --- >> qemu-sockets.c | 144 +++++++++++++++++++++++++++++++------------------------ >> 1 files changed, 81 insertions(+), 63 deletions(-) >> >> diff --git a/qemu-sockets.c b/qemu-sockets.c >> index 361d890..939a453 100644 > > ... > >> + >> +int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp) >> +{ >> + struct addrinfo *res, *e; >> + int sock = -1; >> + bool block = qemu_opt_get_bool(opts, "block", 0); >> + >> + res = inet_parse_connect_opts(opts, errp); >> + if (!res) { >> + return -1; >> + } >> + > >> + if (in_progress) { >> + *in_progress = false; >> } > > trivial comment: > > You moved this block to head of inet_connect_addr() in [patch 3/3], > why not do this in this patch? > > I know if *in_progress becomes "true", sock is always >= 0, for loop will exits. > But moving this block to inet_connect_addr() is clearer. > sure > >> for (e = res; e != NULL; e = e->ai_next) { >> - if (getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen, >> - uaddr,INET6_ADDRSTRLEN,uport,32, >> - NI_NUMERICHOST | NI_NUMERICSERV) != 0) { >> - fprintf(stderr,"%s: getnameinfo: oops\n", __FUNCTION__); >> - continue; >> - } >> - sock = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol); >> - if (sock < 0) { >> - fprintf(stderr,"%s: socket(%s): %s\n", __FUNCTION__, >> - inet_strfamily(e->ai_family), strerror(errno)); >> - continue; >> - } >> - setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on)); >> - if (!block) { >> - socket_set_nonblock(sock); >> + sock = inet_connect_addr(e, block, in_progress); >> + if (sock >= 0) { >> + break; >> } >> - /* connect to peer */ >> - do { >> - rc = 0; >> - if (connect(sock, e->ai_addr, e->ai_addrlen) < 0) { >> - rc = -socket_error(); >> - } >> - } while (rc == -EINTR); >> - >> - #ifdef _WIN32 >> - if (!block && (rc == -EINPROGRESS || rc == -EWOULDBLOCK >> - || rc == -WSAEALREADY)) { >> - #else >> - if (!block && (rc == -EINPROGRESS)) { >> - #endif >> - if (in_progress) { >> - *in_progress = true; >> - } >> - } else if (rc < 0) { >> - if (NULL == e->ai_next) >> - fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__, >> - inet_strfamily(e->ai_family), >> - e->ai_canonname, uaddr, uport, strerror(errno)); >> - closesocket(sock); >> - continue; >> - } >> - freeaddrinfo(res); >> - return sock; >> } >> - error_set(errp, QERR_SOCKET_CONNECT_FAILED); >> + if (sock < 0) { >> + error_set(errp, QERR_SOCKET_CONNECT_FAILED); >> + } >> freeaddrinfo(res); >> - return -1; >> + return sock; >> } >> >> int inet_dgram_opts(QemuOpts *opts) >> >
On 09/20/2012 05:33 AM, Amos Kong wrote: > On 19/09/12 16:33, Amos Kong wrote: >> On 14/09/12 02:58, Orit Wasserman wrote: >>> From: "Michael S. Tsirkin" <mst@redhat.com> >>> >>> refactor address resolution code to fix nonblocking connect >>> remove getnameinfo call >>> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>> Signed-off-by: Amos Kong <akong@redhat.com> >>> Signed-off-by: Orit Wasserman <owasserm@redhat.com> >> >> Hi Orit, >> >> Happy new year! >> >>> --- >>> qemu-sockets.c | 144 >>> +++++++++++++++++++++++++++++++------------------------ >>> 1 files changed, 81 insertions(+), 63 deletions(-) >>> >>> diff --git a/qemu-sockets.c b/qemu-sockets.c >>> index 361d890..939a453 100644 >>> --- a/qemu-sockets.c >>> +++ b/qemu-sockets.c >>> @@ -209,95 +209,113 @@ listen: >>> return slisten; >>> } >>> >>> -int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp) >>> +#ifdef _WIN32 >>> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \ >>> + ((rc) == -EINPROGRESS || rc == -EWOULDBLOCK || rc == -WSAEALREADY) > > ^^ Brackets are only be used for first 'rc' good catch ! > >>> +#else >>> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \ >>> + ((rc) == -EINPROGRESS) >>> +#endif > >> >>> + >>> +int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp) >>> +{ >>> + struct addrinfo *res, *e; >>> + int sock = -1; >>> + bool block = qemu_opt_get_bool(opts, "block", 0); >>> + >>> + res = inet_parse_connect_opts(opts, errp); >>> + if (!res) { >>> + return -1; > > In this error path, in_progress is not assigned, but it's used > in tcp_start_outgoing_migration() > > Let also set the default value of in_progress to 'false' in > tcp_start_outgoing_migration() > ok. >>> + } >>> + >> >>> + if (in_progress) { >>> + *in_progress = false; >>> } >> >> trivial comment: >> >> You moved this block to head of inet_connect_addr() in [patch 3/3], >> why not do this in this patch? >> >> I know if *in_progress becomes "true", sock is always >= 0, for loop >> will exits. >> But moving this block to inet_connect_addr() is clearer. >> >> >>> for (e = res; e != NULL; e = e->ai_next) { >>> - if (getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen, >>> - uaddr,INET6_ADDRSTRLEN,uport,32, >>> - NI_NUMERICHOST | NI_NUMERICSERV) != 0) { >>> - fprintf(stderr,"%s: getnameinfo: oops\n", __FUNCTION__); >>> - continue; >>> - } >>> - sock = qemu_socket(e->ai_family, e->ai_socktype, >>> e->ai_protocol); >>> - if (sock < 0) { >>> - fprintf(stderr,"%s: socket(%s): %s\n", __FUNCTION__, >>> - inet_strfamily(e->ai_family), strerror(errno)); >>> - continue; >>> - } >>> - setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on)); >>> - if (!block) { >>> - socket_set_nonblock(sock); >>> + sock = inet_connect_addr(e, block, in_progress); >>> + if (sock >= 0) { >>> + break; >>> } > > ... >
Amos Kong <akong@redhat.com> writes: > On 19/09/12 16:33, Amos Kong wrote: >> On 14/09/12 02:58, Orit Wasserman wrote: >>> From: "Michael S. Tsirkin" <mst@redhat.com> >>> >>> refactor address resolution code to fix nonblocking connect >>> remove getnameinfo call >>> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>> Signed-off-by: Amos Kong <akong@redhat.com> >>> Signed-off-by: Orit Wasserman <owasserm@redhat.com> >> >> Hi Orit, >> >> Happy new year! >> >>> --- >>> qemu-sockets.c | 144 >>> +++++++++++++++++++++++++++++++------------------------ >>> 1 files changed, 81 insertions(+), 63 deletions(-) >>> >>> diff --git a/qemu-sockets.c b/qemu-sockets.c >>> index 361d890..939a453 100644 >>> --- a/qemu-sockets.c >>> +++ b/qemu-sockets.c >>> @@ -209,95 +209,113 @@ listen: >>> return slisten; >>> } >>> >>> -int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp) >>> +#ifdef _WIN32 >>> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \ >>> + ((rc) == -EINPROGRESS || rc == -EWOULDBLOCK || rc == -WSAEALREADY) > > ^^ Brackets are only be used for first 'rc' > >>> +#else >>> +#define QEMU_SOCKET_RC_INPROGRESS(rc) \ >>> + ((rc) == -EINPROGRESS) >>> +#endif > >> >>> + >>> +int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp) >>> +{ >>> + struct addrinfo *res, *e; >>> + int sock = -1; >>> + bool block = qemu_opt_get_bool(opts, "block", 0); >>> + >>> + res = inet_parse_connect_opts(opts, errp); >>> + if (!res) { >>> + return -1; > > In this error path, in_progress is not assigned, but it's used > in tcp_start_outgoing_migration() Took me a minute to see what you mean. inet_connect_opts() is called by inet_connect(), which is called by tcp_start_outgoing_migration(), which uses in_progress. The other callers of inet_connect() and inet_connect_opts() aren't affected, because they pass null in_progress. > Let also set the default value of in_progress to 'false' in > tcp_start_outgoing_migration() No, let's make sure the functions that take an in_progress argument always set it. That's easier to use safely. >>> + } >>> + >> >>> + if (in_progress) { >>> + *in_progress = false; >>> } >> >> trivial comment: >> >> You moved this block to head of inet_connect_addr() in [patch 3/3], >> why not do this in this patch? >> >> I know if *in_progress becomes "true", sock is always >= 0, for loop >> will exits. >> But moving this block to inet_connect_addr() is clearer. Makes inet_connect_addr() always set in_progress, which I like. [...]
diff --git a/qemu-sockets.c b/qemu-sockets.c index 361d890..939a453 100644 --- a/qemu-sockets.c +++ b/qemu-sockets.c @@ -209,95 +209,113 @@ listen: return slisten; } -int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp) +#ifdef _WIN32 +#define QEMU_SOCKET_RC_INPROGRESS(rc) \ + ((rc) == -EINPROGRESS || rc == -EWOULDBLOCK || rc == -WSAEALREADY) +#else +#define QEMU_SOCKET_RC_INPROGRESS(rc) \ + ((rc) == -EINPROGRESS) +#endif + +static int inet_connect_addr(struct addrinfo *addr, bool block, + bool *in_progress) { - struct addrinfo ai,*res,*e; + int sock, rc; + + sock = qemu_socket(addr->ai_family, addr->ai_socktype, addr->ai_protocol); + if (sock < 0) { + fprintf(stderr, "%s: socket(%s): %s\n", __func__, + inet_strfamily(addr->ai_family), strerror(errno)); + return -1; + } + setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on)); + if (!block) { + socket_set_nonblock(sock); + } + /* connect to peer */ + do { + rc = 0; + if (connect(sock, addr->ai_addr, addr->ai_addrlen) < 0) { + rc = -socket_error(); + } + } while (rc == -EINTR); + + if (!block && QEMU_SOCKET_RC_INPROGRESS(rc)) { + if (in_progress) { + *in_progress = true; + } + } else if (rc < 0) { + closesocket(sock); + return -1; + } + return sock; +} + +static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp) +{ + struct addrinfo ai, *res; + int rc; const char *addr; const char *port; - char uaddr[INET6_ADDRSTRLEN+1]; - char uport[33]; - int sock,rc; - bool block; - memset(&ai,0, sizeof(ai)); + memset(&ai, 0, sizeof(ai)); ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG; ai.ai_family = PF_UNSPEC; ai.ai_socktype = SOCK_STREAM; - if (in_progress) { - *in_progress = false; - } - addr = qemu_opt_get(opts, "host"); port = qemu_opt_get(opts, "port"); - block = qemu_opt_get_bool(opts, "block", 0); if (addr == NULL || port == NULL) { - fprintf(stderr, "inet_connect: host and/or port not specified\n"); + fprintf(stderr, + "inet_parse_connect_opts: host and/or port not specified\n"); error_set(errp, QERR_SOCKET_CREATE_FAILED); - return -1; + return NULL; } - if (qemu_opt_get_bool(opts, "ipv4", 0)) + if (qemu_opt_get_bool(opts, "ipv4", 0)) { ai.ai_family = PF_INET; - if (qemu_opt_get_bool(opts, "ipv6", 0)) + } + if (qemu_opt_get_bool(opts, "ipv6", 0)) { ai.ai_family = PF_INET6; + } /* lookup */ - if (0 != (rc = getaddrinfo(addr, port, &ai, &res))) { - fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port, + rc = getaddrinfo(addr, port, &ai, &res); + if (rc != 0) { + fprintf(stderr, "getaddrinfo(%s,%s): %s\n", addr, port, gai_strerror(rc)); error_set(errp, QERR_SOCKET_CREATE_FAILED); - return -1; + return NULL; + } + return res; +} + +int inet_connect_opts(QemuOpts *opts, bool *in_progress, Error **errp) +{ + struct addrinfo *res, *e; + int sock = -1; + bool block = qemu_opt_get_bool(opts, "block", 0); + + res = inet_parse_connect_opts(opts, errp); + if (!res) { + return -1; + } + + if (in_progress) { + *in_progress = false; } for (e = res; e != NULL; e = e->ai_next) { - if (getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen, - uaddr,INET6_ADDRSTRLEN,uport,32, - NI_NUMERICHOST | NI_NUMERICSERV) != 0) { - fprintf(stderr,"%s: getnameinfo: oops\n", __FUNCTION__); - continue; - } - sock = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol); - if (sock < 0) { - fprintf(stderr,"%s: socket(%s): %s\n", __FUNCTION__, - inet_strfamily(e->ai_family), strerror(errno)); - continue; - } - setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on)); - if (!block) { - socket_set_nonblock(sock); + sock = inet_connect_addr(e, block, in_progress); + if (sock >= 0) { + break; } - /* connect to peer */ - do { - rc = 0; - if (connect(sock, e->ai_addr, e->ai_addrlen) < 0) { - rc = -socket_error(); - } - } while (rc == -EINTR); - - #ifdef _WIN32 - if (!block && (rc == -EINPROGRESS || rc == -EWOULDBLOCK - || rc == -WSAEALREADY)) { - #else - if (!block && (rc == -EINPROGRESS)) { - #endif - if (in_progress) { - *in_progress = true; - } - } else if (rc < 0) { - if (NULL == e->ai_next) - fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__, - inet_strfamily(e->ai_family), - e->ai_canonname, uaddr, uport, strerror(errno)); - closesocket(sock); - continue; - } - freeaddrinfo(res); - return sock; } - error_set(errp, QERR_SOCKET_CONNECT_FAILED); + if (sock < 0) { + error_set(errp, QERR_SOCKET_CONNECT_FAILED); + } freeaddrinfo(res); - return -1; + return sock; } int inet_dgram_opts(QemuOpts *opts)