[libvirt] [PATCH 2/8] qemuDomainBuildNamespace: Handle special file mount points

[John Ferlan]

On 07/11/2017 01:14 AM, Michal Privoznik wrote:
> On 06/27/2017 11:37 PM, John Ferlan wrote:
>>
>>
>> On 06/22/2017 12:18 PM, Michal Privoznik wrote:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1459592
>>>
>>> In 290a00e41d I've tried to fix the process of building a
>>> qemu namespace when dealing with file mount points. What I
>>> haven't realized then is that we might be dealing not with just
>>> regular files but also special files (like sockets). Indeed, try
>>> the following:
>>>
>>> 1) socat unix-listen:/tmp/soket stdio
>>> 2) touch /dev/socket
>>> 3) mount --bind /tmp/socket /dev/socket
>>> 4) virsh start anyDomain
>>>
>>> Problem with my previous approach is that I wasn't creating the
>>> temporary location (where mount points under /dev are moved) for
>>> anything but directories and regular files.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>>> ---
>>>  src/qemu/qemu_domain.c | 6 ++++--
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>>> index 8e7404da6..212717c80 100644
>>> --- a/src/qemu/qemu_domain.c
>>> +++ b/src/qemu/qemu_domain.c
>>> @@ -8356,9 +8356,11 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,
>>>              goto cleanup;
>>>          }
>>>  
>>> -        /* At this point, devMountsPath is either a regular file or a directory. */
>>> +        /* At this point, devMountsPath is either:
>>> +         * a file (regular or special), or
>>> +         * a directory. */
>>>          if ((S_ISDIR(sb.st_mode) && virFileMakePath(devMountsSavePath[i]) < 0) ||
>>> -            (S_ISREG(sb.st_mode) && virFileTouch(devMountsSavePath[i], sb.st_mode) < 0)) {
>>> +            (!S_ISDIR(sb.st_mode) && virFileTouch(devMountsSavePath[i], sb.st_mode) < 0)) {
>>
>> It would seem to me that this would open Pandora's box to all different
>> types of things (BLK, CHR, FIFO, LNK, NAM, MPB, MPC, NWK) - some of
>> which it may not be so popular to perform a touch on.
>>
>> I think you should keep it specific...  Perhaps use the list from
>> qemuDomainCreateDeviceRecursive:
>>
>>    isReg = S_ISREG(sb.st_mode) || S_ISFIFO(sb.st_mode) ||
>> S_ISSOCK(sb.st_mode);
> 
> Not really. mount --bind makes the target to be the correct type. IOW:
> 

OK - I wasn't sure what all those other things were and going with
!IS_DIR just seemed to open the door to unsurety for me.  Since there
was other code that was more restrictive to decide "ifReg", I figured
using that same list would be fine, but I'm not sure if I ever check
where in the scheme of the path the other check is make.

If you're fine with how things are, then fine consider it...

Reviewed-by: John Ferlan <jferlan at redhat.com>

John

> # create a regular file
> touch /tmp/blah
> 
> # here, assume /source/socket is a socket
> mount --bind /source/socket /tmp/blah
> 
> # now, /tmp/blah will be type of socket too
> 
> The only problem here is that for file based 'devices' (or things in
> general) we have to create the file whereas for directories we have to
> create directories. Just like in the example.
> 
> BTW, what type of file are NAM, MPB, MPC, NWK?
> 
> Michal
>