Re: [PATCH] xenconsole: Ensure exclusive access to console using locks

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2015-07-24 at 14:51 +0200, Martin Lucina wrote:
> If more than one instance of xenconsole is run against the same DOMID
> then each instance will only get some data. This change ensures
> exclusive access to the console by creating and obtaining an 
> exclusive
> lock on <XEN_LOCK_DIR>/xenconsole.<DOMID>.
> 
> Signed-off-by: Martin Lucina <martin@lucina.net>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/console/Makefile      |  2 +-
>  tools/console/client/main.c | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/console/Makefile b/tools/console/Makefile
> index 71f8088..b6a51eb 100644
> --- a/tools/console/Makefile
> +++ b/tools/console/Makefile
> @@ -3,7 +3,7 @@ include $(XEN_ROOT)/tools/Rules.mk
>  
>  CFLAGS  += -Werror
>  
> -CFLAGS  += $(CFLAGS_libxenctrl)
> +CFLAGS  += $(CFLAGS_libxenctrl) -I$(XEN_ROOT)/tools/libxc


I'm afraid this (delving into another components private headers) isn't
allowed. Instead you should add the two lines to tools/console/Makefile
to generate a local _paths.h:

genpath-target = $(call buildmakevars2header,_paths.h)
$(eval $(genpath-target))

Plus a dependency on it in the xenconsole rule and a .gitignore entry.

You might want to generate client/_paths.h instead to avoid needing to
muck around with CFLAGS.

>  CFLAGS  += $(CFLAGS_libxenstore)
>  LDLIBS += $(LDLIBS_libxenctrl)
>  LDLIBS += $(LDLIBS_libxenstore)
> diff --git a/tools/console/client/main.c b/tools/console/client/main.c
> index 753b3aa..709abc1 100644
> --- a/tools/console/client/main.c
> +++ b/tools/console/client/main.c
> @@ -18,6 +18,7 @@
>   *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>  \*/
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -40,10 +41,13 @@
>  
>  #include 
>  #include "xenctrl.h"
> +#include "_paths.h"
>  
>  #define ESCAPE_CHARACTER 0x1d
>  
>  static volatile sig_atomic_t received_signal = 0;
> +static char *lockfile = NULL;
> +static int lockfd = -1;
>  
>  static void sighandler(int signum)
>  {
> @@ -267,6 +271,30 @@ static void restore_term_stdin(void)
>  > 	> restore_term(STDIN_FILENO, &stdin_old_attr);
>  }
>  
> +static void console_lock(int domid)
> +{
> +> 	> lockfile = malloc(PATH_MAX);
> +> 	> if (lockfile == NULL)
> +> 	> 	> err(ENOMEM, "malloc");

malloc sets errno, so I think you can pass that here as well.

Also, a static buffer would be fine in this context I think.

> +	snprintf(lockfile, PATH_MAX - 1, "%s/xenconsole.%d", XEN_LOCK_DIR, domid);

I think you can use string concatenation here, e.g.
    XEN_LOCK_DIR "/xenconsole.%d"


Given the known limits on the size of domid it would probably be
possible to figure out a tighter limit than PATH_MAX.

> +
> +> 	> lockfd = open(lockfile, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);
> +> 	> if (lockfd == -1)
> +> 	> 	> err(errno, "Could not open %s", lockfile);
> +> 	> if (flock(lockfd, LOCK_EX|LOCK_NB) != 0)
> +> 	> 	> err(errno, "Could not lock %s", lockfile);
> +}
> +
> +static void console_unlock(void)
> +{
> +> 	> if (lockfd != -1) {
> +> 	> 	> flock(lockfd, LOCK_UN);
> +> 	> 	> close(lockfd);
> +> 	> }
> +> 	> if (lockfile != NULL)
> +> 	> 	> unlink(lockfile);


I think this introduces a race, if another client arrives between the
unlock/close and the unlink then you will delete the file which that
second client now has a lock on, resulting in a third client being able
to take the lock (by creating a new file) when it should already be
locked.

I think the answer is to either not remove the lockfile or to do so
_before_ dropping the lock, which makes the unlink itself the effective
unlock point.

Ian.