Re: [libvirt] [PATCH v2 2/4] iscsi: Add exit status checking for virISCSIGetSession
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.
Peter
Attachments:
- signature.asc (application/pgp-signature — 819 bytes)