OS-5074 want nvlist_print_json_pretty

[joyent-automation]

OS-5074 want nvlist_print_json_pretty

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

CR discussion

@bahamas10 commented at 2017-10-05T17:06:22

Patch Set 1:

New commits:
commit 9ea869da475ca76c78ef7b601a8d8b97deca1d5b
initial commit

@jclulow commented at 2017-10-05T17:58:15

Patch Set 1:

(10 comments)

@bahamas10 commented at 2017-10-05T18:45:01

Patch Set 1:

(6 comments)

Thanks for the review!

Patch Set 1 code comments

usr/src/lib/libnvpair/libnvpair.h#23 @jclulow

as per eng.git:

Copyright (c) 2017, Joyent, Inc.

usr/src/lib/libnvpair/libnvpair.h#23 @bahamas10

Ah great. I'll use this format for all of my changes going forward, thanks.

usr/src/lib/libnvpair/mapfile-vers#24 @jclulow

as per eng.git:

Copyright (c) 2017, Joyent, Inc.

usr/src/lib/libnvpair/nvpair_json.c#12 @jclulow

as per eng.git:

Copyright (c) 2017, Joyent, Inc.

usr/src/lib/libnvpair/nvpair_json.c#32 @jclulow

I think this might be better spelt "FPRINTF_JSON_ARRAY()".

usr/src/lib/libnvpair/nvpair_json.c#32 @bahamas10

Agreed.

usr/src/lib/libnvpair/nvpair_json.c#42 @jclulow

I think it would be best to pass in "level", rather than leak it in from the containing scope?

usr/src/lib/libnvpair/nvpair_json.c#42 @bahamas10

Ah, good catch. I agree - this was something I must have overlooked.

usr/src/lib/libnvpair/nvpair_json.c#315 @jclulow

Why did you restructure this to use the intermediate function rather than use the FPRINTF() macro directly?

usr/src/lib/libnvpair/nvpair_json.c#315 @bahamas10

For DATA_TYPE_STRING specifically, it is already using nvlist_print_json_string - I haven't changed that.

However, for the other types below (such as DATA_TYPE_BOOLEAN directly below this case) I've refactored it to use nvlist_print_json_boolean_value. My thinking was that the same function used to print a single value would be used to print the values inside an array. That way, anytime a (for example) string is printed, it will always use nvlist_print_json_string, or anytime a byte is printed, it will always use nvlist_print_json_byte, etc. regardless of if the value was printed as a singular value, or as part of an array of values.

usr/src/test/util-tests/tests/libnvpair_json/json_09_pretty.ksh#14 @jclulow

Should match "eng.git" template; i.e.

Copyright (c) 2017, Joyent, Inc.

usr/src/test/util-tests/tests/libnvpair_json/json_09_pretty.ksh#24 @jclulow

I think this was specifically for "json_05_strings.ksh", and shouldn't be here.

usr/src/test/util-tests/tests/libnvpair_json/json_09_pretty.ksh#24 @bahamas10

Done

usr/src/test/util-tests/tests/libnvpair_json/print_json.c#13 @jclulow

as per eng.git:

Copyright (c) 2017, Joyent, Inc.

usr/src/test/util-tests/tests/libnvpair_json/print_json.c#830 @jclulow

Please put braces around the arms of an "if" statement with a multi-line expression; i.e.,

if (nvlist.... ||
    fprintf(stdout, ...) {
        goto out;
}

usr/src/test/util-tests/tests/libnvpair_json/print_json.c#830 @bahamas10

Done

@bahamas10 commented at 2017-10-05T18:45:25

Patch Set 2:

New commits:
commit 9550390c8026dac698fd3e917bfa0c8b02796d9a
changes based on joshc review

@bahamas10 commented at 2017-10-11T15:53:33

Patch Set 2:

Parking this CR for now until a functional consumer is in place for this interface.