Re: [libvirt] [PATCHv2 02/16] storage: treat 'aio' like 'raw' at parse time
On 10/14/2012 08:57 PM, Doug Goldstein wrote:
On Sat, Oct 13, 2012 at 4:59 PM, Eric Blake <eblake(a)redhat.com>
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 617 bytes)