Ensure error code is kept if destination dir is not writable

[ColemanTom]

This resolves #668. The error code was not being retained, so rc was stlil zero. I've added it in, keeping the errno value.

$ bin/dsync -X all -q m.grib2 zzfsdfzz/a; echo $?
2

[ColemanTom]

Hi @ofaaland, I was wondering if you are a maintainer and if so, have time to review this small change to ensure an error is returned from dsync when it should be (trying to write to a dir the user does not have access to).

Sorry for the nudge, I was just going through checking what PR I have open.

[daltonbohning]

This rc=errno seems reasonable, but it seems the goto maybe should actually be goto dsync_common_cleanup since some things like mfu_copy_opts_delete(&copy_opts); need to be done. Though I'm not sure if everything in that cleanup section is needed

[ColemanTom]

You're probably correct. I'm happy to change that if you want.

[jeking3]

errno should be captured earlier, before logging and freeing, since errno can be changed by those calls, perhaps:

    /* check that destination parent is writable */
    if(mfu_file_access(dest_parent_str, W_OK, mfu_dst_file) < 0) {
        mfu_loglevel log_level = MFU_LOG_ERR;
        if (options.dry_run)
            log_level = MFU_LOG_WARN;
        else
            rc = errno;
        MFU_LOG(log_level, "Destination parent directory is not writable `%s' (errno=%d %s)",
                dest_parent_str, errno, strerror(errno));
        if (!options.dry_run) {
            mfu_free(&dest_parent_str);
            goto ERROR;
        }
    }

Although not specifically related to your changes, removing ERROR would be an improvement. Other code paths use goto dsync_common_cleanup; and it would be safe to use that here as well. The ERROR: label could then also be removed. But the logic for emitting "Completed sync" should be changed to be written out only if the sync actually begins. That's something the current code does by skipping to the ERROR label.

For now perhaps change where the errno is captured and update the PR; the other concerns can be a follow-up.

[jeking3]

errno should be captured earlier, before logging and freeing, since errno can be changed by those calls, perhaps:

    /* check that destination parent is writable */
    if(mfu_file_access(dest_parent_str, W_OK, mfu_dst_file) < 0) {
        mfu_loglevel log_level = MFU_LOG_ERR;
        if (options.dry_run)
            log_level = MFU_LOG_WARN;
        else
            rc = errno;
        MFU_LOG(log_level, "Destination parent directory is not writable `%s' (errno=%d %s)",
                dest_parent_str, errno, strerror(errno));
        if (!options.dry_run) {
            mfu_free(&dest_parent_str);
            goto ERROR;
        }
    }

Although not specifically related to your changes, removing ERROR would be an improvement. Other code paths use goto dsync_common_cleanup; and it would be safe to use that here as well. The ERROR: label could then also be removed. But the logic for emitting "Completed sync" should be changed to be written out only if the sync actually begins. That's something the current code does by skipping to the ERROR label.

For now perhaps change where the errno is captured and update the PR; the other concerns can be a follow-up.