* Re: [Qemu-devel] [PATCH] qga: report disk size and free space
2018-07-03 8:31 [Qemu-devel] [PATCH] qga: report disk size and free space Tomáš Golembiovský
@ 2018-07-03 10:01 ` Marc-André Lureau
2018-07-03 10:36 ` Tomáš Golembiovský
2018-07-03 10:54 ` Tomáš Golembiovský
` (3 subsequent siblings)
4 siblings, 1 reply; 8+ messages in thread
From: Marc-André Lureau @ 2018-07-03 10:01 UTC (permalink / raw)
To: Tomáš Golembiovský; +Cc: QEMU, Michael Roth
Hi
On Tue, Jul 3, 2018 at 10:31 AM, Tomáš Golembiovský <tgolembi@redhat.com> wrote:
> Report total file system size and free space in output of command
> "guest-get-fsinfo". Values are optional and it is not an error if they cannot
> be retrieved for some reason.
>
I am afraid this will miss 3.0, since it's a new feature
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
> qga/commands-posix.c | 18 ++++++++++++++++++
> qga/commands-win32.c | 16 ++++++++++++++++
> qga/qapi-schema.json | 5 ++++-
> 3 files changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index eae817191b..1f2fb25b91 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -13,6 +13,7 @@
>
> #include "qemu/osdep.h"
> #include <sys/ioctl.h>
> +#include <sys/statvfs.h>
> #include <sys/utsname.h>
> #include <sys/wait.h>
> #include <dirent.h>
> @@ -1074,11 +1075,28 @@ static GuestFilesystemInfo *build_guest_fsinfo(struct FsMount *mount,
> GuestFilesystemInfo *fs = g_malloc0(sizeof(*fs));
> char *devpath = g_strdup_printf("/sys/dev/block/%u:%u",
> mount->devmajor, mount->devminor);
> + struct statvfs vfs_stats;
>
> fs->mountpoint = g_strdup(mount->dirname);
> fs->type = g_strdup(mount->devtype);
> build_guest_fsinfo_for_device(devpath, fs, errp);
>
> + g_debug(" get filesystem statistics for '%s'", mount->dirname);
> + if (statvfs(mount->dirname, &vfs_stats) != 0) {
> + /* This is not fatal, just log this incident */
> + Error *local_err = NULL;
> + error_setg_errno(&local_err, errno, "statfs(\"%s\")",
statvfs()
> + mount->dirname);
> + slog("failed to stat filesystem: %s",
> + error_get_pretty(local_err));
> + error_free(local_err);
The usage of Error is a bit overkill, perhaps you may juste use slog()
/ strerror() directly.
> + } else {
> + fs->size = vfs_stats.f_frsize * vfs_stats.f_blocks;
> + fs->has_size = true;
> + fs->free = vfs_stats.f_frsize * vfs_stats.f_bfree;
> + fs->has_free = true;
> + }
> +
> g_free(devpath);
> return fs;
> }
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 70ee5379f6..6d6ca05281 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -706,6 +706,22 @@ static GuestFilesystemInfo *build_guest_fsinfo(char *guid, Error **errp)
> }
> fs->type = g_strdup(fs_name);
> fs->disk = build_guest_disk_info(guid, errp);
> +
> + if (len > 0) {
> + if (GetDiskFreeSpaceEx(mnt_point, 0, (PULARGE_INTEGER)&fs->size,
> + (PULARGE_INTEGER)&fs->free) != 0) {
I would rather use some local ULARGE_INTEGER and access the QuadPart
to avoid the cast, but it should work.
> + /* This is not fatal, just log this incident */
> + Error *local_err = NULL;
> + error_setg_win32(&local_err, GetLastError(),
> + "failed to get free space on volume \"%s\"", mnt_point);
> + slog("%s", error_get_pretty(local_err));
> + error_free(local_err);
> + } else {
> + fs->has_size = true;
> + fs->has_free = true;
> + }
> + }
> +
> free:
> g_free(mnt_point);
> return fs;
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index 17884c7c70..28a32444d3 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -848,12 +848,15 @@
> # @type: file system type string
> # @disk: an array of disk hardware information that the volume lies on,
> # which may be empty if the disk type is not supported
> +# @size: total number of bytes on the file system (Since 2.13)
> +# @free: number of bytes available on the file system (Since 2.13)
"Since 3.1", most probably
> #
> # Since: 2.2
> ##
> { 'struct': 'GuestFilesystemInfo',
> 'data': {'name': 'str', 'mountpoint': 'str', 'type': 'str',
> - 'disk': ['GuestDiskAddress']} }
> + 'disk': ['GuestDiskAddress'], '*size': 'uint64',
> + '*free': 'uint64'} }
>
> ##
> # @guest-get-fsinfo:
> --
> 2.17.1
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] qga: report disk size and free space
2018-07-03 10:01 ` Marc-André Lureau
@ 2018-07-03 10:36 ` Tomáš Golembiovský
0 siblings, 0 replies; 8+ messages in thread
From: Tomáš Golembiovský @ 2018-07-03 10:36 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: QEMU, Michael Roth
On Tue, 3 Jul 2018 12:01:55 +0200
Marc-André Lureau <marcandre.lureau@gmail.com> wrote:
> Hi
>
> On Tue, Jul 3, 2018 at 10:31 AM, Tomáš Golembiovský <tgolembi@redhat.com> wrote:
> > Report total file system size and free space in output of command
> > "guest-get-fsinfo". Values are optional and it is not an error if they cannot
> > be retrieved for some reason.
> >
>
> I am afraid this will miss 3.0, since it's a new feature
I kind of expected this.
> > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> > ---
> > qga/commands-posix.c | 18 ++++++++++++++++++
> > qga/commands-win32.c | 16 ++++++++++++++++
> > qga/qapi-schema.json | 5 ++++-
> > 3 files changed, 38 insertions(+), 1 deletion(-)
> >
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index eae817191b..1f2fb25b91 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -13,6 +13,7 @@
> >
> > #include "qemu/osdep.h"
> > #include <sys/ioctl.h>
> > +#include <sys/statvfs.h>
> > #include <sys/utsname.h>
> > #include <sys/wait.h>
> > #include <dirent.h>
> > @@ -1074,11 +1075,28 @@ static GuestFilesystemInfo *build_guest_fsinfo(struct FsMount *mount,
> > GuestFilesystemInfo *fs = g_malloc0(sizeof(*fs));
> > char *devpath = g_strdup_printf("/sys/dev/block/%u:%u",
> > mount->devmajor, mount->devminor);
> > + struct statvfs vfs_stats;
> >
> > fs->mountpoint = g_strdup(mount->dirname);
> > fs->type = g_strdup(mount->devtype);
> > build_guest_fsinfo_for_device(devpath, fs, errp);
> >
> > + g_debug(" get filesystem statistics for '%s'", mount->dirname);
> > + if (statvfs(mount->dirname, &vfs_stats) != 0) {
> > + /* This is not fatal, just log this incident */
> > + Error *local_err = NULL;
> > + error_setg_errno(&local_err, errno, "statfs(\"%s\")",
>
> statvfs()
>
> > + mount->dirname);
> > + slog("failed to stat filesystem: %s",
> > + error_get_pretty(local_err));
> > + error_free(local_err);
>
> The usage of Error is a bit overkill, perhaps you may juste use slog()
> / strerror() directly.
>
> > + } else {
> > + fs->size = vfs_stats.f_frsize * vfs_stats.f_blocks;
> > + fs->has_size = true;
> > + fs->free = vfs_stats.f_frsize * vfs_stats.f_bfree;
> > + fs->has_free = true;
> > + }
> > +
> > g_free(devpath);
> > return fs;
> > }
> > diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> > index 70ee5379f6..6d6ca05281 100644
> > --- a/qga/commands-win32.c
> > +++ b/qga/commands-win32.c
> > @@ -706,6 +706,22 @@ static GuestFilesystemInfo *build_guest_fsinfo(char *guid, Error **errp)
> > }
> > fs->type = g_strdup(fs_name);
> > fs->disk = build_guest_disk_info(guid, errp);
> > +
> > + if (len > 0) {
> > + if (GetDiskFreeSpaceEx(mnt_point, 0, (PULARGE_INTEGER)&fs->size,
> > + (PULARGE_INTEGER)&fs->free) != 0) {
>
> I would rather use some local ULARGE_INTEGER and access the QuadPart
> to avoid the cast, but it should work.
I tried that, but the returned values were nonsensical. Maybe I did
something wrong along the way. I'll give it one more try.
Thanks for quick response,
Tomas
>
> > + /* This is not fatal, just log this incident */
> > + Error *local_err = NULL;
> > + error_setg_win32(&local_err, GetLastError(),
> > + "failed to get free space on volume \"%s\"", mnt_point);
> > + slog("%s", error_get_pretty(local_err));
> > + error_free(local_err);
> > + } else {
> > + fs->has_size = true;
> > + fs->has_free = true;
> > + }
> > + }
> > +
> > free:
> > g_free(mnt_point);
> > return fs;
> > diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> > index 17884c7c70..28a32444d3 100644
> > --- a/qga/qapi-schema.json
> > +++ b/qga/qapi-schema.json
> > @@ -848,12 +848,15 @@
> > # @type: file system type string
> > # @disk: an array of disk hardware information that the volume lies on,
> > # which may be empty if the disk type is not supported
> > +# @size: total number of bytes on the file system (Since 2.13)
> > +# @free: number of bytes available on the file system (Since 2.13)
>
> "Since 3.1", most probably
>
> > #
> > # Since: 2.2
> > ##
> > { 'struct': 'GuestFilesystemInfo',
> > 'data': {'name': 'str', 'mountpoint': 'str', 'type': 'str',
> > - 'disk': ['GuestDiskAddress']} }
> > + 'disk': ['GuestDiskAddress'], '*size': 'uint64',
> > + '*free': 'uint64'} }
> >
> > ##
> > # @guest-get-fsinfo:
> > --
> > 2.17.1
> >
> >
>
>
>
> --
> Marc-André Lureau
--
Tomáš Golembiovský <tgolembi@redhat.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] qga: report disk size and free space
2018-07-03 8:31 [Qemu-devel] [PATCH] qga: report disk size and free space Tomáš Golembiovský
2018-07-03 10:01 ` Marc-André Lureau
@ 2018-07-03 10:54 ` Tomáš Golembiovský
2018-07-03 16:26 ` Eric Blake
2018-07-03 11:39 ` no-reply
` (2 subsequent siblings)
4 siblings, 1 reply; 8+ messages in thread
From: Tomáš Golembiovský @ 2018-07-03 10:54 UTC (permalink / raw)
To: qemu-devel; +Cc: Marc-André Lureau, Michael Roth
On Tue, 3 Jul 2018 10:31:20 +0200
Tomáš Golembiovský <tgolembi@redhat.com> wrote:
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 70ee5379f6..6d6ca05281 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -706,6 +706,22 @@ static GuestFilesystemInfo *build_guest_fsinfo(char *guid, Error **errp)
> }
> fs->type = g_strdup(fs_name);
> fs->disk = build_guest_disk_info(guid, errp);
> +
> + if (len > 0) {
> + if (GetDiskFreeSpaceEx(mnt_point, 0, (PULARGE_INTEGER)&fs->size,
> + (PULARGE_INTEGER)&fs->free) != 0) {
Btw, it seems to me that checkpatch.pl returns some false positives
here. See:
ERROR: spaces required around that '&' (ctx:VxV)
#72: FILE: qga/commands-win32.c:711:
+ if (GetDiskFreeSpaceEx(mnt_point, 0, (PULARGE_INTEGER)&fs->size,
^
ERROR: spaces required around that '&' (ctx:VxV)
#73: FILE: qga/commands-win32.c:712:
+ (PULARGE_INTEGER)&fs->free) != 0) {
^
Seems that & is mistakenly interpreted as bitwise-AND operator.
Or am I expected to format that as
..., (PULARGE_INTEGER) & fs->size, ...
because that looks... weird.
Also, checkpatch.pl itself suggest to look into MAINTAINERS to find out
whom to report false positives, but there is no maintainer there. :)
Tomas
--
Tomáš Golembiovský <tgolembi@redhat.com>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] qga: report disk size and free space
2018-07-03 10:54 ` Tomáš Golembiovský
@ 2018-07-03 16:26 ` Eric Blake
0 siblings, 0 replies; 8+ messages in thread
From: Eric Blake @ 2018-07-03 16:26 UTC (permalink / raw)
To: Tomáš Golembiovský, qemu-devel
Cc: Marc-André Lureau, Michael Roth
On 07/03/2018 05:54 AM, Tomáš Golembiovský wrote:
> On Tue, 3 Jul 2018 10:31:20 +0200
> Tomáš Golembiovský <tgolembi@redhat.com> wrote:
>
>> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
>> index 70ee5379f6..6d6ca05281 100644
>> --- a/qga/commands-win32.c
>> +++ b/qga/commands-win32.c
>> @@ -706,6 +706,22 @@ static GuestFilesystemInfo *build_guest_fsinfo(char *guid, Error **errp)
>> }
>> fs->type = g_strdup(fs_name);
>> fs->disk = build_guest_disk_info(guid, errp);
>> +
>> + if (len > 0) {
>> + if (GetDiskFreeSpaceEx(mnt_point, 0, (PULARGE_INTEGER)&fs->size,
>> + (PULARGE_INTEGER)&fs->free) != 0) {
>
> Btw, it seems to me that checkpatch.pl returns some false positives
> here. See:
>
> ERROR: spaces required around that '&' (ctx:VxV)
> #72: FILE: qga/commands-win32.c:711:
> + if (GetDiskFreeSpaceEx(mnt_point, 0, (PULARGE_INTEGER)&fs->size,
> ^
Yes. PULARGE_INTEGER is not the normal qemu spelling for a typedef, so
it confuses checkpatch into thinking you are doing a binary operator
instead of a cast of a unary operator, hence a false positive not worth
worrying about.
> Also, checkpatch.pl itself suggest to look into MAINTAINERS to find out
> whom to report false positives, but there is no maintainer there. :)
Yes, that's an unfortunate problem - no one has volunteered to maintain
checkpatch, so it just gets sporadic patches.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] qga: report disk size and free space
2018-07-03 8:31 [Qemu-devel] [PATCH] qga: report disk size and free space Tomáš Golembiovský
2018-07-03 10:01 ` Marc-André Lureau
2018-07-03 10:54 ` Tomáš Golembiovský
@ 2018-07-03 11:39 ` no-reply
2018-07-03 16:24 ` Eric Blake
2018-07-10 17:58 ` Tomáš Golembiovský
4 siblings, 0 replies; 8+ messages in thread
From: no-reply @ 2018-07-03 11:39 UTC (permalink / raw)
To: tgolembi; +Cc: famz, qemu-devel, marcandre.lureau, mdroth
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 0b2c142da5d44a4b645dee47b7c50dae87a90a05.1530606672.git.tgolembi@redhat.com
Subject: [Qemu-devel] [PATCH] qga: report disk size and free space
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
c9d39c8274 qga: report disk size and free space
=== OUTPUT BEGIN ===
Checking PATCH 1/1: qga: report disk size and free space...
ERROR: spaces required around that '&' (ctx:VxV)
#67: FILE: qga/commands-win32.c:711:
+ if (GetDiskFreeSpaceEx(mnt_point, 0, (PULARGE_INTEGER)&fs->size,
^
ERROR: spaces required around that '&' (ctx:VxV)
#68: FILE: qga/commands-win32.c:712:
+ (PULARGE_INTEGER)&fs->free) != 0) {
^
total: 2 errors, 0 warnings, 73 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] qga: report disk size and free space
2018-07-03 8:31 [Qemu-devel] [PATCH] qga: report disk size and free space Tomáš Golembiovský
` (2 preceding siblings ...)
2018-07-03 11:39 ` no-reply
@ 2018-07-03 16:24 ` Eric Blake
2018-07-10 17:58 ` Tomáš Golembiovský
4 siblings, 0 replies; 8+ messages in thread
From: Eric Blake @ 2018-07-03 16:24 UTC (permalink / raw)
To: Tomáš Golembiovský, qemu-devel
Cc: Marc-André Lureau, Michael Roth
On 07/03/2018 03:31 AM, Tomáš Golembiovský wrote:
> Report total file system size and free space in output of command
> "guest-get-fsinfo". Values are optional and it is not an error if they cannot
> be retrieved for some reason.
>
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
> qga/commands-posix.c | 18 ++++++++++++++++++
> qga/commands-win32.c | 16 ++++++++++++++++
> qga/qapi-schema.json | 5 ++++-
> 3 files changed, 38 insertions(+), 1 deletion(-)
> +++ b/qga/qapi-schema.json
> @@ -848,12 +848,15 @@
> # @type: file system type string
> # @disk: an array of disk hardware information that the volume lies on,
> # which may be empty if the disk type is not supported
> +# @size: total number of bytes on the file system (Since 2.13)
> +# @free: number of bytes available on the file system (Since 2.13)
s/2.13/3.0/ (if it makes it into today's soft freeze, or 3.1 if not)
> #
> # Since: 2.2
> ##
> { 'struct': 'GuestFilesystemInfo',
> 'data': {'name': 'str', 'mountpoint': 'str', 'type': 'str',
> - 'disk': ['GuestDiskAddress']} }
> + 'disk': ['GuestDiskAddress'], '*size': 'uint64',
> + '*free': 'uint64'} }
>
> ##
> # @guest-get-fsinfo:
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] qga: report disk size and free space
2018-07-03 8:31 [Qemu-devel] [PATCH] qga: report disk size and free space Tomáš Golembiovský
` (3 preceding siblings ...)
2018-07-03 16:24 ` Eric Blake
@ 2018-07-10 17:58 ` Tomáš Golembiovský
4 siblings, 0 replies; 8+ messages in thread
From: Tomáš Golembiovský @ 2018-07-10 17:58 UTC (permalink / raw)
To: qemu-devel; +Cc: Marc-André Lureau, Michael Roth
On Tue, 3 Jul 2018 10:31:20 +0200
Tomáš Golembiovský <tgolembi@redhat.com> wrote:
> Report total file system size and free space in output of command
> "guest-get-fsinfo". Values are optional and it is not an error if they cannot
> be retrieved for some reason.
>
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
> qga/commands-posix.c | 18 ++++++++++++++++++
> qga/commands-win32.c | 16 ++++++++++++++++
> qga/qapi-schema.json | 5 ++++-
> 3 files changed, 38 insertions(+), 1 deletion(-)
Ignore the patch. I see that similar change was already applied by the
following patches:
https://lists.nongnu.org/archive/html/qemu-devel/2018-07/msg01004.html
https://lists.nongnu.org/archive/html/qemu-devel/2018-07/msg00995.html
Tomas
^ permalink raw reply [flat|nested] 8+ messages in thread