On Tue, 2019-01-22 at 16:27 -0500, Cole Robinson wrote:
On 01/22/2019 12:39 PM, Cole Robinson wrote:
On 01/21/2019 11:49 AM, Andrea Bolognani wrote:
Completely agree with the rationale here; however, in order to really match the way other devices are handled, I think we should make it so you can use
<controller type='scsi' model='virtio'/>
as well, which of course would behave the same as the currently available version. What do you think?
Yes I agree it would be nice to add that option. I suggest we make it a separate issue from this series though incase it's a contentious idea, v2 is already shaping up to be ~30 patches...
I don't think that's going to be particularly controversial, and we should really make sure we get all the user-facing bits in at the same time IMHO, so I'd say go for it... If you're really unsure about it you can add that model in a separate patch right after this one, that way if someone happens not to like that we can drop it and otherwise we can squash them together before pushing.
Using strstr() here is kinda crude, especially since the caller has enough information to pass the appropriate virDomainControllerType value, both in this case and later on for serial controllers.
I'd say just add yet another argument to the function. Yeah, it starts to get quite unsightly, but I'd really rather not resort to string comparison when a nice, type safe enum will do.
Yeah it's hacky. Adding another arg here is going to add extra pain if when this is merged into qemuBuildVirtioStr, most callers are going have NULL arguments, and an increased chance of new future callers passing in incorrect values and accidentally triggering a wrong code path. I feel like this is another argument for the separated BuildTransitional or whatever, but I'm not sold either way so I'll stick with your suggestion
What do you think of this approach? See the attached two patches. It adds a domain_conf.c function virDomainDeviceSetData which makes it easier to create a virDomainDeviceDef. qemuBuildVirtioDevStr callers now pass in a virDomainDeviceType and the specific DefPtr for their device (virDomainDiskDefPtr, virDomainNetDefPtr etc). qemuBuildVirtioDevStr then turns that into a virDomainDeviceDef and acts on that locally.
Saves having to extend the argument list several times (like this example above, and the net->model string example). Seperately I think virDomainDeviceSetData can be used to clean up some device interactions elsewhere too
I like it! I actually wanted to suggest something like that earlier, but for some reason I thought it would be more complicated than it turned out to be... Better yet, you don't even need to add that switch statement to qemuBuildVirtioDevStr(): you can just use virDomainDeviceGetInfo() instead, thus making the code shorter and nicer :) -- Andrea Bolognani / Red Hat / Virtualization