Re: [libvirt PATCH 0/3] tests: Fix qemucapabilitiestest on macOS
On 2022-08-10 at 16:29 +02, Andrea Bolognani wrote... > We need to mock the function that probes for HVF support. > > Andrea Bolognani (3): > tests: Use domaincapsmock in qemucapabilitiestest > qemu: Make virQEMUCapsProbeHVF() non-static > tests: Mock virQEMUCapsProbeHVF() > > src/qemu/qemu_capabilities.c | 4 ++-- > src/qemu/qemu_capabilities.h | 2 ++ > tests/domaincapsmock.c | 6 ++ > tests/qemucapabilitiestest.c | 3 ++- > 4 files changed, 12 insertions(+), 3 deletions(-) This works. We now have a clean test suite on macOS 12: Ok: 252 Expected Fail: 0 Fail: 0 Unexpected Pass:0 Skipped:19 Timeout: 0 For the series: Reviewed-by: Christophe de Dinechin Tested-by: Christophe de Dinechin -- Cheers, Christophe de Dinechin (https://c3d.github.io)
Re: Test failures on macOS 12
> On 9 Aug 2022, at 11:50, Andrea Bolognani wrote: > > On Tue, Aug 09, 2022 at 11:34:20AM +0200, Christophe de Dinechin wrote: >> On 9 Aug 2022, at 11:28, Andrea Bolognani wrote: >>> Yeah, this seems to help and the change makes sense to me. >>> >>> I wonder why we didn't run into this much earlier though? As I >>> mentioned, the test runs successfully as-is on macOS 11. Plus, many >>> other tests rely on library injection and yet work okay even without >>> this change. I must admit that this puzzled me a bit too. I spent a bit of time checking for dyld warnings or anything else. One explanation could be if in other cases, the symbols are marked as weak? I did not check that. And I don’t have a macOS 11 machine to compare anymore. >>> >>> Anyway, I'm happy to add my >>> >>> Reviewed-by: Andrea Bolognani >>> >>> to this patch and push it. The authorship information looks a bit >>> funky though, with the two S-o-bs... >> >> I did not know which one you’d prefer (in case there is a policy). >> If I get to choose, assign that to Red Hat (and change the author >> accordingly). >> >> (and I’ll change my libvirt gitconfig accordingly in the future) > > Done. I'll push once CI has passed. > > It would be great if you could use git-publish for future code > submissions: that way patches can be applied locally more > conveniently by the reviewer. I was able to make it work regardless, > it just took a bit more effort :) Ack. https://gitlab.com/c3d/libvirt/-/pipelines/608168172 > > -- > Andrea Bolognani / Red Hat / Virtualization >
Re: Test failures on macOS 12
> On 9 Aug 2022, at 11:28, Andrea Bolognani wrote: > > On Mon, Aug 08, 2022 at 08:19:28PM +0200, Christophe de Dinechin wrote: >> Hi Andrea, >> >> I saw your call to Sergio and Marc-André on IRC, and I thought I would >> spend a few minutes inviestigating. > > Thanks, I appreciate that! > >>>> I'm trying to enable CI coverage for macOS 12, but I'm running into a >>>> couple of issues that I'm not sure how to handle. >>>> >>>> Note that the test suite currently passes on macOS 11[1], so these >>>> failures have to be a consequence to changes made to macOS that we >>>> haven't yet learned how to cope with. >>>> >>>> The first one is in vircryptotest: >>>> >>>> Encrypt aes265cbc ... Expected ciphertext doesn't match >>>> >>>> I've added some debug statements and it looks like the generated data >>>> is different every time, which seems like a pretty good indication >>>> that virrandommock is not being picked up correctly. This is not the >>>> only test program that uses that specific mock though, so I'm not >>>> sure what makes it fail when all the others are succeeding. >> >> I believe that the following patch fixes this one: >> >> From: Christophe de Dinechin >> Date: Mon, 8 Aug 2022 20:14:08 +0200 >> Subject: [PATCH] tests: Pass the flat_namespace option to the linker >> >> This fixes vircryptotest on macOS 12 (Monterey). >> >> The test relies on library injection (using DYLD_INSERT_LIBRARIES) >> to replace the normal random functions with functions giving predictable >> results, defined in virrandommock.c. However, using DYLD_INSERT_LIBRARIES >> only works when building with flat namespaces. >> >> Adding the -Wl,-flat_namespace option to the linker fixes the problem. >> The option was already defined in the top-level meson.build, but had been >> forgotten in the test linker arguments. >> >> Signed-off-by: Christophe de Dinechin >> Signed-off-by: Christophe de Dinechin >> --- >> tests/meson.build | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/tests/meson.build b/tests/meson.build >> index bc9d8ccc4c..d6b1bb2bf0 100644 >> --- a/tests/meson.build >> +++ b/tests/meson.build >> @@ -28,6 +28,7 @@ tests_dep = declare_dependency( >> ], >> link_args: ( >> libvirt_export_dynamic >> ++ libvirt_flat_namespace >> + coverage_flags >> ), >> ) >> -- >> 2.37.1 >> >> >> Could you please check? > > Yeah, this seems to help and the change makes sense to me. > > I wonder why we didn't run into this much earlier though? As I > mentioned, the test runs successfully as-is on macOS 11. Plus, many > other tests rely on library injection and yet work okay even without > this change. > > Anyway, I'm happy to add my > > Reviewed-by: Andrea Bolognani > > to this patch and push it. The authorship information looks a bit > funky though, with the two S-o-bs... I did not know which one you’d prefer (in case there is a policy). If I get to choose, assign that to Red Hat (and change the author accordingly). (and I’ll change my libvirt gitconfig accordingly in the future) Thanks, Christophe > Should I drop the @redhat.com > one and only keep the one with your personal email address, matching > the commit authorship information? > > -- > Andrea Bolognani / Red Hat / Virtualization >
Re: Test failures on macOS 12
> On 10 Jun 2022, at 11:34, Andrea Bolognani wrote:
>
> On Fri, May 06, 2022 at 03:00:14AM -0700, Andrea Bolognani wrote:
>> I'm trying to enable CI coverage for macOS 12, but I'm running into a
>> couple of issues that I'm not sure how to handle.
>>
>> Note that the test suite currently passes on macOS 11[1], so these
>> failures have to be a consequence to changes made to macOS that we
>> haven't yet learned how to cope with.
>>
>> The first one is in vircryptotest:
>>
>> Encrypt aes265cbc ... Expected ciphertext doesn't match
>>
>> I've added some debug statements and it looks like the generated data
>> is different every time, which seems like a pretty good indication
>> that virrandommock is not being picked up correctly. This is not the
>> only test program that uses that specific mock though, so I'm not
>> sure what makes it fail when all the others are succeeding.
>>
>> The other issue is in qemuxml2argvtest:
>>
>> error : virCommandWait:2752 : internal error: Child process
>>(/usr/libexec/qemu/vhost-user/test-vhost-user-gpu --print-capabilities)
>>unexpected fatal signal 6: dyld[8896]: symbol not found in flat
>>namespace '_virQEMUCapsGet'
>> error : qemuVhostUserFillDomainGPU:394 : operation failed: Unable to
>>find a satisfying vhost-user-gpu
I don’t see this one.
What I see instead is failures on qemucapabilitiestest.
The tests seems to depend on a particular install path for the
qemu-system- binaries:
binary = g_strdup_printf("/usr/bin/qemu-system-%s",
data->archName);
On macOS, there is something called “system integrity protection”
which makes it a bit more difficult to add stuff to /usr/bin.
However, I don’t really think that’s the problem. I ran some tests with:
VIR_TEST_DEBUG=1
VIR_TEST_RANGE=1,3,5,7,9,11,13,15,17,19,21,23,25,27,29,31,33,35,37,39,41,43,45,47,49,51,53,55,57,59,61,63,65,67,69
/Users/ddd/Work/libvirt/build/tests/qemucapabilitiestest
The error I get seem to point to ‘hvf’ been unexected in the output:
15) 6.2.0 (x86_64)...
In '/Users/ddd/Work/libvirt/tests/qemucapabilitiesdata/caps_6.2.0.x86_64.xml':
Offset 7557
Expect [c]
Actual [hvf'/>
>
>> So the various virFileWrapperAddPrefix() calls that cause the
>> contents of tests/qemuvhostuserdata/ to override the host's own
>> vhostuser configuration are still effective, but for some reason the
>> trivial test-vhost-user-gpu shell script can't be run successfully
>> because an internal libvirt symbol can't be found somehow? Confusing.
>>
>> Roman, does any of this ring a bell? Any chance you could
>> investigate? macOS 12 has been out for a while now so I'd be very
>> keen to have it added to CI.
>
> Roman, any chance you can find some time to investigate the test
> failures documented above? I'm afraid I've reached the limit of my
> ability to debug macOS-specific behavior :(
>
> --
> Andrea Bolognani / Red Hat / Virtualization
>
Re: Test failures on macOS 12
Hi Andrea, I saw your call to Sergio and Marc-André on IRC, and I thought I would spend a few minutes inviestigating. > On 10 Jun 2022, at 11:34, Andrea Bolognani wrote: > > On Fri, May 06, 2022 at 03:00:14AM -0700, Andrea Bolognani wrote: >> I'm trying to enable CI coverage for macOS 12, but I'm running into a >> couple of issues that I'm not sure how to handle. >> >> Note that the test suite currently passes on macOS 11[1], so these >> failures have to be a consequence to changes made to macOS that we >> haven't yet learned how to cope with. >> >> The first one is in vircryptotest: >> >> Encrypt aes265cbc ... Expected ciphertext doesn't match >> >> I've added some debug statements and it looks like the generated data >> is different every time, which seems like a pretty good indication >> that virrandommock is not being picked up correctly. This is not the >> only test program that uses that specific mock though, so I'm not >> sure what makes it fail when all the others are succeeding. I believe that the following patch fixes this one: From: Christophe de Dinechin Date: Mon, 8 Aug 2022 20:14:08 +0200 Subject: [PATCH] tests: Pass the flat_namespace option to the linker This fixes vircryptotest on macOS 12 (Monterey). The test relies on library injection (using DYLD_INSERT_LIBRARIES) to replace the normal random functions with functions giving predictable results, defined in virrandommock.c. However, using DYLD_INSERT_LIBRARIES only works when building with flat namespaces. Adding the -Wl,-flat_namespace option to the linker fixes the problem. The option was already defined in the top-level meson.build, but had been forgotten in the test linker arguments. Signed-off-by: Christophe de Dinechin Signed-off-by: Christophe de Dinechin --- tests/meson.build | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/meson.build b/tests/meson.build index bc9d8ccc4c..d6b1bb2bf0 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -28,6 +28,7 @@ tests_dep = declare_dependency( ], link_args: ( libvirt_export_dynamic ++ libvirt_flat_namespace + coverage_flags ), ) -- 2.37.1 Could you please check? >> >> The other issue is in qemuxml2argvtest: >> >> error : virCommandWait:2752 : internal error: Child process >>(/usr/libexec/qemu/vhost-user/test-vhost-user-gpu --print-capabilities) >>unexpected fatal signal 6: dyld[8896]: symbol not found in flat >>namespace '_virQEMUCapsGet' >> error : qemuVhostUserFillDomainGPU:394 : operation failed: Unable to >>find a satisfying vhost-user-gpu >> >> So the various virFileWrapperAddPrefix() calls that cause the >> contents of tests/qemuvhostuserdata/ to override the host's own >> vhostuser configuration are still effective, but for some reason the >> trivial test-vhost-user-gpu shell script can't be run successfully >> because an internal libvirt symbol can't be found somehow? Confusing. >> >> Roman, does any of this ring a bell? Any chance you could >> investigate? macOS 12 has been out for a while now so I'd be very >> keen to have it added to CI. > > Roman, any chance you can find some time to investigate the test > failures documented above? I'm afraid I've reached the limit of my > ability to debug macOS-specific behavior :( > > -- > Andrea Bolognani / Red Hat / Virtualization >
Kata needs for device addressing (was Re: libvirt-devaddr: a new library for device address assignment)
> On 4 Mar 2020, at 13:50, Daniel P. Berrangé wrote: > > We've been doing alot of refactoring of code in recent times, and also > have plans for significant infrastructure changes. We still need to > spend time delivering interesting features to users / applications. > This mail is to introduce an idea for a solution to an specific > area applications have had long term pain with libvirt's current > "mechanism, not policy" approach - device addressing. This is a way > for us to show brand new ideas & approaches for what the libvirt > project can deliver in terms of management APIs. > > To set expectations straight: I have written no code for this yet, > merely identified the gap & conceptual solution. > > > The device addressing problem > = > > One of the key jobs libvirt does when processing a new domain XML > configuration is to assign addresses to all devices that are present. > This involves adding various device controllers (PCI bridges, PCI root > ports, IDE/SCSI buses, USB controllers, etc) if they are not already > present, and then assigning PCI, USB, IDE, SCSI, etc, addresses to each > device so they are associated with controllers. When libvirt spawns a > QEMU guest, it will pass full address information to QEMU. > > Libvirt, as a general rule, aims to avoid defining and implementing > policy around expansion of guest configuration / defaults, however, it > is inescapable in the case of device addressing due to the need to > guarantee a stable hardware ABI to make live migration and save/restore > to disk work. The policy that libvirt has implemented for device > addressing is, as much as possible, the same as the addressing scheme > QEMU would apply itself. > > While libvirt succeeds in its goal of providing a stable hardware API, > the addressing scheme used is not well suited to all deployment > scenarios of QEMU. This is an inevitable result of having a specific > assignment policy implemented in libvirt which has to trade off mutually > incompatible use cases/goals. > > When the libvirt addressing policy is not been sufficient, management > applications are forced to take on address assignment themselves, > which is a massive non-trivial job with many subtle problems to > consider. > > Places where libvirt's addressing is insufficient for PCI include > > * Setting up multiple guest NUMA nodes and associating devices to > specific nodes > * Pre-emptive creation of extra PCIe root ports, to allow for later > device hotplug on PCIe topologies > * Determining whether to place a device on a PCI or PCIe bridge > * Controlling whether a device is placed into a hotpluggable slot > * Controlling whether a PCIe root port supports hotplug or not > * Determining whether to places all devices on distinct slots or > buses, vs grouping them all into functions on the same slot > * Ability to expand the device addressing without being on the > hypervisor host > > Libvirt wishes to avoid implementing many different address assignment > policies. It also wishes to keep the domain XML as a representation > of the virtual hardware, not add a bunch of properties to it which > merely serve as tunable input parameters for device addressing > algorithms. > > There is thus a dilemma here. Management applications increasingly > need fine grained control over device addressing, while libvirt > doesn't want to expose fine grained policy controls via the XML. > > > The new libvirt-devaddr API > === > > The way out of this is to define a brand new virt management API > which tackles this specific problem in a way that addresses all the > problems mgmt apps have with device addressing and explicitly > provides a variety of policy impls with tunable behaviour. > > By "new API", I actually mean an entirely new library, completely > distinct from libvirt.so, or anything else we've delivered so > far. The closest we've come to delivering something at this kind > of conceptual level, would be the abortive attempt we made with > "libvirt-builder" to deliver a policy-driven API instead of mechanism > based. This proposal is still quite different from that attempt. > > At a high level > > * The new API is "libvirt-devaddr" - short for "libvirt device addressing" > > * As input it will take > > 1. The guest CPU architecture and machine type > 2. A list of global tunables specifying desired behaviour of the > address assignment policy > 3. A minimal list of devices needed in the virtual machine, with > optional addresses and optional per-device tunables to override > the global tunables > > * As output it will emit > > 1. fully expanded list of devices needed in the virtual machine, > with addressing information sufficient to ensure stable hardware ABI > > Initially the API would implement something that behaves the same > way as libvirt's current address assignment API. > > The intended usage would be > > * Mgmt
Re: On the need to move to a merge request workflow
+1 on the initial thread b > On 6 Mar 2020, at 15:54, Daniel Henrique Barboza > wrote: > > > > On 3/6/20 8:44 AM, Daniel P. Berrangé wrote: > > > [...] > > > What happens with this mailing list when the migration to the new workflow is > completed with all the repos? Is it still going to be used for discussions, > questions, RFCs and etcetera? I'd rather be in Gitlab watching opened issues > and merge requests all the time, without the need to check the Libvirt ML > ever again. > > And apparently we're leaning towards Gitlab. I'll not be standing here > defending closed-source, Microsoft based Github, but I'm curious: aside from > that (and that reason alone is enough, no need to grab the pitchforks), > is there any other technical advantage for going Gitlab? I suppose most > existing "coding support tools" are Github friendly already. Also, due to > Microsoft deep pockets, Github will probably experience less downtime and > have a > better support overall in case something goes wrong. GitHub and GitLab have different approaches to CI, with pros and cons on both sides. Obviously, it is easier to get stuff tested on Windows with GitHub, for example. You can use both, with automatic mirroring of the commits. For some of my own projects, I have dual push targets in git (triple, actually, SourceForge), and then I get two sets of (different) CI tests on a push. For example, GitLab will test a number of Linux targets like Ubuntu, etc, while GitHub will test macOS and someday Windows. I am not aware of a good way to sync issues, though, only commits. Anybody knows differently? > > > Thanks, > > > DHB > >
Re: qemu:///embed and isolation from global components
Le 9 mars 2020 à 14:03, Michal Privoznik a écrit : > > On 3/6/20 6:49 PM, Daniel P. Berrangé wrote: >>> On Fri, Mar 06, 2020 at 06:24:15PM +0100, Andrea Bolognani wrote: >>> Recently I've been working on integrating qemu:///embed into an >>> application. It's been reasonably smooth so far :) >>> >>> There's one thing, however, that has caused a bit of confusion, and >>> I would like to clarify whether my expectations are incorrect, there >>> are genuine bugs in the implementation that need to be addressed, or >>> maybe the experimental nature of embedded driver support results in >>> not all scenarios having yet been considered and catered for. >>> >>> Using virt-qemu-run as an example, when I run it as root I see that >>> >>> * the domain doesn't show up in the output of 'virsh list' for >>> qemu:///system; >> This is good. >>> * it does, however, show up in the output of 'machinectl', with >>> class=vm and service=libvirt-qemu; >> This is bad. It is one of the gaps we need to deal with. >> A long while back I proposed a domain XML option for this: >> https://www.redhat.com/archives/libvir-list/2017-December/msg00729.html >> >> The "none" case meant inherit the cgroups placement of the parent >> libvirtd (or now the app using the embedded driver), and would be >> a reasonable default for the embedded case. >> For the other cases, we certainly need to do something to ensure >> uniqueness. This is possibly as simple as including a fixed >> prefix like "qemu-$PID" where $PID is the app embedding the >> driver. That said, we support closing the app, and re-starting >> using the same embedded driver directory, which would change >> PID. > > Instead of PID we can derive the value from the root path, e.g. i-node of the > root dir or some substring of the path hash, say first 6 letters of > md5($root_path). This would guarantee the stability of the prefix across app > restarts (if we assume all root dirs to be on one FS so we don't have > clashing i-node numbers). > Root path hash is more robust also in other cases, like file copy/rename often used for atomic updates. > Michal >
Re: [libvirt PATCHv5 06/15] docs: add virtiofs kbase
> On 26 Feb 2020, at 12:57, Ján Tomko wrote: > > Add a document describing the usage of virtiofs. > --- > docs/kbase.html.in | 3 + > docs/kbase/virtiofs.rst | 152 > 2 files changed, 155 insertions(+) > create mode 100644 docs/kbase/virtiofs.rst > > diff --git a/docs/kbase.html.in b/docs/kbase.html.in > index db84b95b60..7055f4fda4 100644 > --- a/docs/kbase.html.in > +++ b/docs/kbase.html.in > @@ -33,6 +33,9 @@ > Security with QEMU > passthrough > Examination of the security protections used for QEMU and how > they need > configuring to allow use of QEMU passthrough with host > files/devices. > + > +Virtio-FS > +Share a filesystem between the guest and the host > > > > diff --git a/docs/kbase/virtiofs.rst b/docs/kbase/virtiofs.rst > new file mode 100644 > index 00..fe6885d139 > --- /dev/null > +++ b/docs/kbase/virtiofs.rst > @@ -0,0 +1,152 @@ > + > +Sharing files with Virtio-FS > + > + > +=== 8< delete before merging 8< === > +NOTE: if you're looking at this note, this is just a proposal. > +See the up-to-date version on: https://libvirt.org/kbase/virtiofs.html > +=== 8< - 8< === > + > +.. contents:: > + > += > +Virtio-FS > += > + > +Virtio-FS is a shared file system that lets virtual machines access > +a directory tree on the host. Unlike existing approaches, it > +is designed to offer local file system semantics and performance. > + > +See https://virtio-fs.gitlab.io/ > + > +== > +Host setup > +== > + > +The host-side virtiofsd daemon, like other vhost-user backed devices, > +requires shared memory between the host and the guest. As of QEMU 4.2, this > +requires specifying a NUMA topology for the guest and explicitly specifying > +a memory backend. Multiple options are available: > + > +Either of the following: > + > +* Use file-backed memory > + > + Configure the directory where the files backing the memory will be stored > + with the ``memory_backing_dir`` option in ``/etc/libvirt/qemu.conf`` Would it be useful to add a rationale as to why this config dir is not in the XML file itself? > + > + :: > + > +# This directory is used for memoryBacking source if configured as file. > +# NOTE: big files will be stored here > +memory_backing_dir = "/dev/shm/" > + > +* Use hugepage-backed memory > + > + Make sure there are enough huge pages allocated for the requested guest > memory. > + For example, for one guest with 2 GiB of RAM backed by 2 MiB hugepages: > + > + :: > + > + # virsh allocpages 2M 1024 > + > +=== > +Guest setup > +=== > + > +#. Specify the NUMA topology > + > + in the domain XML of the guest. > + For the simplest one-node topology for a guest with 2GiB of RAM and 8 > vCPUs: > + > + :: > + > + > +... > + > + > + memAccess='shared'/> > + > + > + ... > + > + > + Note that the CPU element might already be specified and only one is > allowed. > + > +#. Specify the memory backend > + > + Either of the following: > + > + * File-backed memory > + > + :: > + > + > + ... > + > + > + > + ... > + > + > + This will create a file in the directory specified in ``qemu.conf`` > + > + * Hugepage-backed memory > + > + :: > + > + > + ... > + > + > + > + > + > + > + ... > + > + > +#. Add the ``vhost-user-fs`` QEMU device via the ``filesystem`` element > + > + :: > + > + > +... > + > + ... > + > + > + > + > + > + ... > + > + > + > + Note that despite its name, the ``target dir`` is actually a mount tag > and does > + not have to correspond to the desired mount point in the guest. > + > + So far, ``passthrough`` is the only supported access mode and it requires > + running the ``virtiofsd`` daemon as root. > + > +#. Boot the guest and mount the filesystem > + > + :: > + > + guest# mount -t virtiofs mount_tag /mnt/mount/path > + > + Note: this requires virtiofs support in the guest kernel (Linux v5.4 or > later) > + > +=== > +Optional parameters > +=== > + > +More optional elements can be specified > + > +:: > + > + > + > + > + > + > -- > 2.24.1 >
Re: [libvirt] [PATCH] doc: vtpm only support secrets by UUID at this point
> On 10 Dec 2019, at 16:08, marcandre.lur...@redhat.com wrote: > > From: Marc-André Lureau > > Support by usage name can be considered separately (with a 'usage' > attribute?). > > Cc: Stefan Berger > Signed-off-by: Marc-André Lureau > --- > docs/formatsecret.html.in | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/docs/formatsecret.html.in b/docs/formatsecret.html.in > index 8d0630a7c3..8f5383cf64 100644 > --- a/docs/formatsecret.html.in > +++ b/docs/formatsecret.html.in > @@ -334,8 +334,8 @@ Secret value set > of the vTPM. > The usage type='vtpm' element must contain > a single name element that specifies a usage name > - for the secret. The vTPM secret can then be used by UUID or by > - this usage name via the encryption element of > + for the secret. The vTPM secret can then be used by UUID > + via the encryption element of > a tpm when using an > emulator. > Since 5.6.0. The following is an example > -- > 2.24.0.308.g228f53135a Reminds me of our discussion :-) Reviewed-by: Christophe de Dinechin > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virkeyfile: fix compilation error with clang
> On 10 Dec 2019, at 15:11, Pavel Hrdina wrote:
>
> Clang complains about condition being always true:
>
> src/util/virkeyfile.c:113:23: error: result of comparison of constant 128
> with expression of type 'const char' is always true
> [-Werror,-Wtautological-constant-out-of-range-compare]
>while (!IS_EOF && IS_ASCII(CUR) && CUR != ']')
> ^
> src/util/virkeyfile.c:80:26: note: expanded from macro 'IS_ASCII'
> ~~~ ^ ~~~
>
> Signed-off-by: Pavel Hrdina
> ---
> src/util/virkeyfile.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/util/virkeyfile.c b/src/util/virkeyfile.c
> index 816bfae96d..ee29bd7aa6 100644
> --- a/src/util/virkeyfile.c
> +++ b/src/util/virkeyfile.c
> @@ -77,7 +77,7 @@ struct _virKeyFileParserCtxt {
> #define IS_EOF (ctxt->cur >= ctxt->end)
> #define IS_EOL(c) (((c) == '\n') || ((c) == '\r'))
> #define IS_BLANK(c) (((c) == ' ') || ((c) == '\t'))
> -#define IS_ASCII(c) ((c) < 128)
> +#define IS_ASCII(c) (((unsigned char) c) < 128)
Probably want parentheses around c.
> #define CUR (*ctxt->cur)
> #define NEXT if (!IS_EOF) ctxt->cur++;
>
> --
> 2.23.0
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
> On 22 Nov 2019, at 09:41, Markus Armbruster wrote: > > Reviving this old thread, because I'd like to connect it to more recent > discussions. > > Christophe de Dinechin mailto:dinec...@redhat.com>> > writes: > >> Markus Armbruster writes: >> >>> Peter Krempa writes: >>> >> [...] >>>> From my experience users report non-fatal messages mostly only if it is >>>> spamming the system log. One of instances are very unlikely to be >>>> noticed. >>>> >>>> In my experience it's better to notify us in libvirt of such change and >>>> we will try our best to fix it. >>> >>> How to best alert the layers above QEMU was one of the topic of the KVM >>> Forum 2018 BoF on deprecating stuff. Minutes: >>> >>>Message-ID: <87mur0ls8o@dusky.pond.sub.org> >>>https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg05828.html >>> >>> Relevant part: >>> >>> * We need to communicate "you're using something that is deprecated". >>> How? Right now, we print a deprecation message. Okay when humans use >>> QEMU directly in a shell. However, when QEMU sits at the bottom of a >>> software stack, the message will likely end up in a log file that is >>> effectively write-only. >>> >>> - The one way to get people read log files is crashing their >>>application. A command line option --future could make QEMU crash >>>right after printing a deprecation message. This could help with >>>finding use of deprecated features in a testing environment. >>> >>> - A less destructive way to grab people's attention is to make things >>>run really, really slow: have QEMU go to sleep for a while after >>>printing a deprecation message. >>> >>> - We can also pass the buck to the next layer up: emit a QMP event. >>> >>>Sadly, by the time the next layer connects to QMP, plenty of stuff >>>already happened. We'd have to buffer deprecation events somehow. >>> >>>What would libvirt do with such an event? Log it, taint the domain, >>>emit a (libvirt) event to pass it on to the next layer up. >>> >>> - A completely different idea is to have a configuratin linter. To >>>support doing this at the libvirt level, QEMU could expose "is >>>deprecated" in interface introspection. Feels feasible for QMP, >>>where we already have sufficiently expressive introspection. For >>>CLI, we'd first have to provide that (but we want that anyway). >>> >>> - We might also want to dispay deprecation messages in QEMU's GUI >>>somehow, or on serial consoles. >> >> Sorry for catching up late, this mail thread happened during my PTO. >> >> I remember bringing up at the time [1] that the correct solution needs >> to take into account usage models that vary from >> >> - a workstation case, where displaying an error box is easy and >> convenient, >> >> - to local headless VMs where system-level notification would do the job >> better, allowing us to leverage things like system-wide email notifications >> >> - to large-scale collections of VMs managed by some layered product, >> where the correct reporting would be through something like Insights, >> i.e. you don't scan individual logs, you want something like "913 VMs >> are using deprecated X" >> >> To me, that implies that we need to have a clear division of roles, with >> a standard way to >> >> a) produce the errors, >> b) propagate them, >> c) consume them (at least up to libvirt) >> >> Notice that this work has already been done for "real" errors, >> i.e. there is a real QAPI notion of "errors". AFAICT, warn_report does >> not connect to it, though, it goes through error_vprintf which is really >> just basic logging. >> >> So would it make sense to: >> >> 1. Add a deprecation_report() alongside warn_report()? > > "Grepability" alone would make that worthwhile, I think. > >> 2. Connect warn_report() and all the error_vprintf output to QAPI, >> e.g. using John's suggestion of adding the messages using some >> "warning" or "deprecated" tag? > > This is the difficult part. See my "Exposing feature deprecation to > machine clients (was: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit > filters)
Re: [libvirt] [PATCH 00/30] storagefile, security: qcow2 data_file support
> On 17 Oct 2019, at 14:35, Cole Robinson wrote: > > Hmm why wasn't I in to/cc list, was that intentional? Not at all. I assumed a reply-to would add it automatically and did not bother checking. Probably something wrong in my mu4e config. > > On 10/17/19 5:26 AM, Christophe de Dinechin wrote: >> >> Cole Robinson writes: >> >>> This series is the first steps to teaching libvirt about qcow2 >>> data_file support, aka external data files or qcow2 external metadata. >>> >>> A bit about the feature: it was added in qemu 4.0. It essentially >>> creates a two part image file: a qcow2 layer that just tracks the >>> image metadata, and a separate data file which is stores the VM >> >> s/is stores/stores/ > > Indeed, but what's the benefit of correcting grammar in a cover letter > for already applied patches? Obviously, I realized you had already applied that patch only after sending my email ;-) I saw that there was still some active discussion on that thread, so I assumed it was still under review. >> >> What happens if you try to do that in place, i.e. EXISTING==NEW? >> > > Results in an unbootable image in my testing. I'm guessing it > initializes the raw image first, similar to trying to use 'qemu-img > create' with an existing raw image as data_file > > https://bugzilla.redhat.com/show_bug.cgi?id=1688814#c8 > <https://bugzilla.redhat.com/show_bug.cgi?id=1688814#c8> OK. Might be worth catching or fixing at some point. Christophe-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 12/30] storagefile: Fix backing format \0 check
Daniel Henrique Barboza writes: > On 10/7/19 6:49 PM, Cole Robinson wrote: >> >From qemu.git docs/interop/qcow2.txt > > Here's the '>' again. I think this is something you're using to cite an > external source in the commit message. Is that it? No, that is the way email "protects" lines that begin with "From". You rarely see it nowadays because the mail itself is encoded, but if you use plain text and send in ASCII, this will happen. See page 78 of "The Unix Haters Handbook" [1] for details [1] https://web.mit.edu/~simsong/www/ugh.pdf -- Cheers, Christophe de Dinechin (IRC c3d) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/30] storagefile, security: qcow2 data_file support
Cole Robinson writes: > This series is the first steps to teaching libvirt about qcow2 > data_file support, aka external data files or qcow2 external metadata. > > A bit about the feature: it was added in qemu 4.0. It essentially > creates a two part image file: a qcow2 layer that just tracks the > image metadata, and a separate data file which is stores the VM s/is stores/stores/ > disk contents. AFAICT the driving use case is to keep a fully coherent > raw disk image on disk, and only use qcow2 as an intermediate metadata > layer when necessary, for things like incremental backup support. > > The original qemu patch posting is here: > https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07496.html > > For testing, you can create a new qcow2+raw data_file image from an > existing image, like: > > qemu-img convert -O qcow2 \ > -o data_file=NEW.raw,data_file_raw=yes > EXISTING.raw NEW.qcow2 What happens if you try to do that in place, i.e. EXISTING==NEW? > > The goal of this series is to teach libvirt enough about this case > so that we can correctly relabel the data_file on VM startup/shutdown. > The main functional changes are > > * Teach storagefile how to parse out data_file from the qcow2 header > * Store the raw string as virStorageSource->externalDataStoreRaw > * Track that as its out virStorageSource in externalDataStore > * dac/selinux relabel externalDataStore as needed > -- Cheers, Christophe de Dinechin (IRC c3d) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] tests: qemublock: Use bigger numbers as dummy capacity/physical
Peter Krempa writes:
> Actually test that the full range is available.
>
[...]
> return -1;
> diff --git a/tests/qemublocktestdata/imagecreate/luks-encopts.json
> b/tests/qemublocktestdata/imagecreate/luks-encopts.json
> index f065ad89a7..c5ca863f5b 100644
> --- a/tests/qemublocktestdata/imagecreate/luks-encopts.json
> +++ b/tests/qemublocktestdata/imagecreate/luks-encopts.json
> @@ -2,7 +2,7 @@ protocol:
> {
>"driver": "file",
>"filename": "/var/lib/libvirt/images/i.img",
> - "size": 42
> + "size": 4294967296
> }
Does any of these tests actually require that much space to run?
If so, what is the impact on QE teams?
--
Cheers,
Christophe de Dinechin (IRC c3d)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v1 1/3] adding unplug_timeout QEMU conf
Daniel P. Berrangé writes: >> > >> >> The reason for this timeout is that we originally promised something >> that we cannot deliver - a synchronous device detach API, while the >> operation itself is asynchronous. I'm not a fan of exposing it and >> making it configurable. > > I'm especially *not* a fan because the commit messages says this is > a problem on certain architectures. Since we know what those arches > are, we should use a larger timeout for those arches out of the box. > Requiring admin to set a config param to fix the architectures is > super unpleasant out of the box experiance. True, but also notice that 5 seconds is also already close to the attention span time limit for users [1]. So increasing it to 10s might bring people to believe things are stuck, unless you provide some sort of feedback that this is normal. https://www.nngroup.com/articles/response-times-3-important-limits/ > > > 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 :| -- Cheers, Christophe de Dinechin (IRC c3d) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH trivial] qemu_conf.c: removing unused virQEMUDriverConfigPtr variable
Daniel Henrique Barboza writes:
> 'virQEMUDriverConfigPtr cfg' is declared, initiated, but never
> used in virQEMUDriverCreateCapabilities().
>
> Signed-off-by: Daniel Henrique Barboza
> ---
>
> Accidental discovery while reading code.
>
> src/qemu/qemu_conf.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 89c183e46a..d771bb6916 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -1266,7 +1266,6 @@ virCapsPtr
> virQEMUDriverCreateCapabilities(virQEMUDriverPtr driver)
> virSecurityManagerPtr *sec_managers = NULL;
> /* Security driver data */
> const char *doi, *model, *lbl, *type;
> -virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> const int virtTypes[] = {VIR_DOMAIN_VIRT_KVM,
> VIR_DOMAIN_VIRT_QEMU,};
>
> @@ -1313,13 +1312,11 @@ virCapsPtr
> virQEMUDriverCreateCapabilities(virQEMUDriverPtr driver)
> }
> VIR_FREE(sec_managers);
>
> -virObjectUnref(cfg);
> return caps;
>
> error:
> VIR_FREE(sec_managers);
> virObjectUnref(caps);
> -virObjectUnref(cfg);
> return NULL;
> }
>
> --
> 2.21.0
Reviewed-by: Christophe de Dinechin
--
Cheers,
Christophe de Dinechin (IRC c3d)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
John Snow writes: > On 8/29/19 12:45 PM, Christophe de Dinechin wrote: >> [...] >> Sorry for catching up late, this mail thread happened during my PTO. >> >> I remember bringing up at the time [1] that the correct solution needs >> to take into account usage models that vary from >> >> - a workstation case, where displaying an error box is easy and >> convenient, >> >> - to local headless VMs where system-level notification would do the job >> better, allowing us to leverage things like system-wide email notifications >> >> - to large-scale collections of VMs managed by some layered product, >> where the correct reporting would be through something like Insights, >> i.e. you don't scan individual logs, you want something like "913 VMs >> are using deprecated X" >> >> To me, that implies that we need to have a clear division of roles, with >> a standard way to >> >> a) produce the errors, >> b) propagate them, > > I started replying to this thread to the other mail you sent; I think > this is going to be fairly involved. I wouldn't mind being proven wrong > though. Yes, I think it does look involved, but mostly for historical reasons. In other words, what is complicated is preserving the historical behaviors so as to not break existing consumers. > >> c) consume them (at least up to libvirt) >> >> Notice that this work has already been done for "real" errors, >> i.e. there is a real QAPI notion of "errors". AFAICT, warn_report does >> not connect to it, though, it goes through error_vprintf which is really >> just basic logging. >> >> So would it make sense to: >> >> 1. Add a deprecation_report() alongside warn_report()? >> > > Where's that get routed to? just an error_vprintf style situation? Yes, but see below. > >> 2. Connect warn_report() and all the error_vprintf output to QAPI, >>e.g. using John's suggestion of adding the messages using some >>"warning" or "deprecated" tag? >> > > How do you correlate them? Without having looked at the code much, I think I would 1. extend the existing QAPI error to support warnings, deprecations and info messages. The first problem I see is that there is no error, so we may sometimes need to create one when there was none before. And of course make sure that this does not ultimately show as an error, but as a success with additional annotations. 2. replace the current "link + if" switching for error_vprintf with some actual notification mechanism, with one option routine to monitor_vprintf, one to stderr, one to log file, and then an additional path that would post a newly minted qapi warning. > >> 3. Teach libvirt how to consume that new tag and pass it along? >> > > I think it's not libvirt's job to pass it along, exactly -- libvirt made > the decision for which features to engage in QEMU, not the end user. First, by "pass along", I meant to possible layered products or management software. We don't necessarily need a new virErrorLevel, deprecation could be a warning with some special domain, e.g. VIR_FROM_DEPRECATION. There may be a need to add some API here. Looking at the code, it's not obvious to me that libvirt has any notion of error priority. In other words, if you raise an error then a warning, you get the warning as the last error, right? Second, why not report the use of deprecated features? I don't fully buy the rationale that libvirt engages the features, because it does not do it on its own, it does it because the user made some specific request. This point of view also seems to require that libvirt or the user should know ahead of time it's about to engage a deprecated feature. To me, the problem is precisely that neither libvirt nor the user knows, which is why we are discussing how to best make it known. > > If the user upgrades QEMU but not libvirt, it's not really anything they > have control over and they shouldn't be pestered with such things. > > However, if libvirt accidentally released a version that engages > deprecated behavior (and were unaware of it), it'd be nice to get user > reports, surely? > > Logging messages for libvirt might be the best that can be done there in > that case. I personally would treat that like any warning. > > > In contrast, power user tools like QMP libraries, qmp-shell and others > allow more direct and meaningful access to QMP, so those should report > deprecation messages to the user. Agreed. > >> >> [1] https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg06131.html >> -- Thanks, Christophe de Dinechin (IRC c3d) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
Markus Armbruster writes: > Peter Krempa writes: > [...] >> From my experience users report non-fatal messages mostly only if it is >> spamming the system log. One of instances are very unlikely to be >> noticed. >> >> In my experience it's better to notify us in libvirt of such change and >> we will try our best to fix it. > > How to best alert the layers above QEMU was one of the topic of the KVM > Forum 2018 BoF on deprecating stuff. Minutes: > > Message-ID: <87mur0ls8o@dusky.pond.sub.org> > https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg05828.html > > Relevant part: > > * We need to communicate "you're using something that is deprecated". > How? Right now, we print a deprecation message. Okay when humans use > QEMU directly in a shell. However, when QEMU sits at the bottom of a > software stack, the message will likely end up in a log file that is > effectively write-only. > > - The one way to get people read log files is crashing their > application. A command line option --future could make QEMU crash > right after printing a deprecation message. This could help with > finding use of deprecated features in a testing environment. > > - A less destructive way to grab people's attention is to make things > run really, really slow: have QEMU go to sleep for a while after > printing a deprecation message. > > - We can also pass the buck to the next layer up: emit a QMP event. > > Sadly, by the time the next layer connects to QMP, plenty of stuff > already happened. We'd have to buffer deprecation events somehow. > > What would libvirt do with such an event? Log it, taint the domain, > emit a (libvirt) event to pass it on to the next layer up. > > - A completely different idea is to have a configuratin linter. To > support doing this at the libvirt level, QEMU could expose "is > deprecated" in interface introspection. Feels feasible for QMP, > where we already have sufficiently expressive introspection. For > CLI, we'd first have to provide that (but we want that anyway). > > - We might also want to dispay deprecation messages in QEMU's GUI > somehow, or on serial consoles. Sorry for catching up late, this mail thread happened during my PTO. I remember bringing up at the time [1] that the correct solution needs to take into account usage models that vary from - a workstation case, where displaying an error box is easy and convenient, - to local headless VMs where system-level notification would do the job better, allowing us to leverage things like system-wide email notifications - to large-scale collections of VMs managed by some layered product, where the correct reporting would be through something like Insights, i.e. you don't scan individual logs, you want something like "913 VMs are using deprecated X" To me, that implies that we need to have a clear division of roles, with a standard way to a) produce the errors, b) propagate them, c) consume them (at least up to libvirt) Notice that this work has already been done for "real" errors, i.e. there is a real QAPI notion of "errors". AFAICT, warn_report does not connect to it, though, it goes through error_vprintf which is really just basic logging. So would it make sense to: 1. Add a deprecation_report() alongside warn_report()? 2. Connect warn_report() and all the error_vprintf output to QAPI, e.g. using John's suggestion of adding the messages using some "warning" or "deprecated" tag? 3. Teach libvirt how to consume that new tag and pass it along? [1] https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg06131.html -- Cheers, Christophe de Dinechin (IRC c3d) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters
John Snow writes:
[...]
>
> This might be OK to do right away, though.
>
> I asked Markus this not too long ago; do we want to amend the QAPI
> schema specification to allow commands to return with "Warning" strings,
> or "Deprecated" stings to allow in-band deprecation notices for cases
> like these?
>
> example:
>
> { "return": {},
> "deprecated": True,
> "warning": "Omitting filter-node-name parameter is deprecated, it will
> be required in the future"
> }
>
> There's no "error" key, so this should be recognized as success by
> compatible clients, but they'll definitely see the extra information.
>
> Part of my motivation is to facilitate a more aggressive deprecation of
> legacy features by ensuring that we are able to rigorously notify users
> through any means that they need to adjust their scripts.
I like this approach even if there is no consumer today. It does not
hurt, and it is indeed a motivation to develop consumers that care.
I personally find this much easier to swallow than any kind of crash on
deprecation, which already at the BoF seemed like a really big hammer to
kill a fly.
CC'ing Andrea as well, because we discussed recently about how to deal
with error checking in general, and if a new error checking framework is
being put in place, adding deprecation to the thinking could be a good
idea.
--
Cheers,
Christophe de Dinechin (IRC c3d)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 46/48] util: storage identity attrs as virTypedParameter internally
Andrea Bolognani writes:
> On Mon, 2019-07-29 at 18:11 +0100, Daniel P. Berrangé wrote:
>> util: storage identity attrs as virTypedParameter internally
>
>> -if (virStrToLong_i(userid, NULL, 10, ) < 0)
>> +ret = virTypedParamsGetULLong(ident->params,
>> + ident->nparams,
>> + VIR_CONNECT_IDENTITY_OS_USER_ID,
>> + );
>> +if (ret < 0)
>> return -1;
>>
>> -*uid = (uid_t)val;
>> +if (ret == 1)
>> +*uid = (uid_t)val;
>
> In case Christophe is following along: this is one of the cases where
> libvirt functions don't follow the usual 0 means success, <0 means
> failure mantra.
Thanks ;-)
>
> [...]
>> int virIdentityGetOSProcessID(virIdentityPtr ident,
>>pid_t *pid)
>> {
>> -unsigned long long val;
>> -const char *processid;
>> +int ret;
>> +long long val;
>
> I still think we should be using ull for pids.
Curious why (I'm too lazy to look it up in earlier discussions)?
In general, giving a name to an int type is a good idea, isn't it?
>
>> -*pid = (pid_t)val;
>> +if (ret == 1)
>> +*pid = (gid_t)val;
>
> This should be
>
> *pid = (pid_t)val;
You made me look at that code again, and now I'm confused as to why it's
OK to leave garbage in *pid if we fail to find the corresponding typed
param. Previously, the function returned -1 in that case, to indicate
failure. Now, it returns 0, but does not update *uid. Is that intentional?
>
> [...]
>> +++ b/tests/viridentitytest.c
>> @@ -166,7 +109,8 @@ static int testIdentityGetSystem(const void *data)
>> goto cleanup;
>>
>> if (STRNEQ_NULLABLE(val, context)) {
>> -VIR_DEBUG("Unexpected SELinux context attribute");
>> +VIR_DEBUG("Unexpected SELinux context attribute '%s' != '%s'",
>> + val, context);
>> goto cleanup;
>> }
>
> This change also doesn't belong in this patch. You can put it in the
> same one as the other SELinux-related test suite fix, though.
>
> And since you're tweaking the message, I suggest something like
>
> VIR_DEBUG("Unexpected SELinux context: expected='%s' actual='%s'",
> context, val);
>
> for easier debugging.
>
> --
> Andrea Bolognani / Red Hat / Virtualization
--
Cheers,
Christophe de Dinechin (IRC c3d)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 45/48] util: make generic identity accessors private
afadab4 100644
> --- a/tests/virnetserverclienttest.c
> +++ b/tests/virnetserverclienttest.c
> @@ -53,9 +53,9 @@ static int testIdentity(const void *opaque ATTRIBUTE_UNUSED)
> virNetServerClientPtr client = NULL;
> virIdentityPtr ident = NULL;
> const char *gotUsername = NULL;
> -const char *gotUserID = NULL;
> +uid_t gotUserID;
> const char *gotGroupname = NULL;
> -const char *gotGroupID = NULL;
> +gid_t gotGroupID;
> const char *gotSELinuxContext = NULL;
>
> if (socketpair(PF_UNIX, SOCK_STREAM, 0, sv) < 0) {
> @@ -85,9 +85,8 @@ static int testIdentity(const void *opaque ATTRIBUTE_UNUSED)
> goto cleanup;
> }
>
> -if (virIdentityGetAttr(ident,
> - VIR_IDENTITY_ATTR_OS_USER_NAME,
> - ) < 0) {
> +if (virIdentityGetOSUserName(ident,
> + ) < 0) {
I think this would fit on a single line now.
> fprintf(stderr, "Missing username in identity\n");
> goto cleanup;
> }
> @@ -97,21 +96,19 @@ static int testIdentity(const void *opaque
> ATTRIBUTE_UNUSED)
> goto cleanup;
> }
>
> -if (virIdentityGetAttr(ident,
> - VIR_IDENTITY_ATTR_OS_USER_ID,
> - ) < 0) {
> +if (virIdentityGetOSUserID(ident,
> + ) < 0) {
Would fit on a single line
> fprintf(stderr, "Missing user ID in identity\n");
> goto cleanup;
> }
> -if (STRNEQ_NULLABLE("666", gotUserID)) {
> -fprintf(stderr, "Want username '666' got '%s'\n",
> -NULLSTR(gotUserID));
> +if (666 != gotUserID) {
> +fprintf(stderr, "Want username '666' got '%llu'\n",
> +(unsigned long long)gotUserID);
> goto cleanup;
> }
>
> -if (virIdentityGetAttr(ident,
> - VIR_IDENTITY_ATTR_OS_GROUP_NAME,
> - ) < 0) {
> +if (virIdentityGetOSGroupName(ident,
> + ) < 0) {
> fprintf(stderr, "Missing groupname in identity\n");
> goto cleanup;
> }
> @@ -121,27 +118,25 @@ static int testIdentity(const void *opaque
> ATTRIBUTE_UNUSED)
> goto cleanup;
> }
>
> -if (virIdentityGetAttr(ident,
> - VIR_IDENTITY_ATTR_OS_GROUP_ID,
> - ) < 0) {
> +if (virIdentityGetOSGroupID(ident,
> +) < 0) {
> fprintf(stderr, "Missing group ID in identity\n");
> goto cleanup;
> }
> -if (STRNEQ_NULLABLE("7337", gotGroupID)) {
> -fprintf(stderr, "Want groupname '7337' got '%s'\n",
> -NULLSTR(gotGroupID));
> +if (7337 != gotGroupID) {
> +fprintf(stderr, "Want groupname '7337' got '%llu'\n",
> +(unsigned long long)gotGroupID);
> goto cleanup;
> }
>
> -if (virIdentityGetAttr(ident,
> - VIR_IDENTITY_ATTR_SELINUX_CONTEXT,
> - ) < 0) {
> +if (virIdentityGetSELinuxContext(ident,
> + ) < 0) {
> fprintf(stderr, "Missing SELinux context in identity\n");
> goto cleanup;
> }
> if (STRNEQ_NULLABLE("foo_u:bar_r:wizz_t:s0-s0:c0.c1023",
> gotSELinuxContext)) {
> -fprintf(stderr, "Want groupname 'foo_u:bar_r:wizz_t:s0-s0:c0.c1023'
> got '%s'\n",
> -NULLSTR(gotGroupID));
> +fprintf(stderr, "Want SELinux context
> 'foo_u:bar_r:wizz_t:s0-s0:c0.c1023' got '%s'\n",
> +NULLSTR(gotSELinuxContext));
> goto cleanup;
That last fix not really related to the rest of the patch, but useful.
(I don't know how important it is in the libvirt community to not mix
"side fixes" with large patch series. I don't personally mind at all.)
Only minor nits, nothing functional.
Reviewed-by: Christophe de Dinechin
> }
>
> --
> 2.21.0
--
Cheers,
Christophe de Dinechin (IRC c3d)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 44/48] util: change identity class attribute names
Daniel P. Berrangé writes:
> Change the identity class attribute names with a "UNIX" tag to have a
> more generic "OS" tag, since when we expose this in the public API we
> want it to be more flexible for the future.
>
> Signed-off-by: Daniel P. Berrangé
> ---
> src/access/viraccessdriverpolkit.c | 12 ++---
> src/admin/admin_server.c | 10 ++--
> src/libvirt_private.syms | 24 -
> src/rpc/virnetserverclient.c | 12 ++---
> src/util/viridentity.c | 84 +++---
> src/util/viridentity.h | 60 ++---
> tests/viridentitytest.c| 18 +++
> tests/virnetserverclienttest.c | 8 +--
> 8 files changed, 114 insertions(+), 114 deletions(-)
>
> diff --git a/src/access/viraccessdriverpolkit.c
> b/src/access/viraccessdriverpolkit.c
> index b1473cd0a4..b98122d4a3 100644
> --- a/src/access/viraccessdriverpolkit.c
> +++ b/src/access/viraccessdriverpolkit.c
> @@ -88,19 +88,19 @@ virAccessDriverPolkitGetCaller(const char *actionid,
> return -1;
> }
>
> -if (virIdentityGetUNIXProcessID(identity, pid) < 0) {
> +if (virIdentityGetOSProcessID(identity, pid) < 0) {
> virAccessError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("No UNIX process ID available"));
> + _("No OS process ID available"));
What about "No process ID available"?
> goto cleanup;
> }
> -if (virIdentityGetUNIXProcessTime(identity, startTime) < 0) {
> +if (virIdentityGetOSProcessTime(identity, startTime) < 0) {
> virAccessError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("No UNIX process start time available"));
> + _("No OS process start time available"));
What about "No process start time available"?
> goto cleanup;
> }
> -if (virIdentityGetUNIXUserID(identity, uid) < 0) {
> +if (virIdentityGetOSUserID(identity, uid) < 0) {
> virAccessError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("No UNIX caller UID available"));
> + _("No OS caller UID available"));
What about "No caller user ID available"?
(the term "user ID" is used elsewhere in the code)
The UID acronym is not widely known outside of programmer circles,
and for those who know it, it's practically always related to Unix-like
systems (on Windows, it's generally called a SID).
> goto cleanup;
> }
>
> diff --git a/src/admin/admin_server.c b/src/admin/admin_server.c
> index f2a38f6dfa..b92eb2fdc6 100644
> --- a/src/admin/admin_server.c
> +++ b/src/admin/admin_server.c
> @@ -257,29 +257,29 @@ adminClientGetInfo(virNetServerClientPtr client,
> pid_t pid;
> uid_t uid;
> gid_t gid;
> -if (virIdentityGetUNIXUserID(identity, ) < 0 ||
> +if (virIdentityGetOSUserID(identity, ) < 0 ||
Even in function names, I believe that "OS" could be removed without
much of a loss in readability.
> virTypedParamsAddInt(, nparams, ,
> VIR_CLIENT_INFO_UNIX_USER_ID, uid) < 0)
> goto cleanup;
>
> -if (virIdentityGetUNIXUserName(identity, ) < 0 ||
> +if (virIdentityGetOSUserName(identity, ) < 0 ||
The above comment applies even more for a user name.
> virTypedParamsAddString(, nparams, ,
> VIR_CLIENT_INFO_UNIX_USER_NAME,
There are some spots where "UNIX" remains...
> attr) < 0)
> goto cleanup;
>
> -if (virIdentityGetUNIXGroupID(identity, ) < 0 ||
> +if (virIdentityGetOSGroupID(identity, ) < 0 ||
> virTypedParamsAddInt(, nparams, ,
> VIR_CLIENT_INFO_UNIX_GROUP_ID, gid) < 0)
> goto cleanup;
>
[...]
>
> if (virIdentityGetAttr(ident,
> - VIR_IDENTITY_ATTR_UNIX_GROUP_ID,
> + VIR_IDENTITY_ATTR_OS_GROUP_ID,
... whereas it is changed at other places.
> ) < 0) {
> fprintf(stderr, "Missing group ID in identity\n");
> goto cleanup;
> --
> 2.21.0
No functional issue found.
Reviewed-by: Christophe de Dinechin
--
Cheers,
Christophe de Dinechin (IRC c3d)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 06/48] remote: stop trying to print help as giant blocks of text
> On 30 Jul 2019, at 12:52, Andrea Bolognani wrote: > > On Mon, 2019-07-29 at 18:10 +0100, Daniel P. Berrangé wrote: >> The remote daemon tries to print out its help text in a couple of giant >> blocks of text. This has already lead to duplication of the text for the >> privileged vs unprivileged execution mode. With the introduction of more >> daemons, this text is going to be duplicated many more times with small >> variations. This is very unfriendly to translators as they have to >> translate approximately the same text many times with small tweaks. >> >> Splitting the text up into individual strings to print means that each >> piece will only need translating once. It also gets rid of all the >> layout information from the translated strings, so avoids the problem of >> translators breaking formatting by mistake. >> >> Signed-off-by: Daniel P. Berrangé >> --- >> src/remote/remote_daemon.c | 128 ++--- >> src/remote/remote_driver.h | 1 - >> 2 files changed, 64 insertions(+), 65 deletions(-) > > While I'm sympathetic to Christophe's plead for being nicer to French > users and translators, right now I'd like to avoid another respin of > this already pretty big series, so whether you address the changes he > asked for before pushing or as a follow-up patch you still get a > Reviewed-by: Andrea Bolognani Fine with me either way. > > -- > Andrea Bolognani / Red Hat / Virtualization > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 22/48] network: introduce virtnetworkd daemon
> On 30 Jul 2019, at 13:20, Daniel P. Berrangé wrote: > > On Tue, Jul 30, 2019 at 01:08:53PM +0200, Christophe de Dinechin wrote: >> >> Daniel P. Berrangé writes: >> >>> The virtnetworkd daemon will be responsible for providing the network API >>> driver functionality. The network driver is still loaded by the main >>> libvirtd daemon at this stage, so virtnetworkd must not be running at >>> the same time. >>> >>> Reviewed-by: Andrea Bolognani >>> Signed-off-by: Daniel P. Berrangé >>> --- >>> .gitignore | 4 ++ >>> libvirt.spec.in | 8 >>> src/network/Makefile.inc.am | 61 + >>> src/network/virtnetworkd.service.in | 25 >>> 4 files changed, 98 insertions(+) >>> create mode 100644 src/network/virtnetworkd.service.in >>> diff --git a/src/network/virtnetworkd.service.in >>> b/src/network/virtnetworkd.service.in >>> new file mode 100644 >>> index 00..656e8b4f84 >>> --- /dev/null >>> +++ b/src/network/virtnetworkd.service.in >>> @@ -0,0 +1,25 @@ >>> +[Unit] >>> +Description=Virtualization network daemon >>> +Conflicts=libvirtd.service >>> +Requires=virtnetworkd.socket >>> +Requires=virtnetworkd-ro.socket >>> +Requires=virtnetworkd-admin.socket >>> +After=network.target >>> +After=dbus.service >>> +After=apparmor.service >>> +After=local-fs.target >>> +Documentation=man:libvirtd(8) >> >> Maybe you need to create man page aliases for each of the new daemon names? > > Yes, I need to figure out a way to auto-generate the man page for each > daemon with irrelevant content stripped. Or simply refdirect to the original man page, using the .so macro? In which case you could generate the man page with a rule like: echo “.so $LIBVIRTD_MANPAGE” > $@ (But then, maybe you really want per-daemon man page…) Thanks, Christophe > > 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 :| > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 10/48] remote: conditionalize IP socket usage in libvirtd daemon
> On 30 Jul 2019, at 13:15, Andrea Bolognani wrote: > > On Tue, 2019-07-30 at 12:00 +0100, Daniel P. Berrangé wrote: >> On Tue, Jul 30, 2019 at 12:46:31PM +0200, Christophe de Dinechin wrote: >>> Daniel P. Berrangé writes: >>>> @@ -381,11 +381,13 @@ static int ATTRIBUTE_NONNULL(3) >>>> daemonSetupNetworking(virNetServerPtr srv, >>>> virNetServerPtr srvAdm, >>>> struct daemonConfig *config, >>>> +#ifdef ENABLE_IP >>>> + bool ipsock, >>>> + bool privileged, >>>> +#endif /* ! ENABLE_IP */ >>> >>> Absolute nit, but I would move the two bool last to avoid arch-dependent >>> and config-dependent padding in the middle of the struct. >> >> I moved them here, because if you have #ifdef conditional around >> the last parameter in the function, the formatting gets messy >> wrt to the closing ')', and need to trim the trailing ',' on the >> previous parameter. > > I think Christophe, despite the fact that he quoted the function > signature, was actually referring to struct daemonConfig and the > members within. Indeed. > > I'm not sure whether we care about the padding and relative alignment > in this case, though. “Absolute nit” :-) > > -- > Andrea Bolognani / Red Hat / Virtualization > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 24/48] storage: introduce virtstoraged daemon
rtd.aug.in | \ > + $(SED) -e '/[@]CUT_ENABLE_IP[@]/,/[@]END[@]/d' \ > + -e 's/[@]DAEMON_NAME[@]/virtstoraged/' \ > + -e 's/[@]DAEMON_NAME_UC[@]/Virtstoraged/' \ > + > $@ || rm -f $@ > + > > libvirt_storage_backend_fs_la_SOURCES = $(STORAGE_DRIVER_FS_SOURCES) > libvirt_storage_backend_fs_la_CFLAGS = \ > diff --git a/src/storage/virtstoraged.service.in > b/src/storage/virtstoraged.service.in > new file mode 100644 > index 00..9aa26764a9 > --- /dev/null > +++ b/src/storage/virtstoraged.service.in > @@ -0,0 +1,26 @@ > +[Unit] > +Description=Virtualization storage daemon > +Conflicts=libvirtd.service > +Requires=virtstoraged.socket > +Requires=virtstoraged-ro.socket > +Requires=virtstoraged-admin.socket > +After=network.target > +After=dbus.service > +After=iscsid.service > +After=apparmor.service > +After=local-fs.target > +After=remote-fs.target > +Documentation=man:libvirtd(8) > +Documentation=https://libvirt.org > + > +[Service] > +Type=notify > +ExecStart=@sbindir@/virtstoraged --timeout 120 > +ExecReload=/bin/kill -HUP $MAINPID > +Restart=on-failure > + > +[Install] > +WantedBy=multi-user.target > +Also=virtstoraged.socket > +Also=virtstoraged-ro.socket > +Also=virtstoraged-admin.socket > -- > 2.21.0 Reviewed-by: Christophe de Dinechin -- Cheers, Christophe de Dinechin (IRC c3d) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 23/48] interface: introduce virtinterfaced daemon
EST) > + $(AM_V_GEN)$(AUG_GENTEST) interface/virtinterfaced.conf \ > + $(srcdir)/remote/test_libvirtd.aug.in | \ > + $(SED) -e '/[@]CUT_ENABLE_IP[@]/,/[@]END[@]/d' \ > + -e 's/[@]DAEMON_NAME[@]/virtinterfaced/' \ > + -e 's/[@]DAEMON_NAME_UC[@]/Virtinterfaced/' \ > + > $@ || rm -f $@ > + > endif WITH_INTERFACE > diff --git a/src/interface/virtinterfaced.service.in > b/src/interface/virtinterfaced.service.in > new file mode 100644 > index 00..ff3a611d16 > --- /dev/null > +++ b/src/interface/virtinterfaced.service.in > @@ -0,0 +1,24 @@ > +[Unit] > +Description=Virtualization interface daemon > +Conflicts=libvirtd.service > +Requires=virtinterfaced.socket > +Requires=virtinterfaced-ro.socket > +Requires=virtinterfaced-admin.socket > +After=network.target > +After=dbus.service > +After=apparmor.service > +After=local-fs.target > +Documentation=man:libvirtd(8) Provide its own man page? > +Documentation=https://libvirt.org > + > +[Service] > +Type=notify > +ExecStart=@sbindir@/virtinterfaced --timeout 120 > +ExecReload=/bin/kill -HUP $MAINPID > +Restart=on-failure > + > +[Install] > +WantedBy=multi-user.target > +Also=virtinterfaced.socket > +Also=virtinterfaced-ro.socket > +Also=virtinterfaced-admin.socket > -- > 2.21.0 Reviewed-by: Christophe de Dinechin -- Cheers, Christophe de Dinechin (IRC c3d) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 22/48] network: introduce virtnetworkd daemon
etwork/virtnetworkd.conf \ > + $(srcdir)/remote/test_libvirtd.aug.in | \ > + $(SED) -e '/[@]CUT_ENABLE_IP[@]/,/[@]END[@]/d' \ > + -e 's/[@]DAEMON_NAME[@]/virtnetworkd/' \ > + -e 's/[@]DAEMON_NAME_UC[@]/Virtnetworkd/' \ > + > $@ || rm -f $@ > + > libexec_PROGRAMS += libvirt_leaseshelper > libvirt_leaseshelper_SOURCES = $(NETWORK_LEASES_HELPER_SOURCES) > libvirt_leaseshelper_LDFLAGS = \ > diff --git a/src/network/virtnetworkd.service.in > b/src/network/virtnetworkd.service.in > new file mode 100644 > index 00..656e8b4f84 > --- /dev/null > +++ b/src/network/virtnetworkd.service.in > @@ -0,0 +1,25 @@ > +[Unit] > +Description=Virtualization network daemon > +Conflicts=libvirtd.service > +Requires=virtnetworkd.socket > +Requires=virtnetworkd-ro.socket > +Requires=virtnetworkd-admin.socket > +After=network.target > +After=dbus.service > +After=apparmor.service > +After=local-fs.target > +Documentation=man:libvirtd(8) Maybe you need to create man page aliases for each of the new daemon names? > +Documentation=https://libvirt.org > + > +[Service] > +Type=notify > +ExecStart=@sbindir@/virtnetworkd --timeout 120 > +ExecReload=/bin/kill -HUP $MAINPID > +Restart=on-failure > +KillMode=process > + > +[Install] > +WantedBy=multi-user.target > +Also=virtnetworkd.socket > +Also=virtnetworkd-ro.socket > +Also=virtnetworkd-admin.socket > -- > 2.21.0 Reviewed-by: Christophe de Dinechin -- Cheers, Christophe de Dinechin (IRC c3d) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 21/48] secret: introduce virtsecretd daemon
EMON_NAME_UC[@]/Virtsecretd/' \ > + > $@ || rm -f $@ > + > endif WITH_SECRETS > diff --git a/src/secret/virtsecretd.service.in > b/src/secret/virtsecretd.service.in > new file mode 100644 > index 00..00cdc26b97 > --- /dev/null > +++ b/src/secret/virtsecretd.service.in > @@ -0,0 +1,24 @@ > +[Unit] > +Description=Virtualization secret daemon > +Conflicts=libvirtd.service > +Requires=virtsecretd.socket > +Requires=virtsecretd-ro.socket > +Requires=virtsecretd-admin.socket > +After=network.target > +After=dbus.service > +After=apparmor.service > +After=local-fs.target > +Documentation=man:libvirtd(8) > +Documentation=https://libvirt.org At some point, would it be useful to have one doc page per driver? > + > +[Service] > +Type=notify > +ExecStart=@sbindir@/virtsecretd --timeout 120 > +ExecReload=/bin/kill -HUP $MAINPID > +Restart=on-failure > + > +[Install] > +WantedBy=multi-user.target > +Also=virtsecretd.socket > +Also=virtsecretd-ro.socket > +Also=virtsecretd-admin.socket > -- > 2.21.0 Reviewed-by: Christophe de Dinechin -- Cheers, Christophe de Dinechin (IRC c3d) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 20/48] remote: introduce virtproxyd daemon to handle IP connectivity
0)
> return -1;
> +# else
> +/* This is virtproxyd which merely proxies to the per-driver
> + * daemons for back compat, and also allows IP connectivity.
> + */
> +# endif
> #else
> /* This is the legacy monolithic libvirtd built with all drivers
> *
> @@ -906,9 +912,9 @@ daemonUsage(const char *argv0, bool privileged)
> { "-h | --help", N_("Display program help") },
> { "-v | --verbose", N_("Verbose messages") },
> { "-d | --daemon", N_("Run as a daemon & write PID file") },
> -#ifdef ENABLE_IP
> +#if defined(ENABLE_IP) && defined(LIBVIRTD)
> { "-l | --listen", N_("Listen for TCP/IP connections") },
> -#endif /* ENABLE_IP */
> +#endif /* ENABLE_IP && LIBVIRTD */
> { "-t | --timeout ", N_("Exit after timeout period") },
> { "-f | --config ", N_("Configuration file") },
> { "-V | --version", N_("Display version information") },
> @@ -985,7 +991,11 @@ int main(int argc, char **argv) {
> int verbose = 0;
> int godaemon = 0;
> #ifdef ENABLE_IP
> +# ifdef LIBVIRTD
> int ipsock = 0;
> +# else
> +int ipsock = 1; /* listen_tcp/listen_tls default to 0 */
> +# endif
> #endif /* ! ENABLE_IP */
> struct daemonConfig *config;
> bool privileged = geteuid() == 0 ? true : false;
> @@ -996,9 +1006,9 @@ int main(int argc, char **argv) {
> struct option opts[] = {
> { "verbose", no_argument, , 'v'},
> { "daemon", no_argument, , 'd'},
> -#ifdef ENABLE_IP
> +#if defined(ENABLE_IP) && defined(LIBVIRTD)
> { "listen", no_argument, , 'l'},
> -#endif /* ! ENABLE_IP */
> +#endif /* ENABLE_IP && LIBVIRTD */
> { "config", required_argument, NULL, 'f'},
> { "timeout", required_argument, NULL, 't'},
> { "pid-file", required_argument, NULL, 'p'},
> @@ -1021,11 +1031,11 @@ int main(int argc, char **argv) {
> int optidx = 0;
> int c;
> char *tmp;
> -#ifdef ENABLE_IP
> +#if defined(ENABLE_IP) && defined(LIBVIRTD)
> const char *optstr = "ldf:p:t:vVh";
> -#else /* ! ENABLE_IP */
> +#else /* ! ENABLE_IP && ! LIBVIRTD */
> const char *optstr = "df:p:t:vVh";
> -#endif /* ! ENABLE_IP */
> +#endif /* ! ENABLE_IP && ! LIBVIRTD */
>
> c = getopt_long(argc, argv, optstr, opts, );
>
> @@ -1043,7 +1053,7 @@ int main(int argc, char **argv) {
> godaemon = 1;
> break;
>
> -#ifdef ENABLE_IP
> +#if defined(ENABLE_IP) && defined(LIBVIRTD)
> case 'l':
> ipsock = 1;
> break;
> diff --git a/src/remote/remote_daemon_config.c
> b/src/remote/remote_daemon_config.c
> index 3c5ccd5ba8..f583442dc7 100644
> --- a/src/remote/remote_daemon_config.c
> +++ b/src/remote/remote_daemon_config.c
> @@ -108,7 +108,11 @@ daemonConfigNew(bool privileged ATTRIBUTE_UNUSED)
> return NULL;
>
> #ifdef ENABLE_IP
> -data->listen_tls = 1;
> +# ifdef LIBVIRTD
> +data->listen_tls = 1; /* Only honoured it --listen is set */
> +# else /* ! LIBVIRTD */
> +data->listen_tls = 0; /* Always honoured, --listen doesn't exist. */
> +# endif /* ! LIBVIRTD */
> data->listen_tcp = 0;
>
> if (VIR_STRDUP(data->tls_port, LIBVIRTD_TLS_PORT) < 0 ||
> diff --git a/src/remote/virtproxyd.service.in
> b/src/remote/virtproxyd.service.in
> new file mode 100644
> index 00..e99e2af19c
> --- /dev/null
> +++ b/src/remote/virtproxyd.service.in
> @@ -0,0 +1,24 @@
> +[Unit]
> +Description=Virtualization daemon
> +Conflicts=libvirtd.service
> +Requires=virtproxyd.socket
> +Requires=virtproxyd-ro.socket
> +Requires=virtproxyd-admin.socket
> +After=network.target
> +After=dbus.service
> +After=apparmor.service
> +After=local-fs.target
> +Documentation=man:libvirtd(8)
> +Documentation=https://libvirt.org
> +
> +[Service]
> +Type=notify
> +ExecStart=@sbindir@/virtproxyd --timeout 120
> +ExecReload=/bin/kill -HUP $MAINPID
> +Restart=on-failure
> +
> +[Install]
> +WantedBy=multi-user.target
> +Also=virtproxyd.socket
> +Also=virtproxyd-ro.socket
> +Also=virtproxyd-admin.socket
> --
> 2.21.0
Reviewed-by: Christophe de Dinechin
--
Cheers,
Christophe de Dinechin (IRC c3d)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 19/48] remote: in per-driver daemons ensure that state initialize succeeds
remote/remote_driver.c
> @@ -195,7 +195,7 @@ remoteStateInitialize(bool privileged ATTRIBUTE_UNUSED,
> * re-entering ourselves
> */
> inside_daemon = true;
> -return 0;
> +return VIR_DRV_STATE_INIT_COMPLETE;
> }
>
>
> diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
> index 0af2bcef96..0d5ea05f56 100644
> --- a/src/secret/secret_driver.c
> +++ b/src/secret/secret_driver.c
> @@ -457,12 +457,12 @@ secretStateInitialize(bool privileged,
>void *opaque ATTRIBUTE_UNUSED)
> {
> if (VIR_ALLOC(driver) < 0)
> -return -1;
> +return VIR_DRV_STATE_INIT_ERROR;
>
> driver->lockFD = -1;
> if (virMutexInit(>lock) < 0) {
> VIR_FREE(driver);
> -return -1;
> +return VIR_DRV_STATE_INIT_ERROR;
> }
> secretDriverLock();
>
> @@ -514,12 +514,12 @@ secretStateInitialize(bool privileged,
> goto error;
>
> secretDriverUnlock();
> -return 0;
> +return VIR_DRV_STATE_INIT_COMPLETE;
>
> error:
> secretDriverUnlock();
> secretStateCleanup();
> -return -1;
> +return VIR_DRV_STATE_INIT_ERROR;
> }
>
>
> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index 03ac6a6845..dfa654178b 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -255,12 +255,12 @@ storageStateInitialize(bool privileged,
> VIR_AUTOFREE(char *) rundir = NULL;
>
> if (VIR_ALLOC(driver) < 0)
> -return -1;
> + return VIR_DRV_STATE_INIT_ERROR;
>
> driver->lockFD = -1;
> if (virMutexInit(>lock) < 0) {
> VIR_FREE(driver);
> -return -1;
> +return VIR_DRV_STATE_INIT_ERROR;
> }
> storageDriverLock();
>
> @@ -326,12 +326,12 @@ storageStateInitialize(bool privileged,
>
> storageDriverUnlock();
>
> -return 0;
> +return VIR_DRV_STATE_INIT_COMPLETE;
>
> error:
> storageDriverUnlock();
> storageStateCleanup();
> -return -1;
> +return VIR_DRV_STATE_INIT_ERROR;
> }
>
> /**
> diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
> index f5d05a7f43..da72b209d1 100644
> --- a/src/vz/vz_driver.c
> +++ b/src/vz/vz_driver.c
> @@ -4118,36 +4118,36 @@ vzStateInitialize(bool privileged,
>void *opaque ATTRIBUTE_UNUSED)
> {
> if (!privileged)
> -return 0;
> +return VIR_DRV_STATE_INIT_SKIPPED;
>
> vz_driver_privileged = privileged;
>
> if (virFileMakePathWithMode(VZ_STATEDIR, S_IRWXU) < 0) {
> virReportSystemError(errno, _("cannot create state directory '%s'"),
> VZ_STATEDIR);
> -return -1;
> +return VIR_DRV_STATE_INIT_ERROR;
> }
>
> if ((vz_driver_lock_fd =
> virPidFileAcquire(VZ_STATEDIR, "driver", false, getpid())) < 0)
> -return -1;
> +return VIR_DRV_STATE_INIT_ERROR;
>
> if (prlsdkInit() < 0) {
> VIR_DEBUG("%s", _("Can't initialize Parallels SDK"));
> -return -1;
> +return VIR_DRV_STATE_INIT_ERROR;
> }
>
> - if (virMutexInit(_driver_lock) < 0)
> +if (virMutexInit(_driver_lock) < 0)
> goto error;
>
> /* Failing to create driver here is not fatal and only means
> * that next driver client will try once more when connecting */
> vz_driver = vzDriverObjNew();
> -return 0;
> +return VIR_DRV_STATE_INIT_COMPLETE;
>
> error:
> vzStateCleanup();
> -return -1;
> +return VIR_DRV_STATE_INIT_ERROR;
> }
>
> static virStateDriver vzStateDriver = {
> --
> 2.21.0
Reviewed-by: Christophe de Dinechin
--
Cheers,
Christophe de Dinechin (IRC c3d)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 16/48] remote: reduce duplication in systemd unit file make rules into one
Reviewed-by: Christophe de Dinechin Daniel P. Berrangé writes: > The make rules for the systemd socket unit files are all essentially > identical and can be collapsed into a single generic rule. The service > unit file rule can be simplified too. > > Reviewed-by: Andrea Bolognani > Signed-off-by: Daniel P. Berrangé > --- > src/Makefile.am| 6 + > src/remote/Makefile.inc.am | 49 +++--- > 2 files changed, 14 insertions(+), 41 deletions(-) > > diff --git a/src/Makefile.am b/src/Makefile.am > index 255e8e2b57..b4544b12a7 100644 > --- a/src/Makefile.am > +++ b/src/Makefile.am > @@ -89,6 +89,12 @@ man8_MANS = > DRIVER_SOURCES = > man7_MANS = > > +COMMON_UNIT_VARS = \ > + -e 's|[@]localstatedir[@]|$(localstatedir)|g' \ > + -e 's|[@]sbindir[@]|$(sbindir)|g' \ > + -e 's|[@]sysconfdir[@]|$(sysconfdir)|g' \ > + $(NULL) > + > include util/Makefile.inc.am > include conf/Makefile.inc.am > include cpu/Makefile.inc.am > diff --git a/src/remote/Makefile.inc.am b/src/remote/Makefile.inc.am > index fc04af5cb5..0c6d859a41 100644 > --- a/src/remote/Makefile.inc.am > +++ b/src/remote/Makefile.inc.am > @@ -292,48 +292,15 @@ INSTALL_DATA_LOCAL += install-sasl > UNINSTALL_LOCAL += uninstall-sasl > endif WITH_SASL > > +LIBVIRTD_UNIT_VARS = \ > + $(COMMON_UNIT_VARS) \ > + $(NULL) > + > libvirtd.service: remote/libvirtd.service.in $(top_builddir)/config.status > - $(AM_V_GEN)sed \ > - -e 's|[@]localstatedir[@]|$(localstatedir)|g' \ > - -e 's|[@]sbindir[@]|$(sbindir)|g' \ > - -e 's|[@]sysconfdir[@]|$(sysconfdir)|g' \ > - < $< > $@-t && \ > - mv $@-t $@ > - > -libvirtd.socket: remote/libvirtd.socket.in $(top_builddir)/config.status > - $(AM_V_GEN)sed \ > - -e 's|[@]localstatedir[@]|$(localstatedir)|g' \ > - -e 's|[@]sysconfdir[@]|$(sysconfdir)|g' \ > - < $< > $@-t && \ > - mv $@-t $@ > - > -libvirtd-ro.socket: remote/libvirtd-ro.socket.in > $(top_builddir)/config.status > - $(AM_V_GEN)sed \ > - -e 's|[@]localstatedir[@]|$(localstatedir)|g' \ > - -e 's|[@]sysconfdir[@]|$(sysconfdir)|g' \ > - < $< > $@-t && \ > - mv $@-t $@ > - > -libvirtd-admin.socket: remote/libvirtd-admin.socket.in > $(top_builddir)/config.status > - $(AM_V_GEN)sed \ > - -e 's|[@]localstatedir[@]|$(localstatedir)|g' \ > - -e 's|[@]sysconfdir[@]|$(sysconfdir)|g' \ > - < $< > $@-t && \ > - mv $@-t $@ > - > -libvirtd-tcp.socket: remote/libvirtd-tcp.socket.in > $(top_builddir)/config.status > - $(AM_V_GEN)sed \ > - -e 's|[@]localstatedir[@]|$(localstatedir)|g' \ > - -e 's|[@]sysconfdir[@]|$(sysconfdir)|g' \ > - < $< > $@-t && \ > - mv $@-t $@ > - > -libvirtd-tls.socket: remote/libvirtd-tls.socket.in > $(top_builddir)/config.status > - $(AM_V_GEN)sed \ > - -e 's|[@]localstatedir[@]|$(localstatedir)|g' \ > - -e 's|[@]sysconfdir[@]|$(sysconfdir)|g' \ > - < $< > $@-t && \ > - mv $@-t $@ > + $(AM_V_GEN)sed $(LIBVIRTD_UNIT_VARS) < $< > $@-t && mv $@-t $@ > + > +libvirt%.socket: remote/libvirt%.socket.in $(top_builddir)/config.status > + $(AM_V_GEN)sed $(LIBVIRTD_UNIT_VARS) < $< > $@-t && mv $@-t $@ > > virt-guest-shutdown.target: remote/virt-guest-shutdown.target.in \ > $(top_builddir)/config.status > -- > 2.21.0 -- Cheers, Christophe de Dinechin (IRC c3d) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 15/48] build: don't hardcode /etc in the config related files
LGTM Reviewed-by: Christophe de Dinechin Daniel P. Berrangé writes: > Substitute in the @sysconfigdir@ value instead of /etc. > > Signed-off-by: Daniel P. Berrangé > --- > src/locking/Makefile.inc.am | 1 + > src/locking/virtlockd.service.in| 2 +- > src/logging/Makefile.inc.am | 1 + > src/logging/virtlogd.service.in | 2 +- > src/remote/Makefile.inc.am | 8 > src/remote/libvirtd-admin.socket.in | 2 +- > src/remote/libvirtd-ro.socket.in| 2 +- > src/remote/libvirtd-tcp.socket.in | 2 +- > src/remote/libvirtd-tls.socket.in | 2 +- > src/remote/libvirtd.aug.in | 4 ++-- > src/remote/libvirtd.conf.in | 18 +- > src/remote/libvirtd.service.in | 2 +- > src/remote/libvirtd.socket.in | 2 +- > src/remote/test_libvirtd.aug.in | 8 > tools/libvirt-guests.service.in | 2 +- > 15 files changed, 34 insertions(+), 24 deletions(-) > > diff --git a/src/locking/Makefile.inc.am b/src/locking/Makefile.inc.am > index 161410051c..bc1a05549a 100644 > --- a/src/locking/Makefile.inc.am > +++ b/src/locking/Makefile.inc.am > @@ -258,6 +258,7 @@ locking/lock_daemon_dispatch_stubs.h: $(LOCK_PROTOCOL) \ > virtlockd.service: locking/virtlockd.service.in $(top_builddir)/config.status > $(AM_V_GEN)sed \ > -e 's|[@]sbindir[@]|$(sbindir)|g' \ > + -e 's|[@]sysconfdir[@]|$(sysconfdir)|g' \ > < $< > $@-t && \ > mv $@-t $@ > > diff --git a/src/locking/virtlockd.service.in > b/src/locking/virtlockd.service.in > index 3c9d587032..e7f8057c06 100644 > --- a/src/locking/virtlockd.service.in > +++ b/src/locking/virtlockd.service.in > @@ -7,7 +7,7 @@ Documentation=man:virtlockd(8) > Documentation=https://libvirt.org > > [Service] > -EnvironmentFile=-/etc/sysconfig/virtlockd > +EnvironmentFile=-@sysconfdir@/sysconfig/virtlockd > ExecStart=@sbindir@/virtlockd $VIRTLOCKD_ARGS > ExecReload=/bin/kill -USR1 $MAINPID > # Loosing the locks is a really bad thing that will > diff --git a/src/logging/Makefile.inc.am b/src/logging/Makefile.inc.am > index f0c49330f5..18772fde2f 100644 > --- a/src/logging/Makefile.inc.am > +++ b/src/logging/Makefile.inc.am > @@ -122,6 +122,7 @@ virtlogd.8.in: logging/virtlogd.pod > virtlogd.service: logging/virtlogd.service.in $(top_builddir)/config.status > $(AM_V_GEN)sed \ > -e 's|[@]sbindir[@]|$(sbindir)|g' \ > + -e 's|[@]sysconfdir[@]|$(sysconfdir)|g' \ > < $< > $@-t && \ > mv $@-t $@ > > diff --git a/src/logging/virtlogd.service.in b/src/logging/virtlogd.service.in > index 3d9ae36150..daff48e67d 100644 > --- a/src/logging/virtlogd.service.in > +++ b/src/logging/virtlogd.service.in > @@ -7,7 +7,7 @@ Documentation=man:virtlogd(8) > Documentation=https://libvirt.org > > [Service] > -EnvironmentFile=-/etc/sysconfig/virtlogd > +EnvironmentFile=-@sysconfdir@/sysconfig/virtlogd > ExecStart=@sbindir@/virtlogd $VIRTLOGD_ARGS > ExecReload=/bin/kill -USR1 $MAINPID > # Loosing the logs is a really bad thing that will > diff --git a/src/remote/Makefile.inc.am b/src/remote/Makefile.inc.am > index 0ec7abb880..fc04af5cb5 100644 > --- a/src/remote/Makefile.inc.am > +++ b/src/remote/Makefile.inc.am > @@ -194,6 +194,7 @@ remote/libvirtd.conf: remote/libvirtd.conf.in > $(AM_V_GEN)$(SED) \ > -e '/[@]CUT_ENABLE_IP[@]/d' \ > -e '/[@]END[@]/d' \ > + -e 's|[@]sysconfdir[@]|@sysconfdir@|' \ > -e 's|[@]DAEMON_NAME[@]|libvirtd|' \ > < $< > $@ > > @@ -209,6 +210,7 @@ remote/libvirtd.aug: remote/libvirtd.aug.in > $(AM_V_GEN)$(SED) \ > -e '/[@]CUT_ENABLE_IP[@]/d' \ > -e '/[@]END[@]/d' \ > + -e 's|[@]sysconfdir[@]|@sysconfdir@|' \ > -e 's|[@]DAEMON_NAME[@]|libvirtd|' \ > -e 's|[@]DAEMON_NAME_UC[@]|Libvirtd|' \ > $< > $@ > @@ -219,6 +221,7 @@ remote/test_libvirtd.aug: remote/test_libvirtd.aug.in \ > $(srcdir)/remote/test_libvirtd.aug.in | \ > $(SED) -e '/[@]CUT_ENABLE_IP[@]/d' \ > -e '/[@]END[@]/d' \ > + -e 's|[@]sysconfdir[@]|@sysconfdir@|' \ > -e 's|[@]DAEMON_NAME[@]|libvirtd|' \ > -e 's|[@]DAEMON_NAME_UC[@]|Libvirtd|' \ > > $@ || rm -f $@ > @@ -300,30 +303,35 @@ libvirtd.service: remote/libvirtd.service.in > $(top_builddir)/config.status > libvirtd.socket: remote/libvirtd.socket.in $(top_builddir)/config.status > $(AM_V_GEN)sed \ > -e 's|[@]localstatedir[@]|$(localstatedir)|g' \ > +
Re: [libvirt] [PATCH v3 12/48] remote: conditionalize IP socket config in libvirtd.conf
#
> #
> # UNIX socket access controls
> @@ -157,6 +159,7 @@
> # If the unix_sock_rw_perms are changed you may wish to enable
> # an authentication mechanism here
> #auth_unix_rw = "none"
> +@CUT_ENABLE_IP@
>
> # Change the authentication scheme for TCP sockets.
> #
> @@ -174,6 +177,7 @@
> # It is possible to make use of any SASL authentication
> # mechanism as well, by using 'sasl' for this option
> #auth_tls = "none"
> +@END@
>
>
> # Change the API access control scheme
> @@ -182,10 +186,11 @@
> # to all APIs. Access drivers can place restrictions
> # on this. By default the 'nop' driver is enabled,
> # meaning no access control checks are done once a
> -# client has authenticated with libvirtd
> +# client has authenticated with @DAEMON_NAME@
> #
> #access_drivers = [ "polkit" ]
>
> +@CUT_ENABLE_IP@
> #
> #
> # TLS x509 certificate configuration
> @@ -225,15 +230,17 @@
>
>
>
> +@END@
> #
> #
> # Authorization controls
> #
>
>
> +@CUT_ENABLE_IP@
> # Flag to disable verification of our own server certificates
> #
> -# When libvirtd starts it performs some sanity checks against
> +# When @DAEMON_NAME@ starts it performs some sanity checks against
> # its own certificates.
> #
> # Default is to always run sanity checks. Uncommenting this
> @@ -265,6 +272,15 @@
> #tls_allowed_dn_list = ["DN1", "DN2"]
>
>
> +# Override the compile time default TLS priority string. The
> +# default is usually "NORMAL" unless overridden at build time.
> +# Only set this is it is desired for libvirt to deviate from
> +# the global default settings.
> +#
> +#tls_priority="NORMAL"
> +
> +
> +@END@
> # A whitelist of allowed SASL usernames. The format for username
> # depends on the SASL authentication mechanism. Kerberos usernames
> # look like username@REALM
> @@ -282,14 +298,6 @@
> #sasl_allowed_username_list = ["j...@example.com", "f...@example.com" ]
>
>
> -# Override the compile time default TLS priority string. The
> -# default is usually "NORMAL" unless overridden at build time.
> -# Only set this is it is desired for libvirt to deviate from
> -# the global default settings.
> -#
> -#tls_priority="NORMAL"
> -
> -
> #
> #
> # Processing controls
> @@ -417,8 +425,8 @@
> #4: ERROR
> #
> # Multiple outputs can be defined, they just need to be separated by spaces.
> -# e.g. to log all warnings and errors to syslog under the libvirtd ident:
> -#log_outputs="3:syslog:libvirtd"
> +# e.g. to log all warnings and errors to syslog under the @DAEMON_NAME@
> ident:
> +#log_outputs="3:syslog:@DAEMON_NAME@"
>
>
> ##
> @@ -461,7 +469,7 @@
>
> ###
> # Keepalive protocol:
> -# This allows libvirtd to detect broken client connections or even
> +# This allows @DAEMON_NAME@ to detect broken client connections or even
> # dead clients. A keepalive message is sent to a client after
> # keepalive_interval seconds of inactivity to check if the client is
> # still responding; keepalive_count is a maximum number of keepalive
> @@ -470,7 +478,7 @@
> # words, the connection is automatically closed approximately after
> # keepalive_interval * (keepalive_count + 1) seconds since the last
> # message received from the client. If keepalive_interval is set to
> -# -1, libvirtd will never send keepalive requests; however clients
> +# -1, @DAEMON_NAME@ will never send keepalive requests; however clients
> # can still send them and the daemon will send responses. When
> # keepalive_count is set to 0, connections will be automatically
> # closed after keepalive_interval seconds of inactivity without
> diff --git a/src/remote/test_libvirtd.aug.in b/src/remote/test_libvirtd.aug.in
> index 6c51b7b9e7..d768b30b55 100644
> --- a/src/remote/test_libvirtd.aug.in
> +++ b/src/remote/test_libvirtd.aug.in
> @@ -29,11 +29,11 @@ module Test_libvirtd =
> { "1" = "DN1"}
> { "2" = "DN2"}
> }
> +{ "tls_priority" = "NORMAL" }
I'm curious about this change? Is that because you changed the order
in the source code? Does that depend on ENABLE_IP?
> { "sasl_allowed_username_list"
> { "1" = "j...@example.com" }
> { "2" = "f...@example.com" }
> }
> -{ "tls_priority" = "NORMAL" }
> { "max_clients" = "5000" }
> { "max_queued_clients" = "1000" }
> { "max_anonymous_clients" = "20" }
> --
> 2.21.0
Reviewed-by: Christophe de Dinechin
--
Cheers,
Christophe de Dinechin (IRC c3d)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 10/48] remote: conditionalize IP socket usage in libvirtd daemon
IR_FREE(data->unix_sock_dir);
>
> -tmp = data->tls_allowed_dn_list;
> +tmp = data->sasl_allowed_username_list;
> while (tmp && *tmp) {
> VIR_FREE(*tmp);
> tmp++;
> }
> -VIR_FREE(data->tls_allowed_dn_list);
> +VIR_FREE(data->sasl_allowed_username_list);
>
> -tmp = data->sasl_allowed_username_list;
> +#ifdef ENABLE_IP
> +tmp = data->tls_allowed_dn_list;
> while (tmp && *tmp) {
> VIR_FREE(*tmp);
> tmp++;
> }
> -VIR_FREE(data->sasl_allowed_username_list);
> +VIR_FREE(data->tls_allowed_dn_list);
> +
> VIR_FREE(data->tls_priority);
>
> VIR_FREE(data->key_file);
> VIR_FREE(data->ca_file);
> VIR_FREE(data->cert_file);
> VIR_FREE(data->crl_file);
> +#endif /* ! ENABLE_IP */
>
> VIR_FREE(data->host_uuid);
> VIR_FREE(data->host_uuid_source);
> @@ -231,6 +241,7 @@ daemonConfigLoadOptions(struct daemonConfig *data,
> const char *filename,
> virConfPtr conf)
> {
> +#ifdef ENABLE_IP
> if (virConfGetValueBool(conf, "listen_tcp", >listen_tcp) < 0)
> goto error;
> if (virConfGetValueBool(conf, "listen_tls", >listen_tls) < 0)
> @@ -241,6 +252,7 @@ daemonConfigLoadOptions(struct daemonConfig *data,
> goto error;
> if (virConfGetValueString(conf, "listen_addr", >listen_addr) < 0)
> goto error;
> +#endif /* !ENABLE_IP */
>
> if (remoteConfigGetAuth(conf, filename, "auth_unix_rw",
> >auth_unix_rw) < 0)
> goto error;
> @@ -256,10 +268,13 @@ daemonConfigLoadOptions(struct daemonConfig *data,
> #endif
> if (remoteConfigGetAuth(conf, filename, "auth_unix_ro",
> >auth_unix_ro) < 0)
> goto error;
> +
> +#ifdef ENABLE_IP
> if (remoteConfigGetAuth(conf, filename, "auth_tcp", >auth_tcp) < 0)
> goto error;
> if (remoteConfigGetAuth(conf, filename, "auth_tls", >auth_tls) < 0)
> goto error;
> +#endif /* ! ENABLE_IP */
>
> if (virConfGetValueStringList(conf, "access_drivers", false,
>>access_drivers) < 0)
> @@ -277,6 +292,7 @@ daemonConfigLoadOptions(struct daemonConfig *data,
> if (virConfGetValueString(conf, "unix_sock_dir", >unix_sock_dir) <
> 0)
> goto error;
>
> +#ifdef ENABLE_IP
> if (virConfGetValueBool(conf, "tls_no_sanity_certificate",
> >tls_no_sanity_certificate) < 0)
> goto error;
> if (virConfGetValueBool(conf, "tls_no_verify_certificate",
> >tls_no_verify_certificate) < 0)
> @@ -295,14 +311,14 @@ daemonConfigLoadOptions(struct daemonConfig *data,
>>tls_allowed_dn_list) < 0)
> goto error;
>
> +if (virConfGetValueString(conf, "tls_priority", >tls_priority) < 0)
> + goto error;
> +#endif /* ! ENABLE_IP */
>
> if (virConfGetValueStringList(conf, "sasl_allowed_username_list", false,
>>sasl_allowed_username_list) < 0)
> goto error;
>
> -if (virConfGetValueString(conf, "tls_priority", >tls_priority) < 0)
> -goto error;
> -
> if (virConfGetValueUInt(conf, "min_workers", >min_workers) < 0)
> goto error;
> if (virConfGetValueUInt(conf, "max_workers", >max_workers) < 0)
> diff --git a/src/remote/remote_daemon_config.h
> b/src/remote/remote_daemon_config.h
> index d580e7d49c..5a54abed85 100644
> --- a/src/remote/remote_daemon_config.h
> +++ b/src/remote/remote_daemon_config.h
> @@ -27,11 +27,13 @@ struct daemonConfig {
> char *host_uuid;
> char *host_uuid_source;
>
> +#ifdef ENABLE_IP
> bool listen_tls;
> bool listen_tcp;
> char *listen_addr;
> char *tls_port;
> char *tcp_port;
> +#endif /* ! ENABLE_IP */
>
> char *unix_sock_admin_perms;
> char *unix_sock_ro_perms;
> @@ -41,21 +43,27 @@ struct daemonConfig {
>
> int auth_unix_rw;
> int auth_unix_ro;
> +
> +#ifdef ENABLE_IP
> int auth_tcp;
> int auth_tls;
> +#endif /* ! ENABLE_IP */
>
> char **access_drivers;
>
> +#ifdef ENABLE_IP
> bool tls_no_verify_certificate;
> bool tls_no_sanity_certificate;
> char **tls_allowed_dn_list;
> -char **sasl_allowed_username_list;
> char *tls_priority;
>
> char *key_file;
> char *cert_file;
> char *ca_file;
> char *crl_file;
> +#endif /* ! ENABLE_IP */
> +
> +char **sasl_allowed_username_list;
>
> unsigned int min_workers;
> unsigned int max_workers;
> --
> 2.21.0
Reviewed-by: Christophe de Dinechin
--
Cheers,
Christophe de Dinechin (IRC c3d)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 09/48] remote: conditionalize driver loading in libvirtd daemon
Do you want to use the "xen" name here too?
Daniel P. Berrangé writes:
> Prepare for reusing libvirtd source to create other daemons by making
> the driver(s) to load conditionally defined by the make rules.
>
> If nothing is set, all drivers will be loaded, ignoring any missing ones
> as historically done.
>
> If MODULE_NAME is set only one driver will be loaded and that one must
> succeed.
>
> Reviewed-by: Andrea Bolognani
> Signed-off-by: Daniel P. Berrangé
> ---
> src/remote/remote_daemon.c | 55 +-
> 1 file changed, 31 insertions(+), 24 deletions(-)
>
> diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
> index f1daaeb944..97621884b0 100644
> --- a/src/remote/remote_daemon.c
> +++ b/src/remote/remote_daemon.c
> @@ -311,60 +311,67 @@ static int daemonErrorLogFilter(virErrorPtr err, int
> priority)
>
> static int daemonInitialize(void)
> {
> -/*
> +#ifdef MODULE_NAME
> +/* This a dedicated per-driver daemon build */
> +if (virDriverLoadModule(MODULE_NAME, MODULE_NAME "Register", true) < 0)
> +return -1;
> +#else
> +/* This is the legacy monolithic libvirtd built with all drivers
> + *
> * Note that the order is important: the first ones have a higher
> * priority when calling virStateInitialize. We must register the
> * network, storage and nodedev drivers before any stateful domain
> * driver, since their resources must be auto-started before any
> * domains can be auto-started.
> */
> -#ifdef WITH_NETWORK
> +# ifdef WITH_NETWORK
> if (virDriverLoadModule("network", "networkRegister", false) < 0)
> return -1;
> -#endif
> -#ifdef WITH_INTERFACE
> +# endif
> +# ifdef WITH_INTERFACE
> if (virDriverLoadModule("interface", "interfaceRegister", false) < 0)
> return -1;
> -#endif
> -#ifdef WITH_SECRETS
> +# endif
> +# ifdef WITH_SECRETS
> if (virDriverLoadModule("secret", "secretRegister", false) < 0)
> return -1;
> -#endif
> -#ifdef WITH_STORAGE
> +# endif
> +# ifdef WITH_STORAGE
> if (virDriverLoadModule("storage", "storageRegister", false) < 0)
> return -1;
> -#endif
> -#ifdef WITH_NODE_DEVICES
> +# endif
> +# ifdef WITH_NODE_DEVICES
> if (virDriverLoadModule("nodedev", "nodedevRegister", false) < 0)
> return -1;
> -#endif
> -#ifdef WITH_NWFILTER
> +# endif
> +# ifdef WITH_NWFILTER
> if (virDriverLoadModule("nwfilter", "nwfilterRegister", false) < 0)
> return -1;
> -#endif
> -#ifdef WITH_LIBXL
> +# endif
> +# ifdef WITH_LIBXL
> if (virDriverLoadModule("libxl", "libxlRegister", false) < 0)
> return -1;
> -#endif
> -#ifdef WITH_QEMU
> +# endif
> +# ifdef WITH_QEMU
> if (virDriverLoadModule("qemu", "qemuRegister", false) < 0)
> return -1;
> -#endif
> -#ifdef WITH_LXC
> +# endif
> +# ifdef WITH_LXC
> if (virDriverLoadModule("lxc", "lxcRegister", false) < 0)
> return -1;
> -#endif
> -#ifdef WITH_VBOX
> +# endif
> +# ifdef WITH_VBOX
> if (virDriverLoadModule("vbox", "vboxRegister", false) < 0)
> return -1;
> -#endif
> -#ifdef WITH_BHYVE
> +# endif
> +# ifdef WITH_BHYVE
> if (virDriverLoadModule("bhyve", "bhyveRegister", false) < 0)
> return -1;
> -#endif
> -#ifdef WITH_VZ
> +# endif
> +# ifdef WITH_VZ
> if (virDriverLoadModule("vz", "vzRegister", false) < 0)
> return -1;
> +# endif
> #endif
> return 0;
> }
> --
> 2.21.0
Reviewed-by: Christophe de Dinechin
--
Cheers,
Christophe de Dinechin (IRC c3d)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 08/48] remote: conditionalize daemon name in libvirtd daemon
iceTCP(srv,
>act,
> - "libvirtd-tls.socket",
> + DAEMON_NAME "-tls.socket",
>config->listen_addr,
>config->tls_port,
>AF_UNSPEC,
> @@ -564,7 +568,7 @@ daemonSetupNetDevOpenvswitch(struct daemonConfig *config)
>
> /*
> * Set up the logging environment
> - * By default if daemonized all errors go to the logfile libvirtd.log,
> + * By default if daemonized all errors go to journald/a logfile
> * but if verbose or error debugging is asked for then also output
> * informational and debug messages. Default size if 64 kB.
> */
> @@ -577,7 +581,7 @@ daemonSetupLogging(struct daemonConfig *config,
> virLogReset();
>
> /*
> - * Libvirtd's order of precedence is:
> + * Logging setup order of precedence is:
> * cmdline > environment > config
> *
> * Given the precedence, we must process the variables in the opposite
> @@ -605,7 +609,7 @@ daemonSetupLogging(struct daemonConfig *config,
> /* Define the default output. This is only applied if there was no
> setting
> * from either the config or the environment.
> */
> -if (virLogSetDefaultOutput("libvirtd", godaemon, privileged) < 0)
> +if (virLogSetDefaultOutput(DAEMON_NAME, godaemon, privileged) < 0)
> return -1;
>
> if (virLogGetNbOutputs() == 0)
> @@ -717,7 +721,7 @@ static void daemonStopWorker(void *opaque)
>
> VIR_DEBUG("Completed stop dmn=%p", dmn);
>
> -/* Exit libvirtd cleanly */
> +/* Exit daemon cleanly */
> virNetDaemonQuit(dmn);
> }
>
> @@ -796,7 +800,7 @@ static void daemonRunStateInit(void *opaque)
> driversInitialized = true;
>
> #ifdef WITH_DBUS
> -/* Tie the non-privileged libvirtd to the session/shutdown lifecycle */
> +/* Tie the non-privileged daemons to the session/shutdown lifecycle */
> if (!virNetDaemonIsPrivileged(dmn)) {
>
> sessionBus = virDBusGetSessionBus();
> @@ -905,8 +909,8 @@ daemonUsage(const char *argv0, bool privileged)
> fprintf(stderr, "\n");
>
> fprintf(stderr, "%s:\n", _("Configuration file (unless overridden by
> -f)"));
> -fprintf(stderr, " %s/libvirt/libvirtd.conf\n",
> -privileged ? SYSCONFDIR : "$XDG_CONFIG_HOME");
> +fprintf(stderr, " %s/libvirt/%s.conf\n",
> +privileged ? SYSCONFDIR : "$XDG_CONFIG_HOME", DAEMON_NAME);
> fprintf(stderr, "\n");
>
> fprintf(stderr, "%s:\n", _("Sockets"));
> @@ -932,9 +936,9 @@ daemonUsage(const char *argv0, bool privileged)
>
> fprintf(stderr, "%s:\n",
> _("PID file (unless overridden by -p)"));
> -fprintf(stderr, " %s\n",
> -privileged ? LOCALSTATEDIR "/run/libvirtd.pid":
> -"$XDG_RUNTIME_DIR/libvirt/libvirtd.pid");
> +fprintf(stderr, " %s/%s.pid\n",
> +privileged ? LOCALSTATEDIR "/run" : "$XDG_RUNTIME_DIR/libvirt",
> +DAEMON_NAME);
> fprintf(stderr, "\n");
> }
>
> @@ -1098,7 +1102,7 @@ int main(int argc, char **argv) {
> if (!pid_file &&
> virPidFileConstructPath(privileged,
> LOCALSTATEDIR,
> -"libvirtd",
> +DAEMON_NAME,
> _file) < 0) {
> VIR_ERROR(_("Can't determine pid file path."));
> exit(EXIT_FAILURE);
> @@ -1178,7 +1182,7 @@ int main(int argc, char **argv) {
> goto cleanup;
> }
>
> -if (!(srv = virNetServerNew("libvirtd", 1,
> +if (!(srv = virNetServerNew(DAEMON_NAME, 1,
> config->min_workers,
> config->max_workers,
> config->prio_workers,
> diff --git a/src/remote/remote_daemon_config.c
> b/src/remote/remote_daemon_config.c
> index 537b90a855..3e62b4203f 100644
> --- a/src/remote/remote_daemon_config.c
> +++ b/src/remote/remote_daemon_config.c
> @@ -77,7 +77,8 @@ int
> daemonConfigFilePath(bool privileged, char **configfile)
> {
> if (privileged) {
> -if (VIR_STRDUP(*configfile, SYSCONFDIR "/libvirt/libvirtd.conf") < 0)
Maybe cleanup configfile on the error path? Not presently a bug,
but might become a leak if additional error exits are added later.
> +if (VIR_STRDUP(*configfile,
> + SYSCONFDIR "/libvirt/" DAEMON_NAME ".conf") < 0)
> goto error;
> } else {
> char *configdir = NULL;
> @@ -85,7 +86,7 @@ daemonConfigFilePath(bool privileged, char **configfile)
> if (!(configdir = virGetUserConfigDirectory()))
> goto error;
>
> -if (virAsprintf(configfile, "%s/libvirtd.conf", configdir) < 0) {
> +if (virAsprintf(configfile, "%s/%s.conf", configdir, DAEMON_NAME) <
> 0) {
> VIR_FREE(configdir);
> goto error;
> }
> --
> 2.21.0
Reviewed-by: Christophe de Dinechin
--
Cheers,
Christophe de Dinechin (IRC c3d)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 07/48] remote: conditionalize socket names in libvirtd daemon
(Sorry if this is a resend, my earlier email fired up too quickly)
Daniel P. Berrangé writes:
> Prepare for reusing libvirtd source to create other daemons by making
> the socket names conditionally defined by the make rules.
>
> Reviewed-by: Andrea Bolognani
> Signed-off-by: Daniel P. Berrangé
> ---
> src/remote/Makefile.inc.am | 1 +
> src/remote/remote_daemon.c | 34 +-
> 2 files changed, 22 insertions(+), 13 deletions(-)
>
> diff --git a/src/remote/Makefile.inc.am b/src/remote/Makefile.inc.am
> index 0400dabad9..ced940d3c1 100644
> --- a/src/remote/Makefile.inc.am
> +++ b/src/remote/Makefile.inc.am
> @@ -146,6 +146,7 @@ libvirtd_CFLAGS = \
> -I$(srcdir)/access \
> -I$(srcdir)/conf \
> -I$(srcdir)/rpc \
> + -DSOCK_PREFIX="\"libvirt\"" \
> $(NULL)
>
> libvirtd_LDFLAGS = \
> diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
> index 69385af1c4..f9d923b357 100644
> --- a/src/remote/remote_daemon.c
> +++ b/src/remote/remote_daemon.c
> @@ -221,19 +221,25 @@ daemonUnixSocketPaths(struct daemonConfig *config,
> char *rundir = NULL;
>
> if (config->unix_sock_dir) {
> -if (virAsprintf(sockfile, "%s/libvirt-sock", config->unix_sock_dir)
> < 0)
> +if (virAsprintf(sockfile, "%s/%s-sock",
> +SOCK_PREFIX, config->unix_sock_dir) < 0)
> goto cleanup;
>
> if (privileged) {
> -if (virAsprintf(rosockfile, "%s/libvirt-sock-ro",
> config->unix_sock_dir) < 0 ||
> -virAsprintf(admsockfile, "%s/libvirt-admin-sock",
> config->unix_sock_dir) < 0)
> +if (virAsprintf(rosockfile, "%s/%s-sock-ro",
> +SOCK_PREFIX, config->unix_sock_dir) < 0 ||
> +virAsprintf(admsockfile, "%s/%s-admin-sock",
> +SOCK_PREFIX, config->unix_sock_dir) < 0)
> goto cleanup;
> }
> } else {
> if (privileged) {
> -if (VIR_STRDUP(*sockfile, LOCALSTATEDIR
> "/run/libvirt/libvirt-sock") < 0 ||
> -VIR_STRDUP(*rosockfile, LOCALSTATEDIR
> "/run/libvirt/libvirt-sock-ro") < 0 ||
> -VIR_STRDUP(*admsockfile, LOCALSTATEDIR
> "/run/libvirt/libvirt-admin-sock") < 0)
> +if (virAsprintf(sockfile, "%s/run/libvirt/%s-sock",
> +LOCALSTATEDIR, SOCK_PREFIX) < 0 ||
> +virAsprintf(sockfile, "%s/run/libvirt/%s-sock-ro",
> +LOCALSTATEDIR, SOCK_PREFIX) < 0 ||
> +virAsprintf(sockfile, "%s/run/libvirt/%s-admin-sock",
> +LOCALSTATEDIR, SOCK_PREFIX) < 0)
Copy-paste error on sockfile variable name, use rosockfile and admsockfile.
Also, there is a memory leak if second or third fails, since the first
one is never deallocated.
Consider adding a VIR_FREE for *sockfile, *rosockfile and *admsockfile
in the cleanup section. Also, to make it real safe, consider adding a
NULL-initialization for *sockfile, *rosockfile an d *admsockfile at the
top of the function.
> goto cleanup;
> } else {
> mode_t old_umask;
> @@ -248,8 +254,10 @@ daemonUnixSocketPaths(struct daemonConfig *config,
> }
> umask(old_umask);
>
> -if (virAsprintf(sockfile, "%s/libvirt-sock", rundir) < 0 ||
> -virAsprintf(admsockfile, "%s/libvirt-admin-sock", rundir) <
> 0)
> +if (virAsprintf(sockfile, "%s/%s-sock",
> +rundir, SOCK_PREFIX) < 0 ||
> +virAsprintf(admsockfile, "%s/%s-admin-sock",
> +rundir, SOCK_PREFIX) < 0)
> goto cleanup;
> }
> }
> @@ -902,12 +910,12 @@ daemonUsage(const char *argv0, bool privileged)
> fprintf(stderr, "\n");
>
> fprintf(stderr, "%s:\n", _("Sockets"));
Localization of :
> -fprintf(stderr, " %s\n",
> -privileged ? LOCALSTATEDIR "/run/libvirt/libvirt-sock" :
> -"$XDG_RUNTIME_DIR/libvirt/libvirt-sock");
> + fprintf(stderr, " %s/libvirt/%s-sock\n",
> +privileged ? LOCALSTATEDIR "/run" : "$XDG_RUNTIME_DIR",
> +SOCK_PREFIX);
> if (privileged)
> -fprintf(stderr, " %s\n",
> -LOCALSTATEDIR "/run/libvirt/libvirt-sock-ro");
> +fprintf(stderr, " %s/run/libvirt/%s-sock-ro\n",
> +LOCALSTATEDIR, SOCK_PREFIX);
> fprintf(stderr, "\n");
>
> fprintf(stderr, "%s:\n", _("TLS"));
Localization of :
> --
> 2.21.0
Reviewed-by: Christophe de Dinechin
--
Cheers,
Christophe de Dinechin (IRC c3d)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 07/48] remote: conditionalize socket names in libvirtd daemon
Daniel P. Berrangé writes:
> Prepare for reusing libvirtd source to create other daemons by making
> the socket names conditionally defined by the make rules.
>
> Reviewed-by: Andrea Bolognani
> Signed-off-by: Daniel P. Berrangé
> ---
> src/remote/Makefile.inc.am | 1 +
> src/remote/remote_daemon.c | 34 +-
> 2 files changed, 22 insertions(+), 13 deletions(-)
>
> diff --git a/src/remote/Makefile.inc.am b/src/remote/Makefile.inc.am
> index 0400dabad9..ced940d3c1 100644
> --- a/src/remote/Makefile.inc.am
> +++ b/src/remote/Makefile.inc.am
> @@ -146,6 +146,7 @@ libvirtd_CFLAGS = \
> -I$(srcdir)/access \
> -I$(srcdir)/conf \
> -I$(srcdir)/rpc \
> + -DSOCK_PREFIX="\"libvirt\"" \
> $(NULL)
>
> libvirtd_LDFLAGS = \
> diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
> index 69385af1c4..f9d923b357 100644
> --- a/src/remote/remote_daemon.c
> +++ b/src/remote/remote_daemon.c
> @@ -221,19 +221,25 @@ daemonUnixSocketPaths(struct daemonConfig *config,
> char *rundir = NULL;
>
> if (config->unix_sock_dir) {
> -if (virAsprintf(sockfile, "%s/libvirt-sock", config->unix_sock_dir)
> < 0)
> +if (virAsprintf(sockfile, "%s/%s-sock",
> +SOCK_PREFIX, config->unix_sock_dir) < 0)
> goto cleanup;
>
> if (privileged) {
> -if (virAsprintf(rosockfile, "%s/libvirt-sock-ro",
> config->unix_sock_dir) < 0 ||
> -virAsprintf(admsockfile, "%s/libvirt-admin-sock",
> config->unix_sock_dir) < 0)
> +if (virAsprintf(rosockfile, "%s/%s-sock-ro",
> +SOCK_PREFIX, config->unix_sock_dir) < 0 ||
> +virAsprintf(admsockfile, "%s/%s-admin-sock",
> +SOCK_PREFIX, config->unix_sock_dir) < 0)
> goto cleanup;
> }
> } else {
> if (privileged) {
> -if (VIR_STRDUP(*sockfile, LOCALSTATEDIR
> "/run/libvirt/libvirt-sock") < 0 ||
> -VIR_STRDUP(*rosockfile, LOCALSTATEDIR
> "/run/libvirt/libvirt-sock-ro") < 0 ||
> -VIR_STRDUP(*admsockfile, LOCALSTATEDIR
> "/run/libvirt/libvirt-admin-sock") < 0)
> +if (virAsprintf(sockfile, "%s/run/libvirt/%s-sock",
> +LOCALSTATEDIR, SOCK_PREFIX) < 0 ||
> +virAsprintf(sockfile, "%s/run/libvirt/%s-sock-ro",
> +LOCALSTATEDIR, SOCK_PREFIX) < 0 ||
> +virAsprintf(sockfile, "%s/run/libvirt/%s-admin-sock",
> +LOCALSTATEDIR, SOCK_PREFIX) < 0)
Copy-paste error on the variable name (use rosockfile and admsockfile)
> goto cleanup;
> } else {
> mode_t old_umask;
> @@ -248,8 +254,10 @@ daemonUnixSocketPaths(struct daemonConfig *config,
> }
> umask(old_umask);
>
> -if (virAsprintf(sockfile, "%s/libvirt-sock", rundir) < 0 ||
> -virAsprintf(admsockfile, "%s/libvirt-admin-sock", rundir) <
> 0)
> +if (virAsprintf(sockfile, "%s/%s-sock",
> +rundir, SOCK_PREFIX) < 0 ||
> +virAsprintf(admsockfile, "%s/%s-admin-sock",
> +rundir, SOCK_PREFIX) < 0)
> goto cleanup;
> }
> }
> @@ -902,12 +910,12 @@ daemonUsage(const char *argv0, bool privileged)
> fprintf(stderr, "\n");
>
> fprintf(stderr, "%s:\n", _("Sockets"));
> -fprintf(stderr, " %s\n",
> -privileged ? LOCALSTATEDIR "/run/libvirt/libvirt-sock" :
> - "$XDG_RUNTIME_DIR/libvirt/libvirt-sock");
> +fprintf(stderr, " %s/libvirt/%s-sock\n",
> +privileged ? LOCALSTATEDIR "/run" : "$XDG_RUNTIME_DIR",
> +SOCK_PREFIX);
> if (privileged)
> -fprintf(stderr, " %s\n",
> -LOCALSTATEDIR "/run/libvirt/libvirt-sock-ro");
> +fprintf(stderr, " %s/run/libvirt/%s-sock-ro\n",
> +LOCALSTATEDIR, SOCK_PREFIX);
> fprintf(stderr, "\n");
>
> fprintf(stderr, "%s:\n", _("TLS"));
> --
> 2.21.0
--
Cheers,
Christophe de Dinechin (IRC c3d)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 00/48] Split the libvirtd daemon into per-driver daemons
> On 30 Jul 2019, at 11:38, Daniel P. Berrangé wrote: > > On Tue, Jul 30, 2019 at 11:05:25AM +0200, Christophe de Dinechin wrote: >> >> Daniel P. Berrangé writes: >> >>> This is what all the driver refactoring I've done has been about >>> enabling. >>> >>> We gain new daemons for each driver, for the primary virt drivers: >>> >>> virtlibxld >> >> virtxend? >> >>> virtlxcd >>> virtqemud >>> virtvboxd >>> virtvzd >>> >>> And again for the secondary drivers >>> >>> virtinterfaced >>> virtnetworkd >>> virtnodedevd >>> virtnwfilterd >>> virtsecretd >>> virtstoraged >>> >>> Finally to support IP connectivity, and also the legacy lbivirtd UNIX >>> domain socket (for the old libvirt remote driver SSH tunnelling): >>> >>> virtproxyd >>> >>> The the sake of facilitating upgrades, the existing libvirtd still >>> exists and works the same way it always has. >>> >>> You either run libvirtd, or you run the per-driver daemons, never both. >> >> What happens if you run both? >> (I'll try to figure out by reviewing the rest of the code and/or testing) > > The drivers acquire an exclusive lock, causing the 2nd daemon to fail > to startup > > $ ./src/libvirtd & > > $ ./src/virtqemud > 2019-07-30 09:36:34.339+: 22809: info : libvirt version: 5.6.0 > 2019-07-30 09:36:34.339+: 22809: info : hostname: > dhcp-16-132.lcy.redhat.com > 2019-07-30 09:36:34.339+: 22809: error : virPidFileAcquirePath:376 : > Failed to acquire pid file '/run/user/501/libvirt/qemu/run/driver.pid': > Resource temporarily unavailable > 2019-07-30 09:36:34.339+: 22809: error : virStateInitialize:688 : > Initialisation of QEMU state driver failed: Failed to acquire pid file > '/run/user/501/libvirt/qemu/run/driver.pid': Resource temporarily unavailable > 2019-07-30 09:36:34.339+: 22809: error : daemonRunStateInit:821 : Driver > state initialisation failed > > > The same works the other way around too > > $ ./src/virtqemud & > > $ ./src/libvirtd > 2019-07-30 09:37:45.398+: 23109: info : libvirt version: 5.6.0 > 2019-07-30 09:37:45.398+: 23109: info : hostname: > dhcp-16-132.lcy.redhat.com > 2019-07-30 09:37:45.398+: 23109: error : virPidFileAcquirePath:376 : > Failed to acquire pid file '/run/user/501/libvirt/qemu/run/driver.pid': > Resource temporarily unavailable > 2019-07-30 09:37:45.398+: 23109: error : virStateInitialize:688 : > Initialisation of QEMU state driver failed: Failed to acquire pid file > '/run/user/501/libvirt/qemu/run/driver.pid': Resource temporarily unavailable > 2019-07-30 09:37:45.398+: 23109: error : daemonRunStateInit:821 : Driver > state initialisation failed > > > > the systemd unit files also have Conflicts rules which should prevent > even getting that far Thanks for testing. But that can only work for one deamon which shares the lock file name with libvirtd. What about the other drivers? I guess they can’t all share the same lock file, or I missed something big in the design. (Sorry, still reviewing, so did not really have time to try it yet) > > > 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 :| > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 06/48] remote: stop trying to print help as giant blocks of text
> On 30 Jul 2019, at 12:04, Daniel P. Berrangé wrote:
>
> On Tue, Jul 30, 2019 at 11:56:45AM +0200, Christophe de Dinechin wrote:
>>
>> Daniel P. Berrangé writes:
>>
>>> The remote daemon tries to print out its help text in a couple of giant
>>> blocks of text. This has already lead to duplication of the text for the
>>> privileged vs unprivileged execution mode. With the introduction of more
>>> daemons, this text is going to be duplicated many more times with small
>>> variations. This is very unfriendly to translators as they have to
>>> translate approximately the same text many times with small tweaks.
>>>
>>> Splitting the text up into individual strings to print means that each
>>> piece will only need translating once. It also gets rid of all the
>>> layout information from the translated strings, so avoids the problem of
>>> translators breaking formatting by mistake.
>>>
>>> Signed-off-by: Daniel P. Berrangé
>>> ---
>>> src/remote/remote_daemon.c | 128 ++---
>>> src/remote/remote_driver.h | 1 -
>>> 2 files changed, 64 insertions(+), 65 deletions(-)
>>>
>>> diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
>>> index d887b7abfb..69385af1c4 100644
>>> --- a/src/remote/remote_daemon.c
>>> +++ b/src/remote/remote_daemon.c
>>> @@ -859,75 +859,75 @@ daemonSetupHostUUID(const struct daemonConfig *config)
>>> return 0;
>>> }
>>>
>>> +typedef struct {
>>> +const char *opts;
>>> +const char *help;
>>> +} virOptionHelp;
>>> +
>>> /* Print command-line usage. */
>>> static void
>>> daemonUsage(const char *argv0, bool privileged)
>>> {
>>> -fprintf(stderr,
>>> -_("\n"
>>> - "Usage:\n"
>>> - " %s [options]\n"
>>> - "\n"
>>> - "Options:\n"
>>> - " -h | --helpDisplay program help:\n"
>>> - " -v | --verbose Verbose messages.\n"
>>> - " -d | --daemon Run as a daemon & write PID
>>> file.\n"
>>> - " -l | --listen Listen for TCP/IP connections.\n"
>>> - " -t | --timeout Exit after timeout period.\n"
>>> - " -f | --configConfiguration file.\n"
>>> - " -V | --version Display version information.\n"
>>> - " -p | --pid-file Change name of PID file.\n"
>>> - "\n"
>>> - "libvirt management daemon:\n"),
>>> -argv0);
>>> +size_t i;
>>> +virOptionHelp opthelp[] = {
>>> +{ "-h | --help", N_("Display program help") },
>>
>> Why use N_ both here and in the printout code (copied below)?
>>
>>fprintf(stderr, " %-22s %s\n", opthelp[i].opts, N_(opthelp[i].help));
>>
>> When is the message translated?
>
> Yeah that's a screwup. The fprintf() should be calling gettext
>
>>> +fprintf(stderr, "\n");
>>> +fprintf(stderr, "%s:\n", _("Usage"));
>>> +fprintf(stderr, " %s [%s]\n", argv0, _("options"));
>>
>> Here, despite your argument regarding formatting, I believe that
>> translators should have a larger context. Also, as the gettext
>> documentation points out, ":" needs a space before in French, so it
>> should be placed within the part to translate.
>
> What documentation are you seeing this in ? I'm not come across
> this suggestion before and want to read more
>
>>
>>fprintf(stderr, _("Usage:\n %s [options]\n\n"), argv0);
>>
>>
>>> +fprintf(stderr, "\n");
>>> +
>>> +fprintf(stderr, "%s:\n", _("Options"));
>>
>> Same, for localization of whitespace around : it would be better to have
>>
>> fprintf(stderr, _("Options:\n"));
>>
>> This applies to all cases where you have %s: below.
>>
>>
>>> +for (i = 0; i < ARRAY_CARDINALITY(opthelp); i++)
>>> +fprintf(stderr, " %-22s %s\n", opthelp[i].opts,
>>> N_(opthelp[i].help));
>>
>> Based
Re: [libvirt] [PATCH v3 06/48] remote: stop trying to print help as giant blocks of text
> On 30 Jul 2019, at 12:04, Daniel P. Berrangé wrote:
>
> On Tue, Jul 30, 2019 at 11:56:45AM +0200, Christophe de Dinechin wrote:
>>
>> Daniel P. Berrangé writes:
>>
>>> The remote daemon tries to print out its help text in a couple of giant
>>> blocks of text. This has already lead to duplication of the text for the
>>> privileged vs unprivileged execution mode. With the introduction of more
>>> daemons, this text is going to be duplicated many more times with small
>>> variations. This is very unfriendly to translators as they have to
>>> translate approximately the same text many times with small tweaks.
>>>
>>> Splitting the text up into individual strings to print means that each
>>> piece will only need translating once. It also gets rid of all the
>>> layout information from the translated strings, so avoids the problem of
>>> translators breaking formatting by mistake.
>>>
>>> Signed-off-by: Daniel P. Berrangé
>>> ---
>>> src/remote/remote_daemon.c | 128 ++---
>>> src/remote/remote_driver.h | 1 -
>>> 2 files changed, 64 insertions(+), 65 deletions(-)
>>>
>>> diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
>>> index d887b7abfb..69385af1c4 100644
>>> --- a/src/remote/remote_daemon.c
>>> +++ b/src/remote/remote_daemon.c
>>> @@ -859,75 +859,75 @@ daemonSetupHostUUID(const struct daemonConfig *config)
>>> return 0;
>>> }
>>>
>>> +typedef struct {
>>> +const char *opts;
>>> +const char *help;
>>> +} virOptionHelp;
>>> +
>>> /* Print command-line usage. */
>>> static void
>>> daemonUsage(const char *argv0, bool privileged)
>>> {
>>> -fprintf(stderr,
>>> -_("\n"
>>> - "Usage:\n"
>>> - " %s [options]\n"
>>> - "\n"
>>> - "Options:\n"
>>> - " -h | --helpDisplay program help:\n"
>>> - " -v | --verbose Verbose messages.\n"
>>> - " -d | --daemon Run as a daemon & write PID
>>> file.\n"
>>> - " -l | --listen Listen for TCP/IP connections.\n"
>>> - " -t | --timeout Exit after timeout period.\n"
>>> - " -f | --configConfiguration file.\n"
>>> - " -V | --version Display version information.\n"
>>> - " -p | --pid-file Change name of PID file.\n"
>>> - "\n"
>>> - "libvirt management daemon:\n"),
>>> -argv0);
>>> +size_t i;
>>> +virOptionHelp opthelp[] = {
>>> +{ "-h | --help", N_("Display program help") },
>>
>> Why use N_ both here and in the printout code (copied below)?
>>
>>fprintf(stderr, " %-22s %s\n", opthelp[i].opts, N_(opthelp[i].help));
>>
>> When is the message translated?
>
> Yeah that's a screwup. The fprintf() should be calling gettext
>
>>> +fprintf(stderr, "\n");
>>> +fprintf(stderr, "%s:\n", _("Usage"));
>>> +fprintf(stderr, " %s [%s]\n", argv0, _("options"));
>>
>> Here, despite your argument regarding formatting, I believe that
>> translators should have a larger context. Also, as the gettext
>> documentation points out, ":" needs a space before in French, so it
>> should be placed within the part to translate.
>
> What documentation are you seeing this in ? I'm not come across
> this suggestion before and want to read more
https://www.gnu.org/software/gettext/manual/gettext.html
See end of section 4.4, specifically:
For example, ‘"%s"’ is an example of string not requiring translation. But
‘"%s: %d"’ does require translation, because in French, unlike in English, it’s
customary to put a space before a colon.
>
>>
>>fprintf(stderr, _("Usage:\n %s [options]\n\n"), argv0);
>>
>>
>>> +fprintf(stderr, "\n");
>>> +
>>> +fprintf(stderr, "%s:\n", _("Options"));
>>
>> Same, for localization of whitespace around : it would be better to have
>>
>> fprintf(stderr, _("Options:\n&qu
Re: [libvirt] [PATCH v3 07/48] remote: conditionalize socket names in libvirtd daemon
Daniel P. Berrangé writes:
> Prepare for reusing libvirtd source to create other daemons by making
> the socket names conditionally defined by the make rules.
>
> Reviewed-by: Andrea Bolognani
> Signed-off-by: Daniel P. Berrangé
> ---
> src/remote/Makefile.inc.am | 1 +
> src/remote/remote_daemon.c | 34 +-
> 2 files changed, 22 insertions(+), 13 deletions(-)
>
> diff --git a/src/remote/Makefile.inc.am b/src/remote/Makefile.inc.am
> index 0400dabad9..ced940d3c1 100644
> --- a/src/remote/Makefile.inc.am
> +++ b/src/remote/Makefile.inc.am
> @@ -146,6 +146,7 @@ libvirtd_CFLAGS = \
> -I$(srcdir)/access \
> -I$(srcdir)/conf \
> -I$(srcdir)/rpc \
> + -DSOCK_PREFIX="\"libvirt\"" \
> $(NULL)
>
> libvirtd_LDFLAGS = \
> diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
> index 69385af1c4..f9d923b357 100644
> --- a/src/remote/remote_daemon.c
> +++ b/src/remote/remote_daemon.c
> @@ -221,19 +221,25 @@ daemonUnixSocketPaths(struct daemonConfig *config,
> char *rundir = NULL;
>
> if (config->unix_sock_dir) {
> -if (virAsprintf(sockfile, "%s/libvirt-sock", config->unix_sock_dir)
> < 0)
> +if (virAsprintf(sockfile, "%s/%s-sock",
> +SOCK_PREFIX, config->unix_sock_dir) < 0)
> goto cleanup;
>
> if (privileged) {
> -if (virAsprintf(rosockfile, "%s/libvirt-sock-ro",
> config->unix_sock_dir) < 0 ||
> -virAsprintf(admsockfile, "%s/libvirt-admin-sock",
> config->unix_sock_dir) < 0)
> +if (virAsprintf(rosockfile, "%s/%s-sock-ro",
> +SOCK_PREFIX, config->unix_sock_dir) < 0 ||
> +virAsprintf(admsockfile, "%s/%s-admin-sock",
> +SOCK_PREFIX, config->unix_sock_dir) < 0)
> goto cleanup;
> }
> } else {
> if (privileged) {
> -if (VIR_STRDUP(*sockfile, LOCALSTATEDIR
> "/run/libvirt/libvirt-sock") < 0 ||
> -VIR_STRDUP(*rosockfile, LOCALSTATEDIR
> "/run/libvirt/libvirt-sock-ro") < 0 ||
> -VIR_STRDUP(*admsockfile, LOCALSTATEDIR
> "/run/libvirt/libvirt-admin-sock") < 0)
> +if (virAsprintf(sockfile, "%s/run/libvirt/%s-sock",
> +LOCALSTATEDIR, SOCK_PREFIX) < 0 ||
> +virAsprintf(sockfile, "%s/run/libvirt/%s-sock-ro",
> +LOCALSTATEDIR, SOCK_PREFIX) < 0 ||
> +virAsprintf(sockfile, "%s/run/libvirt/%s-admin-sock",
> +LOCALSTATEDIR, SOCK_PREFIX) < 0)
> goto cleanup;
> } else {
> mode_t old_umask;
> @@ -248,8 +254,10 @@ daemonUnixSocketPaths(struct daemonConfig *config,
> }
> umask(old_umask);
>
> -if (virAsprintf(sockfile, "%s/libvirt-sock", rundir) < 0 ||
> -virAsprintf(admsockfile, "%s/libvirt-admin-sock", rundir) <
> 0)
> +if (virAsprintf(sockfile, "%s/%s-sock",
> +rundir, SOCK_PREFIX) < 0 ||
> +virAsprintf(admsockfile, "%s/%s-admin-sock",
> +rundir, SOCK_PREFIX) < 0)
> goto cleanup;
> }
> }
> @@ -902,12 +910,12 @@ daemonUsage(const char *argv0, bool privileged)
> fprintf(stderr, "\n");
>
> fprintf(stderr, "%s:\n", _("Sockets"));
Localization of : in French suggests you write this as:
fprintf(stderr, "%s\n", _("Sockets:"));
> -fprintf(stderr, " %s\n",
> -privileged ? LOCALSTATEDIR "/run/libvirt/libvirt-sock" :
> -"$XDG_RUNTIME_DIR/libvirt/libvirt-sock");
> +fprintf(stderr, " %s/libvirt/%s-sock\n",
> +privileged ? LOCALSTATEDIR "/run" : "$XDG_RUNTIME_DIR",
> +SOCK_PREFIX);
> if (privileged)
> -fprintf(stderr, " %s\n",
> -LOCALSTATEDIR "/run/libvirt/libvirt-sock-ro");
> +fprintf(stderr, " %s/run/libvirt/%s-sock-ro\n",
> +LOCALSTATEDIR, SOCK_PREFIX);
> fprintf(stderr, "\n");
>
> fprintf(stderr, "%s:\n", _("TLS"));
fprintf(stderr, "%s\n", _("TLS:"));
> --
> 2.21.0
Reviewed-by: Christophe de Dinechin
--
Cheers,
Christophe de Dinechin (IRC c3d)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 06/48] remote: stop trying to print help as giant blocks of text
uot;
> - "\n"
> - "Configuration file (unless overridden by -f):\n"
> - " $XDG_CONFIG_HOME/libvirt/libvirtd.conf\n"
> - "\n"
> - "Sockets:\n"
> - " $XDG_RUNTIME_DIR/libvirt/libvirt-sock\n"
> - "\n"
> - "TLS:\n"
> - " CA certificate: $HOME/.pki/libvirt/cacert.pem\n"
> - " Server certificate:
> $HOME/.pki/libvirt/servercert.pem\n"
> - " Server private key:
> $HOME/.pki/libvirt/serverkey.pem\n"
> - "\n"
> - "PID file:\n"
> - " $XDG_RUNTIME_DIR/libvirt/libvirtd.pid\n"
> - "\n"));
> -}
> +fprintf(stderr, "\n");
> +fprintf(stderr, "%s:\n", _("Usage"));
> +fprintf(stderr, " %s [%s]\n", argv0, _("options"));
Here, despite your argument regarding formatting, I believe that
translators should have a larger context. Also, as the gettext
documentation points out, ":" needs a space before in French, so it
should be placed within the part to translate.
fprintf(stderr, _("Usage:\n %s [options]\n\n"), argv0);
> +fprintf(stderr, "\n");
> +
> +fprintf(stderr, "%s:\n", _("Options"));
Same, for localization of whitespace around : it would be better to have
fprintf(stderr, _("Options:\n"));
This applies to all cases where you have %s: below.
> +for (i = 0; i < ARRAY_CARDINALITY(opthelp); i++)
> +fprintf(stderr, " %-22s %s\n", opthelp[i].opts,
> N_(opthelp[i].help));
Based on comment above, replace N_ with _ ?
> +fprintf(stderr, "\n");
> +
> +fprintf(stderr, "%s:\n", _("libvirt management daemon"));
> +
> +fprintf(stderr, "\n");
> +fprintf(stderr, " %s:\n", _("Default paths"));
> +fprintf(stderr, "\n");
> +
> +fprintf(stderr, "%s:\n", _("Configuration file (unless overridden by
> -f)"));
> +fprintf(stderr, " %s/libvirt/libvirtd.conf\n",
> +privileged ? SYSCONFDIR : "$XDG_CONFIG_HOME");
Add a N_ ?
> +fprintf(stderr, "\n");
> +
> +fprintf(stderr, "%s:\n", _("Sockets"));
> +fprintf(stderr, " %s\n",
> +privileged ? LOCALSTATEDIR "/run/libvirt/libvirt-sock" :
> +"$XDG_RUNTIME_DIR/libvirt/libvirt-sock");
Add N_ ?
> +if (privileged)
> +fprintf(stderr, " %s\n",
> +LOCALSTATEDIR "/run/libvirt/libvirt-sock-ro");
Add N_?
> +fprintf(stderr, "\n");
> +
> +fprintf(stderr, "%s:\n", _("TLS"));
> +fprintf(stderr, " %s: %s\n",
> + _("CA certificate"),
> +privileged ? LIBVIRT_CACERT : "$HOME/.pki/libvirt/cacert.pem");
> +fprintf(stderr, " %s: %s\n",
> +_("Server certificate"),
> +privileged ? LIBVIRT_SERVERCERT :
> "$HOME/.pki/libvirt/servercert.pem");
> +fprintf(stderr, " %s: %s\n",
> +_("Server private key"),
> +privileged ? LIBVIRT_SERVERKEY :
> "$HOME/.pki/libvirt/serverkey.pem");
> +fprintf(stderr, "\n");
> +
> +fprintf(stderr, "%s:\n",
> +_("PID file (unless overridden by -p)"));
> +fprintf(stderr, " %s\n",
> +privileged ? LOCALSTATEDIR "/run/libvirtd.pid":
> +"$XDG_RUNTIME_DIR/libvirt/libvirtd.pid");
> +fprintf(stderr, "\n");
> }
>
> int main(int argc, char **argv) {
> diff --git a/src/remote/remote_driver.h b/src/remote/remote_driver.h
> index 8c7da6b000..132e478ef3 100644
> --- a/src/remote/remote_driver.h
> +++ b/src/remote/remote_driver.h
> @@ -34,7 +34,6 @@ unsigned long remoteVersion(void);
> #define LIBVIRTD_PRIV_UNIX_SOCKET LOCALSTATEDIR "/run/libvirt/libvirt-sock"
> #define LIBVIRTD_PRIV_UNIX_SOCKET_RO LOCALSTATEDIR
> "/run/libvirt/libvirt-sock-ro"
> #define LIBVIRTD_USER_UNIX_SOCKET "libvirt-sock"
> -#define LIBVIRTD_CONFIGURATION_FILE SYSCONFDIR "/libvirt/libvirtd.conf"
>
> /* Defaults for PKI directory. */
> #define LIBVIRT_PKI_DIR SYSCONFDIR "/pki"
> --
> 2.21.0
Reviewed-by: Christophe de Dinechin
--
Cheers,
Christophe de Dinechin (IRC c3d)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 48/48] remote: pass identity across to newly opened daemons
teConnectGetType, /* 0.3.0 */
> .connectGetVersion = remoteConnectGetVersion, /* 0.3.0 */
> diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
> index 2f91bd1921..42e61ba20f 100644
> --- a/src/remote/remote_protocol.x
> +++ b/src/remote/remote_protocol.x
> @@ -53,6 +53,9 @@ typedef string remote_nonnull_string;
> /* A long string, which may be NULL. */
> typedef remote_nonnull_string *remote_string;
>
> +/* Upper limit on identity parameters */
> +const REMOTE_CONNECT_IDENTITY_PARAMS_MAX = 20;
> +
> /* Upper limit on lists of domains. */
> const REMOTE_DOMAIN_LIST_MAX = 16384;
>
> @@ -3723,6 +3726,11 @@ struct remote_domain_checkpoint_delete_args {
> unsigned int flags;
> };
>
> +struct remote_connect_set_identity_args {
> +remote_typed_param params;
> +unsigned int flags;
> +};
> +
> /*- Protocol. -*/
>
> /* Define the program number, protocol version and procedure numbers here. */
> @@ -6538,7 +6546,7 @@ enum remote_procedure {
> */
> REMOTE_PROC_NETWORK_PORT_DELETE = 410,
>
> - /**
> +/**
> * @generate: both
> * @acl: domain:checkpoint
> * @acl: domain:fs_freeze:VIR_DOMAIN_CHECKPOINT_CREATE_QUIESCE
> @@ -6584,5 +6592,11 @@ enum remote_procedure {
> * @generate: both
> * @acl: domain:checkpoint
> */
> -REMOTE_PROC_DOMAIN_CHECKPOINT_DELETE = 417
> +REMOTE_PROC_DOMAIN_CHECKPOINT_DELETE = 417,
> +
> +/**
> + * @generate: client
> + * @acl: connect:write
> + */
> +REMOTE_PROC_CONNECT_SET_IDENTITY = 418
> };
> diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
> index a42b4a9671..05229f00c5 100644
> --- a/src/remote_protocol-structs
> +++ b/src/remote_protocol-structs
> @@ -3105,6 +3105,13 @@ struct remote_domain_checkpoint_delete_args {
> remote_nonnull_domain_checkpoint checkpoint;
> u_int flags;
> };
> +struct remote_connect_set_identity_args {
> +struct {
> +u_int params_len;
> +remote_typed_param * params_val;
Indent by 8 spaces and try to align variables in the same file?
Nothing good could come out of it ;-)
> +} params;
> +u_int flags;
> +};
> enum remote_procedure {
> REMOTE_PROC_CONNECT_OPEN = 1,
> REMOTE_PROC_CONNECT_CLOSE = 2,
> @@ -3523,4 +3530,5 @@ enum remote_procedure {
> REMOTE_PROC_DOMAIN_CHECKPOINT_LOOKUP_BY_NAME = 415,
> REMOTE_PROC_DOMAIN_CHECKPOINT_GET_PARENT = 416,
> REMOTE_PROC_DOMAIN_CHECKPOINT_DELETE = 417,
> +REMOTE_PROC_CONNECT_SET_IDENTITY = 418,
> };
> diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
> index 1f020a5a04..2a278171f5 100644
> --- a/src/rpc/virnetserverclient.c
> +++ b/src/rpc/virnetserverclient.c
> @@ -844,6 +844,18 @@ virIdentityPtr
> virNetServerClientGetIdentity(virNetServerClientPtr client)
> }
>
>
> +void virNetServerClientSetIdentity(virNetServerClientPtr client,
> + virIdentityPtr identity)
> +{
> +virObjectLock(client);
> +virObjectUnref(client->identity);
> +client->identity = identity;
> +if (client->identity)
> +virObjectRef(client->identity);
> +virObjectUnlock(client);
> +}
> +
> +
> int virNetServerClientGetSELinuxContext(virNetServerClientPtr client,
> char **context)
> {
> diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h
> index 1b01bedbcb..1c520fef6b 100644
> --- a/src/rpc/virnetserverclient.h
> +++ b/src/rpc/virnetserverclient.h
> @@ -123,6 +123,8 @@ int
> virNetServerClientGetSELinuxContext(virNetServerClientPtr client,
> char **context);
>
> virIdentityPtr virNetServerClientGetIdentity(virNetServerClientPtr client);
> +void virNetServerClientSetIdentity(virNetServerClientPtr client,
> + virIdentityPtr identity);
>
> void *virNetServerClientGetPrivateData(virNetServerClientPtr client);
>
> --
> 2.21.0
Reviewed-by: Christophe de Dinechin
--
Cheers,
Christophe de Dinechin (IRC c3d)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 00/48] Split the libvirtd daemon into per-driver daemons
t; src/remote/remote_daemon_config.h | 10 +-
> src/remote/remote_daemon_dispatch.c | 1354 ++---
> src/remote/remote_driver.c| 424 --
> src/remote/remote_driver.h|4 -
> src/remote/remote_protocol.x | 18 +-
> src/remote/test_libvirtd.aug.in | 24 +-
> src/remote/virtproxyd.service.in | 24 +
> src/remote_protocol-structs |8 +
> src/rpc/gendispatch.pl| 96 +-
> src/rpc/virnetserverclient.c | 24 +-
> src/rpc/virnetserverclient.h |2 +
> src/secret/Makefile.inc.am| 62 +
> src/secret/secret_driver.c|8 +-
> src/secret/virtsecretd.service.in | 24 +
> src/storage/Makefile.inc.am | 61 +
> src/storage/storage_driver.c |8 +-
> src/storage/virtstoraged.service.in | 26 +
> src/util/viridentity.c| 483 +++---
> src/util/viridentity.h| 87 +-
> src/vbox/Makefile.inc.am | 62 +
> src/vbox/virtvboxd.service.in | 25 +
> src/vz/Makefile.inc.am| 62 +
> src/vz/virtvzd.service.in | 25 +
> src/vz/vz_driver.c| 14 +-
> tests/viridentitytest.c | 97 +-
> tests/virnetserverclienttest.c| 45 +-
> tools/libvirt-guests.service.in |2 +-
> 96 files changed, 3642 insertions(+), 1703 deletions(-)
> create mode 100644 src/interface/virtinterfaced.service.in
> create mode 100644 src/libxl/virtxend.service.in
> create mode 100644 src/lxc/virtlxcd.service.in
> create mode 100644 src/network/virtnetworkd.service.in
> create mode 100644 src/node_device/virtnodedevd.service.in
> create mode 100644 src/nwfilter/virtnwfilterd.service.in
> create mode 100644 src/qemu/virtqemud.service.in
> rename src/remote/{libvirtd.aug => libvirtd.aug.in} (89%)
> rename src/remote/{libvirtd.conf => libvirtd.conf.in} (92%)
> create mode 100644 src/remote/virtproxyd.service.in
> create mode 100644 src/secret/virtsecretd.service.in
> create mode 100644 src/storage/virtstoraged.service.in
> create mode 100644 src/vbox/virtvboxd.service.in
> create mode 100644 src/vz/virtvzd.service.in
>
> --
> 2.21.0
--
Cheers,
Christophe de Dinechin (IRC c3d)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 0/5] docs: Update third-party JavaScript libraries
> On 20 Jun 2019, at 12:25, Andrea Bolognani wrote: > > On Wed, 2019-06-19 at 17:00 +0200, Christophe de Dinechin wrote: >>> Andrea Bolognani (5): >>> docs: Update jQuery >>> docs: Update Moment.js >>> docs: Update jquery.rss >>> docs: Perform JavaScript minimization at build time >>> spec: Require uglifyjs for RPM build >> >> In the spec file, why do you BuildRequire uglify-js only on Fedora? > > uglifyjs is not packaged for RHEL, which is the other platform our > spec file targets, so we can't really BuildRequire it there. I was afraid that was the reason ;-) > >> Does that mean that if you build elsewhere, you may end up with a >> build that minifies or not depending on whether uglifyjs is in path or not? > > Yeah. Luckily we're not going with these patches but with Martin's > approach instead, which neatly sidesteps this issue :) OK. > > -- > Andrea Bolognani / Red Hat / Virtualization > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] docs: use case sensitive javascript
> On 19 Jun 2019, at 17:22, Martin Kletzander wrote:
>
> Signed-off-by: Martin Kletzander
> ---
> docs/js/main.js | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/docs/js/main.js b/docs/js/main.js
> index 07f79c7ff903..668bf752b73d 100644
> --- a/docs/js/main.js
> +++ b/docs/js/main.js
> @@ -28,7 +28,7 @@ function pageload() {
> advancedSearch.className = "advancedsearch"
>
> simpleSearch = document.getElementById("simplesearch")
> -simplesearch.addEventListener("submit", advancedsearch)
> + simpleSearch.addEventListener("submit", advancedsearch)
Reviewed-by: Christophe de Dinechin
>
> docLoc = document.location;
> if (docLoc.protocol != "file:" ||
> --
> 2.21.0
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 0/5] docs: Update third-party JavaScript libraries
> On 19 Jun 2019, at 14:41, Andrea Bolognani wrote: > > We're carrying around embedded copies of a few JavaScript libraries > for use in our homepage, and we've been unforgivably bad at keeping > them up to date. Address that. > > While doing so, replace the minified version of each library with > its non-minified counterpart so that libvirt.git and release archives > will include the actual source code for them, rather than what is > basically the JavaScript equivalent of binary blobs. > > Note that I consider this to be merely a stop-gap measure until we > start generating the blog roll, the only part of the libvirt website > that uses these libraries, on the server side and finally drop them > entirely. I don't have cycles to dedicate to that effort at the > moment though, and this is better than leaving the situation > unchanged for who knows how much longer. > > Since the diffs are kinda big and honestly just not very interesting, > I've snipped them liberally; the unabridged version of this series > can be fetched with > > $ git fetch https://github.com/andreabolognani/libvirt js > > > Changes from [v2]: > > * import non-minified libraries and minify them at build time. > > Changes from [v1]: > > * drop libraries instead of updating them. > > > [v2] https://www.redhat.com/archives/libvir-list/2019-June/msg00611.html > [v1] https://www.redhat.com/archives/libvir-list/2019-June/msg00551.html > > Andrea Bolognani (5): > docs: Update jQuery > docs: Update Moment.js > docs: Update jquery.rss > docs: Perform JavaScript minimization at build time > spec: Require uglifyjs for RPM build In the spec file, why do you BuildRequire uglify-js only on Fedora? Does that mean that if you build elsewhere, you may end up with a build that minifies or not depending on whether uglifyjs is in path or not? > > .gitignore | 1 + > docs/Makefile.am |37 +- > docs/index.html.in | 6 +- > docs/js/jquery-3.1.1.min.js | 4 - > docs/js/jquery-3.4.1.js | 10598 + > docs/js/jquery.rss-3.3.0.js | 333 ++ > docs/js/jquery.rss.min.js|11 - > docs/js/moment-2.24.0.js | 4602 ++ > docs/js/moment.min.js| 7 - > docs/page.xsl| 2 +- > docs/site.xsl| 1 + > docs/subsite.xsl | 1 + > libvirt.spec.in | 5 + > m4/virt-external-programs.m4 | 2 + > 14 files changed, 15578 insertions(+), 32 deletions(-) > delete mode 100644 docs/js/jquery-3.1.1.min.js > create mode 100644 docs/js/jquery-3.4.1.js > create mode 100644 docs/js/jquery.rss-3.3.0.js > delete mode 100644 docs/js/jquery.rss.min.js > create mode 100644 docs/js/moment-2.24.0.js > delete mode 100644 docs/js/moment.min.js > > -- > 2.21.0 > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 4/5] docs: Perform JavaScript minimization at build time
> On 19 Jun 2019, at 14:41, Andrea Bolognani wrote:
>
> We want to store third-party JavaScript libraries in their
> non-minimized (source) form in the repository, but when users
> are browsing libvirt.org we'd rather only transmit the more
> compact minified variant.
>
> Call uglifyjs at build time, if available, to achieve this.
> Signed-off-by: Andrea Bolognani
Reviewed-by: Christophe de Dinechin
> ---
> .gitignore | 1 +
> docs/Makefile.am | 30 +++---
> docs/index.html.in | 6 +++---
> docs/page.xsl| 2 +-
> docs/site.xsl| 1 +
> docs/subsite.xsl | 1 +
> m4/virt-external-programs.m4 | 2 ++
> 7 files changed, 36 insertions(+), 7 deletions(-)
>
> diff --git a/.gitignore b/.gitignore
> index 727bfdb6ec..522ac762b6 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -61,6 +61,7 @@
> /docs/apibuild.py.stamp
> /docs/devhelp/libvirt.devhelp
> /docs/hvsupport.html.in
> +/docs/js/*.min.js
> /docs/libvirt-admin-*.xml
> /docs/libvirt-api.xml
> /docs/libvirt-lxc-*.xml
> diff --git a/docs/Makefile.am b/docs/Makefile.am
> index 9b79fc60c1..e787a7c47b 100644
> --- a/docs/Makefile.am
> +++ b/docs/Makefile.am
> @@ -240,6 +240,7 @@ news.html.in: \
> $(srcdir)/news.xml \
> $(srcdir)/news-html.xsl
> $(AM_V_GEN)$(XSLTPROC) --nonet \
> + --stringparam js "$(JS)" \
> $(srcdir)/news-html.xsl \
> $(srcdir)/news.xml \
> >$@-tmp \
> @@ -258,7 +259,7 @@ MAINTAINERCLEANFILES += \
> convert -rotate 90 $< $@
>
> %.html.tmp: %.html.in site.xsl subsite.xsl page.xsl \
> - $(acl_generated)
> + $(acl_generated) $(minified_javascript)
> $(AM_V_GEN)name=`echo $@ | sed -e 's/.tmp//'`; \
> dir=`dirname $@` ; \
> if test "$$dir" = "."; \
> @@ -270,24 +271,47 @@ MAINTAINERCLEANFILES += \
> fi; \
> $(XSLTPROC) --stringparam pagename $$name \
> --stringparam timestamp $(timestamp) --nonet \
> + --stringparam js "$(JS)" \
> $(top_srcdir)/docs/$$style $< > $@ \
> + || { rm $@ && exit 1; }; \
> + sed -i 's/@JS@/$(JS)/g' "$@" \
> || { rm $@ && exit 1; }
>
> %.html: %.html.tmp
> $(AM_V_GEN)$(XMLLINT) --nonet --format $< > $(srcdir)/$@ \
> || { rm $(srcdir)/$@ && exit 1; }
>
> +
> +if HAVE_UGLIFYJS
> +JS = min.js
> +minified_javascript = $(javascript:%.js=%.min.js)
> +EXTRA_DIST += $(minified_javascript)
> +MAINTAINERCLEANFILES += $(minified_javascript)
> +
> +%.min.js: %.js
> + $(AM_V_GEN)$(UGLIFYJS) -o $@ $<
> +else ! HAVE_UGLIFYJS
> +JS = js
> +minified_javascript = $(javascript)
> +
> +dist-hook:
> + echo "uglifyjs is required to make dist." >&2
It’s silly, but the Fedora package is called uglify-js. Maybe mention that?
> + exit 1
> +endif ! HAVE_UGLIFYJS
> +
> $(apihtml_generated): html/index.html
>
> html/index.html: libvirt-api.xml newapi.xsl page.xsl $(APIBUILD_STAMP)
> $(AM_V_GEN)$(XSLTPROC) --nonet -o $(srcdir)/ \
> --stringparam builddir '$(abs_top_builddir)' \
> --stringparam timestamp $(timestamp) \
> + --stringparam js "$(JS)" \
> $(srcdir)/newapi.xsl $(srcdir)/libvirt-api.xml && \
> $(XMLLINT) --nonet --noout $(srcdir)/html/*.html
>
> $(addprefix $(srcdir)/,$(devhelphtml)): $(srcdir)/libvirt-api.xml
> $(devhelpxsl)
> $(AM_V_GEN)$(XSLTPROC) --stringparam timestamp $(timestamp) \
> + --stringparam js "$(JS)" \
> --nonet -o $(srcdir)/devhelp/ \
> $(top_srcdir)/docs/devhelp/devhelp.xsl $(srcdir)/libvirt-api.xml
>
> @@ -372,7 +396,7 @@ install-data-local:
> for f in $(css) $(dot_html) $(gif) $(png); do \
> $(INSTALL) -m 0644 $(srcdir)/$$f $(DESTDIR)$(HTML_DIR); done
> $(mkinstalldirs) $(DESTDIR)$(HTML_DIR)/js
> - for f in $(javascript); do \
> + for f in $(minified_javascript); do \
> $(INSTALL) -m 0644 $(srcdir)/$$f $(DESTDIR)$(HTML_DIR)/js/; done
> $(mkinstalldirs) $(DESTDIR)$(HTML_DIR)/logos
> for f in $(logofiles); do \
> @@ -401,7 +425,7 @@ uninstall-local:
> for f in $(logofiles); do \
> rm -f $(DESTDIR)$(HTML_DIR)/$$f; \
> done
> - for f in $(javascript); do \
> + for f in $(minified_javascript); do \
> rm -f $(DESTDIR)$(HTML_DIR)/$$f; \
> done
> for h in $(apihtml); do rm -f $(DESTDIR)$(HTML_DIR)/$$h; done
> diff --git a/docs/index.html.in b/docs/ind
Re: [libvirt] [go PATCH] Add support for virNetworkPort object and APIs
> On 19 Jun 2019, at 11:14, Daniel P. Berrangé wrote:
>
> On Wed, Jun 19, 2019 at 10:50:14AM +0200, Christophe de Dinechin wrote:
>
> So the first thing our Go code does is check the libvirt native library
> version number and report an error
>
> eg
>
> func (n *NetworkPort) Delete(flags uint) error {
>if C.LIBVIR_VERSION_NUMBER < 5005000 {
>return makeNotImplementedError("virNetworkPortDelete")
>}
Duh, of course, that’s supposed to be the only consumer!
I somehow imagined that the wrappers themselves were exposed,
i.e. some Go programmer could have some weird reason to write
C.virtFunc(blah)
I don’t think you can prevent it, but I agree that with proper Go wrappers,
there
is no reason this would ever happen…
FWIW, the code looks sane to me, but I don’t consider myself enough of an
expert in this area to report a Reviewed-by.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [go PATCH] Add support for virNetworkPort object and APIs
> On 18 Jun 2019, at 19:01, Daniel P. Berrangé wrote:
>
> On Tue, Jun 18, 2019 at 06:33:01PM +0200, Christophe de Dinechin wrote:
>>
>>
>>> On 18 Jun 2019, at 17:43, Daniel P. Berrangé wrote:
>>>
>>> Signed-off-by: Daniel P. Berrangé
>>> ---
>>> network.go | 80 ++
>>> network_port.go | 233
>>> network_port_compat.h | 67
>>> network_port_wrapper.go | 197 +
>>> network_port_wrapper.h | 79 ++
>>> network_wrapper.go | 73 +
>>> network_wrapper.h | 23
>>> 7 files changed, 752 insertions(+)
>>> create mode 100644 network_port.go
>>> create mode 100644 network_port_compat.h
>>> create mode 100644 network_port_wrapper.go
>>> create mode 100644 network_port_wrapper.h
>
>>> +package libvirt
>>> +
>>> +/*
>>> +#cgo pkg-config: libvirt
>>> +#include
>>> +#include "network_port_wrapper.h"
>>> +
>>> +virNetworkPtr
>>> +virNetworkPortGetNetworkWrapper(virNetworkPortPtr port,
>>> + virErrorPtr err)
>>> +{
>>> +#if LIBVIR_VERSION_NUMBER < 5005000
>>> +assert(0); // Caller should have checked version
>>
>> What about
>>
>> assert(!”C function not available in this version - Caller should have
>> checked version”);
>>
>> ?
>
> That's certainly possible, but I'm not sure its worth bothering with
> since this should never be hit in practice & if it is hit we'll see
> the offending code in the crash dump easily enough.
By construction, this case happens for a non-libvirt developer.
The stack trace is useless, nothing in the names there explains
that it’s a version mismatch. So the non-libvirt Go developer will need
to install C source code just to be able to read a comment that could
have been shown in the crash? To me, it looks like like the bother
is much higher on their side than on yours.
Also, given that the sole purpose of these wrappers is to fail if
the underlying C function does not exist, would it make sense to
just not emit the wrapper in that case? That would lead to a link-time
failure that might be even more friendly, insofar as it catches things earlier.
Cheers,
Christophe
>
>
> 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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [go PATCH] Add support for virNetworkPort object and APIs
> On 18 Jun 2019, at 17:43, Daniel P. Berrangé wrote:
>
> Signed-off-by: Daniel P. Berrangé
> ---
> network.go | 80 ++
> network_port.go | 233
> network_port_compat.h | 67
> network_port_wrapper.go | 197 +
> network_port_wrapper.h | 79 ++
> network_wrapper.go | 73 +
> network_wrapper.h | 23
> 7 files changed, 752 insertions(+)
> create mode 100644 network_port.go
> create mode 100644 network_port_compat.h
> create mode 100644 network_port_wrapper.go
> create mode 100644 network_port_wrapper.h
>
> diff --git a/network.go b/network.go
> index 99954aa..a0bc361 100644
> --- a/network.go
> +++ b/network.go
> @@ -34,6 +34,7 @@ package libvirt
> import "C"
>
> import (
> + "fmt"
> "reflect"
> "time"
> "unsafe"
> @@ -333,3 +334,82 @@ func (n *Network) GetDHCPLeases() ([]NetworkDHCPLease,
> error) {
> C.free(unsafe.Pointer(cLeases))
> return leases, nil
> }
> +
> +// See also
> https://libvirt.org/html/libvirt-libvirt-network.html#virNetworkPortLookupByUUIDString
> +func (n *Network) LookupNetworkPortByUUIDString(uuid string) (*NetworkPort,
> error) {
> + if C.LIBVIR_VERSION_NUMBER < 5005000 {
> + return nil,
> makeNotImplementedError("virNetworkPortLookupByUUIDString")
> + }
> +
> + cUuid := C.CString(uuid)
> + defer C.free(unsafe.Pointer(cUuid))
> + var err C.virError
> + ptr := C.virNetworkPortLookupByUUIDStringWrapper(n.ptr, cUuid, )
> + if ptr == nil {
> + return nil, makeError()
> + }
> + return {ptr: ptr}, nil
> +}
> +
> +// See also
> https://libvirt.org/html/libvirt-libvirt-network.html#virNetworkPortLookupByUUID
> +func (n *Network) LookupNetworkPortByUUID(uuid []byte) (*NetworkPort, error)
> {
> + if C.LIBVIR_VERSION_NUMBER < 5005000 {
> + return nil,
> makeNotImplementedError("virNetworkPortLookupByUUID")
> + }
> +
> + if len(uuid) != C.VIR_UUID_BUFLEN {
> + return nil, fmt.Errorf("UUID must be exactly %d bytes in size",
> + int(C.VIR_UUID_BUFLEN))
> + }
> + cUuid := make([]C.uchar, C.VIR_UUID_BUFLEN)
> + for i := 0; i < C.VIR_UUID_BUFLEN; i++ {
> + cUuid[i] = C.uchar(uuid[i])
> + }
> + var err C.virError
> + ptr := C.virNetworkPortLookupByUUIDWrapper(n.ptr, [0], )
> + if ptr == nil {
> + return nil, makeError()
> + }
> + return {ptr: ptr}, nil
> +}
> +
> +// See also
> https://libvirt.org/html/libvirt-libvirt-network.html#virNetworkPortCreateXML
> +func (n *Network) PortCreateXML(xmlConfig string, flags uint) (*NetworkPort,
> error) {
> + if C.LIBVIR_VERSION_NUMBER < 5005000 {
> + return nil, makeNotImplementedError("virNetworkPortCreateXML")
> + }
> + cXml := C.CString(string(xmlConfig))
> + defer C.free(unsafe.Pointer(cXml))
> + var err C.virError
> + ptr := C.virNetworkPortCreateXMLWrapper(n.ptr, cXml, C.uint(flags),
> )
> + if ptr == nil {
> + return nil, makeError()
> + }
> + return {ptr: ptr}, nil
> +}
> +
> +// See also
> https://libvirt.org/html/libvirt-libvirt-network.html#virNetworkListAllPorts
> +func (n *Network) ListAllPorts(flags uint) ([]NetworkPort, error) {
> + if C.LIBVIR_VERSION_NUMBER < 5005000 {
> + return []NetworkPort{},
> makeNotImplementedError("virNetworkListAllPorts")
> + }
> +
> + var cList *C.virNetworkPortPtr
> + var err C.virError
> + numPorts := C.virNetworkListAllPortsWrapper(n.ptr,
> (**C.virNetworkPortPtr)(), C.uint(flags), )
> + if numPorts == -1 {
> + return []NetworkPort{}, makeError()
> + }
> + hdr := reflect.SliceHeader{
> + Data: uintptr(unsafe.Pointer(cList)),
> + Len: int(numPorts),
> + Cap: int(numPorts),
> + }
> + var ports []NetworkPort
> + slice := *(*[]C.virNetworkPortPtr)(unsafe.Pointer())
> + for _, ptr := range slice {
> + ports = append(ports, NetworkPort{ptr})
> + }
> + C.free(unsafe.Pointer(cList))
> + return ports, nil
> +}
> diff --git a/network_port.go b/network_port.go
> new file mode 100644
> index 000..e701c2d
> --- /dev/null
> +++ b/network_port.go
> @@ -0,0 +1,233 @@
> +/*
> + * This file is part of the libvirt-go project
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> copy
> + * of this software and associated documentation files (the "Software"), to
> deal
> + * in the Software without restriction, including without limitation the
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission
Re: [libvirt] mdevctl: A shoestring mediated device management and persistence utility
> On 14 Jun 2019, at 16:23, Alex Williamson wrote: > > On Fri, 14 Jun 2019 11:54:42 +0200 > Christophe de Dinechin wrote: > >> That is true irrespective of the usage, isn’t it? In other words, when you >> invoke `mdevctl create-mdev`, you assert “I own that specific parent/type”. >> At least, that’s how I read the way the script behaves today. Whether you >> invoke uuidgen inside or outside the script does not change that assertion >> (at least with today’s code). > > What gives you this impression? That your code does nothing to avoid any race today? Maybe I was confused with the existing `uuidgen` example in you README, but it looks to me like the usage model involves much more than just create-mdev, and that any race that might exist is not in create-mdev itself (or in uuidgen for that matter). > Where is the parent/type ownership implied? I did not imply it, but I read some concern about ownership on your part in "they need to guess that an mdev device with the same parent and type is *theirs*.” (emphasis mine) I personally see no change on the “need to guess” implied by the fact that you run uuidgen inside the script, so that’s why I tried to guess what you meant. > The intended semantics are > "try to create this type of device under this parent”. Agreed. Which is why I don’t see why trying to create with some new UUID introduces any race (as long as the script prints out that UUID, which I admit my patch entirely failed to to) > >>> How do you resolve two instances of this happening in parallel and both >>> coming to the same conclusion which is their device. If a user wants >>> this sort of headache they can call mdevctl with `uuidgen` but I don't >>> think we should encourage it further. >> >> I agree there is a race, but if anything, having a usage where you don’t >> pass the UUID on the command line is a step in the right direction. >> It leaves the door open for the create-mdev script to do smarter things, >> like deferring the allocation of the mdevs to an entity that has slightly >> more knowledge of the global system state than uuidgen. > > A user might (likely) require a specific uuid to match their VM > configuration. I can only think of very niche use cases where a user > doesn't care what uuid they get. They do care. But I typically copy-paste my UUIDs, and then 1. copy-pasting at the end is always faster than between the command and other arguments (3-args case). 2. copy-pasting the output of the previous command is faster than having one extra step where I need to copy the same thing twice (2-args case). So to me, if the script is intended to be used by humans, my proposal makes it slightly more comfortable to use. Nothing more. > >> In other words, in my mind, `mdevctl create-mdev parent type` does not >> imply “this will use uuidgen” but rather, if anything, implies “this will do >> the >> right thing to prevent the race in the future, even if that’s more complex >> than just calling uuidgen”. > > What race are you trying to prevent, uuid collision? Of course not ;-) I only added the part of the discussion below trying to figure out what race you were seeing that was present only with my proposed changes. I (apparently incorrectly) supposed that you had some kind of mdev management within the script in mind. Obviously I misinterpreted. That will teach me to guess when I don’t understand instead of just ask… > >> However, I believe that this means we should reorder the args further. >> I would suggest something like: >> >> mdevctl create-mdev [ []] >> >> where > > Absolutely not, now you've required mdevctl to implement policy in mdev > placement. No, I’m not requiring it. I’m leaving the door open if one day, say, we decide to have libvirt tell us about the placement. That usage needs not go in right away, I marked it as “(future)”. Basically, all I’m saying is that since it’s early, we can reorder the arguments so that the one you are most likely to change when you reuse the command are the one that are last on the command-line, so that it makes editing or copy-pasting easier. There isn’t more to it, and that’s why I still do not see any new race introduced by that change. > mdevctl follows the unix standard, do one thing and do it > well. If someone wants to layer placement policy on top of mdevctl, > great, but let's not impose that within mdevctl. I’m not imposing anything (I believe). I was only trying to guess where you saw things going that would imply there was a race with my proposal that was not there without :-) > >> 1 arg means you let mdevctl choose the parent device for you (future) >> (e.g. I want a VGPU of this type, I don’t really care w
Re: [libvirt] mdevctl: A shoestring mediated device management and persistence utility
> On 24 May 2019, at 01:20, Alex Williamson wrote: > > Hi, > > Currently mediated device management, much like SR-IOV VF management, > is largely left as an exercise for the user. This is an attempt to > provide something and see where it goes. I doubt we'll solve > everyone's needs on the first pass, but maybe we'll solve enough and > provide helpers for the rest. Without further ado, I'll point to what > I have so far: > > https://github.com/awilliam/mdevctl While it’s still early, what about : mdevctl create-mdev [] where if the mdev-uuid is missing, you just run uuidgen within the script? I sent a small PR in case you think it makes sense. Thanks, Christophe > > This is inspired by driverctl, which is also a bash utility. mdevctl > uses udev and systemd to record and recreate mdev devices for > persistence and provides a command line utility for querying, listing, > starting, stopping, adding, and removing mdev devices. Currently, for > better or worse, it considers anything created to be persistent. I can > imagine a global configuration option that might disable this and > perhaps an autostart flag per mdev device, such that mdevctl might > simply "know" about some mdevs but not attempt to create them > automatically. Clearly command line usage help, man pages, and > packaging are lacking as well, release early, release often, plus this > is a discussion starter to see if perhaps this is sufficient to meet > some needs. > > Originally I thought about making a utility to manage both mdev and > SR-IOV VFs all in one, but it seemed more natural to start here > (besides, I couldn't think of a good name for the combined utility). > If this seems useful, maybe I'll start on a vfctl for SR-IOV and we'll > see whether they have enough synergy to become one. > > It would be really useful if s390 folks could help me understand > whether it's possible to glean all the information necessary to > recreate a ccw or ap mdev device from sysfs. I expect the file where > we currently only store the mdev_type to evolve into something that > includes more information to facilitate more complicated devices. For > now I make no claims to maintaining compatibility of recorded mdev > devices, it will absolutely change, but I didn't want to get bogged > down in making sure I don't accidentally source a root kit hidden in an > mdev config file. > > I'm also curious how or if libvirt or openstack might use this. If > nothing else, it makes libvirt hook scripts easier to write, especially > if we add an option not to autostart mdevs, or if users don't mind > persistent mdevs, maybe there's nothing more to do. > > BTW, feel free to clean up by bash, I'm a brute force and ignorance > shell coder ;) Thanks, > > Alex -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] mdevctl: A shoestring mediated device management and persistence utility
> On 13 Jun 2019, at 18:35, Alex Williamson wrote: > > On Thu, 13 Jun 2019 18:17:53 +0200 > Christophe de Dinechin wrote: > >>> On 24 May 2019, at 01:20, Alex Williamson >>> wrote: >>> >>> Hi, >>> >>> Currently mediated device management, much like SR-IOV VF management, >>> is largely left as an exercise for the user. This is an attempt to >>> provide something and see where it goes. I doubt we'll solve >>> everyone's needs on the first pass, but maybe we'll solve enough and >>> provide helpers for the rest. Without further ado, I'll point to what >>> I have so far: >>> >>> https://github.com/awilliam/mdevctl >> >> While it’s still early, what about : >> >> mdevctl create-mdev [] >> >> where if the mdev-uuid is missing, you just run uuidgen within the script? >> >> I sent a small PR in case you think it makes sense. > > It sounds racy. If the user doesn't provide the UUID then they need to > guess that an mdev device with the same parent and type is theirs. That is true irrespective of the usage, isn’t it? In other words, when you invoke `mdevctl create-mdev`, you assert “I own that specific parent/type”. At least, that’s how I read the way the script behaves today. Whether you invoke uuidgen inside or outside the script does not change that assertion (at least with today’s code). > How do you resolve two instances of this happening in parallel and both > coming to the same conclusion which is their device. If a user wants > this sort of headache they can call mdevctl with `uuidgen` but I don't > think we should encourage it further. I agree there is a race, but if anything, having a usage where you don’t pass the UUID on the command line is a step in the right direction. It leaves the door open for the create-mdev script to do smarter things, like deferring the allocation of the mdevs to an entity that has slightly more knowledge of the global system state than uuidgen. In other words, in my mind, `mdevctl create-mdev parent type` does not imply “this will use uuidgen” but rather, if anything, implies “this will do the right thing to prevent the race in the future, even if that’s more complex than just calling uuidgen”. However, I believe that this means we should reorder the args further. I would suggest something like: mdevctl create-mdev [ []] where 1 arg means you let mdevctl choose the parent device for you (future) (e.g. I want a VGPU of this type, I don’t really care where it comes from) 2 args mean you want that specific type/parent combination 3 args mean you assert you own that device That also implies that mdevctl create-mdev should output what it allocated so that some higher-level software can tell “OK, that’s the instance I got”. > BTW, I've moved the project to https://github.com/mdevctl/mdevctl, the > latest commit in the tree above makes that change, I've also updated > the description on my repo to point to the new location. Thanks, Done. > > Alex > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [PATCH 1/2] vfio/mdev: add version field as mandatory attribute for mdev device
> On 23 Apr 2019, at 12:39, Daniel P. Berrangé wrote: > > On Fri, Apr 19, 2019 at 04:35:04AM -0400, Yan Zhao wrote: >> device version attribute in mdev sysfs is used by user space software >> (e.g. libvirt) to query device compatibility for live migration of VFIO >> mdev devices. This attribute is mandatory if a mdev device supports live >> migration. >> >> It consists of two parts: common part and vendor proprietary part. >> common part: 32 bit. lower 16 bits is vendor id and higher 16 bits >> identifies device type. e.g., for pci device, it is >> "pci vendor id" | (VFIO_DEVICE_FLAGS_PCI << 16). >> vendor proprietary part: this part is varied in length. vendor driver can >> specify any string to identify a device. >> >> When reading this attribute, it should show device version string of the >> device of type . If a device does not support live migration, it >> should return errno. >> When writing a string to this attribute, it returns errno for >> incompatibility or returns written string length in compatibility case. >> If a device does not support live migration, it always returns errno. >> >> For user space software to use: >> 1. >> Before starting live migration, user space software first reads source side >> mdev device's version. e.g. >> "#cat \ >> /sys/bus/pci/devices/\:00\:02.0/5ac1fb20-2bbf-4842-bb7e-36c58c3be9cd/mdev_type/version" >> 00028086-193b-i915-GVTg_V5_4 >> >> 2. >> Then, user space software writes the source side returned version string >> to device version attribute in target side, and checks the return value. >> If a negative errno is returned in the target side, then mdev devices in >> source and target sides are not compatible; >> If a positive number is returned and it equals to the length of written >> string, then the two mdev devices in source and target side are compatible. >> e.g. >> (a) compatibility case >> "# echo 00028086-193b-i915-GVTg_V5_4 > >> /sys/bus/pci/devices/\:00\:02.0/882cc4da-dede-11e7-9180-078a62063ab1/mdev_type/version" >> >> (b) incompatibility case >> "#echo 00028086-193b-i915-GVTg_V5_1 > >> /sys/bus/pci/devices/\:00\:02.0/882cc4da-dede-11e7-9180-078a62063ab1/mdev_type/version" >> -bash: echo: write error: Invalid argument > > What you have written here seems to imply that each mdev type is able to > support many different versions at the same time. Writing a version into > this sysfs file then chooses which of the many versions to actually use. > > This is good as it allows for live migration across driver software upgrades. > > A mgmt application may well want to know what versions are supported for an > mdev type *before* starting a migration. A mgmt app can query all the 100's > of hosts it knows and thus figure out which are valid to use as the target > of a migration. > > IOW, we want to avoid the ever hitting the incompatibility case in the > first place, by only choosing to migrate to a host that we know is going > to be compatible. > > This would need some kind of way to report the full list of supported > versions against the mdev supported types on the host. > > >> 3. if two mdev devices are compatible, user space software can start >> live migration, and vice versa. >> >> Note: if a mdev device does not support live migration, it either does >> not provide a version attribute, or always returns errno when its version >> attribute is read/written. >> >> Cc: Alex Williamson >> Cc: Erik Skultety >> Cc: "Dr. David Alan Gilbert" >> Cc: Cornelia Huck >> Cc: "Tian, Kevin" >> Cc: Zhenyu Wang >> Cc: "Wang, Zhi A" >> Cc: Neo Jia >> Cc: Kirti Wankhede >> >> Signed-off-by: Yan Zhao >> --- >> Documentation/vfio-mediated-device.txt | 36 ++ >> samples/vfio-mdev/mbochs.c | 17 >> samples/vfio-mdev/mdpy.c | 16 >> samples/vfio-mdev/mtty.c | 16 >> 4 files changed, 85 insertions(+) >> >> diff --git a/Documentation/vfio-mediated-device.txt >> b/Documentation/vfio-mediated-device.txt >> index c3f69bcaf96e..bc28471c0667 100644 >> --- a/Documentation/vfio-mediated-device.txt >> +++ b/Documentation/vfio-mediated-device.txt >> @@ -202,6 +202,7 @@ Directories and files under the sysfs for Each Physical >> Device >> | | |--- available_instances >> | | |--- device_api >> | | |--- description >> + | | |--- version >> | | |--- [devices] >> | |--- [] >> | | |--- create >> @@ -209,6 +210,7 @@ Directories and files under the sysfs for Each Physical >> Device >> | | |--- available_instances >> | | |--- device_api >> | | |--- description >> + | | |--- version >> | | |--- [devices] >> | |--- [] >> | |--- create >> @@ -216,6 +218,7 @@ Directories and files under the sysfs for Each Physical >> Device >> | |--- available_instances >> | |--- device_api >> | |---
Re: [libvirt] [PATCH] Use port range 5901-5999 if not set in qemu.conf, avoid port 5900
> On 21 Apr 2017, at 11:26, Daniel P. Berrange <berra...@redhat.com> wrote:
>
> On Fri, Apr 21, 2017 at 11:20:11AM +0200, Peter Krempa wrote:
>> On Fri, Apr 21, 2017 at 11:02:36 +0200, Christophe de Dinechin wrote:
>>> In order to avoid conflict with the default port (5900) for host VNC server
>>> (vino-server for example), or to conflict with X11 (starting at port 6000),
>>> restrict range of ports to 5901-5999 unless explicitly specified in
>>> qemu.conf.
>>>
>>> On the other hand, if port range is explicitly specified in qemu.conf,
>>> there is no reason not to allow ports 1024-5900 (system ports are below
>>> 1024).
>>>
>>> Addresses https://bugzilla.redhat.com/show_bug.cgi?id=1442235
>>>
>>> Signed-off-by: Christophe de Dinechin <dinec...@redhat.com>
>>> ---
>>> src/qemu/qemu_conf.c | 12 +---
>>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
>>> index 1b704da..07f3177 100644
>>> --- a/src/qemu/qemu_conf.c
>>> +++ b/src/qemu/qemu_conf.c
>>> @@ -65,9 +65,15 @@ VIR_LOG_INIT("qemu.qemu_conf");
>>> * This limitation is mentioned in qemu.conf, so bear in mind that the
>>> * configuration file should reflect any changes made to these values.
>>> */
>>> -#define QEMU_REMOTE_PORT_MIN 5900
>>> +
>>> +// Range of available ports - Avoid ports below 1024 (system ports)
>>
>> We don't use single line comments.
>>
>>> +#define QEMU_REMOTE_PORT_MIN 1024
>>
>> I don't think it's possible to use ports < 5900 due to the weird way you
>> specify VNC "screens".
>>
>>> #define QEMU_REMOTE_PORT_MAX 65535
>>>
>>> +// Default min and max if not configured in qemu.conf
>>> +#define QEMU_REMOTE_PORT_MIN_DEFAULT 5901
>>> +#define QEMU_REMOTE_PORT_MAX_DEFAULT 5999
>>> +
>>> #define QEMU_WEBSOCKET_PORT_MIN 5700
>>> #define QEMU_WEBSOCKET_PORT_MAX 65535
>>>
>>> @@ -283,8 +289,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool
>>> privileged)
>>>
>>> #undef SET_TLS_X509_CERT_DEFAULT
>>>
>>> -cfg->remotePortMin = QEMU_REMOTE_PORT_MIN;
>>> -cfg->remotePortMax = QEMU_REMOTE_PORT_MAX;
>>> +cfg->remotePortMin = QEMU_REMOTE_PORT_MIN_DEFAULT;
>>> +cfg->remotePortMax = QEMU_REMOTE_PORT_MAX_DEFAULT;
>>
>> This can conflict basically with everything running on non-system
>> ports. Additionally, we tend to shoot to support 4k VMs which would
>> prevent to do so.
>
> I'm not really convinced we want to change anything here. Typical desktop
> virt usage via GNOME Boxes would be using session mode libvirt and so any
> auto-started VMs would happen after Vino has already claimed port 5900.
There are too many limitations with session-mode VMs, like not being able to
connect to them easily from outside.
I also disagree with you assessment of “typical”. I have three auto-start VMs
for my build ring (basically, Linux, Windows and macOS). Because they are part
of a build ring, I need to be able to connect to them from outside (Jenkins
launches jobs on them). I don’t see that setup as outlandish for the typical
open-source developer.
> The privileged libvirtd is targetted at server virt usage where you would
> not have a desktop installed on the same machine. In remaining scenarios
> its already possible to just edit qemu.conf to tweak local setup as desired.
My objection is to require tweaks to qemu.conf from the less experienced users
so that the more advanced sysadmins don’t have to…
Regards,
Christophe
> Regards,
> Daniel
> --
> |: https://berrange.com <https://berrange.com/> -o-
> https://www.flickr.com/photos/dberrange
> <https://www.flickr.com/photos/dberrange> :|
> |: https://libvirt.org <https://libvirt.org/> -o-
> https://fstop138.berrange.com <https://fstop138.berrange.com/> :|
> |: https://entangle-photo.org <https://entangle-photo.org/>-o-
> https://www.instagram.com/dberrange <https://www.instagram.com/dberrange> :|
>
> --
> libvir-list mailing list
> libvir-list@redhat.com <mailto:libvir-list@redhat.com>
> https://www.redhat.com/mailman/listinfo/libvir-list
> <https://www.redhat.com/mailman/listinfo/libvir-list>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Use port range 5901-5999 if not set in qemu.conf, avoid port 5900
> On 21 Apr 2017, at 11:20, Peter Krempa <pkre...@redhat.com> wrote:
>
> On Fri, Apr 21, 2017 at 11:02:36 +0200, Christophe de Dinechin wrote:
>> In order to avoid conflict with the default port (5900) for host VNC server
>> (vino-server for example), or to conflict with X11 (starting at port 6000),
>> restrict range of ports to 5901-5999 unless explicitly specified in
>> qemu.conf.
>>
>> On the other hand, if port range is explicitly specified in qemu.conf,
>> there is no reason not to allow ports 1024-5900 (system ports are below
>> 1024).
>>
>> Addresses https://bugzilla.redhat.com/show_bug.cgi?id=1442235
>>
>> Signed-off-by: Christophe de Dinechin <dinec...@redhat.com>
>> ---
>> src/qemu/qemu_conf.c | 12 +---
>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
>> index 1b704da..07f3177 100644
>> --- a/src/qemu/qemu_conf.c
>> +++ b/src/qemu/qemu_conf.c
>> @@ -65,9 +65,15 @@ VIR_LOG_INIT("qemu.qemu_conf");
>> * This limitation is mentioned in qemu.conf, so bear in mind that the
>> * configuration file should reflect any changes made to these values.
>> */
>> -#define QEMU_REMOTE_PORT_MIN 5900
>> +
>> +// Range of available ports - Avoid ports below 1024 (system ports)
>
> We don't use single line comments.
OK. I will change that. Ah, 1990s nostalgia! ;-)
>
>> +#define QEMU_REMOTE_PORT_MIN 1024
>
> I don't think it's possible to use ports < 5900 due to the weird way you
> specify VNC "screens”.
I considered that. But there are many use cases where you specify port
manually, so this could be a feature. For example, you could manually assign
ports in the range 5900-5999 for “front-end” VMs, with a convenient display
number, but let “back-end VMs” be assigned automatically elsewhere, so that
they don’t steal ports. That means administering a back-end VM would require
you to use an explicit port number, but for normal users, they would access a
front-end VM and just use a display number.
I don’t know if that makes sense or not. This could be a separate patch too.
It’s not really the same problem after all.
>
>> #define QEMU_REMOTE_PORT_MAX 65535
>>
>> +// Default min and max if not configured in qemu.conf
>> +#define QEMU_REMOTE_PORT_MIN_DEFAULT 5901
>> +#define QEMU_REMOTE_PORT_MAX_DEFAULT 5999
>> +
>> #define QEMU_WEBSOCKET_PORT_MIN 5700
>> #define QEMU_WEBSOCKET_PORT_MAX 65535
>>
>> @@ -283,8 +289,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool
>> privileged)
>>
>> #undef SET_TLS_X509_CERT_DEFAULT
>>
>> -cfg->remotePortMin = QEMU_REMOTE_PORT_MIN;
>> -cfg->remotePortMax = QEMU_REMOTE_PORT_MAX;
>> +cfg->remotePortMin = QEMU_REMOTE_PORT_MIN_DEFAULT;
>> +cfg->remotePortMax = QEMU_REMOTE_PORT_MAX_DEFAULT;
>
> This can conflict basically with everything running on non-system
> ports.
This default range is 5901-5999, so this would not conflict with anything
running on non-system ports, unless you specify that in qemu.conf (but then you
know what you are doing)
> Additionally, we tend to shoot to support 4k VMs which would prevent to do so.
I’m not sure I understood your comment. Do you mean 4000 VMs running
concurrently, or 4K display? For the latter, I don’t see how port assignment
matters, so I deduce you talk about running more than 99 VMs simultaneously.
Can you do that without tweaks to qemu.conf? It’s really a trade-off.
Experienced sysadmins will have to extend available ports in qemu.conf (and I
place anybody running more than 100 VMs on a single system in that category).
Regular users won’t have conflicts with vino-server or (if über-unlucky) X11.
That seems like a reasonable trade-off to me.
BTW, if you really run 4K VMs on a single host, assigning one port per VM for
display will become a problem anyway. There are only 64K ports available, so
using 4K just for displays does not scale well, not even counting the ports you
might need for the VM workloads themselves. It might be time to think about
adding a VM ID directly in the protocol and having multiple connections to the
same port.
Christophe--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] Use port range 5901-5999 if not set in qemu.conf, avoid port 5900
In order to avoid conflict with the default port (5900) for host VNC server
(vino-server for example), or to conflict with X11 (starting at port 6000),
restrict range of ports to 5901-5999 unless explicitly specified in qemu.conf.
On the other hand, if port range is explicitly specified in qemu.conf,
there is no reason not to allow ports 1024-5900 (system ports are below 1024).
Addresses https://bugzilla.redhat.com/show_bug.cgi?id=1442235
Signed-off-by: Christophe de Dinechin <dinec...@redhat.com>
---
src/qemu/qemu_conf.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 1b704da..07f3177 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -65,9 +65,15 @@ VIR_LOG_INIT("qemu.qemu_conf");
* This limitation is mentioned in qemu.conf, so bear in mind that the
* configuration file should reflect any changes made to these values.
*/
-#define QEMU_REMOTE_PORT_MIN 5900
+
+// Range of available ports - Avoid ports below 1024 (system ports)
+#define QEMU_REMOTE_PORT_MIN 1024
#define QEMU_REMOTE_PORT_MAX 65535
+// Default min and max if not configured in qemu.conf
+#define QEMU_REMOTE_PORT_MIN_DEFAULT 5901
+#define QEMU_REMOTE_PORT_MAX_DEFAULT 5999
+
#define QEMU_WEBSOCKET_PORT_MIN 5700
#define QEMU_WEBSOCKET_PORT_MAX 65535
@@ -283,8 +289,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool
privileged)
#undef SET_TLS_X509_CERT_DEFAULT
-cfg->remotePortMin = QEMU_REMOTE_PORT_MIN;
-cfg->remotePortMax = QEMU_REMOTE_PORT_MAX;
+cfg->remotePortMin = QEMU_REMOTE_PORT_MIN_DEFAULT;
+cfg->remotePortMax = QEMU_REMOTE_PORT_MAX_DEFAULT;
cfg->webSocketPortMin = QEMU_WEBSOCKET_PORT_MIN;
cfg->webSocketPortMax = QEMU_WEBSOCKET_PORT_MAX;
--
2.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
