| Message ID | 20200509084419.86332-1-taoyunxiang@cmss.chinamobile.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,OVN,v7] ovn-nbctl.c: Add an optional way to delete QoS by uuid ?[6n | expand |
Bleep bloop. Greetings taoyunxiang@cmss.chinamobile.com, I am a robot and I have tried out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Line lacks whitespace around operator
#83 FILE: utilities/ovn-nbctl.c:607:
qos-del SWITCH [{DIRECTION | UUID} [PRIORITY MATCH]]\n\
Lines checked: 147, Warnings: 1, Errors: 0
Please check this out. If you feel there has been an error, please email aconole@redhat.com
Thanks,
0-day Robot
On Sat, May 9, 2020 at 2:15 PM Tao YunXiang < taoyunxiang@cmss.chinamobile.com> wrote: > usage: > ovn-nbctl qos-del ls0 [UUID0] > > Author: Tao YunXiang <taoyunxiang@cmss.chinamobile.com> > Co-authored-by: Liu Chang <liuchang@cmss.chinamobile.com> > Co-authored-by: Rong Yin <rongyin@cmss.chinamobile.com> > Signed-off-by: Tao YunXiang <taoyunxiang@cmss.chinamobile.com> > Signed-off-by: Liu Chang <liuchang@cmss.chinamobile.com> > Signed-off-by: Rong Yin <rongyin@cmss.chinamobile.com> > > --- > v6: ovn-nbctl.c: Add an optional way to delete QoS by uuid > v5: ovn-nbctl.c: Add an optional way to delete QoS by uuid > v4: Add a way to delete QoS by its name or uuid > v3: ovn-nbctl.c: Add a way to delete QoS by its name or uuid > > Thanks for adding the test case. I see a small issue. When I run "ovn-nbctl qos-del <some_uuid>, the commands doesn't print any error if the qos with the given <some_uuid> doesn't exist. Can you please address this. I think the same would go for logical router policy too. Can you please address that also. Thanks Numa --- > tests/ovn-nbctl.at | 7 +++++-- > utilities/ovn-nbctl.8.xml | 19 +++++++++++++------ > utilities/ovn-nbctl.c | 39 ++++++++++++++++++++++++++++----------- > 3 files changed, 46 insertions(+), 19 deletions(-) > > diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at > index 1187fe3e1..14de1a8bf 100644 > --- a/tests/ovn-nbctl.at > +++ b/tests/ovn-nbctl.at > @@ -308,13 +308,16 @@ AT_CHECK([ovn-nbctl qos-list ls0], [0], [dnl > > AT_CHECK([ovn-nbctl qos-add ls0 from-lport 600 ip rate=1000101]) > AT_CHECK([ovn-nbctl qos-add ls0 from-lport 400 tcp dscp=44]) > -AT_CHECK([ovn-nbctl qos-add ls0 from-lport 200 ip burst=1000102 rate=301 > dscp=19]) > > dnl Delete a single flow. > AT_CHECK([ovn-nbctl qos-del ls0 from-lport 400 tcp]) > AT_CHECK([ovn-nbctl qos-list ls0], [0], [dnl > from-lport 600 (ip) rate=1000101 > -from-lport 200 (ip) rate=301 burst=1000102 dscp=19 > +]) > + > +dnl Delete QoS rule by specified uuid > +AT_CHECK([ovn-nbctl qos-del ls0 $(ovn-nbctl --bare --column _uuid list > qos)]) > +AT_CHECK([ovn-nbctl list qos], [0], [dnl > ]) > > AT_CHECK([ovn-nbctl qos-add ls0 from-lport 600 ip rate=100010111111], > [1], [], > diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml > index 9c624d40c..d265c7fcc 100644 > --- a/utilities/ovn-nbctl.8.xml > +++ b/utilities/ovn-nbctl.8.xml > @@ -160,12 +160,19 @@ > > <dt><code>qos-del</code> <var>switch</var> [<var>direction</var> > [<var>priority</var> <var>match</var>]]</dt> > <dd> > - Deletes QoS rules from <var>switch</var>. If only > - <var>switch</var> is supplied, all the QoS rules from the logical > - switch are deleted. If <var>direction</var> is also specified, > - then all the flows in that direction will be deleted from the > - logical switch. If all the fields are supplied, then a single > - flow that matches the given fields will be deleted. > + <p> > + Deletes QoS rules from <var>switch</var>. If only > + <var>switch</var> is supplied, all the QoS rules from the > logical > + switch are deleted. If <var>direction</var> is also specified, > + then all the flows in that direction will be deleted from the > + logical switch. If all the fields are supplied, then a single > + flow that matches the given fields will be deleted. > + </p> > + > + <p> > + If <var>switch</var> and <var>uuid</var> are supplied, then the > + QoS rule with sepcified uuid is deleted. > + </p> > </dd> > > <dt><code>qos-list</code> <var>switch</var></dt> > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > index 02fc10c9e..07ed85251 100644 > --- a/utilities/ovn-nbctl.c > +++ b/utilities/ovn-nbctl.c > @@ -604,7 +604,7 @@ ACL commands:\n\ > QoS commands:\n\ > qos-add SWITCH DIRECTION PRIORITY MATCH [rate=RATE [burst=BURST]] > [dscp=DSCP]\n\ > add an QoS rule to SWITCH\n\ > - qos-del SWITCH [DIRECTION [PRIORITY MATCH]]\n\ > + qos-del SWITCH [{DIRECTION | UUID} [PRIORITY MATCH]]\n\ > remove QoS rules from SWITCH\n\ > qos-list SWITCH print QoS rules for SWITCH\n\ > \n\ > @@ -2526,22 +2526,39 @@ nbctl_qos_del(struct ctl_context *ctx) > } > > const char *direction; > - error = parse_direction(ctx->argv[2], &direction); > - if (error) { > - ctx->error = error; > - return; > + const struct uuid *qos_rule_uuid = NULL; > + struct uuid uuid_from_cmd; > + if (uuid_from_string(&uuid_from_cmd, ctx->argv[2])) { > + qos_rule_uuid = &uuid_from_cmd; > + } else { > + error = parse_direction(ctx->argv[2], &direction); > + if (error) { > + ctx->error = error; > + return; > + } > } > > - /* If priority and match are not specified, delete all qos_rules with > the > - * specified direction. */ > + /* If uuid was specified, delete qos_rule with the > + * specified uuid. */ > if (ctx->argc == 3) { > struct nbrec_qos **new_qos_rules > = xmalloc(sizeof *new_qos_rules * ls->n_qos_rules); > > int n_qos_rules = 0; > - for (size_t i = 0; i < ls->n_qos_rules; i++) { > - if (strcmp(direction, ls->qos_rules[i]->direction)) { > - new_qos_rules[n_qos_rules++] = ls->qos_rules[i]; > + if (qos_rule_uuid) { > + for (size_t i = 0; i < ls->n_qos_rules; i++) { > + if (!uuid_equals(qos_rule_uuid, > + &(ls->qos_rules[i]->header_.uuid))) { > + new_qos_rules[n_qos_rules++] = ls->qos_rules[i]; > + } > + } > + /* If priority and match are not specified, delete all qos_rules > + * with the specified direction. */ > + } else { > + for (size_t i = 0; i < ls->n_qos_rules; i++) { > + if (strcmp(direction, ls->qos_rules[i]->direction)) { > + new_qos_rules[n_qos_rules++] = ls->qos_rules[i]; > + } > } > } > > @@ -6149,7 +6166,7 @@ static const struct ctl_command_syntax > nbctl_commands[] = { > { "qos-add", 5, 7, > "SWITCH DIRECTION PRIORITY MATCH [rate=RATE [burst=BURST]] > [dscp=DSCP]", > NULL, nbctl_qos_add, NULL, "--may-exist", RW }, > - { "qos-del", 1, 4, "SWITCH [DIRECTION [PRIORITY MATCH]]", NULL, > + { "qos-del", 1, 4, "SWITCH [{DIRECTION | UUID} [PRIORITY MATCH]]", > NULL, > nbctl_qos_del, NULL, "", RW }, > { "qos-list", 1, 1, "SWITCH", NULL, nbctl_qos_list, NULL, "", RO }, > > -- > 2.17.1 > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
Hi Numa, I have add codes to check if the uuid is exist. If not, it will print "uuid is not found". Please help to check , if this way is ok , I will summit another patch to fix the same problem in logical router policy. Regards, Yun -------------- taoyunxiang@cmss.chinamobile.com >On Sat, May 9, 2020 at 2:15 PM Tao YunXiang < >taoyunxiang@cmss.chinamobile.com> wrote: > >> usage: >> ovn-nbctl qos-del ls0 [UUID0] >> >> Author: Tao YunXiang <taoyunxiang@cmss.chinamobile.com> >> Co-authored-by: Liu Chang <liuchang@cmss.chinamobile.com> >> Co-authored-by: Rong Yin <rongyin@cmss.chinamobile.com> >> Signed-off-by: Tao YunXiang <taoyunxiang@cmss.chinamobile.com> >> Signed-off-by: Liu Chang <liuchang@cmss.chinamobile.com> >> Signed-off-by: Rong Yin <rongyin@cmss.chinamobile.com> >> >> --- >> v6: ovn-nbctl.c: Add an optional way to delete QoS by uuid >> v5: ovn-nbctl.c: Add an optional way to delete QoS by uuid >> v4: Add a way to delete QoS by its name or uuid >> v3: ovn-nbctl.c: Add a way to delete QoS by its name or uuid >> >> >Thanks for adding the test case. > >I see a small issue. When I run "ovn-nbctl qos-del <some_uuid>, the >commands doesn't >print any error if the qos with the given <some_uuid> doesn't exist. > >Can you please address this. >I think the same would go for logical router policy too. Can you please >address that also. > >Thanks >Numa > >--- >> tests/ovn-nbctl.at | 7 +++++-- >> utilities/ovn-nbctl.8.xml | 19 +++++++++++++------ >> utilities/ovn-nbctl.c | 39 ++++++++++++++++++++++++++++----------- >> 3 files changed, 46 insertions(+), 19 deletions(-) >> >> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at >> index 1187fe3e1..14de1a8bf 100644 >> --- a/tests/ovn-nbctl.at >> +++ b/tests/ovn-nbctl.at >> @@ -308,13 +308,16 @@ AT_CHECK([ovn-nbctl qos-list ls0], [0], [dnl >> >> AT_CHECK([ovn-nbctl qos-add ls0 from-lport 600 ip rate=1000101]) >> AT_CHECK([ovn-nbctl qos-add ls0 from-lport 400 tcp dscp=44]) >> -AT_CHECK([ovn-nbctl qos-add ls0 from-lport 200 ip burst=1000102 rate=301 >> dscp=19]) >> >> dnl Delete a single flow. >> AT_CHECK([ovn-nbctl qos-del ls0 from-lport 400 tcp]) >> AT_CHECK([ovn-nbctl qos-list ls0], [0], [dnl >> from-lport 600 (ip) rate=1000101 >> -from-lport 200 (ip) rate=301 burst=1000102 dscp=19 >> +]) >> + >> +dnl Delete QoS rule by specified uuid >> +AT_CHECK([ovn-nbctl qos-del ls0 $(ovn-nbctl --bare --column _uuid list >> qos)]) >> +AT_CHECK([ovn-nbctl list qos], [0], [dnl >> ]) >> >> AT_CHECK([ovn-nbctl qos-add ls0 from-lport 600 ip rate=100010111111], >> [1], [], >> diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml >> index 9c624d40c..d265c7fcc 100644 >> --- a/utilities/ovn-nbctl.8.xml >> +++ b/utilities/ovn-nbctl.8.xml >> @@ -160,12 +160,19 @@ >> >> <dt><code>qos-del</code> <var>switch</var> [<var>direction</var> >> [<var>priority</var> <var>match</var>]]</dt> >> <dd> >> - Deletes QoS rules from <var>switch</var>. If only >> - <var>switch</var> is supplied, all the QoS rules from the logical >> - switch are deleted. If <var>direction</var> is also specified, >> - then all the flows in that direction will be deleted from the >> - logical switch. If all the fields are supplied, then a single >> - flow that matches the given fields will be deleted. >> + <p> >> + Deletes QoS rules from <var>switch</var>. If only >> + <var>switch</var> is supplied, all the QoS rules from the >> logical >> + switch are deleted. If <var>direction</var> is also specified, >> + then all the flows in that direction will be deleted from the >> + logical switch. If all the fields are supplied, then a single >> + flow that matches the given fields will be deleted. >> + </p> >> + >> + <p> >> + If <var>switch</var> and <var>uuid</var> are supplied, then the >> + QoS rule with sepcified uuid is deleted. >> + </p> >> </dd> >> >> <dt><code>qos-list</code> <var>switch</var></dt> >> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c >> index 02fc10c9e..07ed85251 100644 >> --- a/utilities/ovn-nbctl.c >> +++ b/utilities/ovn-nbctl.c >> @@ -604,7 +604,7 @@ ACL commands:\n\ >> QoS commands:\n\ >> qos-add SWITCH DIRECTION PRIORITY MATCH [rate=RATE [burst=BURST]] >> [dscp=DSCP]\n\ >> add an QoS rule to SWITCH\n\ >> - qos-del SWITCH [DIRECTION [PRIORITY MATCH]]\n\ >> + qos-del SWITCH [{DIRECTION | UUID} [PRIORITY MATCH]]\n\ >> remove QoS rules from SWITCH\n\ >> qos-list SWITCH print QoS rules for SWITCH\n\ >> \n\ >> @@ -2526,22 +2526,39 @@ nbctl_qos_del(struct ctl_context *ctx) >> } >> >> const char *direction; >> - error = parse_direction(ctx->argv[2], &direction); >> - if (error) { >> - ctx->error = error; >> - return; >> + const struct uuid *qos_rule_uuid = NULL; >> + struct uuid uuid_from_cmd; >> + if (uuid_from_string(&uuid_from_cmd, ctx->argv[2])) { >> + qos_rule_uuid = &uuid_from_cmd; >> + } else { >> + error = parse_direction(ctx->argv[2], &direction); >> + if (error) { >> + ctx->error = error; >> + return; >> + } >> } >> >> - /* If priority and match are not specified, delete all qos_rules with >> the >> - * specified direction. */ >> + /* If uuid was specified, delete qos_rule with the >> + * specified uuid. */ >> if (ctx->argc == 3) { >> struct nbrec_qos **new_qos_rules >> = xmalloc(sizeof *new_qos_rules * ls->n_qos_rules); >> >> int n_qos_rules = 0; >> - for (size_t i = 0; i < ls->n_qos_rules; i++) { >> - if (strcmp(direction, ls->qos_rules[i]->direction)) { >> - new_qos_rules[n_qos_rules++] = ls->qos_rules[i]; >> + if (qos_rule_uuid) { >> + for (size_t i = 0; i < ls->n_qos_rules; i++) { >> + if (!uuid_equals(qos_rule_uuid, >> + &(ls->qos_rules[i]->header_.uuid))) { >> + new_qos_rules[n_qos_rules++] = ls->qos_rules[i]; >> + } >> + } >> + /* If priority and match are not specified, delete all qos_rules >> + * with the specified direction. */ >> + } else { >> + for (size_t i = 0; i < ls->n_qos_rules; i++) { >> + if (strcmp(direction, ls->qos_rules[i]->direction)) { >> + new_qos_rules[n_qos_rules++] = ls->qos_rules[i]; >> + } >> } >> } >> >> @@ -6149,7 +6166,7 @@ static const struct ctl_command_syntax >> nbctl_commands[] = { >> { "qos-add", 5, 7, >> "SWITCH DIRECTION PRIORITY MATCH [rate=RATE [burst=BURST]] >> [dscp=DSCP]", >> NULL, nbctl_qos_add, NULL, "--may-exist", RW }, >> - { "qos-del", 1, 4, "SWITCH [DIRECTION [PRIORITY MATCH]]", NULL, >> + { "qos-del", 1, 4, "SWITCH [{DIRECTION | UUID} [PRIORITY MATCH]]", >> NULL, >> nbctl_qos_del, NULL, "", RW }, >> { "qos-list", 1, 1, "SWITCH", NULL, nbctl_qos_list, NULL, "", RO }, >> >> -- >> 2.17.1 >> >> >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >>
diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at index 1187fe3e1..14de1a8bf 100644 --- a/tests/ovn-nbctl.at +++ b/tests/ovn-nbctl.at @@ -308,13 +308,16 @@ AT_CHECK([ovn-nbctl qos-list ls0], [0], [dnl AT_CHECK([ovn-nbctl qos-add ls0 from-lport 600 ip rate=1000101]) AT_CHECK([ovn-nbctl qos-add ls0 from-lport 400 tcp dscp=44]) -AT_CHECK([ovn-nbctl qos-add ls0 from-lport 200 ip burst=1000102 rate=301 dscp=19]) dnl Delete a single flow. AT_CHECK([ovn-nbctl qos-del ls0 from-lport 400 tcp]) AT_CHECK([ovn-nbctl qos-list ls0], [0], [dnl from-lport 600 (ip) rate=1000101 -from-lport 200 (ip) rate=301 burst=1000102 dscp=19 +]) + +dnl Delete QoS rule by specified uuid +AT_CHECK([ovn-nbctl qos-del ls0 $(ovn-nbctl --bare --column _uuid list qos)]) +AT_CHECK([ovn-nbctl list qos], [0], [dnl ]) AT_CHECK([ovn-nbctl qos-add ls0 from-lport 600 ip rate=100010111111], [1], [], diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml index 9c624d40c..d265c7fcc 100644 --- a/utilities/ovn-nbctl.8.xml +++ b/utilities/ovn-nbctl.8.xml @@ -160,12 +160,19 @@ <dt><code>qos-del</code> <var>switch</var> [<var>direction</var> [<var>priority</var> <var>match</var>]]</dt> <dd> - Deletes QoS rules from <var>switch</var>. If only - <var>switch</var> is supplied, all the QoS rules from the logical - switch are deleted. If <var>direction</var> is also specified, - then all the flows in that direction will be deleted from the - logical switch. If all the fields are supplied, then a single - flow that matches the given fields will be deleted. + <p> + Deletes QoS rules from <var>switch</var>. If only + <var>switch</var> is supplied, all the QoS rules from the logical + switch are deleted. If <var>direction</var> is also specified, + then all the flows in that direction will be deleted from the + logical switch. If all the fields are supplied, then a single + flow that matches the given fields will be deleted. + </p> + + <p> + If <var>switch</var> and <var>uuid</var> are supplied, then the + QoS rule with sepcified uuid is deleted. + </p> </dd> <dt><code>qos-list</code> <var>switch</var></dt> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c index 02fc10c9e..07ed85251 100644 --- a/utilities/ovn-nbctl.c +++ b/utilities/ovn-nbctl.c @@ -604,7 +604,7 @@ ACL commands:\n\ QoS commands:\n\ qos-add SWITCH DIRECTION PRIORITY MATCH [rate=RATE [burst=BURST]] [dscp=DSCP]\n\ add an QoS rule to SWITCH\n\ - qos-del SWITCH [DIRECTION [PRIORITY MATCH]]\n\ + qos-del SWITCH [{DIRECTION | UUID} [PRIORITY MATCH]]\n\ remove QoS rules from SWITCH\n\ qos-list SWITCH print QoS rules for SWITCH\n\ \n\ @@ -2526,22 +2526,39 @@ nbctl_qos_del(struct ctl_context *ctx) } const char *direction; - error = parse_direction(ctx->argv[2], &direction); - if (error) { - ctx->error = error; - return; + const struct uuid *qos_rule_uuid = NULL; + struct uuid uuid_from_cmd; + if (uuid_from_string(&uuid_from_cmd, ctx->argv[2])) { + qos_rule_uuid = &uuid_from_cmd; + } else { + error = parse_direction(ctx->argv[2], &direction); + if (error) { + ctx->error = error; + return; + } } - /* If priority and match are not specified, delete all qos_rules with the - * specified direction. */ + /* If uuid was specified, delete qos_rule with the + * specified uuid. */ if (ctx->argc == 3) { struct nbrec_qos **new_qos_rules = xmalloc(sizeof *new_qos_rules * ls->n_qos_rules); int n_qos_rules = 0; - for (size_t i = 0; i < ls->n_qos_rules; i++) { - if (strcmp(direction, ls->qos_rules[i]->direction)) { - new_qos_rules[n_qos_rules++] = ls->qos_rules[i]; + if (qos_rule_uuid) { + for (size_t i = 0; i < ls->n_qos_rules; i++) { + if (!uuid_equals(qos_rule_uuid, + &(ls->qos_rules[i]->header_.uuid))) { + new_qos_rules[n_qos_rules++] = ls->qos_rules[i]; + } + } + /* If priority and match are not specified, delete all qos_rules + * with the specified direction. */ + } else { + for (size_t i = 0; i < ls->n_qos_rules; i++) { + if (strcmp(direction, ls->qos_rules[i]->direction)) { + new_qos_rules[n_qos_rules++] = ls->qos_rules[i]; + } } } @@ -6149,7 +6166,7 @@ static const struct ctl_command_syntax nbctl_commands[] = { { "qos-add", 5, 7, "SWITCH DIRECTION PRIORITY MATCH [rate=RATE [burst=BURST]] [dscp=DSCP]", NULL, nbctl_qos_add, NULL, "--may-exist", RW }, - { "qos-del", 1, 4, "SWITCH [DIRECTION [PRIORITY MATCH]]", NULL, + { "qos-del", 1, 4, "SWITCH [{DIRECTION | UUID} [PRIORITY MATCH]]", NULL, nbctl_qos_del, NULL, "", RW }, { "qos-list", 1, 1, "SWITCH", NULL, nbctl_qos_list, NULL, "", RO },