On 06/01/2020 12:22, Wieczorkiewicz, Pawel wrote: > > >> On 5. Jan 2020, at 12:36, Andrew Cooper <andrew.coop...@citrix.com >> <mailto:andrew.coop...@citrix.com>> wrote: >> >> On 26/11/2019 10:07, Pawel Wieczorkiewicz wrote: >>> @@ -1274,6 +1297,9 @@ static void livepatch_do_action(void) >>> else > <snip> >>> break; >>> @@ -1309,6 +1338,9 @@ static void livepatch_do_action(void) >>> else >>> other->rc = revert_payload(other); >>> >>> + if ( !was_action_consistent(other, rc ? >>> LIVEPATCH_FUNC_APPLIED : LIVEPATCH_FUNC_NOT_APPLIED) ) >>> + panic("livepatch: partially reverted payload >>> '%s'!\n", other->name); >>> + >>> if ( other->rc == 0 ) >>> revert_payload_tail(other); >> >> Coverity highlights that this contains dead code. >> >> The LIVEPATCH_ACTION_REPLACE case, unlike all others, uses other->rc, >> which means the rc ? : check will always pass LIVEPATCH_FUNC_APPLIED >> into was_action_consistent(), due to the rc = 0 at the head of the case >> block. >> > > Yes, this has to be other->rc instead of rc. Thanks! > >> If this were the only problem, switching rc to other->rc might be ok, >> but there look to be other confusions in the surrounding code. Would >> you mind looking over the whole block of code for correct error handling? >> > > What are the confusions in the code? Could you be more specific and > point me to them? > > I have just checked the LIVEPATCH_ACTION_REPLACE case block again. > It looks correct to me. That is, it preserves the original logic of > error handling there. > I just added the extensions. But, the flow for rc and other->rc should > be the same > and correct (modulo the was_action_consistent() bug).
So long as you've double checked and you think the rest of the logic is fine, then ok. It was the further mixed use of rc vs other->rc which I was concerned about. ~Andrew
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
