[libvirt] [PATCH v2] qemu: Warning prompt if invalid dump image format specified
"getCompressionType" trys to throw error when invalid dump image format is specified, or the compression program for the specified image format is not available, but at the same time, it returns enum qemud_save_formats, as a result, upper function will think it was successful, and disgard to check the error. Change "qemuReportError" into "VIR_WARN" instead to let the user known what happened. v1 patch is here: http://www.redhat.com/archives/libvir-list/2011-January/msg00715.html As it reverted b6f13bb7002da76db15242e1d996e8a6222a754, and it was intentionally to chose the behavior of falling back to raw if the compression type is invalid, so simply use "VIR_WARN" to fix the problem. * src/qemu/qemu_driver.c --- src/qemu/qemu_driver.c | 13 ++++++------- 1 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 34cc29f..dac71fa 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4806,16 +4806,15 @@ getCompressionType(struct qemud_driver *driver) if (driver->dumpImageFormat) { compress = qemudSaveCompressionTypeFromString(driver->dumpImageFormat); if (compress < 0) { - qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("Invalid dump image format specified in " - "configuration file, using raw")); + VIR_WARN("Invalid dump image format '%s' specified in " + "configuration file, using raw", + driver->dumpImageFormat); return QEMUD_SAVE_FORMAT_RAW; } if (!qemudCompressProgramAvailable(compress)) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - "%s", _("Compression program for dump image format " - "in configuration file isn't available, " - "using raw")); + VIR_WARN("Compression program for dump image format '%s' " + "in configuration file isn't available, using raw", + driver->dumpImageFormat); return QEMUD_SAVE_FORMAT_RAW; } } -- 1.7.3.2
On 01/25/2011 01:42 AM, Osier Yang wrote:
"getCompressionType" trys to throw error when invalid dump image format is specified, or the compression program for the specified image format is not available, but at the same time, it returns enum qemud_save_formats, as a result, upper function will think it was successful, and disgard to check the error.
s/disgard to check/discard/ (another option might be "disregard")
As it reverted b6f13bb7002da76db15242e1d996e8a6222a754, and it was intentionally to chose the behavior of falling back to raw if the compression type is invalid, so simply use "VIR_WARN" to fix the problem.
* src/qemu/qemu_driver.c --- src/qemu/qemu_driver.c | 13 ++++++------- 1 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 34cc29f..dac71fa 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4806,16 +4806,15 @@ getCompressionType(struct qemud_driver *driver) if (driver->dumpImageFormat) { compress = qemudSaveCompressionTypeFromString(driver->dumpImageFormat); if (compress < 0) { - qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("Invalid dump image format specified in " - "configuration file, using raw")); + VIR_WARN("Invalid dump image format '%s' specified in " + "configuration file, using raw", + driver->dumpImageFormat);
Hmm, I was about to straight ACK this, then I noticed that this loses the translation. Which means that VIR_WARN is generally intended for situations that the end user should never see (or we would have translated it to the user's language). On the other hand, I note that nwfilter/nwfilter_ebiptables_driver.c uses VIR_WARN for an end-user-visible message, by pre-translating the message into an snprintf() buffer, then passing that translation through VIR_WARN0 (in order to bypass the 'make syntax-check' rule that calls us for the majority of VIR_WARN scenarios where we don't want translation). That also feels awkward. I'm not sure what the best approach is here - requiring translation for all VIR_WARN seems awkward, but so does nwfilter's workaround just to avoid a syntax-check rule. Maybe we need a dedicated helper method that requires translation but has the same effect as VIR_WARN, which both nwfilter and this code can use. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
于 2011年01月26日 01:02, Eric Blake 写道:
On 01/25/2011 01:42 AM, Osier Yang wrote:
"getCompressionType" trys to throw error when invalid dump image format is specified, or the compression program for the specified image format is not available, but at the same time, it returns enum qemud_save_formats, as a result, upper function will think it was successful, and disgard to check the error.
s/disgard to check/discard/ (another option might be "disregard")
As it reverted b6f13bb7002da76db15242e1d996e8a6222a754, and it was intentionally to chose the behavior of falling back to raw if the compression type is invalid, so simply use "VIR_WARN" to fix the problem.
* src/qemu/qemu_driver.c --- src/qemu/qemu_driver.c | 13 ++++++------- 1 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 34cc29f..dac71fa 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4806,16 +4806,15 @@ getCompressionType(struct qemud_driver *driver) if (driver->dumpImageFormat) { compress = qemudSaveCompressionTypeFromString(driver->dumpImageFormat); if (compress< 0) { - qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("Invalid dump image format specified in " - "configuration file, using raw")); + VIR_WARN("Invalid dump image format '%s' specified in " + "configuration file, using raw", + driver->dumpImageFormat);
Hmm, I was about to straight ACK this, then I noticed that this loses the translation. Which means that VIR_WARN is generally intended for situations that the end user should never see (or we would have translated it to the user's language).
On the other hand, I note that nwfilter/nwfilter_ebiptables_driver.c uses VIR_WARN for an end-user-visible message, by pre-translating the message into an snprintf() buffer, then passing that translation through VIR_WARN0 (in order to bypass the 'make syntax-check' rule that calls us for the majority of VIR_WARN scenarios where we don't want translation). That also feels awkward.
I'm not sure what the best approach is here - requiring translation for all VIR_WARN seems awkward, but so does nwfilter's workaround just to avoid a syntax-check rule. Maybe we need a dedicated helper method that requires translation but has the same effect as VIR_WARN, which both nwfilter and this code can use.
Didn't noticed nwfilter driver translates messages for VIR_WARN, good to known it. But all the VIR_WARN in qemu driver doesn't translate it. so, will it be fine to push the this patch first, and make dedicated helper method later as another patch? :-) Thanks Osier
On 01/26/2011 04:06 AM, Osier Yang wrote:
+++ b/src/qemu/qemu_driver.c @@ -4806,16 +4806,15 @@ getCompressionType(struct qemud_driver *driver) if (driver->dumpImageFormat) { compress = qemudSaveCompressionTypeFromString(driver->dumpImageFormat); if (compress< 0) { - qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("Invalid dump image format specified in " - "configuration file, using raw")); + VIR_WARN("Invalid dump image format '%s' specified in " + "configuration file, using raw", + driver->dumpImageFormat);
I'm not sure what the best approach is here - requiring translation for all VIR_WARN seems awkward, but so does nwfilter's workaround just to avoid a syntax-check rule. Maybe we need a dedicated helper method that requires translation but has the same effect as VIR_WARN, which both nwfilter and this code can use.
Didn't noticed nwfilter driver translates messages for VIR_WARN, good to known it. But all the VIR_WARN in qemu driver doesn't translate it. so, will it be fine to push the this patch first, and make dedicated helper method later as another patch? :-)
Well, technically committing this as-is would be a regression (the string won't be translated, where it used to be), but no one would see the regression (without the patch, the message got eaten). I'm worried that if we don't keep the translation markup now, that we'll forget and do a release where there is a useful and user-visible message that fell through the translation cracks. Even if that means touching this line twice (once now for the same snprintf hack as nwfilter, and once later once someone writes a cleanup dedicated helper function). It's always worth a little extra pain to avoid a user-visible regression. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Osier Yang