[Libguestfs] [PATCH v4] windows: delay installation of qemu-ga MSI

2020-03-05 Thread Tomáš Golembiovský
Instead of running firstboot script during early boot schedule a task
delayed for 2 minutes.

During the first boot, after virt-v2v conversion, Windows installs the
drivers injected by virt-v2v. When this installation is finished
Windows enforces some kind of internal reboot. This unfortunately
terminates any running firstboot scripts thus killing the installation
of qemu-ga MSI.

This is just a best-effort mitigation. It can still happen (e.g. with
slow disk drives) that the drivers are not yet installed when the
delayed installation starts. On the other hand we cannot delay it too
much otherwise we risk that the users logs in and will be doing some
work when the MSI installation starts. After MSI installation finishes
the VM needs to be rebooted which would be annoying if that would happen
under users hands. Although this is not a best fix (that may come later
as it is more complex, e.g. introducing waiting mechanism), the delay as
it is defined works in most cases. And it dramaticaly improves the
situations -- originaly I experienced more than 90% failure rate.

Signed-off-by: Tomáš Golembiovský 
---
 common |  2 +-
 v2v/convert_windows.ml | 17 +++--
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/common b/common
index ea10827b..5371257c 16
--- a/common
+++ b/common
@@ -1 +1 @@
-Subproject commit ea10827b4cfb3cfe5f782421c01d2902e5f73f90
+Subproject commit 5371257c3cf27fb09d5f2e31ba378b0e6ccf5df6
diff --git a/v2v/convert_windows.ml b/v2v/convert_windows.ml
index 0fda1d4e..9b90f611 100644
--- a/v2v/convert_windows.ml
+++ b/v2v/convert_windows.ml
@@ -428,16 +428,13 @@ popd
  and configure_qemu_ga files =
List.iter (
  fun msi_path ->
-   let fb_script = "\
-echo Installing qemu-ga from " ^ msi_path ^ "
-\"\\" ^ msi_path ^ "\" /norestart /qn /l+*vx \"%~dpn0.log\"
-set elvl=!errorlevel!
-echo Done installing qemu-ga error_level=!elvl!
-if !elvl! == 0 (
-  echo Restarting Windows...
-  shutdown /r /f /c \"rebooted by firstboot script\"
-)
-" in
+   let fb_script = sprintf "\
+echo Removing any previously scheduled qemu-ga installation
+schtasks.exe /Delete /TN Firstboot-qemu-ga /F
+echo Scheduling delayed installation of qemu-ga from %s
+powershell.exe -command \"$d = (get-date).AddSeconds(120); schtasks.exe 
/Create /SC ONCE /ST $d.ToString('HH:mm') /SD $d.ToString('MM/dd/') /RU 
SYSTEM /TN Firstboot-qemu-ga /TR \\\"C:\\%s /forcerestart /qn /l+*vx 
C:\\%s.log\\\"\"
+  "
+  msi_path msi_path msi_path in
   Firstboot.add_firstboot_script g inspect.i_root
 ("install " ^ msi_path) fb_script;
 ) files
-- 
2.25.0


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

[Libguestfs] [PATCH v2 3/4] appliance: Pass root=UUID= instead of appliance device name (RHBZ#1804207).

2020-03-05 Thread Richard W.M. Jones
Appliance device names are not reliable since the kernel no longer
enumerates virtio-scsi devices serially.  Instead get the UUID of the
appliance and pass this as the parameter.

Note this requires supermin >= 5.1.18 (from around July 2017).
---
 docs/guestfs-building.pod |  2 +-
 lib/appliance-kcmdline.c  | 57 ++-
 lib/guestfs-internal.h|  2 +-
 lib/launch-direct.c   | 34 +--
 lib/launch-libvirt.c  | 19 +++--
 m4/guestfs-appliance.m4   |  3 ++-
 6 files changed, 67 insertions(+), 50 deletions(-)

diff --git a/docs/guestfs-building.pod b/docs/guestfs-building.pod
index 005626515..8e3da1d32 100644
--- a/docs/guestfs-building.pod
+++ b/docs/guestfs-building.pod
@@ -83,7 +83,7 @@ I.
 I.  The following features must be enabled:
 C, C, C, C.
 
-=item supermin E 5.1.0
+=item supermin E 5.1.18
 
 I.  For alternatives, see L
 below.
diff --git a/lib/appliance-kcmdline.c b/lib/appliance-kcmdline.c
index 8eb4999af..211cc4687 100644
--- a/lib/appliance-kcmdline.c
+++ b/lib/appliance-kcmdline.c
@@ -26,8 +26,10 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "c-ctype.h"
+#include "ignore-value.h"
 
 #include "guestfs.h"
 #include "guestfs-internal.h"
@@ -54,15 +56,53 @@
 #define EARLYPRINTK "earlyprintk=pl011,0x900"
 #endif
 
+COMPILE_REGEXP (re_uuid, "UUID=([-0-9a-f]+)", 0)
+
+static void
+read_uuid (guestfs_h *g, void *retv, const char *line, size_t len)
+{
+  char **ret = retv;
+
+  *ret = match1 (g, line, re_uuid);
+}
+
+/**
+ * Given a disk image containing an extX filesystem, return the UUID.
+ * The L command does the hard work.
+ */
+static char *
+get_root_uuid (guestfs_h *g, const char *appliance)
+{
+  CLEANUP_CMD_CLOSE struct command *cmd = guestfs_int_new_command (g);
+  char *ret = NULL;
+  int r;
+
+  guestfs_int_cmd_add_arg (cmd, "file");
+  guestfs_int_cmd_add_arg (cmd, "--");
+  guestfs_int_cmd_add_arg (cmd, appliance);
+  guestfs_int_cmd_set_stdout_callback (cmd, read_uuid, &ret, 0);
+  r = guestfs_int_cmd_run (cmd);
+  if (r == -1) {
+if (ret) free (ret);
+return NULL;
+  }
+  if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) {
+guestfs_int_external_command_failed (g, r, "file", NULL);
+if (ret) free (ret);
+return NULL;
+  }
+
+  return ret;
+}
+
 /**
  * Construct the Linux command line passed to the appliance.  This is
  * used by the C and C backends, and is simply
  * located in this file because it's a convenient place for this
  * common code.
  *
- * The C parameter must be the full device name of the
- * appliance disk and must have already been adjusted to take into
- * account virtio-blk or virtio-scsi; eg C.
+ * The C parameter is the filename of the appliance
+ * (could be NULL) from which we obtain the root UUID.
  *
  * The C parameter can contain the following flags logically
  * or'd together (or 0):
@@ -80,7 +120,8 @@
  * be freed by the caller.
  */
 char *
-guestfs_int_appliance_command_line (guestfs_h *g, const char *appliance_dev,
+guestfs_int_appliance_command_line (guestfs_h *g,
+const char *appliance,
int flags)
 {
   CLEANUP_FREE_STRINGSBUF DECLARE_STRINGSBUF (argv);
@@ -164,8 +205,12 @@ guestfs_int_appliance_command_line (guestfs_h *g, const 
char *appliance_dev,
   guestfs_int_add_string (g, &argv, "8250.nr_uarts=1");
 
   /* Tell supermin about the appliance device. */
-  if (appliance_dev)
-guestfs_int_add_sprintf (g, &argv, "root=%s", appliance_dev);
+  if (appliance) {
+CLEANUP_FREE char *uuid = get_root_uuid (g, appliance);
+if (!uuid)
+  return NULL;
+guestfs_int_add_sprintf (g, &argv, "root=UUID=%s", uuid);
+  }
 
   /* SELinux - deprecated setting, never worked and should not be enabled. */
   if (g->selinux)
diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h
index 8c6affa54..fe761a784 100644
--- a/lib/guestfs-internal.h
+++ b/lib/guestfs-internal.h
@@ -688,7 +688,7 @@ extern int guestfs_int_build_appliance (guestfs_h *g, char 
**kernel, char **init
 const char *guestfs_int_get_cpu_model (int kvm);
 
 /* appliance-kcmdline.c */
-extern char *guestfs_int_appliance_command_line (guestfs_h *g, const char 
*appliance_dev, int flags);
+extern char *guestfs_int_appliance_command_line (guestfs_h *g, const char 
*appliance, int flags);
 #define APPLIANCE_COMMAND_LINE_IS_TCG 1
 
 /* appliance-uefi.c */
diff --git a/lib/launch-direct.c b/lib/launch-direct.c
index 1718041a5..2de57ea32 100644
--- a/lib/launch-direct.c
+++ b/lib/launch-direct.c
@@ -61,8 +61,6 @@ struct backend_direct_data {
   char guestfsd_sock[UNIX_PATH_MAX]; /* Path to daemon socket. */
 };
 
-static char *make_appliance_dev (guestfs_h *g);
-
 static char *
 create_cow_overlay_direct (guestfs_h *g, void *datav, struct drive *drv)
 {
@@ -382,7 +380,6 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
   int uefi_flags;
   CLEANUP_FREE char *kernel = NULL, *initrd = NULL, *appliance = NULL;
   

[Libguestfs] [PATCH v2 2/4] daemon: Print device names when they are translated.

2020-03-05 Thread Richard W.M. Jones
This helps to debug problems with the new device name translation
code.  We can think about removing this later when the code is known
to work well.
---
 daemon/device-name-translation.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/daemon/device-name-translation.c b/daemon/device-name-translation.c
index 30173f5c2..b17f3c683 100644
--- a/daemon/device-name-translation.c
+++ b/daemon/device-name-translation.c
@@ -195,6 +195,10 @@ device_name_translation (const char *device)
   return NULL;
   }
 
+  /* If the device name is different, print the translation. */
+  if (STRNEQ (device, ret))
+fprintf (stderr, "device name translated: %s -> %s\n", device, ret);
+
   /* Now check the device is openable. */
   fd = open (ret, O_RDONLY|O_CLOEXEC);
   if (fd >= 0) {
@@ -281,5 +285,9 @@ reverse_device_name_translation (const char *device)
 }
   }
 
+  /* If the device name is different, print the translation. */
+  if (STRNEQ (device, ret))
+fprintf (stderr, "reverse device name translated: %s -> %s\n", device, 
ret);
+
   return ret;
 }
-- 
2.25.0

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



[Libguestfs] [PATCH v2 0/4] daemon: Translate device names if Linux device is unstable (RHBZ#1804207).

2020-03-05 Thread Richard W.M. Jones
v1 was here:
https://www.redhat.com/archives/libguestfs/2020-February/msg00220.html

This patch series is a little bit better.  It's still a bit of a hack.
The _real_ fix for this is outlined in the TODO file (see patch 1) but
that requires a lot more work than we could do before 1.42 is
released, unless we delay 1.42 for a lot longer.  I'm hoping with this
to have something which works for 1.42, and then fix it properly in
1.43.

Patch 2 is a debugging patch.  It's separated out because we might
consider reverting it in future once we are confident that the
approach is working.

Patch 3 is the second change necessary to how we launch the supermin
appliance.  This actually stands alone so could be reviewed
separately.  It requires a modest update to our baseline supermin
version.

Patch 4 should be ignored, it's just for my (additional) debugging.

Rich.


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



[Libguestfs] [PATCH v2 1/4] daemon: Translate device names if Linux device ordering is unstable (RHBZ#1804207).

2020-03-05 Thread Richard W.M. Jones
Linux from around 5.6 now enumerates individual disks in any order
(whereas previously it enumerated only drivers in parallel).  This
means that /dev/sdX ordering is no longer stable - in particular we
cannot be sure that /dev/sda inside the guest is the first disk that
was attached to the appliance, /dev/sdb the second disk and so on.

However we can still use SCSI PCI device numbering as found in
/dev/disk/by-path.  Use this to translate device names in and out of
the appliance.

Thanks: Vitaly Kuznetsov, Paolo Bonzini, Dan Berrangé.
---
 TODO |  13 +++
 daemon/daemon.h  |   2 +
 daemon/device-name-translation.c | 195 +--
 daemon/devsparts.ml  |  23 +++-
 daemon/guestfsd.c|   5 +
 daemon/stubs-macros.h|  10 +-
 lib/canonical-name.c |   5 +
 7 files changed, 235 insertions(+), 18 deletions(-)

diff --git a/TODO b/TODO
index 066f633d4..064386ac2 100644
--- a/TODO
+++ b/TODO
@@ -189,6 +189,19 @@ might be replaced by direct use of the library (if this is 
easier).
 But it is very hard to be compatible between RHEL6 and RHEL5 when
 using the library directly.
 
+Properly fix device name translation
+
+
+Currently for Device parameters we pass a string name like "/dev/sda"
+or "/dev/sdb2" or "/dev/VG/LV" to the daemon.  Since Linux broke
+device enumeration (RHBZ#1804207) this works less well, requiring
+non-trivial translation within the daemon.
+
+It would be far better if in the generator we mapped "/dev/XXX" to a
+proper structure.  Device index + partition | LV | MD | ...
+This would be then used everywhere inside the daemon and mean we would
+no longer need device name translation at all.
+
 Visualization
 -
 
diff --git a/daemon/daemon.h b/daemon/daemon.h
index 170fb2537..24cf8585d 100644
--- a/daemon/daemon.h
+++ b/daemon/daemon.h
@@ -215,6 +215,8 @@ extern void notify_progress_no_ratelimit (uint64_t 
position, uint64_t total, con
 
 /* device-name-translation.c */
 extern char *device_name_translation (const char *device);
+extern void device_name_translation_init (void);
+extern void device_name_translation_free (void);
 extern char *reverse_device_name_translation (const char *device);
 
 /* stubs.c (auto-generated) */
diff --git a/daemon/device-name-translation.c b/daemon/device-name-translation.c
index ed826bbae..30173f5c2 100644
--- a/daemon/device-name-translation.c
+++ b/daemon/device-name-translation.c
@@ -27,12 +27,125 @@
 #include 
 #include 
 #include 
+#include 
+#include 
+
+#include "c-ctype.h"
 
 #include "daemon.h"
 
+static char **cache;
+static size_t cache_size;
+
+/**
+ * Cache daemon disk mapping.
+ *
+ * When the daemon starts up, populate a cache with the contents
+ * of /dev/disk/by-path.  It's easiest to use C here
+ * since the names are sorted awkwardly.
+ */
+void
+device_name_translation_init (void)
+{
+  const char *by_path = "/dev/disk/by-path";
+  CLEANUP_FREE char *out = NULL, *err = NULL;
+  CLEANUP_FREE_STRING_LIST char **lines = NULL;
+  size_t i, j;
+  int r;
+
+  device_name_translation_free ();
+
+  r = command (&out, &err, "ls", "-1v", by_path, NULL);
+  if (r == -1)
+error (EXIT_FAILURE, 0,
+   "failed to initialize device name translation cache: %s", err);
+
+  lines = split_lines (out);
+  if (lines == NULL)
+error (EXIT_FAILURE, errno, "split_lines");
+
+  cache_size = guestfs_int_count_strings (lines);
+  cache = calloc (cache_size, sizeof (char *));
+  if (cache == NULL)
+error (EXIT_FAILURE, errno, "calloc");
+
+  /* Delete entries for partitions.  (Mark them as NULL in the array
+   * and skip them below.)
+   */
+  for (i = 0; i < cache_size; ++i) {
+if (strstr (lines[i], "-part"))
+  lines[i] = NULL;
+  }
+
+  /* Look up each device name.  It should be a symlink to /dev/sdX. */
+  for (i = j = 0; i < cache_size; ++i) {
+if (lines[i] != NULL) {
+  CLEANUP_FREE char *full;
+  char *device;
+
+  if (asprintf (&full, "%s/%s", by_path, lines[i]) == -1)
+error (EXIT_FAILURE, errno, "asprintf");
+
+  device = realpath (full, NULL);
+  if (device == NULL)
+error (EXIT_FAILURE, errno, "realpath: %s", full);
+
+  /* Don't add the root device to the cache. */
+  if (is_root_device (device))
+continue;
+
+  cache[j++] = device;
+}
+  }
+
+  /* This is the final cache size because we deleted entries above. */
+  cache_size = j;
+}
+
+void
+device_name_translation_free (void)
+{
+  size_t i;
+
+  for (i = 0; i < cache_size; ++i)
+free (cache[i]);
+  free (cache);
+  cache = NULL;
+  cache_size = 0;
+}
+
 /**
  * Perform device name translation.
  *
+ * Libguestfs defines a few standard formats for device names.
+ * (see also L and
+ * L).  They are:
+ *
+ * =over 4
+ *
+ * =item F
+ *
+ * =item F
+ *
+ * =item F
+ *
+ * These mean the Nth partition on the Xth device.  Because
+ * Linux no longer enumera

[Libguestfs] [PATCH v2 4/4] DEBUG - DROP THIS PATCH LATER

2020-03-05 Thread Richard W.M. Jones
---
 daemon/device-name-translation.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/daemon/device-name-translation.c b/daemon/device-name-translation.c
index b17f3c683..39266fdd7 100644
--- a/daemon/device-name-translation.c
+++ b/daemon/device-name-translation.c
@@ -100,6 +100,12 @@ device_name_translation_init (void)
 
   /* This is the final cache size because we deleted entries above. */
   cache_size = j;
+
+  /* XXX */
+  for (i = 0; i < cache_size; ++i) {
+fprintf (stderr, "device name translation cache: %zu -> %s\n",
+ i, cache[i]);
+  }
 }
 
 void
@@ -180,6 +186,8 @@ device_name_translation (const char *device)
 strcpy (dev, start);
 dev[len] = '\0';
 
+fprintf (stderr, "DEVICE=%s START=%s LEN=%zu\n", device, start, len);
+
 i = guestfs_int_drive_index (dev);
 if (i >= 0 && i < (ssize_t) cache_size) {
   /* Append the partition name if present. */
@@ -273,6 +281,8 @@ reverse_device_name_translation (const char *device)
 reply_with_perror ("asprintf");
 return NULL;
   }
+  fprintf (stderr, "REVERSE %s -> %s, index %zu, cache[%zu]=%s, len %zu\n",
+   device, ret, i, i, cache[i], len);
   break;
 }
   }
-- 
2.25.0

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



Re: [Libguestfs] [PATCH v3] windows: delay installation of qemu-ga MSI

2020-03-05 Thread Richard W.M. Jones
On Thu, Mar 05, 2020 at 12:47:58PM +0100, Tomáš Golembiovský wrote:
> On Thu, Mar 05, 2020 at 08:49:04AM +, Richard W.M. Jones wrote:
> > On Tue, Mar 03, 2020 at 03:45:49PM +0100, Tomáš Golembiovský wrote:
> > > Instead of running firstboot script during early boot schedule a task
> > > delayed for 2 minutes.
> > > 
> > > During the first boot, after virt-v2v conversion, Windows installs the
> > > drivers injected by virt-v2v. When this installation is finished
> > > Windows enforces some kind of internal reboot. This unfortunately
> > > terminates any running firstboot scripts thus killing the installation
> > > of qemu-ga MSI.
> > > 
> > > This is just a best-effort mitigation. It can still happen (e.g. with
> > > slow disk drives) that the drivers are not yet installed when the
> > > delayed installation starts. On the other hand we cannot delay it too
> > > much otherwise we risk that the users logs in and will be doing some
> > > work when the MSI installation starts. After MSI installation finishes
> > > the VM needs to be rebooted which would be annoying if that would happen
> > > under users hands. Although this is not a best fix (that may come later
> > > as it is more complex, e.g. introducing waiting mechanism), the delay as
> > > it is defined works in most cases. And it dramaticaly improves the
> > > situations -- originaly I experienced more than 90% failure rate.
> > > 
> > > Signed-off-by: Tomáš Golembiovský 
> > > ---
> > >  common |  2 +-
> > >  v2v/convert_windows.ml | 12 
> > >  2 files changed, 5 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/common b/common
> > > index ea10827b..5371257c 16
> > > --- a/common
> > > +++ b/common
> > > @@ -1 +1 @@
> > > -Subproject commit ea10827b4cfb3cfe5f782421c01d2902e5f73f90
> > > +Subproject commit 5371257c3cf27fb09d5f2e31ba378b0e6ccf5df6
> > > diff --git a/v2v/convert_windows.ml b/v2v/convert_windows.ml
> > > index 0fda1d4e..bed5989a 100644
> > > --- a/v2v/convert_windows.ml
> > > +++ b/v2v/convert_windows.ml
> > > @@ -429,14 +429,10 @@ popd
> > > List.iter (
> > >   fun msi_path ->
> > > let fb_script = "\
> > > -echo Installing qemu-ga from " ^ msi_path ^ "
> > > -\"\\" ^ msi_path ^ "\" /norestart /qn /l+*vx \"%~dpn0.log\"
> > > -set elvl=!errorlevel!
> > > -echo Done installing qemu-ga error_level=!elvl!
> > > -if !elvl! == 0 (
> > > -  echo Restarting Windows...
> > > -  shutdown /r /f /c \"rebooted by firstboot script\"
> > > -)
> > > +echo Removing any previously scheduled qemu-ga installation
> > > +schtasks.exe /Delete /TN Firstboot-qemu-ga /F
> > > +echo Scheduling delayed installation of qemu-ga from " ^ msi_path ^ "
> > > +powershell.exe -command \"$d = (get-date).AddSeconds(120); schtasks.exe 
> > > /Create /SC ONCE /ST $d.ToString('HH:mm') /SD $d.ToString('MM/dd/') 
> > > /RU SYSTEM /TN Firstboot-qemu-ga /TR \\\"C:\\" ^ msi_path ^ " 
> > > /forcerestart /qn /l+*vx C:\\" ^ msi_path ^ ".log\\\"\"
> > >  " in
> > >Firstboot.add_firstboot_script g inspect.i_root
> > >  ("install " ^ msi_path) fb_script;
> > 
> > It could be easier to follow if you use sprintf here, something like:
> > 
> > let fb_script = sprintf "\
> > echo Removing any previously scheduled qemu-ga installation
> > schtasks.exe /Delete /TN Firstboot-qemu-ga /F
> > echo Scheduling delayed installation of qemu-ga from %s
> > powershell.exe -command \"$d = (get-date).AddSeconds(120); schtasks.exe 
> > /Create /SC ONCE /ST $d.ToString('HH:mm') /SD $d.ToString('MM/dd/') /RU 
> > SYSTEM /TN Firstboot-qemu-ga /TR \\\"C:\\%s /forcerestart /qn /l+*vx 
> > C:\\%s.log\\\"\"
> >   msi_path msi_path msi_path in
> 
> Ok, I can change that.
> 
> > 
> > The other possible problem is this seems to install a firstboot script
> > for every element of the list qemu_ga_files.  How long is this list?
> 
> At the moment it is just one. Until something changes in virtio-win ISO
> or in our internal logic (like in virtio_iso_path_matches_qemu_ga()) it
> will always be one.
> 
> > 
> > Do filenames on this list need some kind of quoting?  The filenames
> > don't, but they seem to contain a path that comes from the virtio-win
> > ISO.
> 
> Those are really just file names without path. See 
> https://github.com/libguestfs/virt-v2v/blob/master/v2v/windows_virtio.ml#L339

I'll ACK this, but I think using sprintf is better.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

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

Re: [Libguestfs] [PATCH v3] windows: delay installation of qemu-ga MSI

2020-03-05 Thread Tomáš Golembiovský
On Thu, Mar 05, 2020 at 08:49:04AM +, Richard W.M. Jones wrote:
> On Tue, Mar 03, 2020 at 03:45:49PM +0100, Tomáš Golembiovský wrote:
> > Instead of running firstboot script during early boot schedule a task
> > delayed for 2 minutes.
> > 
> > During the first boot, after virt-v2v conversion, Windows installs the
> > drivers injected by virt-v2v. When this installation is finished
> > Windows enforces some kind of internal reboot. This unfortunately
> > terminates any running firstboot scripts thus killing the installation
> > of qemu-ga MSI.
> > 
> > This is just a best-effort mitigation. It can still happen (e.g. with
> > slow disk drives) that the drivers are not yet installed when the
> > delayed installation starts. On the other hand we cannot delay it too
> > much otherwise we risk that the users logs in and will be doing some
> > work when the MSI installation starts. After MSI installation finishes
> > the VM needs to be rebooted which would be annoying if that would happen
> > under users hands. Although this is not a best fix (that may come later
> > as it is more complex, e.g. introducing waiting mechanism), the delay as
> > it is defined works in most cases. And it dramaticaly improves the
> > situations -- originaly I experienced more than 90% failure rate.
> > 
> > Signed-off-by: Tomáš Golembiovský 
> > ---
> >  common |  2 +-
> >  v2v/convert_windows.ml | 12 
> >  2 files changed, 5 insertions(+), 9 deletions(-)
> > 
> > diff --git a/common b/common
> > index ea10827b..5371257c 16
> > --- a/common
> > +++ b/common
> > @@ -1 +1 @@
> > -Subproject commit ea10827b4cfb3cfe5f782421c01d2902e5f73f90
> > +Subproject commit 5371257c3cf27fb09d5f2e31ba378b0e6ccf5df6
> > diff --git a/v2v/convert_windows.ml b/v2v/convert_windows.ml
> > index 0fda1d4e..bed5989a 100644
> > --- a/v2v/convert_windows.ml
> > +++ b/v2v/convert_windows.ml
> > @@ -429,14 +429,10 @@ popd
> > List.iter (
> >   fun msi_path ->
> > let fb_script = "\
> > -echo Installing qemu-ga from " ^ msi_path ^ "
> > -\"\\" ^ msi_path ^ "\" /norestart /qn /l+*vx \"%~dpn0.log\"
> > -set elvl=!errorlevel!
> > -echo Done installing qemu-ga error_level=!elvl!
> > -if !elvl! == 0 (
> > -  echo Restarting Windows...
> > -  shutdown /r /f /c \"rebooted by firstboot script\"
> > -)
> > +echo Removing any previously scheduled qemu-ga installation
> > +schtasks.exe /Delete /TN Firstboot-qemu-ga /F
> > +echo Scheduling delayed installation of qemu-ga from " ^ msi_path ^ "
> > +powershell.exe -command \"$d = (get-date).AddSeconds(120); schtasks.exe 
> > /Create /SC ONCE /ST $d.ToString('HH:mm') /SD $d.ToString('MM/dd/') /RU 
> > SYSTEM /TN Firstboot-qemu-ga /TR \\\"C:\\" ^ msi_path ^ " /forcerestart /qn 
> > /l+*vx C:\\" ^ msi_path ^ ".log\\\"\"
> >  " in
> >Firstboot.add_firstboot_script g inspect.i_root
> >  ("install " ^ msi_path) fb_script;
> 
> It could be easier to follow if you use sprintf here, something like:
> 
> let fb_script = sprintf "\
> echo Removing any previously scheduled qemu-ga installation
> schtasks.exe /Delete /TN Firstboot-qemu-ga /F
> echo Scheduling delayed installation of qemu-ga from %s
> powershell.exe -command \"$d = (get-date).AddSeconds(120); schtasks.exe 
> /Create /SC ONCE /ST $d.ToString('HH:mm') /SD $d.ToString('MM/dd/') /RU 
> SYSTEM /TN Firstboot-qemu-ga /TR \\\"C:\\%s /forcerestart /qn /l+*vx 
> C:\\%s.log\\\"\"
>   msi_path msi_path msi_path in

Ok, I can change that.

> 
> The other possible problem is this seems to install a firstboot script
> for every element of the list qemu_ga_files.  How long is this list?

At the moment it is just one. Until something changes in virtio-win ISO
or in our internal logic (like in virtio_iso_path_matches_qemu_ga()) it
will always be one.

> 
> Do filenames on this list need some kind of quoting?  The filenames
> don't, but they seem to contain a path that comes from the virtio-win
> ISO.

Those are really just file names without path. See 
https://github.com/libguestfs/virt-v2v/blob/master/v2v/windows_virtio.ml#L339

Tomas

> 
> Rich.
> 
> -- 
> Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> virt-builder quickly builds VMs from scratch
> http://libguestfs.org/virt-builder.1.html
> 

-- 
Tomáš Golembiovský 


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

Re: [Libguestfs] Having integration tests "test the future" with QEMU

2020-03-05 Thread Richard W.M. Jones
On Wed, Mar 04, 2020 at 03:48:34PM +0100, Markus Armbruster wrote:
> I understand libguestfs comes with integration tests.  You might be
> interested in something I'm developing for QEMU 5.0 to permit "testing
> the future".  I'd like to ensure it is actually useful before I
> continue.  Let me know what you think.
> 
> = Motivation =
> 
> When layers above QEMU somehow miss feature deprecation, things continue
> to work until the deprecated interface goes away, at which point we have
> an entirely avoidable regression.
> 
> We've been trying to get better at communicating feature deprecation to
> the layers above QEMU.  An obvious first step was systematic
> documentation.
> 
> However, documentation is all too easy to miss.  Even if you don't miss
> it, you may need to continue using the deprecated feature with old
> versions of QEMU, and that logic needs to be tested.
> 
> I'm working on a way to run QEMU with the deprecated features disabled.
> This permits "testing the future".
> 
> >From my cover letter:
> 
> This series extends QMP introspection to cover deprecation.
> Additionally, new option -compat lets you configure what to do when
> deprecated interfaces get used.  This is intended for testing users of
> the management interfaces.  It is experimental.
> 
> -compat deprecated-input= configures what to do when
> deprecated input is received.  Available policies:
> 
> * accept: Accept deprecated commands and arguments (default)
> * reject: Reject them
> * crash: Crash
> 
> -compat deprecated-output= configures what to do when
> deprecated output is sent.  Available output policies:
> 
> * accept: Emit deprecated command results and events (default)
> * hide: Suppress them
> 
> For now, -compat covers only deprecated syntactic aspects of QMP.  We
> may want to extend it to cover semantic aspects, CLI, and experimental
> features.

Yes we would add a -compat option when running qemu (only when running
non-production builds of libguestfs) so that it would fail loudly when
we use a deprecated feature.  This is the best way I've heard of so
far to get advanced warning of deprecated features.

Rich.

> If you want to learn more, check out the last three commit messages in
> the series.
> 
> = Patches =
> 
> [PATCH v2 00/30] Configurable policy for handling deprecated interfaces
> Message-Id: <20200303163505.32041-1-arm...@redhat.com>
> https://lists.nongnu.org/archive/html/qemu-devel/2020-03/msg00645.html
> 
> = Git =
> 
> Tag patchew/20200303163505.32041-1-arm...@redhat.com in repository
> https://github.com/patchew-project/qemu.git
> 
> ___
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

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



Re: [Libguestfs] Bug: readdir() returning null on large number of files

2020-03-05 Thread Richard W.M. Jones
On Wed, Mar 04, 2020 at 03:28:33PM +0300, Sergey Ivanov wrote:
> There's a bug (https://bugzilla.redhat.com/show_bug.cgi?id=1674392) and
> it's almost a year since it has been reported. Is there any
> plans/suggestions on its fixing? Didn't find any commits regarding this
> issue but maybe I'm just blind.

It's open source, we don't have a guarantee of fixing any particular
bug.  I suggest if it's important for you that you take a look at
fixing it yourself.

Rich.


-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

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



Re: [Libguestfs] [PATCH v3] windows: delay installation of qemu-ga MSI

2020-03-05 Thread Richard W.M. Jones
On Tue, Mar 03, 2020 at 03:45:49PM +0100, Tomáš Golembiovský wrote:
> Instead of running firstboot script during early boot schedule a task
> delayed for 2 minutes.
> 
> During the first boot, after virt-v2v conversion, Windows installs the
> drivers injected by virt-v2v. When this installation is finished
> Windows enforces some kind of internal reboot. This unfortunately
> terminates any running firstboot scripts thus killing the installation
> of qemu-ga MSI.
> 
> This is just a best-effort mitigation. It can still happen (e.g. with
> slow disk drives) that the drivers are not yet installed when the
> delayed installation starts. On the other hand we cannot delay it too
> much otherwise we risk that the users logs in and will be doing some
> work when the MSI installation starts. After MSI installation finishes
> the VM needs to be rebooted which would be annoying if that would happen
> under users hands. Although this is not a best fix (that may come later
> as it is more complex, e.g. introducing waiting mechanism), the delay as
> it is defined works in most cases. And it dramaticaly improves the
> situations -- originaly I experienced more than 90% failure rate.
> 
> Signed-off-by: Tomáš Golembiovský 
> ---
>  common |  2 +-
>  v2v/convert_windows.ml | 12 
>  2 files changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/common b/common
> index ea10827b..5371257c 16
> --- a/common
> +++ b/common
> @@ -1 +1 @@
> -Subproject commit ea10827b4cfb3cfe5f782421c01d2902e5f73f90
> +Subproject commit 5371257c3cf27fb09d5f2e31ba378b0e6ccf5df6
> diff --git a/v2v/convert_windows.ml b/v2v/convert_windows.ml
> index 0fda1d4e..bed5989a 100644
> --- a/v2v/convert_windows.ml
> +++ b/v2v/convert_windows.ml
> @@ -429,14 +429,10 @@ popd
> List.iter (
>   fun msi_path ->
> let fb_script = "\
> -echo Installing qemu-ga from " ^ msi_path ^ "
> -\"\\" ^ msi_path ^ "\" /norestart /qn /l+*vx \"%~dpn0.log\"
> -set elvl=!errorlevel!
> -echo Done installing qemu-ga error_level=!elvl!
> -if !elvl! == 0 (
> -  echo Restarting Windows...
> -  shutdown /r /f /c \"rebooted by firstboot script\"
> -)
> +echo Removing any previously scheduled qemu-ga installation
> +schtasks.exe /Delete /TN Firstboot-qemu-ga /F
> +echo Scheduling delayed installation of qemu-ga from " ^ msi_path ^ "
> +powershell.exe -command \"$d = (get-date).AddSeconds(120); schtasks.exe 
> /Create /SC ONCE /ST $d.ToString('HH:mm') /SD $d.ToString('MM/dd/') /RU 
> SYSTEM /TN Firstboot-qemu-ga /TR \\\"C:\\" ^ msi_path ^ " /forcerestart /qn 
> /l+*vx C:\\" ^ msi_path ^ ".log\\\"\"
>  " in
>Firstboot.add_firstboot_script g inspect.i_root
>  ("install " ^ msi_path) fb_script;

It could be easier to follow if you use sprintf here, something like:

let fb_script = sprintf "\
echo Removing any previously scheduled qemu-ga installation
schtasks.exe /Delete /TN Firstboot-qemu-ga /F
echo Scheduling delayed installation of qemu-ga from %s
powershell.exe -command \"$d = (get-date).AddSeconds(120); schtasks.exe /Create 
/SC ONCE /ST $d.ToString('HH:mm') /SD $d.ToString('MM/dd/') /RU SYSTEM /TN 
Firstboot-qemu-ga /TR \\\"C:\\%s /forcerestart /qn /l+*vx C:\\%s.log\\\"\"
  msi_path msi_path msi_path in

The other possible problem is this seems to install a firstboot script
for every element of the list qemu_ga_files.  How long is this list?

Do filenames on this list need some kind of quoting?  The filenames
don't, but they seem to contain a path that comes from the virtio-win
ISO.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

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