[libvirt] libvirt API/design questions

[Cole Robinson]

On 12/12/19 6:13 AM, Daniel P. Berrangé wrote:
> On Wed, Dec 11, 2019 at 09:45:07AM -0500, Cole Robinson wrote:
>> There's some pre-existing libvirt design questions I would like some
>> feedback on, and one new one
>>
>>
>> * `virsh blockresize` doesn't prevent shrink by default, couple users
>> have blown their foot off and there's attempts at patches.
>> https://www.redhat.com/archives/libvir-list/2019-October/msg00258.html
>> https://www.redhat.com/archives/libvir-list/2019-November/msg00843.html
>>
>> Do we implement this as a protection in virsh, or change the API
>> behavior and require a new API flag to allow shrinking? Or some other idea?
> 
> Clearly we should have had protection when first implementing this.
> We can't change the default API behaviour now as that would break
> existing valid usage.
> 
> I think it is reasonable to change virsh though to try to add
> protection in some way.
> 
>> * vhost-user-scsi and vhost-user-blk XML schema
>> https://www.redhat.com/archives/libvirt-users/2019-October/msg00018.html
>>
>> Last proposal was using <hostdev> for this, which revisiting it now
>> makes more sense than <disk>, because it vhost-user-X doesn't seem to
>> use qemu's block layer at all. So vhost-user-scsi would be:
>>
>>     <hostdev mode='subsystem' type='scsi_host'>
>>       <source protocol='vhostuser' type='unix'
>> path='/path/to/vhost-user-scsi.sock' mode='client'/>
>>     </hostdev>
>>
>> vhost-user-blk maybe requires a new type='block' ? Otherwise it's
>> unclear how to differentiate between the two. Regardless it would be
>> nice to give the original patch author guidance here, they seemed
>> motivated to get this working
> 
> This is a really tricky question in general. It is basically saying
> whether we consider <disk> to refer to the guest visible device
> type or the QEMU visible backend type.
> 
> Originally these were always the same thing, but offloading to
> vhostuser has blurred the boundaries. I think in non-QEMU hypervisors
> where you don't have a split of frontend&backend in the config, you'd
> just have disks no matter what.
> 
> With network we've continued to use <interface> with vhost-user.
> 
> This makes me biased towards <disk>, at least for vhost-user-blk.
> 

Okay, makes sense to me.

> I presume that with vhost-user-scsi we're exposing the entire
> SCSI controller, so we'd need a <controller>. As you show
> above we do have use of <hostdev> already for scsi_host
> but that's for something that's known to the kernel. We
> can reasonably argue that vhost-uuser-scsi is not the same
> as scsi_host as its still emulated.
> 
> I think we should bear in mind that using vhost-user-blk/scsi
> does  *not* imply that QEMU's block layer is not there. The
> backend vhost-user process could be implemented in QEMU
> codebase & thus have a QMP monitor and full QEMU block backend
> featureset.  This isn't the case today, but it is at least
> conceivable for the future. This would bias towards <disk>
> at least for vhostuser-blk.  vhost-user-scsi is more difficult
> still.
> 

The downside of using <controller> for the scsi case is that it will
complicate libvirt SCSI address handling. Really in practical terms, a
libvirt controller is something you can assign other device <address>
onto. With vhost-user-scsi that won't be the case at first. So if we use
<controller type='scsi' model='vhostuser'/> or similar it's going to
make things a headache internally and possibly mess up bad app
assumptions. I guess it could be type='vhostuserscsi' too, to signify
it's entirely different.

So maybe <hostdev> is the simpler path forward in that case even though
it's not a clean conceptual fit their either.

>> * Splitting domain_conf up. There's 30k lines in domain_conf.c and 3500
>> in domain_conf.h, it would be nice to unwind it a bit. Getting some sign
>> off on this ahead of time is critical IMO so pieces can be posted and
>> committed quickly because they will obviously go out of date fast. My
>> thoughts:
>>
>> - domain_def.[ch]: DomainDefXXX and enum definitions.
>>   - probably New and Free functions too
>> - domain_parse.[ch]: XML parsing
>> - domain_format.[ch]: XML formatting
>> - domain_validate.[ch]: validate and postparse handling
>> - domain_util.[ch]: everything else, or keep it in domain_conf?
> 
> FWIW, if we're introducing new files, I'd like to see is move
> to the new naming based on object type.
> 
> ie    virdomaindef.[ch],  virdomainobj.[ch]
> 
> if virdomaindef.[ch] are too big, then virdomaindef{validate,util}.[ch]
> etc.  I'd keep the base file name as purely the XML parse/format code,
> and move any helper / addon logic out.
> 

Makes sense, though I think it would be helpful and an easy first step
to move the structs and enums to their own file. So maybe:

virdomaindef.[ch]: structs, enums, maybe New and Free functions
virdomaindefxml.[ch]: XML handling, parse + format
maybe validate and/or util like you suggest.

>> domain_def should be easy but no idea how intertwined the rest are. If
>> the domain_validate naming is acceptable for both validate and postparse
>> functions, we could use that naming for qemu_domain.c split too.
>>
>> Also maybe it's worth considering if there's some way to sensibly split
>> the conf/ directory. We could add a top level domain/ directory, but
>> that's kinda ambiguously named as we already have network/ + storage/ +
>> secret/ etc that have different meanings. Maybe sub dirs like
>> conf/domain/ ? I'm curious if anyone has strong feelings either way.
>> There's not really a clear place to dump shared DomainDef code at the
>> moment, like possible domain_cgroup for sharing cgroup handling across
>> lxc + qemu
> 
> I don't feel we need to split up the conf/ directory at least for the
> core XML handling. We are missing somewhere to put common helper
> logic that is stuff that isn't directly XML parsing/formatting. Stuff
> that starts out in a virt driver & then gets factored out later.
> 
> Perhaps we could have a 'src/hypervisor'  directly for common
> stuff like cgroups, namespaces, etc, since it is basically all
> related to virt drivers.

Good idea.

Thanks,
Cole