4:15 a.m.
otherwise a missing UUID in a domain config just shows:
libxlDomainXMLFromNative:2600 : Internal Error parsing xm config failed
Now we have:
xenXMConfigGetUUID:186 : Internal Error config value uuid was missing
O.k. to apply?
-- Guido
---
src/xenxs/xen_xm.c | 37 +++++++++++++++++++++++++++----------
1 files changed, 27 insertions(+), 10 deletions(-)
diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c
index 03857c8..d057043 100644
--- a/src/xenxs/xen_xm.c
+++ b/src/xenxs/xen_xm.c
@@ -174,21 +174,38 @@ static int xenXMConfigCopyStringOpt(virConfPtr conf,
/* Convenience method to grab a string UUID from the config file object */
static int xenXMConfigGetUUID(virConfPtr conf, const char *name, unsigned char *uuid) {
virConfValuePtr val;
- if (!uuid || !name || !conf)
- return (-1);
+
+ if (!uuid || !name || !conf) {
+ XENXS_ERROR(VIR_ERR_INVALID_ARG,
+ _("Arguments must be non null"));
+ return -1;
+ }
+
if (!(val = virConfGetValue(conf, name))) {
- return (-1);
+ XENXS_ERROR(VIR_ERR_INTERNAL_ERROR,
+ _("config value %s was missing"), name);
+ return -1;
}
- if (val->type != VIR_CONF_STRING)
- return (-1);
- if (!val->str)
- return (-1);
+ if (val->type != VIR_CONF_STRING) {
+ XENXS_ERROR(VIR_ERR_INTERNAL_ERROR,
+ _("config value %s not a string"), name);
+ return -1;
+ }
+
+ if (!val->str) {
+ XENXS_ERROR(VIR_ERR_INTERNAL_ERROR,
+ _("%s can't be empty"), name);
+ return -1;
+ }
- if (virUUIDParse(val->str, uuid) < 0)
- return (-1);
+ if (virUUIDParse(val->str, uuid) < 0) {
+ XENXS_ERROR(VIR_ERR_INTERNAL_ERROR,
+ _("%s not parseable"), val->str);
+ return -1;
+ }
- return (0);
+ return 0;
}
#define MAX_VFB 1024
--
1.7.6.3
11:50 a.m.
On 10/06/2011 03:15 AM, Guido Günther wrote:
otherwise a missing UUID in a domain config just shows:
libxlDomainXMLFromNative:2600 : Internal Error parsing xm config failed
Now we have:
xenXMConfigGetUUID:186 : Internal Error config value uuid was missing
Still doesn't sound quite right - it's not an internal error (where
libvirt has gotten internal logic wrong) so much as a user-supplied data
validation error.
+ if (val->type != VIR_CONF_STRING) {
+ XENXS_ERROR(VIR_ERR_INTERNAL_ERROR,
+ _("config value %s not a string"), name);
+ return -1;
+ }
+
+ if (!val->str) {
+ XENXS_ERROR(VIR_ERR_INTERNAL_ERROR,
+ _("%s can't be empty"), name);
+ return -1;
+ }
- if (virUUIDParse(val->str, uuid)< 0)
- return (-1);
+ if (virUUIDParse(val->str, uuid)< 0) {
+ XENXS_ERROR(VIR_ERR_INTERNAL_ERROR,
+ _("%s not parseable"), val->str);
These three errors should probably all be changed away from
VIR_ERR_INTERNAL_ERROR into something more useful, but I'm not sure
whether that would be VIR_ERR_CONF_SYNTAX, VIR_ERR_CONFIG_UNSUPPORTED,
or something else.
ACK to the concept, once we decide on the correct error code.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
12:36 p.m.
Hi Eric,
On Thu, Oct 06, 2011 at 10:50:46AM -0600, Eric Blake wrote:
On 10/06/2011 03:15 AM, Guido Günther wrote:
>otherwise a missing UUID in a domain config just shows:
>
>libxlDomainXMLFromNative:2600 : Internal Error parsing xm config failed
>
>Now we have:
>
>xenXMConfigGetUUID:186 : Internal Error config value uuid was missing
Still doesn't sound quite right - it's not an internal error (where
libvirt has gotten internal logic wrong) so much as a user-supplied
data validation error.
>+ if (val->type != VIR_CONF_STRING) {
>+ XENXS_ERROR(VIR_ERR_INTERNAL_ERROR,
>+ _("config value %s not a string"), name);
>+ return -1;
>+ }
>+
>+ if (!val->str) {
>+ XENXS_ERROR(VIR_ERR_INTERNAL_ERROR,
>+ _("%s can't be empty"), name);
>+ return -1;
>+ }
>
>- if (virUUIDParse(val->str, uuid)< 0)
>- return (-1);
>+ if (virUUIDParse(val->str, uuid)< 0) {
>+ XENXS_ERROR(VIR_ERR_INTERNAL_ERROR,
>+ _("%s not parseable"), val->str);
These three errors should probably all be changed away from
VIR_ERR_INTERNAL_ERROR into something more useful, but I'm not sure
whether that would be VIR_ERR_CONF_SYNTAX,
VIR_ERR_CONFIG_UNSUPPORTED, or something else.
I was uncertain about the error codes too, that's why I kept
VIR_ERR_INTERNAL_ERROR as used by other functions in this file. I'd
opt for VIR_ERR_CONF_SYNTAX.
-- Guido
ACK to the concept, once we decide on the correct error code.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
2:55 p.m.
On 10/06/2011 01:36 PM, Guido Günther wrote:
Hi Eric,
On Thu, Oct 06, 2011 at 10:50:46AM -0600, Eric Blake wrote:
>
> These three errors should probably all be changed away from
> VIR_ERR_INTERNAL_ERROR into something more useful, but I'm not sure
> whether that would be VIR_ERR_CONF_SYNTAX,
> VIR_ERR_CONFIG_UNSUPPORTED, or something else.
I was uncertain about the error codes too, that's why I kept
VIR_ERR_INTERNAL_ERROR as used by other functions in this file. I'd
opt for VIR_ERR_CONF_SYNTAX.
We really need to agree on a policy on which code to use for which type
of errors, write it down, then go through the code and correct
everything that's wrong. INTERNAL_ERROR is used *a lot* in XML parsing
where it obviously shouldn't be. Much of it is probably code written
before the CONFIG_UNSUPPORTED, XML_ERROR, and CONF_SYNTAX codes were
added, but I'd wager just as much is due to people writing new code who
look to the existing code for guidelines (I know I've done this a lot,
and continue to be confused about which code to use in a lot of cases -
heck, this is the first I've noticed CONF_SYNTAX, and I'm not sure when
it would be appropriate to use that vs. XML_ERROR).
4:18 p.m.
On Thu, Oct 06, 2011 at 03:55:48PM -0400, Laine Stump wrote:
On 10/06/2011 01:36 PM, Guido Günther wrote:
>Hi Eric,
>On Thu, Oct 06, 2011 at 10:50:46AM -0600, Eric Blake wrote:
>>
>>These three errors should probably all be changed away from
>>VIR_ERR_INTERNAL_ERROR into something more useful, but I'm not sure
>>whether that would be VIR_ERR_CONF_SYNTAX,
>>VIR_ERR_CONFIG_UNSUPPORTED, or something else.
>I was uncertain about the error codes too, that's why I kept
>VIR_ERR_INTERNAL_ERROR as used by other functions in this file. I'd
>opt for VIR_ERR_CONF_SYNTAX.
We really need to agree on a policy on which code to use for which
type of errors, write it down, then go through the code and correct
everything that's wrong. INTERNAL_ERROR is used *a lot* in XML
parsing where it obviously shouldn't be. Much of it is probably code
written before the CONFIG_UNSUPPORTED, XML_ERROR, and CONF_SYNTAX
codes were added, but I'd wager just as much is due to people
writing new code who look to the existing code for guidelines (I
know I've done this a lot, and continue to be confused about which
code to use in a lot of cases - heck, this is the first I've noticed
CONF_SYNTAX, and I'm not sure when it would be appropriate to use
that vs. XML_ERROR).
We're not parsing XML here but xen configuration so CONF_SYNTAX fits
better. When parsing XML XML_ERROR makes more sense but you're right
that's mostly guessing at the moment.
Cheers,
-- Guido
5:33 p.m.
On 10/06/2011 11:36 AM, Guido Günther wrote:
> These three errors should probably all be changed away from
> VIR_ERR_INTERNAL_ERROR into something more useful, but I'm not sure
> whether that would be VIR_ERR_CONF_SYNTAX,
> VIR_ERR_CONFIG_UNSUPPORTED, or something else.
I was uncertain about the error codes too, that's why I kept
VIR_ERR_INTERNAL_ERROR as used by other functions in this file. I'd
opt for VIR_ERR_CONF_SYNTAX.
-- Guido
Yeah, for this particular case, where we are parsing a xen conf file,
CONF_SYNTAX is the best fit.
>
> ACK to the concept, once we decide on the correct error code.
I think we've decided.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5:18 a.m.
On Thu, Oct 06, 2011 at 04:33:49PM -0600, Eric Blake wrote:
On 10/06/2011 11:36 AM, Guido Günther wrote:
>>These three errors should probably all be changed away from
>>VIR_ERR_INTERNAL_ERROR into something more useful, but I'm not sure
>>whether that would be VIR_ERR_CONF_SYNTAX,
>>VIR_ERR_CONFIG_UNSUPPORTED, or something else.
>
>I was uncertain about the error codes too, that's why I kept
>VIR_ERR_INTERNAL_ERROR as used by other functions in this file. I'd
>opt for VIR_ERR_CONF_SYNTAX.
> -- Guido
Yeah, for this particular case, where we are parsing a xen conf
file, CONF_SYNTAX is the best fit.
>
>>
>>ACK to the concept, once we decide on the correct error code.
I think we've decided.
(Hopefully) final version attached. I've left the
first error as
INTERNAL_ERROR since this it's there do detect false function
parameters. O.k. to apply?
Cheers,
-- Guido
9:04 a.m.
On Fri, Oct 07, 2011 at 12:18:51PM +0200, Guido Günther wrote:
On Thu, Oct 06, 2011 at 04:33:49PM -0600, Eric Blake wrote:
> On 10/06/2011 11:36 AM, Guido Günther wrote:
> >>These three errors should probably all be changed away from
> >>VIR_ERR_INTERNAL_ERROR into something more useful, but I'm not sure
> >>whether that would be VIR_ERR_CONF_SYNTAX,
> >>VIR_ERR_CONFIG_UNSUPPORTED, or something else.
> >
> >I was uncertain about the error codes too, that's why I kept
> >VIR_ERR_INTERNAL_ERROR as used by other functions in this file. I'd
> >opt for VIR_ERR_CONF_SYNTAX.
> > -- Guido
>
> Yeah, for this particular case, where we are parsing a xen conf
> file, CONF_SYNTAX is the best fit.
>
> >
> >>
> >>ACK to the concept, once we decide on the correct error code.
>
> I think we've decided.
(Hopefully) final version attached. I've left the first error as
INTERNAL_ERROR since this it's there do detect false function
parameters. O.k. to apply?
Ping?
-- Guido
Cheers,
-- Guido
>From 573d2a5a575dd551b92e603201bbae7bb5fa1733 Mon Sep 17 00:00:00
2001
From: =?UTF-8?q?Guido=20G=C3=BCnther?= <agx(a)sigxcpu.org>
Date: Thu, 6 Oct 2011 12:42:39 +0200
Subject: [PATCH] xen: add error handling to UUID parsing
otherwise a missing UUID in a domain config just shows:
error: An error occured, but the cause is unknown
Now we have:
error: configuration file syntax error: config value uuid was missing
---
src/xenxs/xen_xm.c | 37 +++++++++++++++++++++++++++----------
1 files changed, 27 insertions(+), 10 deletions(-)
diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c
index 70facf7..ff173d8 100644
--- a/src/xenxs/xen_xm.c
+++ b/src/xenxs/xen_xm.c
@@ -174,21 +174,38 @@ static int xenXMConfigCopyStringOpt(virConfPtr conf,
/* Convenience method to grab a string UUID from the config file object */
static int xenXMConfigGetUUID(virConfPtr conf, const char *name, unsigned char *uuid) {
virConfValuePtr val;
- if (!uuid || !name || !conf)
- return (-1);
+
+ if (!uuid || !name || !conf) {
+ XENXS_ERROR(VIR_ERR_INVALID_ARG,
+ _("Arguments must be non null"));
+ return -1;
+ }
+
if (!(val = virConfGetValue(conf, name))) {
- return (-1);
+ XENXS_ERROR(VIR_ERR_CONF_SYNTAX,
+ _("config value %s was missing"), name);
+ return -1;
}
- if (val->type != VIR_CONF_STRING)
- return (-1);
- if (!val->str)
- return (-1);
+ if (val->type != VIR_CONF_STRING) {
+ XENXS_ERROR(VIR_ERR_CONF_SYNTAX,
+ _("config value %s not a string"), name);
+ return -1;
+ }
+
+ if (!val->str) {
+ XENXS_ERROR(VIR_ERR_CONF_SYNTAX,
+ _("%s can't be empty"), name);
+ return -1;
+ }
- if (virUUIDParse(val->str, uuid) < 0)
- return (-1);
+ if (virUUIDParse(val->str, uuid) < 0) {
+ XENXS_ERROR(VIR_ERR_CONF_SYNTAX,
+ _("%s not parseable"), val->str);
+ return -1;
+ }
- return (0);
+ return 0;
}
#define MAX_VFB 1024
--
1.7.6.3
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
2:23 p.m.
On 10/07/2011 04:18 AM, Guido Günther wrote:
>>> > >>ACK to the concept, once we decide on the
correct error code.
> >
> > I think we've decided.
(Hopefully) final version attached. I've left the first error as
INTERNAL_ERROR since this it's there do detect false function
parameters.
Yes, you got that part right.
Subject: [PATCH] xen: add error handling to UUID parsing
otherwise a missing UUID in a domain config just shows:
error: An error occured, but the cause is unknown
s/occured/occurred/
I don't see that typo in the source code for virterror, so I suspect you
were re-typing instead of copy-and-paste. (That said, I'll be pushing a
trivial patch to fix up the places that _do_ have a typo.)
Now we have:
error: configuration file syntax error: config value uuid was missing
ACK.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4:13 p.m.
On Mon, Oct 10, 2011 at 01:23:20PM -0600, Eric Blake wrote:
On 10/07/2011 04:18 AM, Guido Günther wrote:
>>>>> >>ACK to the concept, once we decide on the correct error
code.
>>>
>>> I think we've decided.
>(Hopefully) final version attached. I've left the first error as
>INTERNAL_ERROR since this it's there do detect false function
>parameters.
Yes, you got that part right.
>Subject: [PATCH] xen: add error handling to UUID parsing
>
>otherwise a missing UUID in a domain config just shows:
>
>error: An error occured, but the cause is unknown
s/occured/occurred/
I don't see that typo in the source code for virterror, so I suspect
you were re-typing instead of copy-and-paste. (That said, I'll be
Yes, the joys of separate networks. Pushed with typos corrected.
Thanks,
-- Guido
pushing a trivial patch to fix up the places that _do_ have a typo.)
>
>Now we have:
>
>error: configuration file syntax error: config value uuid was missing
ACK.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4794
days inactive
4798
days old
9 comments
3 participants
participants (3)
-
Eric Blake -
Guido Günther -
Laine Stump