On Mon, Dec 16, 2024 at 7:12 PM Peter Xu <pet...@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 <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. > > 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. > > > > > 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. > > OK. > > > > > 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. > > Hmm, I normally always think crash is better especially in tests to keep > everything around when an error happens as general rule. > > TAP parsing especially on errors is more useful to me when we constantly > expect failures, IIUC that's not the case for QEMU tests? Because I do > expect the CI to pass all the tests always. But I also admit I don't know > the whole picture of QEMU tests.. > > If we don't care about retval checks in tests, we can definitely drop patch > 1-2 here in all cases. > > Thanks, > > -- > Peter Xu > >
