[PATCH 0/3] Generic libcephfs Java wrappers

[PATCH 0/3] Generic libcephfs Java wrappers

[Noah Watkins]

Generic libcephfs Java wrappers.

This is the first of a patch series that can eventually replace the current Java wrappers that are Hadoop specific. This group implements enough of the libcephfs API to support Hadoop functionality, includes Debian packaging, autoconf magics that look for default Java installations, and some basic JUnit tests.

The patches are available in the git repository at:

  git://github.com/noahdesu/ceph.git wip/java-cephfs

Noah Watkins (3):
  java: add Java and C++ source files
  java: setup autotools to build cephfs-java
  debian: add libcephfs-java package

 configure.ac                                       |   74 ++
 debian/.gitignore                                  |    1 +
 debian/control                                     |    8 +-
 debian/libceph1-java.install                       |    2 +
 debian/rules                                       |    1 +
 src/Makefile.am                                    |   15 +-
 src/java/.gitignore                                |    4 +
 src/java/Makefile.am                               |   49 +
 src/java/README                                    |   43 +
 src/java/build.xml                                 |   67 ++
 .../java/net/newdream/ceph/fs/CephConstants.java   |   38 +
 .../java/net/newdream/ceph/fs/CephDirectory.java   |   84 ++
 .../java/net/newdream/ceph/fs/CephException.java   |   21 +
 .../ceph/fs/CephInvalidStateException.java         |   11 +
 src/java/java/net/newdream/ceph/fs/CephMount.java  |  360 +++++++
 .../net/newdream/ceph/fs/CephNativeLoader.java     |   16 +
 src/java/java/net/newdream/ceph/fs/CephProxy.java  |  217 ++++
 src/java/java/net/newdream/ceph/fs/CephStat.java   |   15 +
 .../java/net/newdream/ceph/fs/CephStatVFS.java     |   14 +
 src/java/java/net/newdream/ceph/fs/CephStruct.java |   33 +
 src/java/native/libcephfs_jni.cc                   | 1037 ++++++++++++++++++++
 src/java/test/CephMountCreateTest.java             |   76 ++
 src/java/test/CephMountTest.java                   |   72 ++
 23 files changed, 2256 insertions(+), 2 deletions(-)
 create mode 100644 debian/libceph1-java.install
 create mode 100644 src/java/.gitignore
 create mode 100644 src/java/Makefile.am
 create mode 100644 src/java/README
 create mode 100644 src/java/build.xml
 create mode 100644 src/java/java/net/newdream/ceph/fs/CephConstants.java
 create mode 100644 src/java/java/net/newdream/ceph/fs/CephDirectory.java
 create mode 100644 src/java/java/net/newdream/ceph/fs/CephException.java
 create mode 100644 src/java/java/net/newdream/ceph/fs/CephInvalidStateException.java
 create mode 100644 src/java/java/net/newdream/ceph/fs/CephMount.java
 create mode 100644 src/java/java/net/newdream/ceph/fs/CephNativeLoader.java
 create mode 100644 src/java/java/net/newdream/ceph/fs/CephProxy.java
 create mode 100644 src/java/java/net/newdream/ceph/fs/CephStat.java
 create mode 100644 src/java/java/net/newdream/ceph/fs/CephStatVFS.java
 create mode 100644 src/java/java/net/newdream/ceph/fs/CephStruct.java
 create mode 100644 src/java/native/libcephfs_jni.cc
 create mode 100644 src/java/test/CephMountCreateTest.java
 create mode 100644 src/java/test/CephMountTest.java

-- 
1.7.5.4

Re: [PATCH 0/3] Generic libcephfs Java wrappers

[Sage Weil]

Hey Noah-

This looks great!  I'd love to merge this for v0.44, but need your 
Signed-off-by: on the patches.  Also, if you want to switch to com.ceph 
while you're at it, that'd be good too.

I have a trivial makefile fix in wip-java you might want to look at.

Has any of the hadoop side work been done to use this yet?  Is there 
anything we can do help get that in a state where we can send it upstream?

Thanks!
sage


On Fri, 2 Mar 2012, Noah Watkins wrote:

> Generic libcephfs Java wrappers.
> 
> This is the first of a patch series that can eventually replace the current Java wrappers that are Hadoop specific. This group implements enough of the libcephfs API to support Hadoop functionality, includes Debian packaging, autoconf magics that look for default Java installations, and some basic JUnit tests.
> 
> The patches are available in the git repository at:
> 
>   git://github.com/noahdesu/ceph.git wip/java-cephfs
> 
> Noah Watkins (3):
>   java: add Java and C++ source files
>   java: setup autotools to build cephfs-java
>   debian: add libcephfs-java package
> 
>  configure.ac                                       |   74 ++
>  debian/.gitignore                                  |    1 +
>  debian/control                                     |    8 +-
>  debian/libceph1-java.install                       |    2 +
>  debian/rules                                       |    1 +
>  src/Makefile.am                                    |   15 +-
>  src/java/.gitignore                                |    4 +
>  src/java/Makefile.am                               |   49 +
>  src/java/README                                    |   43 +
>  src/java/build.xml                                 |   67 ++
>  .../java/net/newdream/ceph/fs/CephConstants.java   |   38 +
>  .../java/net/newdream/ceph/fs/CephDirectory.java   |   84 ++
>  .../java/net/newdream/ceph/fs/CephException.java   |   21 +
>  .../ceph/fs/CephInvalidStateException.java         |   11 +
>  src/java/java/net/newdream/ceph/fs/CephMount.java  |  360 +++++++
>  .../net/newdream/ceph/fs/CephNativeLoader.java     |   16 +
>  src/java/java/net/newdream/ceph/fs/CephProxy.java  |  217 ++++
>  src/java/java/net/newdream/ceph/fs/CephStat.java   |   15 +
>  .../java/net/newdream/ceph/fs/CephStatVFS.java     |   14 +
>  src/java/java/net/newdream/ceph/fs/CephStruct.java |   33 +
>  src/java/native/libcephfs_jni.cc                   | 1037 ++++++++++++++++++++
>  src/java/test/CephMountCreateTest.java             |   76 ++
>  src/java/test/CephMountTest.java                   |   72 ++
>  23 files changed, 2256 insertions(+), 2 deletions(-)
>  create mode 100644 debian/libceph1-java.install
>  create mode 100644 src/java/.gitignore
>  create mode 100644 src/java/Makefile.am
>  create mode 100644 src/java/README
>  create mode 100644 src/java/build.xml
>  create mode 100644 src/java/java/net/newdream/ceph/fs/CephConstants.java
>  create mode 100644 src/java/java/net/newdream/ceph/fs/CephDirectory.java
>  create mode 100644 src/java/java/net/newdream/ceph/fs/CephException.java
>  create mode 100644 src/java/java/net/newdream/ceph/fs/CephInvalidStateException.java
>  create mode 100644 src/java/java/net/newdream/ceph/fs/CephMount.java
>  create mode 100644 src/java/java/net/newdream/ceph/fs/CephNativeLoader.java
>  create mode 100644 src/java/java/net/newdream/ceph/fs/CephProxy.java
>  create mode 100644 src/java/java/net/newdream/ceph/fs/CephStat.java
>  create mode 100644 src/java/java/net/newdream/ceph/fs/CephStatVFS.java
>  create mode 100644 src/java/java/net/newdream/ceph/fs/CephStruct.java
>  create mode 100644 src/java/native/libcephfs_jni.cc
>  create mode 100644 src/java/test/CephMountCreateTest.java
>  create mode 100644 src/java/test/CephMountTest.java
> 
> -- 
> 1.7.5.4
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH 0/3] Generic libcephfs Java wrappers

[Noah Watkins]

On Mar 7, 2012, at 9:37 AM, Sage Weil wrote:

> Hey Noah-
> 
> This looks great!  I'd love to merge this for v0.44, but need your 
> Signed-off-by: on the patches.  Also, if you want to switch to com.ceph 
> while you're at it, that'd be good too.
> 
> I have a trivial makefile fix in wip-java you might want to look at.

Sounds great. I'll fold in your patches, fix-up the com.ceph path, and add the sign-off. When are you shipping v0.44? I'd like to include some more unit tests...

> Has any of the hadoop side work been done to use this yet?  Is there 
> anything we can do help get that in a state where we can send it upstream?

I've kept an eye on how much things are deviating from the current implementation and its pretty minimal, so I think producing a new version to use the generic libcephfs interface will happen quickly.

As for producing an upstream patch, I think the two biggest concerns are:

1. Reporting hostname/racks rather than IPs for object location.
2. Address (or at least document) the time synchronization issue.

These are largely orthogonal to getting the code ready for upstream, but may be a "political" road block? I can put together the details of these two issues in a separate thread…

Thanks,
Noah

> 
> Thanks!
> sage
> 
> 
> On Fri, 2 Mar 2012, Noah Watkins wrote:
> 
>> Generic libcephfs Java wrappers.
>> 
>> This is the first of a patch series that can eventually replace the current Java wrappers that are Hadoop specific. This group implements enough of the libcephfs API to support Hadoop functionality, includes Debian packaging, autoconf magics that look for default Java installations, and some basic JUnit tests.
>> 
>> The patches are available in the git repository at:
>> 
>>  git://github.com/noahdesu/ceph.git wip/java-cephfs
>> 
>> Noah Watkins (3):
>>  java: add Java and C++ source files
>>  java: setup autotools to build cephfs-java
>>  debian: add libcephfs-java package
>> 
>> configure.ac                                       |   74 ++
>> debian/.gitignore                                  |    1 +
>> debian/control                                     |    8 +-
>> debian/libceph1-java.install                       |    2 +
>> debian/rules                                       |    1 +
>> src/Makefile.am                                    |   15 +-
>> src/java/.gitignore                                |    4 +
>> src/java/Makefile.am                               |   49 +
>> src/java/README                                    |   43 +
>> src/java/build.xml                                 |   67 ++
>> .../java/net/newdream/ceph/fs/CephConstants.java   |   38 +
>> .../java/net/newdream/ceph/fs/CephDirectory.java   |   84 ++
>> .../java/net/newdream/ceph/fs/CephException.java   |   21 +
>> .../ceph/fs/CephInvalidStateException.java         |   11 +
>> src/java/java/net/newdream/ceph/fs/CephMount.java  |  360 +++++++
>> .../net/newdream/ceph/fs/CephNativeLoader.java     |   16 +
>> src/java/java/net/newdream/ceph/fs/CephProxy.java  |  217 ++++
>> src/java/java/net/newdream/ceph/fs/CephStat.java   |   15 +
>> .../java/net/newdream/ceph/fs/CephStatVFS.java     |   14 +
>> src/java/java/net/newdream/ceph/fs/CephStruct.java |   33 +
>> src/java/native/libcephfs_jni.cc                   | 1037 ++++++++++++++++++++
>> src/java/test/CephMountCreateTest.java             |   76 ++
>> src/java/test/CephMountTest.java                   |   72 ++
>> 23 files changed, 2256 insertions(+), 2 deletions(-)
>> create mode 100644 debian/libceph1-java.install
>> create mode 100644 src/java/.gitignore
>> create mode 100644 src/java/Makefile.am
>> create mode 100644 src/java/README
>> create mode 100644 src/java/build.xml
>> create mode 100644 src/java/java/net/newdream/ceph/fs/CephConstants.java
>> create mode 100644 src/java/java/net/newdream/ceph/fs/CephDirectory.java
>> create mode 100644 src/java/java/net/newdream/ceph/fs/CephException.java
>> create mode 100644 src/java/java/net/newdream/ceph/fs/CephInvalidStateException.java
>> create mode 100644 src/java/java/net/newdream/ceph/fs/CephMount.java
>> create mode 100644 src/java/java/net/newdream/ceph/fs/CephNativeLoader.java
>> create mode 100644 src/java/java/net/newdream/ceph/fs/CephProxy.java
>> create mode 100644 src/java/java/net/newdream/ceph/fs/CephStat.java
>> create mode 100644 src/java/java/net/newdream/ceph/fs/CephStatVFS.java
>> create mode 100644 src/java/java/net/newdream/ceph/fs/CephStruct.java
>> create mode 100644 src/java/native/libcephfs_jni.cc
>> create mode 100644 src/java/test/CephMountCreateTest.java
>> create mode 100644 src/java/test/CephMountTest.java
>> 
>> -- 
>> 1.7.5.4
>> --
>> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> 
>> 

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] Generic libcephfs Java wrappers

[Greg Farnum]

I went over all this today and had several line notes. Overall it looks good, though!  

To emphasize what Sage said about the package name: We should change that to com.ceph.fs.
CephMount::conf_get requires a buf_size. But we freely adjust that if we need more space, so is there any point to it? I can't see callers really having a better idea of size than we do.
(Oh, now I see there's another one that doesn't take buf_size. Do we have a use case for the with-size option, though?)
Similarly with getdnames
We don't implement CephMount::mkdir, just mkdirs? I *can* see use cases for not wanting recursive creates...
The ordering is a little funny for some functions — we see close() before open(), for instance. Maybe we should reorder these?
set_default_file_replication doesn't actually do anything internally — I thought that was documented somewhere. Even if it's not, it certainly should be here (and elsewhere, patches welcome! ;). (It passes the value to the Client, and the Client passes it to the MDS, but the MDS never looks at it).
I like the states and require_state() stuff. It's making me notice that you need a little more documentation on stuff like closedir and what happens to the CephDirectory under success and error returns. :)
The CHECK_ARG_* stuff could use some docs. I see now that CHECK_ARG_BOUNDS is expecting a full comparison, so it's probably just my lack of preprocessor idiom knowledge, but it weirded me out. And CHECK_ARG_COND appears to be unused.
do we need mount() when we can just run mount(null)? Similarly, I don't think we need an if-else in CephMountCreateTest::setupMount. :)




On Wednesday, March 7, 2012 at 10:02 AM, Noah Watkins wrote:

> I've kept an eye on how much things are deviating from the current implementation and its pretty minimal, so I think producing a new version to use the generic libcephfs interface will happen quickly.
>  
> As for producing an upstream patch, I think the two biggest concerns are:
>  
> 1. Reporting hostname/racks rather than IPs for object location.
> 2. Address (or at least document) the time synchronization issue.
>  
> These are largely orthogonal to getting the code ready for upstream, but may be a "political" road block? I can put together the details of these two issues in a separate thread…

Please do! I know I've seen most of these issues come up, but I'm still a bit fuzzy on the time sync issue.

Thanks!
-Greg


--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] Generic libcephfs Java wrappers

[Noah Watkins]

On Mar 7, 2012, at 1:40 PM, Greg Farnum wrote:

> CephMount::conf_get requires a buf_size. But we freely adjust that if we need more space, so is there any point to it? I can't see callers really having a better idea of size than we do.
Good point. I can't think of a reason to publicly expose the buffer length param in CephMount to users, but CephProxy is a private interface. On a related a related note:

   int ceph_conf_get(struct ceph_mount_info *cmount, const char *option, char *buf, size_t len);

shouldn't the 'buf' parameter be char ** if libcephfs is going to be doing a re-alloc, or is there some voodoo behind the scenes?


> (Oh, now I see there's another one that doesn't take buf_size. Do we have a use case for the with-size option, though?)
> Similarly with getdnames

Yes, the default buf_size interfaces are meant to be friendly, while the explicit buf_size interfaces are primarily meant for the unit tests so that the buffer expansion logic can be tested. It would be safe to leave it public, but maybe it is better for it to be protected and used explicitly by the unit tests.

> We don't implement CephMount::mkdir, just mkdirs? I *can* see use cases for not wanting recursive creates…

I was going for the minimum to support Hadoop. I'll stick mkdirs on the short list of API expansion TODOs.

> The ordering is a little funny for some functions — we see close() before open(), for instance. Maybe we should reorder these?

Yeh, definitely. I noticed that too for some, but clearly didn't catch them all.

> set_default_file_replication doesn't actually do anything internally — I thought that was documented somewhere. Even if it's not, it certainly should be here (and elsewhere, patches welcome! ;). (It passes the value to the Client, and the Client passes it to the MDS, but the MDS never looks at it).
> I like the states and require_state() stuff. It's making me notice that you need a little more documentation on stuff like closedir and what happens to the CephDirectory under success and error returns. :)

The state checking is really helpful to catch set faults when blindly casting Java long to void* :) I wonder if there is a way to push the state checking even further down in libcephfs? Currently all errors are communicated through a generic CephException, but making things more specific as we go along will be nice. In the mean time, though, I'll add explicit documentation about the state assumptions.

> The CHECK_ARG_* stuff could use some docs. I see now that CHECK_ARG_BOUNDS is expecting a full comparison, so it's probably just my lack of preprocessor idiom knowledge, but it weirded me out. And CHECK_ARG_COND appears to be unused.
> do we need mount() when we can just run mount(null)? Similarly, I don't think we need an if-else in CephMountCreateTest::setupMount. :)

Doh! Fixed.

> 
> 
> 
> 
> On Wednesday, March 7, 2012 at 10:02 AM, Noah Watkins wrote:
> 
>> I've kept an eye on how much things are deviating from the current implementation and its pretty minimal, so I think producing a new version to use the generic libcephfs interface will happen quickly.
>> 
>> As for producing an upstream patch, I think the two biggest concerns are:
>> 
>> 1. Reporting hostname/racks rather than IPs for object location.
>> 2. Address (or at least document) the time synchronization issue.
>> 
>> These are largely orthogonal to getting the code ready for upstream, but may be a "political" road block? I can put together the details of these two issues in a separate thread…
> 
> Please do! I know I've seen most of these issues come up, but I'm still a bit fuzzy on the time sync issue.
> 
> Thanks!
> -Greg
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] Generic libcephfs Java wrappers

[Greg Farnum]

On Thursday, March 8, 2012 at 9:30 AM, Noah Watkins wrote:
>  
> On Mar 7, 2012, at 1:40 PM, Greg Farnum wrote:
>  
> > CephMount::conf_get requires a buf_size. But we freely adjust that if we need more space, so is there any point to it? I can't see callers really having a better idea of size than we do.
> Good point. I can't think of a reason to publicly expose the buffer length param in CephMount to users, but CephProxy is a private interface. On a related a related note:
>  
> int ceph_conf_get(struct ceph_mount_info *cmount, const char *option, char *buf, size_t len);
>  
> shouldn't the 'buf' parameter be char ** if libcephfs is going to be doing a re-alloc, or is there some voodoo behind the scenes?
libcephfs doesn't do a re-alloc there; I was talking about the Java stuff when I said it freely adjusts. :)
  
> > (Oh, now I see there's another one that doesn't take buf_size. Do we have a use case for the with-size option, though?)
> > Similarly with getdnames
>  
> Yes, the default buf_size interfaces are meant to be friendly, while the explicit buf_size interfaces are primarily meant for the unit tests so that the buffer expansion logic can be tested. It would be safe to leave it public, but maybe it is better for it to be protected and used explicitly by the unit tests.

Ah, I see. Well, leaving the explicit buf_size versions public expands the public interface, so I vote to make them protected (or whatever's appropriate in Java; I forget how the privacy settings map).
> > We don't implement CephMount::mkdir, just mkdirs? I *can* see use cases for not wanting recursive creates…
>  
> I was going for the minimum to support Hadoop. I'll stick mkdirs on the short list of API expansion TODOs.

Okay, makes sense.
  
> > I like the states and require_state() stuff. It's making me notice that you need a little more documentation on stuff like closedir and what happens to the CephDirectory under success and error returns. :)
>  
> The state checking is really helpful to catch set faults when blindly casting Java long to void* :) I wonder if there is a way to push the state checking even further down in libcephfs? Currently all errors are communicated through a generic CephException, but making things more specific as we go along will be nice.

I'm not sure how we'd push it farther down in a way that's any cleaner, did you have some thoughts or just a wish to not do it in Java?

Oh, and I forgot something important last time: there's no licensing associated with any of these files right now. :) I think we talked previously about making it Apache, but whatever it is should be clearly noted!
-Greg

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] Generic libcephfs Java wrappers

[Noah Watkins]

On Mar 8, 2012, at 10:48 AM, Greg Farnum wrote:

> On Thursday, March 8, 2012 at 9:30 AM, Noah Watkins wrote:
>> 
>> On Mar 7, 2012, at 1:40 PM, Greg Farnum wrote:
>> 
>>> CephMount::conf_get requires a buf_size. But we freely adjust that if we need more space, so is there any point to it? I can't see callers really having a better idea of size than we do.
>> Good point. I can't think of a reason to publicly expose the buffer length param in CephMount to users, but CephProxy is a private interface. On a related a related note:
>> 
>> int ceph_conf_get(struct ceph_mount_info *cmount, const char *option, char *buf, size_t len);
>> 
>> shouldn't the 'buf' parameter be char ** if libcephfs is going to be doing a re-alloc, or is there some voodoo behind the scenes?
> libcephfs doesn't do a re-alloc there; I was talking about the Java stuff when I said it freely adjusts. :)

The documentation seems to indicate that it does do a malloc. Maybe it is just out of date?

/* Returns a configuration value as a string.
 * If len is positive, that is the maximum number of bytes we'll write into the
 * buffer. If len == -1, we'll call malloc() and set *buf.
 * Returns 0 on success, error code otherwise. Returns ENAMETOOLONG if the
 * buffer is too short. */

>>> I like the states and require_state() stuff. It's making me notice that you need a little more documentation on stuff like closedir and what happens to the CephDirectory under success and error returns. :)
>> 
>> The state checking is really helpful to catch set faults when blindly casting Java long to void* :) I wonder if there is a way to push the state checking even further down in libcephfs? Currently all errors are communicated through a generic CephException, but making things more specific as we go along will be nice.
> 
> I'm not sure how we'd push it farther down in a way that's any cleaner, did you have some thoughts or just a wish to not do it in Java?

Just making unrealistic dreams :) -- I think that the state machine approach is good. Implicitly, users of libcephfs C API must take care do this anyway.

> Oh, and I forgot something important last time: there's no licensing associated with any of these files right now. :) I think we talked previously about making it Apache, but whatever it is should be clearly noted!

Just added a TODO for this. I think it was MIT, but i'll go back and look t the e-mails.

> -Greg
> 

Re: [PATCH 0/3] Generic libcephfs Java wrappers

[Greg Farnum]

On Thursday, March 8, 2012 at 10:54 AM, Noah Watkins wrote:
>  
> On Mar 8, 2012, at 10:48 AM, Greg Farnum wrote:
>  
> > On Thursday, March 8, 2012 at 9:30 AM, Noah Watkins wrote:
> > >  
> > > On Mar 7, 2012, at 1:40 PM, Greg Farnum wrote:
> > >  
> > > > CephMount::conf_get requires a buf_size. But we freely adjust that if we need more space, so is there any point to it? I can't see callers really having a better idea of size than we do.
> > > Good point. I can't think of a reason to publicly expose the buffer length param in CephMount to users, but CephProxy is a private interface. On a related a related note:
> > >  
> > > int ceph_conf_get(struct ceph_mount_info *cmount, const char *option, char *buf, size_t len);
> > >  
> > > shouldn't the 'buf' parameter be char ** if libcephfs is going to be doing a re-alloc, or is there some voodoo behind the scenes?
> > libcephfs doesn't do a re-alloc there; I was talking about the Java stuff when I said it freely adjusts. :)
>  
>  
>  
> The documentation seems to indicate that it does do a malloc. Maybe it is just out of date?
>  
> /* Returns a configuration value as a string.
> * If len is positive, that is the maximum number of bytes we'll write into the
> * buffer. If len == -1, we'll call malloc() and set *buf.
> * Returns 0 on success, error code otherwise. Returns ENAMETOOLONG if the
> * buffer is too short. */

Ah. Two things from that:
1) "If len == -1". Which it isn't, here.
2) There is no voodoo, that just isn't going to work. Either it was created incorrectly in reference to md_config_t::get_val, or else code got moved around without updating that documentation. :/ Options including char ** (as you said), char *& (ewww), or…not doing that?

Anybody have thoughts on the right solution?
-Greg

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] Generic libcephfs Java wrappers

[Noah Watkins]

On Mar 8, 2012, at 6:16 PM, Greg Farnum wrote:

>> /* Returns a configuration value as a string.
>> * If len is positive, that is the maximum number of bytes we'll write into the
>> * buffer. If len == -1, we'll call malloc() and set *buf.
>> * Returns 0 on success, error code otherwise. Returns ENAMETOOLONG if the
>> * buffer is too short. */
> 
> Ah. Two things from that:
> 1) "If len == -1". Which it isn't, here.
> 2) There is no voodoo, that just isn't going to work. Either it was created incorrectly in reference to md_config_t::get_val, or else code got moved around without updating that documentation. :/ Options including char ** (as you said), char *& (ewww), or…not doing that?
> 
> Anybody have thoughts on the right solution?

I am partial to APIs that put the burden on the caller to free/expand the buffer in response to -ENAMETOOLONG. /2cents

-Noah--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] Generic libcephfs Java wrappers

[Josh Durgin]

On 03/08/2012 06:28 PM, Noah Watkins wrote:
>
> On Mar 8, 2012, at 6:16 PM, Greg Farnum wrote:
>
>>> /* Returns a configuration value as a string.
>>> * If len is positive, that is the maximum number of bytes we'll write into the
>>> * buffer. If len == -1, we'll call malloc() and set *buf.
>>> * Returns 0 on success, error code otherwise. Returns ENAMETOOLONG if the
>>> * buffer is too short. */
>>
>> Ah. Two things from that:
>> 1) "If len == -1". Which it isn't, here.
>> 2) There is no voodoo, that just isn't going to work. Either it was created incorrectly in reference to md_config_t::get_val, or else code got moved around without updating that documentation. :/ Options including char ** (as you said), char *&  (ewww), or…not doing that?
>>
>> Anybody have thoughts on the right solution?
>
> I am partial to APIs that put the burden on the caller to free/expand the buffer in response to -ENAMETOOLONG. /2cents

That sounds good to me. That comment exactly describes the behavior of 
md_config_t::get_val(). It would also be cleaner if len didn't have a 
special meaning and md_config_t::get_val() used strings instead of 
char*. It already uses strings internally, and only the librados C++ api 
actually uses len=-1.
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] Generic libcephfs Java wrappers

[Sage Weil]

On Thu, 8 Mar 2012, Josh Durgin wrote:
> On 03/08/2012 06:28 PM, Noah Watkins wrote:
> > 
> > On Mar 8, 2012, at 6:16 PM, Greg Farnum wrote:
> > 
> > > > /* Returns a configuration value as a string.
> > > > * If len is positive, that is the maximum number of bytes we'll write
> > > > into the
> > > > * buffer. If len == -1, we'll call malloc() and set *buf.
> > > > * Returns 0 on success, error code otherwise. Returns ENAMETOOLONG if
> > > > the
> > > > * buffer is too short. */
> > > 
> > > Ah. Two things from that:
> > > 1) "If len == -1". Which it isn't, here.
> > > 2) There is no voodoo, that just isn't going to work. Either it was
> > > created incorrectly in reference to md_config_t::get_val, or else code got
> > > moved around without updating that documentation. :/ Options including
> > > char ** (as you said), char *&  (ewww), or?not doing that?
> > > 
> > > Anybody have thoughts on the right solution?
> > 
> > I am partial to APIs that put the burden on the caller to free/expand the
> > buffer in response to -ENAMETOOLONG. /2cents
> 
> That sounds good to me. That comment exactly describes the behavior of
> md_config_t::get_val(). It would also be cleaner if len didn't have a special
> meaning and md_config_t::get_val() used strings instead of char*. It already
> uses strings internally, and only the librados C++ api actually uses len=-1.

On a related note, I think we should be keeping a list somewhere of all 
the little annoying API changes we want to make, so that when we DO go and 
bump the SONAME we can cram it all in at once.

Something like http://ceph.newdream.net/wiki/API_change_wishlist ?

sage

Re: [PATCH 0/3] Generic libcephfs Java wrappers

[Noah Watkins]

On Mar 7, 2012, at 10:02 AM, Noah Watkins wrote:

> As for producing an upstream patch, I think the two biggest concerns are:
> 
> 1. Reporting hostname/racks rather than IPs for object location.
> 2. Address (or at least document) the time synchronization issue.

Regarding (1), my last comment in this ticket (http://tracker.newdream.net/issues/1666) seems to indicate that a stat occurs on a client and the mtime in the cached inode is returned, but the client is expecting the mtime in the updated inode in the OSD.

Is there an easy way to invalidate the the inodes in a client to at least verify the problem?

Thanks,
- Noah

Re: [PATCH 0/3] Generic libcephfs Java wrappers

[Sage Weil]

On Wed, 14 Mar 2012, Noah Watkins wrote:
> On Mar 7, 2012, at 10:02 AM, Noah Watkins wrote:
> 
> > As for producing an upstream patch, I think the two biggest concerns are:
> > 
> > 1. Reporting hostname/racks rather than IPs for object location.
> > 2. Address (or at least document) the time synchronization issue.
> 
> Regarding (1), my last comment in this ticket (http://tracker.newdream.net/issues/1666) seems to indicate that a stat occurs on a client and the mtime in the cached inode is returned, but the client is expecting the mtime in the updated inode in the OSD.
> 
> Is there an easy way to invalidate the the inodes in a client to at 
> least verify the problem?

Well, you can kludge the code to do it with

diff --git a/src/client/Client.cc b/src/client/Client.cc
index 4a9ae3c..2bb24b7 100644
--- a/src/client/Client.cc
+++ b/src/client/Client.cc
@@ -3858,7 +3858,7 @@ int Client::readlink(const char *relpath, char *buf, loff_t size)
 
 int Client::_getattr(Inode *in, int mask, int uid, int gid)
 {
-  bool yes = in->caps_issued_mask(mask);
+  bool yes = false; //in->caps_issued_mask(mask);
 
   ldout(cct, 10) << "_getattr mask " << ccap_string(mask) << " issued=" << yes << dendl;
   if (yes)

A less intrusive workaround would be to do a readdir on the containing 
directory, which will refresh the client inode.

I'm still not sure what a graceful solution to the problem is... :(

sage

Re: [PATCH 0/3] Generic libcephfs Java wrappers

[Noah Watkins]

On Mar 14, 2012, at 9:38 PM, Sage Weil wrote:

> I'm still not sure what a graceful solution to the problem is... :(

Thanks. Hopefully this verifies the problem.

- Noah