- Download Diff

Summary:

10182 dd: print scaled stats

Review Request #1344 — Created Jan. 6, 2019 and submitted Jan. 10, 2019, 6:56 p.m.

Information
Owner:	tsoome
Repository:	illumos-gate
Branch:
Bugs:	10182
Depends On:
Commit:	00a54ba...
Reviewers
Groups:	general
People:

Description

10182 dd: print scaled stats

Testing Done

Issues

0
0
26
6
All Issues: 32

Description	From	Last Updated
Please, remove the blank before the opening square bracket.	domag02	Jan. 10, 2019, 6:58 a.m.
Same.	domag02	Jan. 10, 2019, 6:58 a.m.
You added missing spaces around & in other cases, so please add it here too.	marcel	Jan. 7, 2019, 2:50 p.m.
You added missing spaces around & in other cases, so please add it here too.	marcel	Jan. 7, 2019, 2:50 p.m.
This looks like an indentation issue.	marcel	Jan. 7, 2019, 2:51 p.m.
Is indentation okay here?	marcel	Jan. 7, 2019, 2:52 p.m.
LGTM, only small nitpicks to be consistent with the style: put a space before obytes remove space between sizeof and ...	xenol	Jan. 7, 2019, 6:49 a.m.
Missing blanks around the operator.	domag02	Jan. 7, 2019, 8:59 p.m.
Same.	domag02	Jan. 7, 2019, 9 p.m.
Same.	domag02	Jan. 7, 2019, 9 p.m.
Same.	domag02	Jan. 7, 2019, 9 p.m.
Same.	domag02	Jan. 7, 2019, 9 p.m.
Same.	domag02	Jan. 7, 2019, 9 p.m.
Same.	domag02	Jan. 7, 2019, 9 p.m.
Same.	domag02	Jan. 7, 2019, 9 p.m.
Same. . . .	domag02	Jan. 7, 2019, 9:02 p.m.
... dup(STDIN_FILENO);	domag02	Jan. 10, 2019, 6:59 a.m.
... dup(STDOUT_FILENO);	domag02	Jan. 10, 2019, 6:59 a.m.
Same.	domag02	Jan. 7, 2019, 9:03 p.m.
Same.	domag02	Jan. 7, 2019, 9:03 p.m.
Same.	domag02	Jan. 7, 2019, 9:04 p.m.
No need to cast NULL to unsigned char *.	domag02	Jan. 7, 2019, 8:56 p.m.
Same.	domag02	Jan. 7, 2019, 9:05 p.m.
Same.	domag02	Jan. 7, 2019, 9:05 p.m.
if (obc) {	domag02	Jan. 7, 2019, 8:57 p.m.
} else {	domag02	Jan. 7, 2019, 8:57 p.m.
Pointless to use BIG here (as a limiting number), if the returned value is casted to unsigned int. #include <limits.h> ...	domag02	Jan. 8, 2019, 1:11 p.m.
Same.	domag02	Jan. 8, 2019, 1:11 p.m.
Same.	domag02	Jan. 8, 2019, 1:11 p.m.
Same.	domag02	Jan. 8, 2019, 1:11 p.m.
Almost the same, but now with INT_MAX.	domag02	Jan. 8, 2019, 1:11 p.m.
..., obytes	domag02	Jan. 10, 2019, 7 a.m.

Ship it!

```
Ship It!
```

usr/src/cmd/dd/dd.c (Diff revision 1)

Show all issues

LGTM, only small nitpicks to be consistent with the style:

put a space before obytes
remove space between sizeof and the following bracket

tsoome Jan. 7, 2019, 6:49 a.m.

This would be against cstyle rules:
C style:
dd.c: 1796: missing space between keyword and paren
dd.c: 1796: space after cast

The sizeof() is particulary nasty because thats only difference with cstyle in FreeBSD:)

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

usr/src/cmd/dd/dd.c (Diff revision 1)

Show all issues

You added missing spaces around & in other cases, so please add it here too.

usr/src/cmd/dd/dd.c (Diff revision 1)

Show all issues

You added missing spaces around & in other cases, so please add it here too.

usr/src/cmd/dd/dd.c (Diff revision 1)
Show all issues
```
This looks like an indentation issue.
```
usr/src/cmd/dd/dd.c (Diff revision 1)
Show all issues
```
Is indentation okay here?
```

Commit:

-	1482325fb16079cffd7139d685e6e2c9b3c1cbc9
+	33d10717ae3226b2be4b5902c0af0da2c5331180

Diff:

Revision 2 (+199 -351)

Show changes

	usr/src/cmd/dd/Makefile
	usr/src/cmd/dd/dd.c

usr/src/cmd/dd/dd.c (Diff revision 2)
Show all issues
```
Missing blanks around the operator.
```
usr/src/cmd/dd/dd.c (Diff revision 2)
Show all issues
```
Same.
```
usr/src/cmd/dd/dd.c (Diff revision 2)
Show all issues
```
Same.
```
usr/src/cmd/dd/dd.c (Diff revision 2)
Show all issues
```
Same.
```
usr/src/cmd/dd/dd.c (Diff revision 2)
Show all issues
```
Same.
```
usr/src/cmd/dd/dd.c (Diff revision 2)
Show all issues
```
Same.
```
usr/src/cmd/dd/dd.c (Diff revision 2)
Show all issues
```
Same.
```
usr/src/cmd/dd/dd.c (Diff revision 2)
Show all issues
```
Same.
```
usr/src/cmd/dd/dd.c (Diff revision 2)
Show all issues
```
Same.
.
.
.
```
usr/src/cmd/dd/dd.c (Diff revision 2)
Show all issues
```
Same.
```
usr/src/cmd/dd/dd.c (Diff revision 2)
Show all issues
```
Same.
```
usr/src/cmd/dd/dd.c (Diff revision 2)
Show all issues
```
Same.
```
usr/src/cmd/dd/dd.c (Diff revision 2)
Show all issues
```
Same.
```
usr/src/cmd/dd/dd.c (Diff revision 2)
Show all issues
```
Same.
```
usr/src/cmd/dd/dd.c (Diff revision 2)
Show all issues
```
if (obc) {
```
usr/src/cmd/dd/dd.c (Diff revision 2)
Show all issues
```
} else {
```

usr/src/cmd/dd/dd.c (Diff revision 2)

Show all issues

No need to cast NULL to unsigned char *.

Commit:

-	33d10717ae3226b2be4b5902c0af0da2c5331180
+	3a3557e00225c644d8bb640987069458f6a1b077

Diff:

Revision 3 (+226 -379)

Show changes

	usr/src/cmd/dd/Makefile
	usr/src/cmd/dd/dd.c

tsoome Jan. 8, 2019, 6:59 a.m.

Those are all fair remarks, but are going out of scope of this change. To be honest, we should also post cstyle fixes separate from the original bug to keep changes separate. I think it is time for you to take next step from reviewing things - file the issue, create patch and post it for review. If you need any help, just ask, I'll be very glad to help out:)

domag02 Jan. 10, 2019, 8:13 a.m.

Thanks, but I don't have a working illumos installation now, so I can't make the necessary build before sending patches to RTI.

usr/src/cmd/dd/dd.c (Diff revisions 1 - 2)

Show all issues

Please, remove the blank before the opening square bracket.

usr/src/cmd/dd/dd.c (Diff revisions 1 - 2)
Show all issues
```
Same.
```

```
Magic numbers.
```

usr/src/cmd/dd/dd.c (Diff revisions 2 - 3)

Show all issues


... dup(STDIN_FILENO);

usr/src/cmd/dd/dd.c (Diff revisions 2 - 3)

Show all issues


... dup(STDOUT_FILENO);

usr/src/cmd/dd/dd.c (Diff revision 3)
Show all issues
```
..., obytes
```

domag02 Jan. 8, 2019, 1:07 p.m.

Not for this issue.
Fix argument handling.

usr/src/cmd/dd/dd.c (Diff revision 3)

Show all issues

Pointless to use BIG here (as a limiting number), if the returned value is casted to unsigned int.

#include <limits.h>
.
.
.
... = (unsigned)number(UINT_MAX);

usr/src/cmd/dd/dd.c (Diff revision 3)
Show all issues
```
Same.
```
usr/src/cmd/dd/dd.c (Diff revision 3)
Show all issues
```
Same.
```
usr/src/cmd/dd/dd.c (Diff revision 3)
Show all issues
```
Same.
```
usr/src/cmd/dd/dd.c (Diff revision 3)
Show all issues
```
Almost the same, but now with INT_MAX.
```

Commit:

-	3a3557e00225c644d8bb640987069458f6a1b077
+	00a54ba47e46cd74e1bc8eba510aef454a38f194

Diff:

Revision 4 (+234 -387)

Show changes

	usr/src/cmd/dd/Makefile
	usr/src/cmd/dd/dd.c

Ship it!

```
Ship It!
```

You have a pending review.

Review Board 3.0.18

10182 dd: print scaled stats

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Status: Closed (submitted)