Fix coding style and API in the PHP PECL module

Clean up the coding style for the PHP PECL module to match the rest of
the project and add copyright headers and license pointers.

Change the extended API exposed by the PECL module to match the API
exposed by other extensions.  This means always providing a data key
with the data, not turning it into stdout and stderr similar to the
simple API, and not adding a done key.  This can be reconsidered later
if there's a very native-PHP way of expressing things, but these
differences seemed unnecessary and not specifically driven by being more
PHP-native.

4 files changed, 387 insertions, 361 deletions

diff --git a/php/config.m4 b/php/config.m4
index 867c09f..2d62f48 100644
--- a/php/config.m4
+++ b/php/config.m4
@@ -1,14 +1,21 @@
-PHP_ARG_ENABLE(remctl, whether to enable remctl php extension,
-		[--enable-remctl	Enable recmtl php extension],
-		yes)
+dnl config.m4 -- PHP remctl PECL module configuration.
+dnl
+dnl Provides additional configuration hooks for the PHP module build system.
+dnl This file is used by the phpize frameowrk.
+dnl
+dnl Written by Andrew Mortensen <admorten@umich.edu>, 2008
+dnl Copyright assigned to Russ Allbery, 2008
+dnl Copyright 2008 Board of Trustees, Leland Stanford Jr. University
+dnl
+dnl See LICENSE for licensing terms.
 
-if test "$PHP_REMCTL" != no; then
-    AC_DEFINE(HAVE_REMCTL, 1, [remctl support])
-    PHP_NEW_EXTENSION(remctl, php_remctl.c, $ext_shared)
+PHP_ARG_ENABLE([remctl], [whether to enable remctl PHP extension],
+    [AC_HELP_STRING([--enable-remctl], [Enable recmtl PHP extension])], [yes])
 
-    # these are clumsy. we don't need to give the compiler
-    # the library search path since the bundle is compiled
-    # with "-undefined suppress".
-    PHP_ADD_INCLUDE([\${ac_top_srcdir}/..])
-    LDFLAGS="$LDFLAGS -lremctl"
-fi
+dnl The escaping on PHP_ADD_INCLUDE seems to work, and seems to be the only
+dnl thing that works, but it makes me nervous.  Needs more testing and maybe a
+dnl better approach that will still work for builddir != srcdir builds.
+AS_IF([test "$PHP_REMCTL" != no],
+    [PHP_NEW_EXTENSION([remctl], [php_remctl.c], [$ext_shared])
+     PHP_ADD_INCLUDE([\${ac_top_srcdir}/..])
+     LDFLAGS="$LDFLAGS -lremctl"])
diff --git a/php/php_remctl.c b/php/php_remctl.c
index 4aafc50..9cb56e0 100644
--- a/php/php_remctl.c
+++ b/php/php_remctl.c
@@ -1,3 +1,16 @@
+/*
+ * PHP remctl PECL module.
+ *
+ * Provides bindings for PHP very similar to the libremctl library for C or
+ * the Net::Remctl bindings for Perl.
+ *
+ * Written by Andrew Mortensen <admorten@umich.edu>, 2008
+ * Copyright 2008 Andrew Mortensen <admorten@umich.edu>
+ * Copyright 2008 Board of Trustees, Leland Stanford Jr. University
+ *
+ * See LICENSE for licensing terms.
+ */
+
 #include <config.h>
 
 #include <errno.h>
@@ -9,31 +22,27 @@
 
 #include <client/remctl.h>
 
-#ifndef IOV_MAX
-# define IOV_MAX UIO_MAXIOV
-#endif
-
-#include "php.h"
-#include "php_remctl.h"
+#include <php.h>
+#include <php_remctl.h>
 
-static int			le_remctl_internal;
+static int le_remctl_internal;
 
-static zend_function_entry	remctl_functions[] = {
-    ZEND_FE( remctl, NULL )
-    ZEND_FE( remctl_new, NULL )
-    ZEND_FE( remctl_open, NULL )
-    ZEND_FE( remctl_close, NULL )
-    ZEND_FE( remctl_command, NULL )
-    ZEND_FE( remctl_output, NULL )
-    ZEND_FE( remctl_error, NULL )
-    { NULL, NULL, NULL }
+static zend_function_entry remctl_functions[] = {
+    ZEND_FE(remctl,         NULL)
+    ZEND_FE(remctl_new,     NULL)
+    ZEND_FE(remctl_open,    NULL)
+    ZEND_FE(remctl_close,   NULL)
+    ZEND_FE(remctl_command, NULL)
+    ZEND_FE(remctl_output,  NULL)
+    ZEND_FE(remctl_error,   NULL)
+    { NULL, NULL, NULL, 0, 0 }
 };
 
-zend_module_entry		remctl_module_entry = {
+zend_module_entry remctl_module_entry = {
     STANDARD_MODULE_HEADER,
     PHP_REMCTL_EXTNAME,
     remctl_functions,
-    PHP_MINIT( remctl ),
+    PHP_MINIT(remctl),
     NULL,
     NULL,
     NULL,
@@ -43,386 +52,386 @@ zend_module_entry		remctl_module_entry = {
 };
 
 #ifdef COMPILE_DL_REMCTL
-ZEND_GET_MODULE( remctl )
-#endif /* COMPILE_DL_REMCTL */
+ZEND_GET_MODULE(remctl)
+#endif
 
-    static void
-php_remctl_dtor( zend_rsrc_list_entry *rsrc TSRMLS_DC )
+/*
+ * Destructor for a remctl object.  Close the underlying connection.
+ */
+static void
+php_remctl_dtor(zend_rsrc_list_entry *rsrc TSRMLS_DC)
 {
-    struct remctl		*r = ( struct remctl * )rsrc->ptr;
+    struct remctl *r = (struct remctl *) rsrc->ptr;
 
-    if ( r ) {
-	remctl_close( r );
-    }
+    if (r != NULL)
+        remctl_close(r);
 }
 
-PHP_MINIT_FUNCTION( remctl )
-{
-    le_remctl_internal = zend_register_list_destructors_ex( php_remctl_dtor,
-				NULL, PHP_REMCTL_RES_NAME, module_number );
 
-    return( SUCCESS );
+/*
+ * Initialize the module and register the destructor.  Stores the resource
+ * number of the module in le_remctl_internal.
+ */
+PHP_MINIT_FUNCTION(remctl)
+{
+    le_remctl_internal =
+        zend_register_list_destructors_ex(php_remctl_dtor, NULL,
+            PHP_REMCTL_RES_NAME, module_number);
+    return SUCCESS;
 }
 
-ZEND_FUNCTION( remctl )
+
+/*
+ * The simplified interface.  Make a call and return the results as an
+ * object.
+ */
+ZEND_FUNCTION(remctl)
 {
-    struct remctl_result	*result = NULL;
-
-    zval			*cmd_array, **data;
-    zval			*remres_obj;
-    HashTable			*hash;
-    HashPosition		pos;
-
-    char			*host;
-    char			*principal;
-    const char			**command = NULL;
-    unsigned short		port;
-    int				hlen, plen, count, i;
-    int				success = 0;
-
-    if ( zend_parse_parameters( ZEND_NUM_ARGS() TSRMLS_CC,
-				"slsa", &host, &hlen, &port,
-				&principal, &plen, &cmd_array ) == FAILURE ) {
-	zend_error( E_WARNING, "%s: invalid parameters", __FUNCTION__ );
-	RETURN_NULL();
-    }
+    zval *cmd_array, **data;
+    void **data_ref;
+    HashTable *hash;
+    HashPosition pos;
+    char *host, *principal = NULL;
+    const char **command = NULL;
+    unsigned short port;
+    int hlen, plen, count, i, status;
+    int success = 0;
+    struct remctl_result *result = NULL;
 
-    if (( hlen == 0 ) || ( plen == 0 )) {
-	zend_error( E_WARNING, "Host and principal must be valid strings\n" );
-	RETURN_NULL();
+    /*
+     * Read the arguments (host, port, principal, and command) and check their
+     * validity.  Host and command are required, so all arguments must be
+     * provided, but an empty string can be passed in as the principal.
+     */
+    status = zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "slsa", &host,
+                 &hlen, &port, &principal, &plen, &cmd_array);
+    if (status == FAILURE) {
+        zend_error(E_WARNING, "remctl: invalid parameters\n");
+        RETURN_NULL();
     }
-
-    hash = Z_ARRVAL_P( cmd_array );
-    if (( count = zend_hash_num_elements( hash )) < 2 ) {
-	zend_error( E_WARNING, "%s: array must contain at least two elements\n",
-				__FUNCTION__ );
-	RETURN_NULL();
+    if (hlen == 0) {
+        zend_error(E_WARNING, "remctl: host must be a valid string\n");
+        RETURN_NULL();
     }
-
-    if (( command = ( const char ** )emalloc(( count + 1 ) *
-					sizeof( char * ))) == NULL ) {
-	zend_error( E_WARNING, "%s: emalloc failed\n", __FUNCTION__ );
-	RETURN_NULL();
+    if (plen == 0)
+        principal = NULL;
+    hash = Z_ARRVAL_P(cmd_array);
+    count = zend_hash_num_elements(hash);
+    if (count < 1) {
+        zend_error(E_WARNING, "remctl: command must not be empty\n");
+        RETURN_NULL();
     }
 
-    for ( i = 0, zend_hash_internal_pointer_reset_ex( hash, &pos );
-		zend_hash_get_current_data_ex( hash, ( void ** )&data,
-						   &pos ) == SUCCESS;
-		i++, zend_hash_move_forward_ex( hash, &pos )) {
-	if ( Z_TYPE_PP( data ) != IS_STRING ) {
-	    zend_error( E_WARNING, "%s: array contains non-string value\n",
-				__FUNCTION__ );
-	    goto remctl_cleanup;
-	}
-
-	if ( i >= count ) {
-	    zend_error( E_WARNING, "%s: exceeded number of elements!\n",
-				__FUNCTION__ );
-	    goto remctl_cleanup;
-	}
-
-	command[ i ] = estrndup( Z_STRVAL_PP( data ), Z_STRLEN_PP( data ));
-	if ( command[ i ] == NULL ) {
-	    zend_error( E_WARNING, "%s: estrndup failed\n", __FUNCTION__ );
-	    count = i;
-	    goto remctl_cleanup;
-	}
+    /*
+     * Convert the command array into the char ** needed by libremctl.  This
+     * is less than ideal because we make another copy of all of the
+     * arguments.  There should be some way to do this without making a copy.
+     */
+    command = emalloc((count + 1) * sizeof(char *));
+    if (command == NULL) {
+        zend_error(E_WARNING, "remctl: emalloc failed\n");
+        RETURN_NULL();
     }
-    /* must NULL-terminate command argv for remctl */
-    command[ count ] = NULL;
+    i = 0;
+    zend_hash_internal_pointer_reset_ex(hash, &pos);
+    data_ref = (void **) &data;
+    while (zend_hash_get_current_data_ex(hash, data_ref, &pos) == SUCCESS) {
+        if (Z_TYPE_PP(data) != IS_STRING) {
+            zend_error(E_WARNING, "remctl: command contains non-string\n");
+            goto cleanup;
+        }
+        if (i >= count) {
+            zend_error(E_WARNING, "remctl: internal error: incorrect count\n");
+            goto cleanup;
+        }
+        command[i] = estrndup(Z_STRVAL_PP(data), Z_STRLEN_PP(data));
+        if (command[i] == NULL) {
+            zend_error(E_WARNING, "remctl: estrndup failed\n");
+            count = i;
+            goto cleanup;
+        }
+        i++;
+        zend_hash_move_forward_ex(hash, &pos);
+    }
+    command[count] = NULL;
 
-    if (( result = remctl( host, port, principal, command )) == NULL ) {
-	zend_error( E_WARNING, "%s: remctl failed\n", __FUNCTION__ );
-	goto remctl_cleanup;
+    /* Run the actual remctl call. */
+    result = remctl(host, port, principal, command);
+    if (result == NULL) {
+        zend_error(E_WARNING, "remctl: %s\n", strerror(errno));
+        goto cleanup;
     }
 
     /*
-     * populate an object with remctl results.
-     * return_value is defined for us by zend.
+     * Convert the remctl result to an object.  return_value is defined for us
+     * by Zend.
      */
-    if ( object_init( return_value ) != SUCCESS ) {
-	zend_error( E_WARNING, "%s: object_init failed\n", __FUNCTION__ );
-	goto remctl_cleanup;
-    }
-    if ( result->error != NULL ) {
-	add_property_string( return_value, "error", result->error, 1 );
-    } else {
-	add_property_string( return_value, "error", "", 1 );
+    if (object_init(return_value) != SUCCESS) {
+        zend_error(E_WARNING, "remctl: object_init failed\n");
+        goto cleanup;
     }
-    add_property_stringl( return_value, "stdout",
-			  result->stdout_buf, result->stdout_len, 1 );
-    add_property_long( return_value, "stdout_len", result->stdout_len );
-    add_property_stringl( return_value, "stderr",
-			  result->stderr_buf, result->stderr_len, 1 );
-    add_property_long( return_value, "stderr_len", result->stderr_len );
-    add_property_long( return_value, "status", result->status );
-
+    if (result->error == NULL)
+        add_property_string(return_value, "error", "", 1);
+    else
+        add_property_string(return_value, "error", result->error, 1);
+    add_property_stringl(return_value, "stdout", result->stdout_buf,
+        result->stdout_len, 1);
+    add_property_long(return_value, "stdout_len", result->stdout_len);
+    add_property_stringl(return_value, "stderr", result->stderr_buf,
+        result->stderr_len, 1);
+    add_property_long(return_value, "stderr_len", result->stderr_len);
+    add_property_long(return_value, "status", result->status);
     success = 1;
 
-remctl_cleanup:
-    if ( command != NULL ) {
-	for ( i = 0; i < count; i++ ) {
-	    efree(( char * )command[ i ] );
-	}
-	efree( command );
-    }
-    if ( result != NULL ) {
-	remctl_result_free( result );
+cleanup:
+    if (command != NULL) {
+        for (i = 0; i < count; i++)
+            efree((char *) command[i]);
+        efree(command);
     }
-
-    if ( !success ) {
-	RETURN_NULL();
-    }
-
-    /*
-     * these functions actually return void. return_value contains
-     * the interesting bits. the RETURN_X macros simply change
-     * what return_value contains.
-     */
+    if (result != NULL)
+        remctl_result_free(result);
+    if (!success)
+        RETURN_NULL();
 }
 
-ZEND_FUNCTION( remctl_new )
+
+/*
+ * Now the complex interface.  First, the constructor.
+ */
+ZEND_FUNCTION(remctl_new)
 {
-    struct remctl		*r;
+    struct remctl *r;
 
-    if (( r = remctl_new()) == NULL ) {
-	zend_error( E_WARNING, "%s: remctl_new failed", __FUNCTION__ );
-	RETURN_NULL();
+    r = remctl_new();
+    if (r == NULL) {
+        zend_error(E_WARNING, "remctl_new: %s", strerror(errno));
+        RETURN_NULL();
     }
-
-    ZEND_REGISTER_RESOURCE( return_value, r, le_remctl_internal );
+    ZEND_REGISTER_RESOURCE(return_value, r, le_remctl_internal);
 }
 
-ZEND_FUNCTION( remctl_open )
-{
-    struct remctl		*r;
-    zval			*zrem;
-    char			*host, *princ;
-    unsigned short		port;
-    int				hlen, prlen;
-
-    if ( zend_parse_parameters( ZEND_NUM_ARGS() TSRMLS_CC,
-				"rsls", &zrem, &host, &hlen,
-				&port, &princ, &prlen ) == FAILURE ) {
-	zend_error( E_WARNING, "%s: invalid parameters", __FUNCTION__ );
-	RETURN_FALSE;
-    }
 
-    ZEND_FETCH_RESOURCE( r, struct remctl *, &zrem, -1,
-			 PHP_REMCTL_RES_NAME, le_remctl_internal );
-
-    if ( !remctl_open( r, host, port, princ )) {
-	RETURN_FALSE;
+/*
+ * Open a connection to the remote host.  Only the host parameter is required;
+ * the rest are optional.  PHP may require something be passed in for
+ * principal, but the empty string is taken to mean "use the library default."
+ */
+ZEND_FUNCTION(remctl_open)
+{
+    struct remctl *r;
+    zval *zrem;
+    char *host;
+    char *principal = NULL;
+    unsigned short port = 0;
+    int hlen, plen, status;
+
+    /* Parse and verify arguments. */
+    status = zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "rs|ls", &zrem,
+                 &host, &hlen, &port, &principal, &plen);
+    if (status == FAILURE) {
+        zend_error(E_WARNING, "remctl_open: invalid parameters\n");
+        RETURN_FALSE;
     }
-
+    if (plen == 0)
+        principal = NULL;
+    ZEND_FETCH_RESOURCE(r, struct remctl *, &zrem, -1, PHP_REMCTL_RES_NAME,
+        le_remctl_internal);
+
+    /* Now we have all the arguments and can do the real work. */
+    if (!remctl_open(r, host, port, principal))
+        RETURN_FALSE;
     RETURN_TRUE;
 }
 
-ZEND_FUNCTION( remctl_command )
-{
-    struct remctl		*r;
-    zval			*zrem, *cmd_array, **data;
-
-    HashTable			*hash;
-    HashPosition		pos;
-
-    struct iovec		*cmd_vec = NULL;
-    int				i, count;
-    int				success = 0;
 
-    if ( zend_parse_parameters( ZEND_NUM_ARGS() TSRMLS_CC,
-				"ra", &zrem, &cmd_array ) == FAILURE ) {
-	zend_error( E_WARNING, "%s: invalid parameters", __FUNCTION__ );
-	RETURN_FALSE;
-    }
-
-    ZEND_FETCH_RESOURCE( r, struct remctl *, &zrem, -1,
-			 PHP_REMCTL_RES_NAME, le_remctl_internal );
-
-    hash = Z_ARRVAL_P( cmd_array );
-    if (( count = zend_hash_num_elements( hash )) < 2 ) {
-	zend_error( E_WARNING, "%s: array must contain at least two elements\n",
-				__FUNCTION__ );
-	RETURN_FALSE;
+/*
+ * Send a command to the remote server.
+ */
+ZEND_FUNCTION(remctl_command)
+{
+    struct remctl *r;
+    zval *zrem, *cmd_array, **data;
+    void **data_ref;
+    HashTable *hash;
+    HashPosition pos;
+    struct iovec *cmd_vec = NULL;
+    int i, count, status;
+    int success = 0;
+
+    /* Parse and verify arguments. */
+    status = zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "ra", &zrem,
+                 &cmd_array);
+    if (status == FAILURE) {
+        zend_error(E_WARNING, "remctl_command: invalid parameters\n");
+        RETURN_FALSE;
     }
-    if ( count > IOV_MAX ) {
-	zend_error( E_WARNING, "%s: array count exceeds IOV_MAX!",
-				__FUNCTION__ );
-	RETURN_FALSE;
+    ZEND_FETCH_RESOURCE(r, struct remctl *, &zrem, -1, PHP_REMCTL_RES_NAME,
+        le_remctl_internal);
+    hash = Z_ARRVAL_P(cmd_array);
+    count = zend_hash_num_elements(hash);
+    if (count < 1) {
+        zend_error(E_WARNING, "remctl_command: command must not be empty\n");
+        RETURN_NULL();
     }
 
-    if (( cmd_vec = ( struct iovec * )emalloc( count *
-					sizeof( struct iovec ))) == NULL ) {
-	zend_error( E_WARNING, "%s: emalloc failed\n", __FUNCTION__ );
-	RETURN_FALSE;
-    }
-
-    for ( i = 0, zend_hash_internal_pointer_reset_ex( hash, &pos );
-		zend_hash_get_current_data_ex( hash, ( void ** )&data,
-						   &pos ) == SUCCESS;
-		i++, zend_hash_move_forward_ex( hash, &pos )) {
-	if ( Z_TYPE_PP( data ) != IS_STRING ) {
-	    zend_error( E_WARNING, "%s: array contains non-string value\n",
-				__FUNCTION__ );
-	    goto remctl_command_cleanup;
-	}
-
-	if ( i >= count ) {
-	    zend_error( E_WARNING, "%s: exceeded number of elements!\n",
-				__FUNCTION__ );
-	    goto remctl_command_cleanup;
-	}
-
-	cmd_vec[ i ].iov_base = estrndup( Z_STRVAL_PP( data ),
-					  Z_STRLEN_PP( data ));
-	if ( cmd_vec[ i ].iov_base == NULL ) {
-	    zend_error( E_WARNING, "%s: estrndup failed\n", __FUNCTION__ );
-	    count = i;
-	    goto remctl_command_cleanup;
-	}
-	cmd_vec[ i ].iov_len = Z_STRLEN_PP( data );
+    /*
+     * Transform the PHP array into an array of struct iovec.  This is less
+     * than ideal because it makes another copy of all of the data.  There
+     * should be some way to do this without copying.
+     */
+    cmd_vec = emalloc(count * sizeof(struct iovec));
+    if (cmd_vec == NULL) {
+        zend_error(E_WARNING, "remctl_command: emalloc failed\n");
+        RETURN_FALSE;
     }
-
-    if ( !remctl_commandv( r, cmd_vec, count )) {
-	/* return false, let script get error from remctl_error */
-	goto remctl_command_cleanup;
+    i = 0;
+    zend_hash_internal_pointer_reset_ex(hash, &pos);
+    data_ref = (void **) &data;
+    while (zend_hash_get_current_data_ex(hash, data_ref, &pos) == SUCCESS) {
+        if (Z_TYPE_PP(data) != IS_STRING) {
+            zend_error(E_WARNING,
+                "remctl_command: command contains non-string\n");
+            goto cleanup;
+        }
+        if (i >= count) {
+            zend_error(E_WARNING,
+                "remctl_command: internal error: incorrect count\n");
+            goto cleanup;
+        }
+        cmd_vec[i].iov_base = emalloc(Z_STRLEN_PP(data) + 1);
+        if (cmd_vec[i].iov_base == NULL) {
+            zend_error(E_WARNING, "remctl_command: emalloc failed\n");
+            count = i;
+            goto cleanup;
+        }
+        cmd_vec[i].iov_len = Z_STRLEN_PP(data);
+        memcpy(cmd_vec[i].iov_base, Z_STRVAL_PP(data), cmd_vec[i].iov_len);
+        i++;
+        zend_hash_move_forward_ex(hash, &pos);
     }
 
+    /* Finally, we can do the work. */
+    if (!remctl_commandv(r, cmd_vec, count))
+        goto cleanup;
     success = 1;
 
-remctl_command_cleanup:
-    if ( cmd_vec != NULL ) {
-	for ( i = 0; i < count; i++ ) {
-	    efree(( char * )cmd_vec[ i ].iov_base );
-	}
-	efree( cmd_vec );
+cleanup:
+    if (cmd_vec != NULL) {
+        for (i = 0; i < count; i++)
+            efree(cmd_vec[i].iov_base);
+        efree(cmd_vec);
     }
-
-    if ( !success ) {
-	RETURN_FALSE;
-    }
-
+    if (!success)
+        RETURN_FALSE;
     RETURN_TRUE;
 }
 
-ZEND_FUNCTION( remctl_output )
-{
-    struct remctl		*r;
-    struct remctl_output	*output;
-    zval			*zrem;
-
-    int				success = 0;
 
-    if ( zend_parse_parameters( ZEND_NUM_ARGS() TSRMLS_CC,
-				"r", &zrem ) == FAILURE ) {
-	zend_error( E_WARNING, "%s: invalid parameters", __FUNCTION__ );
-	RETURN_NULL();
+/*
+ * Get an output token from the server and return it as an object.
+ */
+ZEND_FUNCTION(remctl_output)
+{
+    struct remctl *r;
+    struct remctl_output *output;
+    zval *zrem;
+    int status;
+
+    /* Parse and verify arguments. */
+    status = zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "r", &zrem);
+    if (status == FAILURE) {
+        zend_error(E_WARNING, "remctl_output: invalid parameters\n");
+        RETURN_NULL();
+    }
+    ZEND_FETCH_RESOURCE(r, struct remctl *, &zrem, -1, PHP_REMCTL_RES_NAME,
+        le_remctl_internal);
+
+    /* Get the output token. */
+    output = remctl_output(r);
+    if (output == NULL) {
+        zend_error(E_WARNING, "remctl_output: error reading from server: %s",
+            remctl_error(r));
+        RETURN_NULL();
     }
-
-    ZEND_FETCH_RESOURCE( r, struct remctl *, &zrem, -1,
-			 PHP_REMCTL_RES_NAME, le_remctl_internal );
 
     /*
-     * populate an object with remctl results.
-     * return_value is defined for us by zend.
+     * Populate an object with the output results.  return_value is defined
+     * for us by Zend.
      */
-    if ( object_init( return_value ) != SUCCESS ) {
-	zend_error( E_WARNING, "%s: object_init failed\n", __FUNCTION__ );
-	RETURN_NULL();
+    if (object_init(return_value) != SUCCESS) {
+        zend_error(E_WARNING, "remctl_output: object_init failed\n");
+        RETURN_NULL();
     }
-    
-    if (( output = remctl_output( r )) == NULL ) {
-	zend_error( E_WARNING, "%s: error reading from server: %s",
-				remctl_error( r ));
-	RETURN_NULL();
-    }
-
-    add_property_bool( return_value, "done", 0 );
-    add_property_long( return_value, "stdout_len", 0 );
-    add_property_long( return_value, "stderr_len", 0 );
-
-    switch( output->type ) {
+    switch (output->type) {
     case REMCTL_OUT_OUTPUT:
-	add_property_stringl( return_value, "type",
-				"output", strlen( "output" ), 1 );
-	if ( output->stream == 1 ) {
-	    add_property_stringl( return_value, "stdout",
-				output->data, output->length, 1 );
-	    add_property_long( return_value, "stdout_len", output->length );
-	} else if ( output->stream == 2 ) {
-	    add_property_stringl( return_value, "stderr",
-				output->data, output->length, 1 );
-	    add_property_long( return_value, "stderr_len", output->length );
-	} else {
-	    zend_error( E_WARNING, "%s: unknown output stream %d",
-				output->stream );
-	    add_property_stringl( return_value, "stderr",
-				output->data, output->length, 1 );
-	    add_property_long( return_value, "stderr_len", output->length );
-	}
-	break;
-
+        add_property_string(return_value, "type", "output", 1);
+        add_property_stringl(return_value, "data", output->data,
+            output->length, 1);
+        add_property_long(return_value, "stream", output->stream);
+        break;
     case REMCTL_OUT_ERROR:
-	add_property_stringl( return_value, "type",
-				"error", strlen( "error" ), 1 );
-	add_property_stringl( return_value, "error",
-				output->data, output->length, 1 );
-	break;
-
+        add_property_string(return_value, "type", "error", 1);
+        add_property_stringl(return_value, "data", output->data,
+            output->length, 1);
+        add_property_long(return_value, "error", output->error);
+        break;
     case REMCTL_OUT_STATUS:
-	add_property_stringl( return_value, "type",
-				"status", strlen( "status" ), 1 );
-	add_property_long( return_value, "status", output->status );
-	break;
-
+        add_property_string(return_value, "type", "status", 1);
+        add_property_long(return_value, "status", output->status);
+        break;
     case REMCTL_OUT_DONE:
-	add_property_stringl( return_value, "type",
-				"done", strlen( "done" ), 1 );
-	add_property_bool( return_value, "done", 1 );
-	break;
+        add_property_string(return_value, "type", "done", 1);
+        break;
     }
-
-    /* return_value contains something like a remctl_output object */
 }
 
-ZEND_FUNCTION( remctl_error )
-{
-    struct remctl		*r;
-    zval			*zrem;
-
-    const char			*error;
-    int				success = 0;
 
-    if ( zend_parse_parameters( ZEND_NUM_ARGS() TSRMLS_CC,
-				"r", &zrem ) == FAILURE ) {
-	zend_error( E_WARNING, "%s: invalid parameters", __FUNCTION__ );
-	RETURN_NULL();;
+/*
+ * Returns the error message from a previously failed remctl call.
+ */
+ZEND_FUNCTION(remctl_error)
+{
+    struct remctl *r;
+    zval *zrem;
+    const char *error;
+    int status;
+
+    /* Parse and verify arguments. */
+    status = zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "r", &zrem);
+    if (status == FAILURE) {
+        zend_error(E_WARNING, "remctl_error: invalid parameters\n");
+        RETURN_NULL();
     }
+    ZEND_FETCH_RESOURCE(r, struct remctl *, &zrem, -1, PHP_REMCTL_RES_NAME,
+        le_remctl_internal);
 
-    ZEND_FETCH_RESOURCE( r, struct remctl *, &zrem, -1,
-			 PHP_REMCTL_RES_NAME, le_remctl_internal );
-
-    error = remctl_error( r );
-
-    RETURN_STRING(( char * )error, 1 );
+    /* Do the work. */
+    error = remctl_error(r);
+    RETURN_STRING((char *) error, 1);
 }
 
+
+/*
+ * Close the connection.  This isn't strictly necessary since the destructor
+ * will close the connection for us, but it's part of the interface.
+ */
 ZEND_FUNCTION( remctl_close )
 {
-    struct remctl		*r;
-    zval			*zrem;
-
-    if ( zend_parse_parameters( ZEND_NUM_ARGS() TSRMLS_CC,
-				"r", &zrem ) == FAILURE ) {
-	zend_error( E_WARNING, "%s: invalid parameters", __FUNCTION__ );
-	RETURN_FALSE;
+    struct remctl *r;
+    zval *zrem;
+    int status;
+
+    /* Parse and verify arguments. */
+    status = zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "r", &zrem);
+    if (status == FAILURE) {
+        zend_error(E_WARNING, "remctl_error: invalid parameters\n");
+        RETURN_NULL();
     }
+    ZEND_FETCH_RESOURCE(r, struct remctl *, &zrem, -1, PHP_REMCTL_RES_NAME,
+        le_remctl_internal);
 
-    ZEND_FETCH_RESOURCE( r, struct remctl *, &zrem, -1,
-			 PHP_REMCTL_RES_NAME, le_remctl_internal );
-
-    /* this delete invokes php_remctl_dtor, which calls remctl_close */
-    zend_list_delete( Z_LVAL_P( zrem ));
-
+    /* This delete invokes php_remctl_dtor, which calls remctl_close. */
+    zend_list_delete(Z_LVAL_P(zrem));
     RETURN_TRUE;
 }
diff --git a/php/php_remctl.h b/php/php_remctl.h
index b399b3d..d436d45 100644
--- a/php/php_remctl.h
+++ b/php/php_remctl.h
@@ -1,19 +1,30 @@
+/*
+ * Declarations for the PHP remctl PECL module.
+ *
+ * Written by Andrew Mortensen <admorten@umich.edu>, 2008
+ * Copyright 2008 Andrew Mortensen <admorten@umich.edu>
+ * Copyright 2008 Board of Trustees, Leland Stanford Jr. University
+ *
+ * See LICENSE for licensing terms.
+ */
+
 #ifndef PHP_REMCTL_H
-#define PHP_REMCTL_H			1
+#define PHP_REMCTL_H 1
 
-#define PHP_REMCTL_VERSION		"0.1"
-#define PHP_REMCTL_EXTNAME		"remctl"
-#define PHP_REMCTL_RES_NAME		"remctl_resource"
+/* This should be the same version as the overall remctl package. */
+#define PHP_REMCTL_VERSION  "2.13"
+#define PHP_REMCTL_EXTNAME  "remctl"
+#define PHP_REMCTL_RES_NAME "remctl_resource"
 
-PHP_MINIT_FUNCTION( remctl );
-PHP_FUNCTION( remctl );
-PHP_FUNCTION( remctl_new );
-PHP_FUNCTION( remctl_open );
-PHP_FUNCTION( remctl_command );
-PHP_FUNCTION( remctl_output );
-PHP_FUNCTION( remctl_error );
-PHP_FUNCTION( remctl_close );
+PHP_MINIT_FUNCTION(remctl);
+PHP_FUNCTION(remctl);
+PHP_FUNCTION(remctl_new);
+PHP_FUNCTION(remctl_open);
+PHP_FUNCTION(remctl_command);
+PHP_FUNCTION(remctl_output);
+PHP_FUNCTION(remctl_error);
+PHP_FUNCTION(remctl_close);
 
-extern zend_module_entry		remctl_module_entry;
+extern zend_module_entry remctl_module_entry;
 
-#endif /* REMCTL_H */
+#endif /* PHP_REMCTL_H */
diff --git a/php/test.php b/php/test.php
index d4e1b0c..82db44f 100644
--- a/php/test.php
+++ b/php/test.php
@@ -39,19 +39,18 @@ if ( !remctl_command( $rem, $args )) {
     exit( 2 );
 }
 $output = remctl_output( $rem );
-while ( $output != null && !$output->done ) {
+while ($output != null && $output->type != "done") {
     switch ( $output->type ) {
     case "output":
-	if ( $output->stdout_len ) {
-	    echo "stdout: $output->stdout";
-	}
-	if ( $output->stderr_len ) {
-	    echo "stderr: $output->stderr";
+	if ($output->stream == 1) {
+	    echo "stdout: $output->data";
+	} elseif ($output->stream == 2) {
+	    echo "stderr: $output->data";
 	}
 	break;
 
     case "error":
-	echo "error: $output->error\n";
+	echo "error: $output->error ($output->data)\n";
 	break;
 
     case "status":