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
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;
> }
>
>
* 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
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>
* 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
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...)
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
© 2016 - 2024 Red Hat, Inc.