[xferbuf] Simplify and generalise data transfer buffers

Since all data transfer buffer contents are now accessible via direct
pointer dereferences, remove the unnecessary abstractions for read and
write operations and create two new data transfer buffer types: a
fixed-size buffer, and a void buffer that records its size but can
never receive non-zero lengths of data.  These replace the custom data
buffer types currently implemented for EFI PXE TFTP downloads and for
block device translations.

A new operation xferbuf_detach() is required to take ownership of the
data accumulated in the data transfer buffer, since we no longer rely
on the existence of an independently owned external data pointer for
data transfer buffers allocated via umalloc().

Signed-off-by: Michael Brown <mcb30@ipxe.org>

src/core/blocktrans.c

@@ -38,91 +38,6 @@ FILE_LICENCE ( GPL2_OR_LATER_OR_UBDL );
 #include <ipxe/blockdev.h>
 #include <ipxe/blocktrans.h>
 
-/**
- * Reallocate block device translator data buffer
- *
- * @v xferbuf		Data transfer buffer
- * @v len		New length (or zero to free buffer)
- * @ret rc		Return status code
- */
-static int blktrans_xferbuf_realloc ( struct xfer_buffer *xferbuf,
-				      size_t len ) {
-	struct block_translator *blktrans =
-		container_of ( xferbuf, struct block_translator, xferbuf );
-
-	/* Record length, if applicable */
-	if ( blktrans->buffer ) {
-
-		/* We have a (non-reallocatable) data buffer */
-		return -ENOTSUP;
-
-	} else {
-
-		/* Record length (for block device capacity) */
-		xferbuf->len = len;
-		return 0;
-	}
-}
-
-/**
- * Write data to block device translator data buffer
- *
- * @v xferbuf		Data transfer buffer
- * @v offset		Starting offset
- * @v data		Data to copy
- * @v len		Length of data
- */
-static void blktrans_xferbuf_write ( struct xfer_buffer *xferbuf, size_t offset,
-				     const void *data, size_t len ) {
-	struct block_translator *blktrans =
-		container_of ( xferbuf, struct block_translator, xferbuf );
-
-	/* Write data to buffer, if applicable */
-	if ( blktrans->buffer ) {
-
-		/* Write data to buffer */
-		memcpy ( ( blktrans->buffer + offset ), data, len );
-
-	} else {
-
-		/* Sanity check */
-		assert ( len == 0 );
-	}
-}
-
-/**
- * Read data from block device translator data buffer
- *
- * @v xferbuf		Data transfer buffer
- * @v offset		Starting offset
- * @v data		Data to read
- * @v len		Length of data
- */
-static void blktrans_xferbuf_read ( struct xfer_buffer *xferbuf, size_t offset,
-				    void *data, size_t len ) {
-	struct block_translator *blktrans =
-		container_of ( xferbuf, struct block_translator, xferbuf );
-
-	/* Read data from buffer, if applicable */
-	if ( blktrans->buffer ) {
-
-		/* Read data from buffer */
-		memcpy ( data, ( blktrans->buffer + offset ), len );
-
-	} else {
-
-		/* Sanity check */
-		assert ( len == 0 );
-	}
-}
-
-/** Block device translator data transfer buffer operations */
-static struct xfer_buffer_operations blktrans_xferbuf_operations = {
-	.realloc = blktrans_xferbuf_realloc,
-	.write = blktrans_xferbuf_write,
-	.read = blktrans_xferbuf_read,
-};
-
 /**
  * Close block device translator
  *
@@ -233,11 +148,10 @@ int block_translate ( struct interface *block, void *buffer, size_t size ) {
 	ref_init ( &blktrans->refcnt, NULL );
 	intf_init ( &blktrans->block, &blktrans_block_desc, &blktrans->refcnt );
 	intf_init ( &blktrans->xfer, &blktrans_xfer_desc, &blktrans->refcnt );
-	blktrans->xferbuf.op = &blktrans_xferbuf_operations;
-	blktrans->buffer = buffer;
 	if ( buffer ) {
-		blktrans->xferbuf.len = size;
+		xferbuf_fixed_init ( &blktrans->xferbuf, buffer, size );
 	} else {
+		xferbuf_void_init ( &blktrans->xferbuf );
 		blktrans->blksize = size;
 	}
 

src/core/downloader.c

@@ -67,6 +67,7 @@ static void downloader_free ( struct refcnt *refcnt ) {
 	struct downloader *downloader =
 		container_of ( refcnt, struct downloader, refcnt );
 
+	xferbuf_free ( &downloader->buffer );
 	image_put ( downloader->image );
 	free ( downloader );
 }
@@ -78,18 +79,21 @@ static void downloader_free ( struct refcnt *refcnt ) {
  * @v rc		Reason for termination
  */
 static void downloader_finished ( struct downloader *downloader, int rc ) {
+	struct xfer_buffer *buffer = &downloader->buffer;
+	struct image *image = downloader->image;
 
 	/* Log download status */
 	if ( rc == 0 ) {
-		syslog ( LOG_NOTICE, "Downloaded \"%s\"\n",
+		syslog ( LOG_NOTICE, "Downloaded \"%s\"\n", image->name );
-			 downloader->image->name );
 	} else {
 		syslog ( LOG_ERR, "Download of \"%s\" failed: %s\n",
-			 downloader->image->name, strerror ( rc ) );
+			 image->name, strerror ( rc ) );
 	}
 
-	/* Update image length */
+	/* Transfer ownership from data transfer buffer to image */
-	downloader->image->len = downloader->buffer.len;
+	image->data = buffer->data;
+	image->len = buffer->len;
+	xferbuf_detach ( buffer );
 
 	/* Shut down interfaces */
 	intf_shutdown ( &downloader->xfer, rc );
@@ -269,7 +273,7 @@ int create_downloader ( struct interface *job, struct image *image ) {
 	intf_init ( &downloader->xfer, &downloader_xfer_desc,
 		    &downloader->refcnt );
 	downloader->image = image_get ( image );
-	xferbuf_umalloc_init ( &downloader->buffer, &image->data );
+	xferbuf_umalloc_init ( &downloader->buffer );
 
 	/* Instantiate child objects and attach to our interfaces */
 	if ( ( rc = xfer_open_uri ( &downloader->xfer, image->uri ) ) != 0 )

src/core/xferbuf.c

@@ -50,6 +50,21 @@ static struct profiler xferbuf_write_profiler __profiler =
 static struct profiler xferbuf_read_profiler __profiler =
 	{ .name = "xferbuf.read" };
 
+/**
+ * Detach data from data transfer buffer
+ *
+ * @v xferbuf		Data transfer buffer
+ *
+ * The caller assumes responsibility for eventually freeing the data
+ * previously owned by the data transfer buffer.
+ */
+void xferbuf_detach ( struct xfer_buffer *xferbuf ) {
+
+	xferbuf->data = NULL;
+	xferbuf->len = 0;
+	xferbuf->pos = 0;
+}
+
 /**
  * Free data transfer buffer
  *
@@ -58,8 +73,7 @@ static struct profiler xferbuf_read_profiler __profiler =
 void xferbuf_free ( struct xfer_buffer *xferbuf ) {
 
 	xferbuf->op->realloc ( xferbuf, 0 );
-	xferbuf->len = 0;
+	xferbuf_detach ( xferbuf );
-	xferbuf->pos = 0;
 }
 
 /**
@@ -109,9 +123,13 @@ int xferbuf_write ( struct xfer_buffer *xferbuf, size_t offset,
 	if ( ( rc = xferbuf_ensure_size ( xferbuf, max_len ) ) != 0 )
 		return rc;
 
+	/* Check that buffer is non-void */
+	if ( len && ( ! xferbuf->data ) )
+		return -ENOTTY;
+
 	/* Copy data to buffer */
 	profile_start ( &xferbuf_write_profiler );
-	xferbuf->op->write ( xferbuf, offset, data, len );
+	memcpy ( ( xferbuf->data + offset ), data, len );
 	profile_stop ( &xferbuf_write_profiler );
 
 	return 0;
@@ -133,9 +151,13 @@ int xferbuf_read ( struct xfer_buffer *xferbuf, size_t offset,
 	     ( len > ( xferbuf->len - offset ) ) )
 		return -ENOENT;
 
+	/* Check that buffer is non-void */
+	if ( len && ( ! xferbuf->data ) )
+		return -ENOTTY;
+
 	/* Copy data from buffer */
 	profile_start ( &xferbuf_read_profiler );
-	xferbuf->op->read ( xferbuf, offset, data, len );
+	memcpy ( data, ( xferbuf->data + offset ), len );
 	profile_stop ( &xferbuf_read_profiler );
 
 	return 0;
@@ -178,7 +200,7 @@ int xferbuf_deliver ( struct xfer_buffer *xferbuf, struct io_buffer *iobuf,
 }
 
 /**
- * Reallocate malloc()-based data buffer
+ * Reallocate malloc()-based data transfer buffer
  *
  * @v xferbuf		Data transfer buffer
  * @v len		New length (or zero to free buffer)
@@ -194,94 +216,76 @@ static int xferbuf_malloc_realloc ( struct xfer_buffer *xferbuf, size_t len ) {
 	return 0;
 }
 
-/**
- * Write data to malloc()-based data buffer
- *
- * @v xferbuf		Data transfer buffer
- * @v offset		Starting offset
- * @v data		Data to copy
- * @v len		Length of data
- */
-static void xferbuf_malloc_write ( struct xfer_buffer *xferbuf, size_t offset,
-				   const void *data, size_t len ) {
-
-	memcpy ( ( xferbuf->data + offset ), data, len );
-}
-
-/**
- * Read data from malloc()-based data buffer
- *
- * @v xferbuf		Data transfer buffer
- * @v offset		Starting offset
- * @v data		Data to read
- * @v len		Length of data
- */
-static void xferbuf_malloc_read ( struct xfer_buffer *xferbuf, size_t offset,
-				  void *data, size_t len ) {
-
-	memcpy ( data, ( xferbuf->data + offset ), len );
-}
-
 /** malloc()-based data buffer operations */
 struct xfer_buffer_operations xferbuf_malloc_operations = {
 	.realloc = xferbuf_malloc_realloc,
-	.write = xferbuf_malloc_write,
-	.read = xferbuf_malloc_read,
 };
 
 /**
- * Reallocate umalloc()-based data buffer
+ * Reallocate umalloc()-based data transfer buffer
  *
  * @v xferbuf		Data transfer buffer
  * @v len		New length (or zero to free buffer)
  * @ret rc		Return status code
  */
 static int xferbuf_umalloc_realloc ( struct xfer_buffer *xferbuf, size_t len ) {
-	void **udata = xferbuf->data;
 	void *new_udata;
 
-	new_udata = urealloc ( *udata, len );
+	new_udata = urealloc ( xferbuf->data, len );
 	if ( ! new_udata )
 		return -ENOSPC;
-	*udata = new_udata;
+	xferbuf->data = new_udata;
 	return 0;
 }
 
-/**
- * Write data to umalloc()-based data buffer
- *
- * @v xferbuf		Data transfer buffer
- * @v offset		Starting offset
- * @v data		Data to copy
- * @v len		Length of data
- */
-static void xferbuf_umalloc_write ( struct xfer_buffer *xferbuf, size_t offset,
-				    const void *data, size_t len ) {
-	void **udata = xferbuf->data;
-
-	memcpy ( ( *udata + offset ), data, len );
-}
-
-/**
- * Read data from umalloc()-based data buffer
- *
- * @v xferbuf		Data transfer buffer
- * @v offset		Starting offset
- * @v data		Data to read
- * @v len		Length of data
- */
-static void xferbuf_umalloc_read ( struct xfer_buffer *xferbuf, size_t offset,
-				   void *data, size_t len ) {
-	void **udata = xferbuf->data;
-
-	memcpy ( data, ( *udata + offset ), len );
-}
-
 /** umalloc()-based data buffer operations */
 struct xfer_buffer_operations xferbuf_umalloc_operations = {
 	.realloc = xferbuf_umalloc_realloc,
-	.write = xferbuf_umalloc_write,
+};
-	.read = xferbuf_umalloc_read,
+
+/**
+ * Reallocate fixed-size data transfer buffer
+ *
+ * @v xferbuf		Data transfer buffer
+ * @v len		New length (or zero to free buffer)
+ * @ret rc		Return status code
+ */
+static int xferbuf_fixed_realloc ( struct xfer_buffer *xferbuf, size_t len ) {
+
+	/* Refuse to allocate extra space */
+	if ( len > xferbuf->len ) {
+		/* Note that EFI relies upon this error mapping to
+		 * EFI_BUFFER_TOO_SMALL.
+		 */
+		return -ERANGE;
+	}
+
+	return 0;
+}
+
+/** Fixed-size data buffer operations */
+struct xfer_buffer_operations xferbuf_fixed_operations = {
+	.realloc = xferbuf_fixed_realloc,
+};
+
+/**
+ * Reallocate void data transfer buffer
+ *
+ * @v xferbuf		Data transfer buffer
+ * @v len		New length (or zero to free buffer)
+ * @ret rc		Return status code
+ */
+static int xferbuf_void_realloc ( struct xfer_buffer *xferbuf,
+				  size_t len __unused ) {
+
+	/* Succeed without ever allocating data */
+	assert ( xferbuf->data == NULL );
+	return 0;
+}
+
+/** Void data buffer operations */
+struct xfer_buffer_operations xferbuf_void_operations = {
+	.realloc = xferbuf_void_realloc,
 };
 
 /**

src/include/ipxe/blocktrans.h

@@ -25,8 +25,6 @@ struct block_translator {
 
 	/** Data transfer buffer */
 	struct xfer_buffer xferbuf;
-	/** Data buffer */
-	void *buffer;
 	/** Block size */
 	size_t blksize;
 };

src/include/ipxe/xferbuf.h

@@ -35,41 +35,19 @@ struct xfer_buffer_operations {
 	 * @ret rc		Return status code
 	 */
 	int ( * realloc ) ( struct xfer_buffer *xferbuf, size_t len );
-	/** Write data to buffer
-	 *
-	 * @v xferbuf		Data transfer buffer
-	 * @v offset		Starting offset
-	 * @v data		Data to write
-	 * @v len		Length of data
-	 *
-	 * This call is simply a wrapper for the appropriate
-	 * memcpy()-like operation: the caller is responsible for
-	 * ensuring that the write does not exceed the buffer length.
-	 */
-	void ( * write ) ( struct xfer_buffer *xferbuf, size_t offset,
-			   const void *data, size_t len );
-	/** Read data from buffer
-	 *
-	 * @v xferbuf		Data transfer buffer
-	 * @v offset		Starting offset
-	 * @v data		Data to read
-	 * @v len		Length of data
-	 *
-	 * This call is simply a wrapper for the appropriate
-	 * memcpy()-like operation: the caller is responsible for
-	 * ensuring that the read does not exceed the buffer length.
-	 */
-	void ( * read ) ( struct xfer_buffer *xferbuf, size_t offset,
-			  void *data, size_t len );
 };
 
 extern struct xfer_buffer_operations xferbuf_malloc_operations;
 extern struct xfer_buffer_operations xferbuf_umalloc_operations;
+extern struct xfer_buffer_operations xferbuf_fixed_operations;
+extern struct xfer_buffer_operations xferbuf_void_operations;
 
 /**
  * Initialise malloc()-based data transfer buffer
  *
  * @v xferbuf		Data transfer buffer
+ *
+ * Data will be automatically allocated using malloc().
  */
 static inline __attribute__ (( always_inline )) void
 xferbuf_malloc_init ( struct xfer_buffer *xferbuf ) {
@@ -80,14 +58,45 @@ xferbuf_malloc_init ( struct xfer_buffer *xferbuf ) {
  * Initialise umalloc()-based data transfer buffer
  *
  * @v xferbuf		Data transfer buffer
- * @v data		User pointer
+ *
+ * Data will be automatically allocated using umalloc() (and may
+ * therefore alter the system memory map).
  */
 static inline __attribute__ (( always_inline )) void
-xferbuf_umalloc_init ( struct xfer_buffer *xferbuf, void **data ) {
+xferbuf_umalloc_init ( struct xfer_buffer *xferbuf ) {
-	xferbuf->data = data;
 	xferbuf->op = &xferbuf_umalloc_operations;
 }
 
+/**
+ * Initialise fixed-size data transfer buffer
+ *
+ * @v xferbuf		Data transfer buffer
+ * @v data		Data buffer
+ * @v len		Length of data buffer
+ *
+ * Data will be never be automatically allocated.
+ */
+static inline __attribute__ (( always_inline )) void
+xferbuf_fixed_init ( struct xfer_buffer *xferbuf, void *data, size_t len ) {
+	xferbuf->data = data;
+	xferbuf->len = len;
+	xferbuf->op = &xferbuf_fixed_operations;
+}
+
+/**
+ * Initialise void data transfer buffer
+ *
+ * @v xferbuf		Data transfer buffer
+ *
+ * No data will be allocated, but the length will be recorded.  This
+ * can be used to capture xfer_seek() results.
+ */
+static inline __attribute__ (( always_inline )) void
+xferbuf_void_init ( struct xfer_buffer *xferbuf ) {
+	xferbuf->op = &xferbuf_void_operations;
+}
+
+extern void xferbuf_detach ( struct xfer_buffer *xferbuf );
 extern void xferbuf_free ( struct xfer_buffer *xferbuf );
 extern int xferbuf_write ( struct xfer_buffer *xferbuf, size_t offset,
 			   const void *data, size_t len );

src/interface/efi/efi_pxe.c

@@ -303,48 +303,6 @@ static int efi_pxe_ip_filter ( struct efi_pxe *pxe, EFI_IP_ADDRESS *ip ) {
 	return 0;
 }
 
-/******************************************************************************
- *
- * Data transfer buffer
- *
- ******************************************************************************
- */
-
-/**
- * Reallocate PXE data transfer buffer
- *
- * @v xferbuf		Data transfer buffer
- * @v len		New length (or zero to free buffer)
- * @ret rc		Return status code
- */
-static int efi_pxe_buf_realloc ( struct xfer_buffer *xferbuf __unused,
-				 size_t len __unused ) {
-
-	/* Can never reallocate: return EFI_BUFFER_TOO_SMALL */
-	return -ERANGE;
-}
-
-/**
- * Write data to PXE data transfer buffer
- *
- * @v xferbuf		Data transfer buffer
- * @v offset		Starting offset
- * @v data		Data to copy
- * @v len		Length of data
- */
-static void efi_pxe_buf_write ( struct xfer_buffer *xferbuf, size_t offset,
-				const void *data, size_t len ) {
-
-	/* Copy data to buffer */
-	memcpy ( ( xferbuf->data + offset ), data, len );
-}
-
-/** PXE data transfer buffer operations */
-static struct xfer_buffer_operations efi_pxe_buf_operations = {
-	.realloc = efi_pxe_buf_realloc,
-	.write = efi_pxe_buf_write,
-};
-
 /******************************************************************************
  *
  * (M)TFTP download interface
@@ -966,8 +924,7 @@ efi_pxe_mtftp ( EFI_PXE_BASE_CODE_PROTOCOL *base,
 	pxe->blksize = ( ( callback && blksize ) ? *blksize : -1UL );
 
 	/* Initialise data transfer buffer */
-	pxe->buf.data = data;
+	xferbuf_fixed_init ( &pxe->buf, data, *len );
-	pxe->buf.len = *len;
 
 	/* Open download */
 	if ( ( rc = efi_pxe_tftp_open ( pxe, ip,
@@ -987,6 +944,7 @@ efi_pxe_mtftp ( EFI_PXE_BASE_CODE_PROTOCOL *base,
  err_download:
 	efi_pxe_tftp_close ( pxe, rc );
  err_open:
+	xferbuf_fixed_init ( &pxe->buf, NULL, 0 );
 	efi_snp_release();
  err_opcode:
 	return EFIRC ( rc );
@@ -1611,7 +1569,6 @@ int efi_pxe_install ( EFI_HANDLE handle, struct net_device *netdev ) {
 	pxe->base.Mode = &pxe->mode;
 	memcpy ( &pxe->apple, &efi_apple_net_boot_protocol,
 		 sizeof ( pxe->apple ) );
-	pxe->buf.op = &efi_pxe_buf_operations;
 	intf_init ( &pxe->tftp, &efi_pxe_tftp_desc, &pxe->refcnt );
 	intf_init ( &pxe->udp, &efi_pxe_udp_desc, &pxe->refcnt );
 	INIT_LIST_HEAD ( &pxe->queue );

src/net/peermux.c

@@ -129,6 +129,7 @@ static int peermux_info_deliver ( struct peerdist_multiplexer *peermux,
  * @v rc		Reason for close
  */
 static void peermux_info_close ( struct peerdist_multiplexer *peermux, int rc ){
+	struct xfer_buffer *buffer = &peermux->buffer;
 	struct peerdist_info *info = &peermux->cache.info;
 	size_t len;
 
@@ -145,8 +146,7 @@ static void peermux_info_close ( struct peerdist_multiplexer *peermux, int rc ){
 	intf_shutdown ( &peermux->info, rc );
 
 	/* Parse content information */
-	if ( ( rc = peerdist_info ( info->raw.data, peermux->buffer.len,
+	if ( ( rc = peerdist_info ( buffer->data, buffer->len, info ) ) != 0 ) {
-				    info ) ) != 0 ) {
 		DBGC ( peermux, "PEERMUX %p could not parse content info: %s\n",
 		       peermux, strerror ( rc ) );
 		goto err;
@@ -422,8 +422,7 @@ int peermux_filter ( struct interface *xfer, struct interface *info,
 	intf_init ( &peermux->xfer, &peermux_xfer_desc, &peermux->refcnt );
 	intf_init ( &peermux->info, &peermux_info_desc, &peermux->refcnt );
 	peermux->uri = uri_get ( uri );
-	xferbuf_umalloc_init ( &peermux->buffer,
+	xferbuf_umalloc_init ( &peermux->buffer );
-			       &peermux->cache.info.raw.data );
 	process_init_stopped ( &peermux->process, &peermux_process_desc,
 			       &peermux->refcnt );
 	INIT_LIST_HEAD ( &peermux->busy );