Re: [Qemu-devel] [PATCH 4/4] ATAPI pass through v3

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

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