Re: [libvirt] [PATCH 1/3] qemuDomainBuildNamespace: Clean up temp files
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(a)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