From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id F0B62A0C47; Tue, 5 Oct 2021 10:27:12 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7DCE6412D6; Tue, 5 Oct 2021 10:27:12 +0200 (CEST) Received: from mail-wr1-f50.google.com (mail-wr1-f50.google.com [209.85.221.50]) by mails.dpdk.org (Postfix) with ESMTP id EC855412C6 for ; Tue, 5 Oct 2021 10:27:10 +0200 (CEST) Received: by mail-wr1-f50.google.com with SMTP id v25so25135834wra.2 for ; Tue, 05 Oct 2021 01:27:10 -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; bh=ZfkO8M0c8Mz67Aw0vYgB4WvsP4UzfX6gA8JRKKtF+a8=; b=grEQrGCnbTG8yyLX/dpXQQj3pN54q4ZYcji8nnWvk5A2h/HyttwV6Y4KPGsIMeks+i ZWyV52DPBaHW/bbRS+AG4MYXbVQNBy3FACEsXnlDMQjUBEIrM1s2dJIswlu+A7Mnpg3m HBc5qpmDUTGDeLcsS4ytnpx5dPas8iziILivZ78+7kJq9HoabkV67ppxl2d4NnHKEs5h 8/SfEiq3aD4JP3IOEdliKP5REk+nPpHr6PpXJCI9igN6BEfM6IIN5luy5IsuBM+pE00n qh1ivFUKV489TH3mWjyYdNZZ7EpabH9GwdAhJ/jlL/dFMV+1MCXLi3d2Ko1xJUN5z0ln r47A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=ZfkO8M0c8Mz67Aw0vYgB4WvsP4UzfX6gA8JRKKtF+a8=; b=wDxVANxW8ROVWR53WDgdiFIt+C+HI+8e6a0gjZMfAGMJUvpMmK1wQSitf4fHcGC1dl dJlFIHbYOBpyejndkAsYWho12V8lJkI169uxw5DxPslm+zAyfBoTzXfx4hUk+KF0V4+Y B63AivVbCCfBVeOj7OOaH7RFQQeZPTQeZup8yMEBhOaC/cV2fIr9dADZszPlVyZHP8MN KofBPC7r1/39RtSS/mJyZHTn2nq4Ajh8p2SzrhR3hI7BQNdESlyg1HywuE4pb+Df1PJU x+C74NjeSbXDaEUWOlK2HtLSXeBEUaJWLDroxn7AlQjWITWbOKkY0DDPplhDDgHXri4U EtSw== X-Gm-Message-State: AOAM533H9yZvIUdXYzaIxhCmwBHxA+bdxPZ5wxnK2Z63NQrzMwVo3+oL W/fSxtBQwu/sroahFLXqmMX8Xw== X-Google-Smtp-Source: ABdhPJzbK5F8p+mvlgSZzscZWP0ujpu9Zbc1yT8Zsu2XL3HlyVzdQGhcwmhei3ZDL0LoR0JxlaQRDw== X-Received: by 2002:adf:a40f:: with SMTP id d15mr13816857wra.41.1633422430686; Tue, 05 Oct 2021 01:27:10 -0700 (PDT) Received: from 6wind.com ([2a01:e0a:5ac:6460:c065:401d:87eb:9b25]) by smtp.gmail.com with ESMTPSA id c18sm1153864wmb.27.2021.10.05.01.27.09 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 05 Oct 2021 01:27:10 -0700 (PDT) Date: Tue, 5 Oct 2021 10:27:09 +0200 From: Olivier Matz To: Dmitry Kozlyuk Cc: dev@dpdk.org, David Marchand , Ali Alnubani , Gregory Etelson , Ray Kinsella Message-ID: References: <20210928194714.365563-1-dmitry.kozliuk@gmail.com> <20211005005516.132377-1-dmitry.kozliuk@gmail.com> <20211005005516.132377-3-dmitry.kozliuk@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20211005005516.132377-3-dmitry.kozliuk@gmail.com> Subject: Re: [dpdk-dev] [PATCH v3 2/2] cmdline: make struct rdline opaque X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 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, Few comments below. On Tue, Oct 05, 2021 at 03:55:16AM +0300, Dmitry Kozlyuk wrote: > Hide struct rdline definition and some RDLINE_* constants in order > to be able to change internal buffer sizes transparently to the user. > Add new functions: > > * rdline_create(): allocate and initialize struct rdline. > This function replaces rdline_init() and takes an extra parameter: > opaque user data for the callbacks. > * rdline_free(): deallocate struct rdline. > * rdline_get_history_buffer_size(): for use in tests. > * rdline_get_opaque(): to obtain user data in callback functions. > > Remove rdline_init() function from library headers and export list, > because using it requires the knowledge of sizeof(struct rdline). > > Signed-off-by: Dmitry Kozlyuk > --- > app/test-cmdline/commands.c | 2 +- > app/test/test_cmdline_lib.c | 19 ++++-- > doc/guides/rel_notes/release_21_11.rst | 3 + > lib/cmdline/cmdline.c | 3 +- > lib/cmdline/cmdline_cirbuf.c | 1 - > lib/cmdline/cmdline_private.h | 49 ++++++++++++++ > lib/cmdline/cmdline_rdline.c | 50 ++++++++++++++- > lib/cmdline/cmdline_rdline.h | 88 ++++++++++---------------- > lib/cmdline/version.map | 8 ++- > 9 files changed, 154 insertions(+), 69 deletions(-) > > diff --git a/app/test-cmdline/commands.c b/app/test-cmdline/commands.c > index d732976f08..a13e1d1afd 100644 > --- a/app/test-cmdline/commands.c > +++ b/app/test-cmdline/commands.c > @@ -297,7 +297,7 @@ cmd_get_history_bufsize_parsed(__rte_unused void *parsed_result, > struct rdline *rdl = cmdline_get_rdline(cl); > > cmdline_printf(cl, "History buffer size: %zu\n", > - sizeof(rdl->history_buf)); > + rdline_get_history_buffer_size(rdl)); My first impression was that having this function just for the tests was not really useful. But thinking a bit more about it, now that the structure is hidden, we could have an API in the future to change the size of the history buffer, in which case this function makes sense. [...] > diff --git a/lib/cmdline/cmdline_cirbuf.c b/lib/cmdline/cmdline_cirbuf.c > index 829a8af563..cbb76a7016 100644 > --- a/lib/cmdline/cmdline_cirbuf.c > +++ b/lib/cmdline/cmdline_cirbuf.c > @@ -10,7 +10,6 @@ > > #include "cmdline_cirbuf.h" > > - > int > cirbuf_init(struct cirbuf *cbuf, char *buf, unsigned int start, unsigned int maxlen) > { unexpected change [...] > --- a/lib/cmdline/cmdline_rdline.c > +++ b/lib/cmdline/cmdline_rdline.c > @@ -13,6 +13,7 @@ > #include > > #include "cmdline_cirbuf.h" > +#include "cmdline_private.h" > #include "cmdline_rdline.h" > > static void rdline_puts(struct rdline *rdl, const char *buf); > @@ -37,9 +38,10 @@ isblank2(char c) > > int > rdline_init(struct rdline *rdl, > - rdline_write_char_t *write_char, > - rdline_validate_t *validate, > - rdline_complete_t *complete) > + rdline_write_char_t *write_char, > + rdline_validate_t *validate, > + rdline_complete_t *complete, > + void *opaque) > { > if (!rdl || !write_char || !validate || !complete) > return -EINVAL; > @@ -47,10 +49,40 @@ rdline_init(struct rdline *rdl, > rdl->validate = validate; > rdl->complete = complete; > rdl->write_char = write_char; > + rdl->opaque = opaque; > rdl->status = RDLINE_INIT; > return cirbuf_init(&rdl->history, rdl->history_buf, 0, RDLINE_HISTORY_BUF_SIZE); > } > > +int > +rdline_create(struct rdline **out, > + rdline_write_char_t *write_char, > + rdline_validate_t *validate, > + rdline_complete_t *complete, > + void *opaque) > +{ For consistency, wouldn't it be better to keep the same model than cmdline_new()? I mean return a pointer and name it rdline_new(). > + struct rdline *rdl; > + int ret; > + > + if (out == NULL) > + return -EINVAL; > + rdl = malloc(sizeof(*rdl)); > + if (rdl == NULL) > + return -ENOMEM; > + ret = rdline_init(rdl, write_char, validate, complete, opaque); > + if (ret < 0) > + free(rdl); > + else > + *out = rdl; > + return ret; > +} [...] > +size_t > +rdline_get_history_buffer_size(struct rdline *rdl) > +{ > + return sizeof(rdl->history_buf); > +} > + > +void * > +rdline_get_opaque(struct rdline *rdl) > +{ > + return rdl != NULL ? rdl->opaque : NULL; > +} rdline_get_opaque() is safe when rdl is NULL, but rdline_get_history_buffer_size() is not. To me, both are acceptable but I'd prefer to have the same behavior for these 2 functions. Apart from that, looks good to me. Thanks! Olivier