[ovs-dev] [PATCH 1/2] dpctl: manage ret value when dumping CT entries.
Darrell Ball
dball at vmware.com
Mon Sep 25 19:31:16 UTC 2017
On 9/25/17, 2:27 AM, "Fischetti, Antonio" <antonio.fischetti at intel.com> wrote:
Hi Darrell,
I agree with your suggestion in keeping 'error'
as the only variable to manage return values.
In this case - as I'm assuming we shouldn't return an EOF to the
caller - I should manage error as below?
if (error == EOF) {
error = 0; << EOF is not an issue, so return 0 to the caller
} else if (error) {
dpctl_error(dpctl_p, error, "dumping conntrack entry");
ct_dpif_dump_done(dump);
dpif_close(dpif);
return error;
}
[Darrell] For sure - EOF should not be returned to user since it is not an error.
The other point I wanted to make is:
I think you can trivially fall thru. for dpctl_dump_conntrack()
After doing just dpctl_error(dpctl_p, error, "dumping conntrack entry");
(comments inline).
And in the other 2 cases, I think you can still try to print out whatever is known
after breaking out of the while loop and use the existing cleanup code.
You would print error in the cases as well
What do you think ?
> -----Original Message-----
> From: Darrell Ball [mailto:dball at vmware.com]
> Sent: Friday, September 22, 2017 8:42 AM
> To: Fischetti, Antonio <antonio.fischetti at intel.com>; dev at openvswitch.org
> Subject: Re: [ovs-dev] [PATCH 1/2] dpctl: manage ret value when dumping CT
> entries.
>
> Few comments Antonio
>
> Darrell
>
> On 9/13/17, 5:37 AM, "ovs-dev-bounces at openvswitch.org on behalf of
> antonio.fischetti at intel.com" <ovs-dev-bounces at openvswitch.org on behalf of
> antonio.fischetti at intel.com> wrote:
>
> Manage error value returned by ct_dpif_dump_next.
>
> Signed-off-by: Antonio Fischetti <antonio.fischetti at intel.com>
> ---
> lib/dpctl.c | 28 +++++++++++++++++++++++++---
> 1 file changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index 8951d6e..86d0f90 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -1263,6 +1263,7 @@ dpctl_dump_conntrack(int argc, const char *argv[],
> struct dpif *dpif;
> char *name;
> int error;
> + int ret;
>
> if (argc > 1 && ovs_scan(argv[argc - 1], "zone=%"SCNu16, &zone)) {
> pzone = &zone;
> @@ -1286,7 +1287,7 @@ dpctl_dump_conntrack(int argc, const char *argv[],
> return error;
> }
>
> - while (!ct_dpif_dump_next(dump, &cte)) {
> + while (!(ret = ct_dpif_dump_next(dump, &cte))) {
> struct ds s = DS_EMPTY_INITIALIZER;
>
> ct_dpif_format_entry(&cte, &s, dpctl_p->verbosity,
> @@ -1296,6 +1297,13 @@ dpctl_dump_conntrack(int argc, const char *argv[],
> dpctl_print(dpctl_p, "%s\n", ds_cstr(&s));
> ds_destroy(&s);
> }
> + if (ret && ret != EOF) {
> + dpctl_error(dpctl_p, ret, "dumping conntrack entry");
> + ct_dpif_dump_done(dump);
> + dpif_close(dpif);
> + return ret;
> + }
> +
>
> [Darrell] Maybe we can reuse ‘error’ ?
> if (error && error != EOF) {
> and just do
> dpctl_error(dpctl_p, error, "dumping conntrack entry");
> and then fall thru for cleanup ?
>
>
> ct_dpif_dump_done(dump);
> dpif_close(dpif);
> return error;
> @@ -1348,6 +1356,7 @@ dpctl_ct_stats_show(int argc, const char *argv[],
> int proto_stats[CT_STATS_MAX];
> int tcp_conn_per_states[CT_DPIF_TCPS_MAX_NUM];
> int error;
> + int ret;
>
> while (argc > 1 && lastargc != argc) {
> lastargc = argc;
> @@ -1384,7 +1393,7 @@ dpctl_ct_stats_show(int argc, const char *argv[],
> }
>
> int tot_conn = 0;
> - while (!ct_dpif_dump_next(dump, &cte)) {
> + while (!(ret = ct_dpif_dump_next(dump, &cte))) {
> ct_dpif_entry_uninit(&cte);
> tot_conn++;
> switch (cte.tuple_orig.ip_proto) {
> @@ -1425,6 +1434,12 @@ dpctl_ct_stats_show(int argc, const char *argv[],
> break;
> }
> }
> + if (ret && ret != EOF) {
> + dpctl_error(dpctl_p, ret, "dumping conntrack entry");
> + ct_dpif_dump_done(dump);
> + dpif_close(dpif);
> + return ret;
> + }
>
> [Darrell]
> Can we reuse ‘error’, just print error and fall thru. ?
> It looks like it is safe to print whatever we know, which could be useful.
> Otherwise, if we have an error in dump_next, we may never be able to see any
> useful info.
> for debugging the same error or something else.
>
>
> dpctl_print(dpctl_p, "Connections Stats:\n Total: %d\n", tot_conn);
> if (proto_stats[CT_STATS_TCP]) {
> @@ -1482,6 +1497,7 @@ dpctl_ct_bkts(int argc, const char *argv[],
> uint16_t *pzone = NULL;
> int tot_bkts = 0;
> int error;
> + int ret;
>
> if (argc > 1 && !strncmp(argv[argc - 1], CT_BKTS_GT,
> strlen(CT_BKTS_GT))) {
> if (ovs_scan(argv[argc - 1], CT_BKTS_GT"%"SCNu16, >)) {
> @@ -1521,7 +1537,7 @@ dpctl_ct_bkts(int argc, const char *argv[],
> int tot_conn = 0;
> uint32_t *conn_per_bkts = xzalloc(tot_bkts * sizeof(uint32_t));
>
> - while (!ct_dpif_dump_next(dump, &cte)) {
> + while (!(ret = ct_dpif_dump_next(dump, &cte))) {
> ct_dpif_entry_uninit(&cte);
> tot_conn++;
> if (tot_bkts > 0) {
> @@ -1533,6 +1549,12 @@ dpctl_ct_bkts(int argc, const char *argv[],
> }
> }
> }
> + if (ret && ret != EOF) {
> + dpctl_error(dpctl_p, ret, "dumping conntrack entry");
> + ct_dpif_dump_done(dump);
> + dpif_close(dpif);
> + return ret;
> + }
>
> [Darrell]
> Same comment here:
> Can we reuse ‘error’, just print error and fall thru. ?
> It looks like it is safe to print whatever we know, which could be useful.
> Otherwise, if we have an error in dump_next, we may never be able to see any
> useful info.
> for debugging the same error or something else.
>
> dpctl_print(dpctl_p, "Current Connections: %d\n", tot_conn);
> dpctl_print(dpctl_p, "\n");
> --
> 2.4.11
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__mail.openvswitch.org_mailman_listinfo_ovs-
> 2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-
> uZnsw&m=HjzfhOU1f9U5hUM2bWAuizS921ZAAEjbIRHSM65FIAQ&s=u0r4kn1mAoWiO_rjM95TwOt5t
> -7hC2jiO3y_QKHHXNw&e=
>
More information about the dev
mailing list