On Thu, May 19, 2016 at 8:08 PM, Sage Weil <sage@xxxxxxxxxxxx> wrote: > On Thu, 19 May 2016, Junwang Zhao wrote: >> Hi Sage, Haomai, >> Please check [1] and give some feedback, I am not sure about >> 52436ded943b8f567adb584c50b789431b788a85 >> >> [1] https://github.com/zhjwpku/ceph/commits/wip-addr-work > > Great! I made several suggestions, and I have a question for everyone: > what should we do if we have a "new" addr (say, for the v2 msgr protocol) > and we need to encode it for a client that doesn't have the ADDR2 feature? > Right now we are still stuffing the sockaddr_storage in their with > whatever sockaddr data we have, but it is indistinguishable from a legacy > address. Perhaps it sould encode as blank? (e.g., entity_addr_t())? Or > blank, except with a magic nonce (0xffffffff maybe) so that you can tell > that it is non-blank but not terribly useful/usable? > I don't quite get this, can you explain this more specific? I now consider the first encoded __u32 as the distinguish flag for the client to identify legacy or not, a little bit confused :( BTW, i have add entity_addrvec_t[1], you may want to have a look. [1] https://github.com/zhjwpku/ceph/commits/wip-addr-work > sage > > > >> > Thanks! >> >> On Thu, May 19, 2016 at 4:44 AM, Sage Weil <sage@xxxxxxxxxxxx> wrote: >> > On Tue, 17 May 2016, Junwang Zhao wrote: >> >> Hi Sage, >> >> >> >> Check this[1], we should I do next, entity_addrvec_t ? >> >> >> >> [1] https://github.com/zhjwpku/ceph/commits/wip-addr-work >> > >> > Looks good! I opened the PR >> > >> > https://github.com/ceph/ceph/pull/9184 >> > >> > for review and test. >> > >> > The next step is just to make a new entity_addr_t encoding that is more >> > compact and uses an ADDR2 feature. One commit that defines the new >> > feature, then one commit that changes the entity_addr_t encode/decode, >> > somewhat like in c728926a86e1410f959011d24700bb07bad1dc2c. I would use >> > the new get_sockaddr_len() method for elen, though. >> > >> > Also, the TYPE_ definitions should really be TYPE_LEGACY = 1, not the >> > transport version (IP version), which is already covered by the sa_family >> > field in the sockaddr. I think that makes sense... >> > >> > A third commit would then add the entity_addrvec_t type, similar to what >> > is in that same commit. It should also add the type to >> > test/encoding/types.h. >> > >> > Thanks! >> > sage >> > >> > >> > >> >> >> >> On Tue, May 17, 2016 at 12:29 AM, Sage Weil <sage@xxxxxxxxxxxx> wrote: >> >> > On Mon, 16 May 2016, Sage Weil wrote: >> >> >> Hi Junwang, >> >> >> >> >> >> On Mon, 16 May 2016, Junwang Zhao wrote: >> >> >> > Hi Sage, Haomai, >> >> >> > >> >> >> > I am working on the require-features-patch, I use 'make' to see the conflicts, >> >> >> > and change the code to fit the required-feature-patch, [1] is a huge patch >> >> >> > that I am still working on, it has not been finished. I really need to check >> >> >> > with you whether I am doing it right, since it seems the errors are endless. >> >> >> > >> >> >> > There are some comments where I am not sure in the patch, like 'not sure'. >> >> >> >> >> >> I skimmed through this and it looks mostly right, but I see a few cases >> >> >> where features aren't needed, e.g. cls_refcount_get in >> >> >> cls_refcount_client.cc (there's no addr being encoded in >> >> >> cls_refcount_get_op, so no need to make the encoding featureful). >> >> >> >> >> >> > I didn't split this huge patch into small ones, I am not sure is that a >> >> >> > must, if yes, I will split it into small ones. >> >> >> >> >> >> The end result needs to be a series of small patches, but that doesn't >> >> >> have to happen right away. I think it might be useful to do a few small >> >> >> sets of changes first just to show what the goal is, though. >> >> >> >> >> >> I will take the patch below and pull a few sample changes out so you can >> >> >> see. Traveling at the moment, but I'll have something pushed today (that >> >> >> is also rebased on top of the latest wip-addr-cleanup branch). >> >> > >> >> > I went on a bit of a binge and have a wip-addr-work branch that is mostly >> >> > there. The last thing (I think) is updating msg/simple and msg/async. >> >> > These ones should explicitly encode with features 0 since the protocol is >> >> > defined in terms of the legacy encoding. >> >> > >> >> > sage >> >> >> >> >> >> -- 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
