On Sun, Sep 04, 2022 at 05:44:33PM +0100, Richard W.M. Jones wrote:
So my feeling about this patch series:
I don't understand why the first patch is necessary, _and_ I think it might be "dangerous" (for small values of dangerous).
What happens if in future we add a new RStaticString API which returns a string that does need to be escaped? It should at least be documented that RStaticString must only return simple, printable strings. But ideally the first patch wouldn't exist and we could deal with any RStaticString API.
At present, the only escaping we do is nbd_internal_printable_string(), which converts NULL to "NULL" (not relevant here; either may_set_error=true and we already filter out NULL as an error, or may_set_error=false and the result is guaranteed non-NULL), and which truncates strings longer than 512 bytes to avoid overlong logs. But RStaticString returns are going to be a compile-time constant string, and we aren't going to write a string that long that would need our current form of escaping.
Second patch is completely fine.
I had already pushed both patches. But if we revert the first one, using just the second, I was having difficulties making the generated api.c line up nicely. It was looking something like: if_debug (h) { char *ret_printable = nbd_internal_printable_string (ret); debug_direct (h, "nbd_get_tls", "leave: ret=" "%s", ret_printable ? ret_printable : ""); free (ret_printable); } where the detour through nbd_internal_printable_string() with its extra spacing wasn't making much sense compared to just printing it directly by removing the special handling for RStaticString in general. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org