Philippe Mathieu-Daudé <phi...@redhat.com> writes:
> On 12/16/21 15:11, Alex Bennée wrote: >> Philippe Mathieu-Daudé <phi...@redhat.com> writes: >> >>> When experimenting raising GLIB_VERSION_MIN_REQUIRED to 2.68 >>> (Fedora 34 provides GLib 2.68.1) we get: >>> >>> hw/virtio/virtio-crypto.c:245:24: error: 'g_memdup' is deprecated: Use >>> 'g_memdup2' instead [-Werror,-Wdeprecated-declarations] >>> ... >>> >>> g_memdup() has been updated by g_memdup2() to fix eventual security >>> issues (size argument is 32-bit and could be truncated / wrapping). >>> GLib recommends to copy their static inline version of g_memdup2(): >>> https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538 >>> >>> Our glib-compat.h provides a comment explaining how to deal with >>> these deprecated declarations (see commit e71e8cc0355 >>> "glib: enforce the minimum required version and warn about old APIs"). >>> <snip> >>> +#define g_memdup2(m, s) g_memdup2_qemu(m, s) >>> + >> >> As per our style wouldn't it make sense to just call it qemu_memdup(m, >> s)? > > I followed the documentation in include/glib-compat.h: > > /* > * Note that because of the GLIB_VERSION_MAX_ALLOWED constant above, > allowing > * use of functions from newer GLib via this compat header needs a little > * trickery to prevent warnings being emitted. > * > * Consider a function from newer glib-X.Y that we want to use > * > * int g_foo(const char *wibble) > * > * We must define a static inline function with the same signature that does > * what we need, but with a "_qemu" suffix e.g. > * > * static inline void g_foo_qemu(const char *wibble) > * { > * #if GLIB_CHECK_VERSION(X, Y, 0) > * g_foo(wibble) > * #else > * g_something_equivalent_in_older_glib(wibble); > * #endif > * } > * > * The #pragma at the top of this file turns off -Wdeprecated-declarations, > * ensuring this wrapper function impl doesn't trigger the compiler warning > * about using too new glib APIs. Finally we can do > * > * #define g_foo(a) g_foo_qemu(a) > * > * So now the code elsewhere in QEMU, which *does* have the > * -Wdeprecated-declarations warning active, can call g_foo(...) as normal, > * without generating warnings. > */ > > which is how g_unix_get_passwd_entry_qemu() is implemented. Yet later we have qemu_g_test_slow following the style guide. Also I'm confused by the usage of g_unix_get_passwd_entry_qemu because the only place I see it in qga/commands-posix-ssh.c right before it does: #define g_unix_get_passwd_entry_qemu(username, err) \ test_get_passwd_entry(username, err) although I think that only hold when the file is built with QGA_BUILD_UNIT_TEST. > Should we reword the documentation first? The original wording in glib-compat.h was added by Daniel in 2018 but the commit that added the password function comments: Since the fallback version is still unsafe, I would rather keep the _qemu postfix, to make sure it's not being misused by mistake. When/if necessary, we can implement a safer fallback and drop the _qemu suffix. So if we are going to make a distinction between a qemu prefix and suffix we should agree that and add it to the style document. -- Alex Bennée