Patchwork [05/14] Remove unused argument for check_for_block_signature()

login
register
mail settings
Submitter Jes Sorensen
Date Aug. 30, 2010, 3:59 p.m.
Message ID <1283183960-28404-6-git-send-email-Jes.Sorensen@redhat.com>
Download mbox | patch
Permalink /patch/63073/
State New
Headers show

Comments

Jes Sorensen - Aug. 30, 2010, 3:59 p.m.
From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 block/raw.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Paolo Bonzini - Aug. 30, 2010, 4:40 p.m.
On 08/30/2010 06:16 PM, Anthony Liguori wrote:
> This is why this type of warning sucks.  Passing BlockDriverState is a
> matter of readability because these are roughly methods.  Just because
> 'this' isn't used right now, doesn't mean that it should not be a method.

On the contrary, to me this is acceptable (or even "a good thing") 
because a patch introducing the first use of a so-far-unused argument 
deserves a more careful review.  In fact, if we were using C++, 
check_for_block_signature should have been static.

The cases where the "this" argument is unused in a method should stand 
out as possible bugs, as is the case with the parse errors in the JSON 
parser (which _is_ a bug, as the caller cannot intercept error messages 
right now).  check_for_block_signature is not one of these.

Paolo
Jes Sorensen - Aug. 30, 2010, 6:19 p.m.
On 08/30/10 18:40, Paolo Bonzini wrote:
> On 08/30/2010 06:16 PM, Anthony Liguori wrote:
>> This is why this type of warning sucks.  Passing BlockDriverState is a
>> matter of readability because these are roughly methods.  Just because
>> 'this' isn't used right now, doesn't mean that it should not be a method.
> 
> On the contrary, to me this is acceptable (or even "a good thing")
> because a patch introducing the first use of a so-far-unused argument
> deserves a more careful review.  In fact, if we were using C++,
> check_for_block_signature should have been static.
> 
> The cases where the "this" argument is unused in a method should stand
> out as possible bugs, as is the case with the parse errors in the JSON
> parser (which _is_ a bug, as the caller cannot intercept error messages
> right now).  check_for_block_signature is not one of these.

I totally agree on this. The problem with having such arguments passed
in is that you never know if they were used in the past and it was
forgotten when the code using them was removed, or if it's new code, in
which case they do deserve the extra scrutiny.

Cheers,
Jes
Anthony Liguori - Aug. 30, 2010, 6:27 p.m.
On 08/30/2010 01:19 PM, Jes Sorensen wrote:
>
> I totally agree on this. The problem with having such arguments passed
> in is that you never know if they were used in the past and it was
> forgotten when the code using them was removed, or if it's new code, in
> which case they do deserve the extra scrutiny.
>    

Or, we exercise common sense instead of blinding removing arguments just 
because a certain uncommon warning mode of GCC complains.

Regards,

Anthony Liguori

> Cheers,
> Jes
>
Richard Henderson - Aug. 30, 2010, 7:24 p.m.
On 08/30/2010 11:27 AM, Anthony Liguori wrote:
> On 08/30/2010 01:19 PM, Jes Sorensen wrote:
>> 
>> I totally agree on this. The problem with having such arguments
>> passed in is that you never know if they were used in the past and
>> it was forgotten when the code using them was removed, or if it's
>> new code, in which case they do deserve the extra scrutiny.
>> 
> 
> Or, we exercise common sense instead of blinding removing arguments
> just because a certain uncommon warning mode of GCC complains.

If you make a reasoned decision to keep the argument,
then annotate it with

#define UNUSED  __attribute__((unused))

and the warning will go away.

As to whether the argument should be retained in these
specific cases, I am agnostic.


r~
Jes Sorensen - Aug. 31, 2010, 7:13 a.m.
On 08/30/10 20:27, Anthony Liguori wrote:
> On 08/30/2010 01:19 PM, Jes Sorensen wrote:
>>
>> I totally agree on this. The problem with having such arguments passed
>> in is that you never know if they were used in the past and it was
>> forgotten when the code using them was removed, or if it's new code, in
>> which case they do deserve the extra scrutiny.
>>    
> 
> Or, we exercise common sense instead of blinding removing arguments just
> because a certain uncommon warning mode of GCC complains.

Before making the change, I did indeed look at the code for a while,
considering whether it was reasonable to leave the unused variable in
place. However I don't see anything in there that makes it likely that
the block state parameter is going to be used in that function in the
near future, if at all.

If you feel so strongly about it, then lets apply the __unused attribute
as Richard suggested so it's clear that that argument was added on purpose.

Jes

Patch

diff --git a/block/raw.c b/block/raw.c
index 61e6748..fc057d0 100644
--- a/block/raw.c
+++ b/block/raw.c
@@ -12,7 +12,7 @@  static int raw_open(BlockDriverState *bs, int flags)
 /* check for the user attempting to write something that looks like a
    block format header to the beginning of the image and fail out.
 */
-static int check_for_block_signature(BlockDriverState *bs, const uint8_t *buf)
+static int check_for_block_signature(const uint8_t *buf)
 {
     static const uint8_t signatures[][4] = {
         { 'Q', 'F', 'I', 0xfb }, /* qcow/qcow2 */
@@ -42,7 +42,7 @@  static int check_write_unsafe(BlockDriverState *bs, int64_t sector_num,
     }
 
     if (sector_num == 0 && nb_sectors > 0) {
-        return check_for_block_signature(bs, buf);
+        return check_for_block_signature(buf);
     }
 
     return 0;