On Thu, Mar 19, 2026 at 05:56:52PM -0300, Fabiano Rosas wrote: > Peter Xu <[email protected]> writes: > > > The loader side of ptr marker is pretty straightforward, instead of playing > > the inner_field trick, just do the load manually assuming the marker layout > > is a stable ABI (which it is true already). > > > > This will remove some logic while loading VMSD, and hopefully it makes it > > slightly easier to read. Unfortunately, we still need to keep the sender > > side because of the JSON blob we're maintaining.. > > > > /ramble > Is the JSON blob ABI? Can we kill it?
Yeah, that's a fair ramble. I had sometimes the same feeling that I wished it not be there, it'll save quite some work.. Per my limited understanding of reading the code in the past, the plan 10 years ago was having those blob to always be together with the binary stream so that the stream (especially when causing loading failures..) will be parsable and it's easier to know why the load fail. However I also confess in the past few years since I worked on migration, I never used it to debug any real VMSD issues.. I think one reason might be that when migration was very new and when device emulation developers were easier to mess things up, crash happen more, and at that time this tool helps debugging things. It'll also almost make the VM dump self-explanatory. Nowadays, we added more codes into migration to help, e.g., I recall we added things like QEMU_VM_SECTION_FOOTER and likely more after the vmdesc, which can also help to identify VMSD issues directly from dest QEMU errors. Let me copy some more people to see if there's any thoughts or inputs on the JSON blobs.. > AFAIR, it only serves to add > complexity and to break analyze-script from time to time. We'd be better > off parsing the actual stream from a file. Could maybe even use the same > loadvm code, but mock the .get functions to print instead of actually > loading. The problem might be that direct parsing of the binary stream is almost impossible when without the json blob, due to the fact we have a very condensed VMSD fields in the stream (we have zero headers for fields..), so they're very efficient but compat, I think we won't be able to parse an image if we don't know which version of QEMU it was generated from otherwise. But again, I'm not sure we strictly need this in each VM image we send, especially right now on dest QEMU we explicitly allocates that buffer, load this vmdesc dump and then drop it.. There's also that postcopy won't dump this (pretty much because we know it's an active QEMU instance so JSON blob may not really help..), so it's just a bit weird to not be able to see how it helps besides debugging a "migrate to file" case with unknown QEMU versions, for example. When I know the version of QEMU on src side when something wrong happened (normally we do..), it may not be very needed. > > Having separate code that parses a "thing" that's not the stream, but > that should reflect the stream, but it's not the stream, but pretty > close it's a bit weird to me. I recently had to simply open the raw > stream on emacs and navigate through it because the file: stream was > somehow different from the stream on the qcow2, for the same command > line. Do you mean snapshot-save v.s. live migrate to file URIs when all caps off? > > (that's another point, parsing from the qcow2 would be cool, which the > JSON blob doesn't provide today I think) > ramble/ > > > This paves way for future processing of non-NULL markers as well. > > > > Signed-off-by: Peter Xu <[email protected]> > > --- > > migration/vmstate-types.c | 12 ++++-------- > > migration/vmstate.c | 40 ++++++++++++++++++++++++--------------- > > 2 files changed, 29 insertions(+), 23 deletions(-) > > > > diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c > > index b31689fc3c..ae465c5c2c 100644 > > --- a/migration/vmstate-types.c > > +++ b/migration/vmstate-types.c > > @@ -363,14 +363,10 @@ static bool load_ptr_marker(QEMUFile *f, void *pv, > > size_t size, > > const VMStateField *field, Error **errp) > > > > { > > - int byte = qemu_get_byte(f); > > - > > - if (byte == VMS_MARKER_PTR_NULL || byte == VMS_MARKER_PTR_VALID) { > > - /* TODO: process PTR_VALID case */ > > - return true; > > - } > > - > > - error_setg(errp, "%s: unexpected ptr marker: %d", __func__, byte); > > + /* > > + * Load is done in vmstate core, see vmstate_ptr_marker_load(). > > + */ > > + g_assert_not_reached(); > > return false; > > } > > > > diff --git a/migration/vmstate.c b/migration/vmstate.c > > index a3a5f25946..d65fc84dfa 100644 > > --- a/migration/vmstate.c > > +++ b/migration/vmstate.c > > @@ -142,6 +142,21 @@ static void vmstate_handle_alloc(void *ptr, const > > VMStateField *field, > > } > > } > > > > +static bool vmstate_ptr_marker_load(QEMUFile *f, bool *load_field, > > + Error **errp) > > +{ > > + int byte = qemu_get_byte(f); > > + > > + if (byte == VMS_MARKER_PTR_NULL) { > > + /* When it's a null ptr marker, do not continue the load */ > > + *load_field = false; > > + return true; > > + } > > + > > + error_setg(errp, "Unexpected ptr marker: %d", byte); > > + return false; > > +} > > + > > static bool vmstate_pre_load(const VMStateDescription *vmsd, void *opaque, > > Error **errp) > > { > > @@ -264,30 +279,25 @@ bool vmstate_load_vmsd(QEMUFile *f, const > > VMStateDescription *vmsd, > > } > > > > for (i = 0; i < n_elems; i++) { > > - bool ok; > > + /* If we will process the load of field? */ > > + bool load_field = true; > > maybe valid_ptr would be more clear? I don't think we can? Note, that we will only update this field iff the field used the new VMS_ARRAY_OF_POINTER_AUTO_ALLOC, otherwise it simply should be true always and it says "let's load this VMSD". IOW, in most cases there's no ptr, hence "valid_ptr=true" will be weird on its own I think.. > > > + bool ok = true; > > void *curr_elem = first_elem + size * i; > > - const VMStateField *inner_field; > > > > if (field->flags & VMS_ARRAY_OF_POINTER) { > > curr_elem = *(void **)curr_elem; > > } > > > > if (!curr_elem && size) { > > - /* > > - * If null pointer found (which should only happen in > > - * an array of pointers), use null placeholder and do > > - * not follow. > > - */ > > - inner_field = vmsd_create_ptr_marker_field(field); > > - } else { > > - inner_field = field; > > + /* Read the marker instead of VMSD itself */ > > + if (!vmstate_ptr_marker_load(f, &load_field, errp)) { > > + trace_vmstate_load_field_error(field->name, > > -EINVAL); > > + return false; > > + } > > } > > > > - ok = vmstate_load_field(f, curr_elem, size, inner_field, > > errp); > > - > > - /* If we used a fake temp field.. free it now */ > > - if (inner_field != field) { > > - g_clear_pointer((gpointer *)&inner_field, g_free); > > + if (load_field) { > > + ok = vmstate_load_field(f, curr_elem, size, field, > > errp); > > } > > > > if (ok) { > -- Peter Xu
