From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 ; Thu, 17 Sep 2020 15:34:45 +0200 (CEST) Received: by mail-wm1-f65.google.com with SMTP id z9so2091251wmk.1 for ; 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 To: Dmitry Kozlyuk Cc: dev@dpdk.org, Dmitry Malloy , Narcisa Ana Maria Vasile , Fady Bader , Tal Shnaiderman , "Kadam, Pallavi" , Ray Kinsella , Neil Horman 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 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