On Fri, Jul 24, 2015 at 08:01:05AM -0400, Laine Stump wrote:
On 07/24/2015 07:28 AM, Martin Kletzander wrote:On Thu, Jul 23, 2015 at 03:18:04PM +0200, Ján Tomko wrote:On Fri, Jul 17, 2015 at 02:43:32PM -0400, Laine Stump wrote:This new subelement is used in PCI controllers: the toplevel *attribute* "model" of a controller denotes what kind of PCI controller is being described, e.g. a "dmi-to-pci-bridge", "pci-bridge", or "pci-root". But in the future there will be different implementations of some of those types of PCI controllers, which behave similarly from libvirt's point of view (and so should have the same model), but use a different device in qemu (and present themselves as a different piece of hardware in the guest). In an ideal world we (i.e. "I") would have thought of that back when the pci controllers were added, and used some sort of type/class/model notation (where class was used in the way we are now using model, and model was used for the actual manufacturer's model number of a particular family of PCI controller), but that opportunity is long past, so as an alternative, this patch allows selecting a particular implementation of a pci controller with the "type" attribute of the <model> subelement, e.g.: <controller type='pci' model='dmi-to-pci-bridge' index='1'> <model type='i82801b11-bridge'/> </controller>I'd say 'type' would be more fitting for the generic class of the device (dmi-to-pci-bridge), not the exact device model (i82801b11-bridge) so <model name='i82801b11-bridge'/> looks nicer to me, but I'm not going to suggest it again.Well, you just did. But I must say I like "model name" better too. When I pitched the "model type" I did that to show the creation of the new element, not the spelling itself.Using "model type" is based on the existing "model type" for <interface> and <video>. I also think "model name" is more fitting, but it breaks with precedent. Should we knowingly be inconsistent in order to make the new one better? I'm undecided.
I remember replying to this. Is this just another mail or did I just discovered the rest of the email just now? I guess Friday is to blame. Anyway ...
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 50750c1..09fe3c0 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -797,6 +797,14 @@ typedef virDomainPCIControllerOpts *virDomainPCIControllerOptsPtr; struct _virDomainPCIControllerOpts { bool pcihole64; unsigned long pcihole64size; + + /* the type is in the "model" subelement, e.g.: + * <controller type='pci' model='pcie-root-port'> + * <model type='ioh3420''/> + * ... + * similar to the model of <interface> devices. + */ + char *type; /* the exact name of the device in hypervisor */This would be nicer as an enum - we don't allow arbitrary strings there anyway.The only place where we allow arbitrary strings is the interface model and due to that we're having some problems. Not allowing strings is good, storing the enum value is even better.Again, I used a string because that's the way it is in existing code (interface), but will change it to an enum for efficiency's sake. (I'm curious what problem you're refering to though - is it the fact that qemuBuildNicDevStr() doesn't qualify the string before using it in the commandline? That is taken care of in the case of this code.)
<interface> is the only one that does this. And due to back-compat we can't go back (until I finish one RFC, yay). The problem is that you can do e.g. <model type='AC97'/> for an interface. It's not a big deal, it's just annoying because QEMU will use a weird error message. Two more tiny "problems" arise hand in hand with this. One is that we cannot check whether this or that model has some capabilities. Well, we could, but it's way easier when the names are mapped and it's a known list. Also, if you need to compare these throughout the code (which you do in at least one later patch), you need to compare strings instead of values. Martin
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list