Setting an attribute using plfs_setxattr and then getting it using plfs_getxattr

[hadimontakhabi]

Here is what I have now.
I am opening a file, writing something to it, then setting an attribute (num_hostdir), and then I try to get the same attribute.
The value that I get is different than what I set.
Any thoughts on that?

int main()
{
Plfs_fd *pfd = NULL;
char wpath[100];
char buf[] = "somrthing to write";
int offset = 0;
ssize_t bytes;
char filename[] = "filename.txt";
int flags;
plfs_error_t plfs_ret;
int value = 10;
size_t len = sizeof(int);
char key[] = "num_hostdirs";

getcwd(wpath, sizeof(wpath));
sprintf(wpath,"%s/%s",wpath,filename);
fprintf(stdout, "wpath: %s\n", wpath);

plfs_open( &pfd, wpath, O_CREAT | O_WRONLY | O_RDONLY, 0, 0666, NULL );
plfs_write( pfd, buf, sizeof(buf), offset, 0, &bytes );

plfs_ret = plfs_setxattr( pfd, &value, key, len );
if (plfs_ret != PLFS_SUCCESS) {
printf("ERROR in plf_setxattr:\n%s\n", strplfserr(plfs_ret));
}
else {
printf("after setting: num_hostdir = %d\n",value);
}

plfs_ret = plfs_getxattr( pfd, &value, key, len );
if (plfs_ret != PLFS_SUCCESS) {
printf("ERROR in plf_getxattr:\n%s\n", strplfserr(plfs_ret));
}
else {
printf("after getting: num_hostdir = %d\n",value);
}

plfs_ret = plfs_close(pfd, 0, 0, O_CREAT | O_WRONLY | O_RDONLY ,NULL, &flags);
return 0;
}

[chuckcranor]

On Thu, Dec 05, 2013 at 12:39:26PM -0800, hadimontakhabi wrote:

Here is what I have now.
I am opening a file, writing something to it, then setting an attribute (num_hostdir), and then I try to get the same attribute.
The value that I get is different than what I set.
Any thoughts on that?

did you solve that? the xattr code is pretty simple (see
src/XAttrs.{h,cpp}) so it should be easy to check now that
you can call the functions.

also you can look at the files on the backend. should store
the xattr in the "xattrs" subdir of the container directory.
i'd look at that.

chuck

[brettkettering]

I've assigned this to David Shrader, whose job assignment includes more PLFS development this year. Thanks for the inside information about the xattrs subdirectory. That will help David to investigate this bug.

That said, I don't discourage Hadi from investigating on his own. If he finds a solution, he can let us know.

Thanks,
Brett

From: chuckcranor <notifications@github.commailto:notifications@github.com>
Reply-To: plfs/plfs-core <reply@reply.github.commailto:reply@reply.github.com>
Date: Friday, December 6, 2013 11:02 AM
To: plfs/plfs-core <plfs-core@noreply.github.commailto:plfs-core@noreply.github.com>
Subject: Re: [plfs-core] Setting an attribute using plfs_setxattr and then getting it using plfs_getxattr (#331)

On Thu, Dec 05, 2013 at 12:39:26PM -0800, hadimontakhabi wrote:

Here is what I have now.
I am opening a file, writing something to it, then setting an attribute (num_hostdir), and then I try to get the same attribute.
The value that I get is different than what I set.
Any thoughts on that?

did you solve that? the xattr code is pretty simple (see
src/XAttrs.{h,cpp}) so it should be easy to check now that
you can call the functions.

also you can look at the files on the backend. should store
the xattr in the "xattrs" subdir of the container directory.
i'd look at that.

chuck

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/331#issuecomment-30015043.

[hadimontakhabi]

I haven't figured it out yet, but I will look into it.

[dshrader]

I'm looking in to this now, so I thought I would put down what I have found.

I have reproduced the behavior. That is, the number returned for num_hostdirs from plfs_getxattr is different than that set by plfs_setxattr. The value returned by plfs_getxattr seems to be random.

Looking in the xattrs directory for the file does have a num_hostdirs file, but it doesn't have anything in it that I can read. What format are the files in xattrs supposed to be? Binary?

[hadimontakhabi]

I don't think it is a binary file.
Thinking about it, a text file with the value of the parameter should do the job.

[chuckcranor]

On Wed, Dec 11, 2013 at 09:27:30AM -0800, hadimontakhabi wrote:

I don't think it is a binary file.
Thinking about it, a text file with the value of the parameter should do the job.

see plfs-core/src/XAttrs.cpp ... the key is stored as the filename
and the content of the file is the value. if it isn't working,
seems like it should be an easy debug, right? much easier than
the indexing stuff i'm working on...

chuck

simplified setXAttr:

plfs_error_t XAttrs::setXAttr(string key, const void* value, size_t len)
{
string full_path;

full_path = /* this-> */ path + "/" + key;
err = this->canback->store->Open( full_path.c_str(),
                                  O_WRONLY|O_CREAT|O_TRUNC, 0644, &fh);

ret = fh->Write(value, len, &write_len);

ret = this->canback->store->Close(fh);

return PLFS_SUCCESS;

}

[dshrader]

No so easy for me to find, but I think I have found the issue. Here is the code that I think is the issue from around line 90 of XAttrs.cpp:

char* value = (char*) malloc (len);
memcpy(value, &buf, len);
XAttr *xattr = new XAttr(key, (const void*)value);
free(value);

Here, buff and value contain what we just got from a call to Pread. The problem is that xattr doesn't copy the stuff from value; instead it is still pointing to it after the free command. In my debug sessions, as soon as value is freed, the xattr->value goes to something invalid. Since xattr->value is the result we want to pass back up, we pass the wrong thing up.

I have tested commenting out the "free(value)" line and everything seems to work. However, I am not sure that removing the free call is what we want. It has been a long time since I coded in C++. Do we need to free value? I mean, do we need to remove just the reference "value" without freeing the memory it is pointing to? Basically, we're allocating memory in or below XAttrs::getXAttr and passing that back up to whatever called it. Is that wise? Would it be better to allocate an XAttr before calling XAttrs::getXAttr and passing that in?

[dshrader]

For this issue, the calling function (Container_fd::getxattr) does delete the XAttr that is passed up to it. I think removing the free call should be sufficient, but I'd like others to weigh in just in case I have missed something.

[johnbent]

If you do that, then the preceding malloc() will leak memory. I think the
better solution would be to change the constructor to take a length
variable and then it will do a malloc into which to copy the value. Then
the destructor will need to free it.

Someone checked 80+ lines into XAttr.cpp. :(

John

On Wed, Dec 11, 2013 at 5:06 PM, David notifications@github.com wrote:

For this issue, the calling function (Container_fd::getxattr) does delete
the XAttr that is passed up to it. I think removing the free call should be
sufficient, but I'd like others to weigh in just in case I have missed
something.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/331#issuecomment-30377769
.

[chuckcranor]

On Wed, Dec 11, 2013 at 04:25:18PM -0800, John Bent wrote:

If you do that, then the preceding malloc() will leak memory. I think the
better solution would be to change the constructor to take a length
variable and then it will do a malloc into which to copy the value. Then
the destructor will need to free it.

that seems reasonable. class XAttr should be tracking the length
of value anyway so that callers know how big the buffer is. doesn't
make sense not to track that length.

chuck

[dshrader]

I've been talking with David B about this issue, and we have implemented keeping track of the length inside the XAttr structure.

However, we have discovered that we have been violating the standard convention for getxattr. From the man page for getxattr:

ssize_t getxattr (const char *path, const char *name,
                            void *value, size_t size);
...
       An empty buffer of size zero can be passed into these calls to return  the  current  size  of  the  named
       extended  attribute,  which  can  be used to estimate the size of a buffer which is sufficiently large to
       hold the value associated with the extended attribute.

       The interface is designed to allow guessing of initial buffer sizes, and  to  enlarge  buffers  when  the
       return value indicates that the buffer provided was too small.

Right now, if we pass in a len of 0 to our plfs_getxattr, we get bad things (memcpy of 0). In other words, we are trusting the user to keep track of the size of the extended attribute that we are storing. If they give too big of a length, we read off the extended attribute's file (generates an error); too small and we return a truncated attribute (not helpful). We need a way to let the user know how big of a buffer they need. POSIX getxattr does this via the return value, which we cannot emulate with the new plfs error/return types. We could use another argument to take care of this, but this changes the external interface. This type of change doesn't change the MPI patch as it doesn't use extended attributes.

If we don't want to trust the user to always know how big our extended attributes are, we need to implement a solution to finding out how big an extended attribute is from the file that we drop in the xattrs directory (there is no sense of the XAttr class when plfs_getxattr is called).

[chuckcranor]

On Thu, Dec 12, 2013 at 08:32:34AM -0800, David wrote:

POSIX getxattr does this via the return value, which we cannot emulate
with the new plfs error/return types. We could use another argument to
take care of this, but this changes the external interface.

seems like we've already broken the POSIX thing with the plfs_error_t
changes (e.g. compare plfs_read API to POSIX read), so i'm thinking
that bridge is already crossed and we might as well add the arg.

chuck

[johnbent]

On Dec 12, 2013, at 9:32 AM, David notifications@github.com wrote:

We need a way to let the user know how big of a buffer they need. POSIX getxattr does this via the return value, which we cannot emulate with the new plfs error/return types.

Nice catch. Agreed.

We could use another argument to take care of this, but this changes the external interface.

I'd say let's add this extra argument and change the external interface. We'd have to do that anyway to let the user specify the size of the void* when they set it.

John

[dshrader]

Please take a look at the pull request and let me know if there are any objections. It only fixes the return value of plfs_getxattr; it doesn't deal with the issue that we found with plfs_getxattr and the length of the extended attribute. I'm going to open another issue for that and close this one when we merge in a fix.

[chuckcranor]

On Thu, Dec 12, 2013 at 09:35:36AM -0800, David wrote:

Please take a look at the pull request and let me know if there are
any objections. It only fixes the return value of plfs_getxattr; it
doesn't deal with the issue that we found with plfs_getxattr and the
length of the extended attribute. I'm going to open another issue for
that and close this one when we merge in a fix.

seems about right, but is it doing a double copy of the data?
can we get rid of the malloc/memcpy in XAttrs::getXAttr() and
just let the new one in the constructor do it?

chuck

[dshrader]

Yep, it is. I've made one more change that I'll add to the merge request.

[dshrader]

This should now be fixed in master. I just merged Pull Request #334.