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 
---
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 

Jano


signature.asc
Description: PGP signature

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

[Peter Krempa]

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 
---
 src/util/vircommand.c | 143 ++
 1 file changed, 76 insertions(+), 67 deletions(-)

diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index 28a903e117..2f6635671d 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -161,6 +161,27 @@ static void *dryRunOpaque;
 static int dryRunStatus;
 #endif /* !WIN32 */

+
+static bool
+virCommandHasError(virCommandPtr cmd)
+{
+return !cmd || cmd->has_error != 0;
+}
+
+
+static int
+virCommandRaiseError(virCommandPtr cmd)
+{
+if (!cmd || cmd->has_error != 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("invalid use of command API"));
+return -1;
+}
+
+return 0;
+}
+
+
 /*
  * virCommandFDIsSet:
  * @cmd: pointer to virCommand
@@ -982,7 +1003,7 @@ virCommandNewVAList(const char *binary, va_list list)
 virCommandPtr cmd = virCommandNew(binary);
 const char *arg;

-if (!cmd || cmd->has_error)
+if (virCommandHasError(cmd))
 return cmd;

 while ((arg = va_arg(list, const char *)) != NULL)
@@ -1076,7 +1097,7 @@ virCommandPassFDGetFDIndex(virCommandPtr cmd, int fd)
 {
 size_t i = 0;

-if (!cmd || cmd->has_error)
+if (virCommandHasError(cmd))
 return -1;

 while (i < cmd->npassfd) {
@@ -1100,7 +1121,7 @@ virCommandPassFDGetFDIndex(virCommandPtr cmd, int fd)
 void
 virCommandSetPidFile(virCommandPtr cmd, const char *pidfile)
 {
-if (!cmd || cmd->has_error)
+if (virCommandHasError(cmd))
 return;

 VIR_FREE(cmd->pidfile);
@@ -1125,7 +1146,7 @@ virCommandGetUID(virCommandPtr cmd)
 void
 virCommandSetGID(virCommandPtr cmd, gid_t gid)
 {
-if (!cmd || cmd->has_error)
+if (virCommandHasError(cmd))
 return;

 cmd->gid = gid;
@@ -1134,7 +1155,7 @@ virCommandSetGID(virCommandPtr cmd, gid_t gid)
 void
 virCommandSetUID(virCommandPtr cmd, uid_t uid)
 {
-if (!cmd || cmd->has_error)
+if (virCommandHasError(cmd))
 return;

 cmd->uid = uid;
@@ -1143,7 +1164,7 @@ virCommandSetUID(virCommandPtr cmd, uid_t uid)
 void
 virCommandSetMaxMemLock(virCommandPtr cmd, unsigned long long bytes)
 {
-if (!cmd || cmd->has_error)
+if (virCommandHasError(cmd))
 return;

 cmd->maxMemLock = bytes;
@@ -1152,7 +1173,7 @@ virCommandSetMaxMemLock(virCommandPtr cmd, unsigned long 
long bytes)
 void
 virCommandSetMaxProcesses(virCommandPtr cmd, unsigned int procs)
 {
-if (!cmd || cmd->has_error)
+if (virCommandHasError(cmd))
 return;

 cmd->maxProcesses = procs;
@@ -1161,7 +1182,7 @@ virCommandSetMaxProcesses(virCommandPtr cmd, unsigned int 
procs)
 void
 virCommandSetMaxFiles(virCommandPtr cmd, unsigned int files)
 {
-if (!cmd || cmd->has_error)
+if (virCommandHasError(cmd))
 return;

 cmd->maxFiles = files;
@@ -1169,7 +1190,7 @@ virCommandSetMaxFiles(virCommandPtr cmd, unsigned int 
files)

 void virCommandSetMaxCoreSize(virCommandPtr cmd, unsigned long long bytes)
 {
-if (!cmd || cmd->has_error)
+if (virCommandHasError(cmd))
 return;

 cmd->maxCore = bytes;
@@ -1178,7 +1199,7 @@ void virCommandSetMaxCoreSize(virCommandPtr cmd, unsigned 
long long bytes)

 void virCommandSetUmask(virCommandPtr cmd, int mask)
 {
-if (!cmd || cmd->has_error)
+if (virCommandHasError(cmd))
 return;

 cmd->mask = mask;
@@ -1193,7 +1214,7 @@ void virCommandSetUmask(virCommandPtr cmd, int mask)
 void
 virCommandClearCaps(virCommandPtr cmd)
 {
-if (!cmd || cmd->has_error)
+if (virCommandHasError(cmd))
 return;

 cmd->flags |= VIR_EXEC_CLEAR_CAPS;
@@ -1210,7 +1231,7 @@ void
 virCommandAllowCap(virCommandPtr cmd,
int capability)
 {
-if (!cmd || cmd->has_error)
+if (virCommandHasError(cmd))
 return;

 cmd->capabilities |= (1ULL << capability);
@@ -1231,7 +1252,7 @@ void
 virCommandSetSELinuxLabel(virCommandPtr cmd,
   const char *label G_GNUC_UNUSED)
 {
-if (!cmd || cmd->has_error)
+if (virCommandHasError(cmd))
 return;

 #if defined(WITH_SECDRIVER_SELINUX)
@@ -1255,7 +1276,7 @@ void
 virCommandSetAppArmorProfile(virCommandPtr cmd,
  const char *profile G_GNUC_UNUSED)
 {
-if (!cmd || cmd->has_error)
+if (virCommandHasError(cmd))
 return;

 #if defined(WITH_SECDRIVER_APPARMOR)
@@ -1277,7 +1298,7 @@ virCommandSetAppArmorProfile(virCommandPtr cmd,
 void
 virCommandDaemonize(virCommandPtr cmd)
 {
-if (!cmd || cmd->has_error)
+if (virCommandHasError(cmd))
 return;

 cmd->flags |= VIR_EXEC_DAEMON;
@@ -1293,7 +1314,7 @@ virCommandDaemonize(virCommandPtr cmd)
 void
 virCommandNonblockingFDs(virCommandPtr cmd)
 {
-if (!cmd || cmd->has_error)
+if (virCommandHasError(cmd))
 return;