Re: [Libguestfs] [libnbd PATCH 2/2] debug: Trace even may_set_error=false functions
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