qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [RFC PATCH v2 7/8] migration/vmstate: fix array of poin


From: Halil Pasic
Subject: Re: [Qemu-devel] [RFC PATCH v2 7/8] migration/vmstate: fix array of pointers to struct
Date: Tue, 31 Jan 2017 16:17:23 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0


On 12/15/2016 01:33 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 at least for pointers to
>> structs. 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. Sadly
>> all other error scenarios are not detected by this scheme (and would
>> require the usage of the JSON meta data).
>>
>> Limitations: Does not work for pointers to primitives.
> 
> Please document that limitation in a comment in the vmstate.h near the
> macros that use it.
> 

After looking at this again I do not know what was my intention whit the
limitations statement. It was probably about arrays of pointers to
primitives, but then the statement is wrong. The same scheme does work
for primitives too. It just that I had a previous version which had this
limitation. 

It could also be about just pointer (not array of pointers), but the
commit message does not mention this case at all. 

I will just drop the limitations statement.

>> 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 from within
>> the channel subsystem (see ChannelSubSys.css in hw/s390x/css.c for an
>> example).
>> ---
>>  include/migration/vmstate.h |  5 +++++
>>  migration/vmstate.c         | 32 ++++++++++++++++++++++++++++++--
>>  2 files changed, 35 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>> index 5940db0..86d4aca 100644
>> --- a/include/migration/vmstate.h
>> +++ b/include/migration/vmstate.h
>> @@ -236,6 +236,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 ((int8_t) -1)
> 
> You seem to be making things harder by using a signed int everywhere
> when you just want a byte;  I suggest using '0'
> 

I'm fine with byte 0x30, but I have an irrational bad feeling about
using a character literal here. I know, in practice should
not matter, but the C11 standard says nothing about how members of the
basic execution character set are represented, and it's also not among
the 'usual QEMU platform assumptions' (AFAIK). Are you OK defining
the constant/macro as Ox30U instead of '0'? 

>> +extern const VMStateInfo vmstate_info_nullptr;
>> +
>>  extern const VMStateInfo vmstate_info_float64;
>>  extern const VMStateInfo vmstate_info_cpudouble;
>>  
>> @@ -453,6 +457,7 @@ extern const VMStateInfo vmstate_info_bitmap;
>>      .size       = sizeof(_type *),                                    \
>>      .flags      = VMS_ARRAY|VMS_STRUCT|VMS_ARRAY_OF_POINTER,         \
>>      .offset     = vmstate_offset_array(_s, _f, _type*, _n),          \
>> +    .info       = &vmstate_info_nullptr,                              \
> 
> What's this change for?

It's garbage. A remainder of a previous version I mentioned above. Will
remove. Good catch!

> 
>>  }
>>  
>>  #define VMSTATE_STRUCT_SUB_ARRAY(_field, _state, _start, _num, _version, 
>> _vmsd, _type) { \
>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>> index 10a7645..ce3490a 100644
>> --- a/migration/vmstate.c
>> +++ b/migration/vmstate.c
>> @@ -109,7 +109,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);
>> +                    vmstate_info_nullptr.get(f, curr_elem, size);
>> +                } else if (field->flags & VMS_STRUCT) {
>>                      ret = vmstate_load_state(f, field->vmsd, curr_elem,
>>                                               field->vmsd->version_id);
>>                  } else {
>> @@ -320,7 +324,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);
>> +                } else if (field->flags & VMS_STRUCT) {
>>                      vmstate_save_state(f, field->vmsd, curr_elem, 
>> vmdesc_loop);
>>                  } else {
>>                      field->info->put(f, curr_elem, size);
>> @@ -708,6 +716,26 @@ const VMStateInfo vmstate_info_uint64 = {
>>      .put  = put_uint64,
>>  };
>>  
>> +static int get_nullptr(QEMUFile *f, void *pv, size_t size)
>> +{
>> +    int8_t tmp;
>> +    qemu_get_s8s(f, &tmp);
> 
> Now that I've suggested using just a character you can turn that
> into:
> 
>   return qemu_get_byte(f) == VMS_NULLPTR_MARKER ? 0 : -EINVAL;
> 

You are right! I was over-complicating. Will do.

>> +    return tmp == VMS_NULLPTR_MARKER ? 0 : -EINVAL;
>> +}
>> +
>> +static void put_nullptr(QEMUFile *f, void *pv, size_t size)
>> +{
>> +    int8_t tmp = VMS_NULLPTR_MARKER;
>> +    assert(pv == NULL);
>> +    qemu_put_s8s(f, &tmp);
> 
> and similarly put_byte.
> 

Will do.

> Dave
> 

Thanks for the review. I have already adopted mostly your proposals,
but I will wait a bit in the hope that the fate of this patch borrowed
form my other patch set (subject:"[PATCH v2 2/2] migration: drop unused
VMStateField.start" gets resolved.

By the way you could answer my question regarding that yourself, since
you became co-maintainer for migration starting 2017-01-24. Congrats :).


Regards,
Halil

>> +}
>> +
>> +const VMStateInfo vmstate_info_nullptr = {
>> +    .name = "uint64",
>> +    .get  = get_nullptr,
>> +    .put  = put_nullptr,
>> +};
>> +
>>  /* 64 bit unsigned int. See that the received value is the same than the one
>>     in the field */
>>  
>> -- 
>> 2.8.4
>>
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK
> 




reply via email to

[Prev in Thread] Current Thread [Next in Thread]