| Message ID | 1328623766-12287-10-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 02/07/2012 08:09 AM, Markus Armbruster wrote: > Convert to error_report(). This adds error locations to the messages, > which is particularly important when the location is buried in a > configuration file. Moreover, we'll need this when we create a > monitor command to add character devices, so its errors actually > appear in the monitor, not stderr. > > Also clean up the messages, and get rid of some that look like errors, > but aren't. > > Improves user-hostile messages like this one for "-chardev > socket,id=foo,host=blackfin,port=1,server" > > inet_listen_opts: bind(ipv4,192.168.2.9,1): Permission denied > chardev: opening backend "socket" failed > > to > > qemu-system-x86_64: -chardev socket,id=foo,host=blackfin,port=1,server: Can't bind port blackfin:1: Permission denied > chardev: opening backend "socket" failed > > and this one for "-chardev udp,id=foo,localport=1,port=1" > > inet_dgram_opts: bind(ipv4,0.0.0.0,1): OK > inet_dgram_opts failed > chardev: opening backend "udp" failed > > to > > qemu-system-x86_64: -chardev udp,id=foo,localport=1,port=1: Can't bind port :1: Permission denied > chardev: opening backend "udp" failed > > You got to love the "OK" part. > > The uninformative extra "opening backend failed" message will be > cleaned up shortly. > > Signed-off-by: Markus Armbruster<armbru@redhat.com> > --- > qemu-char.c | 1 - > qemu-sockets.c | 154 +++++++++++++++++++++++++------------------------------ > 2 files changed, 70 insertions(+), 85 deletions(-) > > diff --git a/qemu-char.c b/qemu-char.c > index bb9e3f5..d591f70 100644 > --- a/qemu-char.c > +++ b/qemu-char.c > @@ -2101,7 +2101,6 @@ static CharDriverState *qemu_chr_open_udp(QemuOpts *opts) > > fd = inet_dgram_opts(opts); > if (fd< 0) { > - fprintf(stderr, "inet_dgram_opts failed\n"); > goto return_err; > } Let's add an Error ** argument here. It's easy to bridge errors to error_report (qerror_report_err) so it's conducive to incremental refactoring. Plus it starts getting us away from terminal errors and into proper erroro propagation. > diff --git a/qemu-sockets.c b/qemu-sockets.c > index 6bcb8e3..67e0559 100644 > --- a/qemu-sockets.c > +++ b/qemu-sockets.c > @@ -20,7 +20,8 @@ > #include<unistd.h> > > #include "qemu_socket.h" > -#include "qemu-common.h" /* for qemu_isdigit */ > +#include "qemu-common.h" > +#include "qemu-error.h" > > #ifndef AI_ADDRCONFIG > # define AI_ADDRCONFIG 0 > @@ -107,7 +108,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset) > char port[33]; > char uaddr[INET6_ADDRSTRLEN+1]; > char uport[33]; > - int slisten, rc, to, port_min, port_max, p; > + int slisten, rc, to, port_min, port_max, p, sav_errno; > > memset(&ai,0, sizeof(ai)); > ai.ai_flags = AI_PASSIVE | AI_ADDRCONFIG; > @@ -116,7 +117,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset) > > if ((qemu_opt_get(opts, "host") == NULL) || > (qemu_opt_get(opts, "port") == NULL)) { > - fprintf(stderr, "%s: host and/or port not specified\n", __FUNCTION__); > + error_report("inet socket character device requires parameters host and port"); > return -1; > } > pstrcpy(port, sizeof(port), qemu_opt_get(opts, "port")); > @@ -133,8 +134,8 @@ int inet_listen_opts(QemuOpts *opts, int port_offset) > snprintf(port, sizeof(port), "%d", atoi(port) + port_offset); > rc = getaddrinfo(strlen(addr) ? addr : NULL, port,&ai,&res); > if (rc != 0) { > - fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port, > - gai_strerror(rc)); > + error_report("Can't resolve %s:%s: %s", > + addr, port, gai_strerror(rc)); > return -1; > } > > @@ -145,9 +146,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset) > NI_NUMERICHOST | NI_NUMERICSERV); > slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol); > if (slisten< 0) { > - fprintf(stderr,"%s: socket(%s): %s\n", __FUNCTION__, > - inet_strfamily(e->ai_family), strerror(errno)); > - continue; > + continue; /* try next address */ > } > > setsockopt(slisten,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on)); > @@ -166,25 +165,33 @@ int inet_listen_opts(QemuOpts *opts, int port_offset) > if (bind(slisten, e->ai_addr, e->ai_addrlen) == 0) { > goto listen; > } > - if (p == port_max) { > - fprintf(stderr,"%s: bind(%s,%s,%d): %s\n", __FUNCTION__, > - inet_strfamily(e->ai_family), uaddr, inet_getport(e), > - strerror(errno)); > - } > } > + > + sav_errno = errno; > closesocket(slisten); > + errno = sav_errno; > + /* try next address */ > + } > + > + /* no address worked, errno is from last failed socket() or bind() */ > + if (to) { > + error_report("Can't bind any port %s:%s..%d: %s", > + addr, port, to, strerror(errno)); > + } else { > + error_report("Can't bind port %s:%s: %s", > + addr, port, strerror(errno)); > } > - fprintf(stderr, "%s: FAILED\n", __FUNCTION__); > freeaddrinfo(res); > return -1; > > listen: > if (listen(slisten,1) != 0) { > - perror("listen"); > + error_report("Can't listen on %s:%d: %s", addr, p, strerror(errno)); > closesocket(slisten); > freeaddrinfo(res); > return -1; > } > + > snprintf(uport, sizeof(uport), "%d", inet_getport(e) - port_offset); > qemu_opt_set(opts, "host", uaddr); > qemu_opt_set(opts, "port", uport); > @@ -199,8 +206,6 @@ int inet_connect_opts(QemuOpts *opts) > struct addrinfo ai,*res,*e; > const char *addr; > const char *port; > - char uaddr[INET6_ADDRSTRLEN+1]; > - char uport[33]; > int sock,rc; > > memset(&ai,0, sizeof(ai)); > @@ -211,7 +216,7 @@ int inet_connect_opts(QemuOpts *opts) > addr = qemu_opt_get(opts, "host"); > port = qemu_opt_get(opts, "port"); > if (addr == NULL || port == NULL) { > - fprintf(stderr, "inet_connect: host and/or port not specified\n"); > + error_report("inet socket character device requires parameters host and port"); > return -1; > } > > @@ -222,38 +227,29 @@ int inet_connect_opts(QemuOpts *opts) > > /* lookup */ > if (0 != (rc = getaddrinfo(addr, port,&ai,&res))) { > - fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port, > - gai_strerror(rc)); > + error_report("Can't resolve %s:%s: %s", > + addr, port, gai_strerror(rc)); > return -1; > } > > 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; > + continue; /* try next address */ > } > setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on)); > > /* connect to peer */ > if (connect(sock,e->ai_addr,e->ai_addrlen)< 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; > + continue; /* try next address */ > } > freeaddrinfo(res); > return sock; > } > + > + /* no address worked, errno is from last failed socket() or connect() */ > + error_report("Can't connect to %s:%s: %s", addr, port, strerror(errno)); > freeaddrinfo(res); > return -1; > } > @@ -261,12 +257,9 @@ int inet_connect_opts(QemuOpts *opts) > int inet_dgram_opts(QemuOpts *opts) > { > struct addrinfo ai, *peer = NULL, *local = NULL; > - const char *addr; > - const char *port; > - char uaddr[INET6_ADDRSTRLEN+1]; > - char uport[33]; > + const char *addr, *port; > + const char *localaddr, *localport; > int sock = -1, rc; > - > /* lookup peer addr */ > memset(&ai,0, sizeof(ai)); > ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG; > @@ -279,7 +272,7 @@ int inet_dgram_opts(QemuOpts *opts) > addr = "localhost"; > } > if (port == NULL || strlen(port) == 0) { > - fprintf(stderr, "inet_dgram: port not specified\n"); > + error_report("udp character device requires parameter port"); > return -1; > } > > @@ -289,8 +282,8 @@ int inet_dgram_opts(QemuOpts *opts) > ai.ai_family = PF_INET6; > > if (0 != (rc = getaddrinfo(addr, port,&ai,&peer))) { > - fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port, > - gai_strerror(rc)); > + error_report("Can't resolve %s:%s: %s", > + addr, port, gai_strerror(rc)); > return -1; > } > > @@ -300,53 +293,40 @@ int inet_dgram_opts(QemuOpts *opts) > ai.ai_family = peer->ai_family; > ai.ai_socktype = SOCK_DGRAM; > > - addr = qemu_opt_get(opts, "localaddr"); > - port = qemu_opt_get(opts, "localport"); > - if (addr == NULL || strlen(addr) == 0) { > - addr = NULL; > + localaddr = qemu_opt_get(opts, "localaddr"); > + localport = qemu_opt_get(opts, "localport"); > + if (localaddr == NULL || strlen(localaddr) == 0) { > + localaddr = NULL; > } > - if (!port || strlen(port) == 0) > - port = "0"; > + if (!localport || strlen(localport) == 0) > + localport = "0"; > > - if (0 != (rc = getaddrinfo(addr, port,&ai,&local))) { > - fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port, > - gai_strerror(rc)); > + if (0 != (rc = getaddrinfo(localaddr, localport,&ai,&local))) { > + error_report("Can't resolve %s:%s: %s", > + localaddr ? localaddr : NULL, localport, > + gai_strerror(rc)); > return -1; > } > > /* create socket */ > sock = qemu_socket(peer->ai_family, peer->ai_socktype, peer->ai_protocol); > if (sock< 0) { > - fprintf(stderr,"%s: socket(%s): %s\n", __FUNCTION__, > - inet_strfamily(peer->ai_family), strerror(errno)); > - goto err; > + goto cant_bind; > } > setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on)); > > /* bind socket */ > - if (getnameinfo((struct sockaddr*)local->ai_addr,local->ai_addrlen, > - uaddr,INET6_ADDRSTRLEN,uport,32, > - NI_NUMERICHOST | NI_NUMERICSERV) != 0) { > - fprintf(stderr, "%s: getnameinfo: oops\n", __FUNCTION__); > - goto err; > - } > if (bind(sock, local->ai_addr, local->ai_addrlen)< 0) { > - fprintf(stderr,"%s: bind(%s,%s,%d): OK\n", __FUNCTION__, > - inet_strfamily(local->ai_family), uaddr, inet_getport(local)); > + cant_bind: > + error_report("Can't bind port %s:%s: %s", > + localaddr ? localaddr : "", localport, strerror(errno)); > goto err; > } > > /* connect to peer */ > - if (getnameinfo((struct sockaddr*)peer->ai_addr, peer->ai_addrlen, > - uaddr, INET6_ADDRSTRLEN, uport, 32, > - NI_NUMERICHOST | NI_NUMERICSERV) != 0) { > - fprintf(stderr, "%s: getnameinfo: oops\n", __FUNCTION__); > - goto err; > - } > if (connect(sock,peer->ai_addr,peer->ai_addrlen)< 0) { > - fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__, > - inet_strfamily(peer->ai_family), > - peer->ai_canonname, uaddr, uport, strerror(errno)); > + error_report("Can't connect to %s:%s: %s", > + addr, port, strerror(errno)); > goto err; > } > > @@ -471,8 +451,7 @@ int unix_listen_opts(QemuOpts *opts) > > sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0); > if (sock< 0) { > - perror("socket(unix)"); > - return -1; > + goto err; > } > > memset(&un, 0, sizeof(un)); > @@ -496,18 +475,20 @@ int unix_listen_opts(QemuOpts *opts) > > unlink(un.sun_path); > if (bind(sock, (struct sockaddr*)&un, sizeof(un))< 0) { > - fprintf(stderr, "bind(unix:%s): %s\n", un.sun_path, strerror(errno)); > goto err; > } > if (listen(sock, 1)< 0) { > - fprintf(stderr, "listen(unix:%s): %s\n", un.sun_path, strerror(errno)); > goto err; > } > > return sock; > > err: > - closesocket(sock); > + error_report("Can't create socket %s: %s", > + un.sun_path, strerror(errno)); > + if (sock>= 0) { > + closesocket(sock); > + } > return -1; > } > > @@ -518,26 +499,31 @@ int unix_connect_opts(QemuOpts *opts) > int sock; > > if (NULL == path) { > - fprintf(stderr, "unix connect: no path specified\n"); > + error_report("unix socket character device requires parameter path"); > return -1; > } > > sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0); > if (sock< 0) { > - perror("socket(unix)"); > - return -1; > + goto err; > } > > memset(&un, 0, sizeof(un)); > un.sun_family = AF_UNIX; > snprintf(un.sun_path, sizeof(un.sun_path), "%s", path); > if (connect(sock, (struct sockaddr*)&un, sizeof(un))< 0) { > - fprintf(stderr, "connect(unix:%s): %s\n", path, strerror(errno)); > - close(sock); > - return -1; > + goto err; > } > > return sock; > + > +err: > + error_report("Can't connect to socket %s: %s", > + un.sun_path, strerror(errno)); > + if (sock>= 0) { > + close(sock); > + } > + return -1; > } Basically, same thing here and the remaining functions. Let's not introduce additional uses of error_report(). That said, I imagine you don't want to introduce a bunch of error types for these different things and that's probably not productive anyway. So let's compromise and introduce a generic QERR_INTERNAL_ERROR that takes a single human readable string as an argument. We can have a wrapper for it that also records location information in the error object. Regards, Anthony Liguori
Anthony Liguori <aliguori@us.ibm.com> writes: > On 02/07/2012 08:09 AM, Markus Armbruster wrote: >> Convert to error_report(). This adds error locations to the messages, >> which is particularly important when the location is buried in a >> configuration file. Moreover, we'll need this when we create a >> monitor command to add character devices, so its errors actually >> appear in the monitor, not stderr. >> >> Also clean up the messages, and get rid of some that look like errors, >> but aren't. >> >> Improves user-hostile messages like this one for "-chardev >> socket,id=foo,host=blackfin,port=1,server" >> >> inet_listen_opts: bind(ipv4,192.168.2.9,1): Permission denied >> chardev: opening backend "socket" failed >> >> to >> >> qemu-system-x86_64: -chardev socket,id=foo,host=blackfin,port=1,server: Can't bind port blackfin:1: Permission denied >> chardev: opening backend "socket" failed >> >> and this one for "-chardev udp,id=foo,localport=1,port=1" >> >> inet_dgram_opts: bind(ipv4,0.0.0.0,1): OK >> inet_dgram_opts failed >> chardev: opening backend "udp" failed >> >> to >> >> qemu-system-x86_64: -chardev udp,id=foo,localport=1,port=1: Can't bind port :1: Permission denied >> chardev: opening backend "udp" failed >> >> You got to love the "OK" part. >> >> The uninformative extra "opening backend failed" message will be >> cleaned up shortly. >> >> Signed-off-by: Markus Armbruster<armbru@redhat.com> >> --- >> qemu-char.c | 1 - >> qemu-sockets.c | 154 +++++++++++++++++++++++++------------------------------ >> 2 files changed, 70 insertions(+), 85 deletions(-) >> >> diff --git a/qemu-char.c b/qemu-char.c >> index bb9e3f5..d591f70 100644 >> --- a/qemu-char.c >> +++ b/qemu-char.c >> @@ -2101,7 +2101,6 @@ static CharDriverState *qemu_chr_open_udp(QemuOpts *opts) >> >> fd = inet_dgram_opts(opts); >> if (fd< 0) { >> - fprintf(stderr, "inet_dgram_opts failed\n"); >> goto return_err; >> } > > Let's add an Error ** argument here. > > It's easy to bridge errors to error_report (qerror_report_err) so it's > conducive to incremental refactoring. Plus it starts getting us away > from terminal errors and into proper erroro propagation. [...] >> @@ -518,26 +499,31 @@ int unix_connect_opts(QemuOpts *opts) >> int sock; >> >> if (NULL == path) { >> - fprintf(stderr, "unix connect: no path specified\n"); >> + error_report("unix socket character device requires parameter path"); >> return -1; >> } >> >> sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0); >> if (sock< 0) { >> - perror("socket(unix)"); >> - return -1; >> + goto err; >> } >> >> memset(&un, 0, sizeof(un)); >> un.sun_family = AF_UNIX; >> snprintf(un.sun_path, sizeof(un.sun_path), "%s", path); >> if (connect(sock, (struct sockaddr*)&un, sizeof(un))< 0) { >> - fprintf(stderr, "connect(unix:%s): %s\n", path, strerror(errno)); >> - close(sock); >> - return -1; >> + goto err; >> } >> >> return sock; >> + >> +err: >> + error_report("Can't connect to socket %s: %s", >> + un.sun_path, strerror(errno)); >> + if (sock>= 0) { >> + close(sock); >> + } >> + return -1; >> } > > Basically, same thing here and the remaining functions. Let's not > introduce additional uses of error_report(). > > That said, I imagine you don't want to introduce a bunch of error > types for these different things and that's probably not productive > anyway. You're imagining correctly. I've learned the hard way that replacing nicely crafted error messages by error types frequently degrades the error messages, which first makes me sad, and then makes me surf the web rather than work. You'd need somebody with a higher tolerance for bad error messages, or a lower propensity to indulge in wasting time rather than deliver shoddy work ;) > So let's compromise and introduce a generic QERR_INTERNAL_ERROR that > takes a single human readable string as an argument. We can have a > wrapper for it that also records location information in the error > object. This series goes from stderr to error_report(). That's a relatively simple step, which makes it relatively easy to review. I'm afraid moving all the way to error.h in one step wouldn't be as easy. Kevin suggests to do it in a follow-up series, and I agree. Can you point to an existing conversion from error_report() to error.h, to give us an idea how it's supposed to be done?
Markus Armbruster <armbru@redhat.com> writes: > Anthony Liguori <aliguori@us.ibm.com> writes: [Anthony asking for error_set() instead of error_report()...] >> Basically, same thing here and the remaining functions. Let's not >> introduce additional uses of error_report(). >> >> That said, I imagine you don't want to introduce a bunch of error >> types for these different things and that's probably not productive >> anyway. [...] >> So let's compromise and introduce a generic QERR_INTERNAL_ERROR that >> takes a single human readable string as an argument. We can have a >> wrapper for it that also records location information in the error >> object. > > This series goes from stderr to error_report(). That's a relatively > simple step, which makes it relatively easy to review. I'm afraid > moving all the way to error.h in one step wouldn't be as easy. Kevin > suggests to do it in a follow-up series, and I agree. > > Can you point to an existing conversion from error_report() to error.h, > to give us an idea how it's supposed to be done? Ping?
On 02/14/2012 11:24 AM, Markus Armbruster wrote: > Markus Armbruster<armbru@redhat.com> writes: > >> Anthony Liguori<aliguori@us.ibm.com> writes: > [Anthony asking for error_set() instead of error_report()...] >>> Basically, same thing here and the remaining functions. Let's not >>> introduce additional uses of error_report(). >>> >>> That said, I imagine you don't want to introduce a bunch of error >>> types for these different things and that's probably not productive >>> anyway. > [...] >>> So let's compromise and introduce a generic QERR_INTERNAL_ERROR that >>> takes a single human readable string as an argument. We can have a >>> wrapper for it that also records location information in the error >>> object. >> >> This series goes from stderr to error_report(). That's a relatively >> simple step, which makes it relatively easy to review. I'm afraid >> moving all the way to error.h in one step wouldn't be as easy. Kevin >> suggests to do it in a follow-up series, and I agree. The trouble I have is not about doing things incrementally, but rather touching a lot of code incrementally. Most of the code you touch could be done incrementally with error_set(). For instance, you could touch inet_listen_opts() and just add an Error ** as the last argument. You can change all callers to simply do: Error *err = NULL; ... inet_listen_opts(..., &err); if (err) { error_report_err(err); return -1; } And it's not really all that different from the series as it stands today. I agree that aggressively refactoring error propagation is probably not necessary as a first step, but if we're going to touch a lot of code, we should do it in a way that we don't have to immediately touch it again next. >> >> Can you point to an existing conversion from error_report() to error.h, >> to give us an idea how it's supposed to be done? > > Ping? Sorry, I mentally responded bug neglected to actually respond. All of the QMP work that Luiz is doing effectively does this so there are ample examples right now. The change command is probably a good place to start. >
Anthony Liguori <anthony@codemonkey.ws> writes: > On 02/14/2012 11:24 AM, Markus Armbruster wrote: >> Markus Armbruster<armbru@redhat.com> writes: >> >>> Anthony Liguori<aliguori@us.ibm.com> writes: >> [Anthony asking for error_set() instead of error_report()...] >>>> Basically, same thing here and the remaining functions. Let's not >>>> introduce additional uses of error_report(). >>>> >>>> That said, I imagine you don't want to introduce a bunch of error >>>> types for these different things and that's probably not productive >>>> anyway. >> [...] >>>> So let's compromise and introduce a generic QERR_INTERNAL_ERROR that >>>> takes a single human readable string as an argument. We can have a >>>> wrapper for it that also records location information in the error >>>> object. >>> >>> This series goes from stderr to error_report(). That's a relatively >>> simple step, which makes it relatively easy to review. I'm afraid >>> moving all the way to error.h in one step wouldn't be as easy. Kevin >>> suggests to do it in a follow-up series, and I agree. > > The trouble I have is not about doing things incrementally, but rather > touching a lot of code incrementally. Most of the code you touch > could be done incrementally with error_set(). > > For instance, you could touch inet_listen_opts() and just add an Error > ** as the last argument. You can change all callers to simply do: > > Error *err = NULL; > > ... > inet_listen_opts(..., &err); > if (err) { > error_report_err(err); > return -1; > } > > And it's not really all that different from the series as it stands > today. I agree that aggressively refactoring error propagation is > probably not necessary as a first step, but if we're going to touch a > lot of code, we should do it in a way that we don't have to > immediately touch it again next. Well, the series adds 47 calls of error_report() to five files out of 1850. >>> Can you point to an existing conversion from error_report() to error.h, >>> to give us an idea how it's supposed to be done? >> >> Ping? > > Sorry, I mentally responded bug neglected to actually respond. > > All of the QMP work that Luiz is doing effectively does this so there > are ample examples right now. The change command is probably a good > place to start. Thanks. Unfortunately, I'm out of time on this one, so if you're unwilling to accept this admittedly incremental improvement without substantial rework, I'll have to let it rot in peace. We might want a QMP commands to add character devices some day. Perhaps the person doing it will still be able to find these patches, and get some use out of them. Patches 01-08,14 don't add error_report() calls. What about committing them? The commit messages would need to be reworded not to promise goodies from the other patches, though.
On 02/15/2012 07:33 AM, Markus Armbruster wrote: > Anthony Liguori<anthony@codemonkey.ws> writes: > >> On 02/14/2012 11:24 AM, Markus Armbruster wrote: >>> Markus Armbruster<armbru@redhat.com> writes: >>> >>>> Anthony Liguori<aliguori@us.ibm.com> writes: >>> [Anthony asking for error_set() instead of error_report()...] >>>>> Basically, same thing here and the remaining functions. Let's not >>>>> introduce additional uses of error_report(). >>>>> >>>>> That said, I imagine you don't want to introduce a bunch of error >>>>> types for these different things and that's probably not productive >>>>> anyway. >>> [...] >>>>> So let's compromise and introduce a generic QERR_INTERNAL_ERROR that >>>>> takes a single human readable string as an argument. We can have a >>>>> wrapper for it that also records location information in the error >>>>> object. >>>> >>>> This series goes from stderr to error_report(). That's a relatively >>>> simple step, which makes it relatively easy to review. I'm afraid >>>> moving all the way to error.h in one step wouldn't be as easy. Kevin >>>> suggests to do it in a follow-up series, and I agree. >> >> The trouble I have is not about doing things incrementally, but rather >> touching a lot of code incrementally. Most of the code you touch >> could be done incrementally with error_set(). >> >> For instance, you could touch inet_listen_opts() and just add an Error >> ** as the last argument. You can change all callers to simply do: >> >> Error *err = NULL; >> >> ... >> inet_listen_opts(...,&err); >> if (err) { >> error_report_err(err); >> return -1; >> } >> >> And it's not really all that different from the series as it stands >> today. I agree that aggressively refactoring error propagation is >> probably not necessary as a first step, but if we're going to touch a >> lot of code, we should do it in a way that we don't have to >> immediately touch it again next. > > Well, the series adds 47 calls of error_report() to five files out of > 1850. > >>>> Can you point to an existing conversion from error_report() to error.h, >>>> to give us an idea how it's supposed to be done? >>> >>> Ping? >> >> Sorry, I mentally responded bug neglected to actually respond. >> >> All of the QMP work that Luiz is doing effectively does this so there >> are ample examples right now. The change command is probably a good >> place to start. > > Thanks. > > Unfortunately, I'm out of time on this one, so if you're unwilling to > accept this admittedly incremental improvement without substantial > rework, I'll have to let it rot in peace. > > We might want a QMP commands to add character devices some day. Perhaps > the person doing it will still be able to find these patches, and get > some use out of them. > > Patches 01-08,14 don't add error_report() calls. What about committing > them? The commit messages would need to be reworded not to promise > goodies from the other patches, though. I'm sorry to hear that you can't continue working on this. I'll focus on applying the patches you mentioned. While I admit that it seems counter intuitive to not want to improve error messages (and I fully admit, that this is an improvement), I'm more concerned that this digs us deeper into the qerror_report/error_report hole that we're trying to dig our way out of. If you want to add chardevs dynamically, then I assume your next patch would be switching error_report to qerror_report such that the errors appeared in the monitor. But this is wrong. New QMP functions are not allowed to use qerror_report anymore. So all of this code would need to get changed again anyway. We can't have async commands until qerror_report is removed from all existing monitor commands. We can't plumb guest agent commands without async commands not to mention that many commands are more naturally expressed as async commands anyway. Regards, Anthony Liguori >
Anthony Liguori <anthony@codemonkey.ws> writes: > On 02/15/2012 07:33 AM, Markus Armbruster wrote: >> Anthony Liguori<anthony@codemonkey.ws> writes: >> >>> On 02/14/2012 11:24 AM, Markus Armbruster wrote: >>>> Markus Armbruster<armbru@redhat.com> writes: >>>> >>>>> Anthony Liguori<aliguori@us.ibm.com> writes: >>>> [Anthony asking for error_set() instead of error_report()...] >>>>>> Basically, same thing here and the remaining functions. Let's not >>>>>> introduce additional uses of error_report(). >>>>>> >>>>>> That said, I imagine you don't want to introduce a bunch of error >>>>>> types for these different things and that's probably not productive >>>>>> anyway. >>>> [...] >>>>>> So let's compromise and introduce a generic QERR_INTERNAL_ERROR that >>>>>> takes a single human readable string as an argument. We can have a >>>>>> wrapper for it that also records location information in the error >>>>>> object. >>>>> >>>>> This series goes from stderr to error_report(). That's a relatively >>>>> simple step, which makes it relatively easy to review. I'm afraid >>>>> moving all the way to error.h in one step wouldn't be as easy. Kevin >>>>> suggests to do it in a follow-up series, and I agree. >>> >>> The trouble I have is not about doing things incrementally, but rather >>> touching a lot of code incrementally. Most of the code you touch >>> could be done incrementally with error_set(). >>> >>> For instance, you could touch inet_listen_opts() and just add an Error >>> ** as the last argument. You can change all callers to simply do: >>> >>> Error *err = NULL; >>> >>> ... >>> inet_listen_opts(...,&err); >>> if (err) { >>> error_report_err(err); >>> return -1; >>> } >>> >>> And it's not really all that different from the series as it stands >>> today. I agree that aggressively refactoring error propagation is >>> probably not necessary as a first step, but if we're going to touch a >>> lot of code, we should do it in a way that we don't have to >>> immediately touch it again next. >> >> Well, the series adds 47 calls of error_report() to five files out of >> 1850. >> >>>>> Can you point to an existing conversion from error_report() to error.h, >>>>> to give us an idea how it's supposed to be done? >>>> >>>> Ping? >>> >>> Sorry, I mentally responded bug neglected to actually respond. >>> >>> All of the QMP work that Luiz is doing effectively does this so there >>> are ample examples right now. The change command is probably a good >>> place to start. >> >> Thanks. >> >> Unfortunately, I'm out of time on this one, so if you're unwilling to >> accept this admittedly incremental improvement without substantial >> rework, I'll have to let it rot in peace. >> >> We might want a QMP commands to add character devices some day. Perhaps >> the person doing it will still be able to find these patches, and get >> some use out of them. >> >> Patches 01-08,14 don't add error_report() calls. What about committing >> them? The commit messages would need to be reworded not to promise >> goodies from the other patches, though. > > I'm sorry to hear that you can't continue working on this. Can't be helped. > I'll focus on applying the patches you mentioned. Suggest to reword the commit messages not to promise the parts you don't apply. > While I admit that it seems counter intuitive to not want to improve > error messages (and I fully admit, that this is an improvement), I'm > more concerned that this digs us deeper into the > qerror_report/error_report hole that we're trying to dig our way out > of. > > If you want to add chardevs dynamically, then I assume your next patch Not a priority at this time, I'm afraid. If it becomes one, I might be able to work on it. > would be switching error_report to qerror_report such that the errors > appeared in the monitor. But this is wrong. New QMP functions are > not allowed to use qerror_report anymore. So all of this code would > need to get changed again anyway. No, the next step for errors would be error_report() -> error_set(), precisely because qerror_report() can't be used anymore. Yes, that means the five files that report chardev open errors will need to be touched again. But that'll be a bog-standard error_report() -> error_set() conversion. Easier to code, test and review than combined "track down all the error paths that fail to report errors, or report non-sensical errors" + "convert from fprintf() to error_set() in one go". In my opinion, the "have to touch five files again" developer burden compares quite favorably with "have to check all the error paths again" developer burden. And in any case it's dwarved by the "have to use a debugger to find out what's wrong" user burden. [...]
diff --git a/qemu-char.c b/qemu-char.c index bb9e3f5..d591f70 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -2101,7 +2101,6 @@ static CharDriverState *qemu_chr_open_udp(QemuOpts *opts) fd = inet_dgram_opts(opts); if (fd < 0) { - fprintf(stderr, "inet_dgram_opts failed\n"); goto return_err; } diff --git a/qemu-sockets.c b/qemu-sockets.c index 6bcb8e3..67e0559 100644 --- a/qemu-sockets.c +++ b/qemu-sockets.c @@ -20,7 +20,8 @@ #include <unistd.h> #include "qemu_socket.h" -#include "qemu-common.h" /* for qemu_isdigit */ +#include "qemu-common.h" +#include "qemu-error.h" #ifndef AI_ADDRCONFIG # define AI_ADDRCONFIG 0 @@ -107,7 +108,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset) char port[33]; char uaddr[INET6_ADDRSTRLEN+1]; char uport[33]; - int slisten, rc, to, port_min, port_max, p; + int slisten, rc, to, port_min, port_max, p, sav_errno; memset(&ai,0, sizeof(ai)); ai.ai_flags = AI_PASSIVE | AI_ADDRCONFIG; @@ -116,7 +117,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset) if ((qemu_opt_get(opts, "host") == NULL) || (qemu_opt_get(opts, "port") == NULL)) { - fprintf(stderr, "%s: host and/or port not specified\n", __FUNCTION__); + error_report("inet socket character device requires parameters host and port"); return -1; } pstrcpy(port, sizeof(port), qemu_opt_get(opts, "port")); @@ -133,8 +134,8 @@ int inet_listen_opts(QemuOpts *opts, int port_offset) snprintf(port, sizeof(port), "%d", atoi(port) + port_offset); rc = getaddrinfo(strlen(addr) ? addr : NULL, port, &ai, &res); if (rc != 0) { - fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port, - gai_strerror(rc)); + error_report("Can't resolve %s:%s: %s", + addr, port, gai_strerror(rc)); return -1; } @@ -145,9 +146,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset) NI_NUMERICHOST | NI_NUMERICSERV); slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol); if (slisten < 0) { - fprintf(stderr,"%s: socket(%s): %s\n", __FUNCTION__, - inet_strfamily(e->ai_family), strerror(errno)); - continue; + continue; /* try next address */ } setsockopt(slisten,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on)); @@ -166,25 +165,33 @@ int inet_listen_opts(QemuOpts *opts, int port_offset) if (bind(slisten, e->ai_addr, e->ai_addrlen) == 0) { goto listen; } - if (p == port_max) { - fprintf(stderr,"%s: bind(%s,%s,%d): %s\n", __FUNCTION__, - inet_strfamily(e->ai_family), uaddr, inet_getport(e), - strerror(errno)); - } } + + sav_errno = errno; closesocket(slisten); + errno = sav_errno; + /* try next address */ + } + + /* no address worked, errno is from last failed socket() or bind() */ + if (to) { + error_report("Can't bind any port %s:%s..%d: %s", + addr, port, to, strerror(errno)); + } else { + error_report("Can't bind port %s:%s: %s", + addr, port, strerror(errno)); } - fprintf(stderr, "%s: FAILED\n", __FUNCTION__); freeaddrinfo(res); return -1; listen: if (listen(slisten,1) != 0) { - perror("listen"); + error_report("Can't listen on %s:%d: %s", addr, p, strerror(errno)); closesocket(slisten); freeaddrinfo(res); return -1; } + snprintf(uport, sizeof(uport), "%d", inet_getport(e) - port_offset); qemu_opt_set(opts, "host", uaddr); qemu_opt_set(opts, "port", uport); @@ -199,8 +206,6 @@ int inet_connect_opts(QemuOpts *opts) struct addrinfo ai,*res,*e; const char *addr; const char *port; - char uaddr[INET6_ADDRSTRLEN+1]; - char uport[33]; int sock,rc; memset(&ai,0, sizeof(ai)); @@ -211,7 +216,7 @@ int inet_connect_opts(QemuOpts *opts) addr = qemu_opt_get(opts, "host"); port = qemu_opt_get(opts, "port"); if (addr == NULL || port == NULL) { - fprintf(stderr, "inet_connect: host and/or port not specified\n"); + error_report("inet socket character device requires parameters host and port"); return -1; } @@ -222,38 +227,29 @@ int inet_connect_opts(QemuOpts *opts) /* lookup */ if (0 != (rc = getaddrinfo(addr, port, &ai, &res))) { - fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port, - gai_strerror(rc)); + error_report("Can't resolve %s:%s: %s", + addr, port, gai_strerror(rc)); return -1; } 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; + continue; /* try next address */ } setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on)); /* connect to peer */ if (connect(sock,e->ai_addr,e->ai_addrlen) < 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; + continue; /* try next address */ } freeaddrinfo(res); return sock; } + + /* no address worked, errno is from last failed socket() or connect() */ + error_report("Can't connect to %s:%s: %s", addr, port, strerror(errno)); freeaddrinfo(res); return -1; } @@ -261,12 +257,9 @@ int inet_connect_opts(QemuOpts *opts) int inet_dgram_opts(QemuOpts *opts) { struct addrinfo ai, *peer = NULL, *local = NULL; - const char *addr; - const char *port; - char uaddr[INET6_ADDRSTRLEN+1]; - char uport[33]; + const char *addr, *port; + const char *localaddr, *localport; int sock = -1, rc; - /* lookup peer addr */ memset(&ai,0, sizeof(ai)); ai.ai_flags = AI_CANONNAME | AI_ADDRCONFIG; @@ -279,7 +272,7 @@ int inet_dgram_opts(QemuOpts *opts) addr = "localhost"; } if (port == NULL || strlen(port) == 0) { - fprintf(stderr, "inet_dgram: port not specified\n"); + error_report("udp character device requires parameter port"); return -1; } @@ -289,8 +282,8 @@ int inet_dgram_opts(QemuOpts *opts) ai.ai_family = PF_INET6; if (0 != (rc = getaddrinfo(addr, port, &ai, &peer))) { - fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port, - gai_strerror(rc)); + error_report("Can't resolve %s:%s: %s", + addr, port, gai_strerror(rc)); return -1; } @@ -300,53 +293,40 @@ int inet_dgram_opts(QemuOpts *opts) ai.ai_family = peer->ai_family; ai.ai_socktype = SOCK_DGRAM; - addr = qemu_opt_get(opts, "localaddr"); - port = qemu_opt_get(opts, "localport"); - if (addr == NULL || strlen(addr) == 0) { - addr = NULL; + localaddr = qemu_opt_get(opts, "localaddr"); + localport = qemu_opt_get(opts, "localport"); + if (localaddr == NULL || strlen(localaddr) == 0) { + localaddr = NULL; } - if (!port || strlen(port) == 0) - port = "0"; + if (!localport || strlen(localport) == 0) + localport = "0"; - if (0 != (rc = getaddrinfo(addr, port, &ai, &local))) { - fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port, - gai_strerror(rc)); + if (0 != (rc = getaddrinfo(localaddr, localport, &ai, &local))) { + error_report("Can't resolve %s:%s: %s", + localaddr ? localaddr : NULL, localport, + gai_strerror(rc)); return -1; } /* create socket */ sock = qemu_socket(peer->ai_family, peer->ai_socktype, peer->ai_protocol); if (sock < 0) { - fprintf(stderr,"%s: socket(%s): %s\n", __FUNCTION__, - inet_strfamily(peer->ai_family), strerror(errno)); - goto err; + goto cant_bind; } setsockopt(sock,SOL_SOCKET,SO_REUSEADDR,(void*)&on,sizeof(on)); /* bind socket */ - if (getnameinfo((struct sockaddr*)local->ai_addr,local->ai_addrlen, - uaddr,INET6_ADDRSTRLEN,uport,32, - NI_NUMERICHOST | NI_NUMERICSERV) != 0) { - fprintf(stderr, "%s: getnameinfo: oops\n", __FUNCTION__); - goto err; - } if (bind(sock, local->ai_addr, local->ai_addrlen) < 0) { - fprintf(stderr,"%s: bind(%s,%s,%d): OK\n", __FUNCTION__, - inet_strfamily(local->ai_family), uaddr, inet_getport(local)); + cant_bind: + error_report("Can't bind port %s:%s: %s", + localaddr ? localaddr : "", localport, strerror(errno)); goto err; } /* connect to peer */ - if (getnameinfo((struct sockaddr*)peer->ai_addr, peer->ai_addrlen, - uaddr, INET6_ADDRSTRLEN, uport, 32, - NI_NUMERICHOST | NI_NUMERICSERV) != 0) { - fprintf(stderr, "%s: getnameinfo: oops\n", __FUNCTION__); - goto err; - } if (connect(sock,peer->ai_addr,peer->ai_addrlen) < 0) { - fprintf(stderr, "%s: connect(%s,%s,%s,%s): %s\n", __FUNCTION__, - inet_strfamily(peer->ai_family), - peer->ai_canonname, uaddr, uport, strerror(errno)); + error_report("Can't connect to %s:%s: %s", + addr, port, strerror(errno)); goto err; } @@ -471,8 +451,7 @@ int unix_listen_opts(QemuOpts *opts) sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0); if (sock < 0) { - perror("socket(unix)"); - return -1; + goto err; } memset(&un, 0, sizeof(un)); @@ -496,18 +475,20 @@ int unix_listen_opts(QemuOpts *opts) unlink(un.sun_path); if (bind(sock, (struct sockaddr*) &un, sizeof(un)) < 0) { - fprintf(stderr, "bind(unix:%s): %s\n", un.sun_path, strerror(errno)); goto err; } if (listen(sock, 1) < 0) { - fprintf(stderr, "listen(unix:%s): %s\n", un.sun_path, strerror(errno)); goto err; } return sock; err: - closesocket(sock); + error_report("Can't create socket %s: %s", + un.sun_path, strerror(errno)); + if (sock >= 0) { + closesocket(sock); + } return -1; } @@ -518,26 +499,31 @@ int unix_connect_opts(QemuOpts *opts) int sock; if (NULL == path) { - fprintf(stderr, "unix connect: no path specified\n"); + error_report("unix socket character device requires parameter path"); return -1; } sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0); if (sock < 0) { - perror("socket(unix)"); - return -1; + goto err; } memset(&un, 0, sizeof(un)); un.sun_family = AF_UNIX; snprintf(un.sun_path, sizeof(un.sun_path), "%s", path); if (connect(sock, (struct sockaddr*) &un, sizeof(un)) < 0) { - fprintf(stderr, "connect(unix:%s): %s\n", path, strerror(errno)); - close(sock); - return -1; + goto err; } return sock; + +err: + error_report("Can't connect to socket %s: %s", + un.sun_path, strerror(errno)); + if (sock >= 0) { + close(sock); + } + return -1; } /* compatibility wrapper */ @@ -586,14 +572,14 @@ int unix_connect(const char *path) int unix_listen_opts(QemuOpts *opts) { - fprintf(stderr, "unix sockets are not available on windows\n"); + error_report("unix sockets are not available on windows"); errno = ENOTSUP; return -1; } int unix_connect_opts(QemuOpts *opts) { - fprintf(stderr, "unix sockets are not available on windows\n"); + error_report("unix sockets are not available on windows"); errno = ENOTSUP; return -1; }
Convert to error_report(). This adds error locations to the messages, which is particularly important when the location is buried in a configuration file. Moreover, we'll need this when we create a monitor command to add character devices, so its errors actually appear in the monitor, not stderr. Also clean up the messages, and get rid of some that look like errors, but aren't. Improves user-hostile messages like this one for "-chardev socket,id=foo,host=blackfin,port=1,server" inet_listen_opts: bind(ipv4,192.168.2.9,1): Permission denied chardev: opening backend "socket" failed to qemu-system-x86_64: -chardev socket,id=foo,host=blackfin,port=1,server: Can't bind port blackfin:1: Permission denied chardev: opening backend "socket" failed and this one for "-chardev udp,id=foo,localport=1,port=1" inet_dgram_opts: bind(ipv4,0.0.0.0,1): OK inet_dgram_opts failed chardev: opening backend "udp" failed to qemu-system-x86_64: -chardev udp,id=foo,localport=1,port=1: Can't bind port :1: Permission denied chardev: opening backend "udp" failed You got to love the "OK" part. The uninformative extra "opening backend failed" message will be cleaned up shortly. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- qemu-char.c | 1 - qemu-sockets.c | 154 +++++++++++++++++++++++++------------------------------ 2 files changed, 70 insertions(+), 85 deletions(-)