[Cluster-devel] [PATCH] gfs2_grow: Don't try to open an empty string

[Steven Whitehouse]

Hi,

On Tue, 2013-08-13 at 10:04 +0100, Andrew Price wrote:
> On 09/08/13 10:41, Steven Whitehouse wrote:
> > Hi,
> >
> > On Thu, 2013-08-08 at 18:02 +0100, Andrew Price wrote:
> >> sdp->device_name wasn't getting set in gfs2_grow so is_gfs2() (called
> >> via check_for_gfs2()) fails to open "" and so we get an ENOENT.
> >>
> >> This fixes the open("") by setting sdp->device_name before passing it to
> >> is_pathname_mounted(), which has been updated to make it more clear
> >> which args will be modified by it.
> >>
> >> is_gfs2() and check_for_gfs2() have also been removed from libgfs2 as
> >> these were the only places they were being called and they aren't
> >> needed: superblock checking is done further along via other functions
> >> calling check_sb().
> >>
> > Definitely an improvement over what we had before, but I wonder whether
> > we can do better still. Is the fd always open before we call
> > is_pathname_mounted() I wonder? If so we should be able to just pass the
> > fd to it which reduces the possibility for races I think, and may also
> > simplify things a bit more,
> 
> Well the problem here is that we don't know whether the path is a device 
> or a mount point (gfs2_{grow,jadd} accept both) beforehand so any open 
> before is_pathname_mounted() is called is going to be a guess.
> 
It will be a guess, but at least once it is open, the object that is
referenced cannot change. We can then verify what we got and decide what
to do next with it, safe in the knowledge that nobody can change a
symlink (for example) under us and give us the wrong results later on.

> I think we probably can improve on this function in the process of 
> making libgfs2 more fd-centric but it'll take a bit more work so I'll 
> come back to that later and push this patch independently as it solves a 
> problem that's currently holding up testing.
> 
> Cheers,
> Andy

Yes, I think thats a good approach,

Steve.