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

[Peter Xu <peterx@redhat.com>]

On Mon, Dec 16, 2024 at 07:28:19PM +0200, Konstantin Kostiuk wrote:
> On Mon, Dec 16, 2024 at 7:12 PM Peter Xu <peterx@redhat.com> wrote:
> 
> > 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.
> >
> > That's what qga/ is doing right now, rather than a decision made in this
> > series, though.
> >
> 
> I think we need to fix this behavior in QGA and report the real error,
> instead of wrapping the `assert` into some function that will make
> it not so obvious.

Even if we want to do that, we can also do that on top, btw.  As this patch
is as simple as a cleanup to dedup two chunks of code.

But if this patch is not liked by most from different angles, we can simply
drop it.. together with patch 2, as in my previous reply.

Thanks,

-- 
Peter Xu