Discussion:
4 failing tests
(too old to reply)
Ramkumar Ramachandra
2012-11-29 19:02:12 UTC
Permalink
Hi,

The following tests fail. Is it just me, or does this indicate work
in progress?

$ ./test-id128
random: a08ea8ed34594d4bbd953dd182ec86f9
Assertion 'sd_id128_get_machine(&id) == 0' failed at
src/test/test-id128.c:41, function main(). Aborting.
[1] 8017 abort (core dumped) ./test-id128

$ ./test-journal
Assertion 'journal_file_open("test.journal", O_RDWR|O_CREAT, 0666,
true, true, NULL, NULL, NULL, &f) == 0' failed at
src/journal/test-journal.c:46, function main(). Aborting.
[1] 8059 abort (core dumped) ./test-journal

$ ./test-journal-stream
Assertion 'journal_file_open("one.journal", O_RDWR|O_CREAT, 0666,
true, false, NULL, NULL, NULL, &one) == 0' failed at
src/journal/test-journal-stream.c:88, function main(). Aborting.
[1] 8107 abort (core dumped) ./test-journal-stream

$ ./test-journal-verify
Generating...
Assertion 'journal_file_open("test.journal", O_RDWR|O_CREAT, 0666,
true, !!verification_key, NULL, NULL, NULL, &f) == 0' failed at
src/journal/test-journal-verify.c:87, function main(). Aborting.
[1] 8154 abort (core dumped) ./test-journal-verify

Thanks.

Ram
Zbigniew Jędrzejewski-Szmek
2012-11-29 19:05:51 UTC
Permalink
Post by Ramkumar Ramachandra
Hi,
The following tests fail. Is it just me, or does this indicate work
in progress?
Hi,
I think it's just you, ie. the tests are not expected to fail.

Zbyszek
Post by Ramkumar Ramachandra
$ ./test-id128
random: a08ea8ed34594d4bbd953dd182ec86f9
Assertion 'sd_id128_get_machine(&id) == 0' failed at
src/test/test-id128.c:41, function main(). Aborting.
[1] 8017 abort (core dumped) ./test-id128
$ ./test-journal
Assertion 'journal_file_open("test.journal", O_RDWR|O_CREAT, 0666,
true, true, NULL, NULL, NULL, &f) == 0' failed at
src/journal/test-journal.c:46, function main(). Aborting.
[1] 8059 abort (core dumped) ./test-journal
$ ./test-journal-stream
Assertion 'journal_file_open("one.journal", O_RDWR|O_CREAT, 0666,
true, false, NULL, NULL, NULL, &one) == 0' failed at
src/journal/test-journal-stream.c:88, function main(). Aborting.
[1] 8107 abort (core dumped) ./test-journal-stream
$ ./test-journal-verify
Generating...
Assertion 'journal_file_open("test.journal", O_RDWR|O_CREAT, 0666,
true, !!verification_key, NULL, NULL, NULL, &f) == 0' failed at
src/journal/test-journal-verify.c:87, function main(). Aborting.
[1] 8154 abort (core dumped) ./test-journal-verify
Ramkumar Ramachandra
2012-11-30 10:24:00 UTC
Permalink
/etc/machine-id does not exist on all distributions, and DBus puts its
machine-id in /var/lib/dbus/machine-id. One test in test-id128 fails
if the file isn't present. Make systemd-machine-id-setup create the
file if it doesn't exist, and fix a failing test in test-id128.

Signed-off-by: Ramkumar Ramachandra <***@gmail.com>
---
Post by Ramkumar Ramachandra
$ ./test-id128
random: a08ea8ed34594d4bbd953dd182ec86f9
Assertion 'sd_id128_get_machine(&id) == 0' failed at
src/test/test-id128.c:41, function main(). Aborting.
[1] 8017 abort (core dumped) ./test-id128
Okay, this test fails because I don't have a /etc/machine-id -- I
thought systemd is supposed to create it? However, from the logic in
src/core/machine-id-setup.c, it looks like although open() is called
with O_CREAT on /etc/machine-id, systemd barfs if the file isn't
present. How about changing this?

src/core/machine-id-setup.c | 12 +++++-------
src/test/test-id128.c | 6 ++++--
2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/core/machine-id-setup.c b/src/core/machine-id-setup.c
index 7f4c23b..3f21d58 100644
--- a/src/core/machine-id-setup.c
+++ b/src/core/machine-id-setup.c
@@ -168,12 +168,8 @@ int machine_id_setup(void) {
writable = true;
else {
fd = open("/etc/machine-id", O_RDONLY|O_CLOEXEC|O_NOCTTY);
- if (fd < 0) {
- umask(m);
- log_error("Cannot open /etc/machine-id: %m");
- return -errno;
- }
-
+ if (fd < 0)
+ goto generate;
writable = false;
}

@@ -192,7 +188,9 @@ int machine_id_setup(void) {
}
}

- /* Hmm, so, the id currently stored is not useful, then let's
+generate:
+ /* Hmm, so, either /etc/machine-id doesn't exist, the id
+ * currently stored is not useful, then let's
* generate one */

r = generate(id);
diff --git a/src/test/test-id128.c b/src/test/test-id128.c
index bfd743e..60902d0 100644
--- a/src/test/test-id128.c
+++ b/src/test/test-id128.c
@@ -38,8 +38,10 @@ int main(int argc, char *argv[]) {
assert_se(sd_id128_from_string(t, &id2) == 0);
assert_se(sd_id128_equal(id, id2));

- assert_se(sd_id128_get_machine(&id) == 0);
- printf("machine: %s\n", sd_id128_to_string(id, t));
+ if (sd_id128_get_machine(&id) < 0)
+ printf("machine: run systemd-machine-id-setup first\n");
+ else
+ printf("machine: %s\n", sd_id128_to_string(id, t));

assert_se(sd_id128_get_boot(&id) == 0);
printf("boot: %s\n", sd_id128_to_string(id, t));
--
1.7.8.1.362.g5d6df.dirty
Ramkumar Ramachandra
2012-11-30 10:43:02 UTC
Permalink
Post by Ramkumar Ramachandra
$ ./test-journal
Assertion 'journal_file_open("test.journal", O_RDWR|O_CREAT, 0666,
true, true, NULL, NULL, NULL, &f) == 0' failed at
src/journal/test-journal.c:46, function main(). Aborting.
[1] 8059 abort (core dumped) ./test-journal
$ ./test-journal-stream
Assertion 'journal_file_open("one.journal", O_RDWR|O_CREAT, 0666,
true, false, NULL, NULL, NULL, &one) == 0' failed at
src/journal/test-journal-stream.c:88, function main(). Aborting.
[1] 8107 abort (core dumped) ./test-journal-stream
$ ./test-journal-verify
Generating...
Assertion 'journal_file_open("test.journal", O_RDWR|O_CREAT, 0666,
true, !!verification_key, NULL, NULL, NULL, &f) == 0' failed at
src/journal/test-journal-verify.c:87, function main(). Aborting.
[1] 8154 abort (core dumped) ./test-journal-verify
These tests fail due to the code in src/journal-file.c:167, which
again checks for /etc/machine-id. Why do you expect this file to be
present?

Ram
Kay Sievers
2012-11-30 12:09:01 UTC
Permalink
On Fri, Nov 30, 2012 at 11:43 AM, Ramkumar Ramachandra
Post by Ramkumar Ramachandra
Post by Ramkumar Ramachandra
$ ./test-journal
Assertion 'journal_file_open("test.journal", O_RDWR|O_CREAT, 0666,
true, true, NULL, NULL, NULL, &f) == 0' failed at
src/journal/test-journal.c:46, function main(). Aborting.
[1] 8059 abort (core dumped) ./test-journal
$ ./test-journal-stream
Assertion 'journal_file_open("one.journal", O_RDWR|O_CREAT, 0666,
true, false, NULL, NULL, NULL, &one) == 0' failed at
src/journal/test-journal-stream.c:88, function main(). Aborting.
[1] 8107 abort (core dumped) ./test-journal-stream
$ ./test-journal-verify
Generating...
Assertion 'journal_file_open("test.journal", O_RDWR|O_CREAT, 0666,
true, !!verification_key, NULL, NULL, NULL, &f) == 0' failed at
src/journal/test-journal-verify.c:87, function main(). Aborting.
[1] 8154 abort (core dumped) ./test-journal-verify
These tests fail due to the code in src/journal-file.c:167, which
again checks for /etc/machine-id. Why do you expect this file to be
present?
It is mandatory for systemd systems. We just require it to be around,
and do not work around it.

Kay
Zbigniew Jędrzejewski-Szmek
2012-11-30 12:28:07 UTC
Permalink
Post by Kay Sievers
On Fri, Nov 30, 2012 at 11:43 AM, Ramkumar Ramachandra
Post by Ramkumar Ramachandra
Post by Ramkumar Ramachandra
$ ./test-journal
Assertion 'journal_file_open("test.journal", O_RDWR|O_CREAT, 0666,
true, true, NULL, NULL, NULL, &f) == 0' failed at
src/journal/test-journal.c:46, function main(). Aborting.
[1] 8059 abort (core dumped) ./test-journal
$ ./test-journal-stream
Assertion 'journal_file_open("one.journal", O_RDWR|O_CREAT, 0666,
true, false, NULL, NULL, NULL, &one) == 0' failed at
src/journal/test-journal-stream.c:88, function main(). Aborting.
[1] 8107 abort (core dumped) ./test-journal-stream
$ ./test-journal-verify
Generating...
Assertion 'journal_file_open("test.journal", O_RDWR|O_CREAT, 0666,
true, !!verification_key, NULL, NULL, NULL, &f) == 0' failed at
src/journal/test-journal-verify.c:87, function main(). Aborting.
[1] 8154 abort (core dumped) ./test-journal-verify
These tests fail due to the code in src/journal-file.c:167, which
again checks for /etc/machine-id. Why do you expect this file to be
present?
It is mandatory for systemd systems. We just require it to be around,
and do not work around it.
It'll be created by systemd during init if not present. So we _do_ a
kind of around. I haven't looked at the details of the patch, but
I think that it would be nice to support testing before the first run
with systemd: the tests should support missing /etc/machine-id.

Zbyszek
Kay Sievers
2012-11-30 12:37:24 UTC
Permalink
On Fri, Nov 30, 2012 at 1:28 PM, Zbigniew Jędrzejewski-Szmek
Post by Zbigniew Jędrzejewski-Szmek
It'll be created by systemd during init if not present. So we _do_ a
kind of around. I haven't looked at the details of the patch, but
I think that it would be nice to support testing before the first run
with systemd: the tests should support missing /etc/machine-id.
Don't know if it matters that much. Failing the test with a proper
error message sounds reasonable good enough to me.

The code should surely not get active during normal operation or any
service startup/restart, and magically try to fix the system in a way
we can't know what the right fix should be.

Kay
Ramkumar Ramachandra
2012-11-30 12:45:10 UTC
Permalink
Post by Kay Sievers
On Fri, Nov 30, 2012 at 1:28 PM, Zbigniew Jędrzejewski-Szmek
Post by Zbigniew Jędrzejewski-Szmek
It'll be created by systemd during init if not present. So we _do_ a
kind of around.
Where is the creation code? The systemd-machine-id-setup binary does
_not_ create it, and my patch proposes to fix that.
Post by Kay Sievers
Post by Zbigniew Jędrzejewski-Szmek
I haven't looked at the details of the patch, but
I think that it would be nice to support testing before the first run
with systemd: the tests should support missing /etc/machine-id.
Don't know if it matters that much. Failing the test with a proper
error message sounds reasonable good enough to me.
How do you propose we do that? /etc/machine-id is hardcoded in
src/libsystemd-id128/sd-id128.c:107. Instead of returning -errno, we
can return a special error value when /etc/machine-id can't be
open()'ed, and cascade that value upwards from callers, finally
handling it in the test.
Post by Kay Sievers
The code should surely not get active during normal operation or any
service startup/restart, and magically try to fix the system in a way
we can't know what the right fix should be.
My patch only proposes that systemd-machine-id-setup creates the file;
nothing else.

Ram
Kay Sievers
2012-11-30 12:58:43 UTC
Permalink
On Fri, Nov 30, 2012 at 1:45 PM, Ramkumar Ramachandra
Post by Ramkumar Ramachandra
Post by Kay Sievers
On Fri, Nov 30, 2012 at 1:28 PM, Zbigniew Jędrzejewski-Szmek
Post by Zbigniew Jędrzejewski-Szmek
It'll be created by systemd during init if not present. So we _do_ a
kind of around.
Where is the creation code? The systemd-machine-id-setup binary does
_not_ create it, and my patch proposes to fix that.
We overmount an empty file if needed. We do not want to create
anything if we do not know what to do, we should just fail with an
explaination, not mess around in the system!
Post by Ramkumar Ramachandra
Post by Kay Sievers
Post by Zbigniew Jędrzejewski-Szmek
I haven't looked at the details of the patch, but
I think that it would be nice to support testing before the first run
with systemd: the tests should support missing /etc/machine-id.
Don't know if it matters that much. Failing the test with a proper
error message sounds reasonable good enough to me.
How do you propose we do that? /etc/machine-id is hardcoded in
src/libsystemd-id128/sd-id128.c:107. Instead of returning -errno, we
can return a special error value when /etc/machine-id can't be
open()'ed, and cascade that value upwards from callers, finally
handling it in the test.
Whatever fits. As said, I consider running the tests on non-systemd
machines not a priority.
Post by Ramkumar Ramachandra
Post by Kay Sievers
The code should surely not get active during normal operation or any
service startup/restart, and magically try to fix the system in a way
we can't know what the right fix should be.
My patch only proposes that systemd-machine-id-setup creates the file;
nothing else.
Right, I see and understand what and why you are trying to do that,
but I don't think system should do that.

Booting should not create essential files in /etc which will be
persistent and change the behaviour of the system.

Kay
Ramkumar Ramachandra
2012-11-30 13:20:04 UTC
Permalink
Post by Kay Sievers
On Fri, Nov 30, 2012 at 1:45 PM, Ramkumar Ramachandra
Post by Ramkumar Ramachandra
Where is the creation code? The systemd-machine-id-setup binary does
_not_ create it, and my patch proposes to fix that.
We overmount an empty file if needed. We do not want to create
anything if we do not know what to do, we should just fail with an
explaination, not mess around in the system!
I see.
Post by Kay Sievers
Post by Ramkumar Ramachandra
My patch only proposes that systemd-machine-id-setup creates the file;
nothing else.
Right, I see and understand what and why you are trying to do that,
but I don't think system should do that.
Booting should not create essential files in /etc which will be
persistent and change the behaviour of the system.
My stupidity. Please ignore my previous patch. I've sent out a new
one to skip the tests dependent on /etc/machine-id if it's not
present.

Ram
Timothy Pepper
2012-11-30 17:43:43 UTC
Permalink
Post by Kay Sievers
On Fri, Nov 30, 2012 at 1:45 PM, Ramkumar Ramachandra
Post by Ramkumar Ramachandra
Post by Kay Sievers
On Fri, Nov 30, 2012 at 1:28 PM, Zbigniew Jędrzejewski-Szmek
Post by Zbigniew Jędrzejewski-Szmek
It'll be created by systemd during init if not present. So we _do_ a
kind of around.
Where is the creation code? The systemd-machine-id-setup binary does
_not_ create it, and my patch proposes to fix that.
We overmount an empty file if needed. We do not want to create
anything if we do not know what to do, we should just fail with an
explaination, not mess around in the system!
It would be nice to at least do an overmount in cases where something
sane for a default could be created at boot. For kicks I removed
/etc/machine-id on a test machine and the machine no longer booted,
instead just spewing to the console repeatedly:
systemd[1]: Failed to start Journal Service.
--
Tim Pepper <***@linux.intel.com>
Intel Open Source Technology Centre
Kay Sievers
2012-11-30 18:26:21 UTC
Permalink
On Fri, Nov 30, 2012 at 6:43 PM, Timothy Pepper
Post by Timothy Pepper
Post by Kay Sievers
On Fri, Nov 30, 2012 at 1:45 PM, Ramkumar Ramachandra
Post by Ramkumar Ramachandra
Post by Kay Sievers
On Fri, Nov 30, 2012 at 1:28 PM, Zbigniew Jędrzejewski-Szmek
Post by Zbigniew Jędrzejewski-Szmek
It'll be created by systemd during init if not present. So we _do_ a
kind of around.
Where is the creation code? The systemd-machine-id-setup binary does
_not_ create it, and my patch proposes to fix that.
We overmount an empty file if needed. We do not want to create
anything if we do not know what to do, we should just fail with an
explaination, not mess around in the system!
It would be nice to at least do an overmount in cases where something
sane for a default could be created at boot.
We obviously cannot over mount a non-existing file :) You have to
leave it empty, then we will do that.
Post by Timothy Pepper
For kicks I removed
/etc/machine-id on a test machine and the machine no longer booted,
Well, a fork in the eye hurts or kicks sometimes. :)
Post by Timothy Pepper
systemd[1]: Failed to start Journal Service.
Yeah, right, the error should tell what's wrong.

Kay
Timothy Pepper
2012-11-30 22:00:44 UTC
Permalink
Post by Kay Sievers
Post by Timothy Pepper
It would be nice to at least do an overmount in cases where something
sane for a default could be created at boot.
We obviously cannot over mount a non-existing file :) You have to
leave it empty, then we will do that.
I suppose I was thinking of the broader case of unexpected content,
which non-existing file may or may not be within.
Post by Kay Sievers
Post by Timothy Pepper
For kicks I removed
/etc/machine-id on a test machine and the machine no longer booted,
Well, a fork in the eye hurts or kicks sometimes. :)
Post by Timothy Pepper
systemd[1]: Failed to start Journal Service.
Yeah, right, the error should tell what's wrong.
With an existing but empty /etc/machine-id I do get the slightly more
useful message at boot:
Installed transient /etc/machine-id file.
and the much more useful everything seems to continue running fine
(with an overmounted tmpfs /etc/machine-id with a generated id).

Given a quick read through of that code path, I probably did get a:
"Cannot open /etc/machine-id: %m"
out of machine_id_setup() without a machine-id file, but didn't see it go
by on my console. The code indeed does try to open O_RDWR|O_CREAT, but
the trick is it fails since this is early and the fs is mounted readonly.
The code falls back to a readonly open and would even try to overmount
but the file doesn't exist as a mount point and is not creatable...so
there's not much that can be done anymore. Booting with 'rw' instead of
'ro', I do get both a new machine-id file and content created.

So not only is the file required by systemd, there is actually a full
best effort to insure both sane content and the presence of the file.
--
Tim Pepper <***@linux.intel.com>
Intel Open Source Technology Centre
Ramkumar Ramachandra
2012-11-30 13:17:42 UTC
Permalink
The following tests fail if /etc/machine-id is not present:

$ ./test-id128
random: a08ea8ed34594d4bbd953dd182ec86f9
Assertion 'sd_id128_get_machine(&id) == 0' failed at
src/test/test-id128.c:41, function main(). Aborting.
[1] 8017 abort (core dumped) ./test-id128

$ ./test-journal
Assertion 'journal_file_open("test.journal", O_RDWR|O_CREAT, 0666,
true, true, NULL, NULL, NULL, &f) == 0' failed at
src/journal/test-journal.c:46, function main(). Aborting.
[1] 8059 abort (core dumped) ./test-journal

$ ./test-journal-stream
Assertion 'journal_file_open("one.journal", O_RDWR|O_CREAT, 0666,
true, false, NULL, NULL, NULL, &one) == 0' failed at
src/journal/test-journal-stream.c:88, function main(). Aborting.
[1] 8107 abort (core dumped) ./test-journal-stream

$ ./test-journal-verify
Generating...
Assertion 'journal_file_open("test.journal", O_RDWR|O_CREAT, 0666,
true, !!verification_key, NULL, NULL, NULL, &f) == 0' failed at
src/journal/test-journal-verify.c:87, function main(). Aborting.
[1] 8154 abort (core dumped) ./test-journal-verify

This is because they call sd_id128_get_machine() which barfs if
/etc/machine-id can't be open()'ed. Treat this as a special case and
skip the dependent tests instead of failing them.

Signed-off-by: Ramkumar Ramachandra <***@gmail.com>
---
Post by Ramkumar Ramachandra
Post by Kay Sievers
On Fri, Nov 30, 2012 at 1:28 PM, Zbigniew Jędrzejewski-Szmek
Post by Zbigniew Jędrzejewski-Szmek
I haven't looked at the details of the patch, but
I think that it would be nice to support testing before the first run
with systemd: the tests should support missing /etc/machine-id.
Don't know if it matters that much. Failing the test with a proper
error message sounds reasonable good enough to me.
How do you propose we do that? /etc/machine-id is hardcoded in
src/libsystemd-id128/sd-id128.c:107. Instead of returning -errno, we
can return a special error value when /etc/machine-id can't be
open()'ed, and cascade that value upwards from callers, finally
handling it in the test.
This is a patch that does exactly that.

src/journal/test-journal-stream.c | 5 +++++
src/journal/test-journal-verify.c | 5 +++++
src/journal/test-journal.c | 5 +++++
src/libsystemd-id128/sd-id128.c | 2 +-
src/test/test-id128.c | 8 ++++++--
5 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/src/journal/test-journal-stream.c b/src/journal/test-journal-stream.c
index b3e816d..eaf9daa 100644
--- a/src/journal/test-journal-stream.c
+++ b/src/journal/test-journal-stream.c
@@ -85,6 +85,11 @@ int main(int argc, char *argv[]) {
assert_se(mkdtemp(t));
assert_se(chdir(t) >= 0);

+ if (journal_file_open("one.journal", O_RDWR|O_CREAT, 0666, true, false, NULL, NULL, NULL, &one) == -128) {
+ printf("skipping test: /etc/machine-id not preset\n");
+ return 0;
+ }
+
assert_se(journal_file_open("one.journal", O_RDWR|O_CREAT, 0666, true, false, NULL, NULL, NULL, &one) == 0);
assert_se(journal_file_open("two.journal", O_RDWR|O_CREAT, 0666, true, false, NULL, NULL, NULL, &two) == 0);
assert_se(journal_file_open("three.journal", O_RDWR|O_CREAT, 0666, true, false, NULL, NULL, NULL, &three) == 0);
diff --git a/src/journal/test-journal-verify.c b/src/journal/test-journal-verify.c
index b667721..00068fb 100644
--- a/src/journal/test-journal-verify.c
+++ b/src/journal/test-journal-verify.c
@@ -84,6 +84,11 @@ int main(int argc, char *argv[]) {

log_info("Generating...");

+ if (journal_file_open("test.journal", O_RDWR|O_CREAT, 0666, true, !!verification_key, NULL, NULL, NULL, &f) == -128) {
+ printf("skipping test: /etc/machine-id not preset\n");
+ return 0;
+ }
+
assert_se(journal_file_open("test.journal", O_RDWR|O_CREAT, 0666, true, !!verification_key, NULL, NULL, NULL, &f) == 0);

for (n = 0; n < N_ENTRIES; n++) {
diff --git a/src/journal/test-journal.c b/src/journal/test-journal.c
index f4dc52c..73c7554 100644
--- a/src/journal/test-journal.c
+++ b/src/journal/test-journal.c
@@ -43,6 +43,11 @@ int main(int argc, char *argv[]) {
assert_se(mkdtemp(t));
assert_se(chdir(t) >= 0);

+ if (journal_file_open("test.journal", O_RDWR|O_CREAT, 0666, true, true, NULL, NULL, NULL, &f) == -128) {
+ printf("skipping test: /etc/machine-id not preset\n");
+ return 0;
+ }
+
assert_se(journal_file_open("test.journal", O_RDWR|O_CREAT, 0666, true, true, NULL, NULL, NULL, &f) == 0);

dual_timestamp_get(&ts);
diff --git a/src/libsystemd-id128/sd-id128.c b/src/libsystemd-id128/sd-id128.c
index 4286ae7..4e5aaad 100644
--- a/src/libsystemd-id128/sd-id128.c
+++ b/src/libsystemd-id128/sd-id128.c
@@ -106,7 +106,7 @@ _public_ int sd_id128_get_machine(sd_id128_t *ret) {

fd = open("/etc/machine-id", O_RDONLY|O_CLOEXEC|O_NOCTTY);
if (fd < 0)
- return -errno;
+ return -128; /* Special return value */

k = loop_read(fd, buf, 32, false);
close_nointr_nofail(fd);
diff --git a/src/test/test-id128.c b/src/test/test-id128.c
index bfd743e..ea621f9 100644
--- a/src/test/test-id128.c
+++ b/src/test/test-id128.c
@@ -38,8 +38,12 @@ int main(int argc, char *argv[]) {
assert_se(sd_id128_from_string(t, &id2) == 0);
assert_se(sd_id128_equal(id, id2));

- assert_se(sd_id128_get_machine(&id) == 0);
- printf("machine: %s\n", sd_id128_to_string(id, t));
+ if (sd_id128_get_machine(&id) == -128)
+ printf("skipping test: /etc/machine-id not preset\n");
+ else {
+ assert_se(sd_id128_get_machine(&id) == 0);
+ printf("machine: %s\n", sd_id128_to_string(id, t));
+ }

assert_se(sd_id128_get_boot(&id) == 0);
printf("boot: %s\n", sd_id128_to_string(id, t));
--
1.7.8.1.362.g5d6df.dirty
Zbigniew Jędrzejewski-Szmek
2012-11-30 13:21:30 UTC
Permalink
Post by Ramkumar Ramachandra
+ if (journal_file_open("one.journal", O_RDWR|O_CREAT, 0666, true, false, NULL, NULL, NULL, &one) == -128) {
+ printf("skipping test: /etc/machine-id not preset\n");
+ return 0;
+ }
+
assert_se(journal_file_open("one.journal", O_RDWR|O_CREAT, 0666, true,Hm, this seems fairly intrusive. What about just adding a simple
test in the Makefile, which adds this test to the list of tests
only if /etc/machine-id does not exist?

Zbyszek
Ramkumar Ramachandra
2012-11-30 14:44:20 UTC
Permalink
The following tests fail if /etc/machine-id is not present:

$ ./test-id128
random: a08ea8ed34594d4bbd953dd182ec86f9
Assertion 'sd_id128_get_machine(&id) == 0' failed at
src/test/test-id128.c:41, function main(). Aborting.
[1] 8017 abort (core dumped) ./test-id128

$ ./test-journal
Assertion 'journal_file_open("test.journal", O_RDWR|O_CREAT, 0666,
true, true, NULL, NULL, NULL, &f) == 0' failed at
src/journal/test-journal.c:46, function main(). Aborting.
[1] 8059 abort (core dumped) ./test-journal

$ ./test-journal-stream
Assertion 'journal_file_open("one.journal", O_RDWR|O_CREAT, 0666,
true, false, NULL, NULL, NULL, &one) == 0' failed at
src/journal/test-journal-stream.c:88, function main(). Aborting.
[1] 8107 abort (core dumped) ./test-journal-stream

$ ./test-journal-verify
Generating...
Assertion 'journal_file_open("test.journal", O_RDWR|O_CREAT, 0666,
true, !!verification_key, NULL, NULL, NULL, &f) == 0' failed at
src/journal/test-journal-verify.c:87, function main(). Aborting.
[1] 8154 abort (core dumped) ./test-journal-verify

This is because they call sd_id128_get_machine() which barfs if
/etc/machine-id can't be open()'ed. Treat this as a special case and
skip the dependent tests instead of failing them.
---
Post by Zbigniew Jędrzejewski-Szmek
Post by Ramkumar Ramachandra
+ if (journal_file_open("one.journal", O_RDWR|O_CREAT, 0666, true, false, NULL, NULL, NULL, &one) == -128) {
+ printf("skipping test: /etc/machine-id not preset\n");
+ return 0;
+ }
+
assert_se(journal_file_open("one.journal", O_RDWR|O_CREAT, 0666, true,Hm, this seems fairly intrusive. What about just adding a simple
test in the Makefile, which adds this test to the list of tests
only if /etc/machine-id does not exist?
I wouldn't like that. I don't want to have to regenerate my Makefile
after installing /etc/machine-id. I don't think it's elegant to have
a different number of tests depending on different conditions.

How about this patch as an alternative? I call sd_id_get_machine()
before anything else happens.

src/journal/test-journal-stream.c | 5 +++++
src/journal/test-journal-verify.c | 5 +++++
src/journal/test-journal.c | 5 +++++
src/libsystemd-id128/sd-id128.c | 2 +-
src/test/test-id128.c | 8 ++++++--
5 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/src/journal/test-journal-stream.c b/src/journal/test-journal-stream.c
index b3e816d..b3dd140 100644
--- a/src/journal/test-journal-stream.c
+++ b/src/journal/test-journal-stream.c
@@ -80,6 +80,11 @@ int main(int argc, char *argv[]) {
const void *data;
size_t l;

+ if (sd_id128_get_machine(&id) == -128) {
+ printf("skipping test: /etc/machine-id not preset\n");
+ return 0;
+ }
+
log_set_max_level(LOG_DEBUG);

assert_se(mkdtemp(t));
diff --git a/src/journal/test-journal-verify.c b/src/journal/test-journal-verify.c
index b667721..54dff9b 100644
--- a/src/journal/test-journal-verify.c
+++ b/src/journal/test-journal-verify.c
@@ -77,6 +77,11 @@ int main(int argc, char *argv[]) {
struct stat st;
uint64_t p;

+ if (sd_id128_get_machine(&id) == -128) {
+ printf("skipping test: /etc/machine-id not preset\n");
+ return 0;
+ }
+
log_set_max_level(LOG_DEBUG);

assert_se(mkdtemp(t));
diff --git a/src/journal/test-journal.c b/src/journal/test-journal.c
index f4dc52c..c23aca0 100644
--- a/src/journal/test-journal.c
+++ b/src/journal/test-journal.c
@@ -38,6 +38,11 @@ int main(int argc, char *argv[]) {
uint64_t p;
char t[] = "/tmp/journal-XXXXXX";

+ if (sd_id128_get_machine(&id) == -128) {
+ printf("skipping test: /etc/machine-id not preset\n");
+ return 0;
+ }
+
log_set_max_level(LOG_DEBUG);

assert_se(mkdtemp(t));
diff --git a/src/libsystemd-id128/sd-id128.c b/src/libsystemd-id128/sd-id128.c
index 4286ae7..4e5aaad 100644
--- a/src/libsystemd-id128/sd-id128.c
+++ b/src/libsystemd-id128/sd-id128.c
@@ -106,7 +106,7 @@ _public_ int sd_id128_get_machine(sd_id128_t *ret) {

fd = open("/etc/machine-id", O_RDONLY|O_CLOEXEC|O_NOCTTY);
if (fd < 0)
- return -errno;
+ return -128; /* Special return value */

k = loop_read(fd, buf, 32, false);
close_nointr_nofail(fd);
diff --git a/src/test/test-id128.c b/src/test/test-id128.c
index bfd743e..ea621f9 100644
--- a/src/test/test-id128.c
+++ b/src/test/test-id128.c
@@ -38,8 +38,12 @@ int main(int argc, char *argv[]) {
assert_se(sd_id128_from_string(t, &id2) == 0);
assert_se(sd_id128_equal(id, id2));

- assert_se(sd_id128_get_machine(&id) == 0);
- printf("machine: %s\n", sd_id128_to_string(id, t));
+ if (sd_id128_get_machine(&id) == -128)
+ printf("skipping test: /etc/machine-id not preset\n");
+ else {
+ assert_se(sd_id128_get_machine(&id) == 0);
+ printf("machine: %s\n", sd_id128_to_string(id, t));
+ }

assert_se(sd_id128_get_boot(&id) == 0);
printf("boot: %s\n", sd_id128_to_string(id, t));
--
1.7.8.1.362.g5d6df.dirty
Ramkumar Ramachandra
2012-11-30 14:52:15 UTC
Permalink
The following tests fail if /etc/machine-id is not present:

$ ./test-id128
random: a08ea8ed34594d4bbd953dd182ec86f9
Assertion 'sd_id128_get_machine(&id) == 0' failed at
src/test/test-id128.c:41, function main(). Aborting.
[1] 8017 abort (core dumped) ./test-id128

$ ./test-journal
Assertion 'journal_file_open("test.journal", O_RDWR|O_CREAT, 0666,
true, true, NULL, NULL, NULL, &f) == 0' failed at
src/journal/test-journal.c:46, function main(). Aborting.
[1] 8059 abort (core dumped) ./test-journal

$ ./test-journal-stream
Assertion 'journal_file_open("one.journal", O_RDWR|O_CREAT, 0666,
true, false, NULL, NULL, NULL, &one) == 0' failed at
src/journal/test-journal-stream.c:88, function main(). Aborting.
[1] 8107 abort (core dumped) ./test-journal-stream

$ ./test-journal-verify
Generating...
Assertion 'journal_file_open("test.journal", O_RDWR|O_CREAT, 0666,
true, !!verification_key, NULL, NULL, NULL, &f) == 0' failed at
src/journal/test-journal-verify.c:87, function main(). Aborting.
[1] 8154 abort (core dumped) ./test-journal-verify

This is because they call sd_id128_get_machine() which barfs if
/etc/machine-id can't be open()'ed. Treat this as a special case and
skip the dependent tests instead of failing them.
---
Oops; forgot to declate `id' in the previous iteration.

src/journal/test-journal-stream.c | 6 ++++++
src/journal/test-journal-verify.c | 6 ++++++
src/journal/test-journal.c | 6 ++++++
src/libsystemd-id128/sd-id128.c | 2 +-
src/test/test-id128.c | 8 ++++++--
5 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/src/journal/test-journal-stream.c b/src/journal/test-journal-stream.c
index b3e816d..d07448f 100644
--- a/src/journal/test-journal-stream.c
+++ b/src/journal/test-journal-stream.c
@@ -79,6 +79,12 @@ int main(int argc, char *argv[]) {
char *z;
const void *data;
size_t l;
+ sd_id128_t id;
+
+ if (sd_id128_get_machine(&id) == -128) {
+ printf("skipping test: /etc/machine-id not preset\n");
+ return 0;
+ }

log_set_max_level(LOG_DEBUG);

diff --git a/src/journal/test-journal-verify.c b/src/journal/test-journal-verify.c
index b667721..ee55485 100644
--- a/src/journal/test-journal-verify.c
+++ b/src/journal/test-journal-verify.c
@@ -76,6 +76,12 @@ int main(int argc, char *argv[]) {
char c[FORMAT_TIMESPAN_MAX];
struct stat st;
uint64_t p;
+ sd_id128_t id;
+
+ if (sd_id128_get_machine(&id) == -128) {
+ printf("skipping test: /etc/machine-id not preset\n");
+ return 0;
+ }

log_set_max_level(LOG_DEBUG);

diff --git a/src/journal/test-journal.c b/src/journal/test-journal.c
index f4dc52c..192b787 100644
--- a/src/journal/test-journal.c
+++ b/src/journal/test-journal.c
@@ -37,6 +37,12 @@ int main(int argc, char *argv[]) {
Object *o;
uint64_t p;
char t[] = "/tmp/journal-XXXXXX";
+ sd_id128_t id;
+
+ if (sd_id128_get_machine(&id) == -128) {
+ printf("skipping test: /etc/machine-id not preset\n");
+ return 0;
+ }

log_set_max_level(LOG_DEBUG);

diff --git a/src/libsystemd-id128/sd-id128.c b/src/libsystemd-id128/sd-id128.c
index 4286ae7..4e5aaad 100644
--- a/src/libsystemd-id128/sd-id128.c
+++ b/src/libsystemd-id128/sd-id128.c
@@ -106,7 +106,7 @@ _public_ int sd_id128_get_machine(sd_id128_t *ret) {

fd = open("/etc/machine-id", O_RDONLY|O_CLOEXEC|O_NOCTTY);
if (fd < 0)
- return -errno;
+ return -128; /* Special return value */

k = loop_read(fd, buf, 32, false);
close_nointr_nofail(fd);
diff --git a/src/test/test-id128.c b/src/test/test-id128.c
index bfd743e..ea621f9 100644
--- a/src/test/test-id128.c
+++ b/src/test/test-id128.c
@@ -38,8 +38,12 @@ int main(int argc, char *argv[]) {
assert_se(sd_id128_from_string(t, &id2) == 0);
assert_se(sd_id128_equal(id, id2));

- assert_se(sd_id128_get_machine(&id) == 0);
- printf("machine: %s\n", sd_id128_to_string(id, t));
+ if (sd_id128_get_machine(&id) == -128)
+ printf("skipping test: /etc/machine-id not preset\n");
+ else {
+ assert_se(sd_id128_get_machine(&id) == 0);
+ printf("machine: %s\n", sd_id128_to_string(id, t));
+ }

assert_se(sd_id128_get_boot(&id) == 0);
printf("boot: %s\n", sd_id128_to_string(id, t));
--
1.7.8.1.362.g5d6df.dirty
Ramkumar Ramachandra
2012-12-01 03:52:10 UTC
Permalink
Please fix the spelling of "present."
The number -128, if used, should get #define'd instead of being a magic
number dropped in a bunch of places.
Thanks. Will fix in the next iteration.
It's not good to mask all errors opening the machine-id with the -128
response code. The change here seems to make all opening errors return -128.
Hm, perhaps I shouldn't handle wrong permissions as a special case and
fail the tests in that case. I'll specifically mask only an ENOENT in
the next iteration.
More importantly, what's wrong with looking for the normal [Errno 2] "No
such file or directory" return code?
There would be no way to differentiate between a ENOENT from this and
an ENOENT from callers then -- clearly, I'm not treating this as a
normal ENOENT error; I'd want a way to trap this specific error from
any caller, and handle it carefully.

Ram
David Strauss
2012-12-01 00:59:14 UTC
Permalink
A few things:

- Please fix the spelling of "present."
- The number -128, if used, should get #define'd instead of being a
magic number dropped in a bunch of places.
- It's not good to mask all errors opening the machine-id with the -128
response code. The change here seems to make all opening errors return -128.

More importantly, what's wrong with looking for the normal [Errno 2] "No
such file or directory" return code?
Post by Ramkumar Ramachandra
$ ./test-id128
random: a08ea8ed34594d4bbd953dd182ec86f9
Assertion 'sd_id128_get_machine(&id) == 0' failed at
src/test/test-id128.c:41, function main(). Aborting.
[1] 8017 abort (core dumped) ./test-id128
$ ./test-journal
Assertion 'journal_file_open("test.journal", O_RDWR|O_CREAT, 0666,
true, true, NULL, NULL, NULL, &f) == 0' failed at
src/journal/test-journal.c:46, function main(). Aborting.
[1] 8059 abort (core dumped) ./test-journal
$ ./test-journal-stream
Assertion 'journal_file_open("one.journal", O_RDWR|O_CREAT, 0666,
true, false, NULL, NULL, NULL, &one) == 0' failed at
src/journal/test-journal-stream.c:88, function main(). Aborting.
[1] 8107 abort (core dumped) ./test-journal-stream
$ ./test-journal-verify
Generating...
Assertion 'journal_file_open("test.journal", O_RDWR|O_CREAT, 0666,
true, !!verification_key, NULL, NULL, NULL, &f) == 0' failed at
src/journal/test-journal-verify.c:87, function main(). Aborting.
[1] 8154 abort (core dumped) ./test-journal-verify
This is because they call sd_id128_get_machine() which barfs if
/etc/machine-id can't be open()'ed. Treat this as a special case and
skip the dependent tests instead of failing them.
---
Oops; forgot to declate `id' in the previous iteration.
src/journal/test-journal-stream.c | 6 ++++++
src/journal/test-journal-verify.c | 6 ++++++
src/journal/test-journal.c | 6 ++++++
src/libsystemd-id128/sd-id128.c | 2 +-
src/test/test-id128.c | 8 ++++++--
5 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/src/journal/test-journal-stream.c
b/src/journal/test-journal-stream.c
index b3e816d..d07448f 100644
--- a/src/journal/test-journal-stream.c
+++ b/src/journal/test-journal-stream.c
@@ -79,6 +79,12 @@ int main(int argc, char *argv[]) {
char *z;
const void *data;
size_t l;
+ sd_id128_t id;
+
+ if (sd_id128_get_machine(&id) == -128) {
+ printf("skipping test: /etc/machine-id not preset\n");
+ return 0;
+ }
log_set_max_level(LOG_DEBUG);
diff --git a/src/journal/test-journal-verify.c
b/src/journal/test-journal-verify.c
index b667721..ee55485 100644
--- a/src/journal/test-journal-verify.c
+++ b/src/journal/test-journal-verify.c
@@ -76,6 +76,12 @@ int main(int argc, char *argv[]) {
char c[FORMAT_TIMESPAN_MAX];
struct stat st;
uint64_t p;
+ sd_id128_t id;
+
+ if (sd_id128_get_machine(&id) == -128) {
+ printf("skipping test: /etc/machine-id not preset\n");
+ return 0;
+ }
log_set_max_level(LOG_DEBUG);
diff --git a/src/journal/test-journal.c b/src/journal/test-journal.c
index f4dc52c..192b787 100644
--- a/src/journal/test-journal.c
+++ b/src/journal/test-journal.c
@@ -37,6 +37,12 @@ int main(int argc, char *argv[]) {
Object *o;
uint64_t p;
char t[] = "/tmp/journal-XXXXXX";
+ sd_id128_t id;
+
+ if (sd_id128_get_machine(&id) == -128) {
+ printf("skipping test: /etc/machine-id not preset\n");
+ return 0;
+ }
log_set_max_level(LOG_DEBUG);
diff --git a/src/libsystemd-id128/sd-id128.c
b/src/libsystemd-id128/sd-id128.c
index 4286ae7..4e5aaad 100644
--- a/src/libsystemd-id128/sd-id128.c
+++ b/src/libsystemd-id128/sd-id128.c
@@ -106,7 +106,7 @@ _public_ int sd_id128_get_machine(sd_id128_t *ret) {
fd = open("/etc/machine-id", O_RDONLY|O_CLOEXEC|O_NOCTTY);
if (fd < 0)
- return -errno;
+ return -128; /* Special return value */
k = loop_read(fd, buf, 32, false);
close_nointr_nofail(fd);
diff --git a/src/test/test-id128.c b/src/test/test-id128.c
index bfd743e..ea621f9 100644
--- a/src/test/test-id128.c
+++ b/src/test/test-id128.c
@@ -38,8 +38,12 @@ int main(int argc, char *argv[]) {
assert_se(sd_id128_from_string(t, &id2) == 0);
assert_se(sd_id128_equal(id, id2));
- assert_se(sd_id128_get_machine(&id) == 0);
- printf("machine: %s\n", sd_id128_to_string(id, t));
+ if (sd_id128_get_machine(&id) == -128)
+ printf("skipping test: /etc/machine-id not preset\n");
+ else {
+ assert_se(sd_id128_get_machine(&id) == 0);
+ printf("machine: %s\n", sd_id128_to_string(id, t));
+ }
assert_se(sd_id128_get_boot(&id) == 0);
printf("boot: %s\n", sd_id128_to_string(id, t));
--
1.7.8.1.362.g5d6df.dirty
_______________________________________________
systemd-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
--
David Strauss
| ***@davidstrauss.net
| +1 512 577 5827 [mobile]
Ramkumar Ramachandra
2012-11-30 12:46:46 UTC
Permalink
Post by Kay Sievers
It is mandatory for systemd systems. We just require it to be around,
and do not work around it.
Specifically, which package installs /etc/machine-id? Is it a build
dependency for systemd?

Ram
Ramkumar Ramachandra
2012-12-01 04:31:35 UTC
Permalink
The following tests fail if /etc/machine-id is not present:

$ ./test-id128
random: a08ea8ed34594d4bbd953dd182ec86f9
Assertion 'sd_id128_get_machine(&id) == 0' failed at
src/test/test-id128.c:41, function main(). Aborting.
[1] 8017 abort (core dumped) ./test-id128

$ ./test-journal
Assertion 'journal_file_open("test.journal", O_RDWR|O_CREAT, 0666,
true, true, NULL, NULL, NULL, &f) == 0' failed at
src/journal/test-journal.c:46, function main(). Aborting.
[1] 8059 abort (core dumped) ./test-journal

$ ./test-journal-stream
Assertion 'journal_file_open("one.journal", O_RDWR|O_CREAT, 0666,
true, false, NULL, NULL, NULL, &one) == 0' failed at
src/journal/test-journal-stream.c:88, function main(). Aborting.
[1] 8107 abort (core dumped) ./test-journal-stream

$ ./test-journal-verify
Generating...
Assertion 'journal_file_open("test.journal", O_RDWR|O_CREAT, 0666,
true, !!verification_key, NULL, NULL, NULL, &f) == 0' failed at
src/journal/test-journal-verify.c:87, function main(). Aborting.
[1] 8154 abort (core dumped) ./test-journal-verify

This is because they call sd_id128_get_machine() which barfs if
/etc/machine-id can't be open()'ed. Treat ENOENT as a special case
and skip the dependent tests instead of failing them.
---
src/journal/test-journal-stream.c | 6 ++++++
src/journal/test-journal-verify.c | 6 ++++++
src/journal/test-journal.c | 6 ++++++
src/libsystemd-id128/sd-id128.c | 3 ++-
src/systemd/sd-id128.h | 2 ++
src/test/test-id128.c | 8 ++++++--
6 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/src/journal/test-journal-stream.c b/src/journal/test-journal-stream.c
index b3e816d..05eedfd 100644
--- a/src/journal/test-journal-stream.c
+++ b/src/journal/test-journal-stream.c
@@ -79,6 +79,12 @@ int main(int argc, char *argv[]) {
char *z;
const void *data;
size_t l;
+ sd_id128_t id;
+
+ if (sd_id128_get_machine(&id) == MACHINE_ID_MISSING) {
+ printf("skipping test: /etc/machine-id not present\n");
+ return 0;
+ }

log_set_max_level(LOG_DEBUG);

diff --git a/src/journal/test-journal-verify.c b/src/journal/test-journal-verify.c
index b667721..c35a96b 100644
--- a/src/journal/test-journal-verify.c
+++ b/src/journal/test-journal-verify.c
@@ -76,6 +76,12 @@ int main(int argc, char *argv[]) {
char c[FORMAT_TIMESPAN_MAX];
struct stat st;
uint64_t p;
+ sd_id128_t id;
+
+ if (sd_id128_get_machine(&id) == MACHINE_ID_MISSING) {
+ printf("skipping test: /etc/machine-id not present\n");
+ return 0;
+ }

log_set_max_level(LOG_DEBUG);

diff --git a/src/journal/test-journal.c b/src/journal/test-journal.c
index f4dc52c..27b1cc3 100644
--- a/src/journal/test-journal.c
+++ b/src/journal/test-journal.c
@@ -37,6 +37,12 @@ int main(int argc, char *argv[]) {
Object *o;
uint64_t p;
char t[] = "/tmp/journal-XXXXXX";
+ sd_id128_t id;
+
+ if (sd_id128_get_machine(&id) == MACHINE_ID_MISSING) {
+ printf("skipping test: /etc/machine-id not present\n");
+ return 0;
+ }

log_set_max_level(LOG_DEBUG);

diff --git a/src/libsystemd-id128/sd-id128.c b/src/libsystemd-id128/sd-id128.c
index 4286ae7..6f92cf1 100644
--- a/src/libsystemd-id128/sd-id128.c
+++ b/src/libsystemd-id128/sd-id128.c
@@ -106,7 +106,8 @@ _public_ int sd_id128_get_machine(sd_id128_t *ret) {

fd = open("/etc/machine-id", O_RDONLY|O_CLOEXEC|O_NOCTTY);
if (fd < 0)
- return -errno;
+ /* Special return value if machine-id is missing */
+ return errno == ENOENT ? MACHINE_ID_MISSING : -errno;

k = loop_read(fd, buf, 32, false);
close_nointr_nofail(fd);
diff --git a/src/systemd/sd-id128.h b/src/systemd/sd-id128.h
index 79bb8b3..f4a8914 100644
--- a/src/systemd/sd-id128.h
+++ b/src/systemd/sd-id128.h
@@ -102,6 +102,8 @@ static inline int sd_id128_equal(sd_id128_t a, sd_id128_t b) {

#define SD_ID128_NULL ((sd_id128_t) { .qwords = { 0, 0 }})

+#define MACHINE_ID_MISSING -128
+
#ifdef __cplusplus
}
#endif
diff --git a/src/test/test-id128.c b/src/test/test-id128.c
index bfd743e..f03204d 100644
--- a/src/test/test-id128.c
+++ b/src/test/test-id128.c
@@ -38,8 +38,12 @@ int main(int argc, char *argv[]) {
assert_se(sd_id128_from_string(t, &id2) == 0);
assert_se(sd_id128_equal(id, id2));

- assert_se(sd_id128_get_machine(&id) == 0);
- printf("machine: %s\n", sd_id128_to_string(id, t));
+ if (sd_id128_get_machine(&id) == MACHINE_ID_MISSING)
+ printf("skipping test: /etc/machine-id not present\n");
+ else {
+ assert_se(sd_id128_get_machine(&id) == 0);
+ printf("machine: %s\n", sd_id128_to_string(id, t));
+ }

assert_se(sd_id128_get_boot(&id) == 0);
printf("boot: %s\n", sd_id128_to_string(id, t));
--
1.7.8.1.362.g5d6df.dirty
Zbigniew Jędrzejewski-Szmek
2012-12-01 15:48:04 UTC
Permalink
Post by Ramkumar Ramachandra
$ ./test-id128
random: a08ea8ed34594d4bbd953dd182ec86f9
Assertion 'sd_id128_get_machine(&id) == 0' failed at
src/test/test-id128.c:41, function main(). Aborting.
[1] 8017 abort (core dumped) ./test-id128
$ ./test-journal
Assertion 'journal_file_open("test.journal", O_RDWR|O_CREAT, 0666,
true, true, NULL, NULL, NULL, &f) == 0' failed at
src/journal/test-journal.c:46, function main(). Aborting.
[1] 8059 abort (core dumped) ./test-journal
$ ./test-journal-stream
Assertion 'journal_file_open("one.journal", O_RDWR|O_CREAT, 0666,
true, false, NULL, NULL, NULL, &one) == 0' failed at
src/journal/test-journal-stream.c:88, function main(). Aborting.
[1] 8107 abort (core dumped) ./test-journal-stream
$ ./test-journal-verify
Generating...
Assertion 'journal_file_open("test.journal", O_RDWR|O_CREAT, 0666,
true, !!verification_key, NULL, NULL, NULL, &f) == 0' failed at
src/journal/test-journal-verify.c:87, function main(). Aborting.
[1] 8154 abort (core dumped) ./test-journal-verify
This is because they call sd_id128_get_machine() which barfs if
/etc/machine-id can't be open()'ed. Treat ENOENT as a special case
and skip the dependent tests instead of failing them.
Hi,

I started munging your patch to apply it, but on second thought it is
totally the wrong direction to take. The purpose of tests is to check
if systemd will function after installation. And as mentioned
elsewhere in the thread, journald will break badly, and thus systemd
will not function as expected. So it is much better to have the tests
fail as they do now, then to paper over a missing file. So I think that
the patch should
(a) skip _some_ of the tests to reduce noise,
(b) fail at least one test to tell the user that /etc/machine-id is missing.

I would be nice to
(c) using automake skip (below) to do that,

Automake test skipping:
+ #define EXIT_AUTOMAKE_SKIP 77

+ sd_id128_t id;
+
+ if (sd_id128_get_machine(&id) == -ENOENT) {
+ printf("skipping test: /etc/machine-id not present\n");
+ return EXIT_AUTOMAKE_SKIP;
+ }

SKIP: test-journal-verify
PASS: test-mmap-cache
======================
All 17 tests passed
(4 tests were not run)
======================

Zbyszek
David Strauss
2012-12-02 04:28:43 UTC
Permalink
Would it be possible, for testing purposes, to generate a machine ID
on the fly if one is not present on disk?

On Sat, Dec 1, 2012 at 7:48 AM, Zbigniew Jędrzejewski-Szmek
Post by Zbigniew Jędrzejewski-Szmek
Post by Ramkumar Ramachandra
$ ./test-id128
random: a08ea8ed34594d4bbd953dd182ec86f9
Assertion 'sd_id128_get_machine(&id) == 0' failed at
src/test/test-id128.c:41, function main(). Aborting.
[1] 8017 abort (core dumped) ./test-id128
$ ./test-journal
Assertion 'journal_file_open("test.journal", O_RDWR|O_CREAT, 0666,
true, true, NULL, NULL, NULL, &f) == 0' failed at
src/journal/test-journal.c:46, function main(). Aborting.
[1] 8059 abort (core dumped) ./test-journal
$ ./test-journal-stream
Assertion 'journal_file_open("one.journal", O_RDWR|O_CREAT, 0666,
true, false, NULL, NULL, NULL, &one) == 0' failed at
src/journal/test-journal-stream.c:88, function main(). Aborting.
[1] 8107 abort (core dumped) ./test-journal-stream
$ ./test-journal-verify
Generating...
Assertion 'journal_file_open("test.journal", O_RDWR|O_CREAT, 0666,
true, !!verification_key, NULL, NULL, NULL, &f) == 0' failed at
src/journal/test-journal-verify.c:87, function main(). Aborting.
[1] 8154 abort (core dumped) ./test-journal-verify
This is because they call sd_id128_get_machine() which barfs if
/etc/machine-id can't be open()'ed. Treat ENOENT as a special case
and skip the dependent tests instead of failing them.
Hi,
I started munging your patch to apply it, but on second thought it is
totally the wrong direction to take. The purpose of tests is to check
if systemd will function after installation. And as mentioned
elsewhere in the thread, journald will break badly, and thus systemd
will not function as expected. So it is much better to have the tests
fail as they do now, then to paper over a missing file. So I think that
the patch should
(a) skip _some_ of the tests to reduce noise,
(b) fail at least one test to tell the user that /etc/machine-id is missing.
I would be nice to
(c) using automake skip (below) to do that,
+ #define EXIT_AUTOMAKE_SKIP 77
+ sd_id128_t id;
+
+ if (sd_id128_get_machine(&id) == -ENOENT) {
+ printf("skipping test: /etc/machine-id not present\n");
+ return EXIT_AUTOMAKE_SKIP;
+ }
SKIP: test-journal-verify
PASS: test-mmap-cache
======================
All 17 tests passed
(4 tests were not run)
======================
Zbyszek
_______________________________________________
systemd-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
--
David Strauss
| ***@davidstrauss.net
| +1 512 577 5827 [mobile]
Zbigniew Jędrzejewski-Szmek
2012-12-02 10:34:27 UTC
Permalink
Post by David Strauss
Would it be possible, for testing purposes, to generate a machine ID
on the fly if one is not present on disk?
For systemd to boot properly when the root fs is readonly, either
/etc/machine-id with some value, or an empty /etc/machine-id must
be present. So it would make sense to check for an empty /etc/machine-id
and then generate a value, but the case where there's no /etc/machine-id
should fail, IMHO.

Zbyszek
Post by David Strauss
On Sat, Dec 1, 2012 at 7:48 AM, Zbigniew Jędrzejewski-Szmek
Post by Zbigniew Jędrzejewski-Szmek
Post by Ramkumar Ramachandra
$ ./test-id128
random: a08ea8ed34594d4bbd953dd182ec86f9
Assertion 'sd_id128_get_machine(&id) == 0' failed at
src/test/test-id128.c:41, function main(). Aborting.
[1] 8017 abort (core dumped) ./test-id128
$ ./test-journal
Assertion 'journal_file_open("test.journal", O_RDWR|O_CREAT, 0666,
true, true, NULL, NULL, NULL, &f) == 0' failed at
src/journal/test-journal.c:46, function main(). Aborting.
[1] 8059 abort (core dumped) ./test-journal
$ ./test-journal-stream
Assertion 'journal_file_open("one.journal", O_RDWR|O_CREAT, 0666,
true, false, NULL, NULL, NULL, &one) == 0' failed at
src/journal/test-journal-stream.c:88, function main(). Aborting.
[1] 8107 abort (core dumped) ./test-journal-stream
$ ./test-journal-verify
Generating...
Assertion 'journal_file_open("test.journal", O_RDWR|O_CREAT, 0666,
true, !!verification_key, NULL, NULL, NULL, &f) == 0' failed at
src/journal/test-journal-verify.c:87, function main(). Aborting.
[1] 8154 abort (core dumped) ./test-journal-verify
This is because they call sd_id128_get_machine() which barfs if
/etc/machine-id can't be open()'ed. Treat ENOENT as a special case
and skip the dependent tests instead of failing them.
Hi,
I started munging your patch to apply it, but on second thought it is
totally the wrong direction to take. The purpose of tests is to check
if systemd will function after installation. And as mentioned
elsewhere in the thread, journald will break badly, and thus systemd
will not function as expected. So it is much better to have the tests
fail as they do now, then to paper over a missing file. So I think that
the patch should
(a) skip _some_ of the tests to reduce noise,
(b) fail at least one test to tell the user that /etc/machine-id is missing.
I would be nice to
(c) using automake skip (below) to do that,
+ #define EXIT_AUTOMAKE_SKIP 77
+ sd_id128_t id;
+
+ if (sd_id128_get_machine(&id) == -ENOENT) {
+ printf("skipping test: /etc/machine-id not present\n");
+ return EXIT_AUTOMAKE_SKIP;
+ }
SKIP: test-journal-verify
PASS: test-mmap-cache
======================
All 17 tests passed
(4 tests were not run)
======================
Zbyszek
_______________________________________________
systemd-devel mailing list
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
--
David Strauss
| +1 512 577 5827 [mobile]
Ramkumar Ramachandra
2012-12-02 11:52:00 UTC
Permalink
Post by David Strauss
Would it be possible, for testing purposes, to generate a machine ID
on the fly if one is not present on disk?
Hard, as "/etc/machine-id" is hardcoded in
src/libsystemd-id128/sd-id128.c. I don't think it's worth the effort.

Ram
Ramkumar Ramachandra
2012-12-02 11:59:52 UTC
Permalink
Post by Zbigniew Jędrzejewski-Szmek
I started munging your patch to apply it, but on second thought it is
totally the wrong direction to take. The purpose of tests is to check
if systemd will function after installation. And as mentioned
elsewhere in the thread, journald will break badly, and thus systemd
will not function as expected. So it is much better to have the tests
fail as they do now, then to paper over a missing file. So I think that
the patch should
(a) skip _some_ of the tests to reduce noise,
(b) fail at least one test to tell the user that /etc/machine-id is missing.
Hm, you have a point. On Debian, "/etc/machine-id" is created when
the systemd package is installed -- I'm not sure if it's in the source
code or if it's a custom Debian patch. Either way, there should be
one test checking the pre-existence of files (like /etc/mtab), to
assert whether the OS is systemd-ready -- and this test should fail.
Post by Zbigniew Jędrzejewski-Szmek
I would be nice to
(c) using automake skip (below) to do that,
This is a hack. Ideally, I'd like real testsuite (not a bunch of
asserts thrown around randomly) with a way to execute tests based on
pre-requisites, like the one in git.git. I'll try to write one for
systemd in the evening, and see how that goes.

Ram
Michael Biebl
2012-12-02 12:56:19 UTC
Permalink
Post by Ramkumar Ramachandra
Hm, you have a point. On Debian, "/etc/machine-id" is created when
the systemd package is installed -- I'm not sure if it's in the source
code or if it's a custom Debian patch.
We run systemd-machine-id-setup in postinst.

Michael

[1] http://anonscm.debian.org/gitweb/?p=pkg-systemd/systemd.git;a=blob;f=debian/systemd.postinst;h=366f7f545cfc002160f9821c00777dc442402255;hb=refs/heads/debian#l62
--
Why is it that all of the instruments seeking intelligent life in the
universe are pointed away from Earth?
Ramkumar Ramachandra
2012-12-02 14:02:04 UTC
Permalink
Hi Michael,
Post by Michael Biebl
Post by Ramkumar Ramachandra
Hm, you have a point. On Debian, "/etc/machine-id" is created when
the systemd package is installed -- I'm not sure if it's in the source
code or if it's a custom Debian patch.
We run systemd-machine-id-setup in postinst.
Ah, that explains it. Again, why should any of the tests packaged
with systemd fail if the distribution does this?

Ram

Continue reading on narkive:
Loading...