From patchwork Thu Aug 6 16:23:56 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christoph Hellwig X-Patchwork-Id: 30864 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [199.232.76.165]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by bilbo.ozlabs.org (Postfix) with ESMTPS id F0391B707B for ; Fri, 7 Aug 2009 02:24:36 +1000 (EST) Received: from localhost ([127.0.0.1]:40859 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MZ5lY-0007xf-DQ for incoming@patchwork.ozlabs.org; Thu, 06 Aug 2009 12:24:32 -0400 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MZ5l6-0007xX-Cj for qemu-devel@nongnu.org; Thu, 06 Aug 2009 12:24:04 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MZ5l2-0007xL-V2 for qemu-devel@nongnu.org; Thu, 06 Aug 2009 12:24:04 -0400 Received: from [199.232.76.173] (port=40717 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MZ5l2-0007xI-Pi for qemu-devel@nongnu.org; Thu, 06 Aug 2009 12:24:00 -0400 Received: from verein.lst.de ([213.95.11.210]:33602) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_3DES_EDE_CBC_SHA1:24) (Exim 4.60) (envelope-from ) id 1MZ5l2-0000bH-3G for qemu-devel@nongnu.org; Thu, 06 Aug 2009 12:24:00 -0400 Received: from verein.lst.de (localhost [127.0.0.1]) by verein.lst.de (8.12.3/8.12.3/Debian-7.1) with ESMTP id n76GNvVL024660 (version=TLSv1/SSLv3 cipher=EDH-RSA-DES-CBC3-SHA bits=168 verify=NO); Thu, 6 Aug 2009 18:23:57 +0200 Received: (from hch@localhost) by verein.lst.de (8.12.3/8.12.3/Debian-7.2) id n76GNuoH024659; Thu, 6 Aug 2009 18:23:56 +0200 Date: Thu, 6 Aug 2009 18:23:56 +0200 From: Christoph Hellwig To: Bique Alexandre Subject: Re: [Qemu-devel] [PATCH 4/4] ATAPI pass through v3 Message-ID: <20090806162356.GD22841@lst.de> References: <200908061640.39895.alexandre.bique@citrix.com> <200908061650.05474.alexandre.bique@citrix.com> Mime-Version: 1.0 Content-Disposition: inline In-Reply-To: <200908061650.05474.alexandre.bique@citrix.com> User-Agent: Mutt/1.3.28i X-Spam-Score: 0 () X-Scanned-By: MIMEDefang 2.39 X-detected-operating-system: by monty-python.gnu.org: GNU/Linux 2.6 (newer, 2) Cc: "qemu-devel@nongnu.org" X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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.