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 C57F8A034F;
Wed, 24 Feb 2021 03:15:10 +0100 (CET)
Received: from [217.70.189.124] (localhost [127.0.0.1])
by mails.dpdk.org (Postfix) with ESMTP id 82FD9160788;
Wed, 24 Feb 2021 03:15:10 +0100 (CET)
Received: from mail-pf1-f172.google.com (mail-pf1-f172.google.com
[209.85.210.172])
by mails.dpdk.org (Postfix) with ESMTP id 24A564068C
for <dev@dpdk.org>; Tue, 23 Feb 2021 12:23:21 +0100 (CET)
Received: by mail-pf1-f172.google.com with SMTP id j12so8569253pfj.12
for <dev@dpdk.org>; Tue, 23 Feb 2021 03:23:21 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;
h=mime-version:references:in-reply-to:from:date:message-id:subject:to
:cc; bh=NjZJYwv/XQKWpdgSIteQOUp5KQRgGwLJf+zUjfAVM5Y=;
b=Vim+iEElVwANJY36Ys/HGiFFViWo3JZiEgy9LqyASSty6dPRHEmjZaU96pjWeMYihC
tzzFwFTQO/63TsVUxb0nbJf+4ZrgG0RTeZnJntuW6El4hbvzVthTnu1EZpVF5VZbokrc
rt1LbqxpDLHI9wofEd11cfvU+PoIyR01Un5wqpMV+OWnuDPpmpDOmsmXw8Nk+4bTwx5G
7K6SsG/V5LmEZeX4pMdkG0Lgp1/L9xBjbbLeLEb9DU7gHIUQ9cruYt4sfB57blAoyJSe
JsOOvR5+iesXfp/DRukGpycfkTSDrrUiNcADzaQYkyc9uURx1sbTjCMP+mx9Tb34Rlz6
ELTg==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20161025;
h=x-gm-message-state:mime-version:references:in-reply-to:from:date
:message-id:subject:to:cc;
bh=NjZJYwv/XQKWpdgSIteQOUp5KQRgGwLJf+zUjfAVM5Y=;
b=kYr059Tm6aWMgiIpByZJwzSAmVFlZlDD9S8UGVqDkFoNDn/0hjTbLYwvhjljcbi4Pl
l08W/DySNZ40aceCVYiwddL0K9NL2tEcyZwxlbV4VBuNMaKJbLAFifnyrp5a53iYbzV0
jvRH+nFppdYRSH+tngrH79ZKSEXq+IMNdra3mJE15phsY7cWQ/CW09RfjCXDjojKF6gt
AHptpEDZO4ysm3u0/udETtirqd4XcLJsJ9jozB6uFIn/ihjVNUFZF8jGGkFqdAn85F7b
UpgVR9o6UBqtK/P6HBks9O0FUJMPuem34cD4+ABF/eBOo6l3aikJ5UnByKGJbthUo29m
2b6w==
X-Gm-Message-State: AOAM530cqoZfFvEeqfxwdB8mdwTm+0NG5kYETXru6dhhnktUc/dPGWuT
9eEr3ykWDT7LhTb7WMxAGtNgCHnTpoT75tqjRdo=
X-Google-Smtp-Source: ABdhPJxG8JmcdLt5DA30oRcnbzch4LiCCioUkd4c+o521T5p38CqrZxHQDyrNVvwibO1doIyhNLSIOOIH0/7JbYoiKM=
X-Received: by 2002:aa7:8d0d:0:b029:1d7:3c52:e1f6 with SMTP id
j13-20020aa78d0d0000b02901d73c52e1f6mr26681294pfe.39.1614079399890; Tue, 23
Feb 2021 03:23:19 -0800 (PST)
MIME-Version: 1.0
References: <20210205112433.1681853-1-fengli@smartx.com>
<20210218061253.2812991-1-fengli@smartx.com>
<20210219111120.4e77de89@sovereign>
In-Reply-To: <20210219111120.4e77de89@sovereign>
From: Feng Li <lifeng1519@gmail.com>
Date: Tue, 23 Feb 2021 19:22:54 +0800
Message-ID: <CAEK8JBB3Ucdzw15QnrTs2P6z50myXjihCfFao1LUQ=x7ttGY6g@mail.gmail.com>
To: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
Cc: Li Feng <fengli@smartx.com>, David Marchand <david.marchand@redhat.com>,
Narcisa Ana Maria Vasile <navasile@linux.microsoft.com>,
Dmitry Malloy <dmitrym@microsoft.com>,
Pallavi Kadam <pallavi.kadam@intel.com>, Ray Kinsella <mdr@ashroe.eu>,
Neil Horman <nhorman@tuxdriver.com>, dev <dev@dpdk.org>
Content-Type: text/plain; charset="UTF-8"
X-Mailman-Approved-At: Wed, 24 Feb 2021 03:15:09 +0100
Subject: Re: [dpdk-dev] [PATCH v4] log: support custom log function
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 Dmitry,
Thanks for your comments. I will submit the next patch to address your concern.
On Fri, Feb 19, 2021 at 4:11 PM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> wrote:
>
> On Thu, 18 Feb 2021 14:12:53 +0800, Li Feng wrote:
> > By default, the dpdk log is out to stdout/stderr and syslog.
> > The rte_openlog_stream could set an external FILE* stream, but it
> > asks the consumer to give it a FILE* pointer.
> > For C++ or other languages, it's hard to get a libc FILE*.
> >
> > Support to set a hook is another choice for this scenario.
> >
> > Signed-off-by: Li Feng <fengli@smartx.com>
> > ---
> > v4: Fix the code style.
> > v3: Rename the func, change the comments, add funcs in version.map.
> > v2: Simplify the code.
>
> While I support the feature in general, its current design is imperfect both
> at interface level (documentation, corner cases) and at interaction with the
> rest of EAL logging subsystem (hijacking Linux EAL internals). IMO, proper
> implementation would rework rte_vlog() to print to buffer, then pass it to
> current logging function. But rte_openlog_stream() has to be supported still.
> What do others think?
OK, I have reworked on rte_vlog, it looks more reasonable.
>
> > +/**
> > + * Define a logging write function.
> > + */
> > +typedef ssize_t rte_log_write_function(void *cookie, const char *buf,
> > + size_t size);
>
> You're supposed to document callback parameters and behavior here.
> What about its thread-safety?
> It's not obvious what "cookie" means (suggesting "context").
Acked.
>
> > +
> > +/**
> > + * Change the default stream's write action that will be used by the logging
> > + * system.
> > + *
> > + * This should be done before the 'rte_eal_init' call. And the
> > + * 'rte_openlog_stream' call will override this action.
> > + *
> > + * @param logf
> > + * Pointer to the log write function.
> > + */
> > +__rte_experimental
> > +void
> > +rte_log_sink_set(rte_log_write_function *logf);
>
> Since this API is currently Linux-only, it must report lack of support for
> FreeBSD and Windows, presumably via return value.
Acked.
>
> > +
> > +/**
> > + * Retrieve the log function used by the logging system (see rte_log_sink_set()
> > + * to change it).
> > + *
> > + * @return
> > + * Pointer to the log function.
> > + */
> > +__rte_experimental
> > +rte_log_write_function*
> > +rte_log_sink_get(void);
>
> Doesn't checkpatch complain about "ret_type*" vs "ret_type *"?
Actually, checkpatch treats it ok.
>
> [...]
> > +/**
> > + * Change the default stream's write action that will be used by the logging
> > + * system.
> > + *
> > + * This should be done before the 'rte_eal_init' call. And the
> > + * 'rte_openlog_stream' call will override this action.
> > + */
>
> It's not very helpful to repeat public documnetation.
> Here is a good place to explain implementation, if required.
Acked.
>
> > +void
> > +rte_log_sink_set(rte_log_write_function *logf)
> > +{
> > + console_log_func.write = logf;
> > +}
>
> 1. What if NULL is passed? An app will likely crash far from the culprit.
> 2. This breaks EAL writing to syslog. It's probably intended, but please at
> least document such behavior.
OK, I think that if the logf is NULL, then keep the original function.
>
> [...]
> > +/*
> > + * Retrieve the default log write function.
> > + */
> > +rte_log_write_function*
> > +rte_log_sink_get(void)
> > +{
> > + return NULL;
> > +}
>
> I'd say it's an API inconsistency that you can write logs but sink is not
> callable.
I have rewritten this patch, it will be safe.