| Message ID | 20230224153410.19727-1-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/ide: Remove unuseful IDE_DMA__COUNT definition | expand |
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) >
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 >
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)
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(-)