[Gluster-devel] Change in glusterfs[master]: features/worm: updating function names & unwinding FOPs with...

[Karthik Subrahmanya]

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 at dev.gluster.org>
> To: "Karthik U S" <ksubrahm at redhat.com>
> Cc: "Gluster Build System" <jenkins at build.gluster.com>, "Niels de Vos" <ndevos at redhat.com>, "NetBSD Build System"
> <jenkins at build.gluster.org>, "Raghavendra Talur" <rtalur at redhat.com>, "Vijaikumar Mallikarjuna"
> <vijaym.seetha at gmail.com>, "Joseph Fernandes" <josferna at redhat.com>
> 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 at redhat.com>
> Gerrit-Reviewer: Gluster Build System <jenkins at build.gluster.com>
> Gerrit-Reviewer: Jeff Darcy <jdarcy at redhat.com>
> Gerrit-Reviewer: Joseph Fernandes
> Gerrit-Reviewer: Joseph Fernandes <josferna at redhat.com>
> Gerrit-Reviewer: Karthik U S <ksubrahm at redhat.com>
> Gerrit-Reviewer: NetBSD Build System <jenkins at build.gluster.org>
> Gerrit-Reviewer: Niels de Vos <ndevos at redhat.com>
> Gerrit-Reviewer: Raghavendra Talur <rtalur at redhat.com>
> Gerrit-Reviewer: Vijaikumar Mallikarjuna <vijaym.seetha at gmail.com>
> Gerrit-HasComments: Yes
>