[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 04/47] Add qemu_get_counted_string to read a
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-devel] [PATCH v6 04/47] Add qemu_get_counted_string to read a string prefixed by a count byte |
Date: |
Fri, 15 May 2015 15:06:50 +0100 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
* Amit Shah (address@hidden) wrote:
> 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.
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 / address@hidden / Manchester, UK