[PATCH 1/6] [MTD-UTILS] nandwrite: Qualifier Clean-up

[PATCH 1/6] [MTD-UTILS] nandwrite: Qualifier Clean-up

[Grant Erickson]

Static-qualified all globals except 'main' because they have no use
beyond file scope.
Constant-qualified MTD device and input positional parameter globals.
Constant-qualified argv array.

Signed-off-by: Grant Erickson <gerickson@nuovations.com>
---
 nandwrite.c |   48 ++++++++++++++++++++++++------------------------
 1 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/nandwrite.c b/nandwrite.c
index e745fba..8e0609d 100644
--- a/nandwrite.c
+++ b/nandwrite.c
@@ -45,32 +45,32 @@
 /*
  * Buffer array used for writing data
  */
-unsigned char writebuf[MAX_PAGE_SIZE];
-unsigned char oobbuf[MAX_OOB_SIZE];
-unsigned char oobreadbuf[MAX_OOB_SIZE];
+static unsigned char writebuf[MAX_PAGE_SIZE];
+static unsigned char oobbuf[MAX_OOB_SIZE];
+static unsigned char oobreadbuf[MAX_OOB_SIZE];
 
 // oob layouts to pass into the kernel as default
-struct nand_oobinfo none_oobinfo = {
+static struct nand_oobinfo none_oobinfo = {
 	.useecc = MTD_NANDECC_OFF,
 };
 
-struct nand_oobinfo jffs2_oobinfo = {
+static struct nand_oobinfo jffs2_oobinfo = {
 	.useecc = MTD_NANDECC_PLACE,
 	.eccbytes = 6,
 	.eccpos = { 0, 1, 2, 3, 6, 7 }
 };
 
-struct nand_oobinfo yaffs_oobinfo = {
+static struct nand_oobinfo yaffs_oobinfo = {
 	.useecc = MTD_NANDECC_PLACE,
 	.eccbytes = 6,
 	.eccpos = { 8, 9, 10, 13, 14, 15}
 };
 
-struct nand_oobinfo autoplace_oobinfo = {
+static struct nand_oobinfo autoplace_oobinfo = {
 	.useecc = MTD_NANDECC_AUTOPLACE
 };
 
-void display_help (void)
+static void display_help (void)
 {
 	printf("Usage: nandwrite [OPTION] MTD_DEVICE INPUTFILE\n"
 			"Writes to the specified MTD device.\n"
@@ -91,7 +91,7 @@ void display_help (void)
 	exit(0);
 }
 
-void display_version (void)
+static void display_version (void)
 {
 	printf(PROGRAM " " VERSION "\n"
 			"\n"
@@ -106,20 +106,20 @@ void display_version (void)
 	exit(0);
 }
 
-char	*mtd_device, *img;
-int	mtdoffset = 0;
-int	quiet = 0;
-int	writeoob = 0;
-int	markbad = 0;
-int	autoplace = 0;
-int	forcejffs2 = 0;
-int	forceyaffs = 0;
-int	forcelegacy = 0;
-int	noecc = 0;
-int	pad = 0;
-int	blockalign = 1; /*default to using 16K block size */
-
-void process_options (int argc, char *argv[])
+static const char	*mtd_device, *img;
+static int		mtdoffset = 0;
+static int		quiet = 0;
+static int		writeoob = 0;
+static int		markbad = 0;
+static int		autoplace = 0;
+static int		forcejffs2 = 0;
+static int		forceyaffs = 0;
+static int		forcelegacy = 0;
+static int		noecc = 0;
+static int		pad = 0;
+static int		blockalign = 1; /*default to using 16K block size */
+
+static void process_options (int argc, char * const argv[])
 {
 	int error = 0;
 
@@ -209,7 +209,7 @@ void process_options (int argc, char *argv[])
 /*
  * Main program
  */
-int main(int argc, char **argv)
+int main(int argc, char * const argv[])
 {
 	int cnt, fd, ifd, imglen = 0, pagelen, baderaseblock, blockstart = -1;
 	struct mtd_info_user meminfo;
-- 
1.6.0.1

Re: [PATCH 1/6] [MTD-UTILS] nandwrite: Qualifier Clean-up

[Artem Bityutskiy]

On Sun, 2008-09-07 at 21:24 -0700, Grant Erickson wrote:
> Static-qualified all globals except 'main' because they have no use
> beyond file scope.
> Constant-qualified MTD device and input positional parameter globals.
> Constant-qualified argv array.

Ack for whole series.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [PATCH 1/6] [MTD-UTILS] nandwrite: Qualifier Clean-up

[Josh Boyer]

On Mon, Sep 08, 2008 at 08:28:02AM +0300, Artem Bityutskiy wrote:
>On Sun, 2008-09-07 at 21:24 -0700, Grant Erickson wrote:
>> Static-qualified all globals except 'main' because they have no use
>> beyond file scope.
>> Constant-qualified MTD device and input positional parameter globals.
>> Constant-qualified argv array.
>
>Ack for whole series.

For my benefit, it would be nice if you put the full Acked-by: line in
future emails.  Then I don't have to go lookup what you typically use
for email address, etc in those cases :).

I'm going to assmue this is fine:

Acked-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

Yell if it's not.

josh

Re: [PATCH 1/6] [MTD-UTILS] nandwrite: Qualifier Clean-up

[Artem Bityutskiy]

On Mon, 2008-09-08 at 10:17 -0400, Josh Boyer wrote:
> For my benefit, it would be nice if you put the full Acked-by: line in
> future emails.  Then I don't have to go lookup what you typically use
> for email address, etc in those cases :).

OK, sorry for inconvenience.

> Acked-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

Sure it is, thanks.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [PATCH 1/6] [MTD-UTILS] nandwrite: Qualifier Clean-up

[Enrico Scholz]

Grant Erickson <gerickson@nuovations.com> writes:

> +static const char	*mtd_device, *img;

would it be possible to split this into two declarations?  I
really don't know if 'img' points to a constant or non-constant
char...


> -int main(int argc, char **argv)
> +int main(int argc, char * const argv[])

Is this really correct?  C standard mentions only

               int main(void) { /* ... */ }
               int main(int argc, char *argv[]) { /* ... */ }

as program entry points.  I suggest to cast 'argv' to a corresponding
data type when it is used.


Enrico

Re: [PATCH 1/6] [MTD-UTILS] nandwrite: Qualifier Clean-up

[Grant Erickson]

On 9/9/08 6:23 AM, Enrico Scholz wrote:
> Grant Erickson <gerickson@nuovations.com> writes:
>> +static const char *mtd_device, *img;
> 
> would it be possible to split this into two declarations?  I
> really don't know if 'img' points to a constant or non-constant
> char...

As currently implemented, both mtd_device and img point to effectively
read-only information. That is, there is no need to modify or to attempt to
modify what they point to.

Should that change in the future, these can be declared differently;
however, for the present, the const qualifier accurately represents program
intent.

>> -int main(int argc, char **argv)
>> +int main(int argc, char * const argv[])
> 
> Is this really correct?  C standard mentions only
> 
>                int main(void) { /* ... */ }
>                int main(int argc, char *argv[]) { /* ... */ }
> 
> as program entry points.  I suggest to cast 'argv' to a corresponding
> data type when it is used.

Those are certainly the two most common prototypes for main. However,
because main has no prototype, adding the const qualifier simply conveys
current program intent. That is, the strings pointed to by argv are not
modified.

Regards,

Grant

Re: [PATCH 1/6] [MTD-UTILS] nandwrite: Qualifier Clean-up

[Enrico Scholz]

Grant Erickson <gerickson@nuovations.com> writes:

>>> +static const char *mtd_device, *img;
>> 
>> would it be possible to split this into two declarations?  I
>> really don't know if 'img' points to a constant or non-constant
>> char...
>
> As currently implemented, both mtd_device and img point to
> effectively read-only information. That is, there is no need to
> modify or to attempt to modify what they point to.

Sorry, I might be unclear here. My comment was about readability
not about correctness; e.g. without studying C standard, it is
not obvious to me, whether

| char const *a, *b;

means

| char const *a;
| char const *b;

or

| char const *a;
| char *b;


When you think that answer to this question is trivial, then
please explain 'char * const *a, *b;' ;)


>>> -int main(int argc, char **argv)
>>> +int main(int argc, char * const argv[])
>> 
>> Is this really correct?  C standard mentions only
>> 
>>                int main(void) { /* ... */ }
>>                int main(int argc, char *argv[]) { /* ... */ }
>> 
>> as program entry points.  I suggest to cast 'argv' to a corresponding
>> data type when it is used.
>
> Those are certainly the two most common prototypes for main. However,
> because main has no prototype

Compiler knows about the two possible prototypes. Perhaps next gcc
version or another compiler (icc) complains about the non-standard
main().


> adding the const qualifier simply conveys current program
> intent. That is, the strings pointed to by argv are not
> modified.

Then, you can write

| int main(int argc, char const * const argv[])

;)


fwiw, the const'ness if argv[] content is violated when calling
GNU getopt(3) as it reorders arguments.  E.g.

| argv[] = { "foo", "-a", "bar", "-c", NULL }

is changed by this function to

| argv[] = { "foo", "-a", "-c", "bar", NULL }



Enrico

Re: [PATCH 1/6] [MTD-UTILS] nandwrite: Qualifier Clean-up

[Grant Erickson]

On 9/9/08 10:14 AM, Enrico Scholz wrote:
> Grant Erickson <gerickson@nuovations.com> writes:
> >>> +static const char *mtd_device, *img;
>>> 
>>> would it be possible to split this into two declarations?  I
>>> really don't know if 'img' points to a constant or non-constant
>>> char...
>> 
>> As currently implemented, both mtd_device and img point to
>> effectively read-only information. That is, there is no need to
>> modify or to attempt to modify what they point to.
> 
> Sorry, I might be unclear here. My comment was about readability
> not about correctness; e.g. without studying C standard, it is
> not obvious to me, whether
>
> [ Examples Omitted ]
>
> 
> When you think that answer to this question is trivial, then
> please explain 'char * const *a, *b;' ;)

Fair enough. Patch submitted.

>>>> -int main(int argc, char **argv)
>>>> +int main(int argc, char * const argv[])
>>> 
>>> Is this really correct?  C standard mentions only
>>> 
>>>                int main(void) { /* ... */ }
>>>                int main(int argc, char *argv[]) { /* ... */ }
>>> 
>>> as program entry points.  I suggest to cast 'argv' to a corresponding
>>> data type when it is used.
> 
>> adding the const qualifier simply conveys current program
>> intent. That is, the strings pointed to by argv are not
>> modified.
> 
> Then, you can write
> 
> | int main(int argc, char const * const argv[])

That's certainly possible as well; however, that was not the intent and, as
you cite with getopt below, would not accurately reflect how the program
respect argv.

> fwiw, the const'ness if argv[] content is violated when calling
> GNU getopt(3) as it reorders arguments.  E.g.
> 
> | argv[] = { "foo", "-a", "bar", "-c", NULL }
> 
> is changed by this function to
> 
> | argv[] = { "foo", "-a", "-c", "bar", NULL }

Understood on the operation of getopt. The declared intent was not that
order of the cells is constant but that the contents of the cells (i.e. the
strings) are.

Regards,

Grant