On Wed, Dec 11, 2019 at 09:45:07AM -0500, Cole Robinson wrote: 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. 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. 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. 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. 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. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On 12/12/19 6:13 AM, Daniel P. Berrangé wrote: Okay, makes sense to me. 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. 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. Good idea. Thanks, Cole

On 12/12/19 9:57 AM, Ján Tomko wrote: 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