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