Re: [Qemu-block] [Qemu-devel] [PATCH v2 5/9] IDE: replace DEBUG_AIO with trace events

[John Snow]

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 <address@hidden>
>> ---
>>   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:

address@hidden ~/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);
}


address@hidden ~/s/scratch> gcc -ansi -Wall -pedantic -o enum enum.c
address@hidden ~/s/scratch> ./enum
address@hidden ~/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:

address@hidden ~/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:

address@hidden ~/s/scratch> gcc -ansi -Wall -pedantic -o enum enum.c
address@hidden ~/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:

address@hidden ~/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?

address@hidden ~/s/scratch> clang -ansi -Wall -pedantic -O3 -o enum enum.c
address@hidden ~/s/scratch>


Oh no...

http://i.imgur.com/uRvorq3.jpg

> If you agree:
> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
> 

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)
>>  
>