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

On 12/12/19 9:57 AM, Ján Tomko wrote:

On Wed, Dec 11, 2019 at 09:45:07AM -0500, Cole Robinson wrote:

...

* 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.

Oh yes, so fast I did not even keep the branch for this failed attempt: https://www.redhat.com/archives/libvir-list/2019-July/msg01257.html

Ah I must have missed that posting before. If after the break you want to coordinate, I'm happy to do quick review of any domain_conf split so it can go in quickly Thanks, Cole