[Qemu-devel] [PATCH 7/9] scsi-generic: Make data-copying logic clearer

[Alex Pyrgiotis]

The copying of data to/from the intermediate buffer of the device is
done by scsi_req_data(). Internally, scsi_req_data() also restarts the
request with scsi_req_continue(). Therefore, we need a guard variable to
know when the contents of the intermediate buffer are in sync with the
data of the guest kernel, so that we can proceed with the request.

While there is such a guard variable in the `SCSIGenericReq' struct (the
`len' field), its intent is not clear and is assigned various
values, when only two are necessary; 0 and 1.

Rename the `len' field to `synced' and add an explanation for what it
does.

Signed-off-by: Alex Pyrgiotis <apyr...@arrikto.com>
Signed-off-by: Dimitris Aragiorgis <dim...@arrikto.com>

diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 8e2058d..6c0cfa5 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -54,7 +54,23 @@ typedef struct SCSIGenericReq {
     SCSIRequest req;
     uint8_t *buf;
     int buflen;
-    int len;
+
+    /*
+     * Note for the `synced' field:
+     *
+     * If the controller does not provide the device with a scatter-gather
+     * list, then the device must create an intermediate buffer for the
+     * request's data.  For write requests, this buffer must be filled with
+     * data from the controller. For read requests, the controller must get the
+     * data from the buffer.
+     *
+     * The above transfers are handled by the scsi_req_data() function. Since
+     * scsi_req_data() effectively restarts the request, we need an indication
+     * so that we don't do the same request twice. This indication is the
+     * `synced' field.
+     */
+    int synced;
+
     sg_io_hdr_t io_header;
 } SCSIGenericReq;
 
@@ -192,7 +208,6 @@ static void scsi_buf_read_complete(void *opaque, int ret)
     len = r->io_header.dxfer_len - r->io_header.resid;
     DPRINTF("Data ready tag=0x%x len=%d\n", r->req.tag, len);
 
-    r->len = -1;
     if (len == 0) {
         scsi_command_complete_noio(r, 0);
         return;
@@ -224,6 +239,7 @@ static void scsi_buf_read_complete(void *opaque, int ret)
             r->buf[3] |= 0x80;
         }
     }
+    r->synced = 1;
     scsi_req_data(&r->req, len);
     scsi_req_unref(&r->req);
 }
@@ -260,7 +276,9 @@ static void scsi_buf_read_data(SCSIRequest *req)
 
     /* The request is used as the AIO opaque value, so add a ref.  */
     scsi_req_ref(&r->req);
-    if (r->len == -1) {
+
+    /* If we are here due to scsi_req_data(), we can complete the request. */
+    if (r->synced) {
         scsi_command_complete_noio(r, 0);
         return;
     }
@@ -304,9 +322,14 @@ static void scsi_buf_write_data(SCSIRequest *req)
     SCSIGenericReq *r = DO_UPCAST(SCSIGenericReq, req, req);
 
     DPRINTF("scsi_buf_write_data 0x%x\n", req->tag);
-    if (r->len == 0) {
-        r->len = r->buflen;
-        scsi_req_data(&r->req, r->len);
+
+    /*
+     * Before performing the write request, we need to transfer the data to
+     * our intermediate buffer.
+     */
+    if (!r->synced) {
+        r->synced = 1;
+        scsi_req_data(req, r->buflen);
         return;
     }
 
@@ -365,9 +388,7 @@ static int32_t scsi_common_send_command(SCSIRequest *req, 
uint8_t *cmd)
     }
 
     memset(r->buf, 0, r->buflen);
-    r->len = r->req.cmd.xfer;
     if (r->req.cmd.mode == SCSI_XFER_TO_DEV) {
-        r->len = 0;
         return -r->req.cmd.xfer;
     } else {
         return r->req.cmd.xfer;
-- 
2.6.2