20 Jul
2023
20 Jul
'23
11:35 a.m.
Hi folks,
I noticed that the encode/decode functions enforce versions in order to achieve backward
compatibility and provide a upgrade path forward. However, I'd like to confirm the
standard of practice around the use of versions in this case. If decode() function states
the compatv is 9, there should be no code inside that handles the case of struct_v < 9,
since this condition should never be satisfied. Is this the right understanding? I saw
this block of code in RGWUserInfo::decode():
void decode(bufferlist::const_iterator& bl) {
DECODE_START_LEGACY_COMPAT_LEN_32(22, 9, 9,
bl);
if (struct_v >= 2)
{
uint64_t old_auid;
decode(old_auid,
bl);
}
std::string
access_key;
std::string
secret_key;
decode(access_key,
bl);
decode(secret_key,
bl);
if (struct_v < 6)
{
RGWAccessKey k;
k.id = access_key;
k.key = secret_key;
access_keys[access_key] =
k;
}
I don't see why we need to handle the case of struct_v < 6 when compatv is 9. Is it
safe to assume that this if statement is a dead code? If so, could we also assume that the
following if block in its encode() function should be removed, too?
void encode(bufferlist& bl) const {
ENCODE_START(22, 9, bl);
encode((uint64_t)0, bl); // old auid
std::string access_key;
std::string secret_key;
if (!access_keys.empty()) {
std::map<std::string, RGWAccessKey>::const_iterator iter =
access_keys.begin();
const RGWAccessKey& k = iter->second;
access_key = k.id;
secret_key = k.key;
}
Thanks,Yixin
24 Jul
24 Jul
7:31 a.m.
I think you’re right that currently it’s dead code. However I think there’s a strategy to
make the minimal changes necessary in encode/decode when adding a new version to the
datatype. That serves two purposes. It makes mistakes that would be hard to recover from
much less likely. And it helps document the history of the datatype.
So in the example you provide it appears that prior to version 6 the datatype only allowed
a single access and secret key, but starting with version 6 multiple keys were enabled.
Eric
(he/him)
On Jul 20, 2023, at 2:35 PM, Yixin Jin
<yjin77(a)yahoo.ca> wrote:
Hi folks,
I noticed that the encode/decode functions enforce versions in order to achieve backward
compatibility and provide a upgrade path forward. However, I'd like to confirm the
standard of practice around the use of versions in this case. If decode() function states
the compatv is 9, there should be no code inside that handles the case of struct_v < 9,
since this condition should never be satisfied. Is this the right understanding? I saw
this block of code in RGWUserInfo::decode():
void decode(bufferlist::const_iterator& bl) {
DECODE_START_LEGACY_COMPAT_LEN_32(22, 9, 9, bl);
if (struct_v >= 2) {
uint64_t old_auid;
decode(old_auid, bl);
}
std::string access_key;
std::string secret_key;
decode(access_key, bl);
decode(secret_key, bl);
if (struct_v < 6) {
RGWAccessKey k;
k.id = access_key;
k.key = secret_key;
access_keys[access_key] = k;
}
I don't see why we need to handle the case of struct_v < 6 when compatv is 9. Is
it safe to assume that this if statement is a dead code? If so, could we also assume that
the following if block in its encode() function should be removed, too?
void encode(bufferlist& bl) const {
ENCODE_START(22, 9, bl);
encode((uint64_t)0, bl); // old auid
std::string access_key;
std::string secret_key;
if (!access_keys.empty()) {
std::map<std::string, RGWAccessKey>::const_iterator iter =
access_keys.begin();
const RGWAccessKey& k = iter->second;
access_key = k.id;
secret_key = k.key;
}
Thanks,
Yixin
_______________________________________________
Dev mailing list -- dev(a)ceph.io
To unsubscribe send an email to dev-leave(a)ceph.io
1 Aug
1 Aug
9:22 a.m.
Thanks, Eric, for the confirmation and clarification. That was great! :)
Yixin
On Monday, July 24, 2023 at 10:31:34 a.m. EDT, J. Eric Ivancich
<ivancich(a)redhat.com> wrote:
I think you’re right that currently it’s dead code. However I think there’s a strategy to
make the minimal changes necessary in encode/decode when adding a new version to the
datatype. That serves two purposes. It makes mistakes that would be hard to recover from
much less likely. And it helps document the history of the datatype.
So in the example you provide it appears that prior to version 6 the datatype only allowed
a single access and secret key, but starting with version 6 multiple keys were enabled.
Eric
(he/him)
On Jul 20, 2023, at 2:35 PM, Yixin Jin
<yjin77(a)yahoo.ca> wrote:
Hi folks,
I noticed that the encode/decode functions enforce versions in order to achieve backward
compatibility and provide a upgrade path forward. However, I'd like to confirm the
standard of practice around the use of versions in this case. If decode() function states
the compatv is 9, there should be no code inside that handles the case of struct_v < 9,
since this condition should never be satisfied. Is this the right understanding? I saw
this block of code in RGWUserInfo::decode():
void decode(bufferlist::const_iterator& bl) {
DECODE_START_LEGACY_COMPAT_LEN_32(22, 9, 9, bl);
if (struct_v >= 2) {
uint64_t old_auid;
decode(old_auid, bl);
}
std::string access_key;
std::string secret_key;
decode(access_key, bl);
decode(secret_key, bl);
if (struct_v < 6) {
RGWAccessKey k;
k.id = access_key;
k.key = secret_key;
access_keys[access_key] = k;
}
I don't see why we need to handle the case of struct_v < 6 when compatv is 9. Is
it safe to assume that this if statement is a dead code? If so, could we also assume that
the following if block in its encode() function should be removed, too?
void encode(bufferlist& bl) const {
ENCODE_START(22, 9, bl);
encode((uint64_t)0, bl); // old auid
std::string access_key;
std::string secret_key;
if (!access_keys.empty()) {
std::map<std::string, RGWAccessKey>::const_iterator iter =
access_keys.begin();
const RGWAccessKey& k = iter->second;
access_key = k.id;
secret_key = k.key;
}
Thanks,
Yixin
_______________________________________________
Dev mailing list -- dev(a)ceph.io
To unsubscribe send an email to dev-leave(a)ceph.io
282
days inactive
294
days old
2 comments
2 participants
participants (2)
-
J. Eric Ivancich -
Yixin Jin