[PATCH 0/4] Miscellaneous fixes, somehow Windows related
blurb is here, blurb is there, blurby blurby everywhere Martin Kletzander (4): util: Parse RSS into ullp util: Remove virStrToLong_l util: Use g_ascii_strtoll meson-dist: Use shutil.copy for copying a file scripts/meson-dist.py | 7 ++-- src/libvirt_private.syms | 1 - src/qemu/qemu_driver.c | 2 +- src/util/virprocess.c | 17 ++++++---- src/util/virprocess.h | 2 +- src/util/virstring.c | 73 +++++++++++++++++++--------------------- src/util/virstring.h | 5 --- tests/virstringtest.c | 4 --- 8 files changed, 49 insertions(+), 62 deletions(-) -- 2.41.0
It is used to fill an unsigned long long anyway and if it is negative than there is really an issue somewhere. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 2 +- src/util/virprocess.c | 17 ++++++++++------- src/util/virprocess.h | 2 +- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 56f4cd619715..857fbfb7992c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9937,7 +9937,7 @@ qemuDomainMemoryStatsInternal(virDomainObj *vm, { int ret = -1; - long rss; + unsigned long long rss; if (virDomainObjCheckActive(vm) < 0) return -1; diff --git a/src/util/virprocess.c b/src/util/virprocess.c index d12fe4f7171f..a9f249413ce1 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -23,6 +23,7 @@ #include <config.h> #include <fcntl.h> +#include <limits.h> #include <signal.h> #ifndef WIN32 # include <sys/wait.h> @@ -1739,7 +1740,7 @@ virProcessGetStatInfo(unsigned long long *cpuTime, unsigned long long *userTime, unsigned long long *sysTime, int *lastCpu, - long *vm_rss, + unsigned long long *vm_rss, pid_t pid, pid_t tid) { @@ -1748,14 +1749,16 @@ virProcessGetStatInfo(unsigned long long *cpuTime, unsigned long long stime = 0; const unsigned long long jiff2nsec = 1000ull * 1000ull * 1000ull / (unsigned long long) sysconf(_SC_CLK_TCK); - long rss = 0; + long pagesize = virGetSystemPageSizeKB(); + unsigned long long rss = 0; int cpu = 0; if (!proc_stat || virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_UTIME], NULL, 10, &utime) < 0 || virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10, &stime) < 0 || - virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, &rss) < 0 || - virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, &cpu) < 0) { + virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, &rss) < 0 || + virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, &cpu) < 0 || + rss > ULLONG_MAX / pagesize) { VIR_WARN("cannot parse process status data"); } @@ -1771,10 +1774,10 @@ virProcessGetStatInfo(unsigned long long *cpuTime, *lastCpu = cpu; if (vm_rss) - *vm_rss = rss * virGetSystemPageSizeKB(); + *vm_rss = rss * pagesize; - VIR_DEBUG("Got status for %d/%d user=%llu sys=%llu cpu=%d rss=%ld", + VIR_DEBUG("Got status for %d/%d user=%llu sys=%llu cpu=%d rss=%lld", (int) pid, tid, utime, stime, cpu, rss); return 0; @@ -1851,7 +1854,7 @@ virProcessGetStatInfo(unsigned long long *cpuTime, unsigned long long *userTime, unsigned long long *sysTime, int *lastCpu, - long *vm_rss, + unsigned long long *vm_rss, pid_t pid G_GNUC_UNUSED, pid_t tid G_GNUC_UNUSED) { diff --git a/src/util/virprocess.h b/src/util/virprocess.h index 4e216788389f..c18f87e80ec9 100644 --- a/src/util/virprocess.h +++ b/src/util/virprocess.h @@ -198,7 +198,7 @@ int virProcessGetStatInfo(unsigned long long *cpuTime, unsigned long long *userTime, unsigned long long *sysTime, int *lastCpu, - long *vm_rss, + unsigned long long *vm_rss, pid_t pid, pid_t tid); int virProcessGetSchedInfo(unsigned long long *cpuWait, -- 2.41.0
On 6/12/23 09:55, Martin Kletzander wrote:
It is used to fill an unsigned long long anyway and if it is negative than there is really an issue somewhere.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 2 +- src/util/virprocess.c | 17 ++++++++++------- src/util/virprocess.h | 2 +- 3 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 56f4cd619715..857fbfb7992c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9937,7 +9937,7 @@ qemuDomainMemoryStatsInternal(virDomainObj *vm,
{ int ret = -1; - long rss; + unsigned long long rss;
if (virDomainObjCheckActive(vm) < 0) return -1; diff --git a/src/util/virprocess.c b/src/util/virprocess.c index d12fe4f7171f..a9f249413ce1 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -23,6 +23,7 @@ #include <config.h>
#include <fcntl.h> +#include <limits.h> #include <signal.h> #ifndef WIN32 # include <sys/wait.h> @@ -1739,7 +1740,7 @@ virProcessGetStatInfo(unsigned long long *cpuTime, unsigned long long *userTime, unsigned long long *sysTime, int *lastCpu, - long *vm_rss, + unsigned long long *vm_rss, pid_t pid, pid_t tid) { @@ -1748,14 +1749,16 @@ virProcessGetStatInfo(unsigned long long *cpuTime, unsigned long long stime = 0; const unsigned long long jiff2nsec = 1000ull * 1000ull * 1000ull / (unsigned long long) sysconf(_SC_CLK_TCK); - long rss = 0; + long pagesize = virGetSystemPageSizeKB();
const please. We don't really want anybody change this value.
+ unsigned long long rss = 0; int cpu = 0;
if (!proc_stat || virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_UTIME], NULL, 10, &utime) < 0 || virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10, &stime) < 0 || - virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, &rss) < 0 || - virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, &cpu) < 0) { + virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, &rss) < 0 || + virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, &cpu) < 0 || + rss > ULLONG_MAX / pagesize) { VIR_WARN("cannot parse process status data"); }
Michal
On Mon, Jun 12, 2023 at 10:25:26AM +0200, Michal Prívozník wrote:
On 6/12/23 09:55, Martin Kletzander wrote:
It is used to fill an unsigned long long anyway and if it is negative than there is really an issue somewhere.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_driver.c | 2 +- src/util/virprocess.c | 17 ++++++++++------- src/util/virprocess.h | 2 +- 3 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 56f4cd619715..857fbfb7992c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9937,7 +9937,7 @@ qemuDomainMemoryStatsInternal(virDomainObj *vm,
{ int ret = -1; - long rss; + unsigned long long rss;
if (virDomainObjCheckActive(vm) < 0) return -1; diff --git a/src/util/virprocess.c b/src/util/virprocess.c index d12fe4f7171f..a9f249413ce1 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -23,6 +23,7 @@ #include <config.h>
#include <fcntl.h> +#include <limits.h> #include <signal.h> #ifndef WIN32 # include <sys/wait.h> @@ -1739,7 +1740,7 @@ virProcessGetStatInfo(unsigned long long *cpuTime, unsigned long long *userTime, unsigned long long *sysTime, int *lastCpu, - long *vm_rss, + unsigned long long *vm_rss, pid_t pid, pid_t tid) { @@ -1748,14 +1749,16 @@ virProcessGetStatInfo(unsigned long long *cpuTime, unsigned long long stime = 0; const unsigned long long jiff2nsec = 1000ull * 1000ull * 1000ull / (unsigned long long) sysconf(_SC_CLK_TCK); - long rss = 0; + long pagesize = virGetSystemPageSizeKB();
const please. We don't really want anybody change this value.
Of course, if someone changed the page size the machine would clearly crash =D But in all seriousness, I'll change that, but don't really see the point, especially in C.
+ unsigned long long rss = 0; int cpu = 0;
if (!proc_stat || virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_UTIME], NULL, 10, &utime) < 0 || virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_STIME], NULL, 10, &stime) < 0 || - virStrToLong_l(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, &rss) < 0 || - virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, &cpu) < 0) { + virStrToLong_ullp(proc_stat[VIR_PROCESS_STAT_RSS], NULL, 10, &rss) < 0 || + virStrToLong_i(proc_stat[VIR_PROCESS_STAT_PROCESSOR], NULL, 10, &cpu) < 0 || + rss > ULLONG_MAX / pagesize) { VIR_WARN("cannot parse process status data"); }
Michal
With the last user gone this function can be abolished. It is preferable to use _ll instead since that is not a subject to 32/64 bit scaling. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/libvirt_private.syms | 1 - src/util/virstring.c | 19 +------------------ src/util/virstring.h | 5 ----- tests/virstringtest.c | 4 ---- 4 files changed, 1 insertion(+), 28 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 436d5a07702e..fb7ad9c85511 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3414,7 +3414,6 @@ virStringToUpper; virStringTrimOptionalNewline; virStrToDouble; virStrToLong_i; -virStrToLong_l; virStrToLong_ll; virStrToLong_ui; virStrToLong_uip; diff --git a/src/util/virstring.c b/src/util/virstring.c index d9c9c8f73807..635685eed4a4 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -113,24 +113,7 @@ virStrToLong_uip(char const *s, char **end_ptr, int base, unsigned int *result) return 0; } -/* Just like virStrToLong_i, above, but produce a "long" value. */ -int -virStrToLong_l(char const *s, char **end_ptr, int base, long *result) -{ - long int val; - char *p; - int err; - - errno = 0; - val = strtol(s, &p, base); /* exempt from syntax-check */ - err = (errno || (!end_ptr && *p) || p == s); - if (end_ptr) - *end_ptr = p; - if (err) - return -1; - *result = val; - return 0; -} +/* virStrToLong_l is intentionally skipped, consider virStrToLong_ll instead */ /* Just like virStrToLong_i, above, but produce an "unsigned long" * value. This version allows twos-complement wraparound of negative diff --git a/src/util/virstring.h b/src/util/virstring.h index 0f8b5d066425..16dcce98f42a 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -38,11 +38,6 @@ int virStrToLong_uip(char const *s, int base, unsigned int *result) G_GNUC_WARN_UNUSED_RESULT; -int virStrToLong_l(char const *s, - char **end_ptr, - int base, - long *result) - G_GNUC_WARN_UNUSED_RESULT; int virStrToLong_ul(char const *s, char **end_ptr, int base, diff --git a/tests/virstringtest.c b/tests/virstringtest.c index 7d8d9204cce0..6e697cc240e6 100644 --- a/tests/virstringtest.c +++ b/tests/virstringtest.c @@ -258,7 +258,6 @@ testStringToLong(const void *opaque) const struct stringToLongData *data = opaque; int ret = 0; char *end; - long l; unsigned long ul; bool negative; @@ -308,9 +307,6 @@ testStringToLong(const void *opaque) /* We hate adding new API with 'long', and prefer 'int' or 'long * long' instead, since platform-specific results are evil */ - l = (sizeof(int) == sizeof(long)) ? data->si : data->ll; - TEST_ONE(data->str, data->suffix, long, l, "%ld", - l, (sizeof(int) == sizeof(long)) ? data->si_ret : data->ll_ret); ul = (sizeof(int) == sizeof(long)) ? data->ui : data->ull; TEST_ONE(data->str, data->suffix, unsigned long, ul, "%lu", ul, (sizeof(int) == sizeof(long)) ? data->ui_ret : data->ull_ret); -- 2.41.0
This has two main advantages: - it parses the number with C locale explicitly - it behaves the same on Windows as on Linux and BSD both of which are wanted behaviours. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virstring.c | 54 +++++++++++++++++++++++++++----------------- 1 file changed, 33 insertions(+), 21 deletions(-) diff --git a/src/util/virstring.c b/src/util/virstring.c index 635685eed4a4..6b728ff04759 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -35,7 +35,7 @@ VIR_LOG_INIT("util.string"); -/* Like strtol, but produce an "int" result, and check more carefully. +/* Like strtol with C locale, but produce an "int" result, and check more carefully. Return 0 upon success; return -1 to indicate failure. When END_PTR is NULL, the byte after the final valid digit must be NUL. Otherwise, it's like strtol and lets the caller check any suffix for @@ -44,12 +44,12 @@ VIR_LOG_INIT("util.string"); int virStrToLong_i(char const *s, char **end_ptr, int base, int *result) { - long int val; + long long val; char *p; int err; errno = 0; - val = strtol(s, &p, base); /* exempt from syntax-check */ + val = g_ascii_strtoll(s, &p, base); err = (errno || (!end_ptr && *p) || p == s || (int) val != val); if (end_ptr) *end_ptr = p; @@ -65,22 +65,22 @@ virStrToLong_i(char const *s, char **end_ptr, int base, int *result) int virStrToLong_ui(char const *s, char **end_ptr, int base, unsigned int *result) { - unsigned long int val; + unsigned long long val; char *p; bool err = false; errno = 0; - val = strtoul(s, &p, base); /* exempt from syntax-check */ + val = g_ascii_strtoull(s, &p, base); /* This one's tricky. We _want_ to allow "-1" as shorthand for * UINT_MAX regardless of whether long is 32-bit or 64-bit. But - * strtoul treats "-1" as ULONG_MAX, and going from ulong back - * to uint differs depending on the size of long. */ - if (sizeof(long) > sizeof(int) && memchr(s, '-', p - s)) { + * g_ascii_strtoull treats "-1" as ULLONG_MAX, and going from ullong back + * to uint differs depending on the size of uint. */ + if (memchr(s, '-', p - s)) { if (-val > UINT_MAX) err = true; else - val &= 0xffffffff; + val &= UINT_MAX; } err |= (errno || (!end_ptr && *p) || p == s || (unsigned int) val != val); @@ -97,12 +97,12 @@ virStrToLong_ui(char const *s, char **end_ptr, int base, unsigned int *result) int virStrToLong_uip(char const *s, char **end_ptr, int base, unsigned int *result) { - unsigned long int val; + unsigned long long val; char *p; bool err = false; errno = 0; - val = strtoul(s, &p, base); /* exempt from syntax-check */ + val = g_ascii_strtoull(s, &p, base); err = (memchr(s, '-', p - s) || errno || (!end_ptr && *p) || p == s || (unsigned int) val != val); if (end_ptr) @@ -121,13 +121,25 @@ virStrToLong_uip(char const *s, char **end_ptr, int base, unsigned int *result) int virStrToLong_ul(char const *s, char **end_ptr, int base, unsigned long *result) { - unsigned long int val; + unsigned long long val; char *p; - int err; + bool err = false; errno = 0; - val = strtoul(s, &p, base); /* exempt from syntax-check */ - err = (errno || (!end_ptr && *p) || p == s); + val = g_ascii_strtoull(s, &p, base); + + /* This one's tricky. We _want_ to allow "-1" as shorthand for + * ULONG_MAX regardless of whether long is 32-bit or 64-bit. But + * g_ascii_strtoull treats "-1" as ULLONG_MAX, and going from ullong back + * to ulong differs depending on the size of ulong. */ + if (memchr(s, '-', p - s)) { + if (-val > ULONG_MAX) + err = true; + else + val &= ULONG_MAX; + } + + err |= (errno || (!end_ptr && *p) || p == s || (unsigned long) val != val); if (end_ptr) *end_ptr = p; if (err) @@ -142,14 +154,14 @@ int virStrToLong_ulp(char const *s, char **end_ptr, int base, unsigned long *result) { - unsigned long int val; + unsigned long long val; char *p; int err; errno = 0; - val = strtoul(s, &p, base); /* exempt from syntax-check */ + val = g_ascii_strtoull(s, &p, base); err = (memchr(s, '-', p - s) || - errno || (!end_ptr && *p) || p == s); + errno || (!end_ptr && *p) || p == s || (unsigned long) val != val); if (end_ptr) *end_ptr = p; if (err) @@ -167,7 +179,7 @@ virStrToLong_ll(char const *s, char **end_ptr, int base, long long *result) int err; errno = 0; - val = strtoll(s, &p, base); /* exempt from syntax-check */ + val = g_ascii_strtoll(s, &p, base); err = (errno || (!end_ptr && *p) || p == s); if (end_ptr) *end_ptr = p; @@ -189,7 +201,7 @@ virStrToLong_ull(char const *s, char **end_ptr, int base, int err; errno = 0; - val = strtoull(s, &p, base); /* exempt from syntax-check */ + val = g_ascii_strtoull(s, &p, base); err = (errno || (!end_ptr && *p) || p == s); if (end_ptr) *end_ptr = p; @@ -210,7 +222,7 @@ virStrToLong_ullp(char const *s, char **end_ptr, int base, int err; errno = 0; - val = strtoull(s, &p, base); /* exempt from syntax-check */ + val = g_ascii_strtoull(s, &p, base); err = (memchr(s, '-', p - s) || errno || (!end_ptr && *p) || p == s); if (end_ptr) -- 2.41.0
Using os.system("cp {0} {1}".format(...)) has two issues, it does not work on Windows, but more importantly it can cause issues in case one of the directories has a space in it. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- scripts/meson-dist.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/scripts/meson-dist.py b/scripts/meson-dist.py index a1d36c2533da..1c2364f6b67e 100755 --- a/scripts/meson-dist.py +++ b/scripts/meson-dist.py @@ -2,13 +2,12 @@ import os import sys +import shutil meson_build_root = sys.argv[1] file_name = sys.argv[2] meson_dist_root = os.environ['MESON_DIST_ROOT'] -os.system('cp {0} {1}'.format( - os.path.join(meson_build_root, file_name), - os.path.join(meson_dist_root, file_name) -)) +shutil.copy(os.path.join(meson_build_root, file_name), + os.path.join(meson_dist_root, file_name)) -- 2.41.0
On 6/12/23 09:55, Martin Kletzander wrote:
blurb is here, blurb is there, blurby blurby everywhere
Martin Kletzander (4): util: Parse RSS into ullp util: Remove virStrToLong_l util: Use g_ascii_strtoll meson-dist: Use shutil.copy for copying a file
scripts/meson-dist.py | 7 ++-- src/libvirt_private.syms | 1 - src/qemu/qemu_driver.c | 2 +- src/util/virprocess.c | 17 ++++++---- src/util/virprocess.h | 2 +- src/util/virstring.c | 73 +++++++++++++++++++--------------------- src/util/virstring.h | 5 --- tests/virstringtest.c | 4 --- 8 files changed, 49 insertions(+), 62 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Martin Kletzander -
Michal Prívozník