[PATCH 1/2] kvm tools: Fix formatting of error message in TAP handling

[PATCH 1/2] kvm tools: Fix formatting of error message in TAP handling

[Michael Ellerman]

This error message is missing a space, and has a redundant ":" at the end,
currently it produces:

  You have requested a TAP device, but creation of one hasfailed because:: No such file or directory

Add a space to "hasfailed" and remove the extra ":".

Don't split the line to improve grepability.

Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---
 tools/kvm/virtio/net.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/kvm/virtio/net.c b/tools/kvm/virtio/net.c
index 10420ae..8f3735b 100644
--- a/tools/kvm/virtio/net.c
+++ b/tools/kvm/virtio/net.c
@@ -526,8 +526,7 @@ void virtio_net__init(const struct virtio_net_params *params)
 	ndev->mode = params->mode;
 	if (ndev->mode == NET_MODE_TAP) {
 		if (!virtio_net__tap_init(params, ndev))
-			die_perror("You have requested a TAP device, but creation of one has"
-					"failed because:");
+			die_perror("You have requested a TAP device, but creation of one has failed because");
 		ndev->ops = &tap_ops;
 	} else {
 		ndev->info.host_ip		= ntohl(inet_addr(params->host_ip));
-- 
1.7.9.5

[PATCH 2/2] kvm tools: Check for unknown parameters in network option handling

[Michael Ellerman]

Currently the parsing of network command line parameters doesn't check
for unknown parameters.

For example "-n mod=none" will cause kvmtool to setup TAP networking.

Save users from their typos by checking for unknown parameters and bailing.

Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---
 tools/kvm/builtin-run.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/kvm/builtin-run.c b/tools/kvm/builtin-run.c
index a36bd00..9e5c1d4 100644
--- a/tools/kvm/builtin-run.c
+++ b/tools/kvm/builtin-run.c
@@ -257,7 +257,8 @@ static int set_net_param(struct virtio_net_params *p, const char *param,
 		p->vhost = atoi(val);
 	} else if (strcmp(param, "fd") == 0) {
 		p->fd = atoi(val);
-	}
+	} else
+		die("Unknown network parameter %s", param);
 
 	return 0;
 }
-- 
1.7.9.5

Re: [PATCH 2/2] kvm tools: Check for unknown parameters in network option handling

[Asias He]

On Fri, Aug 10, 2012 at 1:38 PM, Michael Ellerman
<michael@ellerman.id.au> wrote:
> Currently the parsing of network command line parameters doesn't check
> for unknown parameters.
>
> For example "-n mod=none" will cause kvmtool to setup TAP networking.
>
> Save users from their typos by checking for unknown parameters and bailing.
>
> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> ---
>  tools/kvm/builtin-run.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/kvm/builtin-run.c b/tools/kvm/builtin-run.c
> index a36bd00..9e5c1d4 100644
> --- a/tools/kvm/builtin-run.c
> +++ b/tools/kvm/builtin-run.c
> @@ -257,7 +257,8 @@ static int set_net_param(struct virtio_net_params *p, const char *param,
>                 p->vhost = atoi(val);
>         } else if (strcmp(param, "fd") == 0) {
>                 p->fd = atoi(val);
> -       }
> +       } else
> +               die("Unknown network parameter %s", param);

we need braces here:

+ } else {
+           die("Unknown network parameter %s", param);
+ }

Otherwise, ACK.

-- 
Asias He

Re: [PATCH 2/2] kvm tools: Check for unknown parameters in network option handling

[Michael Ellerman]

On Sat, 2012-08-11 at 08:21 +0800, Asias He wrote:
> On Fri, Aug 10, 2012 at 1:38 PM, Michael Ellerman
> <michael@ellerman.id.au> wrote:
> > Currently the parsing of network command line parameters doesn't check
> > for unknown parameters.
> >
> > For example "-n mod=none" will cause kvmtool to setup TAP networking.
> >
> > Save users from their typos by checking for unknown parameters and bailing.
> >
> > Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> > ---
> >  tools/kvm/builtin-run.c |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/kvm/builtin-run.c b/tools/kvm/builtin-run.c
> > index a36bd00..9e5c1d4 100644
> > --- a/tools/kvm/builtin-run.c
> > +++ b/tools/kvm/builtin-run.c
> > @@ -257,7 +257,8 @@ static int set_net_param(struct virtio_net_params *p, const char *param,
> >                 p->vhost = atoi(val);
> >         } else if (strcmp(param, "fd") == 0) {
> >                 p->fd = atoi(val);
> > -       }
> > +       } else
> > +               die("Unknown network parameter %s", param);
> 
> we need braces here:

We don't _need_ braces, but I assume you mean you'd prefer braces?

cheers

Re: [PATCH 2/2] kvm tools: Check for unknown parameters in network option handling

[Pekka Enberg]

On Mon, Aug 13, 2012 at 6:31 AM, Michael Ellerman
<michael@ellerman.id.au> wrote:
>> > diff --git a/tools/kvm/builtin-run.c b/tools/kvm/builtin-run.c
>> > index a36bd00..9e5c1d4 100644
>> > --- a/tools/kvm/builtin-run.c
>> > +++ b/tools/kvm/builtin-run.c
>> > @@ -257,7 +257,8 @@ static int set_net_param(struct virtio_net_params *p, const char *param,
>> >                 p->vhost = atoi(val);
>> >         } else if (strcmp(param, "fd") == 0) {
>> >                 p->fd = atoi(val);
>> > -       }
>> > +       } else
>> > +               die("Unknown network parameter %s", param);
>>
>> we need braces here:
>
> We don't _need_ braces, but I assume you mean you'd prefer braces?

This is Linux coding style so we don't prefer them either.

Re: [PATCH 2/2] kvm tools: Check for unknown parameters in network option handling

[Asias He]

On Mon, Aug 13, 2012 at 3:27 PM, Pekka Enberg <penberg@kernel.org> wrote:
> On Mon, Aug 13, 2012 at 6:31 AM, Michael Ellerman
> <michael@ellerman.id.au> wrote:
>>> > diff --git a/tools/kvm/builtin-run.c b/tools/kvm/builtin-run.c
>>> > index a36bd00..9e5c1d4 100644
>>> > --- a/tools/kvm/builtin-run.c
>>> > +++ b/tools/kvm/builtin-run.c
>>> > @@ -257,7 +257,8 @@ static int set_net_param(struct virtio_net_params *p, const char *param,
>>> >                 p->vhost = atoi(val);
>>> >         } else if (strcmp(param, "fd") == 0) {
>>> >                 p->fd = atoi(val);
>>> > -       }
>>> > +       } else
>>> > +               die("Unknown network parameter %s", param);
>>>
>>> we need braces here:
>>
>> We don't _need_ braces, but I assume you mean you'd prefer braces?
>
> This is Linux coding style so we don't prefer them either.

Documentation/CodingStyle says:
'''
Do not unnecessarily use braces where a single statement will do.

if (condition)
        action();

and

if (condition)
        do_this();
else
        do_that();

This does not apply if only one branch of a conditional statement is a single
statement; in the latter case use braces in both branches:

if (condition) {
        do_this();
        do_that();
} else {
        otherwise();
}
'''
we have a multiple line branch in set_net_param(), so I think it's
better to have the braces for the last die() branch.

-- 
Asias He

Re: [PATCH 1/2] kvm tools: Fix formatting of error message in TAP handling

[Asias He]

On Fri, Aug 10, 2012 at 1:38 PM, Michael Ellerman
<michael@ellerman.id.au> wrote:
> This error message is missing a space, and has a redundant ":" at the end,
> currently it produces:
>
>   You have requested a TAP device, but creation of one hasfailed because:: No such file or directory
>
> Add a space to "hasfailed" and remove the extra ":".
>
> Don't split the line to improve grepability.
>
> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> ---
>  tools/kvm/virtio/net.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/tools/kvm/virtio/net.c b/tools/kvm/virtio/net.c
> index 10420ae..8f3735b 100644
> --- a/tools/kvm/virtio/net.c
> +++ b/tools/kvm/virtio/net.c
> @@ -526,8 +526,7 @@ void virtio_net__init(const struct virtio_net_params *params)
>         ndev->mode = params->mode;
>         if (ndev->mode == NET_MODE_TAP) {
>                 if (!virtio_net__tap_init(params, ndev))
> -                       die_perror("You have requested a TAP device, but creation of one has"
> -                                       "failed because:");
> +                       die_perror("You have requested a TAP device, but creation of one has failed because");
>                 ndev->ops = &tap_ops;
>         } else {
>                 ndev->info.host_ip              = ntohl(inet_addr(params->host_ip));
> --
> 1.7.9.5


Acked-by: Asias He <asias.hejun@gmail.com>

-- 
Asias He