[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 10/10] linkstate: qemu: Add link state modification support to qemu driver.



On 08/11/2011 09:27 AM, Peter Krempa wrote:
Adds support for the new API. While starting a qemu link state
cannot be set with an command line argument and therefore is done
by entering monitor.
---
  src/qemu/qemu_driver.c  |  228 +++++++++++++++++++++++++++++++++++++++++++++++
  src/qemu/qemu_process.c |   37 ++++++++
  2 files changed, 265 insertions(+), 0 deletions(-)

Needs rework to be called as part of virDomainUpdateDevice, but you can reuse a lot of the non-boilerplate code for that purpose.

+
+    /* Check the path is one of the domain's network interfaces. */
+    /* Qemu does not support reading the link state, report saved state */

Don't you just love write-only interfaces :) How does link state fare across migration?

+    if (flags&  VIR_DOMAIN_AFFECT_LIVE) {
+        for (i = 0; i<  def->nnets; i++) {
+            if (memcmp(def->nets[i]->mac, mac, VIR_MAC_BUFLEN) == 0) {
+                if (def->nets[i]->info.alias) {
+
+                    if (!qemuCapsGet(priv->qemuCaps, QEMU_CAPS_LINK_SET)) {
+                        qemuReportError(VIR_ERR_OPERATION_FAILED,
+                                        _("This qemu doesn't support setting link state"));
+                        goto endjob;
+                    }
+
+                    qemuDomainObjEnterMonitor(driver, vm);
+                    ret = qemuMonitorSetLink(priv->mon, def->nets[i]->info.alias, state);
+                    qemuDomainObjExitMonitor(driver, vm);
+                } else {
+                    qemuReportError(VIR_ERR_OPERATION_FAILED,
+                                    _("Device alias not found. (QEMU probably doesn't support device naming)"));

Don't we have a QEMU_CAPS that says whether we have -device (and therefore device naming)? In fact, if settable link state was introduced the same time as -device, then we might not need a new QEMU_CAPS_LINK_SET (but I'd have to research when things appeared).

+
+    /* one or both configurations successfuly altered */

s/successfuly/successfully/; although if you hook into virDomainUpdateDevice code properly, this is probably boilerplate that you don't have to worry about.

+/* set link states to down on interfaces at qemu start */
+static int
+qemuProcessSetLinkStates(virDomainObjPtr vm)
+{
+    qemuDomainObjPrivatePtr priv = vm->privateData;
+    virDomainDefPtr def = vm->def;
+    int i;
+    int ret = 0;
+
+    for (i = 0; i<  def->nnets; i++) {
+        if (def->nets[i]->linkstate == VIR_LINK_STATE_DOWN) {
+            ret = qemuMonitorSetLink(priv->mon,
+                                     def->nets[i]->info.alias,
+                                     VIR_LINK_STATE_DOWN);
+
+            if (ret != 0)
+                break;
+        }
+    }
+
+    return ret;
+}
+
  /* Set CPU affinites for vcpus if vcpupin xml provided. */

As long as you are touching near this code: s/affinites/affinities/


+    /* set default link states */
+    /* qemu doesn't support setting this on the command line, so
+     * enter the monitor */

Oh, well that answers my earlier question about why you didn't change qemu_command.c.

+    if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_LINK_SET)) {
+        VIR_DEBUG("Setting network link states");
+        qemuDomainObjEnterMonitorWithDriver(driver, vm);
+        if (qemuProcessSetLinkStates(vm)<  0) {
+            qemuDomainObjExitMonitorWithDriver(driver, vm);
+            goto cleanup;
+        }

I don't think you quite got the logic right. If qemu lacks the capability, we still need to inspect all interfaces, and issue an error if any of them requested link down (if they all lacked <link>, or requested link up, then things are okay), rather than silently ignoring requests for link down when unsupported by qemu.

Overall, I think the feature addition of being able to simulate link down will be nice to have, but the series needs some polish first :)

--
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]