Messages in this thread | | | Date | Mon, 6 Feb 2023 13:20:28 -0500 | Subject | Re: [PATCH] vc_screen: break from vcs_read() while loop if vcs_vc() returns NULL | From | George Kennedy <> |
| |
On 2/6/2023 1:12 PM, Linus Torvalds wrote: > On Mon, Feb 6, 2023 at 9:34 AM George Kennedy <george.kennedy@oracle.com> wrote: >> >> - ret = -ENXIO; >> vc = vcs_vc(inode, &viewed); >> - if (!vc) >> + if (!vc) { >> + if (read) >> + break; >> + ret = -ENXIO; >> goto unlock_out; >> + } > That works, but the whole "if (read)" thing is already done after the > loop, so instead of essentially duplicating that logic, I really think > the patch should be just a plain > > vc = vcs_vc(inode, &viewed); > if (!vc) > - goto unlock_out; > + break; > > and nothing else. > > And yes, the pre-existing vcs_size() error handling has that same ugly pattern. > > It might be worth cleaning up too, although right now that > > size = vcs_size(vc, attr, uni_mode); > if (size < 0) { > if (read) > break; > > pattern means that if we 'break' there, 'read' is non-zero, so 'ret' > doesn't matter. Which is also ugly, but works. > > I *think* it could all be rewritten to just use 'break' everywhere in > the loop, and make 'ret' handling be saner. > > Something like the attached patch, but while I tried to think about > it, I didn't spend a lot of effort on it, and I certainly didn't test > it. So I'm sending this out as a "Hmm. This _looks_ better to me, but > whatever" patch.
Thank you Linus,
Will start with your suggested patch and will test it.
George > > Linus
| |