12:25 p.m.
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 :|
5:25 a.m.
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.
5:46 a.m.
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 :|
6:56 a.m.
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
2:23 a.m.
Evgeniy Sokolov <evg(a)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)
7:27 a.m.
On Thu, Jul 10, 2008 at 09:23:36AM +0200, Jim Meyering wrote:
Evgeniy Sokolov <evg(a)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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
7:33 a.m.
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(a)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 :|
5982
days inactive
5984
days old
7 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Daniel Veillard -
Evgeniy Sokolov -
Jim Meyering