[libvirt] [PATCH 2/6] iscsi: Add exit status checking for virISCSIGetSession

[John Ferlan]

On 05/16/2016 07:57 AM, Peter Krempa wrote:
> On Fri, May 13, 2016 at 17:29:22 -0400, John Ferlan wrote:
>> Utilize the exit status parameter for virCommandRunRegex in order to
>> check the return error from the 'iscsiadm --mode session' command.
>> Without this enabled, if there are no sessions running then virCommandRun
>> would have displayed an error such as:
>>
>>     2016-05-13 15:17:15.165+0000: 10920: error : virCommandWait:2553 :
>>                internal error: Child process (iscsiadm --mode session)
>>                unexpected exit status 21: iscsiadm: No active sessions.
> 
>                                  ^^^^^^^^^^[1]
> 
>>
>> It is possible that for certain paths (when probe is true) we only care
>> whether it's running or not to make certain decisions.  Spitting out
>> the error for those paths is unnecessary.
>>
>> If we do have a situation where probe = false and there's an error,
>> then display the error from iscsiadm if it's there; otherwise, default
>> to the non descript error.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/util/viriscsi.c | 28 +++++++++++++++++++++++-----
>>  1 file changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
>> index 846ea68..be296d7 100644
>> --- a/src/util/viriscsi.c
>> +++ b/src/util/viriscsi.c
> 
> 
> 
>> @@ -79,21 +80,38 @@ virISCSIGetSession(const char *devpath,
>>          .session = NULL,
>>          .devpath = devpath,
>>      };
>> +    char *error;
>> +    int exitstatus = 0;
>>  
>> -    virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode", "session", NULL);
>> +    virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode",
>> +                                             "session", NULL);
>> +    virCommandSetErrorBuffer(cmd, &error);
>>  
>>      if (virCommandRunRegex(cmd,
>>                             1,
>>                             regexes,
>>                             vars,
>>                             virISCSIExtractSession,
>> -                           &cbdata, NULL, NULL) < 0)
>> +                           &cbdata, NULL, &exitstatus) < 0)
>>          goto cleanup;
>>  
>>      if (cbdata.session == NULL && !probe) {
>> -        virReportError(VIR_ERR_INTERNAL_ERROR,
>> -                       "%s", _("cannot find session"));
>> -        goto cleanup;
>> +        /* If the command failed, let's give some information as to why */
>> +        if (exitstatus != 0) {
>> +            char *str = virCommandToString(cmd);
>> +            char *st = virProcessTranslateStatus(exitstatus);
>> +
>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                           _("'%s --mode session' unexpected %s%s%s"),
>> +                           ISCSIADM, NULLSTR(st),
>> +                           error ? ": " : "",
>> +                           error ? error : "");
> 
> The original code where this was copied from uses 'str' to be part of
> the error message. Here it's unused.
> 

dang - it was used, then I removed the need and forgot to remove it.
simple enough to remove.

> Is it necessary to report the error as virCommandWait() would report?

virCommandWait won't report the error if you pass exitstatus AFAICT, so
that means the caller would need to report it. I figure that's the
reason one passes exitstatus and sets the Error buffer. This code
doesn't set the rawStatus, but the WIFEXITED(status) will return
something if the command failed.  If it didn't then, the else in
virCommandWait doesn't report anything and thus we have the else here
about cannot find session.


> Isn't it enough just to notify the user that the command failed.
> Especially since error code 21 [1] is not really an unexpected error
> code in this code path (it's even documented in the man page for
> iscsiadm). In such case the error message in the "else" section is
> better than the raw output.

Error only printed with probe = false.

Yes, error code 21 means no active sessions, which 3 out of 4 callers
are happy to deal with it. For the 1 which doesn't want that, then an
error code of 21 should be reported.

Prior to this patch - the "error" gets displayed for probe's (e.g.
check, start, and stop). After it only gets displayed from the refresh
path for any error.

> 
> Also is INTERNAL_ERROR really necessary in case where the session
> wasn't found. [2].
> 

Better suggestion for the refresh path that expects to find the session
available? I know it's somewhat of a catch-all and well over-used;
however, still no different that if virCommandWait reported it.

> 
> 
> 
>> +            VIR_FREE(str);
>> +            VIR_FREE(st);
>> +        } else {
>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> 
> [2]. Ah that's pre-existing.
> 
>> +                           "%s", _("cannot find session"));
>> +        }
>>      }
>>  
>>   cleanup:
> 
> Buffer for 'error' is leaked.

Oh right and not initialized... Easy enough to fix.

John

> 
> Peter
>