From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MZSzE-0002E6-VK for qemu-devel@nongnu.org; Fri, 07 Aug 2009 13:12:12 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MZSz9-0002D7-FY for qemu-devel@nongnu.org; Fri, 07 Aug 2009 13:12:11 -0400 Received: from [199.232.76.173] (port=44099 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MZSz9-0002D4-95 for qemu-devel@nongnu.org; Fri, 07 Aug 2009 13:12:07 -0400 Received: from smtp.ctxuk.citrix.com ([62.200.22.115]:11326 helo=SMTP.EU.CITRIX.COM) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MZSz8-0002vw-Kb for qemu-devel@nongnu.org; Fri, 07 Aug 2009 13:12:07 -0400 From: Bique Alexandre Date: Fri, 7 Aug 2009 18:10:08 +0100 References: <200908061640.39895.alexandre.bique@citrix.com> <200908071734.19389.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: <200908071810.08323.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:46:00 Juan Quintela wrote: > Bique Alexandre wrote: > > 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. > > 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 . > > 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