xiaoxiang781216 commented on code in PR #3511:
URL: https://github.com/apache/nuttx-apps/pull/3511#discussion_r3329642941
##########
system/popen/popen.c:
##########
@@ -399,31 +458,30 @@ FILE *popen(FAR const char *command, FAR const char *mode)
int pclose(FILE *stream)
{
- FAR struct popen_file_s *container = (FAR struct popen_file_s *)stream;
- FILE *original;
+ FAR struct popen_file_s *container;
pid_t shell;
#ifdef CONFIG_SCHED_WAITPID
int status;
int result;
#endif
- DEBUGASSERT(container != NULL && container->original != NULL);
- original = container->original;
-
- /* Set the state of the original file descriptor to the state of the
- * working copy
- */
-
- memcpy(original, &container->copy, sizeof(FILE));
+ DEBUGASSERT(stream != NULL);
Review Comment:
why debug here and check stream at line 470
##########
system/popen/popen.c:
##########
@@ -341,14 +396,18 @@ FILE *popen(FAR const char *command, FAR const char *mode)
posix_spawnattr_destroy(&attr);
errout_with_stream:
- fclose(container->original);
+ fclose(stream);
+ container = NULL;
errout_with_pipe:
close(fd[0]);
close(fd[1]);
errout_with_container:
- free(container);
+ if (container != NULL)
Review Comment:
remove the check, free will do it
##########
system/popen/popen.c:
##########
@@ -399,31 +458,30 @@ FILE *popen(FAR const char *command, FAR const char *mode)
int pclose(FILE *stream)
{
- FAR struct popen_file_s *container = (FAR struct popen_file_s *)stream;
- FILE *original;
+ FAR struct popen_file_s *container;
pid_t shell;
#ifdef CONFIG_SCHED_WAITPID
int status;
int result;
#endif
- DEBUGASSERT(container != NULL && container->original != NULL);
- original = container->original;
-
- /* Set the state of the original file descriptor to the state of the
- * working copy
- */
-
- memcpy(original, &container->copy, sizeof(FILE));
+ DEBUGASSERT(stream != NULL);
- /* Then close the original and free the container (saving the PID of the
- * shell process)
- */
+ if (stream == NULL || stream->fs_iofunc.close != popen_file_close)
+ {
+ errno = EINVAL;
+ return ERROR;
+ }
- fclose(original);
+ container = (FAR struct popen_file_s *)stream->fs_cookie;
+ if (container == NULL)
+ {
+ errno = EINVAL;
+ return ERROR;
+ }
shell = container->shell;
- free(container);
+ fclose(stream);
#ifdef CONFIG_SCHED_WAITPID
Review Comment:
why not move the follow code into popen_file_close and call fclose in pclose
directly
##########
system/popen/popen.c:
##########
Review Comment:
change to errout_with_container
##########
system/popen/popen.c:
##########
@@ -341,14 +396,18 @@ FILE *popen(FAR const char *command, FAR const char *mode)
posix_spawnattr_destroy(&attr);
errout_with_stream:
- fclose(container->original);
+ fclose(stream);
+ container = NULL;
errout_with_pipe:
close(fd[0]);
close(fd[1]);
Review Comment:
double close fd too
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
