DPDK patches and discussions
 help / color / mirror / Atom feed
From: Feng Li <lifeng1519@gmail.com>
To: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
Cc: Li Feng <fengli@smartx.com>, dev <dev@dpdk.org>,
	 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>
Subject: Re: [dpdk-dev] [PATCH v2] log: support custom log function
Date: Mon, 8 Feb 2021 14:58:29 +0800	[thread overview]
Message-ID: <CAEK8JBBEYJN-iatFUcg-TAJ8AqosLP1DeS3f61Ek30=+H4DA2w@mail.gmail.com> (raw)
In-Reply-To: <20210205223207.7fbf82ba@sovereign>

Thanks for your comments.

Dmitry Kozlyuk <dmitry.kozliuk@gmail.com> 于2021年2月6日周六 上午3:32写道:
>
> On Sat,  6 Feb 2021 01:42:04 +0800, Li Feng wrote:
> > Currently, 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 method is another choice for this scenario.
>
> It's "DPDK"; and "hard" sounds vague. I'd state the real issue such that
> API to change DPDK log sink mandates the (direct) use of libc in consumer
> application. To invoke arbitrary function, consumers have to use facilities
> that are non-standard (fopencookie) or OS-specific (pipes).
>
> >
> > Change-Id: I3b2419cc2fe5256205daa8077fd8862a8e58fb6c
>
> What is this?
>
It's Gerrit's Change-ID, I will remove it.

> > Signed-off-by: Li Feng <fengli@smartx.com>
> > ---
> > v2: simplify the code.
> >
> >  lib/librte_eal/include/rte_eal.h | 16 ++++++++++++++++
> >  lib/librte_eal/linux/eal_log.c   | 10 ++++++++++
> >  2 files changed, 26 insertions(+)
> >
> > diff --git a/lib/librte_eal/include/rte_eal.h b/lib/librte_eal/include/rte_eal.h
> > index eaf6469e5..bd6cf554b 100644
> > --- a/lib/librte_eal/include/rte_eal.h
> > +++ b/lib/librte_eal/include/rte_eal.h
> > @@ -501,6 +501,22 @@ rte_eal_mbuf_user_pool_ops(void);
> >  const char *
> >  rte_eal_get_runtime_dir(void);
> >
> > +/**
> > + * Usage function typedef used by the application usage function.
>
> It's unrelated to the following typedef purpose, is it?
It's borrowed from the front typedef sentence.

>
> > + *
> > + * Use this function typedef to define a logger formatter.
> > + */
> > +typedef cookie_write_function_t rte_log_func_t;
>
> "cookie_write_function_t" is not standard C, please write this type
> explicitly. POSIX reserves "_t" suffix, "rte_" prefix is enough.
Fix to this?
typedef cookie_write_function_t rte_cookie_write_function;

>
> > +
> > +/**
> > + * Set a customized logger.
> > + *
> > + * @param logf
> > + *   The customized logger function.
> > + */
> > +void
> > +rte_eal_set_log_func(rte_log_func_t *logf);
> > +
>
> How about rte_log_sink_set() and a companion rte_log_sink_get() or returning
> the previous value when setting a new one? Also, like in fopencookie(), a
> cookie argument is needed.

void rte_log_sink_set(rte_cookie_write_function* cookie_write);
rte_cookie_write_function* rte_log_sink_get();
Right?

>
> New functions must be marker __rte_experimental and added to .map and .def
> files. Please use devtools/checkpatches.sh that will run checks like that for
> your commits.
>
I have run the `devtools/checkpatches.sh`, it reports valid, so weird.
I'll fix it.


> >  #ifdef __cplusplus
> >  }
> >  #endif
> > diff --git a/lib/librte_eal/linux/eal_log.c b/lib/librte_eal/linux/eal_log.c
> > index 43c8460bf..c0a7a12ab 100644
> > --- a/lib/librte_eal/linux/eal_log.c
> > +++ b/lib/librte_eal/linux/eal_log.c
> > @@ -60,3 +60,13 @@ rte_eal_log_init(const char *id, int facility)
> >
> >       return 0;
> >  }
> > +
> > +/*
> > + * Set the customized logger, it will override the default action, which is
> > + * writing to syslog and stdout.
> > + */
> > +void
> > +rte_eal_set_log_func(rte_log_func_t *logf)
> > +{
> > +    console_log_func.write = logf;
> > +}
>

  reply	other threads:[~2021-02-08 14:09 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-05 11:24 [dpdk-dev] [PATCH] " Li Feng
2021-02-05 11:31 ` Feng Li
2021-02-05 11:55 ` David Marchand
2021-02-05 12:22   ` Feng Li
2021-02-05 14:08     ` Dmitry Kozlyuk
2021-02-05 16:39     ` Stephen Hemminger
2021-02-05 16:10   ` Stephen Hemminger
2021-02-05 16:54     ` Feng Li
2021-02-05 17:06       ` Feng Li
2021-02-05 17:42 ` [dpdk-dev] [PATCH v2] " Li Feng
2021-02-05 17:47   ` Feng Li
2021-02-05 19:32   ` Dmitry Kozlyuk
2021-02-08  6:58     ` Feng Li [this message]
2021-02-08 22:40       ` Dmitry Kozlyuk
2021-02-10  3:59         ` Feng Li
2021-02-10  5:20 ` [dpdk-dev] [PATCH v3] " Li Feng
2021-02-18  2:55   ` Feng Li
2023-06-15 16:12   ` Stephen Hemminger
2021-02-18  6:12 ` [dpdk-dev] [PATCH v4] " Li Feng
2021-02-19  8:11   ` Dmitry Kozlyuk
2021-02-23 11:22     ` Feng Li

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAEK8JBBEYJN-iatFUcg-TAJ8AqosLP1DeS3f61Ek30=+H4DA2w@mail.gmail.com' \
    --to=lifeng1519@gmail.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=dmitry.kozliuk@gmail.com \
    --cc=dmitrym@microsoft.com \
    --cc=fengli@smartx.com \
    --cc=navasile@linux.microsoft.com \
    --cc=pallavi.kadam@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).