On Tue, Sep 3, 2013 at 8:21 PM, Osier Yang <jyang@xxxxxxxxxx> wrote:
> On 03/09/13 22:37, Nehal J Wani wrote:
>>
>> On Tue, Sep 3, 2013 at 7:46 PM, Osier Yang <jyang@xxxxxxxxxx> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list