From ad49459152d2934b70471b4ade5f4b12009a55ae Mon Sep 17 00:00:00 2001 From: Daniel Donoghue Date: Fri, 28 Aug 2026 13:33:17 +0200 Subject: [PATCH] MT#55283 address review comments Snapshots are now bencode, GLib macros are used throughout, and keys and offsets are built with the existing helpers rather than by hand. New tests cover the snapshot format and the state that exists only inside a snapshot. --- daemon/redis.c | 23 +++++----- t/auto-daemon-tests-rollback-redis.pl | 66 ++++++++++++++++++++++++++- 2 files changed, 77 insertions(+), 12 deletions(-) diff --git a/daemon/redis.c b/daemon/redis.c index 022e616bf..3cfdd509c 100644 --- a/daemon/redis.c +++ b/daemon/redis.c @@ -1259,6 +1259,8 @@ static int redis_hash_get_endpoint(struct endpoint *out, const struct redis_hash return 0; } +define_get_type_format(endpoint, struct endpoint); + static int redis_hash_get_stats(struct stream_stats *out, const struct redis_hash *h, const char *k) { if (redis_hash_get_a64_f(&out->packets, h, "%s-packets", k)) return -1; @@ -2154,9 +2156,7 @@ static int redis_restore_checkpoints(call_t *c, const struct redis_hash *call, for (int64_t i = 0; i < num; i++) { struct redis_hash rh; - char key[64]; - snprintf(key, sizeof(key), "checkpoint-%lld", (long long) i); - if (json_get_hash(&rh, key, -1, root)) + if (json_get_hash(&rh, "checkpoint", (unsigned int) i, root)) return -1; int64_t offerer = -1, answerer = -1, pending = 0; @@ -3108,7 +3108,8 @@ static str redis_encode_json(ng_parser_ctx_t *ctx, call_t *c, void **to_free, bo str redis_snapshot_encode(call_t *c) { ng_parser_ctx_t ctx; bencode_buffer_t bbuf; - redis_format_parsers[rtpe_config.redis_format]->init(&ctx, &bbuf); + // never leaves the daemon, so the format is ours to pick + ng_parser_native.init(&ctx, &bbuf); void *to_free = NULL; str encoded = redis_encode_json(&ctx, c, &to_free, true); @@ -3209,10 +3210,10 @@ static void snapshot_apply_stream(call_t *c, struct packet_stream *ps, redis_hash_get_endpoint(&ps->last_local_endpoint, rh, "last_local_endpoint"); for (unsigned int i = 0; i < G_N_ELEMENTS(ps->detected_endpoints); i++) { - char key[32]; - snprintf(key, sizeof(key), "detected_endpoint-%u", i); - if (!redis_hash_get_str(&s, rh, key) && s.len) - redis_hash_get_endpoint(&ps->detected_endpoints[i], rh, key); + /* an absent endpoint is an empty string, which would parse as 0.0.0.0:0 */ + if (!redis_hash_get_str_f(&s, rh, "detected_endpoint-%u", i) && s.len) + redis_hash_get_endpoint_f(&ps->detected_endpoints[i], rh, + "detected_endpoint-%u", i); else ZERO(ps->detected_endpoints[i]); } @@ -3248,8 +3249,8 @@ static void snapshot_apply_media_codecs(struct call_media *m, parser_arg root) { const char *key; size_t offset; } stores[] = { - { "payload_types-%u", offsetof(struct call_media, codecs) }, - { "offered_payload_types-%u", offsetof(struct call_media, offered_codecs) }, + { "payload_types-%u", G_STRUCT_OFFSET(struct call_media, codecs) }, + { "offered_payload_types-%u", G_STRUCT_OFFSET(struct call_media, offered_codecs) }, }; for (unsigned int i = 0; i < G_N_ELEMENTS(stores); i++) { @@ -3258,7 +3259,7 @@ static void snapshot_apply_media_codecs(struct call_media *m, parser_arg root) { parser_arg list = redis_parser->dict_get_expect(root, key, BENCODE_LIST); if (!list.gen) continue; - struct codec_store *cs = (void *) ((char *) m + stores[i].offset); + struct codec_store *cs = &G_STRUCT_MEMBER(struct codec_store, m, stores[i].offset); codec_store_cleanup(cs); codec_store_init(cs, m); redis_decode_codec_store(redis_parser, list, cs); diff --git a/t/auto-daemon-tests-rollback-redis.pl b/t/auto-daemon-tests-rollback-redis.pl index ecc24dee0..c8c669cc8 100644 --- a/t/auto-daemon-tests-rollback-redis.pl +++ b/t/auto-daemon-tests-rollback-redis.pl @@ -244,6 +244,9 @@ sub inspect_checkpoint { # than shape: what it must contain is asserted by restoring from it. ok(!$expected_pending || length(field($checkpoint->{snapshot}) // ''), "$redis_format pending checkpoint carries a snapshot"); + # A snapshot never leaves the daemon, so it is bencode whatever the record is. + ok(!$expected_pending || (field($checkpoint->{snapshot}) // '') =~ /^d/, + "$redis_format snapshot is bencode"); } # What rollback restored, judged against the record rather than against `query`. @@ -253,7 +256,20 @@ sub inspect_checkpoint { # carries all of it: the committed state must serialise the same before and after. sub durable_state { my ($record) = @_; - my $decoded = decode_record($record); + return durable_fields(decode_record($record)); +} + +# A snapshot is a call record too, so it is filtered the same way. It is always +# bencode, whatever the record around it is. +sub checkpoint_snapshot { + my ($record) = @_; + my $checkpoint = decode_record($record)->{'checkpoint-0'} or return undef; + my $snapshot = field($checkpoint->{snapshot}) or return undef; + return durable_fields(Bencode::bdecode($snapshot, 1)); +} + +sub durable_fields { + my ($decoded) = @_; my %out; for my $key (keys %$decoded) { # Checkpoint entries describe the pending exchange, not the committed @@ -483,5 +499,53 @@ is_deeply(durable_state(redis_record_after($before_up)), durable_state($plain_record), 'rollback removes DTLS state the rejected offer introduced'); +# --- the snapshot-only state has to survive a rollback too --- +# +# ICE credentials and candidates, endpoint learning, offered codecs, tls_id, the +# preferred hash function and the endpoint map are written into snapshots only, +# so a record comparison cannot see them: a rollback could stop restoring any of +# them with every assertion above still green. Comparing the snapshot taken +# before the rejected offer against one taken after the rollback covers all of +# them at once, because a snapshot is taken before its offer is applied and so +# describes the state the rollback was supposed to reproduce. +new_call; +my ($rt_call, $rt_from, $rt_to) = (cid(), ft(), tt()); +redis_rtpe_req(1, 'offer', 'round trip: offer', { + 'from-tag' => $rt_from, flags => ['track-state'], + sdp => sdp('198.51.100.98', 15030, 'roundTripUfrag', 'roundTripPassword0123456', + 'Ai0RVBUpx3FYuJEyv1oOTQVHrfXEIQGRxWLXQBvR', 'sendrecv'), +}); +redis_rtpe_req(0, 'answer', 'round trip: answer', { + 'from-tag' => $rt_from, 'to-tag' => $rt_to, + sdp => sdp('198.51.100.99', 15040, 'roundTripAnswer', 'roundTripAnswerPwd012345', + 'HHf1TXWnpZlfXHBw5Q3xTNTIhFvbEHIYnmSMDGqR', 'sendrecv'), +}); + +redis_rtpe_req(1, 'offer', 'round trip: rejected offer', { + 'from-tag' => $rt_from, 'to-tag' => $rt_to, + sdp => sdp('198.51.100.100', 15050, 'roundTripReject', 'roundTripRejectPwd01234', + 'GHi1TXWnpZlfXHBw5Q3xTNTIhFvbEHIYnmSMDGqQ', 'sendrecv'), +}); +my $snapshot_before = checkpoint_snapshot($last_record); +ok($snapshot_before, 'round trip: committed snapshot captured'); + +my $rt_rollback = rtpe_req('rollback', 'round trip: rollback', { + 'call-id' => $rt_call, 'from-tag' => $rt_from, 'to-tag' => $rt_to, +}); +is($rt_rollback->{'rolled-back'}, 1, 'round trip: rollback applied'); + +# The snapshot for this offer is taken before it is applied, so it describes the +# state the rollback restored. +redis_rtpe_req(1, 'offer', 'round trip: offer after rollback', { + 'from-tag' => $rt_from, 'to-tag' => $rt_to, + sdp => sdp('198.51.100.101', 15060, 'roundTripAfter', 'roundTripAfterPwd012345', + 'JKl1TXWnpZlfXHBw5Q3xTNTIhFvbEHIYnmSMDGqP', 'sendrecv'), +}); +my $snapshot_after = checkpoint_snapshot($last_record); +ok($snapshot_after, 'round trip: post-rollback snapshot captured'); + +is_deeply($snapshot_after, $snapshot_before, + 'rollback restores the snapshot-only state as well'); + NGCP::Rtpengine::AutoTest::shut_rtpe(); done_testing;