Re: [PATCH 3/6] util: vircommand: Add wrappers for virCommand error checking

[Ján Tomko <jtomko@xxxxxxxxxx>]

On a Tuesday in 2021, Peter Krempa wrote:
> Extract the check and reporting of error from the individual virCommand
> APIs into a separate helper. This will aid future refactors.
>
> Signed-off-by: Peter Krempa <pkrempa@xxxxxxxxxx>
> ---
> src/util/vircommand.c | 143 ++++++++++++++++++++++--------------------
> 1 file changed, 76 insertions(+), 67 deletions(-)
>
> @@ -2760,7 +2763,10 @@ int virCommandHandshakeWait(virCommandPtr cmd)
>     char c;
>     int rv;
>
> -    if (!cmd || cmd->has_error || !cmd->handshake) {
> +    if (virCommandRaiseError(cmd) < 0)
> +        return -1;
> +
> +    if (!cmd->handshake) {
>         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                        _("invalid use of command API"));
>         return -1;
> @@ -2818,7 +2824,10 @@ int virCommandHandshakeNotify(virCommandPtr cmd)
> {
>     char c = '1';
>
> -    if (!cmd || cmd->has_error || !cmd->handshake) {
> +    if (virCommandRaiseError(cmd) < 0)
> +        return -1;
> +

From the name of the function, I'd expect it to raise the error
unconditionally. Cache invalidation and naming are hard.

CheckError might be a misleading name too [0].
MaybeRaiseError? RaiseErrorIfNeeded?

Would returning bool make more sense here?

[0] commit 5c63b50a8b9f18b9ab18753cb679065cd31895fb
    conf: rename virDomainCheckVirtioOptions


Also, in both cases the same error message from virCommandRaiseError
is repeated if (!cmd->handshake). Consider void virCommandRaiseError
and:

if (virCommandHasError || !cmd->handshake) {
    virCommandRaiseError();
    return -1;
}

> +    if (!cmd->handshake) {
>         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                        _("invalid use of command API"));


>         return -1;

If you consider any of the points above:
Reviewed-by: Ján Tomko <jtomko@xxxxxxxxxx>

Jano

Attachment: signature.asc
Description: PGP signature