Boaz Harrosh wrote:
> Mike Christie wrote:
>> Boaz Harrosh wrote:
>>> Remove the dark ages /* define debug_print */ in code, to use
>>> a Kconfig option. With a system like Kconfig, in code, commented out,
>>> configuration options are slavery and hard work.
>>> (version control, manual edit ... need I say more)
>>>
>>> I've used an "int" config bit-mask so more areas of code can be
>>> selected with one Koption, but mainly so that allmodconfig will
>>> not turn it on.
>>>
>>> bit-1 - will turn on prints for libiscsi.
>>> bit-2 - will turn on prints for libiscsi_tcp & iscsi_tcp.
>>>
>>> More iscsi drivers should use more bits.
>>>
>> I made this a per module compile time option. I also modified the macors
>> so they added a sessionX or connectionX:Y prefix.
>
> Nice! in Israel when we want to say that something is highly desirable, we
> say "America"
>
> One small nitpicking comment, we might like to change this code to not
> compile at all if CONFIG_DEBUG_KERNEL is not defined something like:
>
> static unsigned int iscsi_max_lun = 512;
> module_param_named(max_lun, iscsi_max_lun, uint, S_IRUGO);
>
> + #ifdef CONFIG_DEBUG_KERNEL
> +static int iscsi_sw_tcp_dbg;
> +module_param_named(debug_iscsi_tcp, iscsi_sw_tcp_dbg, int,
> + S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(debug_iscsi_tcp, "Turn on debugging for iscsi_tcp module "
> + "Set to 1 to turn on, and zero to turn off. Default is off.");
> +
> + #else
> + static const int iscsi_sw_tcp_dbg = 0;
> + #endif /*CONFIG_DEBUG_KERNEL*/
> +#define ISCSI_SW_TCP_DBG(_conn, dbg_fmt, arg...) \
> ...
>
> So the optimizer will discard all this code. Just as we have today.
> (The nice thing is that the code still gets compiled only not included in the
> output)
>
> What ever you decide I don't have any strong feelings about it.
Ok will look into. I have to fix up the patch due to your comments below.
>
> Also
>> diff --git a/include/scsi/libiscsi_tcp.h b/include/scsi/libiscsi_tcp.h
>> index 9e3182e..c11b2e1 100644
>> --- a/include/scsi/libiscsi_tcp.h
>> +++ b/include/scsi/libiscsi_tcp.h
>> @@ -91,6 +91,16 @@ enum {
>> ISCSI_TCP_SUSPENDED, /* conn is suspended */
>> };
>>
>> +
>> +#define ISCSI_DBG_TCP(_conn, dbg_fmt, arg...) \
>> + do { \
>> + if (iscsi_dbg_libtcp) \
>> + iscsi_conn_printk(KERN_INFO, _conn, \
>> + "%s " dbg_fmt, \
>> + __func__, ##arg); \
>> + } while (0);
>> +
>> +
>
> Are you sure you need this exported in header? It looks like it is only used
> in libiscsi_tcp.c? iscsi_tcp.c has it's own parameter.
>
> But if you keep it at header I think it is best for future to also have
> an:
> +extern int iscsi_dbg_libtcp;
> like in libiscsi.h.
>
> BTW same comment for ISCSI_DBG_SESSION/ISCSI_DBG_CONN are they used outside
> libiscsi.c ?
>
>> extern void iscsi_tcp_hdr_recv_prep(struct iscsi_tcp_conn *tcp_conn);
>> extern int iscsi_tcp_recv_skb(struct iscsi_conn *conn, struct sk_buff *skb,
>> unsigned int offset, bool offloaded, int *status);
Ah yeah you are right. I had done this originally because I thought the
LLD (cxgb3i, bnx2i, iscsi_tcp, iser) could use the debug helpers. But
then I thought it became complicated for them because they do not know
when to use the iscsi ones and when to use theirs. So for now I will
punt on it and let the LLDs use their own.
>> --
>> 1.6.0.6
>
> Thanks mike this is very nice I added this to my test scripts and it is very
> easy to turn on prints for debugging when problems happen.
>
> if using module_param_named() does that means we can change that in runtime?
> how?
>
Yeah,
echo > /sys/module/iscsi module name/parameters/name of param
> Boaz
>
> >
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"open-iscsi" group.
To post to this group, send email to open-iscsi@googlegroups.com
To unsubscribe from this group, send email to
open-iscsi+unsubscr...@googlegroups.com
For more options, visit this group at http://groups.google.com/group/open-iscsi
-~----------~----~----~----~------~----~------~--~---
