[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2 2/4] iscsi: Add exit status checking for virISCSIGetSession




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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]