[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