Re: [PATCH v3 2/7] libxl: Add to libxl__domain_type a new return value (LIBXL_DOMAIN_TYPE_NOTFOUND)

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Tue, Mar 24, 2015 at 03:15:30PM +0000, Ian Campbell wrote:
> On Mon, 2015-03-23 at 14:20 -0400, Konrad Rzeszutek Wilk wrote:
> > So that the callers can distinguish between an error and
> > an domain not found. The exposed API calls that are effected
> > by this are: libxl_domain_[remus_start,suspend,unpause,cdrom_insert,
> > set_vcpuonline]
> > 
> > We add an helper function to deal with the two types of errors:
> > libxl_domain_type2err. However for libxl_[pci,dom].c we just add
> > the extra check for LIBXL_DOMAIN_TYPE_NOTFOUND for simplicity
> > reasons.
> > 
> > Suggested-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Did I?

You suggested that 'libxl_set_vcpuonline' deal with all of the
various issues instead of having them in the 'vcpusset'. This would
bolting in libxl_set_vcpuonline' the proper error code support - which
this patch does.

> 
> Anyway, I'm not terribly keen on the approach taken here, sorry, in
> particular extending libxl_domain_type with a subset of the libxl_error
> codes and the shenanigans which are needed to convert between them.

:-)
> 
> Off hand I can think of 3 possible alternative solutions:
> 
> Arrange that the -ve error values in libxl_domain_type directly
> correspond to an appropriate libxl_error code, allowing code to do
>     if (type < 0) return type;
>     if (type < 0) { rc = type ; goto out }
> type stuff.
> 
> Change the prototype of libxl__domain_type to always return a libxl rc
> and on success return the type via a pointer argument.
> 
> Change the return type of libxl_domain_type to an int and return either
> a LIBXL_DOMAIN_TYPE_FOO or an ERROR_*.
> 
> They all have downsides (I don't much like the implicit cast between
> domain type and error code, changing the prototype will probably be
> disruptive, changing the return type means gcc can't catch missing
> switch statements for us).
> 
> Perhaps you or my fellow maintainers have a better idea or a preference
> for one of the above?

Drop the patch altogether and just leave the imprecise errors
that libxl_set_vcpuonline can return?

It is OK if it just returns LIBXL_DOMAIN_TYPE_INVALID - as the message about
what went wrong is most important. With patches:
 "libxl/vcpu-set - allow to decrease vcpu count on overcommitted guests (v3)"
and
 "libxl: In libxl_set_vcpuonline check for maximum number of VCPUs against the cpumap"

we print now the error messages when the user mistakenly:
 1) picks the wrong domain id
 2) picks the wrong cpu count (too many)

So this patch can be ignored.
> 
> Ian.
>