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.
C/C++, rant, wtf
Leave a Reply