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 C7ABEA054A; Fri, 19 Feb 2021 09:11:24 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 74E6940395; Fri, 19 Feb 2021 09:11:24 +0100 (CET) Received: from mail-lj1-f175.google.com (mail-lj1-f175.google.com [209.85.208.175]) by mails.dpdk.org (Postfix) with ESMTP id 1222540042 for ; Fri, 19 Feb 2021 09:11:23 +0100 (CET) Received: by mail-lj1-f175.google.com with SMTP id q14so15770764ljp.4 for ; Fri, 19 Feb 2021 00:11:23 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=apPhhm9FuoyVVLuFEhR/BRj0aBR6sWgWWQ4c9ESi8Bo=; b=irYeTk0RmoClIscgstAUf5JPq1ZFoCv80eth47qfU1HBpf/0g9TfGhO9JVKmbOllBD 7ZxjxGSjDgGqe2lcl/iIldVwtxUr1WM+bk4i5Jxq1IscA4TeBnzCfVC27miuDCfSTNzm ri8eFTiT1E3qRvVxBAaVoNsB3N680VCqlj7skWIEeiYoerB4MVN5DQ45uK1YIQXGU9LQ AKEIoSrVcehEdSSnoGMTxwwR2e3d/UvAQuUpy/FoS5hboLclwT8xei8rA1YGGM+Xwqj4 KSEUSMWaQiE9Fnnt0rkqfA4tC2pPzGY3VYGyt9NG9/vT4mTuP5qpkhCphBim/y5O7SX0 kSZA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=apPhhm9FuoyVVLuFEhR/BRj0aBR6sWgWWQ4c9ESi8Bo=; b=GGu0B+CdfcKHs7XO4OsYDuXtvHYz6JKQwSd1PF6+I2ZE0m+211QIUK3SMTrGn6WOXx cxiQZz0MWnQI43qM1eNITTgP3ZJHs+kxZNkg/BDaK4jrlAdriSCxv4crfh3vu+qeB8oe CE6l/2YoLiGsvwFst1UZOs/6zuaQU2nF/Y6AOkAXd4uH2uPKVQpKuWtGDIHGJLd13w1A uX1+zHGNuNhBvVE3gCQ0OGMBOt2H/70msiMbOGMuDfrN8+ymZk/cP6T9WdDyHFum1um4 MhUdR1utvih0j0LJ5iGZml6m6oF6sCVnbsTlTVUYuBhH/TJMUYL8use00KiX6o+o3FyP t5RA== X-Gm-Message-State: AOAM531pQR7sPXKJuvcGYMAg8yvhP1/Mip9/dwJtIWD8FU7Cnd/e5fUw D6xhRYPBvQzdlZopCKMpL4I= X-Google-Smtp-Source: ABdhPJyINQyFQ/CDPCP+HblMaoKw9Kc6swvTVTNKtsJAaAGJf3QWP5MZICesvtX6bESd+c6sxccCHw== X-Received: by 2002:a19:e41:: with SMTP id 62mr4949464lfo.128.1613722282576; Fri, 19 Feb 2021 00:11:22 -0800 (PST) Received: from sovereign (broadband-37-110-65-23.ip.moscow.rt.ru. [37.110.65.23]) by smtp.gmail.com with ESMTPSA id c16sm854558lfb.36.2021.02.19.00.11.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 19 Feb 2021 00:11:21 -0800 (PST) Date: Fri, 19 Feb 2021 11:11:20 +0300 From: Dmitry Kozlyuk To: Li Feng Cc: David Marchand , Narcisa Ana Maria Vasile , Dmitry Malloy , Pallavi Kadam , Ray Kinsella , Neil Horman , dev@dpdk.org, lifeng1519@gmail.com Message-ID: <20210219111120.4e77de89@sovereign> In-Reply-To: <20210218061253.2812991-1-fengli@smartx.com> References: <20210205112433.1681853-1-fengli@smartx.com> <20210218061253.2812991-1-fengli@smartx.com> X-Mailer: Claws Mail 3.17.6 (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 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > --- > 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? > +/** > + * 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"). > + > +/** > + * 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. > + > +/** > + * 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 *"? [...] > +/** > + * 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. > +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. [...] > +/* > + * 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.