On Wed, Jul 28, 2010 at 6:33 PM, Gustavo Sverzut Barbieri <
barbi...@profusion.mobi> wrote:
> 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.
> I left that out because you can specify as many of the operations as you
want anyway.
Also, why not make it an enum?
>
It was an enum originally, but cedric thought bit flags would be better.
>
> >+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.
>
Agreed, the callbacks need to be fixed to follow EFL. I was just following
what was already there.
>
> > +struct _Eio_File_Op
> > +{
> > + Eio_File common;
> > + const char *file;
> > + short int flags;
> > + struct stat st;
> > + int exists;
>
> why exists is not Eina_Bool?
>
Whoopsie?
>
> > + 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 ...
> }
>
> Yes I can.
>
>
> > +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.
>
> Ahhh the E formatting... :(
>
> > + 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!
>
> Actually CAN_READ CAN_WRITE and CAN_EXECUTE will return error if the file
doesn't exist (see if (!async->exists)). Otherwise is EINA_TRUE or
EINA_FALSE as to whether it can do those. Good catch on the bug.
>
> > + 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, ...)
>
> Gotcha, had no clue.
>
> > + 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?
>
> I'm not sure what you are getting at here? It returns FALSE if it doesn't
exists, and TRUE if it does. The reason the FALSE is called first is
because the app will seg if any other operations are done on a file that
doesn't exists.
>
> > + 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.
>
> Shorter, Shorter, Shorter, Shorter.
> > +/**
> > + * @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
>
------------------------------------------------------------------------------
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
