From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MZSQM-0001CI-AZ for qemu-devel@nongnu.org; Fri, 07 Aug 2009 12:36:10 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MZSQH-0001AX-Gv for qemu-devel@nongnu.org; Fri, 07 Aug 2009 12:36:09 -0400 Received: from [199.232.76.173] (port=50454 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MZSQH-0001AU-BQ for qemu-devel@nongnu.org; Fri, 07 Aug 2009 12:36:05 -0400 Received: from smtp.eu.citrix.com ([62.200.22.115]:27944) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MZSQG-0003oN-TP for qemu-devel@nongnu.org; Fri, 07 Aug 2009 12:36:05 -0400 From: Bique Alexandre Date: Fri, 7 Aug 2009 17:34:18 +0100 References: <200908061640.39895.alexandre.bique@citrix.com> <200908061650.05474.alexandre.bique@citrix.com> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-ID: <200908071734.19389.alexandre.bique@citrix.com> Subject: [Qemu-devel] Re: [PATCH 4/4] ATAPI pass through v3 List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Juan Quintela Cc: "qemu-devel@nongnu.org" On Friday 07 August 2009 17:06:36 Juan Quintela wrote: > Bique Alexandre 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