Patchwork [4/4] ATAPI pass through v3

login
register
mail settings
Submitter Christoph Hellwig
Date Aug. 6, 2009, 4:23 p.m.
Message ID <20090806162356.GD22841@lst.de>
Download mbox | patch
Permalink /patch/30864/
State Superseded
Headers show

Comments

Christoph Hellwig - Aug. 6, 2009, 4:23 p.m.
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:
Bique Alexandre - Aug. 6, 2009, 5:49 p.m.
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!

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.

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.

+#define MSF_TO_FRAMES(M, S, F) (((M) * CD_SECS + (S)) * CD_FRAMES + (F))

Would be much nicer as a small (inline) function.

+/* 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?

@@ -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?

@@ -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.

@@ -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.