On 05/13/2013 01:22 PM, Michal Privoznik wrote:
> Currently, we have one huge function to construct qemu command line.
> This is very ineffective esp. if there's a fault somewhere.
> ---
> src/qemu/qemu_command.c | 228 +++++++++++++++++++++++++-----------------------
> 1 file changed, 121 insertions(+), 107 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index eddc263..7775471 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -6356,6 +6356,121 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg,
> return 0;
> }
>
> +static int
> +qemuBuildInterfaceCommandLine(virCommandPtr cmd,
> + virQEMUDriverPtr driver,
> + virConnectPtr conn,
> + virDomainDefPtr def,
> + virDomainNetDefPtr net,
> + virQEMUCapsPtr qemuCaps,
> + int vlan,
> + int bootindex,
> + enum virNetDevVPortProfileOp vmop)
> +{
> + int ret = -1;
> + int tapfd = -1;
> + char *nic = NULL, *host = NULL;
> + char *tapfdName = NULL;
> + char *vhostfdName = NULL;
> + int actualType = virDomainNetGetActualType(net);
> + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
This made me take a look at how often/where virQEMUDriverGetConfig is
called. Although it does it for a very short time,
virQEMUDriverGetConfig does lock the entire driver, but this is
unnecessary because the caller already has a reference to the config
object. Unless there's some unwritten rule against it (and I don't think
there is, because there are other qemuBuild*CommandLine() functions that
do it), I think we should be passing the driver config pointer into
subordinate functions, rather than going to the time/trouble to lock the
driver just to get another reference to the object when we know that we
already have a reference in the same thread.
(it's not as important here since this is just control plane operations,
but every time you grab a lock, all the caches on all the CPU cores are
flushed.)
> +
> + if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
> + /* NET_TYPE_HOSTDEV devices are really hostdev devices, so
> + * their commandlines are constructed with other hostdevs.
> + */
> + ret = 0;
> + goto cleanup;
> + }
If you did the above check prior to virQEMUDriverGetConfig() (or send
cfg in as an arg), you could just do an immediate "return 0;"
> +
> + if (!bootindex)
> + bootindex = net->info.bootIndex;
> +
> + if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
> + actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
> + tapfd = qemuNetworkIfaceConnect(def, conn, driver, net, qemuCaps);
> + if (tapfd < 0)
> + goto cleanup;
> + } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
> + tapfd = qemuPhysIfaceConnect(def, driver, net, qemuCaps, vmop);
> + if (tapfd < 0)
> + goto cleanup;
> + }
> +
> + if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
> + actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
> + actualType == VIR_DOMAIN_NET_TYPE_ETHERNET ||
> + actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
> + /* Attempt to use vhost-net mode for these types of
> + network device */
> + int vhostfd;
> +
> + if (qemuOpenVhostNet(def, net, qemuCaps, &vhostfd) < 0)
> + goto cleanup;
> + if (vhostfd >= 0) {
> + virCommandTransferFD(cmd, vhostfd);
> +
> + if (virAsprintf(&vhostfdName, "%d", vhostfd) < 0) {
That blank line before the if() looks odd.
> + virReportOOMError();
> + goto cleanup;
> + }
> + }
> + }
> +
> + if (tapfd >= 0) {
> + if (virAsprintf(&tapfdName, "%d", tapfd) < 0) {
> + virReportOOMError();
> + goto cleanup;
> + }
> + virCommandTransferFD(cmd, tapfd);
I know that, practically speaking, it makes no difference, but up above
you call virCommandTransferFD, then virAsprintf, and here you do it in
the opposite order. The OCD in me wants to see them both do it in the
same order :-)
> + }
> +
> + /* Possible combinations:
> + *
> + * 1. Old way: -net nic,model=e1000,vlan=1 -net tap,vlan=1
> + * 2. Semi-new: -device e1000,vlan=1 -net tap,vlan=1
> + * 3. Best way: -netdev type=tap,id=netdev1 -device e1000,id=netdev1
> + *
> + * NB, no support for -netdev without use of -device
> + */
> + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) &&
> + virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
> + if (!(host = qemuBuildHostNetStr(net, driver,
> + ',', vlan, tapfdName,
> + vhostfdName)))
> + goto cleanup;
> + virCommandAddArgList(cmd, "-netdev", host, NULL);
> + }
> + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
> + if (!(nic = qemuBuildNicDevStr(net, vlan, bootindex, qemuCaps)))
> + goto cleanup;
> + virCommandAddArgList(cmd, "-device", nic, NULL);
> + } else {
> + if (!(nic = qemuBuildNicStr(net, "nic,", vlan)))
> + goto cleanup;
> + virCommandAddArgList(cmd, "-net", nic, NULL);
> + }
> + if (!(virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) &&
> + virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))) {
> + if (!(host = qemuBuildHostNetStr(net, driver,
> + ',', vlan, tapfdName,
> + vhostfdName)))
> + goto cleanup;
> + virCommandAddArgList(cmd, "-net", host, NULL);
> + }
This set if if() clauses seems more complex than it needs to be, but
you're just copying existing code, so we can leave it as is.
> +
> + ret = 0;
> +cleanup:
> + if (ret < 0)
> + virDomainConfNWFilterTeardown(net);
> + VIR_FREE(nic);
> + VIR_FREE(host);
> + VIR_FREE(tapfdName);
> + VIR_FREE(vhostfdName);
> + virObjectUnref(cfg);
> + return ret;
> +}
> +
> /*
> * Constructs a argv suitable for launching qemu with config defined
> * for a given virtual machine.
> @@ -7269,118 +7384,17 @@ qemuBuildCommandLine(virConnectPtr conn,
>
> for (i = 0 ; i < def->nnets ; i++) {
> virDomainNetDefPtr net = def->nets[i];
> - char *nic, *host;
> - char tapfd_name[50] = "";
> - char vhostfd_name[50] = "";
> - int vlan;
> - int bootindex = bootNet;
> - int actualType = virDomainNetGetActualType(net);
> -
> - if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
> - /* NET_TYPE_HOSTDEV devices are really hostdev devices, so
> - * their commandlines are constructed with other hostdevs.
> - */
> - continue;
> - }
> -
> - bootNet = 0;
> - if (!bootindex)
> - bootindex = net->info.bootIndex;
> -
> + int vlan = i;
> /* VLANs are not used with -netdev, so don't record them */
> if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) &&
> virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))
> vlan = -1;
> - else
> - vlan = i;
>
> - if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
> - actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
> - int tapfd = qemuNetworkIfaceConnect(def, conn, driver, net,
> - qemuCaps);
> - if (tapfd < 0)
> - goto error;
> -
> - last_good_net = i;
> - virCommandTransferFD(cmd, tapfd);
> -
> - if (snprintf(tapfd_name, sizeof(tapfd_name), "%d",
> - tapfd) >= sizeof(tapfd_name))
> - goto no_memory;
> - } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
> - int tapfd = qemuPhysIfaceConnect(def, driver, net,
> - qemuCaps, vmop);
> - if (tapfd < 0)
> - goto error;
> -
> - last_good_net = i;
> - virCommandTransferFD(cmd, tapfd);
> -
> - if (snprintf(tapfd_name, sizeof(tapfd_name), "%d",
> - tapfd) >= sizeof(tapfd_name))
> - goto no_memory;
> - }
> -
> - if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
> - actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
> - actualType == VIR_DOMAIN_NET_TYPE_ETHERNET ||
> - actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
> - /* Attempt to use vhost-net mode for these types of
> - network device */
> - int vhostfd;
> -
> - if (qemuOpenVhostNet(def, net, qemuCaps, &vhostfd) < 0)
> - goto error;
> - if (vhostfd >= 0) {
> - virCommandTransferFD(cmd, vhostfd);
> -
> - if (snprintf(vhostfd_name, sizeof(vhostfd_name), "%d",
> - vhostfd) >= sizeof(vhostfd_name))
> - goto no_memory;
> - }
> - }
> - /* Possible combinations:
> - *
> - * 1. Old way: -net nic,model=e1000,vlan=1 -net tap,vlan=1
> - * 2. Semi-new: -device e1000,vlan=1 -net tap,vlan=1
> - * 3. Best way: -netdev type=tap,id=netdev1 -device e1000,id=netdev1
> - *
> - * NB, no support for -netdev without use of -device
> - */
> - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) &&
> - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
> - virCommandAddArg(cmd, "-netdev");
> - if (!(host = qemuBuildHostNetStr(net, driver,
> - ',', vlan, tapfd_name,
> - vhostfd_name)))
> - goto error;
> - virCommandAddArg(cmd, host);
> - VIR_FREE(host);
> - }
> - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
> - virCommandAddArg(cmd, "-device");
> - nic = qemuBuildNicDevStr(net, vlan, bootindex, qemuCaps);
> - if (!nic)
> - goto error;
> - virCommandAddArg(cmd, nic);
> - VIR_FREE(nic);
> - } else {
> - virCommandAddArg(cmd, "-net");
> - if (!(nic = qemuBuildNicStr(net, "nic,", vlan)))
> - goto error;
> - virCommandAddArg(cmd, nic);
> - VIR_FREE(nic);
> - }
> - if (!(virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) &&
> - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))) {
> - virCommandAddArg(cmd, "-net");
> - if (!(host = qemuBuildHostNetStr(net, driver,
> - ',', vlan, tapfd_name,
> - vhostfd_name)))
> - goto error;
> - virCommandAddArg(cmd, host);
> - VIR_FREE(host);
> - }
> + if (qemuBuildInterfaceCommandLine(cmd, driver, conn, def, net,
> + qemuCaps, vlan, bootNet, vmop) < 0)
> + goto error;
> + last_good_net = i;
> + bootNet = 0;
> }
> }
>
ACK if you get rid of virQEMUDriverGetConfig and pass cfg as a parameter
instead.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list