[PATCH 3/3] compositor: dump command line options

[Pekka Paalanen]

On Tue, 20 Mar 2012 00:57:49 -0300
Tiago Vignatti <tiago.vignatti at linux.intel.com> wrote:

> >> +	if (help) {
> >> +		dump_options(core_options, ARRAY_LENGTH(core_options));
> >> +		argv[argc] = strdup("--help");
> >> +		argc++;
> >
> > How do you know there is space in argv[] to add that?
> > Also, doesn't strdup() leak here?
> >
> > Hm, so if there was a --help originally, there must be space to add it
> > back... I wonder if it made sense to handle --help specially in option
> > parser, and let it indicate "help encountered" with an error code
> > return. We are going to need the error code return anyway.
> 
> you're right. So argv is at stack and can be referenceable, so maybe we 
> can use the following to get the address back:
> 
> +       if (help) {
> +               dump_options(core_options, ARRAY_LENGTH(core_options));
> +               argv[argc] = argv[argc - 1] + strlen(argv[argc - 1]) +1;
> +               strcpy(argv[argc], "--help");
> +               argc++;
> +       }
> 
> is there a cleaner way maybe to get?

I agree with Bill's comment that a simple argv[argc] = "--help"; is
right, if you want to go there. By space I meant space for a pointer in
argv, not the space for the string.

After all, argv is an array of pointers, and the pointers point to
char arrays where-ever, and the char arrays need not be consecutively
packed in memory (or at least I would never assume that, certainly
after a parse_options() call they are not packed anymore).

I would also never try to write to strings pointed to by argv.

> And I didn't want to prolong too much with this all, for instance 
> treating --help as special case; not sure we'll soon need an error code 
> return there, as you mentioned. Why you say so? I'm seeing the patch as 
> it is as an improvement already and, frankly, it's quite lean.

I would really want to see apps being able to exit nicely with an error,
if the arguments given are invalid, not simply forced to ignore errors
and continue.

Accepting this patch is not up to me, I just would've done it
differently. You are free to get it committed, it is an improvement.

This is why I originally did not comment anything here.


Thanks,
pq