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

2017-07-20 Thread Pino Toscano
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

signature.asc
Description: This is a digitally signed message part.
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs

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

2017-07-20 Thread Richard W.M. Jones

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

___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs

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

2017-07-19 Thread Pino Toscano
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

signature.asc
Description: This is a digitally signed message part.
___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs

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

2017-07-14 Thread Richard W.M. Jones
‘file’ is a small, self-contained API which runs a single command, so
it's a good test case for reimplementing APIs.
---
 daemon/Makefile.am|  2 ++
 daemon/file.c | 80 ---
 daemon/file.ml| 60 +++
 daemon/file.mli   | 19 +++
 generator/actions_core.ml |  1 +
 5 files changed, 82 insertions(+), 80 deletions(-)

diff --git a/daemon/Makefile.am b/daemon/Makefile.am
index 40b770762..6fb1c5384 100644
--- a/daemon/Makefile.am
+++ b/daemon/Makefile.am
@@ -256,6 +256,7 @@ guestfsd_CFLAGS = \
 SOURCES_MLI = \
chroot.mli \
sysroot.mli \
+   file.mli \
utils.mli
 
 SOURCES_ML = \
@@ -263,6 +264,7 @@ SOURCES_ML = \
utils.ml \
sysroot.ml \
chroot.ml \
+   file.ml \
callbacks.ml \
daemon.ml
 
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);
 GUESTFSD_EXT_CMD(str_zcat, zcat);
 GUESTFSD_EXT_CMD(str_bzcat, bzcat);
 
@@ -449,85 +448,6 @@ do_pwrite_device (const char *device, const char *content, 
size_t size,
   return pwrite_fd (fd, content, size, offset, device, 1);
 }
 
-/* This runs the 'file' command. */
-char *
-do_file (const char *path)
-{
-  CLEANUP_FREE char *buf = NULL;
-  const char *display_path = path;
-  const int is_dev = STRPREFIX (path, "/dev/");
-  struct stat statbuf;
-
-  if (!is_dev) {
-buf = sysroot_path (path);
-if (!buf) {
-  reply_with_perror ("malloc");
-  return NULL;
-}
-path = buf;
-
-/* For non-dev, check this is a regular file, else just return the
- * file type as a string (RHBZ#582484).
- */
-if (lstat (path, ) == -1) {
-  reply_with_perror ("lstat: %s", display_path);
-  return NULL;
-}
-
-if (! S_ISREG (statbuf.st_mode)) {
-  char *ret;
-
-  if (S_ISDIR (statbuf.st_mode))
-ret = strdup ("directory");
-  else if (S_ISCHR (statbuf.st_mode))
-ret = strdup ("character device");
-  else if (S_ISBLK (statbuf.st_mode))
-ret = strdup ("block device");
-  else if (S_ISFIFO (statbuf.st_mode))
-ret = strdup ("FIFO");
-  else if (S_ISLNK (statbuf.st_mode))
-ret = strdup ("symbolic link");
-  else if (S_ISSOCK (statbuf.st_mode))
-ret = strdup ("socket");
-  else
-ret = strdup ("unknown, not regular file");
-
-  if (ret == NULL)
-reply_with_perror ("strdup");
-  return ret;
-}
-  }
-
-  /* Which flags to use?  For /dev paths, follow links because
-   * /dev/VG/LV is a symbolic link.
-   */
-  const char *flags = is_dev ? "-zbsL" : "-zb";
-
-  char *out;
-  CLEANUP_FREE char *err = NULL;
-  int r = command (, , str_file, flags, path, NULL);
-
-  if (r == -1) {
-free (out);
-reply_with_error ("%s: %s", display_path, err);
-return NULL;
-  }
-
-  /* We need to remove the trailing \n from output of file(1). */
-  size_t len = strlen (out);
-  if (len > 0 && out[len-1] == '\n')
-out[--len] = '\0';
-
-  /* Some upstream versions of file add a space at the end of the
-   * output.  This is fixed in the Fedora version, but we might as
-   * well fix it here too.  (RHBZ#928995).
-   */
-  if (len > 0 && out[len-1] == ' ')
-out[--len] = '\0';
-
-  return out;  /* caller frees */
-}
-
 /* zcat | file */
 char *
 do_zfile (const char *method, const char *path)
diff --git a/daemon/file.ml b/daemon/file.ml
new file mode 100644
index 0..557de764b
--- /dev/null
+++ b/daemon/file.ml
@@ -0,0 +1,60 @@
+(* guestfs-inspection
+ * Copyright (C) 2009-2017 Red Hat Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *)
+
+open Unix
+open Printf
+
+open Std_utils
+
+open Utils
+
+(* This runs the [file] command. *)
+let file path =
+  let is_dev = String.is_prefix path "/dev/" in
+
+  (* For non-dev, check this is a regular file, else just return the
+   * file type as a string (RHBZ#582484).
+   *)
+  if not is_dev then (
+let sysroot = Sysroot.sysroot () in
+let chroot = Chroot.create sysroot ~name:(sprintf "file: %s" path) in
+
+let statbuf = Chroot.f chroot lstat path in
+