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.