From 8279484a41e94aa8fc20385fad75a0a846ccf1af Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 2 Feb 2024 13:30:43 +0100 Subject: [PATCH 01/10] pcrlock: use log_setup() --- src/pcrlock/pcrlock.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/pcrlock/pcrlock.c b/src/pcrlock/pcrlock.c index 9a9da049b2..cb9d004ea7 100644 --- a/src/pcrlock/pcrlock.c +++ b/src/pcrlock/pcrlock.c @@ -5109,9 +5109,7 @@ static int pcrlock_main(int argc, char *argv[]) { static int run(int argc, char *argv[]) { int r; - log_show_color(true); - log_parse_environment(); - log_open(); + log_setup(); r = parse_argv(argc, argv); if (r <= 0) From 9fe15ce84d52c1f613d5e2a815d0406941bb20e1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 2 Feb 2024 15:15:22 +0100 Subject: [PATCH 02/10] pcrlock: split out generation of CEL objects into helper func This way, we can reuse it later to generate Varlink replies No change in behaviour, just some trivial split out. --- src/pcrlock/pcrlock.c | 130 ++++++++++++++++++++++++------------------ 1 file changed, 74 insertions(+), 56 deletions(-) diff --git a/src/pcrlock/pcrlock.c b/src/pcrlock/pcrlock.c index cb9d004ea7..016d73cc00 100644 --- a/src/pcrlock/pcrlock.c +++ b/src/pcrlock/pcrlock.c @@ -2412,6 +2412,75 @@ static int verb_show_log(int argc, char *argv[], void *userdata) { return 0; } +static int event_log_record_to_cel(EventLogRecord *record, uint64_t *recnum, JsonVariant **ret) { + _cleanup_(json_variant_unrefp) JsonVariant *ja = NULL, *fj = NULL; + JsonVariant *cd = NULL; + const char *ct = NULL; + int r; + + assert(record); + assert(recnum); + assert(ret); + + LIST_FOREACH(banks, bank, record->banks) { + r = json_variant_append_arrayb( + &ja, JSON_BUILD_OBJECT( + JSON_BUILD_PAIR_STRING("hashAlg", tpm2_hash_alg_to_string(bank->algorithm)), + JSON_BUILD_PAIR_HEX("digest", bank->hash.buffer, bank->hash.size))); + if (r < 0) + return log_error_errno(r, "Failed to append CEL digest entry: %m"); + } + + if (!ja) { + r = json_variant_new_array(&ja, NULL, 0); + if (r < 0) + return log_error_errno(r, "Failed to allocate JSON array: %m"); + } + + if (EVENT_LOG_RECORD_IS_FIRMWARE(record)) { + _cleanup_free_ char *et = NULL; + const char *z; + + z = tpm2_log_event_type_to_string(record->firmware_event_type); + if (z) { + _cleanup_free_ char *b = NULL; + + b = strreplace(z, "-", "_"); + if (!b) + return log_oom(); + + et = strjoin("EV_", ascii_strupper(b)); + if (!et) + return log_oom(); + } else if (asprintf(&et, "%" PRIu32, record->firmware_event_type) < 0) + return log_oom(); + + r = json_build(&fj, JSON_BUILD_OBJECT( + JSON_BUILD_PAIR_STRING("event_type", et), + JSON_BUILD_PAIR_HEX("event_data", record->firmware_payload, record->firmware_payload_size))); + if (r < 0) + return log_error_errno(r, "Failed to build firmware event data: %m"); + + cd = fj; + ct = "pcclient_std"; + } else if (EVENT_LOG_RECORD_IS_USERSPACE(record)) { + cd = record->userspace_content; + ct = "systemd"; + } + + r = json_build(ret, + JSON_BUILD_OBJECT( + JSON_BUILD_PAIR_UNSIGNED("pcr", record->pcr), + JSON_BUILD_PAIR_UNSIGNED("recnum", ++(*recnum)), + JSON_BUILD_PAIR_VARIANT("digests", ja), + JSON_BUILD_PAIR_CONDITION(ct, "content_type", JSON_BUILD_STRING(ct)), + JSON_BUILD_PAIR_CONDITION(cd, "content", JSON_BUILD_VARIANT(cd)))); + if (r < 0) + return log_error_errno(r, "Failed to make CEL record: %m"); + + return 0; +} + static int verb_show_cel(int argc, char *argv[], void *userdata) { _cleanup_(json_variant_unrefp) JsonVariant *array = NULL; _cleanup_(event_log_freep) EventLog *el = NULL; @@ -2429,64 +2498,13 @@ static int verb_show_cel(int argc, char *argv[], void *userdata) { /* Output the event log in TCG CEL-JSON. */ FOREACH_ARRAY(rr, el->records, el->n_records) { - _cleanup_(json_variant_unrefp) JsonVariant *ja = NULL, *fj = NULL; - EventLogRecord *record = *rr; - JsonVariant *cd = NULL; - const char *ct = NULL; + _cleanup_(json_variant_unrefp) JsonVariant *cel = NULL; - LIST_FOREACH(banks, bank, record->banks) { - r = json_variant_append_arrayb( - &ja, JSON_BUILD_OBJECT( - JSON_BUILD_PAIR_STRING("hashAlg", tpm2_hash_alg_to_string(bank->algorithm)), - JSON_BUILD_PAIR_HEX("digest", bank->hash.buffer, bank->hash.size))); - if (r < 0) - return log_error_errno(r, "Failed to append CEL digest entry: %m"); - } + r = event_log_record_to_cel(*rr, &recnum, &cel); + if (r < 0) + return r; - if (!ja) { - r = json_variant_new_array(&ja, NULL, 0); - if (r < 0) - return log_error_errno(r, "Failed to allocate JSON array: %m"); - } - - if (EVENT_LOG_RECORD_IS_FIRMWARE(record)) { - _cleanup_free_ char *et = NULL; - const char *z; - - z = tpm2_log_event_type_to_string(record->firmware_event_type); - if (z) { - _cleanup_free_ char *b = NULL; - - b = strreplace(z, "-", "_"); - if (!b) - return log_oom(); - - et = strjoin("EV_", ascii_strupper(b)); - if (!et) - return log_oom(); - } else if (asprintf(&et, "%" PRIu32, record->firmware_event_type) < 0) - return log_oom(); - - r = json_build(&fj, JSON_BUILD_OBJECT( - JSON_BUILD_PAIR_STRING("event_type", et), - JSON_BUILD_PAIR_HEX("event_data", record->firmware_payload, record->firmware_payload_size))); - if (r < 0) - return log_error_errno(r, "Failed to build firmware event data: %m"); - - cd = fj; - ct = "pcclient_std"; - } else if (EVENT_LOG_RECORD_IS_USERSPACE(record)) { - cd = record->userspace_content; - ct = "systemd"; - } - - r = json_variant_append_arrayb(&array, - JSON_BUILD_OBJECT( - JSON_BUILD_PAIR_UNSIGNED("pcr", record->pcr), - JSON_BUILD_PAIR_UNSIGNED("recnum", ++recnum), - JSON_BUILD_PAIR_VARIANT("digests", ja), - JSON_BUILD_PAIR_CONDITION(ct, "content_type", JSON_BUILD_STRING(ct)), - JSON_BUILD_PAIR_CONDITION(cd, "content", JSON_BUILD_VARIANT(cd)))); + r = json_variant_append_array(&array, cel); if (r < 0) return log_error_errno(r, "Failed to append CEL record: %m"); } From 15138e79804cb595829b51eeda3364ec1d70e813 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 2 Feb 2024 15:17:09 +0100 Subject: [PATCH 03/10] pcrlock: add basic Varlink interface This can be used to make or delete a PCR policy via Varlink. It can also be used to query the current event log in CEL format. --- src/pcrlock/pcrlock.c | 141 +++++++++++++++++++++++- src/shared/meson.build | 1 + src/shared/varlink-io.systemd.PCRLock.c | 24 ++++ src/shared/varlink-io.systemd.PCRLock.h | 6 + src/test/test-varlink-idl.c | 3 + units/meson.build | 9 ++ units/systemd-pcrlock.socket | 25 +++++ units/systemd-pcrlock@.service.in | 21 ++++ 8 files changed, 224 insertions(+), 6 deletions(-) create mode 100644 src/shared/varlink-io.systemd.PCRLock.c create mode 100644 src/shared/varlink-io.systemd.PCRLock.h create mode 100644 units/systemd-pcrlock.socket create mode 100644 units/systemd-pcrlock@.service.in diff --git a/src/pcrlock/pcrlock.c b/src/pcrlock/pcrlock.c index 016d73cc00..1dd3d86a73 100644 --- a/src/pcrlock/pcrlock.c +++ b/src/pcrlock/pcrlock.c @@ -48,6 +48,8 @@ #include "unaligned.h" #include "unit-name.h" #include "utf8.h" +#include "varlink.h" +#include "varlink-io.systemd.PCRLock.h" #include "verbs.h" static PagerFlags arg_pager_flags = 0; @@ -65,6 +67,7 @@ static char *arg_policy_path = NULL; static bool arg_force = false; static BootEntryTokenType arg_entry_token_type = BOOT_ENTRY_TOKEN_AUTO; static char *arg_entry_token = NULL; +static bool arg_varlink = false; STATIC_DESTRUCTOR_REGISTER(arg_components, strv_freep); STATIC_DESTRUCTOR_REGISTER(arg_pcrlock_path, freep); @@ -4310,7 +4313,7 @@ static int write_boot_policy_file(const char *json_text) { return 1; } -static int verb_make_policy(int argc, char *argv[], void *userdata) { +static int make_policy(bool force, bool recovery_pin) { int r; /* Here's how this all works: after predicting all possible PCR values for next boot (with @@ -4385,11 +4388,11 @@ static int verb_make_policy(int argc, char *argv[], void *userdata) { if (arg_nv_index != 0 && old_policy.nv_index != arg_nv_index) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Stored policy references different NV index (0x%x) than specified (0x%x), refusing.", old_policy.nv_index, arg_nv_index); - if (!arg_force && + if (!force && old_policy.algorithm == el->primary_algorithm && tpm2_pcr_prediction_equal(&old_policy.prediction, &new_prediction, el->primary_algorithm)) { log_info("Prediction is identical to current policy, skipping update."); - return EXIT_SUCCESS; + return 0; /* NOP */ } } @@ -4434,7 +4437,7 @@ static int verb_make_policy(int argc, char *argv[], void *userdata) { /* Acquire a recovery PIN, either from the user, or create a randomized one */ _cleanup_(erase_and_freep) char *pin = NULL; - if (arg_recovery_pin) { + if (recovery_pin) { r = getenv_steal_erase("PIN", &pin); if (r < 0) return log_error_errno(r, "Failed to acquire PIN from environment: %m"); @@ -4712,7 +4715,11 @@ static int verb_make_policy(int argc, char *argv[], void *userdata) { log_info("Overall time spent: %s", FORMAT_TIMESPAN(usec_sub_unsigned(now(CLOCK_MONOTONIC), start_usec), 1)); - return 0; + return 1; /* installed new policy */ +} + +static int verb_make_policy(int argc, char *argv[], void *userdata) { + return make_policy(arg_force, arg_recovery_pin); } static int undefine_policy_nv_index( @@ -4768,7 +4775,7 @@ static int undefine_policy_nv_index( return 0; } -static int verb_remove_policy(int argc, char *argv[], void *userdata) { +static int remove_policy(void) { int ret = 0, r; _cleanup_(tpm2_pcrlock_policy_done) Tpm2PCRLockPolicy policy = {}; @@ -4807,6 +4814,10 @@ static int verb_remove_policy(int argc, char *argv[], void *userdata) { return ret; } +static int verb_remove_policy(int argc, char *argv[], void *userdata) { + return remove_policy(); +} + static int help(int argc, char *argv[], void *userdata) { _cleanup_free_ char *link = NULL; int r; @@ -5082,6 +5093,14 @@ static int parse_argv(int argc, char *argv[]) { return log_oom(); } + r = varlink_invocation(VARLINK_ALLOW_ACCEPT); + if (r < 0) + return log_error_errno(r, "Failed to check if invoked in Varlink mode: %m"); + if (r > 0) { + arg_varlink = true; + arg_pager_flags |= PAGER_DISABLE; + } + return 1; } @@ -5124,6 +5143,88 @@ static int pcrlock_main(int argc, char *argv[]) { return dispatch_verb(argc, argv, verbs, NULL); } +static int vl_method_read_event_log(Varlink *link, JsonVariant *parameters, VarlinkMethodFlags flags, void *userdata) { + _cleanup_(event_log_freep) EventLog *el = NULL; + uint64_t recnum = 0; + int r; + + assert(link); + + if (json_variant_elements(parameters) > 0) + return varlink_error_invalid_parameter(link, parameters); + + el = event_log_new(); + if (!el) + return log_oom(); + + r = event_log_load(el); + if (r < 0) + return r; + + _cleanup_(json_variant_unrefp) JsonVariant *rec_cel = NULL; + + FOREACH_ARRAY(rr, el->records, el->n_records) { + + if (rec_cel) { + r = varlink_notifyb(link, + JSON_BUILD_OBJECT(JSON_BUILD_PAIR_VARIANT("record", rec_cel))); + if (r < 0) + return r; + + rec_cel = json_variant_unref(rec_cel); + } + + r = event_log_record_to_cel(*rr, &recnum, &rec_cel); + if (r < 0) + return r; + } + + return varlink_replyb(link, + JSON_BUILD_OBJECT(JSON_BUILD_PAIR_CONDITION(rec_cel, "record", JSON_BUILD_VARIANT(rec_cel)))); +} + +typedef struct MethodMakePolicyParameters { + bool force; +} MethodMakePolicyParameters; + +static int vl_method_make_policy(Varlink *link, JsonVariant *parameters, VarlinkMethodFlags flags, void *userdata) { + static const JsonDispatch dispatch_table[] = { + { "force", JSON_VARIANT_BOOLEAN, json_dispatch_boolean, offsetof(MethodMakePolicyParameters, force), 0 }, + {} + }; + MethodMakePolicyParameters p = {}; + int r; + + assert(link); + + r = varlink_dispatch(link, parameters, dispatch_table, &p); + if (r != 0) + return r; + + r = make_policy(p.force, /* recovery_key= */ false); + if (r < 0) + return r; + if (r == 0) + return varlink_error(link, "io.systemd.PCRLock.NoChange", NULL); + + return varlink_reply(link, NULL); +} + +static int vl_method_remove_policy(Varlink *link, JsonVariant *parameters, VarlinkMethodFlags flags, void *userdata) { + int r; + + assert(link); + + if (json_variant_elements(parameters) > 0) + return varlink_error_invalid_parameter(link, parameters); + + r = remove_policy(); + if (r < 0) + return r; + + return varlink_reply(link, NULL); +} + static int run(int argc, char *argv[]) { int r; @@ -5133,6 +5234,34 @@ static int run(int argc, char *argv[]) { if (r <= 0) return r; + if (arg_varlink) { + _cleanup_(varlink_server_unrefp) VarlinkServer *varlink_server = NULL; + + /* Invocation as Varlink service */ + + r = varlink_server_new(&varlink_server, VARLINK_SERVER_ROOT_ONLY); + if (r < 0) + return log_error_errno(r, "Failed to allocate Varlink server: %m"); + + r = varlink_server_add_interface(varlink_server, &vl_interface_io_systemd_PCRLock); + if (r < 0) + return log_error_errno(r, "Failed to add Varlink interface: %m"); + + r = varlink_server_bind_method_many( + varlink_server, + "io.systemd.PCRLock.ReadEventLog", vl_method_read_event_log, + "io.systemd.PCRLock.MakePolicy", vl_method_make_policy, + "io.systemd.PCRLock.RemovePolicy", vl_method_remove_policy); + if (r < 0) + return log_error_errno(r, "Failed to bind Varlink methods: %m"); + + r = varlink_server_loop_auto(varlink_server); + if (r < 0) + return log_error_errno(r, "Failed to run Varlink event loop: %m"); + + return EXIT_SUCCESS; + } + return pcrlock_main(argc, argv); } diff --git a/src/shared/meson.build b/src/shared/meson.build index dc9adeddc1..81de6708f0 100644 --- a/src/shared/meson.build +++ b/src/shared/meson.build @@ -180,6 +180,7 @@ shared_sources = files( 'varlink-io.systemd.ManagedOOM.c', 'varlink-io.systemd.Network.c', 'varlink-io.systemd.PCRExtend.c', + 'varlink-io.systemd.PCRLock.c', 'varlink-io.systemd.Resolve.c', 'varlink-io.systemd.Resolve.Monitor.c', 'varlink-io.systemd.UserDatabase.c', diff --git a/src/shared/varlink-io.systemd.PCRLock.c b/src/shared/varlink-io.systemd.PCRLock.c new file mode 100644 index 0000000000..3b2c408bc9 --- /dev/null +++ b/src/shared/varlink-io.systemd.PCRLock.c @@ -0,0 +1,24 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ + +#include "varlink-io.systemd.PCRLock.h" + +static VARLINK_DEFINE_METHOD( + ReadEventLog); + +static VARLINK_DEFINE_METHOD( + MakePolicy, + VARLINK_DEFINE_INPUT(force, VARLINK_BOOL, VARLINK_NULLABLE)); + +static VARLINK_DEFINE_METHOD( + RemovePolicy); + +VARLINK_DEFINE_ERROR( + NoChange); + +VARLINK_DEFINE_INTERFACE( + io_systemd_PCRLock, + "io.systemd.PCRLock", + &vl_method_ReadEventLog, + &vl_method_MakePolicy, + &vl_method_RemovePolicy, + &vl_error_NoChange); diff --git a/src/shared/varlink-io.systemd.PCRLock.h b/src/shared/varlink-io.systemd.PCRLock.h new file mode 100644 index 0000000000..687f09ef84 --- /dev/null +++ b/src/shared/varlink-io.systemd.PCRLock.h @@ -0,0 +1,6 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +#pragma once + +#include "varlink-idl.h" + +extern const VarlinkInterface vl_interface_io_systemd_PCRLock; diff --git a/src/test/test-varlink-idl.c b/src/test/test-varlink-idl.c index d322c02c55..e5708b73b5 100644 --- a/src/test/test-varlink-idl.c +++ b/src/test/test-varlink-idl.c @@ -13,6 +13,7 @@ #include "varlink-io.systemd.ManagedOOM.h" #include "varlink-io.systemd.Network.h" #include "varlink-io.systemd.PCRExtend.h" +#include "varlink-io.systemd.PCRLock.h" #include "varlink-io.systemd.Resolve.Monitor.h" #include "varlink-io.systemd.Resolve.h" #include "varlink-io.systemd.UserDatabase.h" @@ -143,6 +144,8 @@ TEST(parse_format) { print_separator(); test_parse_format_one(&vl_interface_io_systemd_PCRExtend); print_separator(); + test_parse_format_one(&vl_interface_io_systemd_PCRLock); + print_separator(); test_parse_format_one(&vl_interface_io_systemd_service); print_separator(); test_parse_format_one(&vl_interface_io_systemd_sysext); diff --git a/units/meson.build b/units/meson.build index efd2eac583..acfd8d1dcb 100644 --- a/units/meson.build +++ b/units/meson.build @@ -519,6 +519,15 @@ units = [ 'file' : 'systemd-pcrlock-firmware-config.service.in', 'conditions' : ['ENABLE_BOOTLOADER', 'HAVE_OPENSSL', 'HAVE_TPM2'], }, + { + 'file' : 'systemd-pcrlock@.service.in', + 'conditions' : ['ENABLE_BOOTLOADER', 'HAVE_OPENSSL', 'HAVE_TPM2'], + }, + { + 'file' : 'systemd-pcrlock.socket', + 'conditions' : ['ENABLE_BOOTLOADER', 'HAVE_OPENSSL', 'HAVE_TPM2'], + 'symlinks' : ['sockets.target.wants/'], + }, { 'file' : 'systemd-portabled.service.in', 'conditions' : ['ENABLE_PORTABLED'], diff --git a/units/systemd-pcrlock.socket b/units/systemd-pcrlock.socket new file mode 100644 index 0000000000..21431478b8 --- /dev/null +++ b/units/systemd-pcrlock.socket @@ -0,0 +1,25 @@ +# SPDX-License-Identifier: LGPL-2.1-or-later +# +# This file is part of systemd. +# +# systemd is free software; you can redistribute it and/or modify it +# under the terms of the GNU Lesser General Public License as published by +# the Free Software Foundation; either version 2.1 of the License, or +# (at your option) any later version. + +[Unit] +Description=Make TPM2 PCR Policy (Varlink) +Documentation=man:systemd-pcrlock(8) +DefaultDependencies=no +After=tpm2.target +Before=sockets.target +ConditionSecurity=measured-uki + +[Socket] +ListenStream=/run/systemd/io.systemd.PCRLock +FileDescriptorName=varlink +SocketMode=0600 +Accept=yes + +[Install] +WantedBy=sockets.target diff --git a/units/systemd-pcrlock@.service.in b/units/systemd-pcrlock@.service.in new file mode 100644 index 0000000000..50a0a3de8e --- /dev/null +++ b/units/systemd-pcrlock@.service.in @@ -0,0 +1,21 @@ +# SPDX-License-Identifier: LGPL-2.1-or-later +# +# This file is part of systemd. +# +# systemd is free software; you can redistribute it and/or modify it +# under the terms of the GNU Lesser General Public License as published by +# the Free Software Foundation; either version 2.1 of the License, or +# (at your option) any later version. + +[Unit] +Description=Make TPM2 PCR Policy (Varlink) +Documentation=man:systemd-pcrlock(8) +DefaultDependencies=no +Conflicts=shutdown.target +After=systemd-tpm2-setup.service +Before=sysinit.target shutdown.target +After=systemd-remount-fs.service var.mount + +[Service] +Environment=LISTEN_FDNAMES=varlink +ExecStart={{LIBEXECDIR}}/systemd-pcrlock --location=770 From 0430a11eb41f07788bbeb75f327379acec56fd31 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 7 Feb 2024 11:41:20 +0100 Subject: [PATCH 04/10] varlink: enforce a maximum size limit on replies collected via varlink_collect() We should not allow servers to blow up client's memory without bounds, hence set a (high) limit on replies we'll collect before failing. --- src/shared/varlink.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/shared/varlink.c b/src/shared/varlink.c index 1e1e4d48f9..80e239bf78 100644 --- a/src/shared/varlink.c +++ b/src/shared/varlink.c @@ -37,6 +37,7 @@ #define VARLINK_DEFAULT_TIMEOUT_USEC (45U*USEC_PER_SEC) #define VARLINK_BUFFER_MAX (16U*1024U*1024U) #define VARLINK_READ_SIZE (64U*1024U) +#define VARLINK_COLLECT_MAX 1024U typedef enum VarlinkState { /* Client side states */ @@ -2348,6 +2349,9 @@ static int collect_callback( return 0; } + if (json_variant_elements(context->parameters) >= VARLINK_COLLECT_MAX) + return varlink_log_errno(v, SYNTHETIC_ERRNO(E2BIG), "Number of reply messages grew too large (%zu) while collecting.", json_variant_elements(context->parameters)); + r = json_variant_append_array(&context->parameters, parameters); if (r < 0) return varlink_log_errno(v, r, "Failed to append JSON object to array: %m"); From 72226a2f95aefb9b97baebb68fbac62844eb1575 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 8 Feb 2024 11:33:15 +0100 Subject: [PATCH 05/10] varlink: properly return reply flags to callers We so far have a reply flags return parameter on varlink_call_full(), but we return 0 always. Let's fix that, and return the actual flags we see. --- src/shared/varlink.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/shared/varlink.c b/src/shared/varlink.c index 80e239bf78..10d4da1a79 100644 --- a/src/shared/varlink.c +++ b/src/shared/varlink.c @@ -170,6 +170,7 @@ struct Varlink { VarlinkReply reply_callback; JsonVariant *current; + VarlinkReplyFlags current_reply_flags; VarlinkSymbol *current_method; int peer_pidfd; @@ -690,6 +691,7 @@ static void varlink_clear_current(Varlink *v) { /* Clears the currently processed incoming message */ v->current = json_variant_unref(v->current); v->current_method = NULL; + v->current_reply_flags = 0; close_many(v->input_fds, v->n_input_fds); v->input_fds = mfree(v->input_fds); @@ -1235,6 +1237,8 @@ static int varlink_dispatch_reply(Varlink *v) { if (r < 0) goto invalid; + v->current_reply_flags = flags; + if (IN_SET(v->state, VARLINK_AWAITING_REPLY, VARLINK_AWAITING_REPLY_MORE)) { varlink_set_state(v, VARLINK_PROCESSING_REPLY); @@ -1247,7 +1251,6 @@ static int varlink_dispatch_reply(Varlink *v) { varlink_clear_current(v); if (v->state == VARLINK_PROCESSING_REPLY) { - assert(v->n_pending > 0); if (!FLAGS_SET(flags, VARLINK_REPLY_CONTINUES)) @@ -2200,7 +2203,6 @@ int varlink_call_full( v->timestamp = now(CLOCK_MONOTONIC); while (v->state == VARLINK_CALLING) { - r = varlink_process(v); if (r < 0) return r; @@ -2233,7 +2235,7 @@ int varlink_call_full( if (ret_error_id) *ret_error_id = e ? json_variant_string(e) : NULL; if (ret_flags) - *ret_flags = 0; + *ret_flags = v->current_reply_flags; return 1; } From 9bca9891834e0e71484e2e1b38ac59b3ce257043 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 8 Feb 2024 11:34:49 +0100 Subject: [PATCH 06/10] varlink: rework varlink_collect() This reworks varlink_collect() so that it is not just a wrapper around varlink_observe(), varlink_bind_reply() and others. It becomes a first class operation. This has various benefits: 1. Memory management is normalized: the reply json variant is now tracked as part of the varlink object, and thus we do not pass ownership to the caller. This is just like we do it for simple method calls and removes a lot of confusion. 2. The bind reply/user data pointer can be used for user stuff, we'll not silently override this. 3. We enforce an overall time-out operation on the whole thing, so that this synchronous operation does no longer block forever. --- src/shared/varlink.c | 221 ++++++++++++++++++++-------------------- src/shared/varlink.h | 7 +- src/test/test-varlink.c | 8 +- 3 files changed, 116 insertions(+), 120 deletions(-) diff --git a/src/shared/varlink.c b/src/shared/varlink.c index 10d4da1a79..8a5a9907a8 100644 --- a/src/shared/varlink.c +++ b/src/shared/varlink.c @@ -46,6 +46,8 @@ typedef enum VarlinkState { VARLINK_AWAITING_REPLY_MORE, VARLINK_CALLING, VARLINK_CALLED, + VARLINK_COLLECTING, + VARLINK_COLLECTING_REPLY, VARLINK_PROCESSING_REPLY, /* Server side states */ @@ -80,6 +82,8 @@ typedef enum VarlinkState { VARLINK_AWAITING_REPLY_MORE, \ VARLINK_CALLING, \ VARLINK_CALLED, \ + VARLINK_COLLECTING, \ + VARLINK_COLLECTING_REPLY, \ VARLINK_PROCESSING_REPLY, \ VARLINK_IDLE_SERVER, \ VARLINK_PROCESSING_METHOD, \ @@ -170,6 +174,7 @@ struct Varlink { VarlinkReply reply_callback; JsonVariant *current; + JsonVariant *current_collected; VarlinkReplyFlags current_reply_flags; VarlinkSymbol *current_method; @@ -245,18 +250,14 @@ struct VarlinkServer { bool exit_on_idle; }; -typedef struct VarlinkCollectContext { - JsonVariant *parameters; - const char *error_id; - VarlinkReplyFlags flags; -} VarlinkCollectContext ; - static const char* const varlink_state_table[_VARLINK_STATE_MAX] = { [VARLINK_IDLE_CLIENT] = "idle-client", [VARLINK_AWAITING_REPLY] = "awaiting-reply", [VARLINK_AWAITING_REPLY_MORE] = "awaiting-reply-more", [VARLINK_CALLING] = "calling", [VARLINK_CALLED] = "called", + [VARLINK_COLLECTING] = "collecting", + [VARLINK_COLLECTING_REPLY] = "collecting-reply", [VARLINK_PROCESSING_REPLY] = "processing-reply", [VARLINK_IDLE_SERVER] = "idle-server", [VARLINK_PROCESSING_METHOD] = "processing-method", @@ -690,6 +691,7 @@ static void varlink_clear_current(Varlink *v) { /* Clears the currently processed incoming message */ v->current = json_variant_unref(v->current); + v->current_collected = json_variant_unref(v->current_collected); v->current_method = NULL; v->current_reply_flags = 0; @@ -774,7 +776,7 @@ static int varlink_test_disconnect(Varlink *v) { goto disconnect; /* If we are waiting for incoming data but the read side is shut down, disconnect. */ - if (IN_SET(v->state, VARLINK_AWAITING_REPLY, VARLINK_AWAITING_REPLY_MORE, VARLINK_CALLING, VARLINK_IDLE_SERVER) && v->read_disconnected) + if (IN_SET(v->state, VARLINK_AWAITING_REPLY, VARLINK_AWAITING_REPLY_MORE, VARLINK_CALLING, VARLINK_COLLECTING, VARLINK_IDLE_SERVER) && v->read_disconnected) goto disconnect; /* Similar, if are a client that hasn't written anything yet but the write side is dead, also @@ -897,7 +899,7 @@ static int varlink_read(Varlink *v) { assert(v); - if (!IN_SET(v->state, VARLINK_AWAITING_REPLY, VARLINK_AWAITING_REPLY_MORE, VARLINK_CALLING, VARLINK_IDLE_SERVER)) + if (!IN_SET(v->state, VARLINK_AWAITING_REPLY, VARLINK_AWAITING_REPLY_MORE, VARLINK_CALLING, VARLINK_COLLECTING, VARLINK_IDLE_SERVER)) return 0; if (v->connecting) /* read() on a socket while we are in connect() will fail with EINVAL, hence exit early here */ return 0; @@ -1093,7 +1095,7 @@ static int varlink_parse_message(Varlink *v) { static int varlink_test_timeout(Varlink *v) { assert(v); - if (!IN_SET(v->state, VARLINK_AWAITING_REPLY, VARLINK_AWAITING_REPLY_MORE, VARLINK_CALLING)) + if (!IN_SET(v->state, VARLINK_AWAITING_REPLY, VARLINK_AWAITING_REPLY_MORE, VARLINK_CALLING, VARLINK_COLLECTING)) return 0; if (v->timeout == USEC_INFINITY) return 0; @@ -1183,7 +1185,7 @@ static int varlink_dispatch_reply(Varlink *v) { assert(v); - if (!IN_SET(v->state, VARLINK_AWAITING_REPLY, VARLINK_AWAITING_REPLY_MORE, VARLINK_CALLING)) + if (!IN_SET(v->state, VARLINK_AWAITING_REPLY, VARLINK_AWAITING_REPLY_MORE, VARLINK_CALLING, VARLINK_COLLECTING)) return 0; if (!v->current) return 0; @@ -1226,7 +1228,7 @@ static int varlink_dispatch_reply(Varlink *v) { } /* Replies with 'continue' set are only OK if we set 'more' when the method call was initiated */ - if (v->state != VARLINK_AWAITING_REPLY_MORE && FLAGS_SET(flags, VARLINK_REPLY_CONTINUES)) + if (!IN_SET(v->state, VARLINK_AWAITING_REPLY_MORE, VARLINK_COLLECTING) && FLAGS_SET(flags, VARLINK_REPLY_CONTINUES)) goto invalid; /* An error is final */ @@ -1260,7 +1262,9 @@ static int varlink_dispatch_reply(Varlink *v) { FLAGS_SET(flags, VARLINK_REPLY_CONTINUES) ? VARLINK_AWAITING_REPLY_MORE : v->n_pending == 0 ? VARLINK_IDLE_CLIENT : VARLINK_AWAITING_REPLY); } - } else { + } else if (v->state == VARLINK_COLLECTING) + varlink_set_state(v, VARLINK_COLLECTING_REPLY); + else { assert(v->state == VARLINK_CALLING); varlink_set_state(v, VARLINK_CALLED); } @@ -1738,7 +1742,7 @@ int varlink_get_events(Varlink *v) { return EPOLLOUT; if (!v->read_disconnected && - IN_SET(v->state, VARLINK_AWAITING_REPLY, VARLINK_AWAITING_REPLY_MORE, VARLINK_CALLING, VARLINK_IDLE_SERVER) && + IN_SET(v->state, VARLINK_AWAITING_REPLY, VARLINK_AWAITING_REPLY_MORE, VARLINK_CALLING, VARLINK_COLLECTING, VARLINK_IDLE_SERVER) && !v->current && v->input_buffer_unscanned <= 0) ret |= EPOLLIN; @@ -1756,7 +1760,7 @@ int varlink_get_timeout(Varlink *v, usec_t *ret) { if (v->state == VARLINK_DISCONNECTED) return varlink_log_errno(v, SYNTHETIC_ERRNO(ENOTCONN), "Not connected."); - if (IN_SET(v->state, VARLINK_AWAITING_REPLY, VARLINK_AWAITING_REPLY_MORE, VARLINK_CALLING) && + if (IN_SET(v->state, VARLINK_AWAITING_REPLY, VARLINK_AWAITING_REPLY_MORE, VARLINK_CALLING, VARLINK_COLLECTING) && v->timeout != USEC_INFINITY) { if (ret) *ret = usec_add(v->timestamp, v->timeout); @@ -2321,47 +2325,7 @@ int varlink_callb_and_log( return varlink_call_and_log(v, method, parameters, ret_parameters); } -static void varlink_collect_context_free(VarlinkCollectContext *cc) { - assert(cc); - - json_variant_unref(cc->parameters); - free((char *)cc->error_id); -} - -static int collect_callback( - Varlink *v, - JsonVariant *parameters, - const char *error_id, - VarlinkReplyFlags flags, - void *userdata) { - - VarlinkCollectContext *context = ASSERT_PTR(userdata); - int r; - - assert(v); - - context->flags = flags; - /* If we hit an error, we will drop all collected replies and just return the error_id and flags in varlink_collect() */ - if (error_id) { - context->error_id = error_id; - - json_variant_unref(context->parameters); - context->parameters = json_variant_ref(parameters); - - return 0; - } - - if (json_variant_elements(context->parameters) >= VARLINK_COLLECT_MAX) - return varlink_log_errno(v, SYNTHETIC_ERRNO(E2BIG), "Number of reply messages grew too large (%zu) while collecting.", json_variant_elements(context->parameters)); - - r = json_variant_append_array(&context->parameters, parameters); - if (r < 0) - return varlink_log_errno(v, r, "Failed to append JSON object to array: %m"); - - return 1; -} - -int varlink_collect( +int varlink_collect_full( Varlink *v, const char *method, JsonVariant *parameters, @@ -2369,7 +2333,7 @@ int varlink_collect( const char **ret_error_id, VarlinkReplyFlags *ret_flags) { - _cleanup_(varlink_collect_context_free) VarlinkCollectContext context = {}; + _cleanup_(json_variant_unrefp) JsonVariant *m = NULL, *collected = NULL; int r; assert_return(v, -EINVAL); @@ -2386,71 +2350,102 @@ int varlink_collect( * that we can assign a new reply shortly. */ varlink_clear_current(v); - r = varlink_bind_reply(v, collect_callback); + r = varlink_sanitize_parameters(¶meters); if (r < 0) - return varlink_log_errno(v, r, "Failed to bind collect callback"); + return varlink_log_errno(v, r, "Failed to sanitize parameters: %m"); - varlink_set_userdata(v, &context); - r = varlink_observe(v, method, parameters); + r = json_build(&m, JSON_BUILD_OBJECT( + JSON_BUILD_PAIR("method", JSON_BUILD_STRING(method)), + JSON_BUILD_PAIR("parameters", JSON_BUILD_VARIANT(parameters)), + JSON_BUILD_PAIR("more", JSON_BUILD_BOOLEAN(true)))); if (r < 0) - return varlink_log_errno(v, r, "Failed to collect varlink method: %m"); + return varlink_log_errno(v, r, "Failed to build json message: %m"); - while (v->state == VARLINK_AWAITING_REPLY_MORE) { + r = varlink_enqueue_json(v, m); + if (r < 0) + return varlink_log_errno(v, r, "Failed to enqueue json message: %m"); - r = varlink_process(v); - if (r < 0) - return r; + varlink_set_state(v, VARLINK_COLLECTING); + v->n_pending++; + v->timestamp = now(CLOCK_MONOTONIC); - /* If we get an error from any of the replies, return immediately with just the error_id and flags*/ - if (context.error_id) { + for (;;) { + while (v->state == VARLINK_COLLECTING) { + r = varlink_process(v); + if (r < 0) + return r; + if (r > 0) + continue; - /* If caller doesn't ask for the error string, then let's return an error code in case of failure */ - if (!ret_error_id) - return varlink_error_to_errno(context.error_id, context.parameters); - - if (ret_parameters) - *ret_parameters = TAKE_PTR(context.parameters); - if (ret_error_id) - *ret_error_id = TAKE_PTR(context.error_id); - if (ret_flags) - *ret_flags = context.flags; - return 0; + r = varlink_wait(v, USEC_INFINITY); + if (r < 0) + return r; } - if (r > 0) - continue; + switch (v->state) { - r = varlink_wait(v, USEC_INFINITY); - if (r < 0) - return r; + case VARLINK_COLLECTING_REPLY: { + assert(v->current); + + JsonVariant *e = json_variant_by_key(v->current, "error"), + *p = json_variant_by_key(v->current, "parameters"); + + if (e) { + if (!ret_error_id) + return varlink_error_to_errno(json_variant_string(e), p); + + if (ret_parameters) + *ret_parameters = p; + if (ret_error_id) + *ret_error_id = e ? json_variant_string(e) : NULL; + if (ret_flags) + *ret_flags = v->current_reply_flags; + + return 1; + } + + if (json_variant_elements(collected) >= VARLINK_COLLECT_MAX) + return varlink_log_errno(v, SYNTHETIC_ERRNO(E2BIG), "Number of reply messages grew too large (%zu) while collecting.", json_variant_elements(collected)); + + r = json_variant_append_array(&collected, p); + if (r < 0) + return varlink_log_errno(v, r, "Failed to append JSON object to array: %m"); + + if (FLAGS_SET(v->current_reply_flags, VARLINK_REPLY_CONTINUES)) { + /* There's more to collect, continue */ + varlink_clear_current(v); + varlink_set_state(v, VARLINK_COLLECTING); + continue; + } + + varlink_set_state(v, VARLINK_IDLE_CLIENT); + assert(v->n_pending == 1); + v->n_pending--; + + if (ret_parameters) + /* Install the collection array in the connection object, so that we can hand + * out a pointer to it without passing over ownership, to make it work more + * alike regular method call replies */ + *ret_parameters = v->current_collected = TAKE_PTR(collected); + if (ret_error_id) + *ret_error_id = NULL; + if (ret_flags) + *ret_flags = v->current_reply_flags; + + return 1; + } + + case VARLINK_PENDING_DISCONNECT: + case VARLINK_DISCONNECTED: + return varlink_log_errno(v, SYNTHETIC_ERRNO(ECONNRESET), "Connection was closed."); + + case VARLINK_PENDING_TIMEOUT: + return varlink_log_errno(v, SYNTHETIC_ERRNO(ETIME), "Connection timed out."); + + default: + assert_not_reached(); + } } - - switch (v->state) { - - case VARLINK_IDLE_CLIENT: - break; - - case VARLINK_PENDING_DISCONNECT: - case VARLINK_DISCONNECTED: - return varlink_log_errno(v, SYNTHETIC_ERRNO(ECONNRESET), "Connection was closed."); - - case VARLINK_PENDING_TIMEOUT: - return varlink_log_errno(v, SYNTHETIC_ERRNO(ETIME), "Connection timed out."); - - default: - assert_not_reached(); - } - - if (!ret_error_id && context.error_id) - return varlink_error_to_errno(context.error_id, context.parameters); - - if (ret_parameters) - *ret_parameters = TAKE_PTR(context.parameters); - if (ret_error_id) - *ret_error_id = TAKE_PTR(context.error_id); - if (ret_flags) - *ret_flags = context.flags; - return 1; } int varlink_collectb( @@ -2458,7 +2453,7 @@ int varlink_collectb( const char *method, JsonVariant **ret_parameters, const char **ret_error_id, - VarlinkReplyFlags *ret_flags, ...) { + ...) { _cleanup_(json_variant_unrefp) JsonVariant *parameters = NULL; va_list ap; @@ -2466,14 +2461,14 @@ int varlink_collectb( assert_return(v, -EINVAL); - va_start(ap, ret_flags); + va_start(ap, ret_error_id); r = json_buildv(¶meters, ap); va_end(ap); if (r < 0) return varlink_log_errno(v, r, "Failed to build json message: %m"); - return varlink_collect(v, method, parameters, ret_parameters, ret_error_id, ret_flags); + return varlink_collect_full(v, method, parameters, ret_parameters, ret_error_id, NULL); } int varlink_reply(Varlink *v, JsonVariant *parameters) { diff --git a/src/shared/varlink.h b/src/shared/varlink.h index 622ab797c5..db7227b215 100644 --- a/src/shared/varlink.h +++ b/src/shared/varlink.h @@ -116,8 +116,11 @@ static inline int varlink_callb(Varlink *v, const char *method, JsonVariant **re int varlink_callb_and_log(Varlink *v, const char *method, JsonVariant **ret_parameters, ...); /* Send method call and begin collecting all 'more' replies into an array, finishing when a final reply is sent */ -int varlink_collect(Varlink *v, const char *method, JsonVariant *parameters, JsonVariant **ret_parameters, const char **ret_error_id, VarlinkReplyFlags *ret_flags); -int varlink_collectb(Varlink *v, const char *method, JsonVariant **ret_parameters, const char **ret_error_id, VarlinkReplyFlags *ret_flags, ...); +int varlink_collect_full(Varlink *v, const char *method, JsonVariant *parameters, JsonVariant **ret_parameters, const char **ret_error_id, VarlinkReplyFlags *ret_flags); +static inline int varlink_collect(Varlink *v, const char *method, JsonVariant *parameters, JsonVariant **ret_parameters, const char **ret_error_id) { + return varlink_collect_full(v, method, parameters, ret_parameters, ret_error_id, NULL); +} +int varlink_collectb(Varlink *v, const char *method, JsonVariant **ret_parameters, const char **ret_error_id, ...); /* Enqueue method call, expect a reply, which is eventually delivered to the reply callback */ int varlink_invoke(Varlink *v, const char *method, JsonVariant *parameters); diff --git a/src/test/test-varlink.c b/src/test/test-varlink.c index b0b244e917..67ad213002 100644 --- a/src/test/test-varlink.c +++ b/src/test/test-varlink.c @@ -238,10 +238,9 @@ static void flood_test(const char *address) { static void *thread(void *arg) { _cleanup_(varlink_flush_close_unrefp) Varlink *c = NULL; - _cleanup_(json_variant_unrefp) JsonVariant *i = NULL, *j = NULL; - JsonVariant *o = NULL, *k = NULL; + _cleanup_(json_variant_unrefp) JsonVariant *i = NULL; + JsonVariant *o = NULL, *k = NULL, *j = NULL; const char *error_id; - VarlinkReplyFlags flags = 0; const char *e; int x = 0; @@ -253,10 +252,9 @@ static void *thread(void *arg) { assert_se(varlink_set_allow_fd_passing_input(c, true) >= 0); assert_se(varlink_set_allow_fd_passing_output(c, true) >= 0); - assert_se(varlink_collect(c, "io.test.DoSomethingMore", i, &j, &error_id, &flags) >= 0); + assert_se(varlink_collect(c, "io.test.DoSomethingMore", i, &j, &error_id) >= 0); assert_se(!error_id); - assert_se(!flags); assert_se(json_variant_is_array(j) && !json_variant_is_blank_array(j)); JSON_VARIANT_ARRAY_FOREACH(k, j) { From 7781d28fd75b93e4d1c87644b23b95ea382c5ae7 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 7 Feb 2024 11:20:05 +0100 Subject: [PATCH 07/10] varlinkctl: add new --collect switch It exposes the varlink_collect() call we internally provide: it collects all responses of a method call that is issued with the "more" method call flag. It then returns the result as a single JSON array. --- man/varlinkctl.xml | 9 +++++++++ src/varlinkctl/varlinkctl.c | 29 ++++++++++++++++++++++++++++- 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/man/varlinkctl.xml b/man/varlinkctl.xml index eff49af349..646fea0d24 100644 --- a/man/varlinkctl.xml +++ b/man/varlinkctl.xml @@ -184,6 +184,15 @@ + + + + This is similar to but collects all responses in a JSON + array, and prints it, rather than in JSON_SEQ mode. + + + + diff --git a/src/varlinkctl/varlinkctl.c b/src/varlinkctl/varlinkctl.c index 8f3a88aba5..714396d387 100644 --- a/src/varlinkctl/varlinkctl.c +++ b/src/varlinkctl/varlinkctl.c @@ -19,6 +19,7 @@ static JsonFormatFlags arg_json_format_flags = JSON_FORMAT_OFF; static PagerFlags arg_pager_flags = 0; static VarlinkMethodFlags arg_method_flags = 0; +static bool arg_collect = false; static int help(void) { _cleanup_free_ char *link = NULL; @@ -47,6 +48,7 @@ static int help(void) { " --version Show package version\n" " --no-pager Do not pipe output into a pager\n" " --more Request multiple responses\n" + " --collect Collect multiple responses in a JSON array\n" " --oneway Do not request response\n" " --json=MODE Output as JSON\n" " -j Same as --json=pretty on tty, --json=short otherwise\n" @@ -73,6 +75,7 @@ static int parse_argv(int argc, char *argv[]) { ARG_MORE, ARG_ONEWAY, ARG_JSON, + ARG_COLLECT, }; static const struct option options[] = { @@ -82,6 +85,7 @@ static int parse_argv(int argc, char *argv[]) { { "more", no_argument, NULL, ARG_MORE }, { "oneway", no_argument, NULL, ARG_ONEWAY }, { "json", required_argument, NULL, ARG_JSON }, + { "collect", no_argument, NULL, ARG_COLLECT }, {}, }; @@ -112,6 +116,10 @@ static int parse_argv(int argc, char *argv[]) { arg_method_flags = (arg_method_flags & ~VARLINK_METHOD_MORE) | VARLINK_METHOD_ONEWAY; break; + case ARG_COLLECT: + arg_collect = true; + break; + case ARG_JSON: r = parse_json_argument(optarg, &arg_json_format_flags); if (r <= 0) @@ -388,7 +396,26 @@ static int verb_call(int argc, char *argv[], void *userdata) { if (r < 0) return r; - if (arg_method_flags & VARLINK_METHOD_ONEWAY) { + if (arg_collect) { + JsonVariant *reply = NULL; + const char *error = NULL; + + r = varlink_collect(vl, method, jp, &reply, &error); + if (r < 0) + return log_error_errno(r, "Failed to issue %s() call: %m", method); + if (error) { + /* Propagate the error we received via sd_notify() */ + (void) sd_notifyf(/* unset_environment= */ false, "VARLINKERROR=%s", error); + + r = log_error_errno(SYNTHETIC_ERRNO(EBADE), "Method call %s() failed: %s", method, error); + } else + r = 0; + + pager_open(arg_pager_flags); + json_variant_dump(reply, arg_json_format_flags, stdout, NULL); + return r; + + } else if (arg_method_flags & VARLINK_METHOD_ONEWAY) { r = varlink_send(vl, method, jp); if (r < 0) return log_error_errno(r, "Failed to issue %s() call: %m", method); From 24835e9933ee1dc26876eb4e33adb4769520ddc3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 7 Feb 2024 11:45:33 +0100 Subject: [PATCH 08/10] varlinkctl: if "call" verb is used, imply "-j" For the other verbs turning off JSON mode makes sense, but for "call" not so much, after all the contents of a method call reply is JSON we couldn't really show any other way. Hence, when JSON output was not configured otherwise in "call", default to the same as -j. --- src/varlinkctl/varlinkctl.c | 8 +++++++- test/units/testsuite-54.sh | 4 ++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/varlinkctl/varlinkctl.c b/src/varlinkctl/varlinkctl.c index 714396d387..26b764aca6 100644 --- a/src/varlinkctl/varlinkctl.c +++ b/src/varlinkctl/varlinkctl.c @@ -379,7 +379,13 @@ static int verb_call(int argc, char *argv[], void *userdata) { method = argv[2]; parameter = argc > 3 && !streq(argv[3], "-") ? argv[3] : NULL; - arg_json_format_flags &= ~JSON_FORMAT_OFF; + /* No JSON mode explicitly configured? Then default to the same as -j */ + if (FLAGS_SET(arg_json_format_flags, JSON_FORMAT_OFF)) + arg_json_format_flags = JSON_FORMAT_PRETTY_AUTO|JSON_FORMAT_COLOR_AUTO; + + /* For pipeable text tools it's kinda customary to finish output off in a newline character, and not + * leave incomplete lines hanging around. */ + arg_json_format_flags |= JSON_FORMAT_NEWLINE; if (parameter) { /* is correct, as dispatch_verb() shifts arguments by one for the verb. */ diff --git a/test/units/testsuite-54.sh b/test/units/testsuite-54.sh index 8f20ac4328..7618e92c9f 100755 --- a/test/units/testsuite-54.sh +++ b/test/units/testsuite-54.sh @@ -348,10 +348,10 @@ fi # Decrypt/encrypt via varlink -echo -n '{"data":"Zm9vYmFyCg=="}' > /tmp/vlcredsdata +echo '{"data":"Zm9vYmFyCg=="}' > /tmp/vlcredsdata varlinkctl call /run/systemd/io.systemd.Credentials io.systemd.Credentials.Encrypt "$(cat /tmp/vlcredsdata)" | \ - varlinkctl call /run/systemd/io.systemd.Credentials io.systemd.Credentials.Decrypt > /tmp/vlcredsdata2 + varlinkctl call --json=short /run/systemd/io.systemd.Credentials io.systemd.Credentials.Decrypt > /tmp/vlcredsdata2 cmp /tmp/vlcredsdata /tmp/vlcredsdata2 rm /tmp/vlcredsdata /tmp/vlcredsdata2 From d02018afdb15ca84f02775c5c5ed0ade39475ffa Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 7 Feb 2024 11:51:11 +0100 Subject: [PATCH 09/10] test: add brief test for prclock varlink interfaces and varlinkctl --collect --- test/units/testsuite-70.pcrlock.sh | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/units/testsuite-70.pcrlock.sh b/test/units/testsuite-70.pcrlock.sh index cd031b4ed6..a9c5e9cd5e 100755 --- a/test/units/testsuite-70.pcrlock.sh +++ b/test/units/testsuite-70.pcrlock.sh @@ -156,4 +156,20 @@ SYSTEMD_XBOOTLDR_PATH=/tmp/fakexbootldr SYSTEMD_RELAX_XBOOTLDR_CHECKS=1 "$SD_PCR (! "$SD_PCRLOCK" lock-uki /bin/true) (! "$SD_PCRLOCK" lock-file-system "") +# Excercise Varlink API a bit (but first turn off condition) + +mkdir -p /run/systemd/system/systemd-pcrlock.socket.d +cat > /run/systemd/system/systemd-pcrlock.socket.d/50-no-condition.conf < Date: Wed, 7 Feb 2024 11:41:59 +0100 Subject: [PATCH 10/10] update TODO --- TODO | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/TODO b/TODO index 1be6f2e4b3..97c84f7a23 100644 --- a/TODO +++ b/TODO @@ -132,6 +132,8 @@ Deprecations and removals: Features: +* varlink: extend varlink IDL macros to include documentation strings + * Get rid of the symlinks in /run/systemd/units/* and exclusively use cgroupfs xattrs to convey info about invocation ids, logging settings and so on. support for cgroupfs xattrs in the "trusted." namespace was added in linux @@ -335,7 +337,6 @@ Features: - systemd-dissect - systemd-sysupdate - systemd-analyze - - systemd-pcrlock (to allow fwupd to relax policy) - kernel-install - systemd-mount (with PK so that desktop environments could use it to mount disks)