Peter Xu <[email protected]> writes:
> We used to have one vmstate called "nullptr" which is only used to generate
> one-byte hint to say one pointer is NULL.
>
> Let's extend its use so that it will generate another byte to say the
> pointer is non-NULL.
>
> With that, the name of the info struct (or functions) do not apply anymore.
> Update correspondingly.
>
> No functional change intended yet.
>
> Signed-off-by: Peter Xu <[email protected]>
> ---
> include/migration/vmstate.h | 9 +++++++--
> migration/vmstate-types.c | 34 ++++++++++++++++------------------
> migration/vmstate.c | 29 ++++++++++++++---------------
> 3 files changed, 37 insertions(+), 35 deletions(-)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 092e8f7e9a..2e51b5ea04 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -282,9 +282,14 @@ extern const VMStateInfo vmstate_info_uint32;
> extern const VMStateInfo vmstate_info_uint64;
> extern const VMStateInfo vmstate_info_fd;
>
> -/** Put this in the stream when migrating a null pointer.*/
> +/*
> + * Put this in the stream when migrating a pointer to reflect either a NULL
> + * or valid pointer.
> + */
> #define VMS_MARKER_PTR_NULL (0x30U) /* '0' */
> -extern const VMStateInfo vmstate_info_nullptr;
> +#define VMS_MARKER_PTR_VALID (0x31U) /* '1' */
> +
> +extern const VMStateInfo vmstate_info_ptr_marker;
>
> extern const VMStateInfo vmstate_info_cpudouble;
>
> diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
> index 7622cf8f01..b31689fc3c 100644
> --- a/migration/vmstate-types.c
> +++ b/migration/vmstate-types.c
> @@ -359,36 +359,34 @@ const VMStateInfo vmstate_info_fd = {
> .save = save_fd,
> };
>
> -static bool load_nullptr(QEMUFile *f, void *pv, size_t size,
> - const VMStateField *field, Error **errp)
> +static bool load_ptr_marker(QEMUFile *f, void *pv, size_t size,
> + const VMStateField *field, Error **errp)
>
> {
> - if (qemu_get_byte(f) == VMS_MARKER_PTR_NULL) {
> + 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, "vmstate: load_nullptr expected VMS_NULLPTR_MARKER");
> + error_setg(errp, "%s: unexpected ptr marker: %d", __func__, byte);
> return false;
> }
>
> -static bool save_nullptr(QEMUFile *f, void *pv, size_t size,
> - const VMStateField *field, JSONWriter *vmdesc,
> - Error **errp)
> +static bool save_ptr_marker(QEMUFile *f, void *pv, size_t size,
> + const VMStateField *field, JSONWriter *vmdesc,
> + Error **errp)
>
> {
> - if (pv == NULL) {
> - qemu_put_byte(f, VMS_MARKER_PTR_NULL);
> - return true;
> - }
> -
> - error_setg(errp, "vmstate: save_nullptr must be called with pv == NULL");
> - return false;
> + qemu_put_byte(f, pv ? VMS_MARKER_PTR_VALID : VMS_MARKER_PTR_NULL);
> + return true;
> }
>
> -const VMStateInfo vmstate_info_nullptr = {
> - .name = "nullptr",
> - .load = load_nullptr,
> - .save = save_nullptr,
> +const VMStateInfo vmstate_info_ptr_marker = {
> + .name = "ptr-marker",
> + .load = load_ptr_marker,
> + .save = save_ptr_marker,
> };
>
> /* 64 bit unsigned int. See that the received value is the same than the one
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 5a6b352764..a3a5f25946 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -55,12 +55,12 @@ vmstate_field_exists(const VMStateDescription *vmsd,
> const VMStateField *field,
> }
>
> /*
> - * Create a fake nullptr field when there's a NULL pointer detected in the
> + * Create a ptr marker field when there's a NULL pointer detected in the
> * array of a VMS_ARRAY_OF_POINTER VMSD field. It's needed because we
> * can't dereference the NULL pointer.
> */
> static const VMStateField *
> -vmsd_create_fake_nullptr_field(const VMStateField *field)
> +vmsd_create_ptr_marker_field(const VMStateField *field)
> {
> VMStateField *fake = g_new0(VMStateField, 1);
>
> @@ -76,7 +76,7 @@ vmsd_create_fake_nullptr_field(const VMStateField *field)
>
> /* See vmstate_info_nullptr - use 1 byte to represent nullptr */
This comment needs updating.
> fake->size = 1;
> - fake->info = &vmstate_info_nullptr;
> + fake->info = &vmstate_info_ptr_marker;
> fake->flags = VMS_SINGLE;
>
> /* All the rest fields shouldn't matter.. */
> @@ -278,7 +278,7 @@ bool vmstate_load_vmsd(QEMUFile *f, const
> VMStateDescription *vmsd,
> * an array of pointers), use null placeholder and do
> * not follow.
> */
> - inner_field = vmsd_create_fake_nullptr_field(field);
> + inner_field = vmsd_create_ptr_marker_field(field);
> } else {
> inner_field = field;
> }
> @@ -567,7 +567,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const
> VMStateDescription *vmsd,
> int i, n_elems = vmstate_n_elems(opaque, field);
> int size = vmstate_size(opaque, field);
> JSONWriter *vmdesc_loop = vmdesc;
> - bool is_prev_null = false;
> + bool use_marker_field_prev = false;
The logic below won't handle valid pointer as well, will it? We could
leave the is_null/is_prev_null terminology because it's way more
descriptive. Right? We're adding the marker here because we're
compressing and know the field is null.
>
> trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
> if (field->flags & VMS_POINTER) {
> @@ -578,7 +578,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const
> VMStateDescription *vmsd,
> for (i = 0; i < n_elems; i++) {
> void *curr_elem = first_elem + size * i;
> const VMStateField *inner_field;
> - bool is_null;
> + bool use_marker_field;
> int max_elems = n_elems - i;
>
> if (field->flags & VMS_ARRAY_OF_POINTER) {
> @@ -586,17 +586,16 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const
> VMStateDescription *vmsd,
> curr_elem = *(void **)curr_elem;
> }
>
> - if (!curr_elem && size) {
> + use_marker_field = !curr_elem && size;
> + if (use_marker_field) {
> /*
> * If null pointer found (which should only happen in
> * an array of pointers), use null placeholder and do
> * not follow.
> */
> - inner_field = vmsd_create_fake_nullptr_field(field);
> - is_null = true;
> + inner_field = vmsd_create_ptr_marker_field(field);
> } else {
> inner_field = field;
> - is_null = false;
> }
>
> /*
> @@ -612,16 +611,16 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const
> VMStateDescription *vmsd,
> */
> if (vmdesc && vmsd_can_compress(field) &&
> (field->flags & VMS_ARRAY_OF_POINTER) &&
> - is_null != is_prev_null) {
> + use_marker_field != use_marker_field_prev) {
>
> - is_prev_null = is_null;
> + use_marker_field_prev = use_marker_field;
> vmdesc_loop = vmdesc;
>
> for (int j = i + 1; j < n_elems; j++) {
> void *elem = *(void **)(first_elem + size * j);
> - bool elem_is_null = !elem && size;
> + bool elem_use_marker_field = !elem && size;
See?
>
> - if (is_null != elem_is_null) {
> + if (use_marker_field != elem_use_marker_field) {
> max_elems = j - i;
> break;
> }
> @@ -633,7 +632,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const
> VMStateDescription *vmsd,
> i, max_elems, errp);
>
> /* If we used a fake temp field.. free it now */
> - if (is_null) {
> + if (use_marker_field) {
> g_clear_pointer((gpointer *)&inner_field, g_free);
> }
