[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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?

        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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.