[8u] RFR: 8143925 : enhancing CounterMode.crypt() for AESCrypt.implEncryptBlock() + 8146581 : Minor corrections to the patch submitted for earlier bug id - 8143925

[Hohensee, Paul]

Well put. I'd love to see a variation on this in as a faq. :)

Paul

On 1/22/20, 8:20 PM, "Andrew John Hughes" <gnu.andrew at redhat.com> wrote:

    
    
    On 22/01/2020 01:11, Hohensee, Paul wrote:
    > I'm not a huge fan of making manual changes to get around not having backported patches on which a patch (i.e., this one) depends (whew!). I'd rather see the 8 (!) patches Martin references done first. If we don't, then if we/Oracle ever decide to do any of those backports, it's going to be very confusing. I'd rather have something as close as possible to what was tested in the parent repo. Andrew, what do you think?
    > 
    > Thanks,
    > Paul
    > 
    
    Well, I feel as though I could have written that first sentence myself
    ;) As I'm sure you're aware though, there's a lot of nuance to it in
    practice, and it depends on what patches are judged as dependencies and
    in what way.
    
    The kind of reviews that initially fill me with worry and inevitably end
    up taking a long time to do are those where it's not clear that the
    necessary investigative work has been done on resolving such dependencies.
    
    It's one thing if the author says something along the lines of "I've
    altered X, because we don't have fix Y, and I don't think we should
    backport that for reason Z", where it's clear that the research has been
    done and the decision can be agreed with or not. It's quite another if
    they make no mention of it, or do so with no explanation, because I then
    have to dig into the why myself. And often, it's not even clear why
    after that, other than a lack of desire to do the additional work.
    
    With this particular patch, the initial summary was useful, though
    incomplete. I've been pleased to see the recent e-mails detailing the
    prospective dependencies at a greater length and the reasoning as to why
    not to include them.
    
    A hunk of a patch can not apply for a number of reasons, and also, apply
    but then leave the result unbuildable. When backporting, it's necessary
    to work out why things look different before you can make a decision as
    to whether something else needs to be backported first. There are cases
    where the context ends up being different, but no backport is required
    because the presence (or lack of) the different code has no impact on
    what's actually being backported e.g. where an unrelated variable
    declaration or method has been added nearby.
    
    In this patch, the main cases are instances of a feature being present
    in 9u and up, but not in 8u. This is more common in HotSpot changes,
    because it often sees the addition of new features without the need to
    worry about a public API as the JDK does. In turn, due to the higher
    code complexity and architecture-specific nature of a lot of HotSpot
    code, along with it being generally harder to test and find bugs, I'm
    less keen to backport large changes there than in JDK code. This is why
    I judged the avx512 exemption to be ok in my initial review [0], but
    also deferred to those more familiar with the codebase.
    
    I'm still nervous about dropping hunks of code from the patch that will
    be needed, should avx512 ever be backported. However, the alternative of
    insisting on that as a pre-requisite for this change seems overboard. It
    does suggest that another good idea during backports is to check whether
    the files being altered by patch X also contain later changes already
    backported to 8u, which may have been stripped of elements which
    required X at the time of their backport. That does occur more than you
    might at first think.
    
    Incidentally, the problem of dropping AArch64 chunks in this and like
    patches is one of my motivations for wanting to get that into 8u itself :-)
    
    [0]
    https://mail.openjdk.java.net/pipermail/jdk8u-dev/2019-December/010846.html
    
    Thanks,
    -- 
    Andrew :)
    
    Senior Free Java Software Engineer
    Red Hat, Inc. (http://www.redhat.com)
    
    PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
    Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
    https://keybase.io/gnu_andrew