[libvirt] [PATCHv5 3/5] domifaddr: Implement the API for qemu
Osier Yang
jyang at redhat.com
Tue Sep 3 15:10:45 UTC 2013
On 03/09/13 23:00, Nehal J Wani wrote:
> On Tue, Sep 3, 2013 at 8:21 PM, Osier Yang <jyang at 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 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
> 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];
>
I was confused, but then the refering the index works. However, in
this case, I'm fine with either solution, since anyway the hash value
is a pointer.
More information about the libvir-list
mailing list