[Gluster-devel] Change in glusterfs[master]: features/worm: updating function names & unwinding FOPs with...
Karthik Subrahmanya
ksubrahm at redhat.com
Tue May 31 11:54:58 UTC 2016
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
>
More information about the Gluster-devel
mailing list