Re: [Xen-devel] [PATCH v4 22/32] libxl_qmp: Handle messages from QEMU

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Tue, Aug 21, 2018 at 09:58:57AM +0100, Wei Liu wrote:
> On Fri, Aug 03, 2018 at 06:25:00PM +0100, Anthony PERARD wrote:
> > > > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > > > index 4a385801ba..e104fea941 100644
> > > > --- a/tools/libxl/libxl_types.idl
> > > > +++ b/tools/libxl/libxl_types.idl
> > > > @@ -69,6 +69,10 @@ libxl_error = Enumeration("error", [
> > > >      (-23, "NOTFOUND"),
> > > >      (-24, "DOMAIN_DESTROYED"), # Target domain ceased to exist during 
> > > > op
> > > >      (-25, "FEATURE_REMOVED"), # For functionality that has been removed
> > > > +    (-26, "QMP_GENERIC_ERROR"), # unspecified qmp error
> > > > +    (-27, "QMP_COMMAND_NOT_FOUND"), # the requested command has not 
> > > > been found
> > > > +    (-28, "QMP_DEVICE_NOT_ACTIVE"), # a device has failed to be become 
> > > > active
> > > > +    (-29, "QMP_DEVICE_NOT_FOUND"), # the requested device has not been 
> > > > found
> > > 
> > > Do we really need such granularity for QMP errors? Isn't it enough to
> > > have a single ERROR_QMP or similar?
> > 
> > No I don't think so. QMP_COMMAND_NOT_FOUND can be useful when a qmp
> > command is removed (there is already one that is deprecated and we use).
> > 
> > The last two could be useful to user of libxl as they could provide
> > better error messages. xl don't care because whatever error message is
> > attach to the error, it will be printed.
> 
> If the users of libxl aren't likely to care about the exact error
> messages, we don't need to expose these at all. They are more suitable
> to be put under libxl_internal_types.idl.
> 
> And then you translate the internal error numbers into external ones
> where appropriate.

I've already tried to have qmp specific error number, I've been asked to
invent libxl error number instead:
https://lists.xenproject.org/archives/html/xen-devel/2018-07/msg01151.html

Also, libxl doesn't have internal error number, there is only one type
of error, it's `libxl_error`.

I'm quite certain that those error numbers needs to be exposed to users
of `libxl__ev_qmp` (so internal to libxl), and the only way to return
these error numbers, is via `rc`, which should at all time be
`libxl_error` (see coding style). An other way would be to add an extra
parameter to `libxl__ev_qmp_callback`, a second error number, this time
qmp specific. Now which one should we check? And that would be more
error-prone.

Maybe some of those error numbers could be match back to existing one,
but not all.

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel