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

[Anand Avati]

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


> 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

> 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.

> 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?).

> 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).


>        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).

Avati