Linux-HyperV Archive on lore.kernel.org
 help / color / Atom feed
From: Vitaly Kuznetsov <vkuznets@redhat.com>
To: Olaf Hering <olaf@aepfle.de>,
	"K. Y. Srinivasan" <kys@microsoft.com>,
	Haiyang Zhang <haiyangz@microsoft.com>,
	Stephen Hemminger <sthemmin@microsoft.com>,
	Sasha Levin <sashal@kernel.org>,
	"open list\:Hyper-V CORE AND DRIVERS"
	<linux-hyperv@vger.kernel.org>,
	open list <linux-kernel@vger.kernel.org>
Cc: Olaf Hering <olaf@aepfle.de>
Subject: Re: [PATCH v1] tools/hv: async name resolution in kvp_daemon
Date: Thu, 07 Nov 2019 14:39:11 +0100
Message-ID: <874kzfbybk.fsf@vitty.brq.redhat.com> (raw)
In-Reply-To: <20191024144943.26199-1-olaf@aepfle.de>

Olaf Hering <olaf@aepfle.de> writes:

> The hostname is resolved just once since commit 58125210ab3b ("Tools:
> hv: cache FQDN in kvp_daemon to avoid timeouts") to make sure the VM
> responds within the timeout limits to requests from the host.
>
> If for some reason getaddrinfo fails, the string returned by the
> "FullyQualifiedDomainName" request contains some error string, which is
> then used by tools on the host side.
>
> Adjust the code to resolve the current hostname in a separate thread.
> This thread loops until getaddrinfo returns success. During this time
> all "FullyQualifiedDomainName" requests will be answered with an empty
> string.
>
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
>  tools/hv/Makefile        |  2 ++
>  tools/hv/hv_kvp_daemon.c | 69 ++++++++++++++++++++++++++++++++----------------
>  2 files changed, 48 insertions(+), 23 deletions(-)
>
> diff --git a/tools/hv/Makefile b/tools/hv/Makefile
> index b57143d9459c..3b5481015a84 100644
> --- a/tools/hv/Makefile
> +++ b/tools/hv/Makefile
> @@ -22,6 +22,8 @@ ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS))
>  
>  ALL_SCRIPTS := hv_get_dhcp_info.sh hv_get_dns_info.sh hv_set_ifconfig.sh
>  
> +$(OUTPUT)hv_kvp_daemon: LDFLAGS += -lpthread
> +
>  all: $(ALL_PROGRAMS)
>  
>  export srctree OUTPUT CC LD CFLAGS
> diff --git a/tools/hv/hv_kvp_daemon.c b/tools/hv/hv_kvp_daemon.c
> index e9ef4ca6a655..22cf1c4dbf5c 100644
> --- a/tools/hv/hv_kvp_daemon.c
> +++ b/tools/hv/hv_kvp_daemon.c
> @@ -41,6 +41,7 @@
>  #include <net/if.h>
>  #include <limits.h>
>  #include <getopt.h>
> +#include <pthread.h>
>  
>  /*
>   * KVP protocol: The user mode component first registers with the
> @@ -85,7 +86,7 @@ static char *processor_arch;
>  static char *os_build;
>  static char *os_version;
>  static char *lic_version = "Unknown version";
> -static char full_domain_name[HV_KVP_EXCHANGE_MAX_VALUE_SIZE];
> +static char *full_domain_name;
>  static struct utsname uts_buf;
>  
>  /*
> @@ -1327,27 +1328,53 @@ static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
>  	return error;
>  }
>  
> -
> -static void
> -kvp_get_domain_name(char *buffer, int length)
> +/*
> + * Async retrival of Fully Qualified Domain Name because getaddrinfo takes an
> + * unpredictable amount of time to finish.
> + */
> +static void *kvp_getaddrinfo(void *p)
>  {
> -	struct addrinfo	hints, *info ;
> -	int error = 0;
> +	char *tmp, **str_ptr = (char **)p;
> +	char hostname[HOST_NAME_MAX + 1];
> +	struct addrinfo	*info, hints = {
> +		.ai_family = AF_INET, /* Get only ipv4 addrinfo. */
> +		.ai_socktype = SOCK_STREAM,
> +		.ai_flags = AI_CANONNAME,
> +	};
> +	int ret;
> +
> +	if (gethostname(hostname, sizeof(hostname) - 1) < 0)
> +		goto out;
> +
> +	do {
> +		ret = getaddrinfo(hostname, NULL, &hints, &info);
> +		if (ret)
> +			sleep(1);

Is it only EAI_AGAIN or do you see any other return values which justify
the retry? I'm afraid that in case of a e.g. non-existing hostname we'll
be infinitely looping with EAI_FAIL.

> +	} while (ret);
> +
> +	ret = asprintf(&tmp, "%s", info->ai_canonname);
> +	freeaddrinfo(info);
> +	if (ret <= 0)
> +		goto out;
> +
> +	if (ret > HV_KVP_EXCHANGE_MAX_VALUE_SIZE)
> +		tmp[HV_KVP_EXCHANGE_MAX_VALUE_SIZE - 1] = '\0';
> +	*str_ptr = tmp;
>  
> -	gethostname(buffer, length);
> -	memset(&hints, 0, sizeof(hints));
> -	hints.ai_family = AF_INET; /*Get only ipv4 addrinfo. */
> -	hints.ai_socktype = SOCK_STREAM;
> -	hints.ai_flags = AI_CANONNAME;
> +out:
> +	pthread_exit(NULL);
> +}
> +
> +static void kvp_obtain_domain_name(char **str_ptr)
> +{
> +	pthread_t t;
>  
> -	error = getaddrinfo(buffer, NULL, &hints, &info);
> -	if (error != 0) {
> -		snprintf(buffer, length, "getaddrinfo failed: 0x%x %s",
> -			error, gai_strerror(error));
> +	if (pthread_create(&t, NULL, kvp_getaddrinfo, str_ptr)) {
> +		syslog(LOG_ERR, "pthread_create failed; error: %d %s",
> +			errno, strerror(errno));
>  		return;
>  	}
> -	snprintf(buffer, length, "%s", info->ai_canonname);
> -	freeaddrinfo(info);
> +	pthread_detach(t);

I think this should be complemented with pthread_cancel/pthread_join
before exiting main().

>  }
>  
>  void print_usage(char *argv[])
> @@ -1412,11 +1439,7 @@ int main(int argc, char *argv[])
>  	 * Retrieve OS release information.
>  	 */
>  	kvp_get_os_info();
> -	/*
> -	 * Cache Fully Qualified Domain Name because getaddrinfo takes an
> -	 * unpredictable amount of time to finish.
> -	 */
> -	kvp_get_domain_name(full_domain_name, sizeof(full_domain_name));
> +	kvp_obtain_domain_name(&full_domain_name);
>  
>  	if (kvp_file_init()) {
>  		syslog(LOG_ERR, "Failed to initialize the pools");
> @@ -1571,7 +1594,7 @@ int main(int argc, char *argv[])
>  
>  		switch (hv_msg->body.kvp_enum_data.index) {
>  		case FullyQualifiedDomainName:
> -			strcpy(key_value, full_domain_name);
> +			strcpy(key_value, full_domain_name ? : "");
>  			strcpy(key_name, "FullyQualifiedDomainName");
>  			break;
>  		case IntegrationServicesVersion:

-- 
Vitaly


  parent reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-24 14:49 Olaf Hering
2019-10-28 16:17 ` Sasha Levin
2019-10-28 17:49   ` Olaf Hering
2019-11-01 20:17     ` Olaf Hering
2019-11-02  4:18     ` Sasha Levin
2019-11-11 15:55       ` Olaf Hering
2019-11-07 13:39 ` Vitaly Kuznetsov [this message]
2019-11-07 13:48   ` Olaf Hering
2019-11-07 14:15     ` Vitaly Kuznetsov
2019-11-07 14:20       ` Olaf Hering
2019-11-07 14:44         ` Vitaly Kuznetsov
2019-11-13 15:39           ` Olaf Hering

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=874kzfbybk.fsf@vitty.brq.redhat.com \
    --to=vkuznets@redhat.com \
    --cc=haiyangz@microsoft.com \
    --cc=kys@microsoft.com \
    --cc=linux-hyperv@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=olaf@aepfle.de \
    --cc=sashal@kernel.org \
    --cc=sthemmin@microsoft.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-HyperV Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hyperv/0 linux-hyperv/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-hyperv linux-hyperv/ https://lore.kernel.org/linux-hyperv \
		linux-hyperv@vger.kernel.org
	public-inbox-index linux-hyperv

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-hyperv


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git