Richard W.M. Jones
2022-Sep-04 16:44 UTC
[Libguestfs] [libnbd PATCH 2/2] debug: Trace even may_set_error=false functions
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. Second patch is completely fine. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
Eric Blake
2022-Sep-05 19:41 UTC
[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