On Wed, Jul 28, 2010 at 7:12 PM, Enlightenment SVN
<no-re...@enlightenment.org> wrote:
> Log:
> Add the ability to perform standard IO operations on a file in a thread.
> +#define EIO_FILE_MOD_TIME 1
> +#define EIO_FILE_SIZE 2
> +#define EIO_FILE_EXISTS 4
> +#define EIO_FILE_IS_DIR 8
> +#define EIO_FILE_CAN_READ 16
> +#define EIO_FILE_CAN_WRITE 32
> +#define EIO_FILE_CAN_EXECUTE 64
You should provide some EIO_FILE_STAT, to return the whole stat
result, maybe that is more useful than doing it separated for size,
exists... you can leave what you did, but also provide the generic
function as people will consult more than one property at time.
Also, why not make it an enum?
>+typedef void (*Eio_File_Op_Main_Cb)(void *value, short int flag, void *data);
it would be more usual to have data as first parameter (and thus all
the other eio callbacks are wrong), also handle flag before the value
to make it more meaningful. There is no greatness in using "short int"
there, make it an unsigned int.
typedef void (*Eio_File_Op_Main_Cb)(void *data, short int flag,
void *value);
compare this with evas event callback, it similar in signature.
> +struct _Eio_File_Op
> +{
> + Eio_File common;
> + const char *file;
> + short int flags;
> + struct stat st;
> + int exists;
why exists is not Eina_Bool?
> + Eina_Bool can_read;
> + Eina_Bool can_write;
> + Eina_Bool can_execute;
> + Eio_File_Op_Main_Cb main_cb;
> +};
> +
> +static void
> +_eio_file_op_cb(void *data)
> +{
> + Eio_File_Op *async;
> +
> + async = data;
> + if (stat(async->file, &async->st) >= 0)
> + {
> + if (async->flags & EIO_FILE_CAN_READ == EIO_FILE_CAN_READ)
> + {
> + if (!access(async->file, R_OK))
> + async->can_read = EINA_TRUE;
> + else
> + async->can_read = EINA_FALSE;
> + }
> + if (async->flags & EIO_FILE_CAN_WRITE == EIO_FILE_CAN_WRITE)
> + {
> + if (!access(async->file, W_OK))
> + async->can_write = EINA_TRUE;
> + else
> + async->can_write = EINA_FALSE;
> + }
> + if (async->flags & EIO_FILE_CAN_EXECUTE == EIO_FILE_CAN_EXECUTE)
> + {
> + if (!access(async->file, X_OK))
> + async->can_execute = EINA_TRUE;
> + else
> + async->can_execute = EINA_FALSE;
> + }
> + async->exists = 1;
> + }
> + else
> + async->exists = 0;
> +}
you can simplify this with:
async->exists = (stat(async->file, &async->st) == 0); // note that
==0 is success, not >= 0.
if (async->exists)
{
if ((async->flags & EIO_FILE_CAN_READ) == EIO_FILE_CAN_READ) //
coding style needs parenthesis!
async->can_read = !access(async->file, W_OK);
... others here ...
}
> +static void
> +_eio_file_op_end(void *data)
> +{
> + Eio_File_Op *async;
> +
> + async = data;
Just do Eio_File_Op *async = data, it is smaller and makes sense as
you're just converting the parameter.
> + if (!async->exists)
> + {
> + if (async->flags & EIO_FILE_EXISTS == EIO_FILE_EXISTS)
> + async->main_cb((void *)EINA_FALSE, EIO_FILE_EXISTS, data);
> + ecore_thread_cancel(async->common.thread);
> + return;
> + }
I don't like this, you don't return anything for CAN_READ, CAN_WRITE
and others in the case it does not exists? Not even an error? IMO
the user should always get called back, with an error whenever
appropriate. Or call error_cb if there were requirement not fulfilled.
also a bug there: the last parameter is USER data, not the one you got
from the callback ("async"), you really want: async->common.data
instead!
> + if (async->flags & EIO_FILE_MOD_TIME == EIO_FILE_MOD_TIME)
> + async->main_cb((void *)async->st.st_mtime, EIO_FILE_MOD_TIME, data);
> + if (async->flags & EIO_FILE_SIZE == EIO_FILE_SIZE)
> + async->main_cb((void *)async->st.st_size, EIO_FILE_SIZE, data);
for these, on 64bits you'll get a warning since your converting
integer (32) to pointer (64), you have to cast to long first:
async->main_cb((void *)(long)async->st.st_mtime, ...)
> + if (async->flags & EIO_FILE_EXISTS == EIO_FILE_EXISTS)
> + async->main_cb((void *)EINA_TRUE, EIO_FILE_EXISTS, data);
this is ugly and took me a while to notice why you did the EINA_TRUE
constant here... it was because of the other test before. And really,
why not call this one first?
> + if (async->flags & EIO_FILE_IS_DIR == EIO_FILE_IS_DIR)
> + {
> + if (S_ISDIR(async->st.st_mode))
> + async->main_cb((void *)EINA_TRUE, EIO_FILE_IS_DIR, data);
> + else
> + async->main_cb((void *)EINA_FALSE, EIO_FILE_IS_DIR, data);
> + }
async->main_cb((void *)(long)S_ISDIR(async->st.st_mode), ...)
make it shorter, simpler and easier.
> +/**
> + * @brief Perform standard File IO operations.
> + * @param file The file to operate on.
> + * @param flags Bit flags to specify which operations to do.
if you make the defines an enum, cite the enum name/typedef here.
> + * @param main_cb Callback called from the main loop with the results of the
> file operations.
> + * @param done_cb Callback called from the main loop when the operations are
> through.
> + * @param error_cb Callback called from the main loop when the file
> operations could not be completed.
> + * @return A reference to the IO operation.
> + *
> + * eio_file_operation runs selected operations in a separated thread using
> + * ecore_thread_run. This prevents any locking in your apps.
> + */
--
Gustavo Sverzut Barbieri
http://profusion.mobi embedded systems
--------------------------------------
MSN: barbi...@gmail.com
Skype: gsbarbieri
Mobile: +55 (19) 9225-2202
------------------------------------------------------------------------------
The Palm PDK Hot Apps Program offers developers who use the
Plug-In Development Kit to bring their C/C++ apps to Palm for a share
of $1 Million in cash or HP Products. Visit us here for more details:
http://p.sf.net/sfu/dev2dev-palm
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
