Re: [libvirt] [PATCH v3 1/4] lxc: allow defining environment variables
On Mon, 2017-06-26 at 16:29 +0100, Daniel P. Berrange wrote: > On Mon, Jun 26, 2017 at 11:40:57AM +0200, Cédric Bosdonnat wrote: > > When running an application container, setting environment variables > > could be important. > > > > The newly introduced tag in domain configuration will allow > > setting environment variables to the init program. > > --- > > docs/formatdomain.html.in| 5 + > > docs/schemas/domaincommon.rng| 10 ++ > > src/conf/domain_conf.c | 38 > > > > src/conf/domain_conf.h | 8 > > src/lxc/lxc_container.c | 5 + > > tests/lxcxml2xmldata/lxc-initenv.xml | 30 > > tests/lxcxml2xmltest.c | 1 + > > 7 files changed, 97 insertions(+) > > create mode 100644 tests/lxcxml2xmldata/lxc-initenv.xml > > Reviewed-by: Daniel P. Berrange Thanks. I'll push it post freeze. Did you have some time to review the other ones of the series? Regards, -- Cedric -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 3/3] Add TLS support for Veritas HyperScale (VxHS) block device protocol
On Fri, Jun 30, 2017 at 2:21 PM, John Ferlan wrote:
> [...]
>
>>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>>> index 8e00782..99bc94f 100644
>>> --- a/src/qemu/qemu_command.c
>>> +++ b/src/qemu/qemu_command.c
>>> @@ -931,6 +931,68 @@ qemuBuildGlusterDriveJSON(virStorageSourcePtr src)
>>> return ret;
>>> }
>>>
>>> +/* qemuBuildDiskVxHSTLSinfoCommandLine:
>>> + * @cmd: Pointer to the command string
>>> + * @cfg: Pointer to the qemu driver config
>>> + * @disk: The disk we are processing
>>> + * @qemuCaps: qemu capabilities object
>>> + *
>>> + * Check if the VxHS disk meets all the criteria to enable TLS.
>>> + * If yes, add a new TLS object and mention it's ID on the disk
>>> + * command line.
>>> + *
>>> + * Returns 0 on success, -1 w/ error on some sort of failure.
>>> + */
>>> +static int
>>> +qemuBuildDiskVxHSTLSinfoCommandLine(virCommandPtr cmd,
>>> +virQEMUDriverConfigPtr cfg,
>>> +virDomainDiskDefPtr disk,
>>> +virQEMUCapsPtr qemuCaps)
>>> +{
>>> +int ret = 0;
>>> +
>>> +if (cfg->vxhsTLS == true && disk->src->haveTLS !=
>>> VIR_TRISTATE_BOOL_NO) {
>>> +disk->src->addTLS = true;
>>> +ret = qemuBuildTLSx509CommandLine(cmd, cfg->vxhsTLSx509certdir,
>>> + false,
>>> + true,
>>> + false,
>>> + "vxhs",
>>
>> This argument can't be a constant. This is the alias for the certificate
>> object.
>>
>> Otherwise you'd had to make sure that there's only one such object,
>> which obviously would make sense here, since (if you don't hotplug disks
>> after libvirtd restart) the TLS credentials are the same for this disk.
>>
>> In case of hotplug though you need to make sure that the TLS object is
>> removed with the last disk and added if any other disk needing TLS is
>> added.
>>
>
> So at least the conversation we had last week now makes a bit more sense
> w/r/t the call to qemuBuildTLSx509CommandLine for chardevTLSx509certdir.
> As I look at that code now quickly, although having multiple
> tls-cert-x509 objects for each chardev isn't necessary, it does "work"
> or start qemu because each would have a different alias (@charAlias is
> uniquely generated via qemuAliasChardevFromDevAlias). Theoretically
> speaking two objects wouldn't be required, except for the problem that
> hotunplug would have to be made aware and we'd have to keep track of
> when the last one was removed. So leaving with each having their own
> object is the way the code will stay.
>
> So given all that - your alias creation is going to have to contain that
> uuid or you are going to have to figure out a way to just have one
> object which each disk uses. You'll have to scan the disks looking to
> determine if any of the vxhs ones have tls and if so, break to add the
> object. Then add the 'tls-creds=$object_alias_id'.
>
This makes sense. Will get back with the diff that could achieve this.
> BTW: if you haven't already, read Dan Berrange's blog on TLS:
>
> https://www.berrange.com/posts/2016/04/01/improving-qemu-security-part-2-generic-tls-support/
>
> that's a link to page 2, read/scan the remaining 5 blogs as well. Some
> of the exact qemu syntax has changed since the blog was written, but the
> description is really what helps the frame of reference.
>
Will do. Thanks!
>>> + qemuCaps);
>>> +} else if (cfg->vxhsTLS == false &&
>>> + disk->src->haveTLS == VIR_TRISTATE_BOOL_YES) {
>>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> + _("Please enable VxHS specific TLS options in the
>>> qemu "
>>> + "conf file before using TLS in VxHS device domain
>>> "
>>> + "specification"));
>>> +ret = -1;
>>> +}
>>> +
>>> +return ret;
>>> +}
>
> [...]
>
>>> diff --git
>>> a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args
>>>
>>> b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args
>>> new file mode 100644
>>> index 000..960960d
>>> --- /dev/null
>>> +++
>>> b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args
>>
>> [this file has same mistake as the one below]
>>
>>> diff --git
>>> a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args.new
>>>
>>> b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args.new
>>> new file mode 100644
>>> index 000..960960d
>>> --- /dev/null
>>> +++
>>> b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args.new
>>> @@ -0,0 +1,41 @@
>>> +LC_ALL=C \
>>> +PATH=/bin \
>>> +HOME=/home/test \
>>> +USER=test \
>>> +LOGNA
Re: [libvirt] [PATCH v4 3/3] Add TLS support for Veritas HyperScale (VxHS) block device protocol
[...]
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 8e00782..99bc94f 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -931,6 +931,68 @@ qemuBuildGlusterDriveJSON(virStorageSourcePtr src)
>> return ret;
>> }
>>
>> +/* qemuBuildDiskVxHSTLSinfoCommandLine:
>> + * @cmd: Pointer to the command string
>> + * @cfg: Pointer to the qemu driver config
>> + * @disk: The disk we are processing
>> + * @qemuCaps: qemu capabilities object
>> + *
>> + * Check if the VxHS disk meets all the criteria to enable TLS.
>> + * If yes, add a new TLS object and mention it's ID on the disk
>> + * command line.
>> + *
>> + * Returns 0 on success, -1 w/ error on some sort of failure.
>> + */
>> +static int
>> +qemuBuildDiskVxHSTLSinfoCommandLine(virCommandPtr cmd,
>> +virQEMUDriverConfigPtr cfg,
>> +virDomainDiskDefPtr disk,
>> +virQEMUCapsPtr qemuCaps)
>> +{
>> +int ret = 0;
>> +
>> +if (cfg->vxhsTLS == true && disk->src->haveTLS !=
>> VIR_TRISTATE_BOOL_NO) {
>> +disk->src->addTLS = true;
>> +ret = qemuBuildTLSx509CommandLine(cmd, cfg->vxhsTLSx509certdir,
>> + false,
>> + true,
>> + false,
>> + "vxhs",
>
> This argument can't be a constant. This is the alias for the certificate
> object.
>
> Otherwise you'd had to make sure that there's only one such object,
> which obviously would make sense here, since (if you don't hotplug disks
> after libvirtd restart) the TLS credentials are the same for this disk.
>
> In case of hotplug though you need to make sure that the TLS object is
> removed with the last disk and added if any other disk needing TLS is
> added.
>
So at least the conversation we had last week now makes a bit more sense
w/r/t the call to qemuBuildTLSx509CommandLine for chardevTLSx509certdir.
As I look at that code now quickly, although having multiple
tls-cert-x509 objects for each chardev isn't necessary, it does "work"
or start qemu because each would have a different alias (@charAlias is
uniquely generated via qemuAliasChardevFromDevAlias). Theoretically
speaking two objects wouldn't be required, except for the problem that
hotunplug would have to be made aware and we'd have to keep track of
when the last one was removed. So leaving with each having their own
object is the way the code will stay.
So given all that - your alias creation is going to have to contain that
uuid or you are going to have to figure out a way to just have one
object which each disk uses. You'll have to scan the disks looking to
determine if any of the vxhs ones have tls and if so, break to add the
object. Then add the 'tls-creds=$object_alias_id'.
BTW: if you haven't already, read Dan Berrange's blog on TLS:
https://www.berrange.com/posts/2016/04/01/improving-qemu-security-part-2-generic-tls-support/
that's a link to page 2, read/scan the remaining 5 blogs as well. Some
of the exact qemu syntax has changed since the blog was written, but the
description is really what helps the frame of reference.
>> + qemuCaps);
>> +} else if (cfg->vxhsTLS == false &&
>> + disk->src->haveTLS == VIR_TRISTATE_BOOL_YES) {
>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("Please enable VxHS specific TLS options in the
>> qemu "
>> + "conf file before using TLS in VxHS device domain "
>> + "specification"));
>> +ret = -1;
>> +}
>> +
>> +return ret;
>> +}
[...]
>> diff --git
>> a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args
>>
>> b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args
>> new file mode 100644
>> index 000..960960d
>> --- /dev/null
>> +++
>> b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args
>
> [this file has same mistake as the one below]
>
>> diff --git
>> a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args.new
>>
>> b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args.new
>> new file mode 100644
>> index 000..960960d
>> --- /dev/null
>> +++
>> b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args.new
>> @@ -0,0 +1,41 @@
>> +LC_ALL=C \
>> +PATH=/bin \
>> +HOME=/home/test \
>> +USER=test \
>> +LOGNAME=test \
>> +QEMU_AUDIO_DRV=none \
>> +/usr/bin/qemu-system-x86_64 \
>> +-name QEMUGuest1 \
>> +-S \
>> +-M pc \
>> +-cpu qemu32 \
>> +-m 214 \
>> +-smp 1,sockets=1,cores=1,threads=1 \
>> +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
>> +-nographic \
>> +-nodefaults \
>> +-monitor
Re: [libvirt] [PATCH v4 2/3] conf: Introduce TLS options for VxHS block device clients
On Fri, Jun 30, 2017 at 1:29 PM, John Ferlan wrote: > > > On 06/29/2017 10:02 PM, Ashish Mittal wrote: >> From: Ashish Mittal >> >> Add a new TLS X.509 certificate type - "vxhs". This will handle the >> creation of a TLS certificate capability for properly configured >> VxHS network block device clients. >> >> Signed-off-by: Ashish Mittal >> --- >> Changelog: >> >> (1) Add two new options in /etc/libvirt/qemu.conf >> to control TLS behavior with VxHS block devices >> "vxhs_tls" and "vxhs_tls_x509_cert_dir". >> (2) Setting "vxhs_tls=1" in /etc/libvirt/qemu.conf will enable >> TLS for VxHS block devices. >> (3) "vxhs_tls_x509_cert_dir" can be set to the full path where the >> TLS certificates and keys are saved. If this value is missing, >> the "default_tls_x509_cert_dir" will be used instead. >> >> src/qemu/libvirtd_qemu.aug | 4 >> src/qemu/qemu.conf | 23 +++ >> src/qemu/qemu_conf.c | 7 +++ >> src/qemu/qemu_conf.h | 3 +++ >> src/qemu/test_libvirtd_qemu.aug.in | 2 ++ >> 5 files changed, 39 insertions(+) >> > > FWIW: I have another patch upstream: > > https://www.redhat.com/archives/libvir-list/2017-June/msg01341.html > > Which is making some textual and code changes to qemu.conf and > qemu_conf.c - just so you are aware. > I guess that means I might have to rebase and resolve any potential conflicts in the near future :) It was for the same reason I was hoping that the base VxHS functionality (patch 1) could be merged first. I could then add TLS support in subsequent patches. QEMU vxhs code does support using VxHS devices without TLS. > >> diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug >> index e1983d1..c19bf3a 100644 >> --- a/src/qemu/libvirtd_qemu.aug >> +++ b/src/qemu/libvirtd_qemu.aug >> @@ -115,6 +115,9 @@ module Libvirtd_qemu = >> >> let memory_entry = str_entry "memory_backing_dir" >> >> + let vxhs_entry = bool_entry "vxhs_tls" >> + | str_entry "vxhs_tls_x509_cert_dir" >> + >> (* Each entry in the config is one of the following ... *) >> let entry = default_tls_entry >> | vnc_entry >> @@ -133,6 +136,7 @@ module Libvirtd_qemu = >> | nvram_entry >> | gluster_debug_level_entry >> | memory_entry >> + | vxhs_entry >> >> let comment = [ label "#comment" . del /#[ \t]*/ "# " . store /([^ >> \t\n][^\n]*)?/ . del /\n/ "\n" ] >> let empty = [ label "#empty" . eol ] >> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf >> index e6c0832..83c2377 100644 >> --- a/src/qemu/qemu.conf >> +++ b/src/qemu/qemu.conf >> @@ -250,6 +250,29 @@ >> #chardev_tls_x509_secret_uuid = "----" >> >> >> +# Enable use of TLS encryption on the VxHS network block devices. >> +# >> +# When the VxHS network block device server is set up appropriately, >> +# x509 certificates are used for authentication between the clients >> +# (qemu processes) and the remote VxHS server. >> +# >> +# It is necessary to setup CA and issue client and server certificates >> +# before enabling this. >> +# >> +#vxhs_tls = 1 >> + >> + >> +# In order to override the default TLS certificate location for VxHS >> +# device TCP certificates, supply a valid path to the certificate directory. >> +# This is used to authenticate the VxHS block device clients to the VxHS >> +# server. >> +# >> +# If the provided path does not exist then the default_tls_x509_cert_dir >> +# path will be used. >> +# >> +#vxhs_tls_x509_cert_dir = "/etc/pki/libvirt-vxhs" >> + >> + >> # In order to override the default TLS certificate location for migration >> # certificates, supply a valid path to the certificate directory. If the >> # provided path does not exist then the default_tls_x509_cert_dir path > > In patch 3, the call to qemuBuildTLSx509CommandLine has a @verifypeer > parameter = true, thus it's always going to expect to verify the client > rather than making that determination using *_tls_x509_verify. So that > means you have to describe the environment as requiring the > client-cert.pem and client-key.pem files (like the description for default). > > Since you set the @addpasswordid to false on input, that just means the > certificate server-key.pem file cannot be encrypted - something else > that would need to be described. > > Based on these two configuration settings, that means if vxhs_tls=1, > then vxhs_tls_x509_cert_dir must be set to something other than the > default environment. IOW: both must be defined and that would need to be > validated during config read and cause failure (e.g. vxhsTLSx509certdir > != defaultTLSx509certdir > > IOW: You're essentially *requiring* someone to set this up and don't > handle the/a default environment. Hopefully this makes sense, it's > Friday afternoon and I have a TLS migraine ;-). > Thanks for that insight! I will mull over this for a bit and see how I can r
Re: [libvirt] [PATCH v4 3/3] Add TLS support for Veritas HyperScale (VxHS) block device protocol
On 06/30/2017 04:56 AM, Peter Krempa wrote: > On Fri, Jun 30, 2017 at 10:44:39 +0200, Peter Krempa wrote: >> On Thu, Jun 29, 2017 at 19:02:41 -0700, Ashish Mittal wrote: >>> From: Ashish Mittal > > [...] > >>> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng >>> index 7525a2a..909af50 100644 >>> --- a/docs/schemas/domaincommon.rng >>> +++ b/docs/schemas/domaincommon.rng >>> @@ -1622,6 +1622,11 @@ >>> >>> >>> >>> + >>> + >>> + >>> + >> >> Make this a definition for future reuse. Additionally I think that the >> TLS part should be a separate element here. Something like >> >> >> > > I forgot to finish my thought before sending. I think we want a separate > element with an attribute at this point. This allows adding other TLS > related stuff to it if such need arises. > > > > > > > > [...] > > I don't like a separate element. What do you mean by other TLS related stuff such as 'verify' or 'secret'? Those would be qemu.conf type settings - they wouldn't change on a disk by disk or domain by domain basis. Why not as a or perhaps more precisely a attribute? If you compare with others it's related to the port as I would assume would be the case for storage as well. If my understanding from the cover letter is valid, then this is how QEMU is going to communicate with some remote host/server in order to provide TLS credentials. John For comparison, other consumers of TLS and their XML: VNC: ... ... Configured only via qemu.conf AFAICT Spice: ... ... Chardev: ... ... > > > > -- > 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 v4 2/3] conf: Introduce TLS options for VxHS block device clients
On 06/29/2017 10:02 PM, Ashish Mittal wrote: > From: Ashish Mittal > > Add a new TLS X.509 certificate type - "vxhs". This will handle the > creation of a TLS certificate capability for properly configured > VxHS network block device clients. > > Signed-off-by: Ashish Mittal > --- > Changelog: > > (1) Add two new options in /etc/libvirt/qemu.conf > to control TLS behavior with VxHS block devices > "vxhs_tls" and "vxhs_tls_x509_cert_dir". > (2) Setting "vxhs_tls=1" in /etc/libvirt/qemu.conf will enable > TLS for VxHS block devices. > (3) "vxhs_tls_x509_cert_dir" can be set to the full path where the > TLS certificates and keys are saved. If this value is missing, > the "default_tls_x509_cert_dir" will be used instead. > > src/qemu/libvirtd_qemu.aug | 4 > src/qemu/qemu.conf | 23 +++ > src/qemu/qemu_conf.c | 7 +++ > src/qemu/qemu_conf.h | 3 +++ > src/qemu/test_libvirtd_qemu.aug.in | 2 ++ > 5 files changed, 39 insertions(+) > FWIW: I have another patch upstream: https://www.redhat.com/archives/libvir-list/2017-June/msg01341.html Which is making some textual and code changes to qemu.conf and qemu_conf.c - just so you are aware. > diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug > index e1983d1..c19bf3a 100644 > --- a/src/qemu/libvirtd_qemu.aug > +++ b/src/qemu/libvirtd_qemu.aug > @@ -115,6 +115,9 @@ module Libvirtd_qemu = > > let memory_entry = str_entry "memory_backing_dir" > > + let vxhs_entry = bool_entry "vxhs_tls" > + | str_entry "vxhs_tls_x509_cert_dir" > + > (* Each entry in the config is one of the following ... *) > let entry = default_tls_entry > | vnc_entry > @@ -133,6 +136,7 @@ module Libvirtd_qemu = > | nvram_entry > | gluster_debug_level_entry > | memory_entry > + | vxhs_entry > > let comment = [ label "#comment" . del /#[ \t]*/ "# " . store /([^ > \t\n][^\n]*)?/ . del /\n/ "\n" ] > let empty = [ label "#empty" . eol ] > diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf > index e6c0832..83c2377 100644 > --- a/src/qemu/qemu.conf > +++ b/src/qemu/qemu.conf > @@ -250,6 +250,29 @@ > #chardev_tls_x509_secret_uuid = "----" > > > +# Enable use of TLS encryption on the VxHS network block devices. > +# > +# When the VxHS network block device server is set up appropriately, > +# x509 certificates are used for authentication between the clients > +# (qemu processes) and the remote VxHS server. > +# > +# It is necessary to setup CA and issue client and server certificates > +# before enabling this. > +# > +#vxhs_tls = 1 > + > + > +# In order to override the default TLS certificate location for VxHS > +# device TCP certificates, supply a valid path to the certificate directory. > +# This is used to authenticate the VxHS block device clients to the VxHS > +# server. > +# > +# If the provided path does not exist then the default_tls_x509_cert_dir > +# path will be used. > +# > +#vxhs_tls_x509_cert_dir = "/etc/pki/libvirt-vxhs" > + > + > # In order to override the default TLS certificate location for migration > # certificates, supply a valid path to the certificate directory. If the > # provided path does not exist then the default_tls_x509_cert_dir path In patch 3, the call to qemuBuildTLSx509CommandLine has a @verifypeer parameter = true, thus it's always going to expect to verify the client rather than making that determination using *_tls_x509_verify. So that means you have to describe the environment as requiring the client-cert.pem and client-key.pem files (like the description for default). Since you set the @addpasswordid to false on input, that just means the certificate server-key.pem file cannot be encrypted - something else that would need to be described. Based on these two configuration settings, that means if vxhs_tls=1, then vxhs_tls_x509_cert_dir must be set to something other than the default environment. IOW: both must be defined and that would need to be validated during config read and cause failure (e.g. vxhsTLSx509certdir != defaultTLSx509certdir IOW: You're essentially *requiring* someone to set this up and don't handle the/a default environment. Hopefully this makes sense, it's Friday afternoon and I have a TLS migraine ;-). John > diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c > index 73c33d6..f3813d4 100644 > --- a/src/qemu/qemu_conf.c > +++ b/src/qemu/qemu_conf.c > @@ -280,6 +280,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool > privileged) > SET_TLS_X509_CERT_DEFAULT(spice); > SET_TLS_X509_CERT_DEFAULT(chardev); > SET_TLS_X509_CERT_DEFAULT(migrate); > +SET_TLS_X509_CERT_DEFAULT(vxhs); > > #undef SET_TLS_X509_CERT_DEFAULT > > @@ -395,6 +396,8 @@ static void virQEMUDriverConfigDispose(void *obj) > VIR_FREE(cfg->chardevTLSx509certdir); >
Re: [libvirt] [PATCH v4 0/3] Add support for Veritas HyperScale (VxHS) block device protocol
On Fri, Jun 30, 2017 at 1:07 PM, John Ferlan wrote: > > > On 06/30/2017 11:30 AM, ashish mittal wrote: >> Hi, >> >> Thanks for the review! >> >> On Fri, Jun 30, 2017 at 12:41 AM, Peter Krempa wrote: >>> On Thu, Jun 29, 2017 at 19:02:38 -0700, Ashish Mittal wrote: From: Ashish Mittal QEMU changes for VxHS (including TLS support) are already upstream. This series of patches adds support for VxHS block devices in libvirt. Patch 1 adds the base functionality for supporting VxHS protocol. Patch 2 adds two new configuration options in qemu.conf to enable TLS for VxHS devices. Patch 3 implements the main TLS functionality. >>> >>> This ordering is wrong and also your patches have a lot of stuff going >>> on at once. >>> > > Recall what I pointed out last week when you were @ Westford. Try to > find a "happy medium" between throwing everything into a few patches and > over creating patches for the sake of having really small compileable > and testable patches. It is a delicate balance... > >>> I suggest you add the TLS support (Which should be designed to be >>> generic with other protocols which may start using TLS in mind) first >>> and then start adding the rest of the stuff. >>> >> >> I'm not sure I understand this point. libvirt currently does not have >> support of VxHS devices. So I cannot add TLS support for VxHS before >> base VxHS functionality. If you mean that I should add generic TLS >> handling functionality for block device protocols, then it would >> probably just be some new variables in structures and bare-bone >> functions (1 or 2) that don't do much. None of the block devices at >> present use TLS, so even if I add generic TLS code, how would I test >> it in the same patch unit? >> > > I'm not so sure we could have generic block TLS env. IIUC, the > _tls_x509_cert_dir (or "s:dir" to the tls-creds-x509 object) is the > location for the server certificate that QEMU would present to say VxHS > server. Whereas, NBD which could use the migrate TLS environment (or > it's own I suppose, but we use it for migration). If Gluster, iSCSI, > RBD, etc. allowed the ability to use TLS instead of secrets, > then they too could conceivably have their own environment. > > This isn't a QEMU to QEMU communication, right? It's QEMU to some server > where the storage is located from whence you'll get your storage. > That is correct! In this case qemu is the client and the actual VxHS storage server is remote. The use of TLS/SSL here is to secure this communication. > John > > I'm sure if I have this wrong someone will correct me... > >>> I'll reply to some parts of the patches separately, but in this state >>> it's kind of a mess to go through, so please re-send the patch split >>> into reasonable chunks. >>> >>> Note that each patch needs to make sense and can be compiled and tested >>> individually. >>> >> >> Regards, >> Ashish >> -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 0/3] Add support for Veritas HyperScale (VxHS) block device protocol
On 06/30/2017 11:30 AM, ashish mittal wrote: > Hi, > > Thanks for the review! > > On Fri, Jun 30, 2017 at 12:41 AM, Peter Krempa wrote: >> On Thu, Jun 29, 2017 at 19:02:38 -0700, Ashish Mittal wrote: >>> From: Ashish Mittal >>> >>> QEMU changes for VxHS (including TLS support) are already upstream. >>> This series of patches adds support for VxHS block devices in libvirt. >>> >>> Patch 1 adds the base functionality for supporting VxHS protocol. >>> >>> Patch 2 adds two new configuration options in qemu.conf to enable TLS >>> for VxHS devices. >>> >>> Patch 3 implements the main TLS functionality. >> >> This ordering is wrong and also your patches have a lot of stuff going >> on at once. >> Recall what I pointed out last week when you were @ Westford. Try to find a "happy medium" between throwing everything into a few patches and over creating patches for the sake of having really small compileable and testable patches. It is a delicate balance... >> I suggest you add the TLS support (Which should be designed to be >> generic with other protocols which may start using TLS in mind) first >> and then start adding the rest of the stuff. >> > > I'm not sure I understand this point. libvirt currently does not have > support of VxHS devices. So I cannot add TLS support for VxHS before > base VxHS functionality. If you mean that I should add generic TLS > handling functionality for block device protocols, then it would > probably just be some new variables in structures and bare-bone > functions (1 or 2) that don't do much. None of the block devices at > present use TLS, so even if I add generic TLS code, how would I test > it in the same patch unit? > I'm not so sure we could have generic block TLS env. IIUC, the _tls_x509_cert_dir (or "s:dir" to the tls-creds-x509 object) is the location for the server certificate that QEMU would present to say VxHS server. Whereas, NBD which could use the migrate TLS environment (or it's own I suppose, but we use it for migration). If Gluster, iSCSI, RBD, etc. allowed the ability to use TLS instead of secrets, then they too could conceivably have their own environment. This isn't a QEMU to QEMU communication, right? It's QEMU to some server where the storage is located from whence you'll get your storage. John I'm sure if I have this wrong someone will correct me... >> I'll reply to some parts of the patches separately, but in this state >> it's kind of a mess to go through, so please re-send the patch split >> into reasonable chunks. >> >> Note that each patch needs to make sense and can be compiled and tested >> individually. >> > > Regards, > Ashish > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] pci: add support for VMD domains
VMD domains start at 0x1, so expand dev->name to fit at least this
many characters.
Signed-off-by: Jon Derrick
---
src/util/virpci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 2c1b758..b3afefe 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -50,7 +50,7 @@ VIR_LOG_INIT("util.pci");
#define PCI_SYSFS "/sys/bus/pci/"
#define PCI_ID_LEN 10 /* " " */
-#define PCI_ADDR_LEN 13 /* ":XX:XX.X" */
+#define PCI_ADDR_LEN 14 /* "X:XX:XX.X" */
VIR_ENUM_IMPL(virPCIELinkSpeed, VIR_PCIE_LINK_SPEED_LAST,
"", "2.5", "5", "8", "16")
--
2.9.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 25/26] qemu: Isolate hostdevs on pSeries guests
On Thu, 2017-06-29 at 17:37 +0200, Andrea Bolognani wrote: > > I can see one problem here - a device that is a hostdev network > > interface won't be properly recognized. > > > > We can solve the problem for plain by just > > checking for that in addition to here. > > Right, I hadn't considered that case. I'll make sure it is > handled correctly. After implementing it, I realized this too applies to SR-IOV VFs. So depending on David's reply we might actually want to leave the code as it is. -- 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 08/12] nodedev: Dereference the obj/def in virNodeDeviceObjListFind* APIs
On 06/30/2017 08:06 AM, Erik Skultety wrote:
> On Sat, Jun 03, 2017 at 09:11:58AM -0400, John Ferlan wrote:
>> Create local @obj and @def for the API's rather than referencing the
>> devs->objs[i][->def->]. It'll make future patches easier to read.
^ [1]
>>
>> Signed-off-by: John Ferlan
>> ---
>> src/conf/virnodedeviceobj.c | 60
>> -
>> 1 file changed, 37 insertions(+), 23 deletions(-)
>>
>> diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
>> index fab1cfd..a9187be 100644
>> --- a/src/conf/virnodedeviceobj.c
>> +++ b/src/conf/virnodedeviceobj.c
>> @@ -172,12 +172,16 @@
>> virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs,
>> size_t i;
>>
>> for (i = 0; i < devs->count; i++) {
>> -virNodeDeviceObjLock(devs->objs[i]);
>> -if ((devs->objs[i]->def->sysfs_path != NULL) &&
>> -(STREQ(devs->objs[i]->def->sysfs_path, sysfs_path))) {
>> -return devs->objs[i];
>> +virNodeDeviceObjPtr obj = devs->objs[i];
>> +virNodeDeviceDefPtr def;
>
> Although with good intension, I think having virNodeDeviceObjPtr obj = foo
> only
> will make all the difference, thus dereferencing @def is IMHO not that much
> more beneficial.
[1] I'm trying to avoid syntax such as:
devs->objs[i]->def->sysfs_path
or
objs->def->sysfs_path
If some day ->def is "owned-by" some vir*Object, then -> def would need
to be fetched, e.g. def = virObject*GetDef(obj), so this just makes that
future easier.
John
>
> ACK with that.
>
> Erik
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Entering freeze for libvirt-3.5.0
On Fri, 2017-06-30 at 14:19 +0100, Daniel P. Berrange wrote: > > > Not all builders run all jobs, eg. the test suite is skipped > > > on FreeBSD, but they all at least compile the library. > > > > Is there any specific reason why it doesn't run test suite on FreeBSD? > > Generally, '(g)make check' should run fine, with the only exception that > > virnetsockettest fails from time to time (maybe once in 4-5 runs). qemuxml2argvtest fails consistently in my FreeBSD guest. virnetsockettest also fails pretty often for me, certainly more than your figure; even if that wasn't the case, 1/5 failure rate is way too high for a CI job. > > 'syntax-check' will not work without local hacks though because it hits > > argmax limit that results in 'argument list too long' for a lot of > > checks. > > Our 'check' jobs depend on the 'syntax-check' jobs as a pre-requisite, > so its fallout from not running syntax-check on BSD Can we invert the dependency so that syntax-check requires check instead? Not that it would be a good idea until the issues mentioned above are solved and check can run 100% reliably on FreeBSD, of course. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Entering freeze for libvirt-3.5.0
On Fri, Jun 30, 2017 at 05:45:04PM +0200, Andrea Bolognani wrote: > On Fri, 2017-06-30 at 14:19 +0100, Daniel P. Berrange wrote: > > > > Not all builders run all jobs, eg. the test suite is skipped > > > > on FreeBSD, but they all at least compile the library. > > > > > > Is there any specific reason why it doesn't run test suite on FreeBSD? > > > Generally, '(g)make check' should run fine, with the only exception that > > > virnetsockettest fails from time to time (maybe once in 4-5 runs). > > qemuxml2argvtest fails consistently in my FreeBSD guest. > > virnetsockettest also fails pretty often for me, certainly > more than your figure; even if that wasn't the case, 1/5 > failure rate is way too high for a CI job. > > > > 'syntax-check' will not work without local hacks though because it hits > > > argmax limit that results in 'argument list too long' for a lot of > > > checks. > > > > Our 'check' jobs depend on the 'syntax-check' jobs as a pre-requisite, > > so its fallout from not running syntax-check on BSD > > Can we invert the dependency so that syntax-check requires > check instead? I think we could actually just let them run in parallel, and then make the rpm job depend on both 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] [PATCH v4 0/3] Add support for Veritas HyperScale (VxHS) block device protocol
Hi, Thanks for the review! On Fri, Jun 30, 2017 at 12:41 AM, Peter Krempa wrote: > On Thu, Jun 29, 2017 at 19:02:38 -0700, Ashish Mittal wrote: >> From: Ashish Mittal >> >> QEMU changes for VxHS (including TLS support) are already upstream. >> This series of patches adds support for VxHS block devices in libvirt. >> >> Patch 1 adds the base functionality for supporting VxHS protocol. >> >> Patch 2 adds two new configuration options in qemu.conf to enable TLS >> for VxHS devices. >> >> Patch 3 implements the main TLS functionality. > > This ordering is wrong and also your patches have a lot of stuff going > on at once. > > I suggest you add the TLS support (Which should be designed to be > generic with other protocols which may start using TLS in mind) first > and then start adding the rest of the stuff. > I'm not sure I understand this point. libvirt currently does not have support of VxHS devices. So I cannot add TLS support for VxHS before base VxHS functionality. If you mean that I should add generic TLS handling functionality for block device protocols, then it would probably just be some new variables in structures and bare-bone functions (1 or 2) that don't do much. None of the block devices at present use TLS, so even if I add generic TLS code, how would I test it in the same patch unit? > I'll reply to some parts of the patches separately, but in this state > it's kind of a mess to go through, so please re-send the patch split > into reasonable chunks. > > Note that each patch needs to make sense and can be compiled and tested > individually. > Regards, Ashish -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 01/12] nodedev: Alter virNodeDeviceObjRemove
On 06/30/2017 07:41 AM, Erik Skultety wrote: > On Fri, Jun 30, 2017 at 07:14:13AM -0400, John Ferlan wrote: >> >> >> On 06/30/2017 04:40 AM, Erik Skultety wrote: >>> On Thu, Jun 29, 2017 at 12:07:42PM -0400, John Ferlan wrote: On 06/29/2017 10:28 AM, Erik Skultety wrote: > On Thu, Jun 29, 2017 at 09:57:09AM -0400, John Ferlan wrote: >> >> >> On 06/29/2017 08:12 AM, Erik Skultety wrote: >>> On Sat, Jun 03, 2017 at 09:11:51AM -0400, John Ferlan wrote: Rather than passing the object to be removed by reference, pass by value and then let the caller decide whether or not the object should be free'd. This function should just handle the remove of the object from the list for which it was placed during virNodeDeviceObjAssignDef. One caller in node_device_hal would fail to go through the dev_create path since the @dev would have been NULL after returning from the Remove API. >>> >>> This is the main motivation for the patch I presume - in which case, I'm >>> wondering why do we actually have to remove the device from the list >>> when >>> handling 'change'/'update' for hal instead of just replacing the ->def >>> with a >>> new instance but it's perfectly fine to do that for udev...I don't see >>> the >>> point in doing what we currently do for hal. >>> >>> [...] >> >> The main motivation is that in the previous review pass there was a >> "dislike" of passing the pointer to a pointer for something else I >> changed, see: >> >> https://www.redhat.com/archives/libvir-list/2017-May/msg01074.html >> >> Also the initial pass at altering this function was questioned, see: >> >> https://www.redhat.com/archives/libvir-list/2017-May/msg01001.html >> > > Well, that comment is true, but the commit message of this patch says > that it > drops the requirement of passing by reference, thus leaving the > responsibility > to free the obj to the caller. Now, the way I see it what we should aim at > achieving here is reference counted objects, so the vir*ObjFree in the > caller > would become and *Unref. Therefore IMHO it's not the best approach to > introduce > another boolean for HAL and leave the vir*ObjFree in the Remove routine, > you > wouldn't be clearing the object for the caller anyway, so I don't think > that is > the way to go. > I actually think the better course of action is to be more like the other functions. I believe virNodeDeviceObjRemove should do the virNodeDeviceObjFree for the simple reason that *ObjAssignDef does the alloc and list insertion and *Remove is the antecedent doing the list >>> >>> Well, eventually we'll hopefully end up with reference-counted objects so >>> doing >>> it with having *ObjFree in ObjRemove or in the caller won't matter in the >>> end. >> >> Keeping the Free/Unref as part of the callers responsibility would mean >> that once we ended up with common object code doing the remove, then >> each of the nodedev callers would need to remove their extra Free/Unref. > > I don't think so. Looking at other examples, what happens in most cases is > that > caller already gets the ref'd object, calls Remove() which calls Unref(), > returns to the caller who then calls EndAPI() which in turn destroys the last > reference. Therefore in here, to actually get rid of the object all the > callers > would need to do essentially the same thing - keep their Unref() in the > function > block, solely because GetObj() returns an already locked and ref'd object. > Doh... true - of course I have in mind the end goal which is making this whole process awfully difficult. Going step by step hasn't really helped as much as it should have /-(. Adjustments to later patches will be necessary to either Unref or EndAPI. Without getting into specifics of each EndAPI model (domain, network, and secret) - I'm assuming you can agree that each does things a bit differently and gives a sense for the impetus behind this overall effort. I generally sit here looking at code keeping track on my fingers of the RefCnt and whether the lock is held ;-) >> Which like you point out below is essentially equal work as long as one >> remembers that when it comes time ;-). >> >>> From my perspective, if it's the caller who's responsible to free it, >>> they'll >>> know they cannot touch the pointer afterwards (yeah...I assumed there are no >>> bugs) whereas if freeing the object is done as part of ObjRemove, it's the >>> caller who's the one to assume if the object was both freed and cleared or >>> just >>> freed or neither - which I don't like that much, but again, matter of >>> perspective, I see your reasoning though. >>> You'll have to replace frees with unrefs anyway, thus you won't make it any >>> eas
Re: [libvirt] [PATCH 08/10] conf, qemu: Use virDomainDeviceInfoNew()
On Thu, Jun 29, 2017 at 20:04:01 +0200, Andrea Bolognani wrote:
> Instances allocated on the stack or using VIR_ALLOC()
> directly skip the initialization process. Use the
> proper allocation function instead to ensure all
> virDomainDeviceInfo instances are properly initialized.
>
> Signed-off-by: Andrea Bolognani
> ---
> src/conf/domain_addr.c | 6 --
> src/conf/domain_conf.c | 4 ++--
> src/libvirt_private.syms | 2 ++
> src/qemu/qemu_domain_address.c | 29 +
> 4 files changed, 25 insertions(+), 16 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 178aefc..48e9e33 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -91,6 +91,8 @@ virCPUModeTypeToString;
> # conf/device_conf.h
> virDomainDeviceInfoAddressIsEqual;
> virDomainDeviceInfoCopy;
> +virDomainDeviceInfoFree;
> +virDomainDeviceInfoNew;
Looks like this belongs more like it belongs to the patch moving the
functions.
> virInterfaceLinkFormat;
> virInterfaceLinkParseXML;
> virPCIDeviceAddressEqual;
> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index b5b863f..93ab304 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -1983,6 +1983,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
> int ret = -1;
> virDomainPCIAddressSetPtr addrs = NULL;
> qemuDomainObjPrivatePtr priv = NULL;
> +virDomainDeviceInfoPtr info = virDomainDeviceInfoNew();
> int max_idx = -1;
> int nbuses = 0;
> size_t i;
> @@ -2031,16 +2032,15 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
> */
>
> if (qemuDomainHasPCIRoot(def)) {
> -/* This is a dummy info used to reserve a slot for a
> +bool buses_reserved = true;
> +
> +/* The dummy info is used to reserve a slot for a
> * legacy PCI device that doesn't exist, but may in the
> * future, e.g. if another device is hotplugged into the
> * domain.
> */
> -virDomainDeviceInfo info = {
> -.pciConnectFlags = (VIR_PCI_CONNECT_HOTPLUGGABLE |
> -VIR_PCI_CONNECT_TYPE_PCI_DEVICE)
> -};
> -bool buses_reserved = true;
> +info->pciConnectFlags = (VIR_PCI_CONNECT_HOTPLUGGABLE |
@info may be NULL here.
> + VIR_PCI_CONNECT_TYPE_PCI_DEVICE);
>
> for (i = 0; i < addrs->nbuses; i++) {
> if (!qemuDomainPCIBusFullyReserved(&addrs->buses[i])) {
> @@ -2049,7 +2049,7 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
> }
> }
> if (!buses_reserved &&
> -qemuDomainPCIAddressReserveNextAddr(addrs, &info) < 0)
> +qemuDomainPCIAddressReserveNextAddr(addrs, info) < 0)
> goto cleanup;
> }
>
> @@ -2073,15 +2073,19 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
> if (max_idx <= 0 &&
> addrs->nbuses > max_idx + 1 &&
> qemuDomainHasPCIeRoot(def)) {
> -virDomainDeviceInfo info = {
> -.pciConnectFlags = (VIR_PCI_CONNECT_HOTPLUGGABLE |
> -VIR_PCI_CONNECT_TYPE_PCIE_DEVICE)
> -};
> +
> +/* The dummy info is used to reserve a slot for a
> + * PCI Express device that doesn't exist, but may in the
> + * future, e.g. if another device is hotplugged into the
> + * domain.
> + */
> +info->pciConnectFlags = (VIR_PCI_CONNECT_HOTPLUGGABLE |
Here too.
> + VIR_PCI_CONNECT_TYPE_PCIE_DEVICE);
>
> /* if there isn't an empty pcie-root-port, this will
> * cause one to be added
> */
> -if (qemuDomainPCIAddressReserveNextAddr(addrs, &info) < 0)
> +if (qemuDomainPCIAddressReserveNextAddr(addrs, info) < 0)
> goto cleanup;
> }
>
ACK if you make sure that @info is valid, which kind of wasn't necessary
with stack alloc'd variables.
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] Entering freeze for libvirt-3.5.0
On Fri, Jun 30, 2017 at 05:11:09PM +0400, Roman Bogorodskiy wrote: > Andrea Bolognani wrote: > > > On Thu, 2017-06-29 at 09:04 +0200, Daniel Veillard wrote: > > > BTW what is the list of platfdorms we compile on in CentOS CI ? > > > > * CentOS 6 and 7 > > * Fedora 23, 24, 25 and rawhide > > * Debian (oldstable?) > > * FreeBSD (11?) > > > > https://ci.centos.org/view/libvirt/job/libvirt-master-build/ > > > > Not all builders run all jobs, eg. the test suite is skipped > > on FreeBSD, but they all at least compile the library. > > Is there any specific reason why it doesn't run test suite on FreeBSD? > Generally, '(g)make check' should run fine, with the only exception that > virnetsockettest fails from time to time (maybe once in 4-5 runs). > > 'syntax-check' will not work without local hacks though because it hits > argmax limit that results in 'argument list too long' for a lot of > checks. Our 'check' jobs depend on the 'syntax-check' jobs as a pre-requisite, so its fallout from not running syntax-check on BSD 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] Entering freeze for libvirt-3.5.0
Andrea Bolognani wrote: > On Thu, 2017-06-29 at 09:04 +0200, Daniel Veillard wrote: > > BTW what is the list of platfdorms we compile on in CentOS CI ? > > * CentOS 6 and 7 > * Fedora 23, 24, 25 and rawhide > * Debian (oldstable?) > * FreeBSD (11?) > > https://ci.centos.org/view/libvirt/job/libvirt-master-build/ > > Not all builders run all jobs, eg. the test suite is skipped > on FreeBSD, but they all at least compile the library. Is there any specific reason why it doesn't run test suite on FreeBSD? Generally, '(g)make check' should run fine, with the only exception that virnetsockettest fails from time to time (maybe once in 4-5 runs). 'syntax-check' will not work without local hacks though because it hits argmax limit that results in 'argument list too long' for a lot of checks. > -- > Andrea Bolognani / Red Hat / Virtualization > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list Roman Bogorodskiy signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 11/12] nodedev: Privatize _virNodeDeviceObj and _virNodeDeviceObjList
On Sat, Jun 03, 2017 at 09:12:01AM -0400, John Ferlan wrote: > Move the structures to withing virnodedeviceobj.c > > Move the typedefs from node_device_conf to virnodedeviceobj.h > > Signed-off-by: John Ferlan ACK Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 09/10] conf: Provide missing virDomain*DefNew() functions
On Fri, Jun 30, 2017 at 12:34:15PM +0200, Andrea Bolognani wrote: On Fri, 2017-06-30 at 10:18 +0200, Ján Tomko wrote: > Same as virDomainDeviceInfo itself, any struct that > embeds it needs to be initialized properly before use; > however, none of the structs in question even had a > proper allocation function defined. > > Implement an allocation function for all structs > embedding a virDomainDeviceInfo and use them instead > of plain VIR_ALLOC() everywhere. NACK This is a pointless obfuscation Would you mind spending a few words to explain why you feel that's the case? Having a function where you perform both allocation and initialization of your data structure is a pretty common As of now, nothing special is initialized in the structure. If you are going to need initialize something in the future, please mention it also in the commit message, not just the cover letter. This makes the obfuscation pointful. pattern both outside and inside libvirt, and while it adds one layer of indirection it also improves flexibility and encapsulation, which makes it IMHO well worth it. I am not a fan of adding code just in case we might need it in the future, which is what this patch looks like to a lazy reviewer that does not read cover letters for cleanups/refactors. Jan signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] tpm: Use /dev/null for cancel path if none was found
Hello Stefan,
On 06/29/2017 08:01 PM, Stefan Berger wrote:
> TPM 2 does not implement sysfs files for cancellation of commands.
> We therefore use /dev/null for the cancel path passed to QEMU.
>
> Signed-off-by: Stefan Berger
> ---
> src/util/virtpm.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/src/util/virtpm.c b/src/util/virtpm.c
> index 6d9b065..d5c10da 100644
> --- a/src/util/virtpm.c
> +++ b/src/util/virtpm.c
> @@ -61,9 +61,7 @@ virTPMCreateCancelPath(const char *devpath)
> VIR_FREE(path);
> }
> if (!path)
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("No usable sysfs TPM cancel file could be "
> - "found"));
> +ignore_value(VIR_STRDUP(path, "/dev/null"));
> } else {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("TPM device path %s is invalid"), devpath);
>
Thanks a lot for the patch. Now libvirt does not complain anymore about the
missing cancel file and I could access the host TPM2.0 correctly from guest.
Tested-by: Javier Martinez Canillas
Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 08/12] nodedev: Dereference the obj/def in virNodeDeviceObjListFind* APIs
On Sat, Jun 03, 2017 at 09:11:58AM -0400, John Ferlan wrote:
> Create local @obj and @def for the API's rather than referencing the
> devs->objs[i][->def->]. It'll make future patches easier to read.
>
> Signed-off-by: John Ferlan
> ---
> src/conf/virnodedeviceobj.c | 60
> -
> 1 file changed, 37 insertions(+), 23 deletions(-)
>
> diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c
> index fab1cfd..a9187be 100644
> --- a/src/conf/virnodedeviceobj.c
> +++ b/src/conf/virnodedeviceobj.c
> @@ -172,12 +172,16 @@
> virNodeDeviceObjListFindBySysfsPath(virNodeDeviceObjListPtr devs,
> size_t i;
>
> for (i = 0; i < devs->count; i++) {
> -virNodeDeviceObjLock(devs->objs[i]);
> -if ((devs->objs[i]->def->sysfs_path != NULL) &&
> -(STREQ(devs->objs[i]->def->sysfs_path, sysfs_path))) {
> -return devs->objs[i];
> +virNodeDeviceObjPtr obj = devs->objs[i];
> +virNodeDeviceDefPtr def;
Although with good intension, I think having virNodeDeviceObjPtr obj = foo only
will make all the difference, thus dereferencing @def is IMHO not that much
more beneficial.
ACK with that.
Erik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 13/26] conf: Parse and format
On Fri, Jun 23, 2017 at 8:33 PM, Andrea Bolognani
wrote:
> Signed-off-by: Andrea Bolognani
> ---
> docs/formatdomain.html.in | 5 +
> docs/schemas/domaincommon.rng | 5 +
> src/conf/domain_conf.c| 24
> src/conf/domain_conf.h| 1 +
> 4 files changed, 35 insertions(+)
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index a55a9e1..159c243 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -3768,6 +3768,11 @@
> libvirt API to attach host devices to the correct
> pci-expander-bus when assigning them to the domain).
>
> + index
> +
> +pci-root controllers for pSeries guests will use this attribute to
> +record the order they will show up in the guest.
> +
>
>
>For machine types which provide an implicit PCI bus, the pci-root
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index e259e3e..39eff46 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1997,6 +1997,11 @@
>
>
>
> +
> +
> +
> +
> +
>
>
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 29268a9..1538747 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1866,6 +1866,7 @@ virDomainControllerDefNew(virDomainControllerType
> type)
> def->opts.pciopts.chassis = -1;
> def->opts.pciopts.port = -1;
> def->opts.pciopts.busNr = -1;
> +def->opts.pciopts.idx = -1;
> def->opts.pciopts.numaNode = -1;
> break;
> case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
> @@ -9099,6 +9100,7 @@ virDomainControllerDefParseXML(xmlNodePtr node,
> goto error;
> }
> def->idx = idxVal;
> +VIR_FREE(idx);
> }
>
> cur = node->children;
> @@ -9130,6 +9132,7 @@ virDomainControllerDefParseXML(xmlNodePtr node,
> chassis = virXMLPropString(cur, "chassis");
> port = virXMLPropString(cur, "port");
> busNr = virXMLPropString(cur, "busNr");
> +idx = virXMLPropString(cur, "index");
> processedTarget = true;
> }
> }
> @@ -9348,6 +9351,23 @@ virDomainControllerDefParseXML(xmlNodePtr node,
> goto error;
> }
> }
> +if (idx) {
> +if (virStrToLong_i(idx, NULL, 0,
> + &def->opts.pciopts.idx) < 0) {
> +virReportError(VIR_ERR_XML_ERROR,
> + _("Invalid target index '%s' in PCI
> controller"),
> + idx);
> +goto error;
> +}
> +if (def->opts.pciopts.idx < 0 ||
> +def->opts.pciopts.idx > 31) {
> +virReportError(VIR_ERR_XML_ERROR,
> + _("PCI controller target index '%s' out of
> "
> + "range - must be 0-31"),
> + idx);
> +goto error;
> +}
> +}
>
Better to enforce def->idx == 0 to have def->opts.pciopts.idx as 0 ? That
would always assure def->idx == 0 as the implicit PHB.
Qemu today allows "-device spapr-pci-host-bridge,index=X,id=pci.0" when X
!= 0, and we will generate it for implcit phb(which we normally dont)
when def->opts.pciopts.idx != 0 in Patch 18. Not sure what all
problems/confusions could come because of this inconsistency.
if (numaNode >= 0)
> def->opts.pciopts.numaNode = numaNode;
> break;
> @@ -21748,6 +21768,7 @@ virDomainControllerDefFormat(virBufferPtr buf,
> def->opts.pciopts.chassis != -1 ||
> def->opts.pciopts.port != -1 ||
> def->opts.pciopts.busNr != -1 ||
> +def->opts.pciopts.idx != -1 ||
> def->opts.pciopts.numaNode != -1) {
> virBufferAddLit(&childBuf, " if (def->opts.pciopts.chassisNr != -1)
> @@ -21762,6 +21783,9 @@ virDomainControllerDefFormat(virBufferPtr buf,
> if (def->opts.pciopts.busNr != -1)
> virBufferAsprintf(&childBuf, " busNr='%d'",
>def->opts.pciopts.busNr);
> +if (def->opts.pciopts.idx != -1)
> +virBufferAsprintf(&childBuf, " index='%d'",
> + def->opts.pciopts.idx);
> if (def->opts.pciopts.numaNode == -1) {
> virBufferAddLit(&childBuf, "/>\n");
> } else {
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 6d9ee97..53a10db 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -785,6 +78
Re: [libvirt] [PATCH v3 01/12] nodedev: Alter virNodeDeviceObjRemove
On Fri, Jun 30, 2017 at 07:14:13AM -0400, John Ferlan wrote: > > > On 06/30/2017 04:40 AM, Erik Skultety wrote: > > On Thu, Jun 29, 2017 at 12:07:42PM -0400, John Ferlan wrote: > >> > >> > >> On 06/29/2017 10:28 AM, Erik Skultety wrote: > >>> On Thu, Jun 29, 2017 at 09:57:09AM -0400, John Ferlan wrote: > > > On 06/29/2017 08:12 AM, Erik Skultety wrote: > > On Sat, Jun 03, 2017 at 09:11:51AM -0400, John Ferlan wrote: > >> Rather than passing the object to be removed by reference, pass by > >> value > >> and then let the caller decide whether or not the object should be > >> free'd. > >> This function should just handle the remove of the object from the list > >> for which it was placed during virNodeDeviceObjAssignDef. > >> > >> One caller in node_device_hal would fail to go through the dev_create > >> path > >> since the @dev would have been NULL after returning from the Remove > >> API. > > > > This is the main motivation for the patch I presume - in which case, I'm > > wondering why do we actually have to remove the device from the list > > when > > handling 'change'/'update' for hal instead of just replacing the ->def > > with a > > new instance but it's perfectly fine to do that for udev...I don't see > > the > > point in doing what we currently do for hal. > > > > [...] > > The main motivation is that in the previous review pass there was a > "dislike" of passing the pointer to a pointer for something else I > changed, see: > > https://www.redhat.com/archives/libvir-list/2017-May/msg01074.html > > Also the initial pass at altering this function was questioned, see: > > https://www.redhat.com/archives/libvir-list/2017-May/msg01001.html > > >>> > >>> Well, that comment is true, but the commit message of this patch says > >>> that it > >>> drops the requirement of passing by reference, thus leaving the > >>> responsibility > >>> to free the obj to the caller. Now, the way I see it what we should aim at > >>> achieving here is reference counted objects, so the vir*ObjFree in the > >>> caller > >>> would become and *Unref. Therefore IMHO it's not the best approach to > >>> introduce > >>> another boolean for HAL and leave the vir*ObjFree in the Remove routine, > >>> you > >>> wouldn't be clearing the object for the caller anyway, so I don't think > >>> that is > >>> the way to go. > >>> > >> > >> I actually think the better course of action is to be more like the > >> other functions. I believe virNodeDeviceObjRemove should do the > >> virNodeDeviceObjFree for the simple reason that *ObjAssignDef does the > >> alloc and list insertion and *Remove is the antecedent doing the list > > > > Well, eventually we'll hopefully end up with reference-counted objects so > > doing > > it with having *ObjFree in ObjRemove or in the caller won't matter in the > > end. > > Keeping the Free/Unref as part of the callers responsibility would mean > that once we ended up with common object code doing the remove, then > each of the nodedev callers would need to remove their extra Free/Unref. I don't think so. Looking at other examples, what happens in most cases is that caller already gets the ref'd object, calls Remove() which calls Unref(), returns to the caller who then calls EndAPI() which in turn destroys the last reference. Therefore in here, to actually get rid of the object all the callers would need to do essentially the same thing - keep their Unref() in the function block, solely because GetObj() returns an already locked and ref'd object. > Which like you point out below is essentially equal work as long as one > remembers that when it comes time ;-). > > > From my perspective, if it's the caller who's responsible to free it, > > they'll > > know they cannot touch the pointer afterwards (yeah...I assumed there are no > > bugs) whereas if freeing the object is done as part of ObjRemove, it's the > > caller who's the one to assume if the object was both freed and cleared or > > just > > freed or neither - which I don't like that much, but again, matter of > > perspective, I see your reasoning though. > > You'll have to replace frees with unrefs anyway, thus you won't make it any > > easier by this approach, the amount of work in both cases we're discussing > > here > > is equal, so I'll be probably fine with that adjustment as well. > > > >> removal and free. I will adjust the commit message and can even add > >> comments prior to the function (if desired; however, it'll eventually be > >> just a wrapper to a more generic object function). > >> > >> Also, light has dawned over marble head and for hal's dev_refresh the > >> logic will then work correctly anyway since &dev wouldn't be set to > >> NULL. The code doesn't reference data in @dev. All that is important is > > > > Relying on a dangling pointer just for the sole purpose of checki
Re: [libvirt] [PATCH v3 07/12] nodedev: Alter node device obj list function names
On Sat, Jun 03, 2017 at 09:11:57AM -0400, John Ferlan wrote: > Ensure that any function that walks the node device object list is prefixed > by virNodeDeviceObjList. > > Also, modify the @filter param name for virNodeDeviceObjListExport to > be @aclfilter. > > Signed-off-by: John Ferlan One more from the 'not a fan' patches :)... ACK Erik > static virNodeDeviceObjPtr > -virNodeDeviceFindByWWNs(virNodeDeviceObjListPtr devs, > -const char *parent_wwnn, > -const char *parent_wwpn) > +virNodeDeviceObjListFindByWWNs(virNodeDeviceObjListPtr devs, > + const char *parent_wwnn, > + const char *parent_wwpn) I assume one would argue that calling this function simply *ByWWN wouldn't reflect the reality and justify the actual need for both wwnn and wwpn. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Don't update CPU when checking ABI stability
On Wed, Jun 28, 2017 at 04:24:39PM +0200, Jiri Denemark wrote: > When checking ABI stability between two domain definitions, we first > make migratable copies of them. However, we also asked for the guest CPU > to be updated, even though the updated CPU is supposed to be already > included in the original definitions. Moreover, if we do this on the > destination host during migration, we're potentially updating the > definition with according to an incompatible host CPU. > > While updating the CPU when checking ABI stability doesn't make any > sense, it actually just worked because updating the CPU doesn't do > anything for custom CPUs (only host-model CPUs are affected) and we > updated both definitions in the same way. > > Less then a year ago commit v2.3.0-rc1~42 stopped updating the CPU in > the definition we got internally and only the user supplied definition > was updated. However, the same commit started updating host-model CPUs > to custom CPUs which are not affected by the request to update the CPU. > So it still seemed to work right, unless a user upgraded libvirt 2.2.0 > to a newer version while there were some domains with host-model CPUs > running on the host. Such domains couldn't be migrated with a user > supplied XML since libvirt would complain: > > Target CPU mode custom does not match source host-model > > The fix is pretty straightforward, we just need to stop updating the CPU > when checking ABI stability. > > https://bugzilla.redhat.com/show_bug.cgi?id=1463957 > > Signed-off-by: Jiri Denemark > --- > src/qemu/qemu_domain.c | 1 - > 1 file changed, 1 deletion(-) Reviewed-by: Pavel Hrdina signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 04/10] conf: Clean up virDomain{Memory, Shmem}DefParseXML()
On Fri, Jun 30, 2017 at 13:17:04 +0200, Andrea Bolognani wrote: > On Fri, 2017-06-30 at 12:50 +0200, Peter Krempa wrote: > > > if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) > > > -goto cleanup; > > > - > > > +goto error; > > > > > > -ret = def; > > > -def = NULL; > > > cleanup: > > > -ctxt->node = save; > > > VIR_FREE(tmp); > > > +ctxt->node = save; > > > +return def; > > > + > > > + error: > > > virDomainShmemDefFree(def); > > > -return ret; > > > +def = NULL; > > > +goto cleanup; > > > > I don't see how this is better than it was before. > > It's only better in that it follows the same structure as > other *ParseXML() functions in the same file. Consistency > FTW! Plus we can drop the 'ret' local variable. Okay, NACK to the change then. I prefer having less labels. signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 01/12] nodedev: Alter virNodeDeviceObjRemove
On 06/30/2017 04:40 AM, Erik Skultety wrote: > On Thu, Jun 29, 2017 at 12:07:42PM -0400, John Ferlan wrote: >> >> >> On 06/29/2017 10:28 AM, Erik Skultety wrote: >>> On Thu, Jun 29, 2017 at 09:57:09AM -0400, John Ferlan wrote: On 06/29/2017 08:12 AM, Erik Skultety wrote: > On Sat, Jun 03, 2017 at 09:11:51AM -0400, John Ferlan wrote: >> Rather than passing the object to be removed by reference, pass by value >> and then let the caller decide whether or not the object should be >> free'd. >> This function should just handle the remove of the object from the list >> for which it was placed during virNodeDeviceObjAssignDef. >> >> One caller in node_device_hal would fail to go through the dev_create >> path >> since the @dev would have been NULL after returning from the Remove API. > > This is the main motivation for the patch I presume - in which case, I'm > wondering why do we actually have to remove the device from the list when > handling 'change'/'update' for hal instead of just replacing the ->def > with a > new instance but it's perfectly fine to do that for udev...I don't see the > point in doing what we currently do for hal. > > [...] The main motivation is that in the previous review pass there was a "dislike" of passing the pointer to a pointer for something else I changed, see: https://www.redhat.com/archives/libvir-list/2017-May/msg01074.html Also the initial pass at altering this function was questioned, see: https://www.redhat.com/archives/libvir-list/2017-May/msg01001.html >>> >>> Well, that comment is true, but the commit message of this patch says that >>> it >>> drops the requirement of passing by reference, thus leaving the >>> responsibility >>> to free the obj to the caller. Now, the way I see it what we should aim at >>> achieving here is reference counted objects, so the vir*ObjFree in the >>> caller >>> would become and *Unref. Therefore IMHO it's not the best approach to >>> introduce >>> another boolean for HAL and leave the vir*ObjFree in the Remove routine, you >>> wouldn't be clearing the object for the caller anyway, so I don't think >>> that is >>> the way to go. >>> >> >> I actually think the better course of action is to be more like the >> other functions. I believe virNodeDeviceObjRemove should do the >> virNodeDeviceObjFree for the simple reason that *ObjAssignDef does the >> alloc and list insertion and *Remove is the antecedent doing the list > > Well, eventually we'll hopefully end up with reference-counted objects so > doing > it with having *ObjFree in ObjRemove or in the caller won't matter in the end. Keeping the Free/Unref as part of the callers responsibility would mean that once we ended up with common object code doing the remove, then each of the nodedev callers would need to remove their extra Free/Unref. Which like you point out below is essentially equal work as long as one remembers that when it comes time ;-). > From my perspective, if it's the caller who's responsible to free it, they'll > know they cannot touch the pointer afterwards (yeah...I assumed there are no > bugs) whereas if freeing the object is done as part of ObjRemove, it's the > caller who's the one to assume if the object was both freed and cleared or > just > freed or neither - which I don't like that much, but again, matter of > perspective, I see your reasoning though. > You'll have to replace frees with unrefs anyway, thus you won't make it any > easier by this approach, the amount of work in both cases we're discussing > here > is equal, so I'll be probably fine with that adjustment as well. > >> removal and free. I will adjust the commit message and can even add >> comments prior to the function (if desired; however, it'll eventually be >> just a wrapper to a more generic object function). >> >> Also, light has dawned over marble head and for hal's dev_refresh the >> logic will then work correctly anyway since &dev wouldn't be set to >> NULL. The code doesn't reference data in @dev. All that is important is > > Relying on a dangling pointer just for the sole purpose of checking 'yep, > there > used to be something some time ago' would be very very very poor and fragile > design. Adding a bool there is still only a workaround of a previous > workaround > (the commit you referenced), which doesn't necessarily make the 'more nicely > packaged' fix a better solution. Per [1] we should call dev_create(udi) right > away, dropping the driver lock just before the call - so much more readable. > So I don't agree with the boolean part of the patch attached. Personally I was more in favor of the pass by reference logic because it leaves no question. I'm not as much of a fan of the (sometimes hidden) knowledge that a function "consumes" a callers pointer. We have quite a few of those sprinkled throughout the code. The few code analyze
Re: [libvirt] [PATCH 07/10] conf: Clean up virDomainDeviceInfo functions
On Thu, Jun 29, 2017 at 20:04:00 +0200, Andrea Bolognani wrote:
> Mostly style changes to make them a little bit nicer.
>
> Signed-off-by: Andrea Bolognani
> ---
> src/conf/device_conf.c | 38 +++---
> 1 file changed, 31 insertions(+), 7 deletions(-)
>
> diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
> index 6ead830..facde0e 100644
> --- a/src/conf/device_conf.c
> +++ b/src/conf/device_conf.c
> @@ -45,21 +45,44 @@ virDomainDeviceInfoNew(void)
> return info;
> }
>
> +/**
> + * virDomainDeviceInfoClear:
> + * @info: device info
> + *
> + * Reset @info to its default state: all members will be set to their default
> + * value, and any memory associated with @info will be released. @info itself
> + * will still be valid after this function returns.
This is not true at this point:
@mastertype, @master, @rombar @bootIndex and @pciConnectFlags are not
touched here, so you still rely on some clearing beforehand.
This function is clearly meant just to release any allocated memory as
most of our Clear functions.
If you are going to sell it as a full-purposed cleaning functions, you
need to scrub everything.
> + *
> + * You only need to call this function if you're allocating a
> + * virDomainDeviceInfo on the stack or embedding one inside your own struct:
> + * virDomainDeviceInfoNew() already takes care of calling it for you.
> + */
> void
> virDomainDeviceInfoClear(virDomainDeviceInfoPtr info)
> {
> -VIR_FREE(info->alias);
> -memset(&info->addr, 0, sizeof(info->addr));
> info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
> +memset(&info->addr, 0, sizeof(info->addr));
> +
> +VIR_FREE(info->alias);
> VIR_FREE(info->romfile);
> VIR_FREE(info->loadparm);
> }
>
> +/**
> + * virDomainDeviceInfoCopy:
> + * @dst: destination virDomainDeviceInfo
> + * @src: source virDomainDeviceInfo
> + *
> + * Perform a deep copy of @src into @dst.
> + *
> + * Return: 0 on success, <0 on failure
> + */
> int
> virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst,
> virDomainDeviceInfoPtr src)
> {
> -/* Assume that dst is already cleared */
> +/* Clear the destination */
> +virDomainDeviceInfoClear(dst);
e.g. this would be misleading, since it is not fully cleared ...
>
> /* first a shallow copy of *everything* */
> *dst = *src;
but thankfully fully overwritten ...
Rest looks good though
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 04/10] conf: Clean up virDomain{Memory, Shmem}DefParseXML()
On Fri, 2017-06-30 at 12:50 +0200, Peter Krempa wrote: > > if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) > > -goto cleanup; > > - > > +goto error; > > > > -ret = def; > > -def = NULL; > > cleanup: > > -ctxt->node = save; > > VIR_FREE(tmp); > > +ctxt->node = save; > > +return def; > > + > > + error: > > virDomainShmemDefFree(def); > > -return ret; > > +def = NULL; > > +goto cleanup; > > I don't see how this is better than it was before. It's only better in that it follows the same structure as other *ParseXML() functions in the same file. Consistency FTW! Plus we can drop the 'ret' local variable. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/10] conf: Clean up virDomain*Def creation
On Fri, 2017-06-30 at 12:38 +0200, Peter Krempa wrote:
> > Isolation groups are used to make sure any given device ends
> > up on the same bus as related devices and on a different bus
> > as unrelated devices.
> >
> > They're an abstract concept, and while working on the initial
> > implementation it just happened to be convenient for me to
> > have the isolation group match the IOMMU group. There's no
> > specific reason that has to be the case.
>
> Fair enough. The documentation you are adding in the linked series is
> vague enough to alow this meaning too:
>
> @@ -164,6 +164,16 @@ struct _virDomainDeviceInfo {
> */
> int pciConnectFlags; /* enum virDomainPCIConnectFlags */
> char *loadparm;
> +
> +/* PCI devices will only be automatically placed on a PCI bus
> + * that shares the same isolation group */
> +int isolationGroup;
> +
> +/* Usually, PCI buses will take on the same isolation group
> + * as the first device that is plugged into them, but in some
> + * cases we might want to prevent that from happening by
> + * locking the isolation group */
> +bool isolationGroupLocked;
> };
It's vague on purpose :)
All I'm describing there is the interface from the generic
PCI address allocation code's point of view: the fact that
the QEMU driver derives isolation groups from IOMMU groups
is just an implementation detail and as such should not be
mentioned at all.
> > We're never converting back and forth between the two, which
> > I agree would end up in misery at some point down the line;
> > we just set the isolation group once per device and then just
> > perform comparison between isolation groups from there on.
>
> I'd suggest you create a helper to assign those then (be it from IOMMU
> group or something else), so there's at least a single point that can be
> referenced in the future and which will explain this reasoning.
Good idea, I'll do that!
> Also adding a note that 0 means the device is not isolated would make
> sense in the structure above.
Well, devices *always* get isolated: it's just that all
guests except for pSeries only ever have a single isolation
group ;)
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH RFC V2 1/2] Resctrl: Add new xml element to support cache tune
On Mon, Jun 26, 2017 at 06:33:39PM +0800, Eli Qiao wrote:
This patch adds new xml element to support cache tune as:
...
...
cacheId: reference of the host's cache banks id, it's from capabilities
xml.
type:cache bank type, it could be both, code, data.
sizeKiB: must be multiple of granularity, must be greater than or equal
to minimum.
Why would you come up with something like 'sizeKib'? That's so
inconvenient.
vcpus: cache allocation on vcpu set, if empty, will apply the allocation
on all vcpus.
Signed-off-by: Eli Qiao
---
docs/schemas/domaincommon.rng | 29 +++
src/conf/domain_conf.c| 113 +-
src/conf/domain_conf.h| 19 +++
3 files changed, 160 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 0409c62..fa8d03e 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -17642,7 +17730,6 @@ virDomainDefParseXML(xmlDocPtr xml,
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("cannot extract vcpusched nodes"));
goto error;
-}
for (i = 0; i < n; i++) {
if (virDomainVcpuThreadSchedParse(nodes[i], def) < 0)
And this hunk breaks the whole compilation, of course.
I'm not at work for few days plus there's a public holiday in my
country, so I won't be here for a while. That shouldn't stop anyone
else reviewing your code, but I'm giving you a heads-up that I'll
probably work on a separate solution if I have some extra time on my
hands during vacation.
Have a nice day,
Martin
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/10] conf: Introduce virDomainDeviceInfoNew()
On Thu, Jun 29, 2017 at 20:03:59 +0200, Andrea Bolognani wrote:
> It allocates and initializes a virDomainDeviceInfo struct
> in one fell swoop.
>
> Signed-off-by: Andrea Bolognani
> ---
> src/conf/device_conf.c | 13 +
> src/conf/device_conf.h | 1 +
> 2 files changed, 14 insertions(+)
>
> diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
> index 4644580..6ead830 100644
> --- a/src/conf/device_conf.c
> +++ b/src/conf/device_conf.c
> @@ -32,6 +32,19 @@
>
> #define VIR_FROM_THIS VIR_FROM_DEVICE
>
> +virDomainDeviceInfoPtr
> +virDomainDeviceInfoNew(void)
> +{
> +virDomainDeviceInfoPtr info;
> +
> +if (VIR_ALLOC(info) < 0)
> +return NULL;
> +
> +virDomainDeviceInfoClear(info);
This is a lot of duplicated 0 setting. Given a quick scroll to the other
*New function ... we only set parameters which are non-null.
The only code path calling the clearing fucntion which is not in a
fucntion which then frees the object completely is in
virDomainDeviceInfoParseXML. I don't think we need to do it here and
additionally the objects are not reused thus you should not need to set
anything non-null anywhere else.
ACK to the wrapper except for the call to virDomainDeviceInfoClear
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 05/10] conf: Move some virDomainDeviceInfo functions
On Thu, Jun 29, 2017 at 20:03:58 +0200, Andrea Bolognani wrote: > The virDomainDeviceInfo struct is defined in device_conf, > so generic functions that operate on it should also be > defined there rather than in domain_conf. > > Signed-off-by: Andrea Bolognani > --- > src/conf/device_conf.c | 109 > src/conf/device_conf.h | 8 > src/conf/domain_conf.c | 114 > --- > src/conf/domain_conf.h | 7 --- > src/libvirt_private.syms | 4 +- > 5 files changed, 119 insertions(+), 123 deletions(-) ACK signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 04/10] conf: Clean up virDomain{Memory, Shmem}DefParseXML()
On Thu, Jun 29, 2017 at 20:03:57 +0200, Andrea Bolognani wrote:
> Follow the same style as other similar functions.
>
> Signed-off-by: Andrea Bolognani
> ---
> src/conf/domain_conf.c | 36 ++--
> 1 file changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index ccd3c27..fdb919d 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
[...]
> @@ -13219,20 +13218,21 @@ virDomainShmemDefParseXML(xmlNodePtr node,
> if (def->msi.enabled && !def->server.enabled) {
> virReportError(VIR_ERR_XML_ERROR, "%s",
> _("msi option is only supported with a server"));
> -goto cleanup;
> +goto error;
> }
>
> if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0)
> -goto cleanup;
> -
> +goto error;
>
> -ret = def;
> -def = NULL;
> cleanup:
> -ctxt->node = save;
> VIR_FREE(tmp);
> +ctxt->node = save;
> +return def;
> +
> + error:
> virDomainShmemDefFree(def);
> -return ret;
> +def = NULL;
> +goto cleanup;
I don't see how this is better than it was before.
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/10] conf: Clean up virDomainHostdevDefNew()
On Thu, Jun 29, 2017 at 20:03:56 +0200, Andrea Bolognani wrote:
> Follow the same style as other similar functions.
>
> Signed-off-by: Andrea Bolognani
> ---
> src/conf/domain_conf.c | 13 +++--
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 15dd285..ccd3c27 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2380,25 +2380,26 @@ void virDomainVideoDefFree(virDomainVideoDefPtr def)
> virDomainHostdevDefPtr
> virDomainHostdevDefNew(virDomainXMLOptionPtr xmlopt)
> {
> -virDomainHostdevDefPtr def = NULL;
> +virDomainHostdevDefPtr def;
>
> -if (VIR_ALLOC(def) < 0 ||
> -VIR_ALLOC(def->info) < 0) {
> -VIR_FREE(def);
> +if (VIR_ALLOC(def) < 0)
> return NULL;
> -}
> +
> +if (VIR_ALLOC(def->info) < 0)
> +goto error;
>
> if (xmlopt &&
> xmlopt->privateData.hostdevNew &&
> !(def->privateData = xmlopt->privateData.hostdevNew()))
> goto error;
Changes below a rather pointless, and nothing gets added to the cleanup
section in other patches.
ACK to changes above this line.
>
> + cleanup:
> return def;
>
> error:
> VIR_FREE(def->info);
> VIR_FREE(def);
> -return NULL;
> +goto cleanup;
> }
>
>
> --
> 2.7.5
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 02/10] conf: Rename virDomainHostdevDefAlloc() to virDomainHostdevDefNew()
On Thu, Jun 29, 2017 at 20:03:55 +0200, Andrea Bolognani wrote: > All other virDomain*Def follow this naming convention for > their allocation function. > > Signed-off-by: Andrea Bolognani > --- > src/conf/domain_conf.c| 4 ++-- > src/conf/domain_conf.h| 2 +- > src/libvirt_private.syms | 2 +- > src/lxc/lxc_native.c | 2 +- > src/qemu/qemu_parse_command.c | 4 ++-- > src/vbox/vbox_common.c| 2 +- > src/xenconfig/xen_common.c| 2 +- > src/xenconfig/xen_sxpr.c | 2 +- > src/xenconfig/xen_xl.c| 2 +- > tests/virhostdevtest.c| 2 +- > 10 files changed, 12 insertions(+), 12 deletions(-) ACK signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 01/10] conf: Make virDomainDeviceGetInfo() private
On Thu, Jun 29, 2017 at 20:03:54 +0200, Andrea Bolognani wrote: > There are no external users. > > Signed-off-by: Andrea Bolognani > --- > src/conf/domain_conf.c | 21 - > src/conf/domain_conf.h | 1 - > 2 files changed, 12 insertions(+), 10 deletions(-) ACK signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] tests: check domain XML to libxl structures conversion
On 06/29/2017 11:08 PM, Marek Marczykowski-Górecki wrote: > On Thu, Jun 29, 2017 at 10:06:39AM +0100, Joao Martins wrote: >> On 06/29/2017 02:11 AM, Marek Marczykowski-Górecki wrote: >>> libxl contains a method to dump libxl_domain_config in json format, >>> which is really convenient for tests. Unfortunately it require >>> libxl_ctx, which can be only created when running on Xen (wither in dom0 >>> or domU). Because of this, skip the test in other cases. >>> >>> For now, have just two tests, including just-introduced CPUID handling, >>> but this commit introduces a framework for further tests of >>> src/libxl/libxl_conf.c file. >> >> There's related work about this. You might wanna look at the recent Jim's >> series >> (CC'in him) from a few months ago (see below). He ressurected danpb work on >> these xml <-> libxl_domain_config conversion, and had a whole test suite for >> that where your cpuid test would be better inserted? Patch series is here: >> >> https://www.redhat.com/archives/libvir-list/2017-February/msg01477.html > > This looks like a much more complete solution. Indeed it would make > sense to put this cpuid test there. Is there any reason why > it isn't merged yet and there is no response in that thread? > I guess it wasn't reviewed by folks. I was short on time with other deliverables so that series slipped me and couldn't help much. Perhaps a resubmission to restart discussion - It's really nice work and would greatly improve testing :) Cheers, Joao -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/10] conf: Clean up virDomain*Def creation
On Fri, Jun 30, 2017 at 12:24:33 +0200, Andrea Bolognani wrote:
> On Fri, 2017-06-30 at 10:58 +0200, Peter Krempa wrote:
> > > Or we could just, you know, do the sensible thing and
> > > store (IOMMU group + 1) instead of (IOMMU group) in
> >
> > How is that sensible? That looks as a source of bugs in the long run.
>
> Isolation groups are used to make sure any given device ends
> up on the same bus as related devices and on a different bus
> as unrelated devices.
>
> They're an abstract concept, and while working on the initial
> implementation it just happened to be convenient for me to
> have the isolation group match the IOMMU group. There's no
> specific reason that has to be the case.
Fair enough. The documentation you are adding in the linked series is
vague enough to alow this meaning too:
@@ -164,6 +164,16 @@ struct _virDomainDeviceInfo {
*/
int pciConnectFlags; /* enum virDomainPCIConnectFlags */
char *loadparm;
+
+/* PCI devices will only be automatically placed on a PCI bus
+ * that shares the same isolation group */
+int isolationGroup;
+
+/* Usually, PCI buses will take on the same isolation group
+ * as the first device that is plugged into them, but in some
+ * cases we might want to prevent that from happening by
+ * locking the isolation group */
+bool isolationGroupLocked;
};
> We're never converting back and forth between the two, which
> I agree would end up in misery at some point down the line;
> we just set the isolation group once per device and then just
> perform comparison between isolation groups from there on.
I'd suggest you create a helper to assign those then (be it from IOMMU
group or something else), so there's at least a single point that can be
referenced in the future and which will explain this reasoning.
Also adding a note that 0 means the device is not isolated would make
sense in the structure above.
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 09/10] conf: Provide missing virDomain*DefNew() functions
On Fri, 2017-06-30 at 10:18 +0200, Ján Tomko wrote: > > Same as virDomainDeviceInfo itself, any struct that > > embeds it needs to be initialized properly before use; > > however, none of the structs in question even had a > > proper allocation function defined. > > > > Implement an allocation function for all structs > > embedding a virDomainDeviceInfo and use them instead > > of plain VIR_ALLOC() everywhere. > > NACK > > This is a pointless obfuscation Would you mind spending a few words to explain why you feel that's the case? Having a function where you perform both allocation and initialization of your data structure is a pretty common pattern both outside and inside libvirt, and while it adds one layer of indirection it also improves flexibility and encapsulation, which makes it IMHO well worth it. -- 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 06/12] nodedev: Introduce virNodeDeviceObjListNew
On Sat, Jun 03, 2017 at 09:11:56AM -0400, John Ferlan wrote: > In preparation to make things private, make the ->devs be pointers to a > virNodeDeviceObjList and then manage everything inside virnodedeviceobj > > Signed-off-by: John Ferlan > --- > src/conf/virnodedeviceobj.c | 13 - > src/conf/virnodedeviceobj.h | 5 - > src/libvirt_private.syms | 1 + > src/node_device/node_device_driver.c | 14 +++--- > src/node_device/node_device_hal.c| 19 +++ > src/node_device/node_device_udev.c | 19 +++ > src/test/test_driver.c | 29 +++-- > 7 files changed, 61 insertions(+), 39 deletions(-) ACK Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/10] conf: Clean up virDomain*Def creation
On Fri, 2017-06-30 at 10:58 +0200, Peter Krempa wrote: > > Or we could just, you know, do the sensible thing and > > store (IOMMU group + 1) instead of (IOMMU group) in > > How is that sensible? That looks as a source of bugs in the long run. Isolation groups are used to make sure any given device ends up on the same bus as related devices and on a different bus as unrelated devices. They're an abstract concept, and while working on the initial implementation it just happened to be convenient for me to have the isolation group match the IOMMU group. There's no specific reason that has to be the case. We're never converting back and forth between the two, which I agree would end up in misery at some point down the line; we just set the isolation group once per device and then just perform comparison between isolation groups from there on. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/10] conf: Clean up virDomain*Def creation
On Fri, Jun 30, 2017 at 10:58:42 +0200, Peter Krempa wrote: > On Fri, Jun 30, 2017 at 10:48:36 +0200, Andrea Bolognani wrote: > > On Thu, 2017-06-29 at 20:03 +0200, Andrea Bolognani wrote: > > [...] > > > > That's worked out just fine so far, because zero was a > > > sensible default value for all existing fields; however, > > > when implementing isolation groups, we add a new > > > virDomainDeviceInfo::isolationGroup field which we need > > > to be initialized to -1 instead so that it doesn't overlap > > > with IOMMU group 0 mentioned above. > > > > Or we could just, you know, do the sensible thing and > > store (IOMMU group + 1) instead of (IOMMU group) in > > How is that sensible? That looks as a source of bugs in the long run. Btw, isn't there any external detail which would indicate that the IOMMU group is valid? In that case you could use that and read the value only if it's supposed to be meaningful. Or add a boolean which says that it's meaningfull if you are on the lazy side, but storing the group with any offset is weird. signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/10] conf: Clean up virDomain*Def creation
On Fri, Jun 30, 2017 at 10:48:36 +0200, Andrea Bolognani wrote: > On Thu, 2017-06-29 at 20:03 +0200, Andrea Bolognani wrote: [...] > > That's worked out just fine so far, because zero was a > > sensible default value for all existing fields; however, > > when implementing isolation groups, we add a new > > virDomainDeviceInfo::isolationGroup field which we need > > to be initialized to -1 instead so that it doesn't overlap > > with IOMMU group 0 mentioned above. > > Or we could just, you know, do the sensible thing and > store (IOMMU group + 1) instead of (IOMMU group) in How is that sensible? That looks as a source of bugs in the long run. signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 3/3] Add TLS support for Veritas HyperScale (VxHS) block device protocol
On Fri, Jun 30, 2017 at 10:44:39 +0200, Peter Krempa wrote: > On Thu, Jun 29, 2017 at 19:02:41 -0700, Ashish Mittal wrote: > > From: Ashish Mittal [...] > > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng > > index 7525a2a..909af50 100644 > > --- a/docs/schemas/domaincommon.rng > > +++ b/docs/schemas/domaincommon.rng > > @@ -1622,6 +1622,11 @@ > > > > > > > > + > > + > > + > > + > > Make this a definition for future reuse. Additionally I think that the > TLS part should be a separate element here. Something like > > > I forgot to finish my thought before sending. I think we want a separate element with an attribute at this point. This allows adding other TLS related stuff to it if such need arises. [...] signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 09/10] conf: Provide missing virDomain*DefNew() functions
On Thu, Jun 29, 2017 at 20:04:02 +0200, Andrea Bolognani wrote: > Same as virDomainDeviceInfo itself, any struct that > embeds it needs to be initialized properly before use; > however, none of the structs in question even had a > proper allocation function defined. > > Implement an allocation function for all structs > embedding a virDomainDeviceInfo and use them instead > of plain VIR_ALLOC() everywhere. > > Signed-off-by: Andrea Bolognani > --- > src/bhyve/bhyve_parse_command.c | 4 +- > src/conf/domain_conf.c | 280 > +++- > src/conf/domain_conf.h | 15 +++ > src/libvirt_private.syms| 11 ++ > src/openvz/openvz_conf.c| 2 +- > src/qemu/qemu_command.c | 12 +- > src/qemu/qemu_domain.c | 11 +- > src/qemu/qemu_domain_address.c | 2 +- > src/qemu/qemu_hotplug.c | 5 +- > src/qemu/qemu_parse_command.c | 27 ++-- > src/vbox/vbox_common.c | 12 +- > src/vmx/vmx.c | 2 +- > src/vz/vz_sdk.c | 6 +- > src/xen/xen_driver.c| 2 +- > src/xenapi/xenapi_driver.c | 2 +- > src/xenconfig/xen_common.c | 2 +- > src/xenconfig/xen_sxpr.c| 8 +- > src/xenconfig/xen_xl.c | 2 +- > src/xenconfig/xen_xm.c | 2 +- > 19 files changed, 303 insertions(+), 104 deletions(-) While I agree that having allocation functions which initialize the internals are necessary in some cases, this patch is a mess as you squashed everything into one place. signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 00/10] conf: Clean up virDomain*Def creation
On Thu, 2017-06-29 at 20:03 +0200, Andrea Bolognani wrote: > This series is required to solve a known limitation in the > current incarnation of device isolation support[1], namely > the inability to isolate host devices coming from IOMMU > group 0. > > The issue lies in the fact that virDomainDeviceInfo, and > all virDomain*Def that embed it, are usually allocated > through VIR_ALLOC(), which result in all their fields being > initialized to zero. > > That's worked out just fine so far, because zero was a > sensible default value for all existing fields; however, > when implementing isolation groups, we add a new > virDomainDeviceInfo::isolationGroup field which we need > to be initialized to -1 instead so that it doesn't overlap > with IOMMU group 0 mentioned above. Or we could just, you know, do the sensible thing and store (IOMMU group + 1) instead of (IOMMU group) in virDomainDeviceInfo::isolationGroup and avoid the issue altogether? I'm actually quite embarassed I didn't think of that earlier :/ > Solving the issue involves creating twenty or so > virDomain*DefNew() functions and using them instead of > VIR_ALLOC() every time a virDomain*Def needs to be created, > which is arguably a pretty good idea regardless. We could still merge this series, though, or at least parts of it. It improves upon some questionable code. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 1/3] Add support for Veritas HyperScale (VxHS) block device protocol
On Thu, Jun 29, 2017 at 19:02:39 -0700, Ashish Mittal wrote: > From: Ashish Mittal > > Sample XML for a VxHS disk: > > > > > > > > > eb90327c-8302-4725-9e1b-4e85ed4dc251 > > > > > Signed-off-by: Ashish Mittal > --- [...] Additionally since libvirt supports QAPI introspection, this means we are now able to detect whether qemu supports vxhs and should report an error if it doesn't. You'll need to add a capability bit in qemu_capabilities.h and the detection string in qemu_capabilities.c to virQEMUCapsQMPSchemaQueries[]. signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 3/3] Add TLS support for Veritas HyperScale (VxHS) block device protocol
On Thu, Jun 29, 2017 at 19:02:41 -0700, Ashish Mittal wrote: > From: Ashish Mittal > > The following describes the behavior of TLS for VxHS block device: > > (1) Two new options have been added in /etc/libvirt/qemu.conf > to control TLS behavior with VxHS block devices > "vxhs_tls" and "vxhs_tls_x509_cert_dir". > (2) Setting "vxhs_tls=1" in /etc/libvirt/qemu.conf will enable > TLS for VxHS block devices. > (3) "vxhs_tls_x509_cert_dir" can be set to the full path where the > TLS certificates and keys are saved. If this value is missing, > the "default_tls_x509_cert_dir" will be used instead. > (4) If the value of "vxhs_tls" is set to 1, TLS creds will be added > automatically on the qemu command line for every VxHS > block device. > (5) With "vxhs_tls=1", TLS may selectively be disabled on individual > VxHS disks by specifying tls='no' in the device domain > specification. > (6) Valid values for domain TLS setting are tls='yes|no'. > (7) tls='yes' can only be specified if "vxhs_tls" is enabled. > Specifying tls='yes' when "vxhs_tls=0" results in an error. > (8) Test cases have been added to validate points (4), (5) and (7). > Test case also added to confirm that JSON arguments containing > tls attribute are parsed correctly. > > QEMU changes for VxHS (including TLS support) are already upstream. > > Sample TLS args generated by libvirt - > -object tls-creds-x509,id=objvxhs_tls0,dir=/usr/local/etc/pki/qemu,\ > endpoint=client,verify-peer=yes \ > -drive file.driver=vxhs,file.tls-creds=objvxhs_tls0,\ > file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,\ > file.server.host=192.168.0.1,file.server.port=,format=raw,if=none,\ > id=drive-virtio-disk0,cache=none \ > -device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,\ > id=virtio-disk0 > > Signed-off-by: Ashish Mittal > --- Too much stuff is going on in this patch. You need to split it. > docs/formatdomain.html.in | 18 +++- > docs/schemas/domaincommon.rng | 5 + > src/conf/domain_conf.c | 19 > src/qemu/qemu_command.c| 107 > ++--- The addition to the qemu driver should be separate too. > src/util/virstoragefile.c | 13 +++ > src/util/virstoragefile.h | 9 ++ Changes to the backing store parser should be in the separate patch. > ...ml2argv-disk-drive-network-tlsx509-err-vxhs.xml | 34 +++ > ...-disk-drive-network-tlsx509-multidisk-vxhs.args | 41 > ...k-drive-network-tlsx509-multidisk-vxhs.args.new | 41 > ...v-disk-drive-network-tlsx509-multidisk-vxhs.xml | 56 +++ > ...muxml2argv-disk-drive-network-tlsx509-vxhs.args | 28 ++ > ...emuxml2argv-disk-drive-network-tlsx509-vxhs.xml | 34 +++ > tests/qemuxml2argvtest.c | 9 ++ > tests/virstoragetest.c | 11 +++ Plus these belong to the respective patches. > 14 files changed, 413 insertions(+), 12 deletions(-) > create mode 100644 > tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-err-vxhs.xml > create mode 100644 > tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args > create mode 100644 > tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args.new > create mode 100644 > tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.xml > create mode 100644 > tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args > create mode 100644 > tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.xml > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index 62d67f4..86808e5 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -2511,7 +2511,7 @@ >target's name by a slash (e.g., >iqn.2013-07.com.example:iscsi-pool/1). If not >specified, the default LUN is zero. > - For "vxhs" (since 3.3.0), the > + For "vxhs" (since 3.3.1), the There won't be any 3.3.1 release. The upcomming release is 3.5.0, and your patches will be in 3.6.0 at best since we are already in freeze state. Also this hunk is misplaced from one of the earlier patches probably. >name is the UUID of the volume, assigned by the >HyperScale sever. >Since 0.8.7 > @@ -2630,6 +2630,22 @@ > transport is "unix", the socket attribute specifies the path to > an > AF_UNIX socket. > > + > +Since 3.3.1, the optional attribute > +tls (QEMU only) can be used to control whether a > vxhs > +network block device would utilize a hypervisor configured > +TLS X.509 certificate environment in order to encrypt the data > +
Re: [libvirt] [PATCH v3 01/12] nodedev: Alter virNodeDeviceObjRemove
On Thu, Jun 29, 2017 at 12:07:42PM -0400, John Ferlan wrote: > > > On 06/29/2017 10:28 AM, Erik Skultety wrote: > > On Thu, Jun 29, 2017 at 09:57:09AM -0400, John Ferlan wrote: > >> > >> > >> On 06/29/2017 08:12 AM, Erik Skultety wrote: > >>> On Sat, Jun 03, 2017 at 09:11:51AM -0400, John Ferlan wrote: > Rather than passing the object to be removed by reference, pass by value > and then let the caller decide whether or not the object should be > free'd. > This function should just handle the remove of the object from the list > for which it was placed during virNodeDeviceObjAssignDef. > > One caller in node_device_hal would fail to go through the dev_create > path > since the @dev would have been NULL after returning from the Remove API. > >>> > >>> This is the main motivation for the patch I presume - in which case, I'm > >>> wondering why do we actually have to remove the device from the list when > >>> handling 'change'/'update' for hal instead of just replacing the ->def > >>> with a > >>> new instance but it's perfectly fine to do that for udev...I don't see the > >>> point in doing what we currently do for hal. > >>> > >>> [...] > >> > >> The main motivation is that in the previous review pass there was a > >> "dislike" of passing the pointer to a pointer for something else I > >> changed, see: > >> > >> https://www.redhat.com/archives/libvir-list/2017-May/msg01074.html > >> > >> Also the initial pass at altering this function was questioned, see: > >> > >> https://www.redhat.com/archives/libvir-list/2017-May/msg01001.html > >> > > > > Well, that comment is true, but the commit message of this patch says that > > it > > drops the requirement of passing by reference, thus leaving the > > responsibility > > to free the obj to the caller. Now, the way I see it what we should aim at > > achieving here is reference counted objects, so the vir*ObjFree in the > > caller > > would become and *Unref. Therefore IMHO it's not the best approach to > > introduce > > another boolean for HAL and leave the vir*ObjFree in the Remove routine, you > > wouldn't be clearing the object for the caller anyway, so I don't think > > that is > > the way to go. > > > > I actually think the better course of action is to be more like the > other functions. I believe virNodeDeviceObjRemove should do the > virNodeDeviceObjFree for the simple reason that *ObjAssignDef does the > alloc and list insertion and *Remove is the antecedent doing the list Well, eventually we'll hopefully end up with reference-counted objects so doing it with having *ObjFree in ObjRemove or in the caller won't matter in the end. >From my perspective, if it's the caller who's responsible to free it, they'll know they cannot touch the pointer afterwards (yeah...I assumed there are no bugs) whereas if freeing the object is done as part of ObjRemove, it's the caller who's the one to assume if the object was both freed and cleared or just freed or neither - which I don't like that much, but again, matter of perspective, I see your reasoning though. You'll have to replace frees with unrefs anyway, thus you won't make it any easier by this approach, the amount of work in both cases we're discussing here is equal, so I'll be probably fine with that adjustment as well. > removal and free. I will adjust the commit message and can even add > comments prior to the function (if desired; however, it'll eventually be > just a wrapper to a more generic object function). > > Also, light has dawned over marble head and for hal's dev_refresh the > logic will then work correctly anyway since &dev wouldn't be set to > NULL. The code doesn't reference data in @dev. All that is important is Relying on a dangling pointer just for the sole purpose of checking 'yep, there used to be something some time ago' would be very very very poor and fragile design. Adding a bool there is still only a workaround of a previous workaround (the commit you referenced), which doesn't necessarily make the 'more nicely packaged' fix a better solution. Per [1] we should call dev_create(udi) right away, dropping the driver lock just before the call - so much more readable. So I don't agree with the boolean part of the patch attached. [1] https://www.redhat.com/archives/libvir-list/2017-June/msg01328.html -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 18/26] qemu: Use multiple PHBs for pSeries guests
On Thu, Jun 29, 2017 at 1:03 PM, Andrea Bolognani wrote: > On Tue, 2017-06-27 at 13:53 +0530, Shivaprasad bhat wrote: > > Hi Andrea, > > > > We need to also adjust the memlock limits along with this patch. > > Good point. I must have remembered about that at least > five during development of the feature - and then promptly > forgotten just as many times :/ > > I guessed so.. :) > > I have the changes here if you want to append to this patch. > > > > https://paste.fedoraproject.org/paste/JrI4stQTIYiaecuXPyXh8g > > Please send this as a proper patch, mentioning that it's > meant to be applied on top of my series. > > Just sent the patch. Thanks, Shivaprasad -- > Andrea Bolognani / Red Hat / Virtualization > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] qemu: Take all PHBs into account while calculating memlock limits
Now that the multi-phb support series is in, work on the TODO at
qemuDomainGetMemLockLimitBytes() to arrive at the correct memlock limit
value.
Signed-off-by: Shivaprasad G Bhat
---
This patch should be applied on top of Andrea's multi-phb support
patchset.
src/qemu/qemu_domain.c | 12
tests/qemumemlocktest.c |2 +-
2 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index a3ce10a..a8293b4 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6674,12 +6674,16 @@ qemuDomainGetMemLockLimitBytes(virDomainDefPtr def)
unsigned long long memory;
unsigned long long baseLimit;
unsigned long long passthroughLimit;
-size_t nPCIHostBridges;
+size_t nPCIHostBridges = 0;
bool usesVFIO = false;
-/* TODO: Detect at runtime once we start using more than just
- * the default PCI Host Bridge */
-nPCIHostBridges = 1;
+for (i = 0; i < def->ncontrollers; i++) {
+if (def->controllers[i]->type != VIR_DOMAIN_CONTROLLER_TYPE_PCI ||
+def->controllers[i]->model !=
VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) {
+continue;
+}
+nPCIHostBridges++;
+}
for (i = 0; i < def->nhostdevs; i++) {
virDomainHostdevDefPtr dev = def->hostdevs[i];
diff --git a/tests/qemumemlocktest.c b/tests/qemumemlocktest.c
index c0f1dc3..268563d 100644
--- a/tests/qemumemlocktest.c
+++ b/tests/qemumemlocktest.c
@@ -131,7 +131,7 @@ mymain(void)
DO_TEST("pseries-hardlimit", 2147483648);
DO_TEST("pseries-locked", VIR_DOMAIN_MEMORY_PARAM_UNLIMITED);
-DO_TEST("pseries-hostdev", 2168455168);
+DO_TEST("pseries-hostdev", 4320133120);
DO_TEST("pseries-hardlimit+locked", 2147483648);
DO_TEST("pseries-hardlimit+hostdev", 2147483648);
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 1/3] Add support for Veritas HyperScale (VxHS) block device protocol
On Thu, Jun 29, 2017 at 19:02:39 -0700, Ashish Mittal wrote:
> From: Ashish Mittal
>
> Sample XML for a VxHS disk:
>
>
>
>
>
>
>
>
> eb90327c-8302-4725-9e1b-4e85ed4dc251
>
>
>
>
> Signed-off-by: Ashish Mittal
> ---
> v2 changelog:
> (1) Added code for JSON parsing of a VxHS vdisk.
> (2) Added test case to verify JSON parsing.
> (3) Added missing switch-case checks for VIR_STORAGE_NET_PROTOCOL_VXHS.
> (4) Fixed line wrap in qemuxml2argv-disk-drive-network-vxhs.args.
>
> v3 changelog:
> (1) Implemented the modern syntax for VxHS disk specification.
> (2) Changed qemuxml2argvdata VxHS test case to verify the new syntax.
> (3) Added a negative test case to check failure when multiple hosts
> are specified for a VxHS disk.
>
> v4 changelog:
> (1) Fixes per review comments from v3.
> (2) Had to remove a test from the previous version that checked for
> error when multiple hosts are specified for VxHS device.
> This started failing virschematest with the error
> "XML document failed to validate against schema" as the
> docs/schemas/domain.rng specifies only a single host.
>
> docs/formatdomain.html.in | 15 -
> docs/schemas/domaincommon.rng | 13
> src/libxl/libxl_conf.c | 1 +
> src/qemu/qemu_command.c| 70
> ++
> src/qemu/qemu_driver.c | 3 +
> src/qemu/qemu_parse_command.c | 25
> src/util/virstoragefile.c | 64 +++-
> src/util/virstoragefile.h | 1 +
> src/xenconfig/xen_xl.c | 1 +
> .../qemuargv2xml-disk-drive-network-vxhs-fail.args | 24
> tests/qemuargv2xmltest.c | 17 +-
> .../qemuxml2argv-disk-drive-network-vxhs.args | 25
> .../qemuxml2argv-disk-drive-network-vxhs.xml | 34 +++
> tests/qemuxml2argvtest.c | 1 +
> tests/virstoragetest.c | 19 ++
> 15 files changed, 308 insertions(+), 5 deletions(-)
> create mode 100644
> tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-vxhs-fail.args
> create mode 100644
> tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args
> create mode 100644
> tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 36bea67..62d67f4 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
[...]
> @@ -2511,6 +2511,9 @@
>target's name by a slash (e.g.,
>iqn.2013-07.com.example:iscsi-pool/1). If not
>specified, the default LUN is zero.
> + For "vxhs" (since 3.3.0), thea
3.6.0 at best.
> + name is the UUID of the volume, assigned by the
> + HyperScale sever.
>Since 0.8.7
>
> volume
[...]
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index bdf7103..7525a2a 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1613,6 +1613,18 @@
>
>
>
> +
> +
> +
> +
> + vxhs
> +
> +
> +
> +
This is misaligned.
> +
> +
> +
>
>
>network
[...]
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index c53ab97..8e00782 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -524,6 +524,7 @@ qemuNetworkDriveGetPort(int protocol,
> return 0;
>
> case VIR_STORAGE_NET_PROTOCOL_RBD:
> +case VIR_STORAGE_NET_PROTOCOL_VXHS:
> case VIR_STORAGE_NET_PROTOCOL_LAST:
> case VIR_STORAGE_NET_PROTOCOL_NONE:
> /* not applicable */
Now we are in the section of stuff which should be split into a separate
patch.
Also here you declare that there,s no default port ... (and you said)
> @@ -931,6 +932,65 @@ qemuBuildGlusterDriveJSON(virStorageSourcePtr src)
> }
>
>
> +#define QEMU_DEFAULT_VXHS_PORT ""
And here you declare the default port via a ... define. I'd suggest you
stick with the common code paths.
> +
> +/* Build the VxHS host object */
If you want to add comments, please make them useful. Document
arguments, and return values.
> +static virJSONValuePtr
> +qemuBuildVxHSDriveJSONHost(virStorageSourcePtr src)
> +{
> +virJSONValuePtr server = NULL;
> +virStorageNetHostDefPtr host;
> +const char *portstr;
> +
> +if (src->nhosts != 1) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("protocol VxHS accepts only one host"));
> +goto cleanup;
> +}
> +
> +host = src->hosts;
> +portstr = host->port;
> +
> +if (!portstr)
> +portstr = QEMU_DEFAULT_VXHS_POR
Re: [libvirt] [PATCH 09/10] conf: Provide missing virDomain*DefNew() functions
On Thu, Jun 29, 2017 at 08:04:02PM +0200, Andrea Bolognani wrote: Same as virDomainDeviceInfo itself, any struct that embeds it needs to be initialized properly before use; however, none of the structs in question even had a proper allocation function defined. Implement an allocation function for all structs embedding a virDomainDeviceInfo and use them instead of plain VIR_ALLOC() everywhere. NACK This is a pointless obfuscation Jan signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 0/3] Add support for Veritas HyperScale (VxHS) block device protocol
On Thu, Jun 29, 2017 at 19:02:38 -0700, Ashish Mittal wrote: > From: Ashish Mittal > > QEMU changes for VxHS (including TLS support) are already upstream. > This series of patches adds support for VxHS block devices in libvirt. > > Patch 1 adds the base functionality for supporting VxHS protocol. > > Patch 2 adds two new configuration options in qemu.conf to enable TLS > for VxHS devices. > > Patch 3 implements the main TLS functionality. This ordering is wrong and also your patches have a lot of stuff going on at once. I suggest you add the TLS support (Which should be designed to be generic with other protocols which may start using TLS in mind) first and then start adding the rest of the stuff. I'll reply to some parts of the patches separately, but in this state it's kind of a mess to go through, so please re-send the patch split into reasonable chunks. Note that each patch needs to make sense and can be compiled and tested individually. signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH 0/5] hotplug: fix premature rebinding of VFIO devices to host
On Thu, 2017-06-29 at 15:59 -0500, Michael Roth wrote: > > > Patches 1-4 address 1) by deferring rebinding of a hostdev to the host > > > driver > > > until all the devices in the group have been detached, at which point all > > > the hostdevs are rebound as a group. Until that point, the devices are > > > traced > > > by the drvManager's inactiveList in a similar manner to hostdevs that are > > > assigned to VFIO via the nodedev-detach interface. > > > > What happens if libvirtd is restarted during this period? Is the > > inactiveList rebuilt with all the info necessary to assure that the > > nodedev-reattach does/doesn't happen (as appropriate) for all devices? > > Hmm, good question. > > The Unbindable() check only needs to know that nothing in the activeList > belongs to the group we're checking, and that list at least seems to get > rebuilt appropriately on restart. > > But the Unbind() relies on inactiveList and the behavior there is what > nodedev-detach currently does, which is to add it to inactive list while > libvirtd is running, and then just forget about it when libvirtd restarts. > For nodedev-detach it's fine since virHostdevPreparePCIDevices() re-adds > it as needed in the device-attach path. But yah, for this purpose it ends > up losing track of hostdevs that are still pending rebind to the host, and > that means some devices may not get rebound at the appropriate time if > there was a libvirtd restart. > > Unlike with device-attach, we can't just re-add it on-demand because we > actually need to know whether or not it was previously in the list. So > I think we'd need to add some persistent state to track this. Will look > into adding handling for that. FWIW last time a tried to attack this issue[1] I got pretty much as far as this, eg. the code worked as intended but restarting libvirtd would result in an inconsistent state which prevented you from performing some operations. Unfortunately I got sidetracked by other work and stopped just short of implementing a way to persist the relevant information on disk :( [1] ~1.5 years ago, according to git log -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
