Hi Markus,
Thank you for the patch. It seems you spent quite a lot of time with it.
Improvements, bug reports and bug fixes are always very much appreciated.
On Sun, 24 Feb 2008, SF Markus Elfring wrote:
> I would like to suggest updates for some source files. I see a potential
> for improvements in the areas "error handling" and "array usage" for the
> current software release. Does the appended suggestion contain acceptable
> changes?
Yes, no and maybe ;)
> -static const char *progname = "ntfs-3g-mount";
> +static const char progname[] = "ntfs-3g-mount";
Both ways have advantages and disadvantages.
I know the latter is prefered usually. The former generates somewhat
smaller text segments, so the otherwise expensive context switches (which
is the biggest bottleneck during metadata operations and on low-end CPUs,
especially on ARM where a 50x context switch speedup wasn't accepted into
the mainline kernel) could be cheaper due to less TLB misses.
I would be very surprised if somebody could measure a difference in
performance unless using a specially crafted object but as a Hungarian
proverb says "Those who don't appreciate the small wins do not deserve
the big ones". Computers architectures are discrete and non-linear.
Sometimes positive performance jump magically happens and these tiny
issues can be the explanations.
But I'm open. Why do you think the new version would be better?
> - fprintf(stderr, "%s: could not determine username\n", progname);
> + if (fprintf(stderr, "%s: could not determine username\n", progname)
> < 21) {
> + abort();
> + }
> +
Sorry, this is not ok. Two reasons:
1. abort() should never be used.
Ntfs-3g is often used as root file system (e.g. WUBI). And for
instance a temporary OOM can't justify a total system crash.
Ntfs-3g also returns a unique exist value marking the problem, so
wrappers can act accordingly.
2. fprintf() failures are typically never a fatal error.
> - fclose(fp);
> +
> + if (fclose(fp)) {
Yes, typically it's a very serious error if somebody doesn't check the
return value from the close() functions. However in all the cases you fixed
the return value can be safely ignored since it's irrelevant in the given
contexts.
I'm aware that this is not obvious for somebody who is not familiar with
the internals. Most of the code you fixed was written by Miklos Szeredi and
he very damn knows what he is doing ;)
An explicit cast is enough to silence the compiler warnings, thanks.
> - printf(stderr, "%s: failed to close %s: %s\n",
> + (void) fprintf(stderr, "%s: failed to close %s: %s\n",
This is ok but it would be worth to abstract the logging here too, just
like ntfs_log* does.
However using ntfs_log* is not ok, since fuse-lite shouldn't have
dependnecy on the ntfs library. But both could have dependency on
a logging module derived from ntfs_log* for instance.
> - umask(old_umask);
> + (void) umask(old_umask);
> return res;
Thanks.
> - execl("/bin/mount", "/bin/mount", "-i", "-f", "-t", type, "-o", opts,
> - fsname, mnt, NULL);
> - fprintf(stderr, "%s: failed to execute /bin/mount: %s\n", progname,
> - strerror(errno));
> - exit(1);
> + if (execl("/bin/mount", "/bin/mount", "-i", "-f", "-t", type, "-o",
> opts,
> + fsname, mnt, NULL) == -1) {
> + (void) fprintf(stderr, "%s: failed to execute /bin/mount: %s\n",
> + progname, strerror(errno));
> + exit(1);
> + }
Cast is better. If execl() returns then we always have a problem.
> }
> if (res == 0) {
> - setuid(geteuid());
> + if (setuid(geteuid())) {
> + abort();
> + }
Cast is enough. Failure to raise the privilege is not fatal here and things
could still work fine unprivileged.
Thanks again,
Szaka
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
ntfs-3g-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ntfs-3g-devel
