[libvirt] [PATCH] qemu: Improve some qemu.conf error reporting
Log some info if we can't find a config file. Make parse failures fatal, and actually raise an error message. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_conf.c | 13 +++++++++---- 1 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index ce42bd6..e2f7b31 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -133,16 +133,21 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, /* Just check the file is readable before opening it, otherwise * libvirt emits an error. */ - if (access (filename, R_OK) == -1) return 0; + if (access (filename, R_OK) == -1) { + VIR_INFO("Could not read qemu config file %s", filename); + return 0; + } conf = virConfReadFile (filename, 0); - if (!conf) return 0; + if (!conf) { + return -1; + } #define CHECK_TYPE(name,typ) if (p && p->type != (typ)) { \ qemuReportError(VIR_ERR_INTERNAL_ERROR, \ - "remoteReadConfigFile: %s: %s: expected type " #typ, \ - filename, (name)); \ + "%s: %s: %s: expected type " #typ, \ + __func__, filename, (name)); \ virConfFree(conf); \ return -1; \ } -- 1.6.6.1
On 06/30/2010 02:38 PM, Cole Robinson wrote:
Log some info if we can't find a config file. Make parse failures fatal, and actually raise an error message.
Signed-off-by: Cole Robinson <crobinso@redhat.com>
ACK.
#define CHECK_TYPE(name,typ) if (p && p->type != (typ)) { \ qemuReportError(VIR_ERR_INTERNAL_ERROR, \ - "remoteReadConfigFile: %s: %s: expected type " #typ, \ - filename, (name)); \ + "%s: %s: %s: expected type " #typ, \ + __func__, filename, (name)); \
Hmm, this reminds me of my pending question of whether we should prefer __func__ (C99, and guaranteed by gnulib) or __FUNCTION__ (gcc-only, but easy to wrap on top of __func__): https://www.redhat.com/archives/libvir-list/2010-May/msg00319.html -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 07/01/2010 07:05 AM, Eric Blake wrote: <snip>
Hmm, this reminds me of my pending question of whether we should prefer __func__ (C99, and guaranteed by gnulib) or __FUNCTION__ (gcc-only, but easy to wrap on top of __func__): https://www.redhat.com/archives/libvir-list/2010-May/msg00319.html
Ouch, does this mean we might officially be not supporting compilers other than gcc? ie: Intel's CC http://software.intel.com/en-us/intel-compilers/ Sun (Oracle) Studio http://developers.sun.com/sunstudio/ (etc) It's it's not too late, we should try to allow these other ones to work. The Intel guys in particular seem to be doing a fair amount of optimization stuff with (performance) demanding Open Source projects, so I could see them being involed with libvirt down the track. :) Regards and best wishes, Justin Clift -- Salasaga - Open Source eLearning IDE http://www.salasaga.org
On Thu, Jul 01, 2010 at 11:51:17AM +1000, Justin Clift wrote:
On 07/01/2010 07:05 AM, Eric Blake wrote: <snip>
Hmm, this reminds me of my pending question of whether we should prefer __func__ (C99, and guaranteed by gnulib) or __FUNCTION__ (gcc-only, but easy to wrap on top of __func__): https://www.redhat.com/archives/libvir-list/2010-May/msg00319.html
Ouch, does this mean we might officially be not supporting compilers other than gcc? ie:
No, there is comaptability code /* C99 uses __func__. __FUNCTION__ is legacy. */ # ifndef __GNUC__ # define __FUNCTION__ __func__ # endif Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Wed, Jun 30, 2010 at 04:38:36PM -0400, Cole Robinson wrote:
Log some info if we can't find a config file. Make parse failures fatal, and actually raise an error message.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_conf.c | 13 +++++++++---- 1 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index ce42bd6..e2f7b31 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -133,16 +133,21 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, /* Just check the file is readable before opening it, otherwise * libvirt emits an error. */ - if (access (filename, R_OK) == -1) return 0; + if (access (filename, R_OK) == -1) { + VIR_INFO("Could not read qemu config file %s", filename); + return 0; + }
conf = virConfReadFile (filename, 0); - if (!conf) return 0; + if (!conf) { + return -1; + }
#define CHECK_TYPE(name,typ) if (p && p->type != (typ)) { \ qemuReportError(VIR_ERR_INTERNAL_ERROR, \ - "remoteReadConfigFile: %s: %s: expected type " #typ, \ - filename, (name)); \ + "%s: %s: %s: expected type " #typ, \ + __func__, filename, (name)); \ virConfFree(conf); \ return -1; \
Including the function name in the error message is dubious behaviour, not in line with the rest of libvirt. We should just kill it. For the user only the config filename and parameter are relevant Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 07/01/2010 06:11 AM, Daniel P. Berrange wrote:
On Wed, Jun 30, 2010 at 04:38:36PM -0400, Cole Robinson wrote:
Log some info if we can't find a config file. Make parse failures fatal, and actually raise an error message.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_conf.c | 13 +++++++++---- 1 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index ce42bd6..e2f7b31 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -133,16 +133,21 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, /* Just check the file is readable before opening it, otherwise * libvirt emits an error. */ - if (access (filename, R_OK) == -1) return 0; + if (access (filename, R_OK) == -1) { + VIR_INFO("Could not read qemu config file %s", filename); + return 0; + }
conf = virConfReadFile (filename, 0); - if (!conf) return 0; + if (!conf) { + return -1; + }
#define CHECK_TYPE(name,typ) if (p && p->type != (typ)) { \ qemuReportError(VIR_ERR_INTERNAL_ERROR, \ - "remoteReadConfigFile: %s: %s: expected type " #typ, \ - filename, (name)); \ + "%s: %s: %s: expected type " #typ, \ + __func__, filename, (name)); \ virConfFree(conf); \ return -1; \
Including the function name in the error message is dubious behaviour, not in line with the rest of libvirt. We should just kill it. For the user only the config filename and parameter are relevant
Thanks, I dropped that bit and pushed. - Cole
participants (4)
-
Cole Robinson -
Daniel P. Berrange -
Eric Blake -
Justin Clift