From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MLhn6-0006iv-2N for qemu-devel@nongnu.org; Tue, 30 Jun 2009 14:10:48 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MLhn1-0006iO-Dm for qemu-devel@nongnu.org; Tue, 30 Jun 2009 14:10:47 -0400 Received: from [199.232.76.173] (port=42979 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MLhn1-0006iL-AG for qemu-devel@nongnu.org; Tue, 30 Jun 2009 14:10:43 -0400 Received: from smtp.ctxuk.citrix.com ([62.200.22.115]:12519 helo=SMTP.EU.CITRIX.COM) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MLhn0-0002Yi-ST for qemu-devel@nongnu.org; Tue, 30 Jun 2009 14:10:43 -0400 From: Bique Alexandre Subject: Re: [Qemu-devel] [PATCH] ATAPI pass through Date: Tue, 30 Jun 2009 19:10:00 +0100 References: <200906301634.53197.alexandre.bique@citrix.com> <4A4A3CB7.8090309@redhat.com> In-Reply-To: <4A4A3CB7.8090309@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-ID: <200906301910.01073.alexandre.bique@citrix.com> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: "qemu-devel@nongnu.org" On Tuesday 30 June 2009 17:26:31 Kevin Wolf wrote: > 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. Ok, I will. > > 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? I also use poll (qemu_set_fd_handler). bsg works like that: - write your command => I am not writing a sector, but just a command which is <512B. So I won't use bdrv_write. I may use bdrv_pwrite, but why should I seek before writing ? And where ? - poll for command completion. => I didn't find any polling function from block.h. I can't use bdrv_aio_read because I am not reading a sector. - read the result => The same as for writing. Maybe block is not the right semantic for the bsg device and I should open it manually instead of using QEMU's block driver? > > - 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? It was a macro for historical reason, but it's a static function now. > I haven't really looked at the functionality. There are most probably > people around who can do a better review of it. > > Kevin Thanks Kevin. -- Alexandre Bique