babeltrace | sink.ctf.fs escaping of struct member names

[MOESSBAUER, Felix]

Hi,

while working on the LTTng index addition, I noticed that the traces
with index are not picked up correctly in trace-compass. The reason for
that is the escaping of the package.context -> cpu_id field. When
creating the CTF metadata with sink.ctf.fs, all user-provided struct
member names are escaped with an underscore.

As of CTF 1.8.3, readers are only recommended to strip single
underscores from field names, but they don't have to. And unfortunately
trace-compass does not do this for the package context cpu_id field
(surprisingly only when having an index...). Just manually changing the
packet->context _cpu_id to cpu_id fixes the issue.

In my opinion, the escaping of the struct member names in the CTF
writer is not correct according to the CTF 1.8.3 spec: Section 4.2.1
states, that reserved keywords must not be used and the recommended
escaping is a underscore prefix. However, the spec does not state any
other names that need to be escaped and by that also not names starting
with an underscore itself.

IOW: The CTF writer currently unconditionally escapes all struct member
names, which I assume to be incorrect

I tried fixing this with the attached patch, however this breaks the
test-trace-copy.sh.
To me it at least partially looks like these tests just check against
the implementation, but not against the spec.
Anyways, I would like to clarify this topic upfront before proposing
any changes.

I also tried to find the reason via a git blame, however there were
that many refactorings that the original idea remains hidden.

Best regards,
Felix

diff --git a/src/plugins/ctf/fs-sink/translate-trace-ir-to-ctf-ir.cpp
b/src/plugins/ctf/fs-sink/translate-trace-ir-to-ctf-ir.cpp
index 12ab213d1..43bc8e098 100644
--- a/src/plugins/ctf/fs-sink/translate-trace-ir-to-ctf-ir.cpp
+++ b/src/plugins/ctf/fs-sink/translate-trace-ir-to-ctf-ir.cpp
@@ -159,12 +159,6 @@ static inline bool must_protect_identifier(const
char *name)
         }
     }
 
     }
 
-    /* Protect an identifier which already starts with `_` */
-    if (name[0] == '_') {
-        must_protect = true;
-        goto end;
-    }
-
 end:
     return must_protect;
 }
@@ -182,7 +176,7 @@ static inline int
cur_path_stack_push(ctf::sink::TraceIrToCtfIrCtx *ctx, const c
 
     if (name) {
         if (ctx->ctf_version == 1) {
-            if (force_protect_name) {
+            if (force_protect_name || must_protect_identifier(name)) {
                 g_string_assign(field_path_elem->name, "_");
             }
 
@@ -641,7 +635,7 @@
translate_structure_field_class_members(ctf::sink::TraceIrToCtfIrCtx
*ctx,
         member =
bt_field_class_structure_borrow_member_by_index_const(ir_fc, i);
         name = bt_field_class_structure_member_get_name(member);
         memb_ir_fc =
bt_field_class_structure_member_borrow_field_class_const(member);
-        ret = cur_path_stack_push(ctx, name, true, memb_ir_fc,
&struct_fc->base);
+        ret = cur_path_stack_push(ctx, name, false, memb_ir_fc,
&struct_fc->base);
         if (ret) {
             BT_CPPLOGE_SPEC(ctx->logger,
                             "Cannot translate structure field class
member: "

-- 
Siemens AG
Linux Expert Center
Friedrich-Ludwig-Bauer-Str. 3
85748 Garching, Germany