Received-SPF: pass (google.com: domain of linux-kernel+bounces-117588-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <eea2ddef-ba2e-4424-84d0-2af0340be899@linux.microsoft.com>
Date: Mon, 25 Mar 2024 10:23:25 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v5] hv/hv_kvp_daemon: Handle IPv4 and Ipv6 combination for
 keyfile format
To: Shradha Gupta <shradhagupta@linux.microsoft.com>,
 linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org
Cc: "K. Y. Srinivasan" <kys@microsoft.com>,
 Haiyang Zhang <haiyangz@microsoft.com>, Wei Liu <wei.liu@kernel.org>,
 Dexuan Cui <decui@microsoft.com>, Long Li <longli@microsoft.com>,
 Olaf Hering <olaf@aepfle.de>, Ani Sinha <anisinha@redhat.com>,
 Shradha Gupta <shradhagupta@microsoft.com>
References: <1711115162-11629-1-git-send-email-shradhagupta@linux.microsoft.com>
Content-Language: en-CA
From: Easwar Hariharan <eahariha@linux.microsoft.com>
In-Reply-To: <1711115162-11629-1-git-send-email-shradhagupta@linux.microsoft.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

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@redhat.com>
> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> Signed-off-by: Shradha Gupta <shradhagupta@linux.microsoft.com>
> Reviewed-by: Easwar Hariharan <eahariha@linux.microsoft.com>
> ---
>  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@linux.microsoft.com>

> +				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