On 2012.06.26 08:18, Ludovic Rousseau wrote: > I think it is a bad news to replace strncpy() by a for() loop just > because "WDK's OACR doesn't like strncpy..." > If you need a secure strncpy then strlcpy is there. But it is a BSD variant.
Yes, and there a strncpy_s on Windows, and probably other variants elsewhere, but we're dealing with a sample here, and, as explained below, there are more important matters that fixing warnings with which we need to press on here. Samples are just that: samples. They don't need to win first prize in the "code that absolutely could not be improved any further" contest. If we go with an API call that's nonstandard, instead of a for loop, then we have to test Linux, cygwin, MinGW, OS-X, MSVC and *BSD to make sure that what we pick works everywhere. Well, if anybody wants to do that and submit a patch, they are more than welcome. But don't count on me to do just that. > The code should be: > - use of sizeof instead of the "magic" number 64 I don't mind adding a fix for sizeof if you think it's necessary. > - aways add a NUL byte at the end of the string That was already part of the patch. > I would prefer if each warning fix is in a different commit and if the > warning message from the compiler is added to the commit message. Well, I'll be blunt: I would also prefer having the time to do that. Except I don't. And I don't think anybody else is going to look at submitting a patch to fix these warnings, so when I know additional details, such as the complete warning messages can be obtained regardless, I'll try to proceed with the approach that will save time most overall. We *really* have more important matters to deal with (those OS-X bugs, setting up the continuous review server, USB 3.0 descriptor, addressing Hans' finding with the timerfd, as well as what's on our roadmap for 1.0.13, with time is quickly ticking out), so yeah, I'll be taking the shortcuts that I deem appropriate here when I don't see them endangering the quality of the software as well as not collectively costing more time to other people in the long run than what I am going to be able to spare individually. Why? Because having the best commit breakdown in the world doesn't actually do much in terms of helping our users. But freeing time that can be spent other items than warnings does. Also, with regards to the commit message, I saw the dbg move as part of the dealing with unused variables line, since what OACR complains about here is dbg being unchecked, or "unused", if you will. I also partially clarified in the e-mail what this was all about and we're simply moving an assignation to just before the variable is first tested. There are modifications that require being careful, and therefore clarification, but that's hardly one of them. IMO, someone who need further clarification than what they found on the commit message would head to the mailing list and seek messages of relevance that occurred around the time the patch was fixed. Considering that the commit title states "OACR" and "Level 4", they should easily be able to find Orin's e-mail and will get the full details of what we've been fixing. Thus duplicating details that can easily be obtained regardless seems rather pointless: The very very very few people who will ever want to find them post integration and review, will be able to find them on their own. > It is hard to review a patch with many fixes mixed. It's hard to produce easy to review patches when you're short of time and you know that there are more pressing matters waiting. So what it all boils down to really is how much time you and others think you will spare, in all, from not having to review a monolithic patch vs how much time I'm going to spare from not having to deal with one. Plus I'm sure there are reviewers that tend to prefer single patches rather than multiple ones - I know I'm one them. Also, the addressing of each warning is pretty much a one liner (or 2-3 lines at most), so I beg to differ that it's that hard to review. If you prefer, you can consider that each modified section is an atomic patch that fixes one warning uttered by the compiler, which should be listed in Orin's e-mail. The only one that doesn't fit this description is the moving of the prefast pragma from windows_usb.c into msvc/config.h Regards, /Pete ------------------------------------------------------------------------------ Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ _______________________________________________ libusbx-devel mailing list libusbx-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/libusbx-devel
