All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/3] SCSI ALUA support
@ 2015-11-16 14:36 Hannes Reinecke
  2015-11-16 14:36 ` [Qemu-devel] [PATCH 1/3] scsi-disk: Add 'port_group' property Hannes Reinecke
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Hannes Reinecke @ 2015-11-16 14:36 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Johannes Thumshirn, qemu-devel, Alexander Graf, Hannes Reinecke

Hi all,

here's a patchset to implement ALUA support for SCSI disks. With it
we can easily simulate a multipath setup:

-drive <drive>,if=none,id=disk1 \
-device scsi-disk,wwn=<wwn>,port_group=1,port_index=1,alua_state=0
-drive <drive>,if=none,id=disk2 \
-device scsi-disk,wwn=<wwn>,port_group=2,port_index=1,alua_state=2

What's a bit annoying is that one has to reference the underlying
block device _twice_, which means one has to stick with the 'raw'
format as anything more elaborate will cause data corruption on the guest
if both paths are active. Also the 'wwn' property is _actually_ a
property of the underlying block device, not the scsi disk.

This patchset implements 'implicit' ALUA mode only for the moment;
full explicit ALUA support involves quite a bit of logic in the qemu
backend.

As usual, comments and reviews are welcome.

Hannes Reinecke (3):
  scsi-disk: Add 'port_group' property
  scsi-disk: Add 'alua_state' property
  scsi-disk: Implement 'REPORT TARGET PORT GROUPS'

 hw/scsi/scsi-bus.c     |  15 ++++
 hw/scsi/scsi-disk.c    | 240 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/block/scsi.h   |  13 +++
 include/hw/scsi/scsi.h |   6 ++
 4 files changed, 274 insertions(+)

-- 
1.8.4.5

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [Qemu-devel] [PATCH 1/3] scsi-disk: Add 'port_group' property
  2015-11-16 14:36 [Qemu-devel] [PATCH 0/3] SCSI ALUA support Hannes Reinecke
@ 2015-11-16 14:36 ` Hannes Reinecke
  2015-11-16 14:36 ` [Qemu-devel] [PATCH 2/3] scsi-disk: Add 'alua_state' property Hannes Reinecke
  2015-11-16 14:36 ` [Qemu-devel] [PATCH 3/3] scsi-disk: Implement 'REPORT TARGET PORT GROUPS' Hannes Reinecke
  2 siblings, 0 replies; 6+ messages in thread
From: Hannes Reinecke @ 2015-11-16 14:36 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Johannes Thumshirn, qemu-devel, Alexander Graf, Hannes Reinecke

Each SCSI target port can have a 'target port group' identifier.
This identifier is used for management software to group individual
I_T_L nexus together eg when assembling a multipath topology.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 hw/scsi/scsi-disk.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 4797d83..f544f43 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -79,6 +79,7 @@ struct SCSIDiskState
     uint64_t wwn;
     uint64_t port_wwn;
     uint16_t port_index;
+    uint16_t port_group;
     uint64_t max_unmap_size;
     uint64_t max_io_size;
     QEMUBH *bh;
@@ -658,6 +659,14 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
                 stw_be_p(&outbuf[buflen + 2], s->port_index);
                 buflen += 4;
             }
+            if (s->port_group) {
+                outbuf[buflen++] = 0x61; // SAS / Binary
+                outbuf[buflen++] = 0x95; // PIV / Target port / target port group
+                outbuf[buflen++] = 0;    // reserved
+                outbuf[buflen++] = 4;
+                stw_be_p(&outbuf[buflen + 2], s->port_group);
+                buflen += 4;
+            }
             break;
         }
         case 0xb0: /* block limits */
@@ -2670,6 +2679,7 @@ static Property scsi_hd_properties[] = {
     DEFINE_PROP_UINT64("wwn", SCSIDiskState, wwn, 0),
     DEFINE_PROP_UINT64("port_wwn", SCSIDiskState, port_wwn, 0),
     DEFINE_PROP_UINT16("port_index", SCSIDiskState, port_index, 0),
+    DEFINE_PROP_UINT16("port_group", SCSIDiskState, port_group, 0),
     DEFINE_PROP_UINT64("max_unmap_size", SCSIDiskState, max_unmap_size,
                        DEFAULT_MAX_UNMAP_SIZE),
     DEFINE_PROP_UINT64("max_io_size", SCSIDiskState, max_io_size,
@@ -2720,6 +2730,7 @@ static Property scsi_cd_properties[] = {
     DEFINE_PROP_UINT64("wwn", SCSIDiskState, wwn, 0),
     DEFINE_PROP_UINT64("port_wwn", SCSIDiskState, port_wwn, 0),
     DEFINE_PROP_UINT16("port_index", SCSIDiskState, port_index, 0),
+    DEFINE_PROP_UINT16("port_group", SCSIDiskState, port_group, 0),
     DEFINE_PROP_UINT64("max_io_size", SCSIDiskState, max_io_size,
                        DEFAULT_MAX_IO_SIZE),
     DEFINE_PROP_END_OF_LIST(),
@@ -2785,6 +2796,7 @@ static Property scsi_disk_properties[] = {
     DEFINE_PROP_UINT64("wwn", SCSIDiskState, wwn, 0),
     DEFINE_PROP_UINT64("port_wwn", SCSIDiskState, port_wwn, 0),
     DEFINE_PROP_UINT16("port_index", SCSIDiskState, port_index, 0),
+    DEFINE_PROP_UINT16("port_group", SCSIDiskState, port_group, 0),
     DEFINE_PROP_UINT64("max_unmap_size", SCSIDiskState, max_unmap_size,
                        DEFAULT_MAX_UNMAP_SIZE),
     DEFINE_PROP_UINT64("max_io_size", SCSIDiskState, max_io_size,
-- 
1.8.4.5

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [Qemu-devel] [PATCH 2/3] scsi-disk: Add 'alua_state' property
  2015-11-16 14:36 [Qemu-devel] [PATCH 0/3] SCSI ALUA support Hannes Reinecke
  2015-11-16 14:36 ` [Qemu-devel] [PATCH 1/3] scsi-disk: Add 'port_group' property Hannes Reinecke
@ 2015-11-16 14:36 ` Hannes Reinecke
  2015-11-16 14:36 ` [Qemu-devel] [PATCH 3/3] scsi-disk: Implement 'REPORT TARGET PORT GROUPS' Hannes Reinecke
  2 siblings, 0 replies; 6+ messages in thread
From: Hannes Reinecke @ 2015-11-16 14:36 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Johannes Thumshirn, qemu-devel, Alexander Graf, Hannes Reinecke

To support asymmetric logical unit access (ALUA) we need to store
the ALUA state in the device structure.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 hw/scsi/scsi-bus.c     | 15 +++++++++++
 hw/scsi/scsi-disk.c    | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/block/scsi.h   | 13 +++++++++
 include/hw/scsi/scsi.h |  6 +++++
 4 files changed, 106 insertions(+)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index fd1171e..2096f4c 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -1294,6 +1294,21 @@ const struct SCSISense sense_code_LUN_NOT_READY = {
     .key = NOT_READY, .asc = 0x04, .ascq = 0x03
 };
 
+/* LUN not ready, asymmetric access state transition */
+const struct SCSISense sense_code_STATE_TRANSITION = {
+    .key = NOT_READY, .asc = 0x04, .ascq = 0x0a
+};
+
+/* LUN not ready, target port in standby state */
+const struct SCSISense sense_code_STATE_STANDBY = {
+    .key = NOT_READY, .asc = 0x04, .ascq = 0x0b
+};
+
+/* LUN not ready, target port in unavailable state */
+const struct SCSISense sense_code_STATE_UNAVAILABLE = {
+    .key = NOT_READY, .asc = 0x04, .ascq = 0x0c
+};
+
 /* LUN not ready, Medium not present */
 const struct SCSISense sense_code_NO_MEDIUM = {
     .key = NOT_READY, .asc = 0x3a, .ascq = 0x00
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index f544f43..fbd30f3 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -80,6 +80,7 @@ struct SCSIDiskState
     uint64_t port_wwn;
     uint16_t port_index;
     uint16_t port_group;
+    uint8_t alua_state;
     uint64_t max_unmap_size;
     uint64_t max_io_size;
     QEMUBH *bh;
@@ -1890,6 +1891,60 @@ static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
         break;
     }
 
+    if (s->alua_state != ALUA_STATE_ACTIVE_OPTIMIZED &&
+        s->alua_state != ALUA_STATE_ACTIVE_NON_OPTIMIZED) {
+        bool standby_allowed = true;
+        bool unavailable_allowed = true;
+        bool transition_allowed = true;
+
+        switch(req->cmd.buf[0]) {
+        case MAINTENANCE_IN:
+            if ((req->cmd.buf[1] & 31) != MI_REPORT_TARGET_PORT_GROUPS) {
+                transition_allowed = false;
+                unavailable_allowed = false;
+                standby_allowed = false;
+            }
+            /* Fallthrough */
+        case INQUIRY:
+        case REPORT_LUNS:
+        case REQUEST_SENSE:
+            break;
+        case MAINTENANCE_OUT:
+            transition_allowed = false;
+            break;
+        case PERSISTENT_RESERVE_IN:
+        case PERSISTENT_RESERVE_OUT:
+        case LOG_SENSE:
+        case LOG_SELECT:
+        case MODE_SENSE:
+        case MODE_SENSE_10:
+        case MODE_SELECT:
+        case MODE_SELECT_10:
+        case RECEIVE_DIAGNOSTIC:
+        case SEND_DIAGNOSTIC:
+            transition_allowed = false;
+            unavailable_allowed = false;
+            break;
+        default:
+            transition_allowed = false;
+            unavailable_allowed = false;
+            standby_allowed = false;
+            break;
+        }
+        if (s->alua_state == ALUA_STATE_STANDBY && !standby_allowed) {
+            scsi_check_condition(r, SENSE_CODE(STATE_STANDBY));
+            return 0;
+        }
+        if (s->alua_state == ALUA_STATE_UNAVAILABLE && !unavailable_allowed) {
+            scsi_check_condition(r, SENSE_CODE(STATE_UNAVAILABLE));
+            return 0;
+        }
+        if (s->alua_state == ALUA_STATE_TRANSITION && !transition_allowed) {
+            scsi_check_condition(r, SENSE_CODE(STATE_TRANSITION));
+            return 0;
+        }
+    }
+
     /*
      * FIXME: we shouldn't return anything bigger than 4k, but the code
      * requires the buffer to be as big as req->cmd.xfer in several
@@ -2156,6 +2211,21 @@ static int32_t scsi_disk_dma_command(SCSIRequest *req, uint8_t *buf)
         return 0;
     }
 
+    switch (s->alua_state) {
+    case ALUA_STATE_ACTIVE_OPTIMIZED:
+    case ALUA_STATE_ACTIVE_NON_OPTIMIZED:
+        break;
+    case ALUA_STATE_STANDBY:
+        scsi_check_condition(r, SENSE_CODE(STATE_STANDBY));
+        return 0;
+    case ALUA_STATE_UNAVAILABLE:
+        scsi_check_condition(r, SENSE_CODE(STATE_UNAVAILABLE));
+        return 0;
+    case ALUA_STATE_TRANSITION:
+        scsi_check_condition(r, SENSE_CODE(STATE_TRANSITION));
+        return 0;
+    }
+
     len = scsi_data_cdb_xfer(r->req.cmd.buf);
     switch (command) {
     case READ_6:
@@ -2680,6 +2750,7 @@ static Property scsi_hd_properties[] = {
     DEFINE_PROP_UINT64("port_wwn", SCSIDiskState, port_wwn, 0),
     DEFINE_PROP_UINT16("port_index", SCSIDiskState, port_index, 0),
     DEFINE_PROP_UINT16("port_group", SCSIDiskState, port_group, 0),
+    DEFINE_PROP_UINT8("alua_state", SCSIDiskState, alua_state, 0),
     DEFINE_PROP_UINT64("max_unmap_size", SCSIDiskState, max_unmap_size,
                        DEFAULT_MAX_UNMAP_SIZE),
     DEFINE_PROP_UINT64("max_io_size", SCSIDiskState, max_io_size,
@@ -2797,6 +2868,7 @@ static Property scsi_disk_properties[] = {
     DEFINE_PROP_UINT64("port_wwn", SCSIDiskState, port_wwn, 0),
     DEFINE_PROP_UINT16("port_index", SCSIDiskState, port_index, 0),
     DEFINE_PROP_UINT16("port_group", SCSIDiskState, port_group, 0),
+    DEFINE_PROP_UINT8("alua_state", SCSIDiskState, alua_state, 0),
     DEFINE_PROP_UINT64("max_unmap_size", SCSIDiskState, max_unmap_size,
                        DEFAULT_MAX_UNMAP_SIZE),
     DEFINE_PROP_UINT64("max_io_size", SCSIDiskState, max_io_size,
diff --git a/include/block/scsi.h b/include/block/scsi.h
index a311341..a9d0f64 100644
--- a/include/block/scsi.h
+++ b/include/block/scsi.h
@@ -151,6 +151,11 @@ const char *scsi_command_name(uint8_t cmd);
 #define SAI_READ_CAPACITY_16  0x10
 
 /*
+ * MAINTENANCE IN subcodes
+ */
+#define MI_REPORT_TARGET_PORT_GROUPS 0xa
+
+/*
  * READ POSITION service action codes
  */
 #define SHORT_FORM_BLOCK_ID  0x00
@@ -306,4 +311,12 @@ const char *scsi_command_name(uint8_t cmd);
 #define MMC_PROFILE_HDDVD_RW_DL         0x005A
 #define MMC_PROFILE_INVALID             0xFFFF
 
+#define ALUA_STATE_ACTIVE_OPTIMIZED     0x0
+#define ALUA_STATE_ACTIVE_NON_OPTIMIZED 0x1
+#define ALUA_STATE_STANDBY              0x2
+#define ALUA_STATE_UNAVAILABLE          0x3
+#define ALUA_STATE_LBA_DEPENDENT        0x4
+#define ALUA_STATE_OFFLINE              0xE
+#define ALUA_STATE_TRANSITION           0xF
+
 #endif
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index cdaf0f8..1574862 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -185,6 +185,12 @@ void scsi_bus_legacy_handle_cmdline(SCSIBus *bus, Error **errp);
 extern const struct SCSISense sense_code_NO_SENSE;
 /* LUN not ready, Manual intervention required */
 extern const struct SCSISense sense_code_LUN_NOT_READY;
+/* LUN not ready, asymmetric access state transition */
+extern const struct SCSISense sense_code_STATE_TRANSITION;
+/* LUN not ready, Target Port in standby state */
+extern const struct SCSISense sense_code_STATE_STANDBY;
+/* LUN not ready, Target Port in unavailable state */
+extern const struct SCSISense sense_code_STATE_UNAVAILABLE;
 /* LUN not ready, Medium not present */
 extern const struct SCSISense sense_code_NO_MEDIUM;
 /* LUN not ready, medium removal prevented */
-- 
1.8.4.5

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [Qemu-devel] [PATCH 3/3] scsi-disk: Implement 'REPORT TARGET PORT GROUPS'
  2015-11-16 14:36 [Qemu-devel] [PATCH 0/3] SCSI ALUA support Hannes Reinecke
  2015-11-16 14:36 ` [Qemu-devel] [PATCH 1/3] scsi-disk: Add 'port_group' property Hannes Reinecke
  2015-11-16 14:36 ` [Qemu-devel] [PATCH 2/3] scsi-disk: Add 'alua_state' property Hannes Reinecke
@ 2015-11-16 14:36 ` Hannes Reinecke
  2015-11-25  7:55   ` Stefan Hajnoczi
  2 siblings, 1 reply; 6+ messages in thread
From: Hannes Reinecke @ 2015-11-16 14:36 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Johannes Thumshirn, qemu-devel, Alexander Graf, Hannes Reinecke

Implement support for REPORT TARGET PORT GROUPS scsi command.
Note that target port groups are referenced per SCSI wwn ,
which might be connected to different hosts. So we need to
walk the entire qtree to find all eligible SCSI devices.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 hw/scsi/scsi-disk.c | 156 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 156 insertions(+)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index fbd30f3..2ca2ef0 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -794,6 +794,10 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
         outbuf[4] = 36 - 5;
     }
 
+    /* Enable TGPS bit */
+    if (s->wwn)
+        outbuf[4] = 1;
+
     /* Sync data transfer and TCQ.  */
     outbuf[7] = 0x10 | (req->bus->info->tcq ? 0x02 : 0);
     return buflen;
@@ -1859,6 +1863,145 @@ static void scsi_disk_emulate_write_data(SCSIRequest *req)
     }
 }
 
+typedef struct PortGroupEnumerate {
+    int numgrp;
+    uint64_t wwn;
+    uint16_t grp[16];
+    uint8_t alua_state[16];
+    uint16_t alua_mask;
+} PortGroupEnumerate;
+
+static void qbus_enumerate_port_group(PortGroupEnumerate *, BusState *);
+
+static void qdev_enumerate_port_group(PortGroupEnumerate *pg, DeviceState *dev)
+{
+    BusState *child;
+
+    if (!strcmp(object_get_typename(OBJECT(dev->parent_bus)), TYPE_SCSI_BUS)) {
+        SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev.qdev, dev);
+        DPRINTF("wwn 0x%" PRIx64 " pg %u state %x\n",
+                s->wwn, s->port_group, s->alua_state);
+        if (s->wwn == pg->wwn) {
+            bool pg_found = false;
+            int i;
+
+            for (i = 0; i < pg->numgrp; i++) {
+                if (pg->grp[i] == s->port_group) {
+                    pg_found = true;
+                    break;
+                }
+            }
+            if (!pg_found) {
+                pg->grp[pg->numgrp] = s->port_group;
+                pg->alua_state[pg->numgrp] = s->alua_state;
+                pg->alua_mask |= 1 << s->alua_state;
+                pg->numgrp++;
+            }
+        }
+    }
+    QLIST_FOREACH(child, &dev->child_bus, sibling) {
+        qbus_enumerate_port_group(pg, child);
+    }
+}
+
+static void qbus_enumerate_port_group(PortGroupEnumerate *pg, BusState *bus)
+{
+    BusChild *kid;
+
+    QTAILQ_FOREACH(kid, &bus->children, sibling) {
+        DeviceState *dev = kid->child;
+        qdev_enumerate_port_group(pg, dev);
+    }
+}
+
+typedef struct PortDescEnumerate {
+    int numdesc;
+    uint64_t wwn;
+    uint16_t port_group;
+    uint8_t *desc;
+} PortDescEnumerate;
+
+static void qbus_enumerate_port_desc(PortDescEnumerate *, BusState *);
+
+static void qdev_enumerate_port_desc(PortDescEnumerate *pd, DeviceState *dev)
+{
+    BusState *child;
+
+    if (!strcmp(object_get_typename(OBJECT(dev->parent_bus)), TYPE_SCSI_BUS)) {
+        SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev.qdev, dev);
+        if (s->wwn == pd->wwn &&
+            s->port_group == pd->port_group) {
+            pd->desc[0] = 0;
+            pd->desc[1] = 0;
+            pd->desc[2] = s->port_index >> 16;
+            pd->desc[3] = s->port_index & 0xff;
+            pd->desc += 4;
+            pd->numdesc++;
+        }
+    }
+    QLIST_FOREACH(child, &dev->child_bus, sibling) {
+        qbus_enumerate_port_desc(pd, child);
+    }
+}
+
+static void qbus_enumerate_port_desc(PortDescEnumerate *pd, BusState *bus)
+{
+    BusChild *kid;
+
+    QTAILQ_FOREACH(kid, &bus->children, sibling) {
+        DeviceState *dev = kid->child;
+        qdev_enumerate_port_desc(pd, dev);
+    }
+}
+
+static int scsi_emulate_report_target_port_groups(SCSIDiskState *s, uint8_t *inbuf)
+{
+    uint8_t *p = inbuf;
+    PortGroupEnumerate pg;
+    PortDescEnumerate pd;
+    int buflen = 0, i;
+
+    if (!s->wwn) {
+        return -1;
+    }
+
+    pg.numgrp = 0;
+    pg.wwn = s->wwn;
+
+    if (sysbus_get_default())
+        qbus_enumerate_port_group(&pg, sysbus_get_default());
+
+    if (pg.numgrp == 0) {
+        return -1;
+    }
+    DPRINTF("wwn 0x%" PRIx64 " %d port groups \n", s->wwn, pg.numgrp);
+    p = &inbuf[4];
+    for (i = 0; i < pg.numgrp; i++) {
+        pd.numdesc = 0;
+        pd.wwn = s->wwn;
+        pd.port_group = pg.grp[i];
+        pd.desc = &p[8];
+        buflen += 8;
+        qbus_enumerate_port_desc(&pd, sysbus_get_default());
+        DPRINTF("pg %x: %d port descriptors\n", pg.grp[i], pd.numdesc);
+        p[0] = pg.alua_state[i];
+        p[1] = pg.alua_mask;
+        p[2] = pg.grp[i] >> 8;
+        p[3] = pg.grp[i] & 0xff;
+        p[7] = pd.numdesc;
+        p += 8 + pd.numdesc * 4;
+        buflen += pd.numdesc * 4;
+    }
+    if (buflen) {
+        inbuf[0] = (buflen >> 24) & 0xff;
+        inbuf[1] = (buflen >> 16) & 0xff;
+        inbuf[2] = (buflen >> 8) & 0xff;
+        inbuf[3] = buflen & 0xff;
+    }
+
+    return buflen + 4;
+}
+
 static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
 {
     SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
@@ -2054,6 +2197,19 @@ static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
             goto illegal_request;
         }
         break;
+    case MAINTENANCE_IN:
+        if ((req->cmd.buf[1] & 31) == MI_REPORT_TARGET_PORT_GROUPS) {
+            DPRINTF("MI REPORT TARGET PORT GROUPS\n");
+            memset(outbuf, 0, req->cmd.xfer);
+            buflen = scsi_emulate_report_target_port_groups(s, outbuf);
+            if (buflen < 0) {
+                goto illegal_request;
+            }
+            break;
+        }
+        DPRINTF("Unsupported Maintenance In\n");
+        goto illegal_request;
+        break;
     case MECHANISM_STATUS:
         buflen = scsi_emulate_mechanism_status(s, outbuf);
         if (buflen < 0) {
-- 
1.8.4.5

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH 3/3] scsi-disk: Implement 'REPORT TARGET PORT GROUPS'
  2015-11-16 14:36 ` [Qemu-devel] [PATCH 3/3] scsi-disk: Implement 'REPORT TARGET PORT GROUPS' Hannes Reinecke
@ 2015-11-25  7:55   ` Stefan Hajnoczi
  2015-11-25  8:55     ` Hannes Reinecke
  0 siblings, 1 reply; 6+ messages in thread
From: Stefan Hajnoczi @ 2015-11-25  7:55 UTC (permalink / raw)
  To: Hannes Reinecke
  Cc: Johannes Thumshirn, Paolo Bonzini, qemu-devel, Alexander Graf

[-- Attachment #1: Type: text/plain, Size: 873 bytes --]

On Mon, Nov 16, 2015 at 03:36:58PM +0100, Hannes Reinecke wrote:
> +    /* Enable TGPS bit */
> +    if (s->wwn)
> +        outbuf[4] = 1;

QEMU coding style: Please always use curly braces, even if the if
statement body is just one line.

> +static void qdev_enumerate_port_group(PortGroupEnumerate *pg, DeviceState *dev)
> +{
> +    BusState *child;
> +
> +    if (!strcmp(object_get_typename(OBJECT(dev->parent_bus)), TYPE_SCSI_BUS)) {

object_dynamic_cast(OBJECT(dev->parent_bus), TYPE_SCSI_BUS) is shorter
and doesn't require the explicit strcmp().

> +static int scsi_emulate_report_target_port_groups(SCSIDiskState *s, uint8_t *inbuf)

"inbuf" seems to be an output buffer rather than an input buffer.  The
name "outbuf" would be clearer.

How does this function protect against buffer overflow?  It's not
obvious how we guarantee the output buffer is large enough.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH 3/3] scsi-disk: Implement 'REPORT TARGET PORT GROUPS'
  2015-11-25  7:55   ` Stefan Hajnoczi
@ 2015-11-25  8:55     ` Hannes Reinecke
  0 siblings, 0 replies; 6+ messages in thread
From: Hannes Reinecke @ 2015-11-25  8:55 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: Johannes Thumshirn, Paolo Bonzini, qemu-devel, Alexander Graf

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/25/2015 08:55 AM, Stefan Hajnoczi wrote:
> On Mon, Nov 16, 2015 at 03:36:58PM +0100, Hannes Reinecke
> wrote:
>> +    /* Enable TGPS bit */ +    if (s->wwn) +
>> outbuf[4] = 1;
> 
> QEMU coding style: Please always use curly braces, even if the
> if statement body is just one line.
> 
Yeah, will be doing so on the next round.

>> +static void qdev_enumerate_port_group(PortGroupEnumerate
>> *pg, DeviceState *dev) +{ +    BusState *child; + +    if
>> (!strcmp(object_get_typename(OBJECT(dev->parent_bus)),
>> TYPE_SCSI_BUS)) {
> 
> object_dynamic_cast(OBJECT(dev->parent_bus), TYPE_SCSI_BUS) is
> shorter and doesn't require the explicit strcmp().
> 
The ever-changing qemu infrastructure ... yeah, will be fixing it up.

>> +static int
>> scsi_emulate_report_target_port_groups(SCSIDiskState *s,
>> uint8_t *inbuf)
> 
> "inbuf" seems to be an output buffer rather than an input
> buffer.  The name "outbuf" would be clearer.
> 
> How does this function protect against buffer overflow?  It's
> not obvious how we guarantee the output buffer is large
> enough.
> 
Good question; will have to check.

Cheers,

Hannes
- -- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJWVXeZAAoJEGz4yi9OyKjP2yAQAIENfXolPp+Q6BsWyHEbinMn
P/Z+HlgEZE+fSP8qq4F4txzB/0FUpiuvonKTzJmC41izhXLGDs6vFPNH9CX7bNob
PmbWD7ZXVVJdEX8xpmJRNKN1XfIFcwHVE51FNqM62OggfMZx4XW3zZ9scpzEjaW0
70l1q7FEn+sg7BsE4aH11+fhdAdQaEo3G2tx8g6X2uleiNC0NJWIR7XyFJOl68wY
plmfotRgpX6x7vNKkge5EyMEBhpoltql8K6IsAJfWQlHIWduErGIXmCuaDdz1Whh
aLsfLNajG5CB7gZddu+XsIVlbujHqoTn26UTtPC8dL20w1S7wv/TaYnKLchRCn+5
UydjjI1AdB9EKqhQUGI6itGT+GDE/Rf3vDHzdUmZou4u/U3kuhWlupaCTGqLKpvA
HHOMi0EAqQrM30kYWX4jKhTSAP5Y8GKFQhBxVi7Y9pZSYWHIoMLg/LbKpFx97rN6
iSOkq9KYBJH+rgS5tBiHCTCYjrlbvHV5UiGVt2/Q0WjeRx0kMPLnLA1fGsiubnjs
ZjqpuIih72GTSSSzrvNPcpL4RmDWwXoh0UtamSs7jBMa/Gto8m0cHWxeaiJ3vxgC
E5EgxycmX81/smvq+kdre8XhECDYulyXffZxQWsZqsDChwFB+9OFL523bpZm1Pu4
+pqGLkHyVZPeQUCV1MWC
=b3kw
-----END PGP SIGNATURE-----

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2015-11-25  8:56 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-16 14:36 [Qemu-devel] [PATCH 0/3] SCSI ALUA support Hannes Reinecke
2015-11-16 14:36 ` [Qemu-devel] [PATCH 1/3] scsi-disk: Add 'port_group' property Hannes Reinecke
2015-11-16 14:36 ` [Qemu-devel] [PATCH 2/3] scsi-disk: Add 'alua_state' property Hannes Reinecke
2015-11-16 14:36 ` [Qemu-devel] [PATCH 3/3] scsi-disk: Implement 'REPORT TARGET PORT GROUPS' Hannes Reinecke
2015-11-25  7:55   ` Stefan Hajnoczi
2015-11-25  8:55     ` Hannes Reinecke

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.