search

plfs / plfs-core

LANL no longer develops PLFS. Feel free to fork and develop as you wish.
41 stars 36 forks source link

the use of plfs_getxattr relies upon the user knowing the length of the extended attribute #335

Open dshrader opened 10 years ago

dshrader commented 10 years ago

Please see Issue #331 for where this behavior was noticed. In short, we are not following the capabilities of POSIX getxattr in that a user can find out how big the xattr is so that they can make sure they have enough memory for it. Right now, we rely on the user to provide the length of the attribute, which is somewhat naive. If the length is too small, we return a truncated value; if the length is too big, we read off the end of the file that houses the xattr (generates an error).

dshrader commented 10 years ago

Consensus from comments in 331 suggest adding another argument to plfs_getxattr would be appropriate. Additional error checking should occur to make sure what the user passes as the length is big enough for the xattr. It doesn't look like we have any code that can determine the size of an already set xattr, so this would have to be developed. Probably in XAttr.cpp...

chuckcranor commented 10 years ago

On Thu, Dec 12, 2013 at 09:47:32AM -0800, David wrote:

Consensus from comments in 331 suggest adding another argument to plfs_getxattr would be appropriate. Additional error checking should occur to make sure what the user passes as the length is big enough for the xattr. It doesn't look like we have any code that can determine the size of an already set xattr, so this would have to be developed. Probably in XAttr.cpp...

prob just IOStore Stat() the key file...? st_size of the file would be the size of the value.

struct stat st; full_path = path + "/" + key; err = this->canback->store->Stat( full_path.c_str(), &st);

chuck

johnbent commented 10 years ago

Also add length as a parameter to setattr. getattr can discover length by stat'ing the file, no?

On Dec 12, 2013, at 10:47 AM, David notifications@github.com wrote:

Consensus from comments in 331 suggest adding another argument to plfs_getxattr would be appropriate. Additional error checking should occur to make sure what the user passes as the length is big enough for the xattr. It doesn't look like we have any code that can determine the size of an already set xattr, so this would have to be developed. Probably in XAttr.cpp...

— Reply to this email directly or view it on GitHub.

brettkettering commented 10 years ago

The client user should definitely not stat the file, as that's assuming a certain implementation and the implementation can change. It's safest to have a library call that should know the implementation get the value.

Brett

-----Original Message----- From: John Bent [notifications@github.commailto:notifications@github.com] Sent: Thursday, December 12, 2013 11:52 AM Mountain Standard Time To: plfs/plfs-core Subject: Re: [plfs-core] the use of plfs_getxattr relies upon the user knowing the length of the extended attribute (#335)

Also add length as a parameter to setattr. getattr can discover length by stat'ing the file, no?

On Dec 12, 2013, at 10:47 AM, David notifications@github.com wrote:

Consensus from comments in 331 suggest adding another argument to plfs_getxattr would be appropriate. Additional error checking should occur to make sure what the user passes as the length is big enough for the xattr. It doesn't look like we have any code that can determine the size of an already set xattr, so this would have to be developed. Probably in XAttr.cpp...

— Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHubhttps://github.com/plfs/plfs-core/issues/335#issuecomment-30450192.

dshrader commented 10 years ago

plfs_setxattr already has a required length parameter, so I don't think it needs to change:

plfs_setxattr(Plfs_fd fd, const void value, const char *key, size_t len)

It looks like we do follow POSIX's setxattr syntax as the return value for setxattr is 0 for success, -1 for failure, which is basically what our plfs_errno does. So I think we're ok here unless I have missed something.

johnbent commented 10 years ago

The client user would not be doing the stat. The stat would be done internally within the plfs code. Chuck posted a similar comment above where he explained it better than I did.

On Thu, Dec 12, 2013 at 12:47 PM, Brett Kettering notifications@github.comwrote:

The client user should definitely not stat the file, as that's assuming a certain implementation and the implementation can change. It's safest to have a library call that should know the implementation get the value.

Brett

-----Original Message----- From: John Bent [notifications@github.commailto:notifications@github.com]

Sent: Thursday, December 12, 2013 11:52 AM Mountain Standard Time To: plfs/plfs-core Subject: Re: [plfs-core] the use of plfs_getxattr relies upon the user knowing the length of the extended attribute (#335)

Also add length as a parameter to setattr. getattr can discover length by stat'ing the file, no?

On Dec 12, 2013, at 10:47 AM, David notifications@github.com wrote:

Consensus from comments in 331 suggest adding another argument to plfs_getxattr would be appropriate. Additional error checking should occur to make sure what the user passes as the length is big enough for the xattr. It doesn't look like we have any code that can determine the size of an already set xattr, so this would have to be developed. Probably in XAttr.cpp...

— Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHub< https://github.com/plfs/plfs-core/issues/335#issuecomment-30450192>.

— Reply to this email directly or view it on GitHubhttps://github.com/plfs/plfs-core/issues/335#issuecomment-30455126 .

dshrader commented 10 years ago

We should probably target this fix to go in to 2.6 since it will change the external API.