[Qemu-devel] [PATCH] qga-win: Handle fstrim for OSes lower than Win8

[Qemu-devel] [PATCH] qga-win: Handle fstrim for OSes lower than Win8

[Sameeh Jubran]

From: Sameeh Jubran <sjubran@redhat.com>

The defrag.exe tool which is used for executing the fstrim command
on Windows doesn't support retrim for OSes lower than Win8. This
commit handles this case and returns a suitable error.

Output of fstrim before this commit:
{"execute":"guest-fstrim"}
{"return": {"paths": [{"path": "C:\\", "error": "An invalid command line option
was specified. (0x89000008)"}, {"path": "F:\\", "error": "An invalid command
line option was specified. (0x89000008)"}, {"path": "S:\\", "error": "An
invalid command line option was specified. (0x89000008)"}]}}

Reported on:
https://bugzilla.redhat.com/show_bug.cgi?id=1594113

Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
---
 qga/commands-win32.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index d79974f212..0bdcd9dd38 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -30,6 +30,7 @@
 #include <lm.h>
 #include <wtsapi32.h>
 #include <wininet.h>
+#include <versionhelpers.h>
 
 #include "qga/guest-agent-core.h"
 #include "qga/vss-win32.h"
@@ -852,6 +853,11 @@ qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
     HANDLE handle;
     WCHAR guid[MAX_PATH] = L"";
 
+   if (!IsWindows8OrGreater()) {
+        error_setg(errp, "fstrim is only supported for Win8+");
+        return NULL;
+   }
+
     handle = FindFirstVolumeW(guid, ARRAYSIZE(guid));
     if (handle == INVALID_HANDLE_VALUE) {
         error_setg_win32(errp, GetLastError(), "failed to find any volume");
-- 
2.13.6

Re: [Qemu-devel] [PATCH] qga-win: Handle fstrim for OSes lower than Win8

[Sameeh Jubran]

ping.

On Sun, Jun 24, 2018 at 3:45 PM, Sameeh Jubran <sameeh@daynix.com> wrote:

> From: Sameeh Jubran <sjubran@redhat.com>
>
> The defrag.exe tool which is used for executing the fstrim command
> on Windows doesn't support retrim for OSes lower than Win8. This
> commit handles this case and returns a suitable error.
>
> Output of fstrim before this commit:
> {"execute":"guest-fstrim"}
> {"return": {"paths": [{"path": "C:\\", "error": "An invalid command line
> option
> was specified. (0x89000008)"}, {"path": "F:\\", "error": "An invalid
> command
> line option was specified. (0x89000008)"}, {"path": "S:\\", "error": "An
> invalid command line option was specified. (0x89000008)"}]}}
>
> Reported on:
> https://bugzilla.redhat.com/show_bug.cgi?id=1594113
>
> Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
> ---
>  qga/commands-win32.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index d79974f212..0bdcd9dd38 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -30,6 +30,7 @@
>  #include <lm.h>
>  #include <wtsapi32.h>
>  #include <wininet.h>
> +#include <versionhelpers.h>
>
>  #include "qga/guest-agent-core.h"
>  #include "qga/vss-win32.h"
> @@ -852,6 +853,11 @@ qmp_guest_fstrim(bool has_minimum, int64_t minimum,
> Error **errp)
>      HANDLE handle;
>      WCHAR guid[MAX_PATH] = L"";
>
> +   if (!IsWindows8OrGreater()) {
> +        error_setg(errp, "fstrim is only supported for Win8+");
> +        return NULL;
> +   }
> +
>      handle = FindFirstVolumeW(guid, ARRAYSIZE(guid));
>      if (handle == INVALID_HANDLE_VALUE) {
>          error_setg_win32(errp, GetLastError(), "failed to find any
> volume");
> --
> 2.13.6
>
>


-- 
Respectfully,
*Sameeh Jubran*
*Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>*
*Software Engineer @ Daynix <http://www.daynix.com>.*

Re: [Qemu-devel] [PATCH] qga-win: Handle fstrim for OSes lower than Win8

[Michael Roth]

Quoting Sameeh Jubran (2018-06-24 07:45:40)
> From: Sameeh Jubran <sjubran@redhat.com>
> 
> The defrag.exe tool which is used for executing the fstrim command
> on Windows doesn't support retrim for OSes lower than Win8. This
> commit handles this case and returns a suitable error.
> 
> Output of fstrim before this commit:
> {"execute":"guest-fstrim"}
> {"return": {"paths": [{"path": "C:\\", "error": "An invalid command line option
> was specified. (0x89000008)"}, {"path": "F:\\", "error": "An invalid command
> line option was specified. (0x89000008)"}, {"path": "S:\\", "error": "An
> invalid command line option was specified. (0x89000008)"}]}}
> 
> Reported on:
> https://bugzilla.redhat.com/show_bug.cgi?id=1594113
> 
> Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
> ---
>  qga/commands-win32.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index d79974f212..0bdcd9dd38 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -30,6 +30,7 @@
>  #include <lm.h>
>  #include <wtsapi32.h>
>  #include <wininet.h>
> +#include <versionhelpers.h>

I have this queued locally but the mingw64 build environment I've been
using (fc20) doesn't support this header and I'm running into some odd
null pointer / 0xc0000005 / access violation issues with the binary
generated in newer tool chains. I'll send a pull for this next week when
I can get it tested.

What build envionment are you using?

> 
>  #include "qga/guest-agent-core.h"
>  #include "qga/vss-win32.h"
> @@ -852,6 +853,11 @@ qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error **errp)
>      HANDLE handle;
>      WCHAR guid[MAX_PATH] = L"";
> 
> +   if (!IsWindows8OrGreater()) {
> +        error_setg(errp, "fstrim is only supported for Win8+");
> +        return NULL;
> +   }
> +
>      handle = FindFirstVolumeW(guid, ARRAYSIZE(guid));
>      if (handle == INVALID_HANDLE_VALUE) {
>          error_setg_win32(errp, GetLastError(), "failed to find any volume");
> -- 
> 2.13.6
> 

Re: [Qemu-devel] [PATCH] qga-win: Handle fstrim for OSes lower than Win8

[Sameeh Jubran]

I'v successfully compiled the previous patch on Fedora 27, but it seems to
be failing on RHEL, You can apply this patch instead which avoids using the
versionhelpers header:
---
 qga/commands-win32.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index fb91f5d..e688e71 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -846,6 +846,17 @@ qmp_guest_fstrim(bool has_minimum, int64_t minimum,
Error **errp)
     GuestFilesystemTrimResponse *resp;
     HANDLE handle;
     WCHAR guid[MAX_PATH] = L"";
+    OSVERSIONINFO osvi;
+    BOOL bIsWindows8orLater;
+
+    ZeroMemory(&osvi, sizeof(OSVERSIONINFO));
+    osvi.dwOSVersionInfoSize = sizeof(OSVERSIONINFO);
+    GetVersionEx(&osvi);
+    bIsWindows8orLater = ((osvi.dwMajorVersion == 6) &&
(osvi.dwMinorVersion >= 2));
+    if (!bIsWindows8orLater) {
+         error_setg(errp, "fstrim is only supported for Win8+");
+         return NULL;
+    }

     handle = FindFirstVolumeW(guid, ARRAYSIZE(guid));
     if (handle == INVALID_HANDLE_VALUE) {
--
2.8.1.185.gdc0db2c


On Mon, Jul 16, 2018 at 11:10 PM, Michael Roth <mdroth@linux.vnet.ibm.com>
wrote:

> Quoting Sameeh Jubran (2018-06-24 07:45:40)
> > From: Sameeh Jubran <sjubran@redhat.com>
> >
> > The defrag.exe tool which is used for executing the fstrim command
> > on Windows doesn't support retrim for OSes lower than Win8. This
> > commit handles this case and returns a suitable error.
> >
> > Output of fstrim before this commit:
> > {"execute":"guest-fstrim"}
> > {"return": {"paths": [{"path": "C:\\", "error": "An invalid command line
> option
> > was specified. (0x89000008)"}, {"path": "F:\\", "error": "An invalid
> command
> > line option was specified. (0x89000008)"}, {"path": "S:\\", "error": "An
> > invalid command line option was specified. (0x89000008)"}]}}
> >
> > Reported on:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1594113
> >
> > Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
> > ---
> >  qga/commands-win32.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> > index d79974f212..0bdcd9dd38 100644
> > --- a/qga/commands-win32.c
> > +++ b/qga/commands-win32.c
> > @@ -30,6 +30,7 @@
> >  #include <lm.h>
> >  #include <wtsapi32.h>
> >  #include <wininet.h>
> > +#include <versionhelpers.h>
>
> I have this queued locally but the mingw64 build environment I've been
> using (fc20) doesn't support this header and I'm running into some odd
> null pointer / 0xc0000005 / access violation issues with the binary
> generated in newer tool chains. I'll send a pull for this next week when
> I can get it tested.
>
> What build envionment are you using?
>
> >
> >  #include "qga/guest-agent-core.h"
> >  #include "qga/vss-win32.h"
> > @@ -852,6 +853,11 @@ qmp_guest_fstrim(bool has_minimum, int64_t minimum,
> Error **errp)
> >      HANDLE handle;
> >      WCHAR guid[MAX_PATH] = L"";
> >
> > +   if (!IsWindows8OrGreater()) {
> > +        error_setg(errp, "fstrim is only supported for Win8+");
> > +        return NULL;
> > +   }
> > +
> >      handle = FindFirstVolumeW(guid, ARRAYSIZE(guid));
> >      if (handle == INVALID_HANDLE_VALUE) {
> >          error_setg_win32(errp, GetLastError(), "failed to find any
> volume");
> > --
> > 2.13.6
> >
>



-- 
Respectfully,
*Sameeh Jubran*
*Linkedin <https://il.linkedin.com/pub/sameeh-jubran/87/747/a8a>*
*Software Engineer @ Daynix <http://www.daynix.com>.*

Re: [Qemu-devel] [PATCH] qga-win: Handle fstrim for OSes lower than Win8

[Daniel P. Berrangé]

On Mon, Jul 16, 2018 at 03:10:38PM -0500, Michael Roth wrote:
> Quoting Sameeh Jubran (2018-06-24 07:45:40)
> > From: Sameeh Jubran <sjubran@redhat.com>
> > 
> > The defrag.exe tool which is used for executing the fstrim command
> > on Windows doesn't support retrim for OSes lower than Win8. This
> > commit handles this case and returns a suitable error.
> > 
> > Output of fstrim before this commit:
> > {"execute":"guest-fstrim"}
> > {"return": {"paths": [{"path": "C:\\", "error": "An invalid command line option
> > was specified. (0x89000008)"}, {"path": "F:\\", "error": "An invalid command
> > line option was specified. (0x89000008)"}, {"path": "S:\\", "error": "An
> > invalid command line option was specified. (0x89000008)"}]}}
> > 
> > Reported on:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1594113
> > 
> > Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
> > ---
> >  qga/commands-win32.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> > index d79974f212..0bdcd9dd38 100644
> > --- a/qga/commands-win32.c
> > +++ b/qga/commands-win32.c
> > @@ -30,6 +30,7 @@
> >  #include <lm.h>
> >  #include <wtsapi32.h>
> >  #include <wininet.h>
> > +#include <versionhelpers.h>
> 
> I have this queued locally but the mingw64 build environment I've been
> using (fc20) doesn't support this header and I'm running into some odd

Please tell me that fc20 is a typo, and you're actually using the modern
fc28 :-)  Fedora 20 went end of life 3 years ago, so is not considered
a supported platform for QEMU per our policy here:

  https://qemu.weilnetz.de/doc/qemu-doc.html#Supported-build-platforms

> null pointer / 0xc0000005 / access violation issues with the binary
> generated in newer tool chains. I'll send a pull for this next week when
> I can get it tested.
> 
> What build envionment are you using?

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH] qga-win: Handle fstrim for OSes lower than Win8

[Michael Roth]

Quoting Daniel P. Berrangé (2018-07-17 04:34:18)
> On Mon, Jul 16, 2018 at 03:10:38PM -0500, Michael Roth wrote:
> > Quoting Sameeh Jubran (2018-06-24 07:45:40)
> > > From: Sameeh Jubran <sjubran@redhat.com>
> > > 
> > > The defrag.exe tool which is used for executing the fstrim command
> > > on Windows doesn't support retrim for OSes lower than Win8. This
> > > commit handles this case and returns a suitable error.
> > > 
> > > Output of fstrim before this commit:
> > > {"execute":"guest-fstrim"}
> > > {"return": {"paths": [{"path": "C:\\", "error": "An invalid command line option
> > > was specified. (0x89000008)"}, {"path": "F:\\", "error": "An invalid command
> > > line option was specified. (0x89000008)"}, {"path": "S:\\", "error": "An
> > > invalid command line option was specified. (0x89000008)"}]}}
> > > 
> > > Reported on:
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1594113
> > > 
> > > Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
> > > ---
> > >  qga/commands-win32.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> > > index d79974f212..0bdcd9dd38 100644
> > > --- a/qga/commands-win32.c
> > > +++ b/qga/commands-win32.c
> > > @@ -30,6 +30,7 @@
> > >  #include <lm.h>
> > >  #include <wtsapi32.h>
> > >  #include <wininet.h>
> > > +#include <versionhelpers.h>
> > 
> > I have this queued locally but the mingw64 build environment I've been
> > using (fc20) doesn't support this header and I'm running into some odd
> 
> Please tell me that fc20 is a typo, and you're actually using the modern
> fc28 :-)  Fedora 20 went end of life 3 years ago, so is not considered
> a supported platform for QEMU per our policy here:
> 
>   https://qemu.weilnetz.de/doc/qemu-doc.html#Supported-build-platforms

I'm only using it for mingw builds, but yes, it's definitely time to move to
something a bit less ancient.

> 
> > null pointer / 0xc0000005 / access violation issues with the binary
> > generated in newer tool chains. I'll send a pull for this next week when
> > I can get it tested.
> > 
> > What build envionment are you using?
> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 

Re: [Qemu-devel] [PATCH] qga-win: Handle fstrim for OSes lower than Win8

[Michael Roth]

Quoting Sameeh Jubran (2018-07-17 00:46:27)
> I'v successfully compiled the previous patch on Fedora 27, but it seems to be
> failing on RHEL, You can apply this patch instead which avoids using the
> versionhelpers header:

Thanks, applied to qga tree:
  https://github.com/mdroth/qemu/commits/qga

I went ahead with the GetVersionEx() version since it's also used
elsewhere in qga code. I needed to fix up the version check to be
(major > 6) || (major == 6 && minor >= 2) since that seems to align
with what's documented here:

https://docs.microsoft.com/en-us/windows/desktop/api/winnt/ns-winnt-_osversioninfoa

> ---
>  qga/commands-win32.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index fb91f5d..e688e71 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -846,6 +846,17 @@ qmp_guest_fstrim(bool has_minimum, int64_t minimum, Error
> **errp)
>      GuestFilesystemTrimResponse *resp;
>      HANDLE handle;
>      WCHAR guid[MAX_PATH] = L"";
> +    OSVERSIONINFO osvi;
> +    BOOL bIsWindows8orLater;
> +
> +    ZeroMemory(&osvi, sizeof(OSVERSIONINFO));
> +    osvi.dwOSVersionInfoSize = sizeof(OSVERSIONINFO);
> +    GetVersionEx(&osvi);
> +    bIsWindows8orLater = ((osvi.dwMajorVersion == 6) && (osvi.dwMinorVersion >
> = 2));
> +    if (!bIsWindows8orLater) {
> +         error_setg(errp, "fstrim is only supported for Win8+");
> +         return NULL;
> +    }
> 
>      handle = FindFirstVolumeW(guid, ARRAYSIZE(guid));
>      if (handle == INVALID_HANDLE_VALUE) {
> --
> 2.8.1.185.gdc0db2c
> 
> 
> On Mon, Jul 16, 2018 at 11:10 PM, Michael Roth <mdroth@linux.vnet.ibm.com>
> wrote:
> 
>     Quoting Sameeh Jubran (2018-06-24 07:45:40)
>     > From: Sameeh Jubran <sjubran@redhat.com>
>     >
>     > The defrag.exe tool which is used for executing the fstrim command
>     > on Windows doesn't support retrim for OSes lower than Win8. This
>     > commit handles this case and returns a suitable error.
>     >
>     > Output of fstrim before this commit:
>     > {"execute":"guest-fstrim"}
>     > {"return": {"paths": [{"path": "C:\\", "error": "An invalid command line
>     option
>     > was specified. (0x89000008)"}, {"path": "F:\\", "error": "An invalid
>     command
>     > line option was specified. (0x89000008)"}, {"path": "S:\\", "error": "An
>     > invalid command line option was specified. (0x89000008)"}]}}
>     >
>     > Reported on:
>     > https://bugzilla.redhat.com/show_bug.cgi?id=1594113
>     >
>     > Signed-off-by: Sameeh Jubran <sjubran@redhat.com>
>     > ---
>     >  qga/commands-win32.c | 6 ++++++
>     >  1 file changed, 6 insertions(+)
>     >
>     > diff --git a/qga/commands-win32.c b/qga/commands-win32.c
>     > index d79974f212..0bdcd9dd38 100644
>     > --- a/qga/commands-win32.c
>     > +++ b/qga/commands-win32.c
>     > @@ -30,6 +30,7 @@
>     >  #include <lm.h>
>     >  #include <wtsapi32.h>
>     >  #include <wininet.h>
>     > +#include <versionhelpers.h>
> 
>     I have this queued locally but the mingw64 build environment I've been
>     using (fc20) doesn't support this header and I'm running into some odd
>     null pointer / 0xc0000005 / access violation issues with the binary
>     generated in newer tool chains. I'll send a pull for this next week when
>     I can get it tested.
> 
>     What build envionment are you using?
> 
>     >
>     >  #include "qga/guest-agent-core.h"
>     >  #include "qga/vss-win32.h"
>     > @@ -852,6 +853,11 @@ qmp_guest_fstrim(bool has_minimum, int64_t minimum,
>     Error **errp)
>     >      HANDLE handle;
>     >      WCHAR guid[MAX_PATH] = L"";
>     >
>     > +   if (!IsWindows8OrGreater()) {
>     > +        error_setg(errp, "fstrim is only supported for Win8+");
>     > +        return NULL;
>     > +   }
>     > +
>     >      handle = FindFirstVolumeW(guid, ARRAYSIZE(guid));
>     >      if (handle == INVALID_HANDLE_VALUE) {
>     >          error_setg_win32(errp, GetLastError(), "failed to find any
>     volume");
>     > --
>     > 2.13.6
>     >
> 
> 
> 
> 
> --
> Respectfully,
> Sameeh Jubran
> Linkedin
> Software Engineer @ Daynix.