[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



On Tue, Sep 3, 2013 at 8:21 PM, Osier Yang <jyang redhat com> wrote:
> On 03/09/13 22:37, Nehal J Wani wrote:
>>
>> On Tue, Sep 3, 2013 at 7:46 PM, Osier Yang <jyang 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

To check if interface already exists, I use:

iface = virHashLookup(ifaces_store, ifname_s);

Consider the case when the interface is already present, in that case,
according to you, iface will be equal to (void *)1. Which is not what we
want. We *need*  iface to be equal to that virDomainInterfacePtr which
points to the already existing interface, and hence it is put as hash value.

iface won't always be iface_ret[ifaces_count - 1];

-- 
Nehal J Wani


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