* [PATCH] hw/ide: Remove unuseful IDE_DMA__COUNT definition
@ 2023-02-24 15:34 Philippe Mathieu-Daudé
2023-03-14 17:04 ` Philippe Mathieu-Daudé
2023-05-17 21:39 ` John Snow
0 siblings, 2 replies; 3+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-02-24 15:34 UTC (permalink / raw)
To: qemu-devel
Cc: John Snow, qemu-trivial, qemu-block, Philippe Mathieu-Daudé
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
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] hw/ide: Remove unuseful IDE_DMA__COUNT definition
2023-02-24 15:34 [PATCH] hw/ide: Remove unuseful IDE_DMA__COUNT definition Philippe Mathieu-Daudé
@ 2023-03-14 17:04 ` Philippe Mathieu-Daudé
2023-05-17 21:39 ` John Snow
1 sibling, 0 replies; 3+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-03-14 17:04 UTC (permalink / raw)
To: qemu-devel, John Snow; +Cc: qemu-trivial, qemu-block
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)
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] hw/ide: Remove unuseful IDE_DMA__COUNT definition
2023-02-24 15:34 [PATCH] hw/ide: Remove unuseful IDE_DMA__COUNT definition Philippe Mathieu-Daudé
2023-03-14 17:04 ` Philippe Mathieu-Daudé
@ 2023-05-17 21:39 ` John Snow
1 sibling, 0 replies; 3+ messages in thread
From: John Snow @ 2023-05-17 21:39 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: qemu-devel, qemu-trivial, qemu-block
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-05-17 21:40 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-24 15:34 [PATCH] hw/ide: Remove unuseful IDE_DMA__COUNT definition Philippe Mathieu-Daudé
2023-03-14 17:04 ` Philippe Mathieu-Daudé
2023-05-17 21:39 ` John Snow
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).