Re: [Qemu-devel] [PATCH 2/3] block: Add support to "open" /dev/fd/X filenames

[Corey Bryant <coreyb@linux.vnet.ibm.com>]

On 06/04/2012 10:54 AM, Kevin Wolf wrote:
> Am 04.06.2012 15:10, schrieb Corey Bryant:
>> The main goal of this patch series is to enable isolation of guest
>> images that are stored on the same NFS mount.  This can be achieved
>> if the management application opens files for QEMU, and QEMU is
>> restricted from opening files.
>>
>> This patch adds support to the block layer open paths to dup(X) a
>> pre-opened file descriptor if the filename is of the format
>> /dev/fd/X.
>>
>> One nice thing about this approach is that no new SELinux policy is
>> required to prevent open of NFS files (files with type nfs_t).  The
>> virt_use_nfs boolean type simply needs to be set to false, and open
>> will be prevented (yet dup will be allowed).  For example:
>>
>>    # setsebool virt_use_nfs 0
>>    # getsebool virt_use_nfs
>>    virt_use_nfs -->  off
>>
>> Signed-off-by: Corey Bryant<coreyb@linux.vnet.ibm.com>
>> ---
>>   block.c           |   15 +++++++++++++++
>>   block/raw-posix.c |   20 ++++++++++----------
>>   block/raw-win32.c |    4 ++--
>>   block/vdi.c       |    5 +++--
>>   block/vmdk.c      |   21 +++++++++------------
>>   block/vpc.c       |    2 +-
>>   block/vvfat.c     |   21 +++++++++++----------
>>   block_int.h       |   13 +++++++++++++
>>   8 files changed, 64 insertions(+), 37 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index af2ab4f..8416313 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -102,6 +102,21 @@ static BlockDriverState *bs_snapshots;
>>   /* If non-zero, use only whitelisted block drivers */
>>   static int use_bdrv_whitelist;
>>
>> +int file_open(const char *filename, int flags, mode_t mode)
>> +{
>> +#ifndef _WIN32
>> +    int fd;
>> +    const char *p;
>> +
>> +    if (strstart(filename, "/dev/fd/",&p)) {
>> +        fd = atoi(p);
>> +        return dup(fd);
>> +    }
>> +#endif
>> +
>> +    return qemu_open(filename, flags, mode);
>> +}
>
> What's the reason for having separate file_open() and qemu_open()
> functions? Are there places where you don't want allow to use /dev/fd?
> Otherwise I would have expected that we just extend qemu_open().

I'm not sure there's any good reason to have a separate file_open() vs 
just adding this code to qemu_open().  I followed the lead of Anthony's 
prototype which used file_open() so he may have a reason.  Otherwise 
I'll move the code to qemu_open() in v2.

>
> I'd have the remaining open ->  qemu_open conversions as a separate patch
> then. They do not only add fd support as advertised in the commit
> message, but also add O_CLOEXEC. I don't think that's bad, these are
> likely bonus bug fixes, but they should be mentioned in the commit message.

Yes good point.  I'll be sure to note the addition of O_CLOEXEC for 
these cases in the commit message.

-- 
Regards,
Corey