On 11/18/2020 1:45 AM, Eric Sunshine wrote:
> On Tue, Nov 17, 2020 at 4:13 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@xxxxxxxxx> 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
