On Sun, Jul 14, 2013 at 2:29 PM, Dave Reisner <d@xxxxxxxxxxxxxx> wrote:
> On Sun, Jul 14, 2013 at 01:56:17PM +0200, Tom Gundersen wrote:
>> + strncpy(output, optarg, PATH_MAX);
>
> No need to copy anything here, just retain a pointer to this optarg.
Fixed.
>> + if (out == NULL) {
>
> It took me a minute to realize why this was the correct comparison.
> Maybe it's just me, but I think this would be more readable if we did
> something like the below psuedocode
>
> const char* output = "/dev/stderr";
> /* do option parsing, maybe reassigning 'output' */
>
> /* now open the file, regardless of what it is */
> FILE* out = fopen(output, "we");
> if (out == NULL)
> ...
>
> Seems a bit cleaner to me, even if it duplicates a file descriptor (but
> we'll open a new FILE regardless in the common use case).
Yeah, makes sense to me, I'll do that instead.
>> + out = fopen(output, "we");
>> + if (out == NULL) {
>> + fprintf(stderr, "Error: could not create %s!\n",
>> + output);
>
> I realize this is copied from the old code, but there's no explanation
> as to why this failed. Add an %m token to the format string?
For some reason that change ended up in the subsequent patch, anyway,
moving it to this one where it belong.
Thanks.
Tom
--
To unsubscribe from this list: send the line "unsubscribe linux-modules" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
