Re: [libvirt] [PATCH 2/8] qemuDomainBuildNamespace: Handle special file mount points
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(a)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(a)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