diff mbox series

[v2] tests: Trying fixes test-replication.c on msys2.

Message ID 20200903220655.1333-1-luoyonggang@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v2] tests: Trying fixes test-replication.c on msys2. | expand

Commit Message

Yonggang Luo Sept. 3, 2020, 10:06 p.m. UTC
Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
---
 tests/test-replication.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

Comments

Paolo Bonzini Sept. 4, 2020, 7:58 a.m. UTC | #1
On 04/09/20 00:06, Yonggang Luo wrote:
> Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
> ---
>  tests/test-replication.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/test-replication.c b/tests/test-replication.c
> index 9ab3666a90..d0e06f8d77 100644
> --- a/tests/test-replication.c
> +++ b/tests/test-replication.c
> @@ -23,14 +23,18 @@
>  
>  /* primary */
>  #define P_ID "primary-id"
> -static char p_local_disk[] = "/tmp/p_local_disk.XXXXXX";
> +#define P_LOCAL_DISK "%s/p_local_disk.XXXXXX"
> +static char p_local_disk[PATH_MAX];
>  
>  /* secondary */
>  #define S_ID "secondary-id"
>  #define S_LOCAL_DISK_ID "secondary-local-disk-id"
> -static char s_local_disk[] = "/tmp/s_local_disk.XXXXXX";
> -static char s_active_disk[] = "/tmp/s_active_disk.XXXXXX";
> -static char s_hidden_disk[] = "/tmp/s_hidden_disk.XXXXXX";
> +#define S_LOCAL_DISK "%s/s_local_disk.XXXXXX"
> +static char s_local_disk[PATH_MAX];
> +#define S_ACTIVE_DISK "%s/s_active_disk.XXXXXX"
> +static char s_active_disk[PATH_MAX];
> +#define S_HIDDEN_DISK "%s/s_hidden_disk.XXXXXX"
> +static char s_hidden_disk[PATH_MAX];
>  
>  /* FIXME: steal from blockdev.c */
>  QemuOptsList qemu_drive_opts = {
> @@ -571,7 +575,12 @@ static void setup_sigabrt_handler(void)
>  int main(int argc, char **argv)
>  {
>      int ret;
> +    const char *tmpdir = g_get_tmp_dir();
>      qemu_init_main_loop(&error_fatal);
> +    sprintf(p_local_disk, P_LOCAL_DISK, tmpdir);
> +    sprintf(s_local_disk, S_LOCAL_DISK, tmpdir);
> +    sprintf(s_active_disk, S_ACTIVE_DISK, tmpdir);
> +    sprintf(s_hidden_disk, S_HIDDEN_DISK, tmpdir);
>      bdrv_init();
>  
>      g_test_init(&argc, &argv, NULL);
> 

Cc: qemu-block@nongnu.org
Thomas Huth Sept. 4, 2020, 1:07 p.m. UTC | #2
On 04/09/2020 00.06, Yonggang Luo wrote:
> Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
> ---
>  tests/test-replication.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/test-replication.c b/tests/test-replication.c
> index 9ab3666a90..d0e06f8d77 100644
> --- a/tests/test-replication.c
> +++ b/tests/test-replication.c
> @@ -23,14 +23,18 @@
>  
>  /* primary */
>  #define P_ID "primary-id"
> -static char p_local_disk[] = "/tmp/p_local_disk.XXXXXX";
> +#define P_LOCAL_DISK "%s/p_local_disk.XXXXXX"
> +static char p_local_disk[PATH_MAX];
>  
>  /* secondary */
>  #define S_ID "secondary-id"
>  #define S_LOCAL_DISK_ID "secondary-local-disk-id"
> -static char s_local_disk[] = "/tmp/s_local_disk.XXXXXX";
> -static char s_active_disk[] = "/tmp/s_active_disk.XXXXXX";
> -static char s_hidden_disk[] = "/tmp/s_hidden_disk.XXXXXX";
> +#define S_LOCAL_DISK "%s/s_local_disk.XXXXXX"
> +static char s_local_disk[PATH_MAX];
> +#define S_ACTIVE_DISK "%s/s_active_disk.XXXXXX"
> +static char s_active_disk[PATH_MAX];
> +#define S_HIDDEN_DISK "%s/s_hidden_disk.XXXXXX"
> +static char s_hidden_disk[PATH_MAX];
>  
>  /* FIXME: steal from blockdev.c */
>  QemuOptsList qemu_drive_opts = {
> @@ -571,7 +575,12 @@ static void setup_sigabrt_handler(void)
>  int main(int argc, char **argv)
>  {
>      int ret;
> +    const char *tmpdir = g_get_tmp_dir();
>      qemu_init_main_loop(&error_fatal);
> +    sprintf(p_local_disk, P_LOCAL_DISK, tmpdir);
> +    sprintf(s_local_disk, S_LOCAL_DISK, tmpdir);
> +    sprintf(s_active_disk, S_ACTIVE_DISK, tmpdir);
> +    sprintf(s_hidden_disk, S_HIDDEN_DISK, tmpdir);

Sounds like the right way to go, but I think I'd do it without the
#defines and simply use the strings directly here, what do you think?

 Thomas


PS: Please use scripts/get_maintainer.pl (or the MAINTAINERS file
directly) on your patches to find out the right maintainers that you
should put on CC: for every patch.
Yonggang Luo Sept. 5, 2020, 3:11 a.m. UTC | #3
On Fri, Sep 4, 2020 at 9:07 PM Thomas Huth <thuth@redhat.com> wrote:

> On 04/09/2020 00.06, Yonggang Luo wrote:
> > Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
> > ---
> >  tests/test-replication.c | 17 +++++++++++++----
> >  1 file changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/tests/test-replication.c b/tests/test-replication.c
> > index 9ab3666a90..d0e06f8d77 100644
> > --- a/tests/test-replication.c
> > +++ b/tests/test-replication.c
> > @@ -23,14 +23,18 @@
> >
> >  /* primary */
> >  #define P_ID "primary-id"
> > -static char p_local_disk[] = "/tmp/p_local_disk.XXXXXX";
> > +#define P_LOCAL_DISK "%s/p_local_disk.XXXXXX"
> > +static char p_local_disk[PATH_MAX];
> >
> >  /* secondary */
> >  #define S_ID "secondary-id"
> >  #define S_LOCAL_DISK_ID "secondary-local-disk-id"
> > -static char s_local_disk[] = "/tmp/s_local_disk.XXXXXX";
> > -static char s_active_disk[] = "/tmp/s_active_disk.XXXXXX";
> > -static char s_hidden_disk[] = "/tmp/s_hidden_disk.XXXXXX";
> > +#define S_LOCAL_DISK "%s/s_local_disk.XXXXXX"
> > +static char s_local_disk[PATH_MAX];
> > +#define S_ACTIVE_DISK "%s/s_active_disk.XXXXXX"
> > +static char s_active_disk[PATH_MAX];
> > +#define S_HIDDEN_DISK "%s/s_hidden_disk.XXXXXX"
> > +static char s_hidden_disk[PATH_MAX];
> >
> >  /* FIXME: steal from blockdev.c */
> >  QemuOptsList qemu_drive_opts = {
> > @@ -571,7 +575,12 @@ static void setup_sigabrt_handler(void)
> >  int main(int argc, char **argv)
> >  {
> >      int ret;
> > +    const char *tmpdir = g_get_tmp_dir();
> >      qemu_init_main_loop(&error_fatal);
> > +    sprintf(p_local_disk, P_LOCAL_DISK, tmpdir);
> > +    sprintf(s_local_disk, S_LOCAL_DISK, tmpdir);
> > +    sprintf(s_active_disk, S_ACTIVE_DISK, tmpdir);
> > +    sprintf(s_hidden_disk, S_HIDDEN_DISK, tmpdir);
>
> Sounds like the right way to go, but I think I'd do it without the
> #defines and simply use the strings directly here, what do you think?
>
I place them at the same place by define is for easily readable, if I
directly place at sprintf,
then the code are harder to read

>
>  Thomas
>
>
> PS: Please use scripts/get_maintainer.pl (or the MAINTAINERS file
> directly) on your patches to find out the right maintainers that you
> should put on CC: for every patch.
>
>
Thomas Huth Sept. 5, 2020, 7:25 a.m. UTC | #4
On 05/09/2020 05.11, 罗勇刚(Yonggang Luo) wrote:
> 
> 
> On Fri, Sep 4, 2020 at 9:07 PM Thomas Huth <thuth@redhat.com
> <mailto:thuth@redhat.com>> wrote:
> 
>     On 04/09/2020 00.06, Yonggang Luo wrote:
>     > Signed-off-by: Yonggang Luo <luoyonggang@gmail.com
>     <mailto:luoyonggang@gmail.com>>
>     > ---
>     >  tests/test-replication.c | 17 +++++++++++++----
>     >  1 file changed, 13 insertions(+), 4 deletions(-)
>     >
>     > diff --git a/tests/test-replication.c b/tests/test-replication.c
>     > index 9ab3666a90..d0e06f8d77 100644
>     > --- a/tests/test-replication.c
>     > +++ b/tests/test-replication.c
>     > @@ -23,14 +23,18 @@
>     > 
>     >  /* primary */
>     >  #define P_ID "primary-id"
>     > -static char p_local_disk[] = "/tmp/p_local_disk.XXXXXX";
>     > +#define P_LOCAL_DISK "%s/p_local_disk.XXXXXX"
>     > +static char p_local_disk[PATH_MAX];
>     > 
>     >  /* secondary */
>     >  #define S_ID "secondary-id"
>     >  #define S_LOCAL_DISK_ID "secondary-local-disk-id"
>     > -static char s_local_disk[] = "/tmp/s_local_disk.XXXXXX";
>     > -static char s_active_disk[] = "/tmp/s_active_disk.XXXXXX";
>     > -static char s_hidden_disk[] = "/tmp/s_hidden_disk.XXXXXX";
>     > +#define S_LOCAL_DISK "%s/s_local_disk.XXXXXX"
>     > +static char s_local_disk[PATH_MAX];
>     > +#define S_ACTIVE_DISK "%s/s_active_disk.XXXXXX"
>     > +static char s_active_disk[PATH_MAX];
>     > +#define S_HIDDEN_DISK "%s/s_hidden_disk.XXXXXX"
>     > +static char s_hidden_disk[PATH_MAX];
>     > 
>     >  /* FIXME: steal from blockdev.c */
>     >  QemuOptsList qemu_drive_opts = {
>     > @@ -571,7 +575,12 @@ static void setup_sigabrt_handler(void)
>     >  int main(int argc, char **argv)
>     >  {
>     >      int ret;
>     > +    const char *tmpdir = g_get_tmp_dir();
>     >      qemu_init_main_loop(&error_fatal);
>     > +    sprintf(p_local_disk, P_LOCAL_DISK, tmpdir);
>     > +    sprintf(s_local_disk, S_LOCAL_DISK, tmpdir);
>     > +    sprintf(s_active_disk, S_ACTIVE_DISK, tmpdir);
>     > +    sprintf(s_hidden_disk, S_HIDDEN_DISK, tmpdir);
> 
>     Sounds like the right way to go, but I think I'd do it without the
>     #defines and simply use the strings directly here, what do you think?
> 
> I place them at the same place by define is for easily readable, if I
> directly place at sprintf, then the code are harder to read 

IMHO it's easier to read the code the other way round: For understanding
the sprintf and its arguments, you have to know the format string, e.g.
will the "tmpdir" be handled via "%s", or "%p" or maybe something
completely different? If you then have to look up a macro first, it is a
cumbersome indirection. #defines are certainly fine for things that are
used multiple times, but here the strings are only used once, so the
indirection is really not needed.

 Thomas
diff mbox series

Patch

diff --git a/tests/test-replication.c b/tests/test-replication.c
index 9ab3666a90..d0e06f8d77 100644
--- a/tests/test-replication.c
+++ b/tests/test-replication.c
@@ -23,14 +23,18 @@ 
 
 /* primary */
 #define P_ID "primary-id"
-static char p_local_disk[] = "/tmp/p_local_disk.XXXXXX";
+#define P_LOCAL_DISK "%s/p_local_disk.XXXXXX"
+static char p_local_disk[PATH_MAX];
 
 /* secondary */
 #define S_ID "secondary-id"
 #define S_LOCAL_DISK_ID "secondary-local-disk-id"
-static char s_local_disk[] = "/tmp/s_local_disk.XXXXXX";
-static char s_active_disk[] = "/tmp/s_active_disk.XXXXXX";
-static char s_hidden_disk[] = "/tmp/s_hidden_disk.XXXXXX";
+#define S_LOCAL_DISK "%s/s_local_disk.XXXXXX"
+static char s_local_disk[PATH_MAX];
+#define S_ACTIVE_DISK "%s/s_active_disk.XXXXXX"
+static char s_active_disk[PATH_MAX];
+#define S_HIDDEN_DISK "%s/s_hidden_disk.XXXXXX"
+static char s_hidden_disk[PATH_MAX];
 
 /* FIXME: steal from blockdev.c */
 QemuOptsList qemu_drive_opts = {
@@ -571,7 +575,12 @@  static void setup_sigabrt_handler(void)
 int main(int argc, char **argv)
 {
     int ret;
+    const char *tmpdir = g_get_tmp_dir();
     qemu_init_main_loop(&error_fatal);
+    sprintf(p_local_disk, P_LOCAL_DISK, tmpdir);
+    sprintf(s_local_disk, S_LOCAL_DISK, tmpdir);
+    sprintf(s_active_disk, S_ACTIVE_DISK, tmpdir);
+    sprintf(s_hidden_disk, S_HIDDEN_DISK, tmpdir);
     bdrv_init();
 
     g_test_init(&argc, &argv, NULL);