11055 loader: zfs reader cstyle cleanup

Reviews
Diff

- Download Diff

Information
Submitter:	Toomas Soome
Repository:	illumos-gate
Branch:
Bugs:	11055
Depends On:
Commit:	4e08708...
Reviewers
Groups:	general
People:

Description

11055 loader: zfs reader cstyle cleanup

Testing Done

Issues

30
4
0
All Issues: 34

Description	From	Last Updated
Unnecessary blank line.	Gergő Mihály Doma	Nov. 13, 2019, 9:53 a.m.
"Never use the boolean negation operator (!) with non-boolean expressions." Should be: if (memcmp(name, pairname, namelen) == 0 && type ...	Gergő Mihály Doma	Nov. 13, 2019, 9:53 a.m.
Déjà vu... Type of found variable should be bool.	Gergő Mihály Doma	May 24, 2019, 1:24 p.m.
Format string - argument type mismatch: type of j variable is unsigned int.	Gergő Mihály Doma	May 24, 2019, 1:24 p.m.
"Never use the boolean negation operator (!) with non-boolean expressions."	Gergő Mihály Doma	May 24, 2019, 1:24 p.m.
0 vs NULL. This line should be: return (NULL);	Gergő Mihály Doma	Nov. 13, 2019, 9:55 a.m.
Type of is_new variable (and also the is_newer argument) should be bool.	Gergő Mihály Doma	May 24, 2019, 1:24 p.m.
0 vs NULL.	Gergő Mihály Doma	May 24, 2019, 1:24 p.m.
0 vs NULL.	Gergő Mihály Doma	May 24, 2019, 1:24 p.m.
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ...	Gergő Mihály Doma	May 24, 2019, 1:24 p.m.
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ...	Gergő Mihály Doma	May 24, 2019, 1:24 p.m.
Optional, drop this if you think it isn't important: buf[0] = '\0';	Gergő Mihály Doma	May 24, 2019, 1:24 p.m.
"A blank line should always be used ... After local variable declarations."	Gergő Mihály Doma	May 24, 2019, 1:24 p.m.
Type of first variable should be bool.	Gergő Mihály Doma	May 24, 2019, 1:24 p.m.
is_newer should be bool.	Gergő Mihály Doma	May 24, 2019, 1:24 p.m.
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ...	Gergő Mihály Doma	May 24, 2019, 1:24 p.m.
"A blank line should always be used ... After local variable declarations."	Gergő Mihály Doma	Nov. 13, 2019, 10:23 a.m.
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ...	Gergő Mihály Doma	May 24, 2019, 1:24 p.m.
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ...	Gergő Mihály Doma	May 24, 2019, 1:24 p.m.
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ...	Gergő Mihály Doma	May 24, 2019, 1:24 p.m.
Is it safe to expect that only two types of ZAP can exist? See the implementation of zap_lookup() in this ...	Gergő Mihály Doma	May 24, 2019, 1:24 p.m.
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ...	Gergő Mihály Doma	May 24, 2019, 1:24 p.m.
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ...	Gergő Mihály Doma	May 24, 2019, 1:24 p.m.
Is it safe to expect that only two types of ZAP can exist?	Gergő Mihály Doma	May 24, 2019, 1:24 p.m.
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ...	Gergő Mihály Doma	May 24, 2019, 1:24 p.m.
Possible buffer overrun: component array can hold up to 256 bytes, but zap_rlookup can write into it at most 65535 ...	Gergő Mihály Doma	May 24, 2019, 1:24 p.m.
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ...	Gergő Mihály Doma	May 24, 2019, 1:24 p.m.
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ...	Gergő Mihály Doma	May 24, 2019, 1:24 p.m.
Is it safe to expect that only two types of ZAP can exist?	Gergő Mihály Doma	May 24, 2019, 1:24 p.m.
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ...	Gergő Mihály Doma	May 24, 2019, 1:24 p.m.
Is it safe to expect that only two types of ZAP can exist?	Gergő Mihály Doma	May 24, 2019, 1:24 p.m.
"If one arm of an if-else statement contains braces, all arms should contain braces."	Gergő Mihály Doma	May 24, 2019, 1:24 p.m.
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ...	Gergő Mihály Doma	May 24, 2019, 1:24 p.m.
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ...	Gergő Mihály Doma	May 24, 2019, 1:24 p.m.

Ship it!

Ship It!

Ship it! Fix it!

All of my findings: some of them unrelated to c-style.

Toomas Soome Nov. 13, 2019, 10:21 a.m.

Thanks!

Note, I am keeping this list "open", I want to keep the initial cleanup very simple, just to make the pbchk happy etc, also I need to get few functional changes integrated first:D

I think you have spotted real bugs there too and we will need to address them with separate patches anyhow.

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

Unnecessary blank line.

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

"Never use the boolean negation operator (!) with non-boolean expressions."

Should be:
        if (memcmp(name, pairname, namelen) == 0 && type == pairtype) {

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

This if...else ladder can be rewritten as a switch...case statement, but is it worth doing?

Example:
            switch (type) {
            case DATA_TYPE_UINT64:
                xdr_uint64_t(&p, (uint64_t *)valuep);
                return (0);
                break;
            case DATA_TYPE_STRING: {
                int len;

                xdr_int(&p, &len);
                (*(const char **)valuep) = (const char *)p;
                return (0);
                break;
            }
            case DATA_TYPE_NVLIST:
            case DATA_TYPE_NVLIST_ARRAY:
                (*(const unsigned char **)valuep) =
                    (const unsigned char *)p;
                return (0);
                break;
            default:
                return (EIO);
                break;
            }

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

Déjà vu...
Type of found variable should be bool.

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Instead of casting, format constant can be used here.
            printf(" = 0x%" PRIx64 "\n", val);

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

Format string - argument type mismatch: type of j variable is unsigned int.

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

"Never use the boolean negation operator (!) with non-boolean expressions."

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

0 vs NULL.

This line should be:
    return (NULL);

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Yet another "malloc + memset => calloc" issue?

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

Type of is_new variable (and also the is_newer argument) should be bool.

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

switch instead of if...else ladder?

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

0 vs NULL.

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

0 vs NULL.

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

"Numerical constants should not be coded directly.
The #define feature of the C preprocessor should
be used to assign a meaningful name."

Example:

#include <sys/tem_impl.h>
.
.
.
    char line[TEM_DEFAULT_COLS];

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

"Numerical constants should not be coded directly.
The #define feature of the C preprocessor should
be used to assign a meaningful name."

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

Optional, drop this if you think it isn't important:
    buf[0] = '\0';

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

"A blank line should always be used ... After local variable declarations."

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

Type of first variable should be bool.

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

is_newer should be bool.

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

How about using format constans here instead of casting?

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Instead of casting, format constant can be used here.

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

"Numerical constants should not be coded directly.
The #define feature of the C preprocessor should
be used to assign a meaningful name."

Example:

#define ZFS_SECSIZE 512
.
.
.
    size = dnode->dn_datablkszsec * ZFS_SECSIZE;

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

"A blank line should always be used ... After local variable declarations."

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Unusual to see variable declarations almost in the middle of a function.

Toomas Soome Nov. 13, 2019, 11:41 a.m.

Yes, unfortunately this is new trend, we see it more and more with openzfs...

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

"Numerical constants should not be coded directly.
The #define feature of the C preprocessor should
be used to assign a meaningful name."

    size = dnode->dn_datablkszsec * ZFS_SECSIZE;

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

"Numerical constants should not be coded directly.
The #define feature of the C preprocessor should
be used to assign a meaningful name."
Example:

#define ZAP_LE_NAME_BUFSIZE 256
.
.
.
        char name[ZAP_LE_NAME_BUFSIZE], *p;

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Is it necessary to preserve this commented-out tracing code?
And if it is, how about creating a macro for this purpose?

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

"Numerical constants should not be coded directly.
The #define feature of the C preprocessor should
be used to assign a meaningful name."

    size_t size = dnode->dn_datablkszsec * ZFS_SECSIZE;

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

Is it safe to expect that only two types of ZAP can exist?

See the implementation of zap_lookup() in this file.

Also: Lustre feature ideas - ZFS TinyZAP.

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

"Numerical constants should not be coded directly.
The #define feature of the C preprocessor should
be used to assign a meaningful name."

    size = dnode->dn_datablkszsec * ZFS_SECSIZE;

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

"Numerical constants should not be coded directly.
The #define feature of the C preprocessor should
be used to assign a meaningful name."

    size_t size = dnode->dn_datablkszsec * ZFS_SECSIZE;

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

Is it safe to expect that only two types of ZAP can exist?

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

"Numerical constants should not be coded directly.
The #define feature of the C preprocessor should
be used to assign a meaningful name."

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Instead of casting, format constant can be used here.

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

Possible buffer overrun: component array can hold up to 256 bytes, but zap_rlookup can write into it at most 65535 bytes.

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

"Numerical constants should not be coded directly.
The #define feature of the C preprocessor should
be used to assign a meaningful name."

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Instead of casting, format constant can be used here.

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Instead of casting, format constant can be used here.

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Instead of casting, format constant can be used here.

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Instead of casting, format constant can be used here.

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Instead of casting, format constant can be used here.

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Instead of casting, format constant can be used here.

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

"Numerical constants should not be coded directly.
The #define feature of the C preprocessor should
be used to assign a meaningful name."

        child_dir_zap.dn_datablkszsec * ZFS_SECSIZE);

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

Is it safe to expect that only two types of ZAP can exist?

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Instead of casting, format constant can be used here.

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Instead of casting, format constant can be used here.

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

"Numerical constants should not be coded directly.
The #define feature of the C preprocessor should
be used to assign a meaningful name."

    size = dir.dn_datablkszsec * ZFS_SECSIZE);

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

Is it safe to expect that only two types of ZAP can exist?

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

"If one arm of an if-else statement contains braces, all arms should contain braces."

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

"Numerical constants should not be coded directly.
The #define feature of the C preprocessor should
be used to assign a meaningful name."

usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1)

Show all issues

"Numerical constants should not be coded directly.
The #define feature of the C preprocessor should
be used to assign a meaningful name."

Example:

#define BOOT_MAXLINK    10
.
.
.
            if (symlinks_followed > BOOT_MAXLINK) {

Commit:

-	ce9a6403abcc69576720e9de46bf44fa1676630d
+	9ac02fb709b9f0e6713b3f205540577e1f90b90e

Diff:

Revision 2 (+161 -141)

Show changes

usr/src/boot/lib/libstand/zfs/zfsimpl.c

Change Summary:

usr/src/boot/lib/libstand/zfs/zfsimpl.c: 1442: missing space after (void) cast
usr/src/boot/lib/libstand/zfs/zfsimpl.c: 1737: space after cast

Commit:

-	9ac02fb709b9f0e6713b3f205540577e1f90b90e
+	4e0870808d6fb48d32d7839d3fd280103587fb34

Diff:

Revision 3 (+157 -137)

Show changes

usr/src/boot/lib/libstand/zfs/zfsimpl.c

Note this one is integrated but I keep it visible for list of issues:)

You have a pending review.

Review Board 2.5.16

Screenshots

Files

Issues

Commit:

Diff:

Change Summary:

Commit:

Diff: