On 9/25/17, 2:27 AM, "Fischetti, Antonio" <[email protected]> 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:[email protected]]
> Sent: Friday, September 22, 2017 8:42 AM
> To: Fischetti, Antonio <[email protected]>; [email protected]
> 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, "[email protected] on behalf of
> [email protected]" <[email protected] on behalf of
> [email protected]> wrote:
>
> Manage error value returned by ct_dpif_dump_next.
>
> Signed-off-by: Antonio Fischetti <[email protected]>
> ---
> 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
> [email protected]
> 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=
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
