Reviewed By: allanjude

I wonder if it would make sense to try to stripe the trailing whitespace from the serial # etc. The json output looks a little silly:

Reviewed by: allanjude

Reviewed By: allanjude

In D22193#488245, @trasz wrote:

FWIW, grepping for that pattern only brings up the one hit Warner has already mentioned, and it's already under #ifdef illumos:

% grep -r '%ss%d' *
cddl/contrib/opensolaris/cmd/zpool/zpool_vdev.c:                (void) snprintf(buf, sizeof (buf), "%ss%d", path, slice);

I am fine with moving the text, just make sure you svn commit the correct files

In D22304#487863, @markj wrote:

Why don't we just have a "name" parameter that determines the entire gnop provider name?

it looks like you added conduct-reporting.html (the build artifact) not .xml (the source), and that it was not marked as a text file.

Maybe give Warner a chance to provide any more feedback before we commit this.

Attachment does seem to fit the bill best

Reviewed By: allanjude

In D22193#485070, @imp wrote:

/*
  * Fill in the devid, now that we've labeled the disk.
  */
 (void) snprintf(buf, sizeof (buf), "%ss%d", path, slice);
 if ((fd = open(buf, O_RDONLY)) < 0) {
         (void) fprintf(stderr,
             gettext("cannot open '%s': %s\n"),
             buf, strerror(errno));
         return (-1);
 }

is one example, this one is from cddl/contrib/opensolaris/cmd/zpool/zpool_vdev.c

Is the 'd' or 'c' in the description a typo? Should it be ada0d/s1? or ada0/s1

Reviewed By: allanjude

Committed as rS352504

Approved By: allanjude

Reviewed By: allanjude

Other than fixing the links, looks good to me.

We should make the -M flag be compile time dependant in WITH_STATS

Thank you very much for making this work.

Approved By: allanjude

Approved By: allanjude

Approved By: allanjude (mentor)

This makes sense. It is not clear how many upstream projects will adopt this license, so this seem like the right first step.

I might reverse the order of these, since loading the module is the recommended action, and compiling it into your kernel is for people who want to do custom kernels.

In D20477#443980, @cem wrote:

So like, what's the motivation of this change? What are the goals? How was this design chosen? There's 6000 lines of diff present as a fait accompli without any kind of socialization on arch@ or anywhere else. Code is great, but the high level stuff is missing.
Comment density is "low" and copy-paste density is "high." Did you consider templating this, either with awk or C++?
"stats(3)" is a bit misleading; 3900 LoC are a new kernel file. What is the expected consumer? What is the general API surface? Where is the design doc?
Where is the stats(9) manual page? What do stats(3) consumers look like? Will there ever be any in tree? Can this live in ports?

Approved By: allanjude (mentor)

@rgrimes If you want to commandeer this review and put an updated diff, that would be great.

This is a nice cleanup, thank you.

Send it