[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9d24633d-b2bf-4cbe-86f7-6df56ba14657@linux.microsoft.com>
Date: Mon, 18 Mar 2024 09:19:16 -0700
From: Easwar Hariharan <eahariha@...ux.microsoft.com>
To: Shradha Gupta <shradhagupta@...ux.microsoft.com>,
linux-kernel@...r.kernel.org, linux-hyperv@...r.kernel.org
Cc: "K. Y. Srinivasan" <kys@...rosoft.com>,
Haiyang Zhang <haiyangz@...rosoft.com>, Wei Liu <wei.liu@...nel.org>,
Dexuan Cui <decui@...rosoft.com>, Long Li <longli@...rosoft.com>,
Olaf Hering <olaf@...fle.de>, Ani Sinha <anisinha@...hat.com>,
Shradha Gupta <shradhagupta@...rosoft.com>
Subject: Re: [PATCH v3] hv/hv_kvp_daemon: Handle IPv4 and Ipv6 combination for
keyfile format
On 3/17/2024 7:45 PM, Shradha Gupta wrote:
> If the network configuration strings are passed as a combination of IPv and
Repeating a few unaddressed comments from v2.
Missing a 4 in the IPv4 string here
> IPv6 addresses, the current KVP daemon doesnot handle it for the keyfile
You probably want to add a space so it reads as *...KVP daemon does not*, or contract it to *doesn't*
> configuration format.
> With these changes, the keyfile config generation logic scans through the
> list twice to generate IPv4 and IPv6 sections for the configuration files
> to handle this support.
>
> Testcases ran:Rhel 9, Hyper-V VMs
> (IPv4 only, IPv6 only, IPv4 and IPv6 combination)
> Signed-off-by: Shradha Gupta <shradhagupta@...ux.microsoft.com>
> ---
> Changes in v3
> * Introduced a macro for the output string size
> * Added cound checks and used strncpy instead of strncpy
> * Rearranged code to reduce total lines of code
> ---
> tools/hv/hv_kvp_daemon.c | 177 ++++++++++++++++++++++++++++++---------
> 1 file changed, 136 insertions(+), 41 deletions(-)
>
> diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
> index 318e2dad27e0..156cef99d361 100644
> --- a/tools/hv/hv_kvp_daemon.c
> +++ b/tools/hv/hv_kvp_daemon.c
> @@ -76,6 +76,12 @@ enum {
> DNS
> };
>
> +enum {
> + IPV4 = 1,
> + IPV6,
> + IP_TYPE_MAX
> +};
> +
> static int in_hand_shake;
>
> static char *os_name = "";
> @@ -102,6 +108,11 @@ static struct utsname uts_buf;
>
> #define MAX_FILE_NAME 100
> #define ENTRIES_PER_BLOCK 50
> +/*
> + * Change this entry if the number of addresses increases in future
> + */
> +#define MAX_IP_ENTRIES 64
> +#define OUTSTR_BUF_SIZE ((INET6_ADDRSTRLEN + 1) * MAX_IP_ENTRIES)
>
> struct kvp_record {
> char key[HV_KVP_EXCHANGE_MAX_KEY_SIZE];
> @@ -1171,6 +1182,18 @@ static int process_ip_string(FILE *f, char *ip_string, int type)
> return 0;
> }
>
> +int ip_version_check(const char *input_addr)
> +{
> + struct in6_addr addr;
> +
> + if (inet_pton(AF_INET, input_addr, &addr))
> + return IPV4;
> + else if (inet_pton(AF_INET6, input_addr, &addr))
> + return IPV6;
> +
> + return -EINVAL;
> +}
> +
> /*
> * Only IPv4 subnet strings needs to be converted to plen
> * For IPv6 the subnet is already privided in plen format
> @@ -1197,14 +1220,71 @@ static int kvp_subnet_to_plen(char *subnet_addr_str)
> return plen;
> }
>
> +static int process_dns_gateway_nm(FILE *f, char *ip_string, int type,
> + int ip_sec)
> +{
> + char addr[INET6_ADDRSTRLEN], *output_str;
> + int ip_offset = 0, error = 0, ip_ver;
> + char *param_name;
> +
> + memset(addr, 0, sizeof(addr));
Echoing Ani, you don't need this memset here since your first step in the loop below is to
memset(addr, 0).
> +
> + if (type == DNS)
> + param_name = "dns";
> + else if (type == GATEWAY)
> + param_name = "gateway";
> + else
> + return -EINVAL;
> +
> + output_str = (char *)calloc(OUTSTR_BUF_SIZE, sizeof(char));
> + if (!output_str)
> + return -ENOMEM;
> +
> + while (1) {
> + memset(addr, 0, sizeof(addr));
> +
> + if (!parse_ip_val_buffer(ip_string, &ip_offset, addr,
> + (MAX_IP_ADDR_SIZE * 2)))
> + break;
> +
> + ip_ver = ip_version_check(addr);
> + if (ip_ver < 0)
> + continue;
> +
> + if ((ip_ver == IPV4 && ip_sec == IPV4) ||
> + (ip_ver == IPV6 && ip_sec == IPV6)) {
> + /*
> + * do a bound check to avoid out-of bound writes
> + */
> + if ((OUTSTR_BUF_SIZE - strlen(output_str)) >
> + (strlen(addr) + 1)) {
> + strncat(output_str, addr,
> + OUTSTR_BUF_SIZE - strlen(output_str));
> + strncat(output_str, ",",
> + OUTSTR_BUF_SIZE - strlen(output_str));
> + }
> + } else {
> + continue;
> + }
> + }
> +
> + if (strlen(output_str)) {
> + output_str[strlen(output_str) - 1] = '\0';
You don't need this since you're using strncat which adds its own '\0'. I wasn't quite able to follow along
on the discussion between Ani and you, so putting this in here in case it wasn't already mentioned.
> + error = fprintf(f, "%s=%s\n", param_name, output_str);
> + }
> +
> + free(output_str);
> + return error;
> +}
<snip>
Powered by blists - more mailing lists