Re: [Xen-devel] [PATCH 18/23] xenstored: support running in minios stubdom

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

On 01/25/2012 02:14 PM, Stefano Stabellini wrote:
> On Wed, 25 Jan 2012, Daniel De Graaf wrote:
>> A previous versions of this patch has been sent to xen-devel. See
>> http://lists.xensource.com/archives/html/xen-devel/2009-03/msg01655.html
>>
>> Signed-off-by: Diego Ongaro <diego.ongaro@xxxxxxxxxx>
>> Signed-off-by: Alex Zeffertt <alex.zeffertt@xxxxxxxxxxxxx>
>> Signed-off-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
>> ---
>>  tools/xenstore/Makefile           |    9 ++++++++-
>>  tools/xenstore/tdb.c              |    6 +++---
>>  tools/xenstore/utils.h            |    2 ++
>>  tools/xenstore/xenstored_core.c   |   29 ++++++++++++++++++++++++++---
>>  tools/xenstore/xenstored_domain.c |   11 +++++++++++
>>  5 files changed, 50 insertions(+), 7 deletions(-)
>>
>> diff --git a/tools/xenstore/Makefile b/tools/xenstore/Makefile
>> index 4facb62..be892fd 100644
>> --- a/tools/xenstore/Makefile
>> +++ b/tools/xenstore/Makefile
>> @@ -28,6 +28,10 @@ endif
>>  
>>  ALL_TARGETS = libxenstore.so libxenstore.a clients xs_tdb_dump xenstored
>>  
>> +ifdef CONFIG_STUBDOM
>> +CFLAGS += -DNO_SOCKETS=1
>> +endif
>> +
>>  .PHONY: all
>>  all: $(ALL_TARGETS)
>>  
>> @@ -45,10 +49,13 @@ xenstored_probes.o: xenstored_solaris.o
>>  
>>  CFLAGS += -DHAVE_DTRACE=1
>>  endif
>> - 
>> +
>>  xenstored: $(XENSTORED_OBJS)
>>      $(CC) $(LDFLAGS) $^ $(LDLIBS_libxenctrl) $(SOCKET_LIBS) -o $@ 
>> $(APPEND_LDFLAGS)
>>  
>> +xenstored.a: $(XENSTORED_OBJS)
>> +    $(AR) cr $@ $^
>> +
>>  $(CLIENTS): xenstore
>>      ln -f xenstore $@
>>  
>> diff --git a/tools/xenstore/tdb.c b/tools/xenstore/tdb.c
>> index 3ecd3fc..cb66ea7 100644
>> --- a/tools/xenstore/tdb.c
>> +++ b/tools/xenstore/tdb.c
>> @@ -1334,7 +1334,7 @@ static int tdb_next_lock(TDB_CONTEXT *tdb, struct 
>> tdb_traverse_lock *tlock,
>>  
>>              /* Iterate through chain */
>>              while( tlock->off) {
>> -                    tdb_off current;
>> +                    tdb_off mycurrent;
>>                      if (rec_read(tdb, tlock->off, rec) == -1)
>>                              goto fail;
>>  
>> @@ -1352,10 +1352,10 @@ static int tdb_next_lock(TDB_CONTEXT *tdb, struct 
>> tdb_traverse_lock *tlock,
>>                      }
>>  
>>                      /* Try to clean dead ones from old traverses */
>> -                    current = tlock->off;
>> +                    mycurrent = tlock->off;
>>                      tlock->off = rec->next;
>>                      if (!tdb->read_only && 
>> -                        do_delete(tdb, current, rec) != 0)
>> +                        do_delete(tdb, mycurrent, rec) != 0)
>>                              goto fail;
>>              }
>>              tdb_unlock(tdb, tlock->hash, F_WRLCK);
> 
> Why are you renaming current to mycurrent?

Hmm. While current is a #define in mini-os sched.h, this file doesn't
include that header so it shouldn't break. I'll drop this change.

> 
>> diff --git a/tools/xenstore/utils.h b/tools/xenstore/utils.h
>> index f378343..2effd17 100644
>> --- a/tools/xenstore/utils.h
>> +++ b/tools/xenstore/utils.h
>> @@ -19,7 +19,9 @@ static inline bool strends(const char *a, const char *b)
>>      return streq(a + strlen(a) - strlen(b), b);
>>  }
>>  
>> +#ifndef ARRAY_SIZE
>>  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
>> +#endif
>>  
>>  void barf(const char *fmt, ...) __attribute__((noreturn));
>>  void barf_perror(const char *fmt, ...) __attribute__((noreturn));
>> diff --git a/tools/xenstore/xenstored_core.c 
>> b/tools/xenstore/xenstored_core.c
>> index c1ee932..7564edd 100644
>> --- a/tools/xenstore/xenstored_core.c
>> +++ b/tools/xenstore/xenstored_core.c
>> @@ -92,8 +92,10 @@ TDB_CONTEXT *tdb_context(struct connection *conn)
>>  
>>  bool replace_tdb(const char *newname, TDB_CONTEXT *newtdb)
>>  {
>> +#ifndef __MINIOS__
>>      if (rename(newname, xs_daemon_tdb()) != 0)
>>              return false;
>> +#endif
>>      tdb_close(tdb_ctx);
>>      tdb_ctx = talloc_steal(talloc_autofree_context(), newtdb);
>>      return true;
> 
> Are you doing this because the database is not on file anymore, correct?
> Maybe there is some room for introducing this concept more generally
> because it could be useful on Linux too and it would avoid this and
> other ifdef's.

Yes, this one can check TDB_INTERNAL (not sure why the original author
didn't want to check that flag; it works fine).

> 
>> @@ -223,7 +225,6 @@ static void reopen_log(void)
>>      }
>>  }
>>  
>> -
>>  static bool write_messages(struct connection *conn)
>>  {
>>      int ret;
>> @@ -326,7 +327,8 @@ static int initialize_set(fd_set *inset, fd_set *outset, 
>> int sock, int ro_sock,
>>              set_fd(sock, inset, &max);
>>      if (ro_sock != -1)
>>              set_fd(ro_sock, inset, &max);
>> -    set_fd(reopen_log_pipe[0], inset, &max);
>> +    if (reopen_log_pipe[0] != -1)
>> +            set_fd(reopen_log_pipe[0], inset, &max);
>>  
>>      if (xce_handle != NULL)
>>              set_fd(xc_evtchn_fd(xce_handle), inset, &max);
>> @@ -1410,7 +1412,11 @@ static void accept_connection(int sock, bool canwrite)
>>  }
>>  #endif
>>  
>> +#ifdef __MINIOS__
>> +#define TDB_FLAGS TDB_INTERNAL|TDB_NOLOCK
>> +#else
>>  #define TDB_FLAGS 0
>> +#endif
> 
> See above

I will be adding a command-line argument for this, and force it to be
enabled if __MINIOS__.

> 
>>  /* We create initial nodes manually. */
>>  static void manual_node(const char *name, const char *child)
>> @@ -1435,7 +1441,11 @@ static void setup_structure(void)
>>  {
>>      char *tdbname;
>>      tdbname = talloc_strdup(talloc_autofree_context(), xs_daemon_tdb());
>> +#ifdef __MINIOS__
>> +    tdb_ctx = NULL;
>> +#else
>>      tdb_ctx = tdb_open(tdbname, 0, TDB_FLAGS, O_RDWR, 0);
>> +#endif
> 
> ditto
> 
>>      if (tdb_ctx) {
>>              /* XXX When we make xenstored able to restart, this will have
>> @@ -1661,6 +1671,7 @@ static void corrupt(struct connection *conn, const 
>> char *fmt, ...)
>>  }
>>  
>>  
>> +#ifndef __MINIOS__
>>  static void write_pidfile(const char *pidfile)
>>  {
>>      char buf[100];
>> @@ -1707,6 +1718,7 @@ static void daemonize(void)
>>      /* Discard our parent's old-fashioned umask prejudices. */
>>      umask(0);
>>  }
>> +#endif
>>  
>>  #ifdef NO_SOCKETS
>>  static void init_sockets(int **psock, int **pro_sock)
>> @@ -1866,6 +1878,7 @@ int main(int argc, char *argv[])
>>  
>>      reopen_log();
>>  
>> +#ifndef __MINIOS__
>>      /* make sure xenstored directory exists */
>>      if (mkdir(xs_daemon_rundir(), 0755)) {
>>              if (errno != EEXIST) {
>> @@ -1887,6 +1900,7 @@ int main(int argc, char *argv[])
>>      }
>>      if (pidfile)
>>              write_pidfile(pidfile);
>> +#endif
> 
> Maybe we could just have this fail gracefully: after all even on Linux
> if the socket interface is not available everything should be able to
> run correctly.
> 

That's simple enough to change, although it might make debugging a bit harder
on Linux if there are stale socket files around with bad permissions.

Actually... this is just making the directories. Error reporting doesn't belong
here, it belongs where we open the files that go in the directories.

> 
>>      /* Talloc leak reports go to stderr, which is closed if we fork. */
>>      if (!dofork)
>> @@ -1897,9 +1911,14 @@ int main(int argc, char *argv[])
>>  
>>      init_sockets(&sock, &ro_sock);
>>  
>> +#ifdef __MINIOS__
>> +    reopen_log_pipe[0] = -1;
>> +    reopen_log_pipe[1] = -1;
>> +#else
>>      if (pipe(reopen_log_pipe)) {
>>              barf_perror("pipe");
>>      }
>> +#endif
>>  
>>      /* Setup the database */
>>      setup_structure();
>> @@ -1916,6 +1935,7 @@ int main(int argc, char *argv[])
>>              fflush(stdout);
>>      }
>>  
>> +#ifndef __MINIOS__
>>      /* redirect to /dev/null now we're ready to accept connections */
>>      if (dofork) {
>>              int devnull = open("/dev/null", O_RDWR);
>> @@ -1927,6 +1947,7 @@ int main(int argc, char *argv[])
>>              close(devnull);
>>              xprintf = trace;
>>      }
>> +#endif
> 
> we could probably handle /dev/null in minios from sys.c

This should really be part of daemonize() except that we still need the
original stdout for outputpid; I'll move it to a finish_daemonize().

> 
>>      signal(SIGHUP, trigger_reopen_log);
>>  
>> @@ -1936,8 +1957,10 @@ int main(int argc, char *argv[])
>>      /* Get ready to listen to the tools. */
>>      max = initialize_set(&inset, &outset, *sock, *ro_sock, &timeout);
>>  
>> +#ifndef __MINIOS__
>>      /* Tell the kernel we're up and running. */
>>      xenbus_notify_running();
>> +#endif
>>  
>>      /* Main loop. */
>>      for (;;) {
>> @@ -1949,7 +1972,7 @@ int main(int argc, char *argv[])
>>                      barf_perror("Select failed");
>>              }
>>  
>> -            if (FD_ISSET(reopen_log_pipe[0], &inset)) {
>> +            if (reopen_log_pipe[0] != -1 && FD_ISSET(reopen_log_pipe[0], 
>> &inset)) {
>>                      char c;
>>                      if (read(reopen_log_pipe[0], &c, 1) != 1)
>>                              barf_perror("read failed");
>> diff --git a/tools/xenstore/xenstored_domain.c 
>> b/tools/xenstore/xenstored_domain.c
>> index 661d955..435f76a 100644
>> --- a/tools/xenstore/xenstored_domain.c
>> +++ b/tools/xenstore/xenstored_domain.c
>> @@ -197,10 +197,14 @@ static int destroy_domain(void *_domain)
>>      }
>>  
>>      if (domain->interface) {
>> +#ifdef __MINIOS__
>> +            unmap_interface(domain->interface);
>> +#else
>>              if (domain->domid == 0)
>>                      munmap(domain->interface, getpagesize());
>>              else
>>                      unmap_interface(domain->interface);
>> +#endif
>>      }
> 
> Argh. 
> Why dom0 cannot be handled inside unmap_interface?

Because dom0 isn't mapped the same way as domU. The mapping for dom0
is set up by introduce_dom0 which uses mmap directly, so it cannot be
unmapped using the grant table unmap function. When xenstore was using
map_foreign_range this didn't matter, everyone used munmap. In fact,
on Linux it still doesn't matter as the grant table unmap is munmap.

I'll add (as part of #14) a comment here since it's the second time
I've needed to explain this nuance.

> 
>>      fire_watches(NULL, "@releaseDomain", false);
>> @@ -595,6 +599,12 @@ void restore_existing_connections(void)
>>  {
>>  }
>>  
>> +#ifdef __MINIOS__
>> +static int dom0_init(void)
>> +{
>> +    return 0;
>> +}
>> +#else
>>  static int dom0_init(void) 
>>  { 
>>      evtchn_port_t port;
>> @@ -618,6 +628,7 @@ static int dom0_init(void)
>>  
>>      return 0; 
>>  }
>> +#endif
>>  
>>  void domain_init(void)
>>  {
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel