From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MZ77i-0004VE-Vo for qemu-devel@nongnu.org; Thu, 06 Aug 2009 13:51:31 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MZ77e-0004SJ-F1 for qemu-devel@nongnu.org; Thu, 06 Aug 2009 13:51:30 -0400 Received: from [199.232.76.173] (port=43511 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MZ77e-0004S8-7k for qemu-devel@nongnu.org; Thu, 06 Aug 2009 13:51:26 -0400 Received: from smtp.eu.citrix.com ([62.200.22.115]:57734) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MZ77d-0000jz-Pm for qemu-devel@nongnu.org; Thu, 06 Aug 2009 13:51:26 -0400 From: Bique Alexandre Subject: Re: [Qemu-devel] [PATCH 4/4] ATAPI pass through v3 Date: Thu, 6 Aug 2009 18:49:48 +0100 References: <200908061640.39895.alexandre.bique@citrix.com> <200908061650.05474.alexandre.bique@citrix.com> <20090806162356.GD22841@lst.de> In-Reply-To: <20090806162356.GD22841@lst.de> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-ID: <200908061849.48265.alexandre.bique@citrix.com> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Christoph Hellwig Cc: "qemu-devel@nongnu.org" 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