* [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server
@ 2012-10-10 14:02 Paolo Bonzini
2012-10-10 14:02 ` [Qemu-devel] [PATCH 01/25] error: add error_set_errno and error_setg_errno Paolo Bonzini
` (26 more replies)
0 siblings, 27 replies; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-10 14:02 UTC (permalink / raw)
To: qemu-devel; +Cc: lcapitulino
Luiz,
The following changes since commit b4ae3cfa57b8c1bdbbd7b7d420971e9171203ade:
ssi: Add slave autoconnect helper (2012-10-10 11:13:32 +1000)
are available in the git repository at:
git://github.com/bonzini/qemu.git nbd-next
for you to fetch changes up to 7a057978b8b8f7578c03a470ffd8c486a778497a:
hmp: add NBD server commands (2012-10-10 13:44:09 +0200)
The following patches changed from v1:
04/25 qemu-sockets: add nonblocking connect for Unix sockets
v1->v2: fixed connect_state memory leaks
05/25 migration: avoid using error_is_set and thus relying on errp != NULL
v1->v2: changed summary line of the commit message
08/25 migration (outgoing): add error propagation for all protocols
v1->v2: turn bizarre DPRINTF into an assertion failure or just
drop it for the failure test of O_NONBLOCK. Clean up after this
change.
13/25 vnc: add error propagation to vnc_display_open
v1->v2: small cleanup for the if(reverse) case, do not use
qerror_report_err.
The following patches are unreviewed:
07/25 migration: use qemu-sockets to establish Unix sockets
17/25 qemu-sockets: add error propagation to inet_parse
Plus 11/25 is not reviewed, but that's NBD so it's my own turf. :)
Paolo
Paolo Bonzini (25):
error: add error_set_errno and error_setg_errno
qemu-sockets: add Error ** to all functions
qemu-sockets: unix_listen and unix_connect are portable
qemu-sockets: add nonblocking connect for Unix sockets
migration: avoid using error_is_set and thus relying on errp != NULL
migration: centralize call to migrate_fd_error()
migration: use qemu-sockets to establish Unix sockets
migration (outgoing): add error propagation for all protocols
migration (incoming): add error propagation to fd and exec protocols
qemu-char: ask and print error information from qemu-sockets
nbd: ask and print error information from qemu-sockets
qemu-ga: ask and print error information from qemu-sockets
vnc: add error propagation to vnc_display_open
qemu-sockets: include strerror or gai_strerror output in error messages
qemu-sockets: add error propagation to inet_connect_addr
qemu-sockets: add error propagation to inet_dgram_opts
qemu-sockets: add error propagation to inet_parse
qemu-sockets: add error propagation to Unix socket functions
build: add QAPI files to the tools
qapi: add socket address types
qemu-sockets: return IPSocketAddress from inet_parse
qemu-sockets: add socket_listen, socket_connect, socket_parse
block: add close notifiers
qmp: add NBD server commands
hmp: add NBD server commands
Makefile | 2 +-
Makefile.objs | 9 +-
block.c | 19 ++-
block.h | 1 +
block_int.h | 2 +
blockdev-nbd.c | 119 +++++++++++++++
console.h | 2 +-
error.c | 28 ++++
error.h | 9 ++
hmp-commands.hx | 29 ++++
hmp.c | 55 +++++++
hmp.h | 2 +
migration-exec.c | 26 +---
migration-fd.c | 27 +---
migration-tcp.c | 19 +--
migration-unix.c | 95 ++----------
migration.c | 34 ++---
migration.h | 19 ++-
nbd.c | 39 ++++-
qapi-schema.json | 96 ++++++++++++
qemu-char.c | 24 ++-
qemu-sockets.c | 422 ++++++++++++++++++++++++++++++++++------------------
qemu-tool.c | 6 +
qemu_socket.h | 19 ++-
qga/channel-posix.c | 8 +-
qmp-commands.hx | 16 ++
qmp.c | 6 +-
ui/vnc.c | 91 ++++++-----
vl.c | 25 ++--
29 file modificati, 842 inserzioni(+), 407 rimozioni(-)
create mode 100644 blockdev-nbd.c
--
1.7.12.1
^ permalink raw reply [flat|nested] 72+ messages in thread
* [Qemu-devel] [PATCH 01/25] error: add error_set_errno and error_setg_errno
2012-10-10 14:02 [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Paolo Bonzini
@ 2012-10-10 14:02 ` Paolo Bonzini
2012-10-10 14:02 ` [Qemu-devel] [PATCH 02/25] qemu-sockets: add Error ** to all functions Paolo Bonzini
` (25 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-10 14:02 UTC (permalink / raw)
To: qemu-devel; +Cc: lcapitulino
These functions help maintaining homogeneous formatting of error
messages that include strerror values.
Acked-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
error.c | 28 ++++++++++++++++++++++++++++
error.h | 9 +++++++++
2 file modificati, 37 inserzioni(+)
diff --git a/error.c b/error.c
index 1f05fc4..128d88c 100644
--- a/error.c
+++ b/error.c
@@ -43,6 +43,34 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
*errp = err;
}
+void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
+ const char *fmt, ...)
+{
+ Error *err;
+ char *msg1;
+ va_list ap;
+
+ if (errp == NULL) {
+ return;
+ }
+ assert(*errp == NULL);
+
+ err = g_malloc0(sizeof(*err));
+
+ va_start(ap, fmt);
+ msg1 = g_strdup_vprintf(fmt, ap);
+ if (os_errno != 0) {
+ err->msg = g_strdup_printf("%s: %s", msg1, strerror(os_errno));
+ g_free(msg1);
+ } else {
+ err->msg = msg1;
+ }
+ va_end(ap);
+ err->err_class = err_class;
+
+ *errp = err;
+}
+
Error *error_copy(const Error *err)
{
Error *err_new;
diff --git a/error.h b/error.h
index da7fed3..4d52e73 100644
--- a/error.h
+++ b/error.h
@@ -30,10 +30,19 @@ typedef struct Error Error;
void error_set(Error **err, ErrorClass err_class, const char *fmt, ...) GCC_FMT_ATTR(3, 4);
/**
+ * Set an indirect pointer to an error given a ErrorClass value and a
+ * printf-style human message, followed by a strerror() string if
+ * @os_error is not zero.
+ */
+void error_set_errno(Error **err, int os_error, ErrorClass err_class, const char *fmt, ...) GCC_FMT_ATTR(4, 5);
+
+/**
* Same as error_set(), but sets a generic error
*/
#define error_setg(err, fmt, ...) \
error_set(err, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__)
+#define error_setg_errno(err, os_error, fmt, ...) \
+ error_set_errno(err, os_error, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__)
/**
* Returns true if an indirect pointer to an error is pointing to a valid
--
1.7.12.1
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [Qemu-devel] [PATCH 02/25] qemu-sockets: add Error ** to all functions
2012-10-10 14:02 [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Paolo Bonzini
2012-10-10 14:02 ` [Qemu-devel] [PATCH 01/25] error: add error_set_errno and error_setg_errno Paolo Bonzini
@ 2012-10-10 14:02 ` Paolo Bonzini
2012-10-10 14:02 ` [Qemu-devel] [PATCH 03/25] qemu-sockets: unix_listen and unix_connect are portable Paolo Bonzini
` (24 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-10 14:02 UTC (permalink / raw)
To: qemu-devel; +Cc: lcapitulino
This lets me adjust the clients to do proper error propagation first,
thus avoiding temporary regressions in the quality of the error messages.
Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
nbd.c | 4 ++--
qemu-char.c | 6 +++---
qemu-sockets.c | 30 +++++++++++++++---------------
qemu_socket.h | 10 +++++-----
qga/channel-posix.c | 2 +-
ui/vnc.c | 4 ++--
6 file modificati, 28 inserzioni(+), 28 rimozioni(-)
diff --git a/nbd.c b/nbd.c
index 6f0db62..f61a288 100644
--- a/nbd.c
+++ b/nbd.c
@@ -230,12 +230,12 @@ int unix_socket_incoming(const char *path)
char *ostr = NULL;
int olen = 0;
- return unix_listen(path, ostr, olen);
+ return unix_listen(path, ostr, olen, NULL);
}
int unix_socket_outgoing(const char *path)
{
- return unix_connect(path);
+ return unix_connect(path, NULL);
}
/* Basic flow for negotiation
diff --git a/qemu-char.c b/qemu-char.c
index b082bae..3cc6cb5 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2102,7 +2102,7 @@ static CharDriverState *qemu_chr_open_udp(QemuOpts *opts)
chr = g_malloc0(sizeof(CharDriverState));
s = g_malloc0(sizeof(NetCharDriver));
- fd = inet_dgram_opts(opts);
+ fd = inet_dgram_opts(opts, NULL);
if (fd < 0) {
fprintf(stderr, "inet_dgram_opts failed\n");
goto return_err;
@@ -2448,9 +2448,9 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
if (is_unix) {
if (is_listen) {
- fd = unix_listen_opts(opts);
+ fd = unix_listen_opts(opts, NULL);
} else {
- fd = unix_connect_opts(opts);
+ fd = unix_connect_opts(opts, NULL);
}
} else {
if (is_listen) {
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 2b1ed2f..ed062d3 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -403,7 +403,7 @@ int inet_connect_opts(QemuOpts *opts, Error **errp,
return sock;
}
-int inet_dgram_opts(QemuOpts *opts)
+int inet_dgram_opts(QemuOpts *opts, Error **errp)
{
struct addrinfo ai, *peer = NULL, *local = NULL;
const char *addr;
@@ -653,7 +653,7 @@ int inet_nonblocking_connect(const char *str,
#ifndef _WIN32
-int unix_listen_opts(QemuOpts *opts)
+int unix_listen_opts(QemuOpts *opts, Error **errp)
{
struct sockaddr_un un;
const char *path = qemu_opt_get(opts, "path");
@@ -701,7 +701,7 @@ err:
return -1;
}
-int unix_connect_opts(QemuOpts *opts)
+int unix_connect_opts(QemuOpts *opts, Error **errp)
{
struct sockaddr_un un;
const char *path = qemu_opt_get(opts, "path");
@@ -731,7 +731,7 @@ int unix_connect_opts(QemuOpts *opts)
}
/* compatibility wrapper */
-int unix_listen(const char *str, char *ostr, int olen)
+int unix_listen(const char *str, char *ostr, int olen, Error **errp)
{
QemuOpts *opts;
char *path, *optstr;
@@ -752,7 +752,7 @@ int unix_listen(const char *str, char *ostr, int olen)
qemu_opt_set(opts, "path", str);
}
- sock = unix_listen_opts(opts);
+ sock = unix_listen_opts(opts, errp);
if (sock != -1 && ostr)
snprintf(ostr, olen, "%s%s", qemu_opt_get(opts, "path"), optstr ? optstr : "");
@@ -760,44 +760,44 @@ int unix_listen(const char *str, char *ostr, int olen)
return sock;
}
-int unix_connect(const char *path)
+int unix_connect(const char *path, Error **errp)
{
QemuOpts *opts;
int sock;
opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
qemu_opt_set(opts, "path", path);
- sock = unix_connect_opts(opts);
+ sock = unix_connect_opts(opts, errp);
qemu_opts_del(opts);
return sock;
}
#else
-int unix_listen_opts(QemuOpts *opts)
+int unix_listen_opts(QemuOpts *opts, Error **errp)
{
- fprintf(stderr, "unix sockets are not available on windows\n");
+ error_setg(errp, "unix sockets are not available on windows");
errno = ENOTSUP;
return -1;
}
-int unix_connect_opts(QemuOpts *opts)
+int unix_connect_opts(QemuOpts *opts, Error **errp)
{
- fprintf(stderr, "unix sockets are not available on windows\n");
+ error_setg(errp, "unix sockets are not available on windows");
errno = ENOTSUP;
return -1;
}
-int unix_listen(const char *path, char *ostr, int olen)
+int unix_listen(const char *path, char *ostr, int olen, Error **errp)
{
- fprintf(stderr, "unix sockets are not available on windows\n");
+ error_setg(errp, "unix sockets are not available on windows");
errno = ENOTSUP;
return -1;
}
-int unix_connect(const char *path)
+int unix_connect(const char *path, Error **errp)
{
- fprintf(stderr, "unix sockets are not available on windows\n");
+ error_setg(errp, "unix sockets are not available on windows");
errno = ENOTSUP;
return -1;
}
diff --git a/qemu_socket.h b/qemu_socket.h
index 3e8aee9..ff979b5 100644
--- a/qemu_socket.h
+++ b/qemu_socket.h
@@ -53,13 +53,13 @@ int inet_nonblocking_connect(const char *str,
NonBlockingConnectHandler *callback,
void *opaque, Error **errp);
-int inet_dgram_opts(QemuOpts *opts);
+int inet_dgram_opts(QemuOpts *opts, Error **errp);
const char *inet_strfamily(int family);
-int unix_listen_opts(QemuOpts *opts);
-int unix_listen(const char *path, char *ostr, int olen);
-int unix_connect_opts(QemuOpts *opts);
-int unix_connect(const char *path);
+int unix_listen_opts(QemuOpts *opts, Error **errp);
+int unix_listen(const char *path, char *ostr, int olen, Error **errp);
+int unix_connect_opts(QemuOpts *opts, Error **errp);
+int unix_connect(const char *path, Error **errp);
/* Old, ipv4 only bits. Don't use for new code. */
int parse_host_port(struct sockaddr_in *saddr, const char *str);
diff --git a/qga/channel-posix.c b/qga/channel-posix.c
index 57eea06..e22eee6 100644
--- a/qga/channel-posix.c
+++ b/qga/channel-posix.c
@@ -181,7 +181,7 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path, GAChannelMethod
break;
}
case GA_CHANNEL_UNIX_LISTEN: {
- int fd = unix_listen(path, NULL, strlen(path));
+ int fd = unix_listen(path, NULL, strlen(path), NULL);
if (fd == -1) {
g_critical("error opening path: %s", strerror(errno));
return false;
diff --git a/ui/vnc.c b/ui/vnc.c
index 01b2daf..235596e 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3059,7 +3059,7 @@ int vnc_display_open(DisplayState *ds, const char *display)
if (reverse) {
/* connect to viewer */
if (strncmp(display, "unix:", 5) == 0)
- vs->lsock = unix_connect(display+5);
+ vs->lsock = unix_connect(display+5, NULL);
else
vs->lsock = inet_connect(display, NULL);
if (-1 == vs->lsock) {
@@ -3079,7 +3079,7 @@ int vnc_display_open(DisplayState *ds, const char *display)
dpy = g_malloc(256);
if (strncmp(display, "unix:", 5) == 0) {
pstrcpy(dpy, 256, "unix:");
- vs->lsock = unix_listen(display+5, dpy+5, 256-5);
+ vs->lsock = unix_listen(display+5, dpy+5, 256-5, NULL);
} else {
vs->lsock = inet_listen(display, dpy, 256,
SOCK_STREAM, 5900, NULL);
--
1.7.12.1
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [Qemu-devel] [PATCH 03/25] qemu-sockets: unix_listen and unix_connect are portable
2012-10-10 14:02 [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Paolo Bonzini
2012-10-10 14:02 ` [Qemu-devel] [PATCH 01/25] error: add error_set_errno and error_setg_errno Paolo Bonzini
2012-10-10 14:02 ` [Qemu-devel] [PATCH 02/25] qemu-sockets: add Error ** to all functions Paolo Bonzini
@ 2012-10-10 14:02 ` Paolo Bonzini
2012-10-10 14:02 ` [Qemu-devel] [PATCH 04/25] qemu-sockets: add nonblocking connect for Unix sockets Paolo Bonzini
` (23 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-10 14:02 UTC (permalink / raw)
To: qemu-devel; +Cc: lcapitulino
They are just wrappers and do not need a Win32-specific version.
Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
qemu-sockets.c | 49 +++++++++++++++++--------------------------------
1 file modificato, 17 inserzioni(+), 32 rimozioni(-)
diff --git a/qemu-sockets.c b/qemu-sockets.c
index ed062d3..f7e67b6 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -730,6 +730,23 @@ int unix_connect_opts(QemuOpts *opts, Error **errp)
return sock;
}
+#else
+
+int unix_listen_opts(QemuOpts *opts, Error **errp)
+{
+ error_setg(errp, "unix sockets are not available on windows");
+ errno = ENOTSUP;
+ return -1;
+}
+
+int unix_connect_opts(QemuOpts *opts, Error **errp)
+{
+ error_setg(errp, "unix sockets are not available on windows");
+ errno = ENOTSUP;
+ return -1;
+}
+#endif
+
/* compatibility wrapper */
int unix_listen(const char *str, char *ostr, int olen, Error **errp)
{
@@ -772,38 +789,6 @@ int unix_connect(const char *path, Error **errp)
return sock;
}
-#else
-
-int unix_listen_opts(QemuOpts *opts, Error **errp)
-{
- error_setg(errp, "unix sockets are not available on windows");
- errno = ENOTSUP;
- return -1;
-}
-
-int unix_connect_opts(QemuOpts *opts, Error **errp)
-{
- error_setg(errp, "unix sockets are not available on windows");
- errno = ENOTSUP;
- return -1;
-}
-
-int unix_listen(const char *path, char *ostr, int olen, Error **errp)
-{
- error_setg(errp, "unix sockets are not available on windows");
- errno = ENOTSUP;
- return -1;
-}
-
-int unix_connect(const char *path, Error **errp)
-{
- error_setg(errp, "unix sockets are not available on windows");
- errno = ENOTSUP;
- return -1;
-}
-
-#endif
-
#ifdef _WIN32
static void socket_cleanup(void)
{
--
1.7.12.1
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [Qemu-devel] [PATCH 04/25] qemu-sockets: add nonblocking connect for Unix sockets
2012-10-10 14:02 [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Paolo Bonzini
` (2 preceding siblings ...)
2012-10-10 14:02 ` [Qemu-devel] [PATCH 03/25] qemu-sockets: unix_listen and unix_connect are portable Paolo Bonzini
@ 2012-10-10 14:02 ` Paolo Bonzini
2012-10-17 13:33 ` Markus Armbruster
2012-10-10 14:02 ` [Qemu-devel] [PATCH 05/25] migration: avoid using error_is_set and thus relying on errp != NULL Paolo Bonzini
` (22 subsequent siblings)
26 siblings, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-10 14:02 UTC (permalink / raw)
To: qemu-devel; +Cc: lcapitulino
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
v1->v2: fixed connect_state memory leaks
qemu-char.c | 2 +-
qemu-sockets.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++----------
qemu_socket.h | 6 ++++-
3 file modificati, 70 inserzioni(+), 15 rimozioni(-)
diff --git a/qemu-char.c b/qemu-char.c
index 3cc6cb5..8ebd582 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2450,7 +2450,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
if (is_listen) {
fd = unix_listen_opts(opts, NULL);
} else {
- fd = unix_connect_opts(opts, NULL);
+ fd = unix_connect_opts(opts, NULL, NULL, NULL);
}
} else {
if (is_listen) {
diff --git a/qemu-sockets.c b/qemu-sockets.c
index f7e67b6..6a49429 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -252,16 +252,19 @@ static void wait_for_connect(void *opaque)
}
/* try to connect to the next address on the list */
- while (s->current_addr->ai_next != NULL && s->fd < 0) {
- s->current_addr = s->current_addr->ai_next;
- s->fd = inet_connect_addr(s->current_addr, &in_progress, s);
- /* connect in progress */
- if (in_progress) {
- return;
+ if (s->current_addr) {
+ while (s->current_addr->ai_next != NULL && s->fd < 0) {
+ s->current_addr = s->current_addr->ai_next;
+ s->fd = inet_connect_addr(s->current_addr, &in_progress, s);
+ /* connect in progress */
+ if (in_progress) {
+ return;
+ }
}
+
+ freeaddrinfo(s->addr_list);
}
- freeaddrinfo(s->addr_list);
if (s->callback) {
s->callback(s->fd, s->opaque);
}
@@ -701,11 +704,13 @@ err:
return -1;
}
-int unix_connect_opts(QemuOpts *opts, Error **errp)
+int unix_connect_opts(QemuOpts *opts, Error **errp,
+ NonBlockingConnectHandler *callback, void *opaque)
{
struct sockaddr_un un;
const char *path = qemu_opt_get(opts, "path");
- int sock;
+ ConnectState *connect_state = NULL;
+ int sock, rc;
if (NULL == path) {
fprintf(stderr, "unix connect: no path specified\n");
@@ -717,16 +722,44 @@ int unix_connect_opts(QemuOpts *opts, Error **errp)
perror("socket(unix)");
return -1;
}
+ if (callback != NULL) {
+ connect_state = g_malloc0(sizeof(*connect_state));
+ connect_state->callback = callback;
+ connect_state->opaque = opaque;
+ socket_set_nonblock(sock);
+ }
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) {
+
+ /* connect to peer */
+ do {
+ rc = 0;
+ if (connect(sock, (struct sockaddr *) &un, sizeof(un)) < 0) {
+ rc = -socket_error();
+ }
+ } while (rc == -EINTR);
+
+ if (connect_state != NULL && QEMU_SOCKET_RC_INPROGRESS(rc)) {
+ connect_state->fd = sock;
+ qemu_set_fd_handler2(sock, NULL, NULL, wait_for_connect,
+ connect_state);
+ return sock;
+ } else {
+ /* non blocking socket immediate success, call callback */
+ if (callback != NULL) {
+ callback(sock, opaque);
+ }
+ }
+
+ if (rc < 0) {
fprintf(stderr, "connect(unix:%s): %s\n", path, strerror(errno));
close(sock);
- return -1;
+ sock = -1;
}
+ g_free(connect_state);
return sock;
}
@@ -739,7 +772,8 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)
return -1;
}
-int unix_connect_opts(QemuOpts *opts, Error **errp)
+int unix_connect_opts(QemuOpts *opts, Error **errp,
+ NonBlockingConnectHandler *callback, void *opaque)
{
error_setg(errp, "unix sockets are not available on windows");
errno = ENOTSUP;
@@ -784,7 +818,24 @@ int unix_connect(const char *path, Error **errp)
opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
qemu_opt_set(opts, "path", path);
- sock = unix_connect_opts(opts, errp);
+ sock = unix_connect_opts(opts, errp, NULL, NULL);
+ qemu_opts_del(opts);
+ return sock;
+}
+
+
+int unix_nonblocking_connect(const char *path,
+ NonBlockingConnectHandler *callback,
+ void *opaque, Error **errp)
+{
+ QemuOpts *opts;
+ int sock = -1;
+
+ g_assert(callback != NULL);
+
+ opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
+ qemu_opt_set(opts, "path", path);
+ sock = unix_connect_opts(opts, errp, callback, opaque);
qemu_opts_del(opts);
return sock;
}
diff --git a/qemu_socket.h b/qemu_socket.h
index ff979b5..89a5feb 100644
--- a/qemu_socket.h
+++ b/qemu_socket.h
@@ -58,8 +58,12 @@ const char *inet_strfamily(int family);
int unix_listen_opts(QemuOpts *opts, Error **errp);
int unix_listen(const char *path, char *ostr, int olen, Error **errp);
-int unix_connect_opts(QemuOpts *opts, Error **errp);
+int unix_connect_opts(QemuOpts *opts, Error **errp,
+ NonBlockingConnectHandler *callback, void *opaque);
int unix_connect(const char *path, Error **errp);
+int unix_nonblocking_connect(const char *str,
+ NonBlockingConnectHandler *callback,
+ void *opaque, Error **errp);
/* Old, ipv4 only bits. Don't use for new code. */
int parse_host_port(struct sockaddr_in *saddr, const char *str);
--
1.7.12.1
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [Qemu-devel] [PATCH 05/25] migration: avoid using error_is_set and thus relying on errp != NULL
2012-10-10 14:02 [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Paolo Bonzini
` (3 preceding siblings ...)
2012-10-10 14:02 ` [Qemu-devel] [PATCH 04/25] qemu-sockets: add nonblocking connect for Unix sockets Paolo Bonzini
@ 2012-10-10 14:02 ` Paolo Bonzini
2012-10-17 13:36 ` Markus Armbruster
2012-10-10 14:02 ` [Qemu-devel] [PATCH 06/25] migration: centralize call to migrate_fd_error() Paolo Bonzini
` (21 subsequent siblings)
26 siblings, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-10 14:02 UTC (permalink / raw)
To: qemu-devel; +Cc: lcapitulino
The migration code is using errp to detect "internal" errors,
this means that it relies on errp being non-NULL.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
v1->v2: changed summary line of the commit message
migration-tcp.c | 8 +++++---
migration.c | 13 +++++++------
2 file modificati, 12 inserzioni(+), 9 rimozioni(-)
diff --git a/migration-tcp.c b/migration-tcp.c
index a15c2b8..78337a3 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -71,14 +71,16 @@ static void tcp_wait_for_connect(int fd, void *opaque)
int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
Error **errp)
{
+ Error *local_err = NULL;
+
s->get_error = socket_errno;
s->write = socket_write;
s->close = tcp_close;
- s->fd = inet_nonblocking_connect(host_port, tcp_wait_for_connect, s,
- errp);
- if (error_is_set(errp)) {
+ s->fd = inet_nonblocking_connect(host_port, tcp_wait_for_connect, s, &local_err);
+ if (local_err != NULL) {
migrate_fd_error(s);
+ error_propagate(errp, local_err);
return -1;
}
diff --git a/migration.c b/migration.c
index 22a05c4..8a04174 100644
--- a/migration.c
+++ b/migration.c
@@ -481,6 +481,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
bool has_inc, bool inc, bool has_detach, bool detach,
Error **errp)
{
+ Error *local_err = NULL;
MigrationState *s = migrate_get_current();
MigrationParams params;
const char *p;
@@ -506,7 +507,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
s = migrate_init(¶ms);
if (strstart(uri, "tcp:", &p)) {
- ret = tcp_start_outgoing_migration(s, p, errp);
+ ret = tcp_start_outgoing_migration(s, p, &local_err);
#if !defined(WIN32)
} else if (strstart(uri, "exec:", &p)) {
ret = exec_start_outgoing_migration(s, p);
@@ -520,11 +521,11 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
return;
}
- if (ret < 0) {
- if (!error_is_set(errp)) {
- DPRINTF("migration failed: %s\n", strerror(-ret));
- /* FIXME: we should return meaningful errors */
- error_set(errp, QERR_UNDEFINED_ERROR);
+ if (ret < 0 || local_err) {
+ if (!local_err) {
+ error_set_errno(errp, -ret, QERR_UNDEFINED_ERROR);
+ } else {
+ error_propagate(errp, local_err);
}
return;
}
--
1.7.12.1
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [Qemu-devel] [PATCH 06/25] migration: centralize call to migrate_fd_error()
2012-10-10 14:02 [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Paolo Bonzini
` (4 preceding siblings ...)
2012-10-10 14:02 ` [Qemu-devel] [PATCH 05/25] migration: avoid using error_is_set and thus relying on errp != NULL Paolo Bonzini
@ 2012-10-10 14:02 ` Paolo Bonzini
2012-10-10 14:02 ` [Qemu-devel] [PATCH 07/25] migration: use qemu-sockets to establish Unix sockets Paolo Bonzini
` (20 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-10 14:02 UTC (permalink / raw)
To: qemu-devel; +Cc: lcapitulino
The call to migrate_fd_error() was missing for non-socket backends, so
centralize it in qmp_migrate().
Before:
(qemu) migrate fd:ffff
migrate: An undefined error has occurred
(qemu) info migrate
(qemu)
After:
(qemu) migrate fd:ffff
migrate: An undefined error has occurred
(qemu) info migrate
capabilities: xbzrle: off
Migration status: failed
total time: 0 milliseconds
(The awful error message will be fixed later in the series).
Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
migration-tcp.c | 1 -
migration-unix.c | 1 -
migration.c | 1 +
3 file modificati, 1 inserzione(+), 2 rimozioni(-)
diff --git a/migration-tcp.c b/migration-tcp.c
index 78337a3..e8bc76a 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -79,7 +79,6 @@ int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
s->fd = inet_nonblocking_connect(host_port, tcp_wait_for_connect, s, &local_err);
if (local_err != NULL) {
- migrate_fd_error(s);
error_propagate(errp, local_err);
return -1;
}
diff --git a/migration-unix.c b/migration-unix.c
index 169de88..d349662 100644
--- a/migration-unix.c
+++ b/migration-unix.c
@@ -111,7 +111,6 @@ int unix_start_outgoing_migration(MigrationState *s, const char *path)
if (ret < 0) {
DPRINTF("connect failed\n");
- migrate_fd_error(s);
return ret;
}
migrate_fd_connect(s);
diff --git a/migration.c b/migration.c
index 8a04174..a56358e 100644
--- a/migration.c
+++ b/migration.c
@@ -522,6 +522,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
}
if (ret < 0 || local_err) {
+ migrate_fd_error(s);
if (!local_err) {
error_set_errno(errp, -ret, QERR_UNDEFINED_ERROR);
} else {
--
1.7.12.1
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [Qemu-devel] [PATCH 07/25] migration: use qemu-sockets to establish Unix sockets
2012-10-10 14:02 [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Paolo Bonzini
` (5 preceding siblings ...)
2012-10-10 14:02 ` [Qemu-devel] [PATCH 06/25] migration: centralize call to migrate_fd_error() Paolo Bonzini
@ 2012-10-10 14:02 ` Paolo Bonzini
2012-10-17 14:30 ` Markus Armbruster
2012-10-10 14:02 ` [Qemu-devel] [PATCH 08/25] migration (outgoing): add error propagation for all protocols Paolo Bonzini
` (19 subsequent siblings)
26 siblings, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-10 14:02 UTC (permalink / raw)
To: qemu-devel; +Cc: lcapitulino
This makes migration-unix.c again a cut-and-paste job from migration-tcp.c,
exactly as it was in the beginning. :)
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
migration-unix.c | 94 ++++++++++----------------------------------------------
migration.c | 4 +--
migration.h | 4 +--
3 file modificati, 21 inserzioni(+), 81 rimozioni(-)
diff --git a/migration-unix.c b/migration-unix.c
index d349662..5387c21 100644
--- a/migration-unix.c
+++ b/migration-unix.c
@@ -53,67 +53,34 @@ static int unix_close(MigrationState *s)
return r;
}
-static void unix_wait_for_connect(void *opaque)
+static void unix_wait_for_connect(int fd, void *opaque)
{
MigrationState *s = opaque;
- int val, ret;
- socklen_t valsize = sizeof(val);
- DPRINTF("connect completed\n");
- do {
- ret = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *) &val, &valsize);
- } while (ret == -1 && errno == EINTR);
-
- if (ret < 0) {
+ if (fd < 0) {
+ DPRINTF("migrate connect error\n");
+ s->fd = -1;
migrate_fd_error(s);
- return;
- }
-
- qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
-
- if (val == 0)
+ } else {
+ DPRINTF("migrate connect success\n");
+ s->fd = fd;
migrate_fd_connect(s);
- else {
- DPRINTF("error connecting %d\n", val);
- migrate_fd_error(s);
}
}
-int unix_start_outgoing_migration(MigrationState *s, const char *path)
+int unix_start_outgoing_migration(MigrationState *s, const char *path, Error **errp)
{
- struct sockaddr_un addr;
- int ret;
+ Error *local_err = NULL;
- addr.sun_family = AF_UNIX;
- snprintf(addr.sun_path, sizeof(addr.sun_path), "%s", path);
s->get_error = unix_errno;
s->write = unix_write;
s->close = unix_close;
- s->fd = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
- if (s->fd == -1) {
- DPRINTF("Unable to open socket");
- return -errno;
- }
-
- socket_set_nonblock(s->fd);
-
- do {
- ret = connect(s->fd, (struct sockaddr *)&addr, sizeof(addr));
- if (ret == -1) {
- ret = -errno;
- }
- if (ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
- qemu_set_fd_handler2(s->fd, NULL, NULL, unix_wait_for_connect, s);
- return 0;
- }
- } while (ret == -EINTR);
-
- if (ret < 0) {
- DPRINTF("connect failed\n");
- return ret;
+ s->fd = unix_nonblocking_connect(path, unix_wait_for_connect, s, &local_err);
+ if (local_err != NULL) {
+ error_propagate(errp, local_err);
+ return -1;
}
- migrate_fd_connect(s);
return 0;
}
@@ -151,43 +118,16 @@ out2:
close(s);
}
-int unix_start_incoming_migration(const char *path)
+int unix_start_incoming_migration(const char *path, Error **errp)
{
- struct sockaddr_un addr;
int s;
- int ret;
-
- DPRINTF("Attempting to start an incoming migration\n");
-
- s = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
- if (s == -1) {
- fprintf(stderr, "Could not open unix socket: %s\n", strerror(errno));
- return -errno;
- }
-
- memset(&addr, 0, sizeof(addr));
- addr.sun_family = AF_UNIX;
- snprintf(addr.sun_path, sizeof(addr.sun_path), "%s", path);
- unlink(addr.sun_path);
- if (bind(s, (struct sockaddr *) &addr, sizeof(addr)) < 0) {
- ret = -errno;
- fprintf(stderr, "bind(unix:%s): %s\n", addr.sun_path, strerror(errno));
- goto err;
- }
- if (listen(s, 1) == -1) {
- fprintf(stderr, "listen(unix:%s): %s\n", addr.sun_path,
- strerror(errno));
- ret = -errno;
- goto err;
+ s = unix_listen(path, NULL, 0, errp);
+ if (s < 0) {
+ return -1;
}
qemu_set_fd_handler2(s, NULL, unix_accept_incoming_migration, NULL,
(void *)(intptr_t)s);
-
return 0;
-
-err:
- close(s);
- return ret;
}
diff --git a/migration.c b/migration.c
index a56358e..767e297 100644
--- a/migration.c
+++ b/migration.c
@@ -75,7 +75,7 @@ int qemu_start_incoming_migration(const char *uri, Error **errp)
else if (strstart(uri, "exec:", &p))
ret = exec_start_incoming_migration(p);
else if (strstart(uri, "unix:", &p))
- ret = unix_start_incoming_migration(p);
+ ret = unix_start_incoming_migration(p, errp);
else if (strstart(uri, "fd:", &p))
ret = fd_start_incoming_migration(p);
#endif
@@ -512,7 +512,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
} else if (strstart(uri, "exec:", &p)) {
ret = exec_start_outgoing_migration(s, p);
} else if (strstart(uri, "unix:", &p)) {
- ret = unix_start_outgoing_migration(s, p);
+ ret = unix_start_outgoing_migration(s, p, &local_err);
} else if (strstart(uri, "fd:", &p)) {
ret = fd_start_outgoing_migration(s, p);
#endif
diff --git a/migration.h b/migration.h
index a9852fc..e0612a3 100644
--- a/migration.h
+++ b/migration.h
@@ -63,9 +63,9 @@ int tcp_start_incoming_migration(const char *host_port, Error **errp);
int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
Error **errp);
-int unix_start_incoming_migration(const char *path);
+int unix_start_incoming_migration(const char *path, Error **errp);
-int unix_start_outgoing_migration(MigrationState *s, const char *path);
+int unix_start_outgoing_migration(MigrationState *s, const char *path, Error **errp);
int fd_start_incoming_migration(const char *path);
--
1.7.12.1
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [Qemu-devel] [PATCH 08/25] migration (outgoing): add error propagation for all protocols
2012-10-10 14:02 [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Paolo Bonzini
` (6 preceding siblings ...)
2012-10-10 14:02 ` [Qemu-devel] [PATCH 07/25] migration: use qemu-sockets to establish Unix sockets Paolo Bonzini
@ 2012-10-10 14:02 ` Paolo Bonzini
2012-10-17 14:43 ` Markus Armbruster
2012-10-10 14:02 ` [Qemu-devel] [PATCH 09/25] migration (incoming): add error propagation to fd and exec protocols Paolo Bonzini
` (18 subsequent siblings)
26 siblings, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-10 14:02 UTC (permalink / raw)
To: qemu-devel; +Cc: lcapitulino
Error propagation is already there for socket backends, even though
it is (and remains) incomplete because no Error is passed to the
NonBlockingConnectHandler. Add it to other protocols, simplifying
code that tests for errors that will never happen.
With all protocols understanding Error, the code can be simplified
further by removing the return value.
Before:
(qemu) migrate fd:ffff
migrate: An undefined error has occurred
(qemu) info migrate
(qemu)
After:
(qemu) migrate fd:ffff
migrate: File descriptor named 'ffff' has not been found
(qemu) info migrate
capabilities: xbzrle: off
Migration status: failed
total time: 0 milliseconds
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
v1->v2: turn bizarre DPRINTF into an assertion failure or just
drop it for the failure test of O_NONBLOCK. Clean up after this
change.
migration-fd.c | 19 ++++---------------
migration-tcp.c | 13 ++-----------
migration-unix.c | 11 ++---------
migration.c | 17 ++++++-----------
migration.h | 9 ++++-----
6 file modificati, 22 inserzioni(+), 65 rimozioni(-)
diff --git a/migration-exec.c b/migration-exec.c
index 6c97db9..5f3f4b2 100644
--- a/migration-exec.c
+++ b/migration-exec.c
@@ -60,22 +60,18 @@ static int exec_close(MigrationState *s)
return ret;
}
-int exec_start_outgoing_migration(MigrationState *s, const char *command)
+void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
{
FILE *f;
f = popen(command, "w");
if (f == NULL) {
- DPRINTF("Unable to popen exec target\n");
- goto err_after_popen;
+ error_setg_errno(errp, errno, "failed to popen the migration target");
+ return;
}
s->fd = fileno(f);
- if (s->fd == -1) {
- DPRINTF("Unable to retrieve file descriptor for popen'd handle\n");
- goto err_after_open;
- }
-
+ assert(s->fd != -1);
socket_set_nonblock(s->fd);
s->opaque = qemu_popen(f, "w");
@@ -85,12 +81,6 @@ int exec_start_outgoing_migration(MigrationState *s, const char *command)
s->write = file_write;
migrate_fd_connect(s);
- return 0;
-
-err_after_open:
- pclose(f);
-err_after_popen:
- return -1;
}
static void exec_accept_incoming_migration(void *opaque)
diff --git a/migration-fd.c b/migration-fd.c
index 7335167..a7c800a 100644
--- a/migration-fd.c
+++ b/migration-fd.c
@@ -73,30 +73,19 @@ static int fd_close(MigrationState *s)
return 0;
}
-int fd_start_outgoing_migration(MigrationState *s, const char *fdname)
+void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **errp)
{
- s->fd = monitor_get_fd(cur_mon, fdname, NULL);
+ s->fd = monitor_get_fd(cur_mon, fdname, errp);
if (s->fd == -1) {
- DPRINTF("fd_migration: invalid file descriptor identifier\n");
- goto err_after_get_fd;
- }
-
- if (fcntl(s->fd, F_SETFL, O_NONBLOCK) == -1) {
- DPRINTF("Unable to set nonblocking mode on file descriptor\n");
- goto err_after_open;
+ return;
}
+ fcntl(s->fd, F_SETFL, O_NONBLOCK);
s->get_error = fd_errno;
s->write = fd_write;
s->close = fd_close;
migrate_fd_connect(s);
- return 0;
-
-err_after_open:
- close(s->fd);
-err_after_get_fd:
- return -1;
}
static void fd_accept_incoming_migration(void *opaque)
diff --git a/migration-tcp.c b/migration-tcp.c
index e8bc76a..5e54e68 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -68,22 +68,13 @@ static void tcp_wait_for_connect(int fd, void *opaque)
}
}
-int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
- Error **errp)
+void tcp_start_outgoing_migration(MigrationState *s, const char *host_port, Error **errp)
{
- Error *local_err = NULL;
-
s->get_error = socket_errno;
s->write = socket_write;
s->close = tcp_close;
- s->fd = inet_nonblocking_connect(host_port, tcp_wait_for_connect, s, &local_err);
- if (local_err != NULL) {
- error_propagate(errp, local_err);
- return -1;
- }
-
- return 0;
+ s->fd = inet_nonblocking_connect(host_port, tcp_wait_for_connect, s, errp);
}
static void tcp_accept_incoming_migration(void *opaque)
diff --git a/migration-unix.c b/migration-unix.c
index 5387c21..34a413a 100644
--- a/migration-unix.c
+++ b/migration-unix.c
@@ -68,20 +68,13 @@ static void unix_wait_for_connect(int fd, void *opaque)
}
}
-int unix_start_outgoing_migration(MigrationState *s, const char *path, Error **errp)
+void unix_start_outgoing_migration(MigrationState *s, const char *path, Error **errp)
{
- Error *local_err = NULL;
-
s->get_error = unix_errno;
s->write = unix_write;
s->close = unix_close;
- s->fd = unix_nonblocking_connect(path, unix_wait_for_connect, s, &local_err);
- if (local_err != NULL) {
- error_propagate(errp, local_err);
- return -1;
- }
- return 0;
+ s->fd = unix_nonblocking_connect(path, unix_wait_for_connect, s, errp);
}
static void unix_accept_incoming_migration(void *opaque)
diff --git a/migration.c b/migration.c
index 767e297..f7f0138 100644
--- a/migration.c
+++ b/migration.c
@@ -485,7 +485,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
MigrationState *s = migrate_get_current();
MigrationParams params;
const char *p;
- int ret;
params.blk = blk;
params.shared = inc;
@@ -507,27 +506,23 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
s = migrate_init(¶ms);
if (strstart(uri, "tcp:", &p)) {
- ret = tcp_start_outgoing_migration(s, p, &local_err);
+ tcp_start_outgoing_migration(s, p, &local_err);
#if !defined(WIN32)
} else if (strstart(uri, "exec:", &p)) {
- ret = exec_start_outgoing_migration(s, p);
+ exec_start_outgoing_migration(s, p, &local_err);
} else if (strstart(uri, "unix:", &p)) {
- ret = unix_start_outgoing_migration(s, p, &local_err);
+ unix_start_outgoing_migration(s, p, &local_err);
} else if (strstart(uri, "fd:", &p)) {
- ret = fd_start_outgoing_migration(s, p);
+ fd_start_outgoing_migration(s, p, &local_err);
#endif
} else {
error_set(errp, QERR_INVALID_PARAMETER_VALUE, "uri", "a valid migration protocol");
return;
}
- if (ret < 0 || local_err) {
+ if (local_err) {
migrate_fd_error(s);
- if (!local_err) {
- error_set_errno(errp, -ret, QERR_UNDEFINED_ERROR);
- } else {
- error_propagate(errp, local_err);
- }
+ error_propagate(errp, local_err);
return;
}
diff --git a/migration.h b/migration.h
index e0612a3..275d2d8 100644
--- a/migration.h
+++ b/migration.h
@@ -56,20 +56,19 @@ void do_info_migrate(Monitor *mon, QObject **ret_data);
int exec_start_incoming_migration(const char *host_port);
-int exec_start_outgoing_migration(MigrationState *s, const char *host_port);
+void exec_start_outgoing_migration(MigrationState *s, const char *host_port, Error **errp);
int tcp_start_incoming_migration(const char *host_port, Error **errp);
-int tcp_start_outgoing_migration(MigrationState *s, const char *host_port,
- Error **errp);
+void tcp_start_outgoing_migration(MigrationState *s, const char *host_port, Error **errp);
int unix_start_incoming_migration(const char *path, Error **errp);
-int unix_start_outgoing_migration(MigrationState *s, const char *path, Error **errp);
+void unix_start_outgoing_migration(MigrationState *s, const char *path, Error **errp);
int fd_start_incoming_migration(const char *path);
-int fd_start_outgoing_migration(MigrationState *s, const char *fdname);
+void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **errp);
void migrate_fd_error(MigrationState *s);
--
1.7.12.1
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [Qemu-devel] [PATCH 09/25] migration (incoming): add error propagation to fd and exec protocols
2012-10-10 14:02 [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Paolo Bonzini
` (7 preceding siblings ...)
2012-10-10 14:02 ` [Qemu-devel] [PATCH 08/25] migration (outgoing): add error propagation for all protocols Paolo Bonzini
@ 2012-10-10 14:02 ` Paolo Bonzini
2012-10-10 14:02 ` [Qemu-devel] [PATCH 10/25] qemu-char: ask and print error information from qemu-sockets Paolo Bonzini
` (17 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-10 14:02 UTC (permalink / raw)
To: qemu-devel; +Cc: lcapitulino
And remove the superfluous integer return value.
Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
migration-exec.c | 8 +++-----
migration-fd.c | 8 +++-----
migration-tcp.c | 7 ++-----
migration-unix.c | 5 ++---
migration.c | 15 ++++++---------
migration.h | 10 +++++-----
vl.c | 16 ++++++----------
7 file modificati, 27 inserzioni(+), 42 rimozioni(-)
diff --git a/migration-exec.c b/migration-exec.c
index 5f3f4b2..519af57 100644
--- a/migration-exec.c
+++ b/migration-exec.c
@@ -92,19 +92,17 @@ static void exec_accept_incoming_migration(void *opaque)
qemu_fclose(f);
}
-int exec_start_incoming_migration(const char *command)
+void exec_start_incoming_migration(const char *command, Error **errp)
{
QEMUFile *f;
DPRINTF("Attempting to start an incoming migration\n");
f = qemu_popen_cmd(command, "r");
if(f == NULL) {
- DPRINTF("Unable to apply qemu wrapper to popen file\n");
- return -errno;
+ error_setg_errno(errp, errno, "failed to popen the migration source");
+ return;
}
qemu_set_fd_handler2(qemu_stdio_fd(f), NULL,
exec_accept_incoming_migration, NULL, f);
-
- return 0;
}
diff --git a/migration-fd.c b/migration-fd.c
index a7c800a..ce6932d 100644
--- a/migration-fd.c
+++ b/migration-fd.c
@@ -97,7 +97,7 @@ static void fd_accept_incoming_migration(void *opaque)
qemu_fclose(f);
}
-int fd_start_incoming_migration(const char *infd)
+void fd_start_incoming_migration(const char *infd, Error **errp)
{
int fd;
QEMUFile *f;
@@ -107,11 +107,9 @@ int fd_start_incoming_migration(const char *infd)
fd = strtol(infd, NULL, 0);
f = qemu_fdopen(fd, "rb");
if(f == NULL) {
- DPRINTF("Unable to apply qemu wrapper to file descriptor\n");
- return -errno;
+ error_setg_errno(errp, errno, "failed to open the source descriptor");
+ return;
}
qemu_set_fd_handler2(fd, NULL, fd_accept_incoming_migration, NULL, f);
-
- return 0;
}
diff --git a/migration-tcp.c b/migration-tcp.c
index 5e54e68..46f6ac5 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -111,18 +111,15 @@ out2:
close(s);
}
-int tcp_start_incoming_migration(const char *host_port, Error **errp)
+void tcp_start_incoming_migration(const char *host_port, Error **errp)
{
int s;
s = inet_listen(host_port, NULL, 256, SOCK_STREAM, 0, errp);
-
if (s < 0) {
- return -1;
+ return;
}
qemu_set_fd_handler2(s, NULL, tcp_accept_incoming_migration, NULL,
(void *)(intptr_t)s);
-
- return 0;
}
diff --git a/migration-unix.c b/migration-unix.c
index 34a413a..ed3db3a 100644
--- a/migration-unix.c
+++ b/migration-unix.c
@@ -111,16 +111,15 @@ out2:
close(s);
}
-int unix_start_incoming_migration(const char *path, Error **errp)
+void unix_start_incoming_migration(const char *path, Error **errp)
{
int s;
s = unix_listen(path, NULL, 0, errp);
if (s < 0) {
- return -1;
+ return;
}
qemu_set_fd_handler2(s, NULL, unix_accept_incoming_migration, NULL,
(void *)(intptr_t)s);
- return 0;
}
diff --git a/migration.c b/migration.c
index f7f0138..2802918 100644
--- a/migration.c
+++ b/migration.c
@@ -64,26 +64,23 @@ static MigrationState *migrate_get_current(void)
return ¤t_migration;
}
-int qemu_start_incoming_migration(const char *uri, Error **errp)
+void qemu_start_incoming_migration(const char *uri, Error **errp)
{
const char *p;
- int ret;
if (strstart(uri, "tcp:", &p))
- ret = tcp_start_incoming_migration(p, errp);
+ tcp_start_incoming_migration(p, errp);
#if !defined(WIN32)
else if (strstart(uri, "exec:", &p))
- ret = exec_start_incoming_migration(p);
+ exec_start_incoming_migration(p, errp);
else if (strstart(uri, "unix:", &p))
- ret = unix_start_incoming_migration(p, errp);
+ unix_start_incoming_migration(p, errp);
else if (strstart(uri, "fd:", &p))
- ret = fd_start_incoming_migration(p);
+ fd_start_incoming_migration(p, errp);
#endif
else {
- fprintf(stderr, "unknown migration protocol: %s\n", uri);
- ret = -EPROTONOSUPPORT;
+ error_setg(errp, "unknown migration protocol: %s\n", uri);
}
- return ret;
}
void process_incoming_migration(QEMUFile *f)
diff --git a/migration.h b/migration.h
index 275d2d8..aa44e1b 100644
--- a/migration.h
+++ b/migration.h
@@ -46,7 +46,7 @@ struct MigrationState
void process_incoming_migration(QEMUFile *f);
-int qemu_start_incoming_migration(const char *uri, Error **errp);
+void qemu_start_incoming_migration(const char *uri, Error **errp);
uint64_t migrate_max_downtime(void);
@@ -54,19 +54,19 @@ void do_info_migrate_print(Monitor *mon, const QObject *data);
void do_info_migrate(Monitor *mon, QObject **ret_data);
-int exec_start_incoming_migration(const char *host_port);
+void exec_start_incoming_migration(const char *host_port, Error **errp);
void exec_start_outgoing_migration(MigrationState *s, const char *host_port, Error **errp);
-int tcp_start_incoming_migration(const char *host_port, Error **errp);
+void tcp_start_incoming_migration(const char *host_port, Error **errp);
void tcp_start_outgoing_migration(MigrationState *s, const char *host_port, Error **errp);
-int unix_start_incoming_migration(const char *path, Error **errp);
+void unix_start_incoming_migration(const char *path, Error **errp);
void unix_start_outgoing_migration(MigrationState *s, const char *path, Error **errp);
-int fd_start_incoming_migration(const char *path);
+void fd_start_incoming_migration(const char *path, Error **errp);
void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **errp);
diff --git a/vl.c b/vl.c
index 5b357a3..2a072e8 100644
--- a/vl.c
+++ b/vl.c
@@ -3761,16 +3761,12 @@ int main(int argc, char **argv, char **envp)
}
if (incoming) {
- Error *errp = NULL;
- int ret = qemu_start_incoming_migration(incoming, &errp);
- if (ret < 0) {
- if (error_is_set(&errp)) {
- fprintf(stderr, "Migrate: %s\n", error_get_pretty(errp));
- error_free(errp);
- }
- fprintf(stderr, "Migration failed. Exit code %s(%d), exiting.\n",
- incoming, ret);
- exit(ret);
+ Error *local_err = NULL;
+ qemu_start_incoming_migration(incoming, &local_err);
+ if (local_err) {
+ fprintf(stderr, "-incoming %s: %s\n", incoming, error_get_pretty(local_err));
+ error_free(local_err);
+ exit(1);
}
} else if (autostart) {
vm_start();
--
1.7.12.1
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [Qemu-devel] [PATCH 10/25] qemu-char: ask and print error information from qemu-sockets
2012-10-10 14:02 [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Paolo Bonzini
` (8 preceding siblings ...)
2012-10-10 14:02 ` [Qemu-devel] [PATCH 09/25] migration (incoming): add error propagation to fd and exec protocols Paolo Bonzini
@ 2012-10-10 14:02 ` Paolo Bonzini
2012-10-10 14:02 ` [Qemu-devel] [PATCH 11/25] nbd: " Paolo Bonzini
` (16 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-10 14:02 UTC (permalink / raw)
To: qemu-devel; +Cc: lcapitulino
Before:
$ qemu-system-x86_64 -monitor tcp:localhost:6000
(starts despite error)
$ qemu-system-x86_64 -monitor tcp:foo.bar:12345
getaddrinfo(foo.bar,12345): Name or service not known
chardev: opening backend "socket" failed
$ qemu-system-x86_64 -monitor tcp:localhost:443,server=on
inet_listen_opts: bind(ipv4,127.0.0.1,443): Permission denied
inet_listen_opts: FAILED
chardev: opening backend "socket" failed
After:
$ x86_64-softmmu/qemu-system-x86_64 -monitor tcp:localhost:6000
x86_64-softmmu/qemu-system-x86_64: -monitor tcp:localhost:6000: Failed to connect to socket: Connection refused
chardev: opening backend "socket" failed
$ x86_64-softmmu/qemu-system-x86_64 -monitor tcp:foo.bar:12345
qemu-system-x86_64: -monitor tcp:foo.bar:12345: address resolution failed for foo.bar:12345: Name or service not known
chardev: opening backend "socket" failed
$ x86_64-softmmu/qemu-system-x86_64 -monitor tcp:localhost:443,server=on
qemu-system-x86_64: -monitor tcp:localhost:443,server=on: Failed to bind socket: Permission denied
chardev: opening backend "socket" failed
Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
qemu-char.c | 23 +++++++++++++++++------
1 file modificato, 17 inserzioni(+), 6 rimozioni(-)
diff --git a/qemu-char.c b/qemu-char.c
index 8ebd582..04b5c23 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2097,12 +2097,13 @@ static CharDriverState *qemu_chr_open_udp(QemuOpts *opts)
{
CharDriverState *chr = NULL;
NetCharDriver *s = NULL;
+ Error *local_err = NULL;
int fd = -1;
chr = g_malloc0(sizeof(CharDriverState));
s = g_malloc0(sizeof(NetCharDriver));
- fd = inet_dgram_opts(opts, NULL);
+ fd = inet_dgram_opts(opts, &local_err);
if (fd < 0) {
fprintf(stderr, "inet_dgram_opts failed\n");
goto return_err;
@@ -2118,6 +2119,10 @@ static CharDriverState *qemu_chr_open_udp(QemuOpts *opts)
return chr;
return_err:
+ if (local_err) {
+ qerror_report_err(local_err);
+ error_free(local_err);
+ }
g_free(chr);
g_free(s);
if (fd >= 0) {
@@ -2428,6 +2433,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
{
CharDriverState *chr = NULL;
TCPCharDriver *s = NULL;
+ Error *local_err = NULL;
int fd = -1;
int is_listen;
int is_waitconnect;
@@ -2448,15 +2454,15 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
if (is_unix) {
if (is_listen) {
- fd = unix_listen_opts(opts, NULL);
+ fd = unix_listen_opts(opts, &local_err);
} else {
- fd = unix_connect_opts(opts, NULL, NULL, NULL);
+ fd = unix_connect_opts(opts, &local_err, NULL, NULL);
}
} else {
if (is_listen) {
- fd = inet_listen_opts(opts, 0, NULL);
+ fd = inet_listen_opts(opts, 0, &local_err);
} else {
- fd = inet_connect_opts(opts, NULL, NULL, NULL);
+ fd = inet_connect_opts(opts, &local_err, NULL, NULL);
}
}
if (fd < 0) {
@@ -2517,8 +2523,13 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
return chr;
fail:
- if (fd >= 0)
+ if (local_err) {
+ qerror_report_err(local_err);
+ error_free(local_err);
+ }
+ if (fd >= 0) {
closesocket(fd);
+ }
g_free(s);
g_free(chr);
return NULL;
--
1.7.12.1
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [Qemu-devel] [PATCH 11/25] nbd: ask and print error information from qemu-sockets
2012-10-10 14:02 [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Paolo Bonzini
` (9 preceding siblings ...)
2012-10-10 14:02 ` [Qemu-devel] [PATCH 10/25] qemu-char: ask and print error information from qemu-sockets Paolo Bonzini
@ 2012-10-10 14:02 ` Paolo Bonzini
2012-10-17 14:51 ` Markus Armbruster
2012-10-10 14:02 ` [Qemu-devel] [PATCH 12/25] qemu-ga: " Paolo Bonzini
` (15 subsequent siblings)
26 siblings, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-10 14:02 UTC (permalink / raw)
To: qemu-devel; +Cc: lcapitulino
Before:
$ qemu-system-x86_64 nbd:localhost:12345
inet_connect_opts: connect(ipv4,yakj.usersys.redhat.com,127.0.0.1,12345): Connection refused
qemu-system-x86_64: could not open disk image nbd:localhost:12345: Connection refused
After:
$ x86_64-softmmu/qemu-system-x86_64 nbd:localhost:12345
qemu-system-x86_64: Failed to connect to socket: Connection refused
qemu-system-x86_64: could not open disk image nbd:localhost:12345: Connection refused
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
nbd.c | 39 +++++++++++++++++++++++++++++++--------
1 file modificato, 31 inserzioni(+), 8 rimozioni(-)
diff --git a/nbd.c b/nbd.c
index f61a288..cec5a94 100644
--- a/nbd.c
+++ b/nbd.c
@@ -208,7 +208,14 @@ int tcp_socket_outgoing(const char *address, uint16_t port)
int tcp_socket_outgoing_spec(const char *address_and_port)
{
- return inet_connect(address_and_port, NULL);
+ Error *local_err = NULL;
+ int fd = inet_connect(address_and_port, &local_err);
+
+ if (local_err != NULL) {
+ qerror_report_err(local_err);
+ error_free(local_err);
+ }
+ return fd;
}
int tcp_socket_incoming(const char *address, uint16_t port)
@@ -220,22 +227,38 @@ int tcp_socket_incoming(const char *address, uint16_t port)
int tcp_socket_incoming_spec(const char *address_and_port)
{
- char *ostr = NULL;
- int olen = 0;
- return inet_listen(address_and_port, ostr, olen, SOCK_STREAM, 0, NULL);
+ Error *local_err = NULL;
+ int fd = inet_listen(address_and_port, NULL, 0, SOCK_STREAM, 0, &local_err);
+
+ if (local_err != NULL) {
+ qerror_report_err(local_err);
+ error_free(local_err);
+ }
+ return fd;
}
int unix_socket_incoming(const char *path)
{
- char *ostr = NULL;
- int olen = 0;
+ Error *local_err = NULL;
+ int fd = unix_listen(path, NULL, 0, &local_err);
- return unix_listen(path, ostr, olen, NULL);
+ if (local_err != NULL) {
+ qerror_report_err(local_err);
+ error_free(local_err);
+ }
+ return fd;
}
int unix_socket_outgoing(const char *path)
{
- return unix_connect(path, NULL);
+ Error *local_err = NULL;
+ int fd = unix_connect(path, &local_err);
+
+ if (local_err != NULL) {
+ qerror_report_err(local_err);
+ error_free(local_err);
+ }
+ return fd;
}
/* Basic flow for negotiation
--
1.7.12.1
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [Qemu-devel] [PATCH 12/25] qemu-ga: ask and print error information from qemu-sockets
2012-10-10 14:02 [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Paolo Bonzini
` (10 preceding siblings ...)
2012-10-10 14:02 ` [Qemu-devel] [PATCH 11/25] nbd: " Paolo Bonzini
@ 2012-10-10 14:02 ` Paolo Bonzini
2012-10-10 14:02 ` [Qemu-devel] [PATCH 13/25] vnc: add error propagation to vnc_display_open Paolo Bonzini
` (14 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-10 14:02 UTC (permalink / raw)
To: qemu-devel; +Cc: lcapitulino
Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
qga/channel-posix.c | 8 +++++---
1 file modificato, 5 inserzioni(+), 3 rimozioni(-)
diff --git a/qga/channel-posix.c b/qga/channel-posix.c
index e22eee6..d152827 100644
--- a/qga/channel-posix.c
+++ b/qga/channel-posix.c
@@ -181,9 +181,11 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path, GAChannelMethod
break;
}
case GA_CHANNEL_UNIX_LISTEN: {
- int fd = unix_listen(path, NULL, strlen(path), NULL);
- if (fd == -1) {
- g_critical("error opening path: %s", strerror(errno));
+ Error *local_err = NULL;
+ int fd = unix_listen(path, NULL, strlen(path), &local_err);
+ if (local_err != NULL) {
+ g_critical("%s", error_get_pretty(local_err));
+ error_free(local_err);
return false;
}
ga_channel_listen_add(c, fd, true);
--
1.7.12.1
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [Qemu-devel] [PATCH 13/25] vnc: add error propagation to vnc_display_open
2012-10-10 14:02 [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Paolo Bonzini
` (11 preceding siblings ...)
2012-10-10 14:02 ` [Qemu-devel] [PATCH 12/25] qemu-ga: " Paolo Bonzini
@ 2012-10-10 14:02 ` Paolo Bonzini
2012-10-17 15:17 ` Markus Armbruster
2012-10-10 14:02 ` [Qemu-devel] [PATCH 14/25] qemu-sockets: include strerror or gai_strerror output in error messages Paolo Bonzini
` (13 subsequent siblings)
26 siblings, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-10 14:02 UTC (permalink / raw)
To: qemu-devel; +Cc: lcapitulino
Before:
$ qemu-system-x86_64 -vnc foo.bar:12345
getaddrinfo(foo.bar,18245): Name or service not known
Failed to start VNC server on `foo.bar:12345'
$ qemu-system-x86_64 -vnc localhost:12345,reverse=on
inet_connect_opts: connect(ipv4,yakj.usersys.redhat.com,127.0.0.1,12345): Connection refused
Failed to start VNC server on `localhost:12345,reverse=on'
After:
$ x86_64-softmmu/qemu-system-x86_64 -vnc foo.bar:12345
Failed to start VNC server on `foo.bar:12345': address resolution failed for foo.bar:18245: Name or service not known
$ x86_64-softmmu/qemu-system-x86_64 -vnc localhost:12345,reverse=on
Failed to start VNC server on `localhost:12345,reverse=on': Failed to connect to socket: Connection refused
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
v1->v2: small cleanup for the if(reverse) case, do not use
qerror_report_err.
migration-exec.c | 18 ++++--------------
console.h | 2 +-
qmp.c | 6 ++---
ui/vnc.c | 91 +++++++++++++++++++++++++++++----------------------------------
vl.c | 9 ++++---
4 file modificati, 51 inserzioni(+), 57 rimozioni(-)
diff --git a/console.h b/console.h
index f990684..6099d8d 100644
--- a/console.h
+++ b/console.h
@@ -378,7 +378,7 @@ void cocoa_display_init(DisplayState *ds, int full_screen);
/* vnc.c */
void vnc_display_init(DisplayState *ds);
void vnc_display_close(DisplayState *ds);
-int vnc_display_open(DisplayState *ds, const char *display);
+void vnc_display_open(DisplayState *ds, const char *display, Error **errp);
void vnc_display_add_client(DisplayState *ds, int csock, int skipauth);
int vnc_display_disable_login(DisplayState *ds);
char *vnc_display_local_addr(DisplayState *ds);
diff --git a/qmp.c b/qmp.c
index 36c54c5..31bc3bf 100644
--- a/qmp.c
+++ b/qmp.c
@@ -349,11 +349,9 @@ void qmp_change_vnc_password(const char *password, Error **errp)
}
}
-static void qmp_change_vnc_listen(const char *target, Error **err)
+static void qmp_change_vnc_listen(const char *target, Error **errp)
{
- if (vnc_display_open(NULL, target) < 0) {
- error_set(err, QERR_VNC_SERVER_FAILED, target);
- }
+ vnc_display_open(NULL, target, errp);
}
static void qmp_change_vnc(const char *target, bool has_arg, const char *arg,
diff --git a/ui/vnc.c b/ui/vnc.c
index 235596e..b8e46ca 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2844,7 +2844,7 @@ char *vnc_display_local_addr(DisplayState *ds)
return vnc_socket_local_addr("%s:%s", vs->lsock);
}
-int vnc_display_open(DisplayState *ds, const char *display)
+void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
{
VncDisplay *vs = ds ? (VncDisplay *)ds->opaque : vnc_display;
const char *options;
@@ -2863,13 +2863,12 @@ int vnc_display_open(DisplayState *ds, const char *display)
int lock_key_sync = 1;
if (!vnc_display)
- return -1;
+ goto fail;
vnc_display_close(ds);
if (strcmp(display, "none") == 0)
- return 0;
+ return;
- if (!(vs->display = strdup(display)))
- return -1;
+ vs->display = g_strdup(display);
vs->share_policy = VNC_SHARE_POLICY_ALLOW_EXCLUSIVE;
options = display;
@@ -2877,13 +2876,11 @@ int vnc_display_open(DisplayState *ds, const char *display)
options++;
if (strncmp(options, "password", 8) == 0) {
if (fips_get_state()) {
- fprintf(stderr,
- "VNC password auth disabled due to FIPS mode, "
- "consider using the VeNCrypt or SASL authentication "
- "methods as an alternative\n");
- g_free(vs->display);
- vs->display = NULL;
- return -1;
+ error_setg(errp,
+ "VNC password auth disabled due to FIPS mode, "
+ "consider using the VeNCrypt or SASL authentication "
+ "methods as an alternative\n");
+ goto fail;
}
password = 1; /* Require password auth */
} else if (strncmp(options, "reverse", 7) == 0) {
@@ -2913,18 +2910,14 @@ int vnc_display_open(DisplayState *ds, const char *display)
VNC_DEBUG("Trying certificate path '%s'\n", path);
if (vnc_tls_set_x509_creds_dir(vs, path) < 0) {
- fprintf(stderr, "Failed to find x509 certificates/keys in %s\n", path);
+ error_setg(errp, "Failed to find x509 certificates/keys in %s\n", path);
g_free(path);
- g_free(vs->display);
- vs->display = NULL;
- return -1;
+ goto fail;
}
g_free(path);
} else {
- fprintf(stderr, "No certificate path provided\n");
- g_free(vs->display);
- vs->display = NULL;
- return -1;
+ error_setg(errp, "No certificate path provided\n");
+ goto fail;
}
#endif
#if defined(CONFIG_VNC_TLS) || defined(CONFIG_VNC_SASL)
@@ -2943,10 +2936,8 @@ int vnc_display_open(DisplayState *ds, const char *display)
} else if (strncmp(options+6, "force-shared", 12) == 0) {
vs->share_policy = VNC_SHARE_POLICY_FORCE_SHARED;
} else {
- fprintf(stderr, "unknown vnc share= option\n");
- g_free(vs->display);
- vs->display = NULL;
- return -1;
+ error_setg(errp, "unknown vnc share= option\n");
+ goto fail;
}
}
}
@@ -3047,52 +3038,54 @@ int vnc_display_open(DisplayState *ds, const char *display)
#ifdef CONFIG_VNC_SASL
if ((saslErr = sasl_server_init(NULL, "qemu")) != SASL_OK) {
- fprintf(stderr, "Failed to initialize SASL auth %s",
- sasl_errstring(saslErr, NULL, NULL));
- g_free(vs->display);
- vs->display = NULL;
- return -1;
+ error_setg(errp, "Failed to initialize SASL auth %s",
+ sasl_errstring(saslErr, NULL, NULL));
+ goto fail;
}
#endif
vs->lock_key_sync = lock_key_sync;
if (reverse) {
/* connect to viewer */
- if (strncmp(display, "unix:", 5) == 0)
- vs->lsock = unix_connect(display+5, NULL);
- else
- vs->lsock = inet_connect(display, NULL);
- if (-1 == vs->lsock) {
- g_free(vs->display);
- vs->display = NULL;
- return -1;
+ int csock;
+ vs->lsock = -1;
+ if (strncmp(display, "unix:", 5) == 0) {
+ csock = unix_connect(display+5, errp);
} else {
- int csock = vs->lsock;
- vs->lsock = -1;
- vnc_connect(vs, csock, 0);
+ csock = inet_connect(display, errp);
}
- return 0;
-
+ if (-1 == csock) {
+ goto fail;
+ }
+ vnc_connect(vs, csock, 0);
} else {
/* listen for connects */
char *dpy;
dpy = g_malloc(256);
if (strncmp(display, "unix:", 5) == 0) {
pstrcpy(dpy, 256, "unix:");
- vs->lsock = unix_listen(display+5, dpy+5, 256-5, NULL);
+ vs->lsock = unix_listen(display+5, dpy+5, 256-5, errp);
} else {
vs->lsock = inet_listen(display, dpy, 256,
- SOCK_STREAM, 5900, NULL);
+ SOCK_STREAM, 5900, errp);
}
if (-1 == vs->lsock) {
g_free(dpy);
- return -1;
- } else {
- g_free(vs->display);
- vs->display = dpy;
+ goto fail;
}
+ g_free(vs->display);
+ vs->display = dpy;
+ qemu_set_fd_handler2(vs->lsock, NULL, vnc_listen_read, NULL, vs);
+ }
+ return;
+
+fail:
+ if (!error_is_set(errp)) {
+ error_set(errp, QERR_VNC_SERVER_FAILED, display);
}
- return qemu_set_fd_handler2(vs->lsock, NULL, vnc_listen_read, NULL, vs);
+ g_free(vs->display);
+ vs->display = NULL;
+ return;
}
void vnc_display_add_client(DisplayState *ds, int csock, int skipauth)
diff --git a/vl.c b/vl.c
index 2a072e8..67a624f 100644
--- a/vl.c
+++ b/vl.c
@@ -3706,10 +3706,13 @@ int main(int argc, char **argv, char **envp)
#ifdef CONFIG_VNC
/* init remote displays */
if (vnc_display) {
+ Error *local_err = NULL;
vnc_display_init(ds);
- if (vnc_display_open(ds, vnc_display) < 0) {
- fprintf(stderr, "Failed to start VNC server on `%s'\n",
- vnc_display);
+ vnc_display_open(ds, vnc_display, &local_err);
+ if (local_err != NULL) {
+ fprintf(stderr, "Failed to start VNC server on `%s': %s\n",
+ vnc_display, error_get_pretty(local_err));
+ error_free(local_err);
exit(1);
}
--
1.7.12.1
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [Qemu-devel] [PATCH 14/25] qemu-sockets: include strerror or gai_strerror output in error messages
2012-10-10 14:02 [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Paolo Bonzini
` (12 preceding siblings ...)
2012-10-10 14:02 ` [Qemu-devel] [PATCH 13/25] vnc: add error propagation to vnc_display_open Paolo Bonzini
@ 2012-10-10 14:02 ` Paolo Bonzini
2012-10-10 14:02 ` [Qemu-devel] [PATCH 15/25] qemu-sockets: add error propagation to inet_connect_addr Paolo Bonzini
` (12 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-10 14:02 UTC (permalink / raw)
To: qemu-devel; +Cc: lcapitulino
Among others, before:
$ qemu-system-x86_64 -chardev socket,port=12345,id=char
inet_connect: host and/or port not specified
chardev: opening backend "socket" failed
After:
$ x86_64-softmmu/qemu-system-x86_64 -chardev socket,port=12345,id=char
qemu-system-x86_64: -chardev socket,port=12345,id=char: host and/or port not specified
chardev: opening backend "socket" failed
perror and fprintf can be removed because all clients can now
consume Errors properly.
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
qemu-sockets.c | 30 +++++++++---------------------
1 file modificato, 9 inserzioni(+), 21 rimozioni(-)
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 6a49429..3c19463 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -120,8 +120,7 @@ int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp)
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_set(errp, QERR_SOCKET_CREATE_FAILED);
+ error_setg(errp, "host and/or port not specified");
return -1;
}
pstrcpy(port, sizeof(port), qemu_opt_get(opts, "port"));
@@ -138,9 +137,8 @@ int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp)
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_set(errp, QERR_SOCKET_CREATE_FAILED);
+ error_setg(errp, "address resolution failed for %s:%s: %s", addr, port,
+ gai_strerror(rc));
return -1;
}
@@ -151,10 +149,8 @@ int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp)
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));
if (!e->ai_next) {
- error_set(errp, QERR_SOCKET_CREATE_FAILED);
+ error_set_errno(errp, errno, QERR_SOCKET_CREATE_FAILED);
}
continue;
}
@@ -176,24 +172,19 @@ int inet_listen_opts(QemuOpts *opts, int port_offset, Error **errp)
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));
if (!e->ai_next) {
- error_set(errp, QERR_SOCKET_BIND_FAILED);
+ error_set_errno(errp, errno, QERR_SOCKET_BIND_FAILED);
}
}
}
closesocket(slisten);
}
- fprintf(stderr, "%s: FAILED\n", __FUNCTION__);
freeaddrinfo(res);
return -1;
listen:
if (listen(slisten,1) != 0) {
- error_set(errp, QERR_SOCKET_LISTEN_FAILED);
- perror("listen");
+ error_set_errno(errp, errno, QERR_SOCKET_LISTEN_FAILED);
closesocket(slisten);
freeaddrinfo(res);
return -1;
@@ -324,9 +315,7 @@ static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
addr = qemu_opt_get(opts, "host");
port = qemu_opt_get(opts, "port");
if (addr == NULL || port == NULL) {
- fprintf(stderr,
- "inet_parse_connect_opts: host and/or port not specified\n");
- error_set(errp, QERR_SOCKET_CREATE_FAILED);
+ error_setg(errp, "host and/or port not specified");
return NULL;
}
@@ -340,9 +329,8 @@ static struct addrinfo *inet_parse_connect_opts(QemuOpts *opts, Error **errp)
/* lookup */
rc = getaddrinfo(addr, port, &ai, &res);
if (rc != 0) {
- fprintf(stderr, "getaddrinfo(%s,%s): %s\n", addr, port,
- gai_strerror(rc));
- error_set(errp, QERR_SOCKET_CREATE_FAILED);
+ error_setg(errp, "address resolution failed for %s:%s: %s", addr, port,
+ gai_strerror(rc));
return NULL;
}
return res;
--
1.7.12.1
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [Qemu-devel] [PATCH 15/25] qemu-sockets: add error propagation to inet_connect_addr
2012-10-10 14:02 [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Paolo Bonzini
` (13 preceding siblings ...)
2012-10-10 14:02 ` [Qemu-devel] [PATCH 14/25] qemu-sockets: include strerror or gai_strerror output in error messages Paolo Bonzini
@ 2012-10-10 14:02 ` Paolo Bonzini
2012-10-16 12:53 ` Luiz Capitulino
2012-10-17 15:40 ` Markus Armbruster
2012-10-10 14:02 ` [Qemu-devel] [PATCH 16/25] qemu-sockets: add error propagation to inet_dgram_opts Paolo Bonzini
` (11 subsequent siblings)
26 siblings, 2 replies; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-10 14:02 UTC (permalink / raw)
To: qemu-devel; +Cc: lcapitulino
perror and fprintf can be removed because all clients can now consume
Errors properly. However, we need to change the non-blocking connect
handlers to take an Error, in order to improve error handling for
migration with the TCP protocol.
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
qemu-sockets.c | 15 ++++++---------
1 file modificato, 6 inserzioni(+), 9 rimozioni(-)
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 3c19463..d0e1a41 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -216,7 +216,7 @@ typedef struct ConnectState {
} ConnectState;
static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
- ConnectState *connect_state);
+ ConnectState *connect_state, Error **errp);
static void wait_for_connect(void *opaque)
{
@@ -246,7 +246,7 @@ static void wait_for_connect(void *opaque)
if (s->current_addr) {
while (s->current_addr->ai_next != NULL && s->fd < 0) {
s->current_addr = s->current_addr->ai_next;
- s->fd = inet_connect_addr(s->current_addr, &in_progress, s);
+ s->fd = inet_connect_addr(s->current_addr, &in_progress, s, NULL);
/* connect in progress */
if (in_progress) {
return;
@@ -263,7 +263,7 @@ static void wait_for_connect(void *opaque)
}
static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
- ConnectState *connect_state)
+ ConnectState *connect_state, Error **errp)
{
int sock, rc;
@@ -271,8 +271,7 @@ static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
sock = qemu_socket(addr->ai_family, addr->ai_socktype, addr->ai_protocol);
if (sock < 0) {
- fprintf(stderr, "%s: socket(%s): %s\n", __func__,
- inet_strfamily(addr->ai_family), strerror(errno));
+ error_set_errno(errp, errno, QERR_SOCKET_CREATE_FAILED);
return -1;
}
qemu_setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
@@ -293,6 +292,7 @@ static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
connect_state);
*in_progress = true;
} else if (rc < 0) {
+ error_set_errno(errp, errno, QERR_SOCKET_CONNECT_FAILED);
closesocket(sock);
return -1;
}
@@ -375,7 +375,7 @@ int inet_connect_opts(QemuOpts *opts, Error **errp,
if (connect_state != NULL) {
connect_state->current_addr = e;
}
- sock = inet_connect_addr(e, &in_progress, connect_state);
+ sock = inet_connect_addr(e, &in_progress, connect_state, errp);
if (in_progress) {
return sock;
} else if (sock >= 0) {
@@ -386,9 +386,6 @@ int inet_connect_opts(QemuOpts *opts, Error **errp,
break;
}
}
- if (sock < 0) {
- error_set(errp, QERR_SOCKET_CONNECT_FAILED);
- }
g_free(connect_state);
freeaddrinfo(res);
return sock;
--
1.7.12.1
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [Qemu-devel] [PATCH 16/25] qemu-sockets: add error propagation to inet_dgram_opts
2012-10-10 14:02 [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Paolo Bonzini
` (14 preceding siblings ...)
2012-10-10 14:02 ` [Qemu-devel] [PATCH 15/25] qemu-sockets: add error propagation to inet_connect_addr Paolo Bonzini
@ 2012-10-10 14:02 ` Paolo Bonzini
2012-10-10 14:02 ` [Qemu-devel] [PATCH 17/25] qemu-sockets: add error propagation to inet_parse Paolo Bonzini
` (10 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-10 14:02 UTC (permalink / raw)
To: qemu-devel; +Cc: lcapitulino
Before:
$ qemu-system-x86_64 -monitor udp:localhost:631@localhost:631
inet_dgram_opts: bind(ipv4,127.0.0.1,631): OK
inet_dgram_opts failed
chardev: opening backend "udp" failed
After:
$ x86_64-softmmu/qemu-system-x86_64 -monitor udp:localhost:631@localhost:631
qemu-system-x86_64: -monitor udp:localhost:631@localhost:631: Failed to bind socket: Address already in use
chardev: opening backend "udp" failed
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
qemu-char.c | 1 -
qemu-sockets.c | 34 ++++++++--------------------------
2 file modificati, 8 inserzioni(+), 27 rimozioni(-)
diff --git a/qemu-char.c b/qemu-char.c
index 04b5c23..afe2bfb 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2105,7 +2105,6 @@ static CharDriverState *qemu_chr_open_udp(QemuOpts *opts)
fd = inet_dgram_opts(opts, &local_err);
if (fd < 0) {
- fprintf(stderr, "inet_dgram_opts failed\n");
goto return_err;
}
diff --git a/qemu-sockets.c b/qemu-sockets.c
index d0e1a41..c639a39 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -396,8 +396,6 @@ int inet_dgram_opts(QemuOpts *opts, Error **errp)
struct addrinfo ai, *peer = NULL, *local = NULL;
const char *addr;
const char *port;
- char uaddr[INET6_ADDRSTRLEN+1];
- char uport[33];
int sock = -1, rc;
/* lookup peer addr */
@@ -412,7 +410,7 @@ int inet_dgram_opts(QemuOpts *opts, Error **errp)
addr = "localhost";
}
if (port == NULL || strlen(port) == 0) {
- fprintf(stderr, "inet_dgram: port not specified\n");
+ error_setg(errp, "remote port not specified");
return -1;
}
@@ -422,8 +420,8 @@ int inet_dgram_opts(QemuOpts *opts, Error **errp)
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_setg(errp, "address resolution failed for %s:%s: %s", addr, port,
+ gai_strerror(rc));
return -1;
}
@@ -442,44 +440,28 @@ int inet_dgram_opts(QemuOpts *opts, Error **errp)
port = "0";
if (0 != (rc = getaddrinfo(addr, port, &ai, &local))) {
- fprintf(stderr,"getaddrinfo(%s,%s): %s\n", addr, port,
- gai_strerror(rc));
+ error_setg(errp, "address resolution failed for %s:%s: %s", addr, port,
+ gai_strerror(rc));
goto err;
}
/* 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));
+ error_set_errno(errp, errno, QERR_SOCKET_CREATE_FAILED);
goto err;
}
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));
+ error_set_errno(errp, errno, QERR_SOCKET_BIND_FAILED);
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_set_errno(errp, errno, QERR_SOCKET_CONNECT_FAILED);
goto err;
}
--
1.7.12.1
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [Qemu-devel] [PATCH 17/25] qemu-sockets: add error propagation to inet_parse
2012-10-10 14:02 [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Paolo Bonzini
` (15 preceding siblings ...)
2012-10-10 14:02 ` [Qemu-devel] [PATCH 16/25] qemu-sockets: add error propagation to inet_dgram_opts Paolo Bonzini
@ 2012-10-10 14:02 ` Paolo Bonzini
2012-10-10 14:02 ` [Qemu-devel] [PATCH 18/25] qemu-sockets: add error propagation to Unix socket functions Paolo Bonzini
` (9 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-10 14:02 UTC (permalink / raw)
To: qemu-devel; +Cc: lcapitulino
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
qemu-sockets.c | 41 +++++++++++++++++++++--------------------
1 file modificato, 21 inserzioni(+), 20 rimozioni(-)
diff --git a/qemu-sockets.c b/qemu-sockets.c
index c639a39..39d07c0 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -480,7 +480,7 @@ err:
}
/* compatibility wrapper */
-static int inet_parse(QemuOpts *opts, const char *str)
+static void inet_parse(QemuOpts *opts, const char *str, Error **errp)
{
const char *optstr, *h;
char addr[64];
@@ -492,32 +492,28 @@ static int inet_parse(QemuOpts *opts, const char *str)
/* no host given */
addr[0] = '\0';
if (1 != sscanf(str,":%32[^,]%n",port,&pos)) {
- fprintf(stderr, "%s: portonly parse error (%s)\n",
- __FUNCTION__, str);
- return -1;
+ error_setg(errp, "error parsing port in address '%s'", str);
+ return;
}
} else if (str[0] == '[') {
/* IPv6 addr */
if (2 != sscanf(str,"[%64[^]]]:%32[^,]%n",addr,port,&pos)) {
- fprintf(stderr, "%s: ipv6 parse error (%s)\n",
- __FUNCTION__, str);
- return -1;
+ error_setg(errp, "error parsing IPv6 address '%s'", str);
+ return;
}
qemu_opt_set(opts, "ipv6", "on");
} else if (qemu_isdigit(str[0])) {
/* IPv4 addr */
if (2 != sscanf(str,"%64[0-9.]:%32[^,]%n",addr,port,&pos)) {
- fprintf(stderr, "%s: ipv4 parse error (%s)\n",
- __FUNCTION__, str);
- return -1;
+ error_setg(errp, "error parsing IPv4 address '%s'", str);
+ return;
}
qemu_opt_set(opts, "ipv4", "on");
} else {
/* hostname */
if (2 != sscanf(str,"%64[^:]:%32[^,]%n",addr,port,&pos)) {
- fprintf(stderr, "%s: hostname parse error (%s)\n",
- __FUNCTION__, str);
- return -1;
+ error_setg(errp, "error parsing address '%s'", str);
+ return;
}
}
qemu_opt_set(opts, "host", addr);
@@ -532,7 +528,6 @@ static int inet_parse(QemuOpts *opts, const char *str)
qemu_opt_set(opts, "ipv4", "on");
if (strstr(optstr, ",ipv6"))
qemu_opt_set(opts, "ipv6", "on");
- return 0;
}
int inet_listen(const char *str, char *ostr, int olen,
@@ -541,9 +536,11 @@ int inet_listen(const char *str, char *ostr, int olen,
QemuOpts *opts;
char *optstr;
int sock = -1;
+ Error *local_err = NULL;
opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
- if (inet_parse(opts, str) == 0) {
+ inet_parse(opts, str, &local_err);
+ if (local_err == NULL) {
sock = inet_listen_opts(opts, port_offset, errp);
if (sock != -1 && ostr) {
optstr = strchr(str, ',');
@@ -560,7 +557,7 @@ int inet_listen(const char *str, char *ostr, int olen,
}
}
} else {
- error_set(errp, QERR_SOCKET_CREATE_FAILED);
+ error_propagate(errp, local_err);
}
qemu_opts_del(opts);
return sock;
@@ -578,12 +575,14 @@ int inet_connect(const char *str, Error **errp)
{
QemuOpts *opts;
int sock = -1;
+ Error *local_err = NULL;
opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
- if (inet_parse(opts, str) == 0) {
+ inet_parse(opts, str, &local_err);
+ if (local_err == NULL) {
sock = inet_connect_opts(opts, errp, NULL, NULL);
} else {
- error_set(errp, QERR_SOCKET_CREATE_FAILED);
+ error_propagate(errp, local_err);
}
qemu_opts_del(opts);
return sock;
@@ -608,14 +607,16 @@ int inet_nonblocking_connect(const char *str,
{
QemuOpts *opts;
int sock = -1;
+ Error *local_err = NULL;
g_assert(callback != NULL);
opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
- if (inet_parse(opts, str) == 0) {
+ inet_parse(opts, str, &local_err);
+ if (local_err == NULL) {
sock = inet_connect_opts(opts, errp, callback, opaque);
} else {
- error_set(errp, QERR_SOCKET_CREATE_FAILED);
+ error_propagate(errp, local_err);
}
qemu_opts_del(opts);
return sock;
--
1.7.12.1
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [Qemu-devel] [PATCH 18/25] qemu-sockets: add error propagation to Unix socket functions
2012-10-10 14:02 [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Paolo Bonzini
` (16 preceding siblings ...)
2012-10-10 14:02 ` [Qemu-devel] [PATCH 17/25] qemu-sockets: add error propagation to inet_parse Paolo Bonzini
@ 2012-10-10 14:02 ` Paolo Bonzini
2012-10-10 14:03 ` [Qemu-devel] [PATCH 19/25] build: add QAPI files to the tools Paolo Bonzini
` (8 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-10 14:02 UTC (permalink / raw)
To: qemu-devel; +Cc: lcapitulino
Before:
$ qemu-system-x86_64 -monitor unix:/vvv,server=off
connect(unix:/vvv): No such file or directory
chardev: opening backend "socket" failed
After:
$ x86_64-softmmu/qemu-system-x86_64 -monitor unix:/vvv,server=off
qemu-system-x86_64: -monitor unix:/vvv,server=off: Failed to connect to socket: No such file or directory
chardev: opening backend "socket" failed
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
qemu-sockets.c | 12 ++++++------
1 file modificato, 6 inserzioni(+), 6 rimozioni(-)
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 39d07c0..a37f35d 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -632,7 +632,7 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)
sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
if (sock < 0) {
- perror("socket(unix)");
+ error_set_errno(errp, errno, QERR_SOCKET_CREATE_FAILED);
return -1;
}
@@ -657,11 +657,11 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)
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));
+ error_set_errno(errp, errno, QERR_SOCKET_BIND_FAILED);
goto err;
}
if (listen(sock, 1) < 0) {
- fprintf(stderr, "listen(unix:%s): %s\n", un.sun_path, strerror(errno));
+ error_set_errno(errp, errno, QERR_SOCKET_LISTEN_FAILED);
goto err;
}
@@ -681,13 +681,13 @@ int unix_connect_opts(QemuOpts *opts, Error **errp,
int sock, rc;
if (NULL == path) {
- fprintf(stderr, "unix connect: no path specified\n");
+ error_setg(errp, "unix connect: no path specified\n");
return -1;
}
sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
if (sock < 0) {
- perror("socket(unix)");
+ error_set_errno(errp, errno, QERR_SOCKET_CREATE_FAILED);
return -1;
}
if (callback != NULL) {
@@ -722,7 +722,7 @@ int unix_connect_opts(QemuOpts *opts, Error **errp,
}
if (rc < 0) {
- fprintf(stderr, "connect(unix:%s): %s\n", path, strerror(errno));
+ error_set_errno(errp, -rc, QERR_SOCKET_CONNECT_FAILED);
close(sock);
sock = -1;
}
--
1.7.12.1
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [Qemu-devel] [PATCH 19/25] build: add QAPI files to the tools
2012-10-10 14:02 [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Paolo Bonzini
` (17 preceding siblings ...)
2012-10-10 14:02 ` [Qemu-devel] [PATCH 18/25] qemu-sockets: add error propagation to Unix socket functions Paolo Bonzini
@ 2012-10-10 14:03 ` Paolo Bonzini
2012-10-10 14:03 ` [Qemu-devel] [PATCH 20/25] qapi: add socket address types Paolo Bonzini
` (7 subsequent siblings)
26 siblings, 0 replies; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-10 14:03 UTC (permalink / raw)
To: qemu-devel; +Cc: lcapitulino
We need them because qemu-sockets will soon be using SocketAddress.
Acked-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
Makefile.objs | 3 ++-
1 file modificato, 2 inserzioni(+). 1 rimozione(-)
diff --git a/Makefile.objs b/Makefile.objs
index 74b3542..3f16d67 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -48,6 +48,7 @@ block-obj-y += $(coroutine-obj-y) $(qobject-obj-y) $(version-obj-y)
block-obj-$(CONFIG_POSIX) += posix-aio-compat.o
block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
block-obj-y += block/
+block-obj-y += $(qapi-obj-y) qapi-types.o qapi-visit.o
ifeq ($(CONFIG_VIRTIO)$(CONFIG_VIRTFS)$(CONFIG_PCI),yyy)
# Lots of the fsdev/9pcode is pulled in by vl.c via qemu_fsdev_add.
@@ -239,9 +240,9 @@ QEMU_CFLAGS+=$(GLIB_CFLAGS)
nested-vars += \
qga-obj-y \
- block-obj-y \
qom-obj-y \
qapi-obj-y \
+ block-obj-y \
user-obj-y \
common-obj-y \
extra-obj-y
--
1.7.12.1
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [Qemu-devel] [PATCH 20/25] qapi: add socket address types
2012-10-10 14:02 [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Paolo Bonzini
` (18 preceding siblings ...)
2012-10-10 14:03 ` [Qemu-devel] [PATCH 19/25] build: add QAPI files to the tools Paolo Bonzini
@ 2012-10-10 14:03 ` Paolo Bonzini
2012-10-17 16:43 ` Markus Armbruster
2012-10-10 14:03 ` [Qemu-devel] [PATCH 21/25] qemu-sockets: return IPSocketAddress from inet_parse Paolo Bonzini
` (6 subsequent siblings)
26 siblings, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-10 14:03 UTC (permalink / raw)
To: qemu-devel; +Cc: lcapitulino
Acked-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
qapi-schema.json | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file modificato, 53 inserzioni(+)
diff --git a/qapi-schema.json b/qapi-schema.json
index f9dbdae..d40b5fc 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2505,6 +2505,59 @@
'opts': 'NetClientOptions' } }
##
+# @IPSocketAddress
+#
+# Captures the destination address of an IP socket
+#
+# @host: host part of the address
+#
+# @port: port part of the address, or lowest port if @to is present
+#
+# @to: highest port to try
+#
+# @ipv4: whether to accept IPv4 addresses, default try both IPv4 and IPv6
+# #optional
+#
+# @ipv6: whether to accept IPv6 addresses, default try both IPv4 and IPv6
+# #optional
+#
+# Since 1.3
+##
+{ 'type': 'IPSocketAddress',
+ 'data': {
+ 'host': 'str',
+ 'port': 'str',
+ '*to': 'uint16',
+ '*ipv4': 'bool',
+ '*ipv6': 'bool' } }
+
+##
+# @UnixSocketAddress
+#
+# Captures the destination address of a Unix socket
+#
+# @path: filesystem path to use
+#
+# Since 1.3
+##
+{ 'type': 'UnixSocketAddress',
+ 'data': {
+ 'path': 'str' } }
+
+##
+# @SocketAddress
+#
+# Captures the address of a socket, which could also be a named file descriptor
+#
+# Since 1.3
+##
+{ 'union': 'SocketAddress',
+ 'data': {
+ 'inet': 'IPSocketAddress',
+ 'unix': 'UnixSocketAddress',
+ 'fd': 'String' } }
+
+##
# @getfd:
#
# Receive a file descriptor via SCM rights and assign it a name
--
1.7.12.1
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [Qemu-devel] [PATCH 21/25] qemu-sockets: return IPSocketAddress from inet_parse
2012-10-10 14:02 [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Paolo Bonzini
` (19 preceding siblings ...)
2012-10-10 14:03 ` [Qemu-devel] [PATCH 20/25] qapi: add socket address types Paolo Bonzini
@ 2012-10-10 14:03 ` Paolo Bonzini
2012-10-17 16:49 ` Markus Armbruster
2012-10-10 14:03 ` [Qemu-devel] [PATCH 22/25] qemu-sockets: add socket_listen, socket_connect, socket_parse Paolo Bonzini
` (5 subsequent siblings)
26 siblings, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-10 14:03 UTC (permalink / raw)
To: qemu-devel; +Cc: lcapitulino
Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
qemu-sockets.c | 119 +++++++++++++++++++++++++++++++++++++--------------------
1 file modificato, 78 inserzioni(+), 41 rimozioni(-)
diff --git a/qemu-sockets.c b/qemu-sockets.c
index a37f35d..5946962 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -480,54 +480,91 @@ err:
}
/* compatibility wrapper */
-static void inet_parse(QemuOpts *opts, const char *str, Error **errp)
+static IPSocketAddress *inet_parse(const char *str, Error **errp)
{
+ IPSocketAddress *addr;
const char *optstr, *h;
- char addr[64];
+ char host[64];
char port[33];
+ int to;
int pos;
+ addr = g_new(IPSocketAddress, 1);
+
/* parse address */
if (str[0] == ':') {
/* no host given */
- addr[0] = '\0';
+ host[0] = '\0';
if (1 != sscanf(str,":%32[^,]%n",port,&pos)) {
error_setg(errp, "error parsing port in address '%s'", str);
- return;
+ goto fail;
}
} else if (str[0] == '[') {
/* IPv6 addr */
- if (2 != sscanf(str,"[%64[^]]]:%32[^,]%n",addr,port,&pos)) {
+ if (2 != sscanf(str,"[%64[^]]]:%32[^,]%n",host,port,&pos)) {
error_setg(errp, "error parsing IPv6 address '%s'", str);
- return;
+ goto fail;
}
- qemu_opt_set(opts, "ipv6", "on");
+ addr->ipv6 = addr->has_ipv6 = true;
} else if (qemu_isdigit(str[0])) {
/* IPv4 addr */
- if (2 != sscanf(str,"%64[0-9.]:%32[^,]%n",addr,port,&pos)) {
+ if (2 != sscanf(str,"%64[0-9.]:%32[^,]%n",host,port,&pos)) {
error_setg(errp, "error parsing IPv4 address '%s'", str);
- return;
+ goto fail;
}
- qemu_opt_set(opts, "ipv4", "on");
+ addr->ipv4 = addr->has_ipv4 = true;
} else {
/* hostname */
- if (2 != sscanf(str,"%64[^:]:%32[^,]%n",addr,port,&pos)) {
+ if (2 != sscanf(str,"%64[^:]:%32[^,]%n",host,port,&pos)) {
error_setg(errp, "error parsing address '%s'", str);
- return;
+ goto fail;
}
}
- qemu_opt_set(opts, "host", addr);
- qemu_opt_set(opts, "port", port);
+
+ addr->host = g_strdup(host);
+ addr->port = g_strdup(port);
/* parse options */
optstr = str + pos;
h = strstr(optstr, ",to=");
- if (h)
- qemu_opt_set(opts, "to", h+4);
- if (strstr(optstr, ",ipv4"))
- qemu_opt_set(opts, "ipv4", "on");
- if (strstr(optstr, ",ipv6"))
- qemu_opt_set(opts, "ipv6", "on");
+ if (h) {
+ if (1 != sscanf(str, "%d%n", &to, &pos) ||
+ (str[pos] != '\0' && str[pos] != ',')) {
+ error_setg(errp, "error parsing to= argument");
+ goto fail;
+ }
+ addr->has_to = true;
+ addr->to = to;
+ }
+ if (strstr(optstr, ",ipv4")) {
+ addr->ipv4 = addr->has_ipv4 = true;
+ }
+ if (strstr(optstr, ",ipv6")) {
+ addr->ipv6 = addr->has_ipv6 = true;
+ }
+ return addr;
+
+fail:
+ qapi_free_IPSocketAddress(addr);
+ return NULL;
+}
+
+static void inet_addr_to_opts(QemuOpts *opts, IPSocketAddress *addr)
+{
+ bool ipv4 = addr->ipv4 || !addr->has_ipv4;
+ bool ipv6 = addr->ipv6 || !addr->has_ipv6;
+
+ if (!ipv4 || !ipv6) {
+ qemu_opt_set_bool(opts, "ipv4", ipv4);
+ qemu_opt_set_bool(opts, "ipv6", ipv6);
+ }
+ if (addr->has_to) {
+ char to[20];
+ snprintf(to, sizeof(to), "%d", addr->to);
+ qemu_opt_set(opts, "to", to);
+ }
+ qemu_opt_set(opts, "host", addr->host);
+ qemu_opt_set(opts, "port", addr->port);
}
int inet_listen(const char *str, char *ostr, int olen,
@@ -536,11 +573,13 @@ int inet_listen(const char *str, char *ostr, int olen,
QemuOpts *opts;
char *optstr;
int sock = -1;
- Error *local_err = NULL;
+ IPSocketAddress *addr;
- opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
- inet_parse(opts, str, &local_err);
- if (local_err == NULL) {
+ addr = inet_parse(str, errp);
+ if (addr != NULL) {
+ opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
+ inet_addr_to_opts(opts, addr);
+ qapi_free_IPSocketAddress(addr);
sock = inet_listen_opts(opts, port_offset, errp);
if (sock != -1 && ostr) {
optstr = strchr(str, ',');
@@ -556,10 +595,8 @@ int inet_listen(const char *str, char *ostr, int olen,
optstr ? optstr : "");
}
}
- } else {
- error_propagate(errp, local_err);
+ qemu_opts_del(opts);
}
- qemu_opts_del(opts);
return sock;
}
@@ -575,16 +612,16 @@ int inet_connect(const char *str, Error **errp)
{
QemuOpts *opts;
int sock = -1;
- Error *local_err = NULL;
+ IPSocketAddress *addr;
- opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
- inet_parse(opts, str, &local_err);
- if (local_err == NULL) {
+ addr = inet_parse(str, errp);
+ if (addr != NULL) {
+ opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
+ inet_addr_to_opts(opts, addr);
+ qapi_free_IPSocketAddress(addr);
sock = inet_connect_opts(opts, errp, NULL, NULL);
- } else {
- error_propagate(errp, local_err);
+ qemu_opts_del(opts);
}
- qemu_opts_del(opts);
return sock;
}
@@ -607,18 +644,18 @@ int inet_nonblocking_connect(const char *str,
{
QemuOpts *opts;
int sock = -1;
- Error *local_err = NULL;
+ IPSocketAddress *addr;
g_assert(callback != NULL);
- opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
- inet_parse(opts, str, &local_err);
- if (local_err == NULL) {
+ addr = inet_parse(str, errp);
+ if (addr != NULL) {
+ opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
+ inet_addr_to_opts(opts, addr);
+ qapi_free_IPSocketAddress(addr);
sock = inet_connect_opts(opts, errp, callback, opaque);
- } else {
- error_propagate(errp, local_err);
+ qemu_opts_del(opts);
}
- qemu_opts_del(opts);
return sock;
}
--
1.7.12.1
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [Qemu-devel] [PATCH 22/25] qemu-sockets: add socket_listen, socket_connect, socket_parse
2012-10-10 14:02 [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Paolo Bonzini
` (20 preceding siblings ...)
2012-10-10 14:03 ` [Qemu-devel] [PATCH 21/25] qemu-sockets: return IPSocketAddress from inet_parse Paolo Bonzini
@ 2012-10-10 14:03 ` Paolo Bonzini
2012-10-19 8:15 ` Markus Armbruster
2012-10-10 14:03 ` [Qemu-devel] [PATCH 23/25] block: add close notifiers Paolo Bonzini
` (4 subsequent siblings)
26 siblings, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-10 14:03 UTC (permalink / raw)
To: qemu-devel; +Cc: lcapitulino
These are QAPI-friendly versions of the qemu-sockets functions. They
support IP sockets, Unix sockets, and named file descriptors, using a
QAPI union to dispatch to the correct function.
Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
Makefile | 2 +-
qemu-sockets.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
qemu-tool.c | 6 ++++
qemu_socket.h | 5 +++
4 file modificati, 111 inserzioni(+). 1 rimozione(-)
diff --git a/Makefile b/Makefile
index a9c22bf..b8b1f3c 100644
--- a/Makefile
+++ b/Makefile
@@ -159,7 +159,7 @@ qemu-img.o: qemu-img-cmds.h
tools-obj-y = $(oslib-obj-y) $(trace-obj-y) qemu-tool.o qemu-timer.o \
qemu-timer-common.o main-loop.o notify.o \
- iohandler.o cutils.o iov.o async.o
+ iohandler.o cutils.o iov.o async.o error.o
tools-obj-$(CONFIG_POSIX) += compatfd.o
qemu-img$(EXESUF): qemu-img.o $(tools-obj-y) $(block-obj-y) $(qapi-obj-y) \
diff --git a/qemu-sockets.c b/qemu-sockets.c
index 5946962..e8d0a3c 100644
--- a/qemu-sockets.c
+++ b/qemu-sockets.c
@@ -22,6 +22,7 @@
#include <errno.h>
#include <unistd.h>
+#include "monitor.h"
#include "qemu_socket.h"
#include "qemu-common.h" /* for qemu_isdigit */
#include "main-loop.h"
@@ -845,6 +846,104 @@ int unix_nonblocking_connect(const char *path,
return sock;
}
+SocketAddress *socket_parse(const char *str, Error **errp)
+{
+ SocketAddress *addr = NULL;
+
+ addr = g_new(SocketAddress, 1);
+ if (strstart(str, "unix:", NULL)) {
+ if (str[5] == '\0') {
+ error_setg(errp, "invalid Unix socket address\n");
+ goto fail;
+ } else {
+ addr->kind = SOCKET_ADDRESS_KIND_UNIX;
+ addr->q_unix = g_new(UnixSocketAddress, 1);
+ addr->q_unix->path = g_strdup(str + 5);
+ }
+ } else if (strstart(str, "fd:", NULL)) {
+ if (str[3] == '\0') {
+ error_setg(errp, "invalid file descriptor address\n");
+ goto fail;
+ } else {
+ addr->kind = SOCKET_ADDRESS_KIND_FD;
+ addr->fd = g_new(String, 1);
+ addr->fd->str = g_strdup(str + 3);
+ }
+ } else {
+ addr->kind = SOCKET_ADDRESS_KIND_INET;
+ addr->inet = g_new(IPSocketAddress, 1);
+ addr->inet = inet_parse(str, errp);
+ if (addr->inet == NULL) {
+ goto fail;
+ }
+ }
+ return addr;
+
+fail:
+ qapi_free_SocketAddress(addr);
+ return NULL;
+}
+
+int socket_connect(SocketAddress *addr, Error **errp,
+ NonBlockingConnectHandler *callback, void *opaque)
+{
+ QemuOpts *opts;
+ int fd;
+
+ opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
+ switch (addr->kind) {
+ case SOCKET_ADDRESS_KIND_INET:
+ inet_addr_to_opts(opts, addr->inet);
+ fd = inet_connect_opts(opts, errp, callback, opaque);
+ break;
+
+ case SOCKET_ADDRESS_KIND_UNIX:
+ qemu_opt_set(opts, "path", addr->q_unix->path);
+ fd = unix_connect_opts(opts, errp, callback, opaque);
+ break;
+
+ case SOCKET_ADDRESS_KIND_FD:
+ fd = monitor_get_fd(cur_mon, addr->fd->str, errp);
+ if (callback) {
+ callback(fd, opaque);
+ }
+ break;
+
+ default:
+ abort();
+ }
+ qemu_opts_del(opts);
+ return fd;
+}
+
+int socket_listen(SocketAddress *addr, Error **errp)
+{
+ QemuOpts *opts;
+ int fd;
+
+ opts = qemu_opts_create(&dummy_opts, NULL, 0, NULL);
+ switch (addr->kind) {
+ case SOCKET_ADDRESS_KIND_INET:
+ inet_addr_to_opts(opts, addr->inet);
+ fd = inet_listen_opts(opts, 0, errp);
+ break;
+
+ case SOCKET_ADDRESS_KIND_UNIX:
+ qemu_opt_set(opts, "path", addr->q_unix->path);
+ fd = unix_listen_opts(opts, errp);
+ break;
+
+ case SOCKET_ADDRESS_KIND_FD:
+ fd = monitor_get_fd(cur_mon, addr->fd->str, errp);
+ break;
+
+ default:
+ abort();
+ }
+ qemu_opts_del(opts);
+ return fd;
+}
+
#ifdef _WIN32
static void socket_cleanup(void)
{
diff --git a/qemu-tool.c b/qemu-tool.c
index f2f9813..6fe1dd2 100644
--- a/qemu-tool.c
+++ b/qemu-tool.c
@@ -48,6 +48,12 @@ int monitor_cur_is_qmp(void)
return 0;
}
+int monitor_get_fd(Monitor *mon, const char *name, Error **errp)
+{
+ error_setg(errp, "only QEMU supports file descriptor passing");
+ return -1;
+}
+
void monitor_set_error(Monitor *mon, QError *qerror)
{
}
diff --git a/qemu_socket.h b/qemu_socket.h
index 89a5feb..02490ad 100644
--- a/qemu_socket.h
+++ b/qemu_socket.h
@@ -65,6 +65,11 @@ int unix_nonblocking_connect(const char *str,
NonBlockingConnectHandler *callback,
void *opaque, Error **errp);
+SocketAddress *socket_parse(const char *str, Error **errp);
+int socket_connect(SocketAddress *addr, Error **errp,
+ NonBlockingConnectHandler *callback, void *opaque);
+int socket_listen(SocketAddress *addr, Error **errp);
+
/* Old, ipv4 only bits. Don't use for new code. */
int parse_host_port(struct sockaddr_in *saddr, const char *str);
int socket_init(void);
--
1.7.12.1
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [Qemu-devel] [PATCH 23/25] block: add close notifiers
2012-10-10 14:02 [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Paolo Bonzini
` (21 preceding siblings ...)
2012-10-10 14:03 ` [Qemu-devel] [PATCH 22/25] qemu-sockets: add socket_listen, socket_connect, socket_parse Paolo Bonzini
@ 2012-10-10 14:03 ` Paolo Bonzini
2012-10-19 8:21 ` Markus Armbruster
2012-10-19 8:26 ` Markus Armbruster
2012-10-10 14:03 ` [Qemu-devel] [PATCH 24/25] qmp: add NBD server commands Paolo Bonzini
` (3 subsequent siblings)
26 siblings, 2 replies; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-10 14:03 UTC (permalink / raw)
To: qemu-devel; +Cc: lcapitulino
The first user of close notifiers will be the embedded NBD server.
It is possible to use them to do some of the ad hoc processing
(e.g. for block jobs and I/O limits) that is currently done by
bdrv_close.
Acked-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
Makefile.objs | 4 ++--
block.c | 19 ++++++++++++++-----
block.h | 1 +
block_int.h | 2 ++
4 file modificati, 19 inserzioni(+), 7 rimozioni(-)
diff --git a/Makefile.objs b/Makefile.objs
index 3f16d67..ca67885 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -43,7 +43,7 @@ coroutine-obj-$(CONFIG_WIN32) += coroutine-win32.o
block-obj-y = cutils.o iov.o cache-utils.o qemu-option.o module.o async.o
block-obj-y += nbd.o block.o blockjob.o aio.o aes.o qemu-config.o
-block-obj-y += qemu-progress.o qemu-sockets.o uri.o
+block-obj-y += qemu-progress.o qemu-sockets.o uri.o notify.o
block-obj-y += $(coroutine-obj-y) $(qobject-obj-y) $(version-obj-y)
block-obj-$(CONFIG_POSIX) += posix-aio-compat.o
block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
@@ -94,7 +94,7 @@ common-obj-y += bt-host.o bt-vhci.o
common-obj-y += dma-helpers.o
common-obj-y += iov.o acl.o
common-obj-$(CONFIG_POSIX) += compatfd.o
-common-obj-y += notify.o event_notifier.o
+common-obj-y += event_notifier.o
common-obj-y += qemu-timer.o qemu-timer-common.o
common-obj-y += qtest.o
common-obj-y += vl.o
diff --git a/block.c b/block.c
index e95f613..56426a9 100644
--- a/block.c
+++ b/block.c
@@ -30,6 +30,7 @@
#include "module.h"
#include "qjson.h"
#include "sysemu.h"
+#include "notify.h"
#include "qemu-coroutine.h"
#include "qmp-commands.h"
#include "qemu-timer.h"
@@ -312,9 +313,16 @@ BlockDriverState *bdrv_new(const char *device_name)
QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
}
bdrv_iostatus_disable(bs);
+ notifier_list_init(&bs->close_notifiers);
+
return bs;
}
+void bdrv_add_close_notifier(BlockDriverState *bs, Notifier *notify)
+{
+ notifier_list_add(&bs->close_notifiers, notify);
+}
+
BlockDriver *bdrv_find_format(const char *format_name)
{
BlockDriver *drv1;
@@ -1098,12 +1106,13 @@ void bdrv_reopen_abort(BDRVReopenState *reopen_state)
void bdrv_close(BlockDriverState *bs)
{
bdrv_flush(bs);
- if (bs->drv) {
- if (bs->job) {
- block_job_cancel_sync(bs->job);
- }
- bdrv_drain_all();
+ if (bs->job) {
+ block_job_cancel_sync(bs->job);
+ }
+ bdrv_drain_all();
+ notifier_list_notify(&bs->close_notifiers, bs);
+ if (bs->drv) {
if (bs == bs_snapshots) {
bs_snapshots = NULL;
}
diff --git a/block.h b/block.h
index e2d89d7..aa608a8 100644
--- a/block.h
+++ b/block.h
@@ -144,6 +144,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
void bdrv_reopen_commit(BDRVReopenState *reopen_state);
void bdrv_reopen_abort(BDRVReopenState *reopen_state);
void bdrv_close(BlockDriverState *bs);
+void bdrv_add_close_notifier(BlockDriverState *bs, Notifier *notify);
int bdrv_attach_dev(BlockDriverState *bs, void *dev);
void bdrv_attach_dev_nofail(BlockDriverState *bs, void *dev);
void bdrv_detach_dev(BlockDriverState *bs, void *dev);
diff --git a/block_int.h b/block_int.h
index f4bae04..cedabbd 100644
--- a/block_int.h
+++ b/block_int.h
@@ -233,6 +233,8 @@ struct BlockDriverState {
BlockDriverState *backing_hd;
BlockDriverState *file;
+ NotifierList close_notifiers;
+
/* number of in-flight copy-on-read requests */
unsigned int copy_on_read_in_flight;
--
1.7.12.1
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [Qemu-devel] [PATCH 24/25] qmp: add NBD server commands
2012-10-10 14:02 [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Paolo Bonzini
` (22 preceding siblings ...)
2012-10-10 14:03 ` [Qemu-devel] [PATCH 23/25] block: add close notifiers Paolo Bonzini
@ 2012-10-10 14:03 ` Paolo Bonzini
2012-10-10 20:41 ` Eric Blake
2012-10-19 8:31 ` Markus Armbruster
2012-10-10 14:03 ` [Qemu-devel] [PATCH 25/25] hmp: " Paolo Bonzini
` (2 subsequent siblings)
26 siblings, 2 replies; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-10 14:03 UTC (permalink / raw)
To: qemu-devel; +Cc: lcapitulino
Adding an NBD server inside QEMU is trivial, since all the logic is
in nbd.c and can be shared easily between qemu-nbd and QEMU itself.
The main difference is that qemu-nbd serves a single unnamed export,
while QEMU serves named exports.
Acked-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
Makefile.objs | 2 +-
blockdev-nbd.c | 119 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
qapi-schema.json | 43 ++++++++++++++++++++
qmp-commands.hx | 16 ++++++++
4 file modificati, 179 inserzioni(+). 1 rimozione(-)
create mode 100644 blockdev-nbd.c
diff --git a/Makefile.objs b/Makefile.objs
index ca67885..9eca179 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -61,7 +61,7 @@ endif
# suppress *all* target specific code in case of system emulation, i.e. a
# single QEMU executable should support all CPUs and machines.
-common-obj-y = $(block-obj-y) blockdev.o block/
+common-obj-y = $(block-obj-y) blockdev.o blockdev-nbd.o block/
common-obj-y += net.o net/
common-obj-y += qom/
common-obj-y += readline.o console.o cursor.o
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
new file mode 100644
index 0000000..8031813
--- /dev/null
+++ b/blockdev-nbd.c
@@ -0,0 +1,119 @@
+/*
+ * Serving QEMU block devices via NBD
+ *
+ * Copyright (c) 2012 Red Hat, Inc.
+ *
+ * Author: Paolo Bonzini <pbonzini@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * later. See the COPYING file in the top-level directory.
+ */
+
+#include "blockdev.h"
+#include "hw/block-common.h"
+#include "monitor.h"
+#include "qerror.h"
+#include "sysemu.h"
+#include "qmp-commands.h"
+#include "trace.h"
+#include "nbd.h"
+#include "qemu_socket.h"
+
+static int server_fd = -1;
+
+static void nbd_accept(void *opaque)
+{
+ struct sockaddr_in addr;
+ socklen_t addr_len = sizeof(addr);
+
+ int fd = accept(server_fd, (struct sockaddr *)&addr, &addr_len);
+ if (fd >= 0) {
+ nbd_client_new(NULL, fd, nbd_client_put);
+ }
+}
+
+void qmp_nbd_server_start(SocketAddress *addr, Error **errp)
+{
+ if (server_fd != -1) {
+ error_setg(errp, "NBD server already running");
+ return;
+ }
+
+ server_fd = socket_listen(addr, errp);
+ if (server_fd != -1) {
+ qemu_set_fd_handler2(server_fd, NULL, nbd_accept, NULL, NULL);
+ }
+}
+
+/* Hook into the BlockDriverState notifiers to close the export when
+ * the file is closed.
+ */
+typedef struct NBDCloseNotifier {
+ Notifier n;
+ NBDExport *exp;
+ QTAILQ_ENTRY(NBDCloseNotifier) next;
+} NBDCloseNotifier;
+
+static QTAILQ_HEAD(, NBDCloseNotifier) close_notifiers =
+ QTAILQ_HEAD_INITIALIZER(close_notifiers);
+
+static void nbd_close_notifier(Notifier *n, void *data)
+{
+ NBDCloseNotifier *cn = DO_UPCAST(NBDCloseNotifier, n, n);
+
+ notifier_remove(&cn->n);
+ QTAILQ_REMOVE(&close_notifiers, cn, next);
+
+ nbd_export_close(cn->exp);
+ nbd_export_put(cn->exp);
+ g_free(cn);
+}
+
+static void nbd_server_put_ref(NBDExport *exp)
+{
+ BlockDriverState *bs = nbd_export_get_blockdev(exp);
+ drive_put_ref(drive_get_by_blockdev(bs));
+}
+
+void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
+ Error **errp)
+{
+ BlockDriverState *bs;
+ NBDExport *exp;
+ NBDCloseNotifier *n;
+
+ if (nbd_export_find(device)) {
+ error_setg(errp, "NBD server already exporting device '%s'", device);
+ return;
+ }
+
+ bs = bdrv_find(device);
+ if (!bs) {
+ error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+ return;
+ }
+
+ exp = nbd_export_new(bs, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY,
+ nbd_server_put_ref);
+
+ nbd_export_set_name(exp, device);
+ drive_get_ref(drive_get_by_blockdev(bs));
+
+ n = g_malloc0(sizeof(NBDCloseNotifier));
+ n->n.notify = nbd_close_notifier;
+ n->exp = exp;
+ bdrv_add_close_notifier(bs, &n->n);
+ QTAILQ_INSERT_TAIL(&close_notifiers, n, next);
+}
+
+void qmp_nbd_server_stop(Error **errp)
+{
+ while (!QTAILQ_EMPTY(&close_notifiers)) {
+ NBDCloseNotifier *cn = QTAILQ_FIRST(&close_notifiers);
+ nbd_close_notifier(&cn->n, nbd_export_get_blockdev(cn->exp));
+ }
+
+ qemu_set_fd_handler2(server_fd, NULL, NULL, NULL, NULL);
+ close(server_fd);
+ server_fd = -1;
+}
diff --git a/qapi-schema.json b/qapi-schema.json
index d40b5fc..d0fe1ad 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2849,3 +2849,46 @@
# Since: 0.14.0
##
{ 'command': 'screendump', 'data': {'filename': 'str'} }
+
+##
+# @nbd-server-start:
+#
+# Start an NBD server listening on the given host and port. Block
+# devices can then be exported using @nbd-server-add. The NBD
+# server will present them as named exports; for example, another
+# QEMU instance could refer to them as "nbd:HOST:PORT:exportname=NAME".
+#
+# @addr: Address on which to listen.
+#
+# Returns: error if the server is already running.
+#
+# Since: 1.3.0
+##
+{ 'command': 'nbd-server-start',
+ 'data': { 'addr': 'SocketAddress' } }
+
+##
+# @nbd-server-add:
+#
+# Export a device to QEMU's embedded NBD server.
+#
+# @device: Block device to be exported
+#
+# @writable: Whether clients should be able to write to the device via the
+# NBD connection (default false). #optional
+#
+# Returns: error if the device is already marked for export.
+#
+# Since: 1.3.0
+##
+{ 'command': 'nbd-server-add', 'data': {'device': 'str', '*writable': 'bool'} }
+
+##
+# @nbd-server-stop:
+#
+# Stop QEMU's embedded NBD server, and unregister all devices previously
+# added via @nbd-server-add.
+#
+# Since: 1.3.0
+##
+{ 'command': 'nbd-server-stop' }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 2f8477e..8b40902 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2543,6 +2543,22 @@ EQMP
},
{
+ .name = "nbd-server-start",
+ .args_type = "addr:q",
+ .mhandler.cmd_new = qmp_marshal_input_nbd_server_start,
+ },
+ {
+ .name = "nbd-server-add",
+ .args_type = "device:B,writable:b?",
+ .mhandler.cmd_new = qmp_marshal_input_nbd_server_add,
+ },
+ {
+ .name = "nbd-server-stop",
+ .args_type = "",
+ .mhandler.cmd_new = qmp_marshal_input_nbd_server_stop,
+ },
+
+ {
.name = "change-vnc-password",
.args_type = "password:s",
.mhandler.cmd_new = qmp_marshal_input_change_vnc_password,
--
1.7.12.1
^ permalink raw reply related [flat|nested] 72+ messages in thread
* [Qemu-devel] [PATCH 25/25] hmp: add NBD server commands
2012-10-10 14:02 [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Paolo Bonzini
` (23 preceding siblings ...)
2012-10-10 14:03 ` [Qemu-devel] [PATCH 24/25] qmp: add NBD server commands Paolo Bonzini
@ 2012-10-10 14:03 ` Paolo Bonzini
2012-10-19 8:34 ` Markus Armbruster
2012-10-16 13:09 ` [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Luiz Capitulino
2012-10-17 17:02 ` Markus Armbruster
26 siblings, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-10 14:03 UTC (permalink / raw)
To: qemu-devel; +Cc: lcapitulino
Acked-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hmp-commands.hx | 29 +++++++++++++++++++++++++++++
hmp.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
hmp.h | 2 ++
3 file modificati, 86 inserzioni(+)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index e0b537d..f5d9204 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1274,6 +1274,35 @@ Remove all matches from the access control list, and set the default
policy back to @code{deny}.
ETEXI
+ {
+ .name = "nbd_server_start",
+ .args_type = "writable:-w,uri:s",
+ .params = "nbd_server_start [-w] host:port",
+ .help = "serve block devices on the given host and port",
+ .mhandler.cmd = hmp_nbd_server_start,
+ },
+STEXI
+@item nbd_server_start @var{host}:@var{port}
+@findex nbd_server_start
+Start an NBD server on the given host and/or port, and serve all of the
+virtual machine's block devices that have an inserted media on it.
+The @option{-w} option makes the devices writable.
+ETEXI
+
+ {
+ .name = "nbd_server_stop",
+ .args_type = "",
+ .params = "nbd_server_stop",
+ .help = "stop serving block devices using the NBD protocol",
+ .mhandler.cmd = hmp_nbd_server_stop,
+ },
+STEXI
+@item nbd_server_stop
+@findex nbd_server_stop
+Stop the QEMU embedded NBD server.
+ETEXI
+
+
#if defined(TARGET_I386)
{
diff --git a/hmp.c b/hmp.c
index 70bdec2..edf6397 100644
--- a/hmp.c
+++ b/hmp.c
@@ -18,6 +18,7 @@
#include "qemu-option.h"
#include "qemu-timer.h"
#include "qmp-commands.h"
+#include "qemu_socket.h"
#include "monitor.h"
#include "console.h"
@@ -1209,3 +1210,57 @@ void hmp_screen_dump(Monitor *mon, const QDict *qdict)
qmp_screendump(filename, &err);
hmp_handle_error(mon, &err);
}
+
+void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
+{
+ const char *uri = qdict_get_str(qdict, "uri");
+ int writable = qdict_get_try_bool(qdict, "writable", 0);
+ Error *local_err = NULL;
+ BlockInfoList *block_list, *info;
+ SocketAddress *addr;
+
+ /* First check if the address is valid and start the server. */
+ addr = socket_parse(uri, &local_err);
+ if (local_err != NULL) {
+ goto exit;
+ }
+
+ qmp_nbd_server_start(addr, &local_err);
+ qapi_free_SocketAddress(addr);
+ if (local_err != NULL) {
+ goto exit;
+ }
+
+ /* Then try adding all block devices. If one fails, close all and
+ * exit.
+ */
+ block_list = qmp_query_block(NULL);
+
+ for (info = block_list; info; info = info->next) {
+ if (!info->value->has_inserted) {
+ continue;
+ }
+
+ qmp_nbd_server_add(info->value->device,
+ true, !info->value->inserted->ro && writable,
+ &local_err);
+
+ if (local_err != NULL) {
+ qmp_nbd_server_stop(NULL);
+ break;
+ }
+ }
+
+ qapi_free_BlockInfoList(block_list);
+
+exit:
+ hmp_handle_error(mon, &local_err);
+}
+
+void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict)
+{
+ Error *errp = NULL;
+
+ qmp_nbd_server_stop(&errp);
+ hmp_handle_error(mon, &errp);
+}
diff --git a/hmp.h b/hmp.h
index 71ea384..45b7c48 100644
--- a/hmp.h
+++ b/hmp.h
@@ -75,5 +75,7 @@ void hmp_getfd(Monitor *mon, const QDict *qdict);
void hmp_closefd(Monitor *mon, const QDict *qdict);
void hmp_send_key(Monitor *mon, const QDict *qdict);
void hmp_screen_dump(Monitor *mon, const QDict *qdict);
+void hmp_nbd_server_start(Monitor *mon, const QDict *qdict);
+void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
#endif
--
1.7.12.1
^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 24/25] qmp: add NBD server commands
2012-10-10 14:03 ` [Qemu-devel] [PATCH 24/25] qmp: add NBD server commands Paolo Bonzini
@ 2012-10-10 20:41 ` Eric Blake
2012-10-11 13:06 ` Paolo Bonzini
2012-10-19 8:31 ` Markus Armbruster
1 sibling, 1 reply; 72+ messages in thread
From: Eric Blake @ 2012-10-10 20:41 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 880 bytes --]
On 10/10/2012 08:03 AM, Paolo Bonzini wrote:
> Adding an NBD server inside QEMU is trivial, since all the logic is
> in nbd.c and can be shared easily between qemu-nbd and QEMU itself.
> The main difference is that qemu-nbd serves a single unnamed export,
> while QEMU serves named exports.
>
> Acked-by: Luiz Capitulino <lcapitulino@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> +##
> +# @nbd-server-add:
> +#
> +# Export a device to QEMU's embedded NBD server.
> +#
> +# @device: Block device to be exported
> +#
> +# @writable: Whether clients should be able to write to the device via the
> +# NBD connection (default false). #optional
Isn't the #optional designation supposed to come first, before 'Whether'?
--
Eric Blake eblake@redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 617 bytes --]
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 24/25] qmp: add NBD server commands
2012-10-10 20:41 ` Eric Blake
@ 2012-10-11 13:06 ` Paolo Bonzini
2012-10-11 13:14 ` Eric Blake
0 siblings, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-11 13:06 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, lcapitulino
Il 10/10/2012 22:41, Eric Blake ha scritto:
> On 10/10/2012 08:03 AM, Paolo Bonzini wrote:
>> Adding an NBD server inside QEMU is trivial, since all the logic is
>> in nbd.c and can be shared easily between qemu-nbd and QEMU itself.
>> The main difference is that qemu-nbd serves a single unnamed export,
>> while QEMU serves named exports.
>>
>> Acked-by: Luiz Capitulino <lcapitulino@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>
>> +##
>> +# @nbd-server-add:
>> +#
>> +# Export a device to QEMU's embedded NBD server.
>> +#
>> +# @device: Block device to be exported
>> +#
>> +# @writable: Whether clients should be able to write to the device via the
>> +# NBD connection (default false). #optional
>
> Isn't the #optional designation supposed to come first, before 'Whether'?
Does it really matter with no program yet written to consume it?
Putting it at the end matches the old qmp-commands.hx format better (for
commands that do have qmp-commands.hx documentation).
Paolo
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 24/25] qmp: add NBD server commands
2012-10-11 13:06 ` Paolo Bonzini
@ 2012-10-11 13:14 ` Eric Blake
2012-10-11 13:22 ` Paolo Bonzini
0 siblings, 1 reply; 72+ messages in thread
From: Eric Blake @ 2012-10-11 13:14 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, lcapitulino
[-- Attachment #1: Type: text/plain, Size: 1527 bytes --]
On 10/11/2012 07:06 AM, Paolo Bonzini wrote:
> Il 10/10/2012 22:41, Eric Blake ha scritto:
>> On 10/10/2012 08:03 AM, Paolo Bonzini wrote:
>>> Adding an NBD server inside QEMU is trivial, since all the logic is
>>> in nbd.c and can be shared easily between qemu-nbd and QEMU itself.
>>> The main difference is that qemu-nbd serves a single unnamed export,
>>> while QEMU serves named exports.
>>>
>>> Acked-by: Luiz Capitulino <lcapitulino@redhat.com>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>
>>> +##
>>> +# @nbd-server-add:
>>> +#
>>> +# Export a device to QEMU's embedded NBD server.
>>> +#
>>> +# @device: Block device to be exported
>>> +#
>>> +# @writable: Whether clients should be able to write to the device via the
>>> +# NBD connection (default false). #optional
>>
>> Isn't the #optional designation supposed to come first, before 'Whether'?
>
> Does it really matter with no program yet written to consume it?
> Putting it at the end matches the old qmp-commands.hx format better (for
> commands that do have qmp-commands.hx documentation).
I'm just asking on the grounds of consistency based on observation, and
not based on an actual hard requirement of something that goes wrong if
it's out of order. Therefore, I'm okay with your explanation, as long
as no one else can provide hard evidence for a mandatory positioning of
the marker.
--
Eric Blake eblake@redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 617 bytes --]
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 24/25] qmp: add NBD server commands
2012-10-11 13:14 ` Eric Blake
@ 2012-10-11 13:22 ` Paolo Bonzini
0 siblings, 0 replies; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-11 13:22 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, lcapitulino
Il 11/10/2012 15:14, Eric Blake ha scritto:
>>>> +##
>>>> +# @nbd-server-add:
>>>> +#
>>>> +# Export a device to QEMU's embedded NBD server.
>>>> +#
>>>> +# @device: Block device to be exported
>>>> +#
>>>> +# @writable: Whether clients should be able to write to the device via the
>>>> +# NBD connection (default false). #optional
>>>
>>> Isn't the #optional designation supposed to come first, before 'Whether'?
>>
>> Does it really matter with no program yet written to consume it?
>> Putting it at the end matches the old qmp-commands.hx format better (for
>> commands that do have qmp-commands.hx documentation).
>
> I'm just asking on the grounds of consistency based on observation, and
> not based on an actual hard requirement of something that goes wrong if
> it's out of order. Therefore, I'm okay with your explanation, as long
> as no one else can provide hard evidence for a mandatory positioning of
> the marker.
In fact, if such a program existed, it would be able to derive the
optional-ness of the argument from the schema, and it would make sense
for consistency to eliminate all #optional markers...
Paolo
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 15/25] qemu-sockets: add error propagation to inet_connect_addr
2012-10-10 14:02 ` [Qemu-devel] [PATCH 15/25] qemu-sockets: add error propagation to inet_connect_addr Paolo Bonzini
@ 2012-10-16 12:53 ` Luiz Capitulino
2012-10-17 15:40 ` Markus Armbruster
1 sibling, 0 replies; 72+ messages in thread
From: Luiz Capitulino @ 2012-10-16 12:53 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
On Wed, 10 Oct 2012 16:02:56 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:
> perror and fprintf can be removed because all clients can now consume
> Errors properly. However, we need to change the non-blocking connect
> handlers to take an Error, in order to improve error handling for
> migration with the TCP protocol.
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Are you guys related? :)
Do you want me to clean this up before applying or doesn't matter?
> ---
> qemu-sockets.c | 15 ++++++---------
> 1 file modificato, 6 inserzioni(+), 9 rimozioni(-)
>
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 3c19463..d0e1a41 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -216,7 +216,7 @@ typedef struct ConnectState {
> } ConnectState;
>
> static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
> - ConnectState *connect_state);
> + ConnectState *connect_state, Error **errp);
>
> static void wait_for_connect(void *opaque)
> {
> @@ -246,7 +246,7 @@ static void wait_for_connect(void *opaque)
> if (s->current_addr) {
> while (s->current_addr->ai_next != NULL && s->fd < 0) {
> s->current_addr = s->current_addr->ai_next;
> - s->fd = inet_connect_addr(s->current_addr, &in_progress, s);
> + s->fd = inet_connect_addr(s->current_addr, &in_progress, s, NULL);
> /* connect in progress */
> if (in_progress) {
> return;
> @@ -263,7 +263,7 @@ static void wait_for_connect(void *opaque)
> }
>
> static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
> - ConnectState *connect_state)
> + ConnectState *connect_state, Error **errp)
> {
> int sock, rc;
>
> @@ -271,8 +271,7 @@ static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
>
> sock = qemu_socket(addr->ai_family, addr->ai_socktype, addr->ai_protocol);
> if (sock < 0) {
> - fprintf(stderr, "%s: socket(%s): %s\n", __func__,
> - inet_strfamily(addr->ai_family), strerror(errno));
> + error_set_errno(errp, errno, QERR_SOCKET_CREATE_FAILED);
> return -1;
> }
> qemu_setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
> @@ -293,6 +292,7 @@ static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
> connect_state);
> *in_progress = true;
> } else if (rc < 0) {
> + error_set_errno(errp, errno, QERR_SOCKET_CONNECT_FAILED);
> closesocket(sock);
> return -1;
> }
> @@ -375,7 +375,7 @@ int inet_connect_opts(QemuOpts *opts, Error **errp,
> if (connect_state != NULL) {
> connect_state->current_addr = e;
> }
> - sock = inet_connect_addr(e, &in_progress, connect_state);
> + sock = inet_connect_addr(e, &in_progress, connect_state, errp);
> if (in_progress) {
> return sock;
> } else if (sock >= 0) {
> @@ -386,9 +386,6 @@ int inet_connect_opts(QemuOpts *opts, Error **errp,
> break;
> }
> }
> - if (sock < 0) {
> - error_set(errp, QERR_SOCKET_CONNECT_FAILED);
> - }
> g_free(connect_state);
> freeaddrinfo(res);
> return sock;
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server
2012-10-10 14:02 [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Paolo Bonzini
` (24 preceding siblings ...)
2012-10-10 14:03 ` [Qemu-devel] [PATCH 25/25] hmp: " Paolo Bonzini
@ 2012-10-16 13:09 ` Luiz Capitulino
2012-10-16 13:10 ` Paolo Bonzini
2012-10-17 17:02 ` Markus Armbruster
26 siblings, 1 reply; 72+ messages in thread
From: Luiz Capitulino @ 2012-10-16 13:09 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
On Wed, 10 Oct 2012 16:02:41 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:
> Luiz,
>
> The following changes since commit b4ae3cfa57b8c1bdbbd7b7d420971e9171203ade:
>
> ssi: Add slave autoconnect helper (2012-10-10 11:13:32 +1000)
>
> are available in the git repository at:
>
> git://github.com/bonzini/qemu.git nbd-next
>
> for you to fetch changes up to 7a057978b8b8f7578c03a470ffd8c486a778497a:
>
> hmp: add NBD server commands (2012-10-10 13:44:09 +0200)
I've applied all these patches to the qmp tree, ie. I didn't pull. I did this
because it was easier to me and because of the following changes:
1. I've added my signed-off to all patches
2. I've dropped previous acked & reviewed-by from me (my signed-off should
replace those)
3. I've dropped duplicated reviewed-by/signed-off-by from you
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server
2012-10-16 13:09 ` [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Luiz Capitulino
@ 2012-10-16 13:10 ` Paolo Bonzini
0 siblings, 0 replies; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-16 13:10 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: qemu-devel
Il 16/10/2012 15:09, Luiz Capitulino ha scritto:
> I've applied all these patches to the qmp tree, ie. I didn't pull. I did this
> because it was easier to me and because of the following changes:
>
> 1. I've added my signed-off to all patches
> 2. I've dropped previous acked & reviewed-by from me (my signed-off should
> replace those)
> 3. I've dropped duplicated reviewed-by/signed-off-by from you
>
Great! (The PULL was just for convenience in case it helped you).
Paolo
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 04/25] qemu-sockets: add nonblocking connect for Unix sockets
2012-10-10 14:02 ` [Qemu-devel] [PATCH 04/25] qemu-sockets: add nonblocking connect for Unix sockets Paolo Bonzini
@ 2012-10-17 13:33 ` Markus Armbruster
2012-10-17 13:44 ` Paolo Bonzini
0 siblings, 1 reply; 72+ messages in thread
From: Markus Armbruster @ 2012-10-17 13:33 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, lcapitulino
Paolo Bonzini <pbonzini@redhat.com> writes:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> v1->v2: fixed connect_state memory leaks
>
> qemu-char.c | 2 +-
> qemu-sockets.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++----------
> qemu_socket.h | 6 ++++-
> 3 file modificati, 70 inserzioni(+), 15 rimozioni(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index 3cc6cb5..8ebd582 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2450,7 +2450,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
> if (is_listen) {
> fd = unix_listen_opts(opts, NULL);
> } else {
> - fd = unix_connect_opts(opts, NULL);
> + fd = unix_connect_opts(opts, NULL, NULL, NULL);
> }
> } else {
> if (is_listen) {
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index f7e67b6..6a49429 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -252,16 +252,19 @@ static void wait_for_connect(void *opaque)
> }
>
> /* try to connect to the next address on the list */
> - while (s->current_addr->ai_next != NULL && s->fd < 0) {
> - s->current_addr = s->current_addr->ai_next;
> - s->fd = inet_connect_addr(s->current_addr, &in_progress, s);
> - /* connect in progress */
> - if (in_progress) {
> - return;
> + if (s->current_addr) {
> + while (s->current_addr->ai_next != NULL && s->fd < 0) {
> + s->current_addr = s->current_addr->ai_next;
> + s->fd = inet_connect_addr(s->current_addr, &in_progress, s);
> + /* connect in progress */
> + if (in_progress) {
> + return;
> + }
> }
> +
> + freeaddrinfo(s->addr_list);
> }
>
> - freeaddrinfo(s->addr_list);
> if (s->callback) {
> s->callback(s->fd, s->opaque);
> }
> @@ -701,11 +704,13 @@ err:
> return -1;
> }
>
> -int unix_connect_opts(QemuOpts *opts, Error **errp)
> +int unix_connect_opts(QemuOpts *opts, Error **errp,
> + NonBlockingConnectHandler *callback, void *opaque)
> {
> struct sockaddr_un un;
> const char *path = qemu_opt_get(opts, "path");
> - int sock;
> + ConnectState *connect_state = NULL;
> + int sock, rc;
>
> if (NULL == path) {
> fprintf(stderr, "unix connect: no path specified\n");
> @@ -717,16 +722,44 @@ int unix_connect_opts(QemuOpts *opts, Error **errp)
> perror("socket(unix)");
> return -1;
> }
> + if (callback != NULL) {
> + connect_state = g_malloc0(sizeof(*connect_state));
> + connect_state->callback = callback;
> + connect_state->opaque = opaque;
> + socket_set_nonblock(sock);
> + }
>
> 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) {
> +
> + /* connect to peer */
> + do {
> + rc = 0;
> + if (connect(sock, (struct sockaddr *) &un, sizeof(un)) < 0) {
> + rc = -socket_error();
> + }
> + } while (rc == -EINTR);
Isn't this a silent EINTR bug fix?
> +
> + if (connect_state != NULL && QEMU_SOCKET_RC_INPROGRESS(rc)) {
> + connect_state->fd = sock;
> + qemu_set_fd_handler2(sock, NULL, NULL, wait_for_connect,
> + connect_state);
> + return sock;
> + } else {
> + /* non blocking socket immediate success, call callback */
> + if (callback != NULL) {
> + callback(sock, opaque);
> + }
> + }
> +
> + if (rc < 0) {
> fprintf(stderr, "connect(unix:%s): %s\n", path, strerror(errno));
> close(sock);
> - return -1;
> + sock = -1;
> }
>
> + g_free(connect_state);
> return sock;
> }
>
Unlike inet_connect_opts(), this one runs callback() even when connect()
fails. Are you sure that's what you want?
[...]
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 05/25] migration: avoid using error_is_set and thus relying on errp != NULL
2012-10-10 14:02 ` [Qemu-devel] [PATCH 05/25] migration: avoid using error_is_set and thus relying on errp != NULL Paolo Bonzini
@ 2012-10-17 13:36 ` Markus Armbruster
2012-10-17 13:37 ` Paolo Bonzini
0 siblings, 1 reply; 72+ messages in thread
From: Markus Armbruster @ 2012-10-17 13:36 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, lcapitulino
Paolo Bonzini <pbonzini@redhat.com> writes:
> The migration code is using errp to detect "internal" errors,
> this means that it relies on errp being non-NULL.
Impact?
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 05/25] migration: avoid using error_is_set and thus relying on errp != NULL
2012-10-17 13:36 ` Markus Armbruster
@ 2012-10-17 13:37 ` Paolo Bonzini
2012-10-17 15:44 ` Markus Armbruster
0 siblings, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-17 13:37 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, lcapitulino
Il 17/10/2012 15:36, Markus Armbruster ha scritto:
>> > The migration code is using errp to detect "internal" errors,
>> > this means that it relies on errp being non-NULL.
> Impact?
None because so far our only QMP client is HMP and never passes a NULL
Error **. But if we had others, it would make sure migration can work
with a NULL Error **.
Paolo
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 04/25] qemu-sockets: add nonblocking connect for Unix sockets
2012-10-17 13:33 ` Markus Armbruster
@ 2012-10-17 13:44 ` Paolo Bonzini
0 siblings, 0 replies; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-17 13:44 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, lcapitulino
Il 17/10/2012 15:33, Markus Armbruster ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>> v1->v2: fixed connect_state memory leaks
>>
>> qemu-char.c | 2 +-
>> qemu-sockets.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++----------
>> qemu_socket.h | 6 ++++-
>> 3 file modificati, 70 inserzioni(+), 15 rimozioni(-)
>>
>> diff --git a/qemu-char.c b/qemu-char.c
>> index 3cc6cb5..8ebd582 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -2450,7 +2450,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
>> if (is_listen) {
>> fd = unix_listen_opts(opts, NULL);
>> } else {
>> - fd = unix_connect_opts(opts, NULL);
>> + fd = unix_connect_opts(opts, NULL, NULL, NULL);
>> }
>> } else {
>> if (is_listen) {
>> diff --git a/qemu-sockets.c b/qemu-sockets.c
>> index f7e67b6..6a49429 100644
>> --- a/qemu-sockets.c
>> +++ b/qemu-sockets.c
>> @@ -252,16 +252,19 @@ static void wait_for_connect(void *opaque)
>> }
>>
>> /* try to connect to the next address on the list */
>> - while (s->current_addr->ai_next != NULL && s->fd < 0) {
>> - s->current_addr = s->current_addr->ai_next;
>> - s->fd = inet_connect_addr(s->current_addr, &in_progress, s);
>> - /* connect in progress */
>> - if (in_progress) {
>> - return;
>> + if (s->current_addr) {
>> + while (s->current_addr->ai_next != NULL && s->fd < 0) {
>> + s->current_addr = s->current_addr->ai_next;
>> + s->fd = inet_connect_addr(s->current_addr, &in_progress, s);
>> + /* connect in progress */
>> + if (in_progress) {
>> + return;
>> + }
>> }
>> +
>> + freeaddrinfo(s->addr_list);
>> }
>>
>> - freeaddrinfo(s->addr_list);
>> if (s->callback) {
>> s->callback(s->fd, s->opaque);
>> }
>> @@ -701,11 +704,13 @@ err:
>> return -1;
>> }
>>
>> -int unix_connect_opts(QemuOpts *opts, Error **errp)
>> +int unix_connect_opts(QemuOpts *opts, Error **errp,
>> + NonBlockingConnectHandler *callback, void *opaque)
>> {
>> struct sockaddr_un un;
>> const char *path = qemu_opt_get(opts, "path");
>> - int sock;
>> + ConnectState *connect_state = NULL;
>> + int sock, rc;
>>
>> if (NULL == path) {
>> fprintf(stderr, "unix connect: no path specified\n");
>> @@ -717,16 +722,44 @@ int unix_connect_opts(QemuOpts *opts, Error **errp)
>> perror("socket(unix)");
>> return -1;
>> }
>> + if (callback != NULL) {
>> + connect_state = g_malloc0(sizeof(*connect_state));
>> + connect_state->callback = callback;
>> + connect_state->opaque = opaque;
>> + socket_set_nonblock(sock);
>> + }
>>
>> 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) {
>> +
>> + /* connect to peer */
>> + do {
>> + rc = 0;
>> + if (connect(sock, (struct sockaddr *) &un, sizeof(un)) < 0) {
>> + rc = -socket_error();
>> + }
>> + } while (rc == -EINTR);
>
> Isn't this a silent EINTR bug fix?
>
>> +
>> + if (connect_state != NULL && QEMU_SOCKET_RC_INPROGRESS(rc)) {
>> + connect_state->fd = sock;
>> + qemu_set_fd_handler2(sock, NULL, NULL, wait_for_connect,
>> + connect_state);
>> + return sock;
>> + } else {
>> + /* non blocking socket immediate success, call callback */
>> + if (callback != NULL) {
>> + callback(sock, opaque);
>> + }
>> + }
>> +
>> + if (rc < 0) {
>> fprintf(stderr, "connect(unix:%s): %s\n", path, strerror(errno));
>> close(sock);
>> - return -1;
>> + sock = -1;
>> }
>>
>> + g_free(connect_state);
>> return sock;
>> }
>>
>
> Unlike inet_connect_opts(), this one runs callback() even when connect()
> fails. Are you sure that's what you want?
I think it's harmless, but it's worth harmonizing it.
Paolo
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 07/25] migration: use qemu-sockets to establish Unix sockets
2012-10-10 14:02 ` [Qemu-devel] [PATCH 07/25] migration: use qemu-sockets to establish Unix sockets Paolo Bonzini
@ 2012-10-17 14:30 ` Markus Armbruster
0 siblings, 0 replies; 72+ messages in thread
From: Markus Armbruster @ 2012-10-17 14:30 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, lcapitulino
Paolo Bonzini <pbonzini@redhat.com> writes:
> This makes migration-unix.c again a cut-and-paste job from migration-tcp.c,
> exactly as it was in the beginning. :)
Neat!
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 08/25] migration (outgoing): add error propagation for all protocols
2012-10-10 14:02 ` [Qemu-devel] [PATCH 08/25] migration (outgoing): add error propagation for all protocols Paolo Bonzini
@ 2012-10-17 14:43 ` Markus Armbruster
2012-10-17 14:50 ` Paolo Bonzini
0 siblings, 1 reply; 72+ messages in thread
From: Markus Armbruster @ 2012-10-17 14:43 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, lcapitulino
Paolo Bonzini <pbonzini@redhat.com> writes:
> Error propagation is already there for socket backends, even though
> it is (and remains) incomplete because no Error is passed to the
> NonBlockingConnectHandler.
Why is that a problem?
> Add it to other protocols, simplifying
> code that tests for errors that will never happen.
>
> With all protocols understanding Error, the code can be simplified
> further by removing the return value.
>
> Before:
>
> (qemu) migrate fd:ffff
> migrate: An undefined error has occurred
> (qemu) info migrate
> (qemu)
>
> After:
>
> (qemu) migrate fd:ffff
> migrate: File descriptor named 'ffff' has not been found
> (qemu) info migrate
> capabilities: xbzrle: off
> Migration status: failed
> total time: 0 milliseconds
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> v1->v2: turn bizarre DPRINTF into an assertion failure or just
> drop it for the failure test of O_NONBLOCK. Clean up after this
> change.
>
> migration-fd.c | 19 ++++---------------
> migration-tcp.c | 13 ++-----------
> migration-unix.c | 11 ++---------
> migration.c | 17 ++++++-----------
> migration.h | 9 ++++-----
> 6 file modificati, 22 inserzioni(+), 65 rimozioni(-)
>
> diff --git a/migration-exec.c b/migration-exec.c
> index 6c97db9..5f3f4b2 100644
> --- a/migration-exec.c
> +++ b/migration-exec.c
> @@ -60,22 +60,18 @@ static int exec_close(MigrationState *s)
> return ret;
> }
>
> -int exec_start_outgoing_migration(MigrationState *s, const char *command)
> +void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
> {
> FILE *f;
>
> f = popen(command, "w");
> if (f == NULL) {
> - DPRINTF("Unable to popen exec target\n");
> - goto err_after_popen;
> + error_setg_errno(errp, errno, "failed to popen the migration target");
> + return;
> }
>
> s->fd = fileno(f);
> - if (s->fd == -1) {
> - DPRINTF("Unable to retrieve file descriptor for popen'd handle\n");
> - goto err_after_open;
> - }
> -
> + assert(s->fd != -1);
Okay, because fileno() can fail only if its argument is not a valid
stream, which really shouldn't be possible here.
> socket_set_nonblock(s->fd);
>
> s->opaque = qemu_popen(f, "w");
> @@ -85,12 +81,6 @@ int exec_start_outgoing_migration(MigrationState *s, const char *command)
> s->write = file_write;
>
> migrate_fd_connect(s);
> - return 0;
> -
> -err_after_open:
> - pclose(f);
> -err_after_popen:
> - return -1;
> }
>
> static void exec_accept_incoming_migration(void *opaque)
> diff --git a/migration-fd.c b/migration-fd.c
> index 7335167..a7c800a 100644
> --- a/migration-fd.c
> +++ b/migration-fd.c
> @@ -73,30 +73,19 @@ static int fd_close(MigrationState *s)
> return 0;
> }
>
> -int fd_start_outgoing_migration(MigrationState *s, const char *fdname)
> +void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **errp)
> {
> - s->fd = monitor_get_fd(cur_mon, fdname, NULL);
> + s->fd = monitor_get_fd(cur_mon, fdname, errp);
> if (s->fd == -1) {
> - DPRINTF("fd_migration: invalid file descriptor identifier\n");
> - goto err_after_get_fd;
> - }
> -
> - if (fcntl(s->fd, F_SETFL, O_NONBLOCK) == -1) {
> - DPRINTF("Unable to set nonblocking mode on file descriptor\n");
> - goto err_after_open;
> + return;
> }
>
> + fcntl(s->fd, F_SETFL, O_NONBLOCK);
Sure this can't fail?
> s->get_error = fd_errno;
> s->write = fd_write;
> s->close = fd_close;
>
> migrate_fd_connect(s);
> - return 0;
> -
> -err_after_open:
> - close(s->fd);
> -err_after_get_fd:
> - return -1;
> }
>
> static void fd_accept_incoming_migration(void *opaque)
[...]
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 08/25] migration (outgoing): add error propagation for all protocols
2012-10-17 14:43 ` Markus Armbruster
@ 2012-10-17 14:50 ` Paolo Bonzini
2012-10-17 15:43 ` Markus Armbruster
0 siblings, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-17 14:50 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, lcapitulino
Il 17/10/2012 16:43, Markus Armbruster ha scritto:
>> > Error propagation is already there for socket backends, even though
>> > it is (and remains) incomplete because no Error is passed to the
>> > NonBlockingConnectHandler.
> Why is that a problem?
It means that the exact error message still cannot be sent to the user
if the OS reports it asynchronously via SO_ERROR. If
NonBlockingConnectHandler received an Error**, we could for example
report the error class and/or message via a new field of the
query-migration command even if it is reported asynchronously.
Paolo
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 11/25] nbd: ask and print error information from qemu-sockets
2012-10-10 14:02 ` [Qemu-devel] [PATCH 11/25] nbd: " Paolo Bonzini
@ 2012-10-17 14:51 ` Markus Armbruster
2012-10-17 14:53 ` Paolo Bonzini
0 siblings, 1 reply; 72+ messages in thread
From: Markus Armbruster @ 2012-10-17 14:51 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, lcapitulino
Paolo Bonzini <pbonzini@redhat.com> writes:
> Before:
>
> $ qemu-system-x86_64 nbd:localhost:12345
> inet_connect_opts: connect(ipv4,yakj.usersys.redhat.com,127.0.0.1,12345): Connection refused
> qemu-system-x86_64: could not open disk image nbd:localhost:12345: Connection refused
>
> After:
>
> $ x86_64-softmmu/qemu-system-x86_64 nbd:localhost:12345
> qemu-system-x86_64: Failed to connect to socket: Connection refused
> qemu-system-x86_64: could not open disk image nbd:localhost:12345: Connection refused
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> nbd.c | 39 +++++++++++++++++++++++++++++++--------
> 1 file modificato, 31 inserzioni(+), 8 rimozioni(-)
>
> diff --git a/nbd.c b/nbd.c
> index f61a288..cec5a94 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -208,7 +208,14 @@ int tcp_socket_outgoing(const char *address, uint16_t port)
>
> int tcp_socket_outgoing_spec(const char *address_and_port)
> {
> - return inet_connect(address_and_port, NULL);
> + Error *local_err = NULL;
> + int fd = inet_connect(address_and_port, &local_err);
> +
> + if (local_err != NULL) {
> + qerror_report_err(local_err);
> + error_free(local_err);
> + }
> + return fd;
> }
>
> int tcp_socket_incoming(const char *address, uint16_t port)
> @@ -220,22 +227,38 @@ int tcp_socket_incoming(const char *address, uint16_t port)
>
> int tcp_socket_incoming_spec(const char *address_and_port)
> {
> - char *ostr = NULL;
> - int olen = 0;
> - return inet_listen(address_and_port, ostr, olen, SOCK_STREAM, 0, NULL);
> + Error *local_err = NULL;
> + int fd = inet_listen(address_and_port, NULL, 0, SOCK_STREAM, 0, &local_err);
> +
> + if (local_err != NULL) {
> + qerror_report_err(local_err);
> + error_free(local_err);
> + }
> + return fd;
> }
>
> int unix_socket_incoming(const char *path)
> {
> - char *ostr = NULL;
> - int olen = 0;
> + Error *local_err = NULL;
> + int fd = unix_listen(path, NULL, 0, &local_err);
>
> - return unix_listen(path, ostr, olen, NULL);
> + if (local_err != NULL) {
> + qerror_report_err(local_err);
> + error_free(local_err);
> + }
> + return fd;
> }
>
> int unix_socket_outgoing(const char *path)
> {
> - return unix_connect(path, NULL);
> + Error *local_err = NULL;
> + int fd = unix_connect(path, &local_err);
> +
> + if (local_err != NULL) {
> + qerror_report_err(local_err);
> + error_free(local_err);
> + }
> + return fd;
> }
>
> /* Basic flow for negotiation
The boilerplate pattern
Error *local_err = NULL;
FOO(..., *local_err);
if (local_err) {
qerror_report_err(local_err);
error_free(local_err);
}
is spreading. Not quite sure it's worth a macro.
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 11/25] nbd: ask and print error information from qemu-sockets
2012-10-17 14:51 ` Markus Armbruster
@ 2012-10-17 14:53 ` Paolo Bonzini
0 siblings, 0 replies; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-17 14:53 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, lcapitulino
Il 17/10/2012 16:51, Markus Armbruster ha scritto:
>> > /* Basic flow for negotiation
> The boilerplate pattern
>
> Error *local_err = NULL;
> FOO(..., *local_err);
> if (local_err) {
> qerror_report_err(local_err);
> error_free(local_err);
> }
>
> is spreading. Not quite sure it's worth a macro.
Actually this should not spread, but this one should:
Error *local_err = NULL;
FOO(..., *local_err);
if (local_err) {
error_propagate(errp, local_err);
return;
}
Not quite sure how to macroize it though, at least without making the
code too ugly to see.
Paolo
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 13/25] vnc: add error propagation to vnc_display_open
2012-10-10 14:02 ` [Qemu-devel] [PATCH 13/25] vnc: add error propagation to vnc_display_open Paolo Bonzini
@ 2012-10-17 15:17 ` Markus Armbruster
2012-10-17 15:48 ` Paolo Bonzini
0 siblings, 1 reply; 72+ messages in thread
From: Markus Armbruster @ 2012-10-17 15:17 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, lcapitulino
Paolo Bonzini <pbonzini@redhat.com> writes:
> Before:
>
> $ qemu-system-x86_64 -vnc foo.bar:12345
> getaddrinfo(foo.bar,18245): Name or service not known
> Failed to start VNC server on `foo.bar:12345'
>
> $ qemu-system-x86_64 -vnc localhost:12345,reverse=on
> inet_connect_opts: connect(ipv4,yakj.usersys.redhat.com,127.0.0.1,12345): Connection refused
> Failed to start VNC server on `localhost:12345,reverse=on'
>
> After:
>
> $ x86_64-softmmu/qemu-system-x86_64 -vnc foo.bar:12345
> Failed to start VNC server on `foo.bar:12345': address resolution failed for foo.bar:18245: Name or service not known
>
> $ x86_64-softmmu/qemu-system-x86_64 -vnc localhost:12345,reverse=on
> Failed to start VNC server on `localhost:12345,reverse=on': Failed to connect to socket: Connection refused
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> v1->v2: small cleanup for the if(reverse) case, do not use
> qerror_report_err.
>
> migration-exec.c | 18 ++++--------------
> console.h | 2 +-
> qmp.c | 6 ++---
> ui/vnc.c | 91 +++++++++++++++++++++++++++++----------------------------------
> vl.c | 9 ++++---
> 4 file modificati, 51 inserzioni(+), 57 rimozioni(-)
>
> diff --git a/console.h b/console.h
> index f990684..6099d8d 100644
> --- a/console.h
> +++ b/console.h
> @@ -378,7 +378,7 @@ void cocoa_display_init(DisplayState *ds, int full_screen);
> /* vnc.c */
> void vnc_display_init(DisplayState *ds);
> void vnc_display_close(DisplayState *ds);
> -int vnc_display_open(DisplayState *ds, const char *display);
> +void vnc_display_open(DisplayState *ds, const char *display, Error **errp);
> void vnc_display_add_client(DisplayState *ds, int csock, int skipauth);
> int vnc_display_disable_login(DisplayState *ds);
> char *vnc_display_local_addr(DisplayState *ds);
> diff --git a/qmp.c b/qmp.c
> index 36c54c5..31bc3bf 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -349,11 +349,9 @@ void qmp_change_vnc_password(const char *password, Error **errp)
> }
> }
>
> -static void qmp_change_vnc_listen(const char *target, Error **err)
> +static void qmp_change_vnc_listen(const char *target, Error **errp)
> {
> - if (vnc_display_open(NULL, target) < 0) {
> - error_set(err, QERR_VNC_SERVER_FAILED, target);
> - }
> + vnc_display_open(NULL, target, errp);
> }
>
> static void qmp_change_vnc(const char *target, bool has_arg, const char *arg,
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 235596e..b8e46ca 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -2844,7 +2844,7 @@ char *vnc_display_local_addr(DisplayState *ds)
> return vnc_socket_local_addr("%s:%s", vs->lsock);
> }
>
> -int vnc_display_open(DisplayState *ds, const char *display)
> +void vnc_display_open(DisplayState *ds, const char *display, Error **errp)
> {
> VncDisplay *vs = ds ? (VncDisplay *)ds->opaque : vnc_display;
> const char *options;
> @@ -2863,13 +2863,12 @@ int vnc_display_open(DisplayState *ds, const char *display)
> int lock_key_sync = 1;
>
> if (!vnc_display)
> - return -1;
> + goto fail;
Now executes
g_free(vs->display);
vs->display = NULL;
The latter looks innocent enough, but why is the former safe?
> vnc_display_close(ds);
> if (strcmp(display, "none") == 0)
> - return 0;
> + return;
>
> - if (!(vs->display = strdup(display)))
> - return -1;
> + vs->display = g_strdup(display);
> vs->share_policy = VNC_SHARE_POLICY_ALLOW_EXCLUSIVE;
>
> options = display;
> @@ -2877,13 +2876,11 @@ int vnc_display_open(DisplayState *ds, const char *display)
> options++;
> if (strncmp(options, "password", 8) == 0) {
> if (fips_get_state()) {
> - fprintf(stderr,
> - "VNC password auth disabled due to FIPS mode, "
> - "consider using the VeNCrypt or SASL authentication "
> - "methods as an alternative\n");
> - g_free(vs->display);
> - vs->display = NULL;
> - return -1;
> + error_setg(errp,
> + "VNC password auth disabled due to FIPS mode, "
> + "consider using the VeNCrypt or SASL authentication "
> + "methods as an alternative\n");
Message for error_setg() should not end with '\n'.
> + goto fail;
> }
> password = 1; /* Require password auth */
> } else if (strncmp(options, "reverse", 7) == 0) {
> @@ -2913,18 +2910,14 @@ int vnc_display_open(DisplayState *ds, const char *display)
>
> VNC_DEBUG("Trying certificate path '%s'\n", path);
> if (vnc_tls_set_x509_creds_dir(vs, path) < 0) {
> - fprintf(stderr, "Failed to find x509 certificates/keys in %s\n", path);
> + error_setg(errp, "Failed to find x509 certificates/keys in %s\n", path);
Likewise.
> g_free(path);
> - g_free(vs->display);
> - vs->display = NULL;
> - return -1;
> + goto fail;
> }
> g_free(path);
> } else {
> - fprintf(stderr, "No certificate path provided\n");
> - g_free(vs->display);
> - vs->display = NULL;
> - return -1;
> + error_setg(errp, "No certificate path provided\n");
Likewise.
> + goto fail;
> }
> #endif
> #if defined(CONFIG_VNC_TLS) || defined(CONFIG_VNC_SASL)
> @@ -2943,10 +2936,8 @@ int vnc_display_open(DisplayState *ds, const char *display)
> } else if (strncmp(options+6, "force-shared", 12) == 0) {
> vs->share_policy = VNC_SHARE_POLICY_FORCE_SHARED;
> } else {
> - fprintf(stderr, "unknown vnc share= option\n");
> - g_free(vs->display);
> - vs->display = NULL;
> - return -1;
> + error_setg(errp, "unknown vnc share= option\n");
Likewise.
> + goto fail;
> }
> }
> }
> @@ -3047,52 +3038,54 @@ int vnc_display_open(DisplayState *ds, const char *display)
>
> #ifdef CONFIG_VNC_SASL
> if ((saslErr = sasl_server_init(NULL, "qemu")) != SASL_OK) {
> - fprintf(stderr, "Failed to initialize SASL auth %s",
> - sasl_errstring(saslErr, NULL, NULL));
> - g_free(vs->display);
> - vs->display = NULL;
> - return -1;
> + error_setg(errp, "Failed to initialize SASL auth %s",
> + sasl_errstring(saslErr, NULL, NULL));
> + goto fail;
> }
> #endif
> vs->lock_key_sync = lock_key_sync;
>
> if (reverse) {
> /* connect to viewer */
> - if (strncmp(display, "unix:", 5) == 0)
> - vs->lsock = unix_connect(display+5, NULL);
> - else
> - vs->lsock = inet_connect(display, NULL);
> - if (-1 == vs->lsock) {
> - g_free(vs->display);
> - vs->display = NULL;
> - return -1;
> + int csock;
> + vs->lsock = -1;
> + if (strncmp(display, "unix:", 5) == 0) {
> + csock = unix_connect(display+5, errp);
> } else {
> - int csock = vs->lsock;
> - vs->lsock = -1;
> - vnc_connect(vs, csock, 0);
> + csock = inet_connect(display, errp);
> }
The csock / vs->lsock change confused me. I think it's correct, but I
wish it was a separate cleanup commit.
Same for the goto fail cleanup, by the way.
> - return 0;
> -
> + if (-1 == csock) {
csock < 0, please.
> + goto fail;
> + }
> + vnc_connect(vs, csock, 0);
> } else {
> /* listen for connects */
> char *dpy;
> dpy = g_malloc(256);
> if (strncmp(display, "unix:", 5) == 0) {
> pstrcpy(dpy, 256, "unix:");
> - vs->lsock = unix_listen(display+5, dpy+5, 256-5, NULL);
> + vs->lsock = unix_listen(display+5, dpy+5, 256-5, errp);
> } else {
> vs->lsock = inet_listen(display, dpy, 256,
> - SOCK_STREAM, 5900, NULL);
> + SOCK_STREAM, 5900, errp);
> }
> if (-1 == vs->lsock) {
> g_free(dpy);
> - return -1;
> - } else {
> - g_free(vs->display);
> - vs->display = dpy;
> + goto fail;
Now executes
g_free(vs->display);
vs->display = NULL;
Silent bug fix?
If yes, it should really be a separate commit!
> }
> + g_free(vs->display);
> + vs->display = dpy;
> + qemu_set_fd_handler2(vs->lsock, NULL, vnc_listen_read, NULL, vs);
> + }
> + return;
> +
> +fail:
> + if (!error_is_set(errp)) {
> + error_set(errp, QERR_VNC_SERVER_FAILED, display);
How can we get here with no error set?
1. !vnc_display (first goto fail).
2. unit_connect() or inet_listen() return failure, but don't set error.
3. unix_listen() or inet_listen() return failure, but don't set error.
Can 2. or 3. happen?
If yes, these functions suck. If no, let's fix up 1. to set a suitable
error, and drop the uninformative generic error here.
> }
> - return qemu_set_fd_handler2(vs->lsock, NULL, vnc_listen_read, NULL, vs);
> + g_free(vs->display);
> + vs->display = NULL;
> + return;
> }
>
> void vnc_display_add_client(DisplayState *ds, int csock, int skipauth)
> diff --git a/vl.c b/vl.c
> index 2a072e8..67a624f 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3706,10 +3706,13 @@ int main(int argc, char **argv, char **envp)
> #ifdef CONFIG_VNC
> /* init remote displays */
> if (vnc_display) {
> + Error *local_err = NULL;
> vnc_display_init(ds);
> - if (vnc_display_open(ds, vnc_display) < 0) {
> - fprintf(stderr, "Failed to start VNC server on `%s'\n",
> - vnc_display);
> + vnc_display_open(ds, vnc_display, &local_err);
> + if (local_err != NULL) {
> + fprintf(stderr, "Failed to start VNC server on `%s': %s\n",
> + vnc_display, error_get_pretty(local_err));
> + error_free(local_err);
> exit(1);
> }
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 15/25] qemu-sockets: add error propagation to inet_connect_addr
2012-10-10 14:02 ` [Qemu-devel] [PATCH 15/25] qemu-sockets: add error propagation to inet_connect_addr Paolo Bonzini
2012-10-16 12:53 ` Luiz Capitulino
@ 2012-10-17 15:40 ` Markus Armbruster
2012-10-17 15:50 ` Paolo Bonzini
1 sibling, 1 reply; 72+ messages in thread
From: Markus Armbruster @ 2012-10-17 15:40 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, lcapitulino
Paolo Bonzini <pbonzini@redhat.com> writes:
> perror and fprintf can be removed because all clients can now consume
> Errors properly. However, we need to change the non-blocking connect
> handlers to take an Error, in order to improve error handling for
> migration with the TCP protocol.
The second sentence is about future work, isn't it?
If yes, suggest "we'll need to change" to make it obvious.
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> qemu-sockets.c | 15 ++++++---------
> 1 file modificato, 6 inserzioni(+), 9 rimozioni(-)
>
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 3c19463..d0e1a41 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -216,7 +216,7 @@ typedef struct ConnectState {
> } ConnectState;
>
> static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
> - ConnectState *connect_state);
> + ConnectState *connect_state, Error **errp);
>
> static void wait_for_connect(void *opaque)
> {
> @@ -246,7 +246,7 @@ static void wait_for_connect(void *opaque)
> if (s->current_addr) {
> while (s->current_addr->ai_next != NULL && s->fd < 0) {
> s->current_addr = s->current_addr->ai_next;
> - s->fd = inet_connect_addr(s->current_addr, &in_progress, s);
> + s->fd = inet_connect_addr(s->current_addr, &in_progress, s, NULL);
Doesn't this drop error messages?
If yes, but it's healed later in the series, the temporary breakage
still needs to be spelled out in the commit message.
> /* connect in progress */
> if (in_progress) {
> return;
> @@ -263,7 +263,7 @@ static void wait_for_connect(void *opaque)
> }
>
> static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
> - ConnectState *connect_state)
> + ConnectState *connect_state, Error **errp)
> {
> int sock, rc;
>
> @@ -271,8 +271,7 @@ static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
>
> sock = qemu_socket(addr->ai_family, addr->ai_socktype, addr->ai_protocol);
> if (sock < 0) {
> - fprintf(stderr, "%s: socket(%s): %s\n", __func__,
> - inet_strfamily(addr->ai_family), strerror(errno));
> + error_set_errno(errp, errno, QERR_SOCKET_CREATE_FAILED);
> return -1;
> }
> qemu_setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
> @@ -293,6 +292,7 @@ static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
> connect_state);
> *in_progress = true;
> } else if (rc < 0) {
> + error_set_errno(errp, errno, QERR_SOCKET_CONNECT_FAILED);
Looks like this reports a previously unreported error condition.
If it does, it's a fix worth mentioning in commit message.
> closesocket(sock);
> return -1;
> }
> @@ -375,7 +375,7 @@ int inet_connect_opts(QemuOpts *opts, Error **errp,
> if (connect_state != NULL) {
> connect_state->current_addr = e;
> }
> - sock = inet_connect_addr(e, &in_progress, connect_state);
> + sock = inet_connect_addr(e, &in_progress, connect_state, errp);
> if (in_progress) {
> return sock;
> } else if (sock >= 0) {
> @@ -386,9 +386,6 @@ int inet_connect_opts(QemuOpts *opts, Error **errp,
> break;
> }
> }
> - if (sock < 0) {
> - error_set(errp, QERR_SOCKET_CONNECT_FAILED);
> - }
> g_free(connect_state);
> freeaddrinfo(res);
> return sock;
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 08/25] migration (outgoing): add error propagation for all protocols
2012-10-17 14:50 ` Paolo Bonzini
@ 2012-10-17 15:43 ` Markus Armbruster
0 siblings, 0 replies; 72+ messages in thread
From: Markus Armbruster @ 2012-10-17 15:43 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, lcapitulino
Paolo Bonzini <pbonzini@redhat.com> writes:
> Il 17/10/2012 16:43, Markus Armbruster ha scritto:
>>> > Error propagation is already there for socket backends, even though
>>> > it is (and remains) incomplete because no Error is passed to the
>>> > NonBlockingConnectHandler.
>> Why is that a problem?
>
> It means that the exact error message still cannot be sent to the user
> if the OS reports it asynchronously via SO_ERROR. If
> NonBlockingConnectHandler received an Error**, we could for example
> report the error class and/or message via a new field of the
> query-migration command even if it is reported asynchronously.
First time you mention the problem in a commit message could use a
variant of this explanation.
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 05/25] migration: avoid using error_is_set and thus relying on errp != NULL
2012-10-17 13:37 ` Paolo Bonzini
@ 2012-10-17 15:44 ` Markus Armbruster
0 siblings, 0 replies; 72+ messages in thread
From: Markus Armbruster @ 2012-10-17 15:44 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, lcapitulino
Paolo Bonzini <pbonzini@redhat.com> writes:
> Il 17/10/2012 15:36, Markus Armbruster ha scritto:
>>> > The migration code is using errp to detect "internal" errors,
>>> > this means that it relies on errp being non-NULL.
>> Impact?
>
> None because so far our only QMP client is HMP and never passes a NULL
> Error **. But if we had others, it would make sure migration can work
> with a NULL Error **.
Explanation would be nice to have in the commit message.
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 13/25] vnc: add error propagation to vnc_display_open
2012-10-17 15:17 ` Markus Armbruster
@ 2012-10-17 15:48 ` Paolo Bonzini
2012-10-19 7:49 ` Markus Armbruster
0 siblings, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-17 15:48 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, lcapitulino
Il 17/10/2012 17:17, Markus Armbruster ha scritto:
>> > +fail:
>> > + if (!error_is_set(errp)) {
>> > + error_set(errp, QERR_VNC_SERVER_FAILED, display);
> How can we get here with no error set?
>
> 1. !vnc_display (first goto fail).
This can be fixed up to give a separate error.
> 2. unit_connect() or inet_listen() return failure, but don't set error.
>
> 3. unix_listen() or inet_listen() return failure, but don't set error.
>
> Can 2. or 3. happen?
>
> If yes, these functions suck. If no, let's fix up 1. to set a suitable
> error, and drop the uninformative generic error here.
>
It can at this point in the series, but not at the end.
I tried to split this one into many commits, but I wasn't sure it was
worth to make a mini-series out of one function. In retrospect
it was.
Paolo
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 15/25] qemu-sockets: add error propagation to inet_connect_addr
2012-10-17 15:40 ` Markus Armbruster
@ 2012-10-17 15:50 ` Paolo Bonzini
2012-10-19 7:59 ` Markus Armbruster
0 siblings, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-17 15:50 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, lcapitulino
Il 17/10/2012 17:40, Markus Armbruster ha scritto:
>> > if (s->current_addr) {
>> > while (s->current_addr->ai_next != NULL && s->fd < 0) {
>> > s->current_addr = s->current_addr->ai_next;
>> > - s->fd = inet_connect_addr(s->current_addr, &in_progress, s);
>> > + s->fd = inet_connect_addr(s->current_addr, &in_progress, s, NULL);
> Doesn't this drop error messages?
Depends on what you mean by drop. The error messages previously were
sent to stdio, hidden in a log file or just invisible, depending on how
QEMU is run. (This is just for outgoing migration currently, so you
cannot rely on stdio as you can for command-line parsing).
> If yes, but it's healed later in the series, the temporary breakage
> still needs to be spelled out in the commit message.
No, it's not healed, which is why it's mentioned in the commit message
that future work is needed.
Paolo
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 20/25] qapi: add socket address types
2012-10-10 14:03 ` [Qemu-devel] [PATCH 20/25] qapi: add socket address types Paolo Bonzini
@ 2012-10-17 16:43 ` Markus Armbruster
2012-10-17 16:48 ` Paolo Bonzini
0 siblings, 1 reply; 72+ messages in thread
From: Markus Armbruster @ 2012-10-17 16:43 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, lcapitulino
Paolo Bonzini <pbonzini@redhat.com> writes:
> Acked-by: Luiz Capitulino <lcapitulino@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> qapi-schema.json | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file modificato, 53 inserzioni(+)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index f9dbdae..d40b5fc 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2505,6 +2505,59 @@
> 'opts': 'NetClientOptions' } }
>
> ##
> +# @IPSocketAddress
> +#
> +# Captures the destination address of an IP socket
What's a "destination address of an IP socket"?
> +#
> +# @host: host part of the address
> +#
> +# @port: port part of the address, or lowest port if @to is present
> +#
> +# @to: highest port to try
> +#
> +# @ipv4: whether to accept IPv4 addresses, default try both IPv4 and IPv6
> +# #optional
> +#
> +# @ipv6: whether to accept IPv6 addresses, default try both IPv4 and IPv6
> +# #optional
> +#
> +# Since 1.3
> +##
> +{ 'type': 'IPSocketAddress',
> + 'data': {
> + 'host': 'str',
> + 'port': 'str',
> + '*to': 'uint16',
Two port members, one is 'str', the other is 'uint16'. Ugly.
> + '*ipv4': 'bool',
> + '*ipv6': 'bool' } }
> +
> +##
> +# @UnixSocketAddress
> +#
> +# Captures the destination address of a Unix socket
What's a "destination address of a Unix socket"?
> +#
> +# @path: filesystem path to use
> +#
> +# Since 1.3
> +##
> +{ 'type': 'UnixSocketAddress',
> + 'data': {
> + 'path': 'str' } }
> +
> +##
> +# @SocketAddress
> +#
> +# Captures the address of a socket, which could also be a named file descriptor
> +#
> +# Since 1.3
> +##
> +{ 'union': 'SocketAddress',
> + 'data': {
> + 'inet': 'IPSocketAddress',
Call it InetSocketAddress, like 'inet', AF_INET, PF_INET, and so forth.
> + 'unix': 'UnixSocketAddress',
> + 'fd': 'String' } }
> +
> +##
> # @getfd:
> #
> # Receive a file descriptor via SCM rights and assign it a name
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 20/25] qapi: add socket address types
2012-10-17 16:43 ` Markus Armbruster
@ 2012-10-17 16:48 ` Paolo Bonzini
2012-10-17 16:50 ` Luiz Capitulino
2012-10-19 8:10 ` Markus Armbruster
0 siblings, 2 replies; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-17 16:48 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, lcapitulino
Il 17/10/2012 18:43, Markus Armbruster ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> Acked-by: Luiz Capitulino <lcapitulino@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>> qapi-schema.json | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file modificato, 53 inserzioni(+)
>>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index f9dbdae..d40b5fc 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -2505,6 +2505,59 @@
>> 'opts': 'NetClientOptions' } }
>>
>> ##
>> +# @IPSocketAddress
>> +#
>> +# Captures the destination address of an IP socket
>
> What's a "destination address of an IP socket"?
Any suggestions?
>> +#
>> +# @host: host part of the address
>> +#
>> +# @port: port part of the address, or lowest port if @to is present
>> +#
>> +# @to: highest port to try
>> +#
>> +# @ipv4: whether to accept IPv4 addresses, default try both IPv4 and IPv6
>> +# #optional
>> +#
>> +# @ipv6: whether to accept IPv6 addresses, default try both IPv4 and IPv6
>> +# #optional
>> +#
>> +# Since 1.3
>> +##
>> +{ 'type': 'IPSocketAddress',
>> + 'data': {
>> + 'host': 'str',
>> + 'port': 'str',
>> + '*to': 'uint16',
>
> Two port members, one is 'str', the other is 'uint16'. Ugly.
This is because port can be a service name. Using a union was deemed
overkill.
>> + '*ipv4': 'bool',
>> + '*ipv6': 'bool' } }
>> +
>> +##
>> +# @UnixSocketAddress
>> +#
>> +# Captures the destination address of a Unix socket
>
> What's a "destination address of a Unix socket"?
>
>> +#
>> +# @path: filesystem path to use
>> +#
>> +# Since 1.3
>> +##
>> +{ 'type': 'UnixSocketAddress',
>> + 'data': {
>> + 'path': 'str' } }
>> +
>> +##
>> +# @SocketAddress
>> +#
>> +# Captures the address of a socket, which could also be a named file descriptor
>> +#
>> +# Since 1.3
>> +##
>> +{ 'union': 'SocketAddress',
>> + 'data': {
>> + 'inet': 'IPSocketAddress',
>
> Call it InetSocketAddress, like 'inet', AF_INET, PF_INET, and so forth.
Ok. Luiz, at this point please unqueue the series. I'll send a pull
request myself to Anthony.
Paolo
>> + 'unix': 'UnixSocketAddress',
>> + 'fd': 'String' } }
>> +
>> +##
>> # @getfd:
>> #
>> # Receive a file descriptor via SCM rights and assign it a name
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 21/25] qemu-sockets: return IPSocketAddress from inet_parse
2012-10-10 14:03 ` [Qemu-devel] [PATCH 21/25] qemu-sockets: return IPSocketAddress from inet_parse Paolo Bonzini
@ 2012-10-17 16:49 ` Markus Armbruster
0 siblings, 0 replies; 72+ messages in thread
From: Markus Armbruster @ 2012-10-17 16:49 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, lcapitulino
Paolo Bonzini <pbonzini@redhat.com> writes:
> Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> qemu-sockets.c | 119 +++++++++++++++++++++++++++++++++++++--------------------
> 1 file modificato, 78 inserzioni(+), 41 rimozioni(-)
>
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index a37f35d..5946962 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -480,54 +480,91 @@ err:
> }
>
> /* compatibility wrapper */
> -static void inet_parse(QemuOpts *opts, const char *str, Error **errp)
> +static IPSocketAddress *inet_parse(const char *str, Error **errp)
> {
> + IPSocketAddress *addr;
> const char *optstr, *h;
> - char addr[64];
> + char host[64];
> char port[33];
> + int to;
> int pos;
>
> + addr = g_new(IPSocketAddress, 1);
> +
> /* parse address */
> if (str[0] == ':') {
> /* no host given */
> - addr[0] = '\0';
> + host[0] = '\0';
> if (1 != sscanf(str,":%32[^,]%n",port,&pos)) {
> error_setg(errp, "error parsing port in address '%s'", str);
> - return;
> + goto fail;
> }
> } else if (str[0] == '[') {
> /* IPv6 addr */
> - if (2 != sscanf(str,"[%64[^]]]:%32[^,]%n",addr,port,&pos)) {
> + if (2 != sscanf(str,"[%64[^]]]:%32[^,]%n",host,port,&pos)) {
Since you touch the line anyway, you could fix up style (space after
comma). More of the same below.
[...]
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 20/25] qapi: add socket address types
2012-10-17 16:48 ` Paolo Bonzini
@ 2012-10-17 16:50 ` Luiz Capitulino
2012-10-19 8:10 ` Markus Armbruster
1 sibling, 0 replies; 72+ messages in thread
From: Luiz Capitulino @ 2012-10-17 16:50 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Markus Armbruster, qemu-devel
On Wed, 17 Oct 2012 18:48:08 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 17/10/2012 18:43, Markus Armbruster ha scritto:
> > Paolo Bonzini <pbonzini@redhat.com> writes:
> >
> >> Acked-by: Luiz Capitulino <lcapitulino@redhat.com>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> ---
> >> qapi-schema.json | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> 1 file modificato, 53 inserzioni(+)
> >>
> >> diff --git a/qapi-schema.json b/qapi-schema.json
> >> index f9dbdae..d40b5fc 100644
> >> --- a/qapi-schema.json
> >> +++ b/qapi-schema.json
> >> @@ -2505,6 +2505,59 @@
> >> 'opts': 'NetClientOptions' } }
> >>
> >> ##
> >> +# @IPSocketAddress
> >> +#
> >> +# Captures the destination address of an IP socket
> >
> > What's a "destination address of an IP socket"?
>
> Any suggestions?
>
> >> +#
> >> +# @host: host part of the address
> >> +#
> >> +# @port: port part of the address, or lowest port if @to is present
> >> +#
> >> +# @to: highest port to try
> >> +#
> >> +# @ipv4: whether to accept IPv4 addresses, default try both IPv4 and IPv6
> >> +# #optional
> >> +#
> >> +# @ipv6: whether to accept IPv6 addresses, default try both IPv4 and IPv6
> >> +# #optional
> >> +#
> >> +# Since 1.3
> >> +##
> >> +{ 'type': 'IPSocketAddress',
> >> + 'data': {
> >> + 'host': 'str',
> >> + 'port': 'str',
> >> + '*to': 'uint16',
> >
> > Two port members, one is 'str', the other is 'uint16'. Ugly.
>
> This is because port can be a service name. Using a union was deemed
> overkill.
>
> >> + '*ipv4': 'bool',
> >> + '*ipv6': 'bool' } }
> >> +
> >> +##
> >> +# @UnixSocketAddress
> >> +#
> >> +# Captures the destination address of a Unix socket
> >
> > What's a "destination address of a Unix socket"?
> >
> >> +#
> >> +# @path: filesystem path to use
> >> +#
> >> +# Since 1.3
> >> +##
> >> +{ 'type': 'UnixSocketAddress',
> >> + 'data': {
> >> + 'path': 'str' } }
> >> +
> >> +##
> >> +# @SocketAddress
> >> +#
> >> +# Captures the address of a socket, which could also be a named file descriptor
> >> +#
> >> +# Since 1.3
> >> +##
> >> +{ 'union': 'SocketAddress',
> >> + 'data': {
> >> + 'inet': 'IPSocketAddress',
> >
> > Call it InetSocketAddress, like 'inet', AF_INET, PF_INET, and so forth.
>
> Ok. Luiz, at this point please unqueue the series. I'll send a pull
> request myself to Anthony.
Done.
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server
2012-10-10 14:02 [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Paolo Bonzini
` (25 preceding siblings ...)
2012-10-16 13:09 ` [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Luiz Capitulino
@ 2012-10-17 17:02 ` Markus Armbruster
26 siblings, 0 replies; 72+ messages in thread
From: Markus Armbruster @ 2012-10-17 17:02 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, lcapitulino
Paolo Bonzini <pbonzini@redhat.com> writes:
> Luiz,
>
> The following changes since commit b4ae3cfa57b8c1bdbbd7b7d420971e9171203ade:
>
> ssi: Add slave autoconnect helper (2012-10-10 11:13:32 +1000)
>
> are available in the git repository at:
>
> git://github.com/bonzini/qemu.git nbd-next
>
> for you to fetch changes up to 7a057978b8b8f7578c03a470ffd8c486a778497a:
>
> hmp: add NBD server commands (2012-10-10 13:44:09 +0200)
Reviewed up to 21/25, out of steam for today.
Paolo, you did a huge chunk of dirty work; thank you so much!
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 13/25] vnc: add error propagation to vnc_display_open
2012-10-17 15:48 ` Paolo Bonzini
@ 2012-10-19 7:49 ` Markus Armbruster
2012-10-19 8:40 ` Paolo Bonzini
0 siblings, 1 reply; 72+ messages in thread
From: Markus Armbruster @ 2012-10-19 7:49 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, lcapitulino
Paolo Bonzini <pbonzini@redhat.com> writes:
> Il 17/10/2012 17:17, Markus Armbruster ha scritto:
>>> > +fail:
>>> > + if (!error_is_set(errp)) {
>>> > + error_set(errp, QERR_VNC_SERVER_FAILED, display);
>> How can we get here with no error set?
>>
>> 1. !vnc_display (first goto fail).
>
> This can be fixed up to give a separate error.
>
>> 2. unit_connect() or inet_listen() return failure, but don't set error.
>>
>> 3. unix_listen() or inet_listen() return failure, but don't set error.
>>
>> Can 2. or 3. happen?
>>
>> If yes, these functions suck. If no, let's fix up 1. to set a suitable
>> error, and drop the uninformative generic error here.
>>
>
> It can at this point in the series, but not at the end.
Feel free to make the fix up at the end then.
> I tried to split this one into many commits, but I wasn't sure it was
> worth to make a mini-series out of one function. In retrospect
> it was.
Review of a long series is unrewarding when the patches are all perfect
;)
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 15/25] qemu-sockets: add error propagation to inet_connect_addr
2012-10-17 15:50 ` Paolo Bonzini
@ 2012-10-19 7:59 ` Markus Armbruster
2012-10-19 8:50 ` Paolo Bonzini
0 siblings, 1 reply; 72+ messages in thread
From: Markus Armbruster @ 2012-10-19 7:59 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, lcapitulino
Paolo Bonzini <pbonzini@redhat.com> writes:
> Il 17/10/2012 17:40, Markus Armbruster ha scritto:
>>> > if (s->current_addr) {
>>> > while (s->current_addr->ai_next != NULL && s->fd < 0) {
>>> > s->current_addr = s->current_addr->ai_next;
>>> > - s->fd = inet_connect_addr(s->current_addr, &in_progress, s);
>>> > + s->fd = inet_connect_addr(s->current_addr, &in_progress, s, NULL);
>> Doesn't this drop error messages?
>
> Depends on what you mean by drop. The error messages previously were
> sent to stdio, hidden in a log file or just invisible, depending on how
> QEMU is run. (This is just for outgoing migration currently, so you
> cannot rely on stdio as you can for command-line parsing).
I agree the existing error reporting is very bad, but even very bad
reporting is (slightly) better than no error reporting.
Moreover, there's a use case where errors are reported in the correct
place now: -monitor stdio. I'm afraid your patch hurts it.
>> If yes, but it's healed later in the series, the temporary breakage
>> still needs to be spelled out in the commit message.
>
> No, it's not healed, which is why it's mentioned in the commit message
> that future work is needed.
I really hate "reward" submissions of tons of useful work with demands
for more even work... but here goes anyway: what's your take on
completing the job? Straightfoward, or another ton of work?
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 20/25] qapi: add socket address types
2012-10-17 16:48 ` Paolo Bonzini
2012-10-17 16:50 ` Luiz Capitulino
@ 2012-10-19 8:10 ` Markus Armbruster
2012-10-19 8:39 ` Paolo Bonzini
1 sibling, 1 reply; 72+ messages in thread
From: Markus Armbruster @ 2012-10-19 8:10 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, lcapitulino
Paolo Bonzini <pbonzini@redhat.com> writes:
> Il 17/10/2012 18:43, Markus Armbruster ha scritto:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>>> Acked-by: Luiz Capitulino <lcapitulino@redhat.com>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>> qapi-schema.json | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file modificato, 53 inserzioni(+)
>>>
>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>> index f9dbdae..d40b5fc 100644
>>> --- a/qapi-schema.json
>>> +++ b/qapi-schema.json
>>> @@ -2505,6 +2505,59 @@
>>> 'opts': 'NetClientOptions' } }
>>>
>>> ##
>>> +# @IPSocketAddress
>>> +#
>>> +# Captures the destination address of an IP socket
>>
>> What's a "destination address of an IP socket"?
>
> Any suggestions?
"socket address in the Internet namespace"?
Actually, it's either an address or an address range (when @to is
present).
>>> +#
>>> +# @host: host part of the address
>>> +#
>>> +# @port: port part of the address, or lowest port if @to is present
>>> +#
>>> +# @to: highest port to try
>>> +#
>>> +# @ipv4: whether to accept IPv4 addresses, default try both IPv4 and IPv6
>>> +# #optional
>>> +#
>>> +# @ipv6: whether to accept IPv6 addresses, default try both IPv4 and IPv6
>>> +# #optional
>>> +#
>>> +# Since 1.3
>>> +##
>>> +{ 'type': 'IPSocketAddress',
>>> + 'data': {
>>> + 'host': 'str',
>>> + 'port': 'str',
>>> + '*to': 'uint16',
>>
>> Two port members, one is 'str', the other is 'uint16'. Ugly.
>
> This is because port can be a service name. Using a union was deemed
> overkill.
Two ways to reduce the ugliness:
1. Make @to a string, too. Yes, users don't normally want to specify
the upper bound as service name, but if a user wanted to, it would work
just fine.
2. Use a number of ports instead of an upper port bound: replace @to by
@port+@nports.
>>> + '*ipv4': 'bool',
>>> + '*ipv6': 'bool' } }
>>> +
>>> +##
>>> +# @UnixSocketAddress
>>> +#
>>> +# Captures the destination address of a Unix socket
>>
>> What's a "destination address of a Unix socket"?
"Socket address in the local namespace"
[...]
>>> +#
>>> +# @path: filesystem path to use
>>> +#
>>> +# Since 1.3
>>> +##
>>> +{ 'type': 'UnixSocketAddress',
>>> + 'data': {
>>> + 'path': 'str' } }
>>> +
>>> +##
>>> +# @SocketAddress
>>> +#
>>> +# Captures the address of a socket, which could also be a named
>>> file descriptor
>>> +#
>>> +# Since 1.3
>>> +##
>>> +{ 'union': 'SocketAddress',
>>> + 'data': {
>>> + 'inet': 'IPSocketAddress',
>>
>> Call it InetSocketAddress, like 'inet', AF_INET, PF_INET, and so forth.
>
> Ok. Luiz, at this point please unqueue the series. I'll send a pull
> request myself to Anthony.
>
> Paolo
>
>>> + 'unix': 'UnixSocketAddress',
>>> + 'fd': 'String' } }
'String'? Do you mean 'str'?
>>> +
>>> +##
>>> # @getfd:
>>> #
>>> # Receive a file descriptor via SCM rights and assign it a name
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 22/25] qemu-sockets: add socket_listen, socket_connect, socket_parse
2012-10-10 14:03 ` [Qemu-devel] [PATCH 22/25] qemu-sockets: add socket_listen, socket_connect, socket_parse Paolo Bonzini
@ 2012-10-19 8:15 ` Markus Armbruster
2012-10-19 8:37 ` Paolo Bonzini
0 siblings, 1 reply; 72+ messages in thread
From: Markus Armbruster @ 2012-10-19 8:15 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, lcapitulino
Paolo Bonzini <pbonzini@redhat.com> writes:
> These are QAPI-friendly versions of the qemu-sockets functions. They
> support IP sockets, Unix sockets, and named file descriptors, using a
> QAPI union to dispatch to the correct function.
>
> Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> Makefile | 2 +-
> qemu-sockets.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> qemu-tool.c | 6 ++++
> qemu_socket.h | 5 +++
> 4 file modificati, 111 inserzioni(+). 1 rimozione(-)
>
> diff --git a/Makefile b/Makefile
> index a9c22bf..b8b1f3c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -159,7 +159,7 @@ qemu-img.o: qemu-img-cmds.h
>
> tools-obj-y = $(oslib-obj-y) $(trace-obj-y) qemu-tool.o qemu-timer.o \
> qemu-timer-common.o main-loop.o notify.o \
> - iohandler.o cutils.o iov.o async.o
> + iohandler.o cutils.o iov.o async.o error.o
> tools-obj-$(CONFIG_POSIX) += compatfd.o
>
> qemu-img$(EXESUF): qemu-img.o $(tools-obj-y) $(block-obj-y) $(qapi-obj-y) \
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 5946962..e8d0a3c 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -22,6 +22,7 @@
> #include <errno.h>
> #include <unistd.h>
>
> +#include "monitor.h"
> #include "qemu_socket.h"
> #include "qemu-common.h" /* for qemu_isdigit */
> #include "main-loop.h"
> @@ -845,6 +846,104 @@ int unix_nonblocking_connect(const char *path,
> return sock;
> }
>
> +SocketAddress *socket_parse(const char *str, Error **errp)
> +{
> + SocketAddress *addr = NULL;
> +
> + addr = g_new(SocketAddress, 1);
> + if (strstart(str, "unix:", NULL)) {
> + if (str[5] == '\0') {
> + error_setg(errp, "invalid Unix socket address\n");
> + goto fail;
> + } else {
> + addr->kind = SOCKET_ADDRESS_KIND_UNIX;
> + addr->q_unix = g_new(UnixSocketAddress, 1);
> + addr->q_unix->path = g_strdup(str + 5);
> + }
> + } else if (strstart(str, "fd:", NULL)) {
> + if (str[3] == '\0') {
> + error_setg(errp, "invalid file descriptor address\n");
> + goto fail;
> + } else {
> + addr->kind = SOCKET_ADDRESS_KIND_FD;
> + addr->fd = g_new(String, 1);
> + addr->fd->str = g_strdup(str + 3);
> + }
> + } else {
> + addr->kind = SOCKET_ADDRESS_KIND_INET;
> + addr->inet = g_new(IPSocketAddress, 1);
> + addr->inet = inet_parse(str, errp);
> + if (addr->inet == NULL) {
> + goto fail;
> + }
> + }
> + return addr;
> +
> +fail:
> + qapi_free_SocketAddress(addr);
> + return NULL;
> +}
Yet another ad hoc parser. Have you considered sticking to QemuOpts
syntax?
Hmm, I guess it's for use with the existing ad hoc parser inet_parse().
Correct?
[...]
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 23/25] block: add close notifiers
2012-10-10 14:03 ` [Qemu-devel] [PATCH 23/25] block: add close notifiers Paolo Bonzini
@ 2012-10-19 8:21 ` Markus Armbruster
2012-10-19 9:38 ` Paolo Bonzini
2012-10-19 8:26 ` Markus Armbruster
1 sibling, 1 reply; 72+ messages in thread
From: Markus Armbruster @ 2012-10-19 8:21 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, lcapitulino
Paolo Bonzini <pbonzini@redhat.com> writes:
> The first user of close notifiers will be the embedded NBD server.
> It is possible to use them to do some of the ad hoc processing
> (e.g. for block jobs and I/O limits) that is currently done by
> bdrv_close.
>
> Acked-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> Makefile.objs | 4 ++--
> block.c | 19 ++++++++++++++-----
> block.h | 1 +
> block_int.h | 2 ++
> 4 file modificati, 19 inserzioni(+), 7 rimozioni(-)
>
> diff --git a/Makefile.objs b/Makefile.objs
> index 3f16d67..ca67885 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -43,7 +43,7 @@ coroutine-obj-$(CONFIG_WIN32) += coroutine-win32.o
>
> block-obj-y = cutils.o iov.o cache-utils.o qemu-option.o module.o async.o
> block-obj-y += nbd.o block.o blockjob.o aio.o aes.o qemu-config.o
> -block-obj-y += qemu-progress.o qemu-sockets.o uri.o
> +block-obj-y += qemu-progress.o qemu-sockets.o uri.o notify.o
> block-obj-y += $(coroutine-obj-y) $(qobject-obj-y) $(version-obj-y)
> block-obj-$(CONFIG_POSIX) += posix-aio-compat.o
> block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
> @@ -94,7 +94,7 @@ common-obj-y += bt-host.o bt-vhci.o
> common-obj-y += dma-helpers.o
> common-obj-y += iov.o acl.o
> common-obj-$(CONFIG_POSIX) += compatfd.o
> -common-obj-y += notify.o event_notifier.o
> +common-obj-y += event_notifier.o
> common-obj-y += qemu-timer.o qemu-timer-common.o
> common-obj-y += qtest.o
> common-obj-y += vl.o
> diff --git a/block.c b/block.c
> index e95f613..56426a9 100644
> --- a/block.c
> +++ b/block.c
> @@ -30,6 +30,7 @@
> #include "module.h"
> #include "qjson.h"
> #include "sysemu.h"
> +#include "notify.h"
> #include "qemu-coroutine.h"
> #include "qmp-commands.h"
> #include "qemu-timer.h"
> @@ -312,9 +313,16 @@ BlockDriverState *bdrv_new(const char *device_name)
> QTAILQ_INSERT_TAIL(&bdrv_states, bs, list);
> }
> bdrv_iostatus_disable(bs);
> + notifier_list_init(&bs->close_notifiers);
> +
> return bs;
> }
>
> +void bdrv_add_close_notifier(BlockDriverState *bs, Notifier *notify)
> +{
> + notifier_list_add(&bs->close_notifiers, notify);
> +}
> +
> BlockDriver *bdrv_find_format(const char *format_name)
> {
> BlockDriver *drv1;
> @@ -1098,12 +1106,13 @@ void bdrv_reopen_abort(BDRVReopenState *reopen_state)
> void bdrv_close(BlockDriverState *bs)
> {
> bdrv_flush(bs);
> - if (bs->drv) {
> - if (bs->job) {
> - block_job_cancel_sync(bs->job);
> - }
> - bdrv_drain_all();
> + if (bs->job) {
> + block_job_cancel_sync(bs->job);
> + }
> + bdrv_drain_all();
Dropping the bs->drv condition in a separate commit gives you a nice
place to explain why it's fine: the commit message. I figure it is, but
it's not 100% obvious.
> + notifier_list_notify(&bs->close_notifiers, bs);
>
> + if (bs->drv) {
> if (bs == bs_snapshots) {
> bs_snapshots = NULL;
> }
[...]
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 23/25] block: add close notifiers
2012-10-10 14:03 ` [Qemu-devel] [PATCH 23/25] block: add close notifiers Paolo Bonzini
2012-10-19 8:21 ` Markus Armbruster
@ 2012-10-19 8:26 ` Markus Armbruster
1 sibling, 0 replies; 72+ messages in thread
From: Markus Armbruster @ 2012-10-19 8:26 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, lcapitulino
Paolo Bonzini <pbonzini@redhat.com> writes:
> The first user of close notifiers will be the embedded NBD server.
> It is possible to use them to do some of the ad hoc processing
> (e.g. for block jobs and I/O limits) that is currently done by
> bdrv_close.
If the second sentence is an idea for future work, you could make that
clearer by wriging "It would be possible to use them".
[...]
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 24/25] qmp: add NBD server commands
2012-10-10 14:03 ` [Qemu-devel] [PATCH 24/25] qmp: add NBD server commands Paolo Bonzini
2012-10-10 20:41 ` Eric Blake
@ 2012-10-19 8:31 ` Markus Armbruster
1 sibling, 0 replies; 72+ messages in thread
From: Markus Armbruster @ 2012-10-19 8:31 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, lcapitulino
Paolo Bonzini <pbonzini@redhat.com> writes:
> Adding an NBD server inside QEMU is trivial, since all the logic is
> in nbd.c and can be shared easily between qemu-nbd and QEMU itself.
> The main difference is that qemu-nbd serves a single unnamed export,
> while QEMU serves named exports.
For NBD noobs like me, a short paragraph on what's served to whom would
be useful.
Noob says code looks sane enough (for whatever that's worth).
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 25/25] hmp: add NBD server commands
2012-10-10 14:03 ` [Qemu-devel] [PATCH 25/25] hmp: " Paolo Bonzini
@ 2012-10-19 8:34 ` Markus Armbruster
2012-10-19 8:58 ` Paolo Bonzini
0 siblings, 1 reply; 72+ messages in thread
From: Markus Armbruster @ 2012-10-19 8:34 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, lcapitulino
Paolo Bonzini <pbonzini@redhat.com> writes:
> Acked-by: Luiz Capitulino <lcapitulino@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hmp-commands.hx | 29 +++++++++++++++++++++++++++++
> hmp.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> hmp.h | 2 ++
> 3 file modificati, 86 inserzioni(+)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index e0b537d..f5d9204 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1274,6 +1274,35 @@ Remove all matches from the access control list, and set the default
> policy back to @code{deny}.
> ETEXI
>
> + {
> + .name = "nbd_server_start",
> + .args_type = "writable:-w,uri:s",
> + .params = "nbd_server_start [-w] host:port",
> + .help = "serve block devices on the given host and port",
> + .mhandler.cmd = hmp_nbd_server_start,
> + },
> +STEXI
> +@item nbd_server_start @var{host}:@var{port}
> +@findex nbd_server_start
> +Start an NBD server on the given host and/or port, and serve all of the
> +virtual machine's block devices that have an inserted media on it.
> +The @option{-w} option makes the devices writable.
> +ETEXI
> +
> + {
> + .name = "nbd_server_stop",
> + .args_type = "",
> + .params = "nbd_server_stop",
> + .help = "stop serving block devices using the NBD protocol",
> + .mhandler.cmd = hmp_nbd_server_stop,
> + },
> +STEXI
> +@item nbd_server_stop
> +@findex nbd_server_stop
> +Stop the QEMU embedded NBD server.
> +ETEXI
> +
> +
Why's nbd_server_add not needed in HMP?
Oh, it's because HMP's nbd_server_start auto-adds *all* block devices,
unlinke QMP's nbd-server-start.
Are you sure that's a good idea?
> #if defined(TARGET_I386)
>
> {
> diff --git a/hmp.c b/hmp.c
> index 70bdec2..edf6397 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -18,6 +18,7 @@
> #include "qemu-option.h"
> #include "qemu-timer.h"
> #include "qmp-commands.h"
> +#include "qemu_socket.h"
> #include "monitor.h"
> #include "console.h"
>
> @@ -1209,3 +1210,57 @@ void hmp_screen_dump(Monitor *mon, const QDict *qdict)
> qmp_screendump(filename, &err);
> hmp_handle_error(mon, &err);
> }
> +
> +void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
> +{
> + const char *uri = qdict_get_str(qdict, "uri");
> + int writable = qdict_get_try_bool(qdict, "writable", 0);
> + Error *local_err = NULL;
> + BlockInfoList *block_list, *info;
> + SocketAddress *addr;
> +
> + /* First check if the address is valid and start the server. */
> + addr = socket_parse(uri, &local_err);
> + if (local_err != NULL) {
> + goto exit;
> + }
> +
> + qmp_nbd_server_start(addr, &local_err);
> + qapi_free_SocketAddress(addr);
> + if (local_err != NULL) {
> + goto exit;
> + }
> +
> + /* Then try adding all block devices. If one fails, close all and
> + * exit.
> + */
> + block_list = qmp_query_block(NULL);
> +
> + for (info = block_list; info; info = info->next) {
> + if (!info->value->has_inserted) {
> + continue;
> + }
> +
> + qmp_nbd_server_add(info->value->device,
> + true, !info->value->inserted->ro && writable,
> + &local_err);
> +
> + if (local_err != NULL) {
> + qmp_nbd_server_stop(NULL);
> + break;
> + }
> + }
Here's where it auto-adds block devices.
> +
> + qapi_free_BlockInfoList(block_list);
> +
> +exit:
> + hmp_handle_error(mon, &local_err);
> +}
> +
> +void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict)
> +{
> + Error *errp = NULL;
> +
> + qmp_nbd_server_stop(&errp);
> + hmp_handle_error(mon, &errp);
> +}
> diff --git a/hmp.h b/hmp.h
> index 71ea384..45b7c48 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -75,5 +75,7 @@ void hmp_getfd(Monitor *mon, const QDict *qdict);
> void hmp_closefd(Monitor *mon, const QDict *qdict);
> void hmp_send_key(Monitor *mon, const QDict *qdict);
> void hmp_screen_dump(Monitor *mon, const QDict *qdict);
> +void hmp_nbd_server_start(Monitor *mon, const QDict *qdict);
> +void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
>
> #endif
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 22/25] qemu-sockets: add socket_listen, socket_connect, socket_parse
2012-10-19 8:15 ` Markus Armbruster
@ 2012-10-19 8:37 ` Paolo Bonzini
0 siblings, 0 replies; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-19 8:37 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, lcapitulino
> > These are QAPI-friendly versions of the qemu-sockets functions.
> > They support IP sockets, Unix sockets, and named file descriptors, using
> > a QAPI union to dispatch to the correct function.
> >
> > Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> > Makefile | 2 +-
> > qemu-sockets.c | 99
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > qemu-tool.c | 6 ++++
> > qemu_socket.h | 5 +++
> > 4 file modificati, 111 inserzioni(+). 1 rimozione(-)
> >
> > diff --git a/Makefile b/Makefile
> > index a9c22bf..b8b1f3c 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -159,7 +159,7 @@ qemu-img.o: qemu-img-cmds.h
> >
> > tools-obj-y = $(oslib-obj-y) $(trace-obj-y) qemu-tool.o
> > qemu-timer.o \
> > qemu-timer-common.o main-loop.o notify.o \
> > - iohandler.o cutils.o iov.o async.o
> > + iohandler.o cutils.o iov.o async.o error.o
> > tools-obj-$(CONFIG_POSIX) += compatfd.o
> >
> > qemu-img$(EXESUF): qemu-img.o $(tools-obj-y) $(block-obj-y)
> > $(qapi-obj-y) \
> > diff --git a/qemu-sockets.c b/qemu-sockets.c
> > index 5946962..e8d0a3c 100644
> > --- a/qemu-sockets.c
> > +++ b/qemu-sockets.c
> > @@ -22,6 +22,7 @@
> > #include <errno.h>
> > #include <unistd.h>
> >
> > +#include "monitor.h"
> > #include "qemu_socket.h"
> > #include "qemu-common.h" /* for qemu_isdigit */
> > #include "main-loop.h"
> > @@ -845,6 +846,104 @@ int unix_nonblocking_connect(const char
> > *path,
> > return sock;
> > }
> >
> > +SocketAddress *socket_parse(const char *str, Error **errp)
> > +{
> > + SocketAddress *addr = NULL;
> > +
> > + addr = g_new(SocketAddress, 1);
> > + if (strstart(str, "unix:", NULL)) {
> > + if (str[5] == '\0') {
> > + error_setg(errp, "invalid Unix socket address\n");
> > + goto fail;
> > + } else {
> > + addr->kind = SOCKET_ADDRESS_KIND_UNIX;
> > + addr->q_unix = g_new(UnixSocketAddress, 1);
> > + addr->q_unix->path = g_strdup(str + 5);
> > + }
> > + } else if (strstart(str, "fd:", NULL)) {
> > + if (str[3] == '\0') {
> > + error_setg(errp, "invalid file descriptor address\n");
> > + goto fail;
> > + } else {
> > + addr->kind = SOCKET_ADDRESS_KIND_FD;
> > + addr->fd = g_new(String, 1);
> > + addr->fd->str = g_strdup(str + 3);
> > + }
> > + } else {
> > + addr->kind = SOCKET_ADDRESS_KIND_INET;
> > + addr->inet = g_new(IPSocketAddress, 1);
> > + addr->inet = inet_parse(str, errp);
> > + if (addr->inet == NULL) {
> > + goto fail;
> > + }
> > + }
> > + return addr;
> > +
> > +fail:
> > + qapi_free_SocketAddress(addr);
> > + return NULL;
> > +}
>
> Yet another ad hoc parser. Have you considered sticking to QemuOpts
> syntax?
This is legacy/HMP syntax support. You may wonder why introduce
_more_ legacy support, and the plan there is basically to replace
all the legacy-syntax _connect and _listen calls with socket_parse +
socket_connect/socket_listen, so that they automatically gain
Unix and FD support. This should also allow unification
of migration-tcp.c and migration-unix.c (not sure about migration-fd.c,
it's possible that incoming migration screws up the plan there).
> Hmm, I guess it's for use with the existing ad hoc parser
> inet_parse().
> Correct?
Roughly, see above.
Paolo
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 20/25] qapi: add socket address types
2012-10-19 8:10 ` Markus Armbruster
@ 2012-10-19 8:39 ` Paolo Bonzini
0 siblings, 0 replies; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-19 8:39 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, lcapitulino
> >> Two port members, one is 'str', the other is 'uint16'. Ugly.
> >
> > This is because port can be a service name. Using a union was
> > deemed
> > overkill.
>
> Two ways to reduce the ugliness:
>
> 1. Make @to a string, too. Yes, users don't normally want to specify
> the upper bound as service name, but if a user wanted to, it would
> work just fine.
>
> 2. Use a number of ports instead of an upper port bound: replace @to
> by @port+@nports.
Both considered, but they make it an absolute pain to deal with the
current QemuOpts code that expects a @to and expects it to be an
integer.
> >>> + '*ipv4': 'bool',
> >>> + '*ipv6': 'bool' } }
> >>> +
> >>> +##
> >>> +# @UnixSocketAddress
> >>> +#
> >>> +# Captures the destination address of a Unix socket
> >>
> >> What's a "destination address of a Unix socket"?
>
> "Socket address in the local namespace"
Ok.
> >>> + 'unix': 'UnixSocketAddress',
> >>> + 'fd': 'String' } }
>
> 'String'? Do you mean 'str'?
'String' is a boxed 'str'. It gives more freedom to extend the API
later in a backwards-compatible way.
Paolo
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 13/25] vnc: add error propagation to vnc_display_open
2012-10-19 7:49 ` Markus Armbruster
@ 2012-10-19 8:40 ` Paolo Bonzini
0 siblings, 0 replies; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-19 8:40 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, lcapitulino
> > I tried to split this one into many commits, but I wasn't sure it
> > was worth to make a mini-series out of one function. In retrospect
> > it was.
>
> Review of a long series is unrewarding when the patches are all
> perfect ;)
LOL, will split the patch anyway.
Paolo
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 15/25] qemu-sockets: add error propagation to inet_connect_addr
2012-10-19 7:59 ` Markus Armbruster
@ 2012-10-19 8:50 ` Paolo Bonzini
0 siblings, 0 replies; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-19 8:50 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Orit Wasserman, qemu-devel, lcapitulino
> >> If yes, but it's healed later in the series, the temporary
> >> breakage still needs to be spelled out in the commit message.
> >
> > No, it's not healed, which is why it's mentioned in the commit
> > message that future work is needed.
>
> I really hate "reward" submissions of tons of useful work with demands
> for more even work... but here goes anyway: what's your take on
> completing the job? Straightfoward, or another ton of work?
Straightforward, also because we have exactly one user of asynchronous
connection (migration). But I'll let Orit do it. :)
Paolo
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 25/25] hmp: add NBD server commands
2012-10-19 8:34 ` Markus Armbruster
@ 2012-10-19 8:58 ` Paolo Bonzini
2012-10-19 12:44 ` Luiz Capitulino
2012-10-19 12:44 ` Markus Armbruster
0 siblings, 2 replies; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-19 8:58 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, lcapitulino
> Why's nbd_server_add not needed in HMP?
>
> Oh, it's because HMP's nbd_server_start auto-adds *all* block
> devices, unlinke QMP's nbd-server-start.
>
> Are you sure that's a good idea?
Yes. Now that we have QMP we can go back and treat HMP as (mostly) a
debugging interface as it was meant to be.
You don't need any fine-grained control in that case. From the server's
point of view, serving all devices or just one is exactly the same thing.
If you use NBD for block migration, for example, you can choose _on the
source_ which devices you are migrating, but it doesn't harm if more
devices are exposed on the target.
The only case where this loses is when you hotplug a device and you want
to start serving it. I think this is a minor loss for HMP, but it is a
show-stopper for QMP which hence has to have nbd-server-add.
Paolo
> > #if defined(TARGET_I386)
> >
> > {
> > diff --git a/hmp.c b/hmp.c
> > index 70bdec2..edf6397 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -18,6 +18,7 @@
> > #include "qemu-option.h"
> > #include "qemu-timer.h"
> > #include "qmp-commands.h"
> > +#include "qemu_socket.h"
> > #include "monitor.h"
> > #include "console.h"
> >
> > @@ -1209,3 +1210,57 @@ void hmp_screen_dump(Monitor *mon, const
> > QDict *qdict)
> > qmp_screendump(filename, &err);
> > hmp_handle_error(mon, &err);
> > }
> > +
> > +void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
> > +{
> > + const char *uri = qdict_get_str(qdict, "uri");
> > + int writable = qdict_get_try_bool(qdict, "writable", 0);
> > + Error *local_err = NULL;
> > + BlockInfoList *block_list, *info;
> > + SocketAddress *addr;
> > +
> > + /* First check if the address is valid and start the server.
> > */
> > + addr = socket_parse(uri, &local_err);
> > + if (local_err != NULL) {
> > + goto exit;
> > + }
> > +
> > + qmp_nbd_server_start(addr, &local_err);
> > + qapi_free_SocketAddress(addr);
> > + if (local_err != NULL) {
> > + goto exit;
> > + }
> > +
> > + /* Then try adding all block devices. If one fails, close all
> > and
> > + * exit.
> > + */
> > + block_list = qmp_query_block(NULL);
> > +
> > + for (info = block_list; info; info = info->next) {
> > + if (!info->value->has_inserted) {
> > + continue;
> > + }
> > +
> > + qmp_nbd_server_add(info->value->device,
> > + true, !info->value->inserted->ro &&
> > writable,
> > + &local_err);
> > +
> > + if (local_err != NULL) {
> > + qmp_nbd_server_stop(NULL);
> > + break;
> > + }
> > + }
>
> Here's where it auto-adds block devices.
>
> > +
> > + qapi_free_BlockInfoList(block_list);
> > +
> > +exit:
> > + hmp_handle_error(mon, &local_err);
> > +}
> > +
> > +void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict)
> > +{
> > + Error *errp = NULL;
> > +
> > + qmp_nbd_server_stop(&errp);
> > + hmp_handle_error(mon, &errp);
> > +}
> > diff --git a/hmp.h b/hmp.h
> > index 71ea384..45b7c48 100644
> > --- a/hmp.h
> > +++ b/hmp.h
> > @@ -75,5 +75,7 @@ void hmp_getfd(Monitor *mon, const QDict *qdict);
> > void hmp_closefd(Monitor *mon, const QDict *qdict);
> > void hmp_send_key(Monitor *mon, const QDict *qdict);
> > void hmp_screen_dump(Monitor *mon, const QDict *qdict);
> > +void hmp_nbd_server_start(Monitor *mon, const QDict *qdict);
> > +void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
> >
> > #endif
>
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 23/25] block: add close notifiers
2012-10-19 8:21 ` Markus Armbruster
@ 2012-10-19 9:38 ` Paolo Bonzini
0 siblings, 0 replies; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-19 9:38 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, lcapitulino
> > @@ -1098,12 +1106,13 @@ void bdrv_reopen_abort(BDRVReopenState *reopen_state)
> > void bdrv_close(BlockDriverState *bs)
> > {
> > bdrv_flush(bs);
> > - if (bs->drv) {
> > - if (bs->job) {
> > - block_job_cancel_sync(bs->job);
> > - }
> > - bdrv_drain_all();
> > + if (bs->job) {
> > + block_job_cancel_sync(bs->job);
> > + }
> > + bdrv_drain_all();
>
> Dropping the bs->drv condition in a separate commit gives you a nice
> place to explain why it's fine: the commit message. I figure it is,
> but it's not 100% obvious.
Will do, thanks.
Paolo
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 25/25] hmp: add NBD server commands
2012-10-19 8:58 ` Paolo Bonzini
@ 2012-10-19 12:44 ` Luiz Capitulino
2012-10-19 12:44 ` Markus Armbruster
1 sibling, 0 replies; 72+ messages in thread
From: Luiz Capitulino @ 2012-10-19 12:44 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Markus Armbruster, qemu-devel
On Fri, 19 Oct 2012 04:58:45 -0400 (EDT)
Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> > Why's nbd_server_add not needed in HMP?
> >
> > Oh, it's because HMP's nbd_server_start auto-adds *all* block
> > devices, unlinke QMP's nbd-server-start.
> >
> > Are you sure that's a good idea?
>
> Yes. Now that we have QMP we can go back and treat HMP as (mostly) a
> debugging interface as it was meant to be.
We can add it later case we regret, so I'd vote for the simpler interface
for now.
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 25/25] hmp: add NBD server commands
2012-10-19 8:58 ` Paolo Bonzini
2012-10-19 12:44 ` Luiz Capitulino
@ 2012-10-19 12:44 ` Markus Armbruster
2012-10-19 13:11 ` Paolo Bonzini
1 sibling, 1 reply; 72+ messages in thread
From: Markus Armbruster @ 2012-10-19 12:44 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, lcapitulino
Paolo Bonzini <pbonzini@redhat.com> writes:
>> Why's nbd_server_add not needed in HMP?
>>
>> Oh, it's because HMP's nbd_server_start auto-adds *all* block
>> devices, unlinke QMP's nbd-server-start.
>>
>> Are you sure that's a good idea?
>
> Yes. Now that we have QMP we can go back and treat HMP as (mostly) a
> debugging interface as it was meant to be.
>
> You don't need any fine-grained control in that case. From the server's
> point of view, serving all devices or just one is exactly the same thing.
> If you use NBD for block migration, for example, you can choose _on the
> source_ which devices you are migrating, but it doesn't harm if more
> devices are exposed on the target.
>
> The only case where this loses is when you hotplug a device and you want
> to start serving it. I think this is a minor loss for HMP, but it is a
> show-stopper for QMP which hence has to have nbd-server-add.
Apropos hotplug. The only way to unexport a block device is to stop the
NBD server outright. Once the device backend has been exported,
unplugging the device gets rid of the frontend, but the backend stays
until you stop the NBD server, or you kill the backend with the big
drive_del hammer. Makes me wonder whether we need QMP command
nbd-server-del.
Back to HMP.
I agree that we can the human monitor is again for human users only,
thus we don't have to encumber it with the need of tools. I might also
buy your argument that extra exports don't really hurt. But device plug
/ -unplug is just as relevant there as in QMP. If it's a show-stopper
for QMP, then I don't think it can be a "minor loss" for HMP.
Additionally, two details I don't like:
1. The HMP command looks just like the QMP command (name & arguments),
yet does something else.
2. What if we ever change our minds regarding fine-grained NBD export
control in the human monitor? Add a second command to start the server
without exporting everything? Add a "don't auto-export" argument to the
existing command?
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 25/25] hmp: add NBD server commands
2012-10-19 12:44 ` Markus Armbruster
@ 2012-10-19 13:11 ` Paolo Bonzini
2012-10-19 13:53 ` Markus Armbruster
0 siblings, 1 reply; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-19 13:11 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, lcapitulino
Il 19/10/2012 14:44, Markus Armbruster ha scritto:
> Apropos hotplug. The only way to unexport a block device is to stop the
> NBD server outright. Once the device backend has been exported,
> unplugging the device gets rid of the frontend, but the backend stays
> until you stop the NBD server, or you kill the backend with the big
> drive_del hammer.
Right. (Though for removable devices you can just eject it, which calls
the close notifier).
> Makes me wonder whether we need QMP command nbd-server-del.
Perhaps yes, but it can be added later.
> I agree that we can the human monitor is again for human users only,
> thus we don't have to encumber it with the need of tools. I might also
> buy your argument that extra exports don't really hurt. But device plug
> / -unplug is just as relevant there as in QMP. If it's a show-stopper
> for QMP, then I don't think it can be a "minor loss" for HMP.
The question is, who do we expect to use HMP at this time?
> 2. What if we ever change our minds regarding fine-grained NBD export
> control in the human monitor? Add a second command to start the server
> without exporting everything? Add a "don't auto-export" argument to the
> existing command?
The latter.
But indeed one alternative, possibly better, is to remove the auto-add
and add a "nbd_server_add -a" (for "all") flag to HMP. I think I'll
drop the last patch and send a pull request for the others (with your
suggested changes), while we discuss this.
Paolo
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 25/25] hmp: add NBD server commands
2012-10-19 13:11 ` Paolo Bonzini
@ 2012-10-19 13:53 ` Markus Armbruster
2012-10-19 13:57 ` Paolo Bonzini
0 siblings, 1 reply; 72+ messages in thread
From: Markus Armbruster @ 2012-10-19 13:53 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, lcapitulino
Paolo Bonzini <pbonzini@redhat.com> writes:
> Il 19/10/2012 14:44, Markus Armbruster ha scritto:
>> Apropos hotplug. The only way to unexport a block device is to stop the
>> NBD server outright. Once the device backend has been exported,
>> unplugging the device gets rid of the frontend, but the backend stays
>> until you stop the NBD server, or you kill the backend with the big
>> drive_del hammer.
>
> Right. (Though for removable devices you can just eject it, which calls
> the close notifier).
>
>> Makes me wonder whether we need QMP command nbd-server-del.
>
> Perhaps yes, but it can be added later.
Perhaps deleting the backend should automatically unexport it. I
suspect that's not the case for the automatic delete on unplug.
>> I agree that we can the human monitor is again for human users only,
>> thus we don't have to encumber it with the need of tools. I might also
>> buy your argument that extra exports don't really hurt. But device plug
>> / -unplug is just as relevant there as in QMP. If it's a show-stopper
>> for QMP, then I don't think it can be a "minor loss" for HMP.
>
> The question is, who do we expect to use HMP at this time?
Humans. I don't think we have data to justify narrower assumptions.
Some human users will happily use libvirt to unplug devices, others may
want to do it in the human monitor. "Users of the human monitor don't
want to do X" is a rather questionable argument for me.
>> 2. What if we ever change our minds regarding fine-grained NBD export
>> control in the human monitor? Add a second command to start the server
>> without exporting everything? Add a "don't auto-export" argument to the
>> existing command?
>
> The latter.
>
> But indeed one alternative, possibly better, is to remove the auto-add
> and add a "nbd_server_add -a" (for "all") flag to HMP. I think I'll
> drop the last patch and send a pull request for the others (with your
> suggested changes), while we discuss this.
I like the -a idea. In fact, I was pondering it myself :)
^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [Qemu-devel] [PATCH 25/25] hmp: add NBD server commands
2012-10-19 13:53 ` Markus Armbruster
@ 2012-10-19 13:57 ` Paolo Bonzini
0 siblings, 0 replies; 72+ messages in thread
From: Paolo Bonzini @ 2012-10-19 13:57 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, lcapitulino
Il 19/10/2012 15:53, Markus Armbruster ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> Il 19/10/2012 14:44, Markus Armbruster ha scritto:
>>> Apropos hotplug. The only way to unexport a block device is to stop the
>>> NBD server outright. Once the device backend has been exported,
>>> unplugging the device gets rid of the frontend, but the backend stays
>>> until you stop the NBD server, or you kill the backend with the big
>>> drive_del hammer.
>>
>> Right. (Though for removable devices you can just eject it, which calls
>> the close notifier).
>>
>>> Makes me wonder whether we need QMP command nbd-server-del.
>>
>> Perhaps yes, but it can be added later.
>
> Perhaps deleting the backend should automatically unexport it. I
> suspect that's not the case for the automatic delete on unplug.
No, because we keep a reference via drive_put_ref. But that's by
design, drive_del exists after all and we can add a more fine-grained
nbd_server_del too.
Paolo
^ permalink raw reply [flat|nested] 72+ messages in thread
end of thread, other threads:[~2012-10-19 13:58 UTC | newest]
Thread overview: 72+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-10 14:02 [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Paolo Bonzini
2012-10-10 14:02 ` [Qemu-devel] [PATCH 01/25] error: add error_set_errno and error_setg_errno Paolo Bonzini
2012-10-10 14:02 ` [Qemu-devel] [PATCH 02/25] qemu-sockets: add Error ** to all functions Paolo Bonzini
2012-10-10 14:02 ` [Qemu-devel] [PATCH 03/25] qemu-sockets: unix_listen and unix_connect are portable Paolo Bonzini
2012-10-10 14:02 ` [Qemu-devel] [PATCH 04/25] qemu-sockets: add nonblocking connect for Unix sockets Paolo Bonzini
2012-10-17 13:33 ` Markus Armbruster
2012-10-17 13:44 ` Paolo Bonzini
2012-10-10 14:02 ` [Qemu-devel] [PATCH 05/25] migration: avoid using error_is_set and thus relying on errp != NULL Paolo Bonzini
2012-10-17 13:36 ` Markus Armbruster
2012-10-17 13:37 ` Paolo Bonzini
2012-10-17 15:44 ` Markus Armbruster
2012-10-10 14:02 ` [Qemu-devel] [PATCH 06/25] migration: centralize call to migrate_fd_error() Paolo Bonzini
2012-10-10 14:02 ` [Qemu-devel] [PATCH 07/25] migration: use qemu-sockets to establish Unix sockets Paolo Bonzini
2012-10-17 14:30 ` Markus Armbruster
2012-10-10 14:02 ` [Qemu-devel] [PATCH 08/25] migration (outgoing): add error propagation for all protocols Paolo Bonzini
2012-10-17 14:43 ` Markus Armbruster
2012-10-17 14:50 ` Paolo Bonzini
2012-10-17 15:43 ` Markus Armbruster
2012-10-10 14:02 ` [Qemu-devel] [PATCH 09/25] migration (incoming): add error propagation to fd and exec protocols Paolo Bonzini
2012-10-10 14:02 ` [Qemu-devel] [PATCH 10/25] qemu-char: ask and print error information from qemu-sockets Paolo Bonzini
2012-10-10 14:02 ` [Qemu-devel] [PATCH 11/25] nbd: " Paolo Bonzini
2012-10-17 14:51 ` Markus Armbruster
2012-10-17 14:53 ` Paolo Bonzini
2012-10-10 14:02 ` [Qemu-devel] [PATCH 12/25] qemu-ga: " Paolo Bonzini
2012-10-10 14:02 ` [Qemu-devel] [PATCH 13/25] vnc: add error propagation to vnc_display_open Paolo Bonzini
2012-10-17 15:17 ` Markus Armbruster
2012-10-17 15:48 ` Paolo Bonzini
2012-10-19 7:49 ` Markus Armbruster
2012-10-19 8:40 ` Paolo Bonzini
2012-10-10 14:02 ` [Qemu-devel] [PATCH 14/25] qemu-sockets: include strerror or gai_strerror output in error messages Paolo Bonzini
2012-10-10 14:02 ` [Qemu-devel] [PATCH 15/25] qemu-sockets: add error propagation to inet_connect_addr Paolo Bonzini
2012-10-16 12:53 ` Luiz Capitulino
2012-10-17 15:40 ` Markus Armbruster
2012-10-17 15:50 ` Paolo Bonzini
2012-10-19 7:59 ` Markus Armbruster
2012-10-19 8:50 ` Paolo Bonzini
2012-10-10 14:02 ` [Qemu-devel] [PATCH 16/25] qemu-sockets: add error propagation to inet_dgram_opts Paolo Bonzini
2012-10-10 14:02 ` [Qemu-devel] [PATCH 17/25] qemu-sockets: add error propagation to inet_parse Paolo Bonzini
2012-10-10 14:02 ` [Qemu-devel] [PATCH 18/25] qemu-sockets: add error propagation to Unix socket functions Paolo Bonzini
2012-10-10 14:03 ` [Qemu-devel] [PATCH 19/25] build: add QAPI files to the tools Paolo Bonzini
2012-10-10 14:03 ` [Qemu-devel] [PATCH 20/25] qapi: add socket address types Paolo Bonzini
2012-10-17 16:43 ` Markus Armbruster
2012-10-17 16:48 ` Paolo Bonzini
2012-10-17 16:50 ` Luiz Capitulino
2012-10-19 8:10 ` Markus Armbruster
2012-10-19 8:39 ` Paolo Bonzini
2012-10-10 14:03 ` [Qemu-devel] [PATCH 21/25] qemu-sockets: return IPSocketAddress from inet_parse Paolo Bonzini
2012-10-17 16:49 ` Markus Armbruster
2012-10-10 14:03 ` [Qemu-devel] [PATCH 22/25] qemu-sockets: add socket_listen, socket_connect, socket_parse Paolo Bonzini
2012-10-19 8:15 ` Markus Armbruster
2012-10-19 8:37 ` Paolo Bonzini
2012-10-10 14:03 ` [Qemu-devel] [PATCH 23/25] block: add close notifiers Paolo Bonzini
2012-10-19 8:21 ` Markus Armbruster
2012-10-19 9:38 ` Paolo Bonzini
2012-10-19 8:26 ` Markus Armbruster
2012-10-10 14:03 ` [Qemu-devel] [PATCH 24/25] qmp: add NBD server commands Paolo Bonzini
2012-10-10 20:41 ` Eric Blake
2012-10-11 13:06 ` Paolo Bonzini
2012-10-11 13:14 ` Eric Blake
2012-10-11 13:22 ` Paolo Bonzini
2012-10-19 8:31 ` Markus Armbruster
2012-10-10 14:03 ` [Qemu-devel] [PATCH 25/25] hmp: " Paolo Bonzini
2012-10-19 8:34 ` Markus Armbruster
2012-10-19 8:58 ` Paolo Bonzini
2012-10-19 12:44 ` Luiz Capitulino
2012-10-19 12:44 ` Markus Armbruster
2012-10-19 13:11 ` Paolo Bonzini
2012-10-19 13:53 ` Markus Armbruster
2012-10-19 13:57 ` Paolo Bonzini
2012-10-16 13:09 ` [Qemu-devel] [PULL for Luiz 00/25] Combined qemu-sockets cleanup v2 + NBD server Luiz Capitulino
2012-10-16 13:10 ` Paolo Bonzini
2012-10-17 17:02 ` Markus Armbruster
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).