Message ID | e6385fc7-0889-ea16-4fc0-337796814636@tls.msk.ru |
---|---|
State | New |
Headers | show |
Series | cherry-picking something to -stable which might require other changes | expand |
When I backport patches into RHEL, the general process I follow is: 1. For context conflicts, just adjust the patch to resolve them. 2. For real dependencies, backport the dependencies, if possible. 3. If backporting the dependencies is not possible, think of a downstream-only solution. This should be rare. People make different backporting decisions (just like structuring patch series). It can be a matter of taste. Stefan
On Tue, Sep 12, 2023, 8:01 AM Stefan Hajnoczi <stefanha@gmail.com> wrote: > When I backport patches into RHEL, the general process I follow is: > 1. For context conflicts, just adjust the patch to resolve them. > 2. For real dependencies, backport the dependencies, if possible. > 3. If backporting the dependencies is not possible, think of a > downstream-only solution. This should be rare. > > People make different backporting decisions (just like structuring > patch series). It can be a matter of taste. > We've done almost exactly the same thing in FreeBSD for the past almost 30 years (with varying degrees of success and nuance, to be true, often limited by early tools). It's an excellent ideal to shoot for, and we've had troubles more often than not the further one gets aways from it. Warner Stefan > >
On Tue, Sep 12, 2023 at 10:00:46AM -0400, Stefan Hajnoczi wrote: > When I backport patches into RHEL, the general process I follow is: > 1. For context conflicts, just adjust the patch to resolve them. > 2. For real dependencies, backport the dependencies, if possible. > 3. If backporting the dependencies is not possible, think of a > downstream-only solution. This should be rare. > > People make different backporting decisions (just like structuring > patch series). It can be a matter of taste. I tend to try to cherry-pick the dependancies in case (1) too unless they are functionally invasive. Any time you manually adjust a patch, you increase the likelihood that later cherry picks will also require manual work. So I always favour a clean cherry-pick until the point the functional risk becomes unacceptable in the context of testing the change I'm pulling back. With regards, Daniel
12.09.2023 18:23, Daniel P. Berrangé wrote: .. > I tend to try to cherry-pick the dependancies in case (1) too > unless they are functionally invasive. Any time you manually > adjust a patch, you increase the likelihood that later cherry > picks will also require manual work. So I always favour a clean > cherry-pick until the point the functional risk becomes > unacceptable in the context of testing the change I'm pulling > back. Yeah, that's exactly my thought: if something in the subsystem has changed, esp. when the new thing is now widely used, it is best to try to pick it up (unless it is a big change by itself or is a part of big change). I already mentioned a trivial fix c255946e3df4 in this thread, which can be applied cleanly if two other no-change patches are in, 753ae97abc7 and dadee9e3ce6. It is much more likely to hit conflicts in this area in future updates if such updates will happen if such renames like these two aren't picked up. So, right in this same patch series, there's one more very similar change: commit 9ff31406312500053ecb5f92df01dd9ce52e635d Author: Conor Dooley <conor.dooley@microchip.com> Date: Thu Jul 27 15:24:17 2023 +0100 hw/riscv: virt: Fix riscv,pmu DT node path --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -719,7 +719,7 @@ static void create_fdt_pmu(RISCVVirtState *s) MachineState *ms = MACHINE(s); RISCVCPU hart = s->soc[0].harts[0]; - pmu_name = g_strdup_printf("/soc/pmu"); + pmu_name = g_strdup_printf("/pmu"); qemu_fdt_add_subnode(ms->fdt, pmu_name); qemu_fdt_setprop_string(ms->fdt, pmu_name, "compatible", "riscv,pmu"); riscv_pmu_generate_fdt_node(ms->fdt, hart.cfg.pmu_num, pmu_name); But all the nearby lines are touched by previous patch: commit 568e0614d0979e0431a8d9dc0503a63b8b0f2d81 Author: Daniel Henrique Barboza <dbarboza@ventanamicro.com> Date: Tue Jan 24 18:22:33 2023 -0300 hw/riscv/virt.c: rename MachineState 'mc' pointers to 'ms' ... Rename all 'mc' MachineState pointers to 'ms'. This is a very tedious and mechanical patch that was produced by doing the following: - find/replace all 'MachineState *mc' to 'MachineState *ms'; - find/replace all 'mc->fdt' to 'ms->fdt'; - find/replace all 'mc->smp.cpus' to 'ms->smp.cpus'; - replace any remaining occurrences of 'mc' that the compiler complained about. This patch by Daniel is a no-code-change, it really is just a rename of variables. I can rename variable back from ms to mc in the fix patch, or I can apply this rename first and apply the fix patch cleanly, and all subsequent changes will have much more chance to apply cleanly too. What a wonderful world.. ;) Thankfully, such cases are rare. But we do have a few famous cases like this still, some of which I also mentioned in the first message in this thread. /mjt
On Tue, Sep 12, 2023 at 09:01:43PM +0300, Michael Tokarev wrote: > 12.09.2023 18:23, Daniel P. Berrangé wrote: > .. > > I tend to try to cherry-pick the dependancies in case (1) too > > unless they are functionally invasive. Any time you manually > > adjust a patch, you increase the likelihood that later cherry > > picks will also require manual work. So I always favour a clean > > cherry-pick until the point the functional risk becomes > > unacceptable in the context of testing the change I'm pulling > > back. > > Yeah, that's exactly my thought: if something in the subsystem > has changed, esp. when the new thing is now widely used, it is > best to try to pick it up (unless it is a big change by itself > or is a part of big change). > > I already mentioned a trivial fix c255946e3df4 in this thread, > which can be applied cleanly if two other no-change patches are > in, 753ae97abc7 and dadee9e3ce6. It is much more likely to hit > conflicts in this area in future updates if such updates will > happen if such renames like these two aren't picked up. > > So, right in this same patch series, there's one more very similar > change: > > commit 9ff31406312500053ecb5f92df01dd9ce52e635d > Author: Conor Dooley <conor.dooley@microchip.com> > Date: Thu Jul 27 15:24:17 2023 +0100 > > hw/riscv: virt: Fix riscv,pmu DT node path > > --- a/hw/riscv/virt.c > +++ b/hw/riscv/virt.c > @@ -719,7 +719,7 @@ static void create_fdt_pmu(RISCVVirtState *s) > MachineState *ms = MACHINE(s); > RISCVCPU hart = s->soc[0].harts[0]; > > - pmu_name = g_strdup_printf("/soc/pmu"); > + pmu_name = g_strdup_printf("/pmu"); > qemu_fdt_add_subnode(ms->fdt, pmu_name); > qemu_fdt_setprop_string(ms->fdt, pmu_name, "compatible", "riscv,pmu"); > riscv_pmu_generate_fdt_node(ms->fdt, hart.cfg.pmu_num, pmu_name); > > But all the nearby lines are touched by previous patch: > > commit 568e0614d0979e0431a8d9dc0503a63b8b0f2d81 > Author: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > Date: Tue Jan 24 18:22:33 2023 -0300 > > hw/riscv/virt.c: rename MachineState 'mc' pointers to 'ms' > ... > Rename all 'mc' MachineState pointers to 'ms'. This is a very tedious > and mechanical patch that was produced by doing the following: > > - find/replace all 'MachineState *mc' to 'MachineState *ms'; > - find/replace all 'mc->fdt' to 'ms->fdt'; > - find/replace all 'mc->smp.cpus' to 'ms->smp.cpus'; > - replace any remaining occurrences of 'mc' that the compiler complained > about. > > This patch by Daniel is a no-code-change, it really is just a rename of > variables. I can rename variable back from ms to mc in the fix patch, > or I can apply this rename first and apply the fix patch cleanly, and > all subsequent changes will have much more chance to apply cleanly too. > > What a wonderful world.. ;) > > Thankfully, such cases are rare. But we do have a few famous cases like this > still, some of which I also mentioned in the first message in this thread. Also this is the key reason why many reviewers will complain if patches are too large, or contain a mixture of functional and non-functional changes, or do two jobs at once. Bigger commits with varying & unrelated changes makes cherry picking much more painful With regards, Daniel
--- a/hw/char/riscv_htif.c +++ b/hw/char/riscv_htif.c @@ -232,7 +232,8 @@ static void htif_handle_tohost_write(HTIFState *s, uint64_t val_written) s->tohost = 0; /* clear to indicate we read */ return; } else if (cmd == HTIF_CONSOLE_CMD_PUTC) { - qemu_chr_fe_write(&s->chr, (uint8_t *)&payload, 1); + uint8_t ch = (uint8_t)payload; + qemu_chr_fe_write(&s->chr, &ch, 1); resp = 0x100 | (uint8_t)payload; } else { qemu_log("HTIF device %d: unknown command\n", device);