If the network configuration strings are passed as a combination of IPv and
IPv6 addresses, the current KVP daemon doesnot handle it for the keyfile
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.
Built-on: Rhel9
Tested-on: Rhel9(IPv4 only, IPv6 only, IPv4 and IPv6 combination)
Signed-off-by: Shradha Gupta <[email protected]>
---
Changes in v2
* Use calloc to avoid initialization later
* Return standard error codes
* Free the output_str pointer on completion
* Add out-of bound checks while writing to buffers
---
tools/hv/hv_kvp_daemon.c | 173 +++++++++++++++++++++++++++++----------
1 file changed, 132 insertions(+), 41 deletions(-)
diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
index 318e2dad27e0..ae65be004eb1 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,7 @@ static struct utsname uts_buf;
#define MAX_FILE_NAME 100
#define ENTRIES_PER_BLOCK 50
+#define MAX_IP_ENTRIES 64
struct kvp_record {
char key[HV_KVP_EXCHANGE_MAX_KEY_SIZE];
@@ -1171,6 +1178,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;
+ else
+ return -EINVAL;
+}
+
/*
* Only IPv4 subnet strings needs to be converted to plen
* For IPv6 the subnet is already privided in plen format
@@ -1197,14 +1216,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;
+
+ output_str = (char *)calloc(INET6_ADDRSTRLEN * MAX_IP_ENTRIES,
+ sizeof(char));
+
+ if (!output_str)
+ return -ENOMEM;
+
+ memset(addr, 0, sizeof(addr));
+
+ if (type == DNS) {
+ param_name = "dns";
+ } else if (type == GATEWAY) {
+ param_name = "gateway";
+ } else {
+ error = -EINVAL;
+ goto cleanup;
+ }
+
+ while (parse_ip_val_buffer(ip_string, &ip_offset, addr,
+ (MAX_IP_ADDR_SIZE * 2))) {
+ ip_ver = ip_version_check(addr);
+ if (ip_ver < 0)
+ continue;
+
+ if ((ip_ver == IPV4 && ip_sec == IPV4) ||
+ (ip_ver == IPV6 && ip_sec == IPV6)) {
+ if (((INET6_ADDRSTRLEN * MAX_IP_ENTRIES) - strlen(output_str)) >
+ (strlen(addr))) {
+ strcat(output_str, addr);
+ strcat(output_str, ",");
+ }
+ memset(addr, 0, sizeof(addr));
+
+ } else {
+ memset(addr, 0, sizeof(addr));
+ continue;
+ }
+ }
+
+ if (strlen(output_str)) {
+ output_str[strlen(output_str) - 1] = '\0';
+ error = fprintf(f, "%s=%s\n", param_name, output_str);
+ if (error < 0)
+ goto cleanup;
+ }
+
+cleanup:
+ free(output_str);
+ return error;
+}
+
static int process_ip_string_nm(FILE *f, char *ip_string, char *subnet,
- int is_ipv6)
+ int ip_sec)
{
char addr[INET6_ADDRSTRLEN];
char subnet_addr[INET6_ADDRSTRLEN];
int error, i = 0;
int ip_offset = 0, subnet_offset = 0;
- int plen;
+ int plen, ip_ver;
memset(addr, 0, sizeof(addr));
memset(subnet_addr, 0, sizeof(subnet_addr));
@@ -1216,10 +1292,16 @@ static int process_ip_string_nm(FILE *f, char *ip_string, char *subnet,
subnet_addr,
(MAX_IP_ADDR_SIZE *
2))) {
- if (!is_ipv6)
+ ip_ver = ip_version_check(addr);
+ if (ip_ver < 0)
+ continue;
+
+ if (ip_ver == IPV4 && ip_sec == IPV4)
plen = kvp_subnet_to_plen((char *)subnet_addr);
- else
+ else if (ip_ver == IPV6 && ip_sec == IPV6)
plen = atoi(subnet_addr);
+ else
+ continue;
if (plen < 0)
return plen;
@@ -1238,12 +1320,11 @@ static int process_ip_string_nm(FILE *f, char *ip_string, char *subnet,
static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
{
- int error = 0;
+ int error = 0, ip_type;
char if_filename[PATH_MAX];
char nm_filename[PATH_MAX];
FILE *ifcfg_file, *nmfile;
char cmd[PATH_MAX];
- int is_ipv6 = 0;
char *mac_addr;
int str_len;
@@ -1421,52 +1502,62 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
if (error)
goto setval_error;
- if (new_val->addr_family & ADDR_FAMILY_IPV6) {
- error = fprintf(nmfile, "\n[ipv6]\n");
- if (error < 0)
- goto setval_error;
- is_ipv6 = 1;
- } else {
- error = fprintf(nmfile, "\n[ipv4]\n");
- if (error < 0)
- goto setval_error;
- }
-
/*
- * Now we populate the keyfile format
+ * The keyfile format expects the IPv6 and IPv4 configuration in
+ * different sections. Therefore we iterate through the list twice,
+ * once to populate the IPv4 section and the next time for IPv6
*/
+ ip_type = IPV4;
+ do {
+ if (ip_type == IPV4) {
+ error = fprintf(nmfile, "\n[ipv4]\n");
+ if (error < 0)
+ goto setval_error;
+ } else {
+ error = fprintf(nmfile, "\n[ipv6]\n");
+ if (error < 0)
+ goto setval_error;
+ }
- if (new_val->dhcp_enabled) {
- error = kvp_write_file(nmfile, "method", "", "auto");
- if (error < 0)
- goto setval_error;
- } else {
- error = kvp_write_file(nmfile, "method", "", "manual");
+ /*
+ * Now we populate the keyfile format
+ */
+
+ if (new_val->dhcp_enabled) {
+ error = kvp_write_file(nmfile, "method", "", "auto");
+ if (error < 0)
+ goto setval_error;
+ } else {
+ error = kvp_write_file(nmfile, "method", "", "manual");
+ if (error < 0)
+ goto setval_error;
+ }
+
+ /*
+ * Write the configuration for ipaddress, netmask, gateway and
+ * name services
+ */
+ error = process_ip_string_nm(nmfile, (char *)new_val->ip_addr,
+ (char *)new_val->sub_net,
+ ip_type);
if (error < 0)
goto setval_error;
- }
- /*
- * Write the configuration for ipaddress, netmask, gateway and
- * name services
- */
- error = process_ip_string_nm(nmfile, (char *)new_val->ip_addr,
- (char *)new_val->sub_net, is_ipv6);
- if (error < 0)
- goto setval_error;
-
- /* we do not want ipv4 addresses in ipv6 section and vice versa */
- if (is_ipv6 != is_ipv4((char *)new_val->gate_way)) {
- error = fprintf(nmfile, "gateway=%s\n", (char *)new_val->gate_way);
+ error = process_dns_gateway_nm(nmfile,
+ (char *)new_val->gate_way,
+ GATEWAY, ip_type);
if (error < 0)
goto setval_error;
- }
- if (is_ipv6 != is_ipv4((char *)new_val->dns_addr)) {
- error = fprintf(nmfile, "dns=%s\n", (char *)new_val->dns_addr);
+ error = process_dns_gateway_nm(nmfile,
+ (char *)new_val->dns_addr, DNS,
+ ip_type);
if (error < 0)
goto setval_error;
- }
+
+ ip_type++;
+ } while (ip_type < IP_TYPE_MAX);
+
fclose(nmfile);
fclose(ifcfg_file);
--
2.34.1
On Tue, 12 Mar 2024, Shradha Gupta wrote:
> If the network configuration strings are passed as a combination of IPv and
> IPv6 addresses, the current KVP daemon doesnot handle it for the keyfile
> 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.
>
> Built-on: Rhel9
> Tested-on: Rhel9(IPv4 only, IPv6 only, IPv4 and IPv6 combination)
> Signed-off-by: Shradha Gupta <[email protected]>
> ---
> Changes in v2
> * Use calloc to avoid initialization later
> * Return standard error codes
> * Free the output_str pointer on completion
> * Add out-of bound checks while writing to buffers
> ---
> tools/hv/hv_kvp_daemon.c | 173 +++++++++++++++++++++++++++++----------
> 1 file changed, 132 insertions(+), 41 deletions(-)
>
> diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
> index 318e2dad27e0..ae65be004eb1 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,7 @@ static struct utsname uts_buf;
>
> #define MAX_FILE_NAME 100
> #define ENTRIES_PER_BLOCK 50
> +#define MAX_IP_ENTRIES 64
>
> struct kvp_record {
> char key[HV_KVP_EXCHANGE_MAX_KEY_SIZE];
> @@ -1171,6 +1178,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;
> + else
> + return -EINVAL;
> +}
> +
> /*
> * Only IPv4 subnet strings needs to be converted to plen
> * For IPv6 the subnet is already privided in plen format
> @@ -1197,14 +1216,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;
> +
> + output_str = (char *)calloc(INET6_ADDRSTRLEN * MAX_IP_ENTRIES,
> + sizeof(char));
Can we define INET6_ADDRSTRLEN * MAX_IP_ENTRIES as something like
OUTSTR_BUF_SZ or some such? Then it becomes more readable here and below.
> +
> + if (!output_str)
> + return -ENOMEM;
> +
> + memset(addr, 0, sizeof(addr));
> +
> + if (type == DNS) {
> + param_name = "dns";
> + } else if (type == GATEWAY) {
> + param_name = "gateway";
> + } else {
> + error = -EINVAL;
> + goto cleanup;
> + }
If you move the above check before you allocate memory for output_str, you
can return right away without doing a free().
> +
> + while (parse_ip_val_buffer(ip_string, &ip_offset, addr,
> + (MAX_IP_ADDR_SIZE * 2))) {
> + ip_ver = ip_version_check(addr);
> + if (ip_ver < 0)
> + continue;
> +
> + if ((ip_ver == IPV4 && ip_sec == IPV4) ||
> + (ip_ver == IPV6 && ip_sec == IPV6)) {
> + if (((INET6_ADDRSTRLEN * MAX_IP_ENTRIES) - strlen(output_str)) >
> + (strlen(addr))) {
> + strcat(output_str, addr);
> + strcat(output_str, ",");
Your bound check does not take into consideration one additional character
(the ","). It should be
(INET6_ADDRSTRLEN * MAX_IP_ENTRIES) - strlen(output_str) > strlen(addr) + 1
> + }
> + memset(addr, 0, sizeof(addr));
> +
> + } else {
> + memset(addr, 0, sizeof(addr));
if you do memset() at the beginning of the loop, you do not need to do
this separately for both branches. Plus there would be no need to do this
at the beginning of the function as well.
So you could do something like:
while(1) {
memset(addr ...);
if (!parse_ip_val_buffer(...))
break;
...
}
> + continue;
> + }
> + }
> +
> + if (strlen(output_str)) {
// remove the last comma character
> + output_str[strlen(output_str) - 1] = '\0';
> + error = fprintf(f, "%s=%s\n", param_name, output_str);
> + if (error < 0)
> + goto cleanup;
You need to free memory regardless of whether there is an error or not.
> + }
> +
> +cleanup:
> + free(output_str);
> + return error;
> +}
> +
> static int process_ip_string_nm(FILE *f, char *ip_string, char *subnet,
> - int is_ipv6)
> + int ip_sec)
> {
> char addr[INET6_ADDRSTRLEN];
> char subnet_addr[INET6_ADDRSTRLEN];
> int error, i = 0;
> int ip_offset = 0, subnet_offset = 0;
> - int plen;
> + int plen, ip_ver;
>
> memset(addr, 0, sizeof(addr));
> memset(subnet_addr, 0, sizeof(subnet_addr));
> @@ -1216,10 +1292,16 @@ static int process_ip_string_nm(FILE *f, char *ip_string, char *subnet,
> subnet_addr,
> (MAX_IP_ADDR_SIZE *
> 2))) {
> - if (!is_ipv6)
> + ip_ver = ip_version_check(addr);
> + if (ip_ver < 0)
> + continue;
> +
> + if (ip_ver == IPV4 && ip_sec == IPV4)
> plen = kvp_subnet_to_plen((char *)subnet_addr);
> - else
> + else if (ip_ver == IPV6 && ip_sec == IPV6)
> plen = atoi(subnet_addr);
> + else
> + continue;
>
> if (plen < 0)
> return plen;
> @@ -1238,12 +1320,11 @@ static int process_ip_string_nm(FILE *f, char *ip_string, char *subnet,
>
> static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
> {
> - int error = 0;
> + int error = 0, ip_type;
> char if_filename[PATH_MAX];
> char nm_filename[PATH_MAX];
> FILE *ifcfg_file, *nmfile;
> char cmd[PATH_MAX];
> - int is_ipv6 = 0;
> char *mac_addr;
> int str_len;
>
> @@ -1421,52 +1502,62 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
> if (error)
> goto setval_error;
>
> - if (new_val->addr_family & ADDR_FAMILY_IPV6) {
> - error = fprintf(nmfile, "\n[ipv6]\n");
> - if (error < 0)
> - goto setval_error;
> - is_ipv6 = 1;
> - } else {
> - error = fprintf(nmfile, "\n[ipv4]\n");
> - if (error < 0)
> - goto setval_error;
> - }
> -
> /*
> - * Now we populate the keyfile format
> + * The keyfile format expects the IPv6 and IPv4 configuration in
> + * different sections. Therefore we iterate through the list twice,
> + * once to populate the IPv4 section and the next time for IPv6
> */
> + ip_type = IPV4;
> + do {
> + if (ip_type == IPV4) {
> + error = fprintf(nmfile, "\n[ipv4]\n");
> + if (error < 0)
> + goto setval_error;
> + } else {
> + error = fprintf(nmfile, "\n[ipv6]\n");
> + if (error < 0)
> + goto setval_error;
> + }
>
> - if (new_val->dhcp_enabled) {
> - error = kvp_write_file(nmfile, "method", "", "auto");
> - if (error < 0)
> - goto setval_error;
> - } else {
> - error = kvp_write_file(nmfile, "method", "", "manual");
> + /*
> + * Now we populate the keyfile format
> + */
> +
> + if (new_val->dhcp_enabled) {
> + error = kvp_write_file(nmfile, "method", "", "auto");
> + if (error < 0)
> + goto setval_error;
> + } else {
> + error = kvp_write_file(nmfile, "method", "", "manual");
> + if (error < 0)
> + goto setval_error;
> + }
> +
> + /*
> + * Write the configuration for ipaddress, netmask, gateway and
> + * name services
> + */
> + error = process_ip_string_nm(nmfile, (char *)new_val->ip_addr,
> + (char *)new_val->sub_net,
> + ip_type);
> if (error < 0)
> goto setval_error;
> - }
>
> - /*
> - * Write the configuration for ipaddress, netmask, gateway and
> - * name services
> - */
> - error = process_ip_string_nm(nmfile, (char *)new_val->ip_addr,
> - (char *)new_val->sub_net, is_ipv6);
> - if (error < 0)
> - goto setval_error;
> -
> - /* we do not want ipv4 addresses in ipv6 section and vice versa */
> - if (is_ipv6 != is_ipv4((char *)new_val->gate_way)) {
> - error = fprintf(nmfile, "gateway=%s\n", (char *)new_val->gate_way);
> + error = process_dns_gateway_nm(nmfile,
> + (char *)new_val->gate_way,
> + GATEWAY, ip_type);
> if (error < 0)
> goto setval_error;
> - }
>
> - if (is_ipv6 != is_ipv4((char *)new_val->dns_addr)) {
> - error = fprintf(nmfile, "dns=%s\n", (char *)new_val->dns_addr);
> + error = process_dns_gateway_nm(nmfile,
> + (char *)new_val->dns_addr, DNS,
> + ip_type);
> if (error < 0)
> goto setval_error;
> - }
> +
> + ip_type++;
> + } while (ip_type < IP_TYPE_MAX);
> +
> fclose(nmfile);
> fclose(ifcfg_file);
>
> --
> 2.34.1
>
>
On 3/12/2024 5:38 AM, Shradha Gupta wrote:
> If the network configuration strings are passed as a combination of IPv and
*IPv4*
> IPv6 addresses, the current KVP daemon doesnot handle it for the keyfile
*does not/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.
>
> Built-on: Rhel9
> Tested-on: Rhel9(IPv4 only, IPv6 only, IPv4 and IPv6 combination)
As mentioned by Jakub[1], what value does this information provide?
Please follow Haiyang's suggestion [2] and put SKU and test information, or just
skip it.
[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/DM6PR21MB14817597567C638DEF020FE3CA202@DM6PR21MB1481.namprd21.prod.outlook.com/
> Signed-off-by: Shradha Gupta <[email protected]>
> ---
> Changes in v2
> * Use calloc to avoid initialization later
> * Return standard error codes
> * Free the output_str pointer on completion
> * Add out-of bound checks while writing to buffers
> ---
> tools/hv/hv_kvp_daemon.c | 173 +++++++++++++++++++++++++++++----------
> 1 file changed, 132 insertions(+), 41 deletions(-)
>
> diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
> index 318e2dad27e0..ae65be004eb1 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,7 @@ static struct utsname uts_buf;
>
> #define MAX_FILE_NAME 100
> #define ENTRIES_PER_BLOCK 50
> +#define MAX_IP_ENTRIES 64
Is this a limitation defined by hv_kvp? If so, is it possible it may change in a later
version? A comment would help here
>
> struct kvp_record {
> char key[HV_KVP_EXCHANGE_MAX_KEY_SIZE];
> @@ -1171,6 +1178,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;
You can skip the else here...
> + else
> + return -EINVAL;
..and you can skip the else here as well and just return -EINVAL
> +}
> +
> /*
> * Only IPv4 subnet strings needs to be converted to plen
> * For IPv6 the subnet is already privided in plen format
> @@ -1197,14 +1216,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;
> +
> + output_str = (char *)calloc(INET6_ADDRSTRLEN * MAX_IP_ENTRIES,
> + sizeof(char));
> +
> + if (!output_str)
> + return -ENOMEM;
> +
> + memset(addr, 0, sizeof(addr));
> +
> + if (type == DNS) {
> + param_name = "dns";
> + } else if (type == GATEWAY) {
> + param_name = "gateway";
> + } else {
> + error = -EINVAL;
> + goto cleanup;
> + }
> +
> + while (parse_ip_val_buffer(ip_string, &ip_offset, addr,
> + (MAX_IP_ADDR_SIZE * 2))) {
> + ip_ver = ip_version_check(addr);
> + if (ip_ver < 0)
> + continue;
> +
> + if ((ip_ver == IPV4 && ip_sec == IPV4) ||
> + (ip_ver == IPV6 && ip_sec == IPV6)) {
> + if (((INET6_ADDRSTRLEN * MAX_IP_ENTRIES) - strlen(output_str)) >
> + (strlen(addr))) {
> + strcat(output_str, addr);
> + strcat(output_str, ",");
Prefer strncat() here
> + }
> + memset(addr, 0, sizeof(addr));
> +
> + } else {
> + memset(addr, 0, sizeof(addr));
> + continue;
> + }
> + }
> +
> + if (strlen(output_str)) {
> + output_str[strlen(output_str) - 1] = '\0';
> + error = fprintf(f, "%s=%s\n", param_name, output_str);
> + if (error < 0)
> + goto cleanup;
> + }
> +
> +cleanup:
> + free(output_str);
> + return error;
> +}
> +
> static int process_ip_string_nm(FILE *f, char *ip_string, char *subnet,
> - int is_ipv6)
> + int ip_sec)
> {
> char addr[INET6_ADDRSTRLEN];
> char subnet_addr[INET6_ADDRSTRLEN];
> int error, i = 0;
> int ip_offset = 0, subnet_offset = 0;
> - int plen;
> + int plen, ip_ver;
>
> memset(addr, 0, sizeof(addr));
> memset(subnet_addr, 0, sizeof(subnet_addr));
> @@ -1216,10 +1292,16 @@ static int process_ip_string_nm(FILE *f, char *ip_string, char *subnet,
> subnet_addr,
> (MAX_IP_ADDR_SIZE *
> 2))) {
> - if (!is_ipv6)
> + ip_ver = ip_version_check(addr);
> + if (ip_ver < 0)
> + continue;
> +
> + if (ip_ver == IPV4 && ip_sec == IPV4)
> plen = kvp_subnet_to_plen((char *)subnet_addr);
> - else
> + else if (ip_ver == IPV6 && ip_sec == IPV6)
> plen = atoi(subnet_addr);
> + else
> + continue;
>
> if (plen < 0)
> return plen;
> @@ -1238,12 +1320,11 @@ static int process_ip_string_nm(FILE *f, char *ip_string, char *subnet,
>
> static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
> {
> - int error = 0;
> + int error = 0, ip_type;
nit: Can we keep ip_ver through all the functions for consistency
<snip>
Thanks,
Easwar
On Tue, Mar 12, 2024 at 08:09:03PM +0530, Ani Sinha wrote:
>
>
> On Tue, 12 Mar 2024, Shradha Gupta wrote:
>
> > If the network configuration strings are passed as a combination of IPv and
> > IPv6 addresses, the current KVP daemon doesnot handle it for the keyfile
> > 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.
> >
> > Built-on: Rhel9
> > Tested-on: Rhel9(IPv4 only, IPv6 only, IPv4 and IPv6 combination)
> > Signed-off-by: Shradha Gupta <[email protected]>
> > ---
> > Changes in v2
> > * Use calloc to avoid initialization later
> > * Return standard error codes
> > * Free the output_str pointer on completion
> > * Add out-of bound checks while writing to buffers
> > ---
> > tools/hv/hv_kvp_daemon.c | 173 +++++++++++++++++++++++++++++----------
> > 1 file changed, 132 insertions(+), 41 deletions(-)
> >
> > diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
> > index 318e2dad27e0..ae65be004eb1 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,7 @@ static struct utsname uts_buf;
> >
> > #define MAX_FILE_NAME 100
> > #define ENTRIES_PER_BLOCK 50
> > +#define MAX_IP_ENTRIES 64
> >
> > struct kvp_record {
> > char key[HV_KVP_EXCHANGE_MAX_KEY_SIZE];
> > @@ -1171,6 +1178,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;
> > + else
> > + return -EINVAL;
> > +}
> > +
> > /*
> > * Only IPv4 subnet strings needs to be converted to plen
> > * For IPv6 the subnet is already privided in plen format
> > @@ -1197,14 +1216,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;
> > +
> > + output_str = (char *)calloc(INET6_ADDRSTRLEN * MAX_IP_ENTRIES,
> > + sizeof(char));
>
> Can we define INET6_ADDRSTRLEN * MAX_IP_ENTRIES as something like
> OUTSTR_BUF_SZ or some such? Then it becomes more readable here and below.
sure, that makes sense.
>
> > +
> > + if (!output_str)
> > + return -ENOMEM;
> > +
> > + memset(addr, 0, sizeof(addr));
>
>
> > +
> > + if (type == DNS) {
> > + param_name = "dns";
> > + } else if (type == GATEWAY) {
> > + param_name = "gateway";
> > + } else {
> > + error = -EINVAL;
> > + goto cleanup;
> > + }
>
> If you move the above check before you allocate memory for output_str, you
> can return right away without doing a free().
Right, I'll do that
>
> > +
> > + while (parse_ip_val_buffer(ip_string, &ip_offset, addr,
> > + (MAX_IP_ADDR_SIZE * 2))) {
> > + ip_ver = ip_version_check(addr);
> > + if (ip_ver < 0)
> > + continue;
> > +
> > + if ((ip_ver == IPV4 && ip_sec == IPV4) ||
> > + (ip_ver == IPV6 && ip_sec == IPV6)) {
> > + if (((INET6_ADDRSTRLEN * MAX_IP_ENTRIES) - strlen(output_str)) >
> > + (strlen(addr))) {
> > + strcat(output_str, addr);
> > + strcat(output_str, ",");
>
> Your bound check does not take into consideration one additional character
> (the ","). It should be
>
> (INET6_ADDRSTRLEN * MAX_IP_ENTRIES) - strlen(output_str) > strlen(addr) + 1
>
> > + }
> > + memset(addr, 0, sizeof(addr));
> > +
> > + } else {
> > + memset(addr, 0, sizeof(addr));
>
> if you do memset() at the beginning of the loop, you do not need to do
> this separately for both branches. Plus there would be no need to do this
> at the beginning of the function as well.
> So you could do something like:
>
> while(1) {
> memset(addr ...);
> if (!parse_ip_val_buffer(...))
> break;
> ...
> }
makes sense.
>
>
> > + continue;
> > + }
> > + }
> > +
> > + if (strlen(output_str)) {
>
> // remove the last comma character
>
> > + output_str[strlen(output_str) - 1] = '\0';
> > + error = fprintf(f, "%s=%s\n", param_name, output_str);
> > + if (error < 0)
> > + goto cleanup;
>
> You need to free memory regardless of whether there is an error or not.
Right, no need for the above check. Thanks
>
> > + }
> > +
> > +cleanup:
> > + free(output_str);
> > + return error;
> > +}
> > +
> > static int process_ip_string_nm(FILE *f, char *ip_string, char *subnet,
> > - int is_ipv6)
> > + int ip_sec)
> > {
> > char addr[INET6_ADDRSTRLEN];
> > char subnet_addr[INET6_ADDRSTRLEN];
> > int error, i = 0;
> > int ip_offset = 0, subnet_offset = 0;
> > - int plen;
> > + int plen, ip_ver;
> >
> > memset(addr, 0, sizeof(addr));
> > memset(subnet_addr, 0, sizeof(subnet_addr));
> > @@ -1216,10 +1292,16 @@ static int process_ip_string_nm(FILE *f, char *ip_string, char *subnet,
> > subnet_addr,
> > (MAX_IP_ADDR_SIZE *
> > 2))) {
> > - if (!is_ipv6)
> > + ip_ver = ip_version_check(addr);
> > + if (ip_ver < 0)
> > + continue;
> > +
> > + if (ip_ver == IPV4 && ip_sec == IPV4)
> > plen = kvp_subnet_to_plen((char *)subnet_addr);
> > - else
> > + else if (ip_ver == IPV6 && ip_sec == IPV6)
> > plen = atoi(subnet_addr);
> > + else
> > + continue;
> >
> > if (plen < 0)
> > return plen;
> > @@ -1238,12 +1320,11 @@ static int process_ip_string_nm(FILE *f, char *ip_string, char *subnet,
> >
> > static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
> > {
> > - int error = 0;
> > + int error = 0, ip_type;
> > char if_filename[PATH_MAX];
> > char nm_filename[PATH_MAX];
> > FILE *ifcfg_file, *nmfile;
> > char cmd[PATH_MAX];
> > - int is_ipv6 = 0;
> > char *mac_addr;
> > int str_len;
> >
> > @@ -1421,52 +1502,62 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
> > if (error)
> > goto setval_error;
> >
> > - if (new_val->addr_family & ADDR_FAMILY_IPV6) {
> > - error = fprintf(nmfile, "\n[ipv6]\n");
> > - if (error < 0)
> > - goto setval_error;
> > - is_ipv6 = 1;
> > - } else {
> > - error = fprintf(nmfile, "\n[ipv4]\n");
> > - if (error < 0)
> > - goto setval_error;
> > - }
> > -
> > /*
> > - * Now we populate the keyfile format
> > + * The keyfile format expects the IPv6 and IPv4 configuration in
> > + * different sections. Therefore we iterate through the list twice,
> > + * once to populate the IPv4 section and the next time for IPv6
> > */
> > + ip_type = IPV4;
> > + do {
> > + if (ip_type == IPV4) {
> > + error = fprintf(nmfile, "\n[ipv4]\n");
> > + if (error < 0)
> > + goto setval_error;
> > + } else {
> > + error = fprintf(nmfile, "\n[ipv6]\n");
> > + if (error < 0)
> > + goto setval_error;
> > + }
> >
> > - if (new_val->dhcp_enabled) {
> > - error = kvp_write_file(nmfile, "method", "", "auto");
> > - if (error < 0)
> > - goto setval_error;
> > - } else {
> > - error = kvp_write_file(nmfile, "method", "", "manual");
> > + /*
> > + * Now we populate the keyfile format
> > + */
> > +
> > + if (new_val->dhcp_enabled) {
> > + error = kvp_write_file(nmfile, "method", "", "auto");
> > + if (error < 0)
> > + goto setval_error;
> > + } else {
> > + error = kvp_write_file(nmfile, "method", "", "manual");
> > + if (error < 0)
> > + goto setval_error;
> > + }
> > +
> > + /*
> > + * Write the configuration for ipaddress, netmask, gateway and
> > + * name services
> > + */
> > + error = process_ip_string_nm(nmfile, (char *)new_val->ip_addr,
> > + (char *)new_val->sub_net,
> > + ip_type);
> > if (error < 0)
> > goto setval_error;
> > - }
> >
> > - /*
> > - * Write the configuration for ipaddress, netmask, gateway and
> > - * name services
> > - */
> > - error = process_ip_string_nm(nmfile, (char *)new_val->ip_addr,
> > - (char *)new_val->sub_net, is_ipv6);
> > - if (error < 0)
> > - goto setval_error;
> > -
> > - /* we do not want ipv4 addresses in ipv6 section and vice versa */
> > - if (is_ipv6 != is_ipv4((char *)new_val->gate_way)) {
> > - error = fprintf(nmfile, "gateway=%s\n", (char *)new_val->gate_way);
> > + error = process_dns_gateway_nm(nmfile,
> > + (char *)new_val->gate_way,
> > + GATEWAY, ip_type);
> > if (error < 0)
> > goto setval_error;
> > - }
> >
> > - if (is_ipv6 != is_ipv4((char *)new_val->dns_addr)) {
> > - error = fprintf(nmfile, "dns=%s\n", (char *)new_val->dns_addr);
> > + error = process_dns_gateway_nm(nmfile,
> > + (char *)new_val->dns_addr, DNS,
> > + ip_type);
> > if (error < 0)
> > goto setval_error;
> > - }
> > +
> > + ip_type++;
> > + } while (ip_type < IP_TYPE_MAX);
> > +
> > fclose(nmfile);
> > fclose(ifcfg_file);
> >
> > --
> > 2.34.1
> >
> >
On Tue, Mar 12, 2024 at 09:58:03AM -0700, Easwar Hariharan wrote:
> On 3/12/2024 5:38 AM, Shradha Gupta wrote:
> > If the network configuration strings are passed as a combination of IPv and
>
> *IPv4*
>
> > IPv6 addresses, the current KVP daemon doesnot handle it for the keyfile
> *does not/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.
> >
> > Built-on: Rhel9
> > Tested-on: Rhel9(IPv4 only, IPv6 only, IPv4 and IPv6 combination)
>
> As mentioned by Jakub[1], what value does this information provide?
> Please follow Haiyang's suggestion [2] and put SKU and test information, or just
> skip it.
>
> [1] https://lore.kernel.org/all/[email protected]/
> [2] https://lore.kernel.org/all/DM6PR21MB14817597567C638DEF020FE3CA202@DM6PR21MB1481.namprd21.prod.outlook.com/
Hi Easwar, unlike the other patch this one has details about the tests that were performed.
Since this is Hyper-v VMs specific, I could not add details around SKU or LISA tests(as it
could not be tested using LISA). In the last patch we had missed the IPv4, IPv6 combination
testing(which had some design issues). That's why I feel it is important to call it out in
this patch.
>
> > Signed-off-by: Shradha Gupta <[email protected]>
> > ---
> > Changes in v2
> > * Use calloc to avoid initialization later
> > * Return standard error codes
> > * Free the output_str pointer on completion
> > * Add out-of bound checks while writing to buffers
> > ---
> > tools/hv/hv_kvp_daemon.c | 173 +++++++++++++++++++++++++++++----------
> > 1 file changed, 132 insertions(+), 41 deletions(-)
> >
> > diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
> > index 318e2dad27e0..ae65be004eb1 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,7 @@ static struct utsname uts_buf;
> >
> > #define MAX_FILE_NAME 100
> > #define ENTRIES_PER_BLOCK 50
> > +#define MAX_IP_ENTRIES 64
>
> Is this a limitation defined by hv_kvp? If so, is it possible it may change in a later
> version? A comment would help here
Sure, would update accordingly
>
> >
> > struct kvp_record {
> > char key[HV_KVP_EXCHANGE_MAX_KEY_SIZE];
> > @@ -1171,6 +1178,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;
>
> You can skip the else here...
>
> > + else
> > + return -EINVAL;
>
> ...and you can skip the else here as well and just return -EINVAL
right, will change this in the next version.
>
> > +}
> > +
> > /*
> > * Only IPv4 subnet strings needs to be converted to plen
> > * For IPv6 the subnet is already privided in plen format
> > @@ -1197,14 +1216,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;
> > +
> > + output_str = (char *)calloc(INET6_ADDRSTRLEN * MAX_IP_ENTRIES,
> > + sizeof(char));
> > +
> > + if (!output_str)
> > + return -ENOMEM;
> > +
> > + memset(addr, 0, sizeof(addr));
> > +
> > + if (type == DNS) {
> > + param_name = "dns";
> > + } else if (type == GATEWAY) {
> > + param_name = "gateway";
> > + } else {
> > + error = -EINVAL;
> > + goto cleanup;
> > + }
> > +
> > + while (parse_ip_val_buffer(ip_string, &ip_offset, addr,
> > + (MAX_IP_ADDR_SIZE * 2))) {
> > + ip_ver = ip_version_check(addr);
> > + if (ip_ver < 0)
> > + continue;
> > +
> > + if ((ip_ver == IPV4 && ip_sec == IPV4) ||
> > + (ip_ver == IPV6 && ip_sec == IPV6)) {
> > + if (((INET6_ADDRSTRLEN * MAX_IP_ENTRIES) - strlen(output_str)) >
> > + (strlen(addr))) {
> > + strcat(output_str, addr);
> > + strcat(output_str, ",");
>
> Prefer strncat() here
>
> > + }
> > + memset(addr, 0, sizeof(addr));
> > +
> > + } else {
> > + memset(addr, 0, sizeof(addr));
> > + continue;
> > + }
> > + }
> > +
> > + if (strlen(output_str)) {
> > + output_str[strlen(output_str) - 1] = '\0';
> > + error = fprintf(f, "%s=%s\n", param_name, output_str);
> > + if (error < 0)
> > + goto cleanup;
> > + }
> > +
> > +cleanup:
> > + free(output_str);
> > + return error;
> > +}
> > +
> > static int process_ip_string_nm(FILE *f, char *ip_string, char *subnet,
> > - int is_ipv6)
> > + int ip_sec)
> > {
> > char addr[INET6_ADDRSTRLEN];
> > char subnet_addr[INET6_ADDRSTRLEN];
> > int error, i = 0;
> > int ip_offset = 0, subnet_offset = 0;
> > - int plen;
> > + int plen, ip_ver;
> >
> > memset(addr, 0, sizeof(addr));
> > memset(subnet_addr, 0, sizeof(subnet_addr));
> > @@ -1216,10 +1292,16 @@ static int process_ip_string_nm(FILE *f, char *ip_string, char *subnet,
> > subnet_addr,
> > (MAX_IP_ADDR_SIZE *
> > 2))) {
> > - if (!is_ipv6)
> > + ip_ver = ip_version_check(addr);
> > + if (ip_ver < 0)
> > + continue;
> > +
> > + if (ip_ver == IPV4 && ip_sec == IPV4)
> > plen = kvp_subnet_to_plen((char *)subnet_addr);
> > - else
> > + else if (ip_ver == IPV6 && ip_sec == IPV6)
> > plen = atoi(subnet_addr);
> > + else
> > + continue;
> >
> > if (plen < 0)
> > return plen;
> > @@ -1238,12 +1320,11 @@ static int process_ip_string_nm(FILE *f, char *ip_string, char *subnet,
> >
> > static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
> > {
> > - int error = 0;
> > + int error = 0, ip_type;
>
> nit: Can we keep ip_ver through all the functions for consistency
sure.
>
> <snip>
>
> Thanks,
> Easwar
On Tue, Mar 12, 2024 at 10:22:12PM -0700, Shradha Gupta wrote:
> On Tue, Mar 12, 2024 at 09:58:03AM -0700, Easwar Hariharan wrote:
> > On 3/12/2024 5:38 AM, Shradha Gupta wrote:
> > > If the network configuration strings are passed as a combination of IPv and
> >
> > *IPv4*
> >
> > > IPv6 addresses, the current KVP daemon doesnot handle it for the keyfile
> > *does not/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.
> > >
> > > Built-on: Rhel9
> > > Tested-on: Rhel9(IPv4 only, IPv6 only, IPv4 and IPv6 combination)
> >
> > As mentioned by Jakub[1], what value does this information provide?
> > Please follow Haiyang's suggestion [2] and put SKU and test information, or just
> > skip it.
> >
> > [1] https://lore.kernel.org/all/[email protected]/
> > [2] https://lore.kernel.org/all/DM6PR21MB14817597567C638DEF020FE3CA202@DM6PR21MB1481.namprd21.prod.outlook.com/
> Hi Easwar, unlike the other patch this one has details about the tests that were performed.
> Since this is Hyper-v VMs specific, I could not add details around SKU or LISA tests(as it
> could not be tested using LISA). In the last patch we had missed the IPv4, IPv6 combination
> testing(which had some design issues). That's why I feel it is important to call it out in
> this patch.
> >
> > > Signed-off-by: Shradha Gupta <[email protected]>
> > > ---
> > > Changes in v2
> > > * Use calloc to avoid initialization later
> > > * Return standard error codes
> > > * Free the output_str pointer on completion
> > > * Add out-of bound checks while writing to buffers
> > > ---
> > > tools/hv/hv_kvp_daemon.c | 173 +++++++++++++++++++++++++++++----------
> > > 1 file changed, 132 insertions(+), 41 deletions(-)
> > >
> > > diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
> > > index 318e2dad27e0..ae65be004eb1 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,7 @@ static struct utsname uts_buf;
> > >
> > > #define MAX_FILE_NAME 100
> > > #define ENTRIES_PER_BLOCK 50
> > > +#define MAX_IP_ENTRIES 64
> >
> > Is this a limitation defined by hv_kvp? If so, is it possible it may change in a later
> > version? A comment would help here
> Sure, would update accordingly
> >
> > >
> > > struct kvp_record {
> > > char key[HV_KVP_EXCHANGE_MAX_KEY_SIZE];
> > > @@ -1171,6 +1178,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;
> >
> > You can skip the else here...
> >
> > > + else
> > > + return -EINVAL;
> >
> > ...and you can skip the else here as well and just return -EINVAL
> right, will change this in the next version.
> >
> > > +}
> > > +
> > > /*
> > > * Only IPv4 subnet strings needs to be converted to plen
> > > * For IPv6 the subnet is already privided in plen format
> > > @@ -1197,14 +1216,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;
> > > +
> > > + output_str = (char *)calloc(INET6_ADDRSTRLEN * MAX_IP_ENTRIES,
> > > + sizeof(char));
> > > +
> > > + if (!output_str)
> > > + return -ENOMEM;
> > > +
> > > + memset(addr, 0, sizeof(addr));
> > > +
> > > + if (type == DNS) {
> > > + param_name = "dns";
> > > + } else if (type == GATEWAY) {
> > > + param_name = "gateway";
> > > + } else {
> > > + error = -EINVAL;
> > > + goto cleanup;
> > > + }
> > > +
> > > + while (parse_ip_val_buffer(ip_string, &ip_offset, addr,
> > > + (MAX_IP_ADDR_SIZE * 2))) {
> > > + ip_ver = ip_version_check(addr);
> > > + if (ip_ver < 0)
> > > + continue;
> > > +
> > > + if ((ip_ver == IPV4 && ip_sec == IPV4) ||
> > > + (ip_ver == IPV6 && ip_sec == IPV6)) {
> > > + if (((INET6_ADDRSTRLEN * MAX_IP_ENTRIES) - strlen(output_str)) >
> > > + (strlen(addr))) {
> > > + strcat(output_str, addr);
> > > + strcat(output_str, ",");
> >
> > Prefer strncat() here
Is this needed with the bound check above. I am trying to keep parity with the rest of the
file.
> >
> > > + }
> > > + memset(addr, 0, sizeof(addr));
> > > +
> > > + } else {
> > > + memset(addr, 0, sizeof(addr));
> > > + continue;
> > > + }
> > > + }
> > > +
> > > + if (strlen(output_str)) {
> > > + output_str[strlen(output_str) - 1] = '\0';
> > > + error = fprintf(f, "%s=%s\n", param_name, output_str);
> > > + if (error < 0)
> > > + goto cleanup;
> > > + }
> > > +
> > > +cleanup:
> > > + free(output_str);
> > > + return error;
> > > +}
> > > +
> > > static int process_ip_string_nm(FILE *f, char *ip_string, char *subnet,
> > > - int is_ipv6)
> > > + int ip_sec)
> > > {
> > > char addr[INET6_ADDRSTRLEN];
> > > char subnet_addr[INET6_ADDRSTRLEN];
> > > int error, i = 0;
> > > int ip_offset = 0, subnet_offset = 0;
> > > - int plen;
> > > + int plen, ip_ver;
> > >
> > > memset(addr, 0, sizeof(addr));
> > > memset(subnet_addr, 0, sizeof(subnet_addr));
> > > @@ -1216,10 +1292,16 @@ static int process_ip_string_nm(FILE *f, char *ip_string, char *subnet,
> > > subnet_addr,
> > > (MAX_IP_ADDR_SIZE *
> > > 2))) {
> > > - if (!is_ipv6)
> > > + ip_ver = ip_version_check(addr);
> > > + if (ip_ver < 0)
> > > + continue;
> > > +
> > > + if (ip_ver == IPV4 && ip_sec == IPV4)
> > > plen = kvp_subnet_to_plen((char *)subnet_addr);
> > > - else
> > > + else if (ip_ver == IPV6 && ip_sec == IPV6)
> > > plen = atoi(subnet_addr);
> > > + else
> > > + continue;
> > >
> > > if (plen < 0)
> > > return plen;
> > > @@ -1238,12 +1320,11 @@ static int process_ip_string_nm(FILE *f, char *ip_string, char *subnet,
> > >
> > > static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
> > > {
> > > - int error = 0;
> > > + int error = 0, ip_type;
> >
> > nit: Can we keep ip_ver through all the functions for consistency
> sure.
> >
> > <snip>
> >
> > Thanks,
> > Easwar
On 3/15/2024 7:09 AM, Shradha Gupta wrote:
<snip>
>>>> }
>>>>
>>>> +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;
>>>> +
>>>> + output_str = (char *)calloc(INET6_ADDRSTRLEN * MAX_IP_ENTRIES,
>>>> + sizeof(char));
>>>> +
>>>> + if (!output_str)
>>>> + return -ENOMEM;
>>>> +
>>>> + memset(addr, 0, sizeof(addr));
>>>> +
>>>> + if (type == DNS) {
>>>> + param_name = "dns";
>>>> + } else if (type == GATEWAY) {
>>>> + param_name = "gateway";
>>>> + } else {
>>>> + error = -EINVAL;
>>>> + goto cleanup;
>>>> + }
>>>> +
>>>> + while (parse_ip_val_buffer(ip_string, &ip_offset, addr,
>>>> + (MAX_IP_ADDR_SIZE * 2))) {
>>>> + ip_ver = ip_version_check(addr);
>>>> + if (ip_ver < 0)
>>>> + continue;
>>>> +
>>>> + if ((ip_ver == IPV4 && ip_sec == IPV4) ||
>>>> + (ip_ver == IPV6 && ip_sec == IPV6)) {
>>>> + if (((INET6_ADDRSTRLEN * MAX_IP_ENTRIES) - strlen(output_str)) >
>>>> + (strlen(addr))) {
>>>> + strcat(output_str, addr);
>>>> + strcat(output_str, ",");
>>>
>>> Prefer strncat() here
> Is this needed with the bound check above. I am trying to keep parity with the rest of the
> file.
>>>
<snip>
I missed this earlier because my comment was more of a general best practice comment.
Note that in the worst case where your bounds check (INET6_ADDRSTRLEN*MAX_IP_ENTRIES) - strlen(output_str) equals (strlen(addr) + 1),
you will be adding strlen(addr)+1(","), and end up with no ASCII NUL '\0' delimiter.
If you're going to rely on the bounds check to ensure correctness, you'll need to correct that. Generally speaking, strncat would still
be helpful in case the bounds check changes in the future.
Thanks,
Easwar
On Fri, Mar 15, 2024 at 09:56:22AM -0700, Easwar Hariharan wrote:
> On 3/15/2024 7:09 AM, Shradha Gupta wrote:
>
> <snip>
>
> >>>> }
> >>>>
> >>>> +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;
> >>>> +
> >>>> + output_str = (char *)calloc(INET6_ADDRSTRLEN * MAX_IP_ENTRIES,
> >>>> + sizeof(char));
> >>>> +
> >>>> + if (!output_str)
> >>>> + return -ENOMEM;
> >>>> +
> >>>> + memset(addr, 0, sizeof(addr));
> >>>> +
> >>>> + if (type == DNS) {
> >>>> + param_name = "dns";
> >>>> + } else if (type == GATEWAY) {
> >>>> + param_name = "gateway";
> >>>> + } else {
> >>>> + error = -EINVAL;
> >>>> + goto cleanup;
> >>>> + }
> >>>> +
> >>>> + while (parse_ip_val_buffer(ip_string, &ip_offset, addr,
> >>>> + (MAX_IP_ADDR_SIZE * 2))) {
> >>>> + ip_ver = ip_version_check(addr);
> >>>> + if (ip_ver < 0)
> >>>> + continue;
> >>>> +
> >>>> + if ((ip_ver == IPV4 && ip_sec == IPV4) ||
> >>>> + (ip_ver == IPV6 && ip_sec == IPV6)) {
> >>>> + if (((INET6_ADDRSTRLEN * MAX_IP_ENTRIES) - strlen(output_str)) >
> >>>> + (strlen(addr))) {
> >>>> + strcat(output_str, addr);
> >>>> + strcat(output_str, ",");
> >>>
> >>> Prefer strncat() here
> > Is this needed with the bound check above. I am trying to keep parity with the rest of the
> > file.
> >>>
> <snip>
>
> I missed this earlier because my comment was more of a general best practice comment.
>
> Note that in the worst case where your bounds check (INET6_ADDRSTRLEN*MAX_IP_ENTRIES) - strlen(output_str) equals (strlen(addr) + 1),
> you will be adding strlen(addr)+1(","), and end up with no ASCII NUL '\0' delimiter.
>
> If you're going to rely on the bounds check to ensure correctness, you'll need to correct that. Generally speaking, strncat would still
> be helpful in case the bounds check changes in the future.
>
> Thanks,
> Easwar
got it. Thanks.