Re: [Xen-devel] [Patch 2/2] support of cpupools in xl: commands and library changes

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Tue, 2010-10-05 at 14:49 +0100, Juergen Gross wrote:
> >> Renamed all cpu pool related names to *cpupool*
> >
> > But not the actual xl command names -- I presume this is for xm
> > compatibility, which is shame but unavoidable I suppose.
> 
> Hmm. What about adding aliases for xm (xm cpupool-*) and using only
> the cpupool-* commands for xl?

I'm not sure, what do others on the list think of this?

> >
> >> +            }
> >> +        }
> >> +
> >> +    for (;;) {
> >> +        t = xs_transaction_start(ctx->xsh);
> >> +
> >> +        xs_mkdir(ctx->xsh, t, libxl__sprintf(&gc, "/local/pool/%d", 
> >> *poolid));
> >> +        libxl__xs_write(&gc, t, libxl__sprintf(&gc, 
> >> "/local/pool/%d/uuid", *poolid),
> >> +                 uuid_string);
> >> +        libxl__xs_write(&gc, t, libxl__sprintf(&gc, 
> >> "/local/pool/%d/name", *poolid),
> >> +                 name);
> >> +
> >> +        if (xs_transaction_end(ctx->xsh, t, 0) || (errno != EAGAIN))
> >> +            return 0;
> >
> > Does this return success on failure with errno != EAGAIN?
> 
> Like lots of other libxl functions...
> Missing xenstore entries seem not to be regarded as failures.

OK, I'm not sure what is considered correct here but if its the norm
then fine.

> >> +
> >> +int main_poolcreate(int argc, char **argv)
> >> +{
[...]
> >> +    while (1) {
[...]
>     }
> >> +
> >> +    memset(extra_config, 0, sizeof(extra_config));
> >> +    while (optind<  argc) {
[...]
> >> +    }
> >
> > Move this loop until after libxl_read_file_contents so you can add the
> > items directly to the end of the data along with the reallocs and
> > therefore avoid the 1024 character limitation?
> 
> The filename may be a result of this loop...

OK, it's consistent with xl create too so I guess even though it looks
weird to me it must be ok ;-)

> > Need to free name somewhere.
> 
> I can do it, but I think there is much more work ahead for freeing all
> allocated memory in xl!

Some of us have been actively working towards making xl free its memory
correctly since it is a useful to have xl leak free in order to validate
libxl which may be used by longer running toolstacks and hence better
not leak!

I'd certainly like to avoid taking backward steps where possible.

> >> +    if (cpupool_qualifier_to_cpupoolid(pool,&poolid, NULL) ||
> >> +        !libxl_cpupoolid_to_name(&ctx, poolid)) {
> >
> > Given that cpupool_qualifier_to_cpupoolid basically ==
> > libxl_name_to_cpupoolid is there really any need to check the inverse
> > before attempting to destroy the pool?
> 
> Yes.
> cpupool_qualifier_to_cpupoolid() will always return 0 for any positive
> integer given to it (like domain_qualifier_to_domid()). This may be
> still no valid cpupoolid.

I see, cpupool_qualifier_to_cpupoolid is just a more clever strtoul-like
function not an actual cpupoolid lookup function. No problem then.

Thanks,
Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel