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 83AABA0C47; Tue, 5 Oct 2021 11:03:03 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 49E0A412C7; Tue, 5 Oct 2021 11:03:03 +0200 (CEST) Received: from mail-lf1-f48.google.com (mail-lf1-f48.google.com [209.85.167.48]) by mails.dpdk.org (Postfix) with ESMTP id BD46E412A6 for ; Tue, 5 Oct 2021 11:03:02 +0200 (CEST) Received: by mail-lf1-f48.google.com with SMTP id y15so17608449lfk.7 for ; Tue, 05 Oct 2021 02:03:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=4qVi9RhDIlqzaq9rS+w58DrqHBgHs7Pypm54iPPhk2Q=; b=YJ8IXsnMerb6iTdU+MutR8bAMKDbEECQ0vPmkSSOUU8CDKBYN/G6k00oAVWRQ4U6b0 17SpKaN7MveZBcZG2TSy48bJDJTyZJFQO+kZfjJqoSApMT4lSQj6Pfb+CamNLYWD6+Np CV6MSS7yAQGzS1oSO+5MnVv8tpSkb8Xx0/DWCaFVtQzwnnLXO16eOYL9mztB+MRhzVqg KF0swfR1gITMxXsQVObjQ3osjrddWdYrJUvUEhjkBqora+ylTw7fZ15P2NcBHvE5Xn0T 1k4wMX6BvR2dYkbzNItwA5Xejz4SP16P7roQgDykWDc2d8HA1ZJXOZwg99adlRvUMLJF wZwQ== 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:in-reply-to :references:mime-version:content-transfer-encoding; bh=4qVi9RhDIlqzaq9rS+w58DrqHBgHs7Pypm54iPPhk2Q=; b=c8DSIgw9F6GiHkXH6Dr6mHWhWK4Vzl7wci0Yw02vMHzy/eEgN4BHZafg6KcBUiOVWl NGUgHGDL1W/NrwQKafdM8W6tnApr2SlUgNSSpJsXarLIhQOz4urAS7JQGPhEEg8WrrA5 MaUBWiOOkeDlvkf2hLMuiJX+ThKE4JUGj7OY2aGbp5zvSuBS4cdkohMtIQ3YR9t5lUJ1 QYRPmXCiMeMXz8uWc1W7dqivfTynU7sK5AXqNEMxFmbpUA8GuSjBXEqDYGkMRjFbfHTo wjSKJzwKfBEAoFdIdSRq8oJn9ryGAUlRi3RNJ7//4EXP+TJB6rRnbOYu5JGYHiMOfjWr /xlw== X-Gm-Message-State: AOAM530m2AOK31uPD3F0gUfzStpQLRm/p7iUJ+y4I2Bo4JM3b/NozX5y DZUrhGZZ28tG09mE8nEuLIo= X-Google-Smtp-Source: ABdhPJxrx/Z2SAORZYcVCXrVORMl4vtkls6PyMQ0zErgJ2u2MGfLOaGrmLxI3AlOKE/W8DwqWZomVA== X-Received: by 2002:a05:6512:750:: with SMTP id c16mr2370389lfs.21.1633424582187; Tue, 05 Oct 2021 02:03:02 -0700 (PDT) Received: from sovereign (broadband-37-110-65-23.ip.moscow.rt.ru. [37.110.65.23]) by smtp.gmail.com with ESMTPSA id h4sm79741lfk.193.2021.10.05.02.03.01 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 05 Oct 2021 02:03:01 -0700 (PDT) Date: Tue, 5 Oct 2021 12:03:01 +0300 From: Dmitry Kozlyuk To: Olivier Matz Cc: dev@dpdk.org, David Marchand , Ali Alnubani , Gregory Etelson , Ray Kinsella Message-ID: <20211005120301.3c28e806@sovereign> In-Reply-To: References: <20210928194714.365563-1-dmitry.kozliuk@gmail.com> <20211005005516.132377-1-dmitry.kozliuk@gmail.com> <20211005005516.132377-3-dmitry.kozliuk@gmail.com> X-Mailer: Claws Mail 3.18.0 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit 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 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). > [...] > > +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.