On 02/21/2017 01:22 PM, Dr. David Alan Gilbert wrote: > * Halil Pasic (pa...@linux.vnet.ibm.com) wrote: >> >> >> On 02/17/2017 08:42 PM, Dr. David Alan Gilbert wrote: >>> * Halil Pasic (pa...@linux.vnet.ibm.com) wrote: >>>> Make VMS_ARRAY_OF_POINTER cope with null pointers. Previously the >>>> reward for trying to migrate an array with some null pointers in it was >>>> an illegal memory access, that is a swift and painless death of the >>>> process. Let's make vmstate cope with this scenario. >>>> >>>> The general approach is, when we encounter a null pointer (element), >>>> instead of following the pointer to save/load the data behind it, we >>>> save/load a placeholder. This way we can detect if we expected a null >>>> pointer at the load side but not null data was saved instead. >>>> >>>> Signed-off-by: Halil Pasic <pa...@linux.vnet.ibm.com> >>>> Reviewed-by: Guenther Hutzl <hu...@linux.vnet.ibm.com> >>>> >>>> --- >>>> We will need this to load/save some on demand created state in the >>>> (s390x) channel subsystem (see ChannelSubSys.css in hw/s390x/css.c for >>>> an example). >>>> --- >>>> include/migration/vmstate.h | 4 ++++ >>>> migration/vmstate.c | 33 +++++++++++++++++++++++++++++++-- >>>> 2 files changed, 35 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h >>>> index 63e7b02..f2dbf84 100644 >>>> --- a/include/migration/vmstate.h >>>> +++ b/include/migration/vmstate.h >>>> @@ -253,6 +253,10 @@ extern const VMStateInfo vmstate_info_uint16; >>>> extern const VMStateInfo vmstate_info_uint32; >>>> extern const VMStateInfo vmstate_info_uint64; >>>> >>>> +/** Put this in the stream when migrating a null pointer.*/ >>>> +#define VMS_NULLPTR_MARKER (0x30U) /* '0' */ >>>> +extern const VMStateInfo vmstate_info_nullptr; >>>> + >>>> extern const VMStateInfo vmstate_info_float64; >>>> extern const VMStateInfo vmstate_info_cpudouble; >>>> >>>> diff --git a/migration/vmstate.c b/migration/vmstate.c >>>> index 836a7a4..cb81cef 100644 >>>> --- a/migration/vmstate.c >>>> +++ b/migration/vmstate.c >>>> @@ -117,7 +117,11 @@ int vmstate_load_state(QEMUFile *f, const >>>> VMStateDescription *vmsd, >>>> if (field->flags & VMS_ARRAY_OF_POINTER) { >>>> curr_elem = *(void **)curr_elem; >>>> } >>>> - if (field->flags & VMS_STRUCT) { >>>> + if (!curr_elem) { >>>> + /* if null pointer check placeholder and do not >>>> follow */ >>>> + assert(field->flags & VMS_ARRAY_OF_POINTER); >>> >>> That can return an error instead of asserting. >>> >>>> + vmstate_info_nullptr.get(f, curr_elem, size, NULL); >>> >>> You've ignored the return value of the get; that should be ret = >>> >>>> + } else if (field->flags & VMS_STRUCT) { >>>> ret = vmstate_load_state(f, field->vmsd, curr_elem, >>>> field->vmsd->version_id); >>>> } else { >>>> @@ -332,7 +336,11 @@ void vmstate_save_state(QEMUFile *f, const >>>> VMStateDescription *vmsd, >>>> assert(curr_elem); >>>> curr_elem = *(void **)curr_elem; >>>> } >>>> - if (field->flags & VMS_STRUCT) { >>>> + if (!curr_elem) { >>>> + /* if null pointer write placeholder and do not >>>> follow */ >>>> + assert(field->flags & VMS_ARRAY_OF_POINTER); >>>> + vmstate_info_nullptr.put(f, curr_elem, size, NULL, >>>> NULL); >>>> + } else if (field->flags & VMS_STRUCT) { >>>> vmstate_save_state(f, field->vmsd, curr_elem, >>>> vmdesc_loop); >>>> } else { >>>> field->info->put(f, curr_elem, size, field, >>>> vmdesc_loop); >>>> @@ -747,6 +755,27 @@ const VMStateInfo vmstate_info_uint64 = { >>>> .put = put_uint64, >>>> }; >>>> >>>> +static int get_nullptr(QEMUFile *f, void *pv, size_t size, VMStateField >>>> *field) >>>> + >>>> +{ >>>> + return qemu_get_byte(f) == VMS_NULLPTR_MARKER ? 0 : -EINVAL; >>>> +} >>>> + >>>> +static int put_nullptr(QEMUFile *f, void *pv, size_t size, >>>> + VMStateField *field, QJSON *vmdesc) >>>> + >>>> +{ >>>> + assert(pv == NULL); >>>> + qemu_put_byte(f, VMS_NULLPTR_MARKER); >>>> + return 0; >>>> +} >>> >>> Again that assert could just turn into a return -EINVAL >>> >>> Dave >>> >> >> @Dave: I have accidentally answered with 'reply' instead of 'reply all' >> yesterday. Sorry for the noise! >> >> Thanks for the review! You are right, my code is messy. > > It's not that bad - just a few minor issues. > >> Yet I'm not sure what you propose is the best way to clean it up. My >> concern is, that we are loosing some info about what exactly went wrong >> (and where), if returning -EINVAL is not combined with additional >> logging/error reporting. >> >> After re-checking the asserts they all indicate programming errors and >> there is not much (IMHO) client code can do to recover and the user >> should never observe. Unfortunately, I lack knowledge about how is the >> client code handling errors and if there is something we need to clean >> up. I assumed asserting is OK because of the per-existing asserts. > >> You are perfectly right about ignoring the return value of >> vmstate_info_nullptr.get and at least I will have to fix that. >> IMHO we have three options to fix this code: >> a) Make vmstate_info_nullptr.get assert too as we expect null pointer >> and we got not null is pretty much an indicator that we can't proceed >> sanely. Possibly add ret = ... for aesthetic reasons. Keep the asserts. >> b) Follow your suggestions without anything on top of it. >> c) Follow your suggestions and add error_report stating what exactly >> went wrong. >> >> My priority is to get this in, so I will do what you say, or send >> option b) late on Wednesday if no guidance until then. > > It's OK to add an error_report as needed; in particular I try and avoid > assert's on the save path where possible, since the user apparently > has a happy running VM, so even if the migration code has an error in it > I'd rather the user didn't lose their VM. The load path it's OK to > add the asserts since they've not got a working VM yet. > > Dave >
You are right, distinguishing between save and load paths makes a lot of sense. So on the load path I will use asserts, and on the save path, that is static int put_nullptr(QEMUFile *f, void *pv, size_t size, VMStateField *field, QJSON *vmdesc) { here, I could do: - assert(pv == NULL); + if (pv != NULL) { + error_report("put_nullptr must be called with pv == NULL"); + return -EINVAL; + } qemu_put_byte(f, VMS_NULLPTR_MARKER); return 0; } And of course, I will fix ignoring the return value too. Would that be satisfactory? If not, please complain. By the way, I could also omit the check on pv, and ignore pv altogether. A call to put_nullptr means we encountered a nullptr and the check is just for catching buggy client code -- what might be an overkill in this case. Thank you very much. FYI I intend to send out the next version tomorrow. Regards, Halil