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

[Kevin Wolf <kwolf@redhat.com>]

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