[Libguestfs] Re: [PATCH] daemon: Use bind mount to overwrite /etc/resolv.conf

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&gt; This needs a 'Fixes: ...' line. Fixed in Patch V2

> --- > 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