Re: [Qemu-devel] [PATCH v6 04/47] Add qemu_get_counted_string to read a string prefixed by a count byte

[Amit Shah]

On (Tue) 14 Apr 2015 [18:03:30], Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <address@hidden>
> 
> and use it in loadvm_state and ram_load.

This patch is doing several things at once:

- reducing size of a buffer from 257 to 256 (it's safe, but not
  mentioned in the commit log)

- adding an error return to one calling site (again not mentioned
  here)

> @@ -1145,13 +1145,10 @@ static int ram_load(QEMUFile *f, void *opaque, int 
> version_id)
>              total_ram_bytes = addr;
>              while (!ret && total_ram_bytes) {
>                  RAMBlock *block;
> -                uint8_t len;
>                  char id[256];
>                  ram_addr_t length;
>  
> -                len = qemu_get_byte(f);
> -                qemu_get_buffer(f, (uint8_t *)id, len);
> -                id[len] = 0;
> +                qemu_get_counted_string(f, id);
>                  length = qemu_get_be64(f);
>  
>                  QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {

- ... while not doing that for the other calling site.  In fact we
  really should check return value there too, isn't it?  buf[len] is
  set to 0, not buf[0] in case of an error, and ram_load could happily
  start using string functions on the bogus data in id[].

Can you please split the patches up, or write a verbose commit
message?

Also, I think you should just post these preparatory patches in a
separate series so they can be applied as they're good on their own.

Postcopy patches themselves can come as another series, and that also
makes reviewing easier.

Also:

> +
> +/*
> + * Get a string whose length is determined by a single preceding byte
> + * A preallocated 256 byte buffer must be passed in.
> + * Returns: 0 on success and a 0 terminated string in the buffer
> + */
> +int qemu_get_counted_string(QEMUFile *f, char buf[256])
> +{
> +    unsigned int len = qemu_get_byte(f);
> +    int res = qemu_get_buffer(f, (uint8_t *)buf, len);
> +
> +    buf[len] = 0;
> +
> +    return res != len;

since you're returning bool, how about making this bool?  Though I'd
like it if this was

return res == len ? res : 0;

BTW I'd like it if everything (return value, res, len) were all
unsigned.  The operations are safe, but it sucks we use signed values
for counting things all over the place.

Thanks,

                Amit