header image
 

How not to design APIs and other coding horrors

Exhibit one: Xen’s libxc, a control library for the hypervisor.

/*
 * Return an fd that can be select()ed on.
 *
 * Note that due to bugs, setting this fd to non blocking may not
 * work: you would hope that it would result in xc_evtchn_pending
 * failing with EWOULDBLOCK if there are no events signaled, but in
 * fact it may block.  (Bug is present in at least Linux 3.12, and
 * perhaps on other platforms or later version.)
 *
 * To be safe, you must use poll() or select() before each call to
 * xc_evtchn_pending.  If you have multiple threads (or processes)
 * sharing a single xce handle this will not work, and there is no
 * straightforward workaround.  Please design your program some other
 * way.
 */
int xc_evtchn_fd(xc_evtchn *xce);

That’s very cross-platform. I’m sure I can select() on a fd on Windows.

/*
 * Creates and shares a page with another domain, with unmap notification.
 * 
 * @parm xcg a handle to an open grant sharing instance
 * @parm domid the domain to share memory with
 * @parm refs the grant reference of the pages (output)
 * @parm writable true if the other domain can write to the page
 * @parm notify_offset The byte offset in the page to use for unmap
 *                     notification; -1 for none.
 * @parm notify_port The event channel port to use for unmap notify, or -1
 * @return local mapping of the page
 */
void *xc_gntshr_share_page_notify(xc_gntshr *xcg, uint32_t domid,
                                  uint32_t *ref, int writable,
                                  uint32_t notify_offset,
                                  evtchn_port_t notify_port);

I love magic parameters! Especially if you compare an unsigned value to -1.

~ ~ ~

Exhibit two: libxenvchan, a inter-domain communication library for Xen. Let’s look at a few function prototypes:

struct libxenvchan *
libxenvchan_server_init(
    xentoollog_logger *logger,
    int domain,
    const char* xs_path,
    size_t read_min,
    size_t write_min
    );

/**
 * Packet-based receive: always reads exactly $size bytes.
 * @param ctrl The vchan control structure
 * @param data Buffer for data that was read
 * @param size Size of the buffer and amount of data to read
 * @return -1 on error, 0 if nonblocking and insufficient data is available, or $size
 */
int
libxenvchan_recv(
    struct libxenvchan *ctrl,
    void *data,
    size_t size
    );

/** Amount of data it is possible to send without blocking */
int
libxenvchan_buffer_space(
    struct libxenvchan *ctrl
    );

Notice anything strange? All size input parameters are of type size_t. libxenvchan_recv returns -1 on error, 0 if nonblocking and insufficient data is available, or $size. I wonder what does it return if I pass size that’s bigger than INT_MAX?

/**
 * reads exactly size bytes from the vchan.
 * returns 0 if insufficient data is available, -1 on error, or size on success
 */
int libxenvchan_recv(struct libxenvchan *ctrl, void *data, size_t size)
{
    while (1) {
        int avail = fast_get_data_ready(ctrl, size);
        if (size <= avail)
            return do_recv(ctrl, data, size);
        if (!libxenvchan_is_open(ctrl))
            return -1;
        if (!ctrl->blocking)
            return 0;
        if (size > rd_ring_size(ctrl))
            return -1;
        if (libxenvchan_wait(ctrl))
            return -1;
    }
}

static int do_recv(struct libxenvchan *ctrl, void *data, size_t size)
{
    int real_idx = rd_cons(ctrl) & (rd_ring_size(ctrl) - 1);
    int avail_contig = rd_ring_size(ctrl) - real_idx;
    if (avail_contig > size)
        avail_contig = size;

    // ...snip...

    return size;
}

…oh.

Internally all sizes are truncated to int without any warning. I’ve also learned that gcc allows pointer arithmetic on void* without any warnings by default:

case SMALL_RING_SHIFT:
    ctrl->read.buffer = ((void*)ctrl->ring) + SMALL_RING_OFFSET;
    break;
case LARGE_RING_SHIFT:
    ctrl->read.buffer = ((void*)ctrl->ring) + LARGE_RING_OFFSET;
    break;

Oh, and the only way to set the communication mode to blocking is by setting a field in the control structure directly. Why is the structure exposed to clients instead of being an opaque pointer again?

~ ~ ~

And lastly, a great implementation of __sync_fetch_and_and on Windows.

These builtins perform the operation suggested by the name, and returns the value that had previously been in memory. That is,
{ tmp = *ptr; *ptr op= value; return tmp; }
/* unfortunatelly no 8-bit equivalent for Interlocked{And,Or} */
#define __sync_or_and_fetch(a, b) (*(a)) |= (b)
#define __sync_fetch_and_and(a, b) (*(a)) &= (b)

I don’t even know what to say at this point. And yes, InterlockedAnd8 exists.

~ by omeg on May 24, 2015.

C/C++, rant, wtf

Leave a Reply