Re: [Qemu-devel] [PATCH v4 35/47] Page request: Add MIG_RPCOMM_REQPAGES reverse command

[David Gibson]

On Wed, Nov 19, 2014 at 08:01:31PM +0000, Dr. David Alan Gilbert wrote:
> * David Gibson (address@hidden) wrote:
> > On Fri, Oct 03, 2014 at 06:47:41PM +0100, Dr. David Alan Gilbert (git) 
> > wrote:
> > > From: "Dr. David Alan Gilbert" <address@hidden>
> > > 
> > > Add MIG_RPCOMM_REQPAGES command on Return path for the postcopy
> > > destination to request a page from the source.
> 
> > > +    buf64[0] = (uint64_t)start;
> > > +    buf64[0] = cpu_to_be64(buf64[0]);
> > 
> > I think this would be clearer as well as less verbose, as just:
> >     buf64[0] = cpu_to_be64(start);
> 
> I've gone with the halfway mark of:
> 
> buf64[0] = cpu_to_be64((uint64_t)start);
> 
> it jsut doesn't feel right passing something into a byteswap
> unless you know the size.

I've always thought of (this group of) byteswap functions as
specifying the output size.  It's a value parameter so integer
promotion to the required type is pretty safe.

> > > +    buf64[1] = (uint64_t)len;
> > > +    buf64[1] = cpu_to_be64(buf64[1]);
> > > +    migrate_send_rp_message(mis, MIG_RPCOMM_REQPAGES, msglen, bufc);
> > > +}
> > > +
> > >  void qemu_start_incoming_migration(const char *uri, Error **errp)
> > >  {
> > >      const char *p;
> > > @@ -784,6 +816,17 @@ static void source_return_path_bad(MigrationState *s)
> > >  }
> > >  
> > >  /*
> > > + * Process a request for pages received on the return path,
> > > + * We're allowed to send more than requested (e.g. to round to our page 
> > > size)
> > > + * and we don't need to send pages that have already been sent.
> > > + */
> > > +static void migrate_handle_rp_reqpages(MigrationState *ms, const char* 
> > > rbname,
> > > +                                       ram_addr_t start, ram_addr_t len)
> > > +{
> > > +    DPRINTF("migrate_handle_rp_reqpages: at %zx for len %zx", start, 
> > > len);
> > > +}
> > > +
> > > +/*
> > >   * Handles messages sent on the return path towards the source VM
> > >   *
> > >   */
> > > @@ -795,6 +838,8 @@ static void *source_return_path_thread(void *opaque)
> > >      const int max_len = 512;
> > >      uint8_t buf[max_len];
> > >      uint32_t tmp32;
> > > +    uint64_t tmp64a, tmp64b;
> > 
> > Hrm.. calling everything "tmp*" doesn't help readability.
> 
> True; most of the rest of those tmps are used by multiple commands and
> just read off the wire and immediately used.
> They're now start/len for tmp64a/b.

Ok, great.  I find it's usually best to declare appropriate variables
for each case (or even use local blocks), rather than share.  The
compiler's smart enough to coalesce them, so there's no real cost.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

pgpNImhbCG_ZI.pgp
Description: PGP signature