On Wed, 2013-06-19 at 13:42 -0400, Ewan D. Milne wrote: > From: "Ewan D. Milne" <emilne@xxxxxxxxxx> > > 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 -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
