On 08/14/16 09:24, Joe Perches wrote:
> On Sun, 2016-08-14 at 14:34 +0000, Bart Van Assche wrote:
>> As one can see in be_main.h the "level" argument of macro beiscsi_log()
>> is ignored for log levels KERN_EMERG, KERN_ALERT, KERN_CRIT and
>> KERN_ERR. So for these log levels beiscsi_log() is a synonym of
>> shost_printk(). Have you considered to replace beiscsi_log() with
>> shost_printk() for these log levels and additionally to change
>> beiscsi_log() for the other log levels into pr_debug()? pr_debug()
>> statements namely already can be enabled and disabled at runtime. If the
>> BEISCSI_LOG_* log category would be embedded in the log text that would
>> allow to eliminate the phba->attr_log_enable structure member.
>> Additionally, pr_debug() has a facility for displaying the source file
>> name and the line number. That would allow to leave out __LINE__ from
>> be2iscsi log statements. I don't think it is useful to have that line
>> number in non-debug be2iscsi log statements.
>
> My main consideration for submitting a patch at all
> was removing the apparent format/argument mismatches.
>
> As far as I can grep, only KERN_ERR, KERN_WARNING and
> KERN_INFO are actually used by be2iscsi today.
>
> I agree with the removal of __LINE__ from the macros
> as its utility is generally pretty low.
>
> Besides, using stringify(__LINE__) is almost always
> smaller object code than a format with "%d", __LINE__.
>
> Prefixes like "BC" and "BS" are __FILE__ equivalents,
> and could be removed as well with something like
> "%s, kbasename(__FILE__)" used if _really_ desired.
>
> I have no issue with defining and using beiscsi_<level>
> equivalents to shost_printks.
>
> I think the test inside beiscsi_log is better removed
> with multiple specific beiscsi_<level> calls used.
>
> I don't know why any KERN_ERR should ever be masked,
> but perhaps something like:
>
> #define beiscsi_printk(level, phba, mask, fmt, ...) \
> do { \
> if ((mask) & (phba)->attr_log_enable) \
> shost_printk(level, phba->shost, fmt, ##__VA_ARGS__); \
> } while (0)
>
> #define beiscsi_err(phba, mask, fmt, ...) \
> beiscsi_printk(KERN_ERR, phba, mask, fmt, ##__VA_ARGS__)
> #define beiscsi_warn(phba, mask, fmt, ...) \
> beiscsi_printk(KERN_WARNING, phba, mask, fmt, ##__VA_ARGS__)
> #define beiscsi_info(phba, mask, fmt, ...) \
> beiscsi_printk(KERN_INFO, phba, mask, fmt, ##__VA_ARGS__)
>
> with a sed of the .c files:
>
> $ sed -i 's/beiscsi_log(phba, KERN_ERR/beiscsi_err(phba/g' drivers/scsi/be2iscsi/*.c
> $ sed -i 's/beiscsi_log(phba, KERN_WARNING/beiscsi_warn(phba/g' drivers/scsi/be2iscsi/*.c
> $ sed -i 's/beiscsi_log(phba, KERN_INFO/beiscsi_info(phba/g' drivers/scsi/be2iscsi/*.c
>
> with argument realignment of those lines.
>
> All of these are of course up to the actual maintainers of be2iscsi.
Hello Joe,
My primary concern is how to enable and disable log messages from user
space. Many drivers define their own logging macros and export a bitmask
that allows to enable and disable logging messages per category. These
bitmask control mechanisms are annoying because figuring out what bit
controls which message category requires a search through the driver
source code. I'd like to see all these custom logging macros disappear
and being replaced by a single mechanism. The "dynamic debug" mechanism
e.g. is in my opinion much easier to use than the different custom
logging mechanisms.
Bart.
Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
