[RFC PATCH 0/8] Translatable string handling improvements
Many things in libvirt attempt to construct error messages from various sub-strings which is not translation friednly. The beginning of the series refactors the cgroup code to avoid this practice. The end of this series is an example how the worst offender, virsh event handling code, can be improved to avoid the bad practice. I've found most cases using: git grep ' _([^"]' Unfortunateluy the better practice is a bit more verbose, thus RFC. Peter Krempa (8): virCgroupV1GetBlkioIo(Device)Serviced: Refactor extraction of cgroup data Don't translate strings used with VIR_DEBUG virshGraphicsAddressToString: Remove pointless translation vshPrint: Add version using 'va_list' virsh: event: Introduce virshEventPrintf virsh-domain-event: Make 'virshEventTrayChangePrint' translation friendly virsh-domain-event: Make 'virshEventWatchdogPrint' translation friendly virsh-domain-event: Make 'virshEventIOError(Reason)Print' translation friendly src/util/vircgroupv1.c | 264 ++++++++++++++++--------------------- src/util/virnetlink.c | 6 +- src/vz/vz_driver.c | 4 +- tools/virsh-domain-event.c | 223 +++++++++++++++++-------------- tools/vsh.c | 32 +++-- tools/vsh.h | 4 + 6 files changed, 274 insertions(+), 259 deletions(-) -- 2.39.2
Rewrite the code to improve maintainability and also re-do construction of error messages which are assembled from non-translatable parts. Closes: https://gitlab.com/libvirt/libvirt/-/issues/455 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/vircgroupv1.c | 264 ++++++++++++++++++----------------------- 1 file changed, 117 insertions(+), 147 deletions(-) diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c index c0cac2a6b6..77c7e209ce 100644 --- a/src/util/vircgroupv1.c +++ b/src/util/vircgroupv1.c @@ -1051,189 +1051,159 @@ virCgroupV1GetBlkioWeight(virCgroup *group, } + static int -virCgroupV1GetBlkioIoServiced(virCgroup *group, - long long *bytes_read, - long long *bytes_write, - long long *requests_read, - long long *requests_write) +virCgroupV1GetBlkioIoServicedOne(virCgroup *group, + const char *field, + const char *devpath, + long long *dataRead, + long long *dataWrite) { - long long stats_val; - g_autofree char *str1 = NULL; - g_autofree char *str2 = NULL; - char *p1 = NULL; - char *p2 = NULL; - size_t i; + g_autofree char *serviced = NULL; + g_autofree char *filterWrite = NULL; + g_autofree char *filterRead = NULL; + unsigned long long tmpval; + char *tmp; + size_t len; - const char *value_names[] = { - "Read ", - "Write " - }; - long long *bytes_ptrs[] = { - bytes_read, - bytes_write - }; - long long *requests_ptrs[] = { - requests_read, - requests_write - }; - - *bytes_read = 0; - *bytes_write = 0; - *requests_read = 0; - *requests_write = 0; - - if (virCgroupGetValueStr(group, - VIR_CGROUP_CONTROLLER_BLKIO, - "blkio.throttle.io_service_bytes", &str1) < 0) - return -1; + *dataRead = 0; + *dataWrite = 0; - if (virCgroupGetValueStr(group, - VIR_CGROUP_CONTROLLER_BLKIO, - "blkio.throttle.io_serviced", &str2) < 0) + if (virCgroupGetValueStr(group, VIR_CGROUP_CONTROLLER_BLKIO, field, &serviced) < 0) return -1; - /* sum up all entries of the same kind, from all devices */ - for (i = 0; i < G_N_ELEMENTS(value_names); i++) { - p1 = str1; - p2 = str2; - - while ((p1 = strstr(p1, value_names[i]))) { - p1 += strlen(value_names[i]); - if (virStrToLong_ll(p1, &p1, 10, &stats_val) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Cannot parse byte %1$sstat '%2$s'"), - value_names[i], - p1); - return -1; - } + if (devpath) { + g_autofree char *devstr = NULL; - if (stats_val < 0 || - (stats_val > 0 && *bytes_ptrs[i] > (LLONG_MAX - stats_val))) - { - virReportError(VIR_ERR_OVERFLOW, - _("Sum of byte %1$sstat overflows"), - value_names[i]); - return -1; - } - *bytes_ptrs[i] += stats_val; - } + if (!(devstr = virCgroupGetBlockDevString(devpath))) + return -1; - while ((p2 = strstr(p2, value_names[i]))) { - p2 += strlen(value_names[i]); - if (virStrToLong_ll(p2, &p2, 10, &stats_val) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Cannot parse %1$srequest stat '%2$s'"), - value_names[i], - p2); - return -1; - } + filterWrite = g_strdup_printf("%sWrite ", devstr); + filterRead = g_strdup_printf("%sRead ", devstr); - if (stats_val < 0 || - (stats_val > 0 && *requests_ptrs[i] > (LLONG_MAX - stats_val))) - { - virReportError(VIR_ERR_OVERFLOW, - _("Sum of %1$srequest stat overflows"), - value_names[i]); - return -1; - } - *requests_ptrs[i] += stats_val; + if (!strstr(serviced, filterWrite) || + !strstr(serviced, filterRead)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot find blkio cgroup stats (%1$s) for block device '%2$s' (%3$s)"), + field, devstr, devpath); + return -1; } + } else { + filterWrite = g_strdup("Write "); + filterRead = g_strdup("Read "); } - return 0; -} - - -static int -virCgroupV1GetBlkioIoDeviceServiced(virCgroup *group, - const char *path, - long long *bytes_read, - long long *bytes_write, - long long *requests_read, - long long *requests_write) -{ - g_autofree char *str1 = NULL; - g_autofree char *str2 = NULL; - g_autofree char *str3 = NULL; - char *p1 = NULL; - char *p2 = NULL; - size_t i; - - const char *value_names[] = { - "Read ", - "Write " - }; - long long *bytes_ptrs[] = { - bytes_read, - bytes_write - }; - long long *requests_ptrs[] = { - requests_read, - requests_write - }; - - if (virCgroupGetValueStr(group, - VIR_CGROUP_CONTROLLER_BLKIO, - "blkio.throttle.io_service_bytes", &str1) < 0) - return -1; + len = strlen(filterRead); - if (virCgroupGetValueStr(group, - VIR_CGROUP_CONTROLLER_BLKIO, - "blkio.throttle.io_serviced", &str2) < 0) - return -1; + for (tmp = strstr(serviced, filterRead); tmp; tmp = strstr(tmp, filterRead)) { + char *cur = tmp; + tmp += len; - if (!(str3 = virCgroupGetBlockDevString(path))) - return -1; + VIR_DEBUG("filter='%s' line='%.*s'", filterRead, (int) (strchr(tmp, '\n') - tmp), tmp); - if (!(p1 = strstr(str1, str3))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Cannot find byte stats for block device '%1$s'"), - str3); - return -1; - } + if (virStrToLong_ullp(tmp, &tmp, 10, &tmpval) < 0) { + char *eol; - if (!(p2 = strstr(str2, str3))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Cannot find request stats for block device '%1$s'"), - str3); - return -1; - } + if ((eol = strchr(cur, '\n'))) + *eol = '\0'; - for (i = 0; i < G_N_ELEMENTS(value_names); i++) { - if (!(p1 = strstr(p1, value_names[i]))) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Cannot find byte %1$sstats for block device '%2$s'"), - value_names[i], str3); + _("Cannot parse blkio cgroup (%1$s) entry '%2$s'"), + field, cur); return -1; } - if (virStrToLong_ll(p1 + strlen(value_names[i]), &p1, 10, bytes_ptrs[i]) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Cannot parse %1$sstat '%2$s'"), - value_names[i], p1 + strlen(value_names[i])); + if (tmpval + *dataRead > LLONG_MAX) { + virReportError(VIR_ERR_OVERFLOW, + _("overflow in sum of statistic for blkio cgroup (%1$s) field '%2$s'"), + field, filterRead); return -1; } - if (!(p2 = strstr(p2, value_names[i]))) { + *dataRead += tmpval; + } + + len = strlen(filterWrite); + + for (tmp = strstr(serviced, filterWrite); tmp; tmp = strstr(tmp, filterWrite)) { + char *cur = tmp; + tmp += len; + + VIR_DEBUG("filter='%s' line='%.*s'", filterWrite, (int) (strchr(cur, '\n') - cur), cur); + + if (virStrToLong_ullp(tmp, &tmp, 10, &tmpval) < 0) { + char *eol; + + if ((eol = strchr(cur, '\n'))) + *eol = '\0'; + virReportError(VIR_ERR_INTERNAL_ERROR, - _("Cannot find request %1$sstats for block device '%2$s'"), - value_names[i], str3); + _("Cannot parse blkio cgroup ('%1$s') entry '%2$s'"), + field, cur); return -1; } - if (virStrToLong_ll(p2 + strlen(value_names[i]), &p2, 10, requests_ptrs[i]) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Cannot parse %1$sstat '%2$s'"), - value_names[i], p2 + strlen(value_names[i])); + if (tmpval + *dataWrite > LLONG_MAX) { + virReportError(VIR_ERR_OVERFLOW, + _("overflow in sum of statistic for blkio cgroup (%1$s) field '%2$s'"), + field, filterWrite); return -1; } + + *dataWrite += tmpval; } return 0; } +static int +virCgroupV1GetBlkioIoServicedInternal(virCgroup *group, + const char *devpath, + long long *bytes_read, + long long *bytes_write, + long long *requests_read, + long long *requests_write) +{ + if (virCgroupV1GetBlkioIoServicedOne(group, "blkio.throttle.io_service_bytes", + devpath, bytes_read, bytes_write) < 0) + return -1; + + if (virCgroupV1GetBlkioIoServicedOne(group, "blkio.throttle.io_serviced", + devpath, requests_read, requests_write) < 0) + return -1; + + return 0; +} + + +static int +virCgroupV1GetBlkioIoServiced(virCgroup *group, + long long *bytes_read, + long long *bytes_write, + long long *requests_read, + long long *requests_write) +{ + return virCgroupV1GetBlkioIoServicedInternal(group, NULL, + bytes_read, bytes_write, + requests_read, requests_write); +} + + +static int +virCgroupV1GetBlkioIoDeviceServiced(virCgroup *group, + const char *path, + long long *bytes_read, + long long *bytes_write, + long long *requests_read, + long long *requests_write) +{ + return virCgroupV1GetBlkioIoServicedInternal(group, path, + bytes_read, bytes_write, + requests_read, requests_write); +} + + static int virCgroupV1SetBlkioDeviceWeight(virCgroup *group, const char *devPath, -- 2.39.2
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virnetlink.c | 6 +++--- src/vz/vz_driver.c | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index f18a08c277..866f4d8f2b 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -1300,7 +1300,7 @@ virNetlinkGetNeighbor(void **nlData G_GNUC_UNUSED, */ int virNetlinkEventServiceStop(unsigned int protocol G_GNUC_UNUSED) { - VIR_DEBUG("%s", _(unsupported)); + VIR_DEBUG("%s", unsupported); return 0; } @@ -1310,7 +1310,7 @@ int virNetlinkEventServiceStop(unsigned int protocol G_GNUC_UNUSED) */ int virNetlinkEventServiceStopAll(void) { - VIR_DEBUG("%s", _(unsupported)); + VIR_DEBUG("%s", unsupported); return 0; } @@ -1321,7 +1321,7 @@ int virNetlinkEventServiceStopAll(void) int virNetlinkEventServiceStart(unsigned int protocol G_GNUC_UNUSED, unsigned int groups G_GNUC_UNUSED) { - VIR_DEBUG("%s", _(unsupported)); + VIR_DEBUG("%s", unsupported); return 0; } diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 56dc236233..401ca041ed 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -4101,7 +4101,7 @@ vzStateInitialize(bool privileged, return VIR_DRV_STATE_INIT_ERROR; if (prlsdkInit() < 0) { - VIR_DEBUG("%s", _("Can't initialize Parallels SDK")); + VIR_DEBUG("Can't initialize Parallels SDK"); return VIR_DRV_STATE_INIT_ERROR; } @@ -4144,7 +4144,7 @@ vzRegister(void) prlctl_path = virFindFileInPath(PRLCTL); if (!prlctl_path) { - VIR_DEBUG("%s", _("Can't find prlctl command in the PATH env")); + VIR_DEBUG("Can't find prlctl command in the PATH env"); return 0; } -- 2.39.2
On a Thursday in 2023, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virnetlink.c | 6 +++--- src/vz/vz_driver.c | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
There's no point in marking the protocol name as translatable. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain-event.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tools/virsh-domain-event.c b/tools/virsh-domain-event.c index 2ad4573ee9..1ed204fa9a 100644 --- a/tools/virsh-domain-event.c +++ b/tools/virsh-domain-event.c @@ -211,15 +211,19 @@ virshGraphicsPhaseToString(int phase) VIR_ENUM_DECL(virshGraphicsAddress); VIR_ENUM_IMPL(virshGraphicsAddress, VIR_DOMAIN_EVENT_GRAPHICS_ADDRESS_LAST, - N_("IPv4"), - N_("IPv6"), - N_("unix")); + "IPv4", + "IPv6", + "unix"); static const char * virshGraphicsAddressToString(int family) { const char *str = virshGraphicsAddressTypeToString(family); - return str ? _(str) : _("unknown"); + + if (str) + return str; + + return _("unknown"); } VIR_ENUM_DECL(virshDomainBlockJobStatus); -- 2.39.2
On a Thursday in 2023, Peter Krempa wrote:
There's no point in marking the protocol name as translatable.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain-event.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/tools/virsh-domain-event.c b/tools/virsh-domain-event.c index 2ad4573ee9..1ed204fa9a 100644 --- a/tools/virsh-domain-event.c +++ b/tools/virsh-domain-event.c @@ -211,15 +211,19 @@ virshGraphicsPhaseToString(int phase) VIR_ENUM_DECL(virshGraphicsAddress); VIR_ENUM_IMPL(virshGraphicsAddress, VIR_DOMAIN_EVENT_GRAPHICS_ADDRESS_LAST, - N_("IPv4"), - N_("IPv6"), - N_("unix")); + "IPv4", + "IPv6", + "unix");
"unix" is actually translated in our Korean translation. Jano
On Thu, Apr 13, 2023 at 11:23:18 +0200, Ján Tomko wrote:
On a Thursday in 2023, Peter Krempa wrote:
There's no point in marking the protocol name as translatable.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain-event.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/tools/virsh-domain-event.c b/tools/virsh-domain-event.c index 2ad4573ee9..1ed204fa9a 100644 --- a/tools/virsh-domain-event.c +++ b/tools/virsh-domain-event.c @@ -211,15 +211,19 @@ virshGraphicsPhaseToString(int phase) VIR_ENUM_DECL(virshGraphicsAddress); VIR_ENUM_IMPL(virshGraphicsAddress, VIR_DOMAIN_EVENT_GRAPHICS_ADDRESS_LAST, - N_("IPv4"), - N_("IPv6"), - N_("unix")); + "IPv4", + "IPv6", + "unix");
"unix" is actually translated in our Korean translation.
I'm fairly sure that's wrong then since this is referring to the protocol.
Add a version for functions which may already need to take a printf format string. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/vsh.c | 32 ++++++++++++++++++++++---------- tools/vsh.h | 4 ++++ 2 files changed, 26 insertions(+), 10 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index 5b672b8edf..41f55a91fa 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -1865,34 +1865,46 @@ vshDebug(vshControl *ctl, int level, const char *format, ...) fflush(stdout); } + void -vshPrintExtra(vshControl *ctl, const char *format, ...) +vshPrintVa(vshControl *ctl G_GNUC_UNUSED, + const char *format, + va_list ap) { - va_list ap; g_autofree char *str = NULL; + str = g_strdup_vprintf(format, ap); + fputs(str, stdout); + fflush(stdout); +} + + +void +vshPrintExtra(vshControl *ctl, + const char *format, + ...) +{ + va_list ap; + if (ctl && ctl->quiet) return; va_start(ap, format); - str = g_strdup_vprintf(format, ap); + vshPrintVa(ctl, format, ap); va_end(ap); - fputs(str, stdout); - fflush(stdout); } void -vshPrint(vshControl *ctl G_GNUC_UNUSED, const char *format, ...) +vshPrint(vshControl *ctl, + const char *format, + ...) { va_list ap; - g_autofree char *str = NULL; va_start(ap, format); - str = g_strdup_vprintf(format, ap); + vshPrintVa(ctl, format, ap); va_end(ap); - fputs(str, stdout); - fflush(stdout); } diff --git a/tools/vsh.h b/tools/vsh.h index 51f09cd2b0..377c5947c1 100644 --- a/tools/vsh.h +++ b/tools/vsh.h @@ -302,6 +302,10 @@ const vshCmdOpt *vshCommandOptArgv(vshControl *ctl, const vshCmd *cmd, bool vshCommandArgvParse(vshControl *ctl, int nargs, char **argv); int vshCommandOptTimeoutToMs(vshControl *ctl, const vshCmd *cmd, int *timeout); +void vshPrintVa(vshControl *ctl, + const char *format, + va_list ap) + G_GNUC_PRINTF(2, 0); void vshPrint(vshControl *ctl, const char *format, ...) G_GNUC_PRINTF(2, 3); void vshPrintExtra(vshControl *ctl, const char *format, ...) -- 2.39.2
Extract internals of virshEventPrint into a function that can take the format string. The function will be used in upcoming patches which make the event formatting translatable. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain-event.c | 46 ++++++++++++++++++++++++-------------- 1 file changed, 29 insertions(+), 17 deletions(-) diff --git a/tools/virsh-domain-event.c b/tools/virsh-domain-event.c index 1ed204fa9a..a8a321590c 100644 --- a/tools/virsh-domain-event.c +++ b/tools/virsh-domain-event.c @@ -286,6 +286,33 @@ struct virshDomEventData { }; typedef struct virshDomEventData virshDomEventData; + +static void G_GNUC_PRINTF(2, 3) +virshEventPrintf(virshDomEventData *data, + const char *fmt, + ...) +{ + va_list ap; + + if (!data->loop && *data->count) + return; + + if (data->timestamp) { + char timestamp[VIR_TIME_STRING_BUFLEN] = ""; + + ignore_value(virTimeStringNowRaw(timestamp)); + vshPrint(data->ctl, "%s: ", timestamp); + } + + va_start(ap, fmt); + vshPrintVa(data->ctl, fmt, ap); + va_end(ap); + + (*data->count)++; + if (!data->loop) + vshEventDone(data->ctl); +} + /** * virshEventPrint: * @@ -305,25 +332,10 @@ virshEventPrint(virshDomEventData *data, if (!(msg = virBufferContentAndReset(buf))) return; - if (!data->loop && *data->count) - return; - - if (data->timestamp) { - char timestamp[VIR_TIME_STRING_BUFLEN]; - - if (virTimeStringNowRaw(timestamp) < 0) - timestamp[0] = '\0'; - - vshPrint(data->ctl, "%s: %s", timestamp, msg); - } else { - vshPrint(data->ctl, "%s", msg); - } - - (*data->count)++; - if (!data->loop) - vshEventDone(data->ctl); + virshEventPrintf(data, "%s", msg); } + static void virshEventGenericPrint(virConnectPtr conn G_GNUC_UNUSED, virDomainPtr dom, -- 2.39.2
Remove construction of the event string from sub-strings marked as translatable. Without context it's impossible to translate it correctly. This slightly increases verbosity of the code but actually makes it more readable as everything is inline. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain-event.c | 36 ++++++++++++++++-------------------- 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/tools/virsh-domain-event.c b/tools/virsh-domain-event.c index a8a321590c..c522d79dd7 100644 --- a/tools/virsh-domain-event.c +++ b/tools/virsh-domain-event.c @@ -254,20 +254,6 @@ virshDomainEventDiskChangeToString(int reason) return str ? _(str) : _("unknown"); } -VIR_ENUM_DECL(virshDomainEventTrayChange); -VIR_ENUM_IMPL(virshDomainEventTrayChange, - VIR_DOMAIN_EVENT_TRAY_CHANGE_LAST, - N_("opened"), - N_("closed")); - -static const char * -virshDomainEventTrayChangeToString(int reason) -{ - const char *str = virshDomainEventTrayChangeTypeToString(reason); - return str ? _(str) : _("unknown"); -} - - struct virshDomainEventCallback { const char *name; virConnectDomainEventGenericCallback cb; @@ -511,13 +497,23 @@ virshEventTrayChangePrint(virConnectPtr conn G_GNUC_UNUSED, int reason, void *opaque) { - g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; + switch ((virDomainEventTrayChangeReason) reason) { + case VIR_DOMAIN_EVENT_TRAY_CHANGE_OPEN: + virshEventPrintf(opaque, _("event 'tray-change' for domain '%1$s' disk %2$s: opened\n"), + virDomainGetName(dom), alias); + break; - virBufferAsprintf(&buf, _("event 'tray-change' for domain '%1$s' disk %2$s: %3$s\n"), - virDomainGetName(dom), - alias, - virshDomainEventTrayChangeToString(reason)); - virshEventPrint(opaque, &buf); + case VIR_DOMAIN_EVENT_TRAY_CHANGE_CLOSE: + virshEventPrintf(opaque, _("event 'tray-change' for domain '%1$s' disk %2$s: closed\n"), + virDomainGetName(dom), alias); + break; + + case VIR_DOMAIN_EVENT_TRAY_CHANGE_LAST: + default: + virshEventPrintf(opaque, _("event 'tray-change' for domain '%1$s' disk %2$s: unknown\n"), + virDomainGetName(dom), alias); + break; + } } static void -- 2.39.2
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain-event.c | 59 ++++++++++++++++++++++---------------- 1 file changed, 35 insertions(+), 24 deletions(-) diff --git a/tools/virsh-domain-event.c b/tools/virsh-domain-event.c index c522d79dd7..8656b0fa09 100644 --- a/tools/virsh-domain-event.c +++ b/tools/virsh-domain-event.c @@ -162,24 +162,6 @@ virshDomainEventDetailToString(int event, int detail) return str ? _(str) : _("unknown"); } -VIR_ENUM_DECL(virshDomainEventWatchdog); -VIR_ENUM_IMPL(virshDomainEventWatchdog, - VIR_DOMAIN_EVENT_WATCHDOG_LAST, - N_("none"), - N_("pause"), - N_("reset"), - N_("poweroff"), - N_("shutdown"), - N_("debug"), - N_("inject-nmi")); - -static const char * -virshDomainEventWatchdogToString(int action) -{ - const char *str = virshDomainEventWatchdogTypeToString(action); - return str ? _(str) : _("unknown"); -} - VIR_ENUM_DECL(virshDomainEventIOError); VIR_ENUM_IMPL(virshDomainEventIOError, VIR_DOMAIN_EVENT_IO_ERROR_LAST, @@ -371,12 +353,41 @@ virshEventWatchdogPrint(virConnectPtr conn G_GNUC_UNUSED, int action, void *opaque) { - g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; - - virBufferAsprintf(&buf, _("event 'watchdog' for domain '%1$s': %2$s\n"), - virDomainGetName(dom), - virshDomainEventWatchdogToString(action)); - virshEventPrint(opaque, &buf); + switch ((virDomainEventWatchdogAction) action) { + case VIR_DOMAIN_EVENT_WATCHDOG_NONE: + virshEventPrintf(opaque, _("event 'watchdog' for domain '%1$s': none\n"), + virDomainGetName(dom)); + break; + case VIR_DOMAIN_EVENT_WATCHDOG_PAUSE: + virshEventPrintf(opaque, _("event 'watchdog' for domain '%1$s': pause\n"), + virDomainGetName(dom)); + break; + case VIR_DOMAIN_EVENT_WATCHDOG_RESET: + virshEventPrintf(opaque, _("event 'watchdog' for domain '%1$s': reset\n"), + virDomainGetName(dom)); + break; + case VIR_DOMAIN_EVENT_WATCHDOG_POWEROFF: + virshEventPrintf(opaque, _("event 'watchdog' for domain '%1$s': poweroff\n"), + virDomainGetName(dom)); + break; + case VIR_DOMAIN_EVENT_WATCHDOG_SHUTDOWN: + virshEventPrintf(opaque, _("event 'watchdog' for domain '%1$s': shutdown\n"), + virDomainGetName(dom)); + break; + case VIR_DOMAIN_EVENT_WATCHDOG_DEBUG: + virshEventPrintf(opaque, _("event 'watchdog' for domain '%1$s': debug\n"), + virDomainGetName(dom)); + break; + case VIR_DOMAIN_EVENT_WATCHDOG_INJECTNMI: + virshEventPrintf(opaque, _("event 'watchdog' for domain '%1$s': inject-nmi\n"), + virDomainGetName(dom)); + break; + case VIR_DOMAIN_EVENT_WATCHDOG_LAST: + default: + virshEventPrintf(opaque, _("event 'watchdog' for domain '%1$s': unknown\n"), + virDomainGetName(dom)); + break; + } } static void -- 2.39.2
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain-event.c | 70 +++++++++++++++++++++----------------- 1 file changed, 38 insertions(+), 32 deletions(-) diff --git a/tools/virsh-domain-event.c b/tools/virsh-domain-event.c index 8656b0fa09..2969c22a91 100644 --- a/tools/virsh-domain-event.c +++ b/tools/virsh-domain-event.c @@ -162,20 +162,6 @@ virshDomainEventDetailToString(int event, int detail) return str ? _(str) : _("unknown"); } -VIR_ENUM_DECL(virshDomainEventIOError); -VIR_ENUM_IMPL(virshDomainEventIOError, - VIR_DOMAIN_EVENT_IO_ERROR_LAST, - N_("none"), - N_("pause"), - N_("report")); - -static const char * -virshDomainEventIOErrorToString(int action) -{ - const char *str = virshDomainEventIOErrorTypeToString(action); - return str ? _(str) : _("unknown"); -} - VIR_ENUM_DECL(virshGraphicsPhase); VIR_ENUM_IMPL(virshGraphicsPhase, VIR_DOMAIN_EVENT_GRAPHICS_LAST, @@ -398,14 +384,25 @@ virshEventIOErrorPrint(virConnectPtr conn G_GNUC_UNUSED, int action, void *opaque) { - g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; - - virBufferAsprintf(&buf, _("event 'io-error' for domain '%1$s': %2$s (%3$s) %4$s\n"), - virDomainGetName(dom), - srcPath, - devAlias, - virshDomainEventIOErrorToString(action)); - virshEventPrint(opaque, &buf); + switch ((virDomainEventIOErrorAction) action) { + case VIR_DOMAIN_EVENT_IO_ERROR_NONE: + virshEventPrintf(opaque, _("event 'io-error' for domain '%1$s': %2$s (%3$s) none\n"), + virDomainGetName(dom), srcPath, devAlias); + break; + case VIR_DOMAIN_EVENT_IO_ERROR_PAUSE: + virshEventPrintf(opaque, _("event 'io-error' for domain '%1$s': %2$s (%3$s) pause\n"), + virDomainGetName(dom), srcPath, devAlias); + break; + case VIR_DOMAIN_EVENT_IO_ERROR_REPORT: + virshEventPrintf(opaque, _("event 'io-error' for domain '%1$s': %2$s (%3$s) report\n"), + virDomainGetName(dom), srcPath, devAlias); + break; + case VIR_DOMAIN_EVENT_IO_ERROR_LAST: + default: + virshEventPrintf(opaque, _("event 'io-error' for domain '%1$s': %2$s (%3$s) unknown\n"), + virDomainGetName(dom), srcPath, devAlias); + break; + } } static void @@ -449,16 +446,25 @@ virshEventIOErrorReasonPrint(virConnectPtr conn G_GNUC_UNUSED, const char *reason, void *opaque) { - g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; - - virBufferAsprintf(&buf, _("event 'io-error-reason' for domain '%1$s': " - "%2$s (%3$s) %4$s due to %5$s\n"), - virDomainGetName(dom), - srcPath, - devAlias, - virshDomainEventIOErrorToString(action), - reason); - virshEventPrint(opaque, &buf); + switch ((virDomainEventIOErrorAction) action) { + case VIR_DOMAIN_EVENT_IO_ERROR_NONE: + virshEventPrintf(opaque, _("event 'io-error' for domain '%1$s': %2$s (%3$s) none due to %4$s\n"), + virDomainGetName(dom), srcPath, devAlias, reason); + break; + case VIR_DOMAIN_EVENT_IO_ERROR_PAUSE: + virshEventPrintf(opaque, _("event 'io-error' for domain '%1$s': %2$s (%3$s) pause due to %4$s\n"), + virDomainGetName(dom), srcPath, devAlias, reason); + break; + case VIR_DOMAIN_EVENT_IO_ERROR_REPORT: + virshEventPrintf(opaque, _("event 'io-error' for domain '%1$s': %2$s (%3$s) report due to %4$s\n"), + virDomainGetName(dom), srcPath, devAlias, reason); + break; + case VIR_DOMAIN_EVENT_IO_ERROR_LAST: + default: + virshEventPrintf(opaque, _("event 'io-error' for domain '%1$s': %2$s (%3$s) unknown due to %4$s\n"), + virDomainGetName(dom), srcPath, devAlias, reason); + break; + } } static void -- 2.39.2
On 4/13/23 10:15, Peter Krempa wrote:
Many things in libvirt attempt to construct error messages from various sub-strings which is not translation friednly.
The beginning of the series refactors the cgroup code to avoid this practice.
The end of this series is an example how the worst offender, virsh event handling code, can be improved to avoid the bad practice.
I've found most cases using:
git grep ' _([^"]'
Unfortunateluy the better practice is a bit more verbose, thus RFC.
Peter Krempa (8): virCgroupV1GetBlkioIo(Device)Serviced: Refactor extraction of cgroup data Don't translate strings used with VIR_DEBUG virshGraphicsAddressToString: Remove pointless translation vshPrint: Add version using 'va_list' virsh: event: Introduce virshEventPrintf virsh-domain-event: Make 'virshEventTrayChangePrint' translation friendly virsh-domain-event: Make 'virshEventWatchdogPrint' translation friendly virsh-domain-event: Make 'virshEventIOError(Reason)Print' translation friendly
src/util/vircgroupv1.c | 264 ++++++++++++++++--------------------- src/util/virnetlink.c | 6 +- src/vz/vz_driver.c | 4 +- tools/virsh-domain-event.c | 223 +++++++++++++++++-------------- tools/vsh.c | 32 +++-- tools/vsh.h | 4 + 6 files changed, 274 insertions(+), 259 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (3)
-
Ján Tomko -
Michal Prívozník -
Peter Krempa