[libvirt] [PATCH 1/2] xen: add error handling to UUID parsing
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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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).
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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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
On Thu, Oct 06, 2011 at 04:33:49PM -0600, Eric Blake wrote: parameters. O.k. to apply? Cheers, -- Guido
On Fri, Oct 07, 2011 at 12:18:51PM +0200, Guido Günther 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
On Thu, Oct 06, 2011 at 04:33:49PM -0600, Eric Blake wrote: 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@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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
Guido Günther -
Laine Stump