OS-7869 want kstat mdb module

[joyent-automation]

OS-7869 want kstat mdb module

This PR was migrated-from-gerrit, https://cr.joyent.us/#/c/6523/.
The raw archive of this CR is here.
See MANTA-4594 for info on Joyent Eng's migration from Gerrit.

CR discussion

@KodyKantor commented at 2019-06-28T19:48:54

Uploaded patch set 2: Commit message was updated.

@rmustacc commented at 2019-06-28T20:43:59

Patch Set 3:

(18 comments)

Thanks for putting this together, it looks like it'll be pretty useful.

Patch Set 3 code comments

usr/src/cmd/mdb/common/modules/kstat/Makefile.kstat#31 @rmustacc

Is this really necessary now? Isn't lint gone? I suspect you don't need this file at all.

usr/src/cmd/mdb/common/modules/kstat/Makefile.kstat#31 @KodyKantor

Perfect!

usr/src/cmd/mdb/common/modules/kstat/kstat.c#2 @rmustacc

Please use the new CCDL header in usr/src/prototypes/prototype.c.

usr/src/cmd/mdb/common/modules/kstat/kstat.c#2 @KodyKantor

Nice. TIL new file prototypes! Thanks.

usr/src/cmd/mdb/common/modules/kstat/kstat.c#21 @rmustacc

Minor nit, but conventionally there's a blank line between the two blocks.

usr/src/cmd/mdb/common/modules/kstat/kstat.c#30 @rmustacc

Any reason there isn't a typedef for this?

usr/src/cmd/mdb/common/modules/kstat/kstat.c#34 @rmustacc

This is unused. Did you intend to use this with the mdb_ekstat_t so that way we weren't embedding the full kstat structure of the current implementation below? mdb_ctf_vread can go on for nested members, so if all we care about are those members plus ks_class, then there's no reason to use embed the exact structure size and implementation of the kstat_t in the dmod.

usr/src/cmd/mdb/common/modules/kstat/kstat.c#34 @KodyKantor

Ah. I assumed that the CTF functionality needed this to operate properly. It makes sense that we don't though. The way that this is written doesn't really stand to reason anyway.

usr/src/cmd/mdb/common/modules/kstat/kstat.c#34 @rmustacc

Well, there's still value to having something like this here. As I pointed out, right now you're embedding the structure and size of the ekstat_t and by implication the kstat_t that is used in the kernel. If this were to change for whatever reason, it would break this module if you were debugging an older/newer dump. If you do have nested types that you define with something like this, then you'll be able to survive that.

usr/src/cmd/mdb/common/modules/kstat/kstat.c#54 @rmustacc

This warning gag for lint isn't required any more.

usr/src/cmd/mdb/common/modules/kstat/kstat.c#92 @rmustacc

Minor, but illumos C style asks for this to be on the previous line.

usr/src/cmd/mdb/common/modules/kstat/kstat.c#94 @rmustacc

I feel like we should probably have a warning message printed here for the cases where we can't read it and indicate that. Otherwise someone might just get no output.

usr/src/cmd/mdb/common/modules/kstat/kstat.c#110 @rmustacc

Since these are all using a strncmp, should we make sure that the user's requested items aren't too long here?

usr/src/cmd/mdb/common/modules/kstat/kstat.c#140 @rmustacc

Since we know this symbol is supposed to be in the unix module, should we actually ask it to search everything versus just the one that we want?

usr/src/cmd/mdb/common/modules/kstat/kstat.c#140 @KodyKantor

Ah, nice. I'm sure this is a basic question, but is there a way to determine which module a symbol belongs to?

Since you mentioned the unix module I was able to find it by running '::nm unix ! grep kstat_avl_bykid', but that doesn't seem optimal.

usr/src/cmd/mdb/common/modules/kstat/kstat.c#140 @johnlevon

kstat_avl_bykid::nm -f obj

usr/src/cmd/mdb/common/modules/kstat/kstat.c#141 @rmustacc

You don't want the \n here. mdb_warn uses the presence of the \n to indicate whether or not it should concatenate the error message that was found (kind of like warn(3C) adds errno).

usr/src/cmd/mdb/common/modules/kstat/kstat.c#148 @rmustacc

I'd nix the\n so you get the underlying error here as well.

usr/src/cmd/mdb/common/modules/kstat/kstat.c#162 @rmustacc

I'm not sure if this comment is necessary. But if you find it helpful, no reason to get rid of it.

usr/src/cmd/mdb/common/modules/kstat/kstat.c#169 @rmustacc

Is there any particular reason you don't have a filter based on the instance like the kstat command line tool?

usr/src/cmd/mdb/common/modules/kstat/kstat.c#169 @KodyKantor

I've personally never found it particularly useful, but we should include it to maintain consistency with the CLI tool. Let me know what you think of the argument parsing I added for this in the latest patch set. It feels clunky to me.

usr/src/cmd/mdb/common/modules/kstat/kstat.c#174 @rmustacc

So conventionally just the synopsis of options and arguments should be here. The dcmd structure has an optional usage method. An example of this is prtconf_help(). So you should break this up into just a synopsis and the actual help message. That way when someone runs ::help kstat they'll get all this detailed information you've written up.

usr/src/cmd/mdb/common/modules/kstat/kstat.c#174 @KodyKantor

We had a separate help function for this in an earlier iteration. I was following the zfs '::help spa' dcmd's lead here.

usr/src/cmd/mdb/common/modules/kstat/kstat.c#174 @rmustacc

OK, I understand where you're coming from. I'm not sure that's the right lead. Especially as some of the feedback I've had is around discoverability, so I think going through and adding more explanations and examples (ala ::help typedef) would be useful. So thanks for adding this as a separate function.

usr/src/cmd/mdb/intel/amd64/kstat/Makefile#1 @rmustacc

Please use the new prototype file in usr/src/prototypes/prototype.Makefile for the CDDL header.

usr/src/cmd/mdb/intel/amd64/kstat/Makefile#40 @rmustacc

Why is this required? Is there a reason we can't actually fix the warning that this is indicating?

usr/src/cmd/mdb/sparc/v9/kstat/Makefile#1 @rmustacc

Please see the notes in the x86 version of this.

@KodyKantor commented at 2019-07-01T16:07:55

Patch Set 3:

(6 comments)

Thanks for taking a look, Robert! Let me know what you think of the latest changes.

@johnlevon commented at 2019-07-01T16:28:36

Patch Set 3:

(1 comment)

@johnlevon commented at 2019-07-01T17:26:39

Patch Set 4:

(8 comments)

Out of interest, are you planning to add some actual ks_data digging later?

Patch Set 4 code comments

usr/src/cmd/mdb/common/modules/kstat/kstat.c#21 @johnlevon

nit, shouldn't this be mdb_kstat_flags... and MDB_KSTAT... ?
Or just drop it, since there's only one flag?

usr/src/cmd/mdb/common/modules/kstat/kstat.c#21 @KodyKantor

Sure, I'll namespace these with mdb/MDB.

I was planning to use this for other flags in the near future, so I'd like to keep this where it is if that's agreeable.

usr/src/cmd/mdb/common/modules/kstat/kstat.c#42 @johnlevon

Can we make all these const chars?

usr/src/cmd/mdb/common/modules/kstat/kstat.c#57 @johnlevon

We already report the failure (and the errno) as part of the walk, I don't think this line is necessary.

usr/src/cmd/mdb/common/modules/kstat/kstat.c#69 @johnlevon

You need a NULL terminator at the end of these options

usr/src/cmd/mdb/common/modules/kstat/kstat.c#77 @johnlevon

They must be < KSTAT_STRLEN:

62 #define KSTAT_STRLEN 31 /* 30 chars + NULL; must be 16 * n - 1 */

So we can only fit 30 chars.

usr/src/cmd/mdb/common/modules/kstat/kstat.c#82 @johnlevon

Are you missing an INT version of MDB_OPT_UINTPTR_SET? If so, I'd prefer you to use that, then cast to int as needed. (Or, if you like, add OPT_INT_SET).

usr/src/cmd/mdb/common/modules/kstat/kstat.c#82 @KodyKantor

Yes, for this specific case a signed getopt (with a bool so we don't need the magic number thing) would be ideal.

I added MDB_OPT_INT_SET in the latest patch set. Let me know what you think of that. If that looks good I'll open a separate ticket (and add it to this commit message) so the change is recorded.

usr/src/cmd/mdb/common/modules/kstat/kstat.c#163 @johnlevon

Would this be slightly more friendly if we used kstat_avl_byname ?

usr/src/cmd/mdb/common/modules/kstat/kstat.c#163 @KodyKantor

It appears so. Using _byname appears to sort alphabetically first by module, then by name.

usr/src/cmd/mdb/sparc/v9/kstat/Makefile#25 @johnlevon

I think this is dangling

usr/src/cmd/mdb/sparc/v9/kstat/Makefile#25 @KodyKantor

It is! Do we have a way to verify sparc builds?

usr/src/cmd/mdb/sparc/v9/kstat/Makefile#25 @johnlevon

Essentially no. We just wait for Peter Tribble to fix it up for us.

@KodyKantor commented at 2019-07-02T16:23:53

Patch Set 5:

(4 comments)

Thanks, John! Yes, I'm planning to add 'addr::kstat_named' or something similar in a later commit. If you have something specific in mind as far as verbiage or output formatting, let me know.

@johnlevon commented at 2019-07-02T16:35:07

Patch Set 5: Code-Review+1

(2 comments)

@rmustacc commented at 2019-07-02T16:43:32

Patch Set 5:

(6 comments)

Patch Set 5 code comments

usr/src/cmd/mdb/common/mdb/mdb_argvec.c#233 @rmustacc

I realize that mdb doesn't have a signed version of this, but is strict integer truncation what we want here? If someone passes something outside of the range of an integer, shouldn't we instead warn about that and error rather that continue processing it? I think this would lead to really confusing behavior. For example, this means that if I pass UINT64_MAX or INT64_MAX, I get -1. If I pass (1 << 33) + 5, I'll get 5. I think we should probably be rigorous here. It may even be worth having an signed mdb conversion since our goal is a signed value.

usr/src/cmd/mdb/common/mdb/mdb_argvec.h#48 @johnlevon

I suppose this is private to MDB instead of a module API, so this is OK.

usr/src/cmd/mdb/common/mdb/mdb_modapi.h#252 @rmustacc

I noticed that we're changing this but not revving the module API in any way. Have you gone through and figured out what will happen with newer modules at different versions of the module API and how that will interact with them using this value and the fact that older MDB will think it's OK for that to be loaded and run?

Also, do we really want this to be an arbitrary sized integer? That means we'll have to change its actual size to match the ABI. Maybe this should be an int32_t if that's actually what you want?

usr/src/cmd/mdb/common/modules/kstat/kstat.c#75 @rmustacc

Can we spell out the error message please? Also, if we're going to constrain it, I'd recommend that we include in parens what the actual value is. So, for example:

"provided module, class, and name lengths must be less than KSTAT_STRLEN (%u)\n"

usr/src/cmd/mdb/common/modules/kstat/kstat.c#124 @rmustacc

Can we add an examples section like a manual page?