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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Amit Shah (amit.shah@redhat.com) wrote:
> On (Tue) 14 Apr 2015 [18:03:30], Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > 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.

Yep; OK, I can split them out on an individual basis.

> 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.

Yes, I can do that (I intend some day to fix qemu_get_buffer and co to use size_t).

Thanks,

Dave

> Thanks,
> 
> 		Amit
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK