Libguestfs - Jul 2017 - Re: [PATCH 03/27] daemon: Reimplement ‘file’ API in OCaml.

On Friday, 14 July 2017 15:39:11 CEST Richard W.M. Jones
wrote:
> diff --git a/daemon/file.c b/daemon/file.c
> index 84874dc6f..ee79eb507 100644
> --- a/daemon/file.c
> +++ b/daemon/file.c
> @@ -30,7 +30,6 @@
>  #include "actions.h"
>  #include "optgroups.h"
>  
> -GUESTFSD_EXT_CMD(str_file, file);
When migrating to OCaml, these extra sections in the ELF are not added
anymore.  I read they were added to help packagers (IIRC it started
from the SUSE guys), so they could easily notice the binaries used by
the daemon, and add the proper dependencies.

Do we still want to keep this possibility, somehow?  If not, I'd just
do a wholesale removal of GUESTFSD_EXT_CMD.
> +  if not is_dev then (
> +    let sysroot = Sysroot.sysroot () in
> +    let chroot = Chroot.create sysroot ~name:(sprintf "file: %s"
path) in
I notice this pattern done every time, and IMHO it could be simplified:
in utils.ml(i), add something like:

  let create_chroot ?name ()     Chroot.create (Sysroot.sysroot ()) ?name

this way it can be used like:

  let chroot = create_chroot ~name:(sprintf "file: %s" path) in
> +
> +    let statbuf = Chroot.f chroot lstat path in
Hm is chroot needed for this?  The current C implementation does not
use CHROOT_IN/OUT, and it does not even resolve symlinks, so it should
be safe.

-- 
Pino Toscano

On Wed, Jul 19, 2017 at 03:14:48PM +0200, Pino Toscano
wrote:
> > +
> > +    let statbuf = Chroot.f chroot lstat path in
> 
> Hm is chroot needed for this?  The current C implementation does not
> use CHROOT_IN/OUT, and it does not even resolve symlinks, so it should
> be safe.
The implementation is different, but I think it's equivalent and safe.

The ‘Chroot’ module is a significant enhancement over the C CHROOT_*
hacks and over the cases where the C code should be doing a chroot but
doesn't even bother.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

On Thursday, 20 July 2017 09:54:51 CEST Richard W.M. Jones
wrote:
> 
> On Wed, Jul 19, 2017 at 03:14:48PM +0200, Pino Toscano wrote:
> > > +
> > > +    let statbuf = Chroot.f chroot lstat path in
> > 
> > Hm is chroot needed for this?  The current C implementation does not
> > use CHROOT_IN/OUT, and it does not even resolve symlinks, so it should
> > be safe.
> 
> The implementation is different, but I think it's equivalent and safe.
> 
> The ‘Chroot’ module is a significant enhancement over the C CHROOT_*
> hacks and over the cases where the C code should be doing a chroot but
> doesn't even bother.
Yes, I understand that Chroot is better, although my point here is that
it should not be needed, like CHROOT_* was not needed before either.
In the end the code is just stat'ing a file, without resolving it in
case it is a symlink, so not using Chroot should be still safe.

-- 
Pino Toscano