Re: [PATCH] conf: Restrict model type to enum members

[Ján Tomko <jtomko@xxxxxxxxxx>]

On a Wednesday in 2025, Adam Julis wrote:
> Historically, we supported any string as the model type for
> network devices. This approach allowed us to stay up-to-date with
> QEMU's frequent introduction of new model types. However, this
> solution now causes more problems than benefits. A clearly
> nonsensical model name can pass validation but result in a QEMU
> internal error when the VM is started.
>
> This patch restricts model types to members of the
> 'virDomainNetModelType' enum. The enum has been extended to include
> all model types currently supported by QEMU. If QEMU introduces
> new models in the future, the enum must be updated to support
> them.
>
> Resolves: https://issues.redhat.com/browse/RHEL-72082

I apologize for suggesting that issue to you, it looked easy at
the first glance but then I looked into it and wrote the comment
that I wrote.

Personally, I'm inclined to close the issue as WONTFIX, or at least
without adding any models we don't know to work for sure with libvirt.

> Signed-off-by: Adam Julis <ajulis@xxxxxxxxxx>
> ---
>
> I'm not sure if the virtio-net-pci should be in this enum, since its not
> located in qemu repo in hw/net/ but in hw/virtio/. When I manually
> tested it, the qemu this option supported. The last two members have not
> been supported for several years but I wanted to keep backward compatibility.

I think neither "virtio-net" nor "virtio-net-pci" should be in this
list. For virtio devices, libvirt expects the model to be just "virtio";
then we have logic based on the model being VIR_DOMAIN_NET_MODEL_VIRTIO
which adds the correct suffix to "virtio-net" based on whether the
address is PCI or CCW (on s390x)

> src/conf/domain_conf.c     | 23 +++++++++++++++++++++++
> src/conf/domain_conf.h     | 23 +++++++++++++++++++++++
> src/conf/domain_validate.c |  7 +++++++
> 3 files changed, 53 insertions(+)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index af88d0bcfd..5e8ca0015d 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -613,6 +613,29 @@ VIR_ENUM_IMPL(virDomainNetModel,
>               "82540EM",
>               "82545EM",
>               "82543GC",
> +              "dp8393x",
> +              "eepro100",

That is not a valid device name. If you derived the list from the
filenames in QEMU's hw/net, that's not going to work.

hw/net/eepro100.c seems to implement various models named i8255*

> +              "ftgmac100",
> +              "igbvf",
> +              "lasi_i82596",
> +              "mcf_fec",
> +              "mipsnet",
> +              "ne2000-isa",

This also won't work because libvirt tries to assign a PCI address to
any net device that
1) should not use a S390 or CCW model
2) is not usb-net

Jano

> +              "ne2000-pci",
> +              "npcm7xx_emc",
> +              "npcm_gmac",
> +              "opencores_eth",
> +              "pcnet-pci",
> +              "rocker",
> +              "spapr_llan",
> +              "sungem",
> +              "sunhme",
> +              "tulip",
> +              "virtio-net",
> +              "xen_nic",
> +              "virtio-net-pci",
> +              "etraxfs_eth",
> +              "milkymist-minimac2",
> );
>
> VIR_ENUM_IMPL(virDomainNetDriver,
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 9f7c28343f..d7f0073ba5 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -951,6 +951,29 @@ typedef enum {
>     VIR_DOMAIN_NET_MODEL_82540EM,
>     VIR_DOMAIN_NET_MODEL_82545EM,
>     VIR_DOMAIN_NET_MODEL_82543GC,
> +    VIR_DOMAIN_NET_MODEL_DP8393X,
> +    VIR_DOMAIN_NET_MODEL_EEPRO100,
> +    VIR_DOMAIN_NET_MODEL_FTGMAC100,
> +    VIR_DOMAIN_NET_MODEL_IGBVF,
> +    VIR_DOMAIN_NET_MODEL_LASI_I82596,
> +    VIR_DOMAIN_NET_MODEL_MCF_FEC,
> +    VIR_DOMAIN_NET_MODEL_MIPSNET,
> +    VIR_DOMAIN_NET_MODEL_NE2000_ISA,
> +    VIR_DOMAIN_NET_MODEL_NE2000_PCI,
> +    VIR_DOMAIN_NET_MODEL_NPCM7XX_EMC,
> +    VIR_DOMAIN_NET_MODEL_NPCM_GMAC,
> +    VIR_DOMAIN_NET_MODEL_OPENCORES_ETH,
> +    VIR_DOMAIN_NET_MODEL_PCNET_PCI,
> +    VIR_DOMAIN_NET_MODEL_ROCKER,
> +    VIR_DOMAIN_NET_MODEL_SPARP_LLAN,
> +    VIR_DOMAIN_NET_MODEL_SUNGEM,
> +    VIR_DOMAIN_NET_MODEL_SUNHME,
> +    VIR_DOMAIN_NET_MODEL_TULIP,
> +    VIR_DOMAIN_NET_MODEL_VIRTIO_NET,
> +    VIR_DOMAIN_NET_MODEL_XEN_NIC,
> +    VIR_DOMAIN_NET_MODEL_VIRTIO_NET_PCI,
> +    VIR_DOMAIN_NET_MODEL_ETRAXFS_ETH,
> +    VIR_DOMAIN_NET_MODEL_MILKYMIST_MINIMAC2,
>
>     VIR_DOMAIN_NET_MODEL_LAST
> } virDomainNetModelType;
> diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
> index 1034bb57f5..fed013835d 100644
> --- a/src/conf/domain_validate.c
> +++ b/src/conf/domain_validate.c
> @@ -2218,6 +2218,13 @@ virDomainNetDefValidate(const virDomainNetDef *net)
>         break;
>
>     case VIR_DOMAIN_NET_TYPE_NETWORK:
> +        if (net->modelstr) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("Unsupported model type '%s'"),
> +                           net->modelstr);
> +            return -1;
> +        }
> +        break;
>     case VIR_DOMAIN_NET_TYPE_VDPA:
>     case VIR_DOMAIN_NET_TYPE_BRIDGE:
>     case VIR_DOMAIN_NET_TYPE_CLIENT:
> --
> 2.47.1

Attachment: signature.asc
Description: PGP signature