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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list