On 01/23/2019 07:08 AM, Andrea Bolognani wrote: > 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. > Alright will do >>>> 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 :) > Ah good catch, though the switch statement will end up in the final result anyways with all the transitional additions Thanks, Cole -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
