* [PATCH 1/2] mountpoint: refactor exit path
@ 2011-10-09 15:12 Dave Reisner
2011-10-09 15:12 ` [PATCH 2/2] mountpoint: support symbolic and relative paths Dave Reisner
2011-10-10 19:45 ` [PATCH 1/2] mountpoint: refactor exit path Dave Reisner
0 siblings, 2 replies; 11+ messages in thread
From: Dave Reisner @ 2011-10-09 15:12 UTC (permalink / raw)
To: util-linux; +Cc: Dave Reisner
There's only one condition for which we declare success, but many for
failure. Initialize rc as failure and set to success on this single
condition. In all cases, jump to a label to exit instead of exiting
immediately. This will be used later on to ease cleanup of any heap
allocations.
Signed-off-by: Dave Reisner <dreisner@archlinux.org>
---
sys-utils/mountpoint.c | 15 +++++++++------
1 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/sys-utils/mountpoint.c b/sys-utils/mountpoint.c
index 1297d82..e021c70 100644
--- a/sys-utils/mountpoint.c
+++ b/sys-utils/mountpoint.c
@@ -105,7 +105,7 @@ static void __attribute__((__noreturn__)) usage(FILE *out)
int main(int argc, char **argv)
{
- int c, fs_devno = 0, dev_devno = 0, rc = 0;
+ int c, fs_devno = 0, dev_devno = 0, rc = EXIT_FAILURE;
char *spec;
struct stat st;
@@ -152,7 +152,7 @@ int main(int argc, char **argv)
if (stat(spec, &st)) {
if (!quiet)
err(EXIT_FAILURE, "%s", spec);
- return EXIT_FAILURE;
+ goto finish;
}
if (dev_devno)
rc = print_devno(spec, &st);
@@ -162,19 +162,22 @@ int main(int argc, char **argv)
if (!S_ISDIR(st.st_mode)) {
if (!quiet)
errx(EXIT_FAILURE, _("%s: not a directory"), spec);
- return EXIT_FAILURE;
+ goto finish;
}
src = dir_to_device(spec);
if (src == (dev_t)-1) {
if (!quiet)
printf(_("%s is not a mountpoint\n"), spec);
- return EXIT_FAILURE;
+ goto finish;
}
if (fs_devno)
printf("%u:%u\n", major(src), minor(src));
- else if (!quiet)
+ else if (!quiet) {
printf(_("%s is a mountpoint\n"), spec);
+ rc = EXIT_SUCCESS;
+ }
}
- return rc ? EXIT_FAILURE : EXIT_SUCCESS;
+finish:
+ return rc;
}
--
1.7.7
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] mountpoint: support symbolic and relative paths
2011-10-09 15:12 [PATCH 1/2] mountpoint: refactor exit path Dave Reisner
@ 2011-10-09 15:12 ` Dave Reisner
2011-10-10 19:45 ` [PATCH 1/2] mountpoint: refactor exit path Dave Reisner
1 sibling, 0 replies; 11+ messages in thread
From: Dave Reisner @ 2011-10-09 15:12 UTC (permalink / raw)
To: util-linux; +Cc: Dave Reisner
Use our own canonicalize library to resolve a provided path before
checking if its a mountpoint.
Signed-off-by: Dave Reisner <dreisner@archlinux.org>
---
Unless I've missed something, this should bring u-l's mountpoint to feature
parity with sysvinit's.
sys-utils/Makefile.am | 1 +
sys-utils/mountpoint.c | 5 ++++-
2 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/sys-utils/Makefile.am b/sys-utils/Makefile.am
index 8059ee6..34108da 100644
--- a/sys-utils/Makefile.am
+++ b/sys-utils/Makefile.am
@@ -44,6 +44,7 @@ flock_SOURCES = flock.c $(top_srcdir)/lib/strutils.c
if BUILD_MOUNTPOINT
bin_PROGRAMS += mountpoint
+mountpoint_SOURCES = mountpoint.c $(top_srcdir)/lib/canonicalize.c
mountpoint_LDADD = $(ul_libmount_la)
mountpoint_CFLAGS = $(AM_CFLAGS) -I$(ul_libmount_incdir)
dist_man_MANS += mountpoint.1
diff --git a/sys-utils/mountpoint.c b/sys-utils/mountpoint.c
index e021c70..204f061 100644
--- a/sys-utils/mountpoint.c
+++ b/sys-utils/mountpoint.c
@@ -37,6 +37,7 @@
#include "nls.h"
#include "xalloc.h"
#include "c.h"
+#include "canonicalize.h"
static int quiet;
@@ -147,7 +148,8 @@ int main(int argc, char **argv)
if (optind + 1 != argc)
usage(stderr);
- spec = argv[optind++];
+ /* resolve relative paths to absolute */
+ spec = canonicalize_path(argv[optind++]);
if (stat(spec, &st)) {
if (!quiet)
@@ -179,5 +181,6 @@ int main(int argc, char **argv)
}
finish:
+ free(spec);
return rc;
}
--
1.7.7
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] mountpoint: refactor exit path
2011-10-09 15:12 [PATCH 1/2] mountpoint: refactor exit path Dave Reisner
2011-10-09 15:12 ` [PATCH 2/2] mountpoint: support symbolic and relative paths Dave Reisner
@ 2011-10-10 19:45 ` Dave Reisner
2011-10-10 23:05 ` Karel Zak
1 sibling, 1 reply; 11+ messages in thread
From: Dave Reisner @ 2011-10-10 19:45 UTC (permalink / raw)
To: util-linux
On Sun, Oct 09, 2011 at 11:12:04AM -0400, Dave Reisner wrote:
> There's only one condition for which we declare success, but many for
> failure. Initialize rc as failure and set to success on this single
> condition. In all cases, jump to a label to exit instead of exiting
> immediately. This will be used later on to ease cleanup of any heap
> allocations.
>
> Signed-off-by: Dave Reisner <dreisner@archlinux.org>
> ---
Noticing that this could be done better -- the return/goto calls here
don't do anything because err/errx are exiting after throwing the error
message. They should be replaced with equivalent warn/warnx calls so we
hit the goto on error and cleanup before exiting.
@Karel: I can resend this if you like.
Regards,
dave
> sys-utils/mountpoint.c | 15 +++++++++------
> 1 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/sys-utils/mountpoint.c b/sys-utils/mountpoint.c
> index 1297d82..e021c70 100644
> --- a/sys-utils/mountpoint.c
> +++ b/sys-utils/mountpoint.c
> @@ -105,7 +105,7 @@ static void __attribute__((__noreturn__)) usage(FILE *out)
>
> int main(int argc, char **argv)
> {
> - int c, fs_devno = 0, dev_devno = 0, rc = 0;
> + int c, fs_devno = 0, dev_devno = 0, rc = EXIT_FAILURE;
> char *spec;
> struct stat st;
>
> @@ -152,7 +152,7 @@ int main(int argc, char **argv)
> if (stat(spec, &st)) {
> if (!quiet)
> err(EXIT_FAILURE, "%s", spec);
> - return EXIT_FAILURE;
> + goto finish;
> }
> if (dev_devno)
> rc = print_devno(spec, &st);
> @@ -162,19 +162,22 @@ int main(int argc, char **argv)
> if (!S_ISDIR(st.st_mode)) {
> if (!quiet)
> errx(EXIT_FAILURE, _("%s: not a directory"), spec);
> - return EXIT_FAILURE;
> + goto finish;
> }
> src = dir_to_device(spec);
> if (src == (dev_t)-1) {
> if (!quiet)
> printf(_("%s is not a mountpoint\n"), spec);
> - return EXIT_FAILURE;
> + goto finish;
> }
> if (fs_devno)
> printf("%u:%u\n", major(src), minor(src));
> - else if (!quiet)
> + else if (!quiet) {
> printf(_("%s is a mountpoint\n"), spec);
> + rc = EXIT_SUCCESS;
> + }
> }
>
> - return rc ? EXIT_FAILURE : EXIT_SUCCESS;
> +finish:
> + return rc;
> }
> --
> 1.7.7
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] mountpoint: refactor exit path
2011-10-10 19:45 ` [PATCH 1/2] mountpoint: refactor exit path Dave Reisner
@ 2011-10-10 23:05 ` Karel Zak
0 siblings, 0 replies; 11+ messages in thread
From: Karel Zak @ 2011-10-10 23:05 UTC (permalink / raw)
To: Dave Reisner; +Cc: util-linux
On Mon, Oct 10, 2011 at 03:45:45PM -0400, Dave Reisner wrote:
> On Sun, Oct 09, 2011 at 11:12:04AM -0400, Dave Reisner wrote:
> > There's only one condition for which we declare success, but many for
> > failure. Initialize rc as failure and set to success on this single
> > condition. In all cases, jump to a label to exit instead of exiting
> > immediately. This will be used later on to ease cleanup of any heap
> > allocations.
Well, free() before exit() is usually waste of time... but maybe we
can think about mountpoint(1) as about libmount test application,
then it would be nice to keep it without leaks (for example for
valgrind).
> @Karel: I can resend this if you like.
go ahead, thanks
--
Karel Zak <kzak@redhat.com>
http://karelzak.blogspot.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] mountpoint: refactor exit path
@ 2011-10-10 23:14 Dave Reisner
2011-10-11 7:07 ` Voelker, Bernhard
2011-10-11 9:06 ` Karel Zak
0 siblings, 2 replies; 11+ messages in thread
From: Dave Reisner @ 2011-10-10 23:14 UTC (permalink / raw)
To: util-linux; +Cc: Dave Reisner
There's only one condition for which we declare success, but many for
failure. Initialize rc as failure and set to success on this single
condition. In all cases, jump to a label to exit instead of exiting
immediately. This will be used later on to ease cleanup of any heap
allocations.
Signed-off-by: Dave Reisner <dreisner@archlinux.org>
---
v2: use warn{,x} instead of err{,x} to avoid leaks.
sys-utils/mountpoint.c | 19 +++++++++++--------
1 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/sys-utils/mountpoint.c b/sys-utils/mountpoint.c
index 1297d82..e075b83 100644
--- a/sys-utils/mountpoint.c
+++ b/sys-utils/mountpoint.c
@@ -105,7 +105,7 @@ static void __attribute__((__noreturn__)) usage(FILE *out)
int main(int argc, char **argv)
{
- int c, fs_devno = 0, dev_devno = 0, rc = 0;
+ int c, fs_devno = 0, dev_devno = 0, rc = EXIT_FAILURE;
char *spec;
struct stat st;
@@ -151,8 +151,8 @@ int main(int argc, char **argv)
if (stat(spec, &st)) {
if (!quiet)
- err(EXIT_FAILURE, "%s", spec);
- return EXIT_FAILURE;
+ warn("%s", spec);
+ goto finish;
}
if (dev_devno)
rc = print_devno(spec, &st);
@@ -161,20 +161,23 @@ int main(int argc, char **argv)
if (!S_ISDIR(st.st_mode)) {
if (!quiet)
- errx(EXIT_FAILURE, _("%s: not a directory"), spec);
- return EXIT_FAILURE;
+ warnx(_("%s: not a directory"), spec);
+ goto finish;
}
src = dir_to_device(spec);
if (src == (dev_t)-1) {
if (!quiet)
printf(_("%s is not a mountpoint\n"), spec);
- return EXIT_FAILURE;
+ goto finish;
}
if (fs_devno)
printf("%u:%u\n", major(src), minor(src));
- else if (!quiet)
+ else if (!quiet) {
printf(_("%s is a mountpoint\n"), spec);
+ rc = EXIT_SUCCESS;
+ }
}
- return rc ? EXIT_FAILURE : EXIT_SUCCESS;
+finish:
+ return rc;
}
--
1.7.7
^ permalink raw reply related [flat|nested] 11+ messages in thread
* RE: [PATCH 1/2] mountpoint: refactor exit path
2011-10-10 23:14 Dave Reisner
@ 2011-10-11 7:07 ` Voelker, Bernhard
2011-10-11 7:44 ` Karel Zak
2011-10-11 9:06 ` Karel Zak
1 sibling, 1 reply; 11+ messages in thread
From: Voelker, Bernhard @ 2011-10-11 7:07 UTC (permalink / raw)
To: Dave Reisner, util-linux@vger.kernel.org; +Cc: Dave Reisner
Dave Reisner wrote:
> + else if (!quiet) {
> printf(_("%s is a mountpoint\n"), spec);
> + rc = EXIT_SUCCESS;
> + }
So mountpoint will never return SUCCESS if -q is used?
> - return rc ? EXIT_FAILURE : EXIT_SUCCESS;
> +finish:
> + return rc;
IMHO 'goto' is ugly and should only be used in more difficult
cases. In this case - if you really liked to avoid several
free(spec) statements - it could be easily done with a few
else statements.
Berny
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] mountpoint: refactor exit path
2011-10-11 7:07 ` Voelker, Bernhard
@ 2011-10-11 7:44 ` Karel Zak
2011-10-11 7:55 ` Voelker, Bernhard
0 siblings, 1 reply; 11+ messages in thread
From: Karel Zak @ 2011-10-11 7:44 UTC (permalink / raw)
To: Voelker, Bernhard; +Cc: Dave Reisner, util-linux@vger.kernel.org, Dave Reisner
On Tue, Oct 11, 2011 at 09:07:15AM +0200, Voelker, Bernhard wrote:
> > +finish:
> > + return rc;
>
> IMHO 'goto' is ugly and should only be used in more difficult
> cases. In this case - if you really liked to avoid several
> free(spec) statements - it could be easily done with a few
> else statements.
kernel Documentation/CodingStyle:
Albeit deprecated by some people, the equivalent of the goto statement is
used frequently by compilers in form of the unconditional jump instruction.
The goto statement comes in handy when a function exits from multiple
locations and some common work such as cleanup has to be done.
The rationale is:
- unconditional statements are easier to understand and follow
- nesting is reduced
- errors by not updating individual exit points when making
modifications are prevented
- saves the compiler work to optimize redundant code away ;)
BTW, now you know why I don't like free-before-exit in simple utils
like mountpoint(1). It makes code less readable and needlessly
complicated.
Karel
--
Karel Zak <kzak@redhat.com>
http://karelzak.blogspot.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 1/2] mountpoint: refactor exit path
2011-10-11 7:44 ` Karel Zak
@ 2011-10-11 7:55 ` Voelker, Bernhard
2011-10-11 8:14 ` Karel Zak
0 siblings, 1 reply; 11+ messages in thread
From: Voelker, Bernhard @ 2011-10-11 7:55 UTC (permalink / raw)
To: Karel Zak; +Cc: Dave Reisner, util-linux@vger.kernel.org, Dave Reisner
Karel Zak wrote:
> On Tue, Oct 11, 2011 at 09:07:15AM +0200, Voelker, Bernhard wrote:
> > > +finish:
> > > + return rc;
> >
> > IMHO 'goto' is ugly and should only be used in more difficult
> > cases. In this case - if you really liked to avoid several
> > free(spec) statements - it could be easily done with a few
> > else statements.
> BTW, now you know why I don't like free-before-exit in simple utils
> like mountpoint(1). It makes code less readable and needlessly
> complicated.
no worries. I'm okay with it if you are. ;-)
Re free-before-exit: the problem is in tools like valgrind.
I'm not much into it, but don't they know about 'noreturn'?
Have a nice day,
Berny
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] mountpoint: refactor exit path
2011-10-11 7:55 ` Voelker, Bernhard
@ 2011-10-11 8:14 ` Karel Zak
0 siblings, 0 replies; 11+ messages in thread
From: Karel Zak @ 2011-10-11 8:14 UTC (permalink / raw)
To: Voelker, Bernhard; +Cc: Dave Reisner, util-linux@vger.kernel.org, Dave Reisner
On Tue, Oct 11, 2011 at 09:55:43AM +0200, Voelker, Bernhard wrote:
> Karel Zak wrote:
>
> > On Tue, Oct 11, 2011 at 09:07:15AM +0200, Voelker, Bernhard wrote:
> > > > +finish:
> > > > + return rc;
> > >
> > > IMHO 'goto' is ugly and should only be used in more difficult
> > > cases. In this case - if you really liked to avoid several
> > > free(spec) statements - it could be easily done with a few
> > > else statements.
>
>
> > BTW, now you know why I don't like free-before-exit in simple utils
> > like mountpoint(1). It makes code less readable and needlessly
> > complicated.
>
> no worries. I'm okay with it if you are. ;-)
>
> Re free-before-exit: the problem is in tools like valgrind.
Yes if we want to use mountpoint(1) as a test (referential)
application for libmount, otherwise it's better to keep so small
utils simple and stupid.
Karel
--
Karel Zak <kzak@redhat.com>
http://karelzak.blogspot.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] mountpoint: refactor exit path
2011-10-10 23:14 Dave Reisner
2011-10-11 7:07 ` Voelker, Bernhard
@ 2011-10-11 9:06 ` Karel Zak
2011-10-11 9:31 ` Karel Zak
1 sibling, 1 reply; 11+ messages in thread
From: Karel Zak @ 2011-10-11 9:06 UTC (permalink / raw)
To: Dave Reisner; +Cc: util-linux, Dave Reisner
On Mon, Oct 10, 2011 at 07:14:15PM -0400, Dave Reisner wrote:
> if (dev_devno)
> rc = print_devno(spec, &st);
print_devno() returns -1 on error. Fixed.
> @@ -161,20 +161,23 @@ int main(int argc, char **argv)
>
> if (!S_ISDIR(st.st_mode)) {
> if (!quiet)
> - errx(EXIT_FAILURE, _("%s: not a directory"), spec);
> - return EXIT_FAILURE;
> + warnx(_("%s: not a directory"), spec);
> + goto finish;
> }
> src = dir_to_device(spec);
> if (src == (dev_t)-1) {
> if (!quiet)
> printf(_("%s is not a mountpoint\n"), spec);
> - return EXIT_FAILURE;
> + goto finish;
> }
> if (fs_devno)
> printf("%u:%u\n", major(src), minor(src));
> - else if (!quiet)
> + else if (!quiet) {
> printf(_("%s is a mountpoint\n"), spec);
> + rc = EXIT_SUCCESS;
Fixed. (see Berny's email)
Applied, thanks.
Karel
--
Karel Zak <kzak@redhat.com>
http://karelzak.blogspot.com
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2011-10-11 9:31 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-10-09 15:12 [PATCH 1/2] mountpoint: refactor exit path Dave Reisner
2011-10-09 15:12 ` [PATCH 2/2] mountpoint: support symbolic and relative paths Dave Reisner
2011-10-10 19:45 ` [PATCH 1/2] mountpoint: refactor exit path Dave Reisner
2011-10-10 23:05 ` Karel Zak
-- strict thread matches above, loose matches on Subject: below --
2011-10-10 23:14 Dave Reisner
2011-10-11 7:07 ` Voelker, Bernhard
2011-10-11 7:44 ` Karel Zak
2011-10-11 7:55 ` Voelker, Bernhard
2011-10-11 8:14 ` Karel Zak
2011-10-11 9:06 ` Karel Zak
2011-10-11 9:31 ` Karel Zak
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).