From 3f952f92b9f401fbe4c4876541ca145a551df039 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 15 Oct 2015 18:37:03 +0200 Subject: btrfs: always remove the per-subvol qgroup when removing a subvol btrfs doesn't do that automatically, hence let's do that explicitly each time. --- src/basic/btrfs-util.c | 63 ++++++++++++++++++++++++++++++++++++++++++-------- src/basic/btrfs-util.h | 3 +++ src/basic/missing.h | 4 ++++ 3 files changed, 60 insertions(+), 10 deletions(-) (limited to 'src') diff --git a/src/basic/btrfs-util.c b/src/basic/btrfs-util.c index 074deeccda..ec7e00986b 100644 --- a/src/basic/btrfs-util.c +++ b/src/basic/btrfs-util.c @@ -799,6 +799,45 @@ int btrfs_resize_loopback(const char *p, uint64_t new_size, bool grow_only) { return btrfs_resize_loopback_fd(fd, new_size, grow_only); } +static int make_qgroup_id(uint64_t level, uint64_t id, uint64_t *ret) { + assert(ret); + + if (level >= (UINT64_C(1) << (64 - BTRFS_QGROUP_LEVEL_SHIFT))) + return -EINVAL; + + if (id >= (UINT64_C(1) << BTRFS_QGROUP_LEVEL_SHIFT)) + return -EINVAL; + + *ret = (level << BTRFS_QGROUP_LEVEL_SHIFT) | id; + return 0; +} + +static int qgroup_create_or_destroy(int fd, bool b, uint64_t level, uint64_t id) { + + struct btrfs_ioctl_qgroup_create_args args = { + .create = b, + }; + + int r; + + r = make_qgroup_id(level, id, (uint64_t*) &args.qgroupid); + if (r < 0) + return r; + + if (ioctl(fd, BTRFS_IOC_QGROUP_CREATE, &args) < 0) + return -errno; + + return 0; +} + +int btrfs_qgroup_create(int fd, uint64_t level, uint64_t id) { + return qgroup_create_or_destroy(fd, true, level, id); +} + +int btrfs_qgroup_destroy(int fd, uint64_t level, uint64_t id) { + return qgroup_create_or_destroy(fd, false, level, id); +} + static int subvol_remove_children(int fd, const char *subvolume, uint64_t subvol_id, bool recursive) { struct btrfs_ioctl_search_args args = { .key.tree_id = BTRFS_ROOT_TREE_OBJECTID, @@ -828,16 +867,6 @@ static int subvol_remove_children(int fd, const char *subvolume, uint64_t subvol if (!S_ISDIR(st.st_mode)) return -EINVAL; - /* First, try to remove the subvolume. If it happens to be - * already empty, this will just work. */ - strncpy(vol_args.name, subvolume, sizeof(vol_args.name)-1); - if (ioctl(fd, BTRFS_IOC_SNAP_DESTROY, &vol_args) >= 0) - return 0; - if (!recursive || errno != ENOTEMPTY) - return -errno; - - /* OK, the subvolume is not empty, let's look for child - * subvolumes, and remove them, first */ subvol_fd = openat(fd, subvolume, O_RDONLY|O_NOCTTY|O_CLOEXEC|O_DIRECTORY); if (subvol_fd < 0) return -errno; @@ -848,6 +877,19 @@ static int subvol_remove_children(int fd, const char *subvolume, uint64_t subvol return r; } + /* First, try to remove the subvolume. If it happens to be + * already empty, this will just work. */ + strncpy(vol_args.name, subvolume, sizeof(vol_args.name)-1); + if (ioctl(fd, BTRFS_IOC_SNAP_DESTROY, &vol_args) >= 0) { + (void) btrfs_qgroup_destroy(fd, 0, subvol_id); + return 0; + } + if (!recursive || errno != ENOTEMPTY) + return -errno; + + /* OK, the subvolume is not empty, let's look for child + * subvolumes, and remove them, first */ + args.key.min_offset = args.key.max_offset = subvol_id; while (btrfs_ioctl_search_args_compare(&args) <= 0) { @@ -925,6 +967,7 @@ static int subvol_remove_children(int fd, const char *subvolume, uint64_t subvol if (ioctl(fd, BTRFS_IOC_SNAP_DESTROY, &vol_args) < 0) return -errno; + (void) btrfs_qgroup_destroy(fd, 0, subvol_id); return 0; } diff --git a/src/basic/btrfs-util.h b/src/basic/btrfs-util.h index 8632c3638c..ad7c7009ab 100644 --- a/src/basic/btrfs-util.h +++ b/src/basic/btrfs-util.h @@ -86,3 +86,6 @@ int btrfs_resize_loopback(const char *path, uint64_t size, bool grow_only); int btrfs_subvol_remove(const char *path, bool recursive); int btrfs_subvol_remove_fd(int fd, const char *subvolume, bool recursive); + +int btrfs_qgroup_create(int fd, uint64_t level, uint64_t id); +int btrfs_qgroup_destroy(int fd, uint64_t level, uint64_t id); diff --git a/src/basic/missing.h b/src/basic/missing.h index 59e835a466..8d11b80d95 100644 --- a/src/basic/missing.h +++ b/src/basic/missing.h @@ -248,6 +248,10 @@ static inline int getrandom(void *buffer, size_t count, unsigned flags) { #define BTRFS_SEARCH_ARGS_BUFSIZE (4096 - sizeof(struct btrfs_ioctl_search_key)) #endif +#ifndef BTRFS_QGROUP_LEVEL_SHIFT +#define BTRFS_QGROUP_LEVEL_SHIFT 48 +#endif + #ifndef HAVE_LINUX_BTRFS_H struct btrfs_ioctl_vol_args { int64_t fd; -- cgit v1.2.3-54-g00ecf From 1a37c9756f0c55917192e1a229977734b1f7ea45 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sat, 17 Oct 2015 16:20:38 +0200 Subject: bus-proxy: don't close local bus fds twice Clear up how we pass fd owner ship to proxy and bus objects. Document that ownership is passed of the fds in question even in case of failing constructors, and that callers should forget about fds pass into the proxy object. The alternative would be to duplicate the fds, but given that fds are a relatively scarce and heavy resource let's better avoid that. Fixes #1591. --- src/bus-proxyd/bus-proxyd.c | 4 ++-- src/bus-proxyd/proxy.c | 38 +++++++++++++++++++++++++++++--------- 2 files changed, 31 insertions(+), 11 deletions(-) (limited to 'src') diff --git a/src/bus-proxyd/bus-proxyd.c b/src/bus-proxyd/bus-proxyd.c index 2bc265d9b4..6a4da0f2e2 100644 --- a/src/bus-proxyd/bus-proxyd.c +++ b/src/bus-proxyd/bus-proxyd.c @@ -85,11 +85,11 @@ static void *run_client(void *userdata) { int r; r = proxy_new(&p, c->fd, c->fd, arg_address); + c->fd = -1; + if (r < 0) goto exit; - c->fd = -1; - /* set comm to "p$PIDu$UID" and suffix with '*' if truncated */ r = snprintf(comm, sizeof(comm), "p" PID_FMT "u" UID_FMT, p->local_creds.pid, p->local_creds.uid); if (r >= (ssize_t)sizeof(comm)) diff --git a/src/bus-proxyd/proxy.c b/src/bus-proxyd/proxy.c index 88800f5e7f..bc8516f5c6 100644 --- a/src/bus-proxyd/proxy.c +++ b/src/bus-proxyd/proxy.c @@ -100,18 +100,24 @@ static int proxy_create_destination(Proxy *p, const char *destination, const cha return 0; } -static int proxy_create_local(Proxy *p, int in_fd, int out_fd, bool negotiate_fds) { - _cleanup_bus_flush_close_unref_ sd_bus *b = NULL; +static int proxy_create_local(Proxy *p, bool negotiate_fds) { sd_id128_t server_id; + sd_bus *b; int r; r = sd_bus_new(&b); if (r < 0) return log_error_errno(r, "Failed to allocate bus: %m"); - r = sd_bus_set_fd(b, in_fd, out_fd); - if (r < 0) + r = sd_bus_set_fd(b, p->local_in, p->local_out); + if (r < 0) { + sd_bus_unref(b); return log_error_errno(r, "Failed to set fds: %m"); + } + + /* The fds are now owned by the bus, and we indicate that by + * storing the bus object in the proxy object. */ + p->local_bus = b; r = sd_bus_get_bus_id(p->destination_bus, &server_id); if (r < 0) @@ -139,8 +145,6 @@ static int proxy_create_local(Proxy *p, int in_fd, int out_fd, bool negotiate_fd if (r < 0) return log_error_errno(r, "Failed to start bus client: %m"); - p->local_bus = b; - b = NULL; return 0; } @@ -224,9 +228,17 @@ int proxy_new(Proxy **out, int in_fd, int out_fd, const char *destination) { bool is_unix; int r; + /* This takes possession/destroys the file descriptors passed + * in even on failure. The caller should hence forget about + * the fds in all cases after calling this function and not + * close them. */ + p = new0(Proxy, 1); - if (!p) + if (!p) { + safe_close(in_fd); + safe_close(out_fd); return log_oom(); + } p->local_in = in_fd; p->local_out = out_fd; @@ -247,7 +259,7 @@ int proxy_new(Proxy **out, int in_fd, int out_fd, const char *destination) { if (r < 0) return r; - r = proxy_create_local(p, in_fd, out_fd, is_unix); + r = proxy_create_local(p, is_unix); if (r < 0) return r; @@ -257,6 +269,7 @@ int proxy_new(Proxy **out, int in_fd, int out_fd, const char *destination) { *out = p; p = NULL; + return 0; } @@ -273,7 +286,14 @@ Proxy *proxy_free(Proxy *p) { free(activation); } - sd_bus_flush_close_unref(p->local_bus); + if (p->local_bus) + sd_bus_flush_close_unref(p->local_bus); + else { + safe_close(p->local_in); + if (p->local_out != p->local_in) + safe_close(p->local_out); + } + sd_bus_flush_close_unref(p->destination_bus); set_free_free(p->owned_names); free(p); -- cgit v1.2.3-54-g00ecf From 50e0d56cf37d8ca5b9162ab83906920392998623 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sat, 17 Oct 2015 16:23:45 +0200 Subject: sd-bus: fix error handling of pthread API calls pthread APIs (unlike the rest of libc) return their errors as positive error codes directly from the functions, rather than using errno. Let's make sure we always handle things that way. --- src/bus-proxyd/bus-proxyd.c | 13 ++++++------- src/bus-proxyd/bus-xml-policy.c | 8 ++++---- src/libsystemd/sd-bus/bus-kernel.c | 12 ++++++------ src/libsystemd/sd-event/sd-event.c | 4 ++-- 4 files changed, 18 insertions(+), 19 deletions(-) (limited to 'src') diff --git a/src/bus-proxyd/bus-proxyd.c b/src/bus-proxyd/bus-proxyd.c index 6a4da0f2e2..64d1c5231f 100644 --- a/src/bus-proxyd/bus-proxyd.c +++ b/src/bus-proxyd/bus-proxyd.c @@ -116,13 +116,12 @@ static int loop_clients(int accept_fd, uid_t bus_uid) { int r; r = pthread_attr_init(&attr); - if (r < 0) { - return log_error_errno(errno, "Cannot initialize pthread attributes: %m"); - } + if (r != 0) + return log_error_errno(r, "Cannot initialize pthread attributes: %m"); r = pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED); - if (r < 0) { - r = log_error_errno(errno, "Cannot mark pthread attributes as detached: %m"); + if (r != 0) { + r = log_error_errno(r, "Cannot mark pthread attributes as detached: %m"); goto finish; } @@ -156,8 +155,8 @@ static int loop_clients(int accept_fd, uid_t bus_uid) { c->bus_uid = bus_uid; r = pthread_create(&tid, &attr, run_client, c); - if (r < 0) { - log_error("Cannot spawn thread: %m"); + if (r != 0) { + log_warning_errno(r, "Cannot spawn thread, ignoring: %m"); client_context_free(c); continue; } diff --git a/src/bus-proxyd/bus-xml-policy.c b/src/bus-proxyd/bus-xml-policy.c index 9a3b451c56..91717653c2 100644 --- a/src/bus-proxyd/bus-xml-policy.c +++ b/src/bus-proxyd/bus-xml-policy.c @@ -1186,14 +1186,14 @@ int shared_policy_new(SharedPolicy **out) { return log_oom(); r = pthread_mutex_init(&sp->lock, NULL); - if (r < 0) { - log_error_errno(r, "Cannot initialize shared policy mutex: %m"); + if (r != 0) { + r = log_error_errno(r, "Cannot initialize shared policy mutex: %m"); goto exit_free; } r = pthread_rwlock_init(&sp->rwlock, NULL); - if (r < 0) { - log_error_errno(r, "Cannot initialize shared policy rwlock: %m"); + if (r != 0) { + r = log_error_errno(r, "Cannot initialize shared policy rwlock: %m"); goto exit_mutex; } diff --git a/src/libsystemd/sd-bus/bus-kernel.c b/src/libsystemd/sd-bus/bus-kernel.c index 577a8b44c3..570d35c7ad 100644 --- a/src/libsystemd/sd-bus/bus-kernel.c +++ b/src/libsystemd/sd-bus/bus-kernel.c @@ -1433,12 +1433,12 @@ int bus_kernel_pop_memfd(sd_bus *bus, void **address, size_t *mapped, size_t *al if (!bus || !bus->is_kernel) return -EOPNOTSUPP; - assert_se(pthread_mutex_lock(&bus->memfd_cache_mutex) >= 0); + assert_se(pthread_mutex_lock(&bus->memfd_cache_mutex) == 0); if (bus->n_memfd_cache <= 0) { int r; - assert_se(pthread_mutex_unlock(&bus->memfd_cache_mutex) >= 0); + assert_se(pthread_mutex_unlock(&bus->memfd_cache_mutex) == 0); r = memfd_new(bus->description); if (r < 0) @@ -1460,7 +1460,7 @@ int bus_kernel_pop_memfd(sd_bus *bus, void **address, size_t *mapped, size_t *al *allocated = c->allocated; fd = c->fd; - assert_se(pthread_mutex_unlock(&bus->memfd_cache_mutex) >= 0); + assert_se(pthread_mutex_unlock(&bus->memfd_cache_mutex) == 0); return fd; } @@ -1484,10 +1484,10 @@ void bus_kernel_push_memfd(sd_bus *bus, int fd, void *address, size_t mapped, si return; } - assert_se(pthread_mutex_lock(&bus->memfd_cache_mutex) >= 0); + assert_se(pthread_mutex_lock(&bus->memfd_cache_mutex) == 0); if (bus->n_memfd_cache >= ELEMENTSOF(bus->memfd_cache)) { - assert_se(pthread_mutex_unlock(&bus->memfd_cache_mutex) >= 0); + assert_se(pthread_mutex_unlock(&bus->memfd_cache_mutex) == 0); close_and_munmap(fd, address, mapped); return; @@ -1507,7 +1507,7 @@ void bus_kernel_push_memfd(sd_bus *bus, int fd, void *address, size_t mapped, si c->allocated = allocated; } - assert_se(pthread_mutex_unlock(&bus->memfd_cache_mutex) >= 0); + assert_se(pthread_mutex_unlock(&bus->memfd_cache_mutex) == 0); } void bus_kernel_flush_memfd(sd_bus *b) { diff --git a/src/libsystemd/sd-event/sd-event.c b/src/libsystemd/sd-event/sd-event.c index 1a82c4c940..1905ebfc73 100644 --- a/src/libsystemd/sd-event/sd-event.c +++ b/src/libsystemd/sd-event/sd-event.c @@ -1123,8 +1123,8 @@ _public_ int sd_event_add_signal( callback = signal_exit_callback; r = pthread_sigmask(SIG_SETMASK, NULL, &ss); - if (r < 0) - return -errno; + if (r != 0) + return -r; if (!sigismember(&ss, sig)) return -EBUSY; -- cgit v1.2.3-54-g00ecf From 9806e87da22d0025d7c427907202e5751a6b5989 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sat, 17 Oct 2015 16:25:10 +0200 Subject: unit: allocate bus name match string on the stack Let's use strjoina() rather than strjoin() for construct dbus match strings. Also, while we are at it, fix parameter ordering, so that our functions always put the object first, like it is customary for OO-like programming. --- src/core/dbus.c | 2 +- src/core/unit.c | 19 ++++++++----------- src/core/unit.h | 2 +- 3 files changed, 10 insertions(+), 13 deletions(-) (limited to 'src') diff --git a/src/core/dbus.c b/src/core/dbus.c index 2d6a1ff836..d8891d49d8 100644 --- a/src/core/dbus.c +++ b/src/core/dbus.c @@ -777,7 +777,7 @@ static int bus_setup_api(Manager *m, sd_bus *bus) { return r; HASHMAP_FOREACH_KEY(u, name, m->watch_bus, i) { - r = unit_install_bus_match(bus, u, name); + r = unit_install_bus_match(u, bus, name); if (r < 0) log_error_errno(r, "Failed to subscribe to NameOwnerChanged signal: %m"); } diff --git a/src/core/unit.c b/src/core/unit.c index 39cd89f1e3..d8f0eb8111 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2508,26 +2508,23 @@ static int signal_name_owner_changed(sd_bus_message *message, void *userdata, sd return 0; } -int unit_install_bus_match(sd_bus *bus, Unit *u, const char *name) { - _cleanup_free_ char *match = NULL; - Manager *m = u->manager; +int unit_install_bus_match(Unit *u, sd_bus *bus, const char *name) { + const char *match; - assert(m); + assert(u); + assert(bus); + assert(name); if (u->match_bus_slot) return -EBUSY; - match = strjoin("type='signal'," + match = strjoina("type='signal'," "sender='org.freedesktop.DBus'," "path='/org/freedesktop/DBus'," "interface='org.freedesktop.DBus'," "member='NameOwnerChanged'," - "arg0='", - name, - "'", + "arg0='", name, "'", NULL); - if (!match) - return -ENOMEM; return sd_bus_add_match(bus, &u->match_bus_slot, match, signal_name_owner_changed, u); } @@ -2544,7 +2541,7 @@ int unit_watch_bus_name(Unit *u, const char *name) { if (u->manager->api_bus) { /* If the bus is already available, install the match directly. * Otherwise, just put the name in the list. bus_setup_api() will take care later. */ - r = unit_install_bus_match(u->manager->api_bus, u, name); + r = unit_install_bus_match(u, u->manager->api_bus, name); if (r < 0) return log_warning_errno(r, "Failed to subscribe to NameOwnerChanged signal: %m"); } diff --git a/src/core/unit.h b/src/core/unit.h index a4a1b011fc..9f8518c720 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -520,7 +520,7 @@ void unit_unwatch_all_pids(Unit *u); void unit_tidy_watch_pids(Unit *u, pid_t except1, pid_t except2); -int unit_install_bus_match(sd_bus *bus, Unit *u, const char *name); +int unit_install_bus_match(Unit *u, sd_bus *bus, const char *name); int unit_watch_bus_name(Unit *u, const char *name); void unit_unwatch_bus_name(Unit *u, const char *name); -- cgit v1.2.3-54-g00ecf