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

Re: [libvirt] [PATCHv5 3/5] domifaddr: Implement the API for qemu




Except what Daniel pointed out:

On 01/09/13 21:43, Nehal J Wani wrote:
By querying the qemu guest agent with the QMP command
"guest-network-get-interfaces" and converting the received JSON
output to structured objects.

Although "ifconfig" is deprecated, IP aliases created by "ifconfig"
are supported by this API. The legacy syntax of an IP alias is:
"<ifname>:<alias-name>". Since we want all aliases to be clubbed
under parent interface, simply stripping ":<alias-name>" suffices.

s/suffices/suffixes/,

Note that IP aliases formed by "ip" aren't visible to "ifconfig",
and aliases created by "ip" do not have any specific name. But
we are lucky, as qemuga detects aliases created by both.

s/qemuga/qemu guest agent/, or s/qemuga/qemu-ga/,


src/qemu/qemu_agent.h:
  * Define qemuAgentGetInterfaces

src/qemu/qemu_agent.c:
  * Implement qemuAgentGetInterface

src/qemu/qemu_driver.c:
  * New function qemuDomainInterfaceAddresses

src/remote_protocol-sructs:
  * Define new structs

tests/qemuagenttest.c:
  * Add new test: testQemuAgentGetInterfaces
    Test cases for IP aliases, 0 or multiple ipv4/ipv6 address(es)

---
 src/qemu/qemu_agent.c  | 193 +++++++++++++++++++++++++++++++++++++++++++++++++
 src/qemu/qemu_agent.h  |   3 +
 src/qemu/qemu_driver.c |  55 ++++++++++++++
 tests/qemuagenttest.c  | 149 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 400 insertions(+)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 2cd0ccc..009ed77 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -1320,6 +1320,199 @@ cleanup:
 }
 
 /*
+ * qemuAgentGetInterfaces:
+ * @mon: Agent

s/Agent/Agent monitor/,

+ * @ifaces: pointer to an array of pointers pointing to interface objects
+ *
+ * Issue guest-network-get-interfaces to guest agent, which returns the

s/the/a/,

+ * list of a interfaces of a running domain along with their IP and MAC

s/of a/of/,

+ * addresses.
+ *
+ * Returns: number of interfaces on success, -1 on error.
+ */
+int
+qemuAgentGetInterfaces(qemuAgentPtr mon,
+                       virDomainInterfacePtr **ifaces)
+{
+    int ret = -1;
+    size_t i, j;
+    int size = -1;
+    virJSONValuePtr cmd = NULL;
+    virJSONValuePtr reply = NULL;
+    virJSONValuePtr ret_array = NULL;
+    size_t ifaces_count = 0;
+    size_t addrs_count = 0;
+    virDomainInterfacePtr *ifaces_ret = NULL;
+    virHashTablePtr ifaces_store = NULL;
+
+    /* Initially the bag of ifaces is empty */

"bag" is magic here, how about:

/* Hash table to handle the interface alias */

+    if (!(ifaces_store = virHashCreate(ifaces_count, NULL)))
+        return -1;
+
+    if (!(cmd = qemuAgentMakeCommand("guest-network-get-interfaces", NULL)))
+       return -1;

You should free the created hash table.

+
+    if (qemuAgentCommand(mon, cmd, &reply, VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0 ||
+        qemuAgentCheckError(cmd, reply) < 0) {
+        goto cleanup;
+    }
+
+    if (!(ret_array = virJSONValueObjectGet(reply, "return"))) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("qemu agent didn't provide 'return' field"));
+        goto cleanup;
+    }
+
+    if ((size = virJSONValueArraySize(ret_array)) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       _("qemu agent didn't return an array of interfaces"));
+        goto cleanup;
+    }
+
+    for (i = 0; i < size; i++) {
+        virJSONValuePtr tmp_iface = virJSONValueArrayGet(ret_array, i);
+        virJSONValuePtr ip_addr_arr = NULL;
+        const char *hwaddr, *ifname_s, *name = NULL;
+        char **ifname = NULL;
+        int ip_addr_arr_size;
+        virDomainInterfacePtr iface = NULL;
+
+        /* Shouldn't happen but doesn't hurt to check neither */
+        if (!tmp_iface) {
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("something has went really wrong"));
+            goto error;
+        }
+
+        /* interface name is required to be presented */
+        name = virJSONValueObjectGetString(tmp_iface, "name");
+        if (!name) {
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("qemu agent didn't provide 'name' field"));
+            goto error;
+        }
+
+        /* Handle aliases of type <ifname>:<alias-name> */

I think no need to mention the "type" here, since it only can be
"ifname:alias". So how about:

/* Handle interface alias (<ifname>:<alias>

+        ifname = virStringSplit(name, ":", 2);
+        ifname_s = ifname[0];
+
+        iface = virHashLookup(ifaces_store, ifname_s);
+
+        /* If the storage bag doesn't contain this iface, add it */

s/storage bag/hash table/,

+        if (!iface) {
+            if (VIR_EXPAND_N(ifaces_ret, ifaces_count, 1) < 0)
+                goto cleanup;

goto error;

+
+            if (VIR_ALLOC(ifaces_ret[ifaces_count - 1]) < 0)
+                goto cleanup;

goto error;

+
+            if (virHashAddEntry(ifaces_store, ifname_s,
+                                ifaces_ret[ifaces_count - 1]) < 0)

Do we really need to use the virDomainInterfacePtr object as hash value? isn't
a reference count is enough? if you use the reference count as the hash value,
the logic can be more compact and clear:

     if (!iface) {
         VIR_EXPAND_N
         VIR_ALLOC
         virHashAddEntry
         .....
     }

     iface = iface_ret[ifaces_count - 1];

+                goto cleanup;

goto error;

+
+            iface = ifaces_ret[ifaces_count - 1];
+            iface->naddrs = 0;
+
+            if (VIR_STRDUP(iface->name, ifname_s) < 0)
+                goto error;
+
+            /* hwaddr might be omitted */
+            hwaddr = virJSONValueObjectGetString(tmp_iface, "hardware-address");
+            if (hwaddr && VIR_STRDUP(iface->hwaddr, hwaddr) < 0)
+                goto error;
+        }
+
+        /* Has to be freed for each interface. */
+        virStringFreeList(ifname);
+
+        /* as well as IP address which - moreover -
+         * can be presented multiple times */
+        ip_addr_arr = virJSONValueObjectGet(tmp_iface, "ip-addresses");
+        if (!ip_addr_arr)
+            continue;
+
+        if ((ip_addr_arr_size = virJSONValueArraySize(ip_addr_arr)) < 0)
+            /* Mmm, empty 'ip-address'? */
+            continue;
+
+        /* If current iface already exists, continue with the count */
+        addrs_count = iface->naddrs;
+
+        for (j = 0; j < ip_addr_arr_size; j++) {
+

Meaningless blank line.

+            if (VIR_EXPAND_N(iface->addrs, addrs_count, 1)  < 0)
+                goto error;
+
+            virJSONValuePtr ip_addr_obj = virJSONValueArrayGet(ip_addr_arr, j);
+            virDomainIPAddressPtr ip_addr = &iface->addrs[addrs_count - 1];
+            const char *type, *addr;
+
+            /* Shouldn't happen but doesn't hurt to check neither */
+            if (!ip_addr_obj) {
+                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                               _("something has went really wrong"));
+                goto error;
+            }
+
+            type = virJSONValueObjectGetString(ip_addr_obj, "ip-address-type");
+            if (!type) {
+                virReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("qemu agent didn't provide 'ip-address-type'"
+                                 " field for interface '%s'"), name);
+                goto error;
+            } else if (STREQ(type, "ipv4")) {
+                ip_addr->type = VIR_IP_ADDR_TYPE_IPV4;
+            } else if (STREQ(type, "ipv6")) {
+                ip_addr->type = VIR_IP_ADDR_TYPE_IPV6;
+            } else {
+                virReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("unknown ip address type '%s'"),
+                               type);
+                goto error;
+            }
+
+            addr = virJSONValueObjectGetString(ip_addr_obj, "ip-address");
+            if (!addr) {
+                virReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("qemu agent didn't provide 'ip-address'"
+                                 " field for interface '%s'"), name);
+                goto error;
+            }
+            if (VIR_STRDUP(ip_addr->addr, addr) < 0)
+                goto error;
+
+            if (virJSONValueObjectGetNumberUint(ip_addr_obj, "prefix",
+                                                &ip_addr->prefix) < 0) {
+                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                               _("malformed 'prefix' field"));
+                goto error;
+            }
+        }
+
+        iface->naddrs = addrs_count;
+    }
+
+    *ifaces = ifaces_ret;
+    ifaces_ret = NULL;
+    ret = ifaces_count;
+
+cleanup:
+    virJSONValueFree(cmd);
+    virJSONValueFree(reply);
+    virHashFree(ifaces_store);
+    return ret;
+
+error:
+    if (ifaces_ret) {
+        for (i = 0; i < ifaces_count; i++)
+            virDomainInterfaceFree(ifaces_ret[i]);
+    }
+    VIR_FREE(ifaces_ret);
+
+    goto cleanup;
+}
+
+/*
  * qemuAgentFSThaw:
  * @mon: Agent
  *
diff --git a/src/qemu/qemu_agent.h b/src/qemu/qemu_agent.h
index 5fbacdb..58b56fd 100644
--- a/src/qemu/qemu_agent.h
+++ b/src/qemu/qemu_agent.h
@@ -76,6 +76,9 @@ int qemuAgentFSThaw(qemuAgentPtr mon);
 int qemuAgentSuspend(qemuAgentPtr mon,
                      unsigned int target);
 
+int qemuAgentGetInterfaces(qemuAgentPtr mon,
+                           virDomainInterfacePtr **ifaces);
+
 int qemuAgentArbitraryCommand(qemuAgentPtr mon,
                               const char *cmd,
                               char **result,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ed29373..908f19c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15870,6 +15870,60 @@ qemuNodeSuspendForDuration(virConnectPtr conn,
     return nodeSuspendForDuration(target, duration, flags);
 }
 
+static int
+qemuDomainInterfaceAddresses(virDomainPtr dom,
+                             virDomainInterfacePtr **ifaces,
+                             unsigned int flags)
+{
+    virQEMUDriverPtr driver = dom->conn->privateData;
+    qemuDomainObjPrivatePtr priv = NULL;
+    virDomainObjPtr vm = NULL;
+    int ret = -1;
+
+    virCheckFlags(0, -1);
+
+    if (!(vm = qemuDomObjFromDomain(dom)))
+        goto cleanup;
+
+    if (!virDomainObjIsActive(vm)) {
+        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+                        _("domain is not running"));
+        goto cleanup;
+    }
+
+    priv = vm->privateData;
+
+    if (virDomainInterfaceAddressesEnsureACL(dom->conn, vm->def) < 0)
+        goto cleanup;
+
+    if (priv->agentError) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                        _("QEMU guest agent is not "
+                          "available due to an error"));
+        goto cleanup;
+    }
+
+    if (!priv->agent) {
+        virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+                        _("QEMU guest agent is not configured"));
+        goto cleanup;
+    }
+
+    if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
+        goto cleanup;
+
+    qemuDomainObjEnterAgent(vm);
+    ret = qemuAgentGetInterfaces(priv->agent, ifaces);
+    qemuDomainObjExitAgent(vm);
+
+    if (qemuDomainObjEndJob(driver, vm) == 0)
+        vm = NULL;
+
+cleanup:
+    if (vm)
+        virObjectUnlock(vm);
+    return ret;
+}
 
 static virDriver qemuDriver = {
     .no = VIR_DRV_QEMU,
@@ -15965,6 +16019,7 @@ static virDriver qemuDriver = {
     .domainMemoryPeek = qemuDomainMemoryPeek, /* 0.4.4 */
     .domainGetBlockInfo = qemuDomainGetBlockInfo, /* 0.8.1 */
     .nodeGetCPUStats = qemuNodeGetCPUStats, /* 0.9.3 */
+    .domainInterfaceAddresses = qemuDomainInterfaceAddresses, /* 1.1.3 */
     .nodeGetMemoryStats = qemuNodeGetMemoryStats, /* 0.9.3 */
     .nodeGetCellsFreeMemory = qemuNodeGetCellsFreeMemory, /* 0.4.4 */
     .nodeGetFreeMemory = qemuNodeGetFreeMemory, /* 0.4.4 */
diff --git a/tests/qemuagenttest.c b/tests/qemuagenttest.c
index 4e27981..4014a09 100644
--- a/tests/qemuagenttest.c
+++ b/tests/qemuagenttest.c
@@ -576,6 +576,154 @@ cleanup:
     return ret;
 }
 
+static const char testQemuAgentGetInterfacesResponse[] =
+    "{\"return\": "
+    "    ["
+    "       {\"name\":\"lo\","
+    "        \"ip-addresses\":"
+    "          ["
+    "             {\"ip-address-type\":\"ipv4\","
+    "              \"ip-address\":\"127.0.0.1\","
+    "              \"prefix\":8"
+    "             },"
+    "             {\"ip-address-type\":\"ipv6\","
+    "              \"ip-address\":\"::1\","
+    "              \"prefix\":128"
+    "             }"
+    "          ],"
+    "        \"hardware-address\":\"00:00:00:00:00:00\""
+    "       },"
+    "       {\"name\":\"eth0\","
+    "        \"ip-addresses\":"
+    "          ["
+    "             {\"ip-address-type\":\"ipv4\","
+    "              \"ip-address\":\"192.168.102.142\","
+    "              \"prefix\":24"
+    "             },"
+    "             {\"ip-address-type\":\"ipv6\","
+    "              \"ip-address\":\"fe80::5054:ff:fe89:ad35\","
+    "              \"prefix\":64"
+    "             },"
+    "             {\"ip-address-type\":\"ipv4\","
+    "              \"ip-address\":\"192.168.234.152\","
+    "              \"prefix\":16"
+    "             },"
+    "             {\"ip-address-type\":\"ipv6\","
+    "              \"ip-address\":\"fe80::5054:ff:fec3:68bb\","
+    "              \"prefix\":64"

Can qemu guest agent really returns mutiple same type IP addresses? Isn't it returning
the other IP addresses as "iface:alias" ?

+    "             }"
+    "          ],"
+    "        \"hardware-address\":\"52:54:00:89:ad:35\""
+    "       },"
+    "       {\"name\":\"eth1\","
+    "        \"ip-addresses\":"
+    "          ["
+    "             {\"ip-address-type\":\"ipv4\","
+    "              \"ip-address\":\"192.168.103.83\","
+    "              \"prefix\":24"
+    "             },"
+    "             {\"ip-address-type\":\"ipv6\","
+    "              \"ip-address\":\"fe80::5054:ff:fed3:39ee\","
+    "              \"prefix\":64"
+    "             }"
+    "          ],"
+    "        \"hardware-address\":\"52:54:00:d3:39:ee\""
+    "       },"
+    "       {\"name\":\"eth1:0\","
+    "        \"ip-addresses\":"
+    "          ["
+    "             {\"ip-address-type\":\"ipv4\","
+    "              \"ip-address\":\"192.168.10.91\","
+    "              \"prefix\":24"
+    "             },"
+    "             {\"ip-address-type\":\"ipv6\","
+    "              \"ip-address\":\"fe80::fc54:ff:fefe:4c4f\","
+    "              \"prefix\":64"
+    "             }"
+    "          ],"
+    "        \"hardware-address\":\"52:54:00:d3:39:ee\""
+    "       },"
+    "       {\"name\":\"eth2\","
+    "        \"hardware-address\":\"52:54:00:36:2a:e5\""
+    "       }"
+    "    ]"
+    "}";
+
+static int
+testQemuAgentGetInterfaces(const void *data)
+{
+    virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data;
+    qemuMonitorTestPtr test = qemuMonitorTestNewAgent(xmlopt);
+    size_t i;
+    int ret = -1;
+    int ifaces_count = 0;
+    virDomainInterfacePtr *ifaces = NULL;
+
+    if (!test)
+        return -1;
+
+    if (qemuMonitorTestAddAgentSyncResponse(test) < 0)
+        goto cleanup;
+
+    if (qemuMonitorTestAddItem(test, "guest-network-get-interfaces",
+                               testQemuAgentGetInterfacesResponse) < 0)
+        goto cleanup;
+
+    if ((ifaces_count = qemuAgentGetInterfaces(qemuMonitorTestGetAgent(test),
+                                               &ifaces)) < 0)
+        goto cleanup;
+
+    if (ifaces_count != 4) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       "expected 4 interfaces, got %d", ret);
+        goto cleanup;
+    }
+
+    if (ifaces[0]->naddrs != 2 ||
+        ifaces[1]->naddrs != 4 ||
+        ifaces[2]->naddrs != 4 ||
+        ifaces[3]->naddrs != 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       "unexpected return value for number of IP addresses");
+        goto cleanup;
+    }
+
+    if (STRNEQ(ifaces[0]->name, "lo") ||
+        STRNEQ(ifaces[0]->addrs[0].addr, "127.0.0.1") ||
+        ifaces[0]->addrs[1].prefix != 128) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       "unexpected return values for interface: lo");
+        goto cleanup;
+    }
+
+    if (STRNEQ(ifaces[1]->hwaddr, "52:54:00:89:ad:35") ||
+        ifaces[1]->addrs[0].prefix != 24 ||
+        ifaces[1]->addrs[1].type != VIR_IP_ADDR_TYPE_IPV6) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       "unexpected return values for interface: eth0");
+        goto cleanup;
+    }
+
+    if (STRNEQ(ifaces[2]->name, "eth1") ||
+        ifaces[2]->addrs[0].type != VIR_IP_ADDR_TYPE_IPV4 ||
+        STRNEQ(ifaces[2]->addrs[1].addr, "fe80::5054:ff:fed3:39ee")) {
+        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                       "unexpected return values for interface: eth1");
+        goto cleanup;
+    }
+
+    ret = 0;
+
+cleanup:
+    qemuMonitorTestFree(test);
+    if (ifaces) {
+        for (i = 0; i < ifaces_count; i++)
+            virDomainInterfaceFree(ifaces[i]);
+    }
+    VIR_FREE(ifaces);
+
+    return ret;
+}
 
 static int
 mymain(void)
@@ -605,6 +753,7 @@ mymain(void)
     DO_TEST(Shutdown);
     DO_TEST(CPU);
     DO_TEST(ArbitraryCommand);
+    DO_TEST(GetInterfaces);
 
     DO_TEST(Timeout); /* Timeout should always be called last */
 


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