Re: [PATCH 1/3] osdep: Add qemu_mkdir_with_parents()

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Dec 16, 2024 at 04:56:33PM +0000, Peter Maydell wrote:
> On Mon, 16 Dec 2024 at 16:14, Peter Xu <peterx@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 <peterx@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 :|