{"thread":{"id":"49585","subject":"Recommendations for updating error() to error_errno()","startedAt":"2018-10-17T04:03:43Z","lastAt":"2018-10-17T08:44:47Z","messageCount":2,"participants":["牛旭","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"360727","messageId":"6849fb3c.10199.166802c8ef3.Coremail.niuxu16@nudt.edu.cn","threadId":"49585","inReplyTo":null,"subject":"Recommendations for updating error() to error_errno()","fromName":"牛旭","fromEmail":"niuxu16@nudt.edu.cn","sentAt":"2018-10-17T03:58:15Z","receivedAt":"2018-10-17T04:03:43Z","isPatch":false,"sender":{"key":"niuxu16@nudt.edu.cn","avatar":null},"body":"Hi,\nOur team works on enhance logging practices by learning from historical log revisions in evolution.\nAnd we find three patches that update error(..., strerror(errmp)) to error_errno(...).\n\nWhile 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:.\n\nHere are the missed spots:\n1) Line 1734 in file git-2.14.2/refs/files-backend.c:\nret = raceproof_create_file(path.buf, rename_tmp_log_callback, &cb);\nif (ret) {\n\tif (errno == EISDIR)\n\t\terror(\"directory not empty: %s\", path.buf);\n\telse\n\t\terror(\"unable to move logfile %s to %s: %s\",\n\t\t\ttmp.buf, path.buf,\n\t\t\tstrerror(cb.true_errno));\n}\n\n2) Line 1795 in file git-2.14.2/refs/files-backend.c: \nif (log && rename(sb_oldref.buf, tmp_renamed_log.buf)) {\n\tret = error(\"unable to move logfile logs/%s to logs/\"TMP_RENAMED_LOG\": %s\",\n\t\t\toldrefname, strerror(errno));\n\tgoto out;\n}\n\n3) Line 1880, 1884 in file git-2.14.2/refs/files-backend.c:\nrollbacklog:\n\tif (logmoved && rename(sb_newref.buf, sb_oldref.buf))\n\t\terror(\"unable to restore logfile %s from %s: %s\",\n\t\t\toldrefname, newrefname, strerror(errno));\n\tif (!logmoved && log &&\n\t    rename(tmp_renamed_log.buf, sb_oldref.buf))\n\t\terror(\"unable to restore logfile %s from logs/\"TMP_RENAMED_LOG\": %s\",\n\t\t\toldrefname, strerror(errno));\n4) Line 2247 in file git-2.14.2/refs/files-backend.c:\nif (commit_ref(lock) < 0)\n\treturn error(\"unable to write symref for %s: %s\", refname,\n\t\t     strerror(errno));\n\n5) Line 2366 in file git-2.14.2/refs/files-backend.c:\nif (fseek(logfp, 0, SEEK_END) < 0)\n\tret = error(\"cannot seek back reflog for %s: %s\",\n\t\t    refname, strerror(errno));\n\n6) Line 2378, 2384 in file git-2.14.2/refs/files-backend.c:\nif (fseek(logfp, pos - cnt, SEEK_SET)) {\n\tret = error(\"cannot seek back reflog for %s: %s\",\n\t    refname, strerror(errno));\n        break;\n} \nnread = fread(buf, cnt, 1, logfp);\nif (nread != 1) {\n\tret = error(\"cannot read %d bytes from reflog for %s: %s\",\n\t    cnt, refname, strerror(errno));\n\tbreak;\n}\n\n7) Line 3312 in file git-2.14.2/refs/files-backend.c:\ncb.newlog = fdopen_lock_file(&reflog_lock, \"w\");\nif (!cb.newlog) {\n\terror(\"cannot fdopen %s (%s)\",\n\t      get_lock_file_path(&reflog_lock), strerror(errno));\n\tgoto failure;\n}\n\n8) Line 3337 in file git-2.14.2/refs/files-backend.c:\nif (close_lock_file(&reflog_lock)) {\n        status |= error(\"couldn't write %s: %s\", log_file,\n\t   strerror(errno));\n\n9) Line 3348 in file git-2.14.2/refs/files-backend.c:\nstatic int files_reflog_expire(struct ref_store *ref_store,...\n{\n\t...\n\t\t} else if (commit_lock_file(&reflog_lock)) {\n\t\t\tstatus |= error(\"unable to write reflog '%s' (%s)\",\n\t\t\t\t\tlog_file, strerror(errno));\n\n10) Line 4859 in file git-2.14.2/apply.c:\nfd = open(arg, O_RDONLY);\nif (fd < 0) {\n\terror(_(\"can't open patch '%s': %s\"), arg, strerror(errno));\n\tres = -128;\n\tfree(to_free);\n\tgoto end;\n}\n\n11) Line 64 in file git-2.14.2/trace.c:\nint fd = open(trace, O_WRONLY | O_APPEND | O_CREAT, 0666);\nif (fd == -1) {\n\twarning(\"could not open '%s' for tracing: %s\",\n\t\ttrace, strerror(errno));\n\n12) Line 133 in file git-2.14.2/trace.c:\nstatic void trace_write(struct trace_key *key, const void *buf, unsigned len)\n{\n\tif (write_in_full(get_trace_fd(key), buf, len) < 0) {\n\t\tnormalize_trace_key(&key);\n\t\twarning(\"unable to write trace for %s: %s\",\n\t\t\tkey->key, strerror(errno));\n\n13) Line 74 in file git-2.14.2/dir-iterator.c:\nlevel->dir = opendir(iter->base.path.buf);\nif (!level->dir && errno != ENOENT) {\n\twarning(\"error opening directory %s: %s\",\n\t\titer->base.path.buf, strerror(errno));\n        /* Popping the level is handled below */\n}\n\n14) Line 125, 128 in file git-2.14.2/dir-iterator.c:\nde = readdir(level->dir);\nif (!de) {\n\t/* This level is exhausted; pop up a level. */\n\tif (errno) {\n\t\twarning(\"error reading directory %s: %s\",\n\t\t\titer->base.path.buf, strerror(errno));\n\t} else if (closedir(level->dir))\n\t\twarning(\"error closing directory %s: %s\",\n\t\t\titer->base.path.buf, strerror(errno));\n\n15) Line 143 in file git-2.14.2/dir-iterator.c:\nif (lstat(iter->base.path.buf, &iter->base.st) < 0) {\n\tif (errno != ENOENT)\n\t\twarning(\"error reading path '%s': %s\",\n\t\t\titer->base.path.buf,\n\t\t\tstrerror(errno));\n\n16) Line 174 in file git-2.14.2/dir-iterator.c:\nif (level->dir && closedir(level->dir)) {\n\tstrbuf_setlen(&iter->base.path, level->prefix_len);\n\twarning(\"error closing directory %s: %s\",\n           iter->base.path.buf, strerror(errno));\n\nFollowing are the 5 patches that support our opinion:\n1) Line 2182 in file git/versions/git-2.8.6/config.c:\n \tif (commit_lock_file(lock) < 0) {\n-\t\terror(\"could not write config file %s: %s\", config_filename,\n-\t\t      strerror(errno));\n+\t\terror_errno(\"could not write config file %s\", config_filename);\n \t\tret = CONFIG_NO_WRITE;\n \t\tlock = NULL;\n \t\tgoto out_free;\n \t}\n\n2) Line 2389 in file git/versions/git-2.8.6/config.c:\n unlock_and_out:\n \tif (commit_lock_file(lock) < 0)\n-\t\tret = error(\"could not write config file %s: %s\",\n-\t\t\t    config_filename, strerror(errno));\n+\t\tret = error_errno(\"could not write config file %s\",\n+\t\t\t\t  config_filename);\n out:\n \tfree(filename_buf);\n\n3) Line 1825 in file git/versions/git-2.8.6/fast-import.c:\n \tdump_marks_helper(f, 0, marks);\n \tif (commit_lock_file(&mark_lock)) {\n-\t\tfailure |= error(\"Unable to write file %s: %s\",\n-\t\t\texport_marks_file, strerror(errno));\n+\t\tfailure |= error_errno(\"Unable to write file %s\",\n+\t\t\t\t       export_marks_file);\n\n4) Line 863 in file git-2.8.6/bisect.c: \n \t/* Create file BISECT_ANCESTORS_OK. */\n \tfd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);\n \tif (fd < 0)\n-\t\twarning(\"could not create file '%s': %s\",\n-\t\t\tfilename, strerror(errno));\n+\t\twarning_errno(\"could not create file '%s'\",\n+\t\t\t      filename);\n \telse\n \t\tclose(fd);\n\n5) Line 78 in file git-2.8.6/ident.c\n \tmailname = fopen(\"/etc/mailname\", \"r\");\n \tif (!mailname) {\n \t\tif (errno != ENOENT)\n-\t\t\twarning(\"cannot open /etc/mailname: %s\",\n-\t\t\t\tstrerror(errno));\n+\t\t\twarning_errno(\"cannot open /etc/mailname\");\n \t\treturn -1;\n \t}\n\nThanks for reading, we are looking forward to your reply about the correctness of our suggestion.\nMay you a good day ^^\n\nBest regards,\nXu\n"},{"id":"360733","messageId":"20181017084443.GD31932@sigill.intra.peff.net","threadId":"49585","inReplyTo":"6849fb3c.10199.166802c8ef3.Coremail.niuxu16@nudt.edu.cn","subject":"Re: Recommendations for updating error() to error_errno()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-17T08:44:43Z","receivedAt":"2018-10-17T08:44:47Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 17, 2018 at 11:58:15AM +0800, 牛旭 wrote:\n\n> Our team works on enhance logging practices by learning from historical log revisions in evolution.\n> And we find three patches that update error(..., strerror(errmp)) to error_errno(...).\n> \n> 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:.\n> \n> Here are the missed spots:\n> 1) Line 1734 in file git-2.14.2/refs/files-backend.c:\n> ret = raceproof_create_file(path.buf, rename_tmp_log_callback, &cb);\n> if (ret) {\n> \tif (errno == EISDIR)\n> \t\terror(\"directory not empty: %s\", path.buf);\n> \telse\n> \t\terror(\"unable to move logfile %s to %s: %s\",\n> \t\t\ttmp.buf, path.buf,\n> \t\t\tstrerror(cb.true_errno));\n\nThis cannot be converted naively. Using error_errno() will always show\nthe value of \"errno\", but here we are using a saved value in\ncb.true_errno.\n\nIt might work to do:\n\n  errno = cb.true_errno;\n  error_errno(...);\n\nbut you would first have to check if the function is depending on the\nvalue of errno for other reasons (not to mention that it kind of defeats\nthe purpose of error_errno() being a shorthand).\n\n> 2) Line 1795 in file git-2.14.2/refs/files-backend.c: \n> if (log && rename(sb_oldref.buf, tmp_renamed_log.buf)) {\n> \tret = error(\"unable to move logfile logs/%s to logs/\"TMP_RENAMED_LOG\": %s\",\n> \t\t\toldrefname, strerror(errno));\n> \tgoto out;\n> }\n\nBut this one, for example, would be fine.\n\n-Peff\n"}]}