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 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 ; Tue, 23 Feb 2021 12:23:21 +0100 (CET) Received: by mail-pf1-f172.google.com with SMTP id j12so8569253pfj.12 for ; 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 Date: Tue, 23 Feb 2021 19:22:54 +0800 Message-ID: To: Dmitry Kozlyuk Cc: Li Feng , David Marchand , Narcisa Ana Maria Vasile , Dmitry Malloy , Pallavi Kadam , Ray Kinsella , Neil Horman , dev 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 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 > > --- > > 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.