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
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
