[v9,04/10] Add host_from_stream_offset_versioned function

Signed-off-by: Orit Wasserman <owasserm@redhat.com>
Signed-off-by: Benoit Hudzia <benoit.hudzia@sap.com>
Signed-off-by: Petter Svard <petters@cs.umu.se>
Signed-off-by: Aidan Shribman <aidan.shribman@sap.com>
---
 arch_init.c |   26 +++++++++++++++++++++++---
 1 files changed, 23 insertions(+), 3 deletions(-)

Orit Wasserman <owasserm@redhat.com> wrote:
> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
> Signed-off-by: Benoit Hudzia <benoit.hudzia@sap.com>
> Signed-off-by: Petter Svard <petters@cs.umu.se>
> Signed-off-by: Aidan Shribman <aidan.shribman@sap.com>
> ---
>  arch_init.c |   26 +++++++++++++++++++++++---
>  1 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/arch_init.c b/arch_init.c
> index 47b9fef..f1690cf 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -599,6 +599,18 @@ static inline void *host_from_stream_offset(QEMUFile *f,
>      return NULL;
>  }

Why do we need this patch?

> +static inline void *host_from_stream_offset_versioned(int version_id,
> +        QEMUFile *f, ram_addr_t offset, int flags)
> +{
> +        void *host;
> +        if (version_id == 3) {
> +                host = qemu_get_ram_ptr(offset);
> +        } else {
> +                host = host_from_stream_offset(f, offset, flags);
> +        }
> +        return host;
> +}
> +
>  int ram_load(QEMUFile *f, void *opaque, int version_id)
>  {

int ram_load(QEMUFile *f, void *opaque, int version_id)
{
    ram_addr_t addr;
    int flags;
    int error;

    if (version_id < 4 || version_id > 4) {
        return -EINVAL;
    }

As you can see, version_id will never be 3, or I am missing something?

Later, Juan.

On 04/11/2012 01:49 PM, Orit Wasserman wrote:
> Signed-off-by: Orit Wasserman<owasserm@redhat.com>
> Signed-off-by: Benoit Hudzia<benoit.hudzia@sap.com>
> Signed-off-by: Petter Svard<petters@cs.umu.se>
> Signed-off-by: Aidan Shribman<aidan.shribman@sap.com>

ENOEXPLANATION

The indentation is wrong in this patch and the commit message needs to explain 
what's going on here.

BTW, SoB is supposed to represent a handling chain.  If you're the last person 
to touch this patch, your SoB should be the last one.

Regards,

Anthony Liguori

> ---
>   arch_init.c |   26 +++++++++++++++++++++++---
>   1 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/arch_init.c b/arch_init.c
> index 47b9fef..f1690cf 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -599,6 +599,18 @@ static inline void *host_from_stream_offset(QEMUFile *f,
>       return NULL;
>   }
>
> +static inline void *host_from_stream_offset_versioned(int version_id,
> +        QEMUFile *f, ram_addr_t offset, int flags)
> +{
> +        void *host;
> +        if (version_id == 3) {
> +                host = qemu_get_ram_ptr(offset);
> +        } else {
> +                host = host_from_stream_offset(f, offset, flags);
> +        }
> +        return host;
> +}
> +
>   int ram_load(QEMUFile *f, void *opaque, int version_id)
>   {
>       ram_addr_t addr;
> @@ -654,8 +666,11 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
>               void *host;
>               uint8_t ch;
>
> -            host = host_from_stream_offset(f, addr, flags);
> +            host = host_from_stream_offset_versioned(version_id,
> +                                                     f, addr, flags);
>               if (!host) {
> +                fprintf(stderr, "Failed to convert RAM address to host"
> +                        " for offset " RAM_ADDR_FMT "\n", addr);
>                   return -EINVAL;
>               }
>
> @@ -670,8 +685,13 @@ int ram_load(QEMUFile *f, void *opaque, int version_id)
>           } else if (flags&  RAM_SAVE_FLAG_PAGE) {
>               void *host;
>
> -            host = host_from_stream_offset(f, addr, flags);
> -
> +            host = host_from_stream_offset_versioned(version_id,
> +                                                     f, addr, flags);
> +            if (!host) {
> +                fprintf(stderr, "Failed to convert RAM address to host"
> +                        " for offset " RAM_ADDR_FMT "\n", addr);
> +                return -EINVAL;
> +            }
>               qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
>           }
>           error = qemu_file_get_error(f);

On 04/18/2012 05:42 PM, Juan Quintela wrote:
> Orit Wasserman <owasserm@redhat.com> wrote:
>> Signed-off-by: Orit Wasserman <owasserm@redhat.com>
>> Signed-off-by: Benoit Hudzia <benoit.hudzia@sap.com>
>> Signed-off-by: Petter Svard <petters@cs.umu.se>
>> Signed-off-by: Aidan Shribman <aidan.shribman@sap.com>
>> ---
>>  arch_init.c |   26 +++++++++++++++++++++++---
>>  1 files changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch_init.c b/arch_init.c
>> index 47b9fef..f1690cf 100644
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -599,6 +599,18 @@ static inline void *host_from_stream_offset(QEMUFile *f,
>>      return NULL;
>>  }
> 
> Why do we need this patch?
> 
>> +static inline void *host_from_stream_offset_versioned(int version_id,
>> +        QEMUFile *f, ram_addr_t offset, int flags)
>> +{
>> +        void *host;
>> +        if (version_id == 3) {
>> +                host = qemu_get_ram_ptr(offset);
>> +        } else {
>> +                host = host_from_stream_offset(f, offset, flags);
>> +        }
>> +        return host;
>> +}
>> +
>>  int ram_load(QEMUFile *f, void *opaque, int version_id)
>>  {
> 
> int ram_load(QEMUFile *f, void *opaque, int version_id)
> {
>     ram_addr_t addr;
>     int flags;
>     int error;
> 
>     if (version_id < 4 || version_id > 4) {
>         return -EINVAL;
>     }
> 
> As you can see, version_id will never be 3, or I am missing something?
> 
You are correct, one less patch.

Thanks,
Orit
> Later, Juan.
>