[libvirt] [PATCH] fix diagnostic when execute openvz commands
On Tue, Jul 08, 2008 at 07:46:52PM +0400, Evgeniy Sokolov wrote:
Index: openvz_driver.c =================================================================== RCS file: /data/cvs/libvirt/src/openvz_driver.c,v retrieving revision 1.23 diff -u -p -r1.23 openvz_driver.c --- openvz_driver.c 7 Jul 2008 11:48:40 -0000 1.23 +++ openvz_driver.c 8 Jul 2008 15:30:16 -0000 @@ -257,7 +257,7 @@ static int openvzDomainShutdown(virDomai
ret = virExec(dom->conn, (char **)cmdExec, &pid, -1, &outfd, &errfd); if(ret == -1) { - error(dom->conn, VIR_ERR_INTERNAL_ERROR, "Could not exec " VZLIST); + error(dom->conn, VIR_ERR_INTERNAL_ERROR, "Could not exec " VZCTL);
This really needs to be changed to pass through gettext, eg error(dom->conn, VIR_ERR_INTERNAL_ERROR, _("Could not exec %s"), VZCTL); NB, we have to use a %s format string when going via gettext rather than just allowing the preprocessor to do string concatenation for us. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
This really needs to be changed to pass through gettext, eg
error(dom->conn, VIR_ERR_INTERNAL_ERROR, _("Could not exec %s"), VZCTL);
NB, we have to use a %s format string when going via gettext rather than just allowing the preprocessor to do string concatenation for us.
Ok, it need. Thanks! fixed patch is attached.
On Wed, Jul 09, 2008 at 02:25:10PM +0400, Evgeniy Sokolov wrote:
This really needs to be changed to pass through gettext, eg
error(dom->conn, VIR_ERR_INTERNAL_ERROR, _("Could not exec %s"), VZCTL);
NB, we have to use a %s format string when going via gettext rather than just allowing the preprocessor to do string concatenation for us.
Ok, it need. Thanks!
fixed patch is attached.
ACK, this looks good to me now. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.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, Jul 09, 2008 at 02:25:10PM +0400, Evgeniy Sokolov wrote:
This really needs to be changed to pass through gettext, eg
error(dom->conn, VIR_ERR_INTERNAL_ERROR, _("Could not exec %s"), VZCTL);
NB, we have to use a %s format string when going via gettext rather than just allowing the preprocessor to do string concatenation for us.
Ok, it need. Thanks!
fixed patch is attached.
Okidoc, applied and commited to CVS, thanks ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Evgeniy Sokolov <evg@openvz.org> wrote:
This really needs to be changed to pass through gettext, eg
error(dom->conn, VIR_ERR_INTERNAL_ERROR, _("Could not exec %s"), VZCTL);
NB, we have to use a %s format string when going via gettext rather than just allowing the preprocessor to do string concatenation for us.
Ok, it need. Thanks!
Index: openvz_conf.c =================================================================== ... -/* For errors internal to this library. */ -static void -error (virConnectPtr conn, virErrorNumber code, const char *info) +void +error (virConnectPtr conn, virErrorNumber code, const char *fmt, ...)
Now that you've given it external scope, please use a different name. There are already at least two other "error" functions, including the static one in remote_internal.c, and the external one in glibc/gnulib. ...
Index: openvz_conf.h =================================================================== RCS file: /data/cvs/libvirt/src/openvz_conf.h,v retrieving revision 1.6 diff -u -p -r1.6 openvz_conf.h --- openvz_conf.h 5 Feb 2008 19:27:37 -0000 1.6 +++ openvz_conf.h 9 Jul 2008 10:08:49 -0000 @@ -110,6 +110,7 @@ openvzIsActiveVM(struct openvz_vm *vm) return vm->vpsid != -1; }
+void error (virConnectPtr conn, virErrorNumber code, const char *fmt, ...);
Whatever its name, please do two more things: - add "ATTRIBUTE_FORMAT(printf, 3, 4)" before the semicolon, so that gcc -Wformat compares the format string with types of corresponding arguments. (this must be done with any varargs "..." printf/scanf-style function) - add the new name to the list in Makefile.cfg (msg_gen_function), and ensure that "make syntax-check" passes. (this must be done for any function that takes a diagnostic string as an argument)
On Thu, Jul 10, 2008 at 09:23:36AM +0200, Jim Meyering wrote:
Evgeniy Sokolov <evg@openvz.org> wrote: [...]
+void +error (virConnectPtr conn, virErrorNumber code, const char *fmt, ...)
Now that you've given it external scope, please use a different name. There are already at least two other "error" functions, including the static one in remote_internal.c, and the external one in glibc/gnulib.
Hum, maybe we could just rename the one in remote_internal.c to rem_error() to avoid confusion. [...]
+void error (virConnectPtr conn, virErrorNumber code, const char *fmt, ...);
Whatever its name, please do two more things:
- add "ATTRIBUTE_FORMAT(printf, 3, 4)" before the semicolon, so that gcc -Wformat compares the format string with types of corresponding arguments. (this must be done with any varargs "..." printf/scanf-style function)
- add the new name to the list in Makefile.cfg (msg_gen_function), and ensure that "make syntax-check" passes. (this must be done for any function that takes a diagnostic string as an argument)
Everything seems fixed with his latest patch :-) thanks I completely missed those points ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Thu, Jul 10, 2008 at 08:27:25AM -0400, Daniel Veillard wrote:
On Thu, Jul 10, 2008 at 09:23:36AM +0200, Jim Meyering wrote:
Evgeniy Sokolov <evg@openvz.org> wrote: [...]
+void +error (virConnectPtr conn, virErrorNumber code, const char *fmt, ...)
Now that you've given it external scope, please use a different name. There are already at least two other "error" functions, including the static one in remote_internal.c, and the external one in glibc/gnulib.
Hum, maybe we could just rename the one in remote_internal.c to rem_error() to avoid confusion.
The fact that every single driver duplicates the same code to wrap __virRaiseError suggests we need simpler shared version to me. The only thing that differs is the VIR_FROM_QEMU, FROM_XEN, etc, param I'd suggest we add a __virRaiseSimpleError(virConnectPtr conn, int from, int code, const cha *fmt, ...) { va_list args; char errorMessage[1024]; const char *virerr; if (fmt) { va_start(args, fmt); vsnprintf(errorMessage, sizeof(errorMessage)-1, fmt, args); va_end(args); } else { errorMessage[0] = '\0'; } virerr = __virErrorMsg(code, (errorMessage[0] ? errorMessage : NULL)); __virRaiseError(conn, dom, net, from, code, VIR_ERR_ERROR, virerr, errorMessage, NULL, -1, -1, virerr, errorMessage); } and then each driver can 'wrap' it with a macro #define ERROR(conn, code, fmt, ...) \ __virRaiseeSimpleError((conn), VIR_FROM_QEMU, (code), (fmt), __VA_ARGS__) Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (4)
-
Daniel P. Berrange -
Daniel Veillard -
Evgeniy Sokolov -
Jim Meyering