| Message ID | 5767F66902000078000F6ABF@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 20/06/16 12:58, Jan Beulich wrote: > At least similar code should use similar exit mechanisms (return vs > goto). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > RFC reason: There are many more paths where we could return directly, > avoiding the _put_fpu() and put_stub(). Otoh arguably the > two existing return-s around the changes below could also > be changed to "goto done" to restore consistency. > > Subsequently we may then want to consider to reduce the number of > "goto done" by checking rc right after at least the main switch() > statements. I think this patch goes in the wrong direction. Mixing exit mechanisms makes code harder to interpret, and x86_emulate() isn't the easiest function to edit. Using "goto done;" consistently would be a better option. ~Andrew
>>> On 23.06.16 at 12:36, <andrew.cooper3@citrix.com> wrote: > On 20/06/16 12:58, Jan Beulich wrote: >> At least similar code should use similar exit mechanisms (return vs >> goto). >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> RFC reason: There are many more paths where we could return directly, >> avoiding the _put_fpu() and put_stub(). Otoh arguably the >> two existing return-s around the changes below could also >> be changed to "goto done" to restore consistency. >> >> Subsequently we may then want to consider to reduce the number of >> "goto done" by checking rc right after at least the main switch() >> statements. > > I think this patch goes in the wrong direction. Mixing exit mechanisms > makes code harder to interpret, and x86_emulate() isn't the easiest > function to edit. Well, that's what I had put up in the RFC remark. I'm all for consistency, what I'm not sure is whether this ... > Using "goto done;" consistently would be a better option. is better than (always) using plain return where possible. Jan
--- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -2112,7 +2112,7 @@ x86_emulate( op_bytes = 8; if ( (rc = ops->write(x86_seg_ss, sp_pre_dec(op_bytes), ®.sel, op_bytes, ctxt)) != 0 ) - goto done; + return rc; break; } @@ -2125,9 +2125,8 @@ x86_emulate( if ( mode_64bit() && (op_bytes == 4) ) op_bytes = 8; if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes), - &dst.val, op_bytes, ctxt, ops)) != 0 ) - goto done; - if ( (rc = load_seg(src.val, dst.val, 0, NULL, ctxt, ops)) != 0 ) + &dst.val, op_bytes, ctxt, ops)) != 0 || + (rc = load_seg(src.val, dst.val, 0, NULL, ctxt, ops)) != 0 ) return rc; break;