On 09/28/2016 12:18 PM, Shively, Gregory wrote: > The one-time warning sounds like a good idea. Is there a place that > you have to add the one-time message, or should I just add a static variable > to determine if the warning has been displayed the first time down this code > path?
If nobody recommends a better place, then place the warning in your code. "bzr grep WARNING src" for examples. >> IMO, you should not be responsible for fixing any old test-builds.sh bugs, >> only for the problems your changes add or cause. > Thanks - I was mistaken and thought from the merge document you sent that > I should have run the test-builds.sh. I did not send a merge document. You should run the test-builds.sh. I do not know whether running test-builds.sh will expose any bugs you are responsible for. If running test-builds.sh does not expose any bugs you are responsible for [because of all the other bugs that you are not responsible for], then simply say so. >>> + char *cmd = (char *)malloc(sizeof(char) * cmdLen); >>> + snprintf(cmd, cmdLen, cmdFormat, daddr, saddr, established); >> Please rewrite using SBuf. AFAICT, what you want can be written without all >> those unsafe C things like malloc() and snprintf() -- you are simply >> concatenating a few strings to form a command. > Does just using the appendf() method sound alright? Sounds bad to me. Just use SBuf::append() to concatenate strings and characters instead of turning off many C++ protections by using printf() or equivalent. > And am I > correct that the method with throw an exception that should just rollup the > stack? Yes, SBuf methods should throw on overflows and similar non-logic errors. Whether your code can handle those exceptions correctly, I have not checked. >>> + FILE *fp = fdopen(pipefd[0], "r"); >> ... >>> + close(pipefd[0]); >>> + return true; >> ... >>> + close(pipefd[0]); >>> + return false; >> Leaking "fp"? AFAICT, you are supposed to close "fp" instead of pipefd[0] >> after fdopen() but I am not sure. >>> + debugs(89, DBG_IMPORTANT, HERE << "PFCTL failed to find >>> state"); >>> + return false; >> Leaking both "fp" and pipefd[0]? > If my memory serves, I think the FILE * returned from fdopen() is just a > pointer to the > same datastructure in the kernel that fd would be associated to. It is not just a pointer -- there would be no reason to add all those f*() functions if it were! FILE is a libc object with buffers. It also includes a pointer to the kernel structures, of course. > I'll go ahead and change it to a fclose() Yes, that is the right solution if you insist on using FILE. >> I recommend removing "fp" and reading from pipefd[0] directly instead. >> You can use Tokenizer to safely parse what you read without these error- >> prone C tricks. > The reason that I used fgets() instead of read() on the pipefd[0] is that I'm > reading either the length of rdaddr or until I read a newline, which ever > comes first. My recommendation was given with the above assumption in mind. > If I used read(), if the length of the line was less than rdaddr len > (and there was more than one line), it could read the beginning of the next > Line. Yes, but it is not a problem if you parse correctly. Currently, you are combining partial parsing with partial reading. Splitting them often avoids nasty bugs that result from such combination. > I could use Tokenizer or it looks like Just SBuf You should use Tokenizer instead of manually parsing what looks like trivial syntax. Parsing problems in the first patch version imply that it is not as trivial as it may seem (or, to be more precise, parsing trivial syntax by hand is still error-prone!). > if I was to use just read(), I'd have to read a character at a time from the > fd Why not read, say, 4096 characters at a time? > .... or if you want me to throw the input stream into a SBuf or > Tokenizer instead of just using some C calls. I do, but I cannot insist on that. You can stop polishing the patch as soon as it has no known bugs. Others will decide whether to accept it. Please note that you have not shared updated changes AFAICT. The squid-3.5.20-osx-devpf-transparent.patch you attached looked old to me. Thank you, Alex. _______________________________________________ squid-dev mailing list [email protected] http://lists.squid-cache.org/listinfo/squid-dev
