[PATCH] fix a code style in drivers/char/virtio_console.c

[PATCH] fix a code style in drivers/char/virtio_console.c

[Steven Liu]

fix a code style in drivers/char/virtio_console.c

Signed-off-by: Liu Qi <lingjiujianke@gmail.com>
---
 drivers/char/virtio_console.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 196428c..61dad7e 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1299,36 +1299,31 @@ static int init_vqs(struct ports_device *portdev)
        char **io_names;
        struct virtqueue **vqs;
        u32 i, j, nr_ports, nr_queues;
-       int err;
+       int err = -ENOMEM;

        nr_ports = portdev->config.max_nr_ports;
        nr_queues = use_multiport(portdev) ? (nr_ports + 1) * 2 : 2;

        vqs = kmalloc(nr_queues * sizeof(struct virtqueue *), GFP_KERNEL);
        if (!vqs) {
-               err = -ENOMEM;
                goto fail;
        }
        io_callbacks = kmalloc(nr_queues * sizeof(vq_callback_t *), GFP_KERNEL);
        if (!io_callbacks) {
-               err = -ENOMEM;
                goto free_vqs;
        }
        io_names = kmalloc(nr_queues * sizeof(char *), GFP_KERNEL);
        if (!io_names) {
-               err = -ENOMEM;
                goto free_callbacks;
        }
        portdev->in_vqs = kmalloc(nr_ports * sizeof(struct virtqueue *),
                                  GFP_KERNEL);
        if (!portdev->in_vqs) {
-               err = -ENOMEM;
                goto free_names;
        }
        portdev->out_vqs = kmalloc(nr_ports * sizeof(struct virtqueue *),
                                   GFP_KERNEL);
        if (!portdev->out_vqs) {
-               err = -ENOMEM;
                goto free_invqs;
        }

--
1.7.1



Best regards

Re: [PATCH] fix a code style in drivers/char/virtio_console.c

[Amit Shah]

On (Tue) May 18 2010 [13:17:22], Steven Liu wrote:
> fix a code style in drivers/char/virtio_console.c
> 
> Signed-off-by: Liu Qi <lingjiujianke@gmail.com>

NACK.

This style is deliberate and follows Rusty's style: if we end up using
'err' uninitialised in some code path, it can get flagged by source
checkers.

		Amit

Re: [PATCH] fix a code style in drivers/char/virtio_console.c

[Steven Liu]

Hi, Amit,

    if 'err' initialised in this path, it needn't do err = -ENOMEM
after,isn't it?



Best regards


2010/5/18 Amit Shah <amit.shah@redhat.com>:
> On (Tue) May 18 2010 [13:17:22], Steven Liu wrote:
>> fix a code style in drivers/char/virtio_console.c
>>
>> Signed-off-by: Liu Qi <lingjiujianke@gmail.com>
>
> NACK.
>
> This style is deliberate and follows Rusty's style: if we end up using
> 'err' uninitialised in some code path, it can get flagged by source
> checkers.
>
>                Amit
>

Re: [PATCH] fix a code style in drivers/char/virtio_console.c

[Amit Shah]

Hello,

On (Tue) May 18 2010 [13:41:29], Steven Liu wrote:
> Hi, Amit,
> 
>     if 'err' initialised in this path, it needn't do err = -ENOMEM
> after,isn't it?

What I mean is if we later add some code that just does:

	if (err)
		goto fail;

then 'ret' can be -ENOMEM, as it was initialised to, which would be
fine. But if a later patch adds something like:

+	ret = -EIO;
+	err = ...
+	if (err)
+		goto fail;
+
	err = ...
	if (err)
		goto fail;

In this case, the 2nd if() would now return EIO instead of ENOMEM as
earlier.

Also, this style of coding can prevent uninitialised usage of 'ret', eg:


	int ret;

	if (err)
		goto fail;

fail:
	return ret;

In this case, the compiler will warn about 'ret' being used
uninitialised.

This is just a coding style issue. I had initially coded it the way your
patch does, but Rusty asked me to change that and I like this new style
better: there's less scope for surprises.

		Amit

Re: [PATCH] fix a code style in drivers/char/virtio_console.c

[Steven Liu]

okay, i see, thanks for your regards :)

2010/5/18 Amit Shah <amit.shah@redhat.com>:
> Hello,
>
> On (Tue) May 18 2010 [13:41:29], Steven Liu wrote:
>> Hi, Amit,
>>
>>     if 'err' initialised in this path, it needn't do err = -ENOMEM
>> after,isn't it?
>
> What I mean is if we later add some code that just does:
>
>        if (err)
>                goto fail;
>
> then 'ret' can be -ENOMEM, as it was initialised to, which would be
> fine. But if a later patch adds something like:
>
> +       ret = -EIO;
> +       err = ...
> +       if (err)
> +               goto fail;
> +
>        err = ...
>        if (err)
>                goto fail;
>
> In this case, the 2nd if() would now return EIO instead of ENOMEM as
> earlier.
>
> Also, this style of coding can prevent uninitialised usage of 'ret', eg:
>
>
>        int ret;
>
>        if (err)
>                goto fail;
>
> fail:
>        return ret;
>
> In this case, the compiler will warn about 'ret' being used
> uninitialised.
>
> This is just a coding style issue. I had initially coded it the way your
> patch does, but Rusty asked me to change that and I like this new style
> better: there's less scope for surprises.
>
>                Amit
>