1. Patchew
  2. QEMU
  3. View series

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

Sameeh Jubran posted 1 patch 7 years, 4 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20180624124540.6659-1-sameeh@daynix.com
Test checkpatch passed
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
Test s390x passed
qga/commands-win32.c | 6 ++++++
1 file changed, 6 insertions(+)
[Qemu-devel] [PATCH] qga-win: Handle fstrim for OSes lower than Win8
Posted by Sameeh Jubran 7 years, 4 months ago 
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
Posted by Sameeh Jubran 7 years, 3 months ago 
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
Posted by Michael Roth 7 years, 3 months ago 
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
Posted by Sameeh Jubran 7 years, 3 months ago 
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
Posted by Michael Roth 7 years, 3 months ago 
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.
Re: [Qemu-devel] [PATCH] qga-win: Handle fstrim for OSes lower than Win8
Posted by Daniel P. Berrangé 7 years, 3 months ago 
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
Posted by Michael Roth 7 years, 3 months ago 
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 :|
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.