* tcm_fc comments
@ 2011-05-27 10:24 Christoph Hellwig
2011-05-27 21:24 ` Nicholas A. Bellinger
0 siblings, 1 reply; 2+ messages in thread
From: Christoph Hellwig @ 2011-05-27 10:24 UTC (permalink / raw)
To: kiran.patil, nab; +Cc: linux-scsi
- please remove all the extra -I lines in EXTRA_CLFAGs, none of them
should be needed.
- CONFIGFS_TCM_FC_DEBUG should be used directly in the code instead of
definining another symbol
- there's no need for thread_wq, you can just do a wake_up_process
on the task_struct for the thread. That also means you don't have
the races with the kthread API you currently have
- The set_user_nice in ft_thread needs a comment explaning why you
really need to mess with priorities here. I suspect you don't have
to anyway.
- There's lots of code like "if (!(acl)) {" with completley superflous
braces.
In general it would have been nice to have sent a review request to
linux-scsi, especially as most of these items have been commented on
multiple times for other target frontends before.
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: tcm_fc comments
2011-05-27 10:24 tcm_fc comments Christoph Hellwig
@ 2011-05-27 21:24 ` Nicholas A. Bellinger
0 siblings, 0 replies; 2+ messages in thread
From: Nicholas A. Bellinger @ 2011-05-27 21:24 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: kiran.patil, linux-scsi
On Fri, 2011-05-27 at 06:24 -0400, Christoph Hellwig wrote:
> - please remove all the extra -I lines in EXTRA_CLFAGs, none of them
> should be needed.
Dropped
> - CONFIGFS_TCM_FC_DEBUG should be used directly in the code instead of
> definining another symbol
Dropped as this is unused
> - there's no need for thread_wq, you can just do a wake_up_process
> on the task_struct for the thread. That also means you don't have
> the races with the kthread API you currently have
Converted to use wake_up_process() in ft_queue_cmd() and
schedule_timeout_interruptible() in ft_thread()
> - The set_user_nice in ft_thread needs a comment explaning why you
> really need to mess with priorities here. I suspect you don't have
> to anyway.
Dropped
> - There's lots of code like "if (!(acl)) {" with completley superflous
>
>
Mmmm, dropping the a handful of if (!(foo)) usages on tfc_conf.c, but I
don't see the rest of the code with superflous braces..?
> In general it would have been nice to have sent a review request to
> linux-scsi, especially as most of these items have been commented on
> multiple times for other target frontends before.
Yes. So aside from these items, there is one TMR LUN_RESET bugfix patch
that was not included in the initial merge in scsi-misc.
So fixed in lio-core-2.6.git, and sending out the patches shortly.
Thanks,
--nab
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2011-05-27 21:32 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-05-27 10:24 tcm_fc comments Christoph Hellwig
2011-05-27 21:24 ` Nicholas A. Bellinger
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.