MIME-Version: 1.0
References: <1675704844-17228-1-git-send-email-george.kennedy@oracle.com>
In-Reply-To: <1675704844-17228-1-git-send-email-george.kennedy@oracle.com>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Mon, 6 Feb 2023 10:12:43 -0800
Message-ID: <CAHk-=wi5h32VBgzYgFy8KoXbcDMa9K_ihDjfxD-iScy7L+M=QQ@mail.gmail.com>
Subject: Re: [PATCH] vc_screen: break from vcs_read() while loop if vcs_vc()
 returns NULL
To:     George Kennedy <george.kennedy@oracle.com>
Cc:     gregkh@linuxfoundation.org, jslaby@suse.cz, sfr@canb.auug.org.au,
        akpm@linux-foundation.org, linux-kernel@vger.kernel.org,
        linux-serial@vger.kernel.org
Content-Type: multipart/mixed; boundary="000000000000346cf405f40bfb2d"
Precedence: bulk

--000000000000346cf405f40bfb2d
Content-Type: text/plain; charset="UTF-8"

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.

               Linus

--000000000000346cf405f40bfb2d
Content-Type: text/x-patch; charset="US-ASCII"; name="patch.diff"
Content-Disposition: attachment; filename="patch.diff"
Content-Transfer-Encoding: base64
Content-ID: <f_ldt4rzjr0>
X-Attachment-Id: f_ldt4rzjr0

IGRyaXZlcnMvdHR5L3Z0L3ZjX3NjcmVlbi5jIHwgMTQgKysrKysrLS0tLS0tLS0KIDEgZmlsZSBj
aGFuZ2VkLCA2IGluc2VydGlvbnMoKyksIDggZGVsZXRpb25zKC0pCgpkaWZmIC0tZ2l0IGEvZHJp
dmVycy90dHkvdnQvdmNfc2NyZWVuLmMgYi9kcml2ZXJzL3R0eS92dC92Y19zY3JlZW4uYwppbmRl
eCBmNTY2ZWIxODM5ZGMuLmM1OTliNDUyOTY5ZiAxMDA2NDQKLS0tIGEvZHJpdmVycy90dHkvdnQv
dmNfc2NyZWVuLmMKKysrIGIvZHJpdmVycy90dHkvdnQvdmNfc2NyZWVuLmMKQEAgLTQwNiwxOSAr
NDA2LDE3IEBAIHZjc19yZWFkKHN0cnVjdCBmaWxlICpmaWxlLCBjaGFyIF9fdXNlciAqYnVmLCBz
aXplX3QgY291bnQsIGxvZmZfdCAqcHBvcykKIAkJcmV0ID0gLUVOWElPOwogCQl2YyA9IHZjc192
Yyhpbm9kZSwgJnZpZXdlZCk7CiAJCWlmICghdmMpCi0JCQlnb3RvIHVubG9ja19vdXQ7CisJCQli
cmVhazsKIAogCQkvKiBDaGVjayB3aGV0aGVyIHdlIGFyZSBhYm92ZSBzaXplIGVhY2ggcm91bmQs
CiAJCSAqIGFzIGNvcHlfdG9fdXNlciBhdCB0aGUgZW5kIG9mIHRoaXMgbG9vcAogCQkgKiBjb3Vs
ZCBzbGVlcC4KIAkJICovCi0JCXNpemUgPSB2Y3Nfc2l6ZSh2YywgYXR0ciwgdW5pX21vZGUpOwot
CQlpZiAoc2l6ZSA8IDApIHsKLQkJCWlmIChyZWFkKQotCQkJCWJyZWFrOwotCQkJcmV0ID0gc2l6
ZTsKLQkJCWdvdG8gdW5sb2NrX291dDsKLQkJfQorCQlyZXQgPSB2Y3Nfc2l6ZSh2YywgYXR0ciwg
dW5pX21vZGUpOworCQlpZiAocmV0IDwgMCkKKwkJCWJyZWFrOworCQlzaXplID0gcmV0OworCQly
ZXQgPSAwOwogCQlpZiAocG9zID49IHNpemUpCiAJCQlicmVhazsKIAkJaWYgKGNvdW50ID4gc2l6
ZSAtIHBvcykK
--000000000000346cf405f40bfb2d--