fix: changes to samples according to comments

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

samples/client.c

@@ -216,7 +216,7 @@ send_device_reset(int sock)
 }
 
 /* returns whether a VFIO migration capability is found */
-static bool
+static void
 get_region_vfio_caps(struct vfio_info_cap_header *header,
  struct vfio_region_info_cap_sparse_mmap **sparse)
 {
@@ -243,7 +243,6 @@ get_region_vfio_caps(struct vfio_info_cap_header *header,
  }
  header = (struct vfio_info_cap_header*)((char*)header + header->next - sizeof(struct vfio_region_info));
  }
- return false;
 }
 
 static void
@@ -258,40 +257,6 @@ do_get_device_region_info(int sock, struct vfio_region_info *region_info,
  }
 }
 
-static void
-mmap_sparse_areas(int *fds, struct vfio_region_info *region_info,
- struct vfio_region_info_cap_sparse_mmap *sparse)
-{
- size_t i;
-
- for (i = 0; i < sparse->nr_areas; i++) {
-
- ssize_t ret;
- void *addr;
- char pathname[PATH_MAX];
- char buf[PATH_MAX] = "";
-
- ret = snprintf(pathname, sizeof(pathname), "/proc/self/fd/%d", fds[i]);
- assert(ret != -1 && (size_t)ret < sizeof(pathname));
- ret = readlink(pathname, buf, sizeof(buf) - 1);
- if (ret == -1) {
- err(EXIT_FAILURE, "failed to resolve file descriptor %d", fds[i]);
- }
- addr = mmap(NULL, sparse->areas[i].size, PROT_READ | PROT_WRITE,
- MAP_SHARED, fds[i], region_info->offset +
- sparse->areas[i].offset);
- if (addr == MAP_FAILED) {
- err(EXIT_FAILURE,
- "failed to mmap sparse region %zu in %s (%#llx-%#llx)",
- i, buf, (ull_t)sparse->areas[i].offset,
- (ull_t)sparse->areas[i].offset + sparse->areas[i].size - 1);
- }
-
- ret = munmap(addr, sparse->areas[i].size);
- assert(ret == 0);
- }
-}
-
 static void
 get_device_region_info(int sock, uint32_t index)
 {
@@ -335,14 +300,8 @@ get_device_region_info(int sock, uint32_t index)
  nr_fds);
  if (cap_sz) {
  struct vfio_region_info_cap_sparse_mmap *sparse = NULL;
- if (get_region_vfio_caps((struct vfio_info_cap_header*)(region_info + 1),
- &sparse)) {
- if (sparse != NULL) {
- assert((index == VFU_PCI_DEV_BAR1_REGION_IDX && nr_fds == 2));
- assert(nr_fds == sparse->nr_areas);
- mmap_sparse_areas(fds, region_info, sparse);
- }
- }
+ get_region_vfio_caps((struct vfio_info_cap_header*)(region_info + 1),
+ &sparse);
 
  }
  free(region_info);
@@ -520,12 +479,13 @@ set_migration_state(int sock, uint32_t state)
 {
  static int msg_id = 0xfab1;
  struct vfio_user_device_feature req = {
- .argsz = 16,
+ .argsz = sizeof(struct vfio_user_device_feature)
+ + sizeof(struct vfio_user_device_feature_mig_state),
  .flags = VFIO_DEVICE_FEATURE_SET | VFIO_DEVICE_FEATURE_MIG_DEVICE_STATE
  };
  struct vfio_user_device_feature_mig_state change_state = {
  .device_state = state,
- .data_fd = 0
+ .data_fd = -1
  };
  struct iovec send_iovecs[3] = {
  [1] = {
@@ -538,6 +498,10 @@ set_migration_state(int sock, uint32_t state)
  }
  };
  void* response = malloc(sizeof(req) + sizeof(change_state));
+
+ if (response == NULL) {
+ return -1;
+ }
 
  pthread_mutex_lock(&mutex);
  int ret = tran_sock_msg_iovec(sock, msg_id--, VFIO_USER_DEVICE_FEATURE,
@@ -547,12 +511,17 @@ set_migration_state(int sock, uint32_t state)
  pthread_mutex_unlock(&mutex);
 
  if (ret < 0) {
- return -1;
+ return ret;
  }
 
- assert(memcmp(&req, response, sizeof(req)) == 0);
- assert(memcmp(&change_state, response + sizeof(req),
- sizeof(change_state)) == 0);
+ if (memcmp(&req, response, sizeof(req)) != 0) {
+ err(EXIT_FAILURE, "invalid response to set_migration_state (header)");
+ }
+
+ if (memcmp(&change_state, response + sizeof(req),
+ sizeof(change_state)) != 0) {
+ err(EXIT_FAILURE, "invalid response to set_migration_state (payload)");
+ }
 
  return ret;
 }
@@ -562,7 +531,7 @@ read_migr_data(int sock, void *buf, size_t len)
 {
  static int msg_id = 0x6904;
  struct vfio_user_mig_data req = {
- .argsz = 12,
+ .argsz = sizeof(struct vfio_user_mig_data),
  .size = len
  };
  struct iovec send_iovecs[2] = {
@@ -573,6 +542,10 @@ read_migr_data(int sock, void *buf, size_t len)
  };
  struct vfio_user_mig_data *res = calloc(1, sizeof(req) + len);
 
+ if (res == NULL) {
+ return -1;
+ }
+
  pthread_mutex_lock(&mutex);
  int ret = tran_sock_msg_iovec(sock, msg_id--, VFIO_USER_MIG_DATA_READ,
  send_iovecs, 2, NULL, 0, NULL,
@@ -581,7 +554,7 @@ read_migr_data(int sock, void *buf, size_t len)
 
  if (ret < 0) {
  free(res);
- return -1;
+ return ret;
  }
 
  memcpy(buf, res->data, res->size);
@@ -596,7 +569,7 @@ write_migr_data(int sock, void *buf, size_t len)
 {
  static int msg_id = 0x2023;
  struct vfio_user_mig_data req = {
- .argsz = 12 + len,
+ .argsz = sizeof(struct vfio_user_mig_data) + len,
  .size = len
  };
  struct iovec send_iovecs[3] = {
@@ -616,10 +589,6 @@ write_migr_data(int sock, void *buf, size_t len)
  &req, sizeof(req), NULL, 0);
  pthread_mutex_unlock(&mutex);
 
- if (ret < 0) {
- return -1;
- }
-
  return ret;
 }
 
@@ -832,15 +801,29 @@ usage(char *argv0)
  basename(argv0));
 }
 
+/*
+ * Normally each time the source client (QEMU) would read migration data from
+ * the device it would send them to the destination client. However, since in
+ * our sample both the source and the destination client are the same process,
+ * we simply accumulate the migration data of each iteration and apply it to
+ * the destination server at the end.
+ *
+ * Performs as many migration loops as @nr_iters or until the device has no
+ * more migration data (pending_bytes is zero), which ever comes first. The
+ * result of each migration iteration is stored in @migr_iter. @migr_iter must
+ * be at least @nr_iters.
+ *
+ * @returns the number of iterations performed
+ */
 static size_t
-do_migrate(int sock, size_t max_iters, size_t max_iter_size,
+do_migrate(int sock, size_t nr_iters, size_t max_iter_size,
  struct iovec *migr_iter)
 {
  int ret;
- size_t i;
  bool is_more = true;
+ size_t i = 0;
 
- for (i = 0; i < max_iters && is_more; i++) {
+ for (i = 0; i < nr_iters && is_more; i++) {
 
  migr_iter[i].iov_len = max_iter_size;
  migr_iter[i].iov_base = malloc(migr_iter[i].iov_len);
@@ -855,12 +838,9 @@ do_migrate(int sock, size_t max_iters, size_t max_iter_size,
  err(EXIT_FAILURE, "failed to read migration data");
  }
 
- if (ret < (int)migr_iter[i].iov_len) {
- // FIXME is it pointless shuffling stuff around?
- void* buf = malloc(ret);
- memcpy(buf, migr_iter[i].iov_base, ret);
- free(migr_iter[i].iov_base);
- migr_iter[i].iov_base = buf;
+ // We know we've finished transferring data when less is returned
+ // than is requested.
+ if ((size_t)ret < migr_iter[i].iov_len) {
  migr_iter[i].iov_len = ret;
  is_more = false;
  }
@@ -928,7 +908,12 @@ migrate_from(int sock, size_t *nr_iters, struct iovec **migr_iters,
  err(EXIT_FAILURE, "failed to create pthread");
  }
 
- *nr_iters = 16;
+ size_t expected_data = bar1_size + sizeof(time_t);
+
+ *nr_iters = (expected_data + max_iter_size - 1) / max_iter_size;
+
+ printf("client: calculated %ld iters needed to migrate\n", *nr_iters);
+
  *migr_iters = malloc(sizeof(struct iovec) * *nr_iters);
  if (*migr_iters == NULL) {
  err(EXIT_FAILURE, NULL);
@@ -1008,7 +993,7 @@ migrate_to(char *old_sock_path, int *server_max_fds,
  if (ret == -1) {
  err(EXIT_FAILURE, "failed to fork");
  }
- if (ret == 0) { /* child (destination server) */
+ if (ret > 0) { /* child (destination server) */
  char *_argv[] = {
  path_to_server,
  (char *)"-r", // start in VFIO_DEVICE_STATE_RESUMING
@@ -1072,7 +1057,7 @@ migrate_to(char *old_sock_path, int *server_max_fds,
  fprintf(stderr, "client: CRC mismatch: %u != %u\n", src_crc, dst_crc);
  abort();
  } else {
- fprintf(stdout, "client: CRC match, we did it! :)\n");
+ printf("client: CRC match, we did it! :)\n");
  }
 
  /* XXX set device state to running */

samples/gpio-pci-idio-16.c

@@ -86,9 +86,9 @@ migration_read_data(UNUSED vfu_ctx_t *vfu_ctx, void *buf, uint64_t size)
  memcpy(buf, &pin, sizeof(pin));
  dirty = false;
  return sizeof(pin);
- } else {
- return 0;
  }
+
+ return 0;
 }
 
 static ssize_t

samples/server.c

@@ -62,8 +62,7 @@ struct server_data {
  size_t bar1_size;
  struct dma_regions regions[NR_DMA_REGIONS];
  struct {
- uint64_t pending_read;
- uint64_t pending_write;
+ uint64_t pending_bytes;
  vfu_migr_state_t state;
  } migration;
 };
@@ -135,7 +134,7 @@ bar1_access(vfu_ctx_t *vfu_ctx, char * const buf,
  if (is_write) {
  if (server_data->migration.state == VFU_MIGR_STATE_PRE_COPY) {
  /* dirty the whole thing */
- server_data->migration.pending_read = server_data->bar1_size;
+ server_data->migration.pending_bytes = server_data->bar1_size;
  }
  memcpy(server_data->bar1 + offset, buf, count);
  } else {
@@ -261,20 +260,20 @@ migration_device_state_transition(vfu_ctx_t *vfu_ctx, vfu_migr_state_t state)
  if (setitimer(ITIMER_REAL, &new, NULL) != 0) {
  err(EXIT_FAILURE, "failed to disable timer");
  }
- server_data->migration.pending_read = server_data->bar1_size + sizeof(time_t); /* FIXME BAR0 region size */
+ server_data->migration.pending_bytes = server_data->bar1_size + sizeof(time_t); /* FIXME BAR0 region size */
  break;
  case VFU_MIGR_STATE_PRE_COPY:
- server_data->migration.pending_read = server_data->bar1_size;
+ server_data->migration.pending_bytes = server_data->bar1_size;
  break;
  case VFU_MIGR_STATE_STOP:
  /* FIXME should gracefully fail */
- assert(server_data->migration.pending_read == 0);
+ assert(server_data->migration.pending_bytes == 0);
  break;
  case VFU_MIGR_STATE_RESUME:
- server_data->migration.pending_write = server_data->bar1_size + sizeof(time_t);
+ server_data->migration.pending_bytes = server_data->bar1_size + sizeof(time_t);
  break;
  case VFU_MIGR_STATE_RUNNING:
- assert(server_data->migration.pending_write == 0);
+ assert(server_data->migration.pending_bytes == 0);
  ret = arm_timer(vfu_ctx, server_data->bar0);
  if (ret < 0) {
  return ret;
@@ -299,7 +298,8 @@ migration_read_data(vfu_ctx_t *vfu_ctx, void *buf, uint64_t size)
  * more complex state tracking which exceeds the scope of this sample.
  */
 
- if (server_data->migration.pending_read == 0 || size == 0) {
+ if (server_data->migration.pending_bytes == 0 || size == 0) {
+ vfu_log(vfu_ctx, LOG_DEBUG, "no data left to read");
  return 0;
  }
 
@@ -309,7 +309,7 @@ migration_read_data(vfu_ctx_t *vfu_ctx, void *buf, uint64_t size)
  total_read += sizeof(server_data->bar0);
  }
 
- uint32_t read_start = total_read - server_data->migration.pending_read;
+ uint32_t read_start = total_read - server_data->migration.pending_bytes;
  uint32_t read_end = MIN(read_start + size, total_read); // exclusive
  assert(read_end > read_start);
 
@@ -343,7 +343,7 @@ migration_read_data(vfu_ctx_t *vfu_ctx, void *buf, uint64_t size)
  memcpy(buf, &server_data->bar0 + read_start, bytes_read);
  }
 
- server_data->migration.pending_read -= bytes_read;
+ server_data->migration.pending_bytes -= bytes_read;
 
  return bytes_read;
 }
@@ -357,13 +357,13 @@ migration_write_data(vfu_ctx_t *vfu_ctx, void *data, uint64_t size)
  assert(server_data != NULL);
  assert(data != NULL);
 
- if (server_data->migration.pending_write == 0 || size == 0) {
+ if (server_data->migration.pending_bytes == 0 || size == 0) {
  return 0;
  }
 
  uint32_t total_write = server_data->bar1_size + sizeof(server_data->bar0);
 
- uint32_t write_start = total_write - server_data->migration.pending_write;
+ uint32_t write_start = total_write - server_data->migration.pending_bytes;
  uint32_t write_end = MIN(write_start + size, total_write); // exclusive
  assert(write_end > write_start);
 
@@ -397,7 +397,7 @@ migration_write_data(vfu_ctx_t *vfu_ctx, void *data, uint64_t size)
  memcpy(&server_data->bar0 + write_start, buf, bytes_written);
  }
 
- server_data->migration.pending_write -= bytes_written;
+ server_data->migration.pending_bytes -= bytes_written;
 
  return bytes_written;
 }
@@ -434,7 +434,7 @@ int main(int argc, char *argv[])
  case 'r':
  destination = true;
  server_data.migration.state = VFU_MIGR_STATE_RESUME;
- server_data.migration.pending_write =
+ server_data.migration.pending_bytes =
  bar1_size + sizeof(time_t);
  break;
  default: /* '?' */