[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [PATCH] Re: [libvirt] three more clang-inspired dead-store-fixing patches



On Fri, Sep 04, 2009 at 11:34:09AM +0200, Jim Meyering wrote:
> Jim Meyering wrote:
> >>From a2d03c987bb724283207dbeef873178c08a6c4c5 Mon Sep 17 00:00:00 2001
> > From: Jim Meyering <meyering redhat com>
> > Date: Thu, 3 Sep 2009 18:14:48 +0200
> > Subject: [PATCH 1/3] storage_backend_logical.c: appease clang: remove useless increment
> ...
> >>From b72ed1ba5d5e6444f000fd9be706a51a90dd2292 Mon Sep 17 00:00:00 2001
> > From: Jim Meyering <meyering redhat com>
> > Date: Thu, 3 Sep 2009 18:23:51 +0200
> > Subject: [PATCH 2/3] openvz_conf.c: Remove dead store to copy_fd
> ...
> >>From a8d2eca13635578084e3c2cc2093562d9a8fcfed Mon Sep 17 00:00:00 2001
> > From: Jim Meyering <meyering redhat com>
> > Date: Thu, 3 Sep 2009 18:25:03 +0200
> > Subject: [PATCH 3/3] eventtest.c: detect write failure and avoid dead stores
> >
> > * tests/eventtest.c (mymain): Exit nonzero upon write failure.
> > This also avoids several dead stores of the form ret = safewrite...
> ...
> 
> Here's a slight adjustment.
> That third patch missed one of these:
> 
> -    ret = safewrite(handles[1].pipeFD[1], &one, 1);
> +    if (safewrite(handles[1].pipeFD[1], &one, 1) != 1)
> +        return EXIT_FAILURE;
> 
> It's corrected below:
> 
> >From 7cba5226a7a1f1626263e5f35a0e342c7c4d7151 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering redhat com>
> Date: Thu, 3 Sep 2009 18:14:48 +0200
> Subject: [PATCH 1/3] storage_backend_logical.c: appease clang: remove useless increment
> 
> * src/storage_backend_logical.c (virStorageBackendLogicalBuildPool):
> Don't increment "n" when we won't use the result.
> ---
>  src/storage_backend_logical.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/src/storage_backend_logical.c b/src/storage_backend_logical.c
> index bc40dd7..43117e8 100644
> --- a/src/storage_backend_logical.c
> +++ b/src/storage_backend_logical.c
> @@ -437,7 +437,7 @@ virStorageBackendLogicalBuildPool(virConnectPtr conn,
>              goto cleanup;
>      }
> 
> -    vgargv[n++] = NULL;
> +    vgargv[n] = NULL;
> 
>      /* Now create the volume group itself */
>      if (virRun(conn, vgargv, NULL) < 0)
> --
> 1.6.4.2.409.g85dc3

  ACK

> 
> >From 340fe0e28a613b3c15f3c64cb17b4ba64e293128 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering redhat com>
> Date: Thu, 3 Sep 2009 18:23:51 +0200
> Subject: [PATCH 2/3] openvz_conf.c: Remove dead store to copy_fd
> 
> * src/openvz_conf.c (openvz_copyfile): Remove unused assignment.
> ---
>  src/openvz_conf.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/src/openvz_conf.c b/src/openvz_conf.c
> index 41c6684..2f0471c 100644
> --- a/src/openvz_conf.c
> +++ b/src/openvz_conf.c
> @@ -692,7 +692,6 @@ openvz_copyfile(char* from_path, char* to_path)
>      fd = -1;
>      if (close(copy_fd) < 0)
>          goto error;
> -    copy_fd = -1;
> 
>      return 0;
> 
> --
> 1.6.4.2.409.g85dc3

  ACK, though I would expect the compiler is able to optimize this :-)

> >From e6fa6de8689fda71920cf0918a1022230d2cad70 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering redhat com>
> Date: Thu, 3 Sep 2009 18:25:03 +0200
> Subject: [PATCH 3/3] eventtest.c: detect write failure and avoid dead stores
> 
> * tests/eventtest.c (mymain): Exit nonzero upon write failure.
> This also avoids several dead stores of the form ret = safewrite...
> ---
>  tests/eventtest.c |   18 +++++++++++-------
>  1 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/eventtest.c b/tests/eventtest.c
> index 68ac2fc..da34388 100644
> --- a/tests/eventtest.c
> +++ b/tests/eventtest.c
> @@ -248,7 +248,6 @@ resetAll(void)
>  static int
>  mymain(int argc, char **argv)
>  {
> -    int ret = 0;
>      char *progname;
>      int i;
>      pthread_t eventThread;
> @@ -304,7 +303,8 @@ mymain(int argc, char **argv)
>      /* First time, is easy - just try triggering one of our
>       * registered handles */
>      startJob("Simple write", &test);
> -    ret = safewrite(handles[1].pipeFD[1], &one, 1);
> +    if (safewrite(handles[1].pipeFD[1], &one, 1) != 1)
> +        return EXIT_FAILURE;
>      if (finishJob(1, -1) != EXIT_SUCCESS)
>          return EXIT_FAILURE;
> 
> @@ -314,7 +314,8 @@ mymain(int argc, char **argv)
>       * try triggering another handle */
>      virEventRemoveHandleImpl(handles[0].watch);
>      startJob("Deleted before poll", &test);
> -    ret = safewrite(handles[1].pipeFD[1], &one, 1);
> +    if (safewrite(handles[1].pipeFD[1], &one, 1) != 1)
> +        return EXIT_FAILURE;
>      if (finishJob(1, -1) != EXIT_SUCCESS)
>          return EXIT_FAILURE;
> 
> @@ -346,8 +347,9 @@ mymain(int argc, char **argv)
>       * about */
>      startJob("Deleted during dispatch", &test);
>      handles[2].delete = handles[3].watch;
> -    ret = safewrite(handles[2].pipeFD[1], &one, 1);
> -    ret = safewrite(handles[3].pipeFD[1], &one, 1);
> +    if (safewrite(handles[2].pipeFD[1], &one, 1) != 1
> +        || safewrite(handles[3].pipeFD[1], &one, 1) != 1)

<nitpick> I prefer || at end of previous line and superfluous
          but nicer to read extra parenthesis :-) </nitpick>

> +        return EXIT_FAILURE;
>      if (finishJob(2, -1) != EXIT_SUCCESS)
>          return EXIT_FAILURE;
> 
> @@ -356,7 +358,8 @@ mymain(int argc, char **argv)
>      /* Extreme fun, lets delete ourselves during dispatch */
>      startJob("Deleted during dispatch", &test);
>      handles[2].delete = handles[2].watch;
> -    ret = safewrite(handles[2].pipeFD[1], &one, 1);
> +    if (safewrite(handles[2].pipeFD[1], &one, 1) != 1)
> +        return EXIT_FAILURE;
>      if (finishJob(2, -1) != EXIT_SUCCESS)
>          return EXIT_FAILURE;
> 
> @@ -446,7 +449,8 @@ mymain(int argc, char **argv)
>                                               testPipeReader,
>                                               &handles[1], NULL);
>      startJob("Write duplicate", &test);
> -    ret = safewrite(handles[1].pipeFD[1], &one, 1);
> +    if (safewrite(handles[1].pipeFD[1], &one, 1) != 1)
> +        return EXIT_FAILURE;
>      if (finishJob(1, -1) != EXIT_SUCCESS)
>          return EXIT_FAILURE;
> 

 ACK,

   thanks !

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]