On Mon, Sep 05, 2022 at 02:41:57PM -0500, Eric Blake wrote:
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.
It actually hexdumps the buffer (which is a bit weird actually). So I think you're right here that printing it unconditionally is correct. Sorry for the noise!
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.
Let's not worry, thanks. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW