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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
