Skip to content

Conversation

thiagomacieira
Copy link
Member

We hadn't bothered, as this was just example-like code to show how one could convert from CBOR to JSON. But as it was added to the library (no extra dependency), we should Do The Right Thing (DTRT) and escape.

This patch could have used cbor_value_to_pretty() to print the string, which has better support for UTF-8 escaping and thus checks for UTF-8 correctness, but that would make map_to_json()'s metadata functionality much more complex, especially since we cannot rely on open_memstream() always being available. Therefore, we are partially duplicating cborpretty.c's utf8EscapedDump().

Copy link

@georgen117 georgen117 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

I did comment on line of code I think could benefit from code comments to clarify the magic number 0x1f used but have no problem if it is committed as is.

@thiagomacieira thiagomacieira removed the request for review from dangelog March 11, 2025 17:50
We hadn't bothered, as this was just example-like code to show how one
could convert from CBOR to JSON. But as it was added to the library (no
extra dependency), we should Do The Right Thing (DTRT) and escape.

This patch could have used cbor_value_to_pretty() to print the string,
which has better support for UTF-8 escaping and thus checks for UTF-8
correctness, but that would make map_to_json()'s metadata functionality
much more complex, especially since we cannot rely on open_memstream()
always being available. Therefore, we are partially duplicating
cborpretty.c's utf8EscapedDump().

Signed-off-by: Thiago Macieira <thiago.macieira@intel.com>
@thiagomacieira thiagomacieira force-pushed the CBOR_to_JSON_do_properly_escape_JSON_strings branch from 988aa33 to 4863012 Compare March 11, 2025 17:55
@thiagomacieira thiagomacieira merged commit e072bc1 into intel:main Mar 11, 2025
6 checks passed
@thiagomacieira thiagomacieira deleted the CBOR_to_JSON_do_properly_escape_JSON_strings branch March 11, 2025 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants