8:01 a.m.
On Thu, May 28, 2026 at 08:11:52AM -0400, Srihari Parimi wrote:
Implement bind mount to overwrite /etc/resolv.conf instead of the
current method of rename followed by copy. The bind mount method
works even if the file is designated as immutable
Signed-off-by: Srihari Parimi <sparimi(a)redhat.com>
This needs a 'Fixes: ...' line.
---
daemon/sh.c | 86 ++++++++++++++++++++++-------------------------------
1 file changed, 35 insertions(+), 51 deletions(-)
diff --git a/daemon/sh.c b/daemon/sh.c
index 74e003410..aab9a4534 100644
--- a/daemon/sh.c
+++ b/daemon/sh.c
@@ -150,8 +150,9 @@ free_bind_state (struct bind_state *bs)
static int
set_up_etc_resolv_conf (struct resolver_state *rs)
{
+ char *src_etc_resolv = "/etc/resolv.conf";
const? If the compiler didn't warn about this then you're probably
not enabling warnings. You should configure with:
./configure --enable-werror
There are similar options on all the other projects which you should
always use.
struct stat statbuf;
- CLEANUP_FREE char *buf = NULL;
+ int r, fd = -1;
Why is fd initialized here? This stops GCC from detecting variables
that are used when unset.
rs->sysroot_etc_resolv_conf_old = NULL;
@@ -161,50 +162,43 @@ set_up_etc_resolv_conf (struct resolver_state *rs)
reply_with_perror ("malloc");
goto error;
}
-
- /* If /etc/resolv.conf exists, rename it to the backup file. Note
- * that on Ubuntu it's a dangling symlink.
+ /* If /sysroot/etc/resolv.conf does not exist, create one for mount
+ * bind to succeed. lstat() call handles dnagling links
*/
- if (lstat (rs->sysroot_etc_resolv_conf, &statbuf) == 0) {
- /* Make a random name for the backup file. */
- if (asprintf (&buf, "%s/etc/XXXXXXXX", sysroot) == -1) {
- reply_with_perror ("asprintf");
- goto error;
- }
- if (random_name (buf) == -1) {
- reply_with_perror ("random_name");
- goto error;
- }
- rs->sysroot_etc_resolv_conf_old = strdup (buf);
- if (!rs->sysroot_etc_resolv_conf_old) {
- reply_with_perror ("strdup");
- goto error;
+ if (lstat (rs->sysroot_etc_resolv_conf, &statbuf) != 0) {
Using '!= 0' is almost always wrong for system calls. The man page
for lstat documents -1 as the error code, so check only that value.
Also what's the justification for lstat instead of stat?
+ if (errno != ENOENT) {
+ reply_with_perror ("lstat: %s", rs->sysroot_etc_resolv_conf);
+ return -1;
}
-
- if (verbose)
- fprintf (stderr, "renaming %s to %s\n", rs->sysroot_etc_resolv_conf,
- rs->sysroot_etc_resolv_conf_old);
-
- if (rename (rs->sysroot_etc_resolv_conf,
- rs->sysroot_etc_resolv_conf_old) == -1) {
- reply_with_perror ("rename: %s to %s", rs->sysroot_etc_resolv_conf,
- rs->sysroot_etc_resolv_conf_old);
- goto error;
+ /* create empty file */
+ fd = open (rs->sysroot_etc_resolv_conf,
+ O_WRONLY|O_CREAT|O_NOCTTY|O_CLOEXEC, 0644);
+ if (fd == -1) {
+ reply_with_perror ("open: %s", rs->sysroot_etc_resolv_conf);
+ return -1;
}
+ close(fd);
You must always check the return value from close for errors. This is
because Linux can delay certain errors until the close call
(particularly EIO).
After creating the file in the sysroot, how will it get deleted? The
code as written will create /etc/resolv.conf in random guest
filesystems.
}
-
- /* Now that the guest's <sysroot>/etc/resolv.conf is out the way, we
- * can create our own copy of the appliance /etc/resolv.conf.
- */
- ignore_value (command (NULL, NULL, "cp", "/etc/resolv.conf",
- rs->sysroot_etc_resolv_conf, NULL));
-
+ if (verbose)
+ fprintf (stderr, "mount %s to %s\n", src_etc_resolv,
+ rs->sysroot_etc_resolv_conf);
+ /* mount bind read only */
+ r = command (NULL, NULL, "mount", "--bind", "-o ro",
src_etc_resolv,
+ rs->sysroot_etc_resolv_conf, NULL);
+ if (r == -1) {
+ reply_with_perror ("mount: %s to %s failed\n",
+ src_etc_resolv, rs->sysroot_etc_resolv_conf);
+ goto error;
+ }
+ if (verbose)
+ fprintf (stderr, "mount %s to %s done\n", src_etc_resolv,
+ rs->sysroot_etc_resolv_conf);
rs->mounted = true;
+
return 0;
- error:
+error:
Here we add a blank line and reformat the label. If these changes
need to be made then they should be done in a separate commit since
they are not part of this logical change. But in this case I'd avoid
making whitespace changes altogether.
free (rs->sysroot_etc_resolv_conf);
- free (rs->sysroot_etc_resolv_conf_old);
return -1;
}
@@ -212,20 +206,10 @@ static void
free_resolver_state (struct resolver_state *rs)
{
if (rs->mounted) {
- unlink (rs->sysroot_etc_resolv_conf);
-
- if (rs->sysroot_etc_resolv_conf_old) {
- if (verbose)
- fprintf (stderr, "renaming %s to %s\n",
rs->sysroot_etc_resolv_conf_old,
- rs->sysroot_etc_resolv_conf);
-
- if (rename (rs->sysroot_etc_resolv_conf_old,
- rs->sysroot_etc_resolv_conf) == -1)
- perror ("error: could not restore /etc/resolv.conf");
-
- free (rs->sysroot_etc_resolv_conf_old);
- }
-
+ /* umount sysroot_etc_resolv_conf */
This comment says nothing. If you wanted a comment here you could
write something useful for the reader, or just leave it out.
+ if (verbose)
+ fprintf (stderr, "umount %s\n", rs->sysroot_etc_resolv_conf);
+ umount_ignore_fail(rs->sysroot_etc_resolv_conf);
Space before paren.
free (rs->sysroot_etc_resolv_conf);
rs->mounted = false;
}
--
2.52.0
The general plan is good and makes a big improvement, but the patch
needs a few specific changes.
Thanks,
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top