On 8/30/21 7:35 AM, scuzhanglei wrote:
> Signed-off-by: scuzhanglei <greatzhanglei@xxxxxxxxx>
> ---
> NEWS.rst | 5 ++
> docs/manpages/virsh.rst | 12 ++++-
> include/libvirt/libvirt-domain.h | 1 +
> src/libvirt-domain.c | 12 +++++
> src/qemu/qemu_agent.c | 9 ++--
> src/qemu/qemu_agent.h | 3 +-
> src/qemu/qemu_driver.c | 88 +++++++++++++++++++++++++++++++-
> tests/qemuagenttest.c | 2 +-
> tools/virsh-domain.c | 6 +++
> 9 files changed, 129 insertions(+), 9 deletions(-)
The patch is more-or-less correct, however, we like to split things a
bit. In the first patch you can define the public flag and in this
specific case I'd say also do virsh change (virsh-domain.c + virsh.rst).
Or you can split that into two. Then, in the next patch you'd do the
hypervisor driver change (src/qemu/* + tests/qemuagenttest.c). And as
the very last patch you document the feature in NEWS.rst. This should
always be separate.
The reason for this split is to make it easier to backport patches
(should somebody do that).
Do you think you can do that in v2?
BTW: I'm no native speaker but I don't think "information" has a plural
form, thus s/informations/information/.
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index f1f961c51c..0b803c392b 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -18957,7 +18957,7 @@ qemuDomainInterfaceAddresses(virDomainPtr dom,
> goto endjob;
>
> agent = qemuDomainObjEnterAgent(vm);
> - ret = qemuAgentGetInterfaces(agent, ifaces);
> + ret = qemuAgentGetInterfaces(agent, ifaces, true);
> qemuDomainObjExitAgent(vm, agent);
>
> endjob:
> @@ -19903,7 +19903,8 @@ static const unsigned int qemuDomainGetGuestInfoSupportedTypes =
> VIR_DOMAIN_GUEST_INFO_TIMEZONE |
> VIR_DOMAIN_GUEST_INFO_HOSTNAME |
> VIR_DOMAIN_GUEST_INFO_FILESYSTEM |
> - VIR_DOMAIN_GUEST_INFO_DISKS;
> + VIR_DOMAIN_GUEST_INFO_DISKS |
> + VIR_DOMAIN_GUEST_INFO_INTERFACES;
>
> static int
> qemuDomainGetGuestInfoCheckSupport(unsigned int types,
> @@ -20102,6 +20103,69 @@ qemuAgentFSInfoFormatParams(qemuAgentFSInfo **fsinfo,
> }
> }
>
> +static void
> +virDomainInterfaceFormatParams(virDomainInterfacePtr *ifaces,
> +int nifaces,
> +virTypedParameterPtr *params,
> +int *nparams, int * maxparams)
Indentation looks off.
> +{
> + size_t i, j;
> + const char *type = NULL;
> +
> + if (virTypedParamsAddUInt(params, nparams, maxparams,
> + "if.count", nifaces) < 0)
> + return;
> +
> + for (i = 0; i < nifaces; i++) {
> + char param_name[VIR_TYPED_PARAM_FIELD_LENGTH];
> + g_snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
> + "if.%zu.name", i);
We like to separate variable declaration block and code block with an
empty line.
> + if (virTypedParamsAddString(params, nparams, maxparams,
> + param_name, ifaces[i]->name) < 0)
> + return;
> +
> + g_snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
> + "if.%zu.hwaddr", i);
> + if (virTypedParamsAddString(params, nparams, maxparams,
> + param_name, ifaces[i]->hwaddr) < 0)
> + return;
> +
> + g_snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
> + "if.%zu.addr.count", i);
> + if (virTypedParamsAddUInt(params, nparams, maxparams,
> + param_name, ifaces[i]->naddrs) < 0)
> + return;
> +
> + for (j = 0; j < ifaces[i]->naddrs; j++) {
> + switch (ifaces[i]->addrs[j].type) {
> + case VIR_IP_ADDR_TYPE_IPV4:
> + type = "ipv4";
> + break;
> + case VIR_IP_ADDR_TYPE_IPV6:
> + type = "ipv6";
> + break;
> + }
> +
> + g_snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
> + "if.%zu.addr.%zu.type", i, j);
> + if (virTypedParamsAddString(params, nparams, maxparams,
> + param_name, type) < 0)
> + return;
> +
> + g_snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
> + "if.%zu.addr.%zu.addr", i, j);
> + if (virTypedParamsAddString(params, nparams, maxparams,
> + param_name, ifaces[i]->addrs[j].addr) < 0)
> + return;
> +
> + g_snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
> + "if.%zu.addr.%zu.prefix", i, j);
> + if (virTypedParamsAddUInt(params, nparams, maxparams,
> + param_name, ifaces[i]->addrs[j].prefix) < 0)
> + return;
> + }
> + }
> +}
>
Michal
