Re: [systemd-devel] [PATCH v2] fsck: Search for fsck.type in PATH

2014-04-14 Thread Colin Guthrie 
'Twas brillig, and Mike Gilbert at 12/04/14 20:54 did gyre and gimble:
 On Sat, Apr 12, 2014 at 3:10 PM, Mike Gilbert flop...@gentoo.org wrote:
 On Sat, Apr 12, 2014 at 1:40 PM, Zbigniew Jędrzejewski-Szmek
 zbys...@in.waw.pl wrote:
 +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.

 
 I guess I have misinterpreted what assert_se means... is that
 documented somewhere?

the _se suffix just means side effect. So if systemd were compiled
without asserts the action would still happen (whereas a normal assert()
is totally skipped).

In this case the side effect is actually calling fsck_exists().

Col


-- 

Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
  Tribalogic Limited http://www.tribalogic.net/
Open Source:
  Mageia Contributor http://www.mageia.org/
  PulseAudio Hacker http://www.pulseaudio.org/
  Trac Hacker http://trac.edgewall.org/
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH v2] fsck: Search for fsck.type in PATH

2014-04-12 Thread Zbigniew Jędrzejewski-Szmek 
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.

  /* 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.

  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.

  
  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

Re: [systemd-devel] [PATCH v2] fsck: Search for fsck.type in PATH

2014-04-12 Thread Mike Gilbert 
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

Re: [systemd-devel] [PATCH v2] fsck: Search for fsck.type in PATH

2014-04-12 Thread Mike Gilbert 
On Sat, Apr 12, 2014 at 3:10 PM, Mike Gilbert flop...@gentoo.org wrote:
 On Sat, Apr 12, 2014 at 1:40 PM, Zbigniew Jędrzejewski-Szmek
 zbys...@in.waw.pl wrote:
 +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.


I guess I have misinterpreted what assert_se means... is that
documented somewhere?
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel