On Wed, 2023-06-28 at 15:48 +0300, Nikolay Borisov wrote: > > This diff is extremely hard to follow, but I think the change to error > > handling Nikolay proposed has to be applied to the function from the > > beginning, not changed drastically in this patch. > > > > > I agree. That change should be broken across the various patches > introducing each piece of error handling. No I don't want to do this. The TDMRs are only needed to be saved if we want to do the next patch (reset TDX memory). They are always freed in previous patch. We can add justification to keep in previous patch but I now want to avoid such pattern because I now believe it's not the right way to organize patches: Obviously such justification depends on the later patch. In case the later patch has something wrong and needs to be updated, the justification can be invalid, and we need to adjust the previous patches accordingly. This could result in code review frustration. Specifically for this issue, if we always free TDMRs in previous patches, then it's just not right to do what you suggested there. Also, now with dynamic allocation of TDSYSINFO_STRUCT and CMR array, we need to do 3 things when module initialization is successful: put_online_mems(); kfree(sysinfo); kfree(cmr_array); return 0; out_xxx: .... put_online_mems(); kfree(sysinfo); kfree(cmr_array); return ret; I can hardly say which is better. I am willing to do the above pattern if you guys prefer but I certainly don't want to mix this logic to previous patches.
