hw/ide: Remove unuseful IDE_DMA__COUNT definition

[PATCH] hw/ide: Remove unuseful IDE_DMA__COUNT definition

Posted by Philippe Mathieu-Daudé 1 year, 2 months ago

First, IDE_DMA__COUNT represents the number of enumerated
values, but is incorrectly listed as part of the enum.

Second, IDE_DMA_CMD_str() is internal to core.c and only
takes sane enums from ide_dma_cmd. So no need to check the
'enval' argument is within the enum range. Only checks
IDE_DMA_CMD_lookup[] entry is not NULL.

Both combined, IDE_DMA__COUNT can go.

Reduce IDE_DMA_CMD_lookup[] scope which is only used locally.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ide/core.c             | 10 +++++-----
 include/hw/ide/internal.h |  3 ---
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 5d1039378f..8bf61e7267 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -63,19 +63,19 @@ static const int smart_attributes[][12] = {
     { 190,  0x03, 0x00, 0x45, 0x45, 0x1f, 0x00, 0x1f, 0x1f, 0x00, 0x00, 0x32},
 };
 
-const char *IDE_DMA_CMD_lookup[IDE_DMA__COUNT] = {
+static const char *IDE_DMA_CMD_lookup[] = {
     [IDE_DMA_READ] = "DMA READ",
     [IDE_DMA_WRITE] = "DMA WRITE",
     [IDE_DMA_TRIM] = "DMA TRIM",
-    [IDE_DMA_ATAPI] = "DMA ATAPI"
+    [IDE_DMA_ATAPI] = "DMA ATAPI",
 };
 
 static const char *IDE_DMA_CMD_str(enum ide_dma_cmd enval)
 {
-    if ((unsigned)enval < IDE_DMA__COUNT) {
-        return IDE_DMA_CMD_lookup[enval];
+    if (!IDE_DMA_CMD_lookup[enval]) {
+        return "DMA UNKNOWN CMD";
     }
-    return "DMA UNKNOWN CMD";
+    return IDE_DMA_CMD_lookup[enval];
 }
 
 static void ide_dummy_transfer_stop(IDEState *s);
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index fc0aa81a88..e864fe8caf 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -352,11 +352,8 @@ enum ide_dma_cmd {
     IDE_DMA_WRITE,
     IDE_DMA_TRIM,
     IDE_DMA_ATAPI,
-    IDE_DMA__COUNT
 };
 
-extern const char *IDE_DMA_CMD_lookup[IDE_DMA__COUNT];
-
 #define ide_cmd_is_read(s) \
         ((s)->dma_cmd == IDE_DMA_READ)
 
-- 
2.38.1

Re: [PATCH] hw/ide: Remove unuseful IDE_DMA__COUNT definition

Posted by John Snow 11 months, 2 weeks ago

On Fri, Feb 24, 2023 at 10:34 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> First, IDE_DMA__COUNT represents the number of enumerated
> values, but is incorrectly listed as part of the enum.
>
> Second, IDE_DMA_CMD_str() is internal to core.c and only
> takes sane enums from ide_dma_cmd. So no need to check the
> 'enval' argument is within the enum range. Only checks
> IDE_DMA_CMD_lookup[] entry is not NULL.
>
> Both combined, IDE_DMA__COUNT can go.
>
> Reduce IDE_DMA_CMD_lookup[] scope which is only used locally.
>

You reviewed the patch where this got written in :)

I didn't think anything actually protected us from giving
IDE_DMA_CMD_str() a bogus integer. I'm not familiar with the idea that
it takes "only sane enums". Is that true? Or, is it just because it's
internal to the file that we can statically prove that it's true?

--js

> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/ide/core.c             | 10 +++++-----
>  include/hw/ide/internal.h |  3 ---
>  2 files changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 5d1039378f..8bf61e7267 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -63,19 +63,19 @@ static const int smart_attributes[][12] = {
>      { 190,  0x03, 0x00, 0x45, 0x45, 0x1f, 0x00, 0x1f, 0x1f, 0x00, 0x00, 0x32},
>  };
>
> -const char *IDE_DMA_CMD_lookup[IDE_DMA__COUNT] = {
> +static const char *IDE_DMA_CMD_lookup[] = {
>      [IDE_DMA_READ] = "DMA READ",
>      [IDE_DMA_WRITE] = "DMA WRITE",
>      [IDE_DMA_TRIM] = "DMA TRIM",
> -    [IDE_DMA_ATAPI] = "DMA ATAPI"
> +    [IDE_DMA_ATAPI] = "DMA ATAPI",
>  };
>
>  static const char *IDE_DMA_CMD_str(enum ide_dma_cmd enval)
>  {
> -    if ((unsigned)enval < IDE_DMA__COUNT) {
> -        return IDE_DMA_CMD_lookup[enval];
> +    if (!IDE_DMA_CMD_lookup[enval]) {
> +        return "DMA UNKNOWN CMD";
>      }
> -    return "DMA UNKNOWN CMD";
> +    return IDE_DMA_CMD_lookup[enval];
>  }
>
>  static void ide_dummy_transfer_stop(IDEState *s);
> diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
> index fc0aa81a88..e864fe8caf 100644
> --- a/include/hw/ide/internal.h
> +++ b/include/hw/ide/internal.h
> @@ -352,11 +352,8 @@ enum ide_dma_cmd {
>      IDE_DMA_WRITE,
>      IDE_DMA_TRIM,
>      IDE_DMA_ATAPI,
> -    IDE_DMA__COUNT
>  };
>
> -extern const char *IDE_DMA_CMD_lookup[IDE_DMA__COUNT];
> -
>  #define ide_cmd_is_read(s) \
>          ((s)->dma_cmd == IDE_DMA_READ)
>
> --
> 2.38.1
>

Re: [PATCH] hw/ide: Remove unuseful IDE_DMA__COUNT definition

Posted by Philippe Mathieu-Daudé 1 year, 1 month ago

ping?

On 24/2/23 16:34, Philippe Mathieu-Daudé wrote:
> First, IDE_DMA__COUNT represents the number of enumerated
> values, but is incorrectly listed as part of the enum.
> 
> Second, IDE_DMA_CMD_str() is internal to core.c and only
> takes sane enums from ide_dma_cmd. So no need to check the
> 'enval' argument is within the enum range. Only checks
> IDE_DMA_CMD_lookup[] entry is not NULL.
> 
> Both combined, IDE_DMA__COUNT can go.
> 
> Reduce IDE_DMA_CMD_lookup[] scope which is only used locally.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/ide/core.c             | 10 +++++-----
>   include/hw/ide/internal.h |  3 ---
>   2 files changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 5d1039378f..8bf61e7267 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -63,19 +63,19 @@ static const int smart_attributes[][12] = {
>       { 190,  0x03, 0x00, 0x45, 0x45, 0x1f, 0x00, 0x1f, 0x1f, 0x00, 0x00, 0x32},
>   };
>   
> -const char *IDE_DMA_CMD_lookup[IDE_DMA__COUNT] = {
> +static const char *IDE_DMA_CMD_lookup[] = {
>       [IDE_DMA_READ] = "DMA READ",
>       [IDE_DMA_WRITE] = "DMA WRITE",
>       [IDE_DMA_TRIM] = "DMA TRIM",
> -    [IDE_DMA_ATAPI] = "DMA ATAPI"
> +    [IDE_DMA_ATAPI] = "DMA ATAPI",
>   };
>   
>   static const char *IDE_DMA_CMD_str(enum ide_dma_cmd enval)
>   {
> -    if ((unsigned)enval < IDE_DMA__COUNT) {
> -        return IDE_DMA_CMD_lookup[enval];
> +    if (!IDE_DMA_CMD_lookup[enval]) {
> +        return "DMA UNKNOWN CMD";
>       }
> -    return "DMA UNKNOWN CMD";
> +    return IDE_DMA_CMD_lookup[enval];
>   }
>   
>   static void ide_dummy_transfer_stop(IDEState *s);
> diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
> index fc0aa81a88..e864fe8caf 100644
> --- a/include/hw/ide/internal.h
> +++ b/include/hw/ide/internal.h
> @@ -352,11 +352,8 @@ enum ide_dma_cmd {
>       IDE_DMA_WRITE,
>       IDE_DMA_TRIM,
>       IDE_DMA_ATAPI,
> -    IDE_DMA__COUNT
>   };
>   
> -extern const char *IDE_DMA_CMD_lookup[IDE_DMA__COUNT];
> -
>   #define ide_cmd_is_read(s) \
>           ((s)->dma_cmd == IDE_DMA_READ)
>