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(a)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(a)redhat.com>
Jano