Move nvmecontrol to using linker sets

[imp]

Summary

More commands will be added to nvmecontrol. Also, there will be a few
more vendor commands (some of which may need to remain private to
companies writing them). The first step on that journey is to move to
using linker sets to dispatch commands. The next step will be using
dlopen to bring in the .so's that have the command that might need
to remain private for seamless integration.

Similar changes to this will be needed for vendor specific log pages.

Sponsored by: Netflix

[imp]

tweaks

[cem]

Mostly looks good to me. A few suggestions but nothing show-stopping other than the "Unknown command" thing. Thanks!

sbin/nvmecontrol/devlist.c

47 ↗

(On Diff #51484)

It seems a little odd to include the leading whitespace that (?)all subcommands will have, instead of doing it generally in whatever processes the linkerset for usage (I'm reading this top to bottom for now, haven't gotten to that part).

I guess it makes sense if you need multi-line usage strings for a complicated subcommand, and don't want to do something clever, like check for newlines in the generic usage routine.

(Edit after reading the rest of the patch: Especially given this is the status quo — we're just moving the strings around here — abstracting out the indentation is something that can be left for a future cleanup patch.)

sbin/nvmecontrol/nvmecontrol.c

58 ↗

(On Diff #51484)

If we wanted to be more clever about indentation, it would go somewhere around here.

76–82 ↗

(On Diff #51484)

We should probably return after finding a match to avoid printing "Unknown command" when we found a command

sbin/nvmecontrol/nvmecontrol.h

37 ↗

(On Diff #51484)

It looks like this is unused for now, aside from NVME_COMMAND directly below. Given that, is there any reason to define it instead of just using DATA_SET directly in NVME_COMMAND?

40 ↗

(On Diff #51484)

name ## _nvme_cmd makes more sense to me, but function isn't very objectionable to me, if you prefer to keep it as-is.

41 ↗

(On Diff #51484)

I'd prefer the C99 designated initializer form:

{
  .name = #name,
  .fn = function,
  .usage = usage,
}

(That might require changing the macro parameter name to something else, but that's ok.)

63–64 ↗

(On Diff #51484)

I'd suggest const nvme_function * const * for the pointers, given we shouldn't mutate the pointer table nor the objects as part of dispatch.

sbin/nvmecontrol/wdc.c

57 ↗

(On Diff #51484)

(The "\t" here doesn't match the style of other usage indentation.)

[imp]

Thanks for the feedback. I'll see how much I can include in my next round as some of them are asking for more structural changes beyond the scope of this set of changes (the suggestions are good, btw, just am keen to avoid too much scope creep).

sbin/nvmecontrol/devlist.c

47 ↗

(On Diff #51484)

Correct, this is just code movement.

sbin/nvmecontrol/nvmecontrol.c

58 ↗

(On Diff #51484)

Right, it's kinda hard given the continuation stuff that's there now. It would require some careful thought.

76–82 ↗

(On Diff #51484)

Today, all those functions exit, so I'll consider this for a future cleanup.

sbin/nvmecontrol/nvmecontrol.h

37 ↗

(On Diff #51484)

(a) it's logical and (b) I may tweak the set name when I introduce the logfile sets.

40 ↗

(On Diff #51484)

name is the user command, function is the function we call. There are some user commands with hyphens, so I need two parameters.

41 ↗

(On Diff #51484)

Meh. I prefer shorter to longer... I'm not sure this adds any value, but I'll think about it.

63–64 ↗

(On Diff #51484)

const foo * const * is a pain in the ass to use. And I think that the set marcros make that a pain to use since they don't have enough consts sprinkled in. So I'll give it a shot, but punt if the PITA factor pops up.

sbin/nvmecontrol/wdc.c

57 ↗

(On Diff #51484)

I'll do a separate fix and fix that when I do the indentation fix. This is just code movement.

[cem]

In D18403#391655, @imp wrote:

Thanks for the feedback. I'll see how much I can include in my next round as some of them are asking for more structural changes beyond the scope of this set of changes (the suggestions are good, btw, just am keen to avoid too much scope creep).

No problem. Yeah, I totally understand keeping the scope narrow.

sbin/nvmecontrol/nvmecontrol.c

76–82 ↗

(On Diff #51484)

Oh, sure, this isn't a regression in this change.

sbin/nvmecontrol/nvmecontrol.h

40 ↗

(On Diff #51484)

Ah, I understood the distinction between name and function, but hadn't noticed the names that included characters not valid in a C symbol yet when I made this comment. function ## is the only option, then.

41 ↗

(On Diff #51484)

The C99 style makes it harder for someone to mess up construction by inserting or reordering struct members. (Alternatively, it makes it easier to insert or reorder struct members because you don't have to modify the initializer.)

Admittedly, that is unlikely to happen for nvmecommand due to the limited number of authors, limited scope of the tool, initializer definition being quite close to that of the struct, and type incompatibility.

It is probably more intuitive for big structures that change over time and are initialized far away from their definition (e.g., a lot of common kernel objects). I use it universally as a "good habit." But that's definitely a personal preference and there's nothing wrong with keeping it short and simple in this instance.

63–64 ↗

(On Diff #51484)

The pointer type shouldn't be painful to use here, assuming assignment from the non-const SET macros doesn't produce some obnoxious error/werror. I'm ok with punting if the PITA factor appears.

I don't know if the SET macros will kill it or not. In a perfect world, maybe we would have const-typed versions of them in linker_sets.h. Adding those is, of course, very out-of-scope for this change. :-)

sbin/nvmecontrol/wdc.c

57 ↗

(On Diff #51484)

Understood. This line isn't even moving ;-). I feel like I'm channeling bde here, sorry.

[imp]

Lots of cleanup, will be separate commits

• Make logpage functions a linker set.
• Move common logpage routines into nvmecontrol.h
• Move the hgst/wdc log page printing code into wdc.c
• Return after we find the dispatched function.
• usage cleanup pt 1
• Usage cleanup pt 2
• Move Intel specific log pages to intel.c
• Delete the undocumented alias 'wds'.

[cem]

The new changes LGTM. The scope of this review is getting broader than I'd prefer, though. It'd be easier on me as a reviewer if any future scope increases went into a new review. (I understand that's more work for you as long as this portion is in flux, but maybe that's motivation for getting this batch committed first? :-))

(I didn't look too closely at the moved code; I assume you can operate copy/paste just fine.) Some of these comments are on moved code, so definitely feel free to punt.

This request is definitely out of scope for this review, but it would be great if the logic was eventually separated out from the print formatting, in a libnvmecontrol or something like that. (Without that, I anticipate someone eventually attempting to apply libxo to this, and I really don't like what libxo does to programs. With the logic in a library, this hypothetical person can have create an independent tool that dumps out machine-parseable output without messing up the the existing print formatting.)

Thanks!

Edit: FYI, arc diff has a --nolint flag that disables the stupid spellchecker.

sbin/nvmecontrol/logpage.c

319 ↗

(On Diff #51489)

"health" ?

437 ↗

(On Diff #51489)

"but"

sbin/nvmecontrol/nvmecontrol.c

53 ↗

(On Diff #51489)

Insert my usual request for const pointers :-)

66 ↗

(On Diff #51489)

Should this one be one character shorter than the one above to accommodate the leading space on trailing lines?

88 ↗

(On Diff #51489)

ditto

107 ↗

(On Diff #51489)

Nice catch. Sorry I missed it earlier.

sbin/nvmecontrol/nvmecontrol.h

38 ↗

(On Diff #51489)

These ones can be const struct nvme_function * without too much PITA, right?

52 ↗

(On Diff #51489)

Is const void *buf reasonable?

71 ↗

(On Diff #51489)

Maybe leave off the trailing semi-colon; the invocations above already end in a semi-colon anyway (and it's typical for macros).

74–75 ↗

(On Diff #51489)

style(9) has the struct name and left brace on one line

sbin/nvmecontrol/wdc.c

199 ↗

(On Diff #51489)

can buf be a const pointer?

201–202 ↗

(On Diff #51489)

ditto

[imp]

I've updated to be close to the end game. All that remains is readdir(/libexec/nvmecontrol and /usr/local/libexec/nvmecontrol) for all the .so's at startup and we'll be to the point where Netflix can deploy the Sekret Soss modules we have from Vendor X w/o changing base or polluting base with NDA material.

sbin/nvmecontrol/nvmecontrol.h

63–64 ↗

(On Diff #51484)

It is painful, so I'm not going to do it. I tried a few different variations and punted.

[imp]

I'll do the ones I marked and call it good and do round 2 for the const changes and the dlopen feature.

sbin/nvmecontrol/logpage.c

319 ↗

(On Diff #51489)

yea, doesn't matter since it just needs to be unique, but hobgoblins and all

sbin/nvmecontrol/nvmecontrol.c

53 ↗

(On Diff #51489)

I'll do that.

66 ↗

(On Diff #51489)

No, things are lined up so it doesn't need that.

sbin/nvmecontrol/nvmecontrol.h

52 ↗

(On Diff #51489)

Not in this change, but maybe in the future.

71 ↗

(On Diff #51489)

True ;; is harmless, but I'll fix.

74–75 ↗

(On Diff #51489)

true, I'll fix this, even though that's supposed to just be code movement.