thr3ads.net - Libguestfs - [Libguestfs] [libnbd PATCH] maint: Drop useless casts [May 2023]

If this information is useful, please help other people find it:
Share via:

Eric Blake

2023-May-24 15:00 UTC

[Libguestfs] [libnbd PATCH] maint: Drop useless casts

While reviewing the previous whitespace-only patch, I noticed several
casts that are semantically unnecessary in C:

- converting pointers to and from void* does not need a cast except
  for function pointers or when also casting away const (different
  from C++ where casts to or from void* are required for type safety);
  includes cases like python PyCapsule code which uses void*

- casting away const on a string literal (in C, string literals are
  typed 'char *', different from C++ where they are 'const char
*';
  that said, C says it is still undefined behavior to modify their
  contents, so string literals should generally be only assigned to
  'const char *' variables or passed to functions that promise not to
  modify contents); includes cases like ocaml's struct
  custom_operations using 'const char *' as its member type (where
  casting away const just to re-add it is pointless)

- casts used to document what would otherwise be implicit type
  conversions when assigning to integers of a different type or
  passing function parameters not through varargs. While this can be
  useful for documentation purposes, it can also mask bugs when the
  cast uses the wrong intermediate type only to be implicitly
  converted back to the destination type, and it is often easier to
  use optional compiler warning flags to catch instances of unintended
  truncation or sign extension by implicit conversions.

Minimizing semantically unneeded casts lets the remaining casts stand
out for why they are necessary: casting aways const, casts between
pointer aliases (such as struct sockaddr*, char* vs unsigned char*,
...), and scalars passed through vargs in printf-like functions.

Signed-off-by: Eric Blake <eblake at redhat.com>
---
 generator/Python.ml                          | 2 +-
 generator/states-connect-socket-activation.c | 2 +-
 generator/states-issue-command.c             | 2 +-
 lib/utils.c                                  | 4 ++--
 python/handle.c                              | 2 +-
 ocaml/nbd-c.h                                | 6 +++---
 tests/opt-set-meta.c                         | 2 +-
 copy/main.c                                  | 6 +++---
 fuse/nbdfuse.c                               | 4 ++--
 fuse/operations.c                            | 2 +-
 ublk/nbdublk.c                               | 4 ++--
 11 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/generator/Python.ml b/generator/Python.ml
index b73f9782..c81878de 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -49,7 +49,7 @@ let
 {
   assert (obj);
   assert (obj != Py_None);
-  return (struct nbd_handle *)PyCapsule_GetPointer(obj,
\"nbd_handle\");
+  return PyCapsule_GetPointer(obj, \"nbd_handle\");
 }

 /* nbd.Error exception. */
diff --git a/generator/states-connect-socket-activation.c
b/generator/states-connect-socket-activation.c
index 98a39c0e..40a3528c 100644
--- a/generator/states-connect-socket-activation.c
+++ b/generator/states-connect-socket-activation.c
@@ -294,7 +294,7 @@  CONNECT_SA.START:

     char buf[32];
     const char *v -      nbd_internal_fork_safe_itoa ((long)getpid (), buf,
sizeof buf);
+      nbd_internal_fork_safe_itoa (getpid (), buf, sizeof buf);
     NBD_INTERNAL_FORK_SAFE_ASSERT (strlen (v) <=
sact_var[pid_ofs].value_len);
     strcpy (env.ptr[pid_ofs] + sact_var[pid_ofs].prefix_len, v);

diff --git a/generator/states-issue-command.c b/generator/states-issue-command.c
index 34ef4652..111e131c 100644
--- a/generator/states-issue-command.c
+++ b/generator/states-issue-command.c
@@ -46,7 +46,7 @@  ISSUE_COMMAND.START:
   h->request.type = htobe16 (cmd->type);
   h->request.handle = htobe64 (cmd->cookie);
   h->request.offset = htobe64 (cmd->offset);
-  h->request.count = htobe32 ((uint32_t)cmd->count);
+  h->request.count = htobe32 (cmd->count);
   h->chunks_sent++;
   h->wbuf = &h->request;
   h->wlen = sizeof (h->request);
diff --git a/lib/utils.c b/lib/utils.c
index e3e13cdd..5df5ae51 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -154,7 +154,7 @@ nbd_internal_set_querylist (struct nbd_handle *h, char
**queries)
 const char *
 nbd_internal_fork_safe_itoa (long v, char *buf, size_t bufsize)
 {
-  unsigned long uv = (unsigned long)v;
+  unsigned long uv = v;
   size_t i = bufsize - 1;
   bool neg = false;

@@ -282,7 +282,7 @@ nbd_internal_fork_safe_perror (const char *s)
 #endif
 #endif
   if (!m)
-    m = nbd_internal_fork_safe_itoa ((long)errno, buf, sizeof buf);
+    m = nbd_internal_fork_safe_itoa (errno, buf, sizeof buf);
   xwritel (STDERR_FILENO, s, ": ", m, "\n", (char *)NULL);

   /* Restore original errno in case it was disturbed by the system
diff --git a/python/handle.c b/python/handle.c
index 8ff6ba81..548f2dfe 100644
--- a/python/handle.c
+++ b/python/handle.c
@@ -46,7 +46,7 @@ static inline PyObject *
 put_handle (struct nbd_handle *h)
 {
   assert (h);
-  return PyCapsule_New ((void *)h, "nbd_handle", NULL);
+  return PyCapsule_New (h, "nbd_handle", NULL);
 }

 PyObject *
diff --git a/ocaml/nbd-c.h b/ocaml/nbd-c.h
index 0cbe36d1..e3abb912 100644
--- a/ocaml/nbd-c.h
+++ b/ocaml/nbd-c.h
@@ -49,7 +49,7 @@ static inline value
 caml_alloc_initialized_string (mlsize_t len, const char *p)
 {
   value sv = caml_alloc_string (len);
-  memcpy ((char *)String_val (sv), p, len);
+  memcpy (String_val (sv), p, len);
   return sv;
 }
 #endif
@@ -70,7 +70,7 @@ extern void nbd_internal_ocaml_exception_in_wrapper (const
char *, value);
 #define NBD_val(v) (*((struct nbd_handle **)Data_custom_val (v)))

 static struct custom_operations libnbd_custom_operations = {
-  (char *)"libnbd_custom_operations",
+  "libnbd_custom_operations",
   nbd_internal_ocaml_handle_finalize,
   custom_compare_default,
   custom_hash_default,
@@ -110,7 +110,7 @@ struct nbd_buffer {
 #define NBD_buffer_val(v) ((struct nbd_buffer *)Data_custom_val (v))

 static struct custom_operations nbd_buffer_custom_operations = {
-  (char *)"nbd_buffer_custom_operations",
+  "nbd_buffer_custom_operations",
   nbd_internal_ocaml_buffer_finalize,
   custom_compare_default,
   custom_hash_default,
diff --git a/tests/opt-set-meta.c b/tests/opt-set-meta.c
index 1bd60a9c..06f503af 100644
--- a/tests/opt-set-meta.c
+++ b/tests/opt-set-meta.c
@@ -219,7 +219,7 @@ main (int argc, char *argv[])
    * or newer with its --no-sr kill switch.
    */
   requires ("nbdkit --no-sr --help");
-  args[ARRAY_SIZE (args) - 2] = (char *)"--no-sr";
+  args[ARRAY_SIZE (args) - 2] = "--no-sr";
   nbd = nbd_create ();
   if (nbd == NULL ||
       nbd_set_opt_mode (nbd, true) == -1 ||
diff --git a/copy/main.c b/copy/main.c
index 391c0c4f..9449440e 100644
--- a/copy/main.c
+++ b/copy/main.c
@@ -371,7 +371,7 @@ main (int argc, char *argv[])
 #else
     t = 1;
 #endif
-    threads = (unsigned)t;
+    threads = t;
   }

   if (synchronous)
@@ -534,7 +534,7 @@ open_local (const char *filename, direction d)
   }
   if (S_ISREG (stat.st_mode))   /* Regular file. */
     return file_create (filename, fd,
-                        stat.st_size, (uint64_t)stat.st_blksize, false, d);
+                        stat.st_size, stat.st_blksize, false, d);
   else if (S_ISBLK (stat.st_mode)) { /* Block device. */
     unsigned int blkioopt;

@@ -549,7 +549,7 @@ open_local (const char *filename, direction d)
 #endif

     return file_create (filename, fd,
-                        stat.st_size, (uint64_t)blkioopt, true, d);
+                        stat.st_size, blkioopt, true, d);
   }
   else {              /* Probably stdin/stdout, a pipe or a socket. */
     synchronous = true;        /* Force synchronous mode for pipes. */
diff --git a/fuse/nbdfuse.c b/fuse/nbdfuse.c
index 491f6db8..35d5ffac 100644
--- a/fuse/nbdfuse.c
+++ b/fuse/nbdfuse.c
@@ -415,7 +415,7 @@ main (int argc, char *argv[])
       handles_append (&nbd, h); /* reserved above, so can't fail */
     }
   }
-  connections = (unsigned)nbd.len;
+  connections = nbd.len;
   if (verbose)
     fprintf (stderr, "nbdfuse: connections=%u\n", connections);

@@ -424,7 +424,7 @@ main (int argc, char *argv[])
     fprintf (stderr, "%s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
   }
-  size = (uint64_t)ssize;
+  size = ssize;

   /* If the remote NBD server is readonly, then act as if the '-r'
    * flag was given on the nbdfuse command line.
diff --git a/fuse/operations.c b/fuse/operations.c
index 2d0634dc..e487d150 100644
--- a/fuse/operations.c
+++ b/fuse/operations.c
@@ -396,7 +396,7 @@ nbdfuse_read (const char *path, char *buf,

   CHECK_NBD_ASYNC_ERROR (nbd_aio_pread (h, buf, count, offset, cb, 0));

-  return (int)count;
+  return count;
 }

 static int
diff --git a/ublk/nbdublk.c b/ublk/nbdublk.c
index b85ac609..2f097f3e 100644
--- a/ublk/nbdublk.c
+++ b/ublk/nbdublk.c
@@ -347,7 +347,7 @@ main (int argc, char *argv[])
       handles_append (&nbd, h); /* reserved above, so can't fail */
     }
   }
-  connections = (unsigned)nbd.len;
+  connections = nbd.len;

   /* Get the size and preferred block sizes. */
   rs = nbd_get_size (nbd.ptr[0]);
@@ -355,7 +355,7 @@ main (int argc, char *argv[])
     fprintf (stderr, "%s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
   }
-  size = (uint64_t)rs;
+  size = rs;

   rs = nbd_get_block_size (nbd.ptr[0], LIBNBD_SIZE_MAXIMUM);
   if (rs <= 0 || rs > 64 * 1024 * 1024)
-- 
2.40.1

Laszlo Ersek

2023-May-24 16:57 UTC

head link

[Libguestfs] [libnbd PATCH] maint: Drop useless casts

On 5/24/23 17:00, Eric Blake wrote:> While reviewing the previous whitespace-only patch, I noticed several
> casts that are semantically unnecessary in C:
> 
> - converting pointers to and from void* does not need a cast except
>   for function pointers or when also casting away const (different
>   from C++ where casts to or from void* are required for type safety);
>   includes cases like python PyCapsule code which uses void*
> 
> - casting away const on a string literal (in C, string literals are
>   typed 'char *', different from C++ where they are 'const char
*';
>   that said, C says it is still undefined behavior to modify their
>   contents, so string literals should generally be only assigned to
>   'const char *' variables or passed to functions that promise not
to
>   modify contents); includes cases like ocaml's struct
>   custom_operations using 'const char *' as its member type (where
>   casting away const just to re-add it is pointless)
> 
> - casts used to document what would otherwise be implicit type
>   conversions when assigning to integers of a different type or
>   passing function parameters not through varargs. While this can be
>   useful for documentation purposes, it can also mask bugs when the
>   cast uses the wrong intermediate type only to be implicitly
>   converted back to the destination type, and it is often easier to
>   use optional compiler warning flags to catch instances of unintended
>   truncation or sign extension by implicit conversions.
> 
> Minimizing semantically unneeded casts lets the remaining casts stand
> out for why they are necessary: casting aways const, casts between
> pointer aliases (such as struct sockaddr*, char* vs unsigned char*,
> ...), and scalars passed through vargs in printf-like functions.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  generator/Python.ml                          | 2 +-
>  generator/states-connect-socket-activation.c | 2 +-
>  generator/states-issue-command.c             | 2 +-
>  lib/utils.c                                  | 4 ++--
>  python/handle.c                              | 2 +-
>  ocaml/nbd-c.h                                | 6 +++---
>  tests/opt-set-meta.c                         | 2 +-
>  copy/main.c                                  | 6 +++---
>  fuse/nbdfuse.c                               | 4 ++--
>  fuse/operations.c                            | 2 +-
>  ublk/nbdublk.c                               | 4 ++--
>  11 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/generator/Python.ml b/generator/Python.ml
> index b73f9782..c81878de 100644
> --- a/generator/Python.ml
> +++ b/generator/Python.ml
> @@ -49,7 +49,7 @@ let
>  {
>    assert (obj);
>    assert (obj != Py_None);
> -  return (struct nbd_handle *)PyCapsule_GetPointer(obj,
\"nbd_handle\");
> +  return PyCapsule_GetPointer(obj, \"nbd_handle\");
>  }
> 
>  /* nbd.Error exception. */
> diff --git a/generator/states-connect-socket-activation.c
b/generator/states-connect-socket-activation.c
> index 98a39c0e..40a3528c 100644
> --- a/generator/states-connect-socket-activation.c
> +++ b/generator/states-connect-socket-activation.c
> @@ -294,7 +294,7 @@  CONNECT_SA.START:
> 
>      char buf[32];
>      const char *v > -      nbd_internal_fork_safe_itoa ((long)getpid
(), buf, sizeof buf);
> +      nbd_internal_fork_safe_itoa (getpid (), buf, sizeof buf);
>      NBD_INTERNAL_FORK_SAFE_ASSERT (strlen (v) <=
sact_var[pid_ofs].value_len);
>      strcpy (env.ptr[pid_ofs] + sact_var[pid_ofs].prefix_len, v);
> 
> diff --git a/generator/states-issue-command.c
b/generator/states-issue-command.c
> index 34ef4652..111e131c 100644
> --- a/generator/states-issue-command.c
> +++ b/generator/states-issue-command.c
> @@ -46,7 +46,7 @@  ISSUE_COMMAND.START:
>    h->request.type = htobe16 (cmd->type);
>    h->request.handle = htobe64 (cmd->cookie);
>    h->request.offset = htobe64 (cmd->offset);
> -  h->request.count = htobe32 ((uint32_t)cmd->count);
> +  h->request.count = htobe32 (cmd->count);
>    h->chunks_sent++;
>    h->wbuf = &h->request;
>    h->wlen = sizeof (h->request);
> diff --git a/lib/utils.c b/lib/utils.c
> index e3e13cdd..5df5ae51 100644
> --- a/lib/utils.c
> +++ b/lib/utils.c
> @@ -154,7 +154,7 @@ nbd_internal_set_querylist (struct nbd_handle *h, char
**queries)
>  const char *
>  nbd_internal_fork_safe_itoa (long v, char *buf, size_t bufsize)
>  {
> -  unsigned long uv = (unsigned long)v;
> +  unsigned long uv = v;
>    size_t i = bufsize - 1;
>    bool neg = false;
> 
> @@ -282,7 +282,7 @@ nbd_internal_fork_safe_perror (const char *s)
>  #endif
>  #endif
>    if (!m)
> -    m = nbd_internal_fork_safe_itoa ((long)errno, buf, sizeof buf);
> +    m = nbd_internal_fork_safe_itoa (errno, buf, sizeof buf);
>    xwritel (STDERR_FILENO, s, ": ", m, "\n", (char
*)NULL);
> 
>    /* Restore original errno in case it was disturbed by the system
> diff --git a/python/handle.c b/python/handle.c
> index 8ff6ba81..548f2dfe 100644
> --- a/python/handle.c
> +++ b/python/handle.c
> @@ -46,7 +46,7 @@ static inline PyObject *
>  put_handle (struct nbd_handle *h)
>  {
>    assert (h);
> -  return PyCapsule_New ((void *)h, "nbd_handle", NULL);
> +  return PyCapsule_New (h, "nbd_handle", NULL);
>  }
> 
>  PyObject *
> diff --git a/ocaml/nbd-c.h b/ocaml/nbd-c.h
> index 0cbe36d1..e3abb912 100644
> --- a/ocaml/nbd-c.h
> +++ b/ocaml/nbd-c.h
> @@ -49,7 +49,7 @@ static inline value
>  caml_alloc_initialized_string (mlsize_t len, const char *p)
>  {
>    value sv = caml_alloc_string (len);
> -  memcpy ((char *)String_val (sv), p, len);
> +  memcpy (String_val (sv), p, len);
>    return sv;
>  }
>  #endif
> @@ -70,7 +70,7 @@ extern void nbd_internal_ocaml_exception_in_wrapper
(const char *, value);
>  #define NBD_val(v) (*((struct nbd_handle **)Data_custom_val (v)))
> 
>  static struct custom_operations libnbd_custom_operations = {
> -  (char *)"libnbd_custom_operations",
> +  "libnbd_custom_operations",
>    nbd_internal_ocaml_handle_finalize,
>    custom_compare_default,
>    custom_hash_default,
> @@ -110,7 +110,7 @@ struct nbd_buffer {
>  #define NBD_buffer_val(v) ((struct nbd_buffer *)Data_custom_val (v))
> 
>  static struct custom_operations nbd_buffer_custom_operations = {
> -  (char *)"nbd_buffer_custom_operations",
> +  "nbd_buffer_custom_operations",
>    nbd_internal_ocaml_buffer_finalize,
>    custom_compare_default,
>    custom_hash_default,
> diff --git a/tests/opt-set-meta.c b/tests/opt-set-meta.c
> index 1bd60a9c..06f503af 100644
> --- a/tests/opt-set-meta.c
> +++ b/tests/opt-set-meta.c
> @@ -219,7 +219,7 @@ main (int argc, char *argv[])
>     * or newer with its --no-sr kill switch.
>     */
>    requires ("nbdkit --no-sr --help");
> -  args[ARRAY_SIZE (args) - 2] = (char *)"--no-sr";
> +  args[ARRAY_SIZE (args) - 2] = "--no-sr";
>    nbd = nbd_create ();
>    if (nbd == NULL ||
>        nbd_set_opt_mode (nbd, true) == -1 ||
> diff --git a/copy/main.c b/copy/main.c
> index 391c0c4f..9449440e 100644
> --- a/copy/main.c
> +++ b/copy/main.c
> @@ -371,7 +371,7 @@ main (int argc, char *argv[])
>  #else
>      t = 1;
>  #endif
> -    threads = (unsigned)t;
> +    threads = t;
>    }
> 
>    if (synchronous)
> @@ -534,7 +534,7 @@ open_local (const char *filename, direction d)
>    }
>    if (S_ISREG (stat.st_mode))   /* Regular file. */
>      return file_create (filename, fd,
> -                        stat.st_size, (uint64_t)stat.st_blksize, false,
d);
> +                        stat.st_size, stat.st_blksize, false, d);
>    else if (S_ISBLK (stat.st_mode)) { /* Block device. */
>      unsigned int blkioopt;
> 
> @@ -549,7 +549,7 @@ open_local (const char *filename, direction d)
>  #endif
> 
>      return file_create (filename, fd,
> -                        stat.st_size, (uint64_t)blkioopt, true, d);
> +                        stat.st_size, blkioopt, true, d);
>    }
>    else {              /* Probably stdin/stdout, a pipe or a socket. */
>      synchronous = true;        /* Force synchronous mode for pipes. */
> diff --git a/fuse/nbdfuse.c b/fuse/nbdfuse.c
> index 491f6db8..35d5ffac 100644
> --- a/fuse/nbdfuse.c
> +++ b/fuse/nbdfuse.c
> @@ -415,7 +415,7 @@ main (int argc, char *argv[])
>        handles_append (&nbd, h); /* reserved above, so can't fail
*/
>      }
>    }
> -  connections = (unsigned)nbd.len;
> +  connections = nbd.len;
>    if (verbose)
>      fprintf (stderr, "nbdfuse: connections=%u\n", connections);
> 
> @@ -424,7 +424,7 @@ main (int argc, char *argv[])
>      fprintf (stderr, "%s\n", nbd_get_error ());
>      exit (EXIT_FAILURE);
>    }
> -  size = (uint64_t)ssize;
> +  size = ssize;
> 
>    /* If the remote NBD server is readonly, then act as if the '-r'
>     * flag was given on the nbdfuse command line.
> diff --git a/fuse/operations.c b/fuse/operations.c
> index 2d0634dc..e487d150 100644
> --- a/fuse/operations.c
> +++ b/fuse/operations.c
> @@ -396,7 +396,7 @@ nbdfuse_read (const char *path, char *buf,
> 
>    CHECK_NBD_ASYNC_ERROR (nbd_aio_pread (h, buf, count, offset, cb, 0));
> 
> -  return (int)count;
> +  return count;
>  }
> 
>  static int
> diff --git a/ublk/nbdublk.c b/ublk/nbdublk.c
> index b85ac609..2f097f3e 100644
> --- a/ublk/nbdublk.c
> +++ b/ublk/nbdublk.c
> @@ -347,7 +347,7 @@ main (int argc, char *argv[])
>        handles_append (&nbd, h); /* reserved above, so can't fail
*/
>      }
>    }
> -  connections = (unsigned)nbd.len;
> +  connections = nbd.len;
> 
>    /* Get the size and preferred block sizes. */
>    rs = nbd_get_size (nbd.ptr[0]);
> @@ -355,7 +355,7 @@ main (int argc, char *argv[])
>      fprintf (stderr, "%s\n", nbd_get_error ());
>      exit (EXIT_FAILURE);
>    }
> -  size = (uint64_t)rs;
> +  size = rs;
> 
>    rs = nbd_get_block_size (nbd.ptr[0], LIBNBD_SIZE_MAXIMUM);
>    if (rs <= 0 || rs > 64 * 1024 * 1024)
Reviewed-by: Laszlo Ersek <lersek at redhat.com>

Richard W.M. Jones

2023-May-24 19:18 UTC

head link

[Libguestfs] [libnbd PATCH] maint: Drop useless casts

On Wed, May 24, 2023 at 10:00:03AM -0500, Eric Blake
wrote:> diff --git a/copy/main.c b/copy/main.c
> index 391c0c4f..9449440e 100644
> --- a/copy/main.c
> +++ b/copy/main.c
> @@ -371,7 +371,7 @@ main (int argc, char *argv[])
>  #else
>      t = 1;
>  #endif
> -    threads = (unsigned)t;
> +    threads = t;
This one doesn't give any warning about truncating from long (64 bits)
to unsigned (32 bits)?
> diff --git a/fuse/nbdfuse.c b/fuse/nbdfuse.c
> index 491f6db8..35d5ffac 100644
> --- a/fuse/nbdfuse.c
> +++ b/fuse/nbdfuse.c
> @@ -415,7 +415,7 @@ main (int argc, char *argv[])
>        handles_append (&nbd, h); /* reserved above, so can't fail
*/
>      }
>    }
> -  connections = (unsigned)nbd.len;
> +  connections = nbd.len;
Similarly this would be size_t -> unsigned, also a truncation on 64 bit.

But if there are no warnings, then:

Reviewed-by: Richard W.M. Jones <rjones at redhat.com>

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Libguestfs - May 2023 - [libnbd PATCH] maint: Drop useless casts

[Libguestfs] [libnbd PATCH] maint: Drop useless casts

[Libguestfs] [libnbd PATCH] maint: Drop useless casts

[Libguestfs] [libnbd PATCH] maint: Drop useless casts