| Message ID | 1414080959-6053-1-git-send-email-fabf@skynet.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Oct 23, 2014 at 8:15 PM, Fabian Frederick <fabf@skynet.be> wrote: > osdmap_epoch is redundant with reassert_epoch and unused. > > Signed-off-by: Fabian Frederick <fabf@skynet.be> > --- > net/ceph/osd_client.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c > index f3fc54e..432bd75 100644 > --- a/net/ceph/osd_client.c > +++ b/net/ceph/osd_client.c > @@ -1700,7 +1700,6 @@ static void handle_reply(struct ceph_osd_client *osdc, struct ceph_msg *msg, > int err; > u32 reassert_epoch; > u64 reassert_version; > - u32 osdmap_epoch; > int already_completed; > u32 bytes; > unsigned int i; > @@ -1725,7 +1724,6 @@ static void handle_reply(struct ceph_osd_client *osdc, struct ceph_msg *msg, > result = ceph_decode_32(&p); > reassert_epoch = ceph_decode_32(&p); > reassert_version = ceph_decode_64(&p); > - osdmap_epoch = ceph_decode_32(&p); > > /* lookup */ > down_read(&osdc->map_sem); Hi Fabian, osdmap_epoch is useful for debugging, but this is wrong anyway - ceph_decode_32() has side effects. Removing it and not adjusting *p would make the whole thing blow up pretty fast.. 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
> On 23 October 2014 at 18:25 Ilya Dryomov <ilya.dryomov@inktank.com> wrote: > > > On Thu, Oct 23, 2014 at 8:15 PM, Fabian Frederick <fabf@skynet.be> wrote: > > osdmap_epoch is redundant with reassert_epoch and unused. > > > > Signed-off-by: Fabian Frederick <fabf@skynet.be> > > --- > > net/ceph/osd_client.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c > > index f3fc54e..432bd75 100644 > > --- a/net/ceph/osd_client.c > > +++ b/net/ceph/osd_client.c > > @@ -1700,7 +1700,6 @@ static void handle_reply(struct ceph_osd_client *osdc, > > struct ceph_msg *msg, > > int err; > > u32 reassert_epoch; > > u64 reassert_version; > > - u32 osdmap_epoch; > > int already_completed; > > u32 bytes; > > unsigned int i; > > @@ -1725,7 +1724,6 @@ static void handle_reply(struct ceph_osd_client *osdc, > > struct ceph_msg *msg, > > result = ceph_decode_32(&p); > > reassert_epoch = ceph_decode_32(&p); > > reassert_version = ceph_decode_64(&p); > > - osdmap_epoch = ceph_decode_32(&p); > > > > /* lookup */ > > down_read(&osdc->map_sem); > > Hi Fabian, > > osdmap_epoch is useful for debugging, but this is wrong anyway - > ceph_decode_32() has side effects. Removing it and not adjusting *p > would make the whole thing blow up pretty fast.. > > Thanks, > > Ilya Hi Ilya, osdmap_epoch generates a warning with -Wunused-but-set-variable. Maybe we could just do ceph_decode_32(&p) and remove it (it doesn't seem to add a warning) and/or add some comment ? Regards, Fabian -- 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 Sat, Oct 25, 2014 at 1:12 PM, Fabian Frederick <fabf@skynet.be> wrote: > > >> On 23 October 2014 at 18:25 Ilya Dryomov <ilya.dryomov@inktank.com> wrote: >> >> >> On Thu, Oct 23, 2014 at 8:15 PM, Fabian Frederick <fabf@skynet.be> wrote: >> > osdmap_epoch is redundant with reassert_epoch and unused. >> > >> > Signed-off-by: Fabian Frederick <fabf@skynet.be> >> > --- >> > net/ceph/osd_client.c | 2 -- >> > 1 file changed, 2 deletions(-) >> > >> > diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c >> > index f3fc54e..432bd75 100644 >> > --- a/net/ceph/osd_client.c >> > +++ b/net/ceph/osd_client.c >> > @@ -1700,7 +1700,6 @@ static void handle_reply(struct ceph_osd_client *osdc, >> > struct ceph_msg *msg, >> > int err; >> > u32 reassert_epoch; >> > u64 reassert_version; >> > - u32 osdmap_epoch; >> > int already_completed; >> > u32 bytes; >> > unsigned int i; >> > @@ -1725,7 +1724,6 @@ static void handle_reply(struct ceph_osd_client *osdc, >> > struct ceph_msg *msg, >> > result = ceph_decode_32(&p); >> > reassert_epoch = ceph_decode_32(&p); >> > reassert_version = ceph_decode_64(&p); >> > - osdmap_epoch = ceph_decode_32(&p); >> > >> > /* lookup */ >> > down_read(&osdc->map_sem); >> >> Hi Fabian, >> >> osdmap_epoch is useful for debugging, but this is wrong anyway - >> ceph_decode_32() has side effects. Removing it and not adjusting *p >> would make the whole thing blow up pretty fast.. >> >> Thanks, >> >> Ilya > > Hi Ilya, > > osdmap_epoch generates a warning with -Wunused-but-set-variable. > Maybe we could just do ceph_decode_32(&p) and remove it > (it doesn't seem to add a warning) and/or add some comment ? I see, I'll fix it up. 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/osd_client.c b/net/ceph/osd_client.c index f3fc54e..432bd75 100644 --- a/net/ceph/osd_client.c +++ b/net/ceph/osd_client.c @@ -1700,7 +1700,6 @@ static void handle_reply(struct ceph_osd_client *osdc, struct ceph_msg *msg, int err; u32 reassert_epoch; u64 reassert_version; - u32 osdmap_epoch; int already_completed; u32 bytes; unsigned int i; @@ -1725,7 +1724,6 @@ static void handle_reply(struct ceph_osd_client *osdc, struct ceph_msg *msg, result = ceph_decode_32(&p); reassert_epoch = ceph_decode_32(&p); reassert_version = ceph_decode_64(&p); - osdmap_epoch = ceph_decode_32(&p); /* lookup */ down_read(&osdc->map_sem);
osdmap_epoch is redundant with reassert_epoch and unused. Signed-off-by: Fabian Frederick <fabf@skynet.be> --- net/ceph/osd_client.c | 2 -- 1 file changed, 2 deletions(-)