[ovs-dev] [PATCH 1/2] dpctl: manage ret value when dumping CT entries.
Fischetti, Antonio
antonio.fischetti at intel.com
Mon Sep 25 09:27:33 UTC 2017
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;
}
> -----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