Re: [Xen-devel] [PATCH v9 6/9] libxl/xl: deprecate the build_info->cpumap field

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On mer, 2014-06-18 at 17:44 +0100, Ian Campbell wrote:
> On Wed, 2014-06-18 at 18:26 +0200, Dario Faggioli wrote:
> > On mer, 2014-06-18 at 16:53 +0100, Ian Campbell wrote:
> > > On Wed, 2014-06-18 at 16:28 +0200, Dario Faggioli wrote:

> > > > @@ -261,6 +262,13 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
> > > >              return rc;
> > > >      }
> > > >      libxl_domain_set_nodeaffinity(ctx, domid, &info->nodemap);
> > > > +    /*
> > > > +     * info->cpumap is DEPRECATED, but we still want old applications
> > > > +     * that may be using it to continue working.
> > > > +     */
> > > > +    if (!libxl_bitmap_is_full(&info->cpumap))
> > > 
> > > The caller is expected to initialise this unused field to a non-default
> > > state? That doesn't sound right. Did you mean !is_empty?
> > > 
> > Nope. The default for this is to be full, so what I'm checking is really
> > that it stayed default. See libxl__domain_build_info_setdefault():
> > 
> >     ...
> >     if (!b_info->cpumap.size) {
> >         if (libxl_cpu_bitmap_alloc(CTX, &b_info->cpumap, 0))
> >             return ERROR_FAIL;
> >         libxl_bitmap_set_any(&b_info->cpumap);
> >     }
> >     ...
> > 
> > Can I change this? If I can, I think the best would be to remove the
> > allocation from libxl__domain_build_info_setdefault(), so that all the
> > checks could become something like `if(cpumap.size)'.
> 
> I agree.
> 
> > But does stop allocating the bitmap qualifies as an incompatible API
> > change?
> 
> I don't think so, do you think it might for some reason?
> 
Not sure... May an existing application rely on the fact that this is
being allocated already? I thought it may, but perhaps I'm
misunderstanding when exactly _setdefaults() is supposed to be called.

If existing apps do as xl, then it's not an issue to change the default
as said above. In fact, in xl, _setdefaults() is called (via
freemem()->libxl_domain_need_memory()) after the config file has been
parsed already, meaning the user has to allocate cpumap himself if he
wants to use it. Is this the intended usage? If yes, I'll happily get
rid of that initializer!

Thanks and 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)

Attachment: signature.asc
Description: This is a digitally signed message part

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel