From: Joe Perches <joe@perches.com>
To: Gaston Gonzalez <gascoar@gmail.com>, linux-staging@lists.linux.dev
Cc: gregkh@linuxfoundation.org, nsaenz@kernel.org,
stefan.wahren@i2se.com, arnd@arndb.de, dan.carpenter@oracle.com,
ojaswin98@gmail.com, amarjargal16@gmail.com,
linux-rpi-kernel@lists.infradead.org,
linux-arm-kernel@lists.infradead.org,
bcm-kernel-feedback-list@broadcom.com,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 8/8] staging: vchiq_core: fix quoted strings split across lines
Date: Sun, 24 Oct 2021 16:56:55 -0700 [thread overview]
Message-ID: <a7d1136e22bdb37170a0fe9a9443ba0cdf5dcd32.camel@perches.com> (raw)
In-Reply-To: <20211024213839.370830-1-gascoar@gmail.com>
On Sun, 2021-10-24 at 18:38 -0300, Gaston Gonzalez wrote:
> Quoted strings should not be split across lines. As put it in [1]:
> "never break user-visible strings such as printk messages because that
> breaks the ability to grep for them."
>
> While at it, fix the alignment of the arguments in the sentence.
>
> Note: this introduce a checkpatch CHECK: line length of 123 exceeds 100
> columns, as the line now is:
>
> vchiq_loud_error("%d: service %d (%c%c%c%c) version mismatch - local (%d, min %d) vs. remote (%d, min %d)",
>
> But now the string is grep-able and the whole function call more
> clear.
IMO: All of these should be changed
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h:#define VCHIQ_LOG_PREFIX KERN_INFO "vchiq: "
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h-
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h-#ifndef vchiq_log_error
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h-#define vchiq_log_error(cat, fmt, ...) \
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h- do { if (cat >= VCHIQ_LOG_ERROR) \
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h: printk(VCHIQ_LOG_PREFIX fmt "\n", ##__VA_ARGS__); } while (0)
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h-#endif
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h-#ifndef vchiq_log_warning
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h-#define vchiq_log_warning(cat, fmt, ...) \
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h- do { if (cat >= VCHIQ_LOG_WARNING) \
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h: printk(VCHIQ_LOG_PREFIX fmt "\n", ##__VA_ARGS__); } while (0)
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h-#endif
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h-#ifndef vchiq_log_info
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h-#define vchiq_log_info(cat, fmt, ...) \
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h- do { if (cat >= VCHIQ_LOG_INFO) \
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h: printk(VCHIQ_LOG_PREFIX fmt "\n", ##__VA_ARGS__); } while (0)
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h-#endif
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h-#ifndef vchiq_log_trace
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h-#define vchiq_log_trace(cat, fmt, ...) \
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h- do { if (cat >= VCHIQ_LOG_TRACE) \
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h: printk(VCHIQ_LOG_PREFIX fmt "\n", ##__VA_ARGS__); } while (0)
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h-#endif
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h-
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h-#define vchiq_loud_error(...) \
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h- vchiq_log_error(vchiq_core_log_level, "===== " __VA_ARGS__)
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h-
I suggest using the rather more common vchiq_err, vchiq_warn, vchiq_info
and if necessary vchiq_trace. Also the cat >= test is unnecessary and
this should just use the more common standard logging facilities.
vchiq_loud_error is IMO unnecessary.
Also several of the uses of these macros already have '\n' terminations
so that just adds unnecessary blank lines in the logging.
prev parent reply other threads:[~2021-10-24 23:57 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20211024212844.370379-1-gascoar@gmail.com>
2021-10-24 21:38 ` Gaston Gonzalez
2021-10-24 23:56 ` Joe Perches [this message]
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=a7d1136e22bdb37170a0fe9a9443ba0cdf5dcd32.camel@perches.com \
--to=joe@perches.com \
--cc=amarjargal16@gmail.com \
--cc=arnd@arndb.de \
--cc=bcm-kernel-feedback-list@broadcom.com \
--cc=dan.carpenter@oracle.com \
--cc=gascoar@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rpi-kernel@lists.infradead.org \
--cc=linux-staging@lists.linux.dev \
--cc=nsaenz@kernel.org \
--cc=ojaswin98@gmail.com \
--cc=stefan.wahren@i2se.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
all inboxes | Powered by JetHome®