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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Sep 22, 2021 at 02:07:44PM +0500, Julien Grall wrote:
> 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@xxxxxxxxxx>
> > ---
> > 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?

Right, I just need to change the order in conn_can_read and
conn_can_write so that the is_ignored check is performed before the
can_{read,write} handler is called.

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

But if the domain has died in the middle of live update
get_domain_info will return false, and thus the code won't get here.

If the domain is in the process of being removed (ie: dominfo.shutdown
being set for example), it would eventually get into dominfo.dying and
thus be removed normally.

I assumed those checks where needed in order to prevent having a
domain without a valid interface on the tracked list while it was on
the process of being removed. With the other changes on this patch
tracking a domain without a valid interface should be fine, and it
will eventually be removed as part of the normal flow.

Thanks, Roger.