* [Qemu-devel] [PATCH 01/19] Revert "qemu-char: Print strerror message on failure" and deps
2012-02-07 14:09 [Qemu-devel] [PATCH 00/19] Fix and improve chardev open error messages Markus Armbruster
@ 2012-02-07 14:09 ` Markus Armbruster
2012-02-07 15:06 ` Anthony Liguori
2012-02-07 14:09 ` [Qemu-devel] [PATCH 02/19] qemu-char: Use qemu_open() to avoid leaking fds to children Markus Armbruster
` (19 subsequent siblings)
20 siblings, 1 reply; 45+ messages in thread
From: Markus Armbruster @ 2012-02-07 14:09 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, aliguori
The commit's purpose is laudable:
The only way for chardev drivers to communicate an error was to
return a NULL pointer, which resulted in an error message that
said _that_ something went wrong, but not _why_.
It attempts to achieve it by changing the interface to return 0/-errno
and update qemu_chr_open_opts() to use strerror() to display a more
helpful error message. Unfortunately, it has serious flaws:
1. Backends "socket" and "udp" return bogus error codes, because
qemu_chr_open_socket() and qemu_chr_open_udp() assume that
unix_listen_opts(), unix_connect_opts(), inet_listen_opts(),
inet_connect_opts() and inet_dgram_opts() fail with errno set
appropriately. That assumption is wrong, and the commit turns
unspecific error messages into misleading error messages. For
instance:
$ qemu-system-x86_64 -nodefaults -vnc :0 -chardev socket,id=bar,host=xxx
inet_connect: host and/or port not specified
chardev: opening backend "socket" failed: No such file or directory
ENOENT is what happens to be in my errno when the backend returns
-errno. Let's put ERANGE there just for giggles:
$ qemu-system-x86_64 -nodefaults -vnc :0 -chardev socket,id=bar,host=xxx -drive if=none,iops=99999999999999999999
inet_connect: host and/or port not specified
chardev: opening backend "socket" failed: Numerical result out of range
Worse: when errno happens to be zero, return -errno erroneously
signals success, and qemu_chr_new_from_opts() dies dereferencing
uninitialized chr. I observe this with "-serial unix:".
2. All qemu_chr_open_opts() knows about the error is an errno error
code. That's simply not enough for a decent message. For instance,
when inet_dgram() can't resolve the parameter host, which errno code
should it use? What if it can't resolve parameter localaddr?
Clue: many backends already report errors in their open methods.
Let's revert the flawed commit along with its dependencies, and fix up
the silent error paths instead.
This reverts commit 6e1db57b2ac9025c2443c665a0d9e78748637b26.
Conflicts:
console.c
hw/baum.c
qemu-char.c
This reverts commit aad04cd024f0c59f0b96f032cde2e24eb3abba6d.
The parts of commit db418a0a "Add stdio char device on windows" that
depend on the reverted change fixed up.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
console.c | 8 +--
console.h | 2 +-
hw/baum.c | 7 +-
hw/baum.h | 2 +-
hw/msmouse.c | 5 +-
hw/msmouse.h | 2 +-
qemu-char.c | 169 +++++++++++++++++++++-------------------------------
spice-qemu-char.c | 9 +--
ui/qemu-spice.h | 2 +-
9 files changed, 84 insertions(+), 122 deletions(-)
diff --git a/console.c b/console.c
index 135394f..2c432e3 100644
--- a/console.c
+++ b/console.c
@@ -1510,7 +1510,7 @@ static void text_console_do_init(CharDriverState *chr, DisplayState *ds)
chr->init(chr);
}
-int text_console_init(QemuOpts *opts, CharDriverState **_chr)
+CharDriverState *text_console_init(QemuOpts *opts)
{
CharDriverState *chr;
TextConsole *s;
@@ -1542,7 +1542,7 @@ int text_console_init(QemuOpts *opts, CharDriverState **_chr)
if (!s) {
g_free(chr);
- return -EBUSY;
+ return NULL;
}
s->chr = chr;
@@ -1550,9 +1550,7 @@ int text_console_init(QemuOpts *opts, CharDriverState **_chr)
s->g_height = height;
chr->opaque = s;
chr->chr_set_echo = text_console_set_echo;
-
- *_chr = chr;
- return 0;
+ return chr;
}
void text_consoles_set_display(DisplayState *ds)
diff --git a/console.h b/console.h
index 6ba0d5d..c33ffe0 100644
--- a/console.h
+++ b/console.h
@@ -356,7 +356,7 @@ void vga_hw_text_update(console_ch_t *chardata);
int is_graphic_console(void);
int is_fixedsize_console(void);
-int text_console_init(QemuOpts *opts, CharDriverState **_chr);
+CharDriverState *text_console_init(QemuOpts *opts);
void text_consoles_set_display(DisplayState *ds);
void console_select(unsigned int index);
void console_color_init(DisplayState *ds);
diff --git a/hw/baum.c b/hw/baum.c
index 86d780a..3e94f84 100644
--- a/hw/baum.c
+++ b/hw/baum.c
@@ -562,7 +562,7 @@ static void baum_close(struct CharDriverState *chr)
g_free(baum);
}
-int chr_baum_init(QemuOpts *opts, CharDriverState **_chr)
+CharDriverState *chr_baum_init(QemuOpts *opts)
{
BaumDriverState *baum;
CharDriverState *chr;
@@ -614,8 +614,7 @@ int chr_baum_init(QemuOpts *opts, CharDriverState **_chr)
qemu_chr_generic_open(chr);
- *_chr = chr;
- return 0;
+ return chr;
fail:
qemu_free_timer(baum->cellCount_timer);
@@ -624,5 +623,5 @@ fail_handle:
g_free(handle);
g_free(chr);
g_free(baum);
- return -EIO;
+ return NULL;
}
diff --git a/hw/baum.h b/hw/baum.h
index 3f28cc3..8af710f 100644
--- a/hw/baum.h
+++ b/hw/baum.h
@@ -23,4 +23,4 @@
*/
/* char device */
-int chr_baum_init(QemuOpts *opts, CharDriverState **_chr);
+CharDriverState *chr_baum_init(QemuOpts *opts);
diff --git a/hw/msmouse.c b/hw/msmouse.c
index c3b57ea..9c492a4 100644
--- a/hw/msmouse.c
+++ b/hw/msmouse.c
@@ -64,7 +64,7 @@ static void msmouse_chr_close (struct CharDriverState *chr)
g_free (chr);
}
-int qemu_chr_open_msmouse(QemuOpts *opts, CharDriverState **_chr)
+CharDriverState *qemu_chr_open_msmouse(QemuOpts *opts)
{
CharDriverState *chr;
@@ -74,6 +74,5 @@ int qemu_chr_open_msmouse(QemuOpts *opts, CharDriverState **_chr)
qemu_add_mouse_event_handler(msmouse_event, chr, 0, "QEMU Microsoft Mouse");
- *_chr = chr;
- return 0;
+ return chr;
}
diff --git a/hw/msmouse.h b/hw/msmouse.h
index 8b853b3..456cb21 100644
--- a/hw/msmouse.h
+++ b/hw/msmouse.h
@@ -1,2 +1,2 @@
/* msmouse.c */
-int qemu_chr_open_msmouse(QemuOpts *opts, CharDriverState **_chr);
+CharDriverState *qemu_chr_open_msmouse(QemuOpts *opts);
diff --git a/qemu-char.c b/qemu-char.c
index b1d80dd..1e882cf 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -218,15 +218,13 @@ static int null_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
return len;
}
-static int qemu_chr_open_null(QemuOpts *opts, CharDriverState **_chr)
+static CharDriverState *qemu_chr_open_null(QemuOpts *opts)
{
CharDriverState *chr;
chr = g_malloc0(sizeof(CharDriverState));
chr->chr_write = null_chr_write;
-
- *_chr= chr;
- return 0;
+ return chr;
}
/* MUX driver for serial I/O splitting */
@@ -636,21 +634,18 @@ static CharDriverState *qemu_chr_open_fd(int fd_in, int fd_out)
return chr;
}
-static int qemu_chr_open_file_out(QemuOpts *opts, CharDriverState **_chr)
+static CharDriverState *qemu_chr_open_file_out(QemuOpts *opts)
{
int fd_out;
TFR(fd_out = qemu_open(qemu_opt_get(opts, "path"),
O_WRONLY | O_TRUNC | O_CREAT | O_BINARY, 0666));
- if (fd_out < 0) {
- return -errno;
- }
-
- *_chr = qemu_chr_open_fd(-1, fd_out);
- return 0;
+ if (fd_out < 0)
+ return NULL;
+ return qemu_chr_open_fd(-1, fd_out);
}
-static int qemu_chr_open_pipe(QemuOpts *opts, CharDriverState **_chr)
+static CharDriverState *qemu_chr_open_pipe(QemuOpts *opts)
{
int fd_in, fd_out;
char filename_in[256], filename_out[256];
@@ -658,7 +653,7 @@ static int qemu_chr_open_pipe(QemuOpts *opts, CharDriverState **_chr)
if (filename == NULL) {
fprintf(stderr, "chardev: pipe: no filename given\n");
- return -EINVAL;
+ return NULL;
}
snprintf(filename_in, 256, "%s.in", filename);
@@ -670,14 +665,11 @@ static int qemu_chr_open_pipe(QemuOpts *opts, CharDriverState **_chr)
close(fd_in);
if (fd_out >= 0)
close(fd_out);
- TFR(fd_in = fd_out = qemu_open(filename, O_RDWR | O_BINARY));
- if (fd_in < 0) {
- return -errno;
- }
+ TFR(fd_in = fd_out = open(filename, O_RDWR | O_BINARY));
+ if (fd_in < 0)
+ return NULL;
}
-
- *_chr = qemu_chr_open_fd(fd_in, fd_out);
- return 0;
+ return qemu_chr_open_fd(fd_in, fd_out);
}
@@ -768,14 +760,12 @@ static void qemu_chr_close_stdio(struct CharDriverState *chr)
fd_chr_close(chr);
}
-static int qemu_chr_open_stdio(QemuOpts *opts, CharDriverState **_chr)
+static CharDriverState *qemu_chr_open_stdio(QemuOpts *opts)
{
CharDriverState *chr;
- if (stdio_nb_clients >= STDIO_MAX_CLIENTS) {
- return -EBUSY;
- }
-
+ if (stdio_nb_clients >= STDIO_MAX_CLIENTS)
+ return NULL;
if (stdio_nb_clients == 0) {
old_fd0_flags = fcntl(0, F_GETFL);
tcgetattr (0, &oldtty);
@@ -792,8 +782,7 @@ static int qemu_chr_open_stdio(QemuOpts *opts, CharDriverState **_chr)
display_type != DT_NOGRAPHIC);
qemu_chr_fe_set_echo(chr, false);
- *_chr = chr;
- return 0;
+ return chr;
}
#ifdef __sun__
@@ -980,7 +969,7 @@ static void pty_chr_close(struct CharDriverState *chr)
qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
}
-static int qemu_chr_open_pty(QemuOpts *opts, CharDriverState **_chr)
+static CharDriverState *qemu_chr_open_pty(QemuOpts *opts)
{
CharDriverState *chr;
PtyCharDriver *s;
@@ -995,7 +984,7 @@ static int qemu_chr_open_pty(QemuOpts *opts, CharDriverState **_chr)
#endif
if (openpty(&master_fd, &slave_fd, pty_name, NULL, NULL) < 0) {
- return -errno;
+ return NULL;
}
/* Set raw attributes on the pty. */
@@ -1021,8 +1010,7 @@ static int qemu_chr_open_pty(QemuOpts *opts, CharDriverState **_chr)
s->fd = master_fd;
s->timer = qemu_new_timer_ms(rt_clock, pty_chr_timer, chr);
- *_chr = chr;
- return 0;
+ return chr;
}
static void tty_serial_init(int fd, int speed,
@@ -1223,28 +1211,30 @@ static void qemu_chr_close_tty(CharDriverState *chr)
}
}
-static int qemu_chr_open_tty(QemuOpts *opts, CharDriverState **_chr)
+static CharDriverState *qemu_chr_open_tty(QemuOpts *opts)
{
const char *filename = qemu_opt_get(opts, "path");
CharDriverState *chr;
int fd;
- TFR(fd = qemu_open(filename, O_RDWR | O_NONBLOCK));
+ TFR(fd = open(filename, O_RDWR | O_NONBLOCK));
if (fd < 0) {
- return -errno;
+ return NULL;
}
tty_serial_init(fd, 115200, 'N', 8, 1);
chr = qemu_chr_open_fd(fd, fd);
+ if (!chr) {
+ close(fd);
+ return NULL;
+ }
chr->chr_ioctl = tty_serial_ioctl;
chr->chr_close = qemu_chr_close_tty;
-
- *_chr = chr;
- return 0;
+ return chr;
}
#else /* ! __linux__ && ! __sun__ */
-static int qemu_chr_open_pty(QemuOpts *opts, CharDriverState **_chr)
+static CharDriverState *qemu_chr_open_pty(QemuOpts *opts)
{
- return -ENOTSUP;
+ return NULL;
}
#endif /* __linux__ || __sun__ */
@@ -1358,7 +1348,7 @@ static void pp_close(CharDriverState *chr)
qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
}
-static int qemu_chr_open_pp(QemuOpts *opts, CharDriverState **_chr)
+static CharDriverState *qemu_chr_open_pp(QemuOpts *opts)
{
const char *filename = qemu_opt_get(opts, "path");
CharDriverState *chr;
@@ -1366,13 +1356,12 @@ static int qemu_chr_open_pp(QemuOpts *opts, CharDriverState **_chr)
int fd;
TFR(fd = open(filename, O_RDWR));
- if (fd < 0) {
- return -errno;
- }
+ if (fd < 0)
+ return NULL;
if (ioctl(fd, PPCLAIM) < 0) {
close(fd);
- return -errno;
+ return NULL;
}
drv = g_malloc0(sizeof(ParallelCharDriver));
@@ -1387,8 +1376,7 @@ static int qemu_chr_open_pp(QemuOpts *opts, CharDriverState **_chr)
qemu_chr_generic_open(chr);
- *_chr = chr;
- return 0;
+ return chr;
}
#endif /* __linux__ */
@@ -1430,24 +1418,21 @@ static int pp_ioctl(CharDriverState *chr, int cmd, void *arg)
return 0;
}
-static int qemu_chr_open_pp(QemuOpts *opts, CharDriverState **_chr)
+static CharDriverState *qemu_chr_open_pp(QemuOpts *opts)
{
const char *filename = qemu_opt_get(opts, "path");
CharDriverState *chr;
int fd;
- fd = qemu_open(filename, O_RDWR);
- if (fd < 0) {
- return -errno;
- }
+ fd = open(filename, O_RDWR);
+ if (fd < 0)
+ return NULL;
chr = g_malloc0(sizeof(CharDriverState));
chr->opaque = (void *)(intptr_t)fd;
chr->chr_write = null_chr_write;
chr->chr_ioctl = pp_ioctl;
-
- *_chr = chr;
- return 0;
+ return chr;
}
#endif
@@ -1663,7 +1648,7 @@ static int win_chr_poll(void *opaque)
return 0;
}
-static int qemu_chr_open_win(QemuOpts *opts, CharDriverState **_chr)
+static CharDriverState *qemu_chr_open_win(QemuOpts *opts)
{
const char *filename = qemu_opt_get(opts, "path");
CharDriverState *chr;
@@ -1678,12 +1663,10 @@ static int qemu_chr_open_win(QemuOpts *opts, CharDriverState **_chr)
if (win_chr_init(chr, filename) < 0) {
g_free(s);
g_free(chr);
- return -EIO;
+ return NULL;
}
qemu_chr_generic_open(chr);
-
- *_chr = chr;
- return 0;
+ return chr;
}
static int win_chr_pipe_poll(void *opaque)
@@ -1765,7 +1748,7 @@ static int win_chr_pipe_init(CharDriverState *chr, const char *filename)
}
-static int qemu_chr_open_win_pipe(QemuOpts *opts, CharDriverState **_chr)
+static CharDriverState *qemu_chr_open_win_pipe(QemuOpts *opts)
{
const char *filename = qemu_opt_get(opts, "path");
CharDriverState *chr;
@@ -1780,15 +1763,13 @@ static int qemu_chr_open_win_pipe(QemuOpts *opts, CharDriverState **_chr)
if (win_chr_pipe_init(chr, filename) < 0) {
g_free(s);
g_free(chr);
- return -EIO;
+ return NULL;
}
qemu_chr_generic_open(chr);
-
- *_chr = chr;
- return 0;
+ return chr;
}
-static int qemu_chr_open_win_file(HANDLE fd_out, CharDriverState **pchr)
+static CharDriverState *qemu_chr_open_win_file(HANDLE fd_out)
{
CharDriverState *chr;
WinCharState *s;
@@ -1799,27 +1780,25 @@ static int qemu_chr_open_win_file(HANDLE fd_out, CharDriverState **pchr)
chr->opaque = s;
chr->chr_write = win_chr_write;
qemu_chr_generic_open(chr);
- *pchr = chr;
- return 0;
+ return chr;
}
-static int qemu_chr_open_win_con(QemuOpts *opts, CharDriverState **chr)
+static CharDriverState *qemu_chr_open_win_con(QemuOpts *opts)
{
- return qemu_chr_open_win_file(GetStdHandle(STD_OUTPUT_HANDLE), chr);
+ return qemu_chr_open_win_file(GetStdHandle(STD_OUTPUT_HANDLE));
}
-static int qemu_chr_open_win_file_out(QemuOpts *opts, CharDriverState **_chr)
+static CharDriverState *qemu_chr_open_win_file_out(QemuOpts *opts)
{
const char *file_out = qemu_opt_get(opts, "path");
HANDLE fd_out;
fd_out = CreateFile(file_out, GENERIC_WRITE, FILE_SHARE_READ, NULL,
OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
- if (fd_out == INVALID_HANDLE_VALUE) {
- return -EIO;
- }
+ if (fd_out == INVALID_HANDLE_VALUE)
+ return NULL;
- return qemu_chr_open_win_file(fd_out, _chr);
+ return qemu_chr_open_win_file(fd_out);
}
static int win_stdio_write(CharDriverState *chr, const uint8_t *buf, int len)
@@ -1960,7 +1939,7 @@ static void win_stdio_close(CharDriverState *chr)
stdio_nb_clients--;
}
-static int qemu_chr_open_win_stdio(QemuOpts *opts, CharDriverState **_chr)
+static CharDriverState *qemu_chr_open_win_stdio(QemuOpts *opts)
{
CharDriverState *chr;
WinStdioCharState *stdio;
@@ -1969,7 +1948,7 @@ static int qemu_chr_open_win_stdio(QemuOpts *opts, CharDriverState **_chr)
if (stdio_nb_clients >= STDIO_MAX_CLIENTS
|| ((display_type != DT_NOGRAPHIC) && (stdio_nb_clients != 0))) {
- return -EIO;
+ return NULL;
}
chr = g_malloc0(sizeof(CharDriverState));
@@ -2028,9 +2007,7 @@ static int qemu_chr_open_win_stdio(QemuOpts *opts, CharDriverState **_chr)
chr->chr_set_echo = qemu_chr_set_echo_win_stdio;
qemu_chr_fe_set_echo(chr, false);
- *_chr = chr;
-
- return 0;
+ return chr;
}
#endif /* !_WIN32 */
@@ -2111,12 +2088,11 @@ static void udp_chr_close(CharDriverState *chr)
qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
}
-static int qemu_chr_open_udp(QemuOpts *opts, CharDriverState **_chr)
+static CharDriverState *qemu_chr_open_udp(QemuOpts *opts)
{
CharDriverState *chr = NULL;
NetCharDriver *s = NULL;
int fd = -1;
- int ret;
chr = g_malloc0(sizeof(CharDriverState));
s = g_malloc0(sizeof(NetCharDriver));
@@ -2124,7 +2100,6 @@ static int qemu_chr_open_udp(QemuOpts *opts, CharDriverState **_chr)
fd = inet_dgram_opts(opts);
if (fd < 0) {
fprintf(stderr, "inet_dgram_opts failed\n");
- ret = -errno;
goto return_err;
}
@@ -2135,9 +2110,7 @@ static int qemu_chr_open_udp(QemuOpts *opts, CharDriverState **_chr)
chr->chr_write = udp_chr_write;
chr->chr_update_read_handler = udp_chr_update_read_handler;
chr->chr_close = udp_chr_close;
-
- *_chr = chr;
- return 0;
+ return chr;
return_err:
g_free(chr);
@@ -2145,7 +2118,7 @@ return_err:
if (fd >= 0) {
closesocket(fd);
}
- return ret;
+ return NULL;
}
/***********************************************************/
@@ -2436,7 +2409,7 @@ static void tcp_chr_close(CharDriverState *chr)
qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
}
-static int qemu_chr_open_socket(QemuOpts *opts, CharDriverState **_chr)
+static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
{
CharDriverState *chr = NULL;
TCPCharDriver *s = NULL;
@@ -2446,7 +2419,6 @@ static int qemu_chr_open_socket(QemuOpts *opts, CharDriverState **_chr)
int do_nodelay;
int is_unix;
int is_telnet;
- int ret;
is_listen = qemu_opt_get_bool(opts, "server", 0);
is_waitconnect = qemu_opt_get_bool(opts, "wait", 1);
@@ -2472,10 +2444,8 @@ static int qemu_chr_open_socket(QemuOpts *opts, CharDriverState **_chr)
fd = inet_connect_opts(opts);
}
}
- if (fd < 0) {
- ret = -errno;
+ if (fd < 0)
goto fail;
- }
if (!is_waitconnect)
socket_set_nonblock(fd);
@@ -2528,16 +2498,14 @@ static int qemu_chr_open_socket(QemuOpts *opts, CharDriverState **_chr)
tcp_chr_accept(chr);
socket_set_nonblock(s->listen_fd);
}
-
- *_chr = chr;
- return 0;
+ return chr;
fail:
if (fd >= 0)
closesocket(fd);
g_free(s);
g_free(chr);
- return ret;
+ return NULL;
}
/***********************************************************/
@@ -2730,7 +2698,7 @@ fail:
static const struct {
const char *name;
- int (*open)(QemuOpts *opts, CharDriverState **chr);
+ CharDriverState *(*open)(QemuOpts *opts);
} backend_table[] = {
{ .name = "null", .open = qemu_chr_open_null },
{ .name = "socket", .open = qemu_chr_open_socket },
@@ -2771,7 +2739,6 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
{
CharDriverState *chr;
int i;
- int ret;
if (qemu_opts_id(opts) == NULL) {
fprintf(stderr, "chardev: no id specified\n");
@@ -2793,10 +2760,10 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
return NULL;
}
- ret = backend_table[i].open(opts, &chr);
- if (ret < 0) {
- fprintf(stderr, "chardev: opening backend \"%s\" failed: %s\n",
- qemu_opt_get(opts, "backend"), strerror(-ret));
+ chr = backend_table[i].open(opts);
+ if (!chr) {
+ fprintf(stderr, "chardev: opening backend \"%s\" failed\n",
+ qemu_opt_get(opts, "backend"));
return NULL;
}
diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index 7e8eaa9..1e735ff 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -188,7 +188,7 @@ static void print_allowed_subtypes(void)
fprintf(stderr, "\n");
}
-int qemu_chr_open_spice(QemuOpts *opts, CharDriverState **_chr)
+CharDriverState *qemu_chr_open_spice(QemuOpts *opts)
{
CharDriverState *chr;
SpiceCharDriver *s;
@@ -200,7 +200,7 @@ int qemu_chr_open_spice(QemuOpts *opts, CharDriverState **_chr)
if (name == NULL) {
fprintf(stderr, "spice-qemu-char: missing name parameter\n");
print_allowed_subtypes();
- return -EINVAL;
+ return NULL;
}
for(;*psubtype != NULL; ++psubtype) {
if (strcmp(name, *psubtype) == 0) {
@@ -211,7 +211,7 @@ int qemu_chr_open_spice(QemuOpts *opts, CharDriverState **_chr)
if (subtype == NULL) {
fprintf(stderr, "spice-qemu-char: unsupported name\n");
print_allowed_subtypes();
- return -EINVAL;
+ return NULL;
}
chr = g_malloc0(sizeof(CharDriverState));
@@ -233,6 +233,5 @@ int qemu_chr_open_spice(QemuOpts *opts, CharDriverState **_chr)
}
#endif
- *_chr = chr;
- return 0;
+ return chr;
}
diff --git a/ui/qemu-spice.h b/ui/qemu-spice.h
index c35b29c..a6c4a2c 100644
--- a/ui/qemu-spice.h
+++ b/ui/qemu-spice.h
@@ -44,7 +44,7 @@ int qemu_spice_migrate_info(const char *hostname, int port, int tls_port,
void do_info_spice_print(Monitor *mon, const QObject *data);
void do_info_spice(Monitor *mon, QObject **ret_data);
-int qemu_chr_open_spice(QemuOpts *opts, CharDriverState **_chr);
+CharDriverState *qemu_chr_open_spice(QemuOpts *opts);
#else /* CONFIG_SPICE */
#include "monitor.h"
--
1.7.6.5
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [Qemu-devel] [PATCH 01/19] Revert "qemu-char: Print strerror message on failure" and deps
2012-02-07 14:09 ` [Qemu-devel] [PATCH 01/19] Revert "qemu-char: Print strerror message on failure" and deps Markus Armbruster
@ 2012-02-07 15:06 ` Anthony Liguori
0 siblings, 0 replies; 45+ messages in thread
From: Anthony Liguori @ 2012-02-07 15:06 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, qemu-devel
On 02/07/2012 08:09 AM, Markus Armbruster wrote:
> The commit's purpose is laudable:
>
> The only way for chardev drivers to communicate an error was to
> return a NULL pointer, which resulted in an error message that
> said _that_ something went wrong, but not _why_.
>
> It attempts to achieve it by changing the interface to return 0/-errno
> and update qemu_chr_open_opts() to use strerror() to display a more
> helpful error message. Unfortunately, it has serious flaws:
>
> 1. Backends "socket" and "udp" return bogus error codes, because
> qemu_chr_open_socket() and qemu_chr_open_udp() assume that
> unix_listen_opts(), unix_connect_opts(), inet_listen_opts(),
> inet_connect_opts() and inet_dgram_opts() fail with errno set
> appropriately. That assumption is wrong, and the commit turns
> unspecific error messages into misleading error messages. For
> instance:
>
> $ qemu-system-x86_64 -nodefaults -vnc :0 -chardev socket,id=bar,host=xxx
> inet_connect: host and/or port not specified
> chardev: opening backend "socket" failed: No such file or directory
>
> ENOENT is what happens to be in my errno when the backend returns
> -errno. Let's put ERANGE there just for giggles:
>
> $ qemu-system-x86_64 -nodefaults -vnc :0 -chardev socket,id=bar,host=xxx -drive if=none,iops=99999999999999999999
> inet_connect: host and/or port not specified
> chardev: opening backend "socket" failed: Numerical result out of range
>
> Worse: when errno happens to be zero, return -errno erroneously
> signals success, and qemu_chr_new_from_opts() dies dereferencing
> uninitialized chr. I observe this with "-serial unix:".
>
> 2. All qemu_chr_open_opts() knows about the error is an errno error
> code. That's simply not enough for a decent message. For instance,
> when inet_dgram() can't resolve the parameter host, which errno code
> should it use? What if it can't resolve parameter localaddr?
>
> Clue: many backends already report errors in their open methods.
> Let's revert the flawed commit along with its dependencies, and fix up
> the silent error paths instead.
>
> This reverts commit 6e1db57b2ac9025c2443c665a0d9e78748637b26.
Ack on this, but see comments in other patches for what I think we should do
instead.
Regards,
Anthony Liguori
>
> Conflicts:
>
> console.c
> hw/baum.c
> qemu-char.c
>
> This reverts commit aad04cd024f0c59f0b96f032cde2e24eb3abba6d.
>
> The parts of commit db418a0a "Add stdio char device on windows" that
> depend on the reverted change fixed up.
>
> Signed-off-by: Markus Armbruster<armbru@redhat.com>
> ---
> console.c | 8 +--
> console.h | 2 +-
> hw/baum.c | 7 +-
> hw/baum.h | 2 +-
> hw/msmouse.c | 5 +-
> hw/msmouse.h | 2 +-
> qemu-char.c | 169 +++++++++++++++++++++-------------------------------
> spice-qemu-char.c | 9 +--
> ui/qemu-spice.h | 2 +-
> 9 files changed, 84 insertions(+), 122 deletions(-)
>
> diff --git a/console.c b/console.c
> index 135394f..2c432e3 100644
> --- a/console.c
> +++ b/console.c
> @@ -1510,7 +1510,7 @@ static void text_console_do_init(CharDriverState *chr, DisplayState *ds)
> chr->init(chr);
> }
>
> -int text_console_init(QemuOpts *opts, CharDriverState **_chr)
> +CharDriverState *text_console_init(QemuOpts *opts)
> {
> CharDriverState *chr;
> TextConsole *s;
> @@ -1542,7 +1542,7 @@ int text_console_init(QemuOpts *opts, CharDriverState **_chr)
>
> if (!s) {
> g_free(chr);
> - return -EBUSY;
> + return NULL;
> }
>
> s->chr = chr;
> @@ -1550,9 +1550,7 @@ int text_console_init(QemuOpts *opts, CharDriverState **_chr)
> s->g_height = height;
> chr->opaque = s;
> chr->chr_set_echo = text_console_set_echo;
> -
> - *_chr = chr;
> - return 0;
> + return chr;
> }
>
> void text_consoles_set_display(DisplayState *ds)
> diff --git a/console.h b/console.h
> index 6ba0d5d..c33ffe0 100644
> --- a/console.h
> +++ b/console.h
> @@ -356,7 +356,7 @@ void vga_hw_text_update(console_ch_t *chardata);
>
> int is_graphic_console(void);
> int is_fixedsize_console(void);
> -int text_console_init(QemuOpts *opts, CharDriverState **_chr);
> +CharDriverState *text_console_init(QemuOpts *opts);
> void text_consoles_set_display(DisplayState *ds);
> void console_select(unsigned int index);
> void console_color_init(DisplayState *ds);
> diff --git a/hw/baum.c b/hw/baum.c
> index 86d780a..3e94f84 100644
> --- a/hw/baum.c
> +++ b/hw/baum.c
> @@ -562,7 +562,7 @@ static void baum_close(struct CharDriverState *chr)
> g_free(baum);
> }
>
> -int chr_baum_init(QemuOpts *opts, CharDriverState **_chr)
> +CharDriverState *chr_baum_init(QemuOpts *opts)
> {
> BaumDriverState *baum;
> CharDriverState *chr;
> @@ -614,8 +614,7 @@ int chr_baum_init(QemuOpts *opts, CharDriverState **_chr)
>
> qemu_chr_generic_open(chr);
>
> - *_chr = chr;
> - return 0;
> + return chr;
>
> fail:
> qemu_free_timer(baum->cellCount_timer);
> @@ -624,5 +623,5 @@ fail_handle:
> g_free(handle);
> g_free(chr);
> g_free(baum);
> - return -EIO;
> + return NULL;
> }
> diff --git a/hw/baum.h b/hw/baum.h
> index 3f28cc3..8af710f 100644
> --- a/hw/baum.h
> +++ b/hw/baum.h
> @@ -23,4 +23,4 @@
> */
>
> /* char device */
> -int chr_baum_init(QemuOpts *opts, CharDriverState **_chr);
> +CharDriverState *chr_baum_init(QemuOpts *opts);
> diff --git a/hw/msmouse.c b/hw/msmouse.c
> index c3b57ea..9c492a4 100644
> --- a/hw/msmouse.c
> +++ b/hw/msmouse.c
> @@ -64,7 +64,7 @@ static void msmouse_chr_close (struct CharDriverState *chr)
> g_free (chr);
> }
>
> -int qemu_chr_open_msmouse(QemuOpts *opts, CharDriverState **_chr)
> +CharDriverState *qemu_chr_open_msmouse(QemuOpts *opts)
> {
> CharDriverState *chr;
>
> @@ -74,6 +74,5 @@ int qemu_chr_open_msmouse(QemuOpts *opts, CharDriverState **_chr)
>
> qemu_add_mouse_event_handler(msmouse_event, chr, 0, "QEMU Microsoft Mouse");
>
> - *_chr = chr;
> - return 0;
> + return chr;
> }
> diff --git a/hw/msmouse.h b/hw/msmouse.h
> index 8b853b3..456cb21 100644
> --- a/hw/msmouse.h
> +++ b/hw/msmouse.h
> @@ -1,2 +1,2 @@
> /* msmouse.c */
> -int qemu_chr_open_msmouse(QemuOpts *opts, CharDriverState **_chr);
> +CharDriverState *qemu_chr_open_msmouse(QemuOpts *opts);
> diff --git a/qemu-char.c b/qemu-char.c
> index b1d80dd..1e882cf 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -218,15 +218,13 @@ static int null_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
> return len;
> }
>
> -static int qemu_chr_open_null(QemuOpts *opts, CharDriverState **_chr)
> +static CharDriverState *qemu_chr_open_null(QemuOpts *opts)
> {
> CharDriverState *chr;
>
> chr = g_malloc0(sizeof(CharDriverState));
> chr->chr_write = null_chr_write;
> -
> - *_chr= chr;
> - return 0;
> + return chr;
> }
>
> /* MUX driver for serial I/O splitting */
> @@ -636,21 +634,18 @@ static CharDriverState *qemu_chr_open_fd(int fd_in, int fd_out)
> return chr;
> }
>
> -static int qemu_chr_open_file_out(QemuOpts *opts, CharDriverState **_chr)
> +static CharDriverState *qemu_chr_open_file_out(QemuOpts *opts)
> {
> int fd_out;
>
> TFR(fd_out = qemu_open(qemu_opt_get(opts, "path"),
> O_WRONLY | O_TRUNC | O_CREAT | O_BINARY, 0666));
> - if (fd_out< 0) {
> - return -errno;
> - }
> -
> - *_chr = qemu_chr_open_fd(-1, fd_out);
> - return 0;
> + if (fd_out< 0)
> + return NULL;
> + return qemu_chr_open_fd(-1, fd_out);
> }
>
> -static int qemu_chr_open_pipe(QemuOpts *opts, CharDriverState **_chr)
> +static CharDriverState *qemu_chr_open_pipe(QemuOpts *opts)
> {
> int fd_in, fd_out;
> char filename_in[256], filename_out[256];
> @@ -658,7 +653,7 @@ static int qemu_chr_open_pipe(QemuOpts *opts, CharDriverState **_chr)
>
> if (filename == NULL) {
> fprintf(stderr, "chardev: pipe: no filename given\n");
> - return -EINVAL;
> + return NULL;
> }
>
> snprintf(filename_in, 256, "%s.in", filename);
> @@ -670,14 +665,11 @@ static int qemu_chr_open_pipe(QemuOpts *opts, CharDriverState **_chr)
> close(fd_in);
> if (fd_out>= 0)
> close(fd_out);
> - TFR(fd_in = fd_out = qemu_open(filename, O_RDWR | O_BINARY));
> - if (fd_in< 0) {
> - return -errno;
> - }
> + TFR(fd_in = fd_out = open(filename, O_RDWR | O_BINARY));
> + if (fd_in< 0)
> + return NULL;
> }
> -
> - *_chr = qemu_chr_open_fd(fd_in, fd_out);
> - return 0;
> + return qemu_chr_open_fd(fd_in, fd_out);
> }
>
>
> @@ -768,14 +760,12 @@ static void qemu_chr_close_stdio(struct CharDriverState *chr)
> fd_chr_close(chr);
> }
>
> -static int qemu_chr_open_stdio(QemuOpts *opts, CharDriverState **_chr)
> +static CharDriverState *qemu_chr_open_stdio(QemuOpts *opts)
> {
> CharDriverState *chr;
>
> - if (stdio_nb_clients>= STDIO_MAX_CLIENTS) {
> - return -EBUSY;
> - }
> -
> + if (stdio_nb_clients>= STDIO_MAX_CLIENTS)
> + return NULL;
> if (stdio_nb_clients == 0) {
> old_fd0_flags = fcntl(0, F_GETFL);
> tcgetattr (0,&oldtty);
> @@ -792,8 +782,7 @@ static int qemu_chr_open_stdio(QemuOpts *opts, CharDriverState **_chr)
> display_type != DT_NOGRAPHIC);
> qemu_chr_fe_set_echo(chr, false);
>
> - *_chr = chr;
> - return 0;
> + return chr;
> }
>
> #ifdef __sun__
> @@ -980,7 +969,7 @@ static void pty_chr_close(struct CharDriverState *chr)
> qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
> }
>
> -static int qemu_chr_open_pty(QemuOpts *opts, CharDriverState **_chr)
> +static CharDriverState *qemu_chr_open_pty(QemuOpts *opts)
> {
> CharDriverState *chr;
> PtyCharDriver *s;
> @@ -995,7 +984,7 @@ static int qemu_chr_open_pty(QemuOpts *opts, CharDriverState **_chr)
> #endif
>
> if (openpty(&master_fd,&slave_fd, pty_name, NULL, NULL)< 0) {
> - return -errno;
> + return NULL;
> }
>
> /* Set raw attributes on the pty. */
> @@ -1021,8 +1010,7 @@ static int qemu_chr_open_pty(QemuOpts *opts, CharDriverState **_chr)
> s->fd = master_fd;
> s->timer = qemu_new_timer_ms(rt_clock, pty_chr_timer, chr);
>
> - *_chr = chr;
> - return 0;
> + return chr;
> }
>
> static void tty_serial_init(int fd, int speed,
> @@ -1223,28 +1211,30 @@ static void qemu_chr_close_tty(CharDriverState *chr)
> }
> }
>
> -static int qemu_chr_open_tty(QemuOpts *opts, CharDriverState **_chr)
> +static CharDriverState *qemu_chr_open_tty(QemuOpts *opts)
> {
> const char *filename = qemu_opt_get(opts, "path");
> CharDriverState *chr;
> int fd;
>
> - TFR(fd = qemu_open(filename, O_RDWR | O_NONBLOCK));
> + TFR(fd = open(filename, O_RDWR | O_NONBLOCK));
> if (fd< 0) {
> - return -errno;
> + return NULL;
> }
> tty_serial_init(fd, 115200, 'N', 8, 1);
> chr = qemu_chr_open_fd(fd, fd);
> + if (!chr) {
> + close(fd);
> + return NULL;
> + }
> chr->chr_ioctl = tty_serial_ioctl;
> chr->chr_close = qemu_chr_close_tty;
> -
> - *_chr = chr;
> - return 0;
> + return chr;
> }
> #else /* ! __linux__&& ! __sun__ */
> -static int qemu_chr_open_pty(QemuOpts *opts, CharDriverState **_chr)
> +static CharDriverState *qemu_chr_open_pty(QemuOpts *opts)
> {
> - return -ENOTSUP;
> + return NULL;
> }
> #endif /* __linux__ || __sun__ */
>
> @@ -1358,7 +1348,7 @@ static void pp_close(CharDriverState *chr)
> qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
> }
>
> -static int qemu_chr_open_pp(QemuOpts *opts, CharDriverState **_chr)
> +static CharDriverState *qemu_chr_open_pp(QemuOpts *opts)
> {
> const char *filename = qemu_opt_get(opts, "path");
> CharDriverState *chr;
> @@ -1366,13 +1356,12 @@ static int qemu_chr_open_pp(QemuOpts *opts, CharDriverState **_chr)
> int fd;
>
> TFR(fd = open(filename, O_RDWR));
> - if (fd< 0) {
> - return -errno;
> - }
> + if (fd< 0)
> + return NULL;
>
> if (ioctl(fd, PPCLAIM)< 0) {
> close(fd);
> - return -errno;
> + return NULL;
> }
>
> drv = g_malloc0(sizeof(ParallelCharDriver));
> @@ -1387,8 +1376,7 @@ static int qemu_chr_open_pp(QemuOpts *opts, CharDriverState **_chr)
>
> qemu_chr_generic_open(chr);
>
> - *_chr = chr;
> - return 0;
> + return chr;
> }
> #endif /* __linux__ */
>
> @@ -1430,24 +1418,21 @@ static int pp_ioctl(CharDriverState *chr, int cmd, void *arg)
> return 0;
> }
>
> -static int qemu_chr_open_pp(QemuOpts *opts, CharDriverState **_chr)
> +static CharDriverState *qemu_chr_open_pp(QemuOpts *opts)
> {
> const char *filename = qemu_opt_get(opts, "path");
> CharDriverState *chr;
> int fd;
>
> - fd = qemu_open(filename, O_RDWR);
> - if (fd< 0) {
> - return -errno;
> - }
> + fd = open(filename, O_RDWR);
> + if (fd< 0)
> + return NULL;
>
> chr = g_malloc0(sizeof(CharDriverState));
> chr->opaque = (void *)(intptr_t)fd;
> chr->chr_write = null_chr_write;
> chr->chr_ioctl = pp_ioctl;
> -
> - *_chr = chr;
> - return 0;
> + return chr;
> }
> #endif
>
> @@ -1663,7 +1648,7 @@ static int win_chr_poll(void *opaque)
> return 0;
> }
>
> -static int qemu_chr_open_win(QemuOpts *opts, CharDriverState **_chr)
> +static CharDriverState *qemu_chr_open_win(QemuOpts *opts)
> {
> const char *filename = qemu_opt_get(opts, "path");
> CharDriverState *chr;
> @@ -1678,12 +1663,10 @@ static int qemu_chr_open_win(QemuOpts *opts, CharDriverState **_chr)
> if (win_chr_init(chr, filename)< 0) {
> g_free(s);
> g_free(chr);
> - return -EIO;
> + return NULL;
> }
> qemu_chr_generic_open(chr);
> -
> - *_chr = chr;
> - return 0;
> + return chr;
> }
>
> static int win_chr_pipe_poll(void *opaque)
> @@ -1765,7 +1748,7 @@ static int win_chr_pipe_init(CharDriverState *chr, const char *filename)
> }
>
>
> -static int qemu_chr_open_win_pipe(QemuOpts *opts, CharDriverState **_chr)
> +static CharDriverState *qemu_chr_open_win_pipe(QemuOpts *opts)
> {
> const char *filename = qemu_opt_get(opts, "path");
> CharDriverState *chr;
> @@ -1780,15 +1763,13 @@ static int qemu_chr_open_win_pipe(QemuOpts *opts, CharDriverState **_chr)
> if (win_chr_pipe_init(chr, filename)< 0) {
> g_free(s);
> g_free(chr);
> - return -EIO;
> + return NULL;
> }
> qemu_chr_generic_open(chr);
> -
> - *_chr = chr;
> - return 0;
> + return chr;
> }
>
> -static int qemu_chr_open_win_file(HANDLE fd_out, CharDriverState **pchr)
> +static CharDriverState *qemu_chr_open_win_file(HANDLE fd_out)
> {
> CharDriverState *chr;
> WinCharState *s;
> @@ -1799,27 +1780,25 @@ static int qemu_chr_open_win_file(HANDLE fd_out, CharDriverState **pchr)
> chr->opaque = s;
> chr->chr_write = win_chr_write;
> qemu_chr_generic_open(chr);
> - *pchr = chr;
> - return 0;
> + return chr;
> }
>
> -static int qemu_chr_open_win_con(QemuOpts *opts, CharDriverState **chr)
> +static CharDriverState *qemu_chr_open_win_con(QemuOpts *opts)
> {
> - return qemu_chr_open_win_file(GetStdHandle(STD_OUTPUT_HANDLE), chr);
> + return qemu_chr_open_win_file(GetStdHandle(STD_OUTPUT_HANDLE));
> }
>
> -static int qemu_chr_open_win_file_out(QemuOpts *opts, CharDriverState **_chr)
> +static CharDriverState *qemu_chr_open_win_file_out(QemuOpts *opts)
> {
> const char *file_out = qemu_opt_get(opts, "path");
> HANDLE fd_out;
>
> fd_out = CreateFile(file_out, GENERIC_WRITE, FILE_SHARE_READ, NULL,
> OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
> - if (fd_out == INVALID_HANDLE_VALUE) {
> - return -EIO;
> - }
> + if (fd_out == INVALID_HANDLE_VALUE)
> + return NULL;
>
> - return qemu_chr_open_win_file(fd_out, _chr);
> + return qemu_chr_open_win_file(fd_out);
> }
>
> static int win_stdio_write(CharDriverState *chr, const uint8_t *buf, int len)
> @@ -1960,7 +1939,7 @@ static void win_stdio_close(CharDriverState *chr)
> stdio_nb_clients--;
> }
>
> -static int qemu_chr_open_win_stdio(QemuOpts *opts, CharDriverState **_chr)
> +static CharDriverState *qemu_chr_open_win_stdio(QemuOpts *opts)
> {
> CharDriverState *chr;
> WinStdioCharState *stdio;
> @@ -1969,7 +1948,7 @@ static int qemu_chr_open_win_stdio(QemuOpts *opts, CharDriverState **_chr)
>
> if (stdio_nb_clients>= STDIO_MAX_CLIENTS
> || ((display_type != DT_NOGRAPHIC)&& (stdio_nb_clients != 0))) {
> - return -EIO;
> + return NULL;
> }
>
> chr = g_malloc0(sizeof(CharDriverState));
> @@ -2028,9 +2007,7 @@ static int qemu_chr_open_win_stdio(QemuOpts *opts, CharDriverState **_chr)
> chr->chr_set_echo = qemu_chr_set_echo_win_stdio;
> qemu_chr_fe_set_echo(chr, false);
>
> - *_chr = chr;
> -
> - return 0;
> + return chr;
> }
> #endif /* !_WIN32 */
>
> @@ -2111,12 +2088,11 @@ static void udp_chr_close(CharDriverState *chr)
> qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
> }
>
> -static int qemu_chr_open_udp(QemuOpts *opts, CharDriverState **_chr)
> +static CharDriverState *qemu_chr_open_udp(QemuOpts *opts)
> {
> CharDriverState *chr = NULL;
> NetCharDriver *s = NULL;
> int fd = -1;
> - int ret;
>
> chr = g_malloc0(sizeof(CharDriverState));
> s = g_malloc0(sizeof(NetCharDriver));
> @@ -2124,7 +2100,6 @@ static int qemu_chr_open_udp(QemuOpts *opts, CharDriverState **_chr)
> fd = inet_dgram_opts(opts);
> if (fd< 0) {
> fprintf(stderr, "inet_dgram_opts failed\n");
> - ret = -errno;
> goto return_err;
> }
>
> @@ -2135,9 +2110,7 @@ static int qemu_chr_open_udp(QemuOpts *opts, CharDriverState **_chr)
> chr->chr_write = udp_chr_write;
> chr->chr_update_read_handler = udp_chr_update_read_handler;
> chr->chr_close = udp_chr_close;
> -
> - *_chr = chr;
> - return 0;
> + return chr;
>
> return_err:
> g_free(chr);
> @@ -2145,7 +2118,7 @@ return_err:
> if (fd>= 0) {
> closesocket(fd);
> }
> - return ret;
> + return NULL;
> }
>
> /***********************************************************/
> @@ -2436,7 +2409,7 @@ static void tcp_chr_close(CharDriverState *chr)
> qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
> }
>
> -static int qemu_chr_open_socket(QemuOpts *opts, CharDriverState **_chr)
> +static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
> {
> CharDriverState *chr = NULL;
> TCPCharDriver *s = NULL;
> @@ -2446,7 +2419,6 @@ static int qemu_chr_open_socket(QemuOpts *opts, CharDriverState **_chr)
> int do_nodelay;
> int is_unix;
> int is_telnet;
> - int ret;
>
> is_listen = qemu_opt_get_bool(opts, "server", 0);
> is_waitconnect = qemu_opt_get_bool(opts, "wait", 1);
> @@ -2472,10 +2444,8 @@ static int qemu_chr_open_socket(QemuOpts *opts, CharDriverState **_chr)
> fd = inet_connect_opts(opts);
> }
> }
> - if (fd< 0) {
> - ret = -errno;
> + if (fd< 0)
> goto fail;
> - }
>
> if (!is_waitconnect)
> socket_set_nonblock(fd);
> @@ -2528,16 +2498,14 @@ static int qemu_chr_open_socket(QemuOpts *opts, CharDriverState **_chr)
> tcp_chr_accept(chr);
> socket_set_nonblock(s->listen_fd);
> }
> -
> - *_chr = chr;
> - return 0;
> + return chr;
>
> fail:
> if (fd>= 0)
> closesocket(fd);
> g_free(s);
> g_free(chr);
> - return ret;
> + return NULL;
> }
>
> /***********************************************************/
> @@ -2730,7 +2698,7 @@ fail:
>
> static const struct {
> const char *name;
> - int (*open)(QemuOpts *opts, CharDriverState **chr);
> + CharDriverState *(*open)(QemuOpts *opts);
> } backend_table[] = {
> { .name = "null", .open = qemu_chr_open_null },
> { .name = "socket", .open = qemu_chr_open_socket },
> @@ -2771,7 +2739,6 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
> {
> CharDriverState *chr;
> int i;
> - int ret;
>
> if (qemu_opts_id(opts) == NULL) {
> fprintf(stderr, "chardev: no id specified\n");
> @@ -2793,10 +2760,10 @@ CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
> return NULL;
> }
>
> - ret = backend_table[i].open(opts,&chr);
> - if (ret< 0) {
> - fprintf(stderr, "chardev: opening backend \"%s\" failed: %s\n",
> - qemu_opt_get(opts, "backend"), strerror(-ret));
> + chr = backend_table[i].open(opts);
> + if (!chr) {
> + fprintf(stderr, "chardev: opening backend \"%s\" failed\n",
> + qemu_opt_get(opts, "backend"));
> return NULL;
> }
>
> diff --git a/spice-qemu-char.c b/spice-qemu-char.c
> index 7e8eaa9..1e735ff 100644
> --- a/spice-qemu-char.c
> +++ b/spice-qemu-char.c
> @@ -188,7 +188,7 @@ static void print_allowed_subtypes(void)
> fprintf(stderr, "\n");
> }
>
> -int qemu_chr_open_spice(QemuOpts *opts, CharDriverState **_chr)
> +CharDriverState *qemu_chr_open_spice(QemuOpts *opts)
> {
> CharDriverState *chr;
> SpiceCharDriver *s;
> @@ -200,7 +200,7 @@ int qemu_chr_open_spice(QemuOpts *opts, CharDriverState **_chr)
> if (name == NULL) {
> fprintf(stderr, "spice-qemu-char: missing name parameter\n");
> print_allowed_subtypes();
> - return -EINVAL;
> + return NULL;
> }
> for(;*psubtype != NULL; ++psubtype) {
> if (strcmp(name, *psubtype) == 0) {
> @@ -211,7 +211,7 @@ int qemu_chr_open_spice(QemuOpts *opts, CharDriverState **_chr)
> if (subtype == NULL) {
> fprintf(stderr, "spice-qemu-char: unsupported name\n");
> print_allowed_subtypes();
> - return -EINVAL;
> + return NULL;
> }
>
> chr = g_malloc0(sizeof(CharDriverState));
> @@ -233,6 +233,5 @@ int qemu_chr_open_spice(QemuOpts *opts, CharDriverState **_chr)
> }
> #endif
>
> - *_chr = chr;
> - return 0;
> + return chr;
> }
> diff --git a/ui/qemu-spice.h b/ui/qemu-spice.h
> index c35b29c..a6c4a2c 100644
> --- a/ui/qemu-spice.h
> +++ b/ui/qemu-spice.h
> @@ -44,7 +44,7 @@ int qemu_spice_migrate_info(const char *hostname, int port, int tls_port,
> void do_info_spice_print(Monitor *mon, const QObject *data);
> void do_info_spice(Monitor *mon, QObject **ret_data);
>
> -int qemu_chr_open_spice(QemuOpts *opts, CharDriverState **_chr);
> +CharDriverState *qemu_chr_open_spice(QemuOpts *opts);
>
> #else /* CONFIG_SPICE */
> #include "monitor.h"
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Qemu-devel] [PATCH 02/19] qemu-char: Use qemu_open() to avoid leaking fds to children
2012-02-07 14:09 [Qemu-devel] [PATCH 00/19] Fix and improve chardev open error messages Markus Armbruster
2012-02-07 14:09 ` [Qemu-devel] [PATCH 01/19] Revert "qemu-char: Print strerror message on failure" and deps Markus Armbruster
@ 2012-02-07 14:09 ` Markus Armbruster
2012-02-07 15:07 ` Anthony Liguori
2012-02-07 14:09 ` [Qemu-devel] [PATCH 03/19] qemu-char: Re-apply style fixes from just reverted aad04cd0 Markus Armbruster
` (18 subsequent siblings)
20 siblings, 1 reply; 45+ messages in thread
From: Markus Armbruster @ 2012-02-07 14:09 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, aliguori
Fixed silently in commit aad04cd0, but that just got reverted.
Re-apply the fixes, plus one missed instance: parport on Linux.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
qemu-char.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/qemu-char.c b/qemu-char.c
index 1e882cf..368df2e 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -665,7 +665,7 @@ static CharDriverState *qemu_chr_open_pipe(QemuOpts *opts)
close(fd_in);
if (fd_out >= 0)
close(fd_out);
- TFR(fd_in = fd_out = open(filename, O_RDWR | O_BINARY));
+ TFR(fd_in = fd_out = qemu_open(filename, O_RDWR | O_BINARY));
if (fd_in < 0)
return NULL;
}
@@ -1217,7 +1217,7 @@ static CharDriverState *qemu_chr_open_tty(QemuOpts *opts)
CharDriverState *chr;
int fd;
- TFR(fd = open(filename, O_RDWR | O_NONBLOCK));
+ TFR(fd = qemu_open(filename, O_RDWR | O_NONBLOCK));
if (fd < 0) {
return NULL;
}
@@ -1355,7 +1355,7 @@ static CharDriverState *qemu_chr_open_pp(QemuOpts *opts)
ParallelCharDriver *drv;
int fd;
- TFR(fd = open(filename, O_RDWR));
+ TFR(fd = qemu_open(filename, O_RDWR));
if (fd < 0)
return NULL;
@@ -1424,7 +1424,7 @@ static CharDriverState *qemu_chr_open_pp(QemuOpts *opts)
CharDriverState *chr;
int fd;
- fd = open(filename, O_RDWR);
+ fd = qemu_open(filename, O_RDWR);
if (fd < 0)
return NULL;
--
1.7.6.5
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [Qemu-devel] [PATCH 02/19] qemu-char: Use qemu_open() to avoid leaking fds to children
2012-02-07 14:09 ` [Qemu-devel] [PATCH 02/19] qemu-char: Use qemu_open() to avoid leaking fds to children Markus Armbruster
@ 2012-02-07 15:07 ` Anthony Liguori
0 siblings, 0 replies; 45+ messages in thread
From: Anthony Liguori @ 2012-02-07 15:07 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, qemu-devel
On 02/07/2012 08:09 AM, Markus Armbruster wrote:
> Fixed silently in commit aad04cd0, but that just got reverted.
> Re-apply the fixes, plus one missed instance: parport on Linux.
>
> Signed-off-by: Markus Armbruster<armbru@redhat.com>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
Regards,
Anthony Liguori
> ---
> qemu-char.c | 8 ++++----
> 1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index 1e882cf..368df2e 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -665,7 +665,7 @@ static CharDriverState *qemu_chr_open_pipe(QemuOpts *opts)
> close(fd_in);
> if (fd_out>= 0)
> close(fd_out);
> - TFR(fd_in = fd_out = open(filename, O_RDWR | O_BINARY));
> + TFR(fd_in = fd_out = qemu_open(filename, O_RDWR | O_BINARY));
> if (fd_in< 0)
> return NULL;
> }
> @@ -1217,7 +1217,7 @@ static CharDriverState *qemu_chr_open_tty(QemuOpts *opts)
> CharDriverState *chr;
> int fd;
>
> - TFR(fd = open(filename, O_RDWR | O_NONBLOCK));
> + TFR(fd = qemu_open(filename, O_RDWR | O_NONBLOCK));
> if (fd< 0) {
> return NULL;
> }
> @@ -1355,7 +1355,7 @@ static CharDriverState *qemu_chr_open_pp(QemuOpts *opts)
> ParallelCharDriver *drv;
> int fd;
>
> - TFR(fd = open(filename, O_RDWR));
> + TFR(fd = qemu_open(filename, O_RDWR));
> if (fd< 0)
> return NULL;
>
> @@ -1424,7 +1424,7 @@ static CharDriverState *qemu_chr_open_pp(QemuOpts *opts)
> CharDriverState *chr;
> int fd;
>
> - fd = open(filename, O_RDWR);
> + fd = qemu_open(filename, O_RDWR);
> if (fd< 0)
> return NULL;
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Qemu-devel] [PATCH 03/19] qemu-char: Re-apply style fixes from just reverted aad04cd0
2012-02-07 14:09 [Qemu-devel] [PATCH 00/19] Fix and improve chardev open error messages Markus Armbruster
2012-02-07 14:09 ` [Qemu-devel] [PATCH 01/19] Revert "qemu-char: Print strerror message on failure" and deps Markus Armbruster
2012-02-07 14:09 ` [Qemu-devel] [PATCH 02/19] qemu-char: Use qemu_open() to avoid leaking fds to children Markus Armbruster
@ 2012-02-07 14:09 ` Markus Armbruster
2012-02-07 15:07 ` Anthony Liguori
2012-02-07 14:09 ` [Qemu-devel] [PATCH 04/19] qemu-char: qemu_chr_open_fd() can't fail, don't check Markus Armbruster
` (17 subsequent siblings)
20 siblings, 1 reply; 45+ messages in thread
From: Markus Armbruster @ 2012-02-07 14:09 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, aliguori
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
qemu-char.c | 21 ++++++++++++++-------
1 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/qemu-char.c b/qemu-char.c
index 368df2e..c25863a 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -640,8 +640,9 @@ static CharDriverState *qemu_chr_open_file_out(QemuOpts *opts)
TFR(fd_out = qemu_open(qemu_opt_get(opts, "path"),
O_WRONLY | O_TRUNC | O_CREAT | O_BINARY, 0666));
- if (fd_out < 0)
+ if (fd_out < 0) {
return NULL;
+ }
return qemu_chr_open_fd(-1, fd_out);
}
@@ -666,8 +667,9 @@ static CharDriverState *qemu_chr_open_pipe(QemuOpts *opts)
if (fd_out >= 0)
close(fd_out);
TFR(fd_in = fd_out = qemu_open(filename, O_RDWR | O_BINARY));
- if (fd_in < 0)
+ if (fd_in < 0) {
return NULL;
+ }
}
return qemu_chr_open_fd(fd_in, fd_out);
}
@@ -764,8 +766,9 @@ static CharDriverState *qemu_chr_open_stdio(QemuOpts *opts)
{
CharDriverState *chr;
- if (stdio_nb_clients >= STDIO_MAX_CLIENTS)
+ if (stdio_nb_clients >= STDIO_MAX_CLIENTS) {
return NULL;
+ }
if (stdio_nb_clients == 0) {
old_fd0_flags = fcntl(0, F_GETFL);
tcgetattr (0, &oldtty);
@@ -1356,8 +1359,9 @@ static CharDriverState *qemu_chr_open_pp(QemuOpts *opts)
int fd;
TFR(fd = qemu_open(filename, O_RDWR));
- if (fd < 0)
+ if (fd < 0) {
return NULL;
+ }
if (ioctl(fd, PPCLAIM) < 0) {
close(fd);
@@ -1425,8 +1429,9 @@ static CharDriverState *qemu_chr_open_pp(QemuOpts *opts)
int fd;
fd = qemu_open(filename, O_RDWR);
- if (fd < 0)
+ if (fd < 0) {
return NULL;
+ }
chr = g_malloc0(sizeof(CharDriverState));
chr->opaque = (void *)(intptr_t)fd;
@@ -1795,8 +1800,9 @@ static CharDriverState *qemu_chr_open_win_file_out(QemuOpts *opts)
fd_out = CreateFile(file_out, GENERIC_WRITE, FILE_SHARE_READ, NULL,
OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
- if (fd_out == INVALID_HANDLE_VALUE)
+ if (fd_out == INVALID_HANDLE_VALUE) {
return NULL;
+ }
return qemu_chr_open_win_file(fd_out);
}
@@ -2444,8 +2450,9 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
fd = inet_connect_opts(opts);
}
}
- if (fd < 0)
+ if (fd < 0) {
goto fail;
+ }
if (!is_waitconnect)
socket_set_nonblock(fd);
--
1.7.6.5
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [Qemu-devel] [PATCH 03/19] qemu-char: Re-apply style fixes from just reverted aad04cd0
2012-02-07 14:09 ` [Qemu-devel] [PATCH 03/19] qemu-char: Re-apply style fixes from just reverted aad04cd0 Markus Armbruster
@ 2012-02-07 15:07 ` Anthony Liguori
0 siblings, 0 replies; 45+ messages in thread
From: Anthony Liguori @ 2012-02-07 15:07 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, qemu-devel
On 02/07/2012 08:09 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster<armbru@redhat.com>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
Regards,
Anthony Liguori
> ---
> qemu-char.c | 21 ++++++++++++++-------
> 1 files changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index 368df2e..c25863a 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -640,8 +640,9 @@ static CharDriverState *qemu_chr_open_file_out(QemuOpts *opts)
>
> TFR(fd_out = qemu_open(qemu_opt_get(opts, "path"),
> O_WRONLY | O_TRUNC | O_CREAT | O_BINARY, 0666));
> - if (fd_out< 0)
> + if (fd_out< 0) {
> return NULL;
> + }
> return qemu_chr_open_fd(-1, fd_out);
> }
>
> @@ -666,8 +667,9 @@ static CharDriverState *qemu_chr_open_pipe(QemuOpts *opts)
> if (fd_out>= 0)
> close(fd_out);
> TFR(fd_in = fd_out = qemu_open(filename, O_RDWR | O_BINARY));
> - if (fd_in< 0)
> + if (fd_in< 0) {
> return NULL;
> + }
> }
> return qemu_chr_open_fd(fd_in, fd_out);
> }
> @@ -764,8 +766,9 @@ static CharDriverState *qemu_chr_open_stdio(QemuOpts *opts)
> {
> CharDriverState *chr;
>
> - if (stdio_nb_clients>= STDIO_MAX_CLIENTS)
> + if (stdio_nb_clients>= STDIO_MAX_CLIENTS) {
> return NULL;
> + }
> if (stdio_nb_clients == 0) {
> old_fd0_flags = fcntl(0, F_GETFL);
> tcgetattr (0,&oldtty);
> @@ -1356,8 +1359,9 @@ static CharDriverState *qemu_chr_open_pp(QemuOpts *opts)
> int fd;
>
> TFR(fd = qemu_open(filename, O_RDWR));
> - if (fd< 0)
> + if (fd< 0) {
> return NULL;
> + }
>
> if (ioctl(fd, PPCLAIM)< 0) {
> close(fd);
> @@ -1425,8 +1429,9 @@ static CharDriverState *qemu_chr_open_pp(QemuOpts *opts)
> int fd;
>
> fd = qemu_open(filename, O_RDWR);
> - if (fd< 0)
> + if (fd< 0) {
> return NULL;
> + }
>
> chr = g_malloc0(sizeof(CharDriverState));
> chr->opaque = (void *)(intptr_t)fd;
> @@ -1795,8 +1800,9 @@ static CharDriverState *qemu_chr_open_win_file_out(QemuOpts *opts)
>
> fd_out = CreateFile(file_out, GENERIC_WRITE, FILE_SHARE_READ, NULL,
> OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
> - if (fd_out == INVALID_HANDLE_VALUE)
> + if (fd_out == INVALID_HANDLE_VALUE) {
> return NULL;
> + }
>
> return qemu_chr_open_win_file(fd_out);
> }
> @@ -2444,8 +2450,9 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
> fd = inet_connect_opts(opts);
> }
> }
> - if (fd< 0)
> + if (fd< 0) {
> goto fail;
> + }
>
> if (!is_waitconnect)
> socket_set_nonblock(fd);
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Qemu-devel] [PATCH 04/19] qemu-char: qemu_chr_open_fd() can't fail, don't check
2012-02-07 14:09 [Qemu-devel] [PATCH 00/19] Fix and improve chardev open error messages Markus Armbruster
` (2 preceding siblings ...)
2012-02-07 14:09 ` [Qemu-devel] [PATCH 03/19] qemu-char: Re-apply style fixes from just reverted aad04cd0 Markus Armbruster
@ 2012-02-07 14:09 ` Markus Armbruster
2012-02-07 15:24 ` Anthony Liguori
2012-02-07 14:09 ` [Qemu-devel] [PATCH 05/19] vl.c: Error locations for options using add_device_config() Markus Armbruster
` (16 subsequent siblings)
20 siblings, 1 reply; 45+ messages in thread
From: Markus Armbruster @ 2012-02-07 14:09 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, aliguori
Cleaned up silently in commit aad04cd0, but that just got reverted.
Re-apply this part.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
qemu-char.c | 4 ----
1 files changed, 0 insertions(+), 4 deletions(-)
diff --git a/qemu-char.c b/qemu-char.c
index c25863a..bb9e3f5 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1226,10 +1226,6 @@ static CharDriverState *qemu_chr_open_tty(QemuOpts *opts)
}
tty_serial_init(fd, 115200, 'N', 8, 1);
chr = qemu_chr_open_fd(fd, fd);
- if (!chr) {
- close(fd);
- return NULL;
- }
chr->chr_ioctl = tty_serial_ioctl;
chr->chr_close = qemu_chr_close_tty;
return chr;
--
1.7.6.5
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [Qemu-devel] [PATCH 04/19] qemu-char: qemu_chr_open_fd() can't fail, don't check
2012-02-07 14:09 ` [Qemu-devel] [PATCH 04/19] qemu-char: qemu_chr_open_fd() can't fail, don't check Markus Armbruster
@ 2012-02-07 15:24 ` Anthony Liguori
0 siblings, 0 replies; 45+ messages in thread
From: Anthony Liguori @ 2012-02-07 15:24 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, qemu-devel
On 02/07/2012 08:09 AM, Markus Armbruster wrote:
> Cleaned up silently in commit aad04cd0, but that just got reverted.
> Re-apply this part.
>
> Signed-off-by: Markus Armbruster<armbru@redhat.com>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
Regards,
Anthony Liguori
> ---
> qemu-char.c | 4 ----
> 1 files changed, 0 insertions(+), 4 deletions(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index c25863a..bb9e3f5 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -1226,10 +1226,6 @@ static CharDriverState *qemu_chr_open_tty(QemuOpts *opts)
> }
> tty_serial_init(fd, 115200, 'N', 8, 1);
> chr = qemu_chr_open_fd(fd, fd);
> - if (!chr) {
> - close(fd);
> - return NULL;
> - }
> chr->chr_ioctl = tty_serial_ioctl;
> chr->chr_close = qemu_chr_close_tty;
> return chr;
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Qemu-devel] [PATCH 05/19] vl.c: Error locations for options using add_device_config()
2012-02-07 14:09 [Qemu-devel] [PATCH 00/19] Fix and improve chardev open error messages Markus Armbruster
` (3 preceding siblings ...)
2012-02-07 14:09 ` [Qemu-devel] [PATCH 04/19] qemu-char: qemu_chr_open_fd() can't fail, don't check Markus Armbruster
@ 2012-02-07 14:09 ` Markus Armbruster
2012-02-07 14:09 ` [Qemu-devel] [PATCH 06/19] gdbstub: Error locations for -gdb Markus Armbruster
` (15 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Markus Armbruster @ 2012-02-07 14:09 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, aliguori
These are -bt, -serial, -virtcon, -parallel, -debugcon, -usbdevice.
Improves messages emitted via proper error reporting interfaces. For
instance:
$ qemu-system-x86_64 -nodefaults -S -usb -usbdevice net:vlan=xxx
qemu-system-x86_64: Parameter 'vlan' expects a number
becomes:
qemu-system-x86_64: -usbdevice net:vlan=xxx: Parameter 'vlan' expects a number
Many more remain unimproved, because they're fprintf()ed. The next
few commits will take care of that.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
vl.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/vl.c b/vl.c
index 2d464cf..e252a9d 100644
--- a/vl.c
+++ b/vl.c
@@ -1859,6 +1859,7 @@ struct device_config {
DEV_DEBUGCON, /* -debugcon */
} type;
const char *cmdline;
+ Location loc;
QTAILQ_ENTRY(device_config) next;
};
QTAILQ_HEAD(, device_config) device_configs = QTAILQ_HEAD_INITIALIZER(device_configs);
@@ -1870,6 +1871,7 @@ static void add_device_config(int type, const char *cmdline)
conf = g_malloc0(sizeof(*conf));
conf->type = type;
conf->cmdline = cmdline;
+ loc_save(&conf->loc);
QTAILQ_INSERT_TAIL(&device_configs, conf, next);
}
@@ -1881,7 +1883,9 @@ static int foreach_device_config(int type, int (*func)(const char *cmdline))
QTAILQ_FOREACH(conf, &device_configs, next) {
if (conf->type != type)
continue;
+ loc_push_restore(&conf->loc);
rc = func(conf->cmdline);
+ loc_pop(&conf->loc);
if (0 != rc)
return rc;
}
--
1.7.6.5
^ permalink raw reply related [flat|nested] 45+ messages in thread* [Qemu-devel] [PATCH 06/19] gdbstub: Error locations for -gdb
2012-02-07 14:09 [Qemu-devel] [PATCH 00/19] Fix and improve chardev open error messages Markus Armbruster
` (4 preceding siblings ...)
2012-02-07 14:09 ` [Qemu-devel] [PATCH 05/19] vl.c: Error locations for options using add_device_config() Markus Armbruster
@ 2012-02-07 14:09 ` Markus Armbruster
2012-02-07 15:32 ` Kevin Wolf
2012-02-07 14:09 ` [Qemu-devel] [PATCH 07/19] sockets: Drop sockets_debug debug code Markus Armbruster
` (14 subsequent siblings)
20 siblings, 1 reply; 45+ messages in thread
From: Markus Armbruster @ 2012-02-07 14:09 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, aliguori
Stash away the option argument with add_device_config(), so we still
have its location when we get around to parsing it.
This doesn't improve any messages I can see just yet, but that'll
change shortly.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
vl.c | 10 ++++------
1 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/vl.c b/vl.c
index e252a9d..1394a08 100644
--- a/vl.c
+++ b/vl.c
@@ -1857,6 +1857,7 @@ struct device_config {
DEV_PARALLEL, /* -parallel */
DEV_VIRTCON, /* -virtioconsole */
DEV_DEBUGCON, /* -debugcon */
+ DEV_GDB, /* -gdb, -s */
} type;
const char *cmdline;
Location loc;
@@ -2178,7 +2179,6 @@ int qemu_init_main_loop(void)
int main(int argc, char **argv, char **envp)
{
- const char *gdbstub_dev = NULL;
int i;
int snapshot, linux_boot;
const char *icount_option = NULL;
@@ -2598,10 +2598,10 @@ int main(int argc, char **argv, char **envp)
log_file = optarg;
break;
case QEMU_OPTION_s:
- gdbstub_dev = "tcp::" DEFAULT_GDBSTUB_PORT;
+ add_device_config(DEV_GDB, "tcp::" DEFAULT_GDBSTUB_PORT);
break;
case QEMU_OPTION_gdb:
- gdbstub_dev = optarg;
+ add_device_config(DEV_GDB, optarg);
break;
case QEMU_OPTION_L:
data_dir = optarg;
@@ -3482,9 +3482,7 @@ int main(int argc, char **argv, char **envp)
}
text_consoles_set_display(ds);
- if (gdbstub_dev && gdbserver_start(gdbstub_dev) < 0) {
- fprintf(stderr, "qemu: could not open gdbserver on device '%s'\n",
- gdbstub_dev);
+ if (foreach_device_config(DEV_GDB, gdbserver_start) < 0) {
exit(1);
}
--
1.7.6.5
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [Qemu-devel] [PATCH 06/19] gdbstub: Error locations for -gdb
2012-02-07 14:09 ` [Qemu-devel] [PATCH 06/19] gdbstub: Error locations for -gdb Markus Armbruster
@ 2012-02-07 15:32 ` Kevin Wolf
2012-02-09 15:08 ` Markus Armbruster
0 siblings, 1 reply; 45+ messages in thread
From: Kevin Wolf @ 2012-02-07 15:32 UTC (permalink / raw)
To: Markus Armbruster; +Cc: aliguori, qemu-devel
Am 07.02.2012 15:09, schrieb Markus Armbruster:
> Stash away the option argument with add_device_config(), so we still
> have its location when we get around to parsing it.
>
> This doesn't improve any messages I can see just yet, but that'll
> change shortly.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
gdb isn't really a device, though. Should the struct and the functions
be renamed?
Kevin
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] [PATCH 06/19] gdbstub: Error locations for -gdb
2012-02-07 15:32 ` Kevin Wolf
@ 2012-02-09 15:08 ` Markus Armbruster
0 siblings, 0 replies; 45+ messages in thread
From: Markus Armbruster @ 2012-02-09 15:08 UTC (permalink / raw)
To: Kevin Wolf; +Cc: aliguori, qemu-devel
Kevin Wolf <kwolf@redhat.com> writes:
> Am 07.02.2012 15:09, schrieb Markus Armbruster:
>> Stash away the option argument with add_device_config(), so we still
>> have its location when we get around to parsing it.
>>
>> This doesn't improve any messages I can see just yet, but that'll
>> change shortly.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> gdb isn't really a device, though. Should the struct and the functions
> be renamed?
The current names don't bother me, but I'd be fine with a rename, too.
Unless I need to respin anyway, I'd rather leave it to a follow-up
patch, though.
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Qemu-devel] [PATCH 07/19] sockets: Drop sockets_debug debug code
2012-02-07 14:09 [Qemu-devel] [PATCH 00/19] Fix and improve chardev open error messages Markus Armbruster
` (5 preceding siblings ...)
2012-02-07 14:09 ` [Qemu-devel] [PATCH 06/19] gdbstub: Error locations for -gdb Markus Armbruster
@ 2012-02-07 14:09 ` Markus Armbruster
2012-02-07 14:09 ` [Qemu-devel] [PATCH 08/19] sockets: Clean up inet_listen_opts()'s convoluted bind() loop Markus Armbruster
` (13 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Markus Armbruster @ 2012-02-07 14:09 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, aliguori
I'm trying to improve this code's error reporting, and the debug code
is getting in my way: it clutters the code, it clobbers errno in
inconvenient places, and it uses the same fprintf() both for error
reporting and debug output in a few places.
Get rid of it. Once decent error reporting is in place, adding back
whatever debug code we need shouldn't be hard.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
qemu-sockets.c | 43 ++-----------------------------------------
1 files changed, 2 insertions(+), 41 deletions(-)
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 61b2247..e6e6c72 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -26,7 +26,6 @@
# define AI_ADDRCONFIG 0
#endif
-static int sockets_debug = 0;
static const int on=1, off=0;
/* used temporarely until all users are converted to QemuOpts */
@@ -101,21 +100,6 @@ const char *inet_strfamily(int family)
return "unknown";
}
-static void inet_print_addrinfo(const char *tag, struct addrinfo *res)
-{
- struct addrinfo *e;
- char uaddr[INET6_ADDRSTRLEN+1];
- char uport[33];
-
- 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);
- fprintf(stderr,"%s: getaddrinfo: family %s, host %s, port %s\n",
- tag, inet_strfamily(e->ai_family), uaddr, uport);
- }
-}
-
int inet_listen_opts(QemuOpts *opts, int port_offset)
{
struct addrinfo ai,*res,*e;
@@ -153,8 +137,6 @@ int inet_listen_opts(QemuOpts *opts, int port_offset)
gai_strerror(rc));
return -1;
}
- if (sockets_debug)
- inet_print_addrinfo(__FUNCTION__, res);
/* create socket + bind */
for (e = res; e != NULL; e = e->ai_next) {
@@ -179,13 +161,10 @@ int inet_listen_opts(QemuOpts *opts, int port_offset)
for (;;) {
if (bind(slisten, e->ai_addr, e->ai_addrlen) == 0) {
- if (sockets_debug)
- fprintf(stderr,"%s: bind(%s,%s,%d): OK\n", __FUNCTION__,
- inet_strfamily(e->ai_family), uaddr, inet_getport(e));
goto listen;
}
try_next = to && (inet_getport(e) <= to + port_offset);
- if (!try_next || sockets_debug)
+ if (!try_next)
fprintf(stderr,"%s: bind(%s,%s,%d): %s\n", __FUNCTION__,
inet_strfamily(e->ai_family), uaddr, inet_getport(e),
strerror(errno));
@@ -249,8 +228,6 @@ int inet_connect_opts(QemuOpts *opts)
gai_strerror(rc));
return -1;
}
- if (sockets_debug)
- inet_print_addrinfo(__FUNCTION__, res);
for (e = res; e != NULL; e = e->ai_next) {
if (getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen,
@@ -269,17 +246,13 @@ int inet_connect_opts(QemuOpts *opts)
/* connect to peer */
if (connect(sock,e->ai_addr,e->ai_addrlen) < 0) {
- if (sockets_debug || NULL == e->ai_next)
+ 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;
}
- if (sockets_debug)
- fprintf(stderr, "%s: connect(%s,%s,%s,%s): OK\n", __FUNCTION__,
- inet_strfamily(e->ai_family),
- e->ai_canonname, uaddr, uport);
freeaddrinfo(res);
return sock;
}
@@ -322,10 +295,6 @@ int inet_dgram_opts(QemuOpts *opts)
gai_strerror(rc));
return -1;
}
- if (sockets_debug) {
- fprintf(stderr, "%s: peer (%s:%s)\n", __FUNCTION__, addr, port);
- inet_print_addrinfo(__FUNCTION__, peer);
- }
/* lookup local addr */
memset(&ai,0, sizeof(ai));
@@ -346,10 +315,6 @@ int inet_dgram_opts(QemuOpts *opts)
gai_strerror(rc));
return -1;
}
- if (sockets_debug) {
- fprintf(stderr, "%s: local (%s:%s)\n", __FUNCTION__, addr, port);
- inet_print_addrinfo(__FUNCTION__, local);
- }
/* create socket */
sock = qemu_socket(peer->ai_family, peer->ai_socktype, peer->ai_protocol);
@@ -541,8 +506,6 @@ int unix_listen_opts(QemuOpts *opts)
goto err;
}
- if (sockets_debug)
- fprintf(stderr, "bind(unix:%s): OK\n", un.sun_path);
return sock;
err:
@@ -576,8 +539,6 @@ int unix_connect_opts(QemuOpts *opts)
return -1;
}
- if (sockets_debug)
- fprintf(stderr, "connect(unix:%s): OK\n", path);
return sock;
}
--
1.7.6.5
^ permalink raw reply related [flat|nested] 45+ messages in thread* [Qemu-devel] [PATCH 08/19] sockets: Clean up inet_listen_opts()'s convoluted bind() loop
2012-02-07 14:09 [Qemu-devel] [PATCH 00/19] Fix and improve chardev open error messages Markus Armbruster
` (6 preceding siblings ...)
2012-02-07 14:09 ` [Qemu-devel] [PATCH 07/19] sockets: Drop sockets_debug debug code Markus Armbruster
@ 2012-02-07 14:09 ` Markus Armbruster
2012-02-07 14:09 ` [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part Markus Armbruster
` (12 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Markus Armbruster @ 2012-02-07 14:09 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, aliguori
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
qemu-sockets.c | 14 ++++++--------
1 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/qemu-sockets.c b/qemu-sockets.c
index e6e6c72..6bcb8e3 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -107,7 +107,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,try_next;
+ int slisten, rc, to, port_min, port_max, p;
memset(&ai,0, sizeof(ai));
ai.ai_flags = AI_PASSIVE | AI_ADDRCONFIG;
@@ -159,20 +159,18 @@ int inet_listen_opts(QemuOpts *opts, int port_offset)
}
#endif
- for (;;) {
+ port_min = inet_getport(e);
+ port_max = to ? to + port_offset : port_min;
+ for (p = port_min; p <= port_max; p++) {
+ inet_setport(e, p);
if (bind(slisten, e->ai_addr, e->ai_addrlen) == 0) {
goto listen;
}
- try_next = to && (inet_getport(e) <= to + port_offset);
- if (!try_next)
+ 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));
- if (try_next) {
- inet_setport(e, inet_getport(e) + 1);
- continue;
}
- break;
}
closesocket(slisten);
}
--
1.7.6.5
^ permalink raw reply related [flat|nested] 45+ messages in thread* [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part
2012-02-07 14:09 [Qemu-devel] [PATCH 00/19] Fix and improve chardev open error messages Markus Armbruster
` (7 preceding siblings ...)
2012-02-07 14:09 ` [Qemu-devel] [PATCH 08/19] sockets: Clean up inet_listen_opts()'s convoluted bind() loop Markus Armbruster
@ 2012-02-07 14:09 ` Markus Armbruster
2012-02-07 15:13 ` Anthony Liguori
2012-02-07 14:09 ` [Qemu-devel] [PATCH 10/19] qemu-char: Chardev open error reporting, !_WIN32 part Markus Armbruster
` (11 subsequent siblings)
20 siblings, 1 reply; 45+ messages in thread
From: Markus Armbruster @ 2012-02-07 14:09 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, aliguori
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;
}
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;
}
--
1.7.6.5
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part
2012-02-07 14:09 ` [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part Markus Armbruster
@ 2012-02-07 15:13 ` Anthony Liguori
2012-02-09 16:05 ` Markus Armbruster
0 siblings, 1 reply; 45+ messages in thread
From: Anthony Liguori @ 2012-02-07 15:13 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, qemu-devel
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
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part
2012-02-07 15:13 ` Anthony Liguori
@ 2012-02-09 16:05 ` Markus Armbruster
2012-02-14 17:24 ` Markus Armbruster
0 siblings, 1 reply; 45+ messages in thread
From: Markus Armbruster @ 2012-02-09 16:05 UTC (permalink / raw)
To: Anthony Liguori; +Cc: kwolf, qemu-devel
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?
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part
2012-02-09 16:05 ` Markus Armbruster
@ 2012-02-14 17:24 ` Markus Armbruster
2012-02-14 19:05 ` Anthony Liguori
0 siblings, 1 reply; 45+ messages in thread
From: Markus Armbruster @ 2012-02-14 17:24 UTC (permalink / raw)
To: Anthony Liguori; +Cc: kwolf, qemu-devel
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?
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part
2012-02-14 17:24 ` Markus Armbruster
@ 2012-02-14 19:05 ` Anthony Liguori
2012-02-15 13:33 ` Markus Armbruster
0 siblings, 1 reply; 45+ messages in thread
From: Anthony Liguori @ 2012-02-14 19:05 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, qemu-devel
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.
>
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part
2012-02-14 19:05 ` Anthony Liguori
@ 2012-02-15 13:33 ` Markus Armbruster
2012-02-22 20:28 ` Anthony Liguori
0 siblings, 1 reply; 45+ messages in thread
From: Markus Armbruster @ 2012-02-15 13:33 UTC (permalink / raw)
To: Anthony Liguori; +Cc: kwolf, qemu-devel
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.
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part
2012-02-15 13:33 ` Markus Armbruster
@ 2012-02-22 20:28 ` Anthony Liguori
2012-02-23 8:15 ` Markus Armbruster
0 siblings, 1 reply; 45+ messages in thread
From: Anthony Liguori @ 2012-02-22 20:28 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, qemu-devel
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
>
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part
2012-02-22 20:28 ` Anthony Liguori
@ 2012-02-23 8:15 ` Markus Armbruster
2012-08-29 15:15 ` Amos Kong
0 siblings, 1 reply; 45+ messages in thread
From: Markus Armbruster @ 2012-02-23 8:15 UTC (permalink / raw)
To: Anthony Liguori; +Cc: kwolf, qemu-devel
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.
[...]
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part
2012-02-23 8:15 ` Markus Armbruster
@ 2012-08-29 15:15 ` Amos Kong
2012-08-29 16:04 ` Amos Kong
0 siblings, 1 reply; 45+ messages in thread
From: Amos Kong @ 2012-08-29 15:15 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, akong, qemu-devel, Anthony Liguori
On Thu, Feb 23, 2012 at 4:15 PM, Markus Armbruster <armbru@redhat.com> wrote:
>
> 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 want to continue working on this work (patch 9~13,15~19).
Makus used error_report() to output the rich/meaningful error message
to monitor,
but Anthony prefers to use error_set(), right?
I would like to introduce a generic QERR_INTERNAL_ERROR as Anthony
said to replace error_report().
Please help to review below RFC patch, thanks.
>From 4b8200ce662dd375819fd24cb932e70131ce0bd3 Mon Sep 17 00:00:00 2001
From: Amos Kong <akong@redhat.com>
Date: Wed, 29 Aug 2012 22:52:48 +0800
Subject: [PATCH RFC] qerror: introduce QERR_INTERNAL_ERROR
Current qerror reporting infrastructure isn't agile,
we have to add a new Class for a new error.
This patch introduced a generic QERR_INTERNAL_ERROR
that takes a single human readable string as an argument.
This patch is a RFC, so I only changed some code of
inet_connect() as an example.
hmp:
(qemu) migrate -d tcp:noname:4446
migrate: Can't resolve noname:4446: Name or service not known
qmp:
{ "execute": "migrate", "arguments": { "uri": "tcp:noname:4446" } }
{"error": {"class": "GenericError", "desc": "Can't resolve noname:4446:
Name or service not known"}}
Signed-off-by: Amos Kong <akong@redhat.com>
---
qemu-sockets.c | 9 +++++----
qerror.h | 3 +++
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 361d890..e528288 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -244,9 +244,9 @@ int inet_connect_opts(QemuOpts *opts, bool
*in_progress, Error **errp)
/* lookup */
if (0 != (rc = getaddrinfo(addr, port, &ai, &res))) {
- fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port,
- gai_strerror(rc));
- error_set(errp, QERR_SOCKET_CREATE_FAILED);
+ char err[50];
+ sprintf(err, "Can't resolve %s:%s: %s", addr, port, gai_strerror(rc));
+ error_set(errp, QERR_INTERNAL_ERROR, err);
return -1;
}
@@ -505,7 +505,8 @@ int inet_connect(const char *str, bool block, bool
*in_progress, Error **errp)
}
sock = inet_connect_opts(opts, in_progress, errp);
} else {
- error_set(errp, QERR_SOCKET_CREATE_FAILED);
+ error_set(errp, QERR_INTERNAL_ERROR,
+ "inet_connect: failed to parse address");
}
qemu_opts_del(opts);
return sock;
diff --git a/qerror.h b/qerror.h
index d0a76a4..97bf5e0 100644
--- a/qerror.h
+++ b/qerror.h
@@ -246,4 +246,7 @@ void assert_no_error(Error *err);
#define QERR_SOCKET_CREATE_FAILED \
ERROR_CLASS_GENERIC_ERROR, "Failed to create socket"
+#define QERR_INTERNAL_ERROR \
+ ERROR_CLASS_GENERIC_ERROR, "%s"
+
#endif /* QERROR_H */
--
1.7.1
>
>
> > 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.
>
> [...]
>
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part
2012-08-29 15:15 ` Amos Kong
@ 2012-08-29 16:04 ` Amos Kong
2012-09-05 2:19 ` Amos Kong
0 siblings, 1 reply; 45+ messages in thread
From: Amos Kong @ 2012-08-29 16:04 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, akong, qemu-devel, Anthony Liguori
On Wed, Aug 29, 2012 at 11:15 PM, Amos Kong <kongjianjun@gmail.com> wrote:
> On Thu, Feb 23, 2012 at 4:15 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> 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 want to continue working on this work (patch 9~13,15~19).
> Makus used error_report() to output the rich/meaningful error message
> to monitor,
> but Anthony prefers to use error_set(), right?
>
> I would like to introduce a generic QERR_INTERNAL_ERROR as Anthony
> said to replace error_report().
error_report() can be used many times/places, we might see many error
in monitor.
but error_set() can only set one kind of error when internal error
occurs, and only
one error will be printed into monitor.
output final/important error by error_set()/error_report_err(), and
output other error to stdio?
---
There are some 'GENERIC ERROR CLASS' in qerror.h, which are not used
very frequently, we can convert them to 'QERR_INTERNAL_ERROR'.
Or convert all 'GENERIC ERROR CLASS' to 'QERR_INTERNAL_ERROR',
and use a single human readable string?
> Please help to review below RFC patch, thanks.
>
>
> From 4b8200ce662dd375819fd24cb932e70131ce0bd3 Mon Sep 17 00:00:00 2001
> From: Amos Kong <akong@redhat.com>
> Date: Wed, 29 Aug 2012 22:52:48 +0800
> Subject: [PATCH RFC] qerror: introduce QERR_INTERNAL_ERROR
>
> Current qerror reporting infrastructure isn't agile,
> we have to add a new Class for a new error.
>
> This patch introduced a generic QERR_INTERNAL_ERROR
> that takes a single human readable string as an argument.
>
> This patch is a RFC, so I only changed some code of
> inet_connect() as an example.
>
> hmp:
> (qemu) migrate -d tcp:noname:4446
> migrate: Can't resolve noname:4446: Name or service not known
>
> qmp:
> { "execute": "migrate", "arguments": { "uri": "tcp:noname:4446" } }
> {"error": {"class": "GenericError", "desc": "Can't resolve noname:4446:
> Name or service not known"}}
>
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
> qemu-sockets.c | 9 +++++----
> qerror.h | 3 +++
> 2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 361d890..e528288 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -244,9 +244,9 @@ int inet_connect_opts(QemuOpts *opts, bool
> *in_progress, Error **errp)
>
> /* lookup */
> if (0 != (rc = getaddrinfo(addr, port, &ai, &res))) {
> - fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port,
> - gai_strerror(rc));
> - error_set(errp, QERR_SOCKET_CREATE_FAILED);
> + char err[50];
> + sprintf(err, "Can't resolve %s:%s: %s", addr, port, gai_strerror(rc));
> + error_set(errp, QERR_INTERNAL_ERROR, err);
> return -1;
> }
>
> @@ -505,7 +505,8 @@ int inet_connect(const char *str, bool block, bool
> *in_progress, Error **errp)
> }
> sock = inet_connect_opts(opts, in_progress, errp);
> } else {
> - error_set(errp, QERR_SOCKET_CREATE_FAILED);
> + error_set(errp, QERR_INTERNAL_ERROR,
> + "inet_connect: failed to parse address");
> }
> qemu_opts_del(opts);
> return sock;
> diff --git a/qerror.h b/qerror.h
> index d0a76a4..97bf5e0 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -246,4 +246,7 @@ void assert_no_error(Error *err);
> #define QERR_SOCKET_CREATE_FAILED \
> ERROR_CLASS_GENERIC_ERROR, "Failed to create socket"
>
> +#define QERR_INTERNAL_ERROR \
> + ERROR_CLASS_GENERIC_ERROR, "%s"
> +
> #endif /* QERROR_H */
> --
> 1.7.1
>
>
>
>>
>>
>> > 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.
>>
>> [...]
>>
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part
2012-08-29 16:04 ` Amos Kong
@ 2012-09-05 2:19 ` Amos Kong
2012-09-05 18:52 ` Luiz Capitulino
0 siblings, 1 reply; 45+ messages in thread
From: Amos Kong @ 2012-09-05 2:19 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, akong, qemu-devel, Anthony Liguori, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 9401 bytes --]
On Thu, Aug 30, 2012 at 12:04 AM, Amos Kong <kongjianjun@gmail.com> wrote:
> On Wed, Aug 29, 2012 at 11:15 PM, Amos Kong <kongjianjun@gmail.com> wrote:
> > On Thu, Feb 23, 2012 at 4:15 PM, Markus Armbruster <armbru@redhat.com>
> wrote:
> >>
> >> 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 want to continue working on this work (patch 9~13,15~19).
>
FYI, URL of the old thread:
http://lists.nongnu.org/archive/html/qemu-devel/2012-02/msg00714.html
> > Makus used error_report() to output the rich/meaningful error message
> > to monitor,
> > but Anthony prefers to use error_set(), right?
> >
> > I would like to introduce a generic QERR_INTERNAL_ERROR as Anthony
> > said to replace error_report().
>
>
> error_report() can be used many times/places, we might see many error
> in monitor.
> but error_set() can only set one kind of error when internal error
> occurs, and only
> one error will be printed into monitor.
>
> output final/important error by error_set()/error_report_err(), and
> output other error to stdio?
>
> ---
> There are some 'GENERIC ERROR CLASS' in qerror.h, which are not used
> very frequently, we can convert them to 'QERR_INTERNAL_ERROR'.
>
> Or convert all 'GENERIC ERROR CLASS' to 'QERR_INTERNAL_ERROR',
> and use a single human readable string?
>
> > Please help to review below RFC patch, thanks.
>
>
Anthony, Luiz, other
any comments?
archive of this patch:
http://lists.nongnu.org/archive/html/qemu-devel/2012-08/msg04927.html
> From 4b8200ce662dd375819fd24cb932e70131ce0bd3 Mon Sep 17 00:00:00 2001
> > From: Amos Kong <akong@redhat.com>
> > Date: Wed, 29 Aug 2012 22:52:48 +0800
> > Subject: [PATCH RFC] qerror: introduce QERR_INTERNAL_ERROR
> >
> > Current qerror reporting infrastructure isn't agile,
> > we have to add a new Class for a new error.
> >
> > This patch introduced a generic QERR_INTERNAL_ERROR
> > that takes a single human readable string as an argument.
> >
> > This patch is a RFC, so I only changed some code of
> > inet_connect() as an example.
> >
> > hmp:
> > (qemu) migrate -d tcp:noname:4446
> > migrate: Can't resolve noname:4446: Name or service not known
> >
> > qmp:
> > { "execute": "migrate", "arguments": { "uri": "tcp:noname:4446" } }
> > {"error": {"class": "GenericError", "desc": "Can't resolve noname:4446:
> > Name or service not known"}}
> >
> > Signed-off-by: Amos Kong <akong@redhat.com>
> > ---
> > qemu-sockets.c | 9 +++++----
> > qerror.h | 3 +++
> > 2 files changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/qemu-sockets.c b/qemu-sockets.c
> > index 361d890..e528288 100644
> > --- a/qemu-sockets.c
> > +++ b/qemu-sockets.c
> > @@ -244,9 +244,9 @@ int inet_connect_opts(QemuOpts *opts, bool
> > *in_progress, Error **errp)
> >
> > /* lookup */
> > if (0 != (rc = getaddrinfo(addr, port, &ai, &res))) {
> > - fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port,
> > - gai_strerror(rc));
> > - error_set(errp, QERR_SOCKET_CREATE_FAILED);
> > + char err[50];
> > + sprintf(err, "Can't resolve %s:%s: %s", addr, port,
> gai_strerror(rc));
> > + error_set(errp, QERR_INTERNAL_ERROR, err);
> > return -1;
> > }
> >
> > @@ -505,7 +505,8 @@ int inet_connect(const char *str, bool block, bool
> > *in_progress, Error **errp)
> > }
> > sock = inet_connect_opts(opts, in_progress, errp);
> > } else {
> > - error_set(errp, QERR_SOCKET_CREATE_FAILED);
> > + error_set(errp, QERR_INTERNAL_ERROR,
> > + "inet_connect: failed to parse address");
> > }
> > qemu_opts_del(opts);
> > return sock;
> > diff --git a/qerror.h b/qerror.h
> > index d0a76a4..97bf5e0 100644
> > --- a/qerror.h
> > +++ b/qerror.h
> > @@ -246,4 +246,7 @@ void assert_no_error(Error *err);
> > #define QERR_SOCKET_CREATE_FAILED \
> > ERROR_CLASS_GENERIC_ERROR, "Failed to create socket"
> >
> > +#define QERR_INTERNAL_ERROR \
> > + ERROR_CLASS_GENERIC_ERROR, "%s"
> > +
> > #endif /* QERROR_H */
> > --
> > 1.7.1
> >
> >
> >
> >>
> >>
> >> > 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.
> >>
> >> [...]
> >>
>
[-- Attachment #2: Type: text/html, Size: 13610 bytes --]
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part
2012-09-05 2:19 ` Amos Kong
@ 2012-09-05 18:52 ` Luiz Capitulino
0 siblings, 0 replies; 45+ messages in thread
From: Luiz Capitulino @ 2012-09-05 18:52 UTC (permalink / raw)
To: Amos Kong; +Cc: kwolf, akong, Markus Armbruster, Anthony Liguori, qemu-devel
On Wed, 5 Sep 2012 10:19:22 +0800
Amos Kong <kongjianjun@gmail.com> wrote:
> On Thu, Aug 30, 2012 at 12:04 AM, Amos Kong <kongjianjun@gmail.com> wrote:
>
> > On Wed, Aug 29, 2012 at 11:15 PM, Amos Kong <kongjianjun@gmail.com> wrote:
> > > On Thu, Feb 23, 2012 at 4:15 PM, Markus Armbruster <armbru@redhat.com>
> > wrote:
> > >>
> > >> 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 want to continue working on this work (patch 9~13,15~19).
> >
>
>
> FYI, URL of the old thread:
> http://lists.nongnu.org/archive/html/qemu-devel/2012-02/msg00714.html
>
>
>
> > > Makus used error_report() to output the rich/meaningful error message
> > > to monitor,
> > > but Anthony prefers to use error_set(), right?
> > >
> > > I would like to introduce a generic QERR_INTERNAL_ERROR as Anthony
> > > said to replace error_report().
> >
> >
> > error_report() can be used many times/places, we might see many error
> > in monitor.
> > but error_set() can only set one kind of error when internal error
> > occurs, and only
> > one error will be printed into monitor.
> >
> > output final/important error by error_set()/error_report_err(), and
> > output other error to stdio?
> >
> > ---
> > There are some 'GENERIC ERROR CLASS' in qerror.h, which are not used
> > very frequently, we can convert them to 'QERR_INTERNAL_ERROR'.
> >
> > Or convert all 'GENERIC ERROR CLASS' to 'QERR_INTERNAL_ERROR',
> > and use a single human readable string?
> >
> > > Please help to review below RFC patch, thanks.
> >
> >
> Anthony, Luiz, other
>
> any comments?
>
> archive of this patch:
> http://lists.nongnu.org/archive/html/qemu-devel/2012-08/msg04927.html
That thread is too old, that work should be re-done on top of the new
error format.
However, we have bug 603266 for that and it has been assigned to me. And
I have started working on it already...
>
>
> > From 4b8200ce662dd375819fd24cb932e70131ce0bd3 Mon Sep 17 00:00:00 2001
> > > From: Amos Kong <akong@redhat.com>
> > > Date: Wed, 29 Aug 2012 22:52:48 +0800
> > > Subject: [PATCH RFC] qerror: introduce QERR_INTERNAL_ERROR
> > >
> > > Current qerror reporting infrastructure isn't agile,
> > > we have to add a new Class for a new error.
> > >
> > > This patch introduced a generic QERR_INTERNAL_ERROR
> > > that takes a single human readable string as an argument.
> > >
> > > This patch is a RFC, so I only changed some code of
> > > inet_connect() as an example.
> > >
> > > hmp:
> > > (qemu) migrate -d tcp:noname:4446
> > > migrate: Can't resolve noname:4446: Name or service not known
> > >
> > > qmp:
> > > { "execute": "migrate", "arguments": { "uri": "tcp:noname:4446" } }
> > > {"error": {"class": "GenericError", "desc": "Can't resolve noname:4446:
> > > Name or service not known"}}
> > >
> > > Signed-off-by: Amos Kong <akong@redhat.com>
> > > ---
> > > qemu-sockets.c | 9 +++++----
> > > qerror.h | 3 +++
> > > 2 files changed, 8 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/qemu-sockets.c b/qemu-sockets.c
> > > index 361d890..e528288 100644
> > > --- a/qemu-sockets.c
> > > +++ b/qemu-sockets.c
> > > @@ -244,9 +244,9 @@ int inet_connect_opts(QemuOpts *opts, bool
> > > *in_progress, Error **errp)
> > >
> > > /* lookup */
> > > if (0 != (rc = getaddrinfo(addr, port, &ai, &res))) {
> > > - fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port,
> > > - gai_strerror(rc));
> > > - error_set(errp, QERR_SOCKET_CREATE_FAILED);
> > > + char err[50];
> > > + sprintf(err, "Can't resolve %s:%s: %s", addr, port,
> > gai_strerror(rc));
> > > + error_set(errp, QERR_INTERNAL_ERROR, err);
> > > return -1;
> > > }
> > >
> > > @@ -505,7 +505,8 @@ int inet_connect(const char *str, bool block, bool
> > > *in_progress, Error **errp)
> > > }
> > > sock = inet_connect_opts(opts, in_progress, errp);
> > > } else {
> > > - error_set(errp, QERR_SOCKET_CREATE_FAILED);
> > > + error_set(errp, QERR_INTERNAL_ERROR,
> > > + "inet_connect: failed to parse address");
> > > }
> > > qemu_opts_del(opts);
> > > return sock;
> > > diff --git a/qerror.h b/qerror.h
> > > index d0a76a4..97bf5e0 100644
> > > --- a/qerror.h
> > > +++ b/qerror.h
> > > @@ -246,4 +246,7 @@ void assert_no_error(Error *err);
> > > #define QERR_SOCKET_CREATE_FAILED \
> > > ERROR_CLASS_GENERIC_ERROR, "Failed to create socket"
> > >
> > > +#define QERR_INTERNAL_ERROR \
> > > + ERROR_CLASS_GENERIC_ERROR, "%s"
> > > +
> > > #endif /* QERROR_H */
> > > --
> > > 1.7.1
> > >
> > >
> > >
> > >>
> > >>
> > >> > 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.
> > >>
> > >> [...]
> > >>
> >
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Qemu-devel] [PATCH 10/19] qemu-char: Chardev open error reporting, !_WIN32 part
2012-02-07 14:09 [Qemu-devel] [PATCH 00/19] Fix and improve chardev open error messages Markus Armbruster
` (8 preceding siblings ...)
2012-02-07 14:09 ` [Qemu-devel] [PATCH 09/19] sockets: Chardev open error reporting, sockets part Markus Armbruster
@ 2012-02-07 14:09 ` Markus Armbruster
2012-02-07 15:52 ` Kevin Wolf
2012-02-07 14:09 ` [Qemu-devel] [PATCH 11/19] qemu-char: Chardev open error reporting, _WIN32 part Markus Armbruster
` (10 subsequent siblings)
20 siblings, 1 reply; 45+ messages in thread
From: Markus Armbruster @ 2012-02-07 14:09 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, aliguori
This part takes care of backends "file", "pipe", "pty" and "stdio".
Unlike many other backends, these leave open error reporting to their
caller. Because the caller doesn't know what went wrong, this results
in a pretty useless error message.
Change them to report their errors. Improves comically user-hostile
messages like this one for "-chardev file,id=foo,path=/x"
chardev: opening backend "file" failed
to
qemu-system-x86_64: -chardev file,id=foo,path=/x: Can't create file '/x': Permission denied
chardev: opening backend "file" failed
The useless "opening backend failed" message will be cleaned up
shortly.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
qemu-char.c | 19 +++++++++++++++----
1 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/qemu-char.c b/qemu-char.c
index d591f70..47bab4f 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -636,11 +636,18 @@ static CharDriverState *qemu_chr_open_fd(int fd_in, int fd_out)
static CharDriverState *qemu_chr_open_file_out(QemuOpts *opts)
{
+ const char *filename = qemu_opt_get(opts, "path");
int fd_out;
- TFR(fd_out = qemu_open(qemu_opt_get(opts, "path"),
- O_WRONLY | O_TRUNC | O_CREAT | O_BINARY, 0666));
+ if (!filename) {
+ error_report("file character device requires parameter path");
+ return NULL;
+ }
+
+ TFR(fd_out = qemu_open(filename,
+ O_WRONLY | O_TRUNC | O_CREAT | O_BINARY, 0666));
if (fd_out < 0) {
+ error_report("Can't create file '%s': %s", filename, strerror(errno));
return NULL;
}
return qemu_chr_open_fd(-1, fd_out);
@@ -653,7 +660,7 @@ static CharDriverState *qemu_chr_open_pipe(QemuOpts *opts)
const char *filename = qemu_opt_get(opts, "path");
if (filename == NULL) {
- fprintf(stderr, "chardev: pipe: no filename given\n");
+ error_report("pipe character device requires parameter path");
return NULL;
}
@@ -668,6 +675,8 @@ static CharDriverState *qemu_chr_open_pipe(QemuOpts *opts)
close(fd_out);
TFR(fd_in = fd_out = qemu_open(filename, O_RDWR | O_BINARY));
if (fd_in < 0) {
+ error_report("Can't create pipe '%s': %s",
+ filename, strerror(errno));
return NULL;
}
}
@@ -767,6 +776,7 @@ static CharDriverState *qemu_chr_open_stdio(QemuOpts *opts)
CharDriverState *chr;
if (stdio_nb_clients >= STDIO_MAX_CLIENTS) {
+ error_report("Too many stdio devices");
return NULL;
}
if (stdio_nb_clients == 0) {
@@ -987,6 +997,7 @@ static CharDriverState *qemu_chr_open_pty(QemuOpts *opts)
#endif
if (openpty(&master_fd, &slave_fd, pty_name, NULL, NULL) < 0) {
+ error_report("Can't open pty: %s", strerror(errno));
return NULL;
}
@@ -1002,7 +1013,7 @@ static CharDriverState *qemu_chr_open_pty(QemuOpts *opts)
chr->filename = g_malloc(len);
snprintf(chr->filename, len, "pty:%s", q_ptsname(master_fd));
qemu_opt_set(opts, "path", q_ptsname(master_fd));
- fprintf(stderr, "char device redirected to %s\n", q_ptsname(master_fd));
+ error_printf("char device redirected to %s\n", q_ptsname(master_fd));
s = g_malloc0(sizeof(PtyCharDriver));
chr->opaque = s;
--
1.7.6.5
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [Qemu-devel] [PATCH 10/19] qemu-char: Chardev open error reporting, !_WIN32 part
2012-02-07 14:09 ` [Qemu-devel] [PATCH 10/19] qemu-char: Chardev open error reporting, !_WIN32 part Markus Armbruster
@ 2012-02-07 15:52 ` Kevin Wolf
2012-02-09 15:16 ` Markus Armbruster
0 siblings, 1 reply; 45+ messages in thread
From: Kevin Wolf @ 2012-02-07 15:52 UTC (permalink / raw)
To: Markus Armbruster; +Cc: aliguori, qemu-devel
Am 07.02.2012 15:09, schrieb Markus Armbruster:
> This part takes care of backends "file", "pipe", "pty" and "stdio".
> Unlike many other backends, these leave open error reporting to their
> caller. Because the caller doesn't know what went wrong, this results
> in a pretty useless error message.
>
> Change them to report their errors. Improves comically user-hostile
> messages like this one for "-chardev file,id=foo,path=/x"
>
> chardev: opening backend "file" failed
>
> to
>
> qemu-system-x86_64: -chardev file,id=foo,path=/x: Can't create file '/x': Permission denied
> chardev: opening backend "file" failed
>
> The useless "opening backend failed" message will be cleaned up
> shortly.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> qemu-char.c | 19 +++++++++++++++----
> 1 files changed, 15 insertions(+), 4 deletions(-)
> @@ -1002,7 +1013,7 @@ static CharDriverState *qemu_chr_open_pty(QemuOpts *opts)
> chr->filename = g_malloc(len);
> snprintf(chr->filename, len, "pty:%s", q_ptsname(master_fd));
> qemu_opt_set(opts, "path", q_ptsname(master_fd));
> - fprintf(stderr, "char device redirected to %s\n", q_ptsname(master_fd));
> + error_printf("char device redirected to %s\n", q_ptsname(master_fd));
>
> s = g_malloc0(sizeof(PtyCharDriver));
> chr->opaque = s;
Not really an error message. Does it make any sense at all to have this
message?
Kevin
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [Qemu-devel] [PATCH 10/19] qemu-char: Chardev open error reporting, !_WIN32 part
2012-02-07 15:52 ` Kevin Wolf
@ 2012-02-09 15:16 ` Markus Armbruster
2012-02-09 15:39 ` Kevin Wolf
0 siblings, 1 reply; 45+ messages in thread
From: Markus Armbruster @ 2012-02-09 15:16 UTC (permalink / raw)
To: Kevin Wolf; +Cc: aliguori, qemu-devel
Kevin Wolf <kwolf@redhat.com> writes:
> Am 07.02.2012 15:09, schrieb Markus Armbruster:
>> This part takes care of backends "file", "pipe", "pty" and "stdio".
>> Unlike many other backends, these leave open error reporting to their
>> caller. Because the caller doesn't know what went wrong, this results
>> in a pretty useless error message.
>>
>> Change them to report their errors. Improves comically user-hostile
>> messages like this one for "-chardev file,id=foo,path=/x"
>>
>> chardev: opening backend "file" failed
>>
>> to
>>
>> qemu-system-x86_64: -chardev file,id=foo,path=/x: Can't create file '/x': Permission denied
>> chardev: opening backend "file" failed
>>
>> The useless "opening backend failed" message will be cleaned up
>> shortly.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> qemu-char.c | 19 +++++++++++++++----
>> 1 files changed, 15 insertions(+), 4 deletions(-)
>
>> @@ -1002,7 +1013,7 @@ static CharDriverState *qemu_chr_open_pty(QemuOpts *opts)
>> chr->filename = g_malloc(len);
>> snprintf(chr->filename, len, "pty:%s", q_ptsname(master_fd));
>> qemu_opt_set(opts, "path", q_ptsname(master_fd));
>> - fprintf(stderr, "char device redirected to %s\n", q_ptsname(master_fd));
>> + error_printf("char device redirected to %s\n", q_ptsname(master_fd));
>>
>> s = g_malloc0(sizeof(PtyCharDriver));
>> chr->opaque = s;
>
> Not really an error message. Does it make any sense at all to have this
> message?
error_printf() prints to the error channel (monitor or stderr), but not
necessarily an error message. See for instance drive_init()'s use of it
to print format help.
Not sure whether it makes sense to have this message. I guess it exists
because the pty is chosen automatically, but the user might still want
to know which one was chosen.
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [Qemu-devel] [PATCH 10/19] qemu-char: Chardev open error reporting, !_WIN32 part
2012-02-09 15:16 ` Markus Armbruster
@ 2012-02-09 15:39 ` Kevin Wolf
2012-02-09 16:19 ` Markus Armbruster
0 siblings, 1 reply; 45+ messages in thread
From: Kevin Wolf @ 2012-02-09 15:39 UTC (permalink / raw)
To: Markus Armbruster; +Cc: aliguori, qemu-devel, Luiz Capitulino
Am 09.02.2012 16:16, schrieb Markus Armbruster:
> Kevin Wolf <kwolf@redhat.com> writes:
>
>> Am 07.02.2012 15:09, schrieb Markus Armbruster:
>>> This part takes care of backends "file", "pipe", "pty" and "stdio".
>>> Unlike many other backends, these leave open error reporting to their
>>> caller. Because the caller doesn't know what went wrong, this results
>>> in a pretty useless error message.
>>>
>>> Change them to report their errors. Improves comically user-hostile
>>> messages like this one for "-chardev file,id=foo,path=/x"
>>>
>>> chardev: opening backend "file" failed
>>>
>>> to
>>>
>>> qemu-system-x86_64: -chardev file,id=foo,path=/x: Can't create file '/x': Permission denied
>>> chardev: opening backend "file" failed
>>>
>>> The useless "opening backend failed" message will be cleaned up
>>> shortly.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>> qemu-char.c | 19 +++++++++++++++----
>>> 1 files changed, 15 insertions(+), 4 deletions(-)
>>
>>> @@ -1002,7 +1013,7 @@ static CharDriverState *qemu_chr_open_pty(QemuOpts *opts)
>>> chr->filename = g_malloc(len);
>>> snprintf(chr->filename, len, "pty:%s", q_ptsname(master_fd));
>>> qemu_opt_set(opts, "path", q_ptsname(master_fd));
>>> - fprintf(stderr, "char device redirected to %s\n", q_ptsname(master_fd));
>>> + error_printf("char device redirected to %s\n", q_ptsname(master_fd));
>>>
>>> s = g_malloc0(sizeof(PtyCharDriver));
>>> chr->opaque = s;
>>
>> Not really an error message. Does it make any sense at all to have this
>> message?
>
> error_printf() prints to the error channel (monitor or stderr), but not
> necessarily an error message. See for instance drive_init()'s use of it
> to print format help.
Ah, right. I confused it with error_report() which includes an error
location. That would be rather unexpected.
> Not sure whether it makes sense to have this message. I guess it exists
> because the pty is chosen automatically, but the user might still want
> to know which one was chosen.
Does "the user" include management tools?
If we had a chardev_add monitor command, its output would be moved from
stderr to the monitor. We don't have that, but there might be commands
that create chardevs internally: gdbserver is one, not sure if I missed
others.
We probably don't care much about breaking tools that use 'gdbserver
pty' and read the device from stderr. (But the information is missing
from QMP - should it be added?)
Kevin
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [Qemu-devel] [PATCH 10/19] qemu-char: Chardev open error reporting, !_WIN32 part
2012-02-09 15:39 ` Kevin Wolf
@ 2012-02-09 16:19 ` Markus Armbruster
2012-02-09 16:31 ` Luiz Capitulino
0 siblings, 1 reply; 45+ messages in thread
From: Markus Armbruster @ 2012-02-09 16:19 UTC (permalink / raw)
To: Kevin Wolf; +Cc: aliguori, qemu-devel, Luiz Capitulino
Kevin Wolf <kwolf@redhat.com> writes:
> Am 09.02.2012 16:16, schrieb Markus Armbruster:
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>>> Am 07.02.2012 15:09, schrieb Markus Armbruster:
>>>> This part takes care of backends "file", "pipe", "pty" and "stdio".
>>>> Unlike many other backends, these leave open error reporting to their
>>>> caller. Because the caller doesn't know what went wrong, this results
>>>> in a pretty useless error message.
>>>>
>>>> Change them to report their errors. Improves comically user-hostile
>>>> messages like this one for "-chardev file,id=foo,path=/x"
>>>>
>>>> chardev: opening backend "file" failed
>>>>
>>>> to
>>>>
>>>> qemu-system-x86_64: -chardev file,id=foo,path=/x: Can't create file '/x': Permission denied
>>>> chardev: opening backend "file" failed
>>>>
>>>> The useless "opening backend failed" message will be cleaned up
>>>> shortly.
>>>>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>> ---
>>>> qemu-char.c | 19 +++++++++++++++----
>>>> 1 files changed, 15 insertions(+), 4 deletions(-)
>>>
>>>> @@ -1002,7 +1013,7 @@ static CharDriverState *qemu_chr_open_pty(QemuOpts *opts)
>>>> chr->filename = g_malloc(len);
>>>> snprintf(chr->filename, len, "pty:%s", q_ptsname(master_fd));
>>>> qemu_opt_set(opts, "path", q_ptsname(master_fd));
>>>> - fprintf(stderr, "char device redirected to %s\n", q_ptsname(master_fd));
>>>> + error_printf("char device redirected to %s\n", q_ptsname(master_fd));
>>>>
>>>> s = g_malloc0(sizeof(PtyCharDriver));
>>>> chr->opaque = s;
>>>
>>> Not really an error message. Does it make any sense at all to have this
>>> message?
>>
>> error_printf() prints to the error channel (monitor or stderr), but not
>> necessarily an error message. See for instance drive_init()'s use of it
>> to print format help.
>
> Ah, right. I confused it with error_report() which includes an error
> location. That would be rather unexpected.
Indeed.
>> Not sure whether it makes sense to have this message. I guess it exists
>> because the pty is chosen automatically, but the user might still want
>> to know which one was chosen.
>
> Does "the user" include management tools?
>
> If we had a chardev_add monitor command, its output would be moved from
> stderr to the monitor. We don't have that,
Yet! One of the reasons for doing this series was preparing the ground
for chardev_add.
> but there might be commands
> that create chardevs internally: gdbserver is one, not sure if I missed
> others.
>
> We probably don't care much about breaking tools that use 'gdbserver
> pty' and read the device from stderr.
And do so using a monitor chardev other than stdio. That would be
weird, wouldn't it?
> (But the information is missing
> from QMP - should it be added?)
Right when somebody asks for it.
For what it's worth, some chardev backend open() methods return such
information via their opts argument. E.g. inet_listen_opts() receives a
port range in opts (options "port" and "to"), and overwrites option
"port" with the port actually chosen. I hate that, should use separate
options.
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [Qemu-devel] [PATCH 10/19] qemu-char: Chardev open error reporting, !_WIN32 part
2012-02-09 16:19 ` Markus Armbruster
@ 2012-02-09 16:31 ` Luiz Capitulino
2012-02-09 17:08 ` Markus Armbruster
0 siblings, 1 reply; 45+ messages in thread
From: Luiz Capitulino @ 2012-02-09 16:31 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Kevin Wolf, aliguori, qemu-devel
On Thu, 09 Feb 2012 17:19:00 +0100
Markus Armbruster <armbru@redhat.com> wrote:
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > Am 09.02.2012 16:16, schrieb Markus Armbruster:
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >>
> >>> Am 07.02.2012 15:09, schrieb Markus Armbruster:
> >>>> This part takes care of backends "file", "pipe", "pty" and "stdio".
> >>>> Unlike many other backends, these leave open error reporting to their
> >>>> caller. Because the caller doesn't know what went wrong, this results
> >>>> in a pretty useless error message.
> >>>>
> >>>> Change them to report their errors. Improves comically user-hostile
> >>>> messages like this one for "-chardev file,id=foo,path=/x"
> >>>>
> >>>> chardev: opening backend "file" failed
> >>>>
> >>>> to
> >>>>
> >>>> qemu-system-x86_64: -chardev file,id=foo,path=/x: Can't create file '/x': Permission denied
> >>>> chardev: opening backend "file" failed
> >>>>
> >>>> The useless "opening backend failed" message will be cleaned up
> >>>> shortly.
> >>>>
> >>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >>>> ---
> >>>> qemu-char.c | 19 +++++++++++++++----
> >>>> 1 files changed, 15 insertions(+), 4 deletions(-)
> >>>
> >>>> @@ -1002,7 +1013,7 @@ static CharDriverState *qemu_chr_open_pty(QemuOpts *opts)
> >>>> chr->filename = g_malloc(len);
> >>>> snprintf(chr->filename, len, "pty:%s", q_ptsname(master_fd));
> >>>> qemu_opt_set(opts, "path", q_ptsname(master_fd));
> >>>> - fprintf(stderr, "char device redirected to %s\n", q_ptsname(master_fd));
> >>>> + error_printf("char device redirected to %s\n", q_ptsname(master_fd));
> >>>>
> >>>> s = g_malloc0(sizeof(PtyCharDriver));
> >>>> chr->opaque = s;
> >>>
> >>> Not really an error message. Does it make any sense at all to have this
> >>> message?
> >>
> >> error_printf() prints to the error channel (monitor or stderr), but not
> >> necessarily an error message. See for instance drive_init()'s use of it
> >> to print format help.
> >
> > Ah, right. I confused it with error_report() which includes an error
> > location. That would be rather unexpected.
>
> Indeed.
>
> >> Not sure whether it makes sense to have this message. I guess it exists
> >> because the pty is chosen automatically, but the user might still want
> >> to know which one was chosen.
> >
> > Does "the user" include management tools?
> >
> > If we had a chardev_add monitor command, its output would be moved from
> > stderr to the monitor. We don't have that,
>
> Yet! One of the reasons for doing this series was preparing the ground
> for chardev_add.
I haven't taken a look in detail in this series, but if your end goal is to
add chardev_add, then you should probably be using error_set() all around.
> > but there might be commands
> > that create chardevs internally: gdbserver is one, not sure if I missed
> > others.
> >
> > We probably don't care much about breaking tools that use 'gdbserver
> > pty' and read the device from stderr.
>
> And do so using a monitor chardev other than stdio. That would be
> weird, wouldn't it?
>
> > (But the information is missing
> > from QMP - should it be added?)
>
> Right when somebody asks for it.
>
>
> For what it's worth, some chardev backend open() methods return such
> information via their opts argument. E.g. inet_listen_opts() receives a
> port range in opts (options "port" and "to"), and overwrites option
> "port" with the port actually chosen. I hate that, should use separate
> options.
>
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [Qemu-devel] [PATCH 10/19] qemu-char: Chardev open error reporting, !_WIN32 part
2012-02-09 16:31 ` Luiz Capitulino
@ 2012-02-09 17:08 ` Markus Armbruster
0 siblings, 0 replies; 45+ messages in thread
From: Markus Armbruster @ 2012-02-09 17:08 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: Kevin Wolf, aliguori, qemu-devel
Luiz Capitulino <lcapitulino@redhat.com> writes:
> On Thu, 09 Feb 2012 17:19:00 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>> > Am 09.02.2012 16:16, schrieb Markus Armbruster:
>> >> Kevin Wolf <kwolf@redhat.com> writes:
>> >>
>> >>> Am 07.02.2012 15:09, schrieb Markus Armbruster:
>> >>>> This part takes care of backends "file", "pipe", "pty" and "stdio".
>> >>>> Unlike many other backends, these leave open error reporting to their
>> >>>> caller. Because the caller doesn't know what went wrong, this results
>> >>>> in a pretty useless error message.
>> >>>>
>> >>>> Change them to report their errors. Improves comically user-hostile
>> >>>> messages like this one for "-chardev file,id=foo,path=/x"
>> >>>>
>> >>>> chardev: opening backend "file" failed
>> >>>>
>> >>>> to
>> >>>>
>> >>>> qemu-system-x86_64: -chardev file,id=foo,path=/x: Can't create file '/x': Permission denied
>> >>>> chardev: opening backend "file" failed
>> >>>>
>> >>>> The useless "opening backend failed" message will be cleaned up
>> >>>> shortly.
>> >>>>
>> >>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >>>> ---
>> >>>> qemu-char.c | 19 +++++++++++++++----
>> >>>> 1 files changed, 15 insertions(+), 4 deletions(-)
>> >>>
>> >>>> @@ -1002,7 +1013,7 @@ static CharDriverState *qemu_chr_open_pty(QemuOpts *opts)
>> >>>> chr->filename = g_malloc(len);
>> >>>> snprintf(chr->filename, len, "pty:%s", q_ptsname(master_fd));
>> >>>> qemu_opt_set(opts, "path", q_ptsname(master_fd));
>> >>>> - fprintf(stderr, "char device redirected to %s\n", q_ptsname(master_fd));
>> >>>> + error_printf("char device redirected to %s\n", q_ptsname(master_fd));
>> >>>>
>> >>>> s = g_malloc0(sizeof(PtyCharDriver));
>> >>>> chr->opaque = s;
>> >>>
>> >>> Not really an error message. Does it make any sense at all to have this
>> >>> message?
>> >>
>> >> error_printf() prints to the error channel (monitor or stderr), but not
>> >> necessarily an error message. See for instance drive_init()'s use of it
>> >> to print format help.
>> >
>> > Ah, right. I confused it with error_report() which includes an error
>> > location. That would be rather unexpected.
>>
>> Indeed.
>>
>> >> Not sure whether it makes sense to have this message. I guess it exists
>> >> because the pty is chosen automatically, but the user might still want
>> >> to know which one was chosen.
>> >
>> > Does "the user" include management tools?
>> >
>> > If we had a chardev_add monitor command, its output would be moved from
>> > stderr to the monitor. We don't have that,
>>
>> Yet! One of the reasons for doing this series was preparing the ground
>> for chardev_add.
>
> I haven't taken a look in detail in this series, but if your end goal is to
> add chardev_add, then you should probably be using error_set() all around.
The goal is to improve the atrocious error reporting, no more, no less.
Nice side effect: it gets us one little step closer to chardev_add.
Converting to error_set() will be much easier after this series than
before.
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Qemu-devel] [PATCH 11/19] qemu-char: Chardev open error reporting, _WIN32 part
2012-02-07 14:09 [Qemu-devel] [PATCH 00/19] Fix and improve chardev open error messages Markus Armbruster
` (9 preceding siblings ...)
2012-02-07 14:09 ` [Qemu-devel] [PATCH 10/19] qemu-char: Chardev open error reporting, !_WIN32 part Markus Armbruster
@ 2012-02-07 14:09 ` Markus Armbruster
2012-02-07 14:09 ` [Qemu-devel] [PATCH 12/19] qemu-char: Chardev open error reporting, tty part Markus Armbruster
` (9 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Markus Armbruster @ 2012-02-07 14:09 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, aliguori
Convert backends "pipe", "console" and "serial" to error_report().
While there, improve the atrocious error messages some.
Unlike many other backends, "file" and "stdio" leave open error
reporting to their caller. Because the caller doesn't know what went
wrong, this results in a pretty useless error message. Change them to
report their errors.
Many error paths smell leaky, but I'm limiting myself strictly to
error reporting here.
Compile-tested only.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
qemu-char.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++------------
1 files changed, 51 insertions(+), 14 deletions(-)
diff --git a/qemu-char.c b/qemu-char.c
index 47bab4f..130ed8b 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1476,6 +1476,15 @@ typedef struct {
static int win_chr_poll(void *opaque);
static int win_chr_pipe_poll(void *opaque);
+static char *strerr_win(DWORD err)
+{
+ char **p;
+
+ FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_ALLOCATE_BUFFER,
+ NULL, err, 0, (LPTSTR)&p, 0, NULL);
+ return *p;
+}
+
static void win_chr_close(CharDriverState *chr)
{
WinCharState *s = chr->opaque;
@@ -1508,28 +1517,29 @@ static int win_chr_init(CharDriverState *chr, const char *filename)
COMSTAT comstat;
DWORD size;
DWORD err;
+ char *errstr;
s->hsend = CreateEvent(NULL, TRUE, FALSE, NULL);
if (!s->hsend) {
- fprintf(stderr, "Failed CreateEvent\n");
+ err = GetLastError();
goto fail;
}
s->hrecv = CreateEvent(NULL, TRUE, FALSE, NULL);
if (!s->hrecv) {
- fprintf(stderr, "Failed CreateEvent\n");
+ err = GetLastError();
goto fail;
}
s->hcom = CreateFile(filename, GENERIC_READ|GENERIC_WRITE, 0, NULL,
OPEN_EXISTING, FILE_FLAG_OVERLAPPED, 0);
if (s->hcom == INVALID_HANDLE_VALUE) {
- fprintf(stderr, "Failed CreateFile (%lu)\n", GetLastError());
+ err = GetLastError();
s->hcom = NULL;
goto fail;
}
if (!SetupComm(s->hcom, NRECVBUF, NSENDBUF)) {
- fprintf(stderr, "Failed SetupComm\n");
+ err = GetLastError();
goto fail;
}
@@ -1540,29 +1550,32 @@ static int win_chr_init(CharDriverState *chr, const char *filename)
CommConfigDialog(filename, NULL, &comcfg);
if (!SetCommState(s->hcom, &comcfg.dcb)) {
- fprintf(stderr, "Failed SetCommState\n");
+ err = GetLastError();
goto fail;
}
if (!SetCommMask(s->hcom, EV_ERR)) {
- fprintf(stderr, "Failed SetCommMask\n");
+ err = GetLastError();
goto fail;
}
cto.ReadIntervalTimeout = MAXDWORD;
if (!SetCommTimeouts(s->hcom, &cto)) {
- fprintf(stderr, "Failed SetCommTimeouts\n");
+ err = GetLastError();
goto fail;
}
if (!ClearCommError(s->hcom, &err, &comstat)) {
- fprintf(stderr, "Failed ClearCommError\n");
+ err = GetLastError();
goto fail;
}
qemu_add_polling_cb(win_chr_poll, chr);
return 0;
fail:
+ errstr = strerr_win(err);
+ error_report("Can't open serial device '%s': %s", filename, errstr);
+ LocalFree(errstr);
win_chr_close(chr);
return -1;
}
@@ -1666,6 +1679,11 @@ static CharDriverState *qemu_chr_open_win(QemuOpts *opts)
CharDriverState *chr;
WinCharState *s;
+ if (!filename) {
+ error_report("serial character device requires parameter path");
+ return NULL;
+ }
+
chr = g_malloc0(sizeof(CharDriverState));
s = g_malloc0(sizeof(WinCharState));
chr->opaque = s;
@@ -1702,19 +1720,20 @@ static int win_chr_pipe_init(CharDriverState *chr, const char *filename)
WinCharState *s = chr->opaque;
OVERLAPPED ov;
int ret;
- DWORD size;
+ DWORD size, err;
+ char *errstr;
char openname[256];
s->fpipe = TRUE;
s->hsend = CreateEvent(NULL, TRUE, FALSE, NULL);
if (!s->hsend) {
- fprintf(stderr, "Failed CreateEvent\n");
+ err = GetLastError();
goto fail;
}
s->hrecv = CreateEvent(NULL, TRUE, FALSE, NULL);
if (!s->hrecv) {
- fprintf(stderr, "Failed CreateEvent\n");
+ err = GetLastError();
goto fail;
}
@@ -1724,7 +1743,7 @@ static int win_chr_pipe_init(CharDriverState *chr, const char *filename)
PIPE_WAIT,
MAXCONNECT, NSENDBUF, NRECVBUF, NTIMEOUT, NULL);
if (s->hcom == INVALID_HANDLE_VALUE) {
- fprintf(stderr, "Failed CreateNamedPipe (%lu)\n", GetLastError());
+ err = GetLastError();
s->hcom = NULL;
goto fail;
}
@@ -1733,13 +1752,13 @@ static int win_chr_pipe_init(CharDriverState *chr, const char *filename)
ov.hEvent = CreateEvent(NULL, TRUE, FALSE, NULL);
ret = ConnectNamedPipe(s->hcom, &ov);
if (ret) {
- fprintf(stderr, "Failed ConnectNamedPipe\n");
+ err = GetLastError();
goto fail;
}
ret = GetOverlappedResult(s->hcom, &ov, &size, TRUE);
if (!ret) {
- fprintf(stderr, "Failed GetOverlappedResult\n");
+ err = GetLastError();
if (ov.hEvent) {
CloseHandle(ov.hEvent);
ov.hEvent = NULL;
@@ -1755,6 +1774,9 @@ static int win_chr_pipe_init(CharDriverState *chr, const char *filename)
return 0;
fail:
+ errstr = strerr_win(err);
+ error_report("Can't create pipe '%s': %s", filename, errstr);
+ LocalFree(errstr);
win_chr_close(chr);
return -1;
}
@@ -1766,6 +1788,11 @@ static CharDriverState *qemu_chr_open_win_pipe(QemuOpts *opts)
CharDriverState *chr;
WinCharState *s;
+ if (!filename) {
+ error_report("pipe character device requires parameter path");
+ return NULL;
+ }
+
chr = g_malloc0(sizeof(CharDriverState));
s = g_malloc0(sizeof(WinCharState));
chr->opaque = s;
@@ -1804,10 +1831,19 @@ static CharDriverState *qemu_chr_open_win_file_out(QemuOpts *opts)
{
const char *file_out = qemu_opt_get(opts, "path");
HANDLE fd_out;
+ char *errstr;
+
+ if (!file_out) {
+ error_report("file character device requires parameter path");
+ return NULL;
+ }
fd_out = CreateFile(file_out, GENERIC_WRITE, FILE_SHARE_READ, NULL,
OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
if (fd_out == INVALID_HANDLE_VALUE) {
+ errstr = strerr_win(GetLastError());
+ error_report("Can't create file '%s': %s", file_out, errstr);
+ LocalFree(errstr);
return NULL;
}
@@ -1961,6 +1997,7 @@ static CharDriverState *qemu_chr_open_win_stdio(QemuOpts *opts)
if (stdio_nb_clients >= STDIO_MAX_CLIENTS
|| ((display_type != DT_NOGRAPHIC) && (stdio_nb_clients != 0))) {
+ error_report("Too many stdio devices");
return NULL;
}
--
1.7.6.5
^ permalink raw reply related [flat|nested] 45+ messages in thread* [Qemu-devel] [PATCH 12/19] qemu-char: Chardev open error reporting, tty part
2012-02-07 14:09 [Qemu-devel] [PATCH 00/19] Fix and improve chardev open error messages Markus Armbruster
` (10 preceding siblings ...)
2012-02-07 14:09 ` [Qemu-devel] [PATCH 11/19] qemu-char: Chardev open error reporting, _WIN32 part Markus Armbruster
@ 2012-02-07 14:09 ` Markus Armbruster
2012-02-07 14:09 ` [Qemu-devel] [PATCH 13/19] qemu-char: Chardev open error reporting, parport part Markus Armbruster
` (8 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Markus Armbruster @ 2012-02-07 14:09 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, aliguori
Unlike many other backends, this one leaves open error reporting to
its caller. Because the caller doesn't know what went wrong, this
results in a pretty useless error message.
Change it to report its errors. Improves user-hostile messages like
this one for "-chardev tty,id=foo,path=/dev/ttyy1"
chardev: opening backend "file" failed
to
qemu-system-x86_64: -chardev tty,id=foo,path=/dev/ttyy1: Can't open '/dev/ttyy1': No such file or directory
chardev: opening backend "file" failed
The useless "opening backend failed" message will be cleaned up
shortly.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
qemu-char.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/qemu-char.c b/qemu-char.c
index 130ed8b..1ff7f4b 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1231,8 +1231,14 @@ static CharDriverState *qemu_chr_open_tty(QemuOpts *opts)
CharDriverState *chr;
int fd;
+ if (!filename) {
+ error_report("tty character device requires parameter path");
+ return NULL;
+ }
+
TFR(fd = qemu_open(filename, O_RDWR | O_NONBLOCK));
if (fd < 0) {
+ error_report("Can't open '%s': %s", filename, strerror(errno));
return NULL;
}
tty_serial_init(fd, 115200, 'N', 8, 1);
--
1.7.6.5
^ permalink raw reply related [flat|nested] 45+ messages in thread* [Qemu-devel] [PATCH 13/19] qemu-char: Chardev open error reporting, parport part
2012-02-07 14:09 [Qemu-devel] [PATCH 00/19] Fix and improve chardev open error messages Markus Armbruster
` (11 preceding siblings ...)
2012-02-07 14:09 ` [Qemu-devel] [PATCH 12/19] qemu-char: Chardev open error reporting, tty part Markus Armbruster
@ 2012-02-07 14:09 ` Markus Armbruster
2012-02-07 14:09 ` [Qemu-devel] [PATCH 14/19] console: Eliminate text_consoles[] Markus Armbruster
` (7 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Markus Armbruster @ 2012-02-07 14:09 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, aliguori
Unlike many other backends, these leave open error reporting to their
caller. Because the caller doesn't know what went wrong, this results
in a pretty useless error message.
Change them to report their errors themselves. Improves user-hostile
messages like this one for "-chardev tty,id=foo,path=/dev/parport0"
chardev: opening backend "parport" failed
to
qemu-system-x86_64: -chardev parport,id=foo,path=/dev/parport0: Can't open '/dev/parport0': Permission denied
chardev: opening backend "parport" failed
The useless "opening backend failed" message will be cleaned up
shortly.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
qemu-char.c | 21 ++++++++++++++++++---
1 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/qemu-char.c b/qemu-char.c
index 1ff7f4b..ecbb595 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1371,14 +1371,18 @@ static CharDriverState *qemu_chr_open_pp(QemuOpts *opts)
ParallelCharDriver *drv;
int fd;
+ if (!filename) {
+ error_report("parport character device requires parameter path");
+ return NULL;
+ }
+
TFR(fd = qemu_open(filename, O_RDWR));
if (fd < 0) {
- return NULL;
+ goto err;
}
if (ioctl(fd, PPCLAIM) < 0) {
- close(fd);
- return NULL;
+ goto err;
}
drv = g_malloc0(sizeof(ParallelCharDriver));
@@ -1394,6 +1398,11 @@ static CharDriverState *qemu_chr_open_pp(QemuOpts *opts)
qemu_chr_generic_open(chr);
return chr;
+
+err:
+ error_report("Can't open '%s': %s", filename, strerror(errno));
+ close(fd);
+ return NULL;
}
#endif /* __linux__ */
@@ -1441,8 +1450,14 @@ static CharDriverState *qemu_chr_open_pp(QemuOpts *opts)
CharDriverState *chr;
int fd;
+ if (!filename) {
+ error_report("parport character device requires parameter path");
+ return NULL;
+ }
+
fd = qemu_open(filename, O_RDWR);
if (fd < 0) {
+ error_report("Can't open '%s': %s", filename, strerror(errno));
return NULL;
}
--
1.7.6.5
^ permalink raw reply related [flat|nested] 45+ messages in thread* [Qemu-devel] [PATCH 14/19] console: Eliminate text_consoles[]
2012-02-07 14:09 [Qemu-devel] [PATCH 00/19] Fix and improve chardev open error messages Markus Armbruster
` (12 preceding siblings ...)
2012-02-07 14:09 ` [Qemu-devel] [PATCH 13/19] qemu-char: Chardev open error reporting, parport part Markus Armbruster
@ 2012-02-07 14:09 ` Markus Armbruster
2012-02-07 14:09 ` [Qemu-devel] [PATCH 15/19] console: Chardev open error reporting, console part Markus Armbruster
` (6 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Markus Armbruster @ 2012-02-07 14:09 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, aliguori
Simply use consoles[] instead.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
console.c | 18 ++++--------------
1 files changed, 4 insertions(+), 14 deletions(-)
diff --git a/console.c b/console.c
index 2c432e3..744ef2d 100644
--- a/console.c
+++ b/console.c
@@ -1441,9 +1441,6 @@ void console_color_init(DisplayState *ds)
}
}
-static int n_text_consoles;
-static CharDriverState *text_consoles[128];
-
static void text_console_set_echo(CharDriverState *chr, bool echo)
{
TextConsole *s = chr->opaque;
@@ -1519,13 +1516,6 @@ CharDriverState *text_console_init(QemuOpts *opts)
chr = g_malloc0(sizeof(CharDriverState));
- if (n_text_consoles == 128) {
- fprintf(stderr, "Too many text consoles\n");
- exit(1);
- }
- text_consoles[n_text_consoles] = chr;
- n_text_consoles++;
-
width = qemu_opt_get_number(opts, "width", 0);
if (width == 0)
width = qemu_opt_get_number(opts, "cols", 0) * FONT_WIDTH;
@@ -1557,11 +1547,11 @@ void text_consoles_set_display(DisplayState *ds)
{
int i;
- for (i = 0; i < n_text_consoles; i++) {
- text_console_do_init(text_consoles[i], ds);
+ for (i = 0; i < nb_consoles; i++) {
+ if (consoles[i]->console_type != GRAPHIC_CONSOLE) {
+ text_console_do_init(consoles[i]->chr, ds);
+ }
}
-
- n_text_consoles = 0;
}
void qemu_console_resize(DisplayState *ds, int width, int height)
--
1.7.6.5
^ permalink raw reply related [flat|nested] 45+ messages in thread* [Qemu-devel] [PATCH 15/19] console: Chardev open error reporting, console part
2012-02-07 14:09 [Qemu-devel] [PATCH 00/19] Fix and improve chardev open error messages Markus Armbruster
` (13 preceding siblings ...)
2012-02-07 14:09 ` [Qemu-devel] [PATCH 14/19] console: Eliminate text_consoles[] Markus Armbruster
@ 2012-02-07 14:09 ` Markus Armbruster
2012-02-07 14:09 ` [Qemu-devel] [PATCH 16/19] spice-qemu-char: Chardev open error reporting, spicevmc part Markus Armbruster
` (5 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Markus Armbruster @ 2012-02-07 14:09 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, aliguori
Unlike many other backends, this one leaves open error reporting to
its caller. Because the caller doesn't know what went wrong, this
results in a pretty useless error message.
Change it to report its errors. Improves the message you get when
attempting to create too many consoles from
chardev: opening backend "vc" failed
to
qemu-system-x86_64: -chardev vc,id=c13: Can't create more than 12 consoles
chardev: opening backend "vc" failed
The useless "opening backend failed" message will be cleaned up
shortly.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
console.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/console.c b/console.c
index 744ef2d..f6b93bd 100644
--- a/console.c
+++ b/console.c
@@ -1529,8 +1529,8 @@ CharDriverState *text_console_init(QemuOpts *opts)
} else {
s = new_console(NULL, TEXT_CONSOLE_FIXED_SIZE);
}
-
if (!s) {
+ error_report("Can't create more than %d consoles", MAX_CONSOLES);
g_free(chr);
return NULL;
}
--
1.7.6.5
^ permalink raw reply related [flat|nested] 45+ messages in thread* [Qemu-devel] [PATCH 16/19] spice-qemu-char: Chardev open error reporting, spicevmc part
2012-02-07 14:09 [Qemu-devel] [PATCH 00/19] Fix and improve chardev open error messages Markus Armbruster
` (14 preceding siblings ...)
2012-02-07 14:09 ` [Qemu-devel] [PATCH 15/19] console: Chardev open error reporting, console part Markus Armbruster
@ 2012-02-07 14:09 ` Markus Armbruster
2012-02-07 14:09 ` [Qemu-devel] [PATCH 17/19] baum: Chardev open error reporting, braille part Markus Armbruster
` (4 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Markus Armbruster @ 2012-02-07 14:09 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, aliguori
Convert to error_report().
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
spice-qemu-char.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index 1e735ff..290437d 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -176,16 +176,16 @@ static void print_allowed_subtypes(void)
const char** psubtype;
int i;
- fprintf(stderr, "allowed names: ");
+ error_printf("allowed names: ");
for(i=0, psubtype = spice_server_char_device_recognized_subtypes();
*psubtype != NULL; ++psubtype, ++i) {
if (i == 0) {
- fprintf(stderr, "%s", *psubtype);
+ error_printf("%s", *psubtype);
} else {
- fprintf(stderr, ", %s", *psubtype);
+ error_printf(", %s", *psubtype);
}
}
- fprintf(stderr, "\n");
+ error_printf("\n");
}
CharDriverState *qemu_chr_open_spice(QemuOpts *opts)
@@ -198,7 +198,7 @@ CharDriverState *qemu_chr_open_spice(QemuOpts *opts)
const char *subtype = NULL;
if (name == NULL) {
- fprintf(stderr, "spice-qemu-char: missing name parameter\n");
+ error_report("spicevmc character device requires parameter name");
print_allowed_subtypes();
return NULL;
}
@@ -209,7 +209,7 @@ CharDriverState *qemu_chr_open_spice(QemuOpts *opts)
}
}
if (subtype == NULL) {
- fprintf(stderr, "spice-qemu-char: unsupported name\n");
+ error_report("name '%s' is not supported", name);
print_allowed_subtypes();
return NULL;
}
--
1.7.6.5
^ permalink raw reply related [flat|nested] 45+ messages in thread* [Qemu-devel] [PATCH 17/19] baum: Chardev open error reporting, braille part
2012-02-07 14:09 [Qemu-devel] [PATCH 00/19] Fix and improve chardev open error messages Markus Armbruster
` (15 preceding siblings ...)
2012-02-07 14:09 ` [Qemu-devel] [PATCH 16/19] spice-qemu-char: Chardev open error reporting, spicevmc part Markus Armbruster
@ 2012-02-07 14:09 ` Markus Armbruster
2012-02-07 14:09 ` [Qemu-devel] [PATCH 18/19] qemu-char: Chardev open error reporting, generic part Markus Armbruster
` (3 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Markus Armbruster @ 2012-02-07 14:09 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, aliguori
Convert to error_report(). Improves user-hostile messages like this
one
baum_init: brlapi_openConnection: connect: No such file or directory
chardev: opening backend "braille" failed
to
qemu-system-x86_64: -chardev braille,id=foo: Can't open braille: connect: No such file or directory
chardev: opening backend "braille" failed
The uninformative extra "opening backend failed" message will be
cleaned up shortly.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/baum.c | 9 ++++++---
1 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/hw/baum.c b/hw/baum.c
index 3e94f84..7453a15 100644
--- a/hw/baum.c
+++ b/hw/baum.c
@@ -585,14 +585,16 @@ CharDriverState *chr_baum_init(QemuOpts *opts)
baum->brlapi_fd = brlapi__openConnection(handle, NULL, NULL);
if (baum->brlapi_fd == -1) {
- brlapi_perror("baum_init: brlapi_openConnection");
+ error_report("Can't open braille: %s",
+ brlapi_strerror(&brlapi_error));
goto fail_handle;
}
baum->cellCount_timer = qemu_new_timer_ns(vm_clock, baum_cellCount_timer_cb, baum);
if (brlapi__getDisplaySize(handle, &baum->x, &baum->y) == -1) {
- brlapi_perror("baum_init: brlapi_getDisplaySize");
+ error_report("Can't get braille display size: %s",
+ brlapi_strerror(&brlapi_error));
goto fail;
}
@@ -606,7 +608,8 @@ CharDriverState *chr_baum_init(QemuOpts *opts)
tty = BRLAPI_TTY_DEFAULT;
if (brlapi__enterTtyMode(handle, tty, NULL) == -1) {
- brlapi_perror("baum_init: brlapi_enterTtyMode");
+ error_report("Can't configure braille: %s",
+ brlapi_strerror(&brlapi_error));
goto fail;
}
--
1.7.6.5
^ permalink raw reply related [flat|nested] 45+ messages in thread* [Qemu-devel] [PATCH 18/19] qemu-char: Chardev open error reporting, generic part
2012-02-07 14:09 [Qemu-devel] [PATCH 00/19] Fix and improve chardev open error messages Markus Armbruster
` (16 preceding siblings ...)
2012-02-07 14:09 ` [Qemu-devel] [PATCH 17/19] baum: Chardev open error reporting, braille part Markus Armbruster
@ 2012-02-07 14:09 ` Markus Armbruster
2012-02-07 14:09 ` [Qemu-devel] [PATCH 19/19] qemu-char: Fix legacy chardev syntax error reporting Markus Armbruster
` (2 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Markus Armbruster @ 2012-02-07 14:09 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, aliguori
1. Convert to error_report().
2. All backends now report their errors, no need to follow up with an
unspecific "opening backend failed". Drop the message.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
qemu-char.c | 15 ++++++---------
1 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/qemu-char.c b/qemu-char.c
index ecbb595..fe0cfce 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2808,33 +2808,30 @@ static const struct {
CharDriverState *qemu_chr_new_from_opts(QemuOpts *opts,
void (*init)(struct CharDriverState *s))
{
+ const char *backend = qemu_opt_get(opts, "backend");
CharDriverState *chr;
int i;
if (qemu_opts_id(opts) == NULL) {
- fprintf(stderr, "chardev: no id specified\n");
+ error_report("character device requires parameter id");
return NULL;
}
- if (qemu_opt_get(opts, "backend") == NULL) {
- fprintf(stderr, "chardev: \"%s\" missing backend\n",
- qemu_opts_id(opts));
+ if (!backend) {
+ error_report("character device requires parameter backend");
return NULL;
}
for (i = 0; i < ARRAY_SIZE(backend_table); i++) {
- if (strcmp(backend_table[i].name, qemu_opt_get(opts, "backend")) == 0)
+ if (strcmp(backend_table[i].name, backend) == 0)
break;
}
if (i == ARRAY_SIZE(backend_table)) {
- fprintf(stderr, "chardev: backend \"%s\" not found\n",
- qemu_opt_get(opts, "backend"));
+ error_report("character device backend '%s' not found", backend);
return NULL;
}
chr = backend_table[i].open(opts);
if (!chr) {
- fprintf(stderr, "chardev: opening backend \"%s\" failed\n",
- qemu_opt_get(opts, "backend"));
return NULL;
}
--
1.7.6.5
^ permalink raw reply related [flat|nested] 45+ messages in thread* [Qemu-devel] [PATCH 19/19] qemu-char: Fix legacy chardev syntax error reporting
2012-02-07 14:09 [Qemu-devel] [PATCH 00/19] Fix and improve chardev open error messages Markus Armbruster
` (17 preceding siblings ...)
2012-02-07 14:09 ` [Qemu-devel] [PATCH 18/19] qemu-char: Chardev open error reporting, generic part Markus Armbruster
@ 2012-02-07 14:09 ` Markus Armbruster
2012-02-07 16:05 ` [Qemu-devel] [PATCH 00/19] Fix and improve chardev open error messages Kevin Wolf
2012-02-24 15:30 ` Anthony Liguori
20 siblings, 0 replies; 45+ messages in thread
From: Markus Armbruster @ 2012-02-07 14:09 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, aliguori
Watch this:
$ qemu-system-x86_64 -nodefaults -vnc :0 -debugcon crap
$ echo $?
1
$ qemu-system-x86_64 -nodefaults -vnc :0 -serial crap
qemu: could not open serial device 'crap': Success
Two issues:
1. qemu_chr_new() fails silently on syntax errors. It does report
other errors.
2. serial_parse() incorrectly asumes qemu_chr_new() sets errno on
failure. Same for parallel_parse() and virtcon_parse().
Fix both of them.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
qemu-char.c | 4 +++-
vl.c | 6 ------
2 files changed, 3 insertions(+), 7 deletions(-)
diff --git a/qemu-char.c b/qemu-char.c
index fe0cfce..c74babb 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2867,8 +2867,10 @@ CharDriverState *qemu_chr_new(const char *label, const char *filename, void (*in
}
opts = qemu_chr_parse_compat(label, filename);
- if (!opts)
+ if (!opts) {
+ error_report("parse error");
return NULL;
+ }
chr = qemu_chr_new_from_opts(opts, init);
if (chr && qemu_opt_get_bool(opts, "mux", 0)) {
diff --git a/vl.c b/vl.c
index 1394a08..941fab0 100644
--- a/vl.c
+++ b/vl.c
@@ -1907,8 +1907,6 @@ static int serial_parse(const char *devname)
snprintf(label, sizeof(label), "serial%d", index);
serial_hds[index] = qemu_chr_new(label, devname, NULL);
if (!serial_hds[index]) {
- fprintf(stderr, "qemu: could not open serial device '%s': %s\n",
- devname, strerror(errno));
return -1;
}
index++;
@@ -1929,8 +1927,6 @@ static int parallel_parse(const char *devname)
snprintf(label, sizeof(label), "parallel%d", index);
parallel_hds[index] = qemu_chr_new(label, devname, NULL);
if (!parallel_hds[index]) {
- fprintf(stderr, "qemu: could not open parallel device '%s': %s\n",
- devname, strerror(errno));
return -1;
}
index++;
@@ -1960,8 +1956,6 @@ static int virtcon_parse(const char *devname)
snprintf(label, sizeof(label), "virtcon%d", index);
virtcon_hds[index] = qemu_chr_new(label, devname, NULL);
if (!virtcon_hds[index]) {
- fprintf(stderr, "qemu: could not open virtio console '%s': %s\n",
- devname, strerror(errno));
return -1;
}
qemu_opt_set(dev_opts, "chardev", label);
--
1.7.6.5
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [Qemu-devel] [PATCH 00/19] Fix and improve chardev open error messages
2012-02-07 14:09 [Qemu-devel] [PATCH 00/19] Fix and improve chardev open error messages Markus Armbruster
` (18 preceding siblings ...)
2012-02-07 14:09 ` [Qemu-devel] [PATCH 19/19] qemu-char: Fix legacy chardev syntax error reporting Markus Armbruster
@ 2012-02-07 16:05 ` Kevin Wolf
2012-02-24 15:30 ` Anthony Liguori
20 siblings, 0 replies; 45+ messages in thread
From: Kevin Wolf @ 2012-02-07 16:05 UTC (permalink / raw)
To: Markus Armbruster; +Cc: aliguori, qemu-devel
Am 07.02.2012 15:09, schrieb Markus Armbruster:
> Our chardev open error messages are an embarrassment. Commit 6e1db57b
> tried to improve the useless "opening backend FOO failed" message in
> qemu_chr_open_opts(), but it is flawed: some failure modes went from
> an unhelpful "failed" to an outright misleading error message (see
> first patch for details). And even for failure modes where the
> message isn't misleading, it's still sub-par.
>
> Clue: many backends already report their errors. The "failed" message
> is merely redundant then.
>
> Since I'm touching the error reporting anyway, convert it to
> error_report(), so that a future a monitor command to add character
> devices emits its errors to the monitor, not stderr.
>
> Outline:
>
> [01-04/19] Revert the flawed commit
> [05-06/19] Prepare for use of error_report()
> [07-17/19] Make the backends report decent errors on all failure paths
> [ 18/18] Rip out the useless "failed" message
> [ 19/19] Bonus fix: legacy chardev syntax error reporting
I had some minor comments which can be fixed on top. I also think that
introducing Error** to these functions can (or actually should) be a
separate step.
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [Qemu-devel] [PATCH 00/19] Fix and improve chardev open error messages
2012-02-07 14:09 [Qemu-devel] [PATCH 00/19] Fix and improve chardev open error messages Markus Armbruster
` (19 preceding siblings ...)
2012-02-07 16:05 ` [Qemu-devel] [PATCH 00/19] Fix and improve chardev open error messages Kevin Wolf
@ 2012-02-24 15:30 ` Anthony Liguori
20 siblings, 0 replies; 45+ messages in thread
From: Anthony Liguori @ 2012-02-24 15:30 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, qemu-devel
On 02/07/2012 08:09 AM, Markus Armbruster wrote:
> Our chardev open error messages are an embarrassment. Commit 6e1db57b
> tried to improve the useless "opening backend FOO failed" message in
> qemu_chr_open_opts(), but it is flawed: some failure modes went from
> an unhelpful "failed" to an outright misleading error message (see
> first patch for details). And even for failure modes where the
> message isn't misleading, it's still sub-par.
>
> Clue: many backends already report their errors. The "failed" message
> is merely redundant then.
>
> Since I'm touching the error reporting anyway, convert it to
> error_report(), so that a future a monitor command to add character
> devices emits its errors to the monitor, not stderr.
I've applied 1-8 and 14 as discussed in the mail exchange.
I've still got the remaining patches in my review queue.
Thanks,
Anthony Liguori
>
> Outline:
>
> [01-04/19] Revert the flawed commit
> [05-06/19] Prepare for use of error_report()
> [07-17/19] Make the backends report decent errors on all failure paths
> [ 18/18] Rip out the useless "failed" message
> [ 19/19] Bonus fix: legacy chardev syntax error reporting
>
> Markus Armbruster (19):
> Revert "qemu-char: Print strerror message on failure" and deps
> qemu-char: Use qemu_open() to avoid leaking fds to children
> qemu-char: Re-apply style fixes from just reverted aad04cd0
> qemu-char: qemu_chr_open_fd() can't fail, don't check
> vl.c: Error locations for options using add_device_config()
> gdbstub: Error locations for -gdb
> sockets: Drop sockets_debug debug code
> sockets: Clean up inet_listen_opts()'s convoluted bind() loop
> sockets: Chardev open error reporting, sockets part
> qemu-char: Chardev open error reporting, !_WIN32 part
> qemu-char: Chardev open error reporting, _WIN32 part
> qemu-char: Chardev open error reporting, tty part
> qemu-char: Chardev open error reporting, parport part
> console: Eliminate text_consoles[]
> console: Chardev open error reporting, console part
> spice-qemu-char: Chardev open error reporting, spicevmc part
> baum: Chardev open error reporting, braille part
> qemu-char: Chardev open error reporting, generic part
> qemu-char: Fix legacy chardev syntax error reporting
>
> console.c | 28 ++----
> console.h | 2 +-
> hw/baum.c | 16 ++--
> hw/baum.h | 2 +-
> hw/msmouse.c | 5 +-
> hw/msmouse.h | 2 +-
> qemu-char.c | 263 ++++++++++++++++++++++++++++++-----------------------
> qemu-sockets.c | 203 +++++++++++++++--------------------------
> spice-qemu-char.c | 21 ++--
> ui/qemu-spice.h | 2 +-
> vl.c | 20 ++---
> 11 files changed, 265 insertions(+), 299 deletions(-)
>
^ permalink raw reply [flat|nested] 45+ messages in thread