[Qemu-devel] [PATCH 1.1] virtio: Fix compiler warning for non Linux hosts

[Qemu-devel] [PATCH 1.1] virtio: Fix compiler warning for non Linux hosts

[Stefan Weil]

The local variables ret, i are only used if __linux__ is defined.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 hw/virtio-blk.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index f9e1896..60750cb 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -147,9 +147,11 @@ static VirtIOBlockReq *virtio_blk_get_request(VirtIOBlock *s)
 
 static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
 {
+#ifdef __linux__
     int ret;
-    int status = VIRTIO_BLK_S_OK;
     int i;
+#endif
+    int status = VIRTIO_BLK_S_OK;
 
     /*
      * We require at least one output segment each for the virtio_blk_outhdr
-- 
1.7.10

Re: [Qemu-devel] [PATCH 1.1] virtio: Fix compiler warning for non Linux hosts

[Stefan Hajnoczi]

On Tue, May 22, 2012 at 10:23 PM, Stefan Weil <sw@weilnetz.de> wrote:
> The local variables ret, i are only used if __linux__ is defined.
>
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>  hw/virtio-blk.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

The #ifdef __linux__ further down in the function declares the local
hdr variable.  We could move ret and i down into the #ifdef instead of
adding a new one.

Re: [Qemu-devel] [PATCH 1.1] virtio: Fix compiler warning for non Linux hosts

[Stefan Weil]

Am 23.05.2012 10:09, schrieb Stefan Hajnoczi:
> On Tue, May 22, 2012 at 10:23 PM, Stefan Weil<sw@weilnetz.de>  wrote:
>> The local variables ret, i are only used if __linux__ is defined.
>>
>> Signed-off-by: Stefan Weil<sw@weilnetz.de>
>> ---
>>   hw/virtio-blk.c |    4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
> The #ifdef __linux__ further down in the function declares the local
> hdr variable.  We could move ret and i down into the #ifdef instead of
> adding a new one.

I noticed that, but declaring variables anywhere is C++, not C code.

hdr violates the QEMU coding rules (other patches which did not
declare local variables at the beginning of a block were already
rejected). That's why I wrote the patch as it is.

Regards,
Stefan W.

Re: [Qemu-devel] [PATCH 1.1] virtio: Fix compiler warning for non Linux hosts

[Kevin Wolf]

Am 23.05.2012 17:29, schrieb Stefan Weil:
> Am 23.05.2012 10:09, schrieb Stefan Hajnoczi:
>> On Tue, May 22, 2012 at 10:23 PM, Stefan Weil<sw@weilnetz.de>  wrote:
>>> The local variables ret, i are only used if __linux__ is defined.
>>>
>>> Signed-off-by: Stefan Weil<sw@weilnetz.de>
>>> ---
>>>   hw/virtio-blk.c |    4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>> The #ifdef __linux__ further down in the function declares the local
>> hdr variable.  We could move ret and i down into the #ifdef instead of
>> adding a new one.
> 
> I noticed that, but declaring variables anywhere is C++, not C code.

It's called C99.

Kevin

Re: [Qemu-devel] [PATCH 1.1] virtio: Fix compiler warning for non Linux hosts

[Stefan Weil]

Am 23.05.2012 17:32, schrieb Kevin Wolf:
> Am 23.05.2012 17:29, schrieb Stefan Weil:
>> Am 23.05.2012 10:09, schrieb Stefan Hajnoczi:
>>> On Tue, May 22, 2012 at 10:23 PM, Stefan Weil<sw@weilnetz.de>   wrote:
>>>> The local variables ret, i are only used if __linux__ is defined.
>>>>
>>>> Signed-off-by: Stefan Weil<sw@weilnetz.de>
>>>> ---
>>>>    hw/virtio-blk.c |    4 +++-
>>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>> The #ifdef __linux__ further down in the function declares the local
>>> hdr variable.  We could move ret and i down into the #ifdef instead of
>>> adding a new one.
>> I noticed that, but declaring variables anywhere is C++, not C code.
> It's called C99.
>
> Kevin
>

Maybe, but I already had patches rejected because of that style.
Did this policy change? I'd appreciate that!

Stefan

Re: [Qemu-devel] [PATCH 1.1] virtio: Fix compiler warning for non Linux hosts

[Stefan Hajnoczi]

On Wed, May 23, 2012 at 5:03 PM, Stefan Weil <sw@weilnetz.de> wrote:
> Am 23.05.2012 17:32, schrieb Kevin Wolf:
>
>> Am 23.05.2012 17:29, schrieb Stefan Weil:
>>>
>>> Am 23.05.2012 10:09, schrieb Stefan Hajnoczi:
>>>>
>>>> On Tue, May 22, 2012 at 10:23 PM, Stefan Weil<sw@weilnetz.de>   wrote:
>>>>>
>>>>> The local variables ret, i are only used if __linux__ is defined.
>>>>>
>>>>> Signed-off-by: Stefan Weil<sw@weilnetz.de>
>>>>> ---
>>>>>   hw/virtio-blk.c |    4 +++-
>>>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> The #ifdef __linux__ further down in the function declares the local
>>>> hdr variable.  We could move ret and i down into the #ifdef instead of
>>>> adding a new one.
>>>
>>> I noticed that, but declaring variables anywhere is C++, not C code.
>>
>> It's called C99.
>>
>> Kevin
>>
>
> Maybe, but I already had patches rejected because of that style.
> Did this policy change? I'd appreciate that!

Agreed, people have been asked to declare variables at the beginning
of the scope.  I don't understand why, C99 allows them to be declared
anywhere and it usually makes the code more readable IMO (you don't
have to jump to the definition to check a variable's type).

What's the problem with C99-style variable declarations anywhere in a function?

Stefan

Re: [Qemu-devel] [PATCH 1.1] virtio: Fix compiler warning for non Linux hosts

[Daniel P. Berrange]

On Mon, May 28, 2012 at 01:39:58PM +0100, Stefan Hajnoczi wrote:
> On Wed, May 23, 2012 at 5:03 PM, Stefan Weil <sw@weilnetz.de> wrote:
> > Am 23.05.2012 17:32, schrieb Kevin Wolf:
> >
> >> Am 23.05.2012 17:29, schrieb Stefan Weil:
> >>>
> >>> Am 23.05.2012 10:09, schrieb Stefan Hajnoczi:
> >>>>
> >>>> On Tue, May 22, 2012 at 10:23 PM, Stefan Weil<sw@weilnetz.de>   wrote:
> >>>>>
> >>>>> The local variables ret, i are only used if __linux__ is defined.
> >>>>>
> >>>>> Signed-off-by: Stefan Weil<sw@weilnetz.de>
> >>>>> ---
> >>>>>   hw/virtio-blk.c |    4 +++-
> >>>>>   1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>
> >>>> The #ifdef __linux__ further down in the function declares the local
> >>>> hdr variable.  We could move ret and i down into the #ifdef instead of
> >>>> adding a new one.
> >>>
> >>> I noticed that, but declaring variables anywhere is C++, not C code.
> >>
> >> It's called C99.
> >>
> >> Kevin
> >>
> >
> > Maybe, but I already had patches rejected because of that style.
> > Did this policy change? I'd appreciate that!
> 
> Agreed, people have been asked to declare variables at the beginning
> of the scope.  I don't understand why, C99 allows them to be declared
> anywhere and it usually makes the code more readable IMO (you don't
> have to jump to the definition to check a variable's type).
>
> What's the problem with C99-style variable declarations anywhere in a function?

Not an issue in this particular patch, but declaration at point of
first use can cause you problems if the code uses 'goto' alot (eg
for return cleanup).

eg

void somefunc(void) {
   ...some code...

   if (...something bad...)
     goto cleanup;

   ... some code...

   char *bar = NULL;
   bar = strdup("Hello World");

   ...some code...

 cleanup:
   free(bar);
}

In that 'cleanup' label 'bar' will potentially be uninitialized,
because it is in scope, but the line where initialization takes
place may never been reached.

So if you do decide to make use of C99 style decl at first use,
then be sure to turn on the GCC warnings to detect these variable
initialization problems.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 1.1] virtio: Fix compiler warning for non Linux hosts

[Peter Maydell]

On 28 May 2012 13:39, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> Agreed, people have been asked to declare variables at the beginning
> of the scope.  I don't understand why, C99 allows them to be declared
> anywhere and it usually makes the code more readable IMO (you don't
> have to jump to the definition to check a variable's type).

If the definition is that far away from the use it's probably a
sign that your function is too large anyway and you should be
looking for a smaller scope within which to declare the variable...

> What's the problem with C99-style variable declarations anywhere in a function?

I think they look slightly uglier but that's purely a personal
prejudice which I don't think I can justify imposing on anybody
else :-)

-- PMM

Re: [Qemu-devel] [PATCH 1.1] virtio: Fix compiler warning for non Linux hosts

[Andreas Färber]

Am 23.05.2012 10:09, schrieb Stefan Hajnoczi:
> On Tue, May 22, 2012 at 10:23 PM, Stefan Weil <sw@weilnetz.de> wrote:
>> The local variables ret, i are only used if __linux__ is defined.
>>
>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>> ---
>>  hw/virtio-blk.c |    4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> The #ifdef __linux__ further down in the function declares the local
> hdr variable.  We could move ret and i down into the #ifdef instead of
> adding a new one.

Sorry, this patch seems to have passed me by: I posted a patch moving
all Linux variables into an #ifdef at the top.

http://patchwork.ozlabs.org/patch/161546/

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg