Re: [PATCH V1 03/26] migration: SAVEVM_FOREACH

Mon, 13 May 2024 12:28:50 -0700 

On 5/6/2024 7:17 PM, Fabiano Rosas wrote:

Steve Sistare <steven.sist...@oracle.com> writes:


Define an abstraction SAVEVM_FOREACH to loop over all savevm state
handlers, and replace QTAILQ_FOREACH.  Define variants for ALL so
we can loop over all handlers vs a subset of handlers in a subsequent
patch, but at this time there is no distinction between the two.
No functional change.

Signed-off-by: Steve Sistare <steven.sist...@oracle.com>
---
  migration/savevm.c | 55 +++++++++++++++++++++++++++++++-----------------------
  1 file changed, 32 insertions(+), 23 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 4509482..6829ba3 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -237,6 +237,15 @@ static SaveState savevm_state = {
      .global_section_id = 0,
  };
+#define SAVEVM_FOREACH(se, entry) \ 
+    QTAILQ_FOREACH(se, &savevm_state.handlers, entry)                \
+
+#define SAVEVM_FOREACH_ALL(se, entry)                                \
+    QTAILQ_FOREACH(se, &savevm_state.handlers, entry)


This feels worse than SAVEVM_FOREACH_NOT_PRECREATED. We'll have to keep
coming back to the definition to figure out which FOREACH is the real
deal.


I take your point, but the majority of the loops do not care about precreated
objects, so it seems backwards to make them more verbose with SAVEVM_FOREACH_NOT_PRECREATE. I can go either way, but we need 
Peter's opinion also.


+
+#define SAVEVM_FOREACH_SAFE_ALL(se, entry, new_se)                   \
+    QTAILQ_FOREACH_SAFE(se, &savevm_state.handlers, entry, new_se)
+
  static SaveStateEntry *find_se(const char *idstr, uint32_t instance_id);
static bool should_validate_capability(int capability) 
@@ -674,7 +683,7 @@ static uint32_t calculate_new_instance_id(const char *idstr)
      SaveStateEntry *se;
      uint32_t instance_id = 0;
- QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { 
+    SAVEVM_FOREACH_ALL(se, entry) {


In this patch we can't have both instances...


          if (strcmp(idstr, se->idstr) == 0
              && instance_id <= se->instance_id) {
              instance_id = se->instance_id + 1;
@@ -690,7 +699,7 @@ static int calculate_compat_instance_id(const char *idstr)
      SaveStateEntry *se;
      int instance_id = 0;
- QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { 
+    SAVEVM_FOREACH(se, entry) {


...otherwise one of the two changes will go undocumented because the
actual reason for it will only be described in the next patch.


Sure, I'll move this to the precreate patch.

- Steve


          if (!se->compat) {
              continue;
          }
@@ -816,7 +825,7 @@ void unregister_savevm(VMStateIf *obj, const char *idstr, 
void *opaque)
      }
      pstrcat(id, sizeof(id), idstr);
- QTAILQ_FOREACH_SAFE(se, &savevm_state.handlers, entry, new_se) { 
+    SAVEVM_FOREACH_SAFE_ALL(se, entry, new_se) {
          if (strcmp(se->idstr, id) == 0 && se->opaque == opaque) {
              savevm_state_handler_remove(se);
              g_free(se->compat);
@@ -939,7 +948,7 @@ void vmstate_unregister(VMStateIf *obj, const 
VMStateDescription *vmsd,
  {
      SaveStateEntry *se, *new_se;
- QTAILQ_FOREACH_SAFE(se, &savevm_state.handlers, entry, new_se) { 
+    SAVEVM_FOREACH_SAFE_ALL(se, entry, new_se) {
          if (se->vmsd == vmsd && se->opaque == opaque) {
              savevm_state_handler_remove(se);
              g_free(se->compat);
@@ -1223,7 +1232,7 @@ bool qemu_savevm_state_blocked(Error **errp)
  {
      SaveStateEntry *se;
- QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { 
+    SAVEVM_FOREACH(se, entry) {
          if (se->vmsd && se->vmsd->unmigratable) {
              error_setg(errp, "State blocked by non-migratable device '%s'",
                         se->idstr);
@@ -1237,7 +1246,7 @@ void qemu_savevm_non_migratable_list(strList **reasons)
  {
      SaveStateEntry *se;
- QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { 
+    SAVEVM_FOREACH(se, entry) {
          if (se->vmsd && se->vmsd->unmigratable) {
              QAPI_LIST_PREPEND(*reasons,
                                g_strdup_printf("non-migratable device: %s",
@@ -1276,7 +1285,7 @@ bool qemu_savevm_state_guest_unplug_pending(void)
  {
      SaveStateEntry *se;
- QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { 
+    SAVEVM_FOREACH(se, entry) {
          if (se->vmsd && se->vmsd->dev_unplug_pending &&
              se->vmsd->dev_unplug_pending(se->opaque)) {
              return true;
@@ -1291,7 +1300,7 @@ int qemu_savevm_state_prepare(Error **errp)
      SaveStateEntry *se;
      int ret;
- QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { 
+    SAVEVM_FOREACH(se, entry) {
          if (!se->ops || !se->ops->save_prepare) {
              continue;
          }
@@ -1321,7 +1330,7 @@ int qemu_savevm_state_setup(QEMUFile *f, Error **errp)
      json_writer_start_array(ms->vmdesc, "devices");
trace_savevm_state_setup(); 
-    QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
+    SAVEVM_FOREACH(se, entry) {
          if (se->vmsd && se->vmsd->early_setup) {
              ret = vmstate_save(f, se, ms->vmdesc, errp);
              if (ret) {
@@ -1365,7 +1374,7 @@ int qemu_savevm_state_resume_prepare(MigrationState *s)
trace_savevm_state_resume_prepare(); - QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { 
+    SAVEVM_FOREACH(se, entry) {
          if (!se->ops || !se->ops->resume_prepare) {
              continue;
          }
@@ -1396,7 +1405,7 @@ int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy)
      int ret;
trace_savevm_state_iterate(); 
-    QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
+    SAVEVM_FOREACH(se, entry) {
          if (!se->ops || !se->ops->save_live_iterate) {
              continue;
          }
@@ -1461,7 +1470,7 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f)
      SaveStateEntry *se;
      int ret;
- QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { 
+    SAVEVM_FOREACH(se, entry) {
          if (!se->ops || !se->ops->save_live_complete_postcopy) {
              continue;
          }
@@ -1495,7 +1504,7 @@ int qemu_savevm_state_complete_precopy_iterable(QEMUFile 
*f, bool in_postcopy)
      SaveStateEntry *se;
      int ret;
- QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { 
+    SAVEVM_FOREACH(se, entry) {
          if (!se->ops ||
              (in_postcopy && se->ops->has_postcopy &&
               se->ops->has_postcopy(se->opaque)) ||
@@ -1543,7 +1552,7 @@ int 
qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
      Error *local_err = NULL;
      int ret;
- QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { 
+    SAVEVM_FOREACH(se, entry) {
          if (se->vmsd && se->vmsd->early_setup) {
              /* Already saved during qemu_savevm_state_setup(). */
              continue;
@@ -1649,7 +1658,7 @@ void qemu_savevm_state_pending_estimate(uint64_t 
*must_precopy,
      *must_precopy = 0;
      *can_postcopy = 0;
- QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { 
+    SAVEVM_FOREACH(se, entry) {
          if (!se->ops || !se->ops->state_pending_estimate) {
              continue;
          }
@@ -1670,7 +1679,7 @@ void qemu_savevm_state_pending_exact(uint64_t 
*must_precopy,
      *must_precopy = 0;
      *can_postcopy = 0;
- QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { 
+    SAVEVM_FOREACH(se, entry) {
          if (!se->ops || !se->ops->state_pending_exact) {
              continue;
          }
@@ -1693,7 +1702,7 @@ void qemu_savevm_state_cleanup(void)
      }
trace_savevm_state_cleanup(); 
-    QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
+    SAVEVM_FOREACH(se, entry) {
          if (se->ops && se->ops->save_cleanup) {
              se->ops->save_cleanup(se->opaque);
          }
@@ -1778,7 +1787,7 @@ int qemu_save_device_state(QEMUFile *f)
      }
      cpu_synchronize_all_states();
- QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { 
+    SAVEVM_FOREACH(se, entry) {
          int ret;
if (se->is_ram) { 
@@ -1801,7 +1810,7 @@ static SaveStateEntry *find_se(const char *idstr, 
uint32_t instance_id)
  {
      SaveStateEntry *se;
- QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { 
+    SAVEVM_FOREACH_ALL(se, entry) {
          if (!strcmp(se->idstr, idstr) &&
              (instance_id == se->instance_id ||
               instance_id == se->alias_id))
@@ -2680,7 +2689,7 @@ qemu_loadvm_section_part_end(QEMUFile *f, 
MigrationIncomingState *mis,
      }
trace_qemu_loadvm_state_section_partend(section_id); 
-    QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
+    SAVEVM_FOREACH(se, entry) {
          if (se->load_section_id == section_id) {
              break;
          }
@@ -2755,7 +2764,7 @@ static void 
qemu_loadvm_state_switchover_ack_needed(MigrationIncomingState *mis)
  {
      SaveStateEntry *se;
- QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { 
+    SAVEVM_FOREACH(se, entry) {
          if (!se->ops || !se->ops->switchover_ack_needed) {
              continue;
          }
@@ -2775,7 +2784,7 @@ static int qemu_loadvm_state_setup(QEMUFile *f, Error 
**errp)
      int ret;
trace_loadvm_state_setup(); 
-    QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
+    SAVEVM_FOREACH(se, entry) {
          if (!se->ops || !se->ops->load_setup) {
              continue;
          }
@@ -2801,7 +2810,7 @@ void qemu_loadvm_state_cleanup(void)
      SaveStateEntry *se;
trace_loadvm_state_cleanup(); 
-    QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
+    SAVEVM_FOREACH(se, entry) {
          if (se->ops && se->ops->load_cleanup) {
              se->ops->load_cleanup(se->opaque);
          }

Reply via email to