| Message ID | 1465983102-19308-2-git-send-email-wei.liu2@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2016-06-15 at 10:31 +0100, Wei Liu wrote: > This function is used to return the memory needed for a guest. It's > not > in a position to modify the b_info passed in (note the _setdefault > function). > > Use a copy of b_info to do the calculation. Define a macro to mark > the > change in API. > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > --- > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > > v3: new > Any suggestion on the macro name? > Maybe LIBXL_HAVE_DOMAIN_NEED_MEMORY_BINFO_CONST (a bit long, I know...) > > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h > index 2c0f868..905852d 100644 > --- a/tools/libxl/libxl.h > +++ b/tools/libxl/libxl.h > @@ -67,6 +67,13 @@ > * the same $(XEN_VERSION) (e.g. throughout a major release). > */ > > +/* LIBXL_HAVE_DOMAIN_NEED_MEMORY_V2 > + * > + * If this is defined, libxl_domain_need_memory no longer modifies > + * passed in b_info. > + */ > +#define LIBXL_HAVE_DOMAIN_NEED_MEMORY_V2 > + > So, out of curiosity (or I should say "ignorance" :-)) how is one supposed to use this macro? Regards, Dario
On Wed, Jun 15, 2016 at 03:31:46PM +0200, Dario Faggioli wrote: > On Wed, 2016-06-15 at 10:31 +0100, Wei Liu wrote: > > This function is used to return the memory needed for a guest. It's > > not > > in a position to modify the b_info passed in (note the _setdefault > > function). > > > > Use a copy of b_info to do the calculation. Define a macro to mark > > the > > change in API. > > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > > --- > > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > > > > v3: new > > Any suggestion on the macro name? > > > Maybe LIBXL_HAVE_DOMAIN_NEED_MEMORY_BINFO_CONST > > (a bit long, I know...) Heh... > > > > > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h > > index 2c0f868..905852d 100644 > > --- a/tools/libxl/libxl.h > > +++ b/tools/libxl/libxl.h > > @@ -67,6 +67,13 @@ > > * the same $(XEN_VERSION) (e.g. throughout a major release). > > */ > > > > +/* LIBXL_HAVE_DOMAIN_NEED_MEMORY_V2 > > + * > > + * If this is defined, libxl_domain_need_memory no longer modifies > > + * passed in b_info. > > + */ > > +#define LIBXL_HAVE_DOMAIN_NEED_MEMORY_V2 > > + > > > So, out of curiosity (or I should say "ignorance" :-)) how is one > supposed to use this macro? #ifdef MACRO /* new API */ #else /* old API */ /* probably want to pass in a scratch copy of b_info */ #endif > > Regards, > Dario > -- > <<This happens because I choose it to happen!>> (Raistlin Majere) > ----------------------------------------------------------------- > Dario Faggioli, Ph.D, http://about.me/dario.faggioli > Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK) >
Wei Liu writes ("[PATCH v3 1/5] libxl: libxl_domain_need_memory shouldn't modify b_info"):
> This function is used to return the memory needed for a guest. It's not
> in a position to modify the b_info passed in (note the _setdefault
> function).
>
> Use a copy of b_info to do the calculation. Define a macro to mark the
> change in API.
Urgh, how unpleasant.
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 5ec4c80..65af9ee 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5123,20 +5123,24 @@ int libxl_domain_need_memory(libxl_ctx *ctx, libxl_domain_build_info *b_info,
> uint32_t *need_memkb)
> {
> GC_INIT(ctx);
> + libxl_domain_build_info tmp;
I would suggest, instead:
int libxl_domain_need_memory(libxl_ctx *ctx,
- libxl_domain_build_info *b_info,
+ const libxl_domain_build_info *b_info_in,
...
+ libxl_domain_build_info b_info[1];
If you do this then you do not need to change the bulk of the body of
the function at all.
> +/* LIBXL_HAVE_DOMAIN_NEED_MEMORY_V2
> + *
> + * If this is defined, libxl_domain_need_memory no longer modifies
> + * passed in b_info.
> + */
> +#define LIBXL_HAVE_DOMAIN_NEED_MEMORY_V2
I suggest constifying it and
LIBXL_HAVE_DOMAIN_NEED_MEMORY_CONST_B_INFO
Ian.
On Fri, Jul 08, 2016 at 06:35:59PM +0100, Ian Jackson wrote: > Wei Liu writes ("[PATCH v3 1/5] libxl: libxl_domain_need_memory shouldn't modify b_info"): > > This function is used to return the memory needed for a guest. It's not > > in a position to modify the b_info passed in (note the _setdefault > > function). > > > > Use a copy of b_info to do the calculation. Define a macro to mark the > > change in API. > > Urgh, how unpleasant. > > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > > index 5ec4c80..65af9ee 100644 > > --- a/tools/libxl/libxl.c > > +++ b/tools/libxl/libxl.c > > @@ -5123,20 +5123,24 @@ int libxl_domain_need_memory(libxl_ctx *ctx, libxl_domain_build_info *b_info, > > uint32_t *need_memkb) > > { > > GC_INIT(ctx); > > + libxl_domain_build_info tmp; > > I would suggest, instead: > > int libxl_domain_need_memory(libxl_ctx *ctx, > - libxl_domain_build_info *b_info, > + const libxl_domain_build_info *b_info_in, > ... > + libxl_domain_build_info b_info[1]; > > If you do this then you do not need to change the bulk of the body of > the function at all. > This is a good idea. Just that I discover there is another unpleasant fact: all the generated copy function doesn't constify their source parameter. I will need to write a patch to fix that first. Wei.
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index 5ec4c80..65af9ee 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -5123,20 +5123,24 @@ int libxl_domain_need_memory(libxl_ctx *ctx, libxl_domain_build_info *b_info, uint32_t *need_memkb) { GC_INIT(ctx); + libxl_domain_build_info tmp; int rc; - rc = libxl__domain_build_info_setdefault(gc, b_info); + libxl_domain_build_info_init(&tmp); + libxl_domain_build_info_copy(ctx, &tmp, b_info); + + rc = libxl__domain_build_info_setdefault(gc, &tmp); if (rc) goto out; - *need_memkb = b_info->target_memkb; - switch (b_info->type) { + *need_memkb = tmp.target_memkb; + switch (tmp.type) { case LIBXL_DOMAIN_TYPE_HVM: - *need_memkb += b_info->shadow_memkb + LIBXL_HVM_EXTRA_MEMORY; - if (libxl_defbool_val(b_info->device_model_stubdomain)) + *need_memkb += tmp.shadow_memkb + LIBXL_HVM_EXTRA_MEMORY; + if (libxl_defbool_val(tmp.device_model_stubdomain)) *need_memkb += 32 * 1024; break; case LIBXL_DOMAIN_TYPE_PV: - *need_memkb += b_info->shadow_memkb + LIBXL_PV_EXTRA_MEMORY; + *need_memkb += tmp.shadow_memkb + LIBXL_PV_EXTRA_MEMORY; break; default: rc = ERROR_INVAL; @@ -5146,6 +5150,7 @@ int libxl_domain_need_memory(libxl_ctx *ctx, libxl_domain_build_info *b_info, *need_memkb += (2 * 1024) - (*need_memkb % (2 * 1024)); rc = 0; out: + libxl_domain_build_info_dispose(&tmp); GC_FREE; return rc; diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h index 2c0f868..905852d 100644 --- a/tools/libxl/libxl.h +++ b/tools/libxl/libxl.h @@ -67,6 +67,13 @@ * the same $(XEN_VERSION) (e.g. throughout a major release). */ +/* LIBXL_HAVE_DOMAIN_NEED_MEMORY_V2 + * + * If this is defined, libxl_domain_need_memory no longer modifies + * passed in b_info. + */ +#define LIBXL_HAVE_DOMAIN_NEED_MEMORY_V2 + /* LIBXL_HAVE_VNUMA * * If this is defined the type libxl_vnode_info exists, and a
This function is used to return the memory needed for a guest. It's not in a position to modify the b_info passed in (note the _setdefault function). Use a copy of b_info to do the calculation. Define a macro to mark the change in API. Signed-off-by: Wei Liu <wei.liu2@citrix.com> --- Cc: Ian Jackson <ian.jackson@eu.citrix.com> v3: new Any suggestion on the macro name? This patch should not be backported because it changes API behaviour. --- tools/libxl/libxl.c | 17 +++++++++++------ tools/libxl/libxl.h | 7 +++++++ 2 files changed, 18 insertions(+), 6 deletions(-)