[libvirt] [PATCH v2 2/4] iscsi: Add exit status checking for virISCSIGetSession
John Ferlan
jferlan at redhat.com
Wed May 18 11:15:54 UTC 2016
On 05/18/2016 01:55 AM, Peter Krempa wrote:
> On Tue, May 17, 2016 at 11:31:07 -0400, John Ferlan wrote:
>> On 05/17/2016 11:04 AM, Peter Krempa wrote:
>>> On Mon, May 16, 2016 at 11:03:09 -0400, John Ferlan wrote:
>
> [...]
>
>>>> +
>>>> + 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.
>
> IMO that would be better than the current message.
>
>> Just figured it'd be nice to
>> actually translate that error (which just doesn't work the same when
>> exitstatus = 0).
>
> I don't object to add the error message from iscsiadm itself to the
> message. That might help debugging stuff. I object that the resulting
> message isn't any better in case when you want to report it to the user.
>
> As I've written above. Error 21 with the message is not unexpected in
> this case.
>
> In terms of transatability the error message string is extremely wrong.
> If you as a translator see just "'%s --mode session' unexpected %s%s%s"
> you definitely need to go check what's happening there.
>
> The original message in the command code is wrong in terms of
> translatability but it's justified by being a catch-all case for every
> possible command execution that doesn't want to handle errors.
>
> In addition as the error string isn't actually exactly the same as in
> the command code (by partially adding the arguments verbatim to the
> message) it would need to be re-translated.
>
>
> "cannot find session iscsiadm session" followed by the message and/or
> containing the code will be probably the best case here. The function
> you are modifying is meant to find the session so any error is basically
> that it couldn't find it. The message from iscsiadm then helps
> debugging.
>
Fair enough... Primary thought was to be consistent with previous error,
but generating/using a unique error is fine... So rather than the if
then else based on exit status, how about:
virReportError(VIR_ERR_INTERNAL_ERROR,
_("cannot find iscsiadm session: %s"),
NULLSTR(error));
John
More information about the libvir-list
mailing list