Re: [PATCH 2 of 8] libxl: introduce libxl_set_relative_memory_target

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

On Tue, 31 Aug 2010, Ian Jackson wrote:
> Stefano Stabellini writes ("[Xen-devel] [PATCH 2 of 8] libxl: introduce libxl_set_relative_memory_target"):
> > libxl: introduce libxl_set_relative_memory_target
> > 
> > Introduce libxl_set_relative_memory_target to modify the memory target
> > of a domain by a relative amount of memory in a single xenstore
> > transaction.
> > Modify libxl_set_memory_target to use xenstore transactions.
> > The first time we are reading/writing dom0 memory target, fill the
> > informations in xenstore if they are missing.
> 
> >  int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid,
> >                              uint32_t target_memkb, int enforce)
> 
> See my earlier comments about memory targets.  I don't think it makes
> much sense to give a domain a memory target and then let it exceed it.
> So I think "enforce" should be abolished (as if it were always set).
> 

I can do that.


> Also please can you try to keep your code to <75ish columns ? :-)
> (75 because there should be room for > and + quoting without wrap
> damage.)
> 

Yes. We need a CODING_STILE, I'll post a patch with it later on.


> >  int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid,
> >                 uint32_t target_memkb, int enforce)
> ...
> > +int libxl_set_relative_memory_target(libxl_ctx *ctx, uint32_t
> > +            domid, int32_t relative_target_memkb, int enforce)
> 
> These functions are really rather too similar for my taste.  They
> seem to differ only in whether they read the existing target and add
> it on.  Surely they should be combined.
> 
> Also, I don't really think this patch to introuce the relative setting
> function should involves adding a lot of code to the absolute setting
> function.  It's a shame that we have to set so many different copies
> of the same value, but if we do then that should be done in a separate
> patch first perhaps ?
> 

The separate patch is a good idea, but merging the two functions
together will result in code harder to read in the implementation of a
very important function.