1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/3] Couple of qemu NS fixes
  4. View patch

[libvirt] [PATCH 1/3] qemuDomainBuildNamespace: Clean up temp files

Michal Privoznik posted 3 patches 8 years, 6 months ago
[libvirt] [PATCH 1/3] qemuDomainBuildNamespace: Clean up temp files
Posted by Michal Privoznik 8 years, 6 months ago 
https://bugzilla.redhat.com/show_bug.cgi?id=1431112

After 290a00e41d we know how to deal with file mount points.
However, when cleaning up the temporary location for preserved
mount points we are still calling rmdir(). This won't fly for
files. We need to call unlink(). Now, since we don't really care
if the cleanup succeeded or not (it's the best effort anyway), we
can call both rmdir() and unlink() without need for
differentiation between files and directories.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_domain.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 36fa450e8..23b92606e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8311,8 +8311,11 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,
 
     ret = 0;
  cleanup:
-    for (i = 0; i < ndevMountsPath; i++)
+    for (i = 0; i < ndevMountsPath; i++) {
+        /* The path can be either a regular file or a dir. */
         rmdir(devMountsSavePath[i]);
+        unlink(devMountsSavePath[i]);
+    }
     virStringListFreeCount(devMountsPath, ndevMountsPath);
     virStringListFreeCount(devMountsSavePath, ndevMountsPath);
     return ret;
-- 
2.13.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] qemuDomainBuildNamespace: Clean up temp files
Posted by John Ferlan 8 years, 6 months ago 

On 06/12/2017 11:57 AM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1431112
> 
> After 290a00e41d we know how to deal with file mount points.
> However, when cleaning up the temporary location for preserved
> mount points we are still calling rmdir(). This won't fly for
> files. We need to call unlink(). Now, since we don't really care
> if the cleanup succeeded or not (it's the best effort anyway), we
> can call both rmdir() and unlink() without need for
> differentiation between files and directories.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_domain.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 

But why call both?

My recollection on this is that unlink() was preferred over rmdir()
unless you cared about the target not existing. It's also 'kinder' if
some existing reference was left on the file.

I would prefer just the unlink for my:

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

but you can convince me for having both as well...

John

Feels like an eblake question, but he's away for a couple weeks, sigh.

> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 36fa450e8..23b92606e 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -8311,8 +8311,11 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,
>  
>      ret = 0;
>   cleanup:
> -    for (i = 0; i < ndevMountsPath; i++)
> +    for (i = 0; i < ndevMountsPath; i++) {
> +        /* The path can be either a regular file or a dir. */
>          rmdir(devMountsSavePath[i]);
> +        unlink(devMountsSavePath[i]);
> +    }
>      virStringListFreeCount(devMountsPath, ndevMountsPath);
>      virStringListFreeCount(devMountsSavePath, ndevMountsPath);
>      return ret;
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] qemuDomainBuildNamespace: Clean up temp files
Posted by Michal Privoznik 8 years, 6 months ago 
On 06/14/2017 09:50 PM, John Ferlan wrote:
> 
> 
> On 06/12/2017 11:57 AM, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1431112
>>
>> After 290a00e41d we know how to deal with file mount points.
>> However, when cleaning up the temporary location for preserved
>> mount points we are still calling rmdir(). This won't fly for
>> files. We need to call unlink(). Now, since we don't really care
>> if the cleanup succeeded or not (it's the best effort anyway), we
>> can call both rmdir() and unlink() without need for
>> differentiation between files and directories.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/qemu/qemu_domain.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
> 
> But why call both?

I don't think you can call unlink() over a directory, can you? And sure,
I could call stat() just to find out if it's a dir or a file and call
just one of the pair. Or I can call both and ignore any errors. The
result is the same, isn't it?

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] qemuDomainBuildNamespace: Clean up temp files
Posted by John Ferlan 8 years, 6 months ago 

On 06/15/2017 04:53 AM, Michal Privoznik wrote:
> On 06/14/2017 09:50 PM, John Ferlan wrote:
>>
>>
>> On 06/12/2017 11:57 AM, Michal Privoznik wrote:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1431112
>>>
>>> After 290a00e41d we know how to deal with file mount points.
>>> However, when cleaning up the temporary location for preserved
>>> mount points we are still calling rmdir(). This won't fly for
>>> files. We need to call unlink(). Now, since we don't really care
>>> if the cleanup succeeded or not (it's the best effort anyway), we
>>> can call both rmdir() and unlink() without need for
>>> differentiation between files and directories.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>> ---
>>>  src/qemu/qemu_domain.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>
>> But why call both?
> 
> I don't think you can call unlink() over a directory, can you? And sure,
> I could call stat() just to find out if it's a dir or a file and call
> just one of the pair. Or I can call both and ignore any errors. The
> result is the same, isn't it?
> 
> Michal
> 

>From the unlink(3p) man page:

"The path argument shall not name a directory unless the process has
appropriate privileges and the implementation supports using unlink() on
directories."

Then a google search on using unlink vs. rmdir uncovers more refs. I
suppose one could also do the "if file, then unlink else rmdir.

Just seems "odd" to see both and leaves one wondering why.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] qemuDomainBuildNamespace: Clean up temp files
Posted by Michal Privoznik 8 years, 6 months ago 
On 06/15/2017 02:03 PM, John Ferlan wrote:
> 
> 
> On 06/15/2017 04:53 AM, Michal Privoznik wrote:
>> On 06/14/2017 09:50 PM, John Ferlan wrote:
>>>
>>>
>>> On 06/12/2017 11:57 AM, Michal Privoznik wrote:
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1431112
>>>>
>>>> After 290a00e41d we know how to deal with file mount points.
>>>> However, when cleaning up the temporary location for preserved
>>>> mount points we are still calling rmdir(). This won't fly for
>>>> files. We need to call unlink(). Now, since we don't really care
>>>> if the cleanup succeeded or not (it's the best effort anyway), we
>>>> can call both rmdir() and unlink() without need for
>>>> differentiation between files and directories.
>>>>
>>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>>> ---
>>>>  src/qemu/qemu_domain.c | 5 ++++-
>>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>
>>> But why call both?
>>
>> I don't think you can call unlink() over a directory, can you? And sure,
>> I could call stat() just to find out if it's a dir or a file and call
>> just one of the pair. Or I can call both and ignore any errors. The
>> result is the same, isn't it?
>>
>> Michal
>>
> 
> From the unlink(3p) man page:
> 
> "The path argument shall not name a directory unless the process has
> appropriate privileges and the implementation supports using unlink() on
> directories."

That's weird. I don't see that in my man page. What I see though is the
following errno code:

 EISDIR pathname refers to a directory.  (This is the non-POSIX value
returned by Linux since 2.1.132.)

The "non-POSIX" bothers me there. But I agree that whole namespaces are
Linux specific, so it doesn't bother me there that much.

> 
> Then a google search on using unlink vs. rmdir uncovers more refs. I
> suppose one could also do the "if file, then unlink else rmdir.

Well, to determine if a path is a file or a dir I'd need to call stat()
which can fail. I'm not a big fan of overcomplicating simple code.

> 
> Just seems "odd" to see both and leaves one wondering why.

Well, there's a comment that says why.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] qemuDomainBuildNamespace: Clean up temp files
Posted by John Ferlan 8 years, 6 months ago 

On 06/15/2017 09:44 AM, Michal Privoznik wrote:
> On 06/15/2017 02:03 PM, John Ferlan wrote:
>>
>>
>> On 06/15/2017 04:53 AM, Michal Privoznik wrote:
>>> On 06/14/2017 09:50 PM, John Ferlan wrote:
>>>>
>>>>
>>>> On 06/12/2017 11:57 AM, Michal Privoznik wrote:
>>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1431112
>>>>>
>>>>> After 290a00e41d we know how to deal with file mount points.
>>>>> However, when cleaning up the temporary location for preserved
>>>>> mount points we are still calling rmdir(). This won't fly for
>>>>> files. We need to call unlink(). Now, since we don't really care
>>>>> if the cleanup succeeded or not (it's the best effort anyway), we
>>>>> can call both rmdir() and unlink() without need for
>>>>> differentiation between files and directories.
>>>>>
>>>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>>>> ---
>>>>>  src/qemu/qemu_domain.c | 5 ++++-
>>>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>
>>>>
>>>> But why call both?
>>>
>>> I don't think you can call unlink() over a directory, can you? And sure,
>>> I could call stat() just to find out if it's a dir or a file and call
>>> just one of the pair. Or I can call both and ignore any errors. The
>>> result is the same, isn't it?
>>>
>>> Michal
>>>
>>
>> From the unlink(3p) man page:
>>
>> "The path argument shall not name a directory unless the process has
>> appropriate privileges and the implementation supports using unlink() on
>> directories."
> 
> That's weird. I don't see that in my man page. What I see though is the
> following errno code:
> 
>  EISDIR pathname refers to a directory.  (This is the non-POSIX value
> returned by Linux since 2.1.132.)
> 
> The "non-POSIX" bothers me there. But I agree that whole namespaces are
> Linux specific, so it doesn't bother me there that much.
> 

I got that from my Fedora install... But I know I've been down this path
before using other U*x OS's - those details have long since left my
short term memory.  Like I said, eblake would be of help here, but he's
away for another week or so...

>>
>> Then a google search on using unlink vs. rmdir uncovers more refs. I
>> suppose one could also do the "if file, then unlink else rmdir.
> 
> Well, to determine if a path is a file or a dir I'd need to call stat()
> which can fail. I'm not a big fan of overcomplicating simple code.
> 
>>
>> Just seems "odd" to see both and leaves one wondering why.
> 
> Well, there's a comment that says why.
> 
> Michal
> 

I recall going down this path before when I updated virFileRemove when
dealing with NFS and the craziness that is a root-squash environment.

I would think the following would work just fine in this case (as
cut-n-paste'd from virFileRemove):

       if (virFileIsDir(path))
           return rmdir(path);
       else
           return unlink(path);

You're ignoring errors anyway so whether stat() fails [in virFileIsDir]
or whether unlink() fails because it's being run on a directory in the
event that virFileIsDir() fails won't matter. But at least you wouldn't
always cause some failure.

The way you've changed things you'll always get an error that's ignored.
If rmdir() succeeded, then unlink() would fail (on directory). If
rmdir() fails (on file), then unlink() could/should/would succeed.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.