From 5e542f8706745fbbbd71acf1d857d86dc50ac328 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 19 Jun 2024 10:46:27 +0200 Subject: [PATCH 1/3] varlink: correctly format comments for enums too I apparently never tested comments on enum values and hence they didn#t work. Fix that. --- src/shared/varlink-idl.c | 99 +++++++++++++++++++++++++++++-------- src/test/test-varlink-idl.c | 6 +++ 2 files changed, 84 insertions(+), 21 deletions(-) diff --git a/src/shared/varlink-idl.c b/src/shared/varlink-idl.c index 78d719a4961..57765b18c29 100644 --- a/src/shared/varlink-idl.c +++ b/src/shared/varlink-idl.c @@ -74,28 +74,94 @@ static int varlink_idl_format_comment( return 0; } +static int varlink_idl_format_comment_fields( + FILE *f, + const VarlinkField *start, + size_t n, + const char *indent, + const char *const colors[static _COLOR_MAX], + size_t cols) { + + int r; + + if (n == 0) + return 0; + + assert(start); + + for (const VarlinkField *c = start; n > 0; c++, n--) { + r = varlink_idl_format_comment(f, ASSERT_PTR(c->name), indent, colors, cols); + if (r < 0) + return r; + } + + return 0; +} + +static const VarlinkField *varlink_idl_symbol_find_start_comment( + const VarlinkSymbol *symbol, + const VarlinkField *field) { + + assert(symbol); + assert(field); + assert(field >= symbol->fields); + + const VarlinkField *start = NULL; + + for (const VarlinkField *c1 = field; c1 > symbol->fields; c1--) { + const VarlinkField *c0 = c1 - 1; + + if (c0->field_type != _VARLINK_FIELD_COMMENT) + break; + + start = c0; + } + + return start; +} + static int varlink_idl_format_enum_values( FILE *f, const VarlinkSymbol *symbol, const char *indent, - const char *const colors[static _COLOR_MAX]) { + const char *const colors[static _COLOR_MAX], + size_t cols) { + _cleanup_free_ char *indent2 = NULL; bool first = true; + int r; assert(f); assert(symbol); assert(symbol->symbol_type == VARLINK_ENUM_TYPE); + indent2 = strjoin(strempty(indent), "\t"); + if (!indent2) + return -ENOMEM; + for (const VarlinkField *field = symbol->fields; field->field_type != _VARLINK_FIELD_TYPE_END_MARKER; field++) { + if (field->field_type == _VARLINK_FIELD_COMMENT) /* skip comments at first */ + continue; + if (first) { first = false; fputs("(\n", f); } else fputs(",\n", f); - fputs(strempty(indent), f); - fputs("\t", f); + /* We found an enum value we want to output. In this case, start by outputting all + * immediately preceding comments. For that find the first comment in the series before the + * enum value, so that we can start printing from there. */ + const VarlinkField *start_comment = varlink_idl_symbol_find_start_comment(symbol, field); + + if (start_comment) { + r = varlink_idl_format_comment_fields(f, start_comment, field - start_comment, indent2, colors, cols); + if (r < 0) + return r; + } + + fputs(indent2, f); fputs(colors[COLOR_IDENTIFIER], f); fputs(field->name, f); fputs(colors[COLOR_RESET], f); @@ -202,7 +268,7 @@ static int varlink_idl_format_field( return varlink_idl_format_all_fields(f, ASSERT_PTR(field->symbol), VARLINK_REGULAR, indent, colors, cols); case VARLINK_ENUM: - return varlink_idl_format_enum_values(f, ASSERT_PTR(field->symbol), indent, colors); + return varlink_idl_format_enum_values(f, ASSERT_PTR(field->symbol), indent, colors, cols); default: assert_not_reached(); @@ -245,24 +311,15 @@ static int varlink_idl_format_all_fields( } else fputs(",\n", f); - /* We found a field we want to output. In this case, output all immediately preceding - * comments first. First, find the first comment in the series before. */ - const VarlinkField *start_comment = NULL; - for (const VarlinkField *c1 = field; c1 > symbol->fields; c1--) { - const VarlinkField *c0 = c1 - 1; - - if (c0->field_type != _VARLINK_FIELD_COMMENT) - break; - - start_comment = c0; - } + /* We found a field we want to output. In this case, start by outputting all immediately + * preceding comments. For that find the first comment in the series before the field, so + * that we can start printing from there. */ + const VarlinkField *start_comment = varlink_idl_symbol_find_start_comment(symbol, field); if (start_comment) { - for (const VarlinkField *c = start_comment; c < field; c++) { - r = varlink_idl_format_comment(f, ASSERT_PTR(c->name), indent2, colors, cols); - if (r < 0) - return r; - } + r = varlink_idl_format_comment_fields(f, start_comment, field - start_comment, indent2, colors, cols); + if (r < 0) + return r; } r = varlink_idl_format_field(f, field, indent2, colors, cols); @@ -300,7 +357,7 @@ static int varlink_idl_format_symbol( fputs(symbol->name, f); fputs(colors[COLOR_RESET], f); - r = varlink_idl_format_enum_values(f, symbol, /* indent= */ NULL, colors); + r = varlink_idl_format_enum_values(f, symbol, /* indent= */ NULL, colors, cols); break; case VARLINK_STRUCT_TYPE: diff --git a/src/test/test-varlink-idl.c b/src/test/test-varlink-idl.c index f8c8080ee55..f14622bd581 100644 --- a/src/test/test-varlink-idl.c +++ b/src/test/test-varlink-idl.c @@ -27,12 +27,16 @@ static VARLINK_DEFINE_ENUM_TYPE( EnumTest, + VARLINK_FIELD_COMMENT("piff paff"), VARLINK_DEFINE_ENUM_VALUE(foo), + VARLINK_FIELD_COMMENT("waldo"), VARLINK_DEFINE_ENUM_VALUE(bar), + VARLINK_FIELD_COMMENT("crux"), VARLINK_DEFINE_ENUM_VALUE(baz)); static VARLINK_DEFINE_STRUCT_TYPE( NestedStructTest, + VARLINK_FIELD_COMMENT("miepf"), VARLINK_DEFINE_FIELD(x, VARLINK_INT, 0)); static VARLINK_DEFINE_STRUCT_TYPE( @@ -45,6 +49,8 @@ static VARLINK_DEFINE_STRUCT_TYPE( VARLINK_DEFINE_FIELD(bbbm, VARLINK_BOOL, VARLINK_MAP), VARLINK_DEFINE_FIELD(bbbnm, VARLINK_BOOL, VARLINK_NULLABLE|VARLINK_MAP), + VARLINK_FIELD_COMMENT("more from here"), + VARLINK_DEFINE_FIELD(iii, VARLINK_INT, 0), VARLINK_DEFINE_FIELD(iiin, VARLINK_INT, VARLINK_NULLABLE), VARLINK_DEFINE_FIELD(iiia, VARLINK_INT, VARLINK_ARRAY), From 9e10f3a7e800ad67be8d8b14ae158a27438814f0 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 19 Jun 2024 13:54:35 +0200 Subject: [PATCH 2/3] bootctl: normalize how we report no boot entries found This normalizes how we report an empty list of boot entries in ListBootEntries(). Our usual pattern is to return one item per method call, but when there is none we usually return a NoSuchXYZ error. Do so here too. Before this we'd return a null item instead here, and only here. This is a minor compat break, but given that this IPC interface is very new and probably not used so far (we don't use it in our code at least, and google doesn#t find any other use) I think this normalization is OK at this point. --- src/boot/bootctl-status.c | 8 +++++++- src/shared/varlink-io.systemd.BootControl.c | 6 +++++- test/units/TEST-74-AUX-UTILS.bootctl.sh | 2 +- 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/boot/bootctl-status.c b/src/boot/bootctl-status.c index 0b222073b4d..d2c7d50d12b 100644 --- a/src/boot/bootctl-status.c +++ b/src/boot/bootctl-status.c @@ -845,6 +845,9 @@ int vl_method_list_boot_entries(Varlink *link, sd_json_variant *parameters, Varl if (sd_json_variant_elements(parameters) > 0) return varlink_error_invalid_parameter(link, parameters); + if (!FLAGS_SET(flags, VARLINK_METHOD_MORE)) + return varlink_error(link, VARLINK_ERROR_EXPECTED_MORE, NULL); + r = acquire_esp(/* unprivileged_mode= */ false, /* graceful= */ false, /* ret_part= */ NULL, @@ -885,5 +888,8 @@ int vl_method_list_boot_entries(Varlink *link, sd_json_variant *parameters, Varl return r; } - return varlink_replybo(link, SD_JSON_BUILD_PAIR_CONDITION(!!previous, "entry", SD_JSON_BUILD_VARIANT(previous))); + if (!previous) + return varlink_error(link, "io.systemd.BootControl.NoSuchBootEntry", NULL); + + return varlink_replybo(link, SD_JSON_BUILD_PAIR_VARIANT("entry", previous)); } diff --git a/src/shared/varlink-io.systemd.BootControl.c b/src/shared/varlink-io.systemd.BootControl.c index 500e07243c7..2557384c6ad 100644 --- a/src/shared/varlink-io.systemd.BootControl.c +++ b/src/shared/varlink-io.systemd.BootControl.c @@ -48,6 +48,9 @@ static VARLINK_DEFINE_METHOD( static VARLINK_DEFINE_ERROR( RebootToFirmwareNotSupported); +static VARLINK_DEFINE_ERROR( + NoSuchBootEntry); + VARLINK_DEFINE_INTERFACE( io_systemd_BootControl, "io.systemd.BootControl", @@ -56,4 +59,5 @@ VARLINK_DEFINE_INTERFACE( &vl_method_ListBootEntries, &vl_method_SetRebootToFirmware, &vl_method_GetRebootToFirmware, - &vl_error_RebootToFirmwareNotSupported); + &vl_error_RebootToFirmwareNotSupported, + &vl_error_NoSuchBootEntry); diff --git a/test/units/TEST-74-AUX-UTILS.bootctl.sh b/test/units/TEST-74-AUX-UTILS.bootctl.sh index 78c0e6e5d9d..5331f5f8b40 100755 --- a/test/units/TEST-74-AUX-UTILS.bootctl.sh +++ b/test/units/TEST-74-AUX-UTILS.bootctl.sh @@ -264,7 +264,7 @@ EOF } testcase_bootctl_varlink() { - varlinkctl call --collect /run/systemd/io.systemd.BootControl io.systemd.BootControl.ListBootEntries '{}' + (varlinkctl call --collect /run/systemd/io.systemd.BootControl io.systemd.BootControl.ListBootEntries '{}' ||:) # We may have UEFI in the test environment. # If we don't have UEFI then we can test whether bootctl's varlink API fails cleanly. From 8710bbfe9696eba72c931f34265a708aac5df9c1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 19 Jun 2024 13:54:47 +0200 Subject: [PATCH 3/3] bootctl: add comments to Varlink interface This is mostly intended as test case for the early enum comment bugfix, as this Varlink IDL description now contains such comments, and test-varlink-idl will process it forth and back aleady. --- src/shared/varlink-io.systemd.BootControl.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/shared/varlink-io.systemd.BootControl.c b/src/shared/varlink-io.systemd.BootControl.c index 2557384c6ad..110d4b7be7f 100644 --- a/src/shared/varlink-io.systemd.BootControl.c +++ b/src/shared/varlink-io.systemd.BootControl.c @@ -4,14 +4,19 @@ static VARLINK_DEFINE_ENUM_TYPE( BootEntryType, + VARLINK_FIELD_COMMENT("Boot Loader Specification Type #1 entries (.conf files)"), VARLINK_DEFINE_ENUM_VALUE(type1), + VARLINK_FIELD_COMMENT("Boot Loader Specification Type #2 entries (UKIs)"), VARLINK_DEFINE_ENUM_VALUE(type2), + VARLINK_FIELD_COMMENT("Additional entries reported by boot loader"), VARLINK_DEFINE_ENUM_VALUE(loader), + VARLINK_FIELD_COMMENT("Automatically generated entries"), VARLINK_DEFINE_ENUM_VALUE(auto)); static VARLINK_DEFINE_STRUCT_TYPE( BootEntry, VARLINK_DEFINE_FIELD_BY_TYPE(type, BootEntryType, 0), + VARLINK_FIELD_COMMENT("The string identifier of the entry"), VARLINK_DEFINE_FIELD(id, VARLINK_STRING, VARLINK_NULLABLE), VARLINK_DEFINE_FIELD(path, VARLINK_STRING, VARLINK_NULLABLE), VARLINK_DEFINE_FIELD(root, VARLINK_STRING, VARLINK_NULLABLE), @@ -27,22 +32,30 @@ static VARLINK_DEFINE_STRUCT_TYPE( VARLINK_DEFINE_FIELD(initrd, VARLINK_STRING, VARLINK_NULLABLE|VARLINK_ARRAY), VARLINK_DEFINE_FIELD(devicetree, VARLINK_STRING, VARLINK_NULLABLE), VARLINK_DEFINE_FIELD(devicetreeOverlay, VARLINK_STRING, VARLINK_NULLABLE|VARLINK_ARRAY), + VARLINK_FIELD_COMMENT("Indicates whether the boot loader reported this entry on the current boot"), VARLINK_DEFINE_FIELD(isReported, VARLINK_BOOL, 0), + VARLINK_FIELD_COMMENT("Indicates the number of tries left for this boot entry before it is assumed to be not working."), VARLINK_DEFINE_FIELD(triesLeft, VARLINK_INT, VARLINK_NULLABLE), + VARLINK_FIELD_COMMENT("Indicates the number of unsuccessful tries already made for this boot entry."), VARLINK_DEFINE_FIELD(triesDone, VARLINK_INT, VARLINK_NULLABLE), + VARLINK_FIELD_COMMENT("Indicates whether this entry is the default entry."), VARLINK_DEFINE_FIELD(isDefault, VARLINK_BOOL, VARLINK_NULLABLE), + VARLINK_FIELD_COMMENT("Indicates whether this entry has been booted."), VARLINK_DEFINE_FIELD(isSelected, VARLINK_BOOL, VARLINK_NULLABLE)); static VARLINK_DEFINE_METHOD( ListBootEntries, + VARLINK_FIELD_COMMENT("A boot menu entry structure"), VARLINK_DEFINE_OUTPUT_BY_TYPE(entry, BootEntry, VARLINK_NULLABLE)); static VARLINK_DEFINE_METHOD( SetRebootToFirmware, + VARLINK_FIELD_COMMENT("The new value of the reboot-to-firmware-UI flag"), VARLINK_DEFINE_INPUT(state, VARLINK_BOOL, 0)); static VARLINK_DEFINE_METHOD( GetRebootToFirmware, + VARLINK_FIELD_COMMENT("The current state of the reboot-to-firmware-UI flag"), VARLINK_DEFINE_OUTPUT(state, VARLINK_BOOL, 0)); static VARLINK_DEFINE_ERROR( @@ -54,10 +67,18 @@ static VARLINK_DEFINE_ERROR( VARLINK_DEFINE_INTERFACE( io_systemd_BootControl, "io.systemd.BootControl", + VARLINK_INTERFACE_COMMENT("Boot Loader control APIs"), + VARLINK_SYMBOL_COMMENT("The type of a boot entry"), &vl_type_BootEntryType, + VARLINK_SYMBOL_COMMENT("A structure encapsulating a boot entry"), &vl_type_BootEntry, + VARLINK_SYMBOL_COMMENT("Enumerates boot entries. Method call must be called with 'more' flag set. Each response returns one entry. If no entries are defined returns the NoSuchBootEntry error."), &vl_method_ListBootEntries, + VARLINK_SYMBOL_COMMENT("Sets the reboot-to-firmware-UI flag of the firmware, if this concept exists. Returns the RebootToFirmwareNotSupported error if not."), &vl_method_SetRebootToFirmware, + VARLINK_SYMBOL_COMMENT("Gets the current state of the reboot-to-firmware-UI flag of the firmware, if this concept exists. Returns the RebootToFirmwareNotSupported error if not."), &vl_method_GetRebootToFirmware, + VARLINK_SYMBOL_COMMENT("SetRebootToFirmware() and GetRebootToFirmware() return this if the firmware does not actually support the reboot-to-firmware-UI concept."), &vl_error_RebootToFirmwareNotSupported, + VARLINK_SYMBOL_COMMENT("No boot entry defined."), &vl_error_NoSuchBootEntry);