Re: [Libguestfs] [PATCH] appliance: read ID_LIKE from os-release as a fallback

[Tomáš Golembiovský]

Hi,

On Thu, 20 Jul 2017 18:09:53 +0200
Cedric Bosdonnat  wrote:

> On Thu, 2017-07-20 at 17:01 +0200, Pino Toscano wrote:
> > On Thursday, 20 July 2017 16:21:40 CEST Cédric Bosdonnat wrote:  
> > > In the appliance used to build the packages for openSUSE, os-release
> > > is super minimal and only had ID_LIKE=suse. The code setting the
> > > DISTRO variable only searches for ID variable so far, resulting in
> > > invalid packagelist on openSUSE.
> > > 
> > > This fix reads ID_LIKE as a fallback if ID contains nothing.
> > > ---
> > >  m4/guestfs_appliance.m4 | 8 +++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/m4/guestfs_appliance.m4 b/m4/guestfs_appliance.m4
> > > index fbba3373f..ce45256bc 100644
> > > --- a/m4/guestfs_appliance.m4
> > > +++ b/m4/guestfs_appliance.m4
> > > @@ -97,9 +97,15 @@ AC_MSG_CHECKING([which Linux distro for package names])
> > >  if test -f /etc/os-release; then
> > >  ( . /etc/os-release && echo $ID | tr '@<:@:lower:@:>@' 
> > > '@<:@:upper:@:>@' ) >_MESSAGE_LOG_FD
> > >  DISTRO="`. /etc/os-release && echo $ID | tr '@<:@:lower:@:>@' 
> > > '@<:@:upper:@:>@'`"
> > > +dnl when building SUSE-family packages, the OBS appliance has no ID 
> > > in os-release,
> > > +dnl only ID_LIKE set to suse. Read ID_LIKE as a fallback if no ID is 
> > > found.
> > > +if test -z "$DISTRO"; then
> > > +( . /etc/os-release && echo $ID_LIKE | tr '@<:@:lower:@:>@' 
> > > '@<:@:upper:@:>@' ) >_MESSAGE_LOG_FD
> > > +DISTRO="`. /etc/os-release && echo $ID_LIKE | tr 
> > > '@<:@:lower:@:>@' '@<:@:upper:@:>@'`"
> > > +fi  
> > 
> > NACK -- while this would be a fallback, theoretically ID_LIKE is a list
> > of distros.  
> 
> Then may be we should handle the values in the list to try to find one that 
> fits
> the values we know
> 
> > >  AS_CASE([$DISTRO],
> > >  [FEDORA | RHEL | CENTOS],[DISTRO=REDHAT],
> > > -[OPENSUSE | SLED | SLES],[DISTRO=SUSE],
> > > +[OPENSUSE | SLED | SLES | SUSE],[DISTRO=SUSE],  
> > 
> > There is no need for this, since a value of $DISTRO not matching any of
> > the cases in AS_CASE is left as-is, and SUSE is already the right value.  
> 
> OK.
> 
> > What I would do is something like the following (untested):
> > 
> > if test -f /etc/os-release; then
> >   [... get DISTRO like done now ...]
> > endif
> > if -n "$DISTRO" then
> >   : # value already found in os-release
> > elif test -f /etc/debian_version; then
> >   [... etc, like now ...]  
> 
> That doesn't change much from the current state, unless we add a case for
> ID_LIKE=suse
> 
> > This way, the lack of ID in os-release will trigger the detection using
> > the various release-like files.  WDYT?  
> 
> My problem is that openSUSE Tumbleweed (the one with the problem) doesn't have
> the SuSE-release file anymore and has a too minimalistic os-release file.

Shouldn't this be rather fixed in openSUSE then? Or is there some
rationale behind why the ID is missing?

If the idea is to distinguish Tumbleweed from Leap then that's what the
fields VARIANT and VARIANT_ID are for I believe.

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

-- 
Tomáš Golembiovský 

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

[Libguestfs] check-release success (was: Re: [PATCH] appliance: read ID_LIKE from os-release as a fallback)

[Richard Jones]

0
# XFAIL: 0
# FAIL:  0
# XPASS: 0
# ERROR: 0

make[3]: Leaving directory '/var/tmp/tmpUR3Q0s/libguestfs/php'
make[2]: Leaving directory '/var/tmp/tmpUR3Q0s/libguestfs/php'
make[1]: Leaving directory '/var/tmp/tmpUR3Q0s/libguestfs/php'
Making check in erlang
make[1]: Entering directory '/var/tmp/tmpUR3Q0s/libguestfs/erlang'
make  check-TESTS
make[2]: Entering directory '/var/tmp/tmpUR3Q0s/libguestfs/erlang'
make[3]: Entering directory '/var/tmp/tmpUR3Q0s/libguestfs/erlang'
PASS: tests/010-load.erl
PASS: run-bindtests
PASS: tests/030-config.erl
PASS: tests/070-optargs.erl
PASS: tests/060-readdir.erl
PASS: tests/050-lvcreate.erl

Testsuite summary for libguestfs 1.37.18

# TOTAL: 6
# PASS:  6
# SKIP:  0
# XFAIL: 0
# FAIL:  0
# XPASS: 0
# ERROR: 0

make[3]: Leaving directory '/var/tmp/tmpUR3Q0s/libguestfs/erlang'
make[2]: Leaving directory '/var/tmp/tmpUR3Q0s/libguestfs/erlang'
make[1]: Leaving directory '/var/tmp/tmpUR3Q0s/libguestfs/erlang'
Making check in erlang/examples
make[1]: Entering directory '/var/tmp/tmpUR3Q0s/libguestfs/erlang/examples'
make[1]: Nothing to be done for 'check'.
make[1]: Leaving directory '/var/tmp/tmpUR3Q0s/libguestfs/erlang/examples'
Making check in lua
make[1]: Entering directory '/var/tmp/tmpUR3Q0s/libguestfs/lua'
ln -sf .libs/libluaguestfs.so guestfs.so
make  check-TESTS
make[2]: Entering directory '/var/tmp/tmpUR3Q0s/libguestfs/lua'
make[3]: Entering directory '/var/tmp/tmpUR3Q0s/libguestfs/lua'
PASS: tests/015-globals.lua
PASS: tests/025-create-flags.lua
PASS: run-bindtests
PASS: tests/020-create.lua
PASS: tests/010-load.lua
PASS: tests/027-create-multiple.lua
PASS: tests/030-config.lua
PASS: tests/070-optargs.lua
PASS: tests/400-events.lua
PASS: tests/900-errors.lua
PASS: tests/060-readdir.lua
PASS: tests/050-lvcreate.lua
PASS: tests/400-progress.lua

Testsuite summary for libguestfs 1.37.18

# TOTAL: 13
# PASS:  13
# SKIP:  0
# XFAIL: 0
# FAIL:  0
# XPASS: 0
# ERROR: 0

make[3]: Leaving directory '/var/tmp/tmpUR3Q0s/libguestfs/lua'
make[2]: Leaving directory '/var/tmp/tmpUR3Q0s/libguestfs/lua'
make[1]: Leaving directory '/var/tmp/tmpUR3Q0s/libguestfs/lua'
Making check in lua/examples
make[1]: Entering directory '/var/tmp/tmpUR3Q0s/libguestfs/lua/examples'
make[1]: Nothing to be done for 'check'.
make[1]: Leaving directory '/var/tmp/tmpUR3Q0s/libguestfs/lua/examples'
Making check in gobject
make[1]: Entering directory '/var/tmp/tmpUR3Q0s/libguestfs/gobject'
make  check-am
make[2]: Entering directory '/var/tmp/tmpUR3Q0s/libguestfs/gobject'
make  check-TESTS
make[3]: Entering directory '/var/tmp/tmpUR3Q0s/libguestfs/gobject'
make[4]: Entering directory '/var/tmp/tmpUR3Q0s/libguestfs/gobject'
SKIP: run-tests
PASS: run-tests-retvalues
PASS: run-live-tests

Testsuite summary for libguestfs 1.37.18

# TOTAL: 3
# PASS:  2
# SKIP:  1
# XFAIL: 0
# FAIL:  0
# XPASS: 0
# ERROR: 0

make[4]: Leaving directory '/var/tmp/tmpUR3Q0s/libguestfs/gobject'
make[3]: Leaving directory '/var/tmp/tmpUR3Q0s/libguestfs/gobject'
make[2]: Leaving directory '/var/tmp/tmpUR3Q0s/libguestfs/gobject'
make[1]: Leaving directory '/var/tmp/tmpUR3Q0s/libguestfs/gobject'
Making check in csharp
make[1]: Entering directory '/var/tmp/tmpUR3Q0s/libguestfs/csharp'
make[1]: Nothing to be done for 'check'.
make[1]: Leaving directory '/var/tmp/tmpUR3Q0s/libguestfs/csharp'
Making check in common/mlutils
make[1]: Entering directory '/var/tmp/tmpUR3Q0s/libguestfs/common/mlutils'
make  c_utils_unit_tests
make[2]: Entering directory '/var/tmp/tmpUR3Q0s/libguestfs/common/mlutils'
  CC   c_utils_unit_tests-dummy.o
  OCAMLOPT c_utils_unit_tests.cmx
  GEN  c_utils_unit_tests
make[2]: Leaving directory '/var/tmp/tmpUR3Q0s/libguestfs/common/mlutils'
make  check-TESTS
make[2]: Entering directory '/var/tmp/tmpUR3Q0s/libguestfs/common/mlutils'
make[3]: Entering directory '/var/tmp/tmpUR3Q0s/libguestfs/common/mlutils'
PASS: c_utils_unit_tests

Testsuite summary for libguestfs 1.37.18

# TOTAL: 1
# PASS:  1
# SKIP:  0
# XFAIL: 0
# FAIL:  0
# XPASS: 0
# ERROR: 0

Re: [Libguestfs] [PATCH] appliance: read ID_LIKE from os-release as a fallback

[Cedric Bosdonnat]

On Thu, 2017-07-20 at 17:01 +0200, Pino Toscano wrote:
> On Thursday, 20 July 2017 16:21:40 CEST Cédric Bosdonnat wrote:
> > In the appliance used to build the packages for openSUSE, os-release
> > is super minimal and only had ID_LIKE=suse. The code setting the
> > DISTRO variable only searches for ID variable so far, resulting in
> > invalid packagelist on openSUSE.
> > 
> > This fix reads ID_LIKE as a fallback if ID contains nothing.
> > ---
> >  m4/guestfs_appliance.m4 | 8 +++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/m4/guestfs_appliance.m4 b/m4/guestfs_appliance.m4
> > index fbba3373f..ce45256bc 100644
> > --- a/m4/guestfs_appliance.m4
> > +++ b/m4/guestfs_appliance.m4
> > @@ -97,9 +97,15 @@ AC_MSG_CHECKING([which Linux distro for package names])
> >  if test -f /etc/os-release; then
> >  ( . /etc/os-release && echo $ID | tr '@<:@:lower:@:>@' 
> > '@<:@:upper:@:>@' ) >_MESSAGE_LOG_FD
> >  DISTRO="`. /etc/os-release && echo $ID | tr '@<:@:lower:@:>@' 
> > '@<:@:upper:@:>@'`"
> > +dnl when building SUSE-family packages, the OBS appliance has no ID in 
> > os-release,
> > +dnl only ID_LIKE set to suse. Read ID_LIKE as a fallback if no ID is 
> > found.
> > +if test -z "$DISTRO"; then
> > +( . /etc/os-release && echo $ID_LIKE | tr '@<:@:lower:@:>@' 
> > '@<:@:upper:@:>@' ) >_MESSAGE_LOG_FD
> > +DISTRO="`. /etc/os-release && echo $ID_LIKE | tr '@<:@:lower:@:>@' 
> > '@<:@:upper:@:>@'`"
> > +fi
> 
> NACK -- while this would be a fallback, theoretically ID_LIKE is a list
> of distros.

Then may be we should handle the values in the list to try to find one that fits
the values we know

> >  AS_CASE([$DISTRO],
> >  [FEDORA | RHEL | CENTOS],[DISTRO=REDHAT],
> > -[OPENSUSE | SLED | SLES],[DISTRO=SUSE],
> > +[OPENSUSE | SLED | SLES | SUSE],[DISTRO=SUSE],
> 
> There is no need for this, since a value of $DISTRO not matching any of
> the cases in AS_CASE is left as-is, and SUSE is already the right value.

OK.

> What I would do is something like the following (untested):
> 
> if test -f /etc/os-release; then
>   [... get DISTRO like done now ...]
> endif
> if -n "$DISTRO" then
>   : # value already found in os-release
> elif test -f /etc/debian_version; then
>   [... etc, like now ...]

That doesn't change much from the current state, unless we add a case for
ID_LIKE=suse

> This way, the lack of ID in os-release will trigger the detection using
> the various release-like files.  WDYT?

My problem is that openSUSE Tumbleweed (the one with the problem) doesn't have
the SuSE-release file anymore and has a too minimalistic os-release file.

--
Cedric

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

Re: [Libguestfs] [PATCH v9 04/11] daemon: Implement umount_all in OCaml.

[Pino Toscano]

On Monday, 17 July 2017 18:55:24 CEST Richard W.M. Jones wrote:
> Unlike previous ‘daemon: Reimplement ...’ patches, this does not
> reimplement the umount_all API completely (yet, but this
> implementation could be completed in future and then replace the C
> one).  However it is necessary to have a version of umount_all which
> we can call from the OCaml inspection code.
> ---

Wouldn't it be easier to wrap the C implementation, for now?  This way
there is no functionality mismatch, and it can be used in both C and
OCaml snippets.

-- 
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 23/27] daemon: Reimplement ‘md_detail’ API in OCaml.

[Pino Toscano]

On Friday, 14 July 2017 15:39:31 CEST Richard W.M. Jones wrote:
> +let md_detail md =
> +  let out = command "mdadm" ["-D"; "--export"; md] in
> +
> +  (* Split the command output into lines. *)
> +  let out = String.trim out in
> +  let lines = String.nsplit "\n" out in

If here we do:

  let lines = List.filter ((<>) "") lines in

then later on we can use List.map instead of filter_map.

-- 
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 20/27] daemon: Reimplement ‘part_list’ API in OCaml.

[Pino Toscano]

On Friday, 14 July 2017 15:39:28 CEST Richard W.M. Jones wrote:
> +let print_partition_table ~add_m_option device =
> +  udev_settle ();
> +
> +  let args = ref [] in
> +  if add_m_option then push_back args "-m";
> +  push_back args "-s";
> +  push_back args "--";
> +  push_back args device;
> +  push_back args "unit";
> +  push_back args "b";
> +  push_back args "print";
> +
> +  let out =
> +try command "parted" !args
> +with
> +  (* Translate "unrecognised disk label" into an errno code. *)
> +  Failure str when String.find str "unrecognised disk label" >= 0 ->
> +raise (Unix.Unix_error (Unix.EINVAL, "parted", device ^ ": " ^ str)) 
> in
> +
> +  udev_settle ();
> +
> +  (* Split the output into lines. *)
> +  let out = String.trim out in
> +  let lines = String.nsplit "\n" out in
> +
> +  (* lines[0] is "BYT;", lines[1] is the device line which we ignore,
> +   * lines[2..] are the partitions themselves.
> +   *)
> +  match lines with
> +  | "BYT;" :: _ :: lines -> lines
> +  | [] | [_] ->
> + failwith "too few rows of output from 'parted print' command"
> +  | _ ->
> + failwith "did not see 'BYT;' magic value in 'parted print' command"

Note the first two lines with "BYT;", and the device name must be
filtered only when running in machine-parseable mode, otherwise the
match above will fail (since the output is very different in
non-machine-parseable mode).

The other option is making this function always use -m, and implement a
separate print_partition_table only in case part_get_mbr_part_type is
ported to OCaml (since it's the single user of it).

> diff --git a/daemon/parted.mli b/daemon/parted.mli
> index 33eb6d30d..057d7e8c7 100644
> --- a/daemon/parted.mli
> +++ b/daemon/parted.mli
> @@ -16,4 +16,12 @@
>   * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
>   *)
>  
> +type partition = {
> +  part_num : int32;
> +  part_start : int64;
> +  part_end : int64;
> +  part_size : int64;
> +}

Is this needed? Could Structs.partition be used below?

-- 
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 19/27] daemon: Reimplement ‘list_filesystems’ API in the daemon, in OCaml.

[Pino Toscano]

On Friday, 14 July 2017 15:39:27 CEST Richard W.M. Jones wrote:
> +let rec list_filesystems () =
> +  let has_lvm2 = Lvm.available () in
> +  let has_ldm = Ldm.available () in
> +
> +  let devices = Devsparts.list_devices () in
> +  let partitions = Devsparts.list_partitions () in
> +  let mds = Md.list_md_devices () in
> +
> +  (* Look to see if any devices directly contain filesystems
> +   * (RHBZ#590167).  However vfs-type will fail to tell us anything
> +   * useful about devices which just contain partitions, so we also
> +   * get the list of partitions and exclude the corresponding devices
> +   * by using part-to-dev.
> +   *)
> +  let devices = List.fold_left (
> +fun devices part ->
> +  let d = Devsparts.part_to_dev part in
> +  List.filter ((<>) d) devices
> +  ) devices partitions in

Hm it took me a couple of reading to get it -- what would you think
about the following (untested):

  module StringSet = Set.Make (String)
  ...
  let devices_of_partitions = List.fold_left (
fun set part ->
  StringSet.add part set
  ) partitions StringSet.empty in
  (* Remove  *)
  let devices = List.filter (
fun dev ->
  not (StringSet.mem dev devices_of_partitions)
  ) devices in
  let devices = devices @ partitions in

> +  (* Use vfs-type to check for filesystems on devices. *)
> +  let ret = filter_map check_with_vfs_type devices in
> +
> +  (* Use vfs-type to check for filesystems on partitions, but
> +   * ignore MBR partition type 42 used by LDM.
> +   *)
> +  let ret =
> +ret @
> +  filter_map (
> +fun part ->
> +  if not has_ldm || not (is_mbr_partition_type_42 part) then
> +check_with_vfs_type part
> +  else
> +None(* ignore type 42 *)
> +  ) partitions in

Now this will run is_mbr_partition_type_42 on devices as well, in case
ldm is available -- I guess it should not be an issue?

-- 
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 18/27] daemon: Reimplement ‘btrfs_subvolume_list’ and ‘btrfs_subvolume_get_default’ in OCaml.

[Pino Toscano]

On Friday, 14 July 2017 15:39:26 CEST Richard W.M. Jones wrote:
> +(* In order to examine subvolumes, quota and other things, the btrfs
> + * filesystem has to be mounted.  However we're passed a mountable
> + * in these cases, so we must mount the filesystem.  But we cannot
> + * mount it under the sysroot, as something else might be mounted
> + * there so this function mounts the filesystem on a temporary
> + * directory and ensures it is always unmounted afterwards.
> + *)
> +let with_mounted mountable f =
> +  let tmpdir = sprintf "/tmp/%s" (String.random8 ()) in

  let tmpdir = Mkdtemp.temp_dir ~base_dir:"/tmp" "btrfs." "" in

(or even without ~base_dir, I guess the default should be fine.)
This will also avoid the mkdir calls later on.

> +  (* This is the cleanup function which is called to unmount and
> +   * remove the temporary directory.  This is called on error and
> +   * ordinary exit paths.
> +   *)
> +  let finally () =
> +ignore (Sys.command (sprintf "umount %s" (quote tmpdir)));
> +rmdir tmpdir
> +  in
> +
> +  match mountable.m_type with
> +  | MountablePath ->
> + (* This corner-case happens for Mountable_or_Path parameters, where
> +  * a path was supplied by the caller.  The path (the m_device
> +  * field) is relative to the sysroot.
> +  *)
> + f (Sysroot.sysroot () // mountable.m_device)

After using Mkdtemp.temp_dir above, "rmdir tmpdir" will be needed here.

> +
> +  | MountableDevice ->
> + protect ~finally ~f:(
> +   fun () ->
> + mkdir tmpdir 0o700;
> + ignore (command "mount" [mountable.m_device; tmpdir]);
> + f tmpdir
> + )
> +
> +  | MountableBtrfsVol subvol ->
> + protect ~finally ~f:(
> +   fun () ->
> + mkdir tmpdir 0o700;
> + ignore (command "mount" ["-o"; "subvol=" ^ subvol (* XXX quoting? 
> *);
> +  mountable.m_device; tmpdir]);
> + f tmpdir
> + )
> +
> +let re_btrfs_subvolume_list =
> +  Str.regexp ("ID[ \t]+\\([0-9]+\\).*[ \t]" ^
> +  "top level[ \t]+\\([0-9]+\\).*[ \t]" ^
> +  "path[ \t]+\\(.*\\)")

Sigh, Str does not support simple character classes like \s :( No wonder
there are at least two or three "re" OCaml modules providing sane regular
expression engines (with a less awkward syntax for captures, etc).

> +let btrfs_subvolume_list mountable =
> +  (* Execute 'btrfs subvolume list ', and split the output into lines *)
> +  let lines =
> +with_mounted mountable (
> +  fun mp -> command "btrfs" ["subvolume"; "list"; mp]
> +) in
> +  let lines = String.nsplit "\n" lines in

If here we do:

  let lines = List.filter ((<>) "") lines in

then later on we can use List.map instead of filter_map.

> diff --git a/daemon/btrfs.mli b/daemon/btrfs.mli
> new file mode 100644
> index 0..55a38e42d
> --- /dev/null
> +++ b/daemon/btrfs.mli
> @@ -0,0 +1,26 @@
> +(* 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.
> + *)
> +
> +type btrfssubvolume = {
> +  btrfssubvolume_id : int64;
> +  btrfssubvolume_top_level_id : int64;
> +  btrfssubvolume_path : string;
> +}

Is this needed? Could Structs.btrfssubvolume be used below?

-- 
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 16/27] daemon: Generate OCaml wrappers for optgroup_*_available functions.

[Pino Toscano]

On Friday, 14 July 2017 15:39:24 CEST Richard W.M. Jones wrote:
> It is sometimes useful to be able to call these from OCaml code.
> ---
>  generator/daemon.ml | 23 ++-
>  1 file changed, 22 insertions(+), 1 deletion(-)

I see in patch #19:

  external available : unit -> bool =
"guestfs_int_daemon_optgroup_lvm2_available" "noalloc"

I think it'd be better to generate daemon/optgroups.ml{,i} instead,
so there is no need for manual extern declarations.

> diff --git a/generator/daemon.ml b/generator/daemon.ml
> index fd01e5d8a..1d7461f8c 100644
> --- a/generator/daemon.ml
> +++ b/generator/daemon.ml
> @@ -976,6 +976,10 @@ let generate_daemon_optgroups_c () =
>generate_header CStyle GPLv2plus;
>  
>pr "#include \n";
> +  pr "#include \n";
> +  pr "#include \n";

Not a problem, but are they needed?

> @@ -999,7 +1003,24 @@ let generate_daemon_optgroups_c () =
>  pr "  { \"%s\", optgroup_%s_available },\n" group group
>) optgroups_names_all;
>pr "  { NULL, NULL }\n";
> -  pr "};\n"
> +  pr "};\n";
> +  pr "\n";
> +  pr "/* Wrappers so these functions can be called from OCaml code. */\n";
> +  List.iter (
> +fun group ->
> +  if not (List.mem group optgroups_retired) then (
> +pr "extern value guestfs_int_daemon_optgroup_%s_available (value);\n"
> +   group;
> +pr "\n";
> +pr "/* NB: This is a \"noalloc\" call. */\n";
> +pr "value\n";
> +pr "guestfs_int_daemon_optgroup_%s_available (value unitv)\n" group;
> +pr "{\n";
> +pr "  return Val_bool (optgroup_%s_available ());\n" group;
> +pr "}\n";
> +pr "\n"
> +  )
> +  ) optgroups_names_all

I'd use 'optgroups_names' here, that contains only non-retired optgroups
(thus avoiding the check in the iteration).

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

[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] appliance: read ID_LIKE from os-release as a fallback

[Pino Toscano]

On Thursday, 20 July 2017 16:21:40 CEST Cédric Bosdonnat wrote:
> In the appliance used to build the packages for openSUSE, os-release
> is super minimal and only had ID_LIKE=suse. The code setting the
> DISTRO variable only searches for ID variable so far, resulting in
> invalid packagelist on openSUSE.
> 
> This fix reads ID_LIKE as a fallback if ID contains nothing.
> ---
>  m4/guestfs_appliance.m4 | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/m4/guestfs_appliance.m4 b/m4/guestfs_appliance.m4
> index fbba3373f..ce45256bc 100644
> --- a/m4/guestfs_appliance.m4
> +++ b/m4/guestfs_appliance.m4
> @@ -97,9 +97,15 @@ AC_MSG_CHECKING([which Linux distro for package names])
>  if test -f /etc/os-release; then
>  ( . /etc/os-release && echo $ID | tr '@<:@:lower:@:>@' '@<:@:upper:@:>@' 
> ) >_MESSAGE_LOG_FD
>  DISTRO="`. /etc/os-release && echo $ID | tr '@<:@:lower:@:>@' 
> '@<:@:upper:@:>@'`"
> +dnl when building SUSE-family packages, the OBS appliance has no ID in 
> os-release,
> +dnl only ID_LIKE set to suse. Read ID_LIKE as a fallback if no ID is 
> found.
> +if test -z "$DISTRO"; then
> +( . /etc/os-release && echo $ID_LIKE | tr '@<:@:lower:@:>@' 
> '@<:@:upper:@:>@' ) >_MESSAGE_LOG_FD
> +DISTRO="`. /etc/os-release && echo $ID_LIKE | tr '@<:@:lower:@:>@' 
> '@<:@:upper:@:>@'`"
> +fi

NACK -- while this would be a fallback, theoretically ID_LIKE is a list
of distros.

>  AS_CASE([$DISTRO],
>  [FEDORA | RHEL | CENTOS],[DISTRO=REDHAT],
> -[OPENSUSE | SLED | SLES],[DISTRO=SUSE],
> +[OPENSUSE | SLED | SLES | SUSE],[DISTRO=SUSE],

There is no need for this, since a value of $DISTRO not matching any of
the cases in AS_CASE is left as-is, and SUSE is already the right value.

What I would do is something like the following (untested):

if test -f /etc/os-release; then
  [... get DISTRO like done now ...]
endif
if -n "$DISTRO" then
  : # value already found in os-release
elif test -f /etc/debian_version; then
  [... etc, like now ...]

This way, the lack of ID in os-release will trigger the detection using
the various release-like files.  WDYT?

-- 
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] appliance: read ID_LIKE from os-release as a fallback

[Cédric Bosdonnat]

In the appliance used to build the packages for openSUSE, os-release
is super minimal and only had ID_LIKE=suse. The code setting the
DISTRO variable only searches for ID variable so far, resulting in
invalid packagelist on openSUSE.

This fix reads ID_LIKE as a fallback if ID contains nothing.
---
 m4/guestfs_appliance.m4 | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/m4/guestfs_appliance.m4 b/m4/guestfs_appliance.m4
index fbba3373f..ce45256bc 100644
--- a/m4/guestfs_appliance.m4
+++ b/m4/guestfs_appliance.m4
@@ -97,9 +97,15 @@ AC_MSG_CHECKING([which Linux distro for package names])
 if test -f /etc/os-release; then
 ( . /etc/os-release && echo $ID | tr '@<:@:lower:@:>@' '@<:@:upper:@:>@' ) 
>_MESSAGE_LOG_FD
 DISTRO="`. /etc/os-release && echo $ID | tr '@<:@:lower:@:>@' 
'@<:@:upper:@:>@'`"
+dnl when building SUSE-family packages, the OBS appliance has no ID in 
os-release,
+dnl only ID_LIKE set to suse. Read ID_LIKE as a fallback if no ID is found.
+if test -z "$DISTRO"; then
+( . /etc/os-release && echo $ID_LIKE | tr '@<:@:lower:@:>@' 
'@<:@:upper:@:>@' ) >_MESSAGE_LOG_FD
+DISTRO="`. /etc/os-release && echo $ID_LIKE | tr '@<:@:lower:@:>@' 
'@<:@:upper:@:>@'`"
+fi
 AS_CASE([$DISTRO],
 [FEDORA | RHEL | CENTOS],[DISTRO=REDHAT],
-[OPENSUSE | SLED | SLES],[DISTRO=SUSE],
+[OPENSUSE | SLED | SLES | SUSE],[DISTRO=SUSE],
 [ARCH],[DISTRO=ARCHLINUX])
 elif test -f /etc/debian_version; then
 DISTRO=DEBIAN
-- 
2.13.2

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

Re: [Libguestfs] [PATCH v7 2/9] lib: extract osinfo DB traversing API

[Cedric Bosdonnat]

On Fri, 2017-07-07 at 15:04 +0200, Pino Toscano wrote:
> On Monday, 19 June 2017 10:48:30 CEST Cédric Bosdonnat wrote:
> > Split lib/osinfo.c to provide an API for other pieces of code (namely
> > mllib) to reuse it. The ISO-related processing is thus moved into a
> > lib/osinfo-iso.c file.
> > ---
> 
> This (and patch #3) conflict with Rich's patch to deprecate the
> inspection of ISO images. So we should decide what is the fate of this
> code:
> a) move the C code as-is to virt-builder
> b) apply this (and following) patch to make it a bit more general
> c) write the osinfo-db code in OCaml, directly in virt-builder
> d) any other solution I missed above

So you mean that this code was bound to die sooner or later? Do we need
the osinfo data in the virt-builder index after all?

BTW, I'm currently rebasing the patch series on master... and it's painful.

--
Cedric

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

Re: [Libguestfs] [PATCH 02/27] daemon: Allow parts of the daemon and APIs to be written in OCaml.

[Pino Toscano]

On Wednesday, 19 July 2017 22:25:41 CEST Richard W.M. Jones wrote:
> On Wed, Jul 19, 2017 at 03:13:47PM +0200, Pino Toscano wrote:
> > On Friday, 14 July 2017 15:39:10 CEST Richard W.M. Jones wrote:
> > >  .gitignore |   6 +-
> > >  Makefile.am|   2 +-
> > >  common/mlutils/Makefile.am |   4 -
> > >  daemon/Makefile.am | 103 +++--
> > >  daemon/chroot.ml   |  85 +
> > >  daemon/chroot.mli  |  35 +
> > >  daemon/daemon-c.c  |  35 +
> > >  daemon/daemon.ml   |  39 ++
> > >  daemon/guestfsd.c  |  50 
> > >  daemon/sysroot-c.c |  37 +
> > >  daemon/sysroot.ml  |  19 +
> > >  daemon/sysroot.mli |  22 ++
> > >  daemon/utils.ml| 156 +
> > >  daemon/utils.mli   |  65 
> > 
> > TBH I'd just have a single "Daemon" module for the OCaml helpers for
> > the daemon, instead of different modules, wirh a single -c.c file for
> > all the C implementations.  The Sysroot submodule could be implemented
> > like the various submodules in Unix_utils.
> 
> Do you mean Daemon.Chroot, Daemon.Sysroot etc?

Yes, exactly.

> > > +val f : t -> ('a -> 'b) -> 'a -> 'b
> > > +(** Run a function in the chroot, returning the result or re-raising
> > > +any exception thrown. *)
> > 
> > After reading patch #11, IMHO there should be a variant that takes a
> > generic (unit -> unit) function (called 'fn', maybe?), and have 'f'
> > use it:
> > 
> >   let f t fun arg =
> > f (fun () -> fun arg)
> 
> I'm a bit confused, do you have a compilable example?

Not really without rewriting all of it, but I can improve the snippets.

An interface like:
  val f : t -> ('a -> 'b) -> 'a -> 'b
  val fn : t -> (unit -> unit) -> 'a

With the implementation like:
  let f t func arg =
fn (fun () -> func arg)
  let fn func =
...
  let ret =
try Either (func ())
with exn -> Or exn in

This way, when calling more more than a single-parameter function, it
is slightly easier to read (IMHO, of course):

  let res = Chroot.f chroot (fun () -> ...) in

than

  let res = Chroot.f chroot (fun () -> ...) () in

This is mostly syntactic sugar.

> > > +/* Convert an OCaml exception to a reply_with_error_errno call
> > > + * as best we can.
> > > + */
> > > +extern void ocaml_exn_to_reply_with_error (const char *func, value exn);
> > > +
> > > +void
> > > +ocaml_exn_to_reply_with_error (const char *func, value exn)
> > > +{
> > 
> > Shouldn't this use CAMLparam1 + CAMLreturn?
> 
> It doesn't allocate on the OCaml heap so it should be safe.
> 
> > > +let udev_settle ?filename () =
> > 
> > Ditto.
> 
> ‘Ditto’ means bind the C udev_settle_* functions?

Yes, that's correct.

-- 
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 06/27] daemon: Add unit tests of the ‘Utils’ module.

[Pino Toscano]

On Thursday, 20 July 2017 09:59:19 CEST Richard W.M. Jones wrote:
> On Thu, Jul 20, 2017 at 09:48:52AM +0200, Pino Toscano wrote:
> > On Wednesday, 19 July 2017 16:33:39 CEST Richard W.M. Jones wrote:
> > > On Wed, Jul 19, 2017 at 02:29:33PM +0200, Pino Toscano wrote:
> > > > On Wednesday, 19 July 2017 14:21:51 CEST Richard W.M. Jones wrote:
> > > > > On Wed, Jul 19, 2017 at 12:57:08PM +0200, Pino Toscano wrote:
> > > > > > Would it be possible to use oUnit too?
> > > > > 
> > > > > I'm not clear on what benefit oUnit gives us which is worth the extra
> > > > > dependency it pulls in.
> > > > 
> > > > I was referring to OUnit2, which we already have an optional dependency
> > > > for almost all the OCaml tests.
> > > 
> > > Sure, understood.  But what would we lose by converting those to
> > > simple programs that just ran a series of "assert"s?  If a single
> > > test fails we have to fix it anyway.
> > 
> > The assert means that you will just get the failure, and you will need
> > to edit the test just to see what was the actual failure.  Sure, it is
> > doable, but it adds extra steps to debugging.
> > 
> > > If the lack of oUnit2 means that other users are skipping
> > > those tests, oUnit2 may be a negative.
> > 
> > I'm not sure I follow this.  We already implemented in the past tests
> > using OUnit (then converted to OUnit2), which means we sort of agreed
> > to use a (relatively simple) unit test framework.  Most of the OCaml
> > tests use it already, so the logic that follows here would be to keep
> > using it.
> > 
> > If the problem is "this test is already sent for review as series of
> > assert", then I can volunteer to rewrite it to OUnit2.  Otherwise,
> > I'd like to know what are the reasons against using something that
> > already discussed in the past, approved, and established.
> 
> My reasoning is that oUnit2 is optional.

Sure.  OTOH, we already do use oUnit2, so these reasonings were already
discussed in the past, and (considered we have tests based on oUnit2)
deemed not a problem.

What I still don't understand is why adding a new test based on oUnit2
would be a problem, while the rest of the tests are fine as they are.

> If packagers don't have it and they run the tests then the oUnit2
> tests are skipped (see ‘if HAVE_OCAML_PKG_OUNIT’ in the makefiles).

Packagers rarely run our unit tests, since they make the build times
much longer, even on fast builders.  (Not to mention that sometimes
they use VMs, and thus this adds more issues to that.)  Even in Fedora
the general test suite is disabled, only `make quickcheck` is run.
This means it is mostly us developers (or CI) building and testing the
full test suite.

Regarding the availability: oUnit2 is available in the latest stable
versions of all the major distros, even in older versions.

> A simpler test framework which didn't use an external dependency
> wouldn't be skipped.

Surely I'm not an expert OCaml hacker, but at least to me oUnit2 does
not seem that complex, and a manually written framework would end up
reimplementing most of it, in the end.

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

[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 06/27] daemon: Add unit tests of the ‘Utils’ module.

[Richard W.M. Jones]

On Thu, Jul 20, 2017 at 09:48:52AM +0200, Pino Toscano wrote:
> On Wednesday, 19 July 2017 16:33:39 CEST Richard W.M. Jones wrote:
> > On Wed, Jul 19, 2017 at 02:29:33PM +0200, Pino Toscano wrote:
> > > On Wednesday, 19 July 2017 14:21:51 CEST Richard W.M. Jones wrote:
> > > > On Wed, Jul 19, 2017 at 12:57:08PM +0200, Pino Toscano wrote:
> > > > > Would it be possible to use oUnit too?
> > > > 
> > > > I'm not clear on what benefit oUnit gives us which is worth the extra
> > > > dependency it pulls in.
> > > 
> > > I was referring to OUnit2, which we already have an optional dependency
> > > for almost all the OCaml tests.
> > 
> > Sure, understood.  But what would we lose by converting those to
> > simple programs that just ran a series of "assert"s?  If a single
> > test fails we have to fix it anyway.
> 
> The assert means that you will just get the failure, and you will need
> to edit the test just to see what was the actual failure.  Sure, it is
> doable, but it adds extra steps to debugging.
> 
> > If the lack of oUnit2 means that other users are skipping
> > those tests, oUnit2 may be a negative.
> 
> I'm not sure I follow this.  We already implemented in the past tests
> using OUnit (then converted to OUnit2), which means we sort of agreed
> to use a (relatively simple) unit test framework.  Most of the OCaml
> tests use it already, so the logic that follows here would be to keep
> using it.
> 
> If the problem is "this test is already sent for review as series of
> assert", then I can volunteer to rewrite it to OUnit2.  Otherwise,
> I'd like to know what are the reasons against using something that
> already discussed in the past, approved, and established.

My reasoning is that oUnit2 is optional.  If packagers don't have
it and they run the tests then the oUnit2 tests are skipped
(see ‘if HAVE_OCAML_PKG_OUNIT’ in the makefiles).

A simpler test framework which didn't use an external dependency
wouldn't be skipped.

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

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

Re: [Libguestfs] [PATCH 06/27] daemon: Add unit tests of the ‘Utils’ module.

[Pino Toscano]

On Wednesday, 19 July 2017 16:33:39 CEST Richard W.M. Jones wrote:
> On Wed, Jul 19, 2017 at 02:29:33PM +0200, Pino Toscano wrote:
> > On Wednesday, 19 July 2017 14:21:51 CEST Richard W.M. Jones wrote:
> > > On Wed, Jul 19, 2017 at 12:57:08PM +0200, Pino Toscano wrote:
> > > > Would it be possible to use oUnit too?
> > > 
> > > I'm not clear on what benefit oUnit gives us which is worth the extra
> > > dependency it pulls in.
> > 
> > I was referring to OUnit2, which we already have an optional dependency
> > for almost all the OCaml tests.
> 
> Sure, understood.  But what would we lose by converting those to
> simple programs that just ran a series of "assert"s?  If a single
> test fails we have to fix it anyway.

The assert means that you will just get the failure, and you will need
to edit the test just to see what was the actual failure.  Sure, it is
doable, but it adds extra steps to debugging.

> If the lack of oUnit2 means that other users are skipping
> those tests, oUnit2 may be a negative.

I'm not sure I follow this.  We already implemented in the past tests
using OUnit (then converted to OUnit2), which means we sort of agreed
to use a (relatively simple) unit test framework.  Most of the OCaml
tests use it already, so the logic that follows here would be to keep
using it.

If the problem is "this test is already sent for review as series of
assert", then I can volunteer to rewrite it to OUnit2.  Otherwise,
I'd like to know what are the reasons against using something that
already discussed in the past, approved, and established.

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