11055 loader: zfs reader cstyle cleanup
Review Request #1852 — Created May 21, 2019 and updated
Information | |
---|---|
tsoome | |
illumos-gate | |
11055 | |
4e08708... | |
Reviewers | |
general | |
11055 loader: zfs reader cstyle cleanup
- 30
- 0
- 4
- 0
- 34
Description | From | Last Updated |
---|---|---|
Déjà vu... Type of found variable should be bool. |
|
|
Format string - argument type mismatch: type of j variable is unsigned int. |
|
|
"Never use the boolean negation operator (!) with non-boolean expressions." |
|
|
Type of is_new variable (and also the is_newer argument) should be bool. |
|
|
0 vs NULL. |
|
|
0 vs NULL. |
|
|
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ... |
|
|
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ... |
|
|
Optional, drop this if you think it isn't important: buf[0] = '\0'; |
|
|
"A blank line should always be used ... After local variable declarations." |
|
|
Type of first variable should be bool. |
|
|
is_newer should be bool. |
|
|
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ... |
|
|
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ... |
|
|
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ... |
|
|
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ... |
|
|
Is it safe to expect that only two types of ZAP can exist? See the implementation of zap_lookup() in this ... |
|
|
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ... |
|
|
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ... |
|
|
Is it safe to expect that only two types of ZAP can exist? |
|
|
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ... |
|
|
Possible buffer overrun: component array can hold up to 256 bytes, but zap_rlookup can write into it at most 65535 ... |
|
|
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ... |
|
|
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ... |
|
|
Is it safe to expect that only two types of ZAP can exist? |
|
|
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ... |
|
|
Is it safe to expect that only two types of ZAP can exist? |
|
|
"If one arm of an if-else statement contains braces, all arms should contain braces." |
|
|
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ... |
|
|
"Numerical constants should not be coded directly. The #define feature of the C preprocessor should be used to assign a ... |
|
-
All of my findings: some of them unrelated to c-style.
-
-
usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1) "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) Déjà vu...
Type offound
variable should bebool
. -
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) Format string - argument type mismatch: type of
j
variable isunsigned int
. -
usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1) "Never use the boolean negation operator (!) with non-boolean expressions."
-
usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1) 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) Type of
is_new
variable (and also theis_newer
argument) should bebool
. -
-
-
-
usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1) "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) "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) Optional, drop this if you think it isn't important:
buf[0] = '\0';
-
usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1) "A blank line should always be used ... After local variable declarations."
-
-
-
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) "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) "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.
-
usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1) "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) "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) "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) Is it safe to expect that only two types of ZAP can exist?
See the implementation of
zap_lookup()
in this file. -
usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1) "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) "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) Is it safe to expect that only two types of ZAP can exist?
-
usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1) "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) Possible buffer overrun:
component
array can hold up to 256 bytes, butzap_rlookup
can write into it at most 65535 bytes. -
usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1) "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) "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) 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) "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) Is it safe to expect that only two types of ZAP can exist?
-
usr/src/boot/lib/libstand/zfs/zfsimpl.c (Diff revision 1) "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) "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) "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: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+161 -141) |
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: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+157 -137) |