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
