Re: [Qemu-devel] [PATCH v2] Define the architecture for compressed dump format.

[Laszlo Ersek <lersek@redhat.com>]

one not-so-important comment below:

On 01/31/14 14:45, Ekaterina Tumanova wrote:
> Signed-off-by: Ekaterina Tumanova <tumanova@linux.vnet.ibm.com>
> ---
>  dump.c             | 7 +++++--
>  target-i386/cpu.h  | 2 ++
>  target-s390x/cpu.h | 1 +
>  3 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/dump.c b/dump.c
> index 8f64aab..25503bc 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -32,6 +32,9 @@
>  #ifdef CONFIG_SNAPPY
>  #include <snappy-c.h>
>  #endif
> +#ifndef ELF_MACHINE_UNAME
> +#define ELF_MACHINE_UNAME "Unknown"
> +#endif
>  
>  static uint16_t cpu_convert_to_target16(uint16_t val, int endian)
>  {
> @@ -817,7 +820,7 @@ static int create_header32(DumpState *s)
>      dh->nr_cpus = cpu_convert_to_target32(s->nr_cpus, endian);
>      bitmap_blocks = DIV_ROUND_UP(s->len_dump_bitmap, block_size) * 2;
>      dh->bitmap_blocks = cpu_convert_to_target32(bitmap_blocks, endian);
> -    memcpy(&(dh->utsname.machine), "i686", 4);
> +    strncpy((char *)&(dh->utsname.machine), ELF_MACHINE_UNAME, sizeof(dh->utsname.machine));

"dh->utsname.machine" is actually an array of characters, if I recall
correctly (see NewUtsname in patch 08).

The expression in the first argument takes the address of the entire
array (the resultant pointer has type pointer-to-array). I didn't call
it out with memcpy(), because it didn't really matter. But now it looks
a bit gross, because as 2nd step we convert the pointer-to-array back to
pointer-to-char. It would be simpler to write

    strncpy(dh->utsname.machine, ELF_MACHINE_UNAME,
            sizeof(dh->utsname.machine))

where "dh->utsname.machine" decays to a pointer to its first element:

6.      Language
6.3     Conversions
6.3.2   Other operands
6.3.2.1 Lvalues, arrays, and function designators

  3  Except when it is the operand of the sizeof operator or the unary
     & operator, or is a string literal used to initialize an array, an
     expression that has type ‘‘array of type’’ is converted to an
     expression with type ‘‘pointer to type’’ that points to the
     initial element of the array object and is not an lvalue. If the
     array object has register storage class, the behavior is undefined.

I should have probably noticed this in v1 of the followup patch. I don't
insist on upating it of course.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Nonetheless, if you want to fix that up in a v3, please keep my R-b.

Thanks
Laszlo