4:08 a.m.
On 31/03/16 20:13, Daniel P. Berrange wrote:
On Thu, Mar 31, 2016 at 07:49:02PM +0200, Erik Skultety wrote:
> If the API isn't closed, a situation with 2 setters where one is about to
> define a set of outputs and the other is already defining a new one, may occur.
> By resetting all outputs, all file descriptors are closed. However, the other
> setter may still have a dangling reference to a file descriptor which may have
> already been closed.
> ---
> src/libvirt_private.syms | 2 ++
> src/util/virlog.c | 15 +++++++++++++++
> src/util/virlog.h | 2 ++
> 3 files changed, 19 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index cc40b46..14047f5 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1741,6 +1741,8 @@ virLockSpaceReleaseResourcesForOwner;
>
>
> # util/virlog.h
> +virLogAPILock;
> +virLogAPIUnlock;
> virLogDefineFilters;
> virLogDefineOutputs;
> virLogFilterListFree;
> diff --git a/src/util/virlog.c b/src/util/virlog.c
> index 769dcec..6aa9c91 100644
> --- a/src/util/virlog.c
> +++ b/src/util/virlog.c
> @@ -128,6 +128,21 @@ static void virLogOutputToFd(virLogSourcePtr src,
> void *data);
>
>
> +/* Setters need to be serialized on API entry point */
> +static virMutex virLogAPIMutex;
> +
> +void
> +virLogAPILock(void)
> +{
> + virMutexLock(&virLogAPIMutex);
> +}
> +
> +void
> +virLogAPIUnlock(void)
> +{
> + virMutexUnlock(&virLogAPIMutex);
> +}
> +
> /*
> * Logs accesses must be serialized though a mutex
> */
> diff --git a/src/util/virlog.h b/src/util/virlog.h
> index 1c55a48..f5c0a4f 100644
> --- a/src/util/virlog.h
> +++ b/src/util/virlog.h
> @@ -203,6 +203,8 @@ extern void virLogFilterListFree(virLogFilterPtr *list, int
count);
> * Internal logging API
> */
>
> +extern void virLogAPILock(void);
> +extern void virLogAPIUnlock(void);
> extern void virLogLock(void);
> extern void virLogUnlock(void);
I'm not really seeing the reason why we need a second mutex instead of
just ensuring we acquire the existing mutex in the right places.
Regards,
Daniel
Could you provide more details on this, because what I had in mind (and
I still see it that way), if we agree that operation of setting log
outputs exposed via public API should be done atomically for the whole
set rather than doing it gradually for each output, then there isn't
much else to do. From my point of view, the critical section should be
as small as possible, so moving the locks to the place where I proposed
to use the API locks would mean that the critical section would include
the parsing as well, not sure if that's the best approach. Let's say we
do it that way, we put our existing log mutex to virLogSetOutputs entry
point. The other thing is that from the point we start parsing to the
point where the outputs are actually defined, a number of errors could
occur and none of them can be reported, otherwise a deadlock occurs.
We also cannot omit locking at the entry point completely, i.e. only
having our original virLogMutex where I proposed to have it - before the
actual definition takes place. Imagine 2 admin clients, both attempting
to set their own set of outputs, sharing some of those outputs with the
currently defined global set of log outputs. Now, both clients create
their own private copies, opening all outputs that are not defined yet
and copying all file descriptors from outputs that should continue to
exist once the new copy replaces the original. The tricky part: client
n.1 acquires the lock, swaps the original with its copy and forces the
original to close all outputs that aren't shared between copy n.1 and
the original. Now client n.2 acquires the lock and attempts to swap copy
n.1 with its copy n.2. The problem is, that copy n2. wasn't created
against copy n.1, instead, it was created against the original. So, it
might happen that copy n.2 now might have some invalid file descriptors
(that client n.1 forced the original to close, since these two didn't
share them). If used with the new mutex I proposed, this scenario can't
happen (similar to what I actually wrote into the cover-letter).
Other solution that might (might as well not) work, rather than copying
file descriptors and picking which output should or should not be
closed, we could open a single file repeatedly (increasing the counter
of references to an open file), with the exception of syslog and
journald, that will both have to be special-cased inside the critical
section. This way, syslog is ok, journald is ok, and all files can be
safely closed because most of the time, only counter will be
decremented, thus no dangling file descriptor can exist. Not sure
however, if we want to force open each file multiple times instead of
copying the existing file descriptors.
Anyway, I'd like to hear your opinion and see your idea, how would you
approach it.
Erik