CCing Laszlo Ersek literally just for laughs, as he is the most
entertaining language lawyer I know of.
Laszlo, please feel free to ignore this if you don't care :P
On 08/29/2017 11:14 PM, Philippe Mathieu-Daudé wrote:
> Hi John,
>
> On 08/29/2017 05:49 PM, John Snow wrote:
>> Signed-off-by: John Snow <js...@redhat.com>
>> ---
>> hw/ide/atapi.c | 5 +----
>> hw/ide/core.c | 17 ++++++++++-------
>> hw/ide/trace-events | 3 +++
>> include/hw/ide/internal.h | 6 ++++--
>> 4 files changed, 18 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
>> index 37fa699..b8fc51e 100644
>> --- a/hw/ide/atapi.c
>> +++ b/hw/ide/atapi.c
>> @@ -416,10 +416,7 @@ static void ide_atapi_cmd_read_dma_cb(void
>> *opaque, int ret)
>> s->io_buffer_size = n * 2048;
>> data_offset = 0;
>> }
>> -#ifdef DEBUG_AIO
>> - printf("aio_read_cd: lba=%u n=%d\n", s->lba, n);
>> -#endif
>> -
>> + trace_ide_atapi_cmd_read_dma_cb_aio(s, s->lba, n);
>> s->bus->dma->iov.iov_base = (void *)(s->io_buffer + data_offset);
>> s->bus->dma->iov.iov_len = n * ATAPI_SECTOR_SIZE;
>> qemu_iovec_init_external(&s->bus->dma->qiov, &s->bus->dma->iov, 1);
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 82a19b1..a1c90e9 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -58,6 +58,13 @@ 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] = {
>> + [IDE_DMA_READ] = "DMA READ",
>> + [IDE_DMA_WRITE] = "DMA WRITE",
>> + [IDE_DMA_TRIM] = "DMA TRIM",
>> + [IDE_DMA_ATAPI] = "DMA ATAPI"
>> +};
>> +
>> static void ide_dummy_transfer_stop(IDEState *s);
>> static void padstr(char *str, const char *src, int len)
>> @@ -860,10 +867,8 @@ static void ide_dma_cb(void *opaque, int ret)
>> goto eot;
>> }
>> -#ifdef DEBUG_AIO
>> - printf("ide_dma_cb: sector_num=%" PRId64 " n=%d, cmd_cmd=%d\n",
>> - sector_num, n, s->dma_cmd);
>> -#endif
>> + trace_ide_dma_cb(s, sector_num, n,
>> + IDE_DMA_CMD_lookup[s->dma_cmd]);
>> if ((s->dma_cmd == IDE_DMA_READ || s->dma_cmd ==
>> IDE_DMA_WRITE) &&
>> !ide_sect_range_ok(s, sector_num, n)) {
>> @@ -2391,9 +2396,7 @@ void ide_bus_reset(IDEBus *bus)
>> /* pending async DMA */
>> if (bus->dma->aiocb) {
>> -#ifdef DEBUG_AIO
>> - printf("aio_cancel\n");
>> -#endif
>> + trace_ide_bus_reset_aio();
>> blk_aio_cancel(bus->dma->aiocb);
>> bus->dma->aiocb = NULL;
>> }
>> diff --git a/hw/ide/trace-events b/hw/ide/trace-events
>> index 8c79a6c..cc8949c 100644
>> --- a/hw/ide/trace-events
>> +++ b/hw/ide/trace-events
>> @@ -18,6 +18,8 @@ ide_cancel_dma_sync_remaining(void) "draining all
>> remaining requests"
>> ide_sector_read(int64_t sector_num, int nsectors) "sector=%"PRId64"
>> nsectors=%d"
>> ide_sector_write(int64_t sector_num, int nsectors) "sector=%"PRId64"
>> nsectors=%d"
>> ide_reset(void *s) "IDEstate %p"
>> +ide_bus_reset_aio(void) "aio_cancel"
>> +ide_dma_cb(void *s, int64_t sector_num, int n, const char *dma)
>> "IDEState %p; sector_num=%"PRId64" n=%d cmd=%s"
>> # BMDMA HBAs:
>> @@ -51,5 +53,6 @@ ide_atapi_cmd_reply_end_new(void *s, int status)
>> "IDEState: %p; new transfer sta
>> ide_atapi_cmd_check_status(void *s) "IDEState: %p"
>> ide_atapi_cmd_read(void *s, const char *method, int lba, int
>> nb_sectors) "IDEState: %p; read %s: LBA=%d nb_sectors=%d"
>> ide_atapi_cmd(void *s, uint8_t cmd) "IDEState: %p; cmd: 0x%02x"
>> +ide_atapi_cmd_read_dma_cb_aio(void *s, int lba, int n) "IDEState: %p;
>> aio read: lba=%d n=%d"
>> # Warning: Verbose
>> ide_atapi_cmd_packet(void *s, uint16_t limit, const char *packet)
>> "IDEState: %p; limit=0x%x packet: %s"
>> diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
>> index 74efe8a..db9fde0 100644
>> --- a/include/hw/ide/internal.h
>> +++ b/include/hw/ide/internal.h
>> @@ -14,7 +14,6 @@
>> #include "block/scsi.h"
>> /* debug IDE devices */
>> -//#define DEBUG_AIO
>> #define USE_DMA_CDROM
>> typedef struct IDEBus IDEBus;
>> @@ -333,12 +332,15 @@ struct unreported_events {
>> };
>> enum ide_dma_cmd {
>> - IDE_DMA_READ,
>> + IDE_DMA_READ = 0,
>> IDE_DMA_WRITE,
>> IDE_DMA_TRIM,
>> IDE_DMA_ATAPI,
>> + IDE_DMA__COUNT
>> };
>> +extern const char *IDE_DMA_CMD_lookup[IDE_DMA__COUNT];
>
> I recommend you to avoid this declaring extern const array with size, I
> remember some compilers (old GCC?) ignoring array size in extern. Eric
> will correct me!
>
> It is much safer to use a getter:
>
Well, whether or not the compiler ignores it, you're right that it's
safer to use a getter. I don't think the width being declared HURTS any
compiler though, does it?
> const char *IDE_DMA_CMD_lookup(enum ide_dma_cmd cmd)
> {
> static const char *IDE_DMA_CMD_name[IDE_DMA__COUNT] = {
> [IDE_DMA_READ] = "DMA READ",
> [IDE_DMA_WRITE] = "DMA WRITE",
> [IDE_DMA_TRIM] = "DMA TRIM",
> [IDE_DMA_ATAPI] = "DMA ATAPI"
> };
>
> return IDE_DMA_CMD_name[cmd];
> };
>
Why is this safer...?
Here's my opinion of enums:
jhuston@probe ~/s/scratch> cat enum.c
enum foo {
FOO_A = 0,
FOO_B
};
int fn(enum foo in)
{
switch (in) {
case FOO_A:
case FOO_B:
return 0;
default:
return 1;
}
}
int main(int argc, char *argv[])
{
return fn(2);
}
jhuston@probe ~/s/scratch> gcc -ansi -Wall -pedantic -o enum enum.c
jhuston@probe ~/s/scratch> ./enum
jhuston@probe ~/s/scratch> echo $status
1
No warnings, no messages, it just happily takes a value outside the
domain and in this case it doesn't explode because I caught it, but,
take a look:
jhuston@probe ~/s/scratch> cat enum.c
#include <stdio.h>
enum foo {
FOO_A = 0,
FOO_B,
FOO__COUNT
};
const char *table[FOO__COUNT] = {"FOO_A", "FOO_B"};
int fn(enum foo in)
{
fprintf(stderr, "%s\n", table[in]);
switch (in) {
case FOO_A:
case FOO_B:
return 0;
default:
return 1;
}
}
int main(int argc, char *argv[])
{
return fn(2);
}
Similar program, let's compile it:
jhuston@probe ~/s/scratch> gcc -ansi -Wall -pedantic -o enum enum.c
jhuston@probe ~/s/scratch> ./enum
� �
Ah, dang.
This is why I prefer to have my assertions and don't trust enums to be
checked rigorously.
For anyone playing along at home, yes, if you specify at least -O1 here
it will catch it:
jhuston@probe ~/s/scratch> gcc -ansi -Wall -pedantic -O3 -o enum enum.c
enum.c: In function ‘main’:
enum.c:13:5: warning: array subscript is above array bounds [-Warray-bounds]
fprintf(stderr, "%s\n", table[in]);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Oh no, what the hell is clang up to, though?
jhuston@probe ~/s/scratch> clang -ansi -Wall -pedantic -O3 -o enum enum.c
jhuston@probe ~/s/scratch>
Oh no...
http://i.imgur.com/uRvorq3.jpg
> If you agree:
> Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
>
Somewhat. I will write a getter that does bounds checking. It _is_ safer
that way, though I don't think we'll ever run into a situation where it
would come up.
>> +
>> #define ide_cmd_is_read(s) \
>> ((s)->dma_cmd == IDE_DMA_READ)
>>
>
