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

[Eric Blake]

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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org