[Qemu-devel] [PATCH] slirp: Add explanation for hostfwd parsing failure

[Qemu-devel] [PATCH] slirp: Add explanation for hostfwd parsing failure

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

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 <dgilbert@redhat.com>
---
 net/slirp.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/net/slirp.c b/net/slirp.c
index 01ed21c006..d87664d42e 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -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;
 }
 
-- 
2.13.5

Re: [Qemu-devel] [PATCH] slirp: Add explanation for hostfwd parsing failure

[Philippe Mathieu-Daudé]

Hi David,

On 09/08/2017 12:53 PM, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> 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 <dgilbert@redhat.com>
> ---
>   net/slirp.c | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/net/slirp.c b/net/slirp.c
> index 01ed21c006..d87664d42e 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -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é <f4bug@amsat.org>

>   
>       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;
>   }
>   
> 

Re: [Qemu-devel] [PATCH] slirp: Add explanation for hostfwd parsing failure

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (f4bug@amsat.org) wrote:
> Hi David,
> 
> On 09/08/2017 12:53 PM, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > 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 <dgilbert@redhat.com>
> > ---
> >   net/slirp.c | 13 ++++++++++++-
> >   1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/slirp.c b/net/slirp.c
> > index 01ed21c006..d87664d42e 100644
> > --- a/net/slirp.c
> > +++ b/net/slirp.c
> > @@ -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.

> Anyway:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Thanks.

Dave

> 
> >       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;
> >   }
> > 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] slirp: Add explanation for hostfwd parsing failure

[Philippe Mathieu-Daudé]

On 09/08/2017 01:22 PM, Dr. David Alan Gilbert wrote:
> * Philippe Mathieu-Daudé (f4bug@amsat.org) wrote:
>> Hi David,
>>
>> On 09/08/2017 12:53 PM, Dr. David Alan Gilbert (git) wrote:
>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>>
>>> 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 <dgilbert@redhat.com>
>>> ---
>>>    net/slirp.c | 13 ++++++++++++-
>>>    1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/net/slirp.c b/net/slirp.c
>>> index 01ed21c006..d87664d42e 100644
>>> --- a/net/slirp.c
>>> +++ b/net/slirp.c
>>> @@ -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.

I see, what about:

const char *fail_reason = "Unknown reason";

> 
>> Anyway:
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [Qemu-devel] [PATCH] slirp: Add explanation for hostfwd parsing failure

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (f4bug@amsat.org) wrote:
> On 09/08/2017 01:22 PM, Dr. David Alan Gilbert wrote:
> > * Philippe Mathieu-Daudé (f4bug@amsat.org) wrote:
> > > Hi David,
> > > 
> > > On 09/08/2017 12:53 PM, Dr. David Alan Gilbert (git) wrote:
> > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > > 
> > > > 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 <dgilbert@redhat.com>
> > > > ---
> > > >    net/slirp.c | 13 ++++++++++++-
> > > >    1 file changed, 12 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/net/slirp.c b/net/slirp.c
> > > > index 01ed21c006..d87664d42e 100644
> > > > --- a/net/slirp.c
> > > > +++ b/net/slirp.c
> > > > @@ -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.
> 
> 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

> > 
> > > Anyway:
> > > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] slirp: Add explanation for hostfwd parsing failure

[Philippe Mathieu-Daudé]

On 09/08/2017 03:36 PM, Dr. David Alan Gilbert wrote:
> * Philippe Mathieu-Daudé (f4bug@amsat.org) wrote:
>> On 09/08/2017 01:22 PM, Dr. David Alan Gilbert wrote:
>>> * Philippe Mathieu-Daudé (f4bug@amsat.org) wrote:
>>>> Hi David,
>>>>
>>>> On 09/08/2017 12:53 PM, Dr. David Alan Gilbert (git) wrote:
>>>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>>>>
>>>>> 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 <dgilbert@redhat.com>
>>>>> ---
>>>>>     net/slirp.c | 13 ++++++++++++-
>>>>>     1 file changed, 12 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/net/slirp.c b/net/slirp.c
>>>>> index 01ed21c006..d87664d42e 100644
>>>>> --- a/net/slirp.c
>>>>> +++ b/net/slirp.c
>>>>> @@ -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.
>>
>> 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...)

Re: [Qemu-devel] [PATCH] slirp: Add explanation for hostfwd parsing failure

[Samuel Thibault]

Philippe Mathieu-Daudé, on ven. 08 sept. 2017 13:19:56 -0300, wrote:
> Hi David,
> 
> On 09/08/2017 12:53 PM, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > 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 <dgilbert@redhat.com>
> > ---
> >   net/slirp.c | 13 ++++++++++++-
> >   1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/slirp.c b/net/slirp.c
> > index 01ed21c006..d87664d42e 100644
> > --- a/net/slirp.c
> > +++ b/net/slirp.c
> > @@ -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é <f4bug@amsat.org>

Applied to my tree, thanks!

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

Samuel