[systemd-devel] [RFC][PATCH] sd-bus: split ref-counting of queues from ref-counting of the rest of the sd-bus object
Introduce a new ref-count, n_ref_queues, which only protects the {r,w}queue of
a bus, and introduce bus_{un,}ref(), which are only available internally, and
which do not protect these queues.
Make sure that sd_bus_message object do not call sd_bus_ref(), but only the
internal bus_ref(). This is ok as the {r,w}queues should never be accessed via
the message object (as doing so would anyway not be thread-safe).
When the refcount on the queues reaches zero (even thought the refcount on the
bus itself may not, due to references held by messages in the queues), the
queues are flushed and their messages unref'ed. This avoids problems due to
mutual references between busses and their queued messages. In particular we
don't get an sd_bus_unref() - sd_bus_message_unref() - sd_bus_unref()
call-chain.
Moreover, we can now enforce that sd_bus_{un,}ref() is only ever called from
the same thread as created the bus (whereas bus_unref() may be called from a
different thread, as part of unref'ing a message being handled in a worker
thread).
---
src/libsystemd/sd-bus/bus-internal.h | 10
src/libsystemd/sd-bus/bus-message.c | 6 +--
src/libsystemd/sd-bus/sd-bus.c | 93 +++-
3 files changed, 51 insertions(+), 58 deletions(-)
diff --git a/src/libsystemd/sd-bus/bus-internal.h
b/src/libsystemd/sd-bus/bus-internal.h
index 3dceb8a..c9b40af 100644
--- a/src/libsystemd/sd-bus/bus-internal.h
+++ b/src/libsystemd/sd-bus/bus-internal.h
@@ -145,6 +145,13 @@ struct sd_bus {
same time. */
RefCount n_ref;
+/* The {r,w}queue may only be accessed from the original
+ thread, so no need for atomic ref counting. The queues
+ are ref-counted separately from the sd_bus object to
+ avoid problems caused by mutual references between
+ busses and their queued messages */
+unsigned n_ref_queues;
+
enum bus_state state;
int input_fd, output_fd;
int message_version;
@@ -340,3 +347,6 @@ int bus_set_address_system(sd_bus *bus);
int bus_set_address_user(sd_bus *bus);
int bus_set_address_system_remote(sd_bus *b, const char *host);
int bus_set_address_system_container(sd_bus *b, const char *machine);
+
+sd_bus *bus_ref(sd_bus* bus);
+sd_bus *bus_unref(sd_bus *bus);
diff --git a/src/libsystemd/sd-bus/bus-message.c
b/src/libsystemd/sd-bus/bus-message.c
index 4fcc693..e33c08a 100644
--- a/src/libsystemd/sd-bus/bus-message.c
+++ b/src/libsystemd/sd-bus/bus-message.c
@@ -137,7 +137,7 @@ static void message_free(sd_bus_message *m) {
}
if (m-bus)
-sd_bus_unref(m-bus);
+bus_unref(m-bus);
if (m-free_fds) {
close_many(m-fds, m-n_fds);
@@ -427,7 +427,7 @@ int bus_message_from_header(
}
if (bus)
-m-bus = sd_bus_ref(bus);
+m-bus = bus_ref(bus);
*ret = m;
return 0;
@@ -502,7 +502,7 @@ static sd_bus_message *message_new(sd_bus *bus, uint8_t
type) {
m-root_container.need_offsets = BUS_MESSAGE_IS_GVARIANT(m);
if (bus)
-m-bus = sd_bus_ref(bus);
+m-bus = bus_ref(bus);
return m;
}
diff --git a/src/libsystemd/sd-bus/sd-bus.c b/src/libsystemd/sd-bus/sd-bus.c
index bbe61a6..b09326d 100644
--- a/src/libsystemd/sd-bus/sd-bus.c
+++ b/src/libsystemd/sd-bus/sd-bus.c
@@ -111,12 +111,6 @@ static void bus_node_destroy(sd_bus *b, struct node *n) {
static void bus_reset_queues(sd_bus *b) {
assert(b);
-/* NOTE: We _must_ decrement b-Xqueue_size before calling
- * sd_bus_message_unref() for _each_ message. Otherwise the
- * self-reference checks in sd_bus_unref() will fire for each message.
- * We would thus recurse into sd_bus_message_unref() and trigger the
- * assert(m-n_ref 0) */
-
while (b-rqueue_size 0)
sd_bus_message_unref(b-rqueue[--b-rqueue_size]);
@@ -203,6 +197,7 @@ _public_ int sd_bus_new(sd_bus **ret) {
return -ENOMEM;
r-n_ref = REFCNT_INIT;
+r-n_ref_queues = 1;
r-input_fd = r-output_fd = -1;
r-message_version = 1;
r-creds_mask |=
SD_BUS_CREDS_WELL_KNOWN_NAMES|SD_BUS_CREDS_UNIQUE_NAME;
@@ -1380,72 +1375,60 @@ static void bus_enter_closing(sd_bus *bus) {
bus-state = BUS_CLOSING;
}
-_public_ sd_bus *sd_bus_ref(sd_bus *bus) {
-assert_return(bus, NULL);
+/* Unlike sd_bus_{un,}ref(), bus_{un,}ref() do not protect the
+ {r,w}queue. On the other hand, these methods are thread-safe.
+ */
+sd_bus *bus_ref(sd_bus* bus) {
+assert(bus);
assert_se(REFCNT_INC(bus-n_ref) = 2);
return bus;
}
-_public_ sd_bus *sd_bus_unref(sd_bus *bus) {
+sd_bus *bus_unref(sd_bus *bus) {
unsigned i;
if (!bus)
return NULL;
-/* TODO/FIXME: It's naive to think REFCNT_GET() is thread-safe in any
-
Re: [systemd-devel] [PATCH 1/1] sd-rtnl: add support for tunnel attributes
On Sun, Mar 23, 2014 at 4:14 PM, Susant Sahani sus...@redhat.com wrote:
Added support for tunneling netlink attrributes (ipip, gre, sit).
These works with kernel module ipip, gre and sit . The test cases are
commented out because they requirs super user privileges to run and
respective kernel modules as well.
I guess this relies on the container parsing patch you posted? At
least the current test-code does not seem to work without it.
Otherwise this patch looks good though, but I have some comments on
the container parsing stuff, so let's sort that out before merging
these.
Cheers,
Tom
---
src/libsystemd/sd-rtnl/rtnl-message.c | 37 +---
src/libsystemd/sd-rtnl/test-rtnl.c| 64
+++
2 files changed, 97 insertions(+), 4 deletions(-)
diff --git a/src/libsystemd/sd-rtnl/rtnl-message.c
b/src/libsystemd/sd-rtnl/rtnl-message.c
index e243c7b..6c7bda8 100644
--- a/src/libsystemd/sd-rtnl/rtnl-message.c
+++ b/src/libsystemd/sd-rtnl/rtnl-message.c
@@ -24,6 +24,9 @@
#include stdbool.h
#include unistd.h
#include linux/veth.h
+#include linux/if.h
+#include linux/ip.h
+#include linux/if_tunnel.h
#include util.h
#include refcnt.h
@@ -458,6 +461,12 @@ int sd_rtnl_message_append_u8(sd_rtnl_message *m,
unsigned short type, uint8_t d
case IFLA_CARRIER:
case IFLA_OPERSTATE:
case IFLA_LINKMODE:
+case IFLA_IPTUN_TTL:
+case IFLA_IPTUN_TOS:
+case IFLA_IPTUN_PROTO:
+case IFLA_IPTUN_PMTUDISC:
+case IFLA_IPTUN_ENCAP_LIMIT:
+case IFLA_GRE_TTL:
break;
default:
return -ENOTSUP;
@@ -495,12 +504,22 @@ int sd_rtnl_message_append_u16(sd_rtnl_message *m,
unsigned short type, uint16_t
case RTM_DELLINK:
if (m-n_containers == 2
GET_CONTAINER(m, 0)-rta_type == IFLA_LINKINFO
-GET_CONTAINER(m, 1)-rta_type == IFLA_INFO_DATA
-type == IFLA_VLAN_ID)
-break;
-else
+GET_CONTAINER(m, 1)-rta_type == IFLA_INFO_DATA)
{
+switch (type) {
+ case IFLA_VLAN_ID:
+ case IFLA_IPTUN_FLAGS:
+ case IFLA_GRE_IFLAGS:
+ case IFLA_GRE_OFLAGS:
+ case IFLA_IPTUN_6RD_PREFIXLEN:
+ case IFLA_IPTUN_6RD_RELAY_PREFIXLEN:
+ break;
+ default:
+return -ENOTSUP;
+}
+} else
return -ENOTSUP;
+break;
default:
return -ENOTSUP;
}
@@ -541,7 +560,12 @@ int sd_rtnl_message_append_u32(sd_rtnl_message *m,
unsigned short type, uint32_t
case IFLA_PROMISCUITY:
case IFLA_NUM_TX_QUEUES:
case IFLA_NUM_RX_QUEUES:
+case IFLA_IPTUN_LOCAL:
+case IFLA_IPTUN_REMOTE:
case IFLA_MACVLAN_MODE:
+case IFLA_IPTUN_FLAGS:
+case IFLA_IPTUN_FLOWINFO:
+case IFLA_GRE_FLOWINFO:
break;
default:
return -ENOTSUP;
@@ -596,6 +620,8 @@ int sd_rtnl_message_append_in_addr(sd_rtnl_message *m,
unsigned short type, cons
case IFA_LOCAL:
case IFA_BROADCAST:
case IFA_ANYCAST:
+case IFLA_GRE_LOCAL:
+case IFLA_GRE_REMOTE:
ifa = NLMSG_DATA(m-hdr);
if (ifa-ifa_family != AF_INET)
@@ -658,6 +684,9 @@ int sd_rtnl_message_append_in6_addr(sd_rtnl_message *m,
unsigned short type, con
case IFA_LOCAL:
case IFA_BROADCAST:
case IFA_ANYCAST:
+case IFLA_GRE_LOCAL:
+
Re: [systemd-devel] [PATCH] time-util: accept epoch timetamps prefixed with @
On Sun, Mar 23, 2014 at 11:06:47AM -0400, Dave Reisner wrote:
Also adds a few tests for the absolute cases of parse_timestamp.
Yeah, that looks useful.
You don't test negative values. Maybe you could an example with a negative
value to the documentation and tests?
Zbyszek
Suggested by: Mantas Mikulėnas graw...@gmail.com
---
src/shared/time-util.c | 10 ++
src/test/test-time.c | 21 +
2 files changed, 31 insertions(+)
diff --git a/src/shared/time-util.c b/src/shared/time-util.c
index faa3418..fe43404 100644
--- a/src/shared/time-util.c
+++ b/src/shared/time-util.c
@@ -432,6 +432,7 @@ int parse_timestamp(const char *t, usec_t *usec) {
* tomorrow (time is set to 00:00:00)
* +5min
* -5days
+ * @1395584178 (seconds from the epoch)
*
*/
@@ -473,7 +474,16 @@ int parse_timestamp(const char *t, usec_t *usec) {
return r;
goto finish;
+} else if (t[0] == '@') {
+time_t epoch;
+r = safe_atoli(t+1, epoch);
+if (r 0)
+return r;
+
+assert_se(localtime_r(epoch, tm));
+
+goto finish;
} else if (endswith(t, ago)) {
_cleanup_free_ char *z;
diff --git a/src/test/test-time.c b/src/test/test-time.c
index 36a3304..396111d 100644
--- a/src/test/test-time.c
+++ b/src/test/test-time.c
@@ -126,9 +126,30 @@ static void test_format_timespan(usec_t accuracy) {
test_format_timespan_one(9*USEC_PER_YEAR/5 - 23, accuracy);
}
+static void test_parse_timestamp_one(const char *timestamp, usec_t expected)
{
+usec_t result = 0;
+
+parse_timestamp(timestamp, result);
+printf(timestamp=%s, result=% PRIu64 \n, timestamp, result);
+
+assert_se(expected == result);
+}
+
+static void test_parse_timestamp(void) {
+test_parse_timestamp_one(2012-09-22 16:34:22, 134834606200);
+test_parse_timestamp_one(2012-09-22 16:34, 134834604000);
+test_parse_timestamp_one(2012-09-22, 13482864);
+test_parse_timestamp_one(2012-09, 0);
+test_parse_timestamp_one(@1234567890, 123456789000);
+test_parse_timestamp_one(@1234567890 sec, 0);
+test_parse_timestamp_one(1234567890 sec, 0);
+test_parse_timestamp_one(1234567890, 0);
+}
+
int main(int argc, char *argv[]) {
test_parse_sec();
test_parse_nsec();
+test_parse_timestamp();
test_format_timespan(1);
test_format_timespan(USEC_PER_MSEC);
test_format_timespan(USEC_PER_SEC);
--
1.9.1
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] time-util: accept epoch timetamps prefixed with @
On Sun, Mar 23, 2014 at 05:27:08PM +0100, Zbigniew Jędrzejewski-Szmek wrote:
On Sun, Mar 23, 2014 at 11:06:47AM -0400, Dave Reisner wrote:
Also adds a few tests for the absolute cases of parse_timestamp.
Yeah, that looks useful.
You don't test negative values. Maybe you could an example with a negative
value to the documentation and tests?
Negative epoch values? What would this represent?
Zbyszek
Suggested by: Mantas Mikulėnas graw...@gmail.com
---
src/shared/time-util.c | 10 ++
src/test/test-time.c | 21 +
2 files changed, 31 insertions(+)
diff --git a/src/shared/time-util.c b/src/shared/time-util.c
index faa3418..fe43404 100644
--- a/src/shared/time-util.c
+++ b/src/shared/time-util.c
@@ -432,6 +432,7 @@ int parse_timestamp(const char *t, usec_t *usec) {
* tomorrow (time is set to 00:00:00)
* +5min
* -5days
+ * @1395584178 (seconds from the epoch)
*
*/
@@ -473,7 +474,16 @@ int parse_timestamp(const char *t, usec_t *usec) {
return r;
goto finish;
+} else if (t[0] == '@') {
+time_t epoch;
+r = safe_atoli(t+1, epoch);
+if (r 0)
+return r;
+
+assert_se(localtime_r(epoch, tm));
+
+goto finish;
} else if (endswith(t, ago)) {
_cleanup_free_ char *z;
diff --git a/src/test/test-time.c b/src/test/test-time.c
index 36a3304..396111d 100644
--- a/src/test/test-time.c
+++ b/src/test/test-time.c
@@ -126,9 +126,30 @@ static void test_format_timespan(usec_t accuracy) {
test_format_timespan_one(9*USEC_PER_YEAR/5 - 23, accuracy);
}
+static void test_parse_timestamp_one(const char *timestamp, usec_t
expected) {
+usec_t result = 0;
+
+parse_timestamp(timestamp, result);
+printf(timestamp=%s, result=% PRIu64 \n, timestamp, result);
+
+assert_se(expected == result);
+}
+
+static void test_parse_timestamp(void) {
+test_parse_timestamp_one(2012-09-22 16:34:22, 134834606200);
+test_parse_timestamp_one(2012-09-22 16:34, 134834604000);
+test_parse_timestamp_one(2012-09-22, 13482864);
+test_parse_timestamp_one(2012-09, 0);
+test_parse_timestamp_one(@1234567890, 123456789000);
+test_parse_timestamp_one(@1234567890 sec, 0);
+test_parse_timestamp_one(1234567890 sec, 0);
+test_parse_timestamp_one(1234567890, 0);
+}
+
int main(int argc, char *argv[]) {
test_parse_sec();
test_parse_nsec();
+test_parse_timestamp();
test_format_timespan(1);
test_format_timespan(USEC_PER_MSEC);
test_format_timespan(USEC_PER_SEC);
--
1.9.1
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 1/1] sd-rtnl: add support for tunnel attributes
On 03/23/2014 09:34 PM, Tom Gundersen wrote: On Sun, Mar 23, 2014 at 4:14 PM, Susant Sahani sus...@redhat.com wrote: Added support for tunneling netlink attrributes (ipip, gre, sit). These works with kernel module ipip, gre and sit . The test cases are commented out because they requirs super user privileges to run and respective kernel modules as well. I guess this relies on the container parsing patch you posted? At least the current test-code does not seem to work without it. Otherwise this patch looks good though, but I have some comments on the container parsing stuff, so let's sort that out before merging Not really . The parsing different than the forming of NL messages. On my test machine: ~~~ 11: eth0@NONE: POINTOPOINT,NOARP mtu 1234 qdisc noop state DOWN mode DEFAULT group default link/ipip 192.168.21.1 peer 192.168.21.2 12: eth1: POINTOPOINT,NOARP mtu 1234 qdisc noop state DOWN mode DEFAULT group default link/sit 192.168.21.3 peer 192.168.21.4 ~~~ Thanks ! Susant ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 1/1] sd-rtnl: add support for tunnel attributes
On 03/23/2014 10:00 PM, Zbigniew Jędrzejewski-Szmek wrote: On Sun, Mar 23, 2014 at 08:44:09PM +0530, Susant Sahani wrote: Added support for tunneling netlink attrributes (ipip, gre, sit). These works with kernel module ipip, gre and sit . The test cases are commented out because they requirs super user privileges to run and respective kernel modules as well. This isn't a matter to provide those tests. Split out the tests into a separate file if necessary (i.e. if other tests in the same file do not require privileges) and add it to $(manual_tests) in Makefile.am. If some module cannot be loaded, return EXIT_TEST_SKIP instead of an error. Thanks for the tip let me try on that Zbyszek Thanks ! Susant ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] time-util: accept epoch timetamps prefixed with @
]] Dave Reisner On Sun, Mar 23, 2014 at 05:27:08PM +0100, Zbigniew Jędrzejewski-Szmek wrote: On Sun, Mar 23, 2014 at 11:06:47AM -0400, Dave Reisner wrote: Also adds a few tests for the absolute cases of parse_timestamp. Yeah, that looks useful. You don't test negative values. Maybe you could an example with a negative value to the documentation and tests? Negative epoch values? What would this represent? Time before the epoch? $ LANG=C date -d @-1 Wed Dec 31 22:13:20 CET 1969 So date(1) already understands this. -- Tollef Fog Heen UNIX is user friendly, it's just picky about who its friends are ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] time-util: accept epoch timetamps prefixed with @
On Sun, Mar 23, 2014 at 06:30:02PM +0100, Tollef Fog Heen wrote: ]] Dave Reisner On Sun, Mar 23, 2014 at 05:27:08PM +0100, Zbigniew Jędrzejewski-Szmek wrote: On Sun, Mar 23, 2014 at 11:06:47AM -0400, Dave Reisner wrote: Also adds a few tests for the absolute cases of parse_timestamp. Yeah, that looks useful. You don't test negative values. Maybe you could an example with a negative value to the documentation and tests? Negative epoch values? What would this represent? Time before the epoch? $ LANG=C date -d @-1 Wed Dec 31 22:13:20 CET 1969 So date(1) already understands this. Uggh, do we really need to document this? I don't see why/where it would actually be useful. FWIW, parse_timestamp sort of already handles negative values with the exception of anything that results in a timestamp of -1 -- mktime can't distinguish between a real timestamp of -1 and an invalid struct tm which it can't convert. $ journalctl --since='1969-12-31 18:59:59' Failed to parse timestamp: 1969-12-31 18:59:59 But then there's also some inconsistent behavior which parse_timestamp already exposes when dealing with absolutes: $ journalctl --since='1969-12-31 18:59:58' -- Logs begin at Fri 2013-11-15 18:11:44 EST, end at Sun 2014-03-23 14:01:01 EDT. -- EOF $ journalctl --since='-100 years' -- Logs begin at Fri 2013-11-15 18:11:44 EST, end at Sun 2014-03-23 14:01:01 EDT. -- logs follow I don't think this is going to work out so well when the return type involed (usec_t) is unsigned... -- Tollef Fog Heen UNIX is user friendly, it's just picky about who its friends are ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] time-util: accept epoch timetamps prefixed with @
On Sun, Mar 23, 2014 at 02:27:04PM -0400, Dave Reisner wrote: On Sun, Mar 23, 2014 at 06:30:02PM +0100, Tollef Fog Heen wrote: ]] Dave Reisner On Sun, Mar 23, 2014 at 05:27:08PM +0100, Zbigniew Jędrzejewski-Szmek wrote: On Sun, Mar 23, 2014 at 11:06:47AM -0400, Dave Reisner wrote: Also adds a few tests for the absolute cases of parse_timestamp. Yeah, that looks useful. You don't test negative values. Maybe you could an example with a negative value to the documentation and tests? Negative epoch values? What would this represent? Time before the epoch? $ LANG=C date -d @-1 Wed Dec 31 22:13:20 CET 1969 So date(1) already understands this. Uggh, do we really need to document this? I don't see why/where it would actually be useful. Well, you don't need to document this explictly, i.e. there's no need to talk about negative values, since they're not really useful, but I think it should be mentioned that the '@' is followed by a signed integer. FWIW, parse_timestamp sort of already handles negative values with the exception of anything that results in a timestamp of -1 -- mktime can't distinguish between a real timestamp of -1 and an invalid struct tm which it can't convert. $ journalctl --since='1969-12-31 18:59:59' Failed to parse timestamp: 1969-12-31 18:59:59 But then there's also some inconsistent behavior which parse_timestamp already exposes when dealing with absolutes: $ journalctl --since='1969-12-31 18:59:58' -- Logs begin at Fri 2013-11-15 18:11:44 EST, end at Sun 2014-03-23 14:01:01 EDT. -- EOF $ journalctl --since='-100 years' -- Logs begin at Fri 2013-11-15 18:11:44 EST, end at Sun 2014-03-23 14:01:01 EDT. -- logs follow I don't think this is going to work out so well when the return type involed (usec_t) is unsigned... Are you sure? In your patch you use safe_atoi (not safe_atou). And on my machine, time_t is defined as __time_t, which is __TIME_T_TYPE, __SYSCALL_SLONG_TYPE, which looks signed to me. Actually, it seems that this safe_atoi should be replaced by assert_cc(sizeof(time_t)==sizeof(long)) and safe_atoli. Zbyszek ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] time-util: accept epoch timetamps prefixed with @
On Sun, Mar 23, 2014 at 10:04:00PM +0100, Zbigniew Jędrzejewski-Szmek wrote: On Sun, Mar 23, 2014 at 02:27:04PM -0400, Dave Reisner wrote: On Sun, Mar 23, 2014 at 06:30:02PM +0100, Tollef Fog Heen wrote: ]] Dave Reisner On Sun, Mar 23, 2014 at 05:27:08PM +0100, Zbigniew Jędrzejewski-Szmek wrote: On Sun, Mar 23, 2014 at 11:06:47AM -0400, Dave Reisner wrote: Also adds a few tests for the absolute cases of parse_timestamp. Yeah, that looks useful. You don't test negative values. Maybe you could an example with a negative value to the documentation and tests? Negative epoch values? What would this represent? Time before the epoch? $ LANG=C date -d @-1 Wed Dec 31 22:13:20 CET 1969 So date(1) already understands this. Uggh, do we really need to document this? I don't see why/where it would actually be useful. Well, you don't need to document this explictly, i.e. there's no need to talk about negative values, since they're not really useful, but I think it should be mentioned that the '@' is followed by a signed integer. Ah, I guess you're pointing me at systemd.time(7). FWIW, parse_timestamp sort of already handles negative values with the exception of anything that results in a timestamp of -1 -- mktime can't distinguish between a real timestamp of -1 and an invalid struct tm which it can't convert. $ journalctl --since='1969-12-31 18:59:59' Failed to parse timestamp: 1969-12-31 18:59:59 But then there's also some inconsistent behavior which parse_timestamp already exposes when dealing with absolutes: $ journalctl --since='1969-12-31 18:59:58' -- Logs begin at Fri 2013-11-15 18:11:44 EST, end at Sun 2014-03-23 14:01:01 EDT. -- EOF $ journalctl --since='-100 years' -- Logs begin at Fri 2013-11-15 18:11:44 EST, end at Sun 2014-03-23 14:01:01 EDT. -- logs follow I don't think this is going to work out so well when the return type involed (usec_t) is unsigned... Are you sure? In your patch you use safe_atoi (not safe_atou). And on my machine, time_t is defined as __time_t, which is __TIME_T_TYPE, __SYSCALL_SLONG_TYPE, which looks signed to me. Actually, it seems that this safe_atoi should be replaced by assert_cc(sizeof(time_t)==sizeof(long)) and safe_atoli. This isn't relevant to the problem I'm demonstrating. I agree that time_t is a signed type. The trouble is with the outvalue from parse_timestamps which is usec_t -- a systemd typedef which is uint64_t. Zbyszek ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH 1/2] endpoint: free the policy_db on error path
Signed-off-by: Djalal Harouni tix...@opendz.org
---
endpoint.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/endpoint.c b/endpoint.c
index 465ae31..5afabf9 100644
--- a/endpoint.c
+++ b/endpoint.c
@@ -236,7 +236,7 @@ int kdbus_ep_new(struct kdbus_bus *bus, const char *name,
if (bus-disconnected) {
mutex_unlock(bus-lock);
ret = -ESHUTDOWN;
- goto exit_dev_unregister;
+ goto exit_policy_db_free;
}
e-id = ++bus-ep_seq_last;
e-bus = kdbus_bus_ref(bus);
@@ -247,6 +247,9 @@ int kdbus_ep_new(struct kdbus_bus *bus, const char *name,
*ep = e;
return 0;
+exit_policy_db_free:
+ if (policy)
+ kdbus_policy_db_free(e-policy_db);
exit_dev_unregister:
device_unregister(e-dev);
exit_idr:
--
1.8.5.3
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH 2/2] test-kdbus: rename check bus and domain functions
Signed-off-by: Djalal Harouni tix...@opendz.org
---
test/test-kdbus.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/test/test-kdbus.c b/test/test-kdbus.c
index 0427419..6f9b3e9 100644
--- a/test/test-kdbus.c
+++ b/test/test-kdbus.c
@@ -259,7 +259,7 @@ static int send_message(const struct kdbus_conn *conn,
}
/* ---8--- */
-static int check_nsmake(struct kdbus_check_env *env)
+static int check_domain_make(struct kdbus_check_env *env)
{
int fd, fd2;
struct {
@@ -308,7 +308,7 @@ static int check_nsmake(struct kdbus_check_env *env)
/* ---8--- */
-static int check_busmake(struct kdbus_check_env *env)
+static int check_bus_make(struct kdbus_check_env *env)
{
struct {
struct kdbus_cmd_make head;
@@ -1101,7 +1101,7 @@ void check_unprepare_env(const struct kdbus_check *c,
struct kdbus_check_env *en
}
static const struct kdbus_check checks[] = {
- { bus make, check_busmake, 0
},
+ { bus make, check_bus_make, 0
},
{ hello, check_hello,
CHECK_CREATE_BUS},
{ byebye, check_byebye,
CHECK_CREATE_BUS | CHECK_CREATE_CONN},
{ monitor,check_monitor,
CHECK_CREATE_BUS},
@@ -1116,7 +1116,7 @@ static const struct kdbus_check checks[] = {
{ match name add, check_match_name_add,
CHECK_CREATE_BUS | CHECK_CREATE_CONN},
{ match name remove, check_match_name_remove,
CHECK_CREATE_BUS | CHECK_CREATE_CONN},
{ match name change, check_match_name_change,
CHECK_CREATE_BUS | CHECK_CREATE_CONN},
- { domain make,check_nsmake, 0
},
+ { domain make,check_domain_make, 0
},
{ NULL, NULL, 0 }
};
--
1.8.5.3
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC][PATCH] sd-bus: split ref-counting of queues from ref-counting of the rest of the sd-bus object
Hi
On Sun, Mar 23, 2014 at 4:49 PM, Tom Gundersen t...@jklm.no wrote:
Introduce a new ref-count, n_ref_queues, which only protects the {r,w}queue of
a bus, and introduce bus_{un,}ref(), which are only available internally, and
which do not protect these queues.
Make sure that sd_bus_message object do not call sd_bus_ref(), but only the
internal bus_ref(). This is ok as the {r,w}queues should never be accessed via
the message object (as doing so would anyway not be thread-safe).
When the refcount on the queues reaches zero (even thought the refcount on the
bus itself may not, due to references held by messages in the queues), the
queues are flushed and their messages unref'ed. This avoids problems due to
mutual references between busses and their queued messages. In particular we
don't get an sd_bus_unref() - sd_bus_message_unref() - sd_bus_unref()
call-chain.
Moreover, we can now enforce that sd_bus_{un,}ref() is only ever called from
the same thread as created the bus (whereas bus_unref() may be called from a
different thread, as part of unref'ing a message being handled in a worker
thread).
Code looks good and it should work this way. But as I said earlier,
I'd prefer if we could avoid dual-refcounts and instead take the refs
on the object we _actually_ want. Messages don't need the -bus
pointer at all except for memfds, as far as I can see. So instead of
introducing a circular dependency bus-m-bus, why not take a
reference to the kdbus FD instead? Either by dup()ing the fd or
extracting it to a separate struct/refcount. We can still take an
sd_bus argument in message allocations, but just avoid ref'ing them.
With the current approach, m-bus becomes a pseudo reference as it's
not equivalent to real references. If we forced messages to be
referenced on the bus they're queued on, we could fix this more
easily, but we don't have this requirement. Therefore, I really wonder
whether keeping m-bus is making anyone happy?
Anyhow, I wanna hear first what Lennart had planned with m-bus..
maybe he can shed some light on this.
Thanks
David
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [RFC][PATCH] sd-bus: split ref-counting of queues from ref-counting of the rest of the sd-bus object
On Sun, Mar 23, 2014 at 11:50 PM, David Herrmann dh.herrm...@gmail.com wrote:
On Sun, Mar 23, 2014 at 4:49 PM, Tom Gundersen t...@jklm.no wrote:
Introduce a new ref-count, n_ref_queues, which only protects the {r,w}queue
of
a bus, and introduce bus_{un,}ref(), which are only available internally, and
which do not protect these queues.
Make sure that sd_bus_message object do not call sd_bus_ref(), but only the
internal bus_ref(). This is ok as the {r,w}queues should never be accessed
via
the message object (as doing so would anyway not be thread-safe).
When the refcount on the queues reaches zero (even thought the refcount on
the
bus itself may not, due to references held by messages in the queues), the
queues are flushed and their messages unref'ed. This avoids problems due to
mutual references between busses and their queued messages. In particular we
don't get an sd_bus_unref() - sd_bus_message_unref() - sd_bus_unref()
call-chain.
Moreover, we can now enforce that sd_bus_{un,}ref() is only ever called from
the same thread as created the bus (whereas bus_unref() may be called from a
different thread, as part of unref'ing a message being handled in a worker
thread).
Code looks good and it should work this way. But as I said earlier,
I'd prefer if we could avoid dual-refcounts and instead take the refs
on the object we _actually_ want. Messages don't need the -bus
pointer at all except for memfds, as far as I can see. So instead of
introducing a circular dependency bus-m-bus, why not take a
reference to the kdbus FD instead? Either by dup()ing the fd or
extracting it to a separate struct/refcount. We can still take an
sd_bus argument in message allocations, but just avoid ref'ing them.
Yeah, if that is likely to remain the only use of m-bus, then just
grabbing the memfd directly makes more sense to me too... (and then
just drop all the atomic reference counting from sd_bus to make it
clearer that only sd_bus_message objects may be passed around to
different threads).
-t
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH] handle: unref handle-ep and free handle-meta on error path
Signed-off-by: Djalal Harouni tix...@opendz.org --- handle.c | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/handle.c b/handle.c index 921faca..3af6119 100644 --- a/handle.c +++ b/handle.c @@ -130,7 +130,7 @@ static int kdbus_handle_open(struct inode *inode, struct file *file) /* cache the metadata/credentials of the creator of the connection */ ret = kdbus_meta_new(handle-meta); if (ret 0) - goto exit_unlock; + goto exit_ep_unref; ret = kdbus_meta_append(handle-meta, NULL, 0, KDBUS_ATTACH_CREDS | @@ -142,11 +142,15 @@ static int kdbus_handle_open(struct inode *inode, struct file *file) KDBUS_ATTACH_SECLABEL | KDBUS_ATTACH_AUDIT); if (ret 0) - goto exit_unlock; + goto exit_meta_free; mutex_unlock(handle-domain-lock); return 0; +exit_meta_free: + kdbus_meta_free(handle-meta); +exit_ep_unref: + kdbus_ep_unref(handle-ep); exit_unlock: mutex_unlock(handle-domain-lock); kdbus_domain_unref(handle-domain); -- 1.8.5.3 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] time-util: accept epoch timetamps prefixed with @
On Sun, 23.03.14 11:06, Dave Reisner (dreis...@archlinux.org) wrote:
Also adds a few tests for the absolute cases of parse_timestamp.
Suggested by: Mantas Mikulėnas graw...@gmail.com
---
src/shared/time-util.c | 10 ++
src/test/test-time.c | 21 +
2 files changed, 31 insertions(+)
diff --git a/src/shared/time-util.c b/src/shared/time-util.c
index faa3418..fe43404 100644
--- a/src/shared/time-util.c
+++ b/src/shared/time-util.c
@@ -432,6 +432,7 @@ int parse_timestamp(const char *t, usec_t *usec) {
* tomorrow (time is set to 00:00:00)
* +5min
* -5days
+ * @1395584178 (seconds from the epoch)
*
*/
@@ -473,7 +474,16 @@ int parse_timestamp(const char *t, usec_t *usec) {
return r;
goto finish;
+} else if (t[0] == '@') {
+time_t epoch;
+r = safe_atoli(t+1, epoch);
+if (r 0)
+return r;
+
+assert_se(localtime_r(epoch, tm));
+
+goto finish;
Hmm, this is probably something we should return directly without
involving finish... After all it's kinda pointless to convert these
epoch times to calendar times first, and then back again... We should
just return them directly.
also, wouldn't it be more appropriate to use parse_sec() for pasing
this? That would also allow the syntaxes @30years referrring 30 years
after the epoch and similar. Not that this would be super-useful, but at
least it would correspond nicely with the +5min and -5min syntaxes
we already have, which do use parse_sec()...
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] stopping a systemd-nspwan container
On Sat, 22.03.14 16:35, Kevin Wilson (wkev...@gmail.com) wrote: Hello, I had created a container according to systemd-nspwan man page and ran it by: systemd-nspawn -D/srv/mycontainer I killed it by pkill systemd-nspaw (and not by poweroff from within the container). Now, running machinectl shows that the container still runs: machinectl MACHINE CONTAINER SERVICE mycontainer container nspawn 1 machines listed. but the following is strange: Running: systemd-nspawn -D/srv/mycontainer gives: Spawning namespace container on /srv/mycontainer (console is /dev/pts/2). Init process in the container running as PID 2305. Failed to register machine: File exists Container failed with error code 239. (and running it again gives the same result but with a different pid number). Is there a way to shut down the container which is running in such a scenario ? systemctl stop machine-xyz.scope systemctl reset-failed THis may happen sometimes since cgroup empty notifications is racy when we have subcgroups. This will soonishly get fixed with the kernel cgroup rework. Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] journal handling of process title changes
On Sun, 23.03.14 00:32, Patrick Donnelly (batr...@batbytes.com) wrote:
It seems the journal is reading from /proc/pid/cmdline (argv[0]) for
each entry. So when reading using journalctl, we don't see process
title changes properly. See the below example:
We are reading both /proc/$PID/comm and /proc/$PID/cmdline, and augment
journal entries with that. Unfortunately the kernel is currently too
limited to do this in a race-free way. This means that a service that
logs and terminates quickly afterwrads will trigger a race: journald
won't be able to read comm and cmdline in time. Also, if you keep
changing the comm/argv lines then we might use a later comm/argv for
messages already written long before since we only read this data much
later...
#include sys/prctl.h
#include systemd/sd-journal.h
int main (int argc, char *argv[])
{
printf(%d\n, prctl(PR_SET_NAME, foo, 0, 0, 0));
sd_journal_print(LOG_INFO, hi);
strcpy(argv[0], abc);
sd_journal_print(LOG_INFO, bye2);
return 0;
}
gcc test.c -o a.out -lsystemd
./a.out
we see:
... a.out[10321]: hi
... a.out[10321]: bye
... c[10321]: bye2
I don't see how after a cursory glance at the code, but systemd is
also magically remembering the old beginning of argv[0]. So, even if
I change argv[0], I must start my changes from the beginning of the
basename of the old title. So in the above example, ab replaced ./
in argv[0], only c is printed in the log.
It seems to me systemd should be using /proc/pid/comm for this.
And we do. Can you please dump the respective lines with -o verbose
from the journal? They should explain in detail what we are seeing
there...
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] journal handling of process title changes
[adding message to list, sorry Lennart...]
On Sun, Mar 23, 2014 at 9:25 PM, Lennart Poettering
lenn...@poettering.net wrote:
On Sun, 23.03.14 00:32, Patrick Donnelly (batr...@batbytes.com) wrote:
It seems the journal is reading from /proc/pid/cmdline (argv[0]) for
each entry. So when reading using journalctl, we don't see process
title changes properly. See the below example:
We are reading both /proc/$PID/comm and /proc/$PID/cmdline, and augment
journal entries with that. Unfortunately the kernel is currently too
limited to do this in a race-free way. This means that a service that
logs and terminates quickly afterwrads will trigger a race: journald
won't be able to read comm and cmdline in time. Also, if you keep
changing the comm/argv lines then we might use a later comm/argv for
messages already written long before since we only read this data much
later...
My problem is not related to race conditions. The issue is that
/proc/pid/cmdline is shown instead of /proc/pid/comm for each journal
entry. That is:
$ journalctl --boot
[...]
Mar 23 21:39:01 host a.out[10697]: hi
Mar 23 21:39:01 host c[10697]: bye2
[...]
These identifiers are being pulled from cmdline or argv[0] somehow.
#include sys/prctl.h
#include systemd/sd-journal.h
int main (int argc, char *argv[])
{
printf(%d\n, prctl(PR_SET_NAME, foo, 0, 0, 0));
sd_journal_print(LOG_INFO, hi);
strcpy(argv[0], abc);
sd_journal_print(LOG_INFO, bye2);
return 0;
}
gcc test.c -o a.out -lsystemd
./a.out
we see:
... a.out[10321]: hi
... a.out[10321]: bye
... c[10321]: bye2
I don't see how after a cursory glance at the code, but systemd is
also magically remembering the old beginning of argv[0]. So, even if
I change argv[0], I must start my changes from the beginning of the
basename of the old title. So in the above example, ab replaced ./
in argv[0], only c is printed in the log.
It seems to me systemd should be using /proc/pid/comm for this.
And we do. Can you please dump the respective lines with -o verbose
from the journal? They should explain in detail what we are seeing
there...
Sun 2014-03-23 21:39:01.932343 EDT
[s=485c69d197f74e3d855818a78b6d83b7;i=5d896;b=82f837d8221c4200bb8617f809638558;m=1459584e5;t=4f5504cd311bc;x=f318a8725568092f]
_MACHINE_ID=aae1a91de027484998daf076d8422b7e
_HOSTNAME=host
PRIORITY=6
_GID=100
_TRANSPORT=journal
_UID=1000
CODE_FUNC=main
MESSAGE=hi
CODE_FILE=test.c
CODE_LINE=7
SYSLOG_IDENTIFIER=a.out
_PID=10697
_BOOT_ID=82f837d8221c4200bb8617f809638558
_SOURCE_REALTIME_TIMESTAMP=1395625141932343
Sun 2014-03-23 21:39:01.932352 EDT
[s=485c69d197f74e3d855818a78b6d83b7;i=5d897;b=82f837d8221c4200bb8617f809638558;m=145958577;t=4f5504cd3124e;x=cfde456defb6c064]
_MACHINE_ID=aae1a91de027484998daf076d8422b7e
_HOSTNAME=host
PRIORITY=6
_GID=100
_TRANSPORT=journal
_UID=1000
CODE_FUNC=main
CODE_FILE=test.c
CODE_LINE=9
MESSAGE=bye2
SYSLOG_IDENTIFIER=c
_PID=10697
_BOOT_ID=82f837d8221c4200bb8617f809638558
_SOURCE_REALTIME_TIMESTAMP=1395625141932352
--
Patrick Donnelly
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 1/2] endpoint: free the policy_db on error path
On Sun, Mar 23, 2014 at 11:06 PM, Djalal Harouni tix...@opendz.org wrote: Signed-off-by: Djalal Harouni tix...@opendz.org --- endpoint.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) Applied. Thanks, Kay ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 2/2] test-kdbus: rename check bus and domain functions
On Sun, Mar 23, 2014 at 11:06 PM, Djalal Harouni tix...@opendz.org wrote: Signed-off-by: Djalal Harouni tix...@opendz.org --- test/test-kdbus.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) Applied. Thanks, Kay ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
