Re: [PATCH v5] hv/hv_kvp_daemon: Handle IPv4 and Ipv6 combination for keyfile format

[Easwar Hariharan]

On 3/22/2024 6:46 AM, Shradha Gupta wrote:
> If the network configuration strings are passed as a combination of IPv4
> and IPv6 addresses, the current KVP daemon does not handle processing 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)
> 
> Co-developed-by: Ani Sinha <anisinha@xxxxxxxxxx>
> Signed-off-by: Ani Sinha <anisinha@xxxxxxxxxx>
> Signed-off-by: Shradha Gupta <shradhagupta@xxxxxxxxxxxxxxxxxxx>
> Reviewed-by: Easwar Hariharan <eahariha@xxxxxxxxxxxxxxxxxxx>
> ---
>  Changes in v5
>  * Included Ani's proposed patch and added him as co-author
> ---
>  tools/hv/hv_kvp_daemon.c | 213 +++++++++++++++++++++++++++++++--------
>  1 file changed, 172 insertions(+), 41 deletions(-)
> 

<snip>

>  }
>  
>  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 +1510,94 @@ 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");
> +		/*
> +		 * 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;
> +		/*
> +		 * As dhcp_enabled is only valid for ipv4, we do not set dhcp
> +		 * methods for ipv6 based on dhcp_enabled flag.
> +		 *
> +		 * For ipv4, set method to manual only when dhcp_enabled is
> +		 * false and specific ipv4 addresses are configured. If neither
> +		 * dhcp_enabled is true and no ipv4 addresses are configured,
> +		 * set method to 'disabled'.
> +		 *
> +		 * For ipv6, set method to manual when we configure ipv6
> +		 * addresses. Otherwise set method to 'auto' so that SLAAC from
> +		 * RA may be used.
> +		 */
> +		if (ip_ver == IPV4) {
> +			if (new_val->dhcp_enabled) {
> +				error = kvp_write_file(nmfile, "method", "",
> +						       "auto");
> +				if (error < 0)
> +					goto setval_error;
> +			} else if (error) {

FWIW, for v5:

Reviewed-by: Easwar Hariharan <eahariha@xxxxxxxxxxxxxxxxxxx>

> +				error = kvp_write_file(nmfile, "method", "",
> +						       "manual");
> +				if (error < 0)
> +					goto setval_error;
> +			} else {
> +				error = kvp_write_file(nmfile, "method", "",
> +						       "disabled");
> +				if (error < 0)
> +					goto setval_error;
> +			}


<snip>

Thanks,
Easwar