Thanks Harald for checking the license thing out, although I think the code is so trivial that it's difficult to claim copyright. Let's see. Thanks Pavel for the patch! I have some comments: Pavel Baturko wrote: Please set the git config user.name and user.email options correctly: git config --global user.name 'Pavel Baturko' git config --global user.email your(a)email.here There is no commit message. I don't know why this would be OK. Please write a few relevant sentences about the change *in* the commit message, rather than in the email you send where you attach the commit message. The commit message is the correct place for the information, since that is the canonical storage facility for the code. The mailing list is not. Are these files always guaranteed to be available, or are some configure.ac checks required in this change? Please remove all unrelated changes from the commit. I am strongly in favor of cleaning up code, but it needs to be done in separate commits, not mixed in with other changes. Mixing changes makes life very difficult for anyone who might need to bisect the code. It is completely unneccessary. It's also really ugly. I really like the kernel style for switch: switch (foo) { case BAR: case BAZ: quux(); break; } Ie. moving the opening brace, and using one less tab inside. The added newline seems an unrelated change. The one above too. Please pay attention to the existing style in code that you work on, and make sure that you follow it exactly. You put the opening brace on the following line, while the rest of the code does not do this. In any case, please change this if (x == foo) else if (x == bar) code to use switch (). The hex_or_str variable name is absolutely horrible. Please change it to something that also conveys the meaning of the value; something self-descriptive. One suggestion would be have_utf8_locale. This discards the last surplus byte if there is one. Good! Passing an empty string as locale is an invasive operation, which is why I suggested to pass NULL. Why do you use the empty string instead of NULL? I'd suggest variable = strstr() != NULL; Three comments for a single line of code: First the style, I don't think that conditions and their code is on the same line anywhere else in the code. Following existing style. Second, I don't believe other parts of the code write directly to any file descriptors, but rather they all use logging macros. Follow existing style. Third, in the patch there are several occurences of the "UFT-8" typo. Look at how other code uses whitespace and follow existing style. I would either just include the code here, or write a new function which suits our purposes which converts the full string. I still think printf() is not used anywhere else in the code. And the code makes absolutely no sense. In one case there is a memcpy(), in the other case there is printf(). Also, why is there a memcpy at all - please just write the data to the correct location in the first place. Always strive for zero copy. Thank you. Did you investigate if this function will handle UTF-8 data correctly? I sure don't know that. This seems to be an unrelated change. And this. And this. And this. .. Seems unrelated. I hope a better name than DSC_0f can be found for it! Please talk to the developer who added the constant, to understand what it is about. Use git blame to find out who it is. Lots of new defines. They're not all required, but I do like that they are all there now. If you do end up keeping this function then even if it is too trivial to be copyrightable and/or the author doesn't care I think it is polite to at the very least reference it's origin. Thanks for your effort - I hope you'll spin a new improved patch! //Peter