On 02/15/2013 10:01 PM, Eric Blake wrote:
On 02/14/2013 05:00 AM, Stefan Berger wrote:
+static char * +qemuCommandPrintFDSet(int fdset, int fd, int open_flags, const char *name) +{ + const char *mode = ""; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (name) { + switch ((open_flags & O_ACCMODE)) { + case O_RDONLY: + mode = "RDONLY:"; + break; + case O_WRONLY: + mode = "WRONLY:"; + break; + case O_RDWR: + mode = "RDWR:"; + break;
Is it worth a default case when the mode is unrecognized? Then again, unless the Linux kernel ever gains O_SEARCH/O_EXEC support, there probably won't ever be any code hitting the default case.
Then we can leave it as-is I suppose. We can always treat other flags separately in this case statement if needed in the future.
+ } + } + + virBufferAsprintf(&buf, "set=%d,fd=%d%s%s", fdset, fd, + (name) ? ",opaque=" : "", + mode); + if (name) + virBufferEscape(&buf, ',', ",", "%s", name); Slightly easier to read as:
virBufferAsprintf(&buf, "set=%d,fd=%d", fdset, fd); if (name) virBufferEscape(&buf, ',', ",", ",opaque=%s", name);
Something like this, yes. 'mode' still needs to be printed in the if(name) part but cannot be part of virBufferEscape.
Rather than having the user supply a sentinel, would it be better to have the user provide nopenFlags? That is, when opening a single fd, passing '&mode, 1' is easier than passing 'int[] { mode, -1}', especially if we don't want to use C99 initializer syntax. For that matter, would it be any easier to pass a flags instead of a mode, where we have the bitwise-or of:
QEMU_USE_RDONLY = 1 << 0, // O_RDONLY QEMU_USE_RDWR = 1 << 1, // O_RDWR QEMU_USE WRONLY = 1 << 2, // O_WRONLY
on the grounds that writing 'QEMU_USE_RDONLY | QEMU_USE_RDWR' looks a little cleaner than writing '(int[]){O_RDONLY, O_RDWR, -1}' (no temporary arrays required).
For that we would need additional code everywhere where we need to convert these QEMU_USE_* to the POSIX flags by bitwise sampling the flags in a loop,which is practically everywhere where the POSIX flags are understood today, e.g., qemuOpenFile(). I am not sure it will make things easier. Stefan