]> git.ipfire.org Git - thirdparty/rrdtool-1.x.git/commit
add read locking through rrd_open() (#800)
authorAlan Jenkins <alan.christopher.jenkins@gmail.com>
Fri, 7 Jul 2017 10:16:06 +0000 (11:16 +0100)
committerTobias Oetiker <tobi@oetiker.ch>
Fri, 7 Jul 2017 10:16:06 +0000 (12:16 +0200)
commit248671a941dad879cc90dfe081a1ea77cc34aec1
tree6da969666dc4ec7dac7f6e7cb9918a36e02999d5
parentda65effdeee895f3bbb7b41e5f8af77c7ab47afc
add read locking through rrd_open() (#800)

* fix cleanup in rrd_open()

* rados
  * don't leak rrd_file->rados
  * don't mysteriously close STDIN
* !HAVE_MMAP
  * don't leak rrd->stat_head

I have no explanation for why we were chosing to leak rrd->stat_head
(and only rrd->stat_head).  If it _was_ required for some reason, there was
a bug anyway, as we did not leak it if we fail to allocate live_hdr.

If it's required by legacy ABI, then we should remove rrd_open() from the
public API (and bump the soname).  It was originally scheduled for removal
in v1.4.

* avoid unmapping pages that don't belong to us in rrd_close()

When a rados RRD is closed, don't `munmap(0, rrd_file->file_len)` !

Note munmap() does not return an error when there were no pages mapped, so
our callers wouldn't notice even if they check the result of rrd_close().

Also fix the code for munmap errors - so it doesn't format one message and
then immediately replace it with a less specific one.  (If both munmap()
and close() fails though, we will describe the later error only).

Also, there's no point rrd_close() setting rrd_file to NULL at the end,
it won't affect the variable used by the caller.

* Add read locking through rrd_open()

Read lock or write lock is taken according to whether RRD_READONLY or
RRD_READWRITE is passed.  For librrd compat, the lock is only taken
when explicitly requested by passing RRD_LOCK.  Similarly rrd_lock()
is still available, but it should not be needed by new code.

So `RRD_LOCK|RRD_READONLY` is intended to take a read lock, however I must
admit to limitations of this new code:

* On rados, our write lock expires after 2 seconds.  This write-lock is
  probably useful as a way to detect conflicts.  However I would not want
  to rely on a lock that works like this to guarantee consistent reads for
  a backup.  I did not add read locking on rados.

  If rados users need to e.g. obtain a consistent backup, they should
  create a rados snapshot.   (This is able to work because we use a single
  atomic rados operation when when we *write* to RRDs).

* On the Windows _locking() function, read locks are treated the same as
  write locks.

  https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/locking

  The Windows code required several changes, and has not been compiled
  as of this moment.

With Windows _locking(), we should have been unlocking before close().
Apparently the automatic unlock on close() is not guaranteed to occur
synchronously.  This isn't specific to _locking(), it's a limitation
of winapi LockFileEx().

https://docs.microsoft.com/cpp/c-runtime-library/reference/locking

https://msdn.microsoft.com/en-us/library/windows/desktop/aa365203(v=vs.85).aspx

To manually unlock, we need to know what we actually locked.
But on Windows, rrd_lock() was locking the bytes starting at the current
rrd_file->pos, instead of always starting from 0.

Also, I thought RRD_LOCK should lock the file immediately, to guarantee we
don't have conflicting writes.  But on Windows we only tried to lock
the bytes that already existed in the file... which would be 0 bytes for
RRD_CREATE.  Fortunately, even Windows recognizes this problem and allows
locking bytes that don't exist yet.  So we can just lock the maximum number
of bytes allowed.  Locking 1 byte would probably work equally well for us,
but using LONG_MAX seems cleaner to me.
13 files changed:
src/rrd_create.c
src/rrd_daemon.c
src/rrd_dump.c
src/rrd_fetch.c
src/rrd_first.c
src/rrd_info.c
src/rrd_last.c
src/rrd_lastupdate.c
src/rrd_open.c
src/rrd_resize.c
src/rrd_tool.h
src/rrd_tune.c
src/rrd_update.c