On 12/07/2011 11:08 AM, Peter Krempa wrote:
> This patch adds a set of functions used in creating console streams for
> domains using PTYs and ensures mutualy exculsive access to the PTYs.
Picking up on my (stalled) review...
> +++ b/src/Makefile.am
> @@ -101,6 +101,9 @@ UTIL_SOURCES = \
> util/virsocketaddr.h util/virsocketaddr.c \
> util/virtime.h util/virtime.c
>
> +SAFE_CONSOLE_SOURCES = \
> + util/domain_safe_console.c util/domain_safe_console.h
Unless you are planning on including these sources into an independent
library, I see no need to make a new variable. Just add these files
into the right place within UTIL_SOURCES.
Those are some rather long names. Also, our convention for new files
has lately been to go with vir<name>.[ch], with APIs starting with
vir<Name>...(). I'm thinking that this patch may be better as:
virconsole.[ch]
virConsoleAlloc()
virConsoleFree()
virConsoleOpen()
That is, the name Domain doesn't add anything (we might want a console
for other reasons) and the name Safe is redundant (the whole point of
adding a virName API is to make the Name action safer and easier to use).
> +
> EXTRA_DIST += $(srcdir)/util/virkeymaps.h $(srcdir)/util/keymaps.csv \
> $(srcdir)/util/virkeycode-mapgen.py
>
> @@ -555,7 +558,7 @@ noinst_LTLIBRARIES = libvirt_util.la
> libvirt_la_LIBADD = $(libvirt_la_BUILT_LIBADD)
> libvirt_la_BUILT_LIBADD = libvirt_util.la
> libvirt_util_la_SOURCES = \
> - $(UTIL_SOURCES)
> + $(UTIL_SOURCES) $(SAFE_CONSOLE_SOURCES)
If you add the new files directly to UTIL_SOURCES, then you don't need
to edit this line.
> +++ b/src/libvirt_private.syms
> @@ -537,6 +537,12 @@ virDomainConfNWFilterTeardown;
> virDomainConfVMNWFilterTeardown;
>
>
> +# domain_safe_console.h
> +virDomainSafeConsoleAlloc;
> +virDomainSafeConsoleFree;
> +virDomainSafeConsoleOpen;
See above about naming ideas.
> +
> +
> # ebtables.h
> ebtablesAddForwardAllowIn;
> ebtablesAddForwardPolicyReject;
> diff --git a/src/util/domain_safe_console.c b/src/util/domain_safe_console.c
> new file mode 100644
> index 0000000..4d5639d
> --- /dev/null
> +++ b/src/util/domain_safe_console.c
> @@ -0,0 +1,399 @@
> +/**
> + * domain_safe_console.c: api to guarantee mutualy exclusive
> + * access to domain's consoles
> + *
> + * Copyright (C) 2011 Red Hat, Inc.
Shoot - I let this go so long that it is now 2012.
> + *
> + * See COPYING.LIB for the License of this software
We probably ought to go with the longer 3-paragraph version that
specifically calls out LGPLv2+, rather than referring to COPYING.LIB.
> +#ifdef VIR_PTY_LOCK_FILE_PATH
> +/**
> + * Create a full filename with path to the lock file based on
> + * name/path of corresponding pty
> + *
> + * @pty path of the console device
> + *
> + * Returns a modified name, that the caller has to free or NULL
> + * on error.
Grammar:
Returns a modified name that the caller has to free, or NULL on error.
> + */
> +static char *virDomainSafeConsoleLockFilePath(const char *pty)
> +{
> + char *path = NULL;
> + char *sanitizedPath = NULL;
> + char *ptyCopy;
> + char *filename;
> + char *p;
> +
> + if (!(ptyCopy = strdup(pty))) {
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> + if ((filename = strstr(ptyCopy, "/dev/"))) {
Do you want to find /dev anywhere within the name, or only skip it at
the beginning?
> + filename += 5; /* skip /dev/ */
> + p = filename;
> + /* substitute path forward slashes for underscores */
> + while (*p) {
> + if (*p == '/')
> + *p = '_';
> + ++p;
Do you want this slash normalization to only occur if /dev/ was in the
name, or shouldn't you be doing it always?
> + }
> + }
> +
> + if (virAsprintf(&path, "%s/LCK..%s", VIR_PTY_LOCK_FILE_PATH, filename) < 0)
> + goto cleanup;
That is, we converted /dev/pts/3 into /var/lock/LCK..pts_3 - and that
looks correct.
> +/**
> + * Verify and create a lock file for a console pty
> + *
> + * @pty Path of the console device
> + *
> + * Returns 0 on success, -1 on error
> + */
> +static int virDomainSafeConsoleLockFileCreate(const char *pty)
> +{
> + char *path = NULL;
> + int ret = -1;
> + int lockfd = -1;
> + char *pidStr = NULL;
> + int pid;
> + ssize_t read_n;
> + /* build lock file path */
> + if (!(path = virDomainSafeConsoleLockFilePath(pty)))
> + goto cleanup;
> +
> + /* check if lock file exists */
> + if ((lockfd = open(path, O_RDONLY)) >= 0) {
> + if ((read_n = virFileReadLimFD(lockfd, 100, &pidStr) > 0) &&
> + (sscanf(pidStr, "%d", &pid) == 1)) {
sscanf() is too heavyweight. Use virStrToLong_i().
> + /* lock file is valid, let's check if the pid is alive */
> + if (pid > 1 && kill((pid_t) pid, 0) == 0) {
Can you reuse anything from virpidfile.h here, rather than rolling your
own pid file reader? In particular, kill() isn't portable to mingw, so
reusing code would automatically make your code more likely to compile.
Hmm - virPidFileReadPathIfAlive() is too strong - it requires that you
specify which executable should own the pid. But virPidFileReadPath()
is too weak - it reads the pid without checking if that pid is still
alive. It might be worth adding an intermediate function, or else
teaching virPidFileReadPathIfAlive to skip the probe of /proc/PID/exe if
pidfile is NULL (do that as a separate pre-req patch).
> + /* lock file is valid and a process with pid corresponding
> + * to the pid stored in the lock file exists */
> + consoleReportError(VIR_ERR_OPERATION_FAILED,
> + _("Requested console pty '%s' is locked by "
> + "lock file '%s' held by process %d"),
> + pty, path, pid);
> + goto cleanup;
> + }
> + /* lock file is stale */
> + VIR_DEBUG("Cleaning up stale lockfile '%s'", path);
> + VIR_FORCE_CLOSE(lockfd);
> + unlink(path);
> + } else {
> + /* lock file is corrupted */
> + VIR_DEBUG("Cleaning up corrupted lockfile '%s'", path);
> + VIR_FORCE_CLOSE(lockfd);
> + unlink(path);
> + /* this might actualy fail, but we get an error while
s/actualy/actually/
> + * creating a new lockfile. Let the user handle the mess. */
> + }
> + VIR_FREE(pidStr);
> + }
> + /* lockfile doesn't (shouldn't) exist */
> +
> + /* ensure correct format according to filesystem hierarchy standard */
> + if (virAsprintf(&pidStr, "%10d\n", (int) getpid()) < 0)
Here, calling out the URL in your comment might help:
http://www.pathname.com/fhs/pub/fhs-2.3.html#VARLOCKLOCKFILES
> + goto cleanup;
> +
> + /* create the lock file */
> + if ((lockfd = open(path, O_WRONLY | O_CREAT | O_EXCL, 00644)) < 0) {
> + /* If we run in session mode, we might have no access to the lock
> + * file directory. We have to check for an permission denied error
> + * and see if we can reach it. This should cause an error only if
> + * we run in daemon mode and thus privileged.
> + */
> + if (errno == EACCES && geteuid() != 0) {
> + VIR_DEBUG("Skipping lock file creation for pty '%s in path '%s'.",
> + pty, path);
> + ret = 0;
> + goto cleanup;
> + }
> + virReportSystemError(errno,
> + _("Couldn't create lock file for "
> + "pty '%s' in path '%s'"),
> + pty, path);
> + goto cleanup;
> + }
> +
> + /* write the pid to the file */
> + if (safewrite(lockfd, pidStr, strlen(pidStr)) < 0) {
> + virReportSystemError(errno,
> + _("Couldn't write to lock file for "
> + "pty '%s' in path '%s'"),
> + pty, path);
> + VIR_FORCE_CLOSE(lockfd);
> + unlink(path);
> + goto cleanup;
> + }
> +
> + /* we hold the lock */
> + ret = 0;
> +
> +cleanup:
> + VIR_FORCE_CLOSE(lockfd);
> + VIR_FREE(path);
> + VIR_FREE(pidStr);
> +
> + return ret;
Complex, but looks right.
> +}
> +
> +/**
> + * Remove a lock file for a pty
> + *
> + * @pty Path of the pty device
> + */
> +static void virDomainSafeConsoleLockFileRemove(const char *pty)
> +{
> + char *path = virDomainSafeConsoleLockFilePath(pty);
> + if (path)
> + unlink(path);
> + VIR_FREE(path);
> +}
> +#else /* #ifdef VIR_PTY_LOCK_FILE_PATH */
> +/* file locking for console devices is disabled */
> +static int virDomainSafeConsoleLockFileCreate(const char *pty ATTRIBUTE_UNUSED)
> +{
> + return 0;
I'm not sure whether it makes sense to log a message that we are
skipping a lock file, due to no configured directory. On one hand, it's
just noise, if you specifically configured things to avoid the
directory. On the other hand, we warned the user about qemu:///session
failing to grab a lock due to EACCES, and this is failure to grab a lock
to to configuration.
> +}
> +
> +static void virDomainSafeConsoleLockFileRemove(const char *py ATTRIBUTE_UNUSED)
s/py/pty/
> +{
> + return;
> +}
> +#endif /* #ifdef VIR_PTY_LOCK_FILE_PATH */
> +
> +/**
> + * Frees an entry from the hash containing domain's active consoles
> + *
> + * @data Opaque data, struct holding informations about the console
s/informations/information/
> +/**
> + * Open a console stream for a domain ensuring that other streams are
> + * not using the console, nor any lockfiles exist. This ensuers that
s/ensuers/ensures/
> + * the console stream does not get corrupted due to a race on reading
> + * same FD by two processes.
> + *
> + * @cons Pointer to private structure holding data about console streams.
> + * @pty Path to the pseudo tty to be opened.
> + * @st Stream the client wishes to use for the console connection.
> + * @force On true, close active console streams for the selected console pty
> + * before opening this connection.
> + *
> + * Returns 0 on success and st is connected to the selected pty and
> + * corresponding lock file is created (if configured with). Returns -1 on
s/if configured with/if configured/
> + * error and 1 if the console stream is open and busy.
> + */
> +int virDomainSafeConsoleOpen(virDomainSafeConsolesPtr cons,
> + const char *pty,
> + virStreamPtr st,
> + bool force)
> +{
> + virDomainSafeConsoleStreamInfoPtr cbdata = NULL;
> + virStreamPtr savedStream;
> + int ret;
> +
> + virMutexLock(&cons->lock);
> +
> + if ((savedStream = virHashLookup(cons->hash, pty))) {
> + if (!force) {
> + /* entry found, console is busy */
> + virMutexUnlock(&cons->lock);
> + return 1;
> + } else {
> + /* terminate existing connection */
> + virFDStreamSetInternalCloseCb(savedStream, NULL, NULL, NULL);
> + virStreamAbort(savedStream);
> + virHashRemoveEntry(cons->hash, pty);
> + /* continue adding a new stream connection */
> + }
> + }
> +
> + /* create the lock file */
> + if ((ret = virDomainSafeConsoleLockFileCreate(pty)) < 0) {
> + virMutexUnlock(&cons->lock);
> + return ret;
> + }
> +
> + /* obtain a reference to the stream */
> + if (virStreamRef(st) < 0) {
> + virMutexUnlock(&cons->lock);
> + return -1;
> + }
If we get here, we added a reference to st,
> +
> + if (VIR_ALLOC(cbdata) < 0)
> + goto error;
> +
> + if (virHashAddEntry(cons->hash, pty, st) < 0)
> + goto error;
> +
> + savedStream = st;
> + st = NULL;
but here, st is set to NULL,
> +
> + cbdata->cons = cons;
> + if (!(cbdata->pty = strdup(pty)))
> + goto error;
and an OOM here,
> +
> + /* open the console pty */
> + if (virFDStreamOpenFile(savedStream, pty, 0, 0, O_RDWR) < 0)
> + goto error;
> +
> +
> + /* add cleanup callback */
> + virFDStreamSetInternalCloseCb(savedStream,
> + virDomainSafeConsoleFDStreamCloseCb,
> + cbdata,
> + virDomainSafeConsoleFDStreamCloseCbFree);
> + cbdata = NULL;
> +
> + virMutexUnlock(&cons->lock);
> + return 0;
> +
> +error:
> + virStreamFree(st);
means that we never remove our reference. You've created a resource
leak on OOM. I'd rearrange the code to sink the swap from st over to
savedStream to occur later on, when we know we are successful.
> + virHashRemoveEntry(cons->hash, pty);
> + if (cbdata)
> + VIR_FREE(cbdata->pty);
> + VIR_FREE(cbdata);
> + virMutexUnlock(&cons->lock);
> + return -1;
> +}
> diff --git a/src/util/domain_safe_console.h b/src/util/domain_safe_console.h
> new file mode 100644
> index 0000000..39536f2
> --- /dev/null
> +++ b/src/util/domain_safe_console.h
> @@ -0,0 +1,28 @@
> +/**
> + * domain_console_lock.h: api to guarantee mutualy exclusive
> + * access to domain consoles
> + *
> + * Copyright (C) 2011 Red Hat, Inc.
> + *
> + * See COPYING.LIB for the License of this software
Same comments as for the .c file (2012, 3-paragraph form).
Definitely some things to fix in v3, but looking like a decent start.
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list