David Kalnischkies wrote:
> Good day mere mortal
I must say I very much enjoyed receiving this code review… *grin*
> > Patch attached.
>
> … but your sacrifice is not enough.
Hah! Okay, updated patch attached:
> On a nitpick level the cow feels kinda tricked by a method named
> GetTimeNow […] GetSecondsSinceEpoch() might be better.
Renamed.
> The cow suggests passing time forward as parameter.
I did initially try this but it makes the code/patch rather ugly in
that one needs to pass the time_t value to DooMooX as well as to
getMooLine and printMooLine… Yuck.
(Your good point about generating errors has been addressed by simply
calling exit(2) on error; if the value is bad, let's just bail out.)
> > + if (!source_date_epoch) {
>
> The cow dislikes the '!' as it is easy to miss and prefers the far
> noisier "== nullptr" here.
Agreed.
> > + const unsigned long long epoch = strtoull(source_date_epoch, &endptr,
> > 10);
>
> The speaker wonders how reproducible such a call is if it is effected by
> LANG – or is that just "bad environment setup"?
(It is not.)
> And while we are at it a minor nitpick: We prefer the const-modifier to be set
> after the type.
Updated throughout.
> > + if ((errno == ERANGE && (epoch == ULLONG_MAX || epoch == 0))
> > + || (errno != 0 && epoch == 0)) {
> > + _error->Error("SOURCE_DATE_EPOCH: strtoull: %s\n", strerror(errno));
>
> Using _error->Errno here would be more consistent.
So, I've made some bigger changes around here:
a) Use std::stringstream to parse the input. This is C++ after all
and means we don't need a cast at the end of the method, the code
is much shorter and far more readable. (eg. ss.eof() instead of
checking for a null pointer, etc.)
b) Call exit(2) on failure as described above.
c) Use _error->Fatal as its now a fatal error due to "b".
Regards,
--
,''`.
: :' : Chris Lamb
`. `'` la...@debian.org / chris-lamb.co.uk
`-
From b9a8fd427e8dfa0a9ecf017c12731df36d57d28f Mon Sep 17 00:00:00 2001
From: Chris Lamb <la...@debian.org>
Date: Sat, 31 Dec 2016 16:30:55 +0000
Subject: [PATCH] Make the "moo" reproducible (Closes: #848721)
Signed-off-by: Chris Lamb <la...@debian.org>
---
apt-private/private-moo.cc | 6 ++++--
apt-private/private-utils.cc | 24 ++++++++++++++++++++++++
apt-private/private-utils.h | 1 +
3 files changed, 29 insertions(+), 2 deletions(-)
diff --git a/apt-private/private-moo.cc b/apt-private/private-moo.cc
index a87999150..9ebb878cd 100644
--- a/apt-private/private-moo.cc
+++ b/apt-private/private-moo.cc
@@ -15,6 +15,7 @@
#include <apt-private/private-moo.h>
#include <apt-private/private-output.h>
+#include <apt-private/private-utils.h>
#include <stddef.h>
#include <string.h>
@@ -27,7 +28,7 @@
/*}}}*/
static std::string getMooLine() { /*{{{*/
- time_t const timenow = time(NULL);
+ time_t const timenow = GetSecondsSinceEpoch();
struct tm special;
localtime_r(&timenow, &special);
enum { NORMAL, PACKAGEMANAGER, APPRECIATION, AGITATION, AIRBORN } line;
@@ -163,7 +164,8 @@ bool DoMooApril(CommandLine &) /*{{{*/
/*}}}*/
bool DoMoo(CommandLine &CmdL) /*{{{*/
{
- time_t const timenow = time(NULL);
+ time_t const timenow = GetSecondsSinceEpoch();
+
struct tm april;
localtime_r(&timenow, &april);
if (april.tm_mday == 1 && april.tm_mon == 3)
diff --git a/apt-private/private-utils.cc b/apt-private/private-utils.cc
index 775bf7e79..0e1a6e886 100644
--- a/apt-private/private-utils.cc
+++ b/apt-private/private-utils.cc
@@ -1,11 +1,13 @@
#include <config.h>
#include <apt-pkg/configuration.h>
+#include <apt-pkg/error.h>
#include <apt-pkg/fileutl.h>
#include <apt-private/private-utils.h>
#include <cstdlib>
+#include <sstream>
#include <unistd.h>
// DisplayFileInPager - Display File with pager /*{{{*/
@@ -74,3 +76,25 @@ bool EditFileInSensibleEditor(std::string const &filename)
return ExecWait(Process, "editor", false);
}
/*}}}*/
+time_t GetSecondsSinceEpoch() /*{{{*/
+{
+ char const *source_date_epoch = getenv("SOURCE_DATE_EPOCH");
+
+ if (source_date_epoch == nullptr)
+ return time(NULL);
+
+ time_t epoch;
+ std::stringstream ss(source_date_epoch);
+
+ ss >> epoch;
+
+ if (ss.fail() || !ss.eof())
+ {
+ _error->Fatal("Environment variable SOURCE_DATE_EPOCH has invalid value: \"%s\"",
+ source_date_epoch);
+ exit(100);
+ }
+
+ return epoch;
+}
+ /*}}}*/
diff --git a/apt-private/private-utils.h b/apt-private/private-utils.h
index b3b249689..4d48bd1ba 100644
--- a/apt-private/private-utils.h
+++ b/apt-private/private-utils.h
@@ -5,5 +5,6 @@
bool DisplayFileInPager(std::string const &filename);
bool EditFileInSensibleEditor(std::string const &filename);
+time_t GetSecondsSinceEpoch();
#endif
--
2.11.0
