Re: [Qemu-devel] [PATCH v2 3/5] ide/atapi: Use table instead of switch for commands

[Kevin Wolf <kwolf@redhat.com>]

Am 20.04.2011 20:13, schrieb Blue Swirl:
> On Wed, Apr 20, 2011 at 2:30 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  hw/ide/atapi.c |  115 +++++++++++++++++++++++--------------------------------
>>  1 files changed, 48 insertions(+), 67 deletions(-)
>>
>> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
>> index d161bf7..d0bf7fd 100644
>> --- a/hw/ide/atapi.c
>> +++ b/hw/ide/atapi.c
>> @@ -533,10 +533,11 @@ static unsigned int event_status_media(IDEState *s,
>>     return 8; /* We wrote to 4 extra bytes from the header */
>>  }
>>
>> -static void handle_get_event_status_notification(IDEState *s,
>> -                                                 uint8_t *buf,
>> -                                                 const uint8_t *packet)
>> +static void cmd_get_event_status_notification(IDEState *s,
>> +                                              uint8_t *buf)
>>  {
>> +    const uint8_t *packet = buf;
>> +
>>     struct {
>>         uint8_t opcode;
>>         uint8_t polled;        /* lsb bit is polled; others are reserved */
>> @@ -1064,6 +1065,38 @@ static void cmd_set_speed(IDEState *s, uint8_t* buf)
>>     ide_atapi_cmd_ok(s);
>>  }
>>
>> +enum {
>> +    /*
>> +     * Only commands flagged as ALLOW_UA are allowed to run under a
>> +     * unit attention condition. (See MMC-5, section 4.1.6.1)
>> +     */
>> +    ALLOW_UA = 0x01,
>> +};
>> +
>> +struct {
>> +    void (*handler)(IDEState *s, uint8_t *buf);
>> +    int flags;
>> +} atapi_cmd_table[0x100] = {
>> +    [ 0x00 ] = { cmd_test_unit_ready,               0 },
> 
> How about using symbols here, like
>     [ GPCMD_TEST_UNIT_READY ] = { cmd_test_unit_ready, 0 },
> ?

I avoided symbols intentionally. In fact, I'm even considering to remove
the symbols from the header file because most of them are unused now
(and it should be easy enough to get rid of the rest of them so that the
only occurrence of the literal opcode is in the table).

The point here is that this table contains all the information about a
command in one place. If you used symbols, you would have the command
name twice, and the opcode would be missing. So not using symbolic
constants but rather literal opcodes improves readability in this case.

> The table can probably be static const.

Right, will fix.

Kevin