From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:45739) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q9f8b-0008Rg-S5 for qemu-devel@nongnu.org; Tue, 12 Apr 2011 11:04:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q9f8a-0004k0-Ut for qemu-devel@nongnu.org; Tue, 12 Apr 2011 11:04:17 -0400 Received: from mx1.redhat.com ([209.132.183.28]:24950) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q9f8a-0004jw-MO for qemu-devel@nongnu.org; Tue, 12 Apr 2011 11:04:16 -0400 Message-ID: <4DA469D1.1010300@redhat.com> Date: Tue, 12 Apr 2011 17:03:45 +0200 From: Jes Sorensen MIME-Version: 1.0 References: <1632463ee09a9df5690475f0b507757c0876fce9.1302617257.git.amit.shah@redhat.com> In-Reply-To: <1632463ee09a9df5690475f0b507757c0876fce9.1302617257.git.amit.shah@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 5/5] atapi: GESN: implement 'media' subcommand List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Amit Shah Cc: Kevin Wolf , Juan Quintela , Stefan Hajnoczi , qemu list , Markus Armbruster , Paolo Bonzini On 04/12/11 16:09, Amit Shah wrote: > diff --git a/hw/ide/core.c b/hw/ide/core.c > index dafc049..209d8e6 100644 > --- a/hw/ide/core.c > +++ b/hw/ide/core.c > @@ -1084,6 +1084,49 @@ static int ide_dvd_read_structure(IDEState *s, int format, > } > } > > +static unsigned int event_status_media(IDEState *s, > + uint8_t *buf, > + unsigned int max_len) > +{ > + enum media_event_code { > + no_change = 0, /* Status unchanged */ > + eject_requested, /* received a request from user to eject */ > + new_media, /* new media inserted and ready for access */ > + media_removal, /* only for media changers */ > + media_changed, /* only for media changers */ > + bg_format_completed, /* MRW or DVD+RW b/g format completed */ > + bg_format_restarted, /* MRW or DVD+RW b/g format restarted */ > + }; > + enum media_status { > + tray_open = 1, > + media_present = 2, > + }; Would be nice if you used upper-case with the enums since they are effectively just defines, to avoid confusing them with variables. > + uint8_t event_code, media_status; > + > + media_status = 0; > + if (s->bs->tray_open) { > + media_status = tray_open; > + } else if (bdrv_is_inserted(s->bs)) { > + media_status = media_present; > + } > + > + /* Event notification descriptor */ > + event_code = no_change; > + if (media_status != tray_open && s->events.new_media) { > + event_code = new_media; > + s->events.new_media = false; > + } > + > + buf[4] = event_code; > + buf[5] = media_status; > + > + /* These fields are reserved, just clear them. */ > + buf[6] = 0; > + buf[7] = 0; > + > + return 8; /* We wrote to 4 extra bytes from the header */ Shouldn't you verify that you don't exceed max_len in this? > @@ -1104,6 +1147,26 @@ static void handle_get_event_status_notification(IDEState *s, > uint8_t supported_events; > } __attribute((packed)) *gesn_event_header; > > + enum notification_class_request_type { > + reserved1 = 1 << 0, > + operational_change = 1 << 1, > + power_management = 1 << 2, > + external_request = 1 << 3, > + media = 1 << 4, > + multi_host = 1 << 5, > + device_busy = 1 << 6, > + reserved2 = 1 << 7, > + }; > + enum event_notification_class_field { > + enc_no_events = 0, > + enc_operational_change, > + enc_power_management, > + enc_external_request, > + enc_media, > + enc_multiple_hosts, > + enc_device_busy, > + enc_reserved, > + }; More lower-case enums :( Otherwise the series looks good. Jes