| Message ID | 1312376904-16115-17-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/03/2011 03:07 PM, Markus Armbruster wrote: > We already track it in BlockDriverState. Just like tray open/close > state, we should track it in the device models instead, because it's > device state. > > Signed-off-by: Markus Armbruster<armbru@redhat.com> > --- > hw/scsi-disk.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c > index db72b86..8ca69f2 100644 > --- a/hw/scsi-disk.c > +++ b/hw/scsi-disk.c > @@ -73,6 +73,7 @@ struct SCSIDiskState > char *serial; > SCSISense sense; > bool tray_open; > + bool tray_locked; > }; > Hmm. Shouldn't we use a more generic 'flags' here and have bits for the individual tray states? Feels like a waste to have individual values here. > static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type); > @@ -678,7 +679,7 @@ static int mode_sense_page(SCSIRequest *req, int page, uint8_t *p, > p[5] = 0xff; /* CD DA, DA accurate, RW supported, > RW corrected, C2 errors, ISRC, > UPC, Bar code */ > - p[6] = 0x2d | (bdrv_is_locked(s->bs)? 2 : 0); > + p[6] = 0x2d | (s->tray_locked ? 2 : 0); > /* Locking supported, jumper present, eject, tray */ > p[7] = 0; /* no volume& mute control, no > changer */ > @@ -896,6 +897,7 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf) > scsi_disk_emulate_start_stop(r); > break; > case ALLOW_MEDIUM_REMOVAL: > + s->tray_locked = req->cmd.buf[4]& 1; > bdrv_set_locked(s->bs, req->cmd.buf[4]& 1); > break; > case READ_CAPACITY_10: Cheers, Hannes
Hannes Reinecke <hare@suse.de> writes: > On 08/03/2011 03:07 PM, Markus Armbruster wrote: >> We already track it in BlockDriverState. Just like tray open/close >> state, we should track it in the device models instead, because it's >> device state. >> >> Signed-off-by: Markus Armbruster<armbru@redhat.com> >> --- >> hw/scsi-disk.c | 4 +++- >> 1 files changed, 3 insertions(+), 1 deletions(-) >> >> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c >> index db72b86..8ca69f2 100644 >> --- a/hw/scsi-disk.c >> +++ b/hw/scsi-disk.c >> @@ -73,6 +73,7 @@ struct SCSIDiskState >> char *serial; >> SCSISense sense; >> bool tray_open; >> + bool tray_locked; >> }; >> > Hmm. Shouldn't we use a more generic 'flags' here and have bits for > the individual tray states? > Feels like a waste to have individual values here. On my system, struct SCSIDiskState is 248 bytes before my series (5 bytes of padding at the end), and 248 bytes after (3 bytes padding). We use one per disk. I dare say switching to flags won't make a noticable difference in memory use ;) The bool members are easy to read and hard to screw up. Flags can be nicer when you manipulate several of them together.
On 08/04/2011 08:39 AM, Markus Armbruster wrote: > Hannes Reinecke<hare@suse.de> writes: > >> On 08/03/2011 03:07 PM, Markus Armbruster wrote: >>> We already track it in BlockDriverState. Just like tray open/close >>> state, we should track it in the device models instead, because it's >>> device state. >>> >>> Signed-off-by: Markus Armbruster<armbru@redhat.com> >>> --- >>> hw/scsi-disk.c | 4 +++- >>> 1 files changed, 3 insertions(+), 1 deletions(-) >>> >>> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c >>> index db72b86..8ca69f2 100644 >>> --- a/hw/scsi-disk.c >>> +++ b/hw/scsi-disk.c >>> @@ -73,6 +73,7 @@ struct SCSIDiskState >>> char *serial; >>> SCSISense sense; >>> bool tray_open; >>> + bool tray_locked; >>> }; >>> >> Hmm. Shouldn't we use a more generic 'flags' here and have bits for >> the individual tray states? >> Feels like a waste to have individual values here. > > On my system, struct SCSIDiskState is 248 bytes before my series (5 > bytes of padding at the end), and 248 bytes after (3 bytes padding). We > use one per disk. > > I dare say switching to flags won't make a noticable difference in > memory use ;) > > The bool members are easy to read and hard to screw up. > > Flags can be nicer when you manipulate several of them together. Well, but this just proves my point. Each 'bool' variable takes up one byte, of which only one bit is used. Which qualifies as a waste of space. But if that's okay with everyone and the general consensus on how to code flags in qemu, I'll shut up. Cheers, Hannes
Hannes Reinecke <hare@suse.de> writes: > On 08/04/2011 08:39 AM, Markus Armbruster wrote: >> Hannes Reinecke<hare@suse.de> writes: >> >>> On 08/03/2011 03:07 PM, Markus Armbruster wrote: >>>> We already track it in BlockDriverState. Just like tray open/close >>>> state, we should track it in the device models instead, because it's >>>> device state. >>>> >>>> Signed-off-by: Markus Armbruster<armbru@redhat.com> >>>> --- >>>> hw/scsi-disk.c | 4 +++- >>>> 1 files changed, 3 insertions(+), 1 deletions(-) >>>> >>>> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c >>>> index db72b86..8ca69f2 100644 >>>> --- a/hw/scsi-disk.c >>>> +++ b/hw/scsi-disk.c >>>> @@ -73,6 +73,7 @@ struct SCSIDiskState >>>> char *serial; >>>> SCSISense sense; >>>> bool tray_open; >>>> + bool tray_locked; >>>> }; >>>> >>> Hmm. Shouldn't we use a more generic 'flags' here and have bits for >>> the individual tray states? >>> Feels like a waste to have individual values here. >> >> On my system, struct SCSIDiskState is 248 bytes before my series (5 >> bytes of padding at the end), and 248 bytes after (3 bytes padding). We >> use one per disk. >> >> I dare say switching to flags won't make a noticable difference in >> memory use ;) >> >> The bool members are easy to read and hard to screw up. >> >> Flags can be nicer when you manipulate several of them together. > > Well, but this just proves my point. > Each 'bool' variable takes up one byte, of which only one bit is > used. Which qualifies as a waste of space. A few data bytes per SCSI disk are a drop in the ocean. Plus they're offset by the flags data word and the extra code bytes you need to extract the bits. > But if that's okay with everyone and the general consensus on how to > code flags in qemu, I'll shut up. It's a big series, and respinning it is work. Can we sort such things in followup patches?
On 08/04/2011 09:25 AM, Markus Armbruster wrote: > Hannes Reinecke<hare@suse.de> writes: > >> On 08/04/2011 08:39 AM, Markus Armbruster wrote: >>> Hannes Reinecke<hare@suse.de> writes: >>> >>>> On 08/03/2011 03:07 PM, Markus Armbruster wrote: >>>>> We already track it in BlockDriverState. Just like tray open/close >>>>> state, we should track it in the device models instead, because it's >>>>> device state. >>>>> >>>>> Signed-off-by: Markus Armbruster<armbru@redhat.com> >>>>> --- >>>>> hw/scsi-disk.c | 4 +++- >>>>> 1 files changed, 3 insertions(+), 1 deletions(-) >>>>> >>>>> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c >>>>> index db72b86..8ca69f2 100644 >>>>> --- a/hw/scsi-disk.c >>>>> +++ b/hw/scsi-disk.c >>>>> @@ -73,6 +73,7 @@ struct SCSIDiskState >>>>> char *serial; >>>>> SCSISense sense; >>>>> bool tray_open; >>>>> + bool tray_locked; >>>>> }; >>>>> >>>> Hmm. Shouldn't we use a more generic 'flags' here and have bits for >>>> the individual tray states? >>>> Feels like a waste to have individual values here. >>> >>> On my system, struct SCSIDiskState is 248 bytes before my series (5 >>> bytes of padding at the end), and 248 bytes after (3 bytes padding). We >>> use one per disk. >>> >>> I dare say switching to flags won't make a noticable difference in >>> memory use ;) >>> >>> The bool members are easy to read and hard to screw up. >>> >>> Flags can be nicer when you manipulate several of them together. >> >> Well, but this just proves my point. >> Each 'bool' variable takes up one byte, of which only one bit is >> used. Which qualifies as a waste of space. > > A few data bytes per SCSI disk are a drop in the ocean. > > Plus they're offset by the flags data word and the extra code bytes you > need to extract the bits. > >> But if that's okay with everyone and the general consensus on how to >> code flags in qemu, I'll shut up. > > It's a big series, and respinning it is work. Can we sort such things > in followup patches? As said, it's a minor point. And as it's only a byte you are correct about a drop in the ocean. And Paolo promised to keep it up in a further patch series. So I'll retract my objection. Cheers, Hannes
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c index db72b86..8ca69f2 100644 --- a/hw/scsi-disk.c +++ b/hw/scsi-disk.c @@ -73,6 +73,7 @@ struct SCSIDiskState char *serial; SCSISense sense; bool tray_open; + bool tray_locked; }; static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type); @@ -678,7 +679,7 @@ static int mode_sense_page(SCSIRequest *req, int page, uint8_t *p, p[5] = 0xff; /* CD DA, DA accurate, RW supported, RW corrected, C2 errors, ISRC, UPC, Bar code */ - p[6] = 0x2d | (bdrv_is_locked(s->bs)? 2 : 0); + p[6] = 0x2d | (s->tray_locked ? 2 : 0); /* Locking supported, jumper present, eject, tray */ p[7] = 0; /* no volume & mute control, no changer */ @@ -896,6 +897,7 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf) scsi_disk_emulate_start_stop(r); break; case ALLOW_MEDIUM_REMOVAL: + s->tray_locked = req->cmd.buf[4] & 1; bdrv_set_locked(s->bs, req->cmd.buf[4] & 1); break; case READ_CAPACITY_10:
We already track it in BlockDriverState. Just like tray open/close state, we should track it in the device models instead, because it's device state. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- hw/scsi-disk.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)