Message ID | 0b2c142da5d44a4b645dee47b7c50dae87a90a05.1530606672.git.tgolembi@redhat.com |
---|---|
State | New |
Headers | show |
Series | qga: report disk size and free space | expand |
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 > >
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
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
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
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: >
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.
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
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\")", + mount->dirname); + slog("failed to stat filesystem: %s", + error_get_pretty(local_err)); + error_free(local_err); + } 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) { + /* 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: 2.2 ## { 'struct': 'GuestFilesystemInfo', 'data': {'name': 'str', 'mountpoint': 'str', 'type': 'str', - 'disk': ['GuestDiskAddress']} } + 'disk': ['GuestDiskAddress'], '*size': 'uint64', + '*free': 'uint64'} } ## # @guest-get-fsinfo:
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(-)