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