public inbox for linux-scsi@vger.kernel.org
 help / color / mirror / Atom feed
From: James Bottomley <jbottomley@parallels.com>
To: "Ewan D. Milne" <emilne@redhat.com>
Cc: "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
	"rwheeler@redhat.com" <rwheeler@redhat.com>,
	"dm-devel@redhat.com" <dm-devel@redhat.com>
Subject: Re: [PATCH v3 2/6] [SCSI] Rename scsi_evt_xxx to sdev_evt_xxx and scsi_event to sdev_event
Date: Wed, 19 Jun 2013 18:36:11 +0000	[thread overview]
Message-ID: <1371666971.2409.74.camel@dabdike> (raw)
In-Reply-To: <1371663761-22481-3-git-send-email-emilne@redhat.com>

On Wed, 2013-06-19 at 13:42 -0400, Ewan D. Milne wrote:
> From: "Ewan D. Milne" <emilne@redhat.com>
> 
> The names of the struct and some of the functions for scsi_device
> events are too generic and do not match the comments in the source.
> Changed all of the names to begin with sdev_ in order to avoid
> naming issues and confusion with scsi_target events to be added.
> Also changed name of sdev_evt_thread() to sdev_evt_work().

I don't really understand the rationale here.  Our usual namespace
prefix is scsi_ although we don't obey it universally, of course.  sdev_
isn't one of our namespace prefixes.  we might use scsi_device_ instead,
but I really don't see the need.  Plus all the name changes makes code
really difficult to review.

The two different event structures you introduce are actually identical
except for the values of the enums, so I don't see a need to have them
as separate.  Since the event hangs off a list in the scsi_device, it
can do the same for a scsi_target.  Events actually fire on generic
devices, so that's probably where we should start: change scsi_evt_emit
to take a generic device.  Then you can actually do a static translation
array between the enumerated event and the environment string.  I think
this unification will drastically reduce the number of lines in this
patch, since most of the infrastructure is now reused instead of being
duplicated.

The only other design point I'd add is that we probably need an internal
way to listen for events (I can see dm wanting this), so probably the
scsi_evt_emit should also send the event down a notifier chain, since
kernel internal stuff can't listen for a kobject uevent.  I cc'd the
dm-devel list to see what their opinion is of this.

For them, you should probably also summarise what events we're actually
proposing to send

scsi_target: 

REPORTED LUNS DATA HAS CHANGED

scsi_device:

MODE PARAMETERS CHANGED
CAPACITY DATA HAS CHANGED
THIN PROVISIONING SOFT THRESHOLD REACHED

James


  reply	other threads:[~2013-06-19 18:36 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-06-19 17:42 [PATCH v3 0/6] [SCSI] Enhanced sense and Unit Attention handling Ewan D. Milne
2013-06-19 17:42 ` [PATCH v3 1/6] [SCSI] Add a kernel config option for enhanced Unit Attention support Ewan D. Milne
2013-06-19 18:35   ` James Bottomley
2013-06-19 18:52     ` Ewan Milne
2013-06-19 17:42 ` [PATCH v3 2/6] [SCSI] Rename scsi_evt_xxx to sdev_evt_xxx and scsi_event to sdev_event Ewan D. Milne
2013-06-19 18:36   ` James Bottomley [this message]
2013-06-24 13:49     ` Ewan Milne
2013-06-19 17:42 ` [PATCH v3 3/6] [SCSI] Add support for scsi_target events Ewan D. Milne
2013-06-19 17:54   ` Bart Van Assche
2013-06-19 18:49     ` Ewan Milne
2013-06-19 17:42 ` [PATCH v3 4/6] [SCSI] Generate uevents for certain Unit Attention codes Ewan D. Milne
2013-06-19 18:48   ` James Bottomley
2013-06-24 14:11     ` Ewan Milne
2013-06-24 14:58       ` James Bottomley
2013-06-27 15:37         ` Ewan Milne
2013-06-20  8:35   ` Hannes Reinecke
2013-06-19 17:42 ` [PATCH v3 5/6] [SCSI] Add sysfs support for enhanced Unit Attention handling Ewan D. Milne
2013-06-19 17:42 ` [PATCH v3 6/6] [SCSI] Add sense and Unit Attention generation to scsi_debug Ewan D. Milne
2013-07-22 15:31 ` [PATCH v3 0/6] [SCSI] Enhanced sense and Unit Attention handling James Bottomley
2013-07-22 21:13   ` Ewan Milne

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=1371666971.2409.74.camel@dabdike \
    --to=jbottomley@parallels.com \
    --cc=dm-devel@redhat.com \
    --cc=emilne@redhat.com \
    --cc=linux-scsi@vger.kernel.org \
    --cc=rwheeler@redhat.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