Re: [Qemu-devel] [PATCH v4 5/5] atapi: GESN: implement 'media' subcommand

[Kevin Wolf <kwolf@redhat.com>]

Am 12.04.2011 17:03, schrieb Jes Sorensen:
> 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?

Not necessary (the buffer is always 2048 bytes), but it looks like the
max_len parameter is unused now, so it could be removed.

Kevin