From 9632f8b465d72c3405fb0d575fab4b953300b88a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 26 Apr 2024 17:40:54 +0200 Subject: [PATCH 1/6] utf8: export utf8_char_console_width() --- src/basic/utf8.c | 2 +- src/basic/utf8.h | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/basic/utf8.c b/src/basic/utf8.c index 4d1b31f26d0..68b7c31aecd 100644 --- a/src/basic/utf8.c +++ b/src/basic/utf8.c @@ -179,7 +179,7 @@ char *utf8_escape_invalid(const char *str) { return str_realloc(p); } -static int utf8_char_console_width(const char *str) { +int utf8_char_console_width(const char *str) { char32_t c; int r; diff --git a/src/basic/utf8.h b/src/basic/utf8.h index 301c50cc2ff..de45fcba5ba 100644 --- a/src/basic/utf8.h +++ b/src/basic/utf8.h @@ -59,4 +59,5 @@ static inline char32_t utf16_surrogate_pair_to_unichar(char16_t lead, char16_t t } size_t utf8_n_codepoints(const char *str); +int utf8_char_console_width(const char *str); size_t utf8_console_width(const char *str); From aca093018c5d2cd8a63129cab67941fe1b8fd850 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 26 Apr 2024 17:40:32 +0200 Subject: [PATCH 2/6] strv: add new helper strv_rebreak_lines() with a simple line breaking algorithm --- src/basic/strv.c | 90 ++++++++++++++++++++++++++++++++++++++++++++ src/basic/strv.h | 2 + src/test/test-strv.c | 58 ++++++++++++++++++++++++++++ 3 files changed, 150 insertions(+) diff --git a/src/basic/strv.c b/src/basic/strv.c index d081821a862..4709dfaf246 100644 --- a/src/basic/strv.c +++ b/src/basic/strv.c @@ -11,11 +11,13 @@ #include "escape.h" #include "extract-word.h" #include "fileio.h" +#include "gunicode.h" #include "memory-util.h" #include "nulstr-util.h" #include "sort-util.h" #include "string-util.h" #include "strv.h" +#include "utf8.h" char* strv_find(char * const *l, const char *name) { assert(name); @@ -967,3 +969,91 @@ int _string_strv_ordered_hashmap_put(OrderedHashmap **h, const char *key, const } DEFINE_HASH_OPS_FULL(string_strv_hash_ops, char, string_hash_func, string_compare_func, free, char*, strv_free); + +int strv_rebreak_lines(char **l, size_t width, char ***ret) { + _cleanup_strv_free_ char **broken = NULL; + int r; + + assert(ret); + + /* Implements a simple UTF-8 line breaking algorithm + * + * Goes through all entries in *l, and line-breaks each line that is longer than the specified + * character width. Breaks at the end of words/beginning of whitespace. Lines that do not contain whitespace are not + * broken. Retains whitespace at beginning of lines, removes it at end of lines. */ + + if (width == SIZE_MAX) { /* NOP? */ + broken = strv_copy(l); + if (!broken) + return -ENOMEM; + + *ret = TAKE_PTR(broken); + return 0; + } + + STRV_FOREACH(i, l) { + const char *start = *i, *whitespace_begin = NULL, *whitespace_end = NULL; + bool in_prefix = true; /* still in the whitespace in the beginning of the line? */ + size_t w = 0; + + for (const char *p = start; *p != 0; p = utf8_next_char(p)) { + if (strchr(NEWLINE, *p)) { + in_prefix = true; + whitespace_begin = whitespace_end = NULL; + w = 0; + } else if (strchr(WHITESPACE, *p)) { + if (!in_prefix && (!whitespace_begin || whitespace_end)) { + whitespace_begin = p; + whitespace_end = NULL; + } + } else { + if (whitespace_begin && !whitespace_end) + whitespace_end = p; + + in_prefix = false; + } + + int cw = utf8_char_console_width(p); + if (cw < 0) { + log_debug_errno(cw, "Comment to line break contains invalid UTF-8, ignoring."); + cw = 1; + } + + w += cw; + + if (w > width && whitespace_begin && whitespace_end) { + _cleanup_free_ char *truncated = NULL; + + truncated = strndup(start, whitespace_begin - start); + if (!truncated) + return -ENOMEM; + + r = strv_consume(&broken, TAKE_PTR(truncated)); + if (r < 0) + return r; + + p = start = whitespace_end; + whitespace_begin = whitespace_end = NULL; + w = cw; + } + } + + if (start) { /* Process rest of the line */ + if (in_prefix) /* Never seen anything non-whitespace? Generate empty line! */ + r = strv_extend(&broken, ""); + else if (whitespace_begin && !whitespace_end) { /* Ends in whitespace? Chop it off! */ + _cleanup_free_ char *truncated = strndup(start, whitespace_begin - start); + if (!truncated) + return -ENOMEM; + + r = strv_consume(&broken, TAKE_PTR(truncated)); + } else /* Otherwise use line as is */ + r = strv_extend(&broken, start); + if (r < 0) + return r; + } + } + + *ret = TAKE_PTR(broken); + return 0; +} diff --git a/src/basic/strv.h b/src/basic/strv.h index 169737d1d8c..c828bd612f5 100644 --- a/src/basic/strv.h +++ b/src/basic/strv.h @@ -257,3 +257,5 @@ int _string_strv_hashmap_put(Hashmap **h, const char *key, const char *value HA int _string_strv_ordered_hashmap_put(OrderedHashmap **h, const char *key, const char *value HASHMAP_DEBUG_PARAMS); #define string_strv_hashmap_put(h, k, v) _string_strv_hashmap_put(h, k, v HASHMAP_DEBUG_SRC_ARGS) #define string_strv_ordered_hashmap_put(h, k, v) _string_strv_ordered_hashmap_put(h, k, v HASHMAP_DEBUG_SRC_ARGS) + +int strv_rebreak_lines(char **l, size_t width, char ***ret); diff --git a/src/test/test-strv.c b/src/test/test-strv.c index 28b8b2270c0..65afefed3be 100644 --- a/src/test/test-strv.c +++ b/src/test/test-strv.c @@ -1055,4 +1055,62 @@ TEST(strv_extend_many) { assert_se(strv_equal(l, STRV_MAKE("foo", "bar", "waldo", "quux", "1", "2", "3", "4", "yes", "no"))); } +TEST(strv_rebreak_lines) { + _cleanup_strv_free_ char **l = NULL; + + assert_se(strv_rebreak_lines(NULL, SIZE_MAX, &l) >= 0); + assert_se(strv_equal(l, NULL)); + l = strv_free(l); + + assert_se(strv_rebreak_lines(STRV_MAKE(""), SIZE_MAX, &l) >= 0); + assert_se(strv_equal(l, STRV_MAKE(""))); + l = strv_free(l); + + assert_se(strv_rebreak_lines(STRV_MAKE("", ""), SIZE_MAX, &l) >= 0); + assert_se(strv_equal(l, STRV_MAKE("", ""))); + l = strv_free(l); + + assert_se(strv_rebreak_lines(STRV_MAKE("foo"), SIZE_MAX, &l) >= 0); + assert_se(strv_equal(l, STRV_MAKE("foo"))); + l = strv_free(l); + + assert_se(strv_rebreak_lines(STRV_MAKE("foo", "bar"), SIZE_MAX, &l) >= 0); + assert_se(strv_equal(l, STRV_MAKE("foo", "bar"))); + l = strv_free(l); + + assert_se(strv_rebreak_lines(STRV_MAKE("Foo fOo foO FOo", "bar Bar bAr baR BAr"), 10, &l) >= 0); + assert_se(strv_equal(l, STRV_MAKE("Foo fOo", "foO FOo", "bar Bar", "bAr baR", "BAr"))); + l = strv_free(l); + + assert_se(strv_rebreak_lines(STRV_MAKE(" foo ", + " foo bar waldo quux "), + 10, &l) >= 0); + assert_se(strv_equal(l, STRV_MAKE(" foo", + " foo", + "bar", + "waldo quux"))); + l = strv_free(l); + + assert_se(strv_rebreak_lines(STRV_MAKE(" ", + "\tfoo bar\t", + "FOO\tBAR"), + 10, &l) >= 0); + assert_se(strv_equal(l, STRV_MAKE("", + "\tfoo", + "bar", + "FOO", + "BAR"))); + l = strv_free(l); + + /* Now make sure that breaking the lines a 2nd time does not modify the output anymore */ + for (size_t i = 1; i < 100; i++) { + _cleanup_strv_free_ char **a = NULL, **b = NULL; + + assert_se(strv_rebreak_lines(STRV_MAKE("foobar waldo waldo quux piep\tschnurz pimm"), i, &a) >= 0); + assert_se(strv_rebreak_lines(a, i, &b) >= 0); + + assert_se(strv_equal(a, b)); + } +} + DEFINE_TEST_MAIN(LOG_INFO); From fbb69c0306d434153ca85b227c7d42b9c92872e4 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 26 Apr 2024 17:43:21 +0200 Subject: [PATCH 3/6] varlink: add concept for embedding comments into IDL structures --- src/shared/varlink-idl.c | 196 ++++++++++++++++++++++++++++++++---- src/shared/varlink-idl.h | 26 ++++- src/test/test-varlink-idl.c | 2 +- src/varlinkctl/varlinkctl.c | 4 +- 4 files changed, 204 insertions(+), 24 deletions(-) diff --git a/src/shared/varlink-idl.c b/src/shared/varlink-idl.c index 5c0d7204591..f249ff5955c 100644 --- a/src/shared/varlink-idl.c +++ b/src/shared/varlink-idl.c @@ -5,6 +5,7 @@ #include "set.h" #include "strv.h" #include "terminal-util.h" +#include "utf8.h" #include "varlink-idl.h" #define DEPTH_MAX 64U @@ -15,13 +16,63 @@ enum { COLOR_IDENTIFIER, COLOR_MARKS, /* [], ->, ?, … */ COLOR_RESET, + COLOR_COMMENT, _COLOR_MAX, }; #define varlink_idl_log(error, format, ...) log_debug_errno(error, "Varlink-IDL: " format, ##__VA_ARGS__) #define varlink_idl_log_full(level, error, format, ...) log_full_errno(level, error, "Varlink-IDL: " format, ##__VA_ARGS__) -static int varlink_idl_format_all_fields(FILE *f, const VarlinkSymbol *symbol, VarlinkFieldDirection direction, const char *indent, const char *const colors[static _COLOR_MAX]); +static int varlink_idl_format_all_fields(FILE *f, const VarlinkSymbol *symbol, VarlinkFieldDirection direction, const char *indent, const char *const colors[static _COLOR_MAX], size_t cols); + +static int varlink_idl_format_comment( + FILE *f, + const char *text, + const char *indent, + const char *const colors[static _COLOR_MAX], + size_t cols) { + + int r; + + assert(f); + assert(colors); + + if (!text) { + /* If text is NULL, output an empty but commented line */ + fputs(strempty(indent), f); + fputs(colors[COLOR_COMMENT], f); + fputs("#", f); + fputs(colors[COLOR_RESET], f); + fputs("\n", f); + return 0; + } + + _cleanup_strv_free_ char **l = NULL; + r = strv_split_full(&l, text, NEWLINE, 0); + if (r < 0) + return log_error_errno(r, "Failed to split comment string: %m"); + + size_t indent_width = utf8_console_width(indent); + size_t max_width = indent_width < cols ? cols - indent_width : 0; + if (max_width < 10) + max_width = 10; + + _cleanup_strv_free_ char **broken = NULL; + r = strv_rebreak_lines(l, max_width, &broken); + if (r < 0) + return log_error_errno(r, "Failed to rebreak lines in comment: %m"); + + STRV_FOREACH(i, broken) { + fputs(strempty(indent), f); + fputs(colors[COLOR_COMMENT], f); + fputs("# ", f); + fputs(*i, f); + fputs(colors[COLOR_RESET], f); + fputs("\n", f); + } + + return 0; +} static int varlink_idl_format_enum_values( FILE *f, @@ -65,10 +116,12 @@ static int varlink_idl_format_field( FILE *f, const VarlinkField *field, const char *indent, - const char *const colors[static _COLOR_MAX]) { + const char *const colors[static _COLOR_MAX], + size_t cols) { assert(f); assert(field); + assert(field->field_type != _VARLINK_FIELD_COMMENT); fputs(strempty(indent), f); fputs(colors[COLOR_IDENTIFIER], f); @@ -146,7 +199,7 @@ static int varlink_idl_format_field( break; case VARLINK_STRUCT: - return varlink_idl_format_all_fields(f, ASSERT_PTR(field->symbol), VARLINK_REGULAR, indent, colors); + 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); @@ -163,7 +216,8 @@ static int varlink_idl_format_all_fields( const VarlinkSymbol *symbol, VarlinkFieldDirection filter_direction, 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; @@ -179,6 +233,9 @@ static int varlink_idl_format_all_fields( 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 (field->field_direction != filter_direction) continue; @@ -188,7 +245,27 @@ static int varlink_idl_format_all_fields( } else fputs(",\n", f); - r = varlink_idl_format_field(f, field, indent2, colors); + /* We found a field we want to output. In this case, output all immediately preceeding + * 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; + } + + 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_field(f, field, indent2, colors, cols); if (r < 0) return r; } @@ -207,7 +284,8 @@ static int varlink_idl_format_all_fields( static int varlink_idl_format_symbol( FILE *f, const VarlinkSymbol *symbol, - const char *const colors[static _COLOR_MAX]) { + const char *const colors[static _COLOR_MAX], + size_t cols) { int r; assert(f); @@ -232,7 +310,7 @@ static int varlink_idl_format_symbol( fputs(symbol->name, f); fputs(colors[COLOR_RESET], f); - r = varlink_idl_format_all_fields(f, symbol, VARLINK_REGULAR, /* indent= */ NULL, colors); + r = varlink_idl_format_all_fields(f, symbol, VARLINK_REGULAR, /* indent= */ NULL, colors, cols); break; case VARLINK_METHOD: @@ -242,7 +320,7 @@ static int varlink_idl_format_symbol( fputs(symbol->name, f); fputs(colors[COLOR_RESET], f); - r = varlink_idl_format_all_fields(f, symbol, VARLINK_INPUT, /* indent= */ NULL, colors); + r = varlink_idl_format_all_fields(f, symbol, VARLINK_INPUT, /* indent= */ NULL, colors, cols); if (r < 0) return r; @@ -250,7 +328,7 @@ static int varlink_idl_format_symbol( fputs(" -> ", f); fputs(colors[COLOR_RESET], f); - r = varlink_idl_format_all_fields(f, symbol, VARLINK_OUTPUT, /* indent= */ NULL, colors); + r = varlink_idl_format_all_fields(f, symbol, VARLINK_OUTPUT, /* indent= */ NULL, colors, cols); break; case VARLINK_ERROR: @@ -260,7 +338,7 @@ static int varlink_idl_format_symbol( fputs(symbol->name, f); fputs(colors[COLOR_RESET], f); - r = varlink_idl_format_all_fields(f, symbol, VARLINK_REGULAR, /* indent= */ NULL, colors); + r = varlink_idl_format_all_fields(f, symbol, VARLINK_REGULAR, /* indent= */ NULL, colors, cols); break; default: @@ -277,7 +355,8 @@ static int varlink_idl_format_all_symbols( FILE *f, const VarlinkInterface *interface, VarlinkSymbolType filter_type, - const char *const colors[static _COLOR_MAX]) { + const char *const colors[static _COLOR_MAX], + size_t cols) { int r; @@ -289,9 +368,38 @@ static int varlink_idl_format_all_symbols( if ((*symbol)->symbol_type != filter_type) continue; + if ((*symbol)->symbol_type == _VARLINK_INTERFACE_COMMENT) { + /* Interface comments we'll output directly. */ + r = varlink_idl_format_comment(f, ASSERT_PTR((*symbol)->name), /* indent= */ NULL, colors, cols); + if (r < 0) + return r; + + continue; + } + fputs("\n", f); - r = varlink_idl_format_symbol(f, *symbol, colors); + /* Symbol comments we'll only output if we are outputing the symbol they belong to. Scan + * backwards for symbol comments. */ + const VarlinkSymbol *const*start_comment = NULL; + for (const VarlinkSymbol *const*c1 = symbol; c1 > interface->symbols; c1--) { + const VarlinkSymbol *const *c0 = c1 - 1; + + if ((*c0)->symbol_type != _VARLINK_SYMBOL_COMMENT) + break; + + start_comment = c0; + } + + /* Found one or more comments, output them now */ + if (start_comment) + for (const VarlinkSymbol *const*c = start_comment; c < symbol; c++) { + r = varlink_idl_format_comment(f, ASSERT_PTR((*c)->name), /* indent= */ NULL, colors, cols); + if (r < 0) + return r; + } + + r = varlink_idl_format_symbol(f, *symbol, colors, cols); if (r < 0) return r; } @@ -299,17 +407,18 @@ static int varlink_idl_format_all_symbols( return 0; } -int varlink_idl_dump(FILE *f, int use_colors, const VarlinkInterface *interface) { +int varlink_idl_dump(FILE *f, int use_colors, size_t cols, const VarlinkInterface *interface) { static const char* const color_table[_COLOR_MAX] = { [COLOR_SYMBOL_TYPE] = ANSI_HIGHLIGHT_GREEN, [COLOR_FIELD_TYPE] = ANSI_HIGHLIGHT_BLUE, [COLOR_IDENTIFIER] = ANSI_NORMAL, [COLOR_MARKS] = ANSI_HIGHLIGHT_MAGENTA, [COLOR_RESET] = ANSI_NORMAL, + [COLOR_COMMENT] = ANSI_GREY, }; static const char* const color_off[_COLOR_MAX] = { - "", "", "", "", "", + "", "", "", "", "", "", }; int r; @@ -324,6 +433,11 @@ int varlink_idl_dump(FILE *f, int use_colors, const VarlinkInterface *interface) const char *const *colors = use_colors ? color_table : color_off; + /* First output interface comments */ + r = varlink_idl_format_all_symbols(f, interface, _VARLINK_INTERFACE_COMMENT, colors, cols); + if (r < 0) + return r; + fputs(colors[COLOR_SYMBOL_TYPE], f); fputs("interface ", f); fputs(colors[COLOR_IDENTIFIER], f); @@ -331,8 +445,15 @@ int varlink_idl_dump(FILE *f, int use_colors, const VarlinkInterface *interface) fputs(colors[COLOR_RESET], f); fputs("\n", f); + /* Then output all symbols, ordered by symbol type */ for (VarlinkSymbolType t = 0; t < _VARLINK_SYMBOL_TYPE_MAX; t++) { - r = varlink_idl_format_all_symbols(f, interface, t, colors); + + /* Interface comments we already have output above. Symbol comments are output when the + * symbol they belong to are output, hence filter both here. */ + if (IN_SET(t, _VARLINK_SYMBOL_COMMENT, _VARLINK_INTERFACE_COMMENT)) + continue; + + r = varlink_idl_format_all_symbols(f, interface, t, colors, cols); if (r < 0) return r; } @@ -340,14 +461,14 @@ int varlink_idl_dump(FILE *f, int use_colors, const VarlinkInterface *interface) return 0; } -int varlink_idl_format(const VarlinkInterface *interface, char **ret) { +int varlink_idl_format_full(const VarlinkInterface *interface, size_t cols, char **ret) { _cleanup_(memstream_done) MemStream memstream = {}; int r; if (!memstream_init(&memstream)) return -errno; - r = varlink_idl_dump(memstream.f, /* use_colors= */ false, interface); + r = varlink_idl_dump(memstream.f, /* use_colors= */ false, cols, interface); if (r < 0) return r; @@ -1201,6 +1322,10 @@ bool varlink_idl_interface_name_is_valid(const char *name) { return true; } +static bool varlink_idl_comment_is_valid(const char *comment) { + return utf8_is_valid(comment); +} + static int varlink_idl_symbol_consistent(const VarlinkInterface *interface, const VarlinkSymbol *symbol, int level); static int varlink_idl_field_consistent( @@ -1293,6 +1418,9 @@ static int varlink_idl_field_consistent( static bool varlink_symbol_is_empty(const VarlinkSymbol *symbol) { assert(symbol); + if (IN_SET(symbol->symbol_type, _VARLINK_SYMBOL_COMMENT, _VARLINK_INTERFACE_COMMENT)) + return true; + return symbol->fields[0].field_type == _VARLINK_FIELD_TYPE_END_MARKER; } @@ -1316,7 +1444,18 @@ static int varlink_idl_symbol_consistent( if (IN_SET(symbol->symbol_type, VARLINK_STRUCT_TYPE, VARLINK_ENUM_TYPE) && varlink_symbol_is_empty(symbol)) return varlink_idl_log_full(level, SYNTHETIC_ERRNO(EUCLEAN), "Symbol '%s' is empty, refusing.", symbol_name); + if (IN_SET(symbol->symbol_type, _VARLINK_SYMBOL_COMMENT, _VARLINK_INTERFACE_COMMENT)) + return 0; + for (const VarlinkField *field = symbol->fields; field->field_type != _VARLINK_FIELD_TYPE_END_MARKER; field++) { + + if (field->field_type == _VARLINK_FIELD_COMMENT) { + if (!varlink_idl_comment_is_valid(field->name)) + return varlink_idl_log_full(level, SYNTHETIC_ERRNO(EUCLEAN), "Comment in symbol '%s' not valid, refusing.", symbol_name); + + continue; + } + Set **name_set = field->field_direction == VARLINK_OUTPUT ? &output_set : &input_set; /* for the method case we need two separate sets, otherwise we use the same */ if (!varlink_idl_field_name_is_valid(field->name)) @@ -1347,6 +1486,12 @@ int varlink_idl_consistent(const VarlinkInterface *interface, int level) { for (const VarlinkSymbol *const *symbol = interface->symbols; *symbol; symbol++) { + if (IN_SET((*symbol)->symbol_type, _VARLINK_SYMBOL_COMMENT, _VARLINK_INTERFACE_COMMENT)) { + if (!varlink_idl_comment_is_valid((*symbol)->name)) + return varlink_idl_log_full(level, SYNTHETIC_ERRNO(EUCLEAN), "Comment in interface '%s' not valid, refusing.", interface->name); + continue; + } + if (!varlink_idl_symbol_name_is_valid((*symbol)->name)) return varlink_idl_log_full(level, SYNTHETIC_ERRNO(EUCLEAN), "Symbol name '%s' is not valid, refusing.", strempty((*symbol)->name)); @@ -1407,6 +1552,9 @@ static int varlink_idl_validate_field_element_type(const VarlinkField *field, sd break; + case _VARLINK_FIELD_COMMENT: + break; + default: assert_not_reached(); } @@ -1418,6 +1566,7 @@ static int varlink_idl_validate_field(const VarlinkField *field, sd_json_variant int r; assert(field); + assert(field->field_type != _VARLINK_FIELD_COMMENT); if (!v || sd_json_variant_is_null(v)) { @@ -1449,7 +1598,6 @@ static int varlink_idl_validate_field(const VarlinkField *field, sd_json_variant return r; } } else { - r = varlink_idl_validate_field_element_type(field, v); if (r < 0) return r; @@ -1462,6 +1610,7 @@ static int varlink_idl_validate_symbol(const VarlinkSymbol *symbol, sd_json_vari int r; assert(symbol); + assert(!IN_SET(symbol->symbol_type, _VARLINK_SYMBOL_COMMENT, _VARLINK_INTERFACE_COMMENT)); if (!v) { if (bad_field) @@ -1537,6 +1686,10 @@ static int varlink_idl_validate_symbol(const VarlinkSymbol *symbol, sd_json_vari break; } + case _VARLINK_SYMBOL_COMMENT: + case _VARLINK_INTERFACE_COMMENT: + break; + default: assert_not_reached(); } @@ -1579,6 +1732,7 @@ const VarlinkSymbol* varlink_idl_find_symbol( assert(interface); assert(type < _VARLINK_SYMBOL_TYPE_MAX); + assert(!IN_SET(type, _VARLINK_SYMBOL_COMMENT, _VARLINK_INTERFACE_COMMENT)); if (isempty(name)) return NULL; @@ -1603,9 +1757,13 @@ const VarlinkField* varlink_idl_find_field( if (isempty(name)) return NULL; - for (const VarlinkField *field = symbol->fields; field->field_type != _VARLINK_FIELD_TYPE_END_MARKER; field++) + for (const VarlinkField *field = symbol->fields; field->field_type != _VARLINK_FIELD_TYPE_END_MARKER; field++) { + if (field->field_type == _VARLINK_FIELD_COMMENT) + continue; + if (streq_ptr(field->name, name)) return field; + } return NULL; } diff --git a/src/shared/varlink-idl.h b/src/shared/varlink-idl.h index 6d0b3490fb2..03a1a2b2e48 100644 --- a/src/shared/varlink-idl.h +++ b/src/shared/varlink-idl.h @@ -20,6 +20,8 @@ typedef enum VarlinkSymbolType { VARLINK_STRUCT_TYPE, VARLINK_METHOD, VARLINK_ERROR, + _VARLINK_INTERFACE_COMMENT, /* Not really a symbol, just a comment about the interface */ + _VARLINK_SYMBOL_COMMENT, /* Not really a symbol, just a comment about a symbol */ _VARLINK_SYMBOL_TYPE_MAX, _VARLINK_SYMBOL_TYPE_INVALID = -EINVAL, } VarlinkSymbolType; @@ -35,6 +37,7 @@ typedef enum VarlinkFieldType { VARLINK_STRING, VARLINK_OBJECT, VARLINK_ENUM_VALUE, + _VARLINK_FIELD_COMMENT, /* Not really a field, just a comment about a field*/ _VARLINK_FIELD_TYPE_MAX, _VARLINK_FIELD_TYPE_INVALID = -EINVAL, } VarlinkFieldType; @@ -103,6 +106,9 @@ struct VarlinkInterface { #define VARLINK_DEFINE_ENUM_VALUE(_name) \ { .name = #_name, .field_type = VARLINK_ENUM_VALUE } +#define VARLINK_FIELD_COMMENT(text) \ + { .name = "" text, .field_type = _VARLINK_FIELD_COMMENT } + #define VARLINK_DEFINE_METHOD(_name, ...) \ const VarlinkSymbol vl_method_ ## _name = { \ .name = #_name, \ @@ -137,8 +143,24 @@ struct VarlinkInterface { .symbols = { __VA_ARGS__ __VA_OPT__(,) NULL}, \ } -int varlink_idl_dump(FILE *f, int use_colors, const VarlinkInterface *interface); -int varlink_idl_format(const VarlinkInterface *interface, char **ret); +#define VARLINK_SYMBOL_COMMENT(text) \ + &(const VarlinkSymbol) { \ + .name = "" text, \ + .symbol_type = _VARLINK_SYMBOL_COMMENT, \ + } + +#define VARLINK_INTERFACE_COMMENT(text) \ + &(const VarlinkSymbol) { \ + .name = "" text, \ + .symbol_type = _VARLINK_INTERFACE_COMMENT, \ + } + +int varlink_idl_dump(FILE *f, int use_colors, size_t cols, const VarlinkInterface *interface); +int varlink_idl_format_full(const VarlinkInterface *interface, size_t cols, char **ret); + +static inline int varlink_idl_format(const VarlinkInterface *interface, char **ret) { + return varlink_idl_format_full(interface, SIZE_MAX, ret); +} int varlink_idl_parse(const char *text, unsigned *ret_line, unsigned *ret_column, VarlinkInterface **ret); VarlinkInterface* varlink_interface_free(VarlinkInterface *interface); diff --git a/src/test/test-varlink-idl.c b/src/test/test-varlink-idl.c index 34d84145fc2..6a28edf4671 100644 --- a/src/test/test-varlink-idl.c +++ b/src/test/test-varlink-idl.c @@ -117,7 +117,7 @@ static void test_parse_format_one(const VarlinkInterface *iface) { assert_se(iface); - assert_se(varlink_idl_dump(stdout, /* use_colors=*/ true, iface) >= 0); + assert_se(varlink_idl_dump(stdout, /* use_colors=*/ true, /* cols= */ SIZE_MAX, iface) >= 0); assert_se(varlink_idl_consistent(iface, LOG_ERR) >= 0); assert_se(varlink_idl_format(iface, &text) >= 0); assert_se(varlink_idl_parse(text, NULL, NULL, &parsed) >= 0); diff --git a/src/varlinkctl/varlinkctl.c b/src/varlinkctl/varlinkctl.c index 3057c0542cb..8752ee575cc 100644 --- a/src/varlinkctl/varlinkctl.c +++ b/src/varlinkctl/varlinkctl.c @@ -392,7 +392,7 @@ static int verb_introspect(int argc, char *argv[], void *userdata) { } } else { pager_open(arg_pager_flags); - r = varlink_idl_dump(stdout, /* use_colors= */ -1, vi); + r = varlink_idl_dump(stdout, /* use_colors= */ -1, on_tty() ? columns() : SIZE_MAX, vi); if (r < 0) return log_error_errno(r, "Failed to format parsed interface description: %m"); } @@ -628,7 +628,7 @@ static int verb_validate_idl(int argc, char *argv[], void *userdata) { pager_open(arg_pager_flags); - r = varlink_idl_dump(stdout, /* use_colors= */ -1, vi); + r = varlink_idl_dump(stdout, /* use_colors= */ -1, on_tty() ? columns() : SIZE_MAX, vi); if (r < 0) return log_error_errno(r, "Failed to format parsed interface description: %m"); From ce2d2260c9e8195b8d8bf4e798c8c2d3c29098ac Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 29 Apr 2024 15:33:02 +0200 Subject: [PATCH 4/6] varlink: parse comments too --- src/shared/varlink-idl.c | 89 ++++++++++++++++++++++++++++++++----- src/test/test-varlink-idl.c | 15 +++++++ 2 files changed, 94 insertions(+), 10 deletions(-) diff --git a/src/shared/varlink-idl.c b/src/shared/varlink-idl.c index f249ff5955c..e100137a757 100644 --- a/src/shared/varlink-idl.c +++ b/src/shared/varlink-idl.c @@ -657,8 +657,10 @@ static int varlink_idl_subparse_token( static int varlink_idl_subparse_comment( const char **p, unsigned *line, - unsigned *column) { + unsigned *column, + char **ret) { + _cleanup_free_ char *comment = NULL; size_t l; assert(p); @@ -667,9 +669,30 @@ static int varlink_idl_subparse_comment( assert(column); l = strcspn(*p, NEWLINE); + + if (!utf8_is_valid_n(*p, l)) + return varlink_idl_log(SYNTHETIC_ERRNO(EBADMSG), "%u:%u: Comment is not valid UTF-8.", *line, *column); + + if (ret) { + /* Remove a single space as prefix of a comment, if one is specified. This is because we + * generally expect comments to be formatted as "# foobar" rather than "#foobar", and will + * ourselves format them that way. We accept the comments without the space too however. We + * will not strip more than one space, to allow indented comment blocks. */ + + if (**p == ' ') + comment = strndup(*p + 1, l - 1); + else + comment = strndup(*p, l); + if (!comment) + return -ENOMEM; + } + advance_line_column(*p, l + 1, line, column); *p += l; + if (ret) + *ret = TAKE_PTR(comment); + return 1; } @@ -856,7 +879,7 @@ static int varlink_idl_subparse_struct_or_enum( return varlink_idl_log(SYNTHETIC_ERRNO(EBADMSG), "%u:%u: Unexpected token '%s'.", *line, *column, token); state = STATE_NAME; - allowed_delimiters = ")"; + allowed_delimiters = ")#"; allowed_chars = VALID_CHARS_IDENTIFIER; break; @@ -865,7 +888,23 @@ static int varlink_idl_subparse_struct_or_enum( if (!token) return varlink_idl_log(SYNTHETIC_ERRNO(EBADMSG), "%u:%u: Premature EOF.", *line, *column); - if (streq(token, ")")) + else if (streq(token, "#")) { + _cleanup_free_ char *comment = NULL; + + r = varlink_idl_subparse_comment(p, line, column, &comment); + if (r < 0) + return r; + + r = varlink_symbol_realloc(symbol, *n_fields + 1); + if (r < 0) + return r; + + VarlinkField *field = (*symbol)->fields + (*n_fields)++; + *field = (VarlinkField) { + .name = TAKE_PTR(comment), + .field_type = _VARLINK_FIELD_COMMENT, + }; + } else if (streq(token, ")")) state = STATE_DONE; else { field_name = TAKE_PTR(token); @@ -929,7 +968,7 @@ static int varlink_idl_subparse_struct_or_enum( if (streq(token, ",")) { state = STATE_NAME; - allowed_delimiters = NULL; + allowed_delimiters = "#"; allowed_chars = VALID_CHARS_IDENTIFIER; } else { assert(streq(token, ")")); @@ -947,7 +986,7 @@ static int varlink_idl_subparse_struct_or_enum( return varlink_idl_log(SYNTHETIC_ERRNO(EBADMSG), "%u:%u: Premature EOF.", *line, *column); if (streq(token, ",")) { state = STATE_NAME; - allowed_delimiters = NULL; + allowed_delimiters = "#"; allowed_chars = VALID_CHARS_IDENTIFIER; } else if (streq(token, ")")) state = STATE_DONE; @@ -1062,9 +1101,24 @@ int varlink_idl_parse( if (!token) return varlink_idl_log(SYNTHETIC_ERRNO(EBADMSG), "%u:%u: Premature EOF.", *line, *column); if (streq(token, "#")) { - r = varlink_idl_subparse_comment(&text, line, column); + _cleanup_free_ char *comment = NULL; + + r = varlink_idl_subparse_comment(&text, line, column, &comment); if (r < 0) return r; + + r = varlink_interface_realloc(&interface, n_symbols + 1); + if (r < 0) + return r; + + r = varlink_symbol_realloc(&symbol, 0); + if (r < 0) + return r; + + symbol->symbol_type = _VARLINK_INTERFACE_COMMENT; + symbol->name = TAKE_PTR(comment); + + interface->symbols[n_symbols++] = TAKE_PTR(symbol); } else if (streq(token, "interface")) { state = STATE_INTERFACE; allowed_delimiters = NULL; @@ -1074,9 +1128,6 @@ int varlink_idl_parse( break; case STATE_INTERFACE: - assert(!interface); - assert(n_symbols == 0); - if (!token) return varlink_idl_log(SYNTHETIC_ERRNO(EBADMSG), "%u:%u: Premature EOF.", *line, *column); @@ -1084,7 +1135,9 @@ int varlink_idl_parse( if (r < 0) return r; + assert(!interface->name); interface->name = TAKE_PTR(token); + state = STATE_PRE_SYMBOL; allowed_delimiters = "#"; allowed_chars = VALID_CHARS_RESERVED; @@ -1097,9 +1150,25 @@ int varlink_idl_parse( } if (streq(token, "#")) { - r = varlink_idl_subparse_comment(&text, line, column); + _cleanup_free_ char *comment = NULL; + + r = varlink_idl_subparse_comment(&text, line, column, &comment); if (r < 0) return r; + + r = varlink_interface_realloc(&interface, n_symbols + 1); + if (r < 0) + return r; + + assert(!symbol); + r = varlink_symbol_realloc(&symbol, 0); + if (r < 0) + return r; + + symbol->symbol_type = _VARLINK_SYMBOL_COMMENT; + symbol->name = TAKE_PTR(comment); + + interface->symbols[n_symbols++] = TAKE_PTR(symbol); } else if (streq(token, "method")) { state = STATE_METHOD; allowed_chars = VALID_CHARS_IDENTIFIER; diff --git a/src/test/test-varlink-idl.c b/src/test/test-varlink-idl.c index 6a28edf4671..f8c8080ee55 100644 --- a/src/test/test-varlink-idl.c +++ b/src/test/test-varlink-idl.c @@ -123,6 +123,21 @@ static void test_parse_format_one(const VarlinkInterface *iface) { assert_se(varlink_idl_parse(text, NULL, NULL, &parsed) >= 0); assert_se(varlink_idl_consistent(parsed, LOG_ERR) >= 0); assert_se(varlink_idl_format(parsed, &text2) >= 0); + + ASSERT_STREQ(text, text2); + + text = mfree(text); + text2 = mfree(text2); + parsed = varlink_interface_free(parsed); + + /* Do the same thing, but aggressively line break, and make sure this is roundtrippable as well */ + assert_se(varlink_idl_dump(stdout, /* use_colors=*/ true, 23, iface) >= 0); + assert_se(varlink_idl_consistent(iface, LOG_ERR) >= 0); + assert_se(varlink_idl_format_full(iface, 23, &text) >= 0); + assert_se(varlink_idl_parse(text, NULL, NULL, &parsed) >= 0); + assert_se(varlink_idl_consistent(parsed, LOG_ERR) >= 0); + assert_se(varlink_idl_format_full(parsed, 23, &text2) >= 0); + ASSERT_STREQ(text, text2); } From b4c91fbc07c011c87a0255d4536e746b19becfbe Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 26 Apr 2024 17:44:12 +0200 Subject: [PATCH 5/6] creds: add comments to credential encryption/decryption method calls --- src/shared/varlink-io.systemd.Credentials.c | 24 +++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/shared/varlink-io.systemd.Credentials.c b/src/shared/varlink-io.systemd.Credentials.c index 03db0b35a0d..aafdfcb4109 100644 --- a/src/shared/varlink-io.systemd.Credentials.c +++ b/src/shared/varlink-io.systemd.Credentials.c @@ -4,24 +4,40 @@ static VARLINK_DEFINE_METHOD( Encrypt, + VARLINK_FIELD_COMMENT("The name for the encrypted credential, a string suitable for inclusion in a file name. If not specified no name is encoded in the credential. Typically, if this credential is stored on disk, this is how the file should be called, and permits authentication of the filename."), VARLINK_DEFINE_INPUT(name, VARLINK_STRING, VARLINK_NULLABLE), + VARLINK_FIELD_COMMENT("Plaintext to encrypt. Suitable only for textual data. Either this field or 'data' (below) must be provided."), VARLINK_DEFINE_INPUT(text, VARLINK_STRING, VARLINK_NULLABLE), + VARLINK_FIELD_COMMENT("Plaintext to encrypt, encoded in Base64. Suitable for binary data. Either this field or 'text' (above) must be provided."), VARLINK_DEFINE_INPUT(data, VARLINK_STRING, VARLINK_NULLABLE), + VARLINK_FIELD_COMMENT("Timestamp to store in the credential. In µs since the UNIX epoch, i.e. Jan 1st 1970. If not specified the current time is used."), VARLINK_DEFINE_INPUT(timestamp, VARLINK_INT, VARLINK_NULLABLE), + VARLINK_FIELD_COMMENT("Timestamp when to the credential should be considered invalid. In µs since the UNIX epoch. If not specified, the credential remains valid forever."), VARLINK_DEFINE_INPUT(notAfter, VARLINK_INT, VARLINK_NULLABLE), + VARLINK_FIELD_COMMENT("The intended scope for the credential. One of 'system' or 'user'. If not specified defaults to 'system', unless an uid is specified (see below), in which case it default to 'user'."), VARLINK_DEFINE_INPUT(scope, VARLINK_STRING, VARLINK_NULLABLE), + VARLINK_FIELD_COMMENT("The numeric UNIX UID of the user the credential shall be scoped to. Only relevant if 'user' scope is selected (see above). If not specified and 'user' scope is selected defaults to the UID of the calling user, if that can be determined."), VARLINK_DEFINE_INPUT(uid, VARLINK_INT, VARLINK_NULLABLE), + VARLINK_FIELD_COMMENT("Controls whether interactive authentication (via polkit) shall be allowed. If unspecified defaults to false."), VARLINK_DEFINE_INPUT(allowInteractiveAuthentication, VARLINK_BOOL, VARLINK_NULLABLE), + VARLINK_FIELD_COMMENT("Encrypted credential in Base64 encoding. This can be stored in a credential file, for consumption in LoadEncryptedCredential= and similar calls. Note that the Base64 encoding should be retained when copied into a file."), VARLINK_DEFINE_OUTPUT(blob, VARLINK_STRING, 0)); static VARLINK_DEFINE_METHOD( Decrypt, + VARLINK_FIELD_COMMENT("The name of the encrypted credential. Must the same string specified when the credential was encrypted, in order to authenticate this. If not specified authentication of the credential name is not done."), VARLINK_DEFINE_INPUT(name, VARLINK_STRING, VARLINK_NULLABLE), + VARLINK_FIELD_COMMENT("The encrypted credential in Base64 encoding. This corresponds of the 'blob' field returned by the 'Encrypt' method."), VARLINK_DEFINE_INPUT(blob, VARLINK_STRING, 0), + VARLINK_FIELD_COMMENT("The timestamp to use when validating the credential's time validity range. If not specified the current time is used."), VARLINK_DEFINE_INPUT(timestamp, VARLINK_INT, VARLINK_NULLABLE), + VARLINK_FIELD_COMMENT("The scope for this credential. If not specified no restrictions on the credential scope are made."), VARLINK_DEFINE_INPUT(scope, VARLINK_STRING, VARLINK_NULLABLE), + VARLINK_FIELD_COMMENT("If the 'user' scope is selected, specifies the numeric UNIX UID of the user the credential is associated with. If not specified this is automatically derived from the UID of the calling user, if that can be determined."), VARLINK_DEFINE_INPUT(uid, VARLINK_INT, VARLINK_NULLABLE), + VARLINK_FIELD_COMMENT("Controls whether interactive authentication (via polkit) shall be allowed. If unspecified defaults to false."), VARLINK_DEFINE_INPUT(allowInteractiveAuthentication, VARLINK_BOOL, VARLINK_NULLABLE), + VARLINK_FIELD_COMMENT("The decrypted plaintext data in Base64 encoding."), VARLINK_DEFINE_OUTPUT(data, VARLINK_STRING, 0)); static VARLINK_DEFINE_ERROR(BadFormat); @@ -33,10 +49,18 @@ static VARLINK_DEFINE_ERROR(BadScope); VARLINK_DEFINE_INTERFACE( io_systemd_Credentials, "io.systemd.Credentials", + VARLINK_INTERFACE_COMMENT("APIs for encrypting and decrypting service credentials."), + VARLINK_SYMBOL_COMMENT("Encrypts some plaintext data, returns an encrypted credential."), &vl_method_Encrypt, + VARLINK_SYMBOL_COMMENT("Decrypts an encrypted credential, returns plaintext data."), &vl_method_Decrypt, + VARLINK_SYMBOL_COMMENT("Indicates that a corrupt and unsupported encrypted credential was provided."), &vl_error_BadFormat, + VARLINK_SYMBOL_COMMENT("The specified name does not match the name stored in the credential."), &vl_error_NameMismatch, + VARLINK_SYMBOL_COMMENT("The credential's is no longer or not yet valid."), &vl_error_TimeMismatch, + VARLINK_SYMBOL_COMMENT("The specified user does not exist."), &vl_error_NoSuchUser, + VARLINK_SYMBOL_COMMENT("The credential does not match the selected scope."), &vl_error_BadScope); From 783236abd74bed5462e89fc8efb71e55b48d6cc9 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 29 Apr 2024 15:47:17 +0200 Subject: [PATCH 6/6] update TODO --- TODO | 7 ------- 1 file changed, 7 deletions(-) diff --git a/TODO b/TODO index 7bbf334ef16..100af7cde24 100644 --- a/TODO +++ b/TODO @@ -250,8 +250,6 @@ Features: pidfd, so that we can reasonably robustly do this. Would only cover the execution environment like namespaces, but not the privilege settings. -* varlink: extend varlink IDL macros to include documentation strings - * Introduce a CGroupRef structure, inspired by PidRef. Should contain cgroup path, cgroup id, and cgroup fd. Use it to continuously pin all v2 cgroups via a cgroup_ref field in the CGroupRuntime structure. Eventually switch things @@ -301,11 +299,6 @@ Features: word in the command line. (maybe use character '.'). Usecase: tool such as run0 can use that to spawn the target user's default shell. -* varlink: figure out how to do docs for our varlink interfaces. Idea: install - interface files augmented with docs in /usr/share/ somewhere. And have - functionality in varlinkctl to merge interface info extracted from binaries - with interface info on disk. And store the doc strings only in the latter. - * introduce mntid_t, and make it 64bit, as apparently the kernel switched to 64bit mount ids