On Sat, Apr 12, 2014 at 1:40 PM, Zbigniew Jędrzejewski-Szmek
<zbys...@in.waw.pl> wrote:
> On Sat, Apr 12, 2014 at 12:07:04PM -0400, Mike Gilbert wrote:
>> Modifies find_binary() to accept NULL in the second argument.
>>
>> fsck.type lookup logic moved to new fsck_exists() function, with a test.
>> ---
>> src/fsck/fsck.c | 9 ++++-----
>> src/shared/generator.c | 11 ++++-------
>> src/shared/path-util.c | 11 ++++++++---
>> src/shared/util.c | 8 ++++++++
>> src/shared/util.h | 2 ++
>> src/test/test-util.c | 11 +++++++++++
>> 6 files changed, 37 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/fsck/fsck.c b/src/fsck/fsck.c
>> index 18f2aca..520b1a6 100644
>> --- a/src/fsck/fsck.c
>> +++ b/src/fsck/fsck.c
>> @@ -285,14 +285,13 @@ int main(int argc, char *argv[]) {
>>
>> type = udev_device_get_property_value(udev_device, "ID_FS_TYPE");
>> if (type) {
>> - const char *checker = strappenda("/sbin/fsck.", type);
>> - r = access(checker, X_OK);
>> + r = fsck_exists(type);
>> if (r < 0) {
>> - if (errno == ENOENT) {
>> - log_info("%s doesn't exist, not checking
>> file system.", checker);
>> + if (r == -ENOENT) {
>> + log_info("fsck.%s doesn't exist, not
>> checking file system.", type);
>> return EXIT_SUCCESS;
>> } else
>> - log_warning("%s cannot be used: %m",
>> checker);
>> + log_warning("fsck.%s cannot be used: %s",
>> type, strerror(-r));
>> }
>> }
>>
>> diff --git a/src/shared/generator.c b/src/shared/generator.c
>> index 6110303..86d30e7 100644
>> --- a/src/shared/generator.c
>> +++ b/src/shared/generator.c
>> @@ -19,6 +19,7 @@
>> along with systemd; If not, see <http://www.gnu.org/licenses/>.
>> ***/
>>
>> +#include <string.h>
>> #include <unistd.h>
>>
>> #include "util.h"
>> @@ -45,16 +46,12 @@ int generator_write_fsck_deps(
>> }
>>
>> if (!isempty(fstype) && !streq(fstype, "auto")) {
>> - const char *checker;
>> int r;
>> -
>> - checker = strappenda("/sbin/fsck.", fstype);
>> - r = access(checker, X_OK);
>> + r = fsck_exists(fstype);
>> if (r < 0) {
>> - log_warning("Checking was requested for %s, but %s
>> cannot be used: %m", what, checker);
>> -
>> + log_warning("Checking was requested for %s, but
>> fsck.%s cannot be used: %s", what, what, strerror(-r));
>
> The second arg should probably be fstype not what.
>
Right, I'll fix that.
>> /* treat missing check as essentially OK */
>> - return errno == ENOENT ? 0 : -errno;
>> + return r == -ENOENT ? 0 : r;
>> }
>> }
>>
>> diff --git a/src/shared/path-util.c b/src/shared/path-util.c
>> index bdc54a9..a530dbe 100644
>> --- a/src/shared/path-util.c
>> +++ b/src/shared/path-util.c
>> @@ -425,7 +425,6 @@ int path_is_os_tree(const char *path) {
>>
>> int find_binary(const char *name, char **filename) {
>> assert(name);
>> - assert(filename);
>>
>> if (strchr(name, '/')) {
>> char *p;
>> @@ -437,7 +436,10 @@ int find_binary(const char *name, char **filename) {
>> if (!p)
>> return -ENOMEM;
>>
>> - *filename = p;
>> + if (filename)
>> + *filename = p;
>> + else
>> + free(p);
> Allocating a string just to free it in the next line doesn't make sense.
>
Yeah, this one could probably be adjusted not to do that. I was trying
to change as little as possible.
>> return 0;
>> } else {
>> const char *path;
>> @@ -464,7 +466,10 @@ int find_binary(const char *name, char **filename) {
>> }
>>
>> path_kill_slashes(p);
>> - *filename = p;
>> + if (filename)
>> + *filename = p;
>> + else
>> + free(p);
> Likewise, no need to mangle the string just to free it.
>
Ah, yeah. I guess this would work:
if (filename)
path_kill_slashes(p);
*filename = p;
else
free(p);
>>
>> return 0;
>> }
>> diff --git a/src/shared/util.c b/src/shared/util.c
>> index ffe6624..4cdb561 100644
>> --- a/src/shared/util.c
>> +++ b/src/shared/util.c
>> @@ -6391,3 +6391,11 @@ void hexdump(FILE *f, const void *p, size_t s) {
>> s -= 16;
>> }
>> }
>> +
>> +int fsck_exists(const char *fstype)
>> +{
> Brace should go on the end of previous line.
>
>> + const char *checker;
>> +
>> + checker = strappenda("fsck.", fstype);
>> + return find_binary(checker, NULL);
>> +}
>> diff --git a/src/shared/util.h b/src/shared/util.h
>> index 90464c9..45a6f26 100644
>> --- a/src/shared/util.h
>> +++ b/src/shared/util.h
>> @@ -922,3 +922,5 @@ uint64_t physical_memory(void);
>> char* mount_test_option(const char *haystack, const char *needle);
>>
>> void hexdump(FILE *f, const void *p, size_t s);
>> +
>> +int fsck_exists(const char *fstype);
>> diff --git a/src/test/test-util.c b/src/test/test-util.c
>> index 93929cd..148e54c 100644
>> --- a/src/test/test-util.c
>> +++ b/src/test/test-util.c
>> @@ -675,6 +675,16 @@ static void test_foreach_string(void) {
>> assert_se(streq(x, "zzz"));
>> }
>>
>> +static void test_fsck_exists(void) {
>> + /* Ensure we use a sane default for PATH. */
>> + unsetenv("PATH");
>> +
>> + /* fsck.minix is provided by util-linux and will probably exist. */
>> + assert(fsck_exists("minix") == 0);
>> +
>> + assert(fsck_exists("AbCdE") == -ENOENT);
>> +}
> assert_se().
>
We are comparing integers, not strings. This matches the other integer
comparisons in this source file.
>> +
>> int main(int argc, char *argv[]) {
>> log_parse_environment();
>> log_open();
>> @@ -719,6 +729,7 @@ int main(int argc, char *argv[]) {
>> test_hexdump();
>> test_log2i();
>> test_foreach_string();
>> + test_fsck_exists();
>>
>> return 0;
>> }
>
> Zbyszek
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
