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

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

On Friday 07 August 2009 17:46:00 Juan Quintela wrote:
> Bique Alexandre <alexandre.bique@citrix.com> wrote:
> > 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.
>
> Being coherent with everyhing else (new fields use C99 initilizers).
> Consistence is important IMHO.

C89 features are also part of C99. I agree that C99 initializers is a great 
feature, but I really feel that it's not the right place to use it :)

> >> +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." ?
>
> The imposible has happened!!! We received a reply with size %d.  Please
> inform <foo@bar.com>.
>
> Not sure about the mail/web adderss, etc.

Hey fun. For sure I'll add this :)

> >> +        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.
>
> Then, were is the reference table pointer? :)

It comes from the "Mt. Fuji Commands for Multimedia Devices SFF8090i v4" page 
343.

> One comment indicating it would be nice.
>
> /* The order of this case is the same that table number foo at page X */
>
> Otherwise, it looks very random.

Done.

> >> +        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.
>
> You are welcome.

Thanks. I send the new patches with git format and git send-mail this time 
:-).

-- 
Alexandre Bique