Skip to contentSkip to conversation

slirp: Add explanation for hostfwd parsing failure

PATCH Applied Dr. David Alan Gilbert (git) 3 participants 7 messages Sep 8, 2017 – Sep 9, 2017 Networking

lore
Share & tools
lore

Dr. David Alan Gilbert (git)

author
#0 lore raw

PATCH 1/1 slirp: Add explanation for hostfwd parsing failure

From: "Dr. David Alan Gilbert" <[email protected]>

e.g.
./x86_64-softmmu/qemu-system-x86_64 -nographic -netdev 'user,id=vnet,hostfwd=:555.0.0.0:0-:22'
qemu-system-x86_64: -netdev user,id=vnet,hostfwd=:555.0.0.0:0-:22: Invalid host forwarding rule ':555.0.0.0:0-:22' (Bad host address)

Signed-off-by Dr. David Alan Gilbert <[email protected]>
---
 net/slirp.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
net/slirp.c +12 -1
index 01ed21c006..d87664d42e 100644
@@ -496,9 +496,11 @@ static int slirp_hostfwd(SlirpState *s, const char *redir_str,
     char buf[256];
     int is_udp;
     char *end;
+    const char *fail_reason = "";
 
     p = redir_str;
     if (!p || get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
+        fail_reason = "No : separators";
         goto fail_syntax;
     }
     if (!strcmp(buf, "tcp") || buf[0] == '\0') {

@@ -506,35 +508,43 @@ static int slirp_hostfwd(SlirpState *s, const char *redir_str,
     } else if (!strcmp(buf, "udp")) {
         is_udp = 1;
     } else {
+        fail_reason = "Bad protocol name";
         goto fail_syntax;
     }
 
     if (!legacy_format) {
         if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
+            fail_reason = "Missing : separator";
             goto fail_syntax;
         }
         if (buf[0] != '\0' && !inet_aton(buf, &host_addr)) {
+            fail_reason = "Bad host address";
             goto fail_syntax;
         }
     }
 
     if (get_str_sep(buf, sizeof(buf), &p, legacy_format ? ':' : '-') < 0) {
+        fail_reason = "Bad host port separator";
         goto fail_syntax;
     }
     host_port = strtol(buf, &end, 0);
     if (*end != '\0' || host_port < 0 || host_port > 65535) {
+        fail_reason = "Bad host port";
         goto fail_syntax;
     }
 
     if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
+        fail_reason = "Missing guest address";
         goto fail_syntax;
     }
     if (buf[0] != '\0' && !inet_aton(buf, &guest_addr)) {
+        fail_reason = "Bad guest address";
         goto fail_syntax;
     }
 
     guest_port = strtol(p, &end, 0);
     if (*end != '\0' || guest_port < 1 || guest_port > 65535) {
+        fail_reason = "Bad guest port";
         goto fail_syntax;
     }
 

@@ -547,7 +557,8 @@ static int slirp_hostfwd(SlirpState *s, const char *redir_str,
     return 0;
 
  fail_syntax:
-    error_setg(errp, "Invalid host forwarding rule '%s'", redir_str);
+    error_setg(errp, "Invalid host forwarding rule '%s' (%s)", redir_str,
+               fail_reason);
     return -1;
 }
Signature · 1 line
2.13.5
Reviewed-by on v1 1/1
#1 lore raw

Hi David,

Quoted 16 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>

e.g.
./x86_64-softmmu/qemu-system-x86_64 -nographic -netdev 'user,id=vnet,hostfwd=:555.0.0.0:0-:22'
qemu-system-x86_64: -netdev user,id=vnet,hostfwd=:555.0.0.0:0-:22: Invalid host forwarding rule ':555.0.0.0:0-:22' (Bad host address)

Signed-off-by Dr. David Alan Gilbert <[email protected]>
---
  net/slirp.c | 13 ++++++++++++-
  1 file changed, 12 insertions(+), 1 deletion(-)
net/slirp.c +0 -0
index 01ed21c006..d87664d42e 100644

net/slirp.c static int slirp_hostfwd(SlirpState *s, const char *redir_str, · line 499

Quoted 4 lines
net/slirp.c +1 -0
@@ -496,9 +496,11 @@ static int slirp_hostfwd(SlirpState *s, const char *redir_str,
      char buf[256];
      int is_udp;
      char *end;
+    const char *fail_reason = "";

Isn't it better not initialize this? So if one add a new failed syntax case the build with abort with -Werror=uninitialized

Anyway:

Reviewed-by Philippe Mathieu-Daudé <[email protected]>
Quoted 62 lines
net/slirp.c +11 -1
      p = redir_str;
      if (!p || get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
+        fail_reason = "No : separators";
          goto fail_syntax;
      }
      if (!strcmp(buf, "tcp") || buf[0] == '\0') {

@@ -506,35 +508,43 @@ static int slirp_hostfwd(SlirpState *s, const char *redir_str,
      } else if (!strcmp(buf, "udp")) {
          is_udp = 1;
      } else {
+        fail_reason = "Bad protocol name";
          goto fail_syntax;
      }
 
      if (!legacy_format) {
          if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
+            fail_reason = "Missing : separator";
              goto fail_syntax;
          }
          if (buf[0] != '\0' && !inet_aton(buf, &host_addr)) {
+            fail_reason = "Bad host address";
              goto fail_syntax;
          }
      }
 
      if (get_str_sep(buf, sizeof(buf), &p, legacy_format ? ':' : '-') < 0) {
+        fail_reason = "Bad host port separator";
          goto fail_syntax;
      }
      host_port = strtol(buf, &end, 0);
      if (*end != '\0' || host_port < 0 || host_port > 65535) {
+        fail_reason = "Bad host port";
          goto fail_syntax;
      }
 
      if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
+        fail_reason = "Missing guest address";
          goto fail_syntax;
      }
      if (buf[0] != '\0' && !inet_aton(buf, &guest_addr)) {
+        fail_reason = "Bad guest address";
          goto fail_syntax;
      }
 
      guest_port = strtol(p, &end, 0);
      if (*end != '\0' || guest_port < 1 || guest_port > 65535) {
+        fail_reason = "Bad guest port";
          goto fail_syntax;
      }
 

@@ -547,7 +557,8 @@ static int slirp_hostfwd(SlirpState *s, const char *redir_str,
      return 0;
 
   fail_syntax:
-    error_setg(errp, "Invalid host forwarding rule '%s'", redir_str);
+    error_setg(errp, "Invalid host forwarding rule '%s' (%s)", redir_str,
+               fail_reason);
      return -1;
  }
author
#2 lore raw
Quoted 14 lines · * Philippe Mathieu-Daudé ([email protected]) wrote:

Hi David,

Quoted 11 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>

e.g.
./x86_64-softmmu/qemu-system-x86_64 -nographic -netdev 'user,id=vnet,hostfwd=:555.0.0.0:0-:22'
qemu-system-x86_64: -netdev user,id=vnet,hostfwd=:555.0.0.0:0-:22: Invalid host forwarding rule ':555.0.0.0:0-:22' (Bad host address)

Signed-off-by Dr. David Alan Gilbert <[email protected]>
---
  net/slirp.c | 13 ++++++++++++-
  1 file changed, 12 insertions(+), 1 deletion(-)
Quoted 12 lines

net/slirp.c static int slirp_hostfwd(SlirpState *s, const char *redir_str, · line 499

Quoted 9 lines
net/slirp.c +1 -0
index 01ed21c006..d87664d42e 100644
@@ -496,9 +496,11 @@ static int slirp_hostfwd(SlirpState *s, const char *redir_str,
      char buf[256];
      int is_udp;
      char *end;
+    const char *fail_reason = "";

Isn't it better not initialize this? So if one add a new failed syntax case the build with abort with -Werror=uninitialized

I never quite trust compilers to spot it or not-moan even though every route to failure will have set it.

Quoted 2 lines

Anyway:

Reviewed-by Philippe Mathieu-Daudé <[email protected]>

Thanks.

Dave

Quoted 47 lines
Quoted 47 lines
net/slirp.c +9 -0
      p = redir_str;
      if (!p || get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
+        fail_reason = "No : separators";
          goto fail_syntax;
      }
      if (!strcmp(buf, "tcp") || buf[0] == '\0') {

@@ -506,35 +508,43 @@ static int slirp_hostfwd(SlirpState *s, const char *redir_str,
      } else if (!strcmp(buf, "udp")) {
          is_udp = 1;
      } else {
+        fail_reason = "Bad protocol name";
          goto fail_syntax;
      }
      if (!legacy_format) {
          if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
+            fail_reason = "Missing : separator";
              goto fail_syntax;
          }
          if (buf[0] != '\0' && !inet_aton(buf, &host_addr)) {
+            fail_reason = "Bad host address";
              goto fail_syntax;
          }
      }
      if (get_str_sep(buf, sizeof(buf), &p, legacy_format ? ':' : '-') < 0) {
+        fail_reason = "Bad host port separator";
          goto fail_syntax;
      }
      host_port = strtol(buf, &end, 0);
      if (*end != '\0' || host_port < 0 || host_port > 65535) {
+        fail_reason = "Bad host port";
          goto fail_syntax;
      }
      if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
+        fail_reason = "Missing guest address";
          goto fail_syntax;
      }
      if (buf[0] != '\0' && !inet_aton(buf, &guest_addr)) {
+        fail_reason = "Bad guest address";
          goto fail_syntax;
      }
      guest_port = strtol(p, &end, 0);
      if (*end != '\0' || guest_port < 1 || guest_port > 65535) {
+        fail_reason = "Bad guest port";
          goto fail_syntax;
      }

@@ -547,7 +557,8 @@ static int slirp_hostfwd(SlirpState *s, const char *redir_str,
Quoted 8 lines

net/slirp.c static int slirp_hostfwd(SlirpState *s, const char *redir_str, · line 564

Quoted 8 lines
net/slirp.c +2 -1
      return 0;
   fail_syntax:
-    error_setg(errp, "Invalid host forwarding rule '%s'", redir_str);
+    error_setg(errp, "Invalid host forwarding rule '%s' (%s)", redir_str,
+               fail_reason);
      return -1;
  }

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

#3 lore raw
Quoted 28 lines · Dr. David Alan Gilbert
Quoted 14 lines · * Philippe Mathieu-Daudé ([email protected]) wrote:

Hi David,

Quoted 11 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>

e.g.
./x86_64-softmmu/qemu-system-x86_64 -nographic -netdev 'user,id=vnet,hostfwd=:555.0.0.0:0-:22'
qemu-system-x86_64: -netdev user,id=vnet,hostfwd=:555.0.0.0:0-:22: Invalid host forwarding rule ':555.0.0.0:0-:22' (Bad host address)

Signed-off-by Dr. David Alan Gilbert <[email protected]>
---
   net/slirp.c | 13 ++++++++++++-
   1 file changed, 12 insertions(+), 1 deletion(-)
Quoted 12 lines

net/slirp.c static int slirp_hostfwd(SlirpState *s, const char *redir_str, · line 499

Quoted 9 lines
net/slirp.c +1 -0
index 01ed21c006..d87664d42e 100644
@@ -496,9 +496,11 @@ static int slirp_hostfwd(SlirpState *s, const char *redir_str,
       char buf[256];
       int is_udp;
       char *end;
+    const char *fail_reason = "";

Isn't it better not initialize this? So if one add a new failed syntax case the build with abort with -Werror=uninitialized

Quoted 2 lines

I never quite trust compilers to spot it or not-moan even though every route to failure will have set it.

I see, what about:

const char *fail_reason = "Unknown reason";

Quoted 3 lines
Quoted 3 lines

Anyway:

Reviewed-by Philippe Mathieu-Daudé <[email protected]>
author
#4 lore raw
Quoted 29 lines · * Philippe Mathieu-Daudé ([email protected]) wrote:
Quoted 28 lines · Dr. David Alan Gilbert
Quoted 26 lines · * Philippe Mathieu-Daudé ([email protected]) wrote:

Hi David,

Quoted 16 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>

e.g.
./x86_64-softmmu/qemu-system-x86_64 -nographic -netdev 'user,id=vnet,hostfwd=:555.0.0.0:0-:22'
qemu-system-x86_64: -netdev user,id=vnet,hostfwd=:555.0.0.0:0-:22: Invalid host forwarding rule ':555.0.0.0:0-:22' (Bad host address)

Signed-off-by Dr. David Alan Gilbert <[email protected]>
---
   net/slirp.c | 13 ++++++++++++-
   1 file changed, 12 insertions(+), 1 deletion(-)
net/slirp.c +0 -0
index 01ed21c006..d87664d42e 100644

net/slirp.c static int slirp_hostfwd(SlirpState *s, const char *redir_str, · line 499

Quoted 4 lines
net/slirp.c +1 -0
@@ -496,9 +496,11 @@ static int slirp_hostfwd(SlirpState *s, const char *redir_str,
       char buf[256];
       int is_udp;
       char *end;
+    const char *fail_reason = "";

Isn't it better not initialize this? So if one add a new failed syntax case the build with abort with -Werror=uninitialized

Quoted 6 lines
Quoted 2 lines

I never quite trust compilers to spot it or not-moan even though every route to failure will have set it.

I see, what about:

const char *fail_reason = "Unknown reason";

I could, but you're right that I shouldn't miss any, and 'Unknown Reason' doesn't tell the user any more than no message.

Dave

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines

Anyway:

Reviewed-by Philippe Mathieu-Daudé <[email protected]>

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

#5 lore raw
Quoted 33 lines · Dr. David Alan Gilbert
Quoted 32 lines · * Philippe Mathieu-Daudé ([email protected]) wrote:
Quoted 30 lines · Dr. David Alan Gilbert
Quoted 14 lines · * Philippe Mathieu-Daudé ([email protected]) wrote:

Hi David,

Quoted 11 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>

e.g.
./x86_64-softmmu/qemu-system-x86_64 -nographic -netdev 'user,id=vnet,hostfwd=:555.0.0.0:0-:22'
qemu-system-x86_64: -netdev user,id=vnet,hostfwd=:555.0.0.0:0-:22: Invalid host forwarding rule ':555.0.0.0:0-:22' (Bad host address)

Signed-off-by Dr. David Alan Gilbert <[email protected]>
---
    net/slirp.c | 13 ++++++++++++-
    1 file changed, 12 insertions(+), 1 deletion(-)
Quoted 12 lines

net/slirp.c static int slirp_hostfwd(SlirpState *s, const char *redir_str, · line 499

Quoted 9 lines
net/slirp.c +1 -0
index 01ed21c006..d87664d42e 100644
@@ -496,9 +496,11 @@ static int slirp_hostfwd(SlirpState *s, const char *redir_str,
        char buf[256];
        int is_udp;
        char *end;
+    const char *fail_reason = "";

Isn't it better not initialize this? So if one add a new failed syntax case the build with abort with -Werror=uninitialized

I never quite trust compilers to spot it or not-moan even though every route to failure will have set it.

Quoted 6 lines
Quoted 3 lines

I see, what about:

const char *fail_reason = "Unknown reason";

I could, but you're right that I shouldn't miss any, and 'Unknown Reason' doesn't tell the user any more than no message.

It's not you I'm worried about ;) but if someone adds a new forward layer tomorrow (sctp, udplite, rudp...)

Applied
#6 lore raw
In reply to Philippe Mathieu-Daudé
Quoted 27 lines · Philippe Mathieu-Daudé, on ven. 08 sept. 2017 13:19:56 -0300, wrote:

Hi David,

Quoted 16 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>

e.g.
./x86_64-softmmu/qemu-system-x86_64 -nographic -netdev 'user,id=vnet,hostfwd=:555.0.0.0:0-:22'
qemu-system-x86_64: -netdev user,id=vnet,hostfwd=:555.0.0.0:0-:22: Invalid host forwarding rule ':555.0.0.0:0-:22' (Bad host address)

Signed-off-by Dr. David Alan Gilbert <[email protected]>
---
  net/slirp.c | 13 ++++++++++++-
  1 file changed, 12 insertions(+), 1 deletion(-)
net/slirp.c +0 -0
index 01ed21c006..d87664d42e 100644

net/slirp.c static int slirp_hostfwd(SlirpState *s, const char *redir_str, · line 499

Quoted 4 lines
net/slirp.c +1 -0
@@ -496,9 +496,11 @@ static int slirp_hostfwd(SlirpState *s, const char *redir_str,
      char buf[256];
      int is_udp;
      char *end;
+    const char *fail_reason = "";

Isn't it better not initialize this? So if one add a new failed syntax case the build with abort with -Werror=uninitialized

Quoted 2 lines

Anyway:

Reviewed-by Philippe Mathieu-Daudé <[email protected]>

Applied to my tree, thanks!

(I have put Unknown reason there, otherwise the message would have oddly-looking "()" )

Samuel