[libvirt] [PATCH V6 3/3] Add support for file descriptor sets

Stefan Berger stefanb at linux.vnet.ibm.com
Mon Feb 18 22:04:19 UTC 2013


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




More information about the libvir-list mailing list