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

[Christoph Hellwig <hch@lst.de>]

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.

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.