Re: [libvirt PATCHv1 8/8] docs: virtiofs: add section about ID remapping
On Wed, Sep 13, 2023 at 04:14:55PM +0100, Daniel P. Berrangé wrote: > On Wed, Sep 13, 2023 at 05:07:27PM +0200, Ján Tomko wrote: > > On a Tuesday in 2023, Daniel P. Berrangé wrote: > > > On Tue, Sep 12, 2023 at 04:05:04PM +0200, Ján Tomko wrote: > > > > On a Monday in 2023, Daniel P. Berrangé wrote: > > > > > I would expect libvirt to "do the right thing" and automatically load > > > > > the /etc/subuid data for the current user and NOT require any extra > > > > > XML mapping to be set for unprivileged usage. > > > > > > > > > > > > > So, by default libvirt would assume that unprivileged > > > > accessmode='passthrough' means "use the whole range for my user > > > > from /etc/subuid"? > > > > > > > > Podman treats /etc/subuid as a pool and chooses a 64K range that is > > > > (to its knowledge) unused. I'm undecided whether that would also be > > > > a reasonable option for a default. > > > > > > I thought podman simply used the entry that is in /etc/subuid > > > as is: > > > > D'oh. Right. By default it uses --userns=host, which behaves as you > > describe. > > > > What I described is --userns=auto behavior, suggested in the bug > > discussion: > > https://bugzilla.redhat.com/show_bug.cgi?id=2034630#c8 > > What I'm also missing is understanding what component enforces that > you have grabbed a range that is actually present for your user > in /etc/subuid, as opposed to grabbing a range belonging to a > different user. > > Something must enforce that otherwise it is a total free for all > and /etc/subuid is largely pointless. Ah, virtiofsd is invoking newuidmap, which is a program with the 'setuid' capability flag set on its binary. This lets us do the privileged /proc/uidmap setup on behalf of virtiofsd and validates the /etc/subuid ranges. I think libvirt could, by default, read /etc/subuid and pick a 64k range from it, if it had more than 64k available. In future we could track the ranges to keep them unique per instance, but for now even the simple picking is better than requiring a manua user config every time. 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: [libvirt PATCHv1 8/8] docs: virtiofs: add section about ID remapping
On Wed, Sep 13, 2023 at 05:07:27PM +0200, Ján Tomko wrote: > On a Tuesday in 2023, Daniel P. Berrangé wrote: > > On Tue, Sep 12, 2023 at 04:05:04PM +0200, Ján Tomko wrote: > > > On a Monday in 2023, Daniel P. Berrangé wrote: > > > > I would expect libvirt to "do the right thing" and automatically load > > > > the /etc/subuid data for the current user and NOT require any extra > > > > XML mapping to be set for unprivileged usage. > > > > > > > > > > So, by default libvirt would assume that unprivileged > > > accessmode='passthrough' means "use the whole range for my user > > > from /etc/subuid"? > > > > > > Podman treats /etc/subuid as a pool and chooses a 64K range that is > > > (to its knowledge) unused. I'm undecided whether that would also be > > > a reasonable option for a default. > > > > I thought podman simply used the entry that is in /etc/subuid > > as is: > > D'oh. Right. By default it uses --userns=host, which behaves as you > describe. > > What I described is --userns=auto behavior, suggested in the bug > discussion: > https://bugzilla.redhat.com/show_bug.cgi?id=2034630#c8 What I'm also missing is understanding what component enforces that you have grabbed a range that is actually present for your user in /etc/subuid, as opposed to grabbing a range belonging to a different user. Something must enforce that otherwise it is a total free for all and /etc/subuid is largely pointless. 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: [libvirt PATCHv1 8/8] docs: virtiofs: add section about ID remapping
On a Tuesday in 2023, Daniel P. Berrangé wrote: On Tue, Sep 12, 2023 at 04:05:04PM +0200, Ján Tomko wrote: On a Monday in 2023, Daniel P. Berrangé wrote: > I would expect libvirt to "do the right thing" and automatically load > the /etc/subuid data for the current user and NOT require any extra > XML mapping to be set for unprivileged usage. > So, by default libvirt would assume that unprivileged accessmode='passthrough' means "use the whole range for my user from /etc/subuid"? Podman treats /etc/subuid as a pool and chooses a 64K range that is (to its knowledge) unused. I'm undecided whether that would also be a reasonable option for a default. I thought podman simply used the entry that is in /etc/subuid as is: D'oh. Right. By default it uses --userns=host, which behaves as you describe. What I described is --userns=auto behavior, suggested in the bug discussion: https://bugzilla.redhat.com/show_bug.cgi?id=2034630#c8 Jano $ grep $LOGNAME /etc/subuid berrange:165536:65536 $ podman run -it centos:stream9 cat /proc/self/uid_map 0 1001 1 1 165536 65536 Maps "root" to my original unpriv login UID, and maps everything else to the 64k IDs reserved in /etc/subuid 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 :| signature.asc Description: PGP signature
Re: [libvirt PATCH] ci: jobs.sh: Add back '--no-suite syntax-check --print-errorlogs'
On Wed, Sep 13, 2023 at 01:55:54PM +0200, Erik Skultety wrote:
> Commit f688a53a converted .gitlab-ci.yml to the usage of ci/jobs.sh
> functions, but in doing that our test options
> '--no-suite syntax-check --print-errorlogs'
> got lost in the process and since commit 8e660c52 didn't introduce them
> in the first place, it caused a behavioral regression. This patch adds
> them back.
>
> Fixes: 8e660c5286d7e2d07dd61681074bf155592d
>
> Signed-off-by: Erik Skultety
> ---
>
> Technically this is a build breaker fix, but sending for review anyway to see
> if there's an agreement on this approach.
These defaults for run_test() are consistent with the override
done in run_codestyle(), so:
Reviewed-by: Daniel P. Berrangé
>
> ci/jobs.sh | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/ci/jobs.sh b/ci/jobs.sh
> index abd695e231..eb4a4e29cf 100644
> --- a/ci/jobs.sh
> +++ b/ci/jobs.sh
> @@ -39,7 +39,10 @@ run_dist() {
> }
>
> run_test() {
> +TEST_ARGS="${TEST_ARGS:=--no-suite syntax-check --print-errorlogs}"
> +
> test -f $GIT_ROOT/build/build.ninja || run_meson_setup
> +
> run_cmd meson test -C build $TEST_ARGS
> }
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: [libvirt PATCH v2 0/5] Add support for vDPA block devices
On Wed, Sep 13, 2023 at 12:57:09PM +0200, Erik Skultety wrote: > On Wed, Sep 13, 2023 at 08:56:35AM +0100, Daniel P. Berrangé wrote: > > On Tue, Sep 12, 2023 at 04:11:01PM -0500, Jonathon Jongsma wrote: > > > On 9/12/23 7:00 AM, Peter Krempa wrote: > > > > On Mon, Sep 11, 2023 at 16:53:42 -0500, Jonathon Jongsma wrote: > > > > > see https://bugzilla.redhat.com/show_bug.cgi?id=1900770. > > > > > > > > > > Changes in v2: > > > > > - Don't use virStorageSource->path for vdpa device path to avoid > > > > > clashing with > > > > > existing path functionality > > > > > - Move vdpa device opening to the > > > > > qemuProcessPrepareHostStorageSource() > > > > > function rather than the qemuDomainPrepareStorageSource() > > > > > function. This > > > > > also required some additional support in the tests for setting up > > > > > the > > > > > objects properly for testing. > > > > > - rebased to latest master branch > > > > > > > > Reviewed-by: Peter Krempa > > > > > > > > > > I pushed this series, but for some reason only the ubuntu images are > > > failing > > > CI with no useful output: > > > https://gitlab.com/libvirt/libvirt/-/pipelines/1001459836 > > > > This is a behavioural regression from the recent CI refactoring of Eriks'. > > > > We have lost the "--no-suite syntax-check --print-errorlogs" arguments > > when running tests. > > Sigh. I'm already running a pipeline with a fix that adds those test options > back. > > Erik How about this one? https://listman.redhat.com/archives/libvir-list/2023-September/242071.html Pipeline: https://gitlab.com/eskultety/libvirt/-/pipelines/1002436800 Erik
[libvirt PATCH] ci: jobs.sh: Add back '--no-suite syntax-check --print-errorlogs'
Commit f688a53a converted .gitlab-ci.yml to the usage of ci/jobs.sh
functions, but in doing that our test options
'--no-suite syntax-check --print-errorlogs'
got lost in the process and since commit 8e660c52 didn't introduce them
in the first place, it caused a behavioral regression. This patch adds
them back.
Fixes: 8e660c5286d7e2d07dd61681074bf155592d
Signed-off-by: Erik Skultety
---
Technically this is a build breaker fix, but sending for review anyway to see
if there's an agreement on this approach.
ci/jobs.sh | 3 +++
1 file changed, 3 insertions(+)
diff --git a/ci/jobs.sh b/ci/jobs.sh
index abd695e231..eb4a4e29cf 100644
--- a/ci/jobs.sh
+++ b/ci/jobs.sh
@@ -39,7 +39,10 @@ run_dist() {
}
run_test() {
+TEST_ARGS="${TEST_ARGS:=--no-suite syntax-check --print-errorlogs}"
+
test -f $GIT_ROOT/build/build.ninja || run_meson_setup
+
run_cmd meson test -C build $TEST_ARGS
}
--
2.41.0
Re: [libvirt PATCH v2 0/5] Add support for vDPA block devices
On Wed, Sep 13, 2023 at 08:56:35AM +0100, Daniel P. Berrangé wrote: > On Tue, Sep 12, 2023 at 04:11:01PM -0500, Jonathon Jongsma wrote: > > On 9/12/23 7:00 AM, Peter Krempa wrote: > > > On Mon, Sep 11, 2023 at 16:53:42 -0500, Jonathon Jongsma wrote: > > > > see https://bugzilla.redhat.com/show_bug.cgi?id=1900770. > > > > > > > > Changes in v2: > > > > - Don't use virStorageSource->path for vdpa device path to avoid > > > > clashing with > > > > existing path functionality > > > > - Move vdpa device opening to the > > > > qemuProcessPrepareHostStorageSource() > > > > function rather than the qemuDomainPrepareStorageSource() function. > > > > This > > > > also required some additional support in the tests for setting up > > > > the > > > > objects properly for testing. > > > > - rebased to latest master branch > > > > > > Reviewed-by: Peter Krempa > > > > > > > I pushed this series, but for some reason only the ubuntu images are failing > > CI with no useful output: > > https://gitlab.com/libvirt/libvirt/-/pipelines/1001459836 > > This is a behavioural regression from the recent CI refactoring of Eriks'. > > We have lost the "--no-suite syntax-check --print-errorlogs" arguments > when running tests. Sigh. I'm already running a pipeline with a fix that adds those test options back. Erik
Re: [libvirt PATCH v2 18/35] .gitlab-ci.yml: Convert the native build job to the build.sh usage
On Mon, Sep 11, 2023 at 03:43:19PM +0200, Erik Skultety wrote: > Individual shell command executions are replaced by respective > functions in the ci/build.sh base script. This will make sure we use > the same recipes in GitLab jobs as well as in local executions. > > Signed-off-by: Erik Skultety > Reviewed-by: Daniel P. Berrangé > Erik Skultety : > --- > .gitlab-ci.yml | 10 -- > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml > index 1c6af8f8b3..c837812091 100644 > --- a/.gitlab-ci.yml > +++ b/.gitlab-ci.yml > @@ -25,15 +25,13 @@ include: >- ccache/ > key: "$CI_JOB_NAME" >script: > -- *script_variables > -- meson setup build --werror $MESON_ARGS || (cat > build/meson-logs/meson-log.txt && exit 1) > -- meson dist -C build --no-tests > +- source ci/jobs.sh > - if test -x /usr/bin/rpmbuild && test "$RPM" != "skip"; >then > -rpmbuild --clean --nodeps --define "_without_mingw 1" -ta > build/meson-dist/libvirt-*.tar.xz; > +run_rpmbuild; >else > -meson compile -C build; > -meson test -C build --no-suite syntax-check --print-errorlogs; > +run_build; > +run_test; I missed a regression here - we're loosing the --no-suite and --print-errorlogs args when running tests, so we can no longer diagnose the failures. >fi >after_script: > - test "$CI_JOB_STATUS" != "success" && exit 1; > -- > 2.41.0 > 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: [libvirt PATCH v2 0/5] Add support for vDPA block devices
On Tue, Sep 12, 2023 at 04:11:01PM -0500, Jonathon Jongsma wrote: > On 9/12/23 7:00 AM, Peter Krempa wrote: > > On Mon, Sep 11, 2023 at 16:53:42 -0500, Jonathon Jongsma wrote: > > > see https://bugzilla.redhat.com/show_bug.cgi?id=1900770. > > > > > > Changes in v2: > > > - Don't use virStorageSource->path for vdpa device path to avoid > > > clashing with > > > existing path functionality > > > - Move vdpa device opening to the qemuProcessPrepareHostStorageSource() > > > function rather than the qemuDomainPrepareStorageSource() function. > > > This > > > also required some additional support in the tests for setting up the > > > objects properly for testing. > > > - rebased to latest master branch > > > > Reviewed-by: Peter Krempa > > > > I pushed this series, but for some reason only the ubuntu images are failing > CI with no useful output: > https://gitlab.com/libvirt/libvirt/-/pipelines/1001459836 This is a behavioural regression from the recent CI refactoring of Eriks'. We have lost the "--no-suite syntax-check --print-errorlogs" arguments when running tests. 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 (pushed)] virStorageSourceClear: Clear 'vdpadev' field
Release memory for the recently added field. Fixes: 1df106cc20a4cc6417cfbaf01860f465ec3dd915 Signed-off-by: Peter Krempa --- src/conf/storage_source_conf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/conf/storage_source_conf.c b/src/conf/storage_source_conf.c index f57cb3241d..f7f62c3966 100644 --- a/src/conf/storage_source_conf.c +++ b/src/conf/storage_source_conf.c @@ -1143,6 +1143,7 @@ virStorageSourceClear(virStorageSource *def) VIR_FREE(def->path); VIR_FREE(def->fdgroup); VIR_FREE(def->volume); +VIR_FREE(def->vdpadev); VIR_FREE(def->snapshot); VIR_FREE(def->configFile); VIR_FREE(def->query); -- 2.41.0
Re: [libvirt PATCH v2 0/5] Add support for vDPA block devices
On Tue, Sep 12, 2023 at 16:11:01 -0500, Jonathon Jongsma wrote: > On 9/12/23 7:00 AM, Peter Krempa wrote: > > On Mon, Sep 11, 2023 at 16:53:42 -0500, Jonathon Jongsma wrote: > > > see https://bugzilla.redhat.com/show_bug.cgi?id=1900770. > > > > > > Changes in v2: > > > - Don't use virStorageSource->path for vdpa device path to avoid > > > clashing with > > > existing path functionality > > > - Move vdpa device opening to the qemuProcessPrepareHostStorageSource() > > > function rather than the qemuDomainPrepareStorageSource() function. > > > This > > > also required some additional support in the tests for setting up the > > > objects properly for testing. > > > - rebased to latest master branch > > > > Reviewed-by: Peter Krempa > > > > I pushed this series, but for some reason only the ubuntu images are failing > CI with no useful output: > https://gitlab.com/libvirt/libvirt/-/pipelines/1001459836 > > I suspect it may be related to address sanitizer stuff, does anybody have > tips on getting more information about this failure? Usually valgrind does a good job of finding everyhting the sanitizer complains about: $ valgrind --trace-children=yes --leak-check=full ./tests/qemuxml2argvtest [...] ==352693== 18 bytes in 1 blocks are definitely lost in loss record 257 of 684 ==352693==at 0x484182F: malloc (vg_replace_malloc.c:431) ==352693==by 0x51DA07F: xmlStrndup (in /usr/lib64/libxml2.so.2.10.4) ==352693==by 0x49D918E: virXMLPropStringRequired (virxml.c:321) ==352693==by 0x4A0D866: virDomainStorageSourceParse (domain_conf.c:7526) ==352693==by 0x4A0E9F0: virDomainDiskDefParseSourceXML (domain_conf.c:7926) ==352693==by 0x4A0ECCF: virDomainDiskDefParseXML (domain_conf.c:8000) ==352693==by 0x4A2DA93: virDomainDefParseXML (domain_conf.c:18767) ==352693==by 0x4A31678: virDomainDefParseNode (domain_conf.c:19557) ==352693==by 0x134000: testCompareXMLToArgv (qemuxml2argvtest.c:566) ==352693==by 0x1356B9: virTestRun (testutils.c:143) ==352693==by 0x135940: virTestRunLog (testutils.c:198) ==352693==by 0x11D422: mymain (qemuxml2argvtest.c:1164) Looks like the new field is not freed. I'll post the patch soon
