Re: [Qemu-devel] [PATCH] ATAPI pass through

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

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