[Toybox] changeset 1571:e85e5f3b87c2

[Rob Landley]

On 11/23/14 00:55, enh wrote:
> (not sure how you guys do code review, so for now just mailing the list...)

To the mailing list is it.

(I have a collection of old code review/cleanup posts at
http://landley.net/toybox/cleanup.html for demonstration purposes, but
no formal tools for performing review.)

> neither clang 3.5 nor GCC 4.8 like changeset 1571:e85e5f3b87c2:
> 
> external/toybox/toys/other/lspci.c: In function 'do_lspci':
> external/toybox/toys/other/lspci.c:56:61: warning: suggest braces
> around empty body in an 'if' statement [-Wempty-body]
>      if (readlinkat(dirfd, "driver", driver, sizeof(driver)));
>                                                              ^
> i think they assume you use a style like this, which also makes clang
> happy (though clang would have settled for the ; on another line too):

Sigh. What I'm trying to do is get fortify to ignore the return value,
withotu adding code size. We zero out the first byte of the buffer, and
if the link isn't there the buffer byte remains zeroed, so we base our
error path off of that instead of checking the return code.

> diff --git a/toys/other/lspci.c b/toys/other/lspci.c
> index 6a265a1..bf8a2e4 100644
> --- a/toys/other/lspci.c
> +++ b/toys/other/lspci.c
> @@ -50,10 +50,11 @@ int do_lspci(struct dirtree *new)
>    if (-1 == (dirfd = openat(dirtree_parentfd(new), new->name, O_RDONLY)))
>      return 0;
> 
> -  // it's ok for the driver link not to be there, whatever fortify says
>    *driver = 0;
>    if (toys.optflags & FLAG_k)
> -    if (readlinkat(dirfd, "driver", driver, sizeof(driver)));
> +    if (readlinkat(dirfd, "driver", driver, sizeof(driver))) {
> +      // it's ok for the driver link not to be there, whatever fortify says
> +    }

Eh, that works.

>    for (fields = (char*[]){"class", "vendor", "device", 0}; *fields; fields++) {
>      int fd, size = 6 + 2*((toys.optflags & FLAG_e) && p == toybuf);
> 
> though this diff (which also makes both happy) feels more like your style:
> 
> diff --git a/toys/other/lspci.c b/toys/other/lspci.c
> index 6a265a1..928e2d3 100644
> --- a/toys/other/lspci.c
> +++ b/toys/other/lspci.c
> @@ -53,7 +53,7 @@ int do_lspci(struct dirtree *new)
>    // it's ok for the driver link not to be there, whatever fortify says
>    *driver = 0;
>    if (toys.optflags & FLAG_k)
> -    if (readlinkat(dirfd, "driver", driver, sizeof(driver)));
> +    (void) readlinkat(dirfd, "driver", driver, sizeof(driver));

Six of one, half dozen of the other...

I actually slightly prefer the second because "the compiler accepts
empty curly brackets but won't accept a semicolon, except it would
accept the semicolon if it was on a different line but not on the same
line even though whitespace isn't supposed to be significant in C"... I
really don't want to try to encapsulate that in a comment. The "typecast
to shut up the compiler" is at least easier to explain.

Thanks,

Rob

 1416786798.0