Re: [libvirt] [PATCH v2 3/8] qemu: Introduce helper qemuGetCompressionProgram
On 09/27/2016 12:40 PM, Ján Tomko wrote:
On Fri, Sep 23, 2016 at 07:30:51AM -0400, John Ferlan wrote:
> Split out the guts of getCompressionType to perform the same
> functionality
> in the new helper program with a subsequent patch goal to be reusable for
> other callers making similar checks/calls to ensure the compression type
> is valid and that the compression program cannot be found.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/qemu/qemu_driver.c | 67
> ++++++++++++++++++++++++++++++++++----------------
> 1 file changed, 46 insertions(+), 21 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 956bddd..3f03576 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -3267,36 +3267,61 @@ qemuCompressProgramAvailable(virQEMUSaveFormat
> compress)
> }
>
>
> +/* qemuGetCompressionProgram:
> + * @imageFormat: String representation from qemu.conf for the
> compression
> + * image format being used (dump, save, or snapshot).
> + *
> + * Returns:
> + * virQEMUSaveFormat - Integer representation of the compression
> + * program to be used for particular style
> + * (e.g. dump, save, or snapshot).
> + * QEMU_SAVE_FORMAT_RAW - If there is no qemu.conf imageFormat
> value or
> + * no there was an error, then just return RAW
> + * indicating none.
> + */
> +static virQEMUSaveFormat
> +qemuGetCompressionProgram(const char *imageFormat)
> +{
> + virQEMUSaveFormat ret;
> +
> + if (!imageFormat)
> + return QEMU_SAVE_FORMAT_RAW;
> +
> + if ((ret = qemuSaveCompressionTypeFromString(imageFormat)) < 0)
> + goto error;
> +
The list of supported values is known at compile time.
If the value is invalid, we should reject it when parsing the config
file.
I'm sure we could if there was a patch for it, but for each of the 3
different types (dump, save, and snapshot) the "check" for validity was
done within this scope (even before this set of patches).
If one peeks at the virQEMUDriverConfigLoadFile where each of the
cfg->{dump|save|snapshot}ImageFormat values are read, the validation of
what's been read (for other fields) is minimal especially for strings.
It seems it's left for "other" code to handle.
I'll review a patch that validates the various fetches from qemu_conf.c
and messages that "%s_image_format" has an invalid value.
That way the 'getCompressionProgram' helper would be faithful to its
name and only try to get the compression program.
If the name is that much of an issue, then feel free to change the name.
John