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

[Osier Yang]

On 03/09/13 22:37, Nehal J Wani wrote:
> On Tue, Sep 3, 2013 at 7:46 PM, Osier Yang <jyang at redhat.com> wrote:
>> 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/,
> Here, I by suffices, I meant: "Be enough or adequate."
>
>
>>
>> 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.
> In the label cleanup, I have freed the has table.

But you "returns -1" here.

>
>
>>
>> +
>> +    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];
>>
> If I am not wrong, are you suggesting to have the position of the nth interface
> in ifaces_ret as the reference count?

No, something like "(void *)1" will work.  The only purpose of the hash 
table in
this function is to check whether the same interface name is already in the
ifaces array, so no need to use a pointer to the interface object as the 
hash
value, though it doesn't hurt.

Osier