| Message ID | 1349877786-23514-16-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 10 Oct 2012 16:02:56 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > perror and fprintf can be removed because all clients can now consume > Errors properly. However, we need to change the non-blocking connect > handlers to take an Error, in order to improve error handling for > migration with the TCP protocol. > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Are you guys related? :) Do you want me to clean this up before applying or doesn't matter? > --- > qemu-sockets.c | 15 ++++++--------- > 1 file modificato, 6 inserzioni(+), 9 rimozioni(-) > > diff --git a/qemu-sockets.c b/qemu-sockets.c > index 3c19463..d0e1a41 100644 > --- a/qemu-sockets.c > +++ b/qemu-sockets.c > @@ -216,7 +216,7 @@ typedef struct ConnectState { > } ConnectState; > > static int inet_connect_addr(struct addrinfo *addr, bool *in_progress, > - ConnectState *connect_state); > + ConnectState *connect_state, Error **errp); > > static void wait_for_connect(void *opaque) > { > @@ -246,7 +246,7 @@ static void wait_for_connect(void *opaque) > if (s->current_addr) { > while (s->current_addr->ai_next != NULL && s->fd < 0) { > s->current_addr = s->current_addr->ai_next; > - s->fd = inet_connect_addr(s->current_addr, &in_progress, s); > + s->fd = inet_connect_addr(s->current_addr, &in_progress, s, NULL); > /* connect in progress */ > if (in_progress) { > return; > @@ -263,7 +263,7 @@ static void wait_for_connect(void *opaque) > } > > static int inet_connect_addr(struct addrinfo *addr, bool *in_progress, > - ConnectState *connect_state) > + ConnectState *connect_state, Error **errp) > { > int sock, rc; > > @@ -271,8 +271,7 @@ static int inet_connect_addr(struct addrinfo *addr, bool *in_progress, > > 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)); > + error_set_errno(errp, errno, QERR_SOCKET_CREATE_FAILED); > return -1; > } > qemu_setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on)); > @@ -293,6 +292,7 @@ static int inet_connect_addr(struct addrinfo *addr, bool *in_progress, > connect_state); > *in_progress = true; > } else if (rc < 0) { > + error_set_errno(errp, errno, QERR_SOCKET_CONNECT_FAILED); > closesocket(sock); > return -1; > } > @@ -375,7 +375,7 @@ int inet_connect_opts(QemuOpts *opts, Error **errp, > if (connect_state != NULL) { > connect_state->current_addr = e; > } > - sock = inet_connect_addr(e, &in_progress, connect_state); > + sock = inet_connect_addr(e, &in_progress, connect_state, errp); > if (in_progress) { > return sock; > } else if (sock >= 0) { > @@ -386,9 +386,6 @@ int inet_connect_opts(QemuOpts *opts, Error **errp, > break; > } > } > - if (sock < 0) { > - error_set(errp, QERR_SOCKET_CONNECT_FAILED); > - } > g_free(connect_state); > freeaddrinfo(res); > return sock;
Paolo Bonzini <pbonzini@redhat.com> writes: > perror and fprintf can be removed because all clients can now consume > Errors properly. However, we need to change the non-blocking connect > handlers to take an Error, in order to improve error handling for > migration with the TCP protocol. The second sentence is about future work, isn't it? If yes, suggest "we'll need to change" to make it obvious. > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > qemu-sockets.c | 15 ++++++--------- > 1 file modificato, 6 inserzioni(+), 9 rimozioni(-) > > diff --git a/qemu-sockets.c b/qemu-sockets.c > index 3c19463..d0e1a41 100644 > --- a/qemu-sockets.c > +++ b/qemu-sockets.c > @@ -216,7 +216,7 @@ typedef struct ConnectState { > } ConnectState; > > static int inet_connect_addr(struct addrinfo *addr, bool *in_progress, > - ConnectState *connect_state); > + ConnectState *connect_state, Error **errp); > > static void wait_for_connect(void *opaque) > { > @@ -246,7 +246,7 @@ static void wait_for_connect(void *opaque) > if (s->current_addr) { > while (s->current_addr->ai_next != NULL && s->fd < 0) { > s->current_addr = s->current_addr->ai_next; > - s->fd = inet_connect_addr(s->current_addr, &in_progress, s); > + s->fd = inet_connect_addr(s->current_addr, &in_progress, s, NULL); Doesn't this drop error messages? If yes, but it's healed later in the series, the temporary breakage still needs to be spelled out in the commit message. > /* connect in progress */ > if (in_progress) { > return; > @@ -263,7 +263,7 @@ static void wait_for_connect(void *opaque) > } > > static int inet_connect_addr(struct addrinfo *addr, bool *in_progress, > - ConnectState *connect_state) > + ConnectState *connect_state, Error **errp) > { > int sock, rc; > > @@ -271,8 +271,7 @@ static int inet_connect_addr(struct addrinfo *addr, bool *in_progress, > > 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)); > + error_set_errno(errp, errno, QERR_SOCKET_CREATE_FAILED); > return -1; > } > qemu_setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on)); > @@ -293,6 +292,7 @@ static int inet_connect_addr(struct addrinfo *addr, bool *in_progress, > connect_state); > *in_progress = true; > } else if (rc < 0) { > + error_set_errno(errp, errno, QERR_SOCKET_CONNECT_FAILED); Looks like this reports a previously unreported error condition. If it does, it's a fix worth mentioning in commit message. > closesocket(sock); > return -1; > } > @@ -375,7 +375,7 @@ int inet_connect_opts(QemuOpts *opts, Error **errp, > if (connect_state != NULL) { > connect_state->current_addr = e; > } > - sock = inet_connect_addr(e, &in_progress, connect_state); > + sock = inet_connect_addr(e, &in_progress, connect_state, errp); > if (in_progress) { > return sock; > } else if (sock >= 0) { > @@ -386,9 +386,6 @@ int inet_connect_opts(QemuOpts *opts, Error **errp, > break; > } > } > - if (sock < 0) { > - error_set(errp, QERR_SOCKET_CONNECT_FAILED); > - } > g_free(connect_state); > freeaddrinfo(res); > return sock;
Il 17/10/2012 17:40, Markus Armbruster ha scritto: >> > if (s->current_addr) { >> > while (s->current_addr->ai_next != NULL && s->fd < 0) { >> > s->current_addr = s->current_addr->ai_next; >> > - s->fd = inet_connect_addr(s->current_addr, &in_progress, s); >> > + s->fd = inet_connect_addr(s->current_addr, &in_progress, s, NULL); > Doesn't this drop error messages? Depends on what you mean by drop. The error messages previously were sent to stdio, hidden in a log file or just invisible, depending on how QEMU is run. (This is just for outgoing migration currently, so you cannot rely on stdio as you can for command-line parsing). > If yes, but it's healed later in the series, the temporary breakage > still needs to be spelled out in the commit message. No, it's not healed, which is why it's mentioned in the commit message that future work is needed. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 17/10/2012 17:40, Markus Armbruster ha scritto: >>> > if (s->current_addr) { >>> > while (s->current_addr->ai_next != NULL && s->fd < 0) { >>> > s->current_addr = s->current_addr->ai_next; >>> > - s->fd = inet_connect_addr(s->current_addr, &in_progress, s); >>> > + s->fd = inet_connect_addr(s->current_addr, &in_progress, s, NULL); >> Doesn't this drop error messages? > > Depends on what you mean by drop. The error messages previously were > sent to stdio, hidden in a log file or just invisible, depending on how > QEMU is run. (This is just for outgoing migration currently, so you > cannot rely on stdio as you can for command-line parsing). I agree the existing error reporting is very bad, but even very bad reporting is (slightly) better than no error reporting. Moreover, there's a use case where errors are reported in the correct place now: -monitor stdio. I'm afraid your patch hurts it. >> If yes, but it's healed later in the series, the temporary breakage >> still needs to be spelled out in the commit message. > > No, it's not healed, which is why it's mentioned in the commit message > that future work is needed. I really hate "reward" submissions of tons of useful work with demands for more even work... but here goes anyway: what's your take on completing the job? Straightfoward, or another ton of work?
> >> If yes, but it's healed later in the series, the temporary > >> breakage still needs to be spelled out in the commit message. > > > > No, it's not healed, which is why it's mentioned in the commit > > message that future work is needed. > > I really hate "reward" submissions of tons of useful work with demands > for more even work... but here goes anyway: what's your take on > completing the job? Straightfoward, or another ton of work? Straightforward, also because we have exactly one user of asynchronous connection (migration). But I'll let Orit do it. :) Paolo
diff --git a/qemu-sockets.c b/qemu-sockets.c index 3c19463..d0e1a41 100644 --- a/qemu-sockets.c +++ b/qemu-sockets.c @@ -216,7 +216,7 @@ typedef struct ConnectState { } ConnectState; static int inet_connect_addr(struct addrinfo *addr, bool *in_progress, - ConnectState *connect_state); + ConnectState *connect_state, Error **errp); static void wait_for_connect(void *opaque) { @@ -246,7 +246,7 @@ static void wait_for_connect(void *opaque) if (s->current_addr) { while (s->current_addr->ai_next != NULL && s->fd < 0) { s->current_addr = s->current_addr->ai_next; - s->fd = inet_connect_addr(s->current_addr, &in_progress, s); + s->fd = inet_connect_addr(s->current_addr, &in_progress, s, NULL); /* connect in progress */ if (in_progress) { return; @@ -263,7 +263,7 @@ static void wait_for_connect(void *opaque) } static int inet_connect_addr(struct addrinfo *addr, bool *in_progress, - ConnectState *connect_state) + ConnectState *connect_state, Error **errp) { int sock, rc; @@ -271,8 +271,7 @@ static int inet_connect_addr(struct addrinfo *addr, bool *in_progress, 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)); + error_set_errno(errp, errno, QERR_SOCKET_CREATE_FAILED); return -1; } qemu_setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on)); @@ -293,6 +292,7 @@ static int inet_connect_addr(struct addrinfo *addr, bool *in_progress, connect_state); *in_progress = true; } else if (rc < 0) { + error_set_errno(errp, errno, QERR_SOCKET_CONNECT_FAILED); closesocket(sock); return -1; } @@ -375,7 +375,7 @@ int inet_connect_opts(QemuOpts *opts, Error **errp, if (connect_state != NULL) { connect_state->current_addr = e; } - sock = inet_connect_addr(e, &in_progress, connect_state); + sock = inet_connect_addr(e, &in_progress, connect_state, errp); if (in_progress) { return sock; } else if (sock >= 0) { @@ -386,9 +386,6 @@ int inet_connect_opts(QemuOpts *opts, Error **errp, break; } } - if (sock < 0) { - error_set(errp, QERR_SOCKET_CONNECT_FAILED); - } g_free(connect_state); freeaddrinfo(res); return sock;