Recommendations for updating error() to error_errno()

Recommendations for updating error() to error_errno()

[牛旭]

Hi,
Our team works on enhance logging practices by learning from historical log revisions in evolution.
And we find three patches that update error(..., strerror(errmp)) to error_errno(...).

While applying this rule to git-2.14.2, we find 9 missed spot in file refs/files-backend.c, 1 in apply.c, 2 in trace.c, 4 in dir-iterator.c:.

Here are the missed spots:
1) Line 1734 in file git-2.14.2/refs/files-backend.c:
ret = raceproof_create_file(path.buf, rename_tmp_log_callback, &cb);
if (ret) {
	if (errno == EISDIR)
		error("directory not empty: %s", path.buf);
	else
		error("unable to move logfile %s to %s: %s",
			tmp.buf, path.buf,
			strerror(cb.true_errno));
}

2) Line 1795 in file git-2.14.2/refs/files-backend.c: 
if (log && rename(sb_oldref.buf, tmp_renamed_log.buf)) {
	ret = error("unable to move logfile logs/%s to logs/"TMP_RENAMED_LOG": %s",
			oldrefname, strerror(errno));
	goto out;
}

3) Line 1880, 1884 in file git-2.14.2/refs/files-backend.c:
rollbacklog:
	if (logmoved && rename(sb_newref.buf, sb_oldref.buf))
		error("unable to restore logfile %s from %s: %s",
			oldrefname, newrefname, strerror(errno));
	if (!logmoved && log &&
	    rename(tmp_renamed_log.buf, sb_oldref.buf))
		error("unable to restore logfile %s from logs/"TMP_RENAMED_LOG": %s",
			oldrefname, strerror(errno));
4) Line 2247 in file git-2.14.2/refs/files-backend.c:
if (commit_ref(lock) < 0)
	return error("unable to write symref for %s: %s", refname,
		     strerror(errno));

5) Line 2366 in file git-2.14.2/refs/files-backend.c:
if (fseek(logfp, 0, SEEK_END) < 0)
	ret = error("cannot seek back reflog for %s: %s",
		    refname, strerror(errno));

6) Line 2378, 2384 in file git-2.14.2/refs/files-backend.c:
if (fseek(logfp, pos - cnt, SEEK_SET)) {
	ret = error("cannot seek back reflog for %s: %s",
	    refname, strerror(errno));
        break;
} 
nread = fread(buf, cnt, 1, logfp);
if (nread != 1) {
	ret = error("cannot read %d bytes from reflog for %s: %s",
	    cnt, refname, strerror(errno));
	break;
}

7) Line 3312 in file git-2.14.2/refs/files-backend.c:
cb.newlog = fdopen_lock_file(&reflog_lock, "w");
if (!cb.newlog) {
	error("cannot fdopen %s (%s)",
	      get_lock_file_path(&reflog_lock), strerror(errno));
	goto failure;
}

8) Line 3337 in file git-2.14.2/refs/files-backend.c:
if (close_lock_file(&reflog_lock)) {
        status |= error("couldn't write %s: %s", log_file,
	   strerror(errno));

9) Line 3348 in file git-2.14.2/refs/files-backend.c:
static int files_reflog_expire(struct ref_store *ref_store,...
{
	...
		} else if (commit_lock_file(&reflog_lock)) {
			status |= error("unable to write reflog '%s' (%s)",
					log_file, strerror(errno));

10) Line 4859 in file git-2.14.2/apply.c:
fd = open(arg, O_RDONLY);
if (fd < 0) {
	error(_("can't open patch '%s': %s"), arg, strerror(errno));
	res = -128;
	free(to_free);
	goto end;
}

11) Line 64 in file git-2.14.2/trace.c:
int fd = open(trace, O_WRONLY | O_APPEND | O_CREAT, 0666);
if (fd == -1) {
	warning("could not open '%s' for tracing: %s",
		trace, strerror(errno));

12) Line 133 in file git-2.14.2/trace.c:
static void trace_write(struct trace_key *key, const void *buf, unsigned len)
{
	if (write_in_full(get_trace_fd(key), buf, len) < 0) {
		normalize_trace_key(&key);
		warning("unable to write trace for %s: %s",
			key->key, strerror(errno));

13) Line 74 in file git-2.14.2/dir-iterator.c:
level->dir = opendir(iter->base.path.buf);
if (!level->dir && errno != ENOENT) {
	warning("error opening directory %s: %s",
		iter->base.path.buf, strerror(errno));
        /* Popping the level is handled below */
}

14) Line 125, 128 in file git-2.14.2/dir-iterator.c:
de = readdir(level->dir);
if (!de) {
	/* This level is exhausted; pop up a level. */
	if (errno) {
		warning("error reading directory %s: %s",
			iter->base.path.buf, strerror(errno));
	} else if (closedir(level->dir))
		warning("error closing directory %s: %s",
			iter->base.path.buf, strerror(errno));

15) Line 143 in file git-2.14.2/dir-iterator.c:
if (lstat(iter->base.path.buf, &iter->base.st) < 0) {
	if (errno != ENOENT)
		warning("error reading path '%s': %s",
			iter->base.path.buf,
			strerror(errno));

16) Line 174 in file git-2.14.2/dir-iterator.c:
if (level->dir && closedir(level->dir)) {
	strbuf_setlen(&iter->base.path, level->prefix_len);
	warning("error closing directory %s: %s",
           iter->base.path.buf, strerror(errno));

Following are the 5 patches that support our opinion:
1) Line 2182 in file git/versions/git-2.8.6/config.c:
 	if (commit_lock_file(lock) < 0) {
-		error("could not write config file %s: %s", config_filename,
-		      strerror(errno));
+		error_errno("could not write config file %s", config_filename);
 		ret = CONFIG_NO_WRITE;
 		lock = NULL;
 		goto out_free;
 	}

2) Line 2389 in file git/versions/git-2.8.6/config.c:
 unlock_and_out:
 	if (commit_lock_file(lock) < 0)
-		ret = error("could not write config file %s: %s",
-			    config_filename, strerror(errno));
+		ret = error_errno("could not write config file %s",
+				  config_filename);
 out:
 	free(filename_buf);

3) Line 1825 in file git/versions/git-2.8.6/fast-import.c:
 	dump_marks_helper(f, 0, marks);
 	if (commit_lock_file(&mark_lock)) {
-		failure |= error("Unable to write file %s: %s",
-			export_marks_file, strerror(errno));
+		failure |= error_errno("Unable to write file %s",
+				       export_marks_file);

4) Line 863 in file git-2.8.6/bisect.c: 
 	/* Create file BISECT_ANCESTORS_OK. */
 	fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);
 	if (fd < 0)
-		warning("could not create file '%s': %s",
-			filename, strerror(errno));
+		warning_errno("could not create file '%s'",
+			      filename);
 	else
 		close(fd);

5) Line 78 in file git-2.8.6/ident.c
 	mailname = fopen("/etc/mailname", "r");
 	if (!mailname) {
 		if (errno != ENOENT)
-			warning("cannot open /etc/mailname: %s",
-				strerror(errno));
+			warning_errno("cannot open /etc/mailname");
 		return -1;
 	}

Thanks for reading, we are looking forward to your reply about the correctness of our suggestion.
May you a good day ^^

Best regards,
Xu

Re: Recommendations for updating error() to error_errno()

[Jeff King]

On Wed, Oct 17, 2018 at 11:58:15AM +0800, 牛旭 wrote:

> Our team works on enhance logging practices by learning from historical log revisions in evolution.
> And we find three patches that update error(..., strerror(errmp)) to error_errno(...).
> 
> While applying this rule to git-2.14.2, we find 9 missed spot in file refs/files-backend.c, 1 in apply.c, 2 in trace.c, 4 in dir-iterator.c:.
> 
> Here are the missed spots:
> 1) Line 1734 in file git-2.14.2/refs/files-backend.c:
> ret = raceproof_create_file(path.buf, rename_tmp_log_callback, &cb);
> if (ret) {
> 	if (errno == EISDIR)
> 		error("directory not empty: %s", path.buf);
> 	else
> 		error("unable to move logfile %s to %s: %s",
> 			tmp.buf, path.buf,
> 			strerror(cb.true_errno));

This cannot be converted naively. Using error_errno() will always show
the value of "errno", but here we are using a saved value in
cb.true_errno.

It might work to do:

  errno = cb.true_errno;
  error_errno(...);

but you would first have to check if the function is depending on the
value of errno for other reasons (not to mention that it kind of defeats
the purpose of error_errno() being a shorthand).

> 2) Line 1795 in file git-2.14.2/refs/files-backend.c: 
> if (log && rename(sb_oldref.buf, tmp_renamed_log.buf)) {
> 	ret = error("unable to move logfile logs/%s to logs/"TMP_RENAMED_LOG": %s",
> 			oldrefname, strerror(errno));
> 	goto out;
> }

But this one, for example, would be fine.

-Peff