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 7E734A0C47; Tue, 5 Oct 2021 11:11:46 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 169FE412B3; Tue, 5 Oct 2021 11:11:46 +0200 (CEST) Received: from mail-wm1-f53.google.com (mail-wm1-f53.google.com [209.85.128.53]) by mails.dpdk.org (Postfix) with ESMTP id DF8344129A for ; Tue, 5 Oct 2021 11:11:44 +0200 (CEST) Received: by mail-wm1-f53.google.com with SMTP id z184-20020a1c7ec1000000b003065f0bc631so2371853wmc.0 for ; Tue, 05 Oct 2021 02:11: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; bh=YyCGCssRCVlTSR6bw0E0SksZlKr4sCi9eAge0fptYKo=; b=b8KVw7+oDeJHv/GLJ+yPzWJrTlGpfLkaKKLJAHmHQzwK2A3rwpbWcmVVsF03bB3MvE PN9J8zSG3d8JGKbh1oEkWCccVNddImJWXAqmi25N82JTU8Uai15C84lm/GdcpPcgvQYG 7M0tBEC2mwHr9bnxQi/4oXw2ArOrhXABsEKvaMzXupRpLxIMTxZBlGs71qAU70XNFaYf w3dYYxgdHI6SqYy5+7ohDfPInBQLvi7pH6MMHhnRRQO+0cdJQw12bruQmM46nLzt/gbj QAXAakGcE+76N6kzRlHCdQn0cNncps/3kelWgUjzBWM4F8ACJ4On3Q/3Yq3IYLSemo7H 0FhA== 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=YyCGCssRCVlTSR6bw0E0SksZlKr4sCi9eAge0fptYKo=; b=5sjBOYVIBifUA9UfKtEQKniYIKxq4tarDCFFmO+A9vBxvO69iHXWk8d8jbcqAEwpFJ yyE16IB6agWuXdHodskbBA/EH1O0bCmavsoCG0d5/z5ZEx4kP0YqH1zBQcn6/r6/I4vT /Xt6du+ZyMeJt0HR1a78eboX6DJuEDSle7GoIIiaimUFNnZpHluTC1UuSllRapIXK74a Hfgt0w0sv/hhDk1XIBfy0uL+pFxG5RVZR9pb9UugPoGKcgrwAGeVGmBphbXfbEGwkUIj YqwgP/kggZ7nlx3Wv5oyDmsuJggKfS42+pAnpR0+fMDZWb0YwaAmgUgq4U8WMpm5VG5W gupg== X-Gm-Message-State: AOAM530yOsMdnktLVNWrSjfHQooSaYS9DDk+ETh3UiY7+ArueE3/rw/Q QaS90qPqY/jVYK+43iPM2+uCFA== X-Google-Smtp-Source: ABdhPJx7Bn46yjWKoRWHnSCJ8Hy+ka2N8TACbcNCbSZOKrKNfvfmszI+Yh/E3NtrW7sDIq6by9xPgA== X-Received: by 2002:a05:600c:35d1:: with SMTP id r17mr2164967wmq.153.1633425104638; Tue, 05 Oct 2021 02:11:44 -0700 (PDT) Received: from 6wind.com ([2a01:e0a:5ac:6460:c065:401d:87eb:9b25]) by smtp.gmail.com with ESMTPSA id x15sm8060856wrl.74.2021.10.05.02.11.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 05 Oct 2021 02:11:44 -0700 (PDT) Date: Tue, 5 Oct 2021 11:11:43 +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> <20211005120301.3c28e806@sovereign> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20211005120301.3c28e806@sovereign> 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" On Tue, Oct 05, 2021 at 12:03:01PM +0300, Dmitry Kozlyuk wrote: > Hi Olivier, > > Thanks for the review, please see below. > > 2021-10-05 10:27 (UTC+0200), Olivier Matz: > > [...] > > > 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 > > Will remove in v4. > > > [...] > > > > > --- 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(). > > If we don't really need to distinguish EINVAL and ENOMEM errors here, > then I agree. Otherwise, do you propose to return the error code via > rte_errno? Currenly no cmdline functions use it. This would also add a > runtime dependency on EAL (currently cmdline only depends on its headers). Good point, I was indeed thinking about NULL + rte_errno, but I did not anticipate the new dependency to eal. Given there's no errno in cmdline_new(), which is the main user API, I think we can do the same for rdline_new(). > > [...] > > > +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. > > rdline_get_history_buffer_size() is safe because sizeof() is evaluated at > compile time. There's a unit test checking that all functions are NULL-safe. Oh yes, of course, thanks.