From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
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 <dev@dpdk.org>; Tue, 5 Oct 2021 11:03:02 +0200 (CEST)
Received: by mail-lf1-f48.google.com with SMTP id y15so17608449lfk.7
for <dev@dpdk.org>; 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 <dmitry.kozliuk@gmail.com>
To: Olivier Matz <olivier.matz@6wind.com>
Cc: dev@dpdk.org, David Marchand <david.marchand@redhat.com>, Ali Alnubani
<alialnu@nvidia.com>, Gregory Etelson <getelson@nvidia.com>, Ray Kinsella
<mdr@ashroe.eu>
Message-ID: <20211005120301.3c28e806@sovereign>
In-Reply-To: <YVwMXR2uz4yOGMGb@platinum>
References: <20210928194714.365563-1-dmitry.kozliuk@gmail.com>
<20211005005516.132377-1-dmitry.kozliuk@gmail.com>
<20211005005516.132377-3-dmitry.kozliuk@gmail.com>
<YVwMXR2uz4yOGMGb@platinum>
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 <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 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 <ctype.h>
> >
> > #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.