Re: [Qemu-devel] [PATCH RFC 08/11] scsi: SCSIDiskReq declaration moved to header

[John Snow <jsnow@redhat.com>]

On 08/18/2015 02:17 AM, Hannes Reinecke wrote:
> On 08/18/2015 01:42 AM, Alexander Bezzubikov wrote:
>> Signed-off-by: Alexander Bezzubikov <abezzubikov@ispras.ru>
>> ---
>>  hw/scsi/scsi-disk.c    | 12 ------------
>>  include/hw/scsi/scsi.h | 13 +++++++++++++
>>  2 files changed, 13 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
>> index f67d816..9d5f0a4 100644
>> --- a/hw/scsi/scsi-disk.c
>> +++ b/hw/scsi/scsi-disk.c
>> @@ -53,18 +53,6 @@ do { printf("scsi-disk: " fmt , ## __VA_ARGS__); } while (0)
>>  
>>  typedef struct SCSIDiskState SCSIDiskState;
>>  
>> -typedef struct SCSIDiskReq {
>> -    SCSIRequest req;
>> -    /* Both sector and sector_count are in terms of qemu 512 byte blocks.  */
>> -    uint64_t sector;
>> -    uint32_t sector_count;
>> -    uint32_t buflen;
>> -    bool started;
>> -    struct iovec iov;
>> -    QEMUIOVector qiov;
>> -    BlockAcctCookie acct;
>> -} SCSIDiskReq;
>> -
>>  #define SCSI_DISK_F_REMOVABLE             0
>>  #define SCSI_DISK_F_DPOFUA                1
>>  #define SCSI_DISK_F_NO_REMOVABLE_DEVOPS   2
>> diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
>> index 881ed23..de0546e 100644
>> --- a/include/hw/scsi/scsi.h
>> +++ b/include/hw/scsi/scsi.h
>> @@ -6,6 +6,7 @@
>>  #include "hw/block/block.h"
>>  #include "sysemu/sysemu.h"
>>  #include "qemu/notify.h"
>> +#include "block/accounting.h"
>>  
>>  #define MAX_SCSI_DEVS	255
>>  
>> @@ -120,6 +121,18 @@ extern const VMStateDescription vmstate_scsi_device;
>>      .offset     = vmstate_offset_value(_state, _field, SCSIDevice),  \
>>  }
>>  
>> +typedef struct SCSIDiskReq {
>> +    SCSIRequest req;
>> +    /* Both sector and sector_count are in terms of qemu 512 byte blocks.  */
>> +    uint64_t sector;
>> +    uint32_t sector_count;
>> +    uint32_t buflen;
>> +    bool started;
>> +    struct iovec iov;
>> +    QEMUIOVector qiov;
>> +    BlockAcctCookie acct;
>> +} SCSIDiskReq;
>> +
>>  /* cdrom.c */
>>  int cdrom_read_toc(int nb_sectors, uint8_t *buf, int msf, int start_track);
>>  int cdrom_read_toc_raw(int nb_sectors, uint8_t *buf, int msf, int session_num);
>>
> I'm not sure this is a good idea; the abstraction model seems to be that
> scsi-disk.h contains all declarations for the SCSI disk model, and as
> such the struct should remain there.
> 
> Probably it might be better to create a 'scsi-disk.h' include file, to
> contain all necessary things.
> 
> But actually I'm not sure.
> John? What's your opinion here?
> 
> Cheers,
> 
> Hannes
> 

I know I'm working from outdated copies here, but my guess would be to
attempt to limit exposure of this struct. Putting it in scsi.h probably
exposes it to more files than we anticipate.

I'll see what happens in v2 through v4 :)

--js