From patchwork Mon Sep 28 19:11:34 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Markus Armbruster X-Patchwork-Id: 34390 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [199.232.76.165]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id E203DB7BC1 for ; Tue, 29 Sep 2009 05:14:22 +1000 (EST) Received: from localhost ([127.0.0.1]:58347 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MsLfu-0000l0-9P for incoming@patchwork.ozlabs.org; Mon, 28 Sep 2009 15:14:18 -0400 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MsLdU-0000DP-9j for qemu-devel@nongnu.org; Mon, 28 Sep 2009 15:11:48 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MsLdP-0000CT-Fj for qemu-devel@nongnu.org; Mon, 28 Sep 2009 15:11:47 -0400 Received: from [199.232.76.173] (port=38792 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MsLdP-0000CQ-AB for qemu-devel@nongnu.org; Mon, 28 Sep 2009 15:11:43 -0400 Received: from oxygen.pond.sub.org ([213.239.205.148]:51134) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1MsLdO-0007eO-C9 for qemu-devel@nongnu.org; Mon, 28 Sep 2009 15:11:43 -0400 Received: from pike.pond.sub.org (pD9E39E9F.dip.t-dialin.net [217.227.158.159]) by oxygen.pond.sub.org (Postfix) with ESMTPA id D7336276D47 for ; Mon, 28 Sep 2009 21:11:40 +0200 (CEST) Received: by pike.pond.sub.org (Postfix, from userid 1000) id 71C0610099; Mon, 28 Sep 2009 21:11:35 +0200 (CEST) From: Markus Armbruster To: qemu-devel@nongnu.org Date: Mon, 28 Sep 2009 21:11:34 +0200 Message-Id: X-Mailer: git-send-email 1.6.0.6 In-Reply-To: References: X-detected-operating-system: by monty-python.gnu.org: GNU/Linux 2.6 (newer, 3) Subject: [Qemu-devel] [PATCH 2/3] Don't exit() in config_error() X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Propagating errors up the call chain is tedious. In startup code, we can take a shortcut: terminate the program. This is wrong elsewhere, the monitor in particular. config_error() tries to cater for both customers: it terminates the program unless its mon parameter tells it it's working for the monitor. Its users need to return status anyway (unless passing a null mon argument, which none do), which their users need to check. So this automatic exit buys us exactly nothing useful. Only the dangerous delusion that we can get away without returning status. Some of its users fell for that. Their callers continue executing after failure when working for the monitor. This bites monitor command host_net_add in two places: * net_slirp_init() continues after slirp_hostfwd(), slirp_guestfwd(), or slirp_smb() failed, and may end up reporting success. This happens for "host_net_add user guestfwd=foo": it complains about the invalid guest forwarding rule, then happily creates the user network without guest forwarding. * net_client_init() can't detect slirp_guestfwd() failure, and gets fooled by net_slirp_init() lying about success. Suppresses its "Could not initialize device" message. Add the missing error reporting, make sure errors are checked, and drop the exit() from config_error(). Signed-off-by: Markus Armbruster --- net.c | 83 ++++++++++++++++++++++++++++++++++++---------------------------- net.h | 4 +- vl.c | 9 ++++-- 3 files changed, 55 insertions(+), 41 deletions(-) diff --git a/net.c b/net.c index 61429ac..9e70f9c 100644 --- a/net.c +++ b/net.c @@ -650,7 +650,6 @@ static void config_error(Monitor *mon, const char *fmt, ...) } else { fprintf(stderr, "qemu: "); vfprintf(stderr, fmt, ap); - exit(1); } va_end(ap); } @@ -684,16 +683,16 @@ const char *legacy_bootp_filename; static QTAILQ_HEAD(slirp_stacks, SlirpState) slirp_stacks = QTAILQ_HEAD_INITIALIZER(slirp_stacks); -static void slirp_hostfwd(SlirpState *s, Monitor *mon, const char *redir_str, +static int slirp_hostfwd(SlirpState *s, Monitor *mon, const char *redir_str, + int legacy_format); +static int slirp_guestfwd(SlirpState *s, Monitor *mon, const char *config_str, int legacy_format); -static void slirp_guestfwd(SlirpState *s, Monitor *mon, const char *config_str, - int legacy_format); #ifndef _WIN32 static const char *legacy_smb_export; -static void slirp_smb(SlirpState *s, Monitor *mon, const char *exported_dir, - struct in_addr vserver_addr); +static int slirp_smb(SlirpState *s, Monitor *mon, const char *exported_dir, + struct in_addr vserver_addr); static void slirp_smb_cleanup(SlirpState *s); #else static inline void slirp_smb_cleanup(SlirpState *s) { } @@ -848,11 +847,13 @@ static int net_slirp_init(Monitor *mon, VLANState *vlan, const char *model, for (config = slirp_configs; config; config = config->next) { if (config->flags & SLIRP_CFG_HOSTFWD) { - slirp_hostfwd(s, mon, config->str, - config->flags & SLIRP_CFG_LEGACY); + if (slirp_hostfwd(s, mon, config->str, + config->flags & SLIRP_CFG_LEGACY) < 0) + return -1; } else { - slirp_guestfwd(s, mon, config->str, - config->flags & SLIRP_CFG_LEGACY); + if (slirp_guestfwd(s, mon, config->str, + config->flags & SLIRP_CFG_LEGACY) < 0) + return -1; } } #ifndef _WIN32 @@ -860,7 +861,8 @@ static int net_slirp_init(Monitor *mon, VLANState *vlan, const char *model, smb_export = legacy_smb_export; } if (smb_export) { - slirp_smb(s, mon, smb_export, smbsrv); + if (slirp_smb(s, mon, smb_export, smbsrv) < 0) + return -1; } #endif @@ -953,8 +955,8 @@ void net_slirp_hostfwd_remove(Monitor *mon, const QDict *qdict) monitor_printf(mon, "invalid format\n"); } -static void slirp_hostfwd(SlirpState *s, Monitor *mon, const char *redir_str, - int legacy_format) +static int slirp_hostfwd(SlirpState *s, Monitor *mon, const char *redir_str, + int legacy_format) { struct in_addr host_addr = { .s_addr = INADDR_ANY }; struct in_addr guest_addr = { .s_addr = 0 }; @@ -1009,11 +1011,13 @@ static void slirp_hostfwd(SlirpState *s, Monitor *mon, const char *redir_str, guest_port) < 0) { config_error(mon, "could not set up host forwarding rule '%s'\n", redir_str); + return -1; } - return; + return 0; fail_syntax: config_error(mon, "invalid host forwarding rule '%s'\n", redir_str); + return -1; } void net_slirp_hostfwd_add(Monitor *mon, const QDict *qdict) @@ -1037,7 +1041,7 @@ void net_slirp_hostfwd_add(Monitor *mon, const QDict *qdict) } -void net_slirp_redir(const char *redir_str) +int net_slirp_redir(const char *redir_str) { struct slirp_config_str *config; @@ -1047,10 +1051,10 @@ void net_slirp_redir(const char *redir_str) config->flags = SLIRP_CFG_HOSTFWD | SLIRP_CFG_LEGACY; config->next = slirp_configs; slirp_configs = config; - return; + return 0; } - slirp_hostfwd(QTAILQ_FIRST(&slirp_stacks), NULL, redir_str, 1); + return slirp_hostfwd(QTAILQ_FIRST(&slirp_stacks), NULL, redir_str, 1); } #ifndef _WIN32 @@ -1067,8 +1071,8 @@ static void slirp_smb_cleanup(SlirpState *s) } } -static void slirp_smb(SlirpState* s, Monitor *mon, const char *exported_dir, - struct in_addr vserver_addr) +static int slirp_smb(SlirpState* s, Monitor *mon, const char *exported_dir, + struct in_addr vserver_addr) { static int instance; char smb_conf[128]; @@ -1080,7 +1084,7 @@ static void slirp_smb(SlirpState* s, Monitor *mon, const char *exported_dir, if (mkdir(s->smb_dir, 0700) < 0) { config_error(mon, "could not create samba server dir '%s'\n", s->smb_dir); - return; + return -1; } snprintf(smb_conf, sizeof(smb_conf), "%s/%s", s->smb_dir, "smb.conf"); @@ -1089,7 +1093,7 @@ static void slirp_smb(SlirpState* s, Monitor *mon, const char *exported_dir, slirp_smb_cleanup(s); config_error(mon, "could not create samba server " "configuration file '%s'\n", smb_conf); - return; + return -1; } fprintf(f, "[global]\n" @@ -1120,23 +1124,26 @@ static void slirp_smb(SlirpState* s, Monitor *mon, const char *exported_dir, if (slirp_add_exec(s->slirp, 0, smb_cmdline, &vserver_addr, 139) < 0) { slirp_smb_cleanup(s); config_error(mon, "conflicting/invalid smbserver address\n"); + return -1; } + return 0; } /* automatic user mode samba server configuration (legacy interface) */ -void net_slirp_smb(const char *exported_dir) +int net_slirp_smb(const char *exported_dir) { struct in_addr vserver_addr = { .s_addr = 0 }; if (legacy_smb_export) { fprintf(stderr, "-smb given twice\n"); - exit(1); + return -1; } legacy_smb_export = exported_dir; if (!QTAILQ_EMPTY(&slirp_stacks)) { - slirp_smb(QTAILQ_FIRST(&slirp_stacks), NULL, exported_dir, - vserver_addr); + return slirp_smb(QTAILQ_FIRST(&slirp_stacks), NULL, exported_dir, + vserver_addr); } + return 0; } #endif /* !defined(_WIN32) */ @@ -1160,8 +1167,8 @@ static void guestfwd_read(void *opaque, const uint8_t *buf, int size) slirp_socket_recv(fwd->slirp, fwd->server, fwd->port, buf, size); } -static void slirp_guestfwd(SlirpState *s, Monitor *mon, const char *config_str, - int legacy_format) +static int slirp_guestfwd(SlirpState *s, Monitor *mon, const char *config_str, + int legacy_format) { struct in_addr server = { .s_addr = 0 }; struct GuestFwd *fwd; @@ -1204,14 +1211,14 @@ static void slirp_guestfwd(SlirpState *s, Monitor *mon, const char *config_str, config_error(mon, "could not open guest forwarding device '%s'\n", buf); qemu_free(fwd); - return; + return -1; } if (slirp_add_exec(s->slirp, 3, fwd->hd, &server, port) < 0) { config_error(mon, "conflicting/invalid host:port in guest forwarding " "rule '%s'\n", config_str); qemu_free(fwd); - return; + return -1; } fwd->server = server; fwd->port = port; @@ -1219,10 +1226,11 @@ static void slirp_guestfwd(SlirpState *s, Monitor *mon, const char *config_str, qemu_chr_add_handlers(fwd->hd, guestfwd_can_read, guestfwd_read, NULL, fwd); - return; + return 0; fail_syntax: config_error(mon, "invalid guest forwarding rule '%s'\n", config_str); + return -1; } void do_info_usernet(Monitor *mon) @@ -1372,7 +1380,7 @@ static void tap_send(void *opaque) */ #define TAP_DEFAULT_SNDBUF 1024*1024 -static void tap_set_sndbuf(TAPState *s, const char *sndbuf_str, Monitor *mon) +static int tap_set_sndbuf(TAPState *s, const char *sndbuf_str, Monitor *mon) { int sndbuf = TAP_DEFAULT_SNDBUF; @@ -1387,14 +1395,18 @@ static void tap_set_sndbuf(TAPState *s, const char *sndbuf_str, Monitor *mon) if (ioctl(s->fd, TUNSETSNDBUF, &sndbuf) == -1 && sndbuf_str) { config_error(mon, "TUNSETSNDBUF ioctl failed: %s\n", strerror(errno)); + return -1; } + return 0; } #else -static void tap_set_sndbuf(TAPState *s, const char *sndbuf_str, Monitor *mon) +static int tap_set_sndbuf(TAPState *s, const char *sndbuf_str, Monitor *mon) { if (sndbuf_str) { config_error(mon, "No '-net tap,sndbuf=' support available\n"); + return -1; } + return 0; } #endif /* TUNSETSNDBUF */ @@ -2603,10 +2615,10 @@ int net_client_init(Monitor *mon, const char *device, const char *p) config->flags = SLIRP_CFG_LEGACY; config->next = slirp_configs; slirp_configs = config; + ret = 0; } else { - slirp_guestfwd(QTAILQ_FIRST(&slirp_stacks), mon, p, 1); + ret = slirp_guestfwd(QTAILQ_FIRST(&slirp_stacks), mon, p, 1); } - ret = 0; } else #endif #ifdef _WIN32 @@ -2680,8 +2692,7 @@ int net_client_init(Monitor *mon, const char *device, const char *p) if (get_param_value(buf, sizeof(buf), "sndbuf", p)) { sndbuf_str = buf; } - tap_set_sndbuf(s, sndbuf_str, mon); - ret = 0; + ret = tap_set_sndbuf(s, sndbuf_str, mon); } else { ret = -1; } diff --git a/net.h b/net.h index 1479826..a8b6257 100644 --- a/net.h +++ b/net.h @@ -137,10 +137,10 @@ extern const char *legacy_bootp_filename; int net_client_init(Monitor *mon, const char *device, const char *p); void net_client_uninit(NICInfo *nd); int net_client_parse(const char *str); -void net_slirp_smb(const char *exported_dir); +int net_slirp_smb(const char *exported_dir); void net_slirp_hostfwd_add(Monitor *mon, const QDict *qdict); void net_slirp_hostfwd_remove(Monitor *mon, const QDict *qdict); -void net_slirp_redir(const char *redir_str); +int net_slirp_redir(const char *redir_str); void net_cleanup(void); void net_client_check(void); void net_set_boot_mask(int boot_mask); diff --git a/vl.c b/vl.c index 7bfd415..96e4312 100644 --- a/vl.c +++ b/vl.c @@ -4990,11 +4990,13 @@ int main(int argc, char **argv, char **envp) break; #ifndef _WIN32 case QEMU_OPTION_smb: - net_slirp_smb(optarg); + if (net_slirp_smb(optarg) < 0) + exit(1); break; #endif case QEMU_OPTION_redir: - net_slirp_redir(optarg); + if (net_slirp_redir(optarg) < 0) + exit(1); break; #endif case QEMU_OPTION_bt: @@ -5766,7 +5768,8 @@ int main(int argc, char **argv, char **envp) /* init USB devices */ if (usb_enabled) { - foreach_device_config(DEV_USB, usb_parse); + if (foreach_device_config(DEV_USB, usb_parse) < 0) + exit(1); } /* init generic devices */