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 <address@hidden>
> wrote:
> 
>     Quoting Sameeh Jubran (2018-06-24 07:45:40)
>     > From: Sameeh Jubran <address@hidden>
>     >
>     > 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 <address@hidden>
>     > ---
>     >  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.