[Qemu-devel] [PATCH 1/3] Make net_client_init() consume slirp_configs even on error

[Qemu-devel] [PATCH 1/3] Make net_client_init() consume slirp_configs even on error

[Markus Armbruster]

net_slirp_init() walks slirp_configs, and stops when it encounters one
that doesn't work.  Instead of consuming slirp_configs members there,
consume them in the sole caller.  This makes sure all are consumed.
Before, the tail starting with the non-working one was left in place,
where it made the next net_slirp_init() fail again.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 net.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/net.c b/net.c
index 3fdf1e6..61429ac 100644
--- a/net.c
+++ b/net.c
@@ -761,6 +761,7 @@ static int net_slirp_init(Monitor *mon, VLANState *vlan, const char *model,
     uint32_t addr;
     int shift;
     char *end;
+    struct slirp_config_str *config;
 
     if (!tftp_export) {
         tftp_export = legacy_tftp_prefix;
@@ -845,9 +846,7 @@ static int net_slirp_init(Monitor *mon, VLANState *vlan, const char *model,
                           tftp_export, bootfile, dhcp, dns, s);
     QTAILQ_INSERT_TAIL(&slirp_stacks, s, entry);
 
-    while (slirp_configs) {
-        struct slirp_config_str *config = slirp_configs;
-
+    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);
@@ -855,8 +854,6 @@ static int net_slirp_init(Monitor *mon, VLANState *vlan, const char *model,
             slirp_guestfwd(s, mon, config->str,
                            config->flags & SLIRP_CFG_LEGACY);
         }
-        slirp_configs = config->next;
-        qemu_free(config);
     }
 #ifndef _WIN32
     if (!smb_export) {
@@ -2583,6 +2580,11 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
         ret = net_slirp_init(mon, vlan, device, name, restricted, vnet, vhost,
                              vhostname, tftp_export, bootfile, vdhcp_start,
                              vnamesrv, smb_export, vsmbsrv);
+        while (slirp_configs) {
+            config = slirp_configs;
+            slirp_configs = config->next;
+            qemu_free(config);
+        }
         qemu_free(vnet);
         qemu_free(vhost);
         qemu_free(vhostname);
-- 
1.6.2.5

[Qemu-devel] [PATCH 2/3] Don't exit() in config_error()

[Markus Armbruster]

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 <armbru@redhat.com>
---
 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=<nbytes>' 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 */
-- 
1.6.2.5

[Qemu-devel] [PATCH 3/3] Drop config_error(), use qemu_error() instead

[Markus Armbruster]

Diagnostic output goes to stderr, except when we're in a monitor
command, when it goes to the monitor instead.

config_error() implements this with a monitor argument: if it's
non-null, report there, else to stderr.  This obliges us to pass the
monitor down various call chains, to make it available to
config_error().

The recently created qemu_error() doesn't need a monitor argument to
route output.  Use it.

There's one user-visible difference: config_error() prepended "qemu: "
to a message bound for stderr.  qemu_error() doesn't, which means the
prefix goes away with this commit.  If such a prefix is desired for
stderr, then I figure it should be slapped on all error messages, not
just the ones that used to go through config_error().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 net.c |  127 ++++++++++++++++++++++++++++-------------------------------------
 1 files changed, 55 insertions(+), 72 deletions(-)

diff --git a/net.c b/net.c
index 9e70f9c..72e94fa 100644
--- a/net.c
+++ b/net.c
@@ -640,20 +640,6 @@ qemu_sendv_packet(VLANClientState *vc, const struct iovec *iov, int iovcnt)
     return qemu_sendv_packet_async(vc, iov, iovcnt, NULL);
 }
 
-static void config_error(Monitor *mon, const char *fmt, ...)
-{
-    va_list ap;
-
-    va_start(ap, fmt);
-    if (mon) {
-        monitor_vprintf(mon, fmt, ap);
-    } else {
-        fprintf(stderr, "qemu: ");
-        vfprintf(stderr, fmt, ap);
-    }
-    va_end(ap);
-}
-
 #if defined(CONFIG_SLIRP)
 
 /* slirp network adapter */
@@ -683,15 +669,15 @@ const char *legacy_bootp_filename;
 static QTAILQ_HEAD(slirp_stacks, SlirpState) slirp_stacks =
     QTAILQ_HEAD_INITIALIZER(slirp_stacks);
 
-static int slirp_hostfwd(SlirpState *s, Monitor *mon, const char *redir_str,
+static int slirp_hostfwd(SlirpState *s, const char *redir_str,
                          int legacy_format);
-static int slirp_guestfwd(SlirpState *s, Monitor *mon, const char *config_str,
+static int slirp_guestfwd(SlirpState *s, const char *config_str,
                           int legacy_format);
 
 #ifndef _WIN32
 static const char *legacy_smb_export;
 
-static int slirp_smb(SlirpState *s, Monitor *mon, const char *exported_dir,
+static int slirp_smb(SlirpState *s, const char *exported_dir,
                      struct in_addr vserver_addr);
 static void slirp_smb_cleanup(SlirpState *s);
 #else
@@ -738,7 +724,7 @@ static void net_slirp_cleanup(VLANClientState *vc)
     qemu_free(s);
 }
 
-static int net_slirp_init(Monitor *mon, VLANState *vlan, const char *model,
+static int net_slirp_init(VLANState *vlan, const char *model,
                           const char *name, int restricted,
                           const char *vnetwork, const char *vhost,
                           const char *vhostname, const char *tftp_export,
@@ -847,11 +833,11 @@ 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) {
-            if (slirp_hostfwd(s, mon, config->str,
+            if (slirp_hostfwd(s, config->str,
                               config->flags & SLIRP_CFG_LEGACY) < 0)
                 return -1;
         } else {
-            if (slirp_guestfwd(s, mon, config->str,
+            if (slirp_guestfwd(s, config->str,
                                config->flags & SLIRP_CFG_LEGACY) < 0)
                 return -1;
         }
@@ -861,7 +847,7 @@ static int net_slirp_init(Monitor *mon, VLANState *vlan, const char *model,
         smb_export = legacy_smb_export;
     }
     if (smb_export) {
-        if (slirp_smb(s, mon, smb_export, smbsrv) < 0)
+        if (slirp_smb(s, smb_export, smbsrv) < 0)
             return -1;
     }
 #endif
@@ -955,7 +941,7 @@ void net_slirp_hostfwd_remove(Monitor *mon, const QDict *qdict)
     monitor_printf(mon, "invalid format\n");
 }
 
-static int slirp_hostfwd(SlirpState *s, Monitor *mon, const char *redir_str,
+static int slirp_hostfwd(SlirpState *s, const char *redir_str,
                          int legacy_format)
 {
     struct in_addr host_addr = { .s_addr = INADDR_ANY };
@@ -1009,14 +995,14 @@ static int slirp_hostfwd(SlirpState *s, Monitor *mon, const char *redir_str,
 
     if (slirp_add_hostfwd(s->slirp, is_udp, host_addr, host_port, guest_addr,
                           guest_port) < 0) {
-        config_error(mon, "could not set up host forwarding rule '%s'\n",
-                     redir_str);
+        qemu_error("could not set up host forwarding rule '%s'\n",
+                   redir_str);
         return -1;
     }
     return 0;
 
  fail_syntax:
-    config_error(mon, "invalid host forwarding rule '%s'\n", redir_str);
+    qemu_error("invalid host forwarding rule '%s'\n", redir_str);
     return -1;
 }
 
@@ -1036,7 +1022,7 @@ void net_slirp_hostfwd_add(Monitor *mon, const QDict *qdict)
         redir_str = arg1;
     }
     if (s) {
-        slirp_hostfwd(s, mon, redir_str, 0);
+        slirp_hostfwd(s, redir_str, 0);
     }
 
 }
@@ -1054,7 +1040,7 @@ int net_slirp_redir(const char *redir_str)
         return 0;
     }
 
-    return slirp_hostfwd(QTAILQ_FIRST(&slirp_stacks), NULL, redir_str, 1);
+    return slirp_hostfwd(QTAILQ_FIRST(&slirp_stacks), redir_str, 1);
 }
 
 #ifndef _WIN32
@@ -1071,7 +1057,7 @@ static void slirp_smb_cleanup(SlirpState *s)
     }
 }
 
-static int slirp_smb(SlirpState* s, Monitor *mon, const char *exported_dir,
+static int slirp_smb(SlirpState* s, const char *exported_dir,
                      struct in_addr vserver_addr)
 {
     static int instance;
@@ -1082,8 +1068,7 @@ static int slirp_smb(SlirpState* s, Monitor *mon, const char *exported_dir,
     snprintf(s->smb_dir, sizeof(s->smb_dir), "/tmp/qemu-smb.%ld-%d",
              (long)getpid(), instance++);
     if (mkdir(s->smb_dir, 0700) < 0) {
-        config_error(mon, "could not create samba server dir '%s'\n",
-                     s->smb_dir);
+        qemu_error("could not create samba server dir '%s'\n", s->smb_dir);
         return -1;
     }
     snprintf(smb_conf, sizeof(smb_conf), "%s/%s", s->smb_dir, "smb.conf");
@@ -1091,8 +1076,8 @@ static int slirp_smb(SlirpState* s, Monitor *mon, const char *exported_dir,
     f = fopen(smb_conf, "w");
     if (!f) {
         slirp_smb_cleanup(s);
-        config_error(mon, "could not create samba server "
-                     "configuration file '%s'\n", smb_conf);
+        qemu_error("could not create samba server configuration file '%s'\n",
+                   smb_conf);
         return -1;
     }
     fprintf(f,
@@ -1123,7 +1108,7 @@ static int 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");
+        qemu_error("conflicting/invalid smbserver address\n");
         return -1;
     }
     return 0;
@@ -1140,7 +1125,7 @@ int net_slirp_smb(const char *exported_dir)
     }
     legacy_smb_export = exported_dir;
     if (!QTAILQ_EMPTY(&slirp_stacks)) {
-        return slirp_smb(QTAILQ_FIRST(&slirp_stacks), NULL, exported_dir,
+        return slirp_smb(QTAILQ_FIRST(&slirp_stacks), exported_dir,
                          vserver_addr);
     }
     return 0;
@@ -1167,7 +1152,7 @@ static void guestfwd_read(void *opaque, const uint8_t *buf, int size)
     slirp_socket_recv(fwd->slirp, fwd->server, fwd->port, buf, size);
 }
 
-static int slirp_guestfwd(SlirpState *s, Monitor *mon, const char *config_str,
+static int slirp_guestfwd(SlirpState *s, const char *config_str,
                           int legacy_format)
 {
     struct in_addr server = { .s_addr = 0 };
@@ -1208,15 +1193,14 @@ static int slirp_guestfwd(SlirpState *s, Monitor *mon, const char *config_str,
     snprintf(buf, sizeof(buf), "guestfwd.tcp:%d", port);
     fwd->hd = qemu_chr_open(buf, p, NULL);
     if (!fwd->hd) {
-        config_error(mon, "could not open guest forwarding device '%s'\n",
-                     buf);
+        qemu_error("could not open guest forwarding device '%s'\n", buf);
         qemu_free(fwd);
         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_error("conflicting/invalid host:port in guest forwarding "
+                   "rule '%s'\n", config_str);
         qemu_free(fwd);
         return -1;
     }
@@ -1229,7 +1213,7 @@ static int slirp_guestfwd(SlirpState *s, Monitor *mon, const char *config_str,
     return 0;
 
  fail_syntax:
-    config_error(mon, "invalid guest forwarding rule '%s'\n", config_str);
+    qemu_error("invalid guest forwarding rule '%s'\n", config_str);
     return -1;
 }
 
@@ -1380,7 +1364,7 @@ static void tap_send(void *opaque)
  */
 #define TAP_DEFAULT_SNDBUF 1024*1024
 
-static int tap_set_sndbuf(TAPState *s, const char *sndbuf_str, Monitor *mon)
+static int tap_set_sndbuf(TAPState *s, const char *sndbuf_str)
 {
     int sndbuf = TAP_DEFAULT_SNDBUF;
 
@@ -1393,17 +1377,16 @@ static int 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));
+        qemu_error("TUNSETSNDBUF ioctl failed: %s\n", strerror(errno));
         return -1;
     }
     return 0;
 }
 #else
-static int tap_set_sndbuf(TAPState *s, const char *sndbuf_str, Monitor *mon)
+static int tap_set_sndbuf(TAPState *s, const char *sndbuf_str)
 {
     if (sndbuf_str) {
-        config_error(mon, "No '-net tap,sndbuf=<nbytes>' support available\n");
+        qemu_error("No '-net tap,sndbuf=<nbytes>' support available\n");
         return -1;
     }
     return 0;
@@ -2287,7 +2270,7 @@ static void net_dump_cleanup(VLANClientState *vc)
     qemu_free(s);
 }
 
-static int net_dump_init(Monitor *mon, VLANState *vlan, const char *device,
+static int net_dump_init(VLANState *vlan, const char *device,
                          const char *name, const char *filename, int len)
 {
     struct pcap_file_hdr hdr;
@@ -2297,7 +2280,7 @@ static int net_dump_init(Monitor *mon, VLANState *vlan, const char *device,
 
     s->fd = open(filename, O_CREAT | O_WRONLY | O_BINARY, 0644);
     if (s->fd < 0) {
-        config_error(mon, "-net dump: can't open %s\n", filename);
+        qemu_error("-net dump: can't open %s\n", filename);
         return -1;
     }
 
@@ -2312,7 +2295,7 @@ static int net_dump_init(Monitor *mon, VLANState *vlan, const char *device,
     hdr.linktype = 1;
 
     if (write(s->fd, &hdr, sizeof(hdr)) < sizeof(hdr)) {
-        config_error(mon, "-net dump write error: %s\n", strerror(errno));
+        qemu_error("-net dump write error: %s\n", strerror(errno));
         close(s->fd);
         qemu_free(s);
         return -1;
@@ -2398,7 +2381,7 @@ static int net_handle_fd_param(Monitor *mon, const char *param)
 
         fd = monitor_get_fd(mon, param);
         if (fd == -1) {
-            config_error(mon, "No file descriptor named %s found", param);
+            qemu_error("No file descriptor named %s found", param);
             return -1;
         }
 
@@ -2433,12 +2416,12 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
         int idx = nic_get_free_idx();
 
         if (check_params(buf, sizeof(buf), nic_params, p) < 0) {
-            config_error(mon, "invalid parameter '%s' in '%s'\n", buf, p);
+            qemu_error("invalid parameter '%s' in '%s'\n", buf, p);
             ret = -1;
             goto out;
         }
         if (idx == -1 || nb_nics >= MAX_NICS) {
-            config_error(mon, "Too Many NICs\n");
+            qemu_error("Too Many NICs\n");
             ret = -1;
             goto out;
         }
@@ -2453,7 +2436,7 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
 
         if (get_param_value(buf, sizeof(buf), "macaddr", p)) {
             if (parse_macaddr(macaddr, buf) < 0) {
-                config_error(mon, "invalid syntax for ethernet address\n");
+                qemu_error("invalid syntax for ethernet address\n");
                 ret = -1;
                 goto out;
             }
@@ -2472,12 +2455,12 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
             char *endptr;
             long vectors = strtol(buf, &endptr, 0);
             if (*endptr) {
-                config_error(mon, "invalid syntax for # of vectors\n");
+                qemu_error("invalid syntax for # of vectors\n");
                 ret = -1;
                 goto out;
             }
             if (vectors < 0 || vectors > 0x7ffffff) {
-                config_error(mon, "invalid # of vectors\n");
+                qemu_error("invalid # of vectors\n");
                 ret = -1;
                 goto out;
             }
@@ -2493,7 +2476,7 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
     } else
     if (!strcmp(device, "none")) {
         if (*p != '\0') {
-            config_error(mon, "'none' takes no parameters\n");
+            qemu_error("'none' takes no parameters\n");
             ret = -1;
             goto out;
         }
@@ -2522,7 +2505,7 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
         const char *q;
 
         if (check_params(buf, sizeof(buf), slirp_params, p) < 0) {
-            config_error(mon, "invalid parameter '%s' in '%s'\n", buf, p);
+            qemu_error("invalid parameter '%s' in '%s'\n", buf, p);
             ret = -1;
             goto out;
         }
@@ -2589,7 +2572,7 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
         }
         qemu_free(config);
         vlan->nb_host_devs++;
-        ret = net_slirp_init(mon, vlan, device, name, restricted, vnet, vhost,
+        ret = net_slirp_init(vlan, device, name, restricted, vnet, vhost,
                              vhostname, tftp_export, bootfile, vdhcp_start,
                              vnamesrv, smb_export, vsmbsrv);
         while (slirp_configs) {
@@ -2617,7 +2600,7 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
             slirp_configs = config;
             ret = 0;
         } else {
-            ret = slirp_guestfwd(QTAILQ_FIRST(&slirp_stacks), mon, p, 1);
+            ret = slirp_guestfwd(QTAILQ_FIRST(&slirp_stacks), p, 1);
         }
     } else
 #endif
@@ -2629,12 +2612,12 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
         char ifname[64];
 
         if (check_params(buf, sizeof(buf), tap_params, p) < 0) {
-            config_error(mon, "invalid parameter '%s' in '%s'\n", buf, p);
+            qemu_error("invalid parameter '%s' in '%s'\n", buf, p);
             ret = -1;
             goto out;
         }
         if (get_param_value(ifname, sizeof(ifname), "ifname", p) <= 0) {
-            config_error(mon, "tap: no interface name\n");
+            qemu_error("tap: no interface name\n");
             ret = -1;
             goto out;
         }
@@ -2655,7 +2638,7 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
             };
             ret = -1;
             if (check_params(chkbuf, sizeof(chkbuf), fd_params, p) < 0) {
-                config_error(mon, "invalid parameter '%s' in '%s'\n", chkbuf, p);
+                qemu_error("invalid parameter '%s' in '%s'\n", chkbuf, p);
                 goto out;
             }
             fd = net_handle_fd_param(mon, buf);
@@ -2672,7 +2655,7 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
                 "vlan", "name", "ifname", "script", "downscript", "sndbuf", NULL
             };
             if (check_params(chkbuf, sizeof(chkbuf), tap_params, p) < 0) {
-                config_error(mon, "invalid parameter '%s' in '%s'\n", chkbuf, p);
+                qemu_error("invalid parameter '%s' in '%s'\n", chkbuf, p);
                 ret = -1;
                 goto out;
             }
@@ -2692,7 +2675,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;
             }
-            ret = tap_set_sndbuf(s, sndbuf_str, mon);
+            ret = tap_set_sndbuf(s, sndbuf_str);
         } else {
             ret = -1;
         }
@@ -2707,7 +2690,7 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
             int fd;
             ret = -1;
             if (check_params(chkbuf, sizeof(chkbuf), fd_params, p) < 0) {
-                config_error(mon, "invalid parameter '%s' in '%s'\n", chkbuf, p);
+                qemu_error("invalid parameter '%s' in '%s'\n", chkbuf, p);
                 goto out;
             }
             fd = net_handle_fd_param(mon, buf);
@@ -2724,7 +2707,7 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
                 "vlan", "name", "listen", NULL
             };
             if (check_params(chkbuf, sizeof(chkbuf), listen_params, p) < 0) {
-                config_error(mon, "invalid parameter '%s' in '%s'\n", chkbuf, p);
+                qemu_error("invalid parameter '%s' in '%s'\n", chkbuf, p);
                 ret = -1;
                 goto out;
             }
@@ -2734,7 +2717,7 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
                 "vlan", "name", "connect", NULL
             };
             if (check_params(chkbuf, sizeof(chkbuf), connect_params, p) < 0) {
-                config_error(mon, "invalid parameter '%s' in '%s'\n", chkbuf, p);
+                qemu_error("invalid parameter '%s' in '%s'\n", chkbuf, p);
                 ret = -1;
                 goto out;
             }
@@ -2744,13 +2727,13 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
                 "vlan", "name", "mcast", NULL
             };
             if (check_params(chkbuf, sizeof(chkbuf), mcast_params, p) < 0) {
-                config_error(mon, "invalid parameter '%s' in '%s'\n", chkbuf, p);
+                qemu_error("invalid parameter '%s' in '%s'\n", chkbuf, p);
                 ret = -1;
                 goto out;
             }
             ret = net_socket_mcast_init(vlan, device, name, buf);
         } else {
-            config_error(mon, "Unknown socket options: %s\n", p);
+            qemu_error("Unknown socket options: %s\n", p);
             ret = -1;
             goto out;
         }
@@ -2765,7 +2748,7 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
 	int vde_port, vde_mode;
 
         if (check_params(buf, sizeof(buf), vde_params, p) < 0) {
-            config_error(mon, "invalid parameter '%s' in '%s'\n", buf, p);
+            qemu_error("invalid parameter '%s' in '%s'\n", buf, p);
             ret = -1;
             goto out;
         }
@@ -2798,14 +2781,14 @@ int net_client_init(Monitor *mon, const char *device, const char *p)
         if (!get_param_value(buf, sizeof(buf), "file", p)) {
             snprintf(buf, sizeof(buf), "qemu-vlan%d.pcap", vlan_id);
         }
-        ret = net_dump_init(mon, vlan, device, name, buf, len);
+        ret = net_dump_init(vlan, device, name, buf, len);
     } else {
-        config_error(mon, "Unknown network device: %s\n", device);
+        qemu_error("Unknown network device: %s\n", device);
         ret = -1;
         goto out;
     }
     if (ret < 0) {
-        config_error(mon, "Could not initialize device '%s'\n", device);
+        qemu_error("Could not initialize device '%s'\n", device);
     }
 out:
     qemu_free(name);
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 2/3] Don't exit() in config_error()

[Mark McLoughlin]

On Mon, 2009-09-28 at 21:11 +0200, Markus Armbruster wrote:
> 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 <armbru@redhat.com>
> ---
>  net.c |   83 ++++++++++++++++++++++++++++++++++++----------------------------
>  net.h |    4 +-
>  vl.c  |    9 ++++--
>  3 files changed, 55 insertions(+), 41 deletions(-)
> 
...
> diff --git a/vl.c b/vl.c
> index 7bfd415..96e4312 100644
> --- a/vl.c
> +++ b/vl.c
...
> @@ -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 */

This hunk appears to be unrelated

Cheers,
Mark.

Re: [Qemu-devel] [PATCH 2/3] Don't exit() in config_error()

[Markus Armbruster]

Mark McLoughlin <markmc@redhat.com> writes:

> On Mon, 2009-09-28 at 21:11 +0200, Markus Armbruster wrote:
>> 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 <armbru@redhat.com>
>> ---
>>  net.c |   83 ++++++++++++++++++++++++++++++++++++----------------------------
>>  net.h |    4 +-
>>  vl.c  |    9 ++++--
>>  3 files changed, 55 insertions(+), 41 deletions(-)
>> 
> ...
>> diff --git a/vl.c b/vl.c
>> index 7bfd415..96e4312 100644
>> --- a/vl.c
>> +++ b/vl.c
> ...
>> @@ -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 */
>
> This hunk appears to be unrelated

net_client_init() returns failure through usb_device_add(), usb_parse(),
foreach_device_config() to main().  Without this hunk, we ignore the
error and continue.

Before patch: net_client_init() or one of its callees terminates the
program on failure, by calling config_error().

Makes sense?

Re: [Qemu-devel] [PATCH 2/3] Don't exit() in config_error()

[Mark McLoughlin]

On Wed, 2009-09-30 at 15:24 +0200, Markus Armbruster wrote:
> Mark McLoughlin <markmc@redhat.com> writes:
> 
> > On Mon, 2009-09-28 at 21:11 +0200, Markus Armbruster wrote:
> >> 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 <armbru@redhat.com>
> >> ---
> >>  net.c |   83 ++++++++++++++++++++++++++++++++++++----------------------------
> >>  net.h |    4 +-
> >>  vl.c  |    9 ++++--
> >>  3 files changed, 55 insertions(+), 41 deletions(-)
> >> 
> > ...
> >> diff --git a/vl.c b/vl.c
> >> index 7bfd415..96e4312 100644
> >> --- a/vl.c
> >> +++ b/vl.c
> > ...
> >> @@ -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 */
> >
> > This hunk appears to be unrelated
> 
> net_client_init() returns failure through usb_device_add(), usb_parse(),
> foreach_device_config() to main().  Without this hunk, we ignore the
> error and continue.
> 
> Before patch: net_client_init() or one of its callees terminates the
> program on failure, by calling config_error().
> 
> Makes sense?

Yes, it does - I shouldn't have doubted you :-)

Re-instated it again in my tree

Thanks,
Mark.