On 11/18/2016 08:01 AM, Eric Farman wrote:
On 11/17/2016 06:18 PM, John Ferlan wrote:
[...]
I don't think the current code naming is incorrect, but it does slightly paint us into a box with this work. I'll mull this over overnight, and maybe cook up a cleanup patch separate from this series. Or perhaps take your other suggestion and go with the inclusion of "vhost" in the functions. John,
I sent an RFC patch [1] separate from this series the other day, but thought that I had the remainder of your comments addressed and so maybe I'd combine everything into one series. Then my brain exploded:
Before: // These three are all existing code virDomainHostdevSubsysSCSIPtr scsisrc = &def->source.subsys.u.scsi; virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host; virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &scsisrc->u.iscsi; // The next one is new virDomainHostdevSubsysHostPtr hostsrc = &def->source.subsys.u.host;
After: // These three are all existing code virDomainHostdevSubsysSCSIPtr scsisrc = &def->source.subsys.u.scsi; virDomainHostdevSubsysSCSISCSIHostPtr scsiscsihostsrc = &scsisrc->u.host; virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &scsisrc->u.iscsi; // The next one is new virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &def->source.subsys.u.scsihost;
So, uh, ugh. (And it still has an inconsistency because I had to prepend another "scsi" on the existing "scsihostsrc" ... which means there's probably a better reworking to have happen here.)
I could take your other suggestion of "SCSIHostVHost", but I still worry that that gets viewed as a subset of the existing SCSIHost stuff (which is a type='scsi' sourceadapter='scsi_host' hostdev), without somehow cleaning up the existing code.
For now, I've stashed these changes off to the side. I could spin a v4 of the vhost-scsi series without any of the s/host/scsihost/ variations you asked for, but this rabbit hole is probably going to consume me until the next freeze/holiday. Thoughts? I've been heads down in some vHBA/NPIV code - less than friendly stuff... It's a customer case, so it's taken priority...
No problem, those definitely win.
Quick thoughts -
... the RFC used SCSISCSI which really looked odd and I think I originally avoided for that very reason when I gone through a similar exercise some time ago.
Sorry for the deja vu then. :-)
... since the first issues that caught my attention were in patch4, maybe attack those first - that is change 'Host' to 'SCSCIVHost' API's
... for this patch, it may just be best to change HOST to VHOST and Host to VHost.
I have patches for these on top of the series (didn't want to rebase things too badly in case they went terribly wrong). The RFC made sense because it lined everything up, without introducing a "why isn't the type='scsi_vhost' then?" question. Seeing how it looks now, I'll toss those aside for the time being and see how things look with just these two. (Well, there are still some s/HOST/SCSIHOST/ involved; the enumerated list comes to mind.)
I'll try to put some more thought into it in the morning...
Again, I appreciate the time you've spent looking at this. Good luck in NPIV-land.
Heads down did good - those got posted today... So I spent some time working through my patch3 and patch4 comments. Rather than post to the list - I'll send directly to you and let you process the data. Essentially though I think I accomplished everything related *only to* name changes for patches 3 and 4 (well they're now 2 and 3). Anyway - I figured since I got you into the problem, I could help get you out at the very least. They'll be arriving in your inbox soon. John