Re: [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 <pkre...@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 <jto...@redhat.com>

Jano

signature.asc
Description: PGP signature