From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MLgBO-0006EU-3V for qemu-devel@nongnu.org; Tue, 30 Jun 2009 12:27:46 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MLgBI-0006EA-Sp for qemu-devel@nongnu.org; Tue, 30 Jun 2009 12:27:45 -0400 Received: from [199.232.76.173] (port=51365 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MLgBI-0006E7-N4 for qemu-devel@nongnu.org; Tue, 30 Jun 2009 12:27:40 -0400 Received: from mx2.redhat.com ([66.187.237.31]:43377) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MLgBI-0003BC-9C for qemu-devel@nongnu.org; Tue, 30 Jun 2009 12:27:40 -0400 Message-ID: <4A4A3CB7.8090309@redhat.com> Date: Tue, 30 Jun 2009 18:26:31 +0200 From: Kevin Wolf MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] ATAPI pass through References: <200906301634.53197.alexandre.bique@citrix.com> In-Reply-To: <200906301634.53197.alexandre.bique@citrix.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Bique Alexandre Cc: qemu-devel@nongnu.org Bique Alexandre schrieb: > Hi, > > This patch implements ATAPI pass through on Linux by using bsg device. > > What works: > - read CD > - burn CD with TAO > - blank CD > - play audio CD > - play DVD movie > > What doesn't work: > - burn CD with SAO > > DVD blanking and burning needs some testing. First of all, I would suggest splitting this patch up. You seem to have included some pieces of unrelated changes like adding comments, changing constants (Are these fixes? Definitely need a good patch description!), moving code (block-raw-posix.h) and probably some more. > Things which can be done better: > - I exported the data structure BDRVRawState from raw-posix.c in raw-posix.h, > because I use the file descriptor of the bsg device to poll, write and read. > Maybe there is a better solution, but I didn't find what I was looking for in > block.h. Don't do that. If there is something missing in the block interface, we need to add it there and properly wire things up. Just poking in block driver internals is clearly the wrong approach. >>From what I see the only thing you are doing with it is read/write. Why can't you use the normal block layer functions for it? > - file splitting: everything is in ide.c, even my atapi pass through code in > the beginning. I didn't want to put everything in this file so I moved my code > in atapi-pt.c but I did #include "atapi-pt.c" in ide.c so it's still in the > spirit of what has been done before: all in ide.c. I'd like to split correctly > this code but I will have to make some static functions public, and if I move > atapi-pt out from ide.c why not atapi out from ide.c and maybe some other > stuff ? That's why I did nothing yet. What would you prefer ? I guess proper splitting wouldn't hurt anyway. ide.c looks way too large. > > Please, can you comment on this patch ? Thank you very much. Another detail I saw is ATAPI_PT_SEND_PACKET. I mean, isn't this a bit too big for a macro? No chance to make this nicer? I haven't really looked at the functionality. There are most probably people around who can do a better review of it. Kevin