[PATCH 0/3] tests: Fix some new coverity issues reported

[PATCH 0/3] tests: Fix some new coverity issues reported

[Peter Xu]

Fix four recent coverity reports on migration tests.  When at it, also
applied the same helper to some crypto/tls tests.

Thanks,

Peter Xu (3):
  osdep: Add qemu_mkdir_with_parents()
  tests: Use qemu_mkdir_with_parents() for all test code
  tests/migration: Drop arch_[source|target]

 include/qemu/osdep.h                  | 7 +++++++
 qga/commands-posix-ssh.c              | 8 ++------
 tests/qtest/migration/framework.c     | 8 ++------
 tests/qtest/migration/tls-tests.c     | 6 +++---
 tests/unit/test-crypto-tlscredsx509.c | 4 ++--
 tests/unit/test-crypto-tlssession.c   | 6 +++---
 tests/unit/test-io-channel-tls.c      | 6 +++---
 util/osdep.c                          | 6 ++++++
 8 files changed, 28 insertions(+), 23 deletions(-)

-- 
2.47.0

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

[Peter Xu]

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);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/qga/commands-posix-ssh.c b/qga/commands-posix-ssh.c
index 246171d323..a39abcbaa5 100644
--- a/qga/commands-posix-ssh.c
+++ b/qga/commands-posix-ssh.c
@@ -18,7 +18,6 @@ static struct passwd *
 test_get_passwd_entry(const gchar *user_name, GError **error)
 {
     struct passwd *p;
-    int ret;
 
     if (!user_name || g_strcmp0(user_name, g_get_user_name())) {
         g_set_error(error, G_UNIX_ERROR, 0, "Invalid user name");
@@ -30,8 +29,7 @@ test_get_passwd_entry(const gchar *user_name, GError **error)
     p->pw_uid = geteuid();
     p->pw_gid = getegid();
 
-    ret = g_mkdir_with_parents(p->pw_dir, 0700);
-    g_assert(ret == 0);
+    qemu_mkdir_with_parents(p->pw_dir, 0700);
 
     return p;
 }
@@ -263,11 +261,9 @@ test_authorized_keys_set(const char *contents)
 {
     g_autoptr(GError) err = NULL;
     g_autofree char *path = NULL;
-    int ret;
 
     path = g_build_filename(g_get_home_dir(), ".ssh", NULL);
-    ret = g_mkdir_with_parents(path, 0700);
-    g_assert(ret == 0);
+    qemu_mkdir_with_parents(path, 0700);
     g_free(path);
 
     path = test_get_authorized_keys_path();
diff --git a/util/osdep.c b/util/osdep.c
index 770369831b..3a724c1814 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -613,3 +613,9 @@ int qemu_fdatasync(int fd)
     return fsync(fd);
 #endif
 }
+
+void qemu_mkdir_with_parents(const char *dir, int mode)
+{
+    int ret = g_mkdir_with_parents(dir, 0700);
+    g_assert(ret == 0);
+}
-- 
2.47.0

[PATCH 2/3] tests: Use qemu_mkdir_with_parents() for all test code

[Peter Xu]

Coverity isn't happy on the QEMU test cases where g_mkdir_with_parents() is
used without checking retvals.  Use qemu_mkdir_with_parents() to fix them.

Resolves: Coverity CID 1568381
Resolves: Coverity CID 1568378
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tests/qtest/migration/tls-tests.c     | 6 +++---
 tests/unit/test-crypto-tlscredsx509.c | 4 ++--
 tests/unit/test-crypto-tlssession.c   | 6 +++---
 tests/unit/test-io-channel-tls.c      | 6 +++---
 4 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/tests/qtest/migration/tls-tests.c b/tests/qtest/migration/tls-tests.c
index 5704a1f992..c78daff998 100644
--- a/tests/qtest/migration/tls-tests.c
+++ b/tests/qtest/migration/tls-tests.c
@@ -43,14 +43,14 @@ migrate_hook_start_tls_psk_common(QTestState *from,
     data->workdir = g_strdup_printf("%s/tlscredspsk0", tmpfs);
     data->pskfile = g_strdup_printf("%s/%s", data->workdir,
                                     QCRYPTO_TLS_CREDS_PSKFILE);
-    g_mkdir_with_parents(data->workdir, 0700);
+    qemu_mkdir_with_parents(data->workdir, 0700);
     test_tls_psk_init(data->pskfile);
 
     if (mismatch) {
         data->workdiralt = g_strdup_printf("%s/tlscredspskalt0", tmpfs);
         data->pskfilealt = g_strdup_printf("%s/%s", data->workdiralt,
                                            QCRYPTO_TLS_CREDS_PSKFILE);
-        g_mkdir_with_parents(data->workdiralt, 0700);
+        qemu_mkdir_with_parents(data->workdiralt, 0700);
         test_tls_psk_init_alt(data->pskfilealt);
     }
 
@@ -152,7 +152,7 @@ migrate_hook_start_tls_x509_common(QTestState *from,
         data->clientcert = g_strdup_printf("%s/client-cert.pem", data->workdir);
     }
 
-    g_mkdir_with_parents(data->workdir, 0700);
+    qemu_mkdir_with_parents(data->workdir, 0700);
 
     test_tls_init(data->keyfile);
 #ifndef _WIN32
diff --git a/tests/unit/test-crypto-tlscredsx509.c b/tests/unit/test-crypto-tlscredsx509.c
index 3c25d75ca1..57ce0134df 100644
--- a/tests/unit/test-crypto-tlscredsx509.c
+++ b/tests/unit/test-crypto-tlscredsx509.c
@@ -75,7 +75,7 @@ static void test_tls_creds(const void *opaque)
     QCryptoTLSCreds *creds;
 
 #define CERT_DIR "tests/test-crypto-tlscredsx509-certs/"
-    g_mkdir_with_parents(CERT_DIR, 0700);
+    qemu_mkdir_with_parents(CERT_DIR, 0700);
 
     unlink(CERT_DIR QCRYPTO_TLS_CREDS_X509_CA_CERT);
     if (data->isServer) {
@@ -141,7 +141,7 @@ int main(int argc, char **argv)
     g_test_init(&argc, &argv, NULL);
     g_setenv("GNUTLS_FORCE_FIPS_MODE", "2", 1);
 
-    g_mkdir_with_parents(WORKDIR, 0700);
+    qemu_mkdir_with_parents(WORKDIR, 0700);
 
     test_tls_init(KEYFILE);
 
diff --git a/tests/unit/test-crypto-tlssession.c b/tests/unit/test-crypto-tlssession.c
index 3395f73560..db97cbefe7 100644
--- a/tests/unit/test-crypto-tlssession.c
+++ b/tests/unit/test-crypto-tlssession.c
@@ -271,8 +271,8 @@ static void test_crypto_tls_session_x509(const void *opaque)
 
 #define CLIENT_CERT_DIR "tests/test-crypto-tlssession-client/"
 #define SERVER_CERT_DIR "tests/test-crypto-tlssession-server/"
-    g_mkdir_with_parents(CLIENT_CERT_DIR, 0700);
-    g_mkdir_with_parents(SERVER_CERT_DIR, 0700);
+    qemu_mkdir_with_parents(CLIENT_CERT_DIR, 0700);
+    qemu_mkdir_with_parents(SERVER_CERT_DIR, 0700);
 
     unlink(SERVER_CERT_DIR QCRYPTO_TLS_CREDS_X509_CA_CERT);
     unlink(SERVER_CERT_DIR QCRYPTO_TLS_CREDS_X509_SERVER_CERT);
@@ -420,7 +420,7 @@ int main(int argc, char **argv)
     g_test_init(&argc, &argv, NULL);
     g_setenv("GNUTLS_FORCE_FIPS_MODE", "2", 1);
 
-    g_mkdir_with_parents(WORKDIR, 0700);
+    qemu_mkdir_with_parents(WORKDIR, 0700);
 
     test_tls_init(KEYFILE);
     test_tls_psk_init(PSKFILE);
diff --git a/tests/unit/test-io-channel-tls.c b/tests/unit/test-io-channel-tls.c
index e036ac5df4..6cb83e891a 100644
--- a/tests/unit/test-io-channel-tls.c
+++ b/tests/unit/test-io-channel-tls.c
@@ -125,8 +125,8 @@ static void test_io_channel_tls(const void *opaque)
 
 #define CLIENT_CERT_DIR "tests/test-io-channel-tls-client/"
 #define SERVER_CERT_DIR "tests/test-io-channel-tls-server/"
-    g_mkdir_with_parents(CLIENT_CERT_DIR, 0700);
-    g_mkdir_with_parents(SERVER_CERT_DIR, 0700);
+    qemu_mkdir_with_parents(CLIENT_CERT_DIR, 0700);
+    qemu_mkdir_with_parents(SERVER_CERT_DIR, 0700);
 
     unlink(SERVER_CERT_DIR QCRYPTO_TLS_CREDS_X509_CA_CERT);
     unlink(SERVER_CERT_DIR QCRYPTO_TLS_CREDS_X509_SERVER_CERT);
@@ -273,7 +273,7 @@ int main(int argc, char **argv)
     g_test_init(&argc, &argv, NULL);
     g_setenv("GNUTLS_FORCE_FIPS_MODE", "2", 1);
 
-    g_mkdir_with_parents(WORKDIR, 0700);
+    qemu_mkdir_with_parents(WORKDIR, 0700);
 
     test_tls_init(KEYFILE);
 
-- 
2.47.0

[PATCH 3/3] tests/migration: Drop arch_[source|target]

[Peter Xu]

Coverity complained about them. These two variables are never used now
after commit 832c732c5d ("migration-test: Create arch_opts"), and/or commit
34cc54fb35 ("tests/qtest/migration-test: Use custom asm bios for ppc64").

Resolves: Coverity CID 1568379
Resolves: Coverity CID 1568380
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tests/qtest/migration/framework.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/tests/qtest/migration/framework.c b/tests/qtest/migration/framework.c
index a902936039..47ce07856e 100644
--- a/tests/qtest/migration/framework.c
+++ b/tests/qtest/migration/framework.c
@@ -197,8 +197,6 @@ static void cleanup(const char *filename)
 int migrate_start(QTestState **from, QTestState **to, const char *uri,
                   MigrateStart *args)
 {
-    g_autofree gchar *arch_source = NULL;
-    g_autofree gchar *arch_target = NULL;
     /* options for source and target */
     g_autofree gchar *arch_opts = NULL;
     g_autofree gchar *cmd_source = NULL;
@@ -307,12 +305,11 @@ int migrate_start(QTestState **from, QTestState **to, const char *uri,
                                  "-name source,debug-threads=on "
                                  "-m %s "
                                  "-serial file:%s/src_serial "
-                                 "%s %s %s %s %s",
+                                 "%s %s %s %s",
                                  kvm_opts ? kvm_opts : "",
                                  machine, machine_opts,
                                  memory_size, tmpfs,
                                  arch_opts ? arch_opts : "",
-                                 arch_source ? arch_source : "",
                                  shmem_opts ? shmem_opts : "",
                                  args->opts_source ? args->opts_source : "",
                                  ignore_stderr);
@@ -329,12 +326,11 @@ int migrate_start(QTestState **from, QTestState **to, const char *uri,
                                  "-m %s "
                                  "-serial file:%s/dest_serial "
                                  "-incoming %s "
-                                 "%s %s %s %s %s",
+                                 "%s %s %s %s",
                                  kvm_opts ? kvm_opts : "",
                                  machine, machine_opts,
                                  memory_size, tmpfs, uri,
                                  arch_opts ? arch_opts : "",
-                                 arch_target ? arch_target : "",
                                  shmem_opts ? shmem_opts : "",
                                  args->opts_target ? args->opts_target : "",
                                  ignore_stderr);
-- 
2.47.0

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

[Peter Maydell]

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.

thanks
-- PMM

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

[Peter Xu]

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.

> 
> 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

Re: [PATCH 3/3] tests/migration: Drop arch_[source|target]

[Alex Bennée]

Peter Xu <peterx@redhat.com> writes:

> Coverity complained about them. These two variables are never used now
> after commit 832c732c5d ("migration-test: Create arch_opts"), and/or commit
> 34cc54fb35 ("tests/qtest/migration-test: Use custom asm bios for ppc64").
>
> Resolves: Coverity CID 1568379
> Resolves: Coverity CID 1568380
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

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

[Alex Bennée]

Peter Xu <peterx@redhat.com> writes:

> 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>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 2/3] tests: Use qemu_mkdir_with_parents() for all test code

[Alex Bennée]

Peter Xu <peterx@redhat.com> writes:

> Coverity isn't happy on the QEMU test cases where g_mkdir_with_parents() is
> used without checking retvals.  Use qemu_mkdir_with_parents() to fix them.
>
> Resolves: Coverity CID 1568381
> Resolves: Coverity CID 1568378
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

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

[Konstantin Kostiuk]

[-- Attachment #1: Type: text/plain, Size: 3407 bytes --]

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.


>
> >
> > 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
>
>

[-- Attachment #2: Type: text/html, Size: 4618 bytes --]

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

[Peter Xu]

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

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

[Daniel P. Berrangé]

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 :|

Re: [PATCH 2/3] tests: Use qemu_mkdir_with_parents() for all test code

[Daniel P. Berrangé]

On Mon, Dec 16, 2024 at 11:14:12AM -0500, Peter Xu wrote:
> Coverity isn't happy on the QEMU test cases where g_mkdir_with_parents() is
> used without checking retvals.  Use qemu_mkdir_with_parents() to fix them.
> 
> Resolves: Coverity CID 1568381
> Resolves: Coverity CID 1568378
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  tests/qtest/migration/tls-tests.c     | 6 +++---
>  tests/unit/test-crypto-tlscredsx509.c | 4 ++--
>  tests/unit/test-crypto-tlssession.c   | 6 +++---
>  tests/unit/test-io-channel-tls.c      | 6 +++---
>  4 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/tests/qtest/migration/tls-tests.c b/tests/qtest/migration/tls-tests.c
> index 5704a1f992..c78daff998 100644
> --- a/tests/qtest/migration/tls-tests.c
> +++ b/tests/qtest/migration/tls-tests.c
> @@ -43,14 +43,14 @@ migrate_hook_start_tls_psk_common(QTestState *from,
>      data->workdir = g_strdup_printf("%s/tlscredspsk0", tmpfs);
>      data->pskfile = g_strdup_printf("%s/%s", data->workdir,
>                                      QCRYPTO_TLS_CREDS_PSKFILE);
> -    g_mkdir_with_parents(data->workdir, 0700);
> +    qemu_mkdir_with_parents(data->workdir, 0700);

I dislike this as a solution as it is not obvious that qemu_mkdir_with_parents
is any different. IMHO the right solution for tests is to assert inline

  g_asset(g_mkdir_with_parents(...) == 0);

>      test_tls_psk_init(data->pskfile);
>  
>      if (mismatch) {
>          data->workdiralt = g_strdup_printf("%s/tlscredspskalt0", tmpfs);
>          data->pskfilealt = g_strdup_printf("%s/%s", data->workdiralt,
>                                             QCRYPTO_TLS_CREDS_PSKFILE);
> -        g_mkdir_with_parents(data->workdiralt, 0700);
> +        qemu_mkdir_with_parents(data->workdiralt, 0700);
>          test_tls_psk_init_alt(data->pskfilealt);
>      }
>  
> @@ -152,7 +152,7 @@ migrate_hook_start_tls_x509_common(QTestState *from,
>          data->clientcert = g_strdup_printf("%s/client-cert.pem", data->workdir);
>      }
>  
> -    g_mkdir_with_parents(data->workdir, 0700);
> +    qemu_mkdir_with_parents(data->workdir, 0700);
>  
>      test_tls_init(data->keyfile);
>  #ifndef _WIN32
> diff --git a/tests/unit/test-crypto-tlscredsx509.c b/tests/unit/test-crypto-tlscredsx509.c
> index 3c25d75ca1..57ce0134df 100644
> --- a/tests/unit/test-crypto-tlscredsx509.c
> +++ b/tests/unit/test-crypto-tlscredsx509.c
> @@ -75,7 +75,7 @@ static void test_tls_creds(const void *opaque)
>      QCryptoTLSCreds *creds;
>  
>  #define CERT_DIR "tests/test-crypto-tlscredsx509-certs/"
> -    g_mkdir_with_parents(CERT_DIR, 0700);
> +    qemu_mkdir_with_parents(CERT_DIR, 0700);
>  
>      unlink(CERT_DIR QCRYPTO_TLS_CREDS_X509_CA_CERT);
>      if (data->isServer) {
> @@ -141,7 +141,7 @@ int main(int argc, char **argv)
>      g_test_init(&argc, &argv, NULL);
>      g_setenv("GNUTLS_FORCE_FIPS_MODE", "2", 1);
>  
> -    g_mkdir_with_parents(WORKDIR, 0700);
> +    qemu_mkdir_with_parents(WORKDIR, 0700);
>  
>      test_tls_init(KEYFILE);
>  
> diff --git a/tests/unit/test-crypto-tlssession.c b/tests/unit/test-crypto-tlssession.c
> index 3395f73560..db97cbefe7 100644
> --- a/tests/unit/test-crypto-tlssession.c
> +++ b/tests/unit/test-crypto-tlssession.c
> @@ -271,8 +271,8 @@ static void test_crypto_tls_session_x509(const void *opaque)
>  
>  #define CLIENT_CERT_DIR "tests/test-crypto-tlssession-client/"
>  #define SERVER_CERT_DIR "tests/test-crypto-tlssession-server/"
> -    g_mkdir_with_parents(CLIENT_CERT_DIR, 0700);
> -    g_mkdir_with_parents(SERVER_CERT_DIR, 0700);
> +    qemu_mkdir_with_parents(CLIENT_CERT_DIR, 0700);
> +    qemu_mkdir_with_parents(SERVER_CERT_DIR, 0700);
>  
>      unlink(SERVER_CERT_DIR QCRYPTO_TLS_CREDS_X509_CA_CERT);
>      unlink(SERVER_CERT_DIR QCRYPTO_TLS_CREDS_X509_SERVER_CERT);
> @@ -420,7 +420,7 @@ int main(int argc, char **argv)
>      g_test_init(&argc, &argv, NULL);
>      g_setenv("GNUTLS_FORCE_FIPS_MODE", "2", 1);
>  
> -    g_mkdir_with_parents(WORKDIR, 0700);
> +    qemu_mkdir_with_parents(WORKDIR, 0700);
>  
>      test_tls_init(KEYFILE);
>      test_tls_psk_init(PSKFILE);
> diff --git a/tests/unit/test-io-channel-tls.c b/tests/unit/test-io-channel-tls.c
> index e036ac5df4..6cb83e891a 100644
> --- a/tests/unit/test-io-channel-tls.c
> +++ b/tests/unit/test-io-channel-tls.c
> @@ -125,8 +125,8 @@ static void test_io_channel_tls(const void *opaque)
>  
>  #define CLIENT_CERT_DIR "tests/test-io-channel-tls-client/"
>  #define SERVER_CERT_DIR "tests/test-io-channel-tls-server/"
> -    g_mkdir_with_parents(CLIENT_CERT_DIR, 0700);
> -    g_mkdir_with_parents(SERVER_CERT_DIR, 0700);
> +    qemu_mkdir_with_parents(CLIENT_CERT_DIR, 0700);
> +    qemu_mkdir_with_parents(SERVER_CERT_DIR, 0700);
>  
>      unlink(SERVER_CERT_DIR QCRYPTO_TLS_CREDS_X509_CA_CERT);
>      unlink(SERVER_CERT_DIR QCRYPTO_TLS_CREDS_X509_SERVER_CERT);
> @@ -273,7 +273,7 @@ int main(int argc, char **argv)
>      g_test_init(&argc, &argv, NULL);
>      g_setenv("GNUTLS_FORCE_FIPS_MODE", "2", 1);
>  
> -    g_mkdir_with_parents(WORKDIR, 0700);
> +    qemu_mkdir_with_parents(WORKDIR, 0700);
>  
>      test_tls_init(KEYFILE);
>  
> -- 
> 2.47.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 :|