On Mon, Dec 16, 2024 at 04:56:33PM +0000, Peter Maydell wrote: > On Mon, 16 Dec 2024 at 16:14, Peter Xu <pet...@redhat.com> wrote: > > > > QEMU uses g_mkdir_with_parents() a lot, especially in the case where the > > failure case is ignored so an abort is expected when happened. > > > > Provide a helper qemu_mkdir_with_parents() to do that, and use it in the > > two cases in qga/. To be used in more places later. > > > > Signed-off-by: Peter Xu <pet...@redhat.com> > > --- > > include/qemu/osdep.h | 7 +++++++ > > qga/commands-posix-ssh.c | 8 ++------ > > util/osdep.c | 6 ++++++ > > 3 files changed, 15 insertions(+), 6 deletions(-) > > > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h > > index fdff07fd99..dc67fb2e5e 100644 > > --- a/include/qemu/osdep.h > > +++ b/include/qemu/osdep.h > > @@ -828,6 +828,13 @@ static inline int > > platform_does_not_support_system(const char *command) > > } > > #endif /* !HAVE_SYSTEM_FUNCTION */ > > > > +/** > > + * qemu_mkdir_with_parents: > > + * > > + * Create directories with parents. Abort on failures. > > + */ > > +void qemu_mkdir_with_parents(const char *dir, int mode); > > Don't put new function prototypes into osdep.h, please. > It is included by every single C file in the codebase. > There is always somewhere better to put things. > > QEMU shouldn't abort on things that are kind of expected > OS errors like "couldn't create a directory", so I'm > a bit dubious about this function. > > The two use cases in this commit seem to be test code, > so asserting is reasonable. But a "for test code only" > function should go in a header file that's only included > by test cases and the comment should be clear about that, > and it shouldn't have a function name that implies > "this is the normal way any code in QEMU might want > to create directories". > > For the qtest tests, I currently ignore Coverity > reports in our test code unless it seems particularly > worthwhile to fix them. This is especially true for > complaints about unchecked return values and the like. > > Even in a test case it is still not great to call > g_assert(), because this makes the test binary crash, > rather than reporting an error. The surrounding TAP > protocol parsing code then doesn't report the test > failure the way you might like.
I also think qemu_mkdir_with_parents is *worse* than the current code. It saves 1 line in the test file, but hides the fact that it asserts on failure which is an relevant observation. If we really want to save that 1 line of code then just condense it inplace g_assert(g_mkdir_with_parents(dir, mode) == 0); With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
