Hi Jeff, Thank you for your time and the valuable reviews. I have addressed the review comments. Can you please have a look. Thanks & Regards, Karthik ----- Original Message ----- > From: "Jeff Darcy (Code Review)" <review@xxxxxxxxxxxxxxx> > To: "Karthik U S" <ksubrahm@xxxxxxxxxx> > Cc: "Gluster Build System" <jenkins@xxxxxxxxxxxxxxxxx>, "Niels de Vos" <ndevos@xxxxxxxxxx>, "NetBSD Build System" > <jenkins@xxxxxxxxxxxxxxxxx>, "Raghavendra Talur" <rtalur@xxxxxxxxxx>, "Vijaikumar Mallikarjuna" > <vijaym.seetha@xxxxxxxxx>, "Joseph Fernandes" <josferna@xxxxxxxxxx> > Sent: Friday, May 27, 2016 2:52:28 AM > Subject: Change in glusterfs[master]: features/worm: updating function names & unwinding FOPs with... > > Jeff Darcy has posted comments on this change. > > Change subject: features/worm: updating function names & unwinding FOPs with > op_errno > ...................................................................... > > > Patch Set 4: > > (1 comment) > > http://review.gluster.org/#/c/14222/4/xlators/features/read-only/src/worm.c > File xlators/features/read-only/src/worm.c: > > Line 83: goto out; > > Done. > I think we can - and should - do better. We don't adhere to a strict "only > return from one place" policy, precisely because sometimes the contortions > needed to comply make the code even less readable. Playing hopscotch among > several goto labels certainly qualifies, and we can see several clues that > simplification is still possible: > > (a) Whether we call STACK_WIND or STACK_UNWIND always corresponds to whether > op_errno is zero or not. At 60 we unwind with non-zero. At 63, 67, and 71 > we wind with zero. At 74 we unwind with non-zero. > > (b) After we've wound or unwound, we return op_errno even though it's always > zero by then (from 68 or 76). In other words, we don't actually need > op_errno by the time we return. > > These "coincidences" suggest that an approach similar to that used in other > translators will work. > > int op_errno = 0; > > /* Example: error or failure. */ > if (is_readonly...) { > op_errno = EROFS; > goto out; > } > > /* Example: optimization or easy case. */ > if (is_wormfile...) { > goto out; > } > > /* Example: result from another function. */ > op_errno = gf_worm_state_transition...; > > out: > > /* Common cleanup actions could go here... */ > > if (op_errno) { > STACK_UNWIND (..., -1, op_errno, ...); > } else { > STACK_WIND (...); > } > > /* ...or here. */ > > return 0; > > Sometimes this is flipped around, with ret/op_errno/whatever initially set to > an error value and only set to zero when we're sure of success. Which to > use is mostly a matter of whether success or failure paths are more common. > In any case, this makes our state explicit in op_errno. It's easier to > verify/ensure that we always wind on success and unwind (with a non-zero > op_errno) on failure, and that we return zero either way. We've had many > bugs in other translators that were the result of "escaping" from a fop > function with neither a wind nor an unwind, and those tend to be hard to > debug. Making it hard for such mistakes to creep in when another engineer > modifies this code a year from now is very valuable. Also, before anyone > else assumes otherwise, we don't have Coverity or clang or any other kind of > rules to detect those particular things automatically. > > I know it's a pain, and it's late in the game, but this seems to be a > technical-debt-reduction patch already (as opposed to a true bug fix) so > let's reduce as much as we can at once instead of having to review and > regression-test the same code twice. BTW, the same pattern recurs in > setattr/setfattr, and there's a typo (perfix/prefix) in the commit message. > > > -- > To view, visit http://review.gluster.org/14222 > To unsubscribe, visit http://review.gluster.org/settings > > Gerrit-MessageType: comment > Gerrit-Change-Id: I3a2f114061aae4b422df54e91c4b3f702af5d0b0 > Gerrit-PatchSet: 4 > Gerrit-Project: glusterfs > Gerrit-Branch: master > Gerrit-Owner: Karthik U S <ksubrahm@xxxxxxxxxx> > Gerrit-Reviewer: Gluster Build System <jenkins@xxxxxxxxxxxxxxxxx> > Gerrit-Reviewer: Jeff Darcy <jdarcy@xxxxxxxxxx> > Gerrit-Reviewer: Joseph Fernandes > Gerrit-Reviewer: Joseph Fernandes <josferna@xxxxxxxxxx> > Gerrit-Reviewer: Karthik U S <ksubrahm@xxxxxxxxxx> > Gerrit-Reviewer: NetBSD Build System <jenkins@xxxxxxxxxxxxxxxxx> > Gerrit-Reviewer: Niels de Vos <ndevos@xxxxxxxxxx> > Gerrit-Reviewer: Raghavendra Talur <rtalur@xxxxxxxxxx> > Gerrit-Reviewer: Vijaikumar Mallikarjuna <vijaym.seetha@xxxxxxxxx> > Gerrit-HasComments: Yes > _______________________________________________ Gluster-devel mailing list Gluster-devel@xxxxxxxxxxx http://www.gluster.org/mailman/listinfo/gluster-devel
