<div dir="ltr"><div><div><div>Hi,<br></div> (aligning the other devs)<br></div>After some discussion on IRC, it seems that the best path forward is to prevent the call to strftime nested deep in debugs() from clobbering errno; any other option has a global impact on the code base and would be a bad hack.<br></div>I'll prepare that in a separate patch to trunk.<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jul 10, 2015 at 3:40 PM, Amos Jeffries <span dir="ltr"><<a href="mailto:squid3@treenet.co.nz" target="_blank">squid3@treenet.co.nz</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On 11/07/2015 1:21 a.m., Kinkie wrote:<br>
> On Fri, Jul 10, 2015 at 2:39 PM, Amos Jeffries <<a href="mailto:squid3@treenet.co.nz">squid3@treenet.co.nz</a>> wrote:<br>
><br>
>> On 10/07/2015 3:18 a.m., Kinkie wrote:<br>
>>> Hi,<br>
>>> I'm going through the issues identified by Coverity Scan, in<br>
>>> chronological order.<br>
>>> This patch covers 11 defects, mostly unchecked return values. It also<br>
>>> converts unlinkd to c++ (original defect: using tainted strings). They<br>
>> have<br>
>>> all build-tested; unlinkd has been (hand)-run tested.<br>
>>><br>
>><br>
>> Audited on IRC, summary below:<br>
>><br>
>> in src/DiskIO/AIO/AIODiskIOStrategy.cc does the change actually do<br>
>> anything useful?<br>
>> - the entire function is dead code due to the return-0 that starts it.<br>
>> - thats why I marked that issue as intentional to be ignored.<br>
>> - Ive been contemplating making it #if 0 wrapped for coveritys pleasure.<br>
>><br>
><br>
> Gah, right. #0-ed the whole function body<br>
><br>
><br>
>> in src/DiskIO/DiskDaemon/DiskdFile.cc I thought you were going to use<br>
>> the unlock() result inline for the if-statement, the variable seems<br>
>> unnecessary since its not even used in the debug statement<br>
>><br>
><br>
> I've used a variable for readability; I can refactor no problem. As the<br>
> variable is const, I expect it to be optimized away anyway. Do you wish me<br>
> to?<br>
><br>
><br>
>> in src/debug.cc (or anywhere else) please dont use xstrerror() anymore.<br>
>> - use xstrerr(errno) instead. since the debugs() initilization<br>
>> overwrites errno value.<br>
>><br>
><br>
> Done<br>
><br>
<br>
</div></div>Meh I mucked up there. THat wont work due to same reason for not using<br>
xstrerror().<br>
<br>
Use a const int xerrno = errno; first, then inside the debugs( ...<br>
xstrerr(xerrno) ...<br>
<span class=""><br>
><br>
>> - also at that point in src/debug.cc the debugs() log file is closed.<br>
>> = at least on Windows builds. have to report errors to stderr instead<br>
>> until we fix debugs() to store in a temporary buffer when waiting on<br>
>> cache.log existence (that could be an XXX comment).<br>
>><br>
><br>
> via std::cerr or some other mechanism?<br>
><br>
><br>
<br>
</span>Good question.<br>
<br>
Looking at it closer I see you were only altering inside the for-loop.<br>
Both the loop and the if-statement need the same checks and output on<br>
failure.<br>
It is probably best to add a helper static function that calls<br>
_db_print_stderr(), _db_print_file(), then _db_print_syslog() in that<br>
specific order with the same arguments to each.<br>
Just be careful not to pass va_args around as they are one-use objects.<br>
<span class=""><br>
<br>
>> in src/ipc.cc the conventions Ive been using is to name the sockopt<br>
>> which failed. ie debugs(..."setsockopt(FOO) failed: " << xstrerr(errno))<br>
>><br>
><br>
> Done.<br>
><br>
><br>
>> in src/unlinkd_daemon.cc #include statements aphabetical please. helps<br>
>> avoid a reformat followup commit.<br>
>> - can you double-check the removal of setbuf() please? AFAIK the<br>
>> iostreams still need un-buffering and theres no iostreams way to do it.<br>
>><br>
><br>
> std::endl flushes the output buffer (from<br>
> <a href="http://en.cppreference.com/w/cpp/io/manip/endl" rel="noreferrer" target="_blank">http://en.cppreference.com/w/cpp/io/manip/endl</a> :<br>
> "Inserts a newline character into the output sequence os and flushes it as<br>
> if by calling os.put(os.widen('\n')) followed by os.flush()."<br>
<br>
</span>Doh. Right, and Squid flushes stdin at its end on sending.<br>
<br>
<br>
+1 with the above extra changes.<br>
<span class="HOEnZb"><font color="#888888"><br>
Amos<br>
</font></span></blockquote></div><br><br clear="all"><br>-- <br><div class="gmail_signature"> Francesco</div>
</div>