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.
Testcases ran:Rhel 9, Hyper-V VMs
(IPv4 only, IPv6 only, IPv4 and IPv6 combination)
Signed-off-by: Shradha Gupta <[email protected]>
---
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));
+
+ 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';
+ error = fprintf(f, "%s=%s\n", param_name, output_str);
+ }
+
+ 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 +1296,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 +1324,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_ver;
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 +1506,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_ver = IPV4;
+ do {
+ if (ip_ver == 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_ver);
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_ver);
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_ver);
if (error < 0)
goto setval_error;
- }
+
+ ip_ver++;
+ } while (ip_ver < IP_TYPE_MAX);
+
fclose(nmfile);
fclose(ifcfg_file);
--
2.34.1
> On 18-Mar-2024, at 08:15, Shradha Gupta <[email protected]> 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.
>
> Testcases ran:Rhel 9, Hyper-V VMs
> (IPv4 only, IPv6 only, IPv4 and IPv6 combination)
> Signed-off-by: Shradha Gupta <[email protected]>
> ---
> 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));
^^^^^^^^^^^^^^^^^^^^^^^^
Is this still needed?
> +
> + 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, ",",
Please see man page for strncat(). Why is the third parameter simply not strlen(addr)?
> + OUTSTR_BUF_SIZE - strlen(output_str));
> + }
> + } else {
> + continue;
> + }
> + }
> +
> + if (strlen(output_str)) {
Please add the comment I mentioned in the previous version as to why you are doing this.
> + output_str[strlen(output_str) - 1] = '\0';
> + error = fprintf(f, "%s=%s\n", param_name, output_str);
> + }
> +
> + 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 +1296,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 +1324,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_ver;
> 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 +1506,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_ver = IPV4;
> + do {
> + if (ip_ver == 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_ver);
> 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_ver);
> 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_ver);
> if (error < 0)
> goto setval_error;
> - }
> +
> + ip_ver++;
> + } while (ip_ver < IP_TYPE_MAX);
> +
> fclose(nmfile);
> fclose(ifcfg_file);
>
> --
> 2.34.1
>
> On 18-Mar-2024, at 09:51, Ani Sinha <[email protected]> wrote:
>
>
>
>> On 18-Mar-2024, at 08:15, Shradha Gupta <[email protected]> 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.
>>
>> Testcases ran:Rhel 9, Hyper-V VMs
>> (IPv4 only, IPv6 only, IPv4 and IPv6 combination)
>> Signed-off-by: Shradha Gupta <[email protected]>
>> ---
>> 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));
>
> ^^^^^^^^^^^^^^^^^^^^^^^^
> Is this still needed?
>
>> +
>> + 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, ",",
>
> Please see man page for strncat(). Why is the third parameter simply not strlen(addr)?
I see what you are trying to do. You are doing a bound check here. In that case, should this be
OUTSTR_BUF_SIZE - strlen(output_str) - 1 to accommodate the terminating NULL? Man page says
> it will use at most n bytes from src
So n bytes from addr and one extra at the end for NULL termination since strncat () will add a terminating NULL byte in the end.
>
>> + OUTSTR_BUF_SIZE - strlen(output_str));
>
>
>
>> + }
>> + } else {
>> + continue;
>> + }
>> + }
>> +
>> + if (strlen(output_str)) {
>
> Please add the comment I mentioned in the previous version as to why you are doing this.
>
>> + output_str[strlen(output_str) - 1] = '\0';
>> + error = fprintf(f, "%s=%s\n", param_name, output_str);
>> + }
>> +
>> + 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 +1296,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 +1324,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_ver;
>> 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 +1506,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_ver = IPV4;
>> + do {
>> + if (ip_ver == 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_ver);
>> 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_ver);
>> 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_ver);
>> if (error < 0)
>> goto setval_error;
>> - }
>> +
>> + ip_ver++;
>> + } while (ip_ver < IP_TYPE_MAX);
>> +
>> fclose(nmfile);
>> fclose(ifcfg_file);
>>
>> --
>> 2.34.1
On Mon, Mar 18, 2024 at 10:26:49AM +0530, Ani Sinha wrote:
>
>
> > On 18-Mar-2024, at 09:51, Ani Sinha <[email protected]> wrote:
> >
> >
> >
> >> On 18-Mar-2024, at 08:15, Shradha Gupta <[email protected]> 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.
> >>
> >> Testcases ran:Rhel 9, Hyper-V VMs
> >> (IPv4 only, IPv6 only, IPv4 and IPv6 combination)
> >> Signed-off-by: Shradha Gupta <[email protected]>
> >> ---
> >> 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));
> >
> > ^^^^^^^^^^^^^^^^^^^^^^^^
> > Is this still needed?
Nope, I'll get that.
> >
> >> +
> >> + 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, ",",
> >
> > Please see man page for strncat(). Why is the third parameter simply not strlen(addr)?
>
> I see what you are trying to do. You are doing a bound check here. In that case, should this be
> OUTSTR_BUF_SIZE - strlen(output_str) - 1 to accommodate the terminating NULL? Man page says
>
> > it will use at most n bytes from src
>
> So n bytes from addr and one extra at the end for NULL termination since strncat () will add a terminating NULL byte in the end.
sure, that sounds right. Will get this too.
>
> >
> >> + OUTSTR_BUF_SIZE - strlen(output_str));
> >
> >
> >
> >> + }
> >> + } else {
> >> + continue;
> >> + }
> >> + }
> >> +
> >> + if (strlen(output_str)) {
> >
> > Please add the comment I mentioned in the previous version as to why you are doing this.
sure. Thanks
> >
> >> + output_str[strlen(output_str) - 1] = '\0';
> >> + error = fprintf(f, "%s=%s\n", param_name, output_str);
> >> + }
> >> +
> >> + 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 +1296,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 +1324,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_ver;
> >> 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 +1506,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_ver = IPV4;
> >> + do {
> >> + if (ip_ver == 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_ver);
> >> 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_ver);
> >> 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_ver);
> >> if (error < 0)
> >> goto setval_error;
> >> - }
> >> +
> >> + ip_ver++;
> >> + } while (ip_ver < IP_TYPE_MAX);
> >> +
> >> fclose(nmfile);
> >> fclose(ifcfg_file);
> >>
> >> --
> >> 2.34.1
>
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 <[email protected]>
> ---
> 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>
> On 18 Mar 2024, at 21:49, Easwar Hariharan <[email protected]> wrote:
>
> 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 <[email protected]>
>> ---
>> 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'.
If I understand this correctly, this code simply eliminates the extra “,” character in the end. Therefore it is needed.
Since it is not obvious, in the previous review and before, I asked the author to add a comment to explain this clearly.
> 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>
On 3/18/2024 10:12 AM, Ani Sinha wrote:
>
>
<snip>
>>> + }
>>> +
>>> + 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'.
>
> If I understand this correctly, this code simply eliminates the extra “,” character in the end. Therefore it is needed.
> Since it is not obvious, in the previous review and before, I asked the author to add a comment to explain this clearly.
>
>> 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.
>>
Ah, great, that makes sense. I did see that it was destroying data but didn't spend enough time to think through
what data it was destroying, and if that was a feature or a bug. Thanks for calling it out!
- Easwar
On Mon, Mar 18, 2024 at 10:16:59AM -0700, Easwar Hariharan wrote:
> On 3/18/2024 10:12 AM, Ani Sinha wrote:
> >
> >
> <snip>
>
> >>> + }
> >>> +
> >>> + 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'.
> >
> > If I understand this correctly, this code simply eliminates the extra ???,??? character in the end. Therefore it is needed.
> > Since it is not obvious, in the previous review and before, I asked the author to add a comment to explain this clearly.
> >
> >> 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.
> >>
>
> Ah, great, that makes sense. I did see that it was destroying data but didn't spend enough time to think through
> what data it was destroying, and if that was a feature or a bug. Thanks for calling it out!
>
> - Easwar
>
>
Thanks Ani, Easwar for all the comments. Apologies for not addressing some of the comments from the previous
version's review. For some reason my editor did not highlight it as expected. I will get them all with a new
version. Thanks.