On 10/14/2012 08:57 PM, Doug Goldstein wrote: > On Sat, Oct 13, 2012 at 4:59 PM, Eric Blake <eblake@xxxxxxxxxx> wrote: >> We have historically allowed 'aio' as a synonym for 'raw' for >> back-compat to xen, but since a future patch will move to using >> an enum value, we have to pick one to be our preferred output >> name. This is a slight change in the XML, but the sexpr and >> xm outputs should still be identical. >> >> + if (STREQ_NULLABLE(driverType, "aio")) >> + memcpy(driverType, "raw", strlen("raw")); > > Two nits here that don't really have to be applied. First is that the > string has to be specified twice so I'd almost prefer there to be a > macro for this that's like: > > #define STRREPLACE(dest, src) memcpy((dest), (src), strlen((src)) Maybe it would be better to just painfully spell out that we are replacing three bytes (a generic STRREPLACE would have to worry about lengths differing between original and new contents; I got lucky that 'aio' and 'raw' are the same size). > > The second would be to potentially use sizeof() on a buffer that's > really a const ro data string. Just cause strlen() is a runtime check. > Obviously this appears throughout this whole commit. strlen("constant") is optimized by gcc into a constant, with no call to strlen() in the source code. Furthermore, I seem to recall different semantics between C and C++ about sizeof("") on whether the trailing NUL byte is included in the result, but never remember which is which, so I prefer strlen() over sizeof() to avoid the ambiguity. But if I open code things, then it's no longer a concern :) Should I post a v2 that does: if (STREQ_NULLABLE(driverType, "aio")) { /* In-place conversion to "raw" */ driverType[0] = 'r'; driverType[1] = 'a'; driverType[2] = 'w'; } Or, since this area of code is changed again later in the series when I rename 'char *driverType' to 'int format', should I just leave this as-is since it is just temporary ugliness? > > Past the nits above. Visual ACK as well. > We'll see if anyone has an opinion about the above in the next day or so. -- Eric Blake eblake@xxxxxxxxxx +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list