On 05/17/2016 11:04 AM, Peter Krempa wrote:
> On Mon, May 16, 2016 at 11:03:09 -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.
>>
>> 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@xxxxxxxxxx>
>> ---
>> src/util/viriscsi.c | 27 ++++++++++++++++++++++-----
>> 1 file changed, 22 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
>> index 846ea68..3627eed 100644
>> --- a/src/util/viriscsi.c
>> +++ b/src/util/viriscsi.c
>
> [...]
>
>> @@ -79,24 +80,40 @@ virISCSIGetSession(const char *devpath,
>
> [....]
>
>>
>> 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 *st = virProcessTranslateStatus(exitstatus);
>> +
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("'%s --mode session' unexpected %s%s%s"),
>
> I still don't think this message is okay. As the error message above
> suggests error code 21 is returned if no active session is found which
> is definitely not unexpected here ... [1]
>
> Also it's quite unlikely that iscsiadm would die of any "unnatural"
> reasons and thus virProcessTranslateStatus would provide any helpful
> data.
>
>> + ISCSIADM, NULLSTR(st),
>> + error ? ": " : "",
>> + error ? error : "");
>> + VIR_FREE(st);
>> + } else {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + "%s", _("cannot find session"));
>
> [1] ... as this suggests.
>
>> + }
>> }
>>
>
> The direction of this patch is okay, but the error could be improved. Do
> you have any strong reason for keeping the message?
>
No - beyond keeping the output the same as previously except that this
message would only be displayed for the Refresh path; whereas, currently
it's displayed for check, startup, and shutdown as well.
I suppose all error paths could use the less than perfect second message
perhaps with a tweak to add the error code to at least indicate what the
error from the executed iscsiadm was. Just figured it'd be nice to
actually translate that error (which just doesn't work the same when
exitstatus = 0).
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list