job: Refactor to use pointers instead of ids

'job_start' returns the id as an out paramter, and the 'job_find' function is
now used by eval.c to translate job ids into pointers.

3 files changed, 71 insertions, 63 deletions

diff --git a/src/nvim/eval.c b/src/nvim/eval.c
index 1de2b7bbf1..00fdecc51c 100644
--- a/src/nvim/eval.c
+++ b/src/nvim/eval.c
@@ -10471,11 +10471,12 @@ static void f_job_start(typval_T *argvars, typval_T *rettv)
   // The last item of argv must be NULL
   argv[i] = NULL;
 
-  rettv->vval.v_number = job_start(argv,
-                                   xstrdup((char *)argvars[0].vval.v_string),
-                                   on_job_stdout,
-                                   on_job_stderr,
-                                   on_job_exit);
+  job_start(argv,
+            xstrdup((char *)argvars[0].vval.v_string),
+            on_job_stdout,
+            on_job_stderr,
+            on_job_exit,
+            &rettv->vval.v_number);
 
   if (rettv->vval.v_number <= 0) {
     if (rettv->vval.v_number == 0) {
@@ -10502,19 +10503,21 @@ static void f_job_stop(typval_T *argvars, typval_T *rettv)
     return;
   }
 
-  if (!job_stop(argvars[0].vval.v_number)) {
+  Job *job = job_find(argvars[0].vval.v_number);
+
+  if (!job) {
     // Probably an invalid job id
     EMSG(_(e_invjob));
     return;
   }
 
+  job_stop(job);
   rettv->vval.v_number = 1;
 }
 
 // "jobwrite()" function
 static void f_job_write(typval_T *argvars, typval_T *rettv)
 {
-  bool res;
   rettv->v_type = VAR_NUMBER;
   rettv->vval.v_number = 0;
 
@@ -10529,16 +10532,16 @@ static void f_job_write(typval_T *argvars, typval_T *rettv)
     return;
   }
 
-  res = job_write(argvars[0].vval.v_number,
-                  xstrdup((char *)argvars[1].vval.v_string),
-                  strlen((char *)argvars[1].vval.v_string));
+  Job *job = job_find(argvars[0].vval.v_number);
 
-  if (!res) {
+  if (!job) {
     // Invalid job id
     EMSG(_(e_invjob));
   }
 
-  rettv->vval.v_number = 1;
+  rettv->vval.v_number = job_write(job,
+                                   xstrdup((char *)argvars[1].vval.v_string),
+                                   strlen((char *)argvars[1].vval.v_string));
 }
 
 /*
diff --git a/src/nvim/os/channel.c b/src/nvim/os/channel.c
index 878aeac0a8..fd5eae505c 100644
--- a/src/nvim/os/channel.c
+++ b/src/nvim/os/channel.c
@@ -24,7 +24,7 @@ typedef struct {
   msgpack_unpacker *unpacker;
   msgpack_sbuffer *sbuffer;
   union {
-    int job_id;
+    Job *job;
     struct {
       RStream *read;
       WStream *write;
@@ -68,11 +68,25 @@ void channel_teardown()
 /// stdin/stdout. stderr is forwarded to the editor error stream.
 ///
 /// @param argv The argument vector for the process
-void channel_from_job(char **argv)
+bool channel_from_job(char **argv)
 {
   Channel *channel = register_channel();
   channel->is_job = true;
-  channel->data.job_id = job_start(argv, channel, job_out, job_err, NULL);
+
+  int status;
+  channel->data.job = job_start(argv,
+                                channel,
+                                job_out,
+                                job_err,
+                                NULL,
+                                &status);
+
+  if (status <= 0) {
+    close_channel(channel);
+    return false;
+  }
+
+  return true;
 }
 
 /// Creates an API channel from a libuv stream representing a tcp or
@@ -282,7 +296,9 @@ static void close_channel(Channel *channel)
   msgpack_unpacker_free(channel->unpacker);
 
   if (channel->is_job) {
-    job_stop(channel->data.job_id);
+    if (channel->data.job) {
+      job_stop(channel->data.job);
+    }
   } else {
     rstream_free(channel->data.streams.read);
     wstream_free(channel->data.streams.write);
diff --git a/src/nvim/os/job.c b/src/nvim/os/job.c
index 29f0eab24d..e5513f1c15 100644
--- a/src/nvim/os/job.c
+++ b/src/nvim/os/job.c
@@ -128,14 +128,15 @@ void job_teardown()
 /// @param stderr_cb Callback that will be invoked when data is available
 ///        on stderr
 /// @param exit_cb Callback that will be invoked when the job exits
-/// @return The job id if the job started successfully. If the the first item /
-///         of `argv`(the program) could not be executed, -1 will be returned.
-//          0 will be returned if the job table is full.
-int job_start(char **argv,
-              void *data,
-              rstream_cb stdout_cb,
-              rstream_cb stderr_cb,
-              job_exit_cb job_exit_cb)
+/// @param[out] The job id if the job started successfully, 0 if the job table
+///             is full, -1 if the program could not be executed.
+/// @return The job pointer if the job started successfully, NULL otherwise
+Job *job_start(char **argv,
+               void *data,
+               rstream_cb stdout_cb,
+               rstream_cb stderr_cb,
+               job_exit_cb job_exit_cb,
+               int *status)
 {
   int i;
   Job *job;
@@ -149,12 +150,14 @@ int job_start(char **argv,
 
   if (i == MAX_RUNNING_JOBS) {
     // No free slots
-    return 0;
+    *status = 0;
+    return NULL;
   }
 
   job = xmalloc(sizeof(Job));
   // Initialize
   job->id = i + 1;
+  *status = job->id;
   job->pending_refs = 3;
   job->pending_closes = 4;
   job->data = data;
@@ -192,7 +195,8 @@ int job_start(char **argv,
   // Spawn the job
   if (uv_spawn(uv_default_loop(), &job->proc, &job->proc_opts) != 0) {
     free_job(job);
-    return -1;
+    *status = -1;
+    return NULL;
   }
 
   // Give all handles a reference to the job
@@ -219,51 +223,45 @@ int job_start(char **argv,
   }
   job_count++;
 
-  return job->id;
+  return job;
 }
 
-/// Terminates a job. This is a non-blocking operation, but if the job exists
-/// it's guaranteed to succeed(SIGKILL will eventually be sent)
+/// Finds a job instance by id
 ///
 /// @param id The job id
-/// @return true if the stop request was successfully sent, false if the job
-///              id is invalid(probably because it has already stopped)
-bool job_stop(int id)
+/// @return the Job instance
+Job *job_find(int id)
 {
-  Job *job = find_job(id);
+  Job *job;
 
-  if (job == NULL || job->stopped) {
-    return false;
+  if (id <= 0 || id > MAX_RUNNING_JOBS || !(job = table[id - 1])
+      || job->stopped) {
+    return NULL;
   }
 
-  job->stopped = true;
+  return job;
+}
 
-  return true;
+/// Terminates a job. This is a non-blocking operation, but if the job exists
+/// it's guaranteed to succeed(SIGKILL will eventually be sent)
+///
+/// @param job The Job instance
+void job_stop(Job *job)
+{
+  job->stopped = true;
 }
 
 /// Writes data to the job's stdin. This is a non-blocking operation, it
 /// returns when the write request was sent.
 ///
-/// @param id The job id
+/// @param job The Job instance
 /// @param data Buffer containing the data to be written
 /// @param len Size of the data
-/// @return true if the write request was successfully sent, false if the job
-///              id is invalid(probably because it has already stopped)
-bool job_write(int id, char *data, uint32_t len)
+/// @return true if the write request was successfully sent, false if writing
+///         to the job stream failed (possibly because the OS buffer is full)
+bool job_write(Job *job, char *data, uint32_t len)
 {
-  Job *job = find_job(id);
-
-  if (job == NULL || job->stopped) {
-    free(data);
-    return false;
-  }
-
-  if (!wstream_write(job->in, wstream_new_buffer(data, len, free))) {
-    job_stop(job->id);
-    return false;
-  }
-
-  return true;
+  return wstream_write(job->in, wstream_new_buffer(data, len, free));
 }
 
 /// Runs the read callback associated with the job exit event
@@ -315,15 +313,6 @@ static bool is_alive(Job *job)
   return uv_process_kill(&job->proc, 0) == 0;
 }
 
-static Job * find_job(int id)
-{
-  if (id <= 0 || id > MAX_RUNNING_JOBS) {
-    return NULL;
-  }
-
-  return table[id - 1];
-}
-
 static void free_job(Job *job)
 {
   uv_close((uv_handle_t *)&job->proc_stdout, close_cb);