[libvirt] [PATCH] qemu: Remove bogus codes in function getCompressionType
The error will never be reported, remove the codes, and also improve the docs in qemu.conf to tell user the truth. --- src/qemu/qemu.conf | 4 ++++ src/qemu/qemu_driver.c | 16 +++++----------- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 145062c..75d945b 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -199,6 +199,10 @@ # save_image_format is used when you use 'virsh save' at scheduled saving. # dump_image_format is used when you use 'virsh dump' at emergency crashdump. # +# If the specified format is not valid (the valid formats are "raw", "lzop", +# "gzip", "bzip2", and "xz"), or the compress program is not available, "raw" +# will be used as the format silently without error or warning. +# # save_image_format = "raw" # dump_image_format = "raw" diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b928004..48fc46a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2712,19 +2712,13 @@ 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")); + /* Use "raw" as the format if the specified format is not valid, + * or the compress program is not available, + */ + if (compress < 0) 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")); + if (!qemudCompressProgramAvailable(compress)) return QEMUD_SAVE_FORMAT_RAW; - } } return compress; } -- 1.7.6
On 07/26/2011 01:04 AM, Osier Yang wrote:
The error will never be reported, remove the codes, and also improve the docs in qemu.conf to tell user the truth. --- src/qemu/qemu.conf | 4 ++++ src/qemu/qemu_driver.c | 16 +++++----------- 2 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 145062c..75d945b 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -199,6 +199,10 @@ # save_image_format is used when you use 'virsh save' at scheduled saving. # dump_image_format is used when you use 'virsh dump' at emergency crashdump. # +# If the specified format is not valid (the valid formats are "raw", "lzop", +# "gzip", "bzip2", and "xz"), or the compress program is not available, "raw"
It's a maintenance nightmare to document the list of compression types twice, especially when it appears only one paragraph earlier. The current wording ties the earlier list to only save_image_format, so a better approach is one that makes it clear that a single list is good for both variables. Maybe a better wording would be: # The default format for Qemu/KVM guest save images is raw; that is, the # memory from the domain is dumped out directly to a file. If you have # guests with a large amount of memory, however, this can take up quite # a bit of space. If you would like to compress the images while they # are being saved to disk, you can also set "lzop", "gzip", "bzip2", or # "xz" for either image variable. Note that this means you slow down # the process of saving a domain in order to save disk space; the list # above is in descending order by performance and ascending order by # compression ratio. # # save_image_format is used when you use 'virsh save' at scheduled # saving, and it is an error if the requested compression is not found. # # dump_image_format is used when you use 'virsh dump' at emergency # crashdump, and if the requested compression is not found, this falls # back to "raw" compression. #
+# will be used as the format silently without error or warning.
Can we at least get a log message? It could be nice when inspecting a dump file to learn why it was not compressed, by then reading the log file and seeing a message that:
+ /* Use "raw" as the format if the specified format is not valid, + * or the compress program is not available, + */ + if (compress< 0) return QEMUD_SAVE_FORMAT_RAW;
the requested qemu.conf format was invalid, or
- } - if (!qemudCompressProgramAvailable(compress)) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - "%s", _("Compression program for dump image format " - "in configuration file isn't available, " - "using raw")); + if (!qemudCompressProgramAvailable(compress)) return QEMUD_SAVE_FORMAT_RAW;
the requested compression is valid, but not found on the machine. I agree that qemuReportError() is not the right approach, since we are proceeding with raw as the fallback, but silence instead of a log message about why we used the fallback is not ideal. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
The error in getCompressionType will never be reported, change the errors codes into warning (VIR_WARN("%s", _(foo)); doesn't break syntax-check rule), and also improve the docs in qemu.conf to tell user the truth. --- src/qemu/qemu.conf | 10 ++++++++-- src/qemu/qemu_driver.c | 15 ++++++++------- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 145062c..7de6863 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -196,8 +196,14 @@ # saving a domain in order to save disk space; the list above is in descending # order by performance and ascending order by compression ratio. # -# save_image_format is used when you use 'virsh save' at scheduled saving. -# dump_image_format is used when you use 'virsh dump' at emergency crashdump. +# save_image_format is used when you use 'virsh save' at scheduled +# saving, and it is an error if the specified save_image_format is +# not valid, or the according compression program can't be found. +# +# dump_image_format is used when you use 'virsh dump' at emergency +# crashdump, and if the specified dump_image_format is not valid, or +# the according compression program can't be found, this falls +# back to "raw" compression. # # save_image_format = "raw" # dump_image_format = "raw" diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0066c55..e9bd421 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2712,17 +2712,18 @@ getCompressionType(struct qemud_driver *driver) */ if (driver->dumpImageFormat) { compress = qemudSaveCompressionTypeFromString(driver->dumpImageFormat); + /* Use "raw" as the format if the specified format is not valid, + * or the compress program is not available, + */ if (compress < 0) { - qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("Invalid dump image format specified in " - "configuration file, using raw")); + VIR_WARN("%s", _("Invalid dump image format specified in " + "configuration file, using raw")); 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("%s", _("Compression program for dump image format " + "in configuration file isn't available, " + "using raw")); return QEMUD_SAVE_FORMAT_RAW; } } -- 1.7.6
On 07/27/2011 04:58 AM, Osier Yang wrote:
The error in getCompressionType will never be reported, change the errors codes into warning (VIR_WARN("%s", _(foo)); doesn't break syntax-check rule), and also improve the docs in qemu.conf to tell user the truth. --- src/qemu/qemu.conf | 10 ++++++++-- src/qemu/qemu_driver.c | 15 ++++++++------- 2 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 145062c..7de6863 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -196,8 +196,14 @@ # saving a domain in order to save disk space; the list above is in descending # order by performance and ascending order by compression ratio. # -# save_image_format is used when you use 'virsh save' at scheduled saving. -# dump_image_format is used when you use 'virsh dump' at emergency crashdump. +# save_image_format is used when you use 'virsh save' at scheduled +# saving, and it is an error if the specified save_image_format is +# not valid, or the according compression program can't be found.
s/according/requested/
+# +# dump_image_format is used when you use 'virsh dump' at emergency +# crashdump, and if the specified dump_image_format is not valid, or +# the according compression program can't be found, this falls
s/according/requested/
+++ b/src/qemu/qemu_driver.c @@ -2712,17 +2712,18 @@ getCompressionType(struct qemud_driver *driver) */ if (driver->dumpImageFormat) { compress = qemudSaveCompressionTypeFromString(driver->dumpImageFormat); + /* Use "raw" as the format if the specified format is not valid, + * or the compress program is not available, + */
s/,/./ at the comment end. ACK with those nits fixed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
于 2011年07月28日 00:18, Eric Blake 写道:
On 07/27/2011 04:58 AM, Osier Yang wrote:
The error in getCompressionType will never be reported, change the errors codes into warning (VIR_WARN("%s", _(foo)); doesn't break syntax-check rule), and also improve the docs in qemu.conf to tell user the truth. --- src/qemu/qemu.conf | 10 ++++++++-- src/qemu/qemu_driver.c | 15 ++++++++------- 2 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 145062c..7de6863 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -196,8 +196,14 @@ # saving a domain in order to save disk space; the list above is in descending # order by performance and ascending order by compression ratio. # -# save_image_format is used when you use 'virsh save' at scheduled saving. -# dump_image_format is used when you use 'virsh dump' at emergency crashdump. +# save_image_format is used when you use 'virsh save' at scheduled +# saving, and it is an error if the specified save_image_format is +# not valid, or the according compression program can't be found.
s/according/requested/
+# +# dump_image_format is used when you use 'virsh dump' at emergency +# crashdump, and if the specified dump_image_format is not valid, or +# the according compression program can't be found, this falls
s/according/requested/
+++ b/src/qemu/qemu_driver.c @@ -2712,17 +2712,18 @@ getCompressionType(struct qemud_driver *driver) */ if (driver->dumpImageFormat) { compress = qemudSaveCompressionTypeFromString(driver->dumpImageFormat); + /* Use "raw" as the format if the specified format is not valid, + * or the compress program is not available, + */
s/,/./ at the comment end.
ACK with those nits fixed.
Thanks, pushed with the fixing. Osier
participants (2)
-
Eric Blake -
Osier Yang