[Qemu-devel] Re: [PATCH 4/4] ATAPI pass through v3

[Bique Alexandre <alexandre.bique@citrix.com>]

On Friday 07 August 2009 17:06:36 Juan Quintela wrote:
> Bique Alexandre <alexandre.bique@citrix.com> wrote:
> > The ATAPI pass through feature.
>
> +# define CHECK_SAME_VALUE(Val1, Val2)
>
> Can we call this something like: assert_equal() ? or anything else more
> descriptive?

It's not an assertion because Val1 and Val2 are allowed to be different and it 
doesn't call abort. It just displays some debut information. Would CHECK_EQUAL 
be alright for you ?

>
> +/* The generic packet command opcodes for CD/DVD Logical Units,
> + * From Table 57 of the SFF8090 Ver. 3 (Mt. Fuji) draft standard. */
> +static const struct {
> +    unsigned short packet_command;
> +    const char * const text;
> +} packet_command_texts[] = {
>
> Please, use C99 intializers:
>
> +    { GPCMD_TEST_UNIT_READY, "Test Unit Ready" },
>
> use this format, same for rest of structs
>
>      {
>        .packet_command = GPCMD_TEST_UNIT_READY,
>        .text = "Test Unit Ready"
>      },

I see no reason to do that. It takes more place. We are not going to add a lot 
of additional fields. And even if we do, this structure should be used only 
time at only one place and if we decide to add a new field in the beginning or 
the middle of this structure, it should be worth to do some regexp to fix the 
declaration.

> +static void ide_atapi_pt_standard_reply(IDEState *s)
> +{
> +    uint32_t size = s->atapi_pt.reply_size_init;
> +
> +    switch (s->atapi_pt.reply_size_len)
> +    {
> +    case 0:
> +        break;
> +    case 1:
> +        size += s->io_buffer[s->atapi_pt.reply_size_offset];
> +        break;
> +    case 2:
> +        size += ube16_to_cpu(s->io_buffer +
> s->atapi_pt.reply_size_offset); +        break;
> +    case 3:
> +        size += ube24_to_cpu(s->io_buffer +
> s->atapi_pt.reply_size_offset); +        break;
> +    case 4:
> +        size += ube32_to_cpu(s->io_buffer +
> s->atapi_pt.reply_size_offset); +        break;
> +    default:
> +        assert(0);
>          ^^^^^^^
> print a nice error message?
> die?
> something?

I Will do it, but what do you want me to say ?
"We reached a part of the code we should never reach. Please send a bug report 
to Qemu developers. Thanks." ?

> +        break;
> +    }
>
> +static int ide_atapi_pt_read_cd_block_size(const uint8_t *io_buffer)
> +{
> +    int sector_type = (io_buffer[2] >> 2) & 7;
> +    int error_flags = (io_buffer[9] >> 1) & 3;
> +    int flags_bits = io_buffer[9] & ~7;
> +    int block_size = 0;
> +
> +    // expected sector type
> +    switch (sector_type)
> +    {
> +    case 0: // Any type
> +    case 1: // CD-DA
> +        block_size = (flags_bits) ? 2352 : 0;
> +        break;
> +
> +    case 2: // Mode 1
> +        switch (flags_bits)
> +        {
> +        case 0x0: block_size = 0; break;
> case 0x40: block_size = 0; break;
>
> move this one here, same fro the same two with 16 value?
> group all of them by block_size?  Same for the rest of the cases.

I can but I don't want to. Because if you want to double check this, you would 
prefer to see this sorted so you can check the reference table at the same 
time you check your switch case.

> +        case 0x10:
> +        case 0x50: block_size = 2048; break;
> +        case 0x18:
> +        case 0x58: block_size = 2336; break;
> +        case 0x20:
> +        case 0x60: block_size = 4; break;
> +        case 0x30:
> +        case 0x70:
> +        case 0x78: block_size = 2052; break;
> +        case 0x38: block_size = 2340; break;
> +        case 0xa0: block_size = 16; break;
> +        case 0xb0: block_size = 2064; break;
> +        case 0xb8: block_size = 2352; break;
> +        case 0xe0: block_size = 16; break;
> +        case 0xf0: block_size = 2064; break;
> +        case 0xf8: block_size = 2352; break;
> +
> +        default: return 0; // illegal
> +        }
>
>
> +#if CONFIG_ATAPI_PT
> +#include "atapi-pt.c"
> +#endif
>
> Why do we include it here?  coulden't yust compile it as a new file?
I did. I am going to send you my new patches where I made some part of ide.c 
public and introduced 3 new headers.

Thanks for review.

-- 
Alexandre Bique