jeffpc / nx01 Goto Github PK

On Illumos, there is pthread_cond_reltimedwait_np which works fine. Sadly, Linux doesn't have this POSIX extension. The good news is, it should be relatively easy to provide an implementation using the absolute timed condition wait (pthread_cond_timedwait).

If arc4random() is available, use it.

This is related to #3.

Not only does libumem provide object caching, it provides some rather nice debugging features. Therefore, we should link all our binaries against it.

There are different ways to generate decent random integers. Sadly, there isn't an easy way defined by POSIX so we'll have to make a wrapper. Some sources of randomness include:

• some systems provide uint32_t arc4random(void) which works well
• all Unix systems have /dev/random and /dev/urandom (this wastes a file descriptor)
• we could import something like mt19937 and seed it using something more heavy weight like /dev/urandom

Allow creation of objects.

Instead of relying on libjeffpc to be in the system's library path, allow the user to specify the location manually when running cmake.

The client receives the RPC args, passes them to the handler function, and then fails to free them. Similarly, the client sends the RPC response structs, and then fails to free them. This leads to memory leaks. E.g.,

vclock leak: 2 buffers, 16 bytes each, 32 bytes total
            ADDR          BUFADDR        TIMESTAMP           THREAD
                            CACHE          LASTLOG         CONTENTS
         8eef7a8          8eebf80      46c356b0247               64
                          8ecb590                0                0
                 libumem.so.1`umem_cache_alloc_debug+0x1fe
                 libumem.so.1`umem_cache_alloc+0x18f
                 libnomad_common.so`nvclock_alloc+0x1f
                 libnomad_common.so`xdr_nvclock+0x96
                 libnomad_common.so`xdr_nobjhndl+0x35
                 libnomad_common.so`xdr_rpc_create_req+0x21
                 fetch_args.isra.0+0xd
                 process_connection+0xf2
                 connection_acceptor+0x39
                 libnomad_common.so`wrap_taskq_callback+0x20
                 libsuntaskq.so`taskq_thread+0xa0
                 libc_hwcap1.so.1`_thrp_setup+0x88
                 libc_hwcap1.so.1`_lwp_start

and

umem_alloc_16 leak: 2 buffers, 16 bytes each, 32 bytes total
            ADDR          BUFADDR        TIMESTAMP           THREAD
                            CACHE          LASTLOG         CONTENTS
         8ed9648          8ed5de0      46c356b11d0               64
                          8ea4590                0                0
                 libumem.so.1`umem_cache_alloc_debug+0x1fe
                 libumem.so.1`umem_cache_alloc+0x18f
                 libumem.so.1`umem_alloc+0x50
                 libumem.so.1`umem_malloc+0x36
                 libumem.so.1`realloc+0x28
                 libnsl.so.1`xdr_string+0xfe
                 libnomad_common.so`xdr_rpc_create_req+0x47
                 fetch_args.isra.0+0xd
                 process_connection+0xf2
                 connection_acceptor+0x39
                 libnomad_common.so`wrap_taskq_callback+0x20
                 libsuntaskq.so`taskq_thread+0xa0
                 libc_hwcap1.so.1`_thrp_setup+0x88
                 libc_hwcap1.so.1`_lwp_start

Instead of keeping a private copy just for us, move the code into a submodule.

It'd be quite useful to have a way to store information between daemon and/or computer restarts.

For example, we could keep a list of vgs and vols that were in-use before a restart. Given such a list, we could load them up on the next start.

Note: this file does not have to be human readable

It looks like left over 0x in each line.

Instead of keeping a private copy just for us, move the code into a submodule.

Design a protocol that will allow the client and the fs component to communicate with each other.

As a datapoint, Minix3 uses a fixed size messages - 64 bytes. For larger messages (e.g., read or write system calls), they pass pointers in the message and the recipient has to get the buffer. This sort of approach seems extremely simple. Obviously, passing pointers won't work in this case, however some other mechanism can be used to transfer the oversized buffers.

The fs RPC changed in 47eeff8 and the fs code needs to be updated to follow suit.

instead of sprinkling the codebase with printf & fprintf calls, provide the one logging function that does the right thing. This may or may not involve logging to syslog as well. Any objections to calling it cmn_err?

Some useful features this function should have:

• take a severity argument (debug, info, warning, error, critical, panic)
• prefix messages with current thread id
• automatically append a \n

Instead of having a number of init functions to call that initialize different parts of the common lib, provide one common library init function that initializes everything within the lib.

Versioning of shared object symbols is important. It is ok to have a "private" verision that no one should be linking against.

E.g.,

SYMBOL_VERSION VERS_0.1 {
        global:
                foo;
};

SYMBOL_VERSION PRIVATE_0.1 {
        global:
                bar;
        local:
                *;
};

The top level cmake file specifies the minimum cmake version to be 2.8.5. Unfortunately, at the very least 2.8.9 fails to work because of the add_library ALIAS command in src/common/CMakeLists.txt. It looks like 2.8.12 added this functionality.

see summary

The daemon receives a version over the socket and then (indirectly) forwards it to the storage backends. The mem backend currently ignores the version info and always uses the default version. This should be fixed.

It'd be nice to have a script that you can run before you push your changes. This script would verify that whatever you are about to push passes some simple sanity checks. These checks are meant to prevent obvious doh moments, not to determine the worthiness of a commit. (IOW, it's possible for this script to scream left and right, but the commit to be quite fine as is.)

Some possible checks:

• you included a copyright header
• the copyright header contains this year
• signoffs (yours + at least one other)

Each host has a random (unique!) 64-bit ID. This ID needs to be persistent between restarts since it is used by vector clocks to properly version objects.

We should store this in some sort of "config" file that's not necessarily human readable. (See bug #37.)

The whole store (memstore) already has a lock. This protects the object lookup. Each object needs a lock of its own to protect against concurrent accesses to the object. Finally, each object should have a reference count that protects it from use-after-free due to a race.

Adding the refcount will be very easy once #47 hits.

Currently, the client daemon treats the connection as totally stateless. Each RPC executes as equals. The procotol spec however says that many of the RPCs are allowed only after a successful LOGIN. To allow for this, we should keep some sort of state between RPCs. It should be as easy as allocating a structure (struct fsconn?) when we receive a connection and passing it to all the cmd_foo handlers.

The struct can be unused (it'll need a dummy var).

Hard-coding /etc/nomad.conf is not the best way to do things.

One of the RPCs causes the root object to leak a reference.

I already have some code that makes it brain-dead simple to add refcounts to any structure. I need to import it into the repo.

Instead of keeping a private copy just for us, move the code into a submodule.

Some RPCs require a LOGIN. The "do we have a vg from a LOGIN" check should be implemented generically (in cmds.c).

We currently pass the file mode from the user all the way to the backend. We should do some basic sanity checking in the generic code. (The alternative is to duplicate that check in every backend. Yuck!)

suntaskq needs them, and chances are that there are other potential users.

Allow objects to be removed.

LOGIN looks up a vg. If the lookup is successful, it should stash it in the fs connection state so that subsequent RPCs can get at it.

depends on #31

Instead of asserting, the config loading function (load_config) should propagate the errors up to the caller.

Not all RPCs are stateless. Instead of having everyone guess what can execute when, document it in the protocol spec.

We already need these functions.

nvclock_cmp() compares two vector clocks: returning NVC_LT, NVC_EQ, NVC_GT, NVC_DIV depending on whether the first argument is less than, equal to, greater than the second, or if the two clocks diverged and therefore neither is an ancestor of the other.

nvclock_cmp_total() returns -1, 0, +1 if the first arg is less than, equal to, or greater than the second. If the two vector clocks have diverged (nvclock_cmp() would return NVC_DIV), then nvclock_cmp_total() returns either -1 or +1 (but is consisent!). This provides a total ordering of vector clocks. This is useful as a comparator callback for avl_create() etc.

It's hard to explain a coding style using random examples. We should write it down.

The change in 3ecfb91 made the vector clock sent over the wire variable length. This decouples the over the wire format from the structure that we're using in C. Unfortunately, the fs code got left in the dust.

Generate a header file that contains all the #defines defining what the system is like - what functions are available, etc.

Depends on #55.

Instead of keeping a private copy just for us, move the code into a submodule.