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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list