respond to Thanos's review

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

3 files changed, 36 insertions, 24 deletions

diff --git a/include/vfio-user.h b/include/vfio-user.h
index a994718..8434852 100644
--- a/include/vfio-user.h
+++ b/include/vfio-user.h
@@ -205,11 +205,13 @@ typedef struct vfio_user_region_io_fds_reply {
     } sub_regions[];
 } __attribute__((packed)) vfio_user_region_io_fds_reply_t;
 
+/* Analogous to struct vfio_device_feature_dma_logging_range */
 struct vfio_user_device_feature_dma_logging_range {
     uint64_t iova;
     uint64_t length;
 } __attribute__((packed));
 
+/* Analogous to struct vfio_device_feature_dma_logging_control */
 struct vfio_user_device_feature_dma_logging_control {
     uint64_t page_size;
     uint32_t num_ranges;
@@ -217,6 +219,7 @@ struct vfio_user_device_feature_dma_logging_control {
     struct vfio_user_device_feature_dma_logging_range ranges[];
 } __attribute__((packed));
 
+/* Analogous to struct vfio_device_feature_dma_logging_report */
 struct vfio_user_device_feature_dma_logging_report {
     uint64_t iova;
     uint64_t length;
@@ -230,7 +233,7 @@ struct vfio_user_device_feature_dma_logging_report {
 #define VFIO_DEVICE_FEATURE_DMA_LOGGING_REPORT 8
 #endif
 
-/* Analogous to vfio_device_feature */
+/* Analogous to struct vfio_device_feature */
 struct vfio_user_device_feature {
 	uint32_t	argsz;
 	uint32_t	flags;
@@ -243,13 +246,14 @@ struct vfio_user_device_feature {
 	uint8_t  	data[];
 } __attribute__((packed));
 
-/* Analogous to vfio_device_feature_migration */
+/* Analogous to struct vfio_device_feature_migration */
 struct vfio_user_device_feature_migration {
     uint64_t flags;
 #ifndef VFIO_REGION_TYPE_MIGRATION_DEPRECATED
 #define VFIO_MIGRATION_STOP_COPY    (1 << 0)
 #define VFIO_MIGRATION_P2P          (1 << 1)
 #endif
+/* PRE_COPY was added in a later kernel version */
 #ifndef VFIO_MIGRATION_PRE_COPY
 #define VFIO_MIGRATION_PRE_COPY     (1 << 2)
 #endif
@@ -260,7 +264,7 @@ struct vfio_user_device_feature_migration {
 _Static_assert(sizeof(struct vfio_user_device_feature_migration) == 8,
                "bad vfio_user_device_feature_migration size");
 
-/* Analogous to vfio_device_feature_mig_state */
+/* Analogous to struct vfio_device_feature_mig_state */
 struct vfio_user_device_feature_mig_state {
     uint32_t    device_state;
     uint32_t    data_fd;
@@ -271,16 +275,16 @@ struct vfio_user_device_feature_mig_state {
 _Static_assert(sizeof(struct vfio_user_device_feature_migration) == 8,
                "bad vfio_user_device_feature_mig_state size");
 
-/* Analogous to vfio_device_mig_state */
+/* Analogous to enum vfio_device_mig_state */
 enum vfio_user_device_mig_state {
-	VFIO_USER_DEVICE_STATE_ERROR = 0,
-	VFIO_USER_DEVICE_STATE_STOP = 1,
-	VFIO_USER_DEVICE_STATE_RUNNING = 2,
-	VFIO_USER_DEVICE_STATE_STOP_COPY = 3,
-	VFIO_USER_DEVICE_STATE_RESUMING = 4,
-	VFIO_USER_DEVICE_STATE_RUNNING_P2P = 5,
-	VFIO_USER_DEVICE_STATE_PRE_COPY = 6,
-	VFIO_USER_DEVICE_STATE_PRE_COPY_P2P = 7,
+    VFIO_USER_DEVICE_STATE_ERROR = 0,
+    VFIO_USER_DEVICE_STATE_STOP = 1,
+    VFIO_USER_DEVICE_STATE_RUNNING = 2,
+    VFIO_USER_DEVICE_STATE_STOP_COPY = 3,
+    VFIO_USER_DEVICE_STATE_RESUMING = 4,
+    VFIO_USER_DEVICE_STATE_RUNNING_P2P = 5,
+    VFIO_USER_DEVICE_STATE_PRE_COPY = 6,
+    VFIO_USER_DEVICE_STATE_PRE_COPY_P2P = 7,
     VFIO_USER_DEVICE_NUM_STATES = 8,
 };
 
diff --git a/lib/dma.c b/lib/dma.c
index e5a2498..05aca6f 100644
--- a/lib/dma.c
+++ b/lib/dma.c
@@ -572,7 +572,11 @@ dma_controller_dirty_page_get(dma_controller_t *dma, vfu_dma_addr_t addr,
      * If dirty page logging is not enabled, the requested page size is zero,
      * or the requested page size is not a power of two, return an error.
      */
-    if (dma->dirty_pgsize == 0 || pgsize == 0 || (pgsize & (pgsize - 1)) != 0) {
+    if (dma->dirty_pgsize == 0) {
+        vfu_log(dma->vfu_ctx, LOG_ERR, "dirty page logging not enabled");
+        return ERROR_INT(EINVAL);
+    }
+    if (pgsize == 0 || (pgsize & (pgsize - 1)) != 0) {
         vfu_log(dma->vfu_ctx, LOG_ERR, "bad page size %zu", pgsize);
         return ERROR_INT(EINVAL);
     }
@@ -607,11 +611,11 @@ dma_controller_dirty_page_get(dma_controller_t *dma, vfu_dma_addr_t addr,
     }
 
     if (pgsize == dma->dirty_pgsize) {
-        dirty_page_get_simple(region, bitmap, bitmap_size);
+        dirty_page_get_equal_pgsize(region, bitmap, bitmap_size);
     } else {
-        dirty_page_get_complex(region, bitmap, bitmap_size,
-                               converted_bitmap_size, pgsize,
-                               dma->dirty_pgsize);
+        dirty_page_get_change_pgsize(region, bitmap, bitmap_size,
+                                     converted_bitmap_size, pgsize,
+                                     dma->dirty_pgsize);
     }
 
 #ifdef DEBUG
@@ -622,7 +626,7 @@ dma_controller_dirty_page_get(dma_controller_t *dma, vfu_dma_addr_t addr,
 }
 
 void
-dirty_page_get_simple(dma_memory_region_t *region, char *bitmap,
+dirty_page_get_equal_pgsize(dma_memory_region_t *region, char *bitmap,
                       size_t bitmap_size)
 {
     for (size_t i = 0; i < bitmap_size; i++) {
@@ -650,9 +654,9 @@ dirty_page_get_simple(dma_memory_region_t *region, char *bitmap,
 }
 
 void
-dirty_page_get_complex(dma_memory_region_t *region, char *bitmap,
-                      size_t bitmap_size, size_t converted_bitmap_size,
-                      size_t pgsize, size_t converted_pgsize)
+dirty_page_get_change_pgsize(dma_memory_region_t *region, char *bitmap,
+                             size_t bitmap_size, size_t converted_bitmap_size,
+                             size_t pgsize, size_t converted_pgsize)
 {
     uint8_t bit = 0;
     size_t i;
@@ -663,7 +667,7 @@ dirty_page_get_complex(dma_memory_region_t *region, char *bitmap,
         converted_pgsize / pgsize : pgsize / converted_pgsize;
 
     for (i = 0; i < bitmap_size &&
-         bit / 8 < (size_t)converted_bitmap_size; i++) {
+         bit / CHAR_BIT < (size_t)converted_bitmap_size; i++) {
         uint8_t val = region->dirty_bitmap[i];
         uint8_t out = 0;
 
@@ -685,7 +689,11 @@ dirty_page_get_complex(dma_memory_region_t *region, char *bitmap,
                                 &out, __ATOMIC_SEQ_CST);
         }
 
-        for (j = 0; j < 8; j++) {
+        /*
+         * Iterate through the bits of the byte, repeating or combining bits
+         * to reach the desired page size.
+         */
+        for (j = 0; j < CHAR_BIT; j++) {
             uint8_t b = (out >> j) & 1;
 
             if (extend) {
diff --git a/lib/libvfio-user.c b/lib/libvfio-user.c
index 6ab6546..e54e95b 100644
--- a/lib/libvfio-user.c
+++ b/lib/libvfio-user.c
@@ -894,7 +894,7 @@ static int
 device_reset(vfu_ctx_t *vfu_ctx, vfu_reset_type_t reason)
 {
     int ret;
-
+    
     ret = call_reset_cb(vfu_ctx, reason);
     if (ret < 0) {
         return ret;