Re: [PATCH v2 5/6] tools/xenstored: partially handle domains without a shared ring

[Julien Grall <julien@xen.org>]

Hi Roger,

On 22/09/2021 13:21, Roger Pau Monne wrote:
> Failure to map the shared ring and thus establish a xenstore
> connection with a domain shouldn't prevent the "@introduceDomain"
> watch from firing, likewise with "@releaseDomain".
> 
> In order to handle such events properly xenstored should keep track of
> the domains even if the shared communication ring cannot be mapped.
> This was partially the case due to the restore mode, which needs to
> handle domains that have been destroyed between the save and restore
> period. This patch extends on the previous limited support of
> temporary adding a domain without a valid interface ring, and modifies
> check_domains to keep domains without an interface on the list.
> 
> Handling domains without a valid shared ring is required in order to
> support domain without a grant table, since the lack of grant table
> will prevent the mapping of the xenstore ring grant reference.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> oxenstored will need a similar treatment once grant mapping is used
> there. For the time being it still works correctly because it uses
> foreign memory to map the shared ring, and that will work in the
> absence of grant tables on the domain.
> ---
> Changes since v1:
>   - New in this version.
> ---
>   tools/xenstore/xenstored_domain.c | 30 ++++++++++++++++++------------
>   1 file changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
> index 9fb78d5772..150c6f082e 100644
> --- a/tools/xenstore/xenstored_domain.c
> +++ b/tools/xenstore/xenstored_domain.c
> @@ -119,6 +119,11 @@ static int writechn(struct connection *conn,
>   	struct xenstore_domain_interface *intf = conn->domain->interface;
>   	XENSTORE_RING_IDX cons, prod;
>   
> +	if (!intf) {
> +		errno = ENODEV;
> +		return -1;
> +	}
> +
>   	/* Must read indexes once, and before anything else, and verified. */
>   	cons = intf->rsp_cons;
>   	prod = intf->rsp_prod;
> @@ -149,6 +154,11 @@ static int readchn(struct connection *conn, void *data, unsigned int len)
>   	struct xenstore_domain_interface *intf = conn->domain->interface;
>   	XENSTORE_RING_IDX cons, prod;
>   
> +	if (!intf) {
> +		errno = ENODEV;
> +		return -1;
> +	}
> +
>   	/* Must read indexes once, and before anything else, and verified. */
>   	cons = intf->req_cons;
>   	prod = intf->req_prod;
> @@ -176,6 +186,9 @@ static bool domain_can_write(struct connection *conn)
>   {
>   	struct xenstore_domain_interface *intf = conn->domain->interface;
>   
> +	if (!intf)
> +		return false;
> +

Rather than adding an extra check, how about taking advantage of is_ignore?

>   	return ((intf->rsp_prod - intf->rsp_cons) != XENSTORE_RING_SIZE);
>   }
>   
> @@ -183,7 +196,8 @@ static bool domain_can_read(struct connection *conn)
>   {
>   	struct xenstore_domain_interface *intf = conn->domain->interface;
>   
> -	if (domain_is_unprivileged(conn) && conn->domain->wrl_credit < 0)
> +	if ((domain_is_unprivileged(conn) && conn->domain->wrl_credit < 0) ||
> +	    !intf)
>   		return false;
>   
>   	return (intf->req_cons != intf->req_prod);
> @@ -268,14 +282,7 @@ void check_domains(void)
>   				domain->shutdown = true;
>   				notify = 1;
>   			}
> -			/*
> -			 * On Restore, we may have been unable to remap the
> -			 * interface and the port. As we don't know whether
> -			 * this was because of a dying domain, we need to
> -			 * check if the interface and port are still valid.
> -			 */
> -			if (!dominfo.dying && domain->port &&
> -			    domain->interface)
> +			if (!dominfo.dying)
>   				continue;

This is mostly a revert on "tools/xenstore: handle dying domains in live 
update". However, IIRC, this check was necessary to release the 
connection if the domain has died in the middle of Live-Update.

So I think this check should stick here. Instead, I think, we should 
mark the "fake domain" so we can ignore them here.

>   		}
>   		if (domain->conn) {
> @@ -450,8 +457,6 @@ static struct domain *introduce_domain(const void *ctx,
>   	if (!domain->introduced) {
>   		interface = is_master_domain ? xenbus_map()
>   					     : map_interface(domid);
> -		if (!interface && !restore)
> -			return NULL;
>   		if (new_domain(domain, port, restore)) {
>   			rc = errno;
>   			if (interface) {
> @@ -505,7 +510,8 @@ int do_introduce(struct connection *conn, struct buffered_data *in)
>   	if (!domain)
>   		return errno;
>   
> -	domain_conn_reset(domain);
> +	if (domain->interface)
> +		domain_conn_reset(domain);
>   
>   	send_ack(conn, XS_INTRODUCE);
>   
> 

Cheers,

-- 
Julien Grall