Re: wip-libcephfs rebased and pulled up to v.65

* Re: wip-libcephfs rebased and pulled up to v.65
       [not found]   ` <941FC8BE87ADE64D90E4EB6CD554517710023305@EPBYMINSA0032.epam.com>
@ 2013-07-11 19:01     ` Sage Weil
  2013-07-12  5:44       ` HA: " Ilya Storozhilov
  2013-07-12 11:23       ` Ilya Storozhilov
  0 siblings, 2 replies; 8+ messages in thread
From: Sage Weil @ 2013-07-11 19:01 UTC (permalink / raw)
  To: Ilya Storozhilov
  Cc: Adam C. Emerson, Matt W. Benjamin, David Zafman, ceph-devel

Please check out the current wip=libcephfs branch and let me know how it 
looks/works for you guys.  I cleaned up your patches a bit and fixed teh 
root cause of the xattr issue you were seeing.

Thanks!
sage

On Thu, 11 Jul 2013, Ilya Storozhilov wrote:

> Hi Adam (CC: Sage, Matt and David),
> 
> it seems to me we have choosen a bad commit description, where instead of "FIX: readlink not copy link path to user's buffer" there should be something like "FIX: ceph_ll_readlink() now fills user provided buffer with the link data instead of returning a pointer to the libcephfs internal memory location". Take a look to the sourcecode - it does actually what you are talking about (see https://github.com/ferustigris/ceph/blob/wip-libcephfs-rebased-v65/src/libcephfs.cc):
> 
> -----------------------------------------------------------------------
> extern "C" int ceph_ll_readlink(struct ceph_mount_info *cmount, Inode *in, char *buf, size_t bufsiz, int uid, int gid)
> {
>   const char *value = NULL;
>   int res = (cmount->get_client()->ll_readlink(in, &value, uid, gid));
>   if (res < 0)
>     return res;
>   if (bufsiz < (size_t)res)
>     return ENAMETOOLONG;
>   memcpy(buf, value, res);  // <-- Here we are copying a link data to the user provided buffer. This is what you want us to do.
>   return res;
> }
> -----------------------------------------------------------------------
> 
> In your branch (see https://github.com/linuxbox2/linuxbox-ceph/blob/wip-libcephfs-rebased-v65/src/libcephfs.cc) this function does not copy link data to the user-provided buffer, but passes back a pointer to the internal libcephfs structure, which is not good solution, as you mentioned below:
> 
> -----------------------------------------------------------------------
> extern "C" int ceph_ll_readlink(struct ceph_mount_info *cmount, Inode *in, char **value, int uid, int gid)
> {
>   return (cmount->get_client()->ll_readlink(in, (const char**) value, uid, gid));
> }
> -----------------------------------------------------------------------
> 
> Regards,
> Ilya
> 
> ________________________________________
> ??: Adam C. Emerson [aemerson@linuxbox.com]
> ??????????: 10 ???? 2013 ?. 20:41
> To: Ilya Storozhilov
> Cc: Sage Weil; Matt W. Benjamin; David Zafman
> ????: Re: wip-libcephfs rebased and pulled up to v.65
> 
> At Wed, 10 Jul 2013 12:17:24 +0000, Ilya Storozhilov wrote:
> [snip]
> > ?wip-libcephfs-rebased-v65? branch of the https://github.com/linuxbox2/linuxbox-ceph repository has not been branched from the ?wip-libcephfs? branch of https://github.com/ceph/ceph as it was made with our ?open_by_handle_api? branch of the https://github.com/ferustigris/ceph repo. That is why we were not able to automatically ?cherry-pick? our changes using respective git command. So we have manually applied our changes to the ?wip-libcephfs-rebased-v65? branch in https://github.com/ferustigris/ceph repo as one commit - you can check it out here: https://github.com/ferustigris/ceph/commit/c3f4940b2cfcfd3ea9a004e6f07f1aa3c0b6c419.
> [snip]
> 
> Good afternoon, sir.
> 
> I was looking at your patch and Matt and I have concerns about the
> change you made to readlink.  By passing back a pointer to a buffer
> rather than copying the link into a supplied buffer, we're opening
> ourselves up to the content changing, being deallocated, or otherwise
> having something bad happen to it.
> 
> Thanks.
> 
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread