Patchwork [02/14] scsi: add a qdev property for the disk's WWN

login
register
mail settings
Submitter Paolo Bonzini
Date July 2, 2012, 9:41 a.m.
Message ID <1341222087-24920-3-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/168531/
State New
Headers show

Comments

Paolo Bonzini - July 2, 2012, 9:41 a.m.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi-disk.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)
Blue Swirl - July 3, 2012, 7:09 p.m.
On Mon, Jul 2, 2012 at 9:41 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/scsi-disk.c |   14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
> index 747e756..e87e57c 100644
> --- a/hw/scsi-disk.c
> +++ b/hw/scsi-disk.c
> @@ -67,6 +67,7 @@ struct SCSIDiskState
>      bool media_changed;
>      bool media_event;
>      bool eject_request;
> +    uint64_t wwn;
>      QEMUBH *bh;
>      char *version;
>      char *serial;
> @@ -587,9 +588,17 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
>              outbuf[buflen++] = 0;   // not officially assigned
>              outbuf[buflen++] = 0;   // reserved
>              outbuf[buflen++] = id_len; // length of data following
> -
>              memcpy(outbuf+buflen, str, id_len);
>              buflen += id_len;
> +
> +            if (s->wwn) {
> +                outbuf[buflen++] = 0x1; // Binary
> +                outbuf[buflen++] = 0x3; // NAA
> +                outbuf[buflen++] = 0;   // reserved

C99 comments.

> +                outbuf[buflen++] = 8;
> +                stq_be_p(&outbuf[buflen], s->wwn);
> +                buflen += 8;
> +            }
>              break;
>          }
>          case 0xb0: /* block limits */
> @@ -1924,6 +1933,7 @@ static Property scsi_hd_properties[] = {
>                      SCSI_DISK_F_REMOVABLE, false),
>      DEFINE_PROP_BIT("dpofua", SCSIDiskState, features,
>                      SCSI_DISK_F_DPOFUA, false),
> +    DEFINE_PROP_HEX64("wwn", SCSIDiskState, wwn, 0),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> @@ -1968,6 +1978,7 @@ static TypeInfo scsi_hd_info = {
>
>  static Property scsi_cd_properties[] = {
>      DEFINE_SCSI_DISK_PROPERTIES(),
> +    DEFINE_PROP_HEX64("wwn", SCSIDiskState, wwn, 0),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> @@ -2029,6 +2040,7 @@ static Property scsi_disk_properties[] = {
>                      SCSI_DISK_F_REMOVABLE, false),
>      DEFINE_PROP_BIT("dpofua", SCSIDiskState, features,
>                      SCSI_DISK_F_DPOFUA, false),
> +    DEFINE_PROP_HEX64("wwn", SCSIDiskState, wwn, 0),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> --
> 1.7.10.2
>
>
>
Paolo Bonzini - July 4, 2012, 7:33 a.m.
Il 03/07/2012 21:09, Blue Swirl ha scritto:
>> > @@ -587,9 +588,17 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
>> >              outbuf[buflen++] = 0;   // not officially assigned
>> >              outbuf[buflen++] = 0;   // reserved
>> >              outbuf[buflen++] = id_len; // length of data following
>> > -
>> >              memcpy(outbuf+buflen, str, id_len);
>> >              buflen += id_len;
>> > +
>> > +            if (s->wwn) {
>> > +                outbuf[buflen++] = 0x1; // Binary
>> > +                outbuf[buflen++] = 0x3; // NAA
>> > +                outbuf[buflen++] = 0;   // reserved
> C99 comments.
> 

Just following the style of this code.  Feel free to send a patch to
replace with #defines.

Paolo
Blue Swirl - July 5, 2012, 6:03 p.m.
On Wed, Jul 4, 2012 at 7:33 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 03/07/2012 21:09, Blue Swirl ha scritto:
>>> > @@ -587,9 +588,17 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
>>> >              outbuf[buflen++] = 0;   // not officially assigned
>>> >              outbuf[buflen++] = 0;   // reserved
>>> >              outbuf[buflen++] = id_len; // length of data following
>>> > -
>>> >              memcpy(outbuf+buflen, str, id_len);
>>> >              buflen += id_len;
>>> > +
>>> > +            if (s->wwn) {
>>> > +                outbuf[buflen++] = 0x1; // Binary
>>> > +                outbuf[buflen++] = 0x3; // NAA
>>> > +                outbuf[buflen++] = 0;   // reserved
>> C99 comments.
>>
>
> Just following the style of this code.  Feel free to send a patch to
> replace with #defines.

That's not how we should work. New code should be compliant with our
goals. Pushing the responsibility for fixing issues to other people
does not scale.

>
> Paolo
>
Paolo Bonzini - July 6, 2012, 7:05 a.m.
Il 05/07/2012 20:03, Blue Swirl ha scritto:
> > > > +            if (s->wwn) {
> > > > +                outbuf[buflen++] = 0x1; // Binary
> > > > +                outbuf[buflen++] = 0x3; // NAA
> > > > +                outbuf[buflen++] = 0;   // reserved
> > >
> > > C99 comments.
> >
> > Just following the style of this code.  Feel free to send a patch to
> > replace with #defines.
> 
> That's not how we should work. New code should be compliant with our
> goals. Pushing the responsibility for fixing issues to other people
> does not scale.

I believe the coding style are attacking the wrong problem.  It's
end-of-line comments that should be avoided in favor of #defines, packed
structs, designated initializers, etc.  But for end-of-line comments,
C++ comments are superior to /* */ comments.

I do plan to fix the clarity issue with SCSI data structures and
constants.  But for now, the best compromise is to keep C++ comments
IMHO.  I'm not pushing the responsibility to other people in general,
but if they think C++ comments are a major issue they can send patches.

Paolo
Blue Swirl - July 7, 2012, 7:48 a.m.
On Fri, Jul 6, 2012 at 7:05 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 05/07/2012 20:03, Blue Swirl ha scritto:
>> > > > +            if (s->wwn) {
>> > > > +                outbuf[buflen++] = 0x1; // Binary
>> > > > +                outbuf[buflen++] = 0x3; // NAA
>> > > > +                outbuf[buflen++] = 0;   // reserved
>> > >
>> > > C99 comments.
>> >
>> > Just following the style of this code.  Feel free to send a patch to
>> > replace with #defines.
>>
>> That's not how we should work. New code should be compliant with our
>> goals. Pushing the responsibility for fixing issues to other people
>> does not scale.
>
> I believe the coding style are attacking the wrong problem.  It's
> end-of-line comments that should be avoided in favor of #defines, packed
> structs, designated initializers, etc.

This applies to C89 comments too. Perhaps HACKING file should mention this.

>  But for end-of-line comments,
> C++ comments are superior to /* */ comments.

Well, submit a change to CODING_STYLE then. I think the only places
where C99 is better are lines like
//#define DEBUG_XYZ

>
> I do plan to fix the clarity issue with SCSI data structures and
> constants.  But for now, the best compromise is to keep C++ comments
> IMHO.  I'm not pushing the responsibility to other people in general,
> but if they think C++ comments are a major issue they can send patches.

When you add them in new code, that is a problem. New code should
comply with CODING_STYLE,  code with other styles should be converted
as it is touched.

It shouldn't be much effort from your part to change the comments in
this patch to C89 style.

>
> Paolo
Paolo Bonzini - July 7, 2012, 12:22 p.m.
Il 07/07/2012 09:48, Blue Swirl ha scritto:
>> > I do plan to fix the clarity issue with SCSI data structures and
>> > constants.  But for now, the best compromise is to keep C++ comments
>> > IMHO.  I'm not pushing the responsibility to other people in general,
>> > but if they think C++ comments are a major issue they can send patches.
> When you add them in new code, that is a problem. New code should
> comply with CODING_STYLE,  code with other styles should be converted
> as it is touched.
> 
> It shouldn't be much effort from your part to change the comments in
> this patch to C89 style.

Sure, but that would be the wrong fix.

The right fix is to augment scsi-defs.h, and avoid the need for comments
completely.

Paolo

Patch

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 747e756..e87e57c 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -67,6 +67,7 @@  struct SCSIDiskState
     bool media_changed;
     bool media_event;
     bool eject_request;
+    uint64_t wwn;
     QEMUBH *bh;
     char *version;
     char *serial;
@@ -587,9 +588,17 @@  static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
             outbuf[buflen++] = 0;   // not officially assigned
             outbuf[buflen++] = 0;   // reserved
             outbuf[buflen++] = id_len; // length of data following
-
             memcpy(outbuf+buflen, str, id_len);
             buflen += id_len;
+
+            if (s->wwn) {
+                outbuf[buflen++] = 0x1; // Binary
+                outbuf[buflen++] = 0x3; // NAA
+                outbuf[buflen++] = 0;   // reserved
+                outbuf[buflen++] = 8;
+                stq_be_p(&outbuf[buflen], s->wwn);
+                buflen += 8;
+            }
             break;
         }
         case 0xb0: /* block limits */
@@ -1924,6 +1933,7 @@  static Property scsi_hd_properties[] = {
                     SCSI_DISK_F_REMOVABLE, false),
     DEFINE_PROP_BIT("dpofua", SCSIDiskState, features,
                     SCSI_DISK_F_DPOFUA, false),
+    DEFINE_PROP_HEX64("wwn", SCSIDiskState, wwn, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -1968,6 +1978,7 @@  static TypeInfo scsi_hd_info = {
 
 static Property scsi_cd_properties[] = {
     DEFINE_SCSI_DISK_PROPERTIES(),
+    DEFINE_PROP_HEX64("wwn", SCSIDiskState, wwn, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2029,6 +2040,7 @@  static Property scsi_disk_properties[] = {
                     SCSI_DISK_F_REMOVABLE, false),
     DEFINE_PROP_BIT("dpofua", SCSIDiskState, features,
                     SCSI_DISK_F_DPOFUA, false),
+    DEFINE_PROP_HEX64("wwn", SCSIDiskState, wwn, 0),
     DEFINE_PROP_END_OF_LIST(),
 };