Here's one final (I hope) question without a complete patch. Given that I already ensure that stubs.c invokes REQUIRE_ROOT_OR_RESOLVE_DEVICE just before calling do_umount, does this new version of do_umount look ok? /* Again, use the external /bin/umount program, so that /etc/mtab * is kept updated. */ int do_umount (const char *pathordevice) { int r; char *err; char *buf = sysroot_path (pathordevice); if (buf == NULL) { reply_with_perror ("malloc"); return -1; } r = command (NULL, &err, "umount", buf, NULL); if (r == -1) { reply_with_error ("umount: %s: %s", pathordevice, err); free (err); free (buf); return -1; } free (err); /* update root_mounted? */ return 0; }
On Wed, Aug 12, 2009 at 10:14:51AM +0200, Jim Meyering wrote:
Here's one final (I hope) question without a complete patch. Given that I already ensure that stubs.c invokes REQUIRE_ROOT_OR_RESOLVE_DEVICE just before calling do_umount, does this new version of do_umount look ok?
/* Again, use the external /bin/umount program, so that /etc/mtab * is kept updated. */ int do_umount (const char *pathordevice) { int r; char *err;
char *buf = sysroot_path (pathordevice); if (buf == NULL) { reply_with_perror ("malloc"); return -1; }
I think the problem here is you'll end up with a buf like '/sysroot/dev/vda'. In the original code: if (strncmp (pathordevice, "/dev/", 5) == 0) { buf = pathordevice; IS_DEVICE (buf, -1); } else { buf = sysroot_path (pathordevice); if (buf == NULL) { reply_with_perror ("malloc"); return -1; } freeit = 1; } it avoids prefixing /dev paths. If Pathname args turn into two parameters (the original path, and the sysroot-prefixed path), then maybe Pathname_or_device args should turn into several parameters too, like: int is_device, const char *original_pathordevice, const char *sysroot_pathordevice (That's just an idea ... maybe not a good one) Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top
Richard W.M. Jones wrote:
On Wed, Aug 12, 2009 at 10:14:51AM +0200, Jim Meyering wrote:
Here's one final (I hope) question without a complete patch. Given that I already ensure that stubs.c invokes REQUIRE_ROOT_OR_RESOLVE_DEVICE just before calling do_umount, does this new version of do_umount look ok?
/* Again, use the external /bin/umount program, so that /etc/mtab * is kept updated. */ int do_umount (const char *pathordevice) { int r; char *err;
char *buf = sysroot_path (pathordevice); if (buf == NULL) { reply_with_perror ("malloc"); return -1; }
I think the problem here is you'll end up with a buf like '/sysroot/dev/vda'.
In the original code:
if (strncmp (pathordevice, "/dev/", 5) == 0) { buf = pathordevice; IS_DEVICE (buf, -1); } else { buf = sysroot_path (pathordevice); if (buf == NULL) { reply_with_perror ("malloc"); return -1; } freeit = 1; }
it avoids prefixing /dev paths.
If Pathname args turn into two parameters (the original path, and the sysroot-prefixed path), then maybe Pathname_or_device args should turn into several parameters too, like:
int is_device, const char *original_pathordevice, const char *sysroot_pathordevice
(That's just an idea ... maybe not a good one)
If do_umount is the only case like that (and it seems to be), a more local change is possible: if pathordevice starts with /sysroot/dev, remove the "/sysroot" part. Not terribly pretty to add the prefix, and then to remove it right afterwards, but...
On Wed, Aug 12, 2009 at 10:25:34AM +0200, Jim Meyering wrote:
If do_umount is the only case like that (and it seems to be), a more local change is possible: if pathordevice starts with /sysroot/dev, remove the "/sysroot" part.
Not terribly pretty to add the prefix, and then to remove it right afterwards, but...
Ick. Remember that "/sysroot" is not a constant. It's the variables sysroot and sysroot_len that contain the actual prefix. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones Read my programming blog: http://rwmj.wordpress.com Fedora now supports 75 OCaml packages (the OPEN alternative to F#) http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora
Richard W.M. Jones wrote:
On Wed, Aug 12, 2009 at 10:25:34AM +0200, Jim Meyering wrote:
If do_umount is the only case like that (and it seems to be), a more local change is possible: if pathordevice starts with /sysroot/dev, remove the "/sysroot" part.
Not terribly pretty to add the prefix, and then to remove it right afterwards, but...
Ick. Remember that "/sysroot" is not a constant. It's the variables sysroot and sysroot_len that contain the actual prefix.
Yes, I know ;-) Sounds like you too think this little bit of pragmatism is the lesser of two evils.
Richard W.M. Jones wrote:
On Wed, Aug 12, 2009 at 10:14:51AM +0200, Jim Meyering wrote:
Here's one final (I hope) question without a complete patch. Given that I already ensure that stubs.c invokes REQUIRE_ROOT_OR_RESOLVE_DEVICE just before calling do_umount, does this new version of do_umount look ok?
/* Again, use the external /bin/umount program, so that /etc/mtab * is kept updated. */ int do_umount (const char *pathordevice) { int r; char *err;
char *buf = sysroot_path (pathordevice); if (buf == NULL) { reply_with_perror ("malloc"); return -1; }
I think the problem here is you'll end up with a buf like '/sysroot/dev/vda'.
In the original code:
if (strncmp (pathordevice, "/dev/", 5) == 0) { buf = pathordevice; IS_DEVICE (buf, -1); } else { buf = sysroot_path (pathordevice); if (buf == NULL) { reply_with_perror ("malloc"); return -1; } freeit = 1; }
it avoids prefixing /dev paths.
No need for ugliness after all. Here's a more faithful new version: /* Again, use the external /bin/umount program, so that /etc/mtab * is kept updated. */ int do_umount (const char *pathordevice) { int r; char *err; char *buf = (strncmp (pathordevice, "/dev/", 5) == 0 ? strdup (pathordevice) : sysroot_path (pathordevice)); if (buf == NULL) { reply_with_perror ("malloc"); return -1; } r = command (NULL, &err, "umount", buf, NULL); free (buf); if (r == -1) { reply_with_error ("umount: %s: %s", pathordevice, err); free (err); return -1; } free (err); /* update root_mounted? */ return 0; }
On Wed, Aug 12, 2009 at 10:49:15AM +0200, Jim Meyering wrote:
Richard W.M. Jones wrote: No need for ugliness after all. Here's a more faithful new version:
/* Again, use the external /bin/umount program, so that /etc/mtab * is kept updated. */ int do_umount (const char *pathordevice) { int r; char *err;
char *buf = (strncmp (pathordevice, "/dev/", 5) == 0 ? strdup (pathordevice) : sysroot_path (pathordevice)); if (buf == NULL) { reply_with_perror ("malloc"); return -1; }
r = command (NULL, &err, "umount", buf, NULL); free (buf);
if (r == -1) { reply_with_error ("umount: %s: %s", pathordevice, err); free (err); return -1; }
free (err);
/* update root_mounted? */
return 0; }
This looks good, so ACK. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones New in Fedora 11: Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 70 libraries supprt'd http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw
participants (2)
-
Jim Meyering -
Richard W.M. Jones