From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
by inbox.dpdk.org (Postfix) with ESMTP id 97B27A04B7;
Thu, 17 Sep 2020 15:34:47 +0200 (CEST)
Received: from [92.243.14.124] (localhost [127.0.0.1])
by dpdk.org (Postfix) with ESMTP id 988CD1D644;
Thu, 17 Sep 2020 15:34:46 +0200 (CEST)
Received: from mail-wm1-f65.google.com (mail-wm1-f65.google.com
[209.85.128.65]) by dpdk.org (Postfix) with ESMTP id 0AE4E1D643
for <dev@dpdk.org>; Thu, 17 Sep 2020 15:34:45 +0200 (CEST)
Received: by mail-wm1-f65.google.com with SMTP id z9so2091251wmk.1
for <dev@dpdk.org>; Thu, 17 Sep 2020 06:34:44 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google;
h=date:from:to:cc:subject:message-id:references:mime-version
:content-disposition:in-reply-to:user-agent;
bh=2EuMLmwgyViDEwIy7Zk2xGBUAT9URgyxb60xO+NkyrQ=;
b=XNsE5j6/GDkuD7Vcv/sy9zpBdvuwH8VP24Gtiy1/konrUm34WKmtH31sb6MFMJGVkv
jXXJkUKOA94CceuGuPfx87/SZQY4PmypbG2QZ3JXF9Ji9HQm4LbkVlWcB2zJvNAs4wy1
u40Hn5mqc1qVz278Vq8v79xl+rZpe0/+dv1g2QLV+DCB+P0ZciqumUwSBW/3wKfMklug
qd8CG9ziwR9Q0x4inIfr0PV08ULc5xn7v+1EALUEaCiWkQdb3AoKXRto3OmqOV/+LqIi
54GYQlZFSdIDNrbaMwZU0gOE/Wnvuiz4A8v3EJfC3+CsjZSdee7LoknlUYQOsTgnYcDK
wx+g==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20161025;
h=x-gm-message-state:date:from:to:cc:subject:message-id:references
:mime-version:content-disposition:in-reply-to:user-agent;
bh=2EuMLmwgyViDEwIy7Zk2xGBUAT9URgyxb60xO+NkyrQ=;
b=ZKh7qJ4zpZPPa9lfnJOm8WlWwSA0H5qb9QSj3lHTUZLOaP10kkCr9hWhh5EjJkuLgR
R9SPNNgDoXjDBWSCcKIkw+mWvKsdleZ9R+BEA6UWREcsoRP3IW2vJ0eXflJNKxJ+iT6j
bEvYkXkkRVDpgM7HYjvAesckyr6lAT47WQS+/SlopyRcBk039uqVZ+UFH1+mggXYDmct
kG1yHLUsu7L8bECb96GPTHBQsZR7Sn9nlxy21Qwzg+3wOdKYPUBGrMjx88IZEYtiQl/h
TvTT8reqT14shGORCyniMR1XZGpAEqEnhQHd9NBI7EwV5OmmtZczcUlxlwVtlgnYNWNz
E9vA==
X-Gm-Message-State: AOAM531Kxi7ZeHRy0uloG8X0DUUuOvre/BQt54sVD+I8DMsXoXA10PBY
U91X15RPJvKW6ssacldNzuv6LQ==
X-Google-Smtp-Source: ABdhPJxB8FubSf74HE0CZGhjrK4+JlYdAou3KNFnvdOnnoS3c8e3C4Sv/R22QU6+NBjCjvoaeGTtmw==
X-Received: by 2002:a1c:9e0e:: with SMTP id h14mr9927411wme.18.1600349684682;
Thu, 17 Sep 2020 06:34:44 -0700 (PDT)
Received: from 6wind.com (2a01cb0c0005a600345636f7e65ed1a0.ipv6.abo.wanadoo.fr.
[2a01:cb0c:5:a600:3456:36f7:e65e:d1a0])
by smtp.gmail.com with ESMTPSA id d18sm38950776wrm.10.2020.09.17.06.34.43
(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);
Thu, 17 Sep 2020 06:34:43 -0700 (PDT)
Date: Thu, 17 Sep 2020 15:34:43 +0200
From: Olivier Matz <olivier.matz@6wind.com>
To: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
Cc: dev@dpdk.org, Dmitry Malloy <dmitrym@microsoft.com>,
Narcisa Ana Maria Vasile <Narcisa.Vasile@microsoft.com>,
Fady Bader <fady@mellanox.com>, Tal Shnaiderman <talshn@mellanox.com>,
"Kadam, Pallavi" <pallavi.kadam@intel.com>,
Ray Kinsella <mdr@ashroe.eu>, Neil Horman <nhorman@tuxdriver.com>
Message-ID: <20200917133443.GR21395@platinum>
References: <20200620210511.13134-1-dmitry.kozliuk@gmail.com>
<20200730210652.14568-1-dmitry.kozliuk@gmail.com>
<20200730210652.14568-2-dmitry.kozliuk@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200730210652.14568-2-dmitry.kozliuk@gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Subject: Re: [dpdk-dev] [PATCH v2 1/7] cmdline: make implementation opaque
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
Hi Dmitry,
On Fri, Jul 31, 2020 at 12:06:45AM +0300, Dmitry Kozlyuk wrote:
> struct cmdline exposes platform-specific members it contains, most
> notably struct termios that is only available on Unix. Make the
> structure opaque.
>
> Remove tests checking struct cmdline content as meaningless.
>
> Add cmdline_get_rdline() to access history buffer.
> The new function is currently used only in tests.
>
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
First, please forgive me for the very late feedback. It is all the more
problematic because I think this patch introduces an ABI breakage, that
should have been announced.
Making cmdline struct opaque clearly goes in the right direction, as
it will prevent future ABI breakage.
In my opinion, we could accept the patch for 20.11, knowing it reduce
the risk of future ABI breakage, and that cmdline is not a core
component of DPDK. However it has to be discussed and accepted by other
people.
Else, the patch would be delayed for 21.11. From what I see from the
other patches, it looks possible to keep cmdline struct public without
breaking the existing ABI by adding #ifdef CONFIG_RTE_EXEC_ENV_WINDOWS,
is it correct?
One minor comment below:
> --- a/app/test/test_cmdline_lib.c
> +++ b/app/test/test_cmdline_lib.c
> @@ -46,22 +46,29 @@ complete_buffer(__rte_unused struct rdline *rdl,
> static int
> test_cmdline_parse_fns(void)
> {
> - struct cmdline cl;
> + struct cmdline *cl;
> + cmdline_parse_ctx_t ctx;
> int i = 0;
> char dst[CMDLINE_TEST_BUFSIZE];
>
> + cl = cmdline_new(&ctx, "prompt", -1, -1);
> + if (cl == NULL) {
> + printf("Error: cannot create cmdline to test parse fns!\n");
> + return -1;
> + }
> +
> if (cmdline_parse(NULL, "buffer") >= 0)
> goto error;
> - if (cmdline_parse(&cl, NULL) >= 0)
> + if (cmdline_parse(cl, NULL) >= 0)
> goto error;
>
> if (cmdline_complete(NULL, "buffer", &i, dst, sizeof(dst)) >= 0)
> goto error;
> - if (cmdline_complete(&cl, NULL, &i, dst, sizeof(dst)) >= 0)
> + if (cmdline_complete(cl, NULL, &i, dst, sizeof(dst)) >= 0)
> goto error;
> - if (cmdline_complete(&cl, "buffer", NULL, dst, sizeof(dst)) >= 0)
> + if (cmdline_complete(cl, "buffer", NULL, dst, sizeof(dst)) >= 0)
> goto error;
> - if (cmdline_complete(&cl, "buffer", &i, NULL, sizeof(dst)) >= 0)
> + if (cmdline_complete(cl, "buffer", &i, NULL, sizeof(dst)) >= 0)
> goto error;
>
> return 0;
> @@ -166,11 +173,11 @@ static int
> test_cmdline_fns(void)
> {
> cmdline_parse_ctx_t ctx;
> - struct cmdline cl, *tmp;
> + struct cmdline *cl;
>
> memset(&ctx, 0, sizeof(ctx));
> - tmp = cmdline_new(&ctx, "test", -1, -1);
> - if (tmp == NULL)
> + cl = cmdline_new(&ctx, "test", -1, -1);
> + if (cl == NULL)
> goto error;
>
> if (cmdline_new(NULL, "prompt", 0, 0) != NULL)
> @@ -179,7 +186,7 @@ test_cmdline_fns(void)
> goto error;
> if (cmdline_in(NULL, "buffer", CMDLINE_TEST_BUFSIZE) >= 0)
> goto error;
> - if (cmdline_in(&cl, NULL, CMDLINE_TEST_BUFSIZE) >= 0)
> + if (cmdline_in(cl, NULL, CMDLINE_TEST_BUFSIZE) >= 0)
> goto error;
> if (cmdline_write_char(NULL, 0) >= 0)
> goto error;
> @@ -188,31 +195,14 @@ test_cmdline_fns(void)
> cmdline_set_prompt(NULL, "prompt");
> cmdline_free(NULL);
> cmdline_printf(NULL, "format");
> - /* this should fail as stream handles are invalid */
> - cmdline_printf(tmp, "format");
> cmdline_interact(NULL);
> cmdline_quit(NULL);
>
> - /* check if void calls change anything when they should fail */
> - cl = *tmp;
> -
> - cmdline_printf(&cl, NULL);
> - if (memcmp(&cl, tmp, sizeof(cl))) goto mismatch;
> - cmdline_set_prompt(&cl, NULL);
> - if (memcmp(&cl, tmp, sizeof(cl))) goto mismatch;
> - cmdline_in(&cl, NULL, CMDLINE_TEST_BUFSIZE);
> - if (memcmp(&cl, tmp, sizeof(cl))) goto mismatch;
> -
> - cmdline_free(tmp);
> -
> return 0;
>
> error:
> printf("Error: function accepted null parameter!\n");
> return -1;
> -mismatch:
> - printf("Error: data changed!\n");
> - return -1;
cmdline_free(cl) is missing.
before your patch, cmdline_free(tmp) was already missing in error case
by the way.
Thanks,
Olivier