Re: [libvirt] [PATCH] Be consistent with setlocale error handling
On 04/12/2016 10:10 AM, Daniel P. Berrange wrote:
On Tue, Apr 12, 2016 at 10:00:48AM -0400, Cole Robinson wrote:
> Take setlocale/gettext error handling pattern from tools/virsh-*
> and use it in all the other standalone binaries. The changes are
>
> * Ignore setlocale errors. virsh has done this forever, presumably for
> good reason. This has been partially responsible for some bug reports:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1312688
> https://bugzilla.redhat.com/show_bug.cgi?id=1026514
> https://bugzilla.redhat.com/show_bug.cgi?id=1016158
>
> * Report the failed function name
> * Report strerror
> ---
> daemon/libvirtd.c | 20 ++++++++++++++++----
> src/locking/lock_daemon.c | 20 ++++++++++++++++----
> src/locking/sanlock_helper.c | 16 ++++++++++++----
> src/logging/log_daemon.c | 20 ++++++++++++++++----
> src/lxc/lxc_controller.c | 20 ++++++++++++++++----
> src/network/leaseshelper.c | 16 ++++++++++++----
> src/security/virt-aa-helper.c | 16 ++++++++++++----
> src/storage/parthelper.c | 16 ++++++++++++----
> src/util/iohelper.c | 16 ++++++++++++----
> 9 files changed, 124 insertions(+), 36 deletions(-)
>
> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
> index 3d38a46..9488950 100644
> --- a/daemon/libvirtd.c
> +++ b/daemon/libvirtd.c
> @@ -1172,10 +1172,22 @@ int main(int argc, char **argv) {
> {0, 0, 0, 0}
> };
>
> - if (setlocale(LC_ALL, "") == NULL ||
> - bindtextdomain(PACKAGE, LOCALEDIR) == NULL ||
> - textdomain(PACKAGE) == NULL ||
> - virInitialize() < 0) {
> + if (!setlocale(LC_ALL, "")) {
> + perror("setlocale");
> + /* failure to setup locale is not fatal */
> + }
> +
> + if (!bindtextdomain(PACKAGE, LOCALEDIR)) {
> + perror("bindtextdomain");
> + exit(EXIT_FAILURE);
> + }
> +
> + if (!textdomain(PACKAGE)) {
> + perror("textdomain");
> + exit(EXIT_FAILURE);
> + }
> +
> + if (virInitialize() < 0) {
> fprintf(stderr, _("%s: initialization failed\n"), argv[0]);
> exit(EXIT_FAILURE);
> }
Instead of repeating this, how about we add src/util/virgettext.h and
then have
int virGettextInit(const char *package, const char *localedir) {
if (!setlocale(LC_ALL, "")) {
perror("setlocale");
/* failure to setup locale is not fatal */
}
if (!bindtextdomain(PACKAGE, LOCALEDIR)) {
perror("bindtextdomain");
return -1;
}
if (!textdomain(PACKAGE)) {
perror("textdomain");
return -1;
}
return 0;
}
And in each app we can just do
if (virGettextInit(PACKAGE, LOCALEDIR) < 0)
exit(EXIT_FAILURE);
Regards,
Daniel
Good idea, I'll send a v2
Thanks,
Cole