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

[Ján Tomko]

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 at redhat.com>
>---
> 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 at redhat.com>

Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20210302/fe475dc2/attachment-0001.sig>