[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 4/4] ATAPI pass through v3
From: |
Bique Alexandre |
Subject: |
Re: [Qemu-devel] [PATCH 4/4] ATAPI pass through v3 |
Date: |
Thu, 6 Aug 2009 18:49:48 +0100 |
User-agent: |
KMail/1.12.0 (Linux/2.6.30-ARCH; KDE/4.3.0; x86_64; ; ) |
On Thursday 06 August 2009 17:23:56 Christoph Hellwig wrote:
> On Thu, Aug 06, 2009 at 04:50:05PM +0100, Bique Alexandre wrote:
> > The ATAPI pass through feature.
>
> Again, this should be the subject, and the body should have a short
> description on how it's done and why it's useful.
>
>
> Is the firmware upgrade command really that special that we need to
> filter it and not other harmfull commands? That really needs
> explanation in the patch description and/or a comment.
>
> Some comments on the patch:
>
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index bdee07f..0510379 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -154,6 +154,12 @@ static int raw_open_common(BlockDriverState *bs, const
> char *filename, else if (!(bdrv_flags & BDRV_O_CACHE_WB))
> s->open_flags |= O_DSYNC;
>
> + /* If we open a cdrom device, and there is no media inside, we
> + * have to add O_NONBLOCK to open else it will fail */
> + if (bdrv_get_type_hint(bs) == BDRV_TYPE_CDROM &&
> + bdrv_is_sg(bs))
> + s->open_flags |= O_NONBLOCK;
>
> this should be in cdrom_open, that way you won't need the
> bdrv_get_type_hint check either.
Thank you for this one :-)
> diff --git a/hw/atapi-pt.c b/hw/atapi-pt.c
> new file mode 100644
> index 0000000..85f6b6a
> --- /dev/null
> +++ b/hw/atapi-pt.c
> @@ -0,0 +1,1002 @@
> +int atapi_pt_allow_fw_upgrade = 0;
> +
>
> This seems to be missing any sort of author/copyright line.
Done.
> +#define MSF_TO_FRAMES(M, S, F) (((M) * CD_SECS + (S)) * CD_FRAMES + (F))
>
> Would be much nicer as a small (inline) function.
Done.
> +/* The generic packet command opcodes for CD/DVD Logical Units,
> + * From Table 57 of the SFF8090 Ver. 3 (Mt. Fuji) draft standard. */
> +static const struct {
> + unsigned short packet_command;
> + const char * const text;
> +} packet_command_texts[] = {
>
> Maybe move all these tables into a separate file?
Alright, moving this in hw/atapi-data.{h,c}
> @@ -1288,6 +1350,14 @@ static inline int ube16_to_cpu(const uint8_t *buf)
> return (buf[0] << 8) | buf[1];
> }
>
> +#if CONFIG_ATAPI_PT /* only atapi-pt uses it so let's avoid unused
> + * warning */
> +static inline int ube24_to_cpu(const uint8_t *buf)
> +{
> + return (buf[0] << 16) | (buf[1] << 8) | buf[2];
> +}
> +#endif /* CONFIG_ATAPI_PT */
>
> When did gcc start issuing unused warnings for inlines?
Because there is the static keyword.
> @@ -1675,6 +1745,10 @@ static int ide_dvd_read_structure(IDEState *s, int
> format, }
> }
>
> +#if CONFIG_ATAPI_PT
> +#include "atapi-pt.c"
> +#endif
>
> Including .c files is areally horrible style, just make the function you
> also need from atapi-pt.c non-static.
Alright. Doing.
> @@ -2872,6 +2946,16 @@ static void ide_init2(IDEState *ide_state,
>
> if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM) {
> s->is_cdrom = 1;
> + if (bdrv_is_sg(s->bs)) {
>
> This check looks reversed to me.
Oh my god, you're right. Fixed!
Thanks for reviewing, I'll send new patches as soon as possible!
--
Alexandre Bique
- [Qemu-devel] [PATCH 1/4] ATAPI pass through v3, (continued)