Skip to content

Fix problems with spawnvpe and pthreads. #232

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Dec 23, 2024
Merged

Fix problems with spawnvpe and pthreads. #232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
394331c
Fix problem with lingering pipes (spawnvpe). Fix problem with underte…
elfpipe Dec 20, 2024
ecc0625
Remove lint in spawnvpe.c.
elfpipe Dec 20, 2024
d763415
Further link removal and clarification before PR finalization (spawnv…
elfpipe Dec 22, 2024
9b92ae1
Free command string. Remove unnecessary strdup. Tidy up comments. (sp…
elfpipe Dec 22, 2024
e12b5c8
Fix spawnv.
elfpipe Dec 23, 2024
d898e96
Merge branch 'development' into development
afxgroup Dec 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions library/c.lib_rev.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
#define REVISION 4
#define SUBREVISION 1

#define DATE "20.12.2024"
#define DATE "23.12.2024"
#define VERS "clib4.library 1.4.1"
#define VSTRING "clib4.library 1.4.1 (20.12.2024)\r\n"
#define VERSTAG "\0$VER: clib4.library 1.4.1-5d728de (20.12.2024)"
#define VSTRING "clib4.library 1.4.1 (23.12.2024)\r\n"
#define VERSTAG "\0$VER: clib4.library 1.4.1-5d728de (23.12.2024)"
7 changes: 6 additions & 1 deletion library/include/pthread.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,12 @@ struct pthread_barrierattr {

typedef struct pthread_barrierattr pthread_barrierattr_t;

struct pthread_barrier;
struct pthread_barrier {
unsigned int curr_height;
unsigned int total_height;
pthread_cond_t breeched;
pthread_mutex_t lock;
};

typedef struct pthread_barrier pthread_barrier_t;

Expand Down
7 changes: 0 additions & 7 deletions library/pthread/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,6 @@
#include "pthread.h"
#include <sys/time.h>

struct pthread_barrier {
unsigned int curr_height;
unsigned int total_height;
pthread_cond_t breeched;
pthread_mutex_t lock;
};

#undef NEWLIST
#define NEWLIST(_l) \
do \
Expand Down
36 changes: 24 additions & 12 deletions library/unistd/spawnv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,30 +161,42 @@ spawnv(int mode, const char *file, const char **argv) {
/* Add a NUL, to be nice... */
arg_string[arg_string_len] = '\0';

char finalpath[PATH_MAX] = {0};
char processName[NAMELEN] = {0};
snprintf(finalpath, PATH_MAX - 1, "%s %s", file, arg_string);
snprintf(processName, NAMELEN - 1, "Spawned Process #%d", __clib4->__children);
int command_len = strlen(file) + 1 + arg_string_len + 1; // '\0'
char *command = (char*)malloc(command_len);
char process_name[32] = {0};
snprintf(command, command_len, "%s %s", file, arg_string);
snprintf(process_name, NAMELEN - 1, "Spawned Process #%d", __clib4->__children);

D(("command : [%s]\n", command));

struct Process *me = __clib4->self;
BPTR in = mode == P_WAIT ? me->pr_CIS : 0;
BPTR out = mode == P_WAIT ? me->pr_COS : 0;
// These will be closed, so we need duplicates :
BPTR in = DupFileHandle(Input());
BPTR out = DupFileHandle(Output());
BPTR err = DupFileHandle(ErrorOutput());
D(("Launching [%s]", finalpath));
ret = SystemTags(finalpath,
ret = SystemTags(command,
SYS_Input, in,
SYS_Output, out,
SYS_Error, out,
SYS_Error, err,
NP_CloseError, TRUE,
SYS_UserShell, TRUE,
SYS_Asynch, mode == P_WAIT ? FALSE : TRUE,
NP_EntryCode, spawnedProcessEnter,
NP_EntryData, getgid(),
NP_ExitCode, spawnedProcessExit,
NP_Name, strdup(processName),
NP_Name, process_name,
NP_Child, TRUE,
TAG_DONE);
if (ret != 0) {
/* SystemTags failed. Clean up file handles */
if (in != 0)
free(command);
if (ret) {
/* SystemTags failed. Clean up file handle */
if (in)
Close(in);
if (out)
Close(out);
if (err)
Close(err);
errno = __translate_io_error_to_errno(IoErr());
}
else {
Expand Down
151 changes: 72 additions & 79 deletions library/unistd/spawnvpe.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,13 @@ get_arg_string_length(char *const argv[]) {
// DebugPrintF("[child :] Done.");
// FreeSignal(ed->childSignal);
// }
/* * * * *
Note for future generations : CreateNewProc is not suited for running shell commands.
The only way to have a full shell environment (apart from using internal packet structures),
is to use System. We keep the code here a display for the event, that someone should like
to investigate further into the mysteries of AmigaDOS. Until then, the following #define is set to 0.
* * * * */
#define USE_CNPT 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are going to use this new definition, we probably need to document it at the README file. Can you please add some info about what this is, what does it enable and how to enable it if needed? It would be great if that was part of this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is overkill. I am primarily keeping the version of the text using CreateNewProcTags() for research reasons. At present only SystemTags will enable full functioning in cmake. CNPT gives the strange side effect, that the child will have its parents name in argv[0] (at least when the child uses newlib, as in the case of (g)make).

I could be convinced to ditch the CNPT code, or just replace it with #if 0.

int
spawnvpe(
const char *file,
Expand Down Expand Up @@ -170,7 +177,6 @@ spawnvpe(
__set_errno(0);

D(("Starting new process [%s]\n", name));
// printf("[spawnvpe :] Starting new process [%s]\n", name);

int error = __translate_unix_to_amiga_path_name(&name, &nti_name);
if (error) {
Expand All @@ -180,21 +186,18 @@ spawnvpe(
}

D(("name after conversion: [%s]\n", name));
// printf("[spawnvpe :] name after conversion [%s]\n", name);

#if 0
#if USE_CNPT
seglist = LoadSeg(name);
if (!seglist)
return -1;
#endif

BPTR fileLock = Lock(name, SHARED_LOCK);
if (fileLock) {
progdirLock = ParentDir(fileLock);
UnLock(fileLock);
}

// printf("[spawnvpe :] cwd before conversion: [%s]\n", cwd);
#endif

if(cwd) {
error = __translate_unix_to_amiga_path_name(&cwd, &nti_cwd);
Expand All @@ -206,20 +209,18 @@ spawnvpe(
}

D(("cwd after conversion: [%s]\n", cwd));
// printf("[spawnvpe :] cwd after conversion [%s]\n", cwd);

BPTR cwdLock = cwd ? Lock(cwd, SHARED_LOCK) : 0;
BPTR cwdLock = cwd ? Lock(cwd, SHARED_LOCK) : 0; //DupLock(GetCurrentDir());

parameter_string_len = get_arg_string_length((char *const *) argv);
// This is probably unnecessary :
// This is probably unnecessary (and harmful to long commands):
//
// if (parameter_string_len > _POSIX_ARG_MAX) {
// __set_errno(E2BIG);
// return ret;
// }

D(("parameter_string_len: [%ld]\n", parameter_string_len));
// printf("[spawnvpe :] parameter_string_len: [%ld]\n", parameter_string_len);

arg_string = malloc(parameter_string_len + 1);
if (arg_string == NULL) {
Expand All @@ -239,24 +240,21 @@ spawnvpe(

D(("arg_string: [%s]\n", arg_string));

int finalpath_len = strlen(name) + 1 + arg_string_len + 1; // '\0'
char *finalpath = (char*)malloc(finalpath_len);
char processName[32] = {0};
snprintf(finalpath, finalpath_len, "%s %s", name, arg_string);
snprintf(processName, NAMELEN - 1, "Spawned Process #%d", __clib4->__children);
int full_command_len = strlen(name) + 1 + arg_string_len + 1; // '\0'
char *full_command = (char*)malloc(full_command_len);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that you renamed the finalpath to full_command but I do not see where this is freed. Maybe that was forgotten or we missed it in first place. Should we free that memory?

char process_name[32] = {0};
snprintf(full_command, full_command_len, "%s %s", name, arg_string);
snprintf(process_name, NAMELEN - 1, "Spawned Process #%d", __clib4->__children);

D(("File to execute: [%s]\n", finalpath));

// printf("[spawnvpe :] full command == <%s>\n", finalpath);
// printf("[spawnvpe :] processName == <%s>\n", processName);
D(("Command to execute: [%s]\n", full_command));

if (fhin >= 0) {
err = __get_default_file(fhin, &fh);
if (err) {
__set_errno(EBADF);
return ret;
}
iofh[0] = DupFileHandle(fh); //in case this is closed by the parent
iofh[0] = DupFileHandle(fh); // This will be closed by ST/CNPT
closefh[0] = TRUE;
}
else {
Expand All @@ -270,7 +268,7 @@ spawnvpe(
__set_errno(EBADF);
return ret;
}
iofh[1] = DupFileHandle(fh); //in case this is closed by the parent
iofh[1] = DupFileHandle(fh); // This will be closed by ST/CNPT
closefh[1] = TRUE;
}
else {
Expand All @@ -284,22 +282,19 @@ spawnvpe(
__set_errno(EBADF);
return ret;
}
iofh[2] = DupFileHandle(fh); //in case this is closed by the parent
iofh[2] = DupFileHandle(fh); // This will be closed by ST/CNPT
closefh[2] = TRUE;
}
else {
iofh[2] = Open("NIL:", MODE_OLDFILE);
closefh[2] = TRUE;
}

D(("(*)Calling SystemTags.\n"));
struct Task *_me = FindTask(0);

// struct EntryData ed;
// ed.parent = FindTask(0);
// ed.parentSignal = AllocSignal(-1);
#if USE_CNPT
D(("(*)Calling CreateNewProcTags.\n"));

struct Task *_me = FindTask(0);
#if 0
struct Process *p = CreateNewProcTags(
NP_Seglist, seglist,
NP_FreeSeglist, TRUE,
Expand All @@ -324,88 +319,86 @@ spawnvpe(
NP_CloseError, FALSE,
#endif

NP_EntryCode, spawnvpe_entryCode,
NP_EntryData, &ed,
NP_EntryCode, spawnedProcessEnter,
NP_EntryData, getgid(),

// NP_FinalCode, amiga_FinalCode,
// NP_FinalData, fd,

// NP_EntryCode, spawnedProcessEnter,
NP_ExitCode, spawnedProcessExit,

NP_Name, strdup(processName),
cwdLock ? NP_CurrentDir : TAG_SKIP, cwdLock,
progdirLock ? NP_ProgramDir : TAG_SKIP, progdirLock,
cwdLock ? NP_CurrentDir : TAG_SKIP, cwdLock,
NP_Name, process_name,

NP_Arguments, arg_string,

TAG_DONE
);
if (p) ret = 0;
#else
// printf("(*)Calling SystemTags.\n");

ret = SystemTags(finalpath,
NP_NotifyOnDeathSigTask, _me,
SYS_Input, iofh[0],
SYS_Output, iofh[1],
SYS_Error, iofh[2],
SYS_UserShell, TRUE,
SYS_Asynch, TRUE,
NP_Child, TRUE,
progdirLock ? NP_ProgramDir : TAG_SKIP, progdirLock,
cwdLock ? NP_CurrentDir : TAG_SKIP, cwdLock,
NP_Name, strdup(processName),

NP_EntryCode, spawnedProcessEnter,
NP_EntryData, getgid(),
// NP_EntryCode, amiga_entryCode,
// NP_EntryData, &ed,
NP_ExitCode, spawnedProcessExit,

TAG_DONE);
D(("(*)Calling SystemTags.\n"));

ret = SystemTags(full_command,
NP_NotifyOnDeathSigTask, _me,

SYS_Input, iofh[0],
SYS_Output, iofh[1],
SYS_Error, iofh[2],

// These will always be true (like you) :
// NP_CloseInput, closefh[0],
// NP_CloseOutput, closefh[1],
NP_CloseError, closefh[2], // <-- This one is needed!

SYS_UserShell, TRUE,
SYS_Asynch, TRUE,
NP_Child, TRUE,

// This is taken care of by the command shell :
// progdirLock ? NP_ProgramDir : TAG_SKIP, progdirLock,

cwdLock ? NP_CurrentDir : TAG_SKIP, cwdLock,

NP_Name, process_name,

NP_EntryCode, spawnedProcessEnter,
NP_EntryData, getgid(),
NP_ExitCode, spawnedProcessExit,

TAG_DONE);

free(full_command);
#endif

if (ret != 0) {
D(("System/CreateNewProc failed. Return value: [%ld]\n", ret));

__set_errno(__translate_io_error_to_errno(IoErr()));

/* SystemTags failed. Clean up file handles */
for (int i = 0; i < 3; i++) {
if (closefh[i])
Close(iofh[i]);
}
}
else {
D(("System/CreateNewProc succeeded. Return value: [%ld]\n", ret));

__clib4->__children++;

/*
* If mode is set as P_NOWAIT we can retrieve process id calling IoErr()
* just after SystemTags. In this case spawnv will return pid
* IoErr() must be called IMMEDIATELY after SystemTags() == no other DOS calls inbetween
*/

// DebugPrintF("[main :] Child created with success.\n");

#if USE_CNPT
pid_t pid = p->pr_ProcessID;
#else
pid_t pid = IoErr();
// gid_t groupId = getgid();
// uint32 ppid = ((struct Process *) FindTask(NULL))->pr_ProcessID;

// if (insertSpawnedChildren(pid, ppid, groupId)) {
// __CLIB4->__children++;
// D(("Children with pid %ld and gid %ld inserted into list\n", pid, groupId));
// }
// else {
// D(("Cannot insert children with pid %ld and gid %ld into list\n", pid, groupId));
// }

// DebugPrintF("[parent :] Waiting for signal from child...\n");
// Wait(1 << ed.parentSignal);
// DebugPrintF("[parent :] Signalling child...\n");
// Signal(ed.child, 1 << ed.childSignal);
// DebugPrintF("[parent :] Done.\n");
// FreeSignal(ed.parentSignal);

#endif
ret = pid;
}
// D(("SystemTags completed. return value: [%ld]\n", ret));
// printf("SystemTags completed. return value: [%ld]\n", ret);

D(("System/CreateNewProc completed. Return value: [%ld]\n", ret));

return ret;
}
Binary file added test_programs/misc/spawnv_test
View file
Open in desktop
Binary file not shown.