| Message ID | 1395944299-21970-18-git-send-email-ilya.dryomov@inktank.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/27/2014 01:18 PM, Ilya Dryomov wrote: > Full and incremental osdmaps are structured identically and have > identical headers. Add a helper to decode both "old" (16-bit version, > v6) and "new" (8-bit struct_v+struct_compat+struct_len, v7) osdmap > enconding headers and switch to it. It wasn't clear to me at first that this was adding a new bit of functionality--support for v7 OSD map encodings. A couple comments below but this looks good. Reviewed-by: Alex Elder <elder@linaro.org> > Signed-off-by: Ilya Dryomov <ilya.dryomov@inktank.com> > --- > net/ceph/osdmap.c | 81 ++++++++++++++++++++++++++++++++++++++++++----------- > 1 file changed, 65 insertions(+), 16 deletions(-) > > diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c > index 0134df3639d2..ae96c73aff71 100644 > --- a/net/ceph/osdmap.c > +++ b/net/ceph/osdmap.c > @@ -683,6 +683,63 @@ static int osdmap_set_max_osd(struct ceph_osdmap *map, int max) > return 0; > } > > +#define OSDMAP_WRAPPER_COMPAT_VER 7 > +#define OSDMAP_CLIENT_DATA_COMPAT_VER 1 Don't these definitions belong in a common header? > +/* > + * Return 0 or error. On success, *v is set to 0 for old (v6) osdmaps, > + * to struct_v of the client_data section for new (v7 and above) > + * osdmaps. > + */ > +static int get_osdmap_client_data_v(void **p, void *end, > + const char *s, u8 *v) I like to avoid one-character names (in this case, "s"). Mainly because it's hard to search for them. You could pass a Boolean "full" and use that to select what's printed in the warning messages. > +{ > + u8 struct_v; > + > + ceph_decode_8_safe(p, end, struct_v, e_inval); > + if (struct_v >= 7) { > + u8 struct_compat; > + > + ceph_decode_8_safe(p, end, struct_compat, e_inval); > + if (struct_compat > OSDMAP_WRAPPER_COMPAT_VER) { > + pr_warning("got v %d cv %d > %d of %s ceph_osdmap\n", > + struct_v, struct_compat, > + OSDMAP_WRAPPER_COMPAT_VER, s); > + return -EINVAL; > + } > + *p += 4; /* ignore wrapper struct_len */ > + > + ceph_decode_8_safe(p, end, struct_v, e_inval); > + ceph_decode_8_safe(p, end, struct_compat, e_inval); > + if (struct_compat > OSDMAP_CLIENT_DATA_COMPAT_VER) { > + pr_warning("got v %d cv %d > %d of %s ceph_osdmap client data\n", > + struct_v, struct_compat, > + OSDMAP_CLIENT_DATA_COMPAT_VER, s); > + return -EINVAL; > + } > + *p += 4; /* ignore client data struct_len */ > + } else { > + u16 version; > + > + *p -= 1; > + ceph_decode_16_safe(p, end, version, e_inval); > + if (version < 6) { > + pr_warning("got v %d < 6 of %s ceph_osdmap\n", version, > + s); > + return -EINVAL; > + } > + > + /* old osdmap enconding */ > + struct_v = 0; > + } > + > + *v = struct_v; > + return 0; > + > +e_inval: > + return -EINVAL; > +} > + > static int __decode_pools(void **p, void *end, struct ceph_osdmap *map, > bool incremental) > { > @@ -798,7 +855,7 @@ static int decode_new_pg_temp(void **p, void *end, struct ceph_osdmap *map) > */ > static int osdmap_decode(void **p, void *end, struct ceph_osdmap *map) > { > - u16 version; > + u8 struct_v; > u32 epoch = 0; > void *start = *p; > u32 max; > @@ -807,15 +864,9 @@ static int osdmap_decode(void **p, void *end, struct ceph_osdmap *map) > > dout("%s %p to %p len %d\n", __func__, *p, end, (int)(end - *p)); > > - ceph_decode_16_safe(p, end, version, e_inval); > - if (version > 6) { > - pr_warning("got unknown v %d > 6 of osdmap\n", version); > - goto e_inval; > - } > - if (version < 6) { > - pr_warning("got old v %d < 6 of osdmap\n", version); > - goto e_inval; > - } > + err = get_osdmap_client_data_v(p, end, "full", &struct_v); > + if (err) > + goto bad; > > /* fsid, epoch, created, modified */ > ceph_decode_need(p, end, sizeof(map->fsid) + sizeof(u32) + > @@ -937,15 +988,13 @@ struct ceph_osdmap *osdmap_apply_incremental(void **p, void *end, > __s32 new_flags, max; > void *start = *p; > int err; > - u16 version; > + u8 struct_v; > > dout("%s %p to %p len %d\n", __func__, *p, end, (int)(end - *p)); > > - ceph_decode_16_safe(p, end, version, e_inval); > - if (version != 6) { > - pr_warning("got unknown v %d != 6 of inc osdmap\n", version); > - goto e_inval; > - } > + err = get_osdmap_client_data_v(p, end, "inc", &struct_v); > + if (err) > + goto bad; > > /* fsid, epoch, modified, new_pool_max, new_flags */ > ceph_decode_need(p, end, sizeof(fsid) + sizeof(u32) + sizeof(modified) + > -- 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
On Thu, Mar 27, 2014 at 10:17 PM, Alex Elder <elder@ieee.org> wrote: > On 03/27/2014 01:18 PM, Ilya Dryomov wrote: >> Full and incremental osdmaps are structured identically and have >> identical headers. Add a helper to decode both "old" (16-bit version, >> v6) and "new" (8-bit struct_v+struct_compat+struct_len, v7) osdmap >> enconding headers and switch to it. > > It wasn't clear to me at first that this was adding a > new bit of functionality--support for v7 OSD map encodings. > > A couple comments below but this looks good. > > Reviewed-by: Alex Elder <elder@linaro.org> > >> Signed-off-by: Ilya Dryomov <ilya.dryomov@inktank.com> >> --- >> net/ceph/osdmap.c | 81 ++++++++++++++++++++++++++++++++++++++++++----------- >> 1 file changed, 65 insertions(+), 16 deletions(-) >> >> diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c >> index 0134df3639d2..ae96c73aff71 100644 >> --- a/net/ceph/osdmap.c >> +++ b/net/ceph/osdmap.c >> @@ -683,6 +683,63 @@ static int osdmap_set_max_osd(struct ceph_osdmap *map, int max) >> return 0; >> } >> >> +#define OSDMAP_WRAPPER_COMPAT_VER 7 >> +#define OSDMAP_CLIENT_DATA_COMPAT_VER 1 > > Don't these definitions belong in a common header? I think it would be out of context in a common header. It's just so that the error string is updated whenever the actual integer changes, a poor substitute for a piece of ceph.git DECODE magic. > >> +/* >> + * Return 0 or error. On success, *v is set to 0 for old (v6) osdmaps, >> + * to struct_v of the client_data section for new (v7 and above) >> + * osdmaps. >> + */ >> +static int get_osdmap_client_data_v(void **p, void *end, >> + const char *s, u8 *v) > > I like to avoid one-character names (in this case, "s"). > Mainly because it's hard to search for them. > > You could pass a Boolean "full" and use that to select > what's printed in the warning messages. I was passing "incremental" bool at one point, similar to the other helpers, but then changed it because it made it look like this function does different things for full and incremental maps, when the whole point is that their headers are the same. I'll rename "s" to "prefix" though. Thanks, Ilya -- 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
diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c index 0134df3639d2..ae96c73aff71 100644 --- a/net/ceph/osdmap.c +++ b/net/ceph/osdmap.c @@ -683,6 +683,63 @@ static int osdmap_set_max_osd(struct ceph_osdmap *map, int max) return 0; } +#define OSDMAP_WRAPPER_COMPAT_VER 7 +#define OSDMAP_CLIENT_DATA_COMPAT_VER 1 + +/* + * Return 0 or error. On success, *v is set to 0 for old (v6) osdmaps, + * to struct_v of the client_data section for new (v7 and above) + * osdmaps. + */ +static int get_osdmap_client_data_v(void **p, void *end, + const char *s, u8 *v) +{ + u8 struct_v; + + ceph_decode_8_safe(p, end, struct_v, e_inval); + if (struct_v >= 7) { + u8 struct_compat; + + ceph_decode_8_safe(p, end, struct_compat, e_inval); + if (struct_compat > OSDMAP_WRAPPER_COMPAT_VER) { + pr_warning("got v %d cv %d > %d of %s ceph_osdmap\n", + struct_v, struct_compat, + OSDMAP_WRAPPER_COMPAT_VER, s); + return -EINVAL; + } + *p += 4; /* ignore wrapper struct_len */ + + ceph_decode_8_safe(p, end, struct_v, e_inval); + ceph_decode_8_safe(p, end, struct_compat, e_inval); + if (struct_compat > OSDMAP_CLIENT_DATA_COMPAT_VER) { + pr_warning("got v %d cv %d > %d of %s ceph_osdmap client data\n", + struct_v, struct_compat, + OSDMAP_CLIENT_DATA_COMPAT_VER, s); + return -EINVAL; + } + *p += 4; /* ignore client data struct_len */ + } else { + u16 version; + + *p -= 1; + ceph_decode_16_safe(p, end, version, e_inval); + if (version < 6) { + pr_warning("got v %d < 6 of %s ceph_osdmap\n", version, + s); + return -EINVAL; + } + + /* old osdmap enconding */ + struct_v = 0; + } + + *v = struct_v; + return 0; + +e_inval: + return -EINVAL; +} + static int __decode_pools(void **p, void *end, struct ceph_osdmap *map, bool incremental) { @@ -798,7 +855,7 @@ static int decode_new_pg_temp(void **p, void *end, struct ceph_osdmap *map) */ static int osdmap_decode(void **p, void *end, struct ceph_osdmap *map) { - u16 version; + u8 struct_v; u32 epoch = 0; void *start = *p; u32 max; @@ -807,15 +864,9 @@ static int osdmap_decode(void **p, void *end, struct ceph_osdmap *map) dout("%s %p to %p len %d\n", __func__, *p, end, (int)(end - *p)); - ceph_decode_16_safe(p, end, version, e_inval); - if (version > 6) { - pr_warning("got unknown v %d > 6 of osdmap\n", version); - goto e_inval; - } - if (version < 6) { - pr_warning("got old v %d < 6 of osdmap\n", version); - goto e_inval; - } + err = get_osdmap_client_data_v(p, end, "full", &struct_v); + if (err) + goto bad; /* fsid, epoch, created, modified */ ceph_decode_need(p, end, sizeof(map->fsid) + sizeof(u32) + @@ -937,15 +988,13 @@ struct ceph_osdmap *osdmap_apply_incremental(void **p, void *end, __s32 new_flags, max; void *start = *p; int err; - u16 version; + u8 struct_v; dout("%s %p to %p len %d\n", __func__, *p, end, (int)(end - *p)); - ceph_decode_16_safe(p, end, version, e_inval); - if (version != 6) { - pr_warning("got unknown v %d != 6 of inc osdmap\n", version); - goto e_inval; - } + err = get_osdmap_client_data_v(p, end, "inc", &struct_v); + if (err) + goto bad; /* fsid, epoch, modified, new_pool_max, new_flags */ ceph_decode_need(p, end, sizeof(fsid) + sizeof(u32) + sizeof(modified) +
Full and incremental osdmaps are structured identically and have identical headers. Add a helper to decode both "old" (16-bit version, v6) and "new" (8-bit struct_v+struct_compat+struct_len, v7) osdmap enconding headers and switch to it. Signed-off-by: Ilya Dryomov <ilya.dryomov@inktank.com> --- net/ceph/osdmap.c | 81 ++++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 65 insertions(+), 16 deletions(-)