[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] ATAPI pass through
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH] ATAPI pass through |
Date: |
Tue, 30 Jun 2009 18:26:31 +0200 |
User-agent: |
Thunderbird 2.0.0.21 (X11/20090320) |
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