[libvirt] [PATCH v3 03/10] Introduce a "scsi_host" hostdev type

[John Ferlan]

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