Re: [PATCH 2/2] mmc: fix integer assignments to pointer

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 08/23/2011 09:56 AM, Sam Ravnborg wrote:
On Tue, Aug 23, 2011 at 12:31:55PM -0400, Chris Ball wrote:
Hi,

[Adding linux-sparse@ to CC]

On Tue, Aug 23 2011, Venkatraman S wrote:
Fix the sparse warning output
"warning: Using plain integer as NULL pointer"

Signed-off-by: Venkatraman S<svenkatr@xxxxxx>
---
  drivers/mmc/card/block.c    |    4 ++--
  drivers/mmc/core/core.c     |    2 +-
  drivers/mmc/core/mmc_ops.c  |    4 ++--
  drivers/mmc/core/sdio_ops.c |    2 +-
  4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 1ff5486..e702c61 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -291,7 +291,7 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
  	struct mmc_card *card;
  	struct mmc_command cmd = {0};
  	struct mmc_data data = {0};
-	struct mmc_request mrq = {0};
+	struct mmc_request mrq = {NULL};
  	struct scatterlist sg;
  	int err;
[...]

The sparse warning is mistaken.  Or I'm mistaken.  But I suspect it's
the sparse warning.


Okay, stupid question times:

>>> -	struct mmc_request mrq = {0};

this is 'struct mmc_request mrq' and not 'struct mmc_request *mrq', right?

Then I think the sparse warning is correct. From my experience, sparse is very clear: if any variable is defined as a pointer, you use NULL to reset the pointer instead of '0', and if any variable is a normal variable, you use '0' to reset the variable. I don't think sparse is smart enough to look at the underlying variables, which in this case, mrq contains members that are pointers to something.

in this case, the defined variable is not a pointer, rather a variable named mrq that is of type 'struct mmc_request'. Therefore, 0 is correct to use and NULL is incorrect.

If you want it to be sparse-correct as well as human-readable correct, you probably should write the code that explicitly shows assigning each pointer member of mrq to NULL, ie:

struct mmc_request mrq;
mrq.sbc = NULL;
mrq.cmd = NULL;
mrq.data = NULL;
.
.
etc.



The {0} syntax is covered by:

    [6.7.8.21] If there are fewer initializers in a brace-enclosed list
    than there are elements or members of an aggregate, or fewer
    characters in a string literal used to initialize an array of known
    size than there are elements in the array, the remainder of the
    aggregate shall be initialized implicitly the same as objects that
    have static storage duration.

So we're not assigning 0 to a pointer, or whatever sparse thinks we're
doing -- we're initializing every member of the struct with 0, which is
a good and safe way to initialize it.

Sparse folks, any comment?

The struct looks like this:
struct mmc_request {
         struct mmc_command      *sbc;           /* SET_BLOCK_COUNT for multiblock */
         struct mmc_command      *cmd;
         struct mmc_data         *data;
         struct mmc_command      *stop;

         struct completion       completion;
         void                    (*done)(struct mmc_request *);/* completion function */
};


So you assing '0' to sbc - which is a pointer.
So sparse warning is correct.

	Sam
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
J (James/Jay) Freyensee
Storage Technology Group
Intel Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Newbies FAQ]     [LKML]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Trinity Fuzzer Tool]

  Powered by Linux