Re: [Gluster-devel] trusted.glusterfs.location

[Ville Tuulos]

On Sun, 23 Aug 2009, Anand Avati wrote:

> Can you please submit this patch via git format-patch and
> mutt/git-send-email it to gluster-devel@ ? More comments inline -

I can re-send the patch once I'm sure that it does what it should.

> > I finally managed to get the trusted.glusterfs.location work with 2.0.6. I
> > needed to make some minor modifications to the patch bfb7af7aa316ba, which
> > adds support for trusted.glusterfs.location, to make it work with the latest
> > release.
> >
> > For some reason, getfattr always returned a zero-length value although it
> > recognized the key. I tracked down this problem to dict_set_static_ptr in
> > posix.c. Changing this to dict_set_str seemed to fix the problem, combined
> > with a small change in fuse-bridge.c to drop the zero byte in the end of the
> > value.
>
> I believe that the fuse-bridge.c change alone fixed the nul
> termination problem. Using dict_set_str in this case is dangerous and
> will result in double freeing of the priv->hostname pointer. Please
> see comments further below

I'm not sure about that. It seems that the client receives zero bytes of 
the value with the original dict_set_static_ptr(). Of course I don't the 
codebase intimately - I just tried to debug this particular problem. See 
below for a comment about dict_set_str.

> > A trickier issue is that the trusted.* namespace is only accessible with the
> > CAP_SYS_ADMIN capability in linux. This means that ordinary users can't
> > resolve locations of files as before. Also, we're using OpenVZ containers
> > for virtualization and apparently not even root has the CAP_SYS_ADMIN
> > capability in a virtual machine, so trusted.glusterfs.location can't be
> > accessed in a container at all.
> >
> > Is there some particular reason why you couldn't expose the attribute in the
> > user.* namespace? I patched our gluster to use user.glusterfs.location,
> > which solves both the issue with ordinary users and makes the attribute
> > available to openvz containers.
>
> There is no particular reason. Using user.* should be just as fine.

Great! This would be really helpful.

> > Find below a patch against 2.0.6 that fixes the aforementioned issues. I'd
> > appreciate if you could consider using the user.* namespace in 2.0.7. Also,
> > as I mentioned in an earlier email, it'd be useful to know all locations of
> > the file when cluster/replicate is in use, to let Disco and others utilize
> > replicas in case that the primary copy fails.
>
> This would need some code change in replicate's getxattr call.
> Currently it picks an (arbitrary) subvolume and forwards the xattr
> read call to it. It should be enhanced to forward the
> "user.glusterfs.location" key call to all subvolumes and merge the
> values in a suitable way (comma separated?).

Hmm, is it really an arbitrary subvolume, i.e. not the one that Gluster 
actually uses as the primary copy? A comma separated and ordered list 
would be perfect for Disco.

> > diff --git a/xlators/mount/fuse/src/fuse-bridge.c
> > b/xlators/mount/fuse/src/fuse-bridge.c
> > index 61fd257..011b280 100644
> > --- a/xlators/mount/fuse/src/fuse-bridge.c
> > +++ b/xlators/mount/fuse/src/fuse-bridge.c
> > @@ -2000,7 +2000,7 @@ fuse_xattr_cbk (call_frame_t *frame, void *cookie,
> > xlator_t *this,
> >                         /* if callback for getxattr */
> >                         value_data = dict_get (dict, state->name);
> >                         if (value_data) {
> > -                                ret = value_data->len; /* Don't return the
> > value for '\0' */
> > +                                ret = value_data->len - 1; /* Don't return
> > the value for '\0' */
>
> This is not a good idea since all data being returned is not
> necessarily a null terminated string. You might want to use
> dict_set_static_bin() with appropriate data length (without counting
> the nul).

Right, good point.

> >        if (loc->inode && S_ISREG (loc->inode->st_mode) && name &&
> > -           (strcmp (name, "trusted.glusterfs.location") == 0)) {
> > -                ret = dict_set_static_ptr (dict,
> > -                                           "trusted.glusterfs.location",
> > -                                           priv->hostname);
> > +           (strcmp (name, "user.glusterfs.location") == 0)) {
> > +                ret = dict_set_str (dict,
> > +                              "user.glusterfs.location",
> > +                                priv->hostname);
>
> dict_set_static_str is the right function to use here. For your case
> use dict_set_static_bin() with len as strlen(priv->hostname).

I'm not sure what you mean by dict_set_static_str? At least dict.c in 
2.0.6 doesn't have such a function.

I tried to understand how different dict_set_* functions work, and looking 
at dict_set_str, it seems to do just

        data_t *data = get_new_data ();
        data->len = strlen (value) + 1;

        data->data = value;
        data->is_static = 1;

whereas dict_set_static_bin does

        data_t *data = get_new_data ();

        data->is_static = 1;
        data->len = len;
        data->data = value;

To me using dict_set_static_bin(dict, "...", priv->hostname, 
strlen(priv->hostname) + 1) seem exactly same as dict_set_str(). I wonder 
what you meant by "Using dict_set_str in this case is dangerous and will 
result in double freeing of the priv->hostname pointer".

I'm sorry if I've missed something obvious. I'm just trying to understand 
the codebase better.


Thanks for your advice again!


Ville