| Message ID | 20220406141041.2336574-1-amorenoz@redhat.com |
|---|---|
| Headers | show |
| Series | Use newest OVS version to fix Undefined Behavior | expand |
Hi Adrian, The patches look correct to me, however when I tried to apply them locally, I hit a failure: Applying: treewide: remove next variable in _SAFE loops error: sha1 information is lacking or useless (controller/ofctrl.c). error: could not build fake ancestor hint: Use 'git am --show-current-patch=diff' to see the failed patch Patch failed at 0003 treewide: remove next variable in _SAFE loops When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort" I'm pretty sure this just means it needs a rebase. If you can rebase and re-upload, that would be great. Thanks, Mark On 4/6/22 10:10, Adrian Moreno wrote: > A series has been recently introduced in OVS that has two main benefits: > > - Undefined Behavior in iterator loops is removed. This enforces some > restrictions on how this macros can be used, namely: > * User-provided iterator variable is set to NULL after the loop ends > normally > * In _SAFE version of the loop, it's not always safe to use the 'next' > variable. When it would point to the list's head, it is instead set > to NULL by the iteration macros. > > - _SAFE version of the iterator macros now do not require the user to > provide a 'next' variable leading to cleaner and less error prone > code. > > This series bumps ovs to the latest HEAD in master branch and adapts the > code accordingly. > > Adrian Moreno (3): > treewide: bump ovs and fix problematic loops > parallel-hmap: rewrite iterator using multivar helpers > treewide: remove next variable in _SAFE loops > > controller-vtep/binding.c | 4 +- > controller-vtep/gateway.c | 4 +- > controller-vtep/vtep.c | 4 +- > controller/binding.c | 21 +++--- > controller/encaps.c | 4 +- > controller/if-status.c | 17 ++--- > controller/lflow-cache.c | 3 +- > controller/lflow-conj-ids.c | 18 +++-- > controller/lflow.c | 36 +++++----- > controller/ofctrl-seqno.c | 4 +- > controller/ofctrl.c | 84 +++++++++++----------- > controller/ovn-controller.c | 20 +++--- > controller/patch.c | 4 +- > controller/physical.c | 4 +- > controller/pinctrl.c | 69 +++++++++--------- > controller/vif-plug.c | 8 +-- > ic/ovn-ic.c | 12 ++-- > lib/actions.c | 2 +- > lib/expr.c | 51 +++++++------ > lib/extend-table.c | 20 +++--- > lib/extend-table.h | 4 +- > lib/ovn-parallel-hmap.h | 10 +-- > lib/ovn-util.c | 2 +- > lib/vif-plug-provider.c | 8 +-- > northd/northd.c | 139 +++++++++++++++++------------------- > northd/ovn-northd-ddlog.c | 4 +- > northd/ovn-northd.c | 16 ++--- > ovs | 2 +- > utilities/ovn-ic-nbctl.c | 4 +- > utilities/ovn-ic-sbctl.c | 4 +- > utilities/ovn-nbctl.c | 14 ++-- > utilities/ovn-sbctl.c | 4 +- > utilities/ovn-trace.c | 12 ++-- > 33 files changed, 299 insertions(+), 313 deletions(-) >
On 4/8/22 21:03, Mark Michelson wrote: > Hi Adrian, > > The patches look correct to me, however when I tried to apply them locally, I > hit a failure: > > Applying: treewide: remove next variable in _SAFE loops > error: sha1 information is lacking or useless (controller/ofctrl.c). > error: could not build fake ancestor > hint: Use 'git am --show-current-patch=diff' to see the failed patch > Patch failed at 0003 treewide: remove next variable in _SAFE loops > When you have resolved this problem, run "git am --continue". > If you prefer to skip this patch, run "git am --skip" instead. > To restore the original branch and stop patching, run "git am --abort" > > I'm pretty sure this just means it needs a rebase. If you can rebase and > re-upload, that would be great. > Sure, I'll rebase and post v2. One question: Patches 1 and 2 are the minimum required to fix the undefined behavior and update to the newer OVS source. Patch 3 is a quite intrusive cleanup patch that makes the code cleaner. Should I send another patch updating LTS branches with only patches 1 and 2 and the correspondent bump in ovs version? Thanks, Adrián > Thanks, > Mark > > On 4/6/22 10:10, Adrian Moreno wrote: >> A series has been recently introduced in OVS that has two main benefits: >> >> - Undefined Behavior in iterator loops is removed. This enforces some >> restrictions on how this macros can be used, namely: >> * User-provided iterator variable is set to NULL after the loop ends >> normally >> * In _SAFE version of the loop, it's not always safe to use the 'next' >> variable. When it would point to the list's head, it is instead set >> to NULL by the iteration macros. >> >> - _SAFE version of the iterator macros now do not require the user to >> provide a 'next' variable leading to cleaner and less error prone >> code. >> >> This series bumps ovs to the latest HEAD in master branch and adapts the >> code accordingly. >> >> Adrian Moreno (3): >> treewide: bump ovs and fix problematic loops >> parallel-hmap: rewrite iterator using multivar helpers >> treewide: remove next variable in _SAFE loops >> >> controller-vtep/binding.c | 4 +- >> controller-vtep/gateway.c | 4 +- >> controller-vtep/vtep.c | 4 +- >> controller/binding.c | 21 +++--- >> controller/encaps.c | 4 +- >> controller/if-status.c | 17 ++--- >> controller/lflow-cache.c | 3 +- >> controller/lflow-conj-ids.c | 18 +++-- >> controller/lflow.c | 36 +++++----- >> controller/ofctrl-seqno.c | 4 +- >> controller/ofctrl.c | 84 +++++++++++----------- >> controller/ovn-controller.c | 20 +++--- >> controller/patch.c | 4 +- >> controller/physical.c | 4 +- >> controller/pinctrl.c | 69 +++++++++--------- >> controller/vif-plug.c | 8 +-- >> ic/ovn-ic.c | 12 ++-- >> lib/actions.c | 2 +- >> lib/expr.c | 51 +++++++------ >> lib/extend-table.c | 20 +++--- >> lib/extend-table.h | 4 +- >> lib/ovn-parallel-hmap.h | 10 +-- >> lib/ovn-util.c | 2 +- >> lib/vif-plug-provider.c | 8 +-- >> northd/northd.c | 139 +++++++++++++++++------------------- >> northd/ovn-northd-ddlog.c | 4 +- >> northd/ovn-northd.c | 16 ++--- >> ovs | 2 +- >> utilities/ovn-ic-nbctl.c | 4 +- >> utilities/ovn-ic-sbctl.c | 4 +- >> utilities/ovn-nbctl.c | 14 ++-- >> utilities/ovn-sbctl.c | 4 +- >> utilities/ovn-trace.c | 12 ++-- >> 33 files changed, 299 insertions(+), 313 deletions(-) >> >
On 4/19/22 05:06, Adrian Moreno wrote: > > > On 4/8/22 21:03, Mark Michelson wrote: >> Hi Adrian, >> >> The patches look correct to me, however when I tried to apply them >> locally, I hit a failure: >> >> Applying: treewide: remove next variable in _SAFE loops >> error: sha1 information is lacking or useless (controller/ofctrl.c). >> error: could not build fake ancestor >> hint: Use 'git am --show-current-patch=diff' to see the failed patch >> Patch failed at 0003 treewide: remove next variable in _SAFE loops >> When you have resolved this problem, run "git am --continue". >> If you prefer to skip this patch, run "git am --skip" instead. >> To restore the original branch and stop patching, run "git am --abort" >> >> I'm pretty sure this just means it needs a rebase. If you can rebase >> and re-upload, that would be great. >> > > Sure, I'll rebase and post v2. > > One question: > Patches 1 and 2 are the minimum required to fix the undefined behavior > and update to the newer OVS source. > > Patch 3 is a quite intrusive cleanup patch that makes the code cleaner. > > Should I send another patch updating LTS branches with only patches 1 > and 2 and the correspondent bump in ovs version? The only reason to do that is if the patches you provide for the main branch do not apply cleanly when applied to branch-22.03. Currently branch-22.03 is the only LTS OVN branch, and it is the most recent version of OVN, so hopefully the patches you create for main will apply cleanly. > > Thanks, > Adrián > >> Thanks, >> Mark >> >> On 4/6/22 10:10, Adrian Moreno wrote: >>> A series has been recently introduced in OVS that has two main benefits: >>> >>> - Undefined Behavior in iterator loops is removed. This enforces some >>> restrictions on how this macros can be used, namely: >>> * User-provided iterator variable is set to NULL after the loop ends >>> normally >>> * In _SAFE version of the loop, it's not always safe to use the >>> 'next' >>> variable. When it would point to the list's head, it is instead set >>> to NULL by the iteration macros. >>> >>> - _SAFE version of the iterator macros now do not require the user to >>> provide a 'next' variable leading to cleaner and less error prone >>> code. >>> >>> This series bumps ovs to the latest HEAD in master branch and adapts the >>> code accordingly. >>> >>> Adrian Moreno (3): >>> treewide: bump ovs and fix problematic loops >>> parallel-hmap: rewrite iterator using multivar helpers >>> treewide: remove next variable in _SAFE loops >>> >>> controller-vtep/binding.c | 4 +- >>> controller-vtep/gateway.c | 4 +- >>> controller-vtep/vtep.c | 4 +- >>> controller/binding.c | 21 +++--- >>> controller/encaps.c | 4 +- >>> controller/if-status.c | 17 ++--- >>> controller/lflow-cache.c | 3 +- >>> controller/lflow-conj-ids.c | 18 +++-- >>> controller/lflow.c | 36 +++++----- >>> controller/ofctrl-seqno.c | 4 +- >>> controller/ofctrl.c | 84 +++++++++++----------- >>> controller/ovn-controller.c | 20 +++--- >>> controller/patch.c | 4 +- >>> controller/physical.c | 4 +- >>> controller/pinctrl.c | 69 +++++++++--------- >>> controller/vif-plug.c | 8 +-- >>> ic/ovn-ic.c | 12 ++-- >>> lib/actions.c | 2 +- >>> lib/expr.c | 51 +++++++------ >>> lib/extend-table.c | 20 +++--- >>> lib/extend-table.h | 4 +- >>> lib/ovn-parallel-hmap.h | 10 +-- >>> lib/ovn-util.c | 2 +- >>> lib/vif-plug-provider.c | 8 +-- >>> northd/northd.c | 139 +++++++++++++++++------------------- >>> northd/ovn-northd-ddlog.c | 4 +- >>> northd/ovn-northd.c | 16 ++--- >>> ovs | 2 +- >>> utilities/ovn-ic-nbctl.c | 4 +- >>> utilities/ovn-ic-sbctl.c | 4 +- >>> utilities/ovn-nbctl.c | 14 ++-- >>> utilities/ovn-sbctl.c | 4 +- >>> utilities/ovn-trace.c | 12 ++-- >>> 33 files changed, 299 insertions(+), 313 deletions(-) >>> >> >
On 4/19/22 15:35, Mark Michelson wrote: > On 4/19/22 05:06, Adrian Moreno wrote: >> >> >> On 4/8/22 21:03, Mark Michelson wrote: >>> Hi Adrian, >>> >>> The patches look correct to me, however when I tried to apply them locally, I >>> hit a failure: >>> >>> Applying: treewide: remove next variable in _SAFE loops >>> error: sha1 information is lacking or useless (controller/ofctrl.c). >>> error: could not build fake ancestor >>> hint: Use 'git am --show-current-patch=diff' to see the failed patch >>> Patch failed at 0003 treewide: remove next variable in _SAFE loops >>> When you have resolved this problem, run "git am --continue". >>> If you prefer to skip this patch, run "git am --skip" instead. >>> To restore the original branch and stop patching, run "git am --abort" >>> >>> I'm pretty sure this just means it needs a rebase. If you can rebase and >>> re-upload, that would be great. >>> >> >> Sure, I'll rebase and post v2. >> >> One question: >> Patches 1 and 2 are the minimum required to fix the undefined behavior and >> update to the newer OVS source. >> >> Patch 3 is a quite intrusive cleanup patch that makes the code cleaner. >> >> Should I send another patch updating LTS branches with only patches 1 and 2 >> and the correspondent bump in ovs version? > > The only reason to do that is if the patches you provide for the main branch do > not apply cleanly when applied to branch-22.03. Currently branch-22.03 is the > only LTS OVN branch, and it is the most recent version of OVN, so hopefully the > patches you create for main will apply cleanly. > > Understood, thanks. >>> On 4/6/22 10:10, Adrian Moreno wrote: >>>> A series has been recently introduced in OVS that has two main benefits: >>>> >>>> - Undefined Behavior in iterator loops is removed. This enforces some >>>> restrictions on how this macros can be used, namely: >>>> * User-provided iterator variable is set to NULL after the loop ends >>>> normally >>>> * In _SAFE version of the loop, it's not always safe to use the 'next' >>>> variable. When it would point to the list's head, it is instead set >>>> to NULL by the iteration macros. >>>> >>>> - _SAFE version of the iterator macros now do not require the user to >>>> provide a 'next' variable leading to cleaner and less error prone >>>> code. >>>> >>>> This series bumps ovs to the latest HEAD in master branch and adapts the >>>> code accordingly. >>>> >>>> Adrian Moreno (3): >>>> treewide: bump ovs and fix problematic loops >>>> parallel-hmap: rewrite iterator using multivar helpers >>>> treewide: remove next variable in _SAFE loops >>>> >>>> controller-vtep/binding.c | 4 +- >>>> controller-vtep/gateway.c | 4 +- >>>> controller-vtep/vtep.c | 4 +- >>>> controller/binding.c | 21 +++--- >>>> controller/encaps.c | 4 +- >>>> controller/if-status.c | 17 ++--- >>>> controller/lflow-cache.c | 3 +- >>>> controller/lflow-conj-ids.c | 18 +++-- >>>> controller/lflow.c | 36 +++++----- >>>> controller/ofctrl-seqno.c | 4 +- >>>> controller/ofctrl.c | 84 +++++++++++----------- >>>> controller/ovn-controller.c | 20 +++--- >>>> controller/patch.c | 4 +- >>>> controller/physical.c | 4 +- >>>> controller/pinctrl.c | 69 +++++++++--------- >>>> controller/vif-plug.c | 8 +-- >>>> ic/ovn-ic.c | 12 ++-- >>>> lib/actions.c | 2 +- >>>> lib/expr.c | 51 +++++++------ >>>> lib/extend-table.c | 20 +++--- >>>> lib/extend-table.h | 4 +- >>>> lib/ovn-parallel-hmap.h | 10 +-- >>>> lib/ovn-util.c | 2 +- >>>> lib/vif-plug-provider.c | 8 +-- >>>> northd/northd.c | 139 +++++++++++++++++------------------- >>>> northd/ovn-northd-ddlog.c | 4 +- >>>> northd/ovn-northd.c | 16 ++--- >>>> ovs | 2 +- >>>> utilities/ovn-ic-nbctl.c | 4 +- >>>> utilities/ovn-ic-sbctl.c | 4 +- >>>> utilities/ovn-nbctl.c | 14 ++-- >>>> utilities/ovn-sbctl.c | 4 +- >>>> utilities/ovn-trace.c | 12 ++-- >>>> 33 files changed, 299 insertions(+), 313 deletions(-) >>>> >>> >> >
A series has been recently introduced in OVS that has two main benefits: - Undefined Behavior in iterator loops is removed. This enforces some restrictions on how this macros can be used, namely: * User-provided iterator variable is set to NULL after the loop ends normally * In _SAFE version of the loop, it's not always safe to use the 'next' variable. When it would point to the list's head, it is instead set to NULL by the iteration macros. - _SAFE version of the iterator macros now do not require the user to provide a 'next' variable leading to cleaner and less error prone code. This series bumps ovs to the latest HEAD in master branch and adapts the code accordingly. Adrian Moreno (3): treewide: bump ovs and fix problematic loops parallel-hmap: rewrite iterator using multivar helpers treewide: remove next variable in _SAFE loops controller-vtep/binding.c | 4 +- controller-vtep/gateway.c | 4 +- controller-vtep/vtep.c | 4 +- controller/binding.c | 21 +++--- controller/encaps.c | 4 +- controller/if-status.c | 17 ++--- controller/lflow-cache.c | 3 +- controller/lflow-conj-ids.c | 18 +++-- controller/lflow.c | 36 +++++----- controller/ofctrl-seqno.c | 4 +- controller/ofctrl.c | 84 +++++++++++----------- controller/ovn-controller.c | 20 +++--- controller/patch.c | 4 +- controller/physical.c | 4 +- controller/pinctrl.c | 69 +++++++++--------- controller/vif-plug.c | 8 +-- ic/ovn-ic.c | 12 ++-- lib/actions.c | 2 +- lib/expr.c | 51 +++++++------ lib/extend-table.c | 20 +++--- lib/extend-table.h | 4 +- lib/ovn-parallel-hmap.h | 10 +-- lib/ovn-util.c | 2 +- lib/vif-plug-provider.c | 8 +-- northd/northd.c | 139 +++++++++++++++++------------------- northd/ovn-northd-ddlog.c | 4 +- northd/ovn-northd.c | 16 ++--- ovs | 2 +- utilities/ovn-ic-nbctl.c | 4 +- utilities/ovn-ic-sbctl.c | 4 +- utilities/ovn-nbctl.c | 14 ++-- utilities/ovn-sbctl.c | 4 +- utilities/ovn-trace.c | 12 ++-- 33 files changed, 299 insertions(+), 313 deletions(-)