[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/5] migration/vmstate: fix array of ptr with nu
From: |
Halil Pasic |
Subject: |
Re: [Qemu-devel] [PATCH 3/5] migration/vmstate: fix array of ptr with nullptrs |
Date: |
Tue, 21 Feb 2017 13:55:06 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 |
On 02/21/2017 01:22 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (address@hidden) wrote:
>>
>>
>> On 02/17/2017 08:42 PM, Dr. David Alan Gilbert wrote:
>>> * Halil Pasic (address@hidden) 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 <address@hidden>
>>>> Reviewed-by: Guenther Hutzl <address@hidden>
>>>>
>>>> ---
>>>> 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
[Qemu-devel] [PATCH 2/5] migration/vmstate: split up vmstate_base_addr, Halil Pasic, 2017/02/16
[Qemu-devel] [PATCH 4/5] tests/test-vmstate.c: test array of ptr with null, Halil Pasic, 2017/02/16
[Qemu-devel] [PATCH 5/5] tests/test-vmstate.c: test array of ptr to primitive, Halil Pasic, 2017/02/16