De-duplicate I/O error-detailing code

[eduard-bagdasaryan]

This improvement eliminates code duplication introduced
by b850f8b fix.

[rousskov]

I would like to avoid adding a second terminateX() method just to encapsulate Error and LogTagsErrors creation code, especially when that encapsulation moves us back from a flexible/safe/explicit ErrorDetail API to none-of-those-things int-is-errno API. Adding terminateX() may also encourage creation of terminateY() and terminateZ() to accommodate other error detail construction cases, and we do not want that kind of API fan-out.

I realize that part of the appeal here is how LogTagsErrors objects are initialized from errno. I am pretty sure that code should eventually disappear because we can compute the same log tags based on the information stored in Error. However, polishing that aspect will result in more changes than I would like to see in this particular PR.

Instead of adding terminateWithError(), please add and use a LogTagsErrors::FromErrno() static method that returns a properly configured LogTagsErrors object. Doing so will arguably leave some of the logic duplicated, but I think it is better than adding a competing terminateX() interface with raw errno argument.

Suggested change

	terminateWithError(ERR_WRITE_ERROR, io.xerrno);
	terminateAll(Error(ERR_WRITE_ERROR, SysErrorDetail::NewIfAny(io.xerrno)), LogTagsErrors::FromErrno(io.xerrno));

[eduard-bagdasaryan]

Done (5a03963).

[rousskov]

Is it possible to do something about ERR_READ_TIMEOUT case (e.g., make sure it always has ETIMEDOUT) so that we can refactor FwdState::updateAleWithFinalError() to use the new method as well?

[eduard-bagdasaryan]

I don't think that ERR_READ_TIMEOUT will be always paired with ETIMEDOUT. ERR_READ_TIMEOUT is set in Ftp::Client::timeout() and HttpStateData::httpTimeout() - this code reflects Squid-configuration level timeouts whereas ETIMEDOUT reflects TCP-level timeouts.

[rousskov]

I do not disagree, but LogTagsErrors do need to signal a TIMEOUT suffix for both kinds of timeouts. Should FwdState::updateAleWithFinalError() do something like the following to simplify that code logic and reduce its duplication?

const auto lte = LogTagsErrors::FromErrno(err->type == ERR_READ_TIMEOUT ? ETIMEOUT : err->xerrno);

The above sketch effectively removes the ERR_NONE check in the existing code. I could not quickly figure out why we added that part of the check in recent commit 21f9015. Can you?

[eduard-bagdasaryan]

why we added that part of the check in recent commit 21f9015

No, I could not find an explanation. It was added in the initial PR186 commit. Maybe some of ErrorState() creators called constructed it with ERR_NONE then? I looked through these callers now and could not find any. Anyway, I think it is better to reduce the code duplication as you suggested.