Re: [PATCH 8/8] build: Decrease maximum stack frame size to 2048
On 8/30/23 13:59, Peter Krempa wrote:
> After recent cleanups we can now restrict the maximum stack frame size
> to 2k.
>
> Signed-off-by: Peter Krempa
> ---
> meson.build | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/meson.build b/meson.build
> index 965ada483b..e45f3e2c39 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -248,7 +248,7 @@ alloc_max = run_command(
> )
>
> # sanitizer instrumentation may enlarge stack frames
> -stack_frame_size = get_option('b_sanitize') == 'none' ? 4096 : 32768
> +stack_frame_size = get_option('b_sanitize') == 'none' ? 2048 : 32768
>
> # array_bounds=2 check triggers false positive on some GCC
> # versions when using sanitizers. Seen on Fedora 34 with
I know this is already pushed but to be honest, I don't really
understand why this is needed. I mean, we certainly do not want large
frames, but IIUC only frames larger than a page are problem (i.e. 4KiB).
Can you please point me to some docs where I can learn more?
Michal
Re: [PATCH] conf: Generate MAC address instead of keeping all zeroes
On Mon, Sep 04, 2023 at 02:34:49PM +0200, Michal Prívozník wrote:
On 9/1/23 17:12, Martin Kletzander wrote:
When we parse we keep that in memory
and pass it down to the hypervisor. However, that MAC address is not
strictly valid as it is not marked as locally administered (bit 0x02)
but it is not even globally unique. It is also used for loopback device
on Linux, for example. And QEMU sees such MAC address just as "not
specified" and generates a new one that libvirt does not even know
about. So to make the overall experience better we now generate it if
the supplied one is all clear.
Please consider s/ / /g
OK, I'll do it for you this time ;) But don't expect me to unlearn this
old monospace habit any time soon.
Resolves: https://issues.redhat.com/browse/RHEL-974
Signed-off-by: Martin Kletzander
---
src/conf/domain_conf.c| 2 +-
src/util/virmacaddr.c | 5 +
src/util/virmacaddr.h | 1 +
.../network-interface-mac-clear.xml | 21 +++
.../network-interface-mac-clear.xml | 21 +++
tests/genericxml2xmltest.c| 4 +++-
6 files changed, 52 insertions(+), 2 deletions(-)
create mode 100644 tests/genericxml2xmlindata/network-interface-mac-clear.xml
create mode 100644 tests/genericxml2xmloutdata/network-interface-mac-clear.xml
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index bb4f1fdb948d..652bd09b21b8 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -9675,7 +9675,7 @@ virDomainNetDefParseXML(virDomainXMLOption *xmlopt,
return NULL;
}
-if (!macaddr) {
+if (!macaddr || virMacAddrIsAllClear(&def->mac)) {
virDomainNetGenerateMAC(xmlopt, &def->mac);
def->mac_generated = true;
}
diff --git a/src/util/virmacaddr.c b/src/util/virmacaddr.c
index 073f298b5b66..e06bb200fc68 100644
--- a/src/util/virmacaddr.c
+++ b/src/util/virmacaddr.c
@@ -246,6 +246,11 @@ virMacAddrIsBroadcastRaw(const unsigned char
s[VIR_MAC_BUFLEN])
return memcmp(virMacAddrBroadcastAddrRaw, s, sizeof(*s)) == 0;
}
+bool virMacAddrIsAllClear(const virMacAddr *addr)
+{
+return !virMacAddrCmpRaw(addr, (const unsigned char[VIR_MAC_BUFLEN]){0});
+}
+
void
virMacAddrFree(virMacAddr *addr)
{
diff --git a/src/util/virmacaddr.h b/src/util/virmacaddr.h
index f32b58805a61..7b9eb7443bd1 100644
--- a/src/util/virmacaddr.h
+++ b/src/util/virmacaddr.h
@@ -58,6 +58,7 @@ int virMacAddrParseHex(const char* str,
bool virMacAddrIsUnicast(const virMacAddr *addr);
bool virMacAddrIsMulticast(const virMacAddr *addr);
bool virMacAddrIsBroadcastRaw(const unsigned char s[VIR_MAC_BUFLEN]);
+bool virMacAddrIsAllClear(const virMacAddr *addr);
void virMacAddrFree(virMacAddr *addr);
G_DEFINE_AUTOPTR_CLEANUP_FUNC(virMacAddr, virMacAddrFree);
Please expose the symbol in src/libvirt_private.syms too.
OK, done and pushed, thanks.
Reviewed-by: Michal Privoznik
Michal
signature.asc
Description: PGP signature
Re: [PATCH Libvirt v2 00/10] Support dirty page rate upper limit
Ping2, I'm hoping for comments about the series. Thanks, Yong On Sun, Aug 27, 2023 at 11:11 AM Yong Huang wrote: > Ping1 > > On Tue, Aug 15, 2023 at 9:48 AM Yong Huang wrote: > >> Ping. >> >> On Mon, Aug 7, 2023 at 11:56 PM ~hyman wrote: >> >>> Hi, This is the latest version for the series, comparing with version >>> 1, there are some key modifications has made inspired and >>> suggested by Peter, see as follows: >>> 1. Introduce XML for dirty limit persistent configuration >>> 2. Merge the cancel API into the set API >>> 3. Extend the domstats/virDomainListGetStats API for dirty limit >>> information query >>> 4. Introduce the virDomainModificationImpact flags to control the >>> behavior of the API >>> 5. Enrich the comments and docs about the feature and API >>> >>> The patch set introduce the new API virDomainSetVcpuDirtyLimit to >>> allow upper Apps to set upper limits of dirty page rate for virtual >>> CPUs, >>> the corresponding virsh API as follows: >>> # limit-dirty-page-rate [--vcpu ] \ >>> [--config] [--live] [--current] >>> >>> We put the dirty limit persistent info with the "vcpus" element in >>> domain XML and >>> extend dirtylimit statistics for domGetStats: >>> >>> ... >>> 3 >>> >>> >>> >>> >>> ... >>> >>> If --vcpu option is not passed in the virsh command, set all virtual >>> CPUs; >>> if rate is set to zero, cancel the upper limit. >>> >>> Examples: >>> To set the dirty page rate upper limit 10 MB/s for all virtual CPUs in >>> c81_node1, use: >>> [root@srv2 my_libvirt]# virsh limit-dirty-page-rate c81_node1 --rate 10 >>> --live >>> Set dirty page rate limit 10(MB/s) for all virtual CPUs successfully >>> >>> [root@srv2 my_libvirt]# virsh dumpxml c81_node1 | grep dirty_limit >>> >> dirty_limit='10'/> >>> >> dirty_limit='10'/> >>> >> dirty_limit='10'/> >>> >>> >>> .. >>> >>> Query the dirty limit info dynamically: >>> [root@srv2 my_libvirt]# virsh domstats c81_node1 --dirtylimit >>> Domain: 'c81_node1' >>> dirtylimit.vcpu.0.limit=10 >>> dirtylimit.vcpu.0.current=0 >>> dirtylimit.vcpu.1.limit=10 >>> dirtylimit.vcpu.1.current=0 >>> dirtylimit.vcpu.2.limit=10 >>> dirtylimit.vcpu.2.current=0 >>> dirtylimit.vcpu.3.limit=10 >>> dirtylimit.vcpu.3.current=0 >>> dirtylimit.vcpu.4.limit=10 >>> dirtylimit.vcpu.4.current=0 >>> .. >>> To cancel the upper limit, use: >>> [root@srv2 my_libvirt]# virsh limit-dirty-page-rate c81_node1 \ >>> --rate 0 --live >>> Cancel dirty page rate limit for all virtual CPUs successfully >>> >>> [root@srv2 my_libvirt]# virsh dumpxml c81_node1 | grep dirty_limit >>> [root@srv2 my_libvirt]# virsh domstats c81_node1 --dirtylimit >>> Domain: 'c81_node1' >>> >>> The dirty limit uses the QEMU dirty-limit feature introduced since >>> 7.1.0, this feature allows CPU to be throttled as needed to keep >>> their dirty page rate within the limit. It could, in some scenes, be >>> used to provide quality-of-service in the aspect of the memory >>> workload for virtual CPUs and QEMU itself use the feature to >>> implement the dirty-limit throttle algorithm and apply it on the >>> live migration, which improve responsiveness of large guests >>> during live migration and can result in more stable read >>> performance. The other application scenarios remain >>> unexplored, before that, Libvirt could provide the basic API. >>> >>> Please review, thanks >>> >>> Yong >>> >>> Hyman Huang(黄勇) (10): >>> qemu_capabilities: Introduce QEMU_CAPS_VCPU_DIRTY_LIMIT capability >>> conf: Introduce XML for dirty limit configuration >>> libvirt: Add virDomainSetVcpuDirtyLimit API >>> qemu_driver: Implement qemuDomainSetVcpuDirtyLimit >>> domain_validate: Export virDomainDefHasDirtyLimitStartupVcpus symbol >>> qemu_process: Setup dirty limit after launching VM >>> virsh: Introduce limit-dirty-page-rate api >>> qemu_monitor: Implement qemuMonitorQueryVcpuDirtyLimit >>> qemu_driver: Extend dirtlimit statistics for domGetStats >>> virsh: Introduce command 'virsh domstats --dirtylimit' >>> >>> docs/formatdomain.rst | 7 +- >>> docs/manpages/virsh.rst | 33 +++- >>> include/libvirt/libvirt-domain.h | 5 + >>> src/conf/domain_conf.c| 26 +++ >>> src/conf/domain_conf.h| 8 + >>> src/conf/domain_validate.c| 33 >>> src/conf/domain_validate.h| 2 + >>> src/conf/schemas/domaincommon.rng | 5 + >>> src/driver-hypervisor.h | 7 + >>> src/libvirt-domain.c | 68 +++ >>> src/libvirt_private.syms | 1 + >>> src/libvirt_public.syms | 5 + >>> src/qemu/qemu_capabilities.c | 2 + >>> src/qemu/qemu_capabilities.h | 1 + >>> src/qemu/qemu_driver.c| 181
[PATCH (pushed)] build: Fix logic bug determining whether running with optimization
The conversion from ternary to a 'if' clause was wrong and thus didn't
properly increase the stack size where needed but only where not
actually needed.
Fixes: b68faa99d9f16c2f504b23737040d25d072ee85d
Signed-off-by: Peter Krempa
---
meson.build | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index e0ee4f2f21..3bf15d93a4 100644
--- a/meson.build
+++ b/meson.build
@@ -255,7 +255,7 @@ if cc.get_id() == 'clang' and get_option('optimization') ==
'0'
endif
# sanitizer instrumentation may enlarge stack frames
-if get_option('b_sanitize') == 'none'
+if get_option('b_sanitize') != 'none'
stack_frame_size = 32768
endif
--
2.41.0
Re: [PATCH] conf: Generate MAC address instead of keeping all zeroes
On 9/1/23 17:12, Martin Kletzander wrote:
> When we parse we keep that in memory
> and pass it down to the hypervisor. However, that MAC address is not
> strictly valid as it is not marked as locally administered (bit 0x02)
> but it is not even globally unique. It is also used for loopback device
> on Linux, for example. And QEMU sees such MAC address just as "not
> specified" and generates a new one that libvirt does not even know
> about. So to make the overall experience better we now generate it if
> the supplied one is all clear.
Please consider s/ / /g
>
> Resolves: https://issues.redhat.com/browse/RHEL-974
>
> Signed-off-by: Martin Kletzander
> ---
> src/conf/domain_conf.c| 2 +-
> src/util/virmacaddr.c | 5 +
> src/util/virmacaddr.h | 1 +
> .../network-interface-mac-clear.xml | 21 +++
> .../network-interface-mac-clear.xml | 21 +++
> tests/genericxml2xmltest.c| 4 +++-
> 6 files changed, 52 insertions(+), 2 deletions(-)
> create mode 100644 tests/genericxml2xmlindata/network-interface-mac-clear.xml
> create mode 100644
> tests/genericxml2xmloutdata/network-interface-mac-clear.xml
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index bb4f1fdb948d..652bd09b21b8 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -9675,7 +9675,7 @@ virDomainNetDefParseXML(virDomainXMLOption *xmlopt,
> return NULL;
> }
>
> -if (!macaddr) {
> +if (!macaddr || virMacAddrIsAllClear(&def->mac)) {
> virDomainNetGenerateMAC(xmlopt, &def->mac);
> def->mac_generated = true;
> }
> diff --git a/src/util/virmacaddr.c b/src/util/virmacaddr.c
> index 073f298b5b66..e06bb200fc68 100644
> --- a/src/util/virmacaddr.c
> +++ b/src/util/virmacaddr.c
> @@ -246,6 +246,11 @@ virMacAddrIsBroadcastRaw(const unsigned char
> s[VIR_MAC_BUFLEN])
> return memcmp(virMacAddrBroadcastAddrRaw, s, sizeof(*s)) == 0;
> }
>
> +bool virMacAddrIsAllClear(const virMacAddr *addr)
> +{
> +return !virMacAddrCmpRaw(addr, (const unsigned char[VIR_MAC_BUFLEN]){0});
> +}
> +
> void
> virMacAddrFree(virMacAddr *addr)
> {
> diff --git a/src/util/virmacaddr.h b/src/util/virmacaddr.h
> index f32b58805a61..7b9eb7443bd1 100644
> --- a/src/util/virmacaddr.h
> +++ b/src/util/virmacaddr.h
> @@ -58,6 +58,7 @@ int virMacAddrParseHex(const char* str,
> bool virMacAddrIsUnicast(const virMacAddr *addr);
> bool virMacAddrIsMulticast(const virMacAddr *addr);
> bool virMacAddrIsBroadcastRaw(const unsigned char s[VIR_MAC_BUFLEN]);
> +bool virMacAddrIsAllClear(const virMacAddr *addr);
> void virMacAddrFree(virMacAddr *addr);
>
> G_DEFINE_AUTOPTR_CLEANUP_FUNC(virMacAddr, virMacAddrFree);
Please expose the symbol in src/libvirt_private.syms too.
Reviewed-by: Michal Privoznik
Michal
[PATCH (pushed)] build: Fix assignment into 'stack_frame_size' when sanitizer is enabled
Instead of an assignment into the 'stack_frame_size' variable when
sanitizers are enabled I've accidentally compared the value against the
requested size.
Fix the typo.
Fixes: b68faa99d9f16c2f504b23737040d25d072ee85d
Signed-off-by: Peter Krempa
---
meson.build | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index d29761bc71..e0ee4f2f21 100644
--- a/meson.build
+++ b/meson.build
@@ -256,7 +256,7 @@ endif
# sanitizer instrumentation may enlarge stack frames
if get_option('b_sanitize') == 'none'
- stack_frame_size == 32768
+ stack_frame_size = 32768
endif
# array_bounds=2 check triggers false positive on some GCC
--
2.41.0
Re: [PATCH 2/2] build: Work around clang's stack size calculation without optimization
On Mon, Sep 04, 2023 at 12:13:37PM +0200, Peter Krempa wrote: > When building without optimization on clang, certain big functions trip > the stack size limit despite not actually reaching it. Relax the stack > limit size for clang without optimization. > > Signed-off-by: Peter Krempa > --- > meson.build | 11 ++- > 1 file changed, 10 insertions(+), 1 deletion(-) Reviewed-by: Daniel P. Berrangé With 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: [PATCH 1/2] docs: compiling: Add a note about use of CFLAGS for optimization
On Mon, Sep 04, 2023 at 12:13:36PM +0200, Peter Krempa wrote: > Meson doesn't interpret what's set in CFLAGS, but rather simply appeds > it to the command line. Thus any logic which is based on the > optimization level will not work. > > Note the caveat in the docs and instruct users to use > ``--optimization=N`` instead. > > Signed-off-by: Peter Krempa > --- > docs/compiling.rst | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/docs/compiling.rst b/docs/compiling.rst > index 800264d2f9..0a47a50569 100644 > --- a/docs/compiling.rst > +++ b/docs/compiling.rst > @@ -112,6 +112,11 @@ Please ensure that you have the appropriate minimal > ``meson`` version installed > in your build environment. The minimal version for a specific package can be > checked in the top level ``meson.build`` file in the ``meson_version`` field. > > +**DO NOT** use the ``CFLAGS`` environment variable to set optimizations > +(e.g. ``CFLAGS=-O0``), but rather use Meson's ``--optimization=0`` option. > +Certain internal build options are based on the configured optimization value > +and Meson does not interpret ``CFLAGS``. Reviewed-by: Daniel P. Berrangé With 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 :|
[PATCH 2/2] build: Work around clang's stack size calculation without optimization
When building without optimization on clang, certain big functions trip
the stack size limit despite not actually reaching it. Relax the stack
limit size for clang without optimization.
Signed-off-by: Peter Krempa
---
meson.build | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index 6b6f7ccb7c..d29761bc71 100644
--- a/meson.build
+++ b/meson.build
@@ -247,8 +247,17 @@ alloc_max = run_command(
check: true,
)
+stack_frame_size = 2048
+
+# clang without optimization enlarges stack frames in certain corner cases
+if cc.get_id() == 'clang' and get_option('optimization') == '0'
+stack_frame_size = 4096
+endif
+
# sanitizer instrumentation may enlarge stack frames
-stack_frame_size = get_option('b_sanitize') == 'none' ? 2048 : 32768
+if get_option('b_sanitize') == 'none'
+ stack_frame_size == 32768
+endif
# array_bounds=2 check triggers false positive on some GCC
# versions when using sanitizers. Seen on Fedora 34 with
--
2.41.0
[PATCH 0/2] build: Fix build with clang without optimization
Add a note for users how optimization is supposed to be controled with meson and fix the build by using 4k stack flame on clang without optimization. Peter Krempa (2): docs: compiling: Add a note about use of CFLAGS for optimization build: Work around clang's stack size calculation without optimization docs/compiling.rst | 5 + meson.build| 11 ++- 2 files changed, 15 insertions(+), 1 deletion(-) -- 2.41.0
[PATCH 1/2] docs: compiling: Add a note about use of CFLAGS for optimization
Meson doesn't interpret what's set in CFLAGS, but rather simply appeds it to the command line. Thus any logic which is based on the optimization level will not work. Note the caveat in the docs and instruct users to use ``--optimization=N`` instead. Signed-off-by: Peter Krempa --- docs/compiling.rst | 5 + 1 file changed, 5 insertions(+) diff --git a/docs/compiling.rst b/docs/compiling.rst index 800264d2f9..0a47a50569 100644 --- a/docs/compiling.rst +++ b/docs/compiling.rst @@ -112,6 +112,11 @@ Please ensure that you have the appropriate minimal ``meson`` version installed in your build environment. The minimal version for a specific package can be checked in the top level ``meson.build`` file in the ``meson_version`` field. +**DO NOT** use the ``CFLAGS`` environment variable to set optimizations +(e.g. ``CFLAGS=-O0``), but rather use Meson's ``--optimization=0`` option. +Certain internal build options are based on the configured optimization value +and Meson does not interpret ``CFLAGS``. + Compiling the sources - -- 2.41.0
Re: [PATCH] Revert "build: Decrease maximum stack frame size to 2048"
On Mon, Sep 04, 2023 at 11:23:17AM +0200, Peter Krempa wrote:
> Build fails with this patch with 'clang' when optimizations are
> disabled in long functions which have many helper variables declared in
> nested blocks such as for-loops.
>
> As there is no clean solution for now, let's keep the stack frame size
> at 4k.
>
> This reverts commit 42bc76cdb8486ef502200f3bce9e3faebdd78103.
>
> Signed-off-by: Peter Krempa
> ---
> meson.build | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/meson.build b/meson.build
> index 6b6f7ccb7c..dd7a4e8d34 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -248,7 +248,7 @@ alloc_max = run_command(
> )
>
> # sanitizer instrumentation may enlarge stack frames
> -stack_frame_size = get_option('b_sanitize') == 'none' ? 2048 : 32768
> +stack_frame_size = get_option('b_sanitize') == 'none' ? 4096 : 32768
You could potentially do a check on get_option('optimization') too ?
With 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 :|
[PATCH] Revert "build: Decrease maximum stack frame size to 2048"
Build fails with this patch with 'clang' when optimizations are
disabled in long functions which have many helper variables declared in
nested blocks such as for-loops.
As there is no clean solution for now, let's keep the stack frame size
at 4k.
This reverts commit 42bc76cdb8486ef502200f3bce9e3faebdd78103.
Signed-off-by: Peter Krempa
---
meson.build | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index 6b6f7ccb7c..dd7a4e8d34 100644
--- a/meson.build
+++ b/meson.build
@@ -248,7 +248,7 @@ alloc_max = run_command(
)
# sanitizer instrumentation may enlarge stack frames
-stack_frame_size = get_option('b_sanitize') == 'none' ? 2048 : 32768
+stack_frame_size = get_option('b_sanitize') == 'none' ? 4096 : 32768
# array_bounds=2 check triggers false positive on some GCC
# versions when using sanitizers. Seen on Fedora 34 with
--
2.41.0
Re: [PATCH 2/2] syntax-check: Introduce a rule for one line error messages
On Mon, Sep 04, 2023 at 10:46:17 +0200, Peter Krempa wrote:
> On Mon, Sep 04, 2023 at 10:31:48 +0200, Michal Privoznik wrote:
> > Okay, this is a shortcut. Our coding style says that error
> > messages are exempt from '80 chars long lines' rule. But in the
> > very same paragraph it is said that all error messages need to be
> > marked for translation (as they might be presented to user).
> >
> > Therefore, the syntax-check rule can check if _("...") is
> > formatted on one line. With exception of _("...\n" ...) (e.g.
> > various outputs from helper binaries like leaseshelper,
> > sshhelper, or daemons like lockd, logd). I believe nobody would
> > chose a substring that contains '\n' for git grep-ping the error
> > message.
> >
> > Signed-off-by: Michal Privoznik
> > ---
> > build-aux/syntax-check.mk | 8
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
> > index 64c1e2773e..618c0546aa 100644
> > --- a/build-aux/syntax-check.mk
> > +++ b/build-aux/syntax-check.mk
> > @@ -440,6 +440,11 @@ sc_prohibit_newline_at_end_of_diagnostic:
> > && { echo 'newline at end of message(s)' 1>&2; \
> > exit 1; } || :
> >
> > +sc_prohibit_error_message_on_multiple_lines:
> > + @prohibit='[^N]_\(".*[^\\n]"$$' \
>
> This doesn't work as you expect it to work, because the [] operator(?)
> in regex defines a character set to match . Thus you are exempting any
> message which ends with a backslash _OR_ with an 'n' rather than ending
> with "\n".
The simplest solution (rather than trying to fiddle with negative
lookahead) seems to be:
sc_prohibit_error_message_on_multiple_lines:
@prohibit='[^N]_\(".*"$$' \
exclude='\\n"$$' \
halt='found error message on multiple lines' \
$(_sc_search_regexp)
And perhaps adding a comment explaining what it actually does, so the
next guy doesn't have to try to understand it.
With the above tweak and comment added:
Reviewed-by: Peter Krempa
Re: [PATCH 2/2] syntax-check: Introduce a rule for one line error messages
On Mon, Sep 04, 2023 at 10:31:48 +0200, Michal Privoznik wrote:
> Okay, this is a shortcut. Our coding style says that error
> messages are exempt from '80 chars long lines' rule. But in the
> very same paragraph it is said that all error messages need to be
> marked for translation (as they might be presented to user).
>
> Therefore, the syntax-check rule can check if _("...") is
> formatted on one line. With exception of _("...\n" ...) (e.g.
> various outputs from helper binaries like leaseshelper,
> sshhelper, or daemons like lockd, logd). I believe nobody would
> chose a substring that contains '\n' for git grep-ping the error
> message.
>
> Signed-off-by: Michal Privoznik
> ---
> build-aux/syntax-check.mk | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
> index 64c1e2773e..618c0546aa 100644
> --- a/build-aux/syntax-check.mk
> +++ b/build-aux/syntax-check.mk
> @@ -440,6 +440,11 @@ sc_prohibit_newline_at_end_of_diagnostic:
> && { echo 'newline at end of message(s)' 1>&2; \
> exit 1; } || :
>
> +sc_prohibit_error_message_on_multiple_lines:
> + @prohibit='[^N]_\(".*[^\\n]"$$' \
This doesn't work as you expect it to work, because the [] operator(?)
in regex defines a character set to match . Thus you are exempting any
message which ends with a backslash _OR_ with an 'n' rather than ending
with "\n".
Re: [PATCH 1/2] tools: Reformat --help output of virsh and virt-admin
On Mon, Sep 04, 2023 at 10:31:47 +0200, Michal Privoznik wrote:
> The --help output of virsh and virt-admin shows supported options
> and commands and as such contains new lines. Both these strings
> are marked for translation btw. But the way they are formatted
> now ('\n' being at the start of new line instead at the end of
> the previous) makes it hard to create a syntax-check rule for
> 'translation message on one line' (next commit).
>
> Reformat both strings a bit (no user visible change though).
It also makes sense because the formatting will make it look the way how
it is actually formatted later.
Reviewed-by: Peter Krempa
Re: [PATCH] docs, passt: Clarify some niche passt usage
On 9/2/23 00:41, Martin Kletzander wrote: > Change example logfile path and clarify how complicated all things passt > are. I chose not to create the non-existing directory because it could > open a whole new can of worms. > > Also explain missing `dev` attribute of `` > > Resolves: https://issues.redhat.com/browse/RHEL-1833 > > Signed-off-by: Martin Kletzander > --- > docs/formatdomain.rst | 21 +++-- > 1 file changed, 15 insertions(+), 6 deletions(-) Reviewed-by: Michal Privoznik Michal
[PATCH 2/2] syntax-check: Introduce a rule for one line error messages
Okay, this is a shortcut. Our coding style says that error
messages are exempt from '80 chars long lines' rule. But in the
very same paragraph it is said that all error messages need to be
marked for translation (as they might be presented to user).
Therefore, the syntax-check rule can check if _("...") is
formatted on one line. With exception of _("...\n" ...) (e.g.
various outputs from helper binaries like leaseshelper,
sshhelper, or daemons like lockd, logd). I believe nobody would
chose a substring that contains '\n' for git grep-ping the error
message.
Signed-off-by: Michal Privoznik
---
build-aux/syntax-check.mk | 8
1 file changed, 8 insertions(+)
diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index 64c1e2773e..618c0546aa 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -440,6 +440,11 @@ sc_prohibit_newline_at_end_of_diagnostic:
&& { echo 'newline at end of message(s)' 1>&2; \
exit 1; } || :
+sc_prohibit_error_message_on_multiple_lines:
+ @prohibit='[^N]_\(".*[^\\n]"$$' \
+ halt='found error message on multiple lines' \
+ $(_sc_search_regexp)
+
# Look for diagnostics that lack a % in the format string, except that we
# allow VIR_ERROR to do this, and ignore functions that take a single
# string rather than a format argument.
@@ -1386,6 +1391,9 @@ exclude_file_name_regexp--sc_prohibit_raw_virclassnew = \
exclude_file_name_regexp--sc_prohibit_newline_at_end_of_diagnostic = \
^src/rpc/gendispatch\.pl$$
+exclude_file_name_regexp--sc_prohibit_error_message_on_multiple_lines = \
+ ^(build-aux/syntax-check\.mk|docs/coding-style.rst)
+
exclude_file_name_regexp--sc_prohibit_nonreentrant = \
^((po|tests|examples)/|docs/.*(py|js|html\.in|.rst)|run.in$$|tools/wireshark/util/genxdrstub\.pl|tools/virt-login-shell\.c$$)
--
2.41.0
[PATCH 1/2] tools: Reformat --help output of virsh and virt-admin
The --help output of virsh and virt-admin shows supported options
and commands and as such contains new lines. Both these strings
are marked for translation btw. But the way they are formatted
now ('\n' being at the start of new line instead at the end of
the previous) makes it hard to create a syntax-check rule for
'translation message on one line' (next commit).
Reformat both strings a bit (no user visible change though).
Signed-off-by: Michal Privoznik
---
tools/virsh.c | 6 --
tools/virt-admin.c | 6 --
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index d9922a35fc..7b71131db3 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -436,8 +436,10 @@ virshUsage(void)
const vshCmdGrp *grp;
const vshCmdDef *cmd;
-fprintf(stdout, _("\n%1$s [options]... []"
- "\n%2$s [options]... [args...]\n\n"
+fprintf(stdout, _("\n"
+ "%1$s [options]... []\n"
+ "%2$s [options]... [args...]\n"
+ "\n"
" options:\n"
"-c | --connect=URI hypervisor connection URI\n"
"-d | --debug=NUMdebug level [0-4]\n"
diff --git a/tools/virt-admin.c b/tools/virt-admin.c
index 15a639f1ea..1e22a3c8a9 100644
--- a/tools/virt-admin.c
+++ b/tools/virt-admin.c
@@ -1242,8 +1242,10 @@ vshAdmUsage(void)
const vshCmdGrp *grp;
const vshCmdDef *cmd;
-fprintf(stdout, _("\n%1$s [options]... []"
- "\n%2$s [options]... [args...]\n\n"
+fprintf(stdout, _("\n"
+ "%1$s [options]... []\n"
+ "%2$s [options]... [args...]\n"
+ "\n"
" options:\n"
"-c | --connect=URI daemon admin connection
URI\n"
"-d | --debug=NUMdebug level [0-4]\n"
--
2.41.0
[PATCH 0/2] syntax-check: Introduce a rule for one line error messages
This is a follow up of: https://listman.redhat.com/archives/libvir-list/2023-August/241416.html And while the regular expression in patch 2/2 is very trivial it mostly works. But I'm open for suggestions. Michal Prívozník (2): tools: Reformat --help output of virsh and virt-admin syntax-check: Introduce a rule for one line error messages build-aux/syntax-check.mk | 8 tools/virsh.c | 6 -- tools/virt-admin.c| 6 -- 3 files changed, 16 insertions(+), 4 deletions(-) -- 2.41.0
Re: [PATCH] conf, schema: Switch iothread/poll values to unsignedLong
On Mon, Sep 04, 2023 at 09:22:54AM +0200, Peter Krempa wrote: On Fri, Sep 01, 2023 at 23:32:14 +0200, Martin Kletzander wrote: They represent nanoseconds, and we accept such values already. Not that anyone would use such values in the wild, but even one person testing QEMU could put in a bigger value and will be bothered with validation errors after every `virsh edit`. Also add a test for it. Resolves: https://issues.redhat.com/browse/RHEL-1717 Signed-off-by: Martin Kletzander --- src/conf/schemas/domaincommon.rng | 6 +++--- tests/genericxml2xmlindata/iothreadids.xml | 23 ++ tests/genericxml2xmltest.c | 2 ++ 3 files changed, 28 insertions(+), 3 deletions(-) create mode 100644 tests/genericxml2xmlindata/iothreadids.xml diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng index de3bd1c35c55..2f9ba31c0aec 100644 --- a/src/conf/schemas/domaincommon.rng +++ b/src/conf/schemas/domaincommon.rng @@ -876,17 +876,17 @@ - + I've looked originally at 'src/conf/schemas/basictypes.rng' which has: [0-9]+ And concluded that there's no actual difference between that and 'unsignedLong' not realizing that this is not the full definition of the type. Exactly. It took me quite some time to figure out where they come from, it can actually be used from various XML namespaces, but hunting this down, for someone like me, took longer than I would want to admit. Reviewed-by: Peter Krempa Thanks ;) signature.asc Description: PGP signature
Re: [libvirt PATCH 5/5] qemu: Implement support for vDPA block devices
On Fri, Sep 01, 2023 at 03:24:11PM -0500, Jonathon Jongsma wrote:
On 8/16/23 4:19 PM, Jonathon Jongsma wrote:
On 8/8/23 6:00 AM, Stefano Garzarella wrote:
On Mon, Aug 07, 2023 at 03:41:21PM +0200, Peter Krempa wrote:
On Thu, Aug 03, 2023 at 09:48:01 +0200, Stefano Garzarella wrote:
On Wed, Aug 2, 2023 at 10:33 PM Jonathon Jongsma
wrote:
On 7/24/23 8:05 AM, Peter Krempa wrote:
[...]
>
> I've also noticed that using 'qcow2' format for the device
doesn't work:
>
> error: internal error: process exited while connecting to
monitor: 2023-07-24T12:54:15.818631Z qemu-system-x86_64:
-blockdev {"node-name":"libvirt-1-format","read-only":false,"cache":{"direct":true,"no-flush":false},"driver":"qcow2","file":"libvirt-1-storage"}:
Could not read qcow2 header: Invalid argument
>
> If that is supposed to work, then qemu devs will probably
need to know
> about that, if that is not supposed to work, libvirt needs to add a
> check, because the error doesn't tell much. It's also possible I've
> messed up when formatting the image though, as didn't really try to
> figure out what's happening.
>
That's a good question, and I don't actually know the
answer. Were you
using an actual vdpa block device for your tests or were you
using the
vdpa block simulator kernel module? How did you set it up? Adding
Stefano to cc for his thoughts.
Yep, I would also like to understand how you initialized the device
with a qcow2 format.
Naively and originally I've simply used it as 'raw' at first and
formatted it from the guest OS. Then I've shut-down the VM and started
it back reconfiguring the image format as qcow2. This normally works
with real-file backed storage, and since the vdpa simulator seems to
persist the contents I supposed this would work.
Cool, I'll try that.
Can you try to reboot the VM, use it as `raw`, and read the qcow2 in the
vm from the guest OS?
Note: there could be some bugs in the simulator!
Theoretically, the best use case for vDPA block is that the backend
handles formats, for QEMU it should just be a virtio device, but being
a blockdev, we should be able to use formats anyway, so it should
work.
Yeah, ideally there will be no format driver in qemu used for these
devices (this is not yet the case, I'll need to fix libvirt to stop
using the 'raw' driver if not needed).
Here I'm more interested whether it is supposed to work, in which case
we want to allow using qcow2 as a format in libvirt, or it's not
supposed to work and we should forbid it before the user gets a
suboptimal error message such as now.
This is a good question. We certainly haven't tested it, because it's an
uncommon scenario, but as I said before, maybe it should work. I need to
check it better.
For now, waiting for real hardware, the only way to test vDPA block
support in QEMU is to use the simulator in the kernel or VDUSE.
With the kernel simulator we only have a 128 MB ramdisk available,
with VDUSE you can use QSD with any file:
$ modprobe -a vhost_vdpa vduse
$ qemu-storage-daemon \
--blockdev
file,filename=/path/to/image.qcow2,cache.direct=on,aio=native,node-name=file
\
--blockdev qcow2,file=file,node-name=qcow2 \
--export
vduse-blk,id=vduse0,name=vduse0,num-queues=1,node-name=qcow2,writable=on
$ vdpa dev add name vduse0 mgmtdev vduse
Then you have a /dev/vhost-vdpa-X device that you can use with the
`virtio-blk-vhost-vdpa` blockdev (note: vduse requires QEMU with a
memory-backed with `share=on`), but using raw since the qcow2 is
handled by QSD.
Of course, we should be able to use raw file with QSD and qcow2 on
qemu (although it's not the optimal configuration), but I don't know
how to initialize a `virtio-blk-vhost-vdpa` blockdev with a qcow2
image :-(
With the above qemu storage daemon you should be able to do that by
simply dropping the qcow2 format driver and simply exposing a qcow2
formatted image. It similarly works with NBD:
I've formatted 2 qcow2 images:
# qemu-img create -f qcow2 /root/image1.qcow2 100M
# qemu-img create -f qcow2 /root/image2.qcow2 100M
And then exported them both via vduse and nbd without interpreting
qcow2, thus making the QSD into just a dumb storage device:
# qemu-storage-daemon \
--blockdev file,filename=/root/image1.qcow2,cache.direct=on,aio=native,node-name=file1
\
--export vduse-blk,id=vduse0,name=vduse0,num-queues=1,node-name=file1,writable=on
\
--blockdev file,filename=/root/image2.qcow2,cache.direct=on,aio=native,node-name=file2
\
--nbd-server addr.type=unix,addr.path=/tmp/nbd.sock \
--export nbd,id=nbd0,node-name=file2,writable=on,name=exportname
Cool! Thanks for sharing!
Now when I start a VM using the NBD export in qcow2 format:
function='0x0'/>
The VM starts fine, but when using:
function='0x0'/>
I get:
error: internal error: QEMU unexpectedly closed the monitor
(vm='vdpa'): 2023-08-07T12:34:21.628520Z qemu-system-x86_64:
-blockdev {"n
Re: [PATCH] conf, schema: Switch iothread/poll values to unsignedLong
On Fri, Sep 01, 2023 at 23:32:14 +0200, Martin Kletzander wrote: > They represent nanoseconds, and we accept such values already. Not that > anyone would use such values in the wild, but even one person testing > QEMU could put in a bigger value and will be bothered with validation > errors after every `virsh edit`. Also add a test for it. > > Resolves: https://issues.redhat.com/browse/RHEL-1717 > > Signed-off-by: Martin Kletzander > --- > src/conf/schemas/domaincommon.rng | 6 +++--- > tests/genericxml2xmlindata/iothreadids.xml | 23 ++ > tests/genericxml2xmltest.c | 2 ++ > 3 files changed, 28 insertions(+), 3 deletions(-) > create mode 100644 tests/genericxml2xmlindata/iothreadids.xml > > diff --git a/src/conf/schemas/domaincommon.rng > b/src/conf/schemas/domaincommon.rng > index de3bd1c35c55..2f9ba31c0aec 100644 > --- a/src/conf/schemas/domaincommon.rng > +++ b/src/conf/schemas/domaincommon.rng > @@ -876,17 +876,17 @@ > > > > - > + > I've looked originally at 'src/conf/schemas/basictypes.rng' which has: [0-9]+ And concluded that there's no actual difference between that and 'unsignedLong' not realizing that this is not the full definition of the type. > > > Reviewed-by: Peter Krempa
