respond to more of Thanos's review

Signed-off-by: William Henderson <william.henderson@nutanix.com>

3 files changed, 32 insertions, 22 deletions

diff --git a/lib/dma.c b/lib/dma.c
index bde1226..b9588b7 100644
--- a/lib/dma.c
+++ b/lib/dma.c
@@ -579,7 +579,7 @@ dirty_page_get_extend(dma_memory_region_t *region, char *bitmap,
      */
     uint8_t client_bit_idx = 0;
     size_t server_byte_idx;
-    int server_bit_idx_into_byte;
+    int server_bit_idx;
     size_t factor = server_pgsize / client_pgsize;
 
     /*
@@ -587,6 +587,7 @@ dirty_page_get_extend(dma_memory_region_t *region, char *bitmap,
      */
     for (server_byte_idx = 0; server_byte_idx < server_bitmap_size;
          server_byte_idx++) {
+
         if (client_bit_idx / CHAR_BIT >= client_bitmap_size) {
             break;
         }
@@ -599,10 +600,8 @@ dirty_page_get_extend(dma_memory_region_t *region, char *bitmap,
          * Iterate through the bits of the server byte, repeating bits to reach
          * the desired page size.
          */
-        for (server_bit_idx_into_byte = 0;
-             server_bit_idx_into_byte < CHAR_BIT;
-             server_bit_idx_into_byte++) {
-            uint8_t server_bit = (out >> server_bit_idx_into_byte) & 1;
+        for (server_bit_idx = 0; server_bit_idx < CHAR_BIT; server_bit_idx++) {
+            uint8_t server_bit = (out >> server_bit_idx) & 1;
 
             /*
              * Repeat `factor` times the bit at index `j` of `out`.
@@ -611,11 +610,12 @@ dirty_page_get_extend(dma_memory_region_t *region, char *bitmap,
              * `factor` bits in the client bitmap, from `client_bit_idx` to
              * `end_client_bit_idx`.
              */
-            size_t end_client_bit_idx = client_bit_idx + factor;
-            while (client_bit_idx < end_client_bit_idx) {
+            for (size_t end_client_bit_idx = client_bit_idx + factor;
+                 client_bit_idx < end_client_bit_idx;
+                 client_bit_idx++) {
+
                 bitmap[client_bit_idx / CHAR_BIT] |=
                     server_bit << (client_bit_idx % CHAR_BIT);
-                client_bit_idx++;
             }
         }
     }
@@ -634,7 +634,7 @@ dirty_page_get_combine(dma_memory_region_t *region, char *bitmap,
      */
     uint8_t client_bit_idx = 0;
     size_t server_byte_idx;
-    int server_bit_idx_into_byte;
+    int server_bit_idx;
     size_t factor = client_pgsize / server_pgsize;
 
     /*
@@ -642,6 +642,7 @@ dirty_page_get_combine(dma_memory_region_t *region, char *bitmap,
      */
     for (server_byte_idx = 0; server_byte_idx < server_bitmap_size;
          server_byte_idx++) {
+
         if (client_bit_idx / CHAR_BIT >= client_bitmap_size) {
             break;
         }
@@ -654,10 +655,8 @@ dirty_page_get_combine(dma_memory_region_t *region, char *bitmap,
          * Iterate through the bits of the server byte, combining bits to reach
          * the desired page size.
          */
-        for (server_bit_idx_into_byte = 0;
-             server_bit_idx_into_byte < CHAR_BIT;
-             server_bit_idx_into_byte++) {
-            uint8_t server_bit = (out >> server_bit_idx_into_byte) & 1;
+        for (server_bit_idx = 0; server_bit_idx < CHAR_BIT; server_bit_idx++) {
+            uint8_t server_bit = (out >> server_bit_idx) & 1;
 
             /*
              * OR `factor` bits of the server bitmap with the same bit at
@@ -670,9 +669,13 @@ dirty_page_get_combine(dma_memory_region_t *region, char *bitmap,
              * Only move onto the next bit in the client bitmap once we've
              * OR'd `factor` bits.
              */
-            if (((server_byte_idx * CHAR_BIT) + server_bit_idx_into_byte)
-                    % factor == factor - 1) {
+            if (((server_byte_idx * CHAR_BIT) + server_bit_idx) % factor
+                    == factor - 1) {
                 client_bit_idx++;
+                
+                if (client_bit_idx / CHAR_BIT >= client_bitmap_size) {
+                    return;
+                }
             }
         }
     }
diff --git a/lib/libvfio-user.c b/lib/libvfio-user.c
index c0ca55d..03b1f9d 100644
--- a/lib/libvfio-user.c
+++ b/lib/libvfio-user.c
@@ -1039,6 +1039,7 @@ handle_dma_device_feature_get(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg,
 
     if (dma == NULL) {
         vfu_log(vfu_ctx, LOG_ERR, "DMA not enabled for DMA device feature");
+        return ERROR_INT(EINVAL);
     }
 
     ssize_t bitmap_size = get_bitmap_size(rep->length, rep->page_size);
@@ -1109,7 +1110,8 @@ handle_device_feature(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg)
     assert(msg != NULL);
 
     if (msg->in.iov.iov_len < sizeof(struct vfio_user_device_feature)) {
-        vfu_log(vfu_ctx, LOG_ERR, "message too short");
+        vfu_log(vfu_ctx, LOG_ERR, "message too short (%ld)",
+                msg->in.iov.iov_len);
         return ERROR_INT(EINVAL);
     }
 
@@ -1121,7 +1123,8 @@ handle_device_feature(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg)
     uint32_t supported_ops = device_feature_flags_supported(vfu_ctx, feature);
 
     if ((req->flags & supported_ops) != operations || supported_ops == 0) {
-        vfu_log(vfu_ctx, LOG_ERR, "unsupported operation(s)");
+        vfu_log(vfu_ctx, LOG_ERR, "unsupported operation(s), flags=%d",
+                req->flags);
         return ERROR_INT(EINVAL);
     }
 
diff --git a/lib/migration.c b/lib/migration.c
index aefd103..8901a00 100644
--- a/lib/migration.c
+++ b/lib/migration.c
@@ -292,7 +292,8 @@ handle_mig_data_read(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg)
     assert(msg != NULL);
 
     if (msg->in.iov.iov_len < sizeof(struct vfio_user_mig_data)) {
-        vfu_log(vfu_ctx, LOG_ERR, "message too short");
+        vfu_log(vfu_ctx, LOG_ERR, "message too short (%ld)",
+                msg->in.iov.iov_len);
         return ERROR_INT(EINVAL);
     }
 
@@ -335,7 +336,7 @@ handle_mig_data_read(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg)
     ssize_t ret = migr->callbacks.read_data(vfu_ctx, &res->data, req->size);
 
     if (ret < 0) {
-        vfu_log(vfu_ctx, LOG_ERR, "read_data callback failed");
+        vfu_log(vfu_ctx, LOG_ERR, "read_data callback failed, errno=%d", errno);
         msg->out.iov.iov_len = 0;
         return ret;
     }
@@ -353,7 +354,8 @@ handle_mig_data_write(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg)
     assert(msg != NULL);
 
     if (msg->in.iov.iov_len < sizeof(struct vfio_user_mig_data)) {
-        vfu_log(vfu_ctx, LOG_ERR, "message too short");
+        vfu_log(vfu_ctx, LOG_ERR, "message too short (%ld)",
+                msg->in.iov.iov_len);
         return ERROR_INT(EINVAL);
     }
 
@@ -386,10 +388,12 @@ handle_mig_data_write(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg)
     ssize_t ret = migr->callbacks.write_data(vfu_ctx, &req->data, req->size);
 
     if (ret < 0) {
-        vfu_log(vfu_ctx, LOG_ERR, "write_data callback failed");
+        vfu_log(vfu_ctx, LOG_ERR, "write_data callback failed, errno=%d",
+                errno);
         return ret;
     } else if (ret != req->size) {
-        vfu_log(vfu_ctx, LOG_ERR, "migration data partial write");
+        vfu_log(vfu_ctx, LOG_ERR, "migration data partial write of size=%ld",
+                ret);
         return ERROR_INT(EINVAL);
     }