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.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home