| Message ID | 20220401081650.1579214-1-odivlad@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,1/2] vtep: correctly bring vtep lport up in SBDB | expand |
Hi, The subject claims this is patch 1/2, but I don't see patch 2/2 on patchwork or on the mailing list. Provisionally, I'm acking this patch Acked-by: Mark Michelson <mmichels@redhat.com> but if there's a part 2, I'd prefer that it gets reviewed before this gets merged. Also, I have one minor thing below. On 4/1/22 04:16, Vladislav Odintsov wrote: > Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> > --- > controller-vtep/binding.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/controller-vtep/binding.c b/controller-vtep/binding.c > index 01d5a16d2..7c7bea90a 100644 > --- a/controller-vtep/binding.c > +++ b/controller-vtep/binding.c > @@ -109,12 +109,11 @@ update_pb_chassis(const struct sbrec_port_binding *port_binding_rec, > port_binding_rec->chassis->name, > chassis_rec->name); > } > - > sbrec_port_binding_set_chassis(port_binding_rec, chassis_rec); > - if (port_binding_rec->n_up) { > - bool up = true; > - sbrec_port_binding_set_up(port_binding_rec, &up, 1); > - } > + } > + else if (port_binding_rec->n_up) { This is a coding guidelines violation. The else should be on the same line as the closing curly brace: } else if (port_binding_rec->n_up) { This is minor enough not to prevent me from acking the patch. But this should be fixed before merging. > + bool up = true; > + sbrec_port_binding_set_up(port_binding_rec, &up, 1); > } > } > >
Hi Mark, Initially there should be a second patch, but after sending the first I’ve decided to send second one separately: http://patchwork.ozlabs.org/project/ovn/patch/20220401083229.1583750-1-odivlad@gmail.com/ regards, Vladislav Odintsov > On 5 Apr 2022, at 23:03, Mark Michelson <mmichels@redhat.com> wrote: > > Hi, > > The subject claims this is patch 1/2, but I don't see patch 2/2 on patchwork or on the mailing list. > > Provisionally, I'm acking this patch > > Acked-by: Mark Michelson <mmichels@redhat.com> > > but if there's a part 2, I'd prefer that it gets reviewed before this gets merged. > > Also, I have one minor thing below. > >> On 4/1/22 04:16, Vladislav Odintsov wrote: >> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> >> --- >> controller-vtep/binding.c | 9 ++++----- >> 1 file changed, 4 insertions(+), 5 deletions(-) >> diff --git a/controller-vtep/binding.c b/controller-vtep/binding.c >> index 01d5a16d2..7c7bea90a 100644 >> --- a/controller-vtep/binding.c >> +++ b/controller-vtep/binding.c >> @@ -109,12 +109,11 @@ update_pb_chassis(const struct sbrec_port_binding *port_binding_rec, >> port_binding_rec->chassis->name, >> chassis_rec->name); >> } >> - >> sbrec_port_binding_set_chassis(port_binding_rec, chassis_rec); >> - if (port_binding_rec->n_up) { >> - bool up = true; >> - sbrec_port_binding_set_up(port_binding_rec, &up, 1); >> - } >> + } >> + else if (port_binding_rec->n_up) { > > This is a coding guidelines violation. The else should be on the same line as the closing curly brace: > > } else if (port_binding_rec->n_up) { > > This is minor enough not to prevent me from acking the patch. But this should be fixed before merging. >> + bool up = true; >> + sbrec_port_binding_set_up(port_binding_rec, &up, 1); >> } >> } >> >
Regarding the code style, should I resend patch or this can be fixed when the patch gets applied? regards, Vladislav Odintsov > On 5 Apr 2022, at 23:03, Mark Michelson <mmichels@redhat.com> wrote: > > Hi, > > The subject claims this is patch 1/2, but I don't see patch 2/2 on patchwork or on the mailing list. > > Provisionally, I'm acking this patch > > Acked-by: Mark Michelson <mmichels@redhat.com> > > but if there's a part 2, I'd prefer that it gets reviewed before this gets merged. > > Also, I have one minor thing below. > >> On 4/1/22 04:16, Vladislav Odintsov wrote: >> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> >> --- >> controller-vtep/binding.c | 9 ++++----- >> 1 file changed, 4 insertions(+), 5 deletions(-) >> diff --git a/controller-vtep/binding.c b/controller-vtep/binding.c >> index 01d5a16d2..7c7bea90a 100644 >> --- a/controller-vtep/binding.c >> +++ b/controller-vtep/binding.c >> @@ -109,12 +109,11 @@ update_pb_chassis(const struct sbrec_port_binding *port_binding_rec, >> port_binding_rec->chassis->name, >> chassis_rec->name); >> } >> - >> sbrec_port_binding_set_chassis(port_binding_rec, chassis_rec); >> - if (port_binding_rec->n_up) { >> - bool up = true; >> - sbrec_port_binding_set_up(port_binding_rec, &up, 1); >> - } >> + } >> + else if (port_binding_rec->n_up) { > > This is a coding guidelines violation. The else should be on the same line as the closing curly brace: > > } else if (port_binding_rec->n_up) { > > This is minor enough not to prevent me from acking the patch. But this should be fixed before merging. >> + bool up = true; >> + sbrec_port_binding_set_up(port_binding_rec, &up, 1); >> } >> } >> >
You don't need to resend the patch. Whoever merges this can fix the problem when they merge it. On 4/6/22 02:26, Vladislav Odintsov wrote: > Regarding the code style, should I resend patch or this can be fixed when the patch gets applied? > > regards, > Vladislav Odintsov > >> On 5 Apr 2022, at 23:03, Mark Michelson <mmichels@redhat.com> wrote: >> >> Hi, >> >> The subject claims this is patch 1/2, but I don't see patch 2/2 on patchwork or on the mailing list. >> >> Provisionally, I'm acking this patch >> >> Acked-by: Mark Michelson <mmichels@redhat.com> >> >> but if there's a part 2, I'd prefer that it gets reviewed before this gets merged. >> >> Also, I have one minor thing below. >> >>> On 4/1/22 04:16, Vladislav Odintsov wrote: >>> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> >>> --- >>> controller-vtep/binding.c | 9 ++++----- >>> 1 file changed, 4 insertions(+), 5 deletions(-) >>> diff --git a/controller-vtep/binding.c b/controller-vtep/binding.c >>> index 01d5a16d2..7c7bea90a 100644 >>> --- a/controller-vtep/binding.c >>> +++ b/controller-vtep/binding.c >>> @@ -109,12 +109,11 @@ update_pb_chassis(const struct sbrec_port_binding *port_binding_rec, >>> port_binding_rec->chassis->name, >>> chassis_rec->name); >>> } >>> - >>> sbrec_port_binding_set_chassis(port_binding_rec, chassis_rec); >>> - if (port_binding_rec->n_up) { >>> - bool up = true; >>> - sbrec_port_binding_set_up(port_binding_rec, &up, 1); >>> - } >>> + } >>> + else if (port_binding_rec->n_up) { >> >> This is a coding guidelines violation. The else should be on the same line as the closing curly brace: >> >> } else if (port_binding_rec->n_up) { >> >> This is minor enough not to prevent me from acking the patch. But this should be fixed before merging. >>> + bool up = true; >>> + sbrec_port_binding_set_up(port_binding_rec, &up, 1); >>> } >>> } >>> >> >
On Wed, Apr 6, 2022 at 9:56 AM Mark Michelson <mmichels@redhat.com> wrote: > > You don't need to resend the patch. Whoever merges this can fix the > problem when they merge it. > > On 4/6/22 02:26, Vladislav Odintsov wrote: > > Regarding the code style, should I resend patch or this can be fixed when the patch gets applied? > > > > regards, > > Vladislav Odintsov > > > >> On 5 Apr 2022, at 23:03, Mark Michelson <mmichels@redhat.com> wrote: > >> > >> Hi, > >> > >> The subject claims this is patch 1/2, but I don't see patch 2/2 on patchwork or on the mailing list. > >> > >> Provisionally, I'm acking this patch > >> > >> Acked-by: Mark Michelson <mmichels@redhat.com> Thanks. I applied this patch to the main branch and backported upto branch-21.09. Numan > >> > >> but if there's a part 2, I'd prefer that it gets reviewed before this gets merged. > >> > >> Also, I have one minor thing below. > >> > >>> On 4/1/22 04:16, Vladislav Odintsov wrote: > >>> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> > >>> --- > >>> controller-vtep/binding.c | 9 ++++----- > >>> 1 file changed, 4 insertions(+), 5 deletions(-) > >>> diff --git a/controller-vtep/binding.c b/controller-vtep/binding.c > >>> index 01d5a16d2..7c7bea90a 100644 > >>> --- a/controller-vtep/binding.c > >>> +++ b/controller-vtep/binding.c > >>> @@ -109,12 +109,11 @@ update_pb_chassis(const struct sbrec_port_binding *port_binding_rec, > >>> port_binding_rec->chassis->name, > >>> chassis_rec->name); > >>> } > >>> - > >>> sbrec_port_binding_set_chassis(port_binding_rec, chassis_rec); > >>> - if (port_binding_rec->n_up) { > >>> - bool up = true; > >>> - sbrec_port_binding_set_up(port_binding_rec, &up, 1); > >>> - } > >>> + } > >>> + else if (port_binding_rec->n_up) { > >> > >> This is a coding guidelines violation. The else should be on the same line as the closing curly brace: > >> > >> } else if (port_binding_rec->n_up) { > >> > >> This is minor enough not to prevent me from acking the patch. But this should be fixed before merging. > >>> + bool up = true; > >>> + sbrec_port_binding_set_up(port_binding_rec, &up, 1); > >>> } > >>> } > >>> > >> > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/controller-vtep/binding.c b/controller-vtep/binding.c index 01d5a16d2..7c7bea90a 100644 --- a/controller-vtep/binding.c +++ b/controller-vtep/binding.c @@ -109,12 +109,11 @@ update_pb_chassis(const struct sbrec_port_binding *port_binding_rec, port_binding_rec->chassis->name, chassis_rec->name); } - sbrec_port_binding_set_chassis(port_binding_rec, chassis_rec); - if (port_binding_rec->n_up) { - bool up = true; - sbrec_port_binding_set_up(port_binding_rec, &up, 1); - } + } + else if (port_binding_rec->n_up) { + bool up = true; + sbrec_port_binding_set_up(port_binding_rec, &up, 1); } }
Signed-off-by: Vladislav Odintsov <odivlad@gmail.com> --- controller-vtep/binding.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)