Re: [PATCH v4 3/4] maintenance: use launchctl on macOS

From: Derrick Stolee <stolee@gmail.com>
Date: Nov 18, 2020, 18:22 UTC
Message-ID: <3dc00733-e736-9483-4a46-372e1560bcc2@gmail.com>
In-Reply-To: <CAPig+cRQJ9VpRk_0oz4BP3GF7dtsHtE3Qcd=nGsS26=JZvps7g@mail.gmail.com>

On 11/18/2020 1:45 AM, Eric Sunshine wrote:

Show 22 quoted lines

> On Tue, Nov 17, 2020 at 4:13 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> +static int launchctl_boot_plist(int enable, const char *filename, const char *cmd)
>> +{
>> +       child.no_stderr = 1;
>> +       child.no_stdout = 1;
>> +       if (start_command(&child))
>> +               die(_("failed to start launchctl"));
> 
> Did you have any thoughts on the observation I made in a followup
> response[1] during review of v3 in which I suggested that we might be
> able to avoid suppressing stderr (and stdout) here? In particular, the
> idea was that if, in launchctl_schedule_plist(), we do a simple
> existence check for the .plist file and only call
> launchctl_boot_plist(0,...) to `bootout` the .plist file if it exists,
> then we shouldn't need to muck with stderr/stdout suppression. The
> benefit is that if `bootout` fails for some reason, then the user
> would see the (hopefully) meaningful error message emitted `launchctl
> bootout`.
> 
> The same .plist existence check could be done in
> launchctl_remove_plist() before trying to `bootout` the file.

If the file exists but isn't still registered with launchd, then the bootout command will send output

"<path>.plist: Could not find specified service"

and return a failure code. This output isn't helpful to users, since we still are in the desired state afterwards.

> Anyhow, such refinement can be done later is desired, so not worth a
> re-roll, but I was curious about your thoughts on the issue.

This pattern of squashing the output even in the successful case is important for Windows where schtasks sends a line of output for each task being registered!

I think it would be a reasonable extension to store the error message for logging or communicating to the user if we actually need it, but I don't believe we should be piping the output directly.

Thanks, -Stolee