5:58 a.m.
On a Tuesday in 2020, Jonathon Jongsma wrote:
On Tue, 6 Oct 2020 08:58:45 +0200
Ján Tomko <jtomko(a)redhat.com> wrote:
> Convert one interface from the "return" array returned by
> "guest-network-get-interfaces" to virDomainInterface.
>
> Due to the functionality of squashing interface aliases together,
> this is not a pure function - it either:
> * Adds the interface to ifaces_ret, incrementing ifaces_count
> and adds a pointer to it into the ifaces_store hash table.
> * Adds the additional IP addresses from the interface alias
> to the existing interface entry, found through the hash table.
> This does not increment ifaces_count or extend the array.
It seems to me that if we were to factor out this function, this
documentation would belong within the code rather than just in the
commit message. Without it, it's not very obvious what this
function does with its arguments. [1]
The fact that it needs this documentation to be understood and it is
only called from a single place makes me think that it probably isn't
worthwhile to factor it out into a separate function.
It is called from one place because it does one very specific thing.
It needs documentation because the code is (still unnecessarily)
complicated. If we were to leave the code as-is, it would still need
that documentation.
I think it actually makes the code a bit harder to understand.
I see it the other way - even if it changes three out of four of its
arguments, it lets me know that it cannot change others.
Arguably, qemuAgentGetInterfaceAddresses is not as descriptive as it
could be.
[1] I was going to suggest simply removing the 'ifaces_ret' and
'ifaces_count' arguments from this function and doing all of the
processing with just the hash table for storage. Then the calling
function could construct the return array from the hash table after
everything is processed. Unfortunately this won't necessarily maintain
the ordering of the returned interfaces. I don't know if the order is
important to maintain or not...
Yes, this can be improved further by giving up on the order, or adding
a second (even a third pass) to collect them from the hash table in
order, or use an array and collapse the entries afterwards.
I specifically wanted to preserve the existing behavior here to improve
the readability without breaking compatibility (or speed? how many
addresses does the guest need to have to make the hash table worth it?)
Jano
>
> Signed-off-by: Ján Tomko <jtomko(a)redhat.com>
> ---
> src/qemu/qemu_agent.c | 141
> +++++++++++++++++++++++------------------- 1 file changed, 79
> insertions(+), 62 deletions(-)
>
> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
> index 7a4f5cd6db..b314c610e7 100644
> --- a/src/qemu/qemu_agent.c
> +++ b/src/qemu/qemu_agent.c
> @@ -2104,6 +2104,82 @@
> qemuAgentGetInterfaceOneAddress(virDomainIPAddressPtr ip_addr, }
>
>
> +static int
> +qemuAgentGetInterfaceAddresses(virDomainInterfacePtr **ifaces_ret,
> + size_t *ifaces_count,
> + virHashTablePtr ifaces_store,
> + virJSONValuePtr tmp_iface)
> +{
> + virJSONValuePtr ip_addr_arr = NULL;
> + const char *hwaddr, *ifname_s, *name = NULL;
> + virDomainInterfacePtr iface = NULL;
> + g_auto(GStrv) ifname = NULL;
> + size_t addrs_count = 0;
> + size_t j;
> +
> + /* 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"));
> + return -1;
> + }
> +
> + /* Handle interface alias (<ifname>:<alias>) */
> + ifname = virStringSplit(name, ":", 2);
> + ifname_s = ifname[0];
> +
> + iface = virHashLookup(ifaces_store, ifname_s);
> +
> + /* If the hash table doesn't contain this iface, add it */
> + if (!iface) {
> + if (VIR_EXPAND_N(*ifaces_ret, *ifaces_count, 1) < 0)
> + return -1;
> +
> + iface = g_new0(virDomainInterface, 1);
> + (*ifaces_ret)[*ifaces_count - 1] = iface;
> +
> + if (virHashAddEntry(ifaces_store, ifname_s, iface) < 0)
> + return -1;
> +
> + iface->naddrs = 0;
> + iface->name = g_strdup(ifname_s);
> +
> + hwaddr = virJSONValueObjectGetString(tmp_iface,
> "hardware-address");
> + iface->hwaddr = g_strdup(hwaddr);
> + }
> +
> + /* as well as IP address which - moreover -
> + * can be presented multiple times */
> + ip_addr_arr = virJSONValueObjectGet(tmp_iface, "ip-addresses");
> + if (!ip_addr_arr)
> + return 0;
> +
> + if (!virJSONValueIsArray(ip_addr_arr)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Malformed ip-addresses array"));
> + return -1;
> + }
> +
> + /* If current iface already exists, continue with the count */
> + addrs_count = iface->naddrs;
> +
> + if (VIR_EXPAND_N(iface->addrs, addrs_count,
> + virJSONValueArraySize(ip_addr_arr)) < 0)
> + return -1;
> +
> + for (j = 0; j < virJSONValueArraySize(ip_addr_arr); j++) {
> + virJSONValuePtr ip_addr_obj =
> virJSONValueArrayGet(ip_addr_arr, j);
> + virDomainIPAddressPtr ip_addr = iface->addrs +
> iface->naddrs++; +
> + if (qemuAgentGetInterfaceOneAddress(ip_addr, ip_addr_obj,
> name) < 0)
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +
> /*
> * qemuAgentGetInterfaces:
> * @agent: agent object
> @@ -2120,7 +2196,7 @@ qemuAgentGetInterfaces(qemuAgentPtr agent,
> virDomainInterfacePtr **ifaces)
> {
> int ret = -1;
> - size_t i, j;
> + size_t i;
> virJSONValuePtr cmd = NULL;
> virJSONValuePtr reply = NULL;
> virJSONValuePtr ret_array = NULL;
> @@ -2151,70 +2227,11 @@ qemuAgentGetInterfaces(qemuAgentPtr agent,
>
> for (i = 0; i < virJSONValueArraySize(ret_array); i++) {
> virJSONValuePtr tmp_iface = virJSONValueArrayGet(ret_array,
> i);
> - virJSONValuePtr ip_addr_arr = NULL;
> - const char *hwaddr, *ifname_s, *name = NULL;
> - virDomainInterfacePtr iface = NULL;
> - g_auto(GStrv) ifname = NULL;
> - size_t addrs_count = 0;
>
> - /* 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"));
> + if (qemuAgentGetInterfaceAddresses(&ifaces_ret,
> &ifaces_count,
> + ifaces_store, tmp_iface)
> < 0) goto error;
> - }
>
> - /* Handle interface alias (<ifname>:<alias>) */
> - ifname = virStringSplit(name, ":", 2);
> - ifname_s = ifname[0];
> -
> - iface = virHashLookup(ifaces_store, ifname_s);
> -
> - /* If the hash table doesn't contain this iface, add it */
> - if (!iface) {
> - if (VIR_EXPAND_N(ifaces_ret, ifaces_count, 1) < 0)
> - goto error;
> -
> - iface = g_new0(virDomainInterface, 1);
> - ifaces_ret[ifaces_count - 1] = iface;
> -
> - if (virHashAddEntry(ifaces_store, ifname_s, iface) < 0)
> - goto error;
> -
> - iface->naddrs = 0;
> - iface->name = g_strdup(ifname_s);
> -
> - hwaddr = virJSONValueObjectGetString(tmp_iface,
> "hardware-address");
> - iface->hwaddr = g_strdup(hwaddr);
> - }
> -
> - /* 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 (!virJSONValueIsArray(ip_addr_arr)) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("Malformed ip-addresses array"));
> - goto error;
> - }
> -
> - /* If current iface already exists, continue with the count
> */
> - addrs_count = iface->naddrs;
> -
> - if (VIR_EXPAND_N(iface->addrs, addrs_count,
> - virJSONValueArraySize(ip_addr_arr)) < 0)
> - goto error;
> -
> - for (j = 0; j < virJSONValueArraySize(ip_addr_arr); j++) {
> - virJSONValuePtr ip_addr_obj =
> virJSONValueArrayGet(ip_addr_arr, j);
> - virDomainIPAddressPtr ip_addr = iface->addrs +
> iface->naddrs++; -
> - if (qemuAgentGetInterfaceOneAddress(ip_addr,
> ip_addr_obj, name) < 0)
> - goto error;
> - }
> }
>
> *ifaces = g_steal_pointer(&ifaces_ret);