Hi, On Mi, 2017-06-14 at 13:11 -0300, Lucas Mendes wrote:
> The extension is located at: https://github.com/devsdmf/php-modplayer > <https://github.com/devsdmf/php-modplayer> > >From scrolling over it a few comments: MODPLAYER_G(pid) is a thread local variable, if this were eve running in a multithreaded environment the value in MSHUTDOWN wouln't really match. I guess you only ever want to have one child process, thus using a true global might be acceptable (and then either disallow building in threaded environment or lock access. E_ERROR should only be used for errors which can't be recovered nicely, actually only in the engine. Either use exceptions or return false and provide a waring or other mechanism. You have if (access(resolved_path, F_OK) != -1) { fptr = fopen(resolved_path, "rb"); theoretically there is a risk of a race condition - access could succeed then the file could be deleted and fopen fails. Simply checking the fopen return value should be enough. This code also doesn't check open-basedir etc. which might be acceptable for the use case. Calling fork() in PHP is dangerous. If the PHP script has other network connections or such they might be closed unexpectedly. if one of the processes quits even for the others. Maybe it's better to use a daemon which is independent from PHP or by doing this from a "worker thread" (for having a worker thread you don't need a threaded PHP, threaded PHP environment is for running multiple PHP scripts in parallel, you'd have to be careful how you return errors, though) ZEND_PARSE_PARAMETERS_START() is the performance improved version, zend_parse_parameters() is the nicer form with more options and better flexibility. As a suggestion you could look into ARGINFO things and provide reflection info for your exported functions. You could move the PHP_FUNCTION and play_audio() declarations from the header into the C file (or even reorder the C file soyou don't need them), see https://wiki.php.net/internals/review_comments play_audio() could also be marked static, this can avoid symbol conflicts and allows more optimization by compiler (which probablyisn't relevant) johannes P.S. For fun: To go the threaded way something like this should work: (untested, incomplete) pthread_t *modthread = NULL; pthread_mutex_t wait_for_start=PTHREAD_MUTEX_INITIALIZER; pthread_cond_t cond = PTHREAD_COND_INITIALIZER; #ifdef ZTS #error No ZTS support #endif typedef struct { FILE *fptr; int maxchan; int curious; int reverb; } play_args; int play_audio(void *args_v) { play_args *args = args_v; pid_t m_pid; signed int s_pid; MODULE *module; MikMod_InitThreads(); MikMod_RegisterAllDrivers(); MikMod_RegisterAllLoaders(); md_mode |= DMODE_SOFT_MUSIC | DMODE_NOISEREDUCTION | DMODE_INTERP; if (args->reverb > 0) { md_reverb = args->reverb; } if (MikMod_Init("")) { php_error_docref(NULL, WARNING, "Could not initialize the MikMod library"); free(args); return -1; } module = Player_LoadFP(args->fptr, args->maxchan, args->curious); if (!module) { fclose(fptr); free(args); return -1; } /* we are running, let the main thread continue, this thread shouldn't call any PHP APIs anymore else we get a mess */ pthread_cond_signal(&cond); module->wrap = 1; module->loop = 0; Player_Start(module); while (Player_Active()) { usleep(10000); MikMod_Update(); } MikMod_Exit(); free(args); return 0; } PHP_FUNCTION(modplayer_play) { /* .... */ play_args args = malloc(play_args); /* using malloc since we free it in non-PHP sthread */ /* ...*/ fptr = fopen(resolved_path, "rb"); if (fptr == NULL) { php_error_docref(NULL, E_WARNING, "An error occurred at try to open the module audio file specified"); RETURN_FALSE; } pthread_mutex_lock(&wait_for_start); pthread_create(&modthread, NULL, args, play_audio; &play_args); /* we now have a second thread, but that other thread owns PHP in order to emit error, wait till it's ready */ pthread_cond_wait(&cond, &wait_for_start); pthread_mutex_unlock(&wait_for_start); } -- PECL development discussion Mailing List (http://pecl.php.net/) To unsubscribe, visit: http://www.php.net/unsub.php
