Re: [libvirt] [PATCH v2 RESEND 00/12] PCI passthrough support on s390

[Cornelia Huck]

On Tue, 24 Jul 2018 10:20:48 +0200
Andrea Bolognani  wrote:

> Looking at both the generated QEMU command line and the qemu-devel
> message linked above, I seem to understand the zpci device is
> basically some sort of adapter that sits between a regular PCI
> device (emulated or otherwise) and an s390 guest and presents an
> ID-based interface to the latter; so IIUC the s390 guest doesn't
> actually see a PCI device, but a couple of IDs that can (somehow)
> be used to access the underlying PCI device.
> 
> From whatever little s390 knowledge I have, I recall the
> architecture using peculiar ways to address and access devices, so
> PCI not being usable wouldn't surprise me that much; what I find
> odd, however, is that regular PCI seems to be available at least
> on the host side, because otherwise stuff like
> 
>   -device zpci,uid=1,fid=1,target=vpci0
>   -device vfio-pci,host=:00:00.0,id=vpci0
> 
> wouldn't be possible. So, would anyone with s390 knowledge please
> spend a few words explaining how the various pieces fit together?

For some hints, let me point to
https://virtualpenguins.blogspot.de/2018/02/notes-on-pci-on-s390x.html
-- for implementation details, I'll point to the IBM folks :)

> 
> Assuming the above is a correct reading of the situation, it
> would seem the IDs are the only guest-visible part that we need
> to make sure doesn't change during the lifetime of the guest,
> while the usual bus/slot/function triplet assigned to the
> underlying PCI device doesn't actually matter. And if that's the
> case, wouldn't something like
> 
>   
> 
> be a better representation of the arrangement? We could leave it
> up to QEMU to assign addresses to the PCI devices in this case...
> But maybe they still matter for things such as migration? Or maybe
> I've just gotten it wrong altogether? :)
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v1 01/40] util: cgroup: modify virCgroupFree to take virCgroupPtr

[Erik Skultety]

On Mon, Jul 23, 2018 at 08:19:21PM +0530, Sukrit Bhatnagar wrote:
> On Mon, 23 Jul 2018 at 16:29, Erik Skultety  wrote:
> >
> > On Sat, Jul 21, 2018 at 05:36:33PM +0530, Sukrit Bhatnagar wrote:
> > > Modify virCgroupFree function signature to take a value of type
> > > virCgroupPtr instead of virCgroupPtr * as the parameter.
> > >
> > > Change the argument type in all calls to virCgroupFree function
> > > from virCgroupPtr * to virCgroupPtr.
> >
> > ^This sentence doesn't add any useful information. Instead, the commit 
> > message
> > should add information about why we're performing this change, i.e. in 
> > order to
> > enable usage of VIR_AUTOPTR with cgroup module or something alike.
> > Also, this patch is oddly placed, IMHO it should come before patch 8, where 
> > the
> > other work on cgroup module is done.
> >
> > With that:
> > Reviewed-by: Erik Skultety 
> >
> > ...
> >
> > > @@ -1379,8 +1379,8 @@ virCgroupNewPartition(const char *path,
> > >  ret = 0;
> > >   cleanup:
> > >  if (ret != 0)
> > > -virCgroupFree(group);
> > > -virCgroupFree();
> > > +virCgroupFree(*group);
> > > +virCgroupFree(parent);
> >
> > Since you're already touching the code, I'd appreciate another "adjustment"
> > patch where occurrences like ^this will be replaced by a VIR_STEAL_PTR, IOW
> > where we're passing a reference to a pointer in order to change the original
> > pointer, we should use a VIR_STEAL_PTR just before the cleanup section, so 
> > that
> > we don't need an if (ret != 0) or if (ret < 0) check only to conditionally 
> > do
> > some cleanup. Feel free to let me know if none of what I just wrote is 
> > clear.
>
> I am assuming that you are referring to `group` variable. If so, then I cannot
> apply cleanup attribute to function parameters and `group` is one of them.

I didn't mean using a cleanup attribute. What I meant was making the necessary
adjustments in order to get rid of the 'if(ret != 0)' check, since you're
already touching the code I thought why not going one step further...

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v1 15/40] util: firewall: use VIR_AUTOFREE instead of VIR_FREE for scalar types

[Erik Skultety]

On Mon, Jul 23, 2018 at 11:18:07PM +0530, Sukrit Bhatnagar wrote:
> On Mon, 23 Jul 2018 at 18:39, Erik Skultety  wrote:
> >
> > On Sat, Jul 21, 2018 at 05:36:47PM +0530, Sukrit Bhatnagar wrote:
> > > By making use of GNU C's cleanup attribute handled by the
> > > VIR_AUTOFREE macro for declaring scalar variables, majority
> > > of the VIR_FREE calls can be dropped, which in turn leads to
> > > getting rid of most of our cleanup sections.
> > >
> > > Signed-off-by: Sukrit Bhatnagar 
> > > ---
> > >  src/util/virfirewall.c | 16 +---
> > >  1 file changed, 5 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
> > > index dfd792f..b4a4d06 100644
> > > --- a/src/util/virfirewall.c
> > > +++ b/src/util/virfirewall.c
> > > @@ -511,7 +511,7 @@ void virFirewallRuleAddArgFormat(virFirewallPtr 
> > > firewall,
> > >   virFirewallRulePtr rule,
> > >   const char *fmt, ...)
> > >  {
> > > -char *arg;
> > > +VIR_AUTOFREE(char *) arg = NULL;
> > >  va_list list;
> > >
> > >  VIR_FIREWALL_RULE_RETURN_IF_ERROR(firewall, rule);
> > > @@ -525,13 +525,11 @@ void virFirewallRuleAddArgFormat(virFirewallPtr 
> > > firewall,
> > >
> > >  va_end(list);
> > >
> > > -VIR_FREE(arg);
> > >  return;
> > >
> > >   no_memory:
> > >  firewall->err = ENOMEM;
> > >  va_end(list);
> > > -VIR_FREE(arg);
> >
> > There could be an additional patch replacing the no_memory label with 
> > 'cleanup'
> > with the obvious adjustments.
>
> There are many functions in virfirewall.c where no_memory is used
> instead of cleanup.
> So I should change either all of them, or none of them.

Yep, so it should be all of them.

Erik

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 RESEND 00/12] PCI passthrough support on s390

[Andrea Bolognani]

On Mon, 2018-07-23 at 13:37 +0200, Ján Tomko wrote:
> On Tue, Jul 10, 2018 at 04:02:14PM +0800, Yi Min Zhao wrote:
> > Abstract
> > 
> > The PCI representation in QEMU has recently been extended for S390
> > allowing configuration of zPCI attributes like uid (user-defined
> > identifier) and fid (PCI function identifier).
> > The details can be found here:
> > https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg07262.html
> > 
> > To support the new zPCI feature of the S390 platform, two new XML
> > attributes, @uid and @fid, are introduced for device addresses of type
> > 'pci', i.e.:
> >  
> >
> >
> >  
> >
> > >  uid='0x0003' fid='0x0027'/>
> >  
> > 
> > uid and fid are optional attributes. If they are defined by the user,
> > unique values within the guest domain must be used. If they are not
> > specified and the architecture requires them, they are automatically
> > generated with non-conflicting values.
> > 
> > Current implementation is the most seamless one for the user as it
> > unites the address specific data of a PCI device on one XML element.
> > It could accommodate both specifying our special parameters (uid and fid)
> > and re-using standard statements (domain, bus, slot and function) for
> > PCI devices. User can still specify bus/slot/function for the virtualized
> > PCI devices in the XML.
> > 
> > Thus uid/fid act as an extension to the PCI address and are stored in
> > a new structure 'virZPCIDeviceAddress' which is a member of common PCI
> > Address structure. Additionally, two hashtables are used for assignment
> > and reservation of uid/fid.
> > 
> > In support of extending the PCI address, a new PCI address extension flag is
> > introduced. This extension flag allows is not only dedicated for the S390
> > platform but also other architectures needing certain extensions to PCI
> > address space.
[...]
> 
> The patches look OK to me, therefore:
> 
> Reviewed-by: Ján Tomko 
> 
> But I'd like to hear from some other developer who touched the PCI code
> last (Laine? Andrea?)

Thanks for bringing this to my attention - it had somehow managed
to escape my radar so far :)

I've quickly gone through the patches and spotted some minor code
style issues that I'd like to see addressed (I'll point them out
separately); first, though, I have a couple of questions about the
general idea behind the feature.

Looking at both the generated QEMU command line and the qemu-devel
message linked above, I seem to understand the zpci device is
basically some sort of adapter that sits between a regular PCI
device (emulated or otherwise) and an s390 guest and presents an
ID-based interface to the latter; so IIUC the s390 guest doesn't
actually see a PCI device, but a couple of IDs that can (somehow)
be used to access the underlying PCI device.

>From whatever little s390 knowledge I have, I recall the
architecture using peculiar ways to address and access devices, so
PCI not being usable wouldn't surprise me that much; what I find
odd, however, is that regular PCI seems to be available at least
on the host side, because otherwise stuff like

  -device zpci,uid=1,fid=1,target=vpci0
  -device vfio-pci,host=:00:00.0,id=vpci0

wouldn't be possible. So, would anyone with s390 knowledge please
spend a few words explaining how the various pieces fit together?

Assuming the above is a correct reading of the situation, it
would seem the IDs are the only guest-visible part that we need
to make sure doesn't change during the lifetime of the guest,
while the usual bus/slot/function triplet assigned to the
underlying PCI device doesn't actually matter. And if that's the
case, wouldn't something like

  

be a better representation of the arrangement? We could leave it
up to QEMU to assign addresses to the PCI devices in this case...
But maybe they still matter for things such as migration? Or maybe
I've just gotten it wrong altogether? :)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 07/10] viriscsitest: Test virISCSIConnectionLogin

[Michal Privoznik]

On 07/23/2018 03:55 PM, John Ferlan wrote:
> 
> 
> On 07/23/2018 08:34 AM, Michal Prívozník wrote:
>> On 07/23/2018 02:12 PM, John Ferlan wrote:
>>>
>>>
>>> On 07/23/2018 04:01 AM, Michal Prívozník wrote:
 On 07/17/2018 09:14 PM, John Ferlan wrote:
>
>
> On 07/04/2018 05:23 AM, Michal Privoznik wrote:
>> Introduce one basic test that tests the simplest case:
>> logging into portal without any IQN.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  tests/viriscsitest.c | 46 ++
>>  1 file changed, 46 insertions(+)
>>
>> diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c
>> index 3bb3993196..a7287069ba 100644
>> --- a/tests/viriscsitest.c
>> +++ b/tests/viriscsitest.c
>> @@ -94,6 +94,16 @@ static void testIscsiadmCb(const char *const*args,
>> args[8] && STREQ(args[8], "nonpersistent") &&
>> args[9] == NULL) {
>>  ignore_value(VIR_STRDUP(*output, iscsiadmSendtargetsOutput));
>>>
>>> here we do some sort of comparison
>>
>> What comparison? The only comparison I see is "args[X] && STREQ(args[X],
>> ...)".
>>
>> If you're referring to VIR_STRDUP() that is setting the pretended output
>> of the iscsiadm command. It's not comparing anything.
>>
> 
> Can you just indicate rather than "nada" what the actual deal is here?

Okay, how about "/* no output */" instead of "nada"?

> That is, in our mocked environment when running this command we don't
> expect to get any output from iscsiadm. If this were a real command then
> the following would be returned:
> > ...
> 
> In my saved iSCSI output file, I have for example:
> 
> iscsiadm -m node -T iqn.2013-12.com.example:iscsi-chap-pool -p
> 192.168.122.1 --login
> 
> returning:
> 
> Logging in to [iface: default, target:
> iqn.2013-12.com.example:iscsi-chap-pool, portal: 192.168.122.1,3260]
> (multiple)
> Login to [iface: default, target:
> iqn.2013-12.com.example:iscsi-chap-pool, portal: 192.168.122.1,3260]
> successful.

But this output is never processed by our code, therefore it doesn't
make much sense for our 'mock' to produce any.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 0/4] New mdev type handling for aggregated resources

[Alex Williamson]

On Fri, 20 Jul 2018 10:19:24 +0800
Zhenyu Wang  wrote:

> Current mdev device create interface depends on fixed mdev type, which get 
> uuid
> from user to create instance of mdev device. If user wants to use customized
> number of resource for mdev device, then only can create new mdev type for 
> that
> which may not be flexible. This requirement comes not only from to be able to
> allocate flexible resources for KVMGT, but also from Intel scalable IO
> virtualization which would use vfio/mdev to be able to allocate arbitrary
> resources on mdev instance. More info on [1] [2] [3].
> 
> To allow to create user defined resources for mdev, it trys to extend mdev
> create interface by adding new "instances=xxx" parameter following uuid, for
> target mdev type if aggregation is supported, it can create new mdev device
> which contains resources combined by number of instances, e.g
> 
> echo ",instances=10" > create
> 
> VM manager e.g libvirt can check mdev type with "aggregation" attribute which
> can support this setting. If no "aggregation" attribute found for mdev type,
> previous behavior is still kept for one instance allocation. And new sysfs
> attribute "instances" is created for each mdev device to show allocated 
> number.
> 
> This trys to create new KVMGT type with minimal vGPU resources which can be
> combined with "instances=x" setting to allocate for user wanted resources.

"instances" makes me think this is arg helps to create multiple mdev
instances rather than consuming multiple instances for a single mdev.
You're already exposing the "aggregation" attribute, so doesn't
"aggregate" perhaps make more sense as the create option?  We're asking
the driver to aggregate $NUM instances into a single mdev.  The mdev
attribute should then perhaps also be "aggregated_instances".

The next user question for the interface might be what aspect of the
device gets multiplied by this aggregation?  In i915 I see you're
multiplying the memory sizes by the instance, but clearly the
resolution doesn't change.  I assume this is sort of like mdev types
themselves, ie. some degree of what a type means is buried in the
implementation and some degree of what some number of those types
aggregated together means is impossible to describe generically.

We're also going to need to add aggregation to the checklist for device
compatibility for migration, for example 1) is it the same mdev_type,
1a) are the aggregation counts the same (new), 2) is the host driver
compatible (TBD).

The count handling in create_store(), particularly MDEV_CREATE_OPT_LEN
looks a little suspicious.  I think we should just be validating that
the string before the ',' or the entire string if there is no comma is
UUID length.  Pass only that to uuid_le_to_bin().  We can then strncmp
as you have for "instances=" (or "aggregate=") but then let's be sure
to end the string we pass to kstrtouint(), ie. assume there could be
further args.

Documentation/ABI/testing/sysfs-bus-vfio-mdev also needs to be updated
with this series.

I'm curious what libvirt folks and Kirti think of this, it looks like
it has a nice degree of backwards compatibility, both in the sysfs
interface and the vendor driver interface.  Thanks,

Alex

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 3/3] storage: Implement iscsi_direct pool backend

[Pavel Hrdina]

On Mon, Jul 23, 2018 at 08:43:01PM +0200, c...@lse.epita.fr wrote:
> From: Clementine Hayat 
> 
> We need here libiscsi for the storgae pool backend.
> For the iscsi-direct storage pool, only checkPool and refreshPool should
> be necessary.
> The pool is state-less and just need the informations within the volume
> to work.
> 
> Signed-off-by: Clementine Hayat 
> ---
>  m4/virt-storage-iscsi-direct.m4|   3 +
>  src/storage/Makefile.inc.am|   2 +
>  src/storage/storage_backend_iscsi_direct.c | 408 -
>  3 files changed, 410 insertions(+), 3 deletions(-)
> 
> diff --git a/m4/virt-storage-iscsi-direct.m4 b/m4/virt-storage-iscsi-direct.m4
> index cc2d490352..dab4414169 100644
> --- a/m4/virt-storage-iscsi-direct.m4
> +++ b/m4/virt-storage-iscsi-direct.m4
> @@ -29,6 +29,9 @@ AC_DEFUN([LIBVIRT_STORAGE_CHECK_ISCSI_DIRECT], [
>  with_storage_iscsi_direct=$with_libiscsi
>fi
>if test "$with_storage_iscsi_direct" = "yes"; then
> +if test "$with_libiscsi" = "no"; then
> +  AC_MSG_ERROR([Need libiscsi for iscsi-direct storage driver])
> +fi
>  AC_DEFINE_UNQUOTED([WITH_STORAGE_ISCSI_DIRECT], [1],
> [whether iSCSI backend for storage driver is enabled])
>fi
> diff --git a/src/storage/Makefile.inc.am b/src/storage/Makefile.inc.am
> index b2714fd960..bd5ea06f8b 100644
> --- a/src/storage/Makefile.inc.am
> +++ b/src/storage/Makefile.inc.am
> @@ -203,6 +203,7 @@ if WITH_STORAGE_ISCSI_DIRECT
>  libvirt_storage_backend_iscsi_direct_la_SOURCES = 
> $(STORAGE_DRIVER_ISCSI_DIRECT_SOURCES)
>  libvirt_storage_backend_iscsi_direct_la_CFLAGS = \
>   -I$(srcdir)/conf \
> + -I$(srcdir)/secret \
>   $(LIBISCSI_CFLAGS) \
>   $(AM_CFLAGS) \
>   $(NULL)
> @@ -211,6 +212,7 @@ storagebackend_LTLIBRARIES += 
> libvirt_storage_backend_iscsi-direct.la
>  libvirt_storage_backend_iscsi_direct_la_LDFLAGS = $(AM_LDFLAGS_MOD)
>  libvirt_storage_backend_iscsi_direct_la_LIBADD = \
>   libvirt.la \
> + $(LIBISCSI_LIBS) \
>   ../gnulib/lib/libgnu.la \
>   $(NULL)
>  endif WITH_STORAGE_ISCSI_DIRECT
> diff --git a/src/storage/storage_backend_iscsi_direct.c 
> b/src/storage/storage_backend_iscsi_direct.c
> index 94c4c989ff..b9a9bb3eb0 100644
> --- a/src/storage/storage_backend_iscsi_direct.c
> +++ b/src/storage/storage_backend_iscsi_direct.c
> @@ -22,28 +22,430 @@
>  
>  #include 
>  
> +#include 
> +#include 
> +
> +#include "datatypes.h"
> +#include "secret_util.h"
>  #include "storage_backend_iscsi_direct.h"
>  #include "storage_util.h"
> +#include "viralloc.h"
> +#include "virerror.h"
>  #include "virlog.h"
> +#include "virobject.h"
> +#include "virstring.h"
> +#include "virtime.h"
> +#include "viruuid.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_STORAGE
>  
> +#define ISCSI_DEFAULT_TARGET_PORT 3260
> +#define VIR_ISCSI_TEST_UNIT_TIMEOUT 30 * 1000
> +
>  VIR_LOG_INIT("storage.storage_backend_iscsi_direct");
>  
> +static struct iscsi_context *
> +virISCSIDirectCreateContext(const char* initiator_iqn)
> +{
> +struct iscsi_context *iscsi = NULL;
> +
> +iscsi = iscsi_create_context(initiator_iqn);
> +if (!iscsi)
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Failed to create iscsi context for %s"),
> +   initiator_iqn);
> +return iscsi;
> +}
> +
> +static char *
> +virStorageBackendISCSIDirectPortal(virStoragePoolSourcePtr source)
> +{
> +char *portal = NULL;
> +
> +if (source->nhost != 1) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("Expected exactly 1 host for the storage pool"));
> +return NULL;
> +}
> +if (source->hosts[0].port == 0) {
> +ignore_value(virAsprintf(, "%s:%d",
> + source->hosts[0].name,
> + ISCSI_DEFAULT_TARGET_PORT));
> +} else if (strchr(source->hosts[0].name, ':')) {
> +ignore_value(virAsprintf(, "[%s]:%d",
> + source->hosts[0].name,
> + source->hosts[0].port));
> +} else {
> +ignore_value(virAsprintf(, "%s:%d",
> + source->hosts[0].name,
> + source->hosts[0].port));
> +}
> +return portal;
> +}
> +
> +static int
> +virStorageBackendISCSIDirectSetAuth(struct iscsi_context *iscsi,
> +virStoragePoolSourcePtr source)
> +{
> +unsigned char *secret_value = NULL;
> +size_t secret_size;
> +virStorageAuthDefPtr authdef = source->auth;
> +int ret = -1;
> +virConnectPtr conn = NULL;
> +
> +if (!authdef || authdef->authType == VIR_STORAGE_AUTH_TYPE_NONE)
> +return 0;
> +
> +VIR_DEBUG("username='%s' authType=%d seclookupdef.type=%d",
> +  authdef->username, authdef->authType, 
> authdef->seclookupdef.type);
> +
> +if (authdef->authType != VIR_STORAGE_AUTH_TYPE_CHAP) 

Re: [libvirt] [PATCH v2 2/3] storage: Introduce iscsi_direct pool type

[Pavel Hrdina]

On Mon, Jul 23, 2018 at 08:43:00PM +0200, c...@lse.epita.fr wrote:
> From: Clementine Hayat 
> 
> Introducing the pool as a noop. Integration inside the build
> system. Implementation will be in the following commits.
> 
> Signed-off-by: Clementine Hayat 
> ---
>  configure.ac   |  6 ++-
>  m4/virt-storage-iscsi-direct.m4| 41 +++
>  src/conf/domain_conf.c |  4 ++
>  src/conf/storage_conf.c| 33 ++--
>  src/conf/storage_conf.h|  1 +
>  src/conf/virstorageobj.c   |  2 +
>  src/storage/Makefile.inc.am| 22 
>  src/storage/storage_backend.c  |  6 +++
>  src/storage/storage_backend_iscsi_direct.c | 58 ++
>  src/storage/storage_backend_iscsi_direct.h |  6 +++
>  src/storage/storage_driver.c   |  1 +
>  tools/virsh-pool.c |  3 ++
>  12 files changed, 178 insertions(+), 5 deletions(-)
>  create mode 100644 m4/virt-storage-iscsi-direct.m4
>  create mode 100644 src/storage/storage_backend_iscsi_direct.c
>  create mode 100644 src/storage/storage_backend_iscsi_direct.h
> 
> diff --git a/configure.ac b/configure.ac
> index c668630a79..87ac4dc2c3 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -564,6 +564,7 @@ LIBVIRT_STORAGE_ARG_DIR
>  LIBVIRT_STORAGE_ARG_FS
>  LIBVIRT_STORAGE_ARG_LVM
>  LIBVIRT_STORAGE_ARG_ISCSI
> +LIBVIRT_STORAGE_ARG_ISCSI_DIRECT
>  LIBVIRT_STORAGE_ARG_SCSI
>  LIBVIRT_STORAGE_ARG_MPATH
>  LIBVIRT_STORAGE_ARG_DISK
> @@ -578,6 +579,7 @@ if test "$with_libvirtd" = "no"; then
>with_storage_fs=no
>with_storage_lvm=no
>with_storage_iscsi=no
> +  with_storage_iscsi_direct=no
>with_storage_scsi=no
>with_storage_mpath=no
>with_storage_disk=no
> @@ -598,6 +600,7 @@ LIBVIRT_STORAGE_CHECK_DIR
>  LIBVIRT_STORAGE_CHECK_FS
>  LIBVIRT_STORAGE_CHECK_LVM
>  LIBVIRT_STORAGE_CHECK_ISCSI
> +LIBVIRT_STORAGE_CHECK_ISCSI_DIRECT
>  LIBVIRT_STORAGE_CHECK_SCSI
>  LIBVIRT_STORAGE_CHECK_MPATH
>  LIBVIRT_STORAGE_CHECK_DISK
> @@ -608,7 +611,7 @@ LIBVIRT_STORAGE_CHECK_ZFS
>  LIBVIRT_STORAGE_CHECK_VSTORAGE
>  
>  with_storage=no
> -for backend in dir fs lvm iscsi scsi mpath rbd disk; do
> +for backend in dir fs lvm iscsi iscsi_direct scsi mpath rbd disk; do
>  if eval test \$with_storage_$backend = yes; then
>  with_storage=yes
>  break
> @@ -936,6 +939,7 @@ LIBVIRT_STORAGE_RESULT_DIR
>  LIBVIRT_STORAGE_RESULT_FS
>  LIBVIRT_STORAGE_RESULT_LVM
>  LIBVIRT_STORAGE_RESULT_ISCSI
> +LIBVIRT_STORAGE_RESULT_ISCSI_DIRECT
>  LIBVIRT_STORAGE_RESULT_SCSI
>  LIBVIRT_STORAGE_RESULT_MPATH
>  LIBVIRT_STORAGE_RESULT_DISK
> diff --git a/m4/virt-storage-iscsi-direct.m4 b/m4/virt-storage-iscsi-direct.m4
> new file mode 100644
> index 00..cc2d490352
> --- /dev/null
> +++ b/m4/virt-storage-iscsi-direct.m4
> @@ -0,0 +1,41 @@
> +dnl Iscsi-direct storage
> +dnl
> +dnl Copyright (C) 2018 Clementine Hayat.
> +dnl
> +dnl This library is free software; you can redistribute it and/or
> +dnl modify it under the terms of the GNU Lesser General Public
> +dnl License as published by the Free Software Foundation; either
> +dnl version 2.1 of the License, or (at your option) any later version.
> +dnl
> +dnl This library is distributed in the hope that it will be useful,
> +dnl but WITHOUT ANY WARRANTY; without even the implied warranty of
> +dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +dnl Lesser General Public License for more details.
> +dnl
> +dnl You should have received a copy of the GNU Lesser General Public
> +dnl License along with this library.  If not, see
> +dnl .
> +dnl
> +
> +AC_DEFUN([LIBVIRT_STORAGE_ARG_ISCSI_DIRECT], [
> +  LIBVIRT_ARG_WITH_FEATURE([STORAGE_ISCSI_DIRECT],
> +   [iscsi-direct backend for the storage driver],
> +   [check])
> +])
> +
> +AC_DEFUN([LIBVIRT_STORAGE_CHECK_ISCSI_DIRECT], [
> +  AC_REQUIRE([LIBVIRT_CHECK_LIBISCSI])
> +  if test "$with_storage_iscsi_direct" = "check"; then
> +with_storage_iscsi_direct=$with_libiscsi
> +  fi
> +  if test "$with_storage_iscsi_direct" = "yes"; then
> +AC_DEFINE_UNQUOTED([WITH_STORAGE_ISCSI_DIRECT], [1],
> +   [whether iSCSI backend for storage driver is enabled])
> +  fi
> +  AM_CONDITIONAL([WITH_STORAGE_ISCSI_DIRECT],
> + [test "$with_storage_iscsi_direct" = "yes"])
> +])
> +
> +AC_DEFUN([LIBVIRT_STORAGE_RESULT_ISCSI_DIRECT], [
> +  LIBVIRT_RESULT([iscsi-direct], [$with_storage_iscsi_direct])
> +])
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 7396616eda..5af27a6ad2 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -30163,6 +30163,10 @@ virDomainDiskTranslateSourcePool(virDomainDiskDefPtr 
> def)
>  
>  break;
>  
> +case VIR_STORAGE_POOL_ISCSI_DIRECT:
> +def->src->srcpool->mode = 

Re: [libvirt] [PATCH v3 0/3] Alter qemu live/cold device attach algorithm

[John Ferlan]

ping.

Tks,

John

On 07/16/2018 05:14 PM, John Ferlan wrote:
> v2: https://www.redhat.com/archives/libvir-list/2018-July/msg00361.html
> 
> Differences to v2:
> 
> Patch1: NEW - As a result of code review the suggestion was to utilize
> the virDomainDefCompatibleDevice in order to make the check more
> generic to include 's which were also afflicted with the
> same problem that I was trying to solve with the former patch1
> just for 's. However, this led me down into the abyss of
> more changes since 's have multiple 
> target bus types (IDE and SCSI). That means we need to have a
> mechanism to pass the target bus along so that a SCSI drive
> address doesn't inadvertently match an IDE drive address. All
> that is complicated by the way virDomainDefHasDeviceAddress
> iterates through all the device lists.  Whether there are more
> similar devices I'm assuming will fall out of code review.
> 
> Patch2: This moves the virDomainDefHasDeviceAddress into the more
> common config checking virDomainDefCompatibleDevice method,
> but now needs to also account for the disk bus issue.
> 
> This theoretically could be combined with Patch1, but keeping
> them separate I would hope makes for simpler code review. I
> could also move code out of virDomainDefHasDeviceAddressIterator
> into patch2, but if just felt better in patch1.
> 
> Patch3: No changes were made (amazingly so).
> 
> In the end quite a bit more complicated
> 
> John Ferlan (3):
>   conf: Add @target_bus to virDomainDefHasDeviceAddress
>   qemu: Check for existing address when cold attach device
>   qemu: Use the correct vm def on cold attach
> 
>  src/conf/domain_conf.c | 54 +++---
>  src/conf/domain_conf.h |  3 ++-
>  src/qemu/qemu_driver.c | 59 --
>  3 files changed, 75 insertions(+), 41 deletions(-)
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [dbus PATCH v2 10/17] Implement MAC property for Interface Interface

[Katerina Koukiou]

On Fri, Jul 20, 2018 at 02:34:54PM -0400, Anya Harter wrote:
> Signed-off-by: Anya Harter 
> ---
>  data/org.libvirt.Interface.xml |  5 +
>  src/interface.c| 22 ++
>  tests/test_interface.py|  1 +
>  3 files changed, 28 insertions(+)

Reviewed-by: Katerina Koukiou 


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH for-3.1] s390x: remove 's390-squash-mcss' option

[Peter Krempa]

On Tue, Jul 24, 2018 at 11:24:13 +0200, Cornelia Huck wrote:
> This option has been deprecated for two releases; remove it.
> 
> Signed-off-by: Cornelia Huck 
> ---
>  hw/s390x/3270-ccw.c|  2 +-
>  hw/s390x/css-bridge.c  |  1 -
>  hw/s390x/css.c |  6 ++
>  hw/s390x/s390-ccw.c|  2 +-
>  hw/s390x/s390-virtio-ccw.c | 37 ++---
>  hw/s390x/virtio-ccw.c  |  2 +-
>  include/hw/s390x/css-bridge.h  |  1 -
>  include/hw/s390x/css.h |  9 +++--
>  include/hw/s390x/s390-virtio-ccw.h |  1 -
>  qemu-deprecated.texi   |  8 
>  qemu-options.hx| 10 --
>  target/s390x/cpu.c | 10 --
>  target/s390x/cpu.h |  1 -
>  13 files changed, 10 insertions(+), 80 deletions(-)

Looks like libvirt did not use this option at all according to my brief
grepping, so it should be okay to remove it.


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 1/3] tests: qemuxml2argv: make CAPS_LATEST arch generic

[Boris Fiuczynski]

From: Bjoern Walk 

Testing with the latest capabilities has been x86_64 centric. Let's
remove the hardcoded architecture and give the user the ability to
specify the desired architecture in the macro.

Signed-off-by: Bjoern Walk 
Reviewed-by: Boris Fiuczynski 
---
 tests/qemuxml2argvtest.c | 96 ++--
 1 file changed, 54 insertions(+), 42 deletions(-)

diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 1a936faef1..097dc6be51 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -621,10 +621,11 @@ testCompareXMLToArgv(const void *data)
 static int
 mymain(void)
 {
-int ret = 0;
+int ret = 0, i;
 char *fakerootdir;
 bool skipLegacyCPUs = false;
-char *capslatest_x86_64 = NULL;
+const char *archs[] = { "x86_64", "s390x" };
+virHashTablePtr capslatest = NULL;
 
 if (VIR_STRDUP_QUIET(fakerootdir, FAKEROOTDIRTEMPLATE) < 0) {
 fprintf(stderr, "Out of memory\n");
@@ -693,12 +694,23 @@ mymain(void)
 if (VIR_STRDUP_QUIET(driver.config->memoryBackingDir, 
"/var/lib/libvirt/qemu/ram") < 0)
 return EXIT_FAILURE;
 
-if (!(capslatest_x86_64 = testQemuGetLatestCapsForArch(abs_srcdir 
"/qemucapabilitiesdata",
-   "x86_64", "xml")))
+capslatest = virHashCreate(4, virHashValueFree);
+if (!capslatest)
 return EXIT_FAILURE;
 
-VIR_TEST_VERBOSE("\nlatest caps x86_64: %s\n", capslatest_x86_64);
+VIR_TEST_VERBOSE("\n");
 
+for (i = 0; i < ARRAY_CARDINALITY(archs); ++i) {
+char *cap = testQemuGetLatestCapsForArch(abs_srcdir 
"/qemucapabilitiesdata",
+ archs[i], "xml");
+
+if (!cap || virHashAddEntry(capslatest, archs[i], cap) < 0)
+return EXIT_FAILURE;
+
+VIR_TEST_VERBOSE("latest caps for %s: %s\n", archs[i], cap);
+}
+
+VIR_TEST_VERBOSE("\n");
 
 /**
  * The following set of macros allows testing of XML -> argv conversion with a
@@ -746,9 +758,9 @@ mymain(void)
 # define DO_TEST_CAPS_VER(name, ver) \
 DO_TEST_CAPS_ARCH_VER(name, "x86_64", ver)
 
-# define DO_TEST_CAPS_LATEST(name) \
-DO_TEST_CAPS_INTERNAL(name, "x86_64-latest", NULL, 0, 0, "x86_64", \
-  capslatest_x86_64, true)
+# define DO_TEST_CAPS_LATEST(name, arch) \
+DO_TEST_CAPS_INTERNAL(name, arch "-latest", NULL, 0, 0, arch, \
+  virHashLookup(capslatest, arch), true)
 
 /**
  * The following test macros should be used only in cases when the tests 
require
@@ -822,8 +834,8 @@ mymain(void)
 DO_TEST_PARSE_ERROR("minimal-no-memory", NONE);
 DO_TEST("minimal-msg-timestamp", QEMU_CAPS_MSG_TIMESTAMP);
 
-DO_TEST_CAPS_LATEST("genid");
-DO_TEST_CAPS_LATEST("genid-auto");
+DO_TEST_CAPS_LATEST("genid", "x86_64");
+DO_TEST_CAPS_LATEST("genid-auto", "x86_64");
 
 DO_TEST("machine-aliases1", NONE);
 DO_TEST("machine-aliases2", QEMU_CAPS_KVM);
@@ -989,15 +1001,15 @@ mymain(void)
 QEMU_CAPS_VIRTIO_SCSI);
 DO_TEST("nosharepages", QEMU_CAPS_MEM_MERGE);
 DO_TEST("disk-cdrom", NONE);
-DO_TEST_CAPS_LATEST("disk-cdrom");
+DO_TEST_CAPS_LATEST("disk-cdrom", "x86_64");
 DO_TEST("disk-iscsi", NONE);
 DO_TEST("disk-cdrom-network", QEMU_CAPS_KVM);
-DO_TEST_CAPS_LATEST("disk-cdrom-network");
+DO_TEST_CAPS_LATEST("disk-cdrom-network", "x86_64");
 DO_TEST("disk-cdrom-tray",
 QEMU_CAPS_VIRTIO_TX_ALG);
-DO_TEST_CAPS_LATEST("disk-cdrom-tray");
+DO_TEST_CAPS_LATEST("disk-cdrom-tray", "x86_64");
 DO_TEST("disk-floppy", NONE);
-DO_TEST_CAPS_LATEST("disk-floppy");
+DO_TEST_CAPS_LATEST("disk-floppy", "x86_64");
 DO_TEST_FAILURE("disk-floppy-pseries",
 QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE);
 DO_TEST("disk-floppy-tray", NONE);
@@ -1020,47 +1032,47 @@ mymain(void)
 QEMU_CAPS_DRIVE_BOOT);
 DO_TEST("floppy-drive-fat",
 QEMU_CAPS_DRIVE_BOOT);
-DO_TEST_CAPS_LATEST("floppy-drive-fat");
+DO_TEST_CAPS_LATEST("floppy-drive-fat", "x86_64");
 DO_TEST("disk-readonly-disk", NONE);
-DO_TEST_CAPS_LATEST("disk-readonly-disk");
+DO_TEST_CAPS_LATEST("disk-readonly-disk", "x86_64");
 DO_TEST("disk-fmt-qcow",
 QEMU_CAPS_DRIVE_BOOT);
 DO_TEST_PARSE_ERROR("disk-fmt-cow", QEMU_CAPS_DRIVE_BOOT);
 DO_TEST_PARSE_ERROR("disk-fmt-dir", QEMU_CAPS_DRIVE_BOOT);
 DO_TEST_PARSE_ERROR("disk-fmt-iso", QEMU_CAPS_DRIVE_BOOT);
 DO_TEST("disk-shared", NONE);
-DO_TEST_CAPS_LATEST("disk-shared");
+DO_TEST_CAPS_LATEST("disk-shared", "x86_64");
 DO_TEST_PARSE_ERROR("disk-shared-qcow", NONE);
 DO_TEST("disk-shared-locking",
 QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_DISK_SHARE_RW);
 DO_TEST("disk-error-policy", NONE);
-DO_TEST_CAPS_LATEST("disk-error-policy");
+DO_TEST_CAPS_LATEST("disk-error-policy", "x86_64");
 DO_TEST("disk-cache", 

[libvirt] [PATCH v2 2/3] qemu: Add ccw support for vhost-vsock

[Boris Fiuczynski]

Add support and tests for vhost-vsock-ccw.

Signed-off-by: Boris Fiuczynski 
---
 src/qemu/qemu_command.c   |  8 +++--
 src/qemu/qemu_domain.c| 10 --
 src/qemu/qemu_domain_address.c|  7 +++-
 .../vhost-vsock-ccw-auto.s390x-latest.args| 32 +++
 .../qemuxml2argvdata/vhost-vsock-ccw-auto.xml | 25 +++
 .../vhost-vsock-ccw.s390x-latest.args | 32 +++
 tests/qemuxml2argvdata/vhost-vsock-ccw.xml| 32 +++
 tests/qemuxml2argvtest.c  |  2 ++
 .../vhost-vsock-ccw-auto.xml  | 32 +++
 tests/qemuxml2xmloutdata/vhost-vsock-ccw.xml  |  1 +
 tests/qemuxml2xmltest.c   |  5 +++
 11 files changed, 181 insertions(+), 5 deletions(-)
 create mode 100644 
tests/qemuxml2argvdata/vhost-vsock-ccw-auto.s390x-latest.args
 create mode 100644 tests/qemuxml2argvdata/vhost-vsock-ccw-auto.xml
 create mode 100644 tests/qemuxml2argvdata/vhost-vsock-ccw.s390x-latest.args
 create mode 100644 tests/qemuxml2argvdata/vhost-vsock-ccw.xml
 create mode 100644 tests/qemuxml2xmloutdata/vhost-vsock-ccw-auto.xml
 create mode 12 tests/qemuxml2xmloutdata/vhost-vsock-ccw.xml

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ae45c45b7f..fa645f3167 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -10217,10 +10217,14 @@ qemuBuildVsockDevStr(virDomainDefPtr def,
 {
 qemuDomainVsockPrivatePtr priv = 
(qemuDomainVsockPrivatePtr)vsock->privateData;
 virBuffer buf = VIR_BUFFER_INITIALIZER;
-const char *device = "vhost-vsock-pci";
 char *ret = NULL;
 
-virBufferAsprintf(, "%s", device);
+if (vsock->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
+virBufferAddLit(, "vhost-vsock-ccw");
+} else {
+virBufferAddLit(, "vhost-vsock-pci");
+}
+
 virBufferAsprintf(, ",id=%s", vsock->info.alias);
 virBufferAsprintf(, ",guest-cid=%u", vsock->guest_cid);
 virBufferAsprintf(, ",vhostfd=%s%u", fdprefix, priv->vhostfd);
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index de056272e8..6b50e0c484 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5546,7 +5546,8 @@ qemuDomainDeviceDefValidateMemory(const 
virDomainMemoryDef *memory ATTRIBUTE_UNU
 
 
 static int
-qemuDomainDeviceDefValidateVsock(const virDomainVsockDef *vsock 
ATTRIBUTE_UNUSED,
+qemuDomainDeviceDefValidateVsock(const virDomainVsockDef *vsock,
+ const virDomainDef *def,
  virQEMUCapsPtr qemuCaps)
 {
 if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VHOST_VSOCK)) {
@@ -,6 +5556,11 @@ qemuDomainDeviceDefValidateVsock(const virDomainVsockDef 
*vsock ATTRIBUTE_UNUSED
  "with this QEMU binary"));
 return -1;
 }
+
+if (!qemuDomainCheckCCWS390AddressSupport(def, vsock->info, qemuCaps,
+  "vsock"))
+return -1;
+
 return 0;
 }
 
@@ -5702,7 +5708,7 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev,
 break;
 
 case VIR_DOMAIN_DEVICE_VSOCK:
-ret = qemuDomainDeviceDefValidateVsock(dev->data.vsock, qemuCaps);
+ret = qemuDomainDeviceDefValidateVsock(dev->data.vsock, def, qemuCaps);
 break;
 
 case VIR_DOMAIN_DEVICE_TPM:
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 1210d4acdd..67de340358 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -306,7 +306,8 @@ qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def,
declare address-less virtio devices to be of address type 'type'
disks, networks, videos, consoles, controllers, memballoon and rng
in this order
-   if type is ccw filesystem devices are declared to be of address type ccw
+   if type is ccw filesystem and vsock devices are declared to be of
+   address type ccw
 */
 size_t i;
 
@@ -373,6 +374,10 @@ qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def,
 if (def->fss[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
 def->fss[i]->info.type = type;
 }
+if (def->vsock &&
+def->vsock->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
+def->vsock->info.type = type;
+}
 }
 }
 
diff --git a/tests/qemuxml2argvdata/vhost-vsock-ccw-auto.s390x-latest.args 
b/tests/qemuxml2argvdata/vhost-vsock-ccw-auto.s390x-latest.args
new file mode 100644
index 00..6092f8e85c
--- /dev/null
+++ b/tests/qemuxml2argvdata/vhost-vsock-ccw-auto.s390x-latest.args
@@ -0,0 +1,32 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-s390x \
+-name guest=QEMUGuest1,debug-threads=on \
+-S \
+-object secret,id=masterKey0,format=raw,\
+file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes 

[libvirt] [PATCH v2 0/3] qemu: Support vhost-vsock-ccw

[Boris Fiuczynski]

Support the vhost-vsock-ccw device on S390.

Since v1
 - adjusted vsock command line generation (Ján Tomko)
 - added CAPS_LATEST test coverage to s390 (Ján Tomko)
 - adjusted new vsock tests to use CAPS_LATEST

Bjoern Walk (1):
  tests: qemuxml2argv: make CAPS_LATEST arch generic

Boris Fiuczynski (2):
  qemu: Add ccw support for vhost-vsock
  news: Update for vhost-vsock-ccw support

 docs/news.xml |  8 ++
 src/qemu/qemu_command.c   |  8 +-
 src/qemu/qemu_domain.c| 10 +-
 src/qemu/qemu_domain_address.c|  7 +-
 .../vhost-vsock-ccw-auto.s390x-latest.args| 32 ++
 .../qemuxml2argvdata/vhost-vsock-ccw-auto.xml | 25 +
 .../vhost-vsock-ccw.s390x-latest.args | 32 ++
 tests/qemuxml2argvdata/vhost-vsock-ccw.xml| 32 ++
 tests/qemuxml2argvtest.c  | 98 +++
 .../vhost-vsock-ccw-auto.xml  | 32 ++
 tests/qemuxml2xmloutdata/vhost-vsock-ccw.xml  |  1 +
 tests/qemuxml2xmltest.c   |  5 +
 12 files changed, 243 insertions(+), 47 deletions(-)
 create mode 100644 
tests/qemuxml2argvdata/vhost-vsock-ccw-auto.s390x-latest.args
 create mode 100644 tests/qemuxml2argvdata/vhost-vsock-ccw-auto.xml
 create mode 100644 tests/qemuxml2argvdata/vhost-vsock-ccw.s390x-latest.args
 create mode 100644 tests/qemuxml2argvdata/vhost-vsock-ccw.xml
 create mode 100644 tests/qemuxml2xmloutdata/vhost-vsock-ccw-auto.xml
 create mode 12 tests/qemuxml2xmloutdata/vhost-vsock-ccw.xml

-- 
2.17.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 3/3] news: Update for vhost-vsock-ccw support

[Boris Fiuczynski]

Signed-off-by: Boris Fiuczynski 
---
 docs/news.xml | 8 
 1 file changed, 8 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml
index 20c2ff7a6f..4c70499d84 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -67,6 +67,14 @@
   only rendering devices within the guest.
 
   
+  
+
+  qemu: Add ccw support for vhost-vsock
+
+
+  Support the vhost-vsock-ccw device on S390.
+
+  
 
 
 
-- 
2.17.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [dbus PATCH v2 09/17] Implement Name property for Interface Interface

[Katerina Koukiou]

On Fri, Jul 20, 2018 at 02:34:53PM -0400, Anya Harter wrote:
> Signed-off-by: Anya Harter 
> ---
>  data/org.libvirt.Interface.xml |  5 +
>  src/interface.c| 22 ++
>  tests/test_interface.py|  8 
>  3 files changed, 35 insertions(+)

Reviewed-by: Katerina Koukiou 


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 10/10] viriscsitest: Extend virISCSIConnectionLogin test

[Michal Privoznik]

On 07/23/2018 03:10 PM, John Ferlan wrote:
> 
> 
> On 07/23/2018 04:01 AM, Michal Prívozník wrote:
>> On 07/17/2018 09:15 PM, John Ferlan wrote:
>>>
>>>
>>> On 07/04/2018 05:23 AM, Michal Privoznik wrote:
 Extend this existing test so that a case when IQN is provided is
 tested too. Since a special iSCSI interface is created and its
 name is randomly generated at runtime we need to link with
 virrandommock to have predictable names.

 Signed-off-by: Michal Privoznik 
 ---
  tests/viriscsitest.c | 75 
 ++--
  1 file changed, 73 insertions(+), 2 deletions(-)

>>>
>>> Should testIscsiadmCbData initialization get { false, false } now (and I
>>> should have mention in patch 9 that :
>>>
>>> +struct testIscsiadmCbData cbData = { 0 };
>>>
>>> should be
>>>
>>> +struct testIscsiadmCbData cbData = { false };>
>>> I know that 0 and false are synonymous, but syntactical correctness is
>>> in play here ;-)
>>
>> No. So { 0 } is basically a shortcut for:
>>
>> struct testIscsiadmCbData cbData;
>>
>> memset(, 0, sizeof(cbData));
> 
> Oh yeah, right  for literal interpretation of data.
> 
>>
>> It has nothing to do with the struct having only two bools (for now). It
>> is used widely in our code, because it has one great advantage: even if
>> we add/remove/rearrange items in the struct it is always going to be
>> zeroed out whereas if I initialized it like this:
>>
>> struct testIscsiadmCbData cbData = {false, false};
>>
>> and then somebody would append yet another member to the struct they
>> would either:
>>
>> a) have to change all the lines where the struct is initialized
>> (possibly touching functions that has nothing to do with actual change), or
>> b) forget to initialize it completely.
>>
>>
>> TL;DR It's just coincidence that the first member is a bool.
>>
>>>
>>> I also think you're doing two things here:
>>>
>>> 1. Generating a dryRun output for "existing" test without initiator.iqn
>>>
>>> 2. Adding tests to test initiator.iqn
>>>
>>
>> Sure. That's how login works. Firstly it lists output of iscsi
>> interfaces, then it creates a new one and then use that to log in. I
>> don't see a problem here.
>>
> 
> OK, right... Maybe it would have been useful from the literal review POV
> to be reminded of this...  While sometimes it's a pain to document the
> reason for something - it perhaps prevents this back and forth later on
> especially when it's "unique case" code paths.  IIQN isn't really well
> documented by any stretch...

Well, I usually read the code when trying to understand how something
works as our comments/documentation is often incorrect.

> 
 diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c
 index c6e0e3b8ff..55889d04a3 100644
 --- a/tests/viriscsitest.c
 +++ b/tests/viriscsitest.c
 @@ -60,8 +60,19 @@ const char *iscsiadmSendtargetsOutput =
  "10.20.30.40:3260,1 iqn.2008-04.example:example1:iscsi.bar\n"
  "10.20.30.40:3260,1 iqn.2009-04.example:example1:iscsi.seven\n";
  
 +const char *iscsiadmIfaceDefaultOutput =
 +"default tcp\n"
 +"iser iser\n";
 +
 +const char *iscsiadmIfaceIfaceOutput =
 +"default tcp\n"
 +"iser iser\n"
 +"libvirt-iface-03020100 
 tcpiqn.2004-06.example:example1:initiator\n";
 +
 +
  struct testIscsiadmCbData {
  bool output_version;
 +bool iface_created;
  };
  
  static void testIscsiadmCb(const char *const*args,
 @@ -103,6 +114,62 @@ static void testIscsiadmCb(const char *const*args,
 args[7] && STREQ(args[7], "--login") &&
 args[8] == NULL) {
  /* nada */
>>>
>>> Is this "nada"
>>
>> Yes, this is "nada" ;-)
>>
> 
> Maybe the explanation as to why it's nada and not just leaving nada
> there would have helped.

As I say in the other reply, how about s/nada/no output/?

> 
> In some instances we're doing output comparison (eventually) and in
> others we're not. In the long run it's mainly a matter of being reminded
> what the processing is and what (if any) the expected output is.
> Sometimes that expected output only occurs on the next query, IIRC.

Yes, this is right.

> 
> Creating IIQN's or iSCSI targets is not something I do all that often -
> I wonder if you came back to this code 6 months or longer from now
> whether you'd (instantly) recall what type of output was generated ;-).
> If you would then your short term memory is better than mine! I have to
> keep some cheat notes for iSCSI processing letting me know which
> commands to use and essentially what they return so I know they
> completed as expected.

Of course I would not. But seeing this pattern would help definitely:

if (some args) {
  VIR_STRDUP(output, expectedOutput1);
} else if (some other args) {
  /* nada */
} else {
  exitstatus = -1;
}

It suggest to me that for "some args" an output 

Re: [libvirt] [dbus PATCH v2 15/17] Implement InterfaceChangeRollback method for Connect Interface

[Katerina Koukiou]

On Fri, Jul 20, 2018 at 02:34:59PM -0400, Anya Harter wrote:
> Signed-off-by: Anya Harter 
> ---
>  data/org.libvirt.Connect.xml |  5 +
>  src/connect.c| 22 ++
>  2 files changed, 27 insertions(+)

Reviewed-by: Katerina Koukiou 


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 RESEND 00/12] PCI passthrough support on s390

[Boris Fiuczynski]

On 07/24/2018 10:20 AM, Andrea Bolognani wrote:

On Mon, 2018-07-23 at 13:37 +0200, Ján Tomko wrote:

On Tue, Jul 10, 2018 at 04:02:14PM +0800, Yi Min Zhao wrote:

Abstract

The PCI representation in QEMU has recently been extended for S390
allowing configuration of zPCI attributes like uid (user-defined
identifier) and fid (PCI function identifier).
The details can be found here:
https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg07262.html

To support the new zPCI feature of the S390 platform, two new XML
attributes, @uid and @fid, are introduced for device addresses of type
'pci', i.e.:
  


  


  

uid and fid are optional attributes. If they are defined by the user,
unique values within the guest domain must be used. If they are not
specified and the architecture requires them, they are automatically
generated with non-conflicting values.

Current implementation is the most seamless one for the user as it
unites the address specific data of a PCI device on one XML element.
It could accommodate both specifying our special parameters (uid and fid)
and re-using standard statements (domain, bus, slot and function) for
PCI devices. User can still specify bus/slot/function for the virtualized
PCI devices in the XML.

Thus uid/fid act as an extension to the PCI address and are stored in
a new structure 'virZPCIDeviceAddress' which is a member of common PCI
Address structure. Additionally, two hashtables are used for assignment
and reservation of uid/fid.

In support of extending the PCI address, a new PCI address extension flag is
introduced. This extension flag allows is not only dedicated for the S390
platform but also other architectures needing certain extensions to PCI
address space.

[...]


The patches look OK to me, therefore:

Reviewed-by: Ján Tomko 

But I'd like to hear from some other developer who touched the PCI code
last (Laine? Andrea?)


Thanks for bringing this to my attention - it had somehow managed
to escape my radar so far :)

I've quickly gone through the patches and spotted some minor code
style issues that I'd like to see addressed (I'll point them out
separately); first, though, I have a couple of questions about the
general idea behind the feature.

Looking at both the generated QEMU command line and the qemu-devel
message linked above, I seem to understand the zpci device is
basically some sort of adapter that sits between a regular PCI
device (emulated or otherwise) and an s390 guest and presents an
ID-based interface to the latter; so IIUC the s390 guest doesn't
actually see a PCI device, but a couple of IDs that can (somehow)
be used to access the underlying PCI device.


From whatever little s390 knowledge I have, I recall the

architecture using peculiar ways to address and access devices, so
PCI not being usable wouldn't surprise me that much; what I find
odd, however, is that regular PCI seems to be available at least
on the host side, because otherwise stuff like

   -device zpci,uid=1,fid=1,target=vpci0
   -device vfio-pci,host=:00:00.0,id=vpci0

wouldn't be possible. So, would anyone with s390 knowledge please
spend a few words explaining how the various pieces fit together?

Assuming the above is a correct reading of the situation, it
would seem the IDs are the only guest-visible part that we need
to make sure doesn't change during the lifetime of the guest,
while the usual bus/slot/function triplet assigned to the
underlying PCI device doesn't actually matter. And if that's the
case, wouldn't something like

   


Then a pci device on s390 would need a special address type zpci in 
libvirt and the design idea for libvirt is to stay compatible with pci.
Therefore uid and fid are optional attributes and if not specified on 
s390 they are generated. The patch series also allows on s390 to specify 
the pci address type just with attributes uid and/or fid causing the 
rest of the pci address attributes to be generated.

That means

is a valid pci address on s390 but would get expanded during definition 
into e.g.
function='0x0' uid='1' fid='1'/>

Also

is a valid pci address on s390 and is would be expanded during 
definition into e.g.
function='0x0' uid='1' fid='1'/>





be a better representation of the arrangement? We could leave it
up to QEMU to assign addresses to the PCI devices in this case...
But maybe they still matter for things such as migration? Or maybe
I've just gotten it wrong altogether? :)




--
Mit freundlichen Grüßen/Kind regards
   Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [dbus PATCH v2 05/17] Introduce Interface Tests & Test Create method for Interface Interface

[Katerina Koukiou]

On Fri, Jul 20, 2018 at 02:34:49PM -0400, Anya Harter wrote:
> Signed-off-by: Anya Harter 
> ---
>  tests/Makefile.am   |  1 +
>  tests/libvirttest.py| 12 
>  tests/test_interface.py | 16 
>  3 files changed, 29 insertions(+)
>  create mode 100755 tests/test_interface.py
> 
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 09c3e2e..cd1fbd7 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -11,6 +11,7 @@ test_helpers = \
>  test_programs = \
>   test_connect.py \
>   test_domain.py \
> + test_interface.py \
>   test_network.py \
>   test_nodedev.py \
>   test_storage.py \
> diff --git a/tests/libvirttest.py b/tests/libvirttest.py
> index 3741abd..2a09182 100644
> --- a/tests/libvirttest.py
> +++ b/tests/libvirttest.py
> @@ -71,6 +71,18 @@ class BaseTestClass():
>  if self.timeout:
>  raise TimeoutError()
>  

This method is not a fixture unless you put the @pytest.fixture
decorator.

This code without the fixture decorator will actually work, but it does
not do exactly what we want it to do.

See explanation bellow.

> +def interface_create(self):
> +""" Fixture to define dummy interface on the test driver
> +
> +This fixture should be used in the setup of every test manipulating
> +with interfaces.
> +"""
> +path = 
> self.connect.InterfaceDefineXML(xmldata.minimal_interface_xml, 0)
> +obj = self.bus.get_object('org.libvirt', path)
> +interface_obj = dbus.Interface(obj, 'org.libvirt.Interface')
> +interface_obj.Create(0)
> +return path, interface_obj
> +
>  @pytest.fixture
>  def node_device_create(self):
>  """ Fixture to create dummy node device on the test driver
> diff --git a/tests/test_interface.py b/tests/test_interface.py
> new file mode 100755
> index 000..88be5dc
> --- /dev/null
> +++ b/tests/test_interface.py
> @@ -0,0 +1,16 @@
> +#!/usr/bin/python3
> +
> +import dbus
> +import libvirttest
> +
> +class TestInterface(libvirttest.BaseTestClass):
> +""" Tests for methods and properties of the Interface interface
> +"""

Here you are calling the interace_create function (! it's not a fixture
without the decorator). It will create a dummy interface on the test
driver so that your tests can use it. However, the interface_create
functionality should not be tested in this test, this interface creation should
be part of the test "setup".

What is "setup"?
pytest defines three test phases for each test, which are "setup", "call" and
"teardown". The "setup" part should be used to prepare the environment for
the test to run (for our case define an test Interface), and teardown to clean
up after the test. Call is the actual test code itself.
The way to run some code as a test setup is using pytest fuxtures.

So the fixture definition you need to mark it with @pytest.fixture and
the functions where the fixture should be applied to with 
@pytest.mark.usefixtures("interface_create")
Then instead of calling the fixture we can use it's result by having the
fixture name in the function arguments. So the following should look
like:

@pytest.mark.fixtures("interface_create")
def test_interface_create(self, test_interface):
_,interface_obj = test_interface
interface_obj.Destroy(0)
interface_obj.Create(0)

> +
> +def test_interface_create(self):
> +_,interface_obj = self.interface_create()
> +interface_obj.Destroy(0)
> +interface_obj.Create(0)
> +
> +if __name__ == '__main__':
> +libvirttest.run()
> -- 
> 2.17.1
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

Although the concept is not correct, similar mistake exist in the
node_device tests, so I can fix it together in another patch set.

Reviewed-by: Katerina Koukiou 


Note: 
If you 're still not sure what the setup fixture does see the code
bellow:

$ cat show-setup.py
import pytest

@pytest.mark.usefixtures("foo")
def test_bar(foo):
print("Here starts test CALL")
print(foo)


@pytest.fixture
def foo():
print("Here starts test SETUP");
return "blabla"


$ pytest show-setup.py -s
 
test session starts 

platform linux2 -- Python 2.7.15, pytest-3.4.2, py-1.5.2, pluggy-0.6.0
rootdir: /home/kkoukiou/repos/libvirt-dbus, inifile:
plugins: ordering-0.5, marker-bugzilla-0.7, jira-0.3.6, forked-0.2
collected 1 item

show-setup.py Here starts test SETUP
Here starts test CALL
blabla
.

= 1 
passed in 0.00 seconds 
==


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com

Re: [libvirt] [dbus PATCH v2 11/17] Implement Active property for Interface Interface

[Katerina Koukiou]

On Fri, Jul 20, 2018 at 02:34:55PM -0400, Anya Harter wrote:
> Signed-off-by: Anya Harter 
> ---
>  data/org.libvirt.Interface.xml |  5 +
>  src/interface.c| 22 ++
>  tests/test_interface.py|  1 +
>  3 files changed, 28 insertions(+)

Reviewed-by: Katerina Koukiou 


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [dbus PATCH v2 14/17] Implement InterfaceChangeCommit method for Connect Interface

[Katerina Koukiou]

On Fri, Jul 20, 2018 at 02:34:58PM -0400, Anya Harter wrote:
> Signed-off-by: Anya Harter 
> ---
>  data/org.libvirt.Connect.xml |  5 +
>  src/connect.c| 22 ++
>  2 files changed, 27 insertions(+)

Reviewed-by: Katerina Koukiou 


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [dbus PATCH v2 13/17] Implement InterfaceChangeBegin method for Connect Interface

[Katerina Koukiou]

On Fri, Jul 20, 2018 at 02:34:57PM -0400, Anya Harter wrote:
> Signed-off-by: Anya Harter 
> ---
>  data/org.libvirt.Connect.xml |  5 +
>  src/connect.c| 22 ++
>  2 files changed, 27 insertions(+)

Reviewed-by: Katerina Koukiou 


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH for-3.1] s390x: remove 's390-squash-mcss' option

[Christian Borntraeger]

Acked-by: Christian Borntraeger 


On 07/24/2018 11:24 AM, Cornelia Huck wrote:
> This option has been deprecated for two releases; remove it.
> 
> Signed-off-by: Cornelia Huck 
> ---
>  hw/s390x/3270-ccw.c|  2 +-
>  hw/s390x/css-bridge.c  |  1 -
>  hw/s390x/css.c |  6 ++
>  hw/s390x/s390-ccw.c|  2 +-
>  hw/s390x/s390-virtio-ccw.c | 37 ++---
>  hw/s390x/virtio-ccw.c  |  2 +-
>  include/hw/s390x/css-bridge.h  |  1 -
>  include/hw/s390x/css.h |  9 +++--
>  include/hw/s390x/s390-virtio-ccw.h |  1 -
>  qemu-deprecated.texi   |  8 
>  qemu-options.hx| 10 --
>  target/s390x/cpu.c | 10 --
>  target/s390x/cpu.h |  1 -
>  13 files changed, 10 insertions(+), 80 deletions(-)
> 
> diff --git a/hw/s390x/3270-ccw.c b/hw/s390x/3270-ccw.c
> index 3af13ea027..cf58b81fc0 100644
> --- a/hw/s390x/3270-ccw.c
> +++ b/hw/s390x/3270-ccw.c
> @@ -104,7 +104,7 @@ static void emulated_ccw_3270_realize(DeviceState *ds, 
> Error **errp)
>  SubchDev *sch;
>  Error *err = NULL;
>  
> -sch = css_create_sch(cdev->devno, cbus->squash_mcss, errp);
> +sch = css_create_sch(cdev->devno, errp);
>  if (!sch) {
>  return;
>  }
> diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c
> index a02d708239..1bd6c8b458 100644
> --- a/hw/s390x/css-bridge.c
> +++ b/hw/s390x/css-bridge.c
> @@ -106,7 +106,6 @@ VirtualCssBus *virtual_css_bus_init(void)
>  /* Create bus on bridge device */
>  bus = qbus_create(TYPE_VIRTUAL_CSS_BUS, dev, "virtual-css");
>  cbus = VIRTUAL_CSS_BUS(bus);
> -cbus->squash_mcss = s390_get_squash_mcss();
>  
>  /* Enable hotplugging */
>  qbus_set_hotplug_handler(bus, dev, _abort);
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 5424ea4562..5a9fe45ce8 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -2359,15 +2359,13 @@ const PropertyInfo css_devid_ro_propinfo = {
>  .get = get_css_devid,
>  };
>  
> -SubchDev *css_create_sch(CssDevId bus_id, bool squash_mcss, Error **errp)
> +SubchDev *css_create_sch(CssDevId bus_id, Error **errp)
>  {
>  uint16_t schid = 0;
>  SubchDev *sch;
>  
>  if (bus_id.valid) {
> -if (squash_mcss) {
> -bus_id.cssid = channel_subsys.default_cssid;
> -} else if (!channel_subsys.css[bus_id.cssid]) {
> +if (!channel_subsys.css[bus_id.cssid]) {
>  css_create_css_image(bus_id.cssid, false);
>  }
>  
> diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
> index 214c940593..d1280bf631 100644
> --- a/hw/s390x/s390-ccw.c
> +++ b/hw/s390x/s390-ccw.c
> @@ -78,7 +78,7 @@ static void s390_ccw_realize(S390CCWDevice *cdev, char 
> *sysfsdev, Error **errp)
>  goto out_err_propagate;
>  }
>  
> -sch = css_create_sch(ccw_dev->devno, cbus->squash_mcss, );
> +sch = css_create_sch(ccw_dev->devno, );
>  if (!sch) {
>  goto out_mdevid_free;
>  }
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 7983185d04..380a41d806 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -282,19 +282,8 @@ static void ccw_init(MachineState *machine)
>  virtio_ccw_register_hcalls();
>  
>  s390_enable_css_support(s390_cpu_addr2state(0));
> -/*
> - * Non mcss-e enabled guests only see the devices from the default
> - * css, which is determined by the value of the squash_mcss property.
> - */
> -if (css_bus->squash_mcss) {
> -ret = css_create_css_image(0, true);
> -} else {
> -ret = css_create_css_image(VIRTUAL_CSSID, true);
> -}
> -if (qemu_opt_get(qemu_get_machine_opts(), "s390-squash-mcss")) {
> -warn_report("The machine property 's390-squash-mcss' is deprecated"
> -" (obsoleted by lifting the cssid restrictions).");
> -}
> +
> +ret = css_create_css_image(VIRTUAL_CSSID, true);
>  
>  assert(ret == 0);
>  if (css_migration_enabled()) {
> @@ -575,21 +564,6 @@ static void machine_set_loadparm(Object *obj, const char 
> *val, Error **errp)
>  ms->loadparm[i] = ' '; /* pad right with spaces */
>  }
>  }
> -static inline bool machine_get_squash_mcss(Object *obj, Error **errp)
> -{
> -S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
> -
> -return ms->s390_squash_mcss;
> -}
> -
> -static inline void machine_set_squash_mcss(Object *obj, bool value,
> -   Error **errp)
> -{
> -S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
> -
> -ms->s390_squash_mcss = value;
> -}
> -
>  static inline void s390_machine_initfn(Object *obj)
>  {
>  object_property_add_bool(obj, "aes-key-wrap",
> @@ -614,13 +588,6 @@ static inline void s390_machine_initfn(Object *obj)
>  " to upper case) to pass to machine loader, boot manager,"
>

[libvirt] [PATCH for-3.1] s390x: remove 's390-squash-mcss' option

[Cornelia Huck]

This option has been deprecated for two releases; remove it.

Signed-off-by: Cornelia Huck 
---
 hw/s390x/3270-ccw.c|  2 +-
 hw/s390x/css-bridge.c  |  1 -
 hw/s390x/css.c |  6 ++
 hw/s390x/s390-ccw.c|  2 +-
 hw/s390x/s390-virtio-ccw.c | 37 ++---
 hw/s390x/virtio-ccw.c  |  2 +-
 include/hw/s390x/css-bridge.h  |  1 -
 include/hw/s390x/css.h |  9 +++--
 include/hw/s390x/s390-virtio-ccw.h |  1 -
 qemu-deprecated.texi   |  8 
 qemu-options.hx| 10 --
 target/s390x/cpu.c | 10 --
 target/s390x/cpu.h |  1 -
 13 files changed, 10 insertions(+), 80 deletions(-)

diff --git a/hw/s390x/3270-ccw.c b/hw/s390x/3270-ccw.c
index 3af13ea027..cf58b81fc0 100644
--- a/hw/s390x/3270-ccw.c
+++ b/hw/s390x/3270-ccw.c
@@ -104,7 +104,7 @@ static void emulated_ccw_3270_realize(DeviceState *ds, 
Error **errp)
 SubchDev *sch;
 Error *err = NULL;
 
-sch = css_create_sch(cdev->devno, cbus->squash_mcss, errp);
+sch = css_create_sch(cdev->devno, errp);
 if (!sch) {
 return;
 }
diff --git a/hw/s390x/css-bridge.c b/hw/s390x/css-bridge.c
index a02d708239..1bd6c8b458 100644
--- a/hw/s390x/css-bridge.c
+++ b/hw/s390x/css-bridge.c
@@ -106,7 +106,6 @@ VirtualCssBus *virtual_css_bus_init(void)
 /* Create bus on bridge device */
 bus = qbus_create(TYPE_VIRTUAL_CSS_BUS, dev, "virtual-css");
 cbus = VIRTUAL_CSS_BUS(bus);
-cbus->squash_mcss = s390_get_squash_mcss();
 
 /* Enable hotplugging */
 qbus_set_hotplug_handler(bus, dev, _abort);
diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 5424ea4562..5a9fe45ce8 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -2359,15 +2359,13 @@ const PropertyInfo css_devid_ro_propinfo = {
 .get = get_css_devid,
 };
 
-SubchDev *css_create_sch(CssDevId bus_id, bool squash_mcss, Error **errp)
+SubchDev *css_create_sch(CssDevId bus_id, Error **errp)
 {
 uint16_t schid = 0;
 SubchDev *sch;
 
 if (bus_id.valid) {
-if (squash_mcss) {
-bus_id.cssid = channel_subsys.default_cssid;
-} else if (!channel_subsys.css[bus_id.cssid]) {
+if (!channel_subsys.css[bus_id.cssid]) {
 css_create_css_image(bus_id.cssid, false);
 }
 
diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
index 214c940593..d1280bf631 100644
--- a/hw/s390x/s390-ccw.c
+++ b/hw/s390x/s390-ccw.c
@@ -78,7 +78,7 @@ static void s390_ccw_realize(S390CCWDevice *cdev, char 
*sysfsdev, Error **errp)
 goto out_err_propagate;
 }
 
-sch = css_create_sch(ccw_dev->devno, cbus->squash_mcss, );
+sch = css_create_sch(ccw_dev->devno, );
 if (!sch) {
 goto out_mdevid_free;
 }
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 7983185d04..380a41d806 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -282,19 +282,8 @@ static void ccw_init(MachineState *machine)
 virtio_ccw_register_hcalls();
 
 s390_enable_css_support(s390_cpu_addr2state(0));
-/*
- * Non mcss-e enabled guests only see the devices from the default
- * css, which is determined by the value of the squash_mcss property.
- */
-if (css_bus->squash_mcss) {
-ret = css_create_css_image(0, true);
-} else {
-ret = css_create_css_image(VIRTUAL_CSSID, true);
-}
-if (qemu_opt_get(qemu_get_machine_opts(), "s390-squash-mcss")) {
-warn_report("The machine property 's390-squash-mcss' is deprecated"
-" (obsoleted by lifting the cssid restrictions).");
-}
+
+ret = css_create_css_image(VIRTUAL_CSSID, true);
 
 assert(ret == 0);
 if (css_migration_enabled()) {
@@ -575,21 +564,6 @@ static void machine_set_loadparm(Object *obj, const char 
*val, Error **errp)
 ms->loadparm[i] = ' '; /* pad right with spaces */
 }
 }
-static inline bool machine_get_squash_mcss(Object *obj, Error **errp)
-{
-S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
-
-return ms->s390_squash_mcss;
-}
-
-static inline void machine_set_squash_mcss(Object *obj, bool value,
-   Error **errp)
-{
-S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
-
-ms->s390_squash_mcss = value;
-}
-
 static inline void s390_machine_initfn(Object *obj)
 {
 object_property_add_bool(obj, "aes-key-wrap",
@@ -614,13 +588,6 @@ static inline void s390_machine_initfn(Object *obj)
 " to upper case) to pass to machine loader, boot manager,"
 " and guest kernel",
 NULL);
-object_property_add_bool(obj, "s390-squash-mcss",
- machine_get_squash_mcss,
- machine_set_squash_mcss, NULL);
-object_property_set_description(obj, "s390-squash-mcss", "(deprecated) "
-"enable/disable squashing 

Re: [libvirt] [dbus PATCH v2 07/17] Implement Undefine method for Interface Interface

[Katerina Koukiou]

On Fri, Jul 20, 2018 at 02:34:51PM -0400, Anya Harter wrote:
> Signed-off-by: Anya Harter 
> ---
>  data/org.libvirt.Interface.xml |  4 
>  src/interface.c| 21 +
>  tests/test_interface.py|  5 +
>  3 files changed, 30 insertions(+)
> 
> diff --git a/data/org.libvirt.Interface.xml b/data/org.libvirt.Interface.xml
> index f5ec281..4534b97 100644
> --- a/data/org.libvirt.Interface.xml
> +++ b/data/org.libvirt.Interface.xml
> @@ -13,5 +13,9 @@
>  value="See 
> https://libvirt.org/html/libvirt-libvirt-interface.html#virInterfaceDestroy"/>
>
>  
> +
> +   +value="See 
> https://libvirt.org/html/libvirt-libvirt-interface.html#virInterfaceUndefine"/>
> +
>
>  
> diff --git a/src/interface.c b/src/interface.c
> index 077b10d..a2d1cef 100644
> --- a/src/interface.c
> +++ b/src/interface.c
> @@ -70,6 +70,26 @@ virtDBusInterfaceDestroy(GVariant *inArgs,
>  virtDBusUtilSetLastVirtError(error);
>  }
>  
> +static void
> +virtDBusInterfaceUndefine(GVariant *inArgs G_GNUC_UNUSED,
> +  GUnixFDList *inFDs G_GNUC_UNUSED,
> +  const gchar *objectPath,
> +  gpointer userData,
> +  GVariant **outArgs G_GNUC_UNUSED,
> +  GUnixFDList **outFDs G_GNUC_UNUSED,
> +  GError **error)
> +{
> +virtDBusConnect *connect = userData;
> +g_autoptr(virInterface) interface = NULL;
> +
> +interface = virtDBusInterfaceGetVirInterface(connect, objectPath, error);
> +if (!interface)
> +return;
> +
> +if (virInterfaceUndefine(interface) < 0)
> +virtDBusUtilSetLastVirtError(error);
> +}
> +
>  static virtDBusGDBusPropertyTable virtDBusInterfacePropertyTable[] = {
>  { 0 }
>  };
> @@ -77,6 +97,7 @@ static virtDBusGDBusPropertyTable 
> virtDBusInterfacePropertyTable[] = {
>  static virtDBusGDBusMethodTable virtDBusInterfaceMethodTable[] = {
>  { "Create", virtDBusInterfaceCreate },
>  { "Destroy", virtDBusInterfaceDestroy },
> +{ "Undefine", virtDBusInterfaceUndefine },
>  { 0 }
>  };
>  
> diff --git a/tests/test_interface.py b/tests/test_interface.py
> index 62fd517..4a83672 100755
> --- a/tests/test_interface.py
> +++ b/tests/test_interface.py
> @@ -7,6 +7,11 @@ class TestInterface(libvirttest.BaseTestClass):
>  """ Tests for methods and properties of the Interface interface
>  """
>  
> +def test_interface_undefine(self):
> +_,interface_obj = self.interface_create()
> +interface_obj.Destroy(0)
> +interface_obj.Undefine(0)

Undefine does not take any flags.

> +
>  def test_interface_destroy(self):
>  _,interface_obj = self.interface_create()
>  interface_obj.Destroy(0)
> -- 
> 2.17.1
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

With the flag argument removed:
Reviewed-by: Katerina Koukiou 



signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [dbus PATCH v2 05/17] Introduce Interface Tests & Test Create method for Interface Interface

[Pavel Hrdina]

On Tue, Jul 24, 2018 at 11:30:10AM +0200, Katerina Koukiou wrote:
> On Fri, Jul 20, 2018 at 02:34:49PM -0400, Anya Harter wrote:
> > Signed-off-by: Anya Harter 
> > ---
> >  tests/Makefile.am   |  1 +
> >  tests/libvirttest.py| 12 
> >  tests/test_interface.py | 16 
> >  3 files changed, 29 insertions(+)
> >  create mode 100755 tests/test_interface.py
> > 
> > diff --git a/tests/Makefile.am b/tests/Makefile.am
> > index 09c3e2e..cd1fbd7 100644
> > --- a/tests/Makefile.am
> > +++ b/tests/Makefile.am
> > @@ -11,6 +11,7 @@ test_helpers = \
> >  test_programs = \
> > test_connect.py \
> > test_domain.py \
> > +   test_interface.py \
> > test_network.py \
> > test_nodedev.py \
> > test_storage.py \
> > diff --git a/tests/libvirttest.py b/tests/libvirttest.py
> > index 3741abd..2a09182 100644
> > --- a/tests/libvirttest.py
> > +++ b/tests/libvirttest.py
> > @@ -71,6 +71,18 @@ class BaseTestClass():
> >  if self.timeout:
> >  raise TimeoutError()
> >  
> 
> This method is not a fixture unless you put the @pytest.fixture
> decorator.
> 
> This code without the fixture decorator will actually work, but it does
> not do exactly what we want it to do.
> 
> See explanation bellow.
> 
> > +def interface_create(self):
> > +""" Fixture to define dummy interface on the test driver
> > +
> > +This fixture should be used in the setup of every test manipulating
> > +with interfaces.
> > +"""
> > +path = 
> > self.connect.InterfaceDefineXML(xmldata.minimal_interface_xml, 0)
> > +obj = self.bus.get_object('org.libvirt', path)
> > +interface_obj = dbus.Interface(obj, 'org.libvirt.Interface')
> > +interface_obj.Create(0)
> > +return path, interface_obj
> > +
> >  @pytest.fixture
> >  def node_device_create(self):
> >  """ Fixture to create dummy node device on the test driver
> > diff --git a/tests/test_interface.py b/tests/test_interface.py
> > new file mode 100755
> > index 000..88be5dc
> > --- /dev/null
> > +++ b/tests/test_interface.py
> > @@ -0,0 +1,16 @@
> > +#!/usr/bin/python3
> > +
> > +import dbus
> > +import libvirttest
> > +
> > +class TestInterface(libvirttest.BaseTestClass):
> > +""" Tests for methods and properties of the Interface interface
> > +"""
> 
> Here you are calling the interace_create function (! it's not a fixture
> without the decorator). It will create a dummy interface on the test
> driver so that your tests can use it. However, the interface_create
> functionality should not be tested in this test, this interface creation 
> should
> be part of the test "setup".
> 
> What is "setup"?
> pytest defines three test phases for each test, which are "setup", "call" and
> "teardown". The "setup" part should be used to prepare the environment for
> the test to run (for our case define an test Interface), and teardown to clean
> up after the test. Call is the actual test code itself.
> The way to run some code as a test setup is using pytest fuxtures.
> 
> So the fixture definition you need to mark it with @pytest.fixture and
> the functions where the fixture should be applied to with 
> @pytest.mark.usefixtures("interface_create")
> Then instead of calling the fixture we can use it's result by having the
> fixture name in the function arguments. So the following should look
> like:
> 
> @pytest.mark.fixtures("interface_create")
> def test_interface_create(self, test_interface):
> _,interface_obj = test_interface
> interface_obj.Destroy(0)
> interface_obj.Create(0)

Nice, I didn't know that this is possible.  I suggested not to use the
fixture because we need the result as well and existing code was calling
the fixture again as normal function which was failing for interfaces.

Pavel


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [dbus PATCH v2 12/17] Implement ListInterfaces method for Connect Interface

[Katerina Koukiou]

On Fri, Jul 20, 2018 at 02:34:56PM -0400, Anya Harter wrote:
> Signed-off-by: Anya Harter 
> ---
>  data/org.libvirt.Connect.xml |  6 ++
>  src/connect.c| 38 
>  2 files changed, 44 insertions(+)

Reviewed-by: Katerina Koukiou 


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 11/11] qemu_driver: BaselineHypervisorCPU supports S390 using QEMU/QMP

[David Hildenbrand]

On 23.07.2018 22:16, Collin Walling wrote:
> On 07/21/2018 12:05 AM, Chris Venteicher wrote:
>> Quoting David Hildenbrand (2018-07-18 02:26:24)
>>> On 18.07.2018 00:39, Collin Walling wrote:
 On 07/17/2018 05:01 PM, David Hildenbrand wrote:
> On 13.07.2018 18:00, Jiri Denemark wrote:
>> On Mon, Jul 09, 2018 at 22:56:55 -0500, Chris Venteicher wrote:
>>> Transient S390 configurations require using QEMU to compute CPU Model
>>> Baseline and to do CPU Feature Expansion.
>>>
>>> Start and use a single QEMU instance to do both the baseline and
>>> expansion transactions required by BaselineHypervisorCPU.
>>>
>>> CPU Feature Expansion uses true / false to indicate if property is/isn't
>>> included in model. Baseline only returns property list where all
>>> enumerated properties are included.
>>
>> So are you saying on s390 there's no chance there would be a CPU model
>> with some feature which is included in the CPU model disabled for some
>> reason? Sounds too good to be true :-) (This is the question I referred
>> to in one of my replies to the other patches.)
>
> Giving some background information: When we expand/baseline CPU models,
> we always expand them to the "-base" variants of our CPU models, which
> contain some set of features we expect to be around in all sane
> configurations ("minimal feature set").
>
> It is very unlikely that we ever have something like
> "z14-base,featx=off", but it could happen
>  - When using an emulator (TCG)
>  - When running nested and the guest hypervisor is started with such a
>strange CPU model
>  - When something in the HW is very wrong or eventually removed in the
>future (unlikely but possible)
>
> On some very weird inputs to a baseline request, such a strange model
> can also be the result. But it is very unusual.
>
> I assume something like "baseline z14-base,featx=off with z14-base" will
> result in "z14-base,featx=off", too.
>
>

 That's correct. A CPU model with a feature disabled that is baseline with 
 a CPU 
 model with that same feature enabled will omit that feature in the QMP 
 response.

 e.g. if z14-base has features x, y, z then

 "baseline z14-base,featx=off with z14-base" will result in 
 "z14-base,featy=on,featz=on"
>>
>> I am runing tests on both S390 and X86 (hacked QEMU to enable baseline).
>>
>> I don't see a "false" property in the baseline response in any of the logs.
> 
> Right... baseline should not be returning any properties paired with false. It
> constructs a third CPU model with properties that can run on both CPUs.
> 

Let me rephrase: We don't return "false" for any property when
baselining as of now, but this might change in the future. It is
undocumented behavior.


-- 

Thanks,

David / dhildenb

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 7/7] conf: Replace SKIP_OSTYPE_CHECKS with SKIP_VALIDATE

[Cole Robinson]

SKIP_OSTYPE_CHECKS only hides some error reporting at this point,
so it can be foled into SKIP_VALIDATE

Signed-off-by: Cole Robinson 
---
 src/conf/domain_conf.c  |  3 +--
 src/conf/domain_conf.h  | 13 +
 src/conf/snapshot_conf.c|  2 --
 src/conf/virdomainobjlist.c |  2 --
 tests/qemuxml2argvtest.c|  2 +-
 tests/qemuxml2xmltest.c |  1 -
 6 files changed, 7 insertions(+), 16 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 78ee000857..41baac08c0 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -28780,8 +28780,7 @@ virDomainDefCopy(virDomainDefPtr src,
 virDomainDefPtr ret;
 unsigned int format_flags = VIR_DOMAIN_DEF_FORMAT_SECURE;
 unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
-   VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE |
-   VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS;
+   VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE;
 
 if (migratable)
 format_flags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE | 
VIR_DOMAIN_DEF_FORMAT_MIGRATABLE;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 5e2f21dea3..a804e86f6c 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2981,24 +2981,21 @@ typedef enum {
 VIR_DOMAIN_DEF_PARSE_DISK_SOURCE = 1 << 6,
 /* perform RNG schema validation on the passed XML document */
 VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA = 1 << 7,
-/* don't validate os.type and arch against capabilities. Prevents
- * VMs from disappearing when qemu is removed and libvirtd is restarted */
-VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS = 1 << 8,
 /* allow updates in post parse callback that would break ABI otherwise */
-VIR_DOMAIN_DEF_PARSE_ABI_UPDATE = 1 << 9,
+VIR_DOMAIN_DEF_PARSE_ABI_UPDATE = 1 << 8,
 /* skip definition validation checks meant to be executed on define time 
only */
-VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE = 1 << 10,
+VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE = 1 << 9,
 /* skip parsing of security labels */
-VIR_DOMAIN_DEF_PARSE_SKIP_SECLABEL= 1 << 11,
+VIR_DOMAIN_DEF_PARSE_SKIP_SECLABEL= 1 << 10,
 /* Allows updates in post parse callback for incoming persistent migration
  * that would break ABI otherwise.  This should be used only if it's safe
  * to do such change. */
-VIR_DOMAIN_DEF_PARSE_ABI_UPDATE_MIGRATION = 1 << 12,
+VIR_DOMAIN_DEF_PARSE_ABI_UPDATE_MIGRATION = 1 << 11,
 /* Allows to ignore certain failures in the post parse callbacks, which
  * may happen due to missing packages and can be fixed by re-running the
  * post parse callbacks before starting. Failure of the post parse callback
  * is recorded as def->postParseFail */
-VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL = 1 << 13,
+VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL = 1 << 12,
 } virDomainDefParseFlags;
 
 typedef enum {
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 9c537ac7d1..adba149241 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -273,8 +273,6 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt,
 if ((tmp = virXPathString("string(./domain/@type)", ctxt))) {
 int domainflags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
   VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE;
-if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL)
-domainflags |= VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS;
 xmlNodePtr domainNode = virXPathNode("./domain", ctxt);
 
 VIR_FREE(tmp);
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index 72064d7c66..52171594f3 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -492,7 +492,6 @@ virDomainObjListLoadConfig(virDomainObjListPtr doms,
 goto error;
 if (!(def = virDomainDefParseFile(configFile, caps, xmlopt, NULL,
   VIR_DOMAIN_DEF_PARSE_INACTIVE |
-  VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS |
   VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE |
   
VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL)))
 goto error;
@@ -544,7 +543,6 @@ virDomainObjListLoadStatus(virDomainObjListPtr doms,
   VIR_DOMAIN_DEF_PARSE_STATUS |
   VIR_DOMAIN_DEF_PARSE_ACTUAL_NET |
   VIR_DOMAIN_DEF_PARSE_PCI_ORIG_STATES |
-  VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS |
   VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE |
   
VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL)))
 goto error;
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 03b6d92912..84117a3e63 100644
--- a/tests/qemuxml2argvtest.c
+++ 

[libvirt] [PATCH 1/7] conf: Break out virDomainDefParseCaps

[Cole Robinson]

Handles parse virtType, os.type, bootloader bits, arch, machine,
emulator

Signed-off-by: Cole Robinson 
---
 src/conf/domain_conf.c | 96 +-
 1 file changed, 58 insertions(+), 38 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 178c6d2711..7eb5ffc718 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -19114,46 +19114,15 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
 }
 
 
-static virDomainDefPtr
-virDomainDefParseXML(xmlDocPtr xml,
- xmlNodePtr root,
- xmlXPathContextPtr ctxt,
- virCapsPtr caps,
- virDomainXMLOptionPtr xmlopt,
- unsigned int flags)
+static int
+virDomainDefParseCaps(virDomainDefPtr def,
+  xmlXPathContextPtr ctxt,
+  virCapsPtr caps,
+  unsigned int flags)
 {
-xmlNodePtr *nodes = NULL, node = NULL;
+int ret = -1;
+int virtType;
 char *tmp = NULL;
-size_t i, j;
-int n, virtType, gic_version;
-long id = -1;
-virDomainDefPtr def;
-bool uuid_generated = false;
-bool usb_none = false;
-bool usb_other = false;
-bool usb_master = false;
-char *netprefix = NULL;
-
-if (flags & VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA) {
-char *schema = virFileFindResource("domain.rng",
-   abs_topsrcdir "/docs/schemas",
-   PKGDATADIR "/schemas");
-if (!schema)
-return NULL;
-if (virXMLValidateAgainstSchema(schema, xml) < 0) {
-VIR_FREE(schema);
-return NULL;
-}
-VIR_FREE(schema);
-}
-
-if (!(def = virDomainDefNew()))
-return NULL;
-
-if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE))
-if (virXPathLong("string(./@id)", ctxt, ) < 0)
-id = -1;
-def->id = (int)id;
 
 /* Find out what type of virtualization to use */
 if (!(tmp = virXMLPropString(ctxt->node, "type"))) {
@@ -19239,6 +19208,57 @@ virDomainDefParseXML(xmlDocPtr xml,
 VIR_FREE(capsdata);
 }
 
+ret = 0;
+ error:
+VIR_FREE(tmp);
+return ret;
+}
+
+
+static virDomainDefPtr
+virDomainDefParseXML(xmlDocPtr xml,
+ xmlNodePtr root,
+ xmlXPathContextPtr ctxt,
+ virCapsPtr caps,
+ virDomainXMLOptionPtr xmlopt,
+ unsigned int flags)
+{
+xmlNodePtr *nodes = NULL, node = NULL;
+char *tmp = NULL;
+size_t i, j;
+int n, gic_version;
+long id = -1;
+virDomainDefPtr def;
+bool uuid_generated = false;
+bool usb_none = false;
+bool usb_other = false;
+bool usb_master = false;
+char *netprefix = NULL;
+
+if (flags & VIR_DOMAIN_DEF_PARSE_VALIDATE_SCHEMA) {
+char *schema = virFileFindResource("domain.rng",
+   abs_topsrcdir "/docs/schemas",
+   PKGDATADIR "/schemas");
+if (!schema)
+return NULL;
+if (virXMLValidateAgainstSchema(schema, xml) < 0) {
+VIR_FREE(schema);
+return NULL;
+}
+VIR_FREE(schema);
+}
+
+if (!(def = virDomainDefNew()))
+return NULL;
+
+if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE))
+if (virXPathLong("string(./@id)", ctxt, ) < 0)
+id = -1;
+def->id = (int)id;
+
+if (virDomainDefParseCaps(def, ctxt, caps, flags) < 0)
+goto error;
+
 /* Extract domain name */
 if (!(def->name = virXPathString("string(./name[1])", ctxt))) {
 virReportError(VIR_ERR_NO_NAME, NULL);
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 0/7] conf: Replace SKIP_OSTYPE with SKIP_VALIDATE

[Cole Robinson]

The SKIP_OSTYPE domain parse flag is largely redundant nowadays
since we have SKIP_VALIDATE. This series smoothes out some of
the SKIP_OSTYPE weirdness so we can drop it

Cole Robinson (7):
  conf: Break out virDomainDefParseCaps
  conf: Clean up virDomainDefParseCaps
  tests: qemuhotplug: Fix segfault when XML loading fails
  conf: Drop unneccessary caps parsing logic
  conf: Sync caps data even when SKIP_OSTYPE_CHECKS
  tests: Remove redundant lxc test
  conf: Replace SKIP_OSTYPE_CHECKS with SKIP_VALIDATE

 src/conf/domain_conf.c | 175 +++--
 src/conf/domain_conf.h |  13 +-
 src/conf/snapshot_conf.c   |   2 -
 src/conf/virdomainobjlist.c|   2 -
 tests/lxcxml2xmltest.c |   2 -
 tests/qemuhotplugtest.c|   2 +
 tests/qemuxml2argvdata/missing-machine.xml |   2 +-
 tests/qemuxml2argvtest.c   |   5 +-
 tests/qemuxml2xmltest.c|   1 -
 tests/testutils.c  |  13 +-
 tests/testutilsqemu.c  |  18 +++
 tests/vircapstest.c|   2 -
 12 files changed, 132 insertions(+), 105 deletions(-)

-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 5/7] conf: Sync caps data even when SKIP_OSTYPE_CHECKS

[Cole Robinson]

We should still make an effort to fill in data, just not raise
an error if say an ostype/virttype combo disappeared from caps.

Signed-off-by: Cole Robinson 
---
 src/conf/domain_conf.c | 13 ++---
 tests/qemuxml2argvdata/missing-machine.xml |  2 +-
 tests/qemuxml2argvtest.c   |  3 +++
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b7f6a22e20..78ee000857 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -19178,18 +19178,17 @@ virDomainDefParseCaps(virDomainDefPtr def,
 goto cleanup;
 }
 
-if (!(flags & VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS)) {
-if (!(capsdata = virCapabilitiesDomainDataLookup(caps,
-def->os.type, def->os.arch, def->virtType,
-NULL, NULL)))
+if (!(capsdata = virCapabilitiesDomainDataLookup(caps, def->os.type,
+def->os.arch, def->virtType, NULL, NULL))) {
+if (!(flags & VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))
 goto cleanup;
-
+virResetLastError();
+} else {
 if (!def->os.arch)
 def->os.arch = capsdata->arch;
 if ((!def->os.machine &&
- VIR_STRDUP(def->os.machine, capsdata->machinetype) < 0)) {
+ VIR_STRDUP(def->os.machine, capsdata->machinetype) < 0))
 goto cleanup;
-}
 }
 
 ret = 0;
diff --git a/tests/qemuxml2argvdata/missing-machine.xml 
b/tests/qemuxml2argvdata/missing-machine.xml
index 4ce7b377a5..2900baec90 100644
--- a/tests/qemuxml2argvdata/missing-machine.xml
+++ b/tests/qemuxml2argvdata/missing-machine.xml
@@ -6,7 +6,7 @@
   219100
   1
   
-hvm
+hvm
 
   
   
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 1a936faef1..03b6d92912 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2773,6 +2773,9 @@ mymain(void)
 QEMU_CAPS_OBJECT_GPEX,
 QEMU_CAPS_NEC_USB_XHCI);
 
+/* VM XML has invalid arch/ostype/virttype combo, but the SKIP flag
+ * will avoid the error. Still, we expect qemu driver to complain about
+ * missing machine error, and not crash */
 DO_TEST_PARSE_FLAGS_ERROR("missing-machine",
   VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS,
   NONE);
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 4/7] conf: Drop unneccessary caps parsing logic

[Cole Robinson]

The comment says:

/* If the logic here seems fairly arbitrary, that's because it is :)
 * This is duplicating how the code worked before
 * CapabilitiesDomainDataLookup was added. We can simplify this,
 * but it would take a bit of work because the test suite fails
 * in numerous minor ways. */

Nowadays the test suite changes appear quite simple, just extending
test capabilities data a bit so that we aren't trying to define
invalid arch/os/virtType/machine combos

Signed-off-by: Cole Robinson 
---
 src/conf/domain_conf.c | 15 ++-
 tests/testutils.c  | 13 -
 tests/testutilsqemu.c  | 18 ++
 tests/vircapstest.c|  2 --
 4 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5a90429cd6..b7f6a22e20 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -19178,20 +19178,9 @@ virDomainDefParseCaps(virDomainDefPtr def,
 goto cleanup;
 }
 
-if ((!def->os.arch || !def->os.machine) &&
-!(flags & VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS)) {
-/* If the logic here seems fairly arbitrary, that's because it is :)
- * This is duplicating how the code worked before
- * CapabilitiesDomainDataLookup was added. We can simplify this,
- * but it would take a bit of work because the test suite fails
- * in numerous minor ways. */
-bool use_virttype = ((def->os.arch == VIR_ARCH_NONE) ||
-!def->os.machine);
-
+if (!(flags & VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS)) {
 if (!(capsdata = virCapabilitiesDomainDataLookup(caps,
-def->os.type,
-def->os.arch,
-use_virttype ? def->virtType : VIR_DOMAIN_VIRT_NONE,
+def->os.type, def->os.arch, def->virtType,
 NULL, NULL)))
 goto cleanup;
 
diff --git a/tests/testutils.c b/tests/testutils.c
index 423f4bfdff..ab938c12fc 100644
--- a/tests/testutils.c
+++ b/tests/testutils.c
@@ -1196,7 +1196,12 @@ virCapsPtr virTestGenericCapsInit(void)
 
 if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_TEST, NULL, 
NULL, 0, NULL))
 goto error;
-
+if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_QEMU,
+   NULL, NULL, 0, NULL))
+goto error;
+if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_KVM,
+   NULL, NULL, 0, NULL))
+goto error;
 
 if ((guest = virCapabilitiesAddGuest(caps, VIR_DOMAIN_OSTYPE_HVM, 
VIR_ARCH_X86_64,
  "/usr/bin/acme-virt", NULL,
@@ -1205,6 +1210,12 @@ virCapsPtr virTestGenericCapsInit(void)
 
 if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_TEST, NULL, 
NULL, 0, NULL))
 goto error;
+if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_QEMU,
+   NULL, NULL, 0, NULL))
+goto error;
+if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_KVM,
+   NULL, NULL, 0, NULL))
+goto error;
 
 
 if (virTestGetDebug() > 1) {
diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
index dc7e90b952..cc2f8a7b64 100644
--- a/tests/testutilsqemu.c
+++ b/tests/testutilsqemu.c
@@ -219,6 +219,9 @@ static int testQemuAddPPC64Guest(virCapsPtr caps)
 
 if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_QEMU, NULL, 
NULL, 0, NULL))
 goto error;
+if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_KVM,
+   NULL, NULL, 0, NULL))
+goto error;
 
 return 0;
 
@@ -246,6 +249,9 @@ static int testQemuAddPPC64LEGuest(virCapsPtr caps)
 
 if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_QEMU, NULL, 
NULL, 0, NULL))
 goto error;
+if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_KVM,
+   NULL, NULL, 0, NULL))
+goto error;
 
 return 0;
 
@@ -276,6 +282,9 @@ static int testQemuAddPPCGuest(virCapsPtr caps)
 
 if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_QEMU, NULL, 
NULL, 0, NULL))
 goto error;
+if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_KVM,
+   NULL, NULL, 0, NULL))
+goto error;
 
 return 0;
 
@@ -307,6 +316,9 @@ static int testQemuAddS390Guest(virCapsPtr caps)
 
 if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_QEMU, NULL, 
NULL, 0, NULL))
 goto error;
+if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_KVM,
+   NULL, NULL, 0, NULL))
+goto error;
 
 return 0;
 
@@ -338,6 +350,9 @@ static int testQemuAddArmGuest(virCapsPtr caps)
 
 if (!virCapabilitiesAddGuestDomain(guest, VIR_DOMAIN_VIRT_QEMU, NULL, 
NULL, 0, NULL))
 goto error;
+if 

[libvirt] [PATCH 6/7] tests: Remove redundant lxc test

[Cole Robinson]

This test was added in 2d40e2da7ba to ensure LXC domains could be
defined correctly when caps probing was skipped due to SKIP_OSTYPE.
However we do caps probing unconditionally now, so this test case
is redundant

Signed-off-by: Cole Robinson 
---
 tests/lxcxml2xmltest.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tests/lxcxml2xmltest.c b/tests/lxcxml2xmltest.c
index 3b96862c62..5dbeb0b2eb 100644
--- a/tests/lxcxml2xmltest.c
+++ b/tests/lxcxml2xmltest.c
@@ -96,8 +96,6 @@ mymain(void)
 DO_TEST("sharenet");
 DO_TEST("ethernet");
 DO_TEST("ethernet-hostip");
-DO_TEST_FULL("filesystem-root", 0, false,
- VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS);
 DO_TEST("initenv");
 DO_TEST("initdir");
 DO_TEST("inituser");
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 2/7] conf: Clean up virDomainDefParseCaps

[Cole Robinson]

- Convert to 'cleanup' label naming
- Use more than one 'tmp' string and do all freeing at the end
- Make the code easier to follow

Signed-off-by: Cole Robinson 
---
 src/conf/domain_conf.c | 76 --
 1 file changed, 37 insertions(+), 39 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 7eb5ffc718..5a90429cd6 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -19121,43 +19121,45 @@ virDomainDefParseCaps(virDomainDefPtr def,
   unsigned int flags)
 {
 int ret = -1;
-int virtType;
-char *tmp = NULL;
+char *virttype = NULL;
+char *arch = NULL;
+char *ostype = NULL;
+virCapsDomainDataPtr capsdata = NULL;
 
-/* Find out what type of virtualization to use */
-if (!(tmp = virXMLPropString(ctxt->node, "type"))) {
+virttype = virXPathString("string(./@type)", ctxt);
+ostype = virXPathString("string(./os/type[1])", ctxt);
+arch = virXPathString("string(./os/type[1]/@arch)", ctxt);
+
+def->os.bootloader = virXPathString("string(./bootloader)", ctxt);
+def->os.bootloaderArgs = virXPathString("string(./bootloader_args)", ctxt);
+def->os.machine = virXPathString("string(./os/type[1]/@machine)", ctxt);
+def->emulator = virXPathString("string(./devices/emulator[1])", ctxt);
+
+if (!virttype) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
"%s", _("missing domain type attribute"));
-goto error;
+goto cleanup;
 }
-
-if ((virtType = virDomainVirtTypeFromString(tmp)) < 0) {
+if ((def->virtType = virDomainVirtTypeFromString(virttype)) < 0) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("invalid domain type %s"), tmp);
-goto error;
+   _("invalid domain type %s"), virttype);
+goto cleanup;
 }
-def->virtType = virtType;
-VIR_FREE(tmp);
-
-def->os.bootloader = virXPathString("string(./bootloader)", ctxt);
-def->os.bootloaderArgs = virXPathString("string(./bootloader_args)", ctxt);
 
-tmp = virXPathString("string(./os/type[1])", ctxt);
-if (!tmp) {
+if (!ostype) {
 if (def->os.bootloader) {
 def->os.type = VIR_DOMAIN_OSTYPE_XEN;
 } else {
 virReportError(VIR_ERR_XML_ERROR, "%s",
_("an os  must be specified"));
-goto error;
+goto cleanup;
 }
 } else {
-if ((def->os.type = virDomainOSTypeFromString(tmp)) < 0) {
+if ((def->os.type = virDomainOSTypeFromString(ostype)) < 0) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("unknown OS type '%s'"), tmp);
-goto error;
+   _("unknown OS type '%s'"), ostype);
+goto cleanup;
 }
-VIR_FREE(tmp);
 }
 
 /*
@@ -19170,17 +19172,11 @@ virDomainDefParseCaps(virDomainDefPtr def,
 def->os.type = VIR_DOMAIN_OSTYPE_XEN;
 }
 
-tmp = virXPathString("string(./os/type[1]/@arch)", ctxt);
-if (tmp && !(def->os.arch = virArchFromString(tmp))) {
+if (arch && !(def->os.arch = virArchFromString(arch))) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("Unknown architecture %s"),
-   tmp);
-goto error;
+   _("Unknown architecture %s"), arch);
+goto cleanup;
 }
-VIR_FREE(tmp);
-
-def->os.machine = virXPathString("string(./os/type[1]/@machine)", ctxt);
-def->emulator = virXPathString("string(./devices/emulator[1])", ctxt);
 
 if ((!def->os.arch || !def->os.machine) &&
 !(flags & VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS)) {
@@ -19191,26 +19187,28 @@ virDomainDefParseCaps(virDomainDefPtr def,
  * in numerous minor ways. */
 bool use_virttype = ((def->os.arch == VIR_ARCH_NONE) ||
 !def->os.machine);
-virCapsDomainDataPtr capsdata = NULL;
 
-if (!(capsdata = virCapabilitiesDomainDataLookup(caps, def->os.type,
-def->os.arch, use_virttype ? def->virtType : 
VIR_DOMAIN_VIRT_NONE,
+if (!(capsdata = virCapabilitiesDomainDataLookup(caps,
+def->os.type,
+def->os.arch,
+use_virttype ? def->virtType : VIR_DOMAIN_VIRT_NONE,
 NULL, NULL)))
-goto error;
+goto cleanup;
 
 if (!def->os.arch)
 def->os.arch = capsdata->arch;
 if ((!def->os.machine &&
  VIR_STRDUP(def->os.machine, capsdata->machinetype) < 0)) {
-VIR_FREE(capsdata);
-goto error;
+goto cleanup;
 }
-VIR_FREE(capsdata);
 }
 
 ret = 0;
- error:
-VIR_FREE(tmp);
+ cleanup:
+VIR_FREE(virttype);
+VIR_FREE(ostype);
+VIR_FREE(arch);
+VIR_FREE(capsdata);
 return ret;
 }
 
-- 
2.17.1

--
libvir-list mailing list

Re: [libvirt] [PATCH 1/9] util: Rename some functions of virresctrl

[John Ferlan]

On 07/18/2018 03:57 AM, bing@intel.com wrote:
> From: Bing Niu 
> 
> Some functions in virresctrl are for CAT only, while some of other
> functions are for resource allocation, not just CAT. So change
> their names to reflect the reality.
> 
> Signed-off-by: Bing Niu 
> ---
>  src/conf/domain_conf.c   |  8 
>  src/libvirt_private.syms |  4 ++--
>  src/util/virresctrl.c| 30 +++---
>  src/util/virresctrl.h| 26 +-
>  4 files changed, 34 insertions(+), 34 deletions(-)
> 

Reviewed-by: John Ferlan 

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/9] util: Refactor virResctrlGetInfo in virresctrl

[John Ferlan]

On 07/18/2018 03:57 AM, bing@intel.com wrote:
> From: Bing Niu 
> 
> Separate resctrl common information parts from CAT specific parts,
> so that common information parts can be reused among different
> resource allocation technologies.
> 
> Signed-off-by: Bing Niu 
> ---
>  src/util/virresctrl.c | 31 ++-
>  1 file changed, 22 insertions(+), 9 deletions(-)
> 

I'm assuming you are coordinating with the [RFC PATCHv2 00/10] x86 RDT
Cache Monitoring Technology (CMT)​ that's on list as well. I don't have
the cycles to do any compare/contrast between the two...

> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> index 6d69c8d..98e7296 100644
> --- a/src/util/virresctrl.c
> +++ b/src/util/virresctrl.c
> @@ -318,9 +318,8 @@ virResctrlUnlock(int fd)
>  
>  /* virResctrlInfo-related definitions */
>  static int
> -virResctrlGetInfo(virResctrlInfoPtr resctrl)
> +virResctrlGetCacheInfo(virResctrlInfoPtr resctrl, DIR *dirp)

One argument per line... I can fix before pushing though.

Reviewed-by: John Ferlan 

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 4/9] util: Add MBA capability information query to resctrl

[John Ferlan]

On 07/18/2018 03:57 AM, bing@intel.com wrote:
> From: Bing Niu 
> 
> Introducing virResctrlInfoMemBW for the information memory bandwidth
> allocation information. Those information is used for memory
> bandwidth allocating.

Last sentence is duplicitous.

> 
> Signed-off-by: Bing Niu 
> ---
>  src/util/virresctrl.c | 104 
> ++
>  1 file changed, 104 insertions(+)
> 
> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> index 1515de2..06e2702 100644
> --- a/src/util/virresctrl.c
> +++ b/src/util/virresctrl.c
> @@ -80,6 +80,9 @@ typedef virResctrlInfoPerType *virResctrlInfoPerTypePtr;
>  typedef struct _virResctrlInfoPerLevel virResctrlInfoPerLevel;
>  typedef virResctrlInfoPerLevel *virResctrlInfoPerLevelPtr;
>  
> +typedef struct _virResctrlInfoMemBW virResctrlInfoMemBW;
> +typedef virResctrlInfoMemBW *virResctrlInfoMemBWPtr;
> +
>  typedef struct _virResctrlAllocPerType virResctrlAllocPerType;
>  typedef virResctrlAllocPerType *virResctrlAllocPerTypePtr;
>  
> @@ -116,11 +119,31 @@ struct _virResctrlInfoPerLevel {
>  virResctrlInfoPerTypePtr *types;
>  };
>  
> +/* Information about memory bandwidth allocation */
> +struct _virResctrlInfoMemBW {
> +/* minimum memory bandwidth allowed */
> +unsigned int min_bandwidth;
> +/* bandwidth granularity */
> +unsigned int bandwidth_granularity;
> +/* Maximum number of simultaneous allocations */
> +unsigned int max_allocation;
> +/* level number of last level cache */
> +unsigned int last_level_cache;
> +/* max id of last level cache, this is used to track
> + * how many last level cache available in host system,
> + * the number of memory bandwidth allocation controller
> + * is identical with last level cache.
> + */

The closing */ can be on the previous line.

> +unsigned int max_id;
> +};
> +
>  struct _virResctrlInfo {
>  virObject parent;
>  
>  virResctrlInfoPerLevelPtr *levels;
>  size_t nlevels;
> +
> +virResctrlInfoMemBWPtr membw_info;
>  };
>  
>  
> @@ -146,6 +169,7 @@ virResctrlInfoDispose(void *obj)
>  VIR_FREE(level);
>  }
>  
> +VIR_FREE(resctrl->membw_info);
>  VIR_FREE(resctrl->levels);
>  }
>  
> @@ -442,6 +466,65 @@ virResctrlGetCacheInfo(virResctrlInfoPtr resctrl, DIR 
> *dirp)
>  
>  
>  static int
> +virResctrlGetMemoryBandwidthInfo(virResctrlInfoPtr resctrl)
> +{
> +int ret = -1;
> +int rv = -1;
> +virResctrlInfoMemBWPtr i_membw = NULL;
> +
> +/* query memory bandwidth allocation info */
> +if (VIR_ALLOC(i_membw) < 0)
> +goto cleanup;
> +rv = virFileReadValueUint(_membw->bandwidth_granularity,
> +  SYSFS_RESCTRL_PATH "/info/MB/bandwidth_gran");
> +if (rv == -2) {
> +/* The file doesn't exist, so it's unusable for us,
> + *  properly mba unsupported */

s/ properly/probably/

??  Is that what you meant?

s/mba/memory bandwidth/

right?

> +VIR_INFO("The path '" SYSFS_RESCTRL_PATH "/info/MB/bandwidth_gran'"
> + "does not exist");
> +ret = 0;
> +goto cleanup;
> +} else if (rv < 0) {
> +/* Other failures are fatal, so just quit */
> +goto cleanup;
> +}
> +
> +rv = virFileReadValueUint(_membw->min_bandwidth,
> +  SYSFS_RESCTRL_PATH "/info/MB/min_bandwidth");
> +if (rv == -2) {
> +/* If the previous file exists, so should this one.  Hence -2 is

s/  Hence/ Hence/

It's a raging debate at times on this list whether to go with 1 space or
2. I think 1 space usually wins, but I use 2 many times as well. I'll
adjust this to 1 space though ;-)

> + * fatal in this case (errors out in next condition) - the
> + * kernel interface might've changed too much or something else is
> + * wrong. */

"kernel" can fit on previous line meaning "wrong." can fit on previous
line.  Also, no need for blank line between here and virReportError

> +
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("Cannot get min bandwidth from resctrl memory 
> info"));
> +}
> +if (rv < 0)
> +goto cleanup;
> +
> +rv = virFileReadValueUint(_membw->max_allocation,
> +  SYSFS_RESCTRL_PATH "/info/MB/num_closids");
> +if (rv == -2) {
> +/* If the previous file exists, so should this one.  Hence -2 is
> + * fatal in this case as well (errors out in next condition) - the
> + * kernel interface might've changed too much or something else is
> + * wrong. */

I'd just note /* Similar reasoning as min_bandwidth above */

> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("Cannot get max allocation from resctrl memory 
> info"));
> +}
> +if (rv < 0)
> +goto cleanup;
> +
> +VIR_STEAL_PTR(resctrl->membw_info, i_membw);
> +ret = 0;
> + cleanup:
> +

[libvirt] [PATCH 3/7] tests: qemuhotplug: Fix segfault when XML loading fails

[Cole Robinson]

Some tests use the same VM state multiple times in a row. But if we
failed loading the VM XML, subsequent tests crash on the NULL def
pointer

Signed-off-by: Cole Robinson 
---
I hit this with failing tests while writing this series

 tests/qemuhotplugtest.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index 674ba92b27..4f9e127f88 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -268,6 +268,8 @@ testQemuHotplug(const void *data)
 
 if (test->vm) {
 vm = test->vm;
+if (!vm->def)
+goto cleanup;
 } else {
 if (qemuHotplugCreateObjects(driver.xmlopt, , domain_xml,
  test->deviceDeletedEvent) < 0)
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/9] util: Refactor virResctrlAllocFormat of virresctrl

[John Ferlan]

On 07/18/2018 03:57 AM, bing@intel.com wrote:
> From: Bing Niu 
> 
> Refactor virResctrlAllocFormat so that it is easy to support other
> resource allocation technologies.
> 
> Signed-off-by: Bing Niu 
> ---
>  src/util/virresctrl.c | 43 ---
>  1 file changed, 28 insertions(+), 15 deletions(-)
> 
> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> index 98e7296..1515de2 100644
> --- a/src/util/virresctrl.c
> +++ b/src/util/virresctrl.c
> @@ -848,17 +848,13 @@ virResctrlAllocGetID(virResctrlAllocPtr alloc)
>  }
>  
>  
> -char *
> -virResctrlAllocFormat(virResctrlAllocPtr alloc)
> +static int
> +virResctrlAllocFormatCache(virResctrlAllocPtr alloc, virBufferPtr buf)

One argument per line.  I'll adjust.

>  {
> -virBuffer buf = VIR_BUFFER_INITIALIZER;
>  unsigned int level = 0;
>  unsigned int type = 0;
>  unsigned int cache = 0;
>  

[...]

>  
> -virBufferCheckError();
> +if (virBufferCheckError(buf) < 0)
> +return -1;
> +else
> +return 0;

Just return virBufferCheckError(buf); directly

I'll adjust before pushing.

Reviewed-by: John Ferlan 

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2] qemuDomainSaveMemory: Don't enforce dynamicOwnership

[John Ferlan]

On 07/09/2018 08:51 AM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1589115
> 
> When doing a memory snapshot qemuOpenFile() is used. This means
> that the file where memory is saved is firstly attempted to be
> created under root:root (because that's what libvirtd is running
> under) and if this fails the second attempt is done under
> domain's uid:gid. This does not make much sense - qemu is given
> opened FD so it does not need to access the file. Moreover, if
> dynamicOwnership is set in qemu.conf and the file lives on a
> squashed NFS this is deadly combination and very likely to fail.
> 
> The fix consists of using:
> 
>   qemuOpenFileAs(fallback_uid = cfg->user,
>  fallback_gid = cfg->group,
>  dynamicOwnership = false)
> 
> In other words, dynamicOwnership is turned off for memory
> snapshot (chown() will still be attempted if the file does not
> live on NFS) and instead of using domain DAC label, configured
> user:group is set as fallback.
> 

for memory snapshot and core files, right?

> Signed-off-by: Michal Privoznik 
> ---
> 
> Diff to v1:
> - Fix doCoreDump too (raised in review by John).
> 
>  src/qemu/qemu_driver.c | 15 +--
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 

Strange - I had this marked as I replied to it, but obviously I didn't.
Wonder WTF happened ...

and the second qemuOpenFile in qemuDomainSaveMemory to touch up the
header (virQEMUSaveDataFinish) probably could use qemuOpenFileAs too
right?  although perhaps less important since the answer should be the
same, just the journey a little different.

Leaving just one consumer for qemuOpenFile and dynamic_ownership
manipulation.

Reviewed-by: John Ferlan 

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] use model "qemu" for s390 tcg cpu-model-expansion

[Collin Walling]

On 07/24/2018 12:03 PM, Jiri Denemark wrote:
> On Mon, Jul 23, 2018 at 17:45:32 -0400, Collin Walling wrote:
>> When calling the query-cpu-model-expansion command via libvirt, we pass in 
>> model 
>> name "max" when using TCG. However, this model name is not supported for 
>> s390 on 
>> TCG. Let's use the model name "qemu" as an alternative.
>>
>> QEMU Results:
>>
>> When executing query-cpu-model-expansion via QEMU on s390 with TCG, the name 
>> "max" 
>> results in this error:
>>
>> { "execute": "query-cpu-model-expansion", "arguments": {"type": "static", 
>> "model": {"name": "max"}}}
>>
>> -> {"error": {"class": "GenericError", "desc": "The CPU definition 'max' is 
>> unknown."}}
>>
>> An alternative is to use model name "qemu." On a z13.2 machine:
>>
>> { "execute": "query-cpu-model-expansion", "arguments": {"type": "static", 
>> "model": {"name": "qemu"}}}
>>
>> -> {"return": {"model": {"name": "zEC12.2-base", "props": {"dateh2": false, 
>> "aen": true, "kmac-tdea-192": false, 
>> "kmc-tdea-192": false, "parseh": false, "csske": false, "hfpm": false, 
>> "hfpue": false, "dfp": false, "km-dea": false, 
>> "emon": false, "kimd-sha-1": false, "cmpsceh": false, "dfpzc": false, 
>> "dfphp": false, "kmc-dea": false, 
>> "klmd-sha-1": false, "asnlxr": false, "km-tdea-192": false, "km-tdea-128": 
>> false, "fpe": false, "kmac-dea": false, 
>> "kmc-tdea-128": false, "ais": true, "kmac-tdea-128": false, "nonqks": false, 
>> "pfpo": false, "msa4-base": true, 
>> "msa3-base": true, "tods": false
> 
> Providing a cover letter for a single patch is pointless, all contents
> which you would put in it should go into the commit message of the patch
> itself.
> 

It probably would've made more sense to send the QEMU output as an attachment 
instead.

>> I am by all means *not* an expert in TCG, but I noticed this error while I 
>> was messing
>> around with things on my system.
> 
> Except this, but you could put it after ---

Good to know. Thanks! :)

> 
> Jirka
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 


-- 
Respectfully,
- Collin Walling

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: caps: use model "qemu" for s390 tcg cpu-model-expansion

[Collin Walling]

On 07/24/2018 12:59 PM, Daniel P. Berrangé wrote:
> On Tue, Jul 24, 2018 at 06:53:54PM +0200, Cornelia Huck wrote:
>> On Tue, 24 Jul 2018 18:08:21 +0200
>> Jiri Denemark  wrote:
>>
>>> On Mon, Jul 23, 2018 at 17:45:33 -0400, Collin Walling wrote:
 Use model name "qemu" instead of "max" when calling
 query-cpu-model-expansion for s390 on tcg.

 Signed-off-by: Collin Walling 
 ---
  src/qemu/qemu_capabilities.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

 diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
 index 23b4833..e9b44cc 100644
 --- a/src/qemu/qemu_capabilities.c
 +++ b/src/qemu/qemu_capabilities.c
 @@ -2356,7 +2356,10 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps,
  
  if (tcg || !virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) {
  virtType = VIR_DOMAIN_VIRT_QEMU;
 -model = "max";
 +if (ARCH_IS_S390(qemuCaps->arch))
 +model = "qemu";
 +else
 +model = "max";  
>>>
>>> I think we should also check if "max" is a supported model
>>> (qemuCaps->tcgCPUModels is already populated at this point) and only use
>>> "qemu" on s390 if "max" is not supported. And please, report the issue
>>> to QEMU developers since one of the reasons behind "max" is its
>>> universal availability on everywhere CPU model expansion is supported.
>>
>> Hm, can you point me to that discussion? A quick search through the
>> QEMU log gives me the addition of the "max" model on i386 as a
>> replacement to the "host" model for !kvm, but nothing about it being
>> universal...
> 
> I don't recall the link but "max" is supposed to be the standard shorthand
> for "enable all the features supported by the virt type". IOW, 'max' should
> work both KVM and TCG ("host" was only well defined for KVM), and across
> all architectures.
> 
> So having to use a different name on s390 is a bug in QEMU imho.
> 
> Regards,
> Daniel
> 

Thanks for expanding on what the "max" model name is suppose to be. I wonder if 
a 
s/"qemu"/"max" in QEMU would suffice (I'm taking a shot in the dark here.)

@Connie, @David, you both are far more knowledgeable in this area than I am. 
What
do either of you suggest for moving forward with this? Should we forward this
discussion on qemu-devel?

-- 
Respectfully,
- Collin Walling

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/9] util: Refactor virResctrlAllocFormat of virresctrl

[bing.niu]

On 2018年07月25日 06:03, John Ferlan wrote:



On 07/18/2018 03:57 AM, bing@intel.com wrote:

From: Bing Niu 

Refactor virResctrlAllocFormat so that it is easy to support other
resource allocation technologies.

Signed-off-by: Bing Niu 
---
  src/util/virresctrl.c | 43 ---
  1 file changed, 28 insertions(+), 15 deletions(-)

diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index 98e7296..1515de2 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -848,17 +848,13 @@ virResctrlAllocGetID(virResctrlAllocPtr alloc)
  }
  
  
-char *

-virResctrlAllocFormat(virResctrlAllocPtr alloc)
+static int
+virResctrlAllocFormatCache(virResctrlAllocPtr alloc, virBufferPtr buf)


One argument per line.  I'll adjust.


  {
-virBuffer buf = VIR_BUFFER_INITIALIZER;
  unsigned int level = 0;
  unsigned int type = 0;
  unsigned int cache = 0;
  


[...]

  
-virBufferCheckError();

+if (virBufferCheckError(buf) < 0)
+return -1;
+else
+return 0;


Just return virBufferCheckError(buf); directly

I'll adjust before pushing.

Thanks for that. ;)


Reviewed-by: John Ferlan 

John



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/9] util: Refactor virResctrlGetInfo in virresctrl

[bing.niu]

On 2018年07月25日 06:02, John Ferlan wrote:



On 07/18/2018 03:57 AM, bing@intel.com wrote:

From: Bing Niu 

Separate resctrl common information parts from CAT specific parts,
so that common information parts can be reused among different
resource allocation technologies.

Signed-off-by: Bing Niu 
---
  src/util/virresctrl.c | 31 ++-
  1 file changed, 22 insertions(+), 9 deletions(-)



I'm assuming you are coordinating with the [RFC PATCHv2 00/10] x86 RDT
Cache Monitoring Technology (CMT)​ that's on list as well. I don't have
the cycles to do any compare/contrast between the two...
Yup~ Huaqiang in cc list is working on CMT part. We already sync off 
line. Since monitor is more complicated and a brand new implementing to 
replace old perf based monitor functions. Huaqiang will absorb Martin's 
comment and update his RFC patch. :)



diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index 6d69c8d..98e7296 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -318,9 +318,8 @@ virResctrlUnlock(int fd)
  
  /* virResctrlInfo-related definitions */

  static int
-virResctrlGetInfo(virResctrlInfoPtr resctrl)
+virResctrlGetCacheInfo(virResctrlInfoPtr resctrl, DIR *dirp)


One argument per line... I can fix before pushing though.
My bad! I should remember one argument per line for function, instead of 
just splitting line only beyond 80 characters. :(


Reviewed-by: John Ferlan 

John



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 4/9] util: Add MBA capability information query to resctrl

[bing.niu]

On 2018年07月25日 06:08, John Ferlan wrote:



On 07/18/2018 03:57 AM, bing@intel.com wrote:

From: Bing Niu 

Introducing virResctrlInfoMemBW for the information memory bandwidth
allocation information. Those information is used for memory
bandwidth allocating.


Last sentence is duplicitous.



Signed-off-by: Bing Niu 
---
  src/util/virresctrl.c | 104 ++
  1 file changed, 104 insertions(+)

diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index 1515de2..06e2702 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -80,6 +80,9 @@ typedef virResctrlInfoPerType *virResctrlInfoPerTypePtr;
  typedef struct _virResctrlInfoPerLevel virResctrlInfoPerLevel;
  typedef virResctrlInfoPerLevel *virResctrlInfoPerLevelPtr;
  
+typedef struct _virResctrlInfoMemBW virResctrlInfoMemBW;

+typedef virResctrlInfoMemBW *virResctrlInfoMemBWPtr;
+
  typedef struct _virResctrlAllocPerType virResctrlAllocPerType;
  typedef virResctrlAllocPerType *virResctrlAllocPerTypePtr;
  
@@ -116,11 +119,31 @@ struct _virResctrlInfoPerLevel {

  virResctrlInfoPerTypePtr *types;
  };
  
+/* Information about memory bandwidth allocation */

+struct _virResctrlInfoMemBW {
+/* minimum memory bandwidth allowed */
+unsigned int min_bandwidth;
+/* bandwidth granularity */
+unsigned int bandwidth_granularity;
+/* Maximum number of simultaneous allocations */
+unsigned int max_allocation;
+/* level number of last level cache */
+unsigned int last_level_cache;
+/* max id of last level cache, this is used to track
+ * how many last level cache available in host system,
+ * the number of memory bandwidth allocation controller
+ * is identical with last level cache.
+ */


The closing */ can be on the previous line.


+unsigned int max_id;
+};
+
  struct _virResctrlInfo {
  virObject parent;
  
  virResctrlInfoPerLevelPtr *levels;

  size_t nlevels;
+
+virResctrlInfoMemBWPtr membw_info;
  };
  
  
@@ -146,6 +169,7 @@ virResctrlInfoDispose(void *obj)

  VIR_FREE(level);
  }
  
+VIR_FREE(resctrl->membw_info);

  VIR_FREE(resctrl->levels);
  }
  
@@ -442,6 +466,65 @@ virResctrlGetCacheInfo(virResctrlInfoPtr resctrl, DIR *dirp)
  
  
  static int

+virResctrlGetMemoryBandwidthInfo(virResctrlInfoPtr resctrl)
+{
+int ret = -1;
+int rv = -1;
+virResctrlInfoMemBWPtr i_membw = NULL;
+
+/* query memory bandwidth allocation info */
+if (VIR_ALLOC(i_membw) < 0)
+goto cleanup;
+rv = virFileReadValueUint(_membw->bandwidth_granularity,
+  SYSFS_RESCTRL_PATH "/info/MB/bandwidth_gran");
+if (rv == -2) {
+/* The file doesn't exist, so it's unusable for us,
+ *  properly mba unsupported */


s/ properly/probably/

??  Is that what you meant?



Yes! probably. :)

s/mba/memory bandwidth/


s/mba/memory bandwidth allocation/


right?


+VIR_INFO("The path '" SYSFS_RESCTRL_PATH "/info/MB/bandwidth_gran'"
+ "does not exist");
+ret = 0;
+goto cleanup;
+} else if (rv < 0) {
+/* Other failures are fatal, so just quit */
+goto cleanup;
+}
+
+rv = virFileReadValueUint(_membw->min_bandwidth,
+  SYSFS_RESCTRL_PATH "/info/MB/min_bandwidth");
+if (rv == -2) {
+/* If the previous file exists, so should this one.  Hence -2 is


s/  Hence/ Hence/

It's a raging debate at times on this list whether to go with 1 space or
2. I think 1 space usually wins, but I use 2 many times as well. I'll
adjust this to 1 space though ;-)


Good to know this history :). Above 2 space is just following the 
existing virresctrl code.



+ * fatal in this case (errors out in next condition) - the
+ * kernel interface might've changed too much or something else is
+ * wrong. */


"kernel" can fit on previous line meaning "wrong." can fit on previous
line.  Also, no need for blank line between here and virReportError


Sorry I am a bit lost. Do you mean put "kernel" in previous line? right?
I have a try in Thunderbird. Looks like put “kernel” in previous line 
beyond max length of line and Thunderbird give me automatically split. :(



+
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Cannot get min bandwidth from resctrl memory info"));
+}
+if (rv < 0)
+goto cleanup;
+
+rv = virFileReadValueUint(_membw->max_allocation,
+  SYSFS_RESCTRL_PATH "/info/MB/num_closids");
+if (rv == -2) {
+/* If the previous file exists, so should this one.  Hence -2 is
+ * fatal in this case as well (errors out in next condition) - the
+ * kernel interface might've changed too much or something else is
+ * wrong. */


I'd just note /* Similar reasoning as min_bandwidth above */


Short and clear!



+

Re: [libvirt] [PATCH v2 2/3] storage: Introduce iscsi_direct pool type

[Michal Privoznik]

On 07/24/2018 06:19 PM, Clementine Hayat wrote:
> 2018-07-24 14:24 GMT+02:00 Michal Privoznik :
>> On 07/23/2018 08:43 PM, c...@lse.epita.fr wrote:
>>> From: Clementine Hayat 
>>>
>>> Introducing the pool as a noop. Integration inside the build
>>> system. Implementation will be in the following commits.
>>>
>>> Signed-off-by: Clementine Hayat 
>>> ---
>>>  configure.ac   |  6 ++-
>>>  m4/virt-storage-iscsi-direct.m4| 41 +++
>>>  src/conf/domain_conf.c |  4 ++
>>>  src/conf/storage_conf.c| 33 ++--
>>>  src/conf/storage_conf.h|  1 +
>>>  src/conf/virstorageobj.c   |  2 +
>>>  src/storage/Makefile.inc.am| 22 
>>>  src/storage/storage_backend.c  |  6 +++
>>>  src/storage/storage_backend_iscsi_direct.c | 58 ++
>>>  src/storage/storage_backend_iscsi_direct.h |  6 +++
>>>  src/storage/storage_driver.c   |  1 +
>>>  tools/virsh-pool.c |  3 ++
>>>  12 files changed, 178 insertions(+), 5 deletions(-)
>>>  create mode 100644 m4/virt-storage-iscsi-direct.m4
>>>  create mode 100644 src/storage/storage_backend_iscsi_direct.c
>>>  create mode 100644 src/storage/storage_backend_iscsi_direct.h
>>


>>> @@ -803,6 +814,19 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
>>>  goto error;
>>>  }
>>>
>>> +if (ret->type == VIR_STORAGE_POOL_ISCSI_DIRECT) {
>>> +if (!ret->source.initiator.iqn) {
>>> +virReportError(VIR_ERR_XML_ERROR, "%s",
>>> +   _("missing storage pool initiator iqn"));
>>> +goto error;
>>> +}
>>> +if (!ret->source.ndevice) {
>>> +virReportError(VIR_ERR_XML_ERROR, "%s",
>>> +   _("missing storage pool device path"));
>>> +goto error;
>>> +}
>>> +}
>>
>> So the wole idea of poolOptions and volOptions is to specify which parts
>> of pool/volume XML are required so that we don't have to base checks
>> like this on ret->type rather than flags.
>> On the other hand, comment just above VIR_STORAGE_POOL_SOURCE_* enum
>> says it declares mandatory components which it clearly doesn't. So after
>> all I think we are safe here.
> 
> That actually isn't the case for the initiator iqn flag.
> Is it on purpose or should I patch it in another thread?

I think saving that for a separate patch is okay.
Speaking of threads, I forgot to mention, feel free to send v3 as a
completely new thread. We don't really thread versions under v1.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 06/10] virCommandWait: Propagate dryRunCallback return value properly

[Michal Privoznik]

On 07/24/2018 04:53 PM, John Ferlan wrote:
> 
> Let's just go back to the start on this one as light has dawned on
> marblehead and it's now clear what the technicality I was missing on the
> reinterpretation of the API docs.
> 
> On 07/04/2018 05:23 AM, Michal Privoznik wrote:
>> The documentation to virCommandWait() function states that if
>> @exitstatus is NULL and command finished with error -1 is
>> returned. In other words, if @dryRunCallback is set and returns
>> an error (by setting its @status argument to a nonzero value) we
>> must propagate this error properly honouring the documentation
>> (and also regular run).
> 
> 
> 
> The documentation to virCommandWait() functions states "If @exitstatus
> is NULL, then the child must exit with status 0 for this to succeed."
> 
> Thus if a dryRunCallback environment doesn't pass @exitstatus, then when
> @dryRunStatus is not zero we must return -1.
> 
> ...
> 
> So the technicality is that dryRunCallback is always set - whether it's
> set to 0 in virCommandRunAsync or (inexplicably at this point) to some
> other value in some as yet to be created mocked/fictitious environment.
> 
> My first gut instinct on this was why not just return dryRunStatus, but
> that conflicts with the exitstatus rules and well I just couldn't get
> past that.
> 
> 
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/util/vircommand.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
> 
> Reviewed-by: John Ferlan 
> 

I've pushed these. Thanks!

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/2] MAINTAINERS: New section "Incompatible changes", copy libvir-list

[Eric Blake]

On 07/16/2018 04:39 AM, Daniel P. Berrangé wrote:



Note, that libvir-list requires senders to be subscribed before mails are
delivered, otherwise they go into moderation queue. My normal approach. as
the person moderating, is to whitelist all human senders who get moderated,
so it is only a one-time pain point per person.


Actually, the list bot is even more stupid than that - a first-time 
poster that subscribes first is STILL moderated (and sometimes you can 
see evidence of that when someone posts 2 or 3 nearly identical messages 
in a short time - first before subscribing, then again after, thinking 
that subscribing will make a difference). Whitelisting happens 
independently from subscription.




Thankyou spammers :-(


But also a hearty thank you to all list moderators that reduce the 
amount of spam actually making it to the list.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] run: Fix LIBVIRTD_PATH

[Cole Robinson]

It wasn't updated when libvirt was moved from daemon/ to src/

Signed-off-by: Cole Robinson 
---
 run.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/run.in b/run.in
index cbef61a674..06ad54b62b 100644
--- a/run.in
+++ b/run.in
@@ -63,7 +63,7 @@ export PKG_CONFIG_PATH
 export LIBVIRT_DRIVER_DIR="$b/src/.libs"
 export LIBVIRT_LOCK_MANAGER_PLUGIN_DIR="$b/src/.libs"
 export VIRTLOCKD_PATH="$b/src"
-export LIBVIRTD_PATH="$b/daemon"
+export LIBVIRTD_PATH="$b/src"
 
 # This is a cheap way to find some use-after-free and uninitialized
 # read problems when using glibc.
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] use model "qemu" for s390 tcg cpu-model-expansion

[Jiri Denemark]

On Mon, Jul 23, 2018 at 17:45:32 -0400, Collin Walling wrote:
> When calling the query-cpu-model-expansion command via libvirt, we pass in 
> model 
> name "max" when using TCG. However, this model name is not supported for s390 
> on 
> TCG. Let's use the model name "qemu" as an alternative.
> 
> QEMU Results:
> 
> When executing query-cpu-model-expansion via QEMU on s390 with TCG, the name 
> "max" 
> results in this error:
> 
> { "execute": "query-cpu-model-expansion", "arguments": {"type": "static", 
> "model": {"name": "max"}}}
> 
> -> {"error": {"class": "GenericError", "desc": "The CPU definition 'max' is 
> unknown."}}
> 
> An alternative is to use model name "qemu." On a z13.2 machine:
> 
> { "execute": "query-cpu-model-expansion", "arguments": {"type": "static", 
> "model": {"name": "qemu"}}}
> 
> -> {"return": {"model": {"name": "zEC12.2-base", "props": {"dateh2": false, 
> "aen": true, "kmac-tdea-192": false, 
> "kmc-tdea-192": false, "parseh": false, "csske": false, "hfpm": false, 
> "hfpue": false, "dfp": false, "km-dea": false, 
> "emon": false, "kimd-sha-1": false, "cmpsceh": false, "dfpzc": false, 
> "dfphp": false, "kmc-dea": false, 
> "klmd-sha-1": false, "asnlxr": false, "km-tdea-192": false, "km-tdea-128": 
> false, "fpe": false, "kmac-dea": false, 
> "kmc-tdea-128": false, "ais": true, "kmac-tdea-128": false, "nonqks": false, 
> "pfpo": false, "msa4-base": true, 
> "msa3-base": true, "tods": false

Providing a cover letter for a single patch is pointless, all contents
which you would put in it should go into the commit message of the patch
itself.

> I am by all means *not* an expert in TCG, but I noticed this error while I 
> was messing
> around with things on my system.

Except this, but you could put it after ---

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] run: Fix LIBVIRTD_PATH

[Jiri Denemark]

On Tue, Jul 24, 2018 at 12:01:35 -0400, Cole Robinson wrote:
> It wasn't updated when libvirt was moved from daemon/ to src/
> 
> Signed-off-by: Cole Robinson 
> ---
>  run.in | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/run.in b/run.in
> index cbef61a674..06ad54b62b 100644
> --- a/run.in
> +++ b/run.in
> @@ -63,7 +63,7 @@ export PKG_CONFIG_PATH
>  export LIBVIRT_DRIVER_DIR="$b/src/.libs"
>  export LIBVIRT_LOCK_MANAGER_PLUGIN_DIR="$b/src/.libs"
>  export VIRTLOCKD_PATH="$b/src"
> -export LIBVIRTD_PATH="$b/daemon"
> +export LIBVIRTD_PATH="$b/src"
>  
>  # This is a cheap way to find some use-after-free and uninitialized
>  # read problems when using glibc.

Reviewed-by: Jiri Denemark 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: caps: use model "qemu" for s390 tcg cpu-model-expansion

[Jiri Denemark]

On Mon, Jul 23, 2018 at 17:45:33 -0400, Collin Walling wrote:
> Use model name "qemu" instead of "max" when calling
> query-cpu-model-expansion for s390 on tcg.
> 
> Signed-off-by: Collin Walling 
> ---
>  src/qemu/qemu_capabilities.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 23b4833..e9b44cc 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -2356,7 +2356,10 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps,
>  
>  if (tcg || !virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) {
>  virtType = VIR_DOMAIN_VIRT_QEMU;
> -model = "max";
> +if (ARCH_IS_S390(qemuCaps->arch))
> +model = "qemu";
> +else
> +model = "max";

I think we should also check if "max" is a supported model
(qemuCaps->tcgCPUModels is already populated at this point) and only use
"qemu" on s390 if "max" is not supported. And please, report the issue
to QEMU developers since one of the reasons behind "max" is its
universal availability on everywhere CPU model expansion is supported.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 2/3] storage: Introduce iscsi_direct pool type

[Clementine Hayat]

2018-07-24 14:24 GMT+02:00 Michal Privoznik :
> On 07/23/2018 08:43 PM, c...@lse.epita.fr wrote:
>> From: Clementine Hayat 
>>
>> Introducing the pool as a noop. Integration inside the build
>> system. Implementation will be in the following commits.
>>
>> Signed-off-by: Clementine Hayat 
>> ---
>>  configure.ac   |  6 ++-
>>  m4/virt-storage-iscsi-direct.m4| 41 +++
>>  src/conf/domain_conf.c |  4 ++
>>  src/conf/storage_conf.c| 33 ++--
>>  src/conf/storage_conf.h|  1 +
>>  src/conf/virstorageobj.c   |  2 +
>>  src/storage/Makefile.inc.am| 22 
>>  src/storage/storage_backend.c  |  6 +++
>>  src/storage/storage_backend_iscsi_direct.c | 58 ++
>>  src/storage/storage_backend_iscsi_direct.h |  6 +++
>>  src/storage/storage_driver.c   |  1 +
>>  tools/virsh-pool.c |  3 ++
>>  12 files changed, 178 insertions(+), 5 deletions(-)
>>  create mode 100644 m4/virt-storage-iscsi-direct.m4
>>  create mode 100644 src/storage/storage_backend_iscsi_direct.c
>>  create mode 100644 src/storage/storage_backend_iscsi_direct.h
>
> Missing documentation. I can not push these without any documentation.
> You need to document what the new type is doing, what the XML should
> look like. Also, might be worth putting some test cases into
> storagepoolxml2xmltest.
> Since you will be sending v3, can you add docs/news.xml entry (in a
> separate patch) too please?

Yes sure.

>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 7396616eda..5af27a6ad2 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -30163,6 +30163,10 @@ 
>> virDomainDiskTranslateSourcePool(virDomainDiskDefPtr def)
>>
>>  break;
>>
>> +case VIR_STORAGE_POOL_ISCSI_DIRECT:
>> +def->src->srcpool->mode = VIR_STORAGE_SOURCE_POOL_MODE_DIRECT;
>> +break;
>> +
>>  case VIR_STORAGE_POOL_ISCSI:
>>  if (def->startupPolicy) {
>>  virReportError(VIR_ERR_XML_ERROR, "%s",
>> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
>> index 5036ab9ef8..ee1586410b 100644
>> --- a/src/conf/storage_conf.c
>> +++ b/src/conf/storage_conf.c
>> @@ -62,9 +62,9 @@ VIR_ENUM_IMPL(virStoragePool,
>>VIR_STORAGE_POOL_LAST,
>>"dir", "fs", "netfs",
>>"logical", "disk", "iscsi",
>> -  "scsi", "mpath", "rbd",
>> -  "sheepdog", "gluster", "zfs",
>> -  "vstorage")
>> +  "iscsi-direct", "scsi", "mpath",
>> +  "rbd", "sheepdog", "gluster",
>> +  "zfs", "vstorage")
>>
>>  VIR_ENUM_IMPL(virStoragePoolFormatFileSystem,
>>VIR_STORAGE_POOL_FS_LAST,
>> @@ -207,6 +207,17 @@ static virStoragePoolTypeInfo poolTypeInfo[] = {
>>   .formatToString = virStoragePoolFormatDiskTypeToString,
>>}
>>  },
>> +{.poolType = VIR_STORAGE_POOL_ISCSI_DIRECT,
>> + .poolOptions = {
>> + .flags = (VIR_STORAGE_POOL_SOURCE_HOST |
>> +   VIR_STORAGE_POOL_SOURCE_DEVICE |
>> +   VIR_STORAGE_POOL_SOURCE_NETWORK |
>> +   VIR_STORAGE_POOL_SOURCE_INITIATOR_IQN),
>> +  },
>> +  .volOptions = {
>> + .formatToString = virStoragePoolFormatDiskTypeToString,
>> +  }
>> +},
>>  {.poolType = VIR_STORAGE_POOL_SCSI,
>>   .poolOptions = {
>>   .flags = (VIR_STORAGE_POOL_SOURCE_ADAPTER),
>> @@ -803,6 +814,19 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
>>  goto error;
>>  }
>>
>> +if (ret->type == VIR_STORAGE_POOL_ISCSI_DIRECT) {
>> +if (!ret->source.initiator.iqn) {
>> +virReportError(VIR_ERR_XML_ERROR, "%s",
>> +   _("missing storage pool initiator iqn"));
>> +goto error;
>> +}
>> +if (!ret->source.ndevice) {
>> +virReportError(VIR_ERR_XML_ERROR, "%s",
>> +   _("missing storage pool device path"));
>> +goto error;
>> +}
>> +}
>
> So the wole idea of poolOptions and volOptions is to specify which parts
> of pool/volume XML are required so that we don't have to base checks
> like this on ret->type rather than flags.
> On the other hand, comment just above VIR_STORAGE_POOL_SOURCE_* enum
> says it declares mandatory components which it clearly doesn't. So after
> all I think we are safe here.

That actually isn't the case for the initiator iqn flag.
Is it on purpose or should I patch it in another thread?

>> +
>>   cleanup:
>>  VIR_FREE(uuid);
>>  VIR_FREE(type);
>> @@ -1004,7 +1028,8 @@ virStoragePoolDefFormatBuf(virBufferPtr buf,
>>   * files, so they don't have a target */
>>  if (def->type != VIR_STORAGE_POOL_RBD &&
>>  def->type != VIR_STORAGE_POOL_SHEEPDOG &&
>> -def->type != 

[libvirt] [dbus PATCH] connect: fix calls to virtDBusConnectOpen

[Anya Harter]

Signed-off-by: Anya Harter 
---
 src/domain.c  | 2 +-
 src/interface.c   | 2 +-
 src/network.c | 2 +-
 src/nodedev.c | 2 +-
 src/nwfilter.c| 2 +-
 src/secret.c  | 2 +-
 src/storagepool.c | 2 +-
 src/storagevol.c  | 2 +-
 8 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/domain.c b/src/domain.c
index 5c5e6da..e8b1a0e 100644
--- a/src/domain.c
+++ b/src/domain.c
@@ -152,7 +152,7 @@ virtDBusDomainGetVirDomain(virtDBusConnect *connect,
 {
 virDomainPtr domain;
 
-if (virtDBusConnectOpen(connect, error) < 0)
+if (!virtDBusConnectOpen(connect, error))
 return NULL;
 
 domain = virtDBusUtilVirDomainFromBusPath(connect->connection,
diff --git a/src/interface.c b/src/interface.c
index 191b8be..6c24885 100644
--- a/src/interface.c
+++ b/src/interface.c
@@ -10,7 +10,7 @@ virtDBusInterfaceGetVirInterface(virtDBusConnect *connect,
 {
 virInterfacePtr interface;
 
-if (virtDBusConnectOpen(connect, error) < 0)
+if (!virtDBusConnectOpen(connect, error))
 return NULL;
 
 interface = virtDBusUtilVirInterfaceFromBusPath(connect->connection,
diff --git a/src/network.c b/src/network.c
index 3957ee6..f3d5472 100644
--- a/src/network.c
+++ b/src/network.c
@@ -21,7 +21,7 @@ virtDBusNetworkGetVirNetwork(virtDBusConnect *connect,
 {
 virNetworkPtr network;
 
-if (virtDBusConnectOpen(connect, error) < 0)
+if (!virtDBusConnectOpen(connect, error))
 return NULL;
 
 network = virtDBusUtilVirNetworkFromBusPath(connect->connection,
diff --git a/src/nodedev.c b/src/nodedev.c
index aa649c6..04fe29c 100644
--- a/src/nodedev.c
+++ b/src/nodedev.c
@@ -10,7 +10,7 @@ virtDBusNodeDeviceGetVirNodeDevice(virtDBusConnect *connect,
 {
 virNodeDevicePtr dev;
 
-if (virtDBusConnectOpen(connect, error) < 0)
+if (!virtDBusConnectOpen(connect, error))
 return NULL;
 
 dev = virtDBusUtilVirNodeDeviceFromBusPath(connect->connection,
diff --git a/src/nwfilter.c b/src/nwfilter.c
index 3fc6e12..d101778 100644
--- a/src/nwfilter.c
+++ b/src/nwfilter.c
@@ -10,7 +10,7 @@ virtDBusNWFilterGetVirNWFilter(virtDBusConnect *connect,
 {
 virNWFilterPtr nwfilter;
 
-if (virtDBusConnectOpen(connect, error) < 0)
+if (!virtDBusConnectOpen(connect, error))
 return NULL;
 
 nwfilter = virtDBusUtilVirNWFilterFromBusPath(connect->connection,
diff --git a/src/secret.c b/src/secret.c
index 903cfc2..85982f1 100644
--- a/src/secret.c
+++ b/src/secret.c
@@ -10,7 +10,7 @@ virtDBusSecretGetVirSecret(virtDBusConnect *connect,
 {
 virSecretPtr secret;
 
-if (virtDBusConnectOpen(connect, error) < 0)
+if (!virtDBusConnectOpen(connect, error))
 return NULL;
 
 secret = virtDBusUtilVirSecretFromBusPath(connect->connection,
diff --git a/src/storagepool.c b/src/storagepool.c
index 6eb6f27..b4d4f93 100644
--- a/src/storagepool.c
+++ b/src/storagepool.c
@@ -10,7 +10,7 @@ virtDBusStoragePoolGetVirStoragePool(virtDBusConnect *connect,
 {
 virStoragePoolPtr storagePool;
 
-if (virtDBusConnectOpen(connect, error) < 0)
+if (!virtDBusConnectOpen(connect, error))
 return NULL;
 
 storagePool = virtDBusUtilVirStoragePoolFromBusPath(connect->connection,
diff --git a/src/storagevol.c b/src/storagevol.c
index e5d1686..6ddff47 100644
--- a/src/storagevol.c
+++ b/src/storagevol.c
@@ -10,7 +10,7 @@ virtDBusStorageVolGetVirStorageVol(virtDBusConnect *connect,
 {
 virStorageVolPtr storageVol;
 
-if (virtDBusConnectOpen(connect, error) < 0)
+if (!virtDBusConnectOpen(connect, error))
 return NULL;
 
 storageVol = virtDBusUtilVirStorageVolFromBusPath(connect->connection,
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2] qemuDomainSaveMemory: Don't enforce dynamicOwnership

[Michal Privoznik]

On 07/09/2018 02:51 PM, Michal Privoznik wrote:
> 

Polite ping.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [dbus PATCH v2 06/17] Test Destroy method for Interface Interface

[Katerina Koukiou]

On Fri, Jul 20, 2018 at 02:34:50PM -0400, Anya Harter wrote:
> Signed-off-by: Anya Harter 
> ---
>  tests/test_interface.py | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tests/test_interface.py b/tests/test_interface.py
> index 88be5dc..62fd517 100755
> --- a/tests/test_interface.py
> +++ b/tests/test_interface.py
> @@ -7,6 +7,10 @@ class TestInterface(libvirttest.BaseTestClass):
>  """ Tests for methods and properties of the Interface interface
>  """
>  
> +def test_interface_destroy(self):
> +_,interface_obj = self.interface_create()

Same as previous patch, interface_create should be fixture and ran at
setup of the test.
I 'll fix it in seperate patch.

> +interface_obj.Destroy(0)
> +
>  def test_interface_create(self):
>  _,interface_obj = self.interface_create()
>  interface_obj.Destroy(0)
> -- 
> 2.17.1
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

Reviewed-by: Katerina Koukiou 


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [dbus PATCH v2 08/17] Implement GetXMLDesc method for Interface Interface

[Katerina Koukiou]

On Fri, Jul 20, 2018 at 02:34:52PM -0400, Anya Harter wrote:
> Signed-off-by: Anya Harter 
> ---
>  data/org.libvirt.Interface.xml |  6 ++
>  src/interface.c| 28 
>  tests/test_interface.py|  4 
>  3 files changed, 38 insertions(+)

Reviewed-by: Katerina Koukiou 


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [dbus PATCH v2 16/17] Implement InterfaceLookupByName method for Connect Interface

[Katerina Koukiou]

On Fri, Jul 20, 2018 at 02:35:00PM -0400, Anya Harter wrote:
> Signed-off-by: Anya Harter 
> ---
>  data/org.libvirt.Connect.xml |  6 ++
>  src/connect.c| 29 +
>  tests/test_connect.py| 12 
>  3 files changed, 47 insertions(+)

Fixture issues will be fixed in seperate patch.

Reviewed-by: Katerina Koukiou 


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [dbus PATCH v2 17/17] Implement InterfaceLookupByMAC method for Connect Interface

[Katerina Koukiou]

On Fri, Jul 20, 2018 at 02:35:01PM -0400, Anya Harter wrote:
> Signed-off-by: Anya Harter 
> ---
>  data/org.libvirt.Connect.xml |  6 ++
>  src/connect.c| 29 +
>  tests/test_connect.py|  1 +
>  3 files changed, 36 insertions(+)

Reviewed-by: Katerina Koukiou 


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH for-3.1] s390x: remove 's390-squash-mcss' option

[Cornelia Huck]

On Tue, 24 Jul 2018 13:15:50 +0200
Peter Krempa  wrote:

> On Tue, Jul 24, 2018 at 11:24:13 +0200, Cornelia Huck wrote:
> > This option has been deprecated for two releases; remove it.
> > 
> > Signed-off-by: Cornelia Huck 
> > ---
> >  hw/s390x/3270-ccw.c|  2 +-
> >  hw/s390x/css-bridge.c  |  1 -
> >  hw/s390x/css.c |  6 ++
> >  hw/s390x/s390-ccw.c|  2 +-
> >  hw/s390x/s390-virtio-ccw.c | 37 
> > ++---
> >  hw/s390x/virtio-ccw.c  |  2 +-
> >  include/hw/s390x/css-bridge.h  |  1 -
> >  include/hw/s390x/css.h |  9 +++--
> >  include/hw/s390x/s390-virtio-ccw.h |  1 -
> >  qemu-deprecated.texi   |  8 
> >  qemu-options.hx| 10 --
> >  target/s390x/cpu.c | 10 --
> >  target/s390x/cpu.h |  1 -
> >  13 files changed, 10 insertions(+), 80 deletions(-)  
> 
> Looks like libvirt did not use this option at all according to my brief
> grepping, so it should be okay to remove it.

Yes, the option has never been used to my knowledge.

Thanks for checking; it seems adding libvir-list as reviewer for
qemu-deprecated.texi is working as designed :)

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 RESEND 00/12] PCI passthrough support on s390

[Andrea Bolognani]

On Tue, 2018-07-24 at 11:31 +0200, Boris Fiuczynski wrote:
> On 07/24/2018 10:20 AM, Andrea Bolognani wrote:
> > Assuming the above is a correct reading of the situation, it
> > would seem the IDs are the only guest-visible part that we need
> > to make sure doesn't change during the lifetime of the guest,
> > while the usual bus/slot/function triplet assigned to the
> > underlying PCI device doesn't actually matter. And if that's the
> > case, wouldn't something like
> > 
> >
> 
> Then a pci device on s390 would need a special address type zpci in 
> libvirt and the design idea for libvirt is to stay compatible with pci.

Being compatible with the existing PCI machinery is certainly a
good idea when it makes sense to do so, but I'm not quite convinced
that is the case here.

According to Cornelia's blog post on the subject, the PCI topology
inside the guest will be determined entirely by the IDs. Is there
even a way to eg. use bridges to create a non-flat PCI hierarchy?
Or to have several PCI devices share the same bus or slot?

If none of the above applies, then that doesn't look a whole lot
like PCI to me :)

Moreover, we already have several address types in addition to PCI
such as USB, virtio-mmio, spapr-vio, ccw... Adding yet another one
is not a problem if it makes the interface more sensible.

More concrete questions: one of the zPCI test cases includes



  
  

  
  


which translates to

  -device zpci,uid=3,fid=2,target=pci.1,id=zpci3 \
  -device pci-bridge,chassis_nr=1,id=pci.1,bus=pci.0,addr=0x1 \
  -device zpci,uid=65535,fid=4294967295,target=hostdev0,id=zpci65535 \
  -device vfio-pci,host=:00:00.0,id=hostdev0,bus=pci.1,addr=0x1f \

How does the pci-bridge controller show up in the guest, if at all?

Do the bus= and addr= attributes of vfio-pci and pci-bridge in the
example above matter eg. for migration purposes?

> Therefore uid and fid are optional attributes and if not specified on 
> s390 they are generated. The patch series also allows on s390 to specify 
> the pci address type just with attributes uid and/or fid causing the 
> rest of the pci address attributes to be generated.

This goes without saying: users should not have to worry about
addresses at all unless they have very specific needs.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH for-3.1] s390x: remove 's390-squash-mcss' option

[Thomas Huth]

On 24.07.2018 11:24, Cornelia Huck wrote:
> This option has been deprecated for two releases; remove it.
> 
> Signed-off-by: Cornelia Huck 
> ---
>  hw/s390x/3270-ccw.c|  2 +-
>  hw/s390x/css-bridge.c  |  1 -
>  hw/s390x/css.c |  6 ++
>  hw/s390x/s390-ccw.c|  2 +-
>  hw/s390x/s390-virtio-ccw.c | 37 ++---
>  hw/s390x/virtio-ccw.c  |  2 +-
>  include/hw/s390x/css-bridge.h  |  1 -
>  include/hw/s390x/css.h |  9 +++--
>  include/hw/s390x/s390-virtio-ccw.h |  1 -
>  qemu-deprecated.texi   |  8 
>  qemu-options.hx| 10 --
>  target/s390x/cpu.c | 10 --
>  target/s390x/cpu.h |  1 -
>  13 files changed, 10 insertions(+), 80 deletions(-)

Reviewed-by: Thomas Huth 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 1/3] tests: qemuxml2argv: make CAPS_LATEST arch generic

[Peter Krempa]

On Tue, Jul 24, 2018 at 13:31:04 +0200, Boris Fiuczynski wrote:
> From: Bjoern Walk 
> 
> Testing with the latest capabilities has been x86_64 centric. Let's
> remove the hardcoded architecture and give the user the ability to
> specify the desired architecture in the macro.
> 
> Signed-off-by: Bjoern Walk 
> Reviewed-by: Boris Fiuczynski 
> ---
>  tests/qemuxml2argvtest.c | 96 ++--
>  1 file changed, 54 insertions(+), 42 deletions(-)

[...]

> @@ -746,9 +758,9 @@ mymain(void)
>  # define DO_TEST_CAPS_VER(name, ver) \
>  DO_TEST_CAPS_ARCH_VER(name, "x86_64", ver)
>  
> -# define DO_TEST_CAPS_LATEST(name) \
> -DO_TEST_CAPS_INTERNAL(name, "x86_64-latest", NULL, 0, 0, "x86_64", \
> -  capslatest_x86_64, true)
> +# define DO_TEST_CAPS_LATEST(name, arch) \
> +DO_TEST_CAPS_INTERNAL(name, arch "-latest", NULL, 0, 0, arch, \
> +  virHashLookup(capslatest, arch), true)

Please add a DO_TEST_CAPS_LATEST_ARCH version rather than modifying all
of the calls. We have a vast majority of x86_64 tests so we should not
need to change them.



signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 2/3] storage: Introduce iscsi_direct pool type

[Michal Privoznik]

On 07/23/2018 08:43 PM, c...@lse.epita.fr wrote:
> From: Clementine Hayat 
> 
> Introducing the pool as a noop. Integration inside the build
> system. Implementation will be in the following commits.
> 
> Signed-off-by: Clementine Hayat 
> ---
>  configure.ac   |  6 ++-
>  m4/virt-storage-iscsi-direct.m4| 41 +++
>  src/conf/domain_conf.c |  4 ++
>  src/conf/storage_conf.c| 33 ++--
>  src/conf/storage_conf.h|  1 +
>  src/conf/virstorageobj.c   |  2 +
>  src/storage/Makefile.inc.am| 22 
>  src/storage/storage_backend.c  |  6 +++
>  src/storage/storage_backend_iscsi_direct.c | 58 ++
>  src/storage/storage_backend_iscsi_direct.h |  6 +++
>  src/storage/storage_driver.c   |  1 +
>  tools/virsh-pool.c |  3 ++
>  12 files changed, 178 insertions(+), 5 deletions(-)
>  create mode 100644 m4/virt-storage-iscsi-direct.m4
>  create mode 100644 src/storage/storage_backend_iscsi_direct.c
>  create mode 100644 src/storage/storage_backend_iscsi_direct.h

Missing documentation. I can not push these without any documentation.
You need to document what the new type is doing, what the XML should
look like. Also, might be worth putting some test cases into
storagepoolxml2xmltest.
Since you will be sending v3, can you add docs/news.xml entry (in a
separate patch) too please?

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 7396616eda..5af27a6ad2 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -30163,6 +30163,10 @@ virDomainDiskTranslateSourcePool(virDomainDiskDefPtr 
> def)
>  
>  break;
>  
> +case VIR_STORAGE_POOL_ISCSI_DIRECT:
> +def->src->srcpool->mode = VIR_STORAGE_SOURCE_POOL_MODE_DIRECT;
> +break;
> +
>  case VIR_STORAGE_POOL_ISCSI:
>  if (def->startupPolicy) {
>  virReportError(VIR_ERR_XML_ERROR, "%s",
> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
> index 5036ab9ef8..ee1586410b 100644
> --- a/src/conf/storage_conf.c
> +++ b/src/conf/storage_conf.c
> @@ -62,9 +62,9 @@ VIR_ENUM_IMPL(virStoragePool,
>VIR_STORAGE_POOL_LAST,
>"dir", "fs", "netfs",
>"logical", "disk", "iscsi",
> -  "scsi", "mpath", "rbd",
> -  "sheepdog", "gluster", "zfs",
> -  "vstorage")
> +  "iscsi-direct", "scsi", "mpath",
> +  "rbd", "sheepdog", "gluster",
> +  "zfs", "vstorage")
>  
>  VIR_ENUM_IMPL(virStoragePoolFormatFileSystem,
>VIR_STORAGE_POOL_FS_LAST,
> @@ -207,6 +207,17 @@ static virStoragePoolTypeInfo poolTypeInfo[] = {
>   .formatToString = virStoragePoolFormatDiskTypeToString,
>}
>  },
> +{.poolType = VIR_STORAGE_POOL_ISCSI_DIRECT,
> + .poolOptions = {
> + .flags = (VIR_STORAGE_POOL_SOURCE_HOST |
> +   VIR_STORAGE_POOL_SOURCE_DEVICE |
> +   VIR_STORAGE_POOL_SOURCE_NETWORK |
> +   VIR_STORAGE_POOL_SOURCE_INITIATOR_IQN),
> +  },
> +  .volOptions = {
> + .formatToString = virStoragePoolFormatDiskTypeToString,
> +  }
> +},
>  {.poolType = VIR_STORAGE_POOL_SCSI,
>   .poolOptions = {
>   .flags = (VIR_STORAGE_POOL_SOURCE_ADAPTER),
> @@ -803,6 +814,19 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
>  goto error;
>  }
>  
> +if (ret->type == VIR_STORAGE_POOL_ISCSI_DIRECT) {
> +if (!ret->source.initiator.iqn) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> +   _("missing storage pool initiator iqn"));
> +goto error;
> +}
> +if (!ret->source.ndevice) {
> +virReportError(VIR_ERR_XML_ERROR, "%s",
> +   _("missing storage pool device path"));
> +goto error;
> +}
> +}

So the wole idea of poolOptions and volOptions is to specify which parts
of pool/volume XML are required so that we don't have to base checks
like this on ret->type rather than flags.
On the other hand, comment just above VIR_STORAGE_POOL_SOURCE_* enum
says it declares mandatory components which it clearly doesn't. So after
all I think we are safe here.

> +
>   cleanup:
>  VIR_FREE(uuid);
>  VIR_FREE(type);
> @@ -1004,7 +1028,8 @@ virStoragePoolDefFormatBuf(virBufferPtr buf,
>   * files, so they don't have a target */
>  if (def->type != VIR_STORAGE_POOL_RBD &&
>  def->type != VIR_STORAGE_POOL_SHEEPDOG &&
> -def->type != VIR_STORAGE_POOL_GLUSTER) {
> +def->type != VIR_STORAGE_POOL_GLUSTER &&
> +def->type != VIR_STORAGE_POOL_ISCSI_DIRECT) {
>  virBufferAddLit(buf, "\n");
>  virBufferAdjustIndent(buf, 2);

Might be worth updating the comment just above this block ;-)


Michal

Re: [libvirt] [PATCH v2 3/3] storage: Implement iscsi_direct pool backend

[Michal Privoznik]

On 07/23/2018 08:43 PM, c...@lse.epita.fr wrote:
> From: Clementine Hayat 
> 
> We need here libiscsi for the storgae pool backend.
> For the iscsi-direct storage pool, only checkPool and refreshPool should
> be necessary.

Well, they are necessary only for basic support. There is still couple
of APIs worth implementing ;-)

> The pool is state-less and just need the informations within the volume
> to work.
> 
> Signed-off-by: Clementine Hayat 
> ---
>  m4/virt-storage-iscsi-direct.m4|   3 +
>  src/storage/Makefile.inc.am|   2 +
>  src/storage/storage_backend_iscsi_direct.c | 408 -
>  3 files changed, 410 insertions(+), 3 deletions(-)

ACK

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 1/3] configure: Introduce libiscsi in build system

[Michal Privoznik]

On 07/23/2018 08:42 PM, c...@lse.epita.fr wrote:
> From: Clementine Hayat 
> 
> The minimal required version is 1.18.0 because the synchrounous function
> needed were introduced here.
> 
> Signed-off-by: Clementine Hayat 
> ---
>  configure.ac|  3 +++
>  m4/virt-libiscsi.m4 | 30 ++
>  2 files changed, 33 insertions(+)
>  create mode 100644 m4/virt-libiscsi.m4

ACK

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 0/3] iscsi-direct: first part

[Michal Privoznik]

On 07/23/2018 08:42 PM, c...@lse.epita.fr wrote:
> From: Clementine Hayat 
> 
> Hello,
> 
> This is the implementation of the iscsi-direct backend storage pool
> version 2.
> The documentation, some API calls and tests are still missing and will
> be comming in a second part.
> 
> Best Regards,
> 

When posting patches to the list, always make sure they are rebased onto
current HEAD. Fortunately, the merge conflict was small so I could
resolve it instantly.

Usually, the cover letter for any new version of patches looks like this:

There is a link to previous version (at least),
there is a diff to previous version.

A good example looks something like this:

https://www.redhat.com/archives/libvir-list/2018-July/msg01343.html

Also, no need to put extra -- at the end of the cover letter - MTAs
usually interpret it as end of git commit message.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 1/3] tests: qemuxml2argv: make CAPS_LATEST arch generic

[Boris Fiuczynski]

On 07/24/2018 01:59 PM, Peter Krempa wrote:

On Tue, Jul 24, 2018 at 13:31:04 +0200, Boris Fiuczynski wrote:

From: Bjoern Walk 

Testing with the latest capabilities has been x86_64 centric. Let's
remove the hardcoded architecture and give the user the ability to
specify the desired architecture in the macro.

Signed-off-by: Bjoern Walk 
Reviewed-by: Boris Fiuczynski 
---
  tests/qemuxml2argvtest.c | 96 ++--
  1 file changed, 54 insertions(+), 42 deletions(-)


[...]


@@ -746,9 +758,9 @@ mymain(void)
  # define DO_TEST_CAPS_VER(name, ver) \
  DO_TEST_CAPS_ARCH_VER(name, "x86_64", ver)
  
-# define DO_TEST_CAPS_LATEST(name) \

-DO_TEST_CAPS_INTERNAL(name, "x86_64-latest", NULL, 0, 0, "x86_64", \
-  capslatest_x86_64, true)
+# define DO_TEST_CAPS_LATEST(name, arch) \
+DO_TEST_CAPS_INTERNAL(name, arch "-latest", NULL, 0, 0, arch, \
+  virHashLookup(capslatest, arch), true)


Please add a DO_TEST_CAPS_LATEST_ARCH version rather than modifying all
of the calls. We have a vast majority of x86_64 tests so we should not
need to change them.


I think that too many code changes (they are already available in this 
patch anyway) should not be the reason to hide the architecture x86_64 
in DO_TEST_CAPS_LATEST and to introduce a new macro 
DO_TEST_CAPS_LATEST_ARCH for all other architectures (including x86_64). 
Would you prefer architecture specific macros instead? e.g. rename 
DO_TEST_CAPS_LATEST to DO_TEST_CAPS_LATEST_X86_64 and create a new 
DO_TEST_CAPS_LATEST_S390.






--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list




--
Mit freundlichen Grüßen/Kind regards
   Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 RESEND 00/12] PCI passthrough support on s390

[Boris Fiuczynski]

On 07/24/2018 03:41 PM, Andrea Bolognani wrote:

On Tue, 2018-07-24 at 11:31 +0200, Boris Fiuczynski wrote:

On 07/24/2018 10:20 AM, Andrea Bolognani wrote:

Assuming the above is a correct reading of the situation, it
would seem the IDs are the only guest-visible part that we need
to make sure doesn't change during the lifetime of the guest,
while the usual bus/slot/function triplet assigned to the
underlying PCI device doesn't actually matter. And if that's the
case, wouldn't something like




Then a pci device on s390 would need a special address type zpci in
libvirt and the design idea for libvirt is to stay compatible with pci.


Being compatible with the existing PCI machinery is certainly a
good idea when it makes sense to do so, but I'm not quite convinced
that is the case here.


From a user perspective:
I take your example below and reduce it to pci only like this:



  
  

  
  


This works on x86 as well as on s390 where as your suggested zpci 
address type would not allow this. This is what I wanted to express with 
the word "compatible".
As I wrote before: It would also be valid for a user to not care about 
the attributes domain, bus, slot and function and reduce the specified 
set of attributes to e.g. 




According to Cornelia's blog post on the subject, the PCI topology
inside the guest will be determined entirely by the IDs. Is there
even a way to eg. use bridges to create a non-flat PCI hierarchy?
Or to have several PCI devices share the same bus or slot?

If none of the above applies, then that doesn't look a whole lot
like PCI to me :)

Moreover, we already have several address types in addition to PCI
such as USB, virtio-mmio, spapr-vio, ccw... Adding yet another one
is not a problem if it makes the interface more sensible.
Sure you can add one more but wouldn't you end up with e.g. a hostdev 
model vfio-pci with an address type of pci on all pci supporting 
architectures but s390 where you need to use zpci? What would be the 
benefit for the user or higher level management software? Actually I 
would not like to introduce special handling unless required.




More concrete questions: one of the zPCI test cases includes

 
 
   
   
 
   
   
 

which translates to

   -device zpci,uid=3,fid=2,target=pci.1,id=zpci3 \
   -device pci-bridge,chassis_nr=1,id=pci.1,bus=pci.0,addr=0x1 \
   -device zpci,uid=65535,fid=4294967295,target=hostdev0,id=zpci65535 \
   -device vfio-pci,host=:00:00.0,id=hostdev0,bus=pci.1,addr=0x1f \

How does the pci-bridge controller show up in the guest, if at all?

Do the bus= and addr= attributes of vfio-pci and pci-bridge in the
example above matter eg. for migration purposes?


Therefore uid and fid are optional attributes and if not specified on
s390 they are generated. The patch series also allows on s390 to specify
the pci address type just with attributes uid and/or fid causing the
rest of the pci address attributes to be generated.


This goes without saying: users should not have to worry about
addresses at all unless they have very specific needs.




--
Mit freundlichen Grüßen/Kind regards
   Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 RESEND 00/12] PCI passthrough support on s390

[Andrea Bolognani]

On Tue, 2018-07-24 at 17:15 +0200, Boris Fiuczynski wrote:
> On 07/24/2018 03:41 PM, Andrea Bolognani wrote:
> > Being compatible with the existing PCI machinery is certainly a
> > good idea when it makes sense to do so, but I'm not quite convinced
> > that is the case here.
> 
>  From a user perspective:
> I take your example below and reduce it to pci only like this:
> 
>  
>  
>
>
>  
>
> function='0x0'/>
>  
> 
> This works on x86 as well as on s390 where as your suggested zpci 
> address type would not allow this. This is what I wanted to express with 
> the word "compatible".
> As I wrote before: It would also be valid for a user to not care about 
> the attributes domain, bus, slot and function and reduce the specified 
> set of attributes to e.g. 

That's not really what users and management applications pass to
libvirt, though: a more realistic example would be

  


  

  

eg. you specify the host address and leave coming up with a
suitable guest address entirely up to libvirt, in which case
whether the resulting address is type=pci or type=zpci hardly
matters.

If you want to take device address assignment upon yourself, then
you're gonna have to assign addresses to controllers as well, not
to mention specify the entire PCI topology with all which that
entails... Not exactly a common scenario.

> > According to Cornelia's blog post on the subject, the PCI topology
> > inside the guest will be determined entirely by the IDs. Is there
> > even a way to eg. use bridges to create a non-flat PCI hierarchy?
> > Or to have several PCI devices share the same bus or slot?
> > 
> > If none of the above applies, then that doesn't look a whole lot
> > like PCI to me :)
> > 
> > Moreover, we already have several address types in addition to PCI
> > such as USB, virtio-mmio, spapr-vio, ccw... Adding yet another one
> > is not a problem if it makes the interface more sensible.
> 
> Sure you can add one more but wouldn't you end up with e.g. a hostdev 
> model vfio-pci with an address type of pci on all pci supporting 
> architectures but s390 where you need to use zpci? What would be the 
> benefit for the user or higher level management software? Actually I 
> would not like to introduce special handling unless required.

I'm all for offering users an interface that abstracts as many
platform-specific quirks as possible, but there's a balance to be
found and we should be careful not to lean too much the opposite
way.

With my current understanding, it doesn't look to me like zPCI
behaves similarly enough to how PCI behaves on other platforms
for us to sensibly describe both using the same interface, and
the fact that QEMU had to come up with a specific middleware
device seems to confirm my suspicion...

In any case, would you mind answering the questions below? That
would certainly help me gain a better understanding of the whole
issue.

> > More concrete questions: one of the zPCI test cases includes
> > 
> >  
> >  
> >
> >
> >  
> >
> > > function='0x0' uid='0x' fid='0x'/>
> >  
> > 
> > which translates to
> > 
> >-device zpci,uid=3,fid=2,target=pci.1,id=zpci3 \
> >-device pci-bridge,chassis_nr=1,id=pci.1,bus=pci.0,addr=0x1 \
> >-device zpci,uid=65535,fid=4294967295,target=hostdev0,id=zpci65535 \
> >-device vfio-pci,host=:00:00.0,id=hostdev0,bus=pci.1,addr=0x1f \
> > 
> > How does the pci-bridge controller show up in the guest, if at all?
> > 
> > Do the bus= and addr= attributes of vfio-pci and pci-bridge in the
> > example above matter eg. for migration purposes?

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/2] rpc: treat EADDRNOTAVAIL as non-fatal when listening

[Jiri Denemark]

On Tue, Jul 24, 2018 at 15:18:10 +0100, Daniel P. Berrangé wrote:
> Consider creating a listener socket from a hostname that resolves to
> multiple addresses. It might be the case that the hostname resolves to
> both an IPv4 and IPv6 address because it is reachable over both
> protocols, but the IPv6 connectivity is provided off-host. In such a
> case no local NIC will have IPv6 and so bind() would fail with the
> EADDRNOTAVAIL errno. Thus it should be treated as non-fatal as long as
> at least one socket was succesfully bound.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/rpc/virnetsocket.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
> index 8e04d61e98..044e6d8804 100644
> --- a/src/rpc/virnetsocket.c
> +++ b/src/rpc/virnetsocket.c
> @@ -382,7 +382,7 @@ int virNetSocketNewListenTCP(const char *nodename,
>  #endif
>  
>  if (bind(fd, runp->ai_addr, runp->ai_addrlen) < 0) {
> -if (errno != EADDRINUSE) {
> +if (errno != EADDRINUSE && errno != EADDRNOTAVAIL) {
>  virReportSystemError(errno, "%s", _("Unable to bind to 
> port"));
>  goto error;
>  }

Reviewed-by: Jiri Denemark 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/2] socket: preserve real errno when socket/bind calls fail

[Jiri Denemark]

On Tue, Jul 24, 2018 at 15:18:09 +0100, Daniel P. Berrangé wrote:
> When reporting socket/bind failures we want to ensure any fatal error
> reported is as accurate as possible. We'll prefer reporting a bind()
> errno over a socket() errno, because if socket() works but bind() fails
> that is a more significant event.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/rpc/virnetsocket.c | 24 
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
> index fee61ace60..8e04d61e98 100644
> --- a/src/rpc/virnetsocket.c
> +++ b/src/rpc/virnetsocket.c
> @@ -310,8 +310,8 @@ int virNetSocketNewListenTCP(const char *nodename,
>  struct addrinfo hints;
>  int fd = -1;
>  size_t i;
> -bool addrInUse = false;
> -bool familyNotSupported = false;
> +int socketErrno = 0;
> +int bindErrno = 0;
>  virSocketAddr tmp_addr;
>  
>  *retsocks = NULL;
> @@ -351,7 +351,7 @@ int virNetSocketNewListenTCP(const char *nodename,
>  if ((fd = socket(runp->ai_family, runp->ai_socktype,
>   runp->ai_protocol)) < 0) {
>  if (errno == EAFNOSUPPORT) {
> -familyNotSupported = true;
> +socketErrno = errno;
>  runp = runp->ai_next;
>  continue;
>  }
> @@ -386,7 +386,7 @@ int virNetSocketNewListenTCP(const char *nodename,
>  virReportSystemError(errno, "%s", _("Unable to bind to 
> port"));
>  goto error;
>  }
> -addrInUse = true;
> +bindErrno = errno;
>  VIR_FORCE_CLOSE(fd);
>  runp = runp->ai_next;
>  continue;
> @@ -409,14 +409,14 @@ int virNetSocketNewListenTCP(const char *nodename,
>  fd = -1;
>  }
>  
> -if (nsocks == 0 && familyNotSupported) {
> -virReportSystemError(EAFNOSUPPORT, "%s", _("Unable to bind to 
> port"));
> -goto error;
> -}
> -
> -if (nsocks == 0 &&
> -addrInUse) {
> -virReportSystemError(EADDRINUSE, "%s", _("Unable to bind to port"));
> +if (nsocks == 0) {
> +if (bindErrno) {
> +virReportSystemError(bindErrno, "%s", _("Unable to bind to 
> port"));
> +} else if (socketErrno) {
> +virReportSystemError(socketErrno, "%s", _("Unable to create 
> socket"));
> +} else {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("No addresses to 
> bind to"));
> +}

We usually don't use {} around single line bodies, you can remove all of
them here.

>  goto error;
>  }

Reviewed-by: Jiri Denemark 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 RESEND 06/12] conf: Introduce address caching for PCI extensions

[Andrea Bolognani]

On Tue, 2018-07-10 at 16:02 +0800, Yi Min Zhao wrote:
> This patch provides a caching mechanism for the device address
> extensions uid and fid on S390. For efficient sparse address allocation,
> we introduce two hash tables for uid/fid which hold the address set
> information per domain. Also in order to improve performance of
> searching available value, we introduce our own callbacks for the two
> hashtables. In this way, uid/fid is saved in hash key and hash value
> could be any non-NULL pointer due to no operation on hash value. That is
> also the reason why we don't introduce hash value free callback.

Pretty much assuming your hash table implementation doesn't have
any issues, because I lack the expertise to review it properly :)

Some code style issues below.

[...]
> +static uint32_t virZPCIAddrCode(const void *name, uint32_t seed)

The return type and each of the function arguments should be on
separate lines, like

  static uint32_t
  virZPCIAddrCode(const void *name,
  uint32_t seed)

[...]
> +static bool virZPCIAddrEqual(const void *namea, const void *nameb)

Same.

[...]
> +static void *virZPCIAddrCopy(const void *name)

Same.

[...]
> +static void virZPCIAddrKeyFree(void *name)

Same.

[...]
> +int
> +virDomainPCIAddressSetExtensionAlloc(virDomainPCIAddressSetPtr addrs,
> + virDomainPCIAddressExtensionFlags 
> extFlags)
> +{
> +if (extFlags == VIR_PCI_ADDRESS_EXTENSION_ZPCI) {

This should probably be

  if (extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI)

since we're dealing with flags, but given the way you end up
calling the function it might be okay 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 v2 07/10] viriscsitest: Test virISCSIConnectionLogin

[John Ferlan]

On 07/24/2018 04:25 AM, Michal Privoznik wrote:
> On 07/23/2018 03:55 PM, John Ferlan wrote:
>>
>>
>> On 07/23/2018 08:34 AM, Michal Prívozník wrote:
>>> On 07/23/2018 02:12 PM, John Ferlan wrote:


 On 07/23/2018 04:01 AM, Michal Prívozník wrote:
> On 07/17/2018 09:14 PM, John Ferlan wrote:
>>
>>
>> On 07/04/2018 05:23 AM, Michal Privoznik wrote:
>>> Introduce one basic test that tests the simplest case:
>>> logging into portal without any IQN.
>>>
>>> Signed-off-by: Michal Privoznik 
>>> ---
>>>  tests/viriscsitest.c | 46 
>>> ++
>>>  1 file changed, 46 insertions(+)
>>>
>>> diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c
>>> index 3bb3993196..a7287069ba 100644
>>> --- a/tests/viriscsitest.c
>>> +++ b/tests/viriscsitest.c
>>> @@ -94,6 +94,16 @@ static void testIscsiadmCb(const char *const*args,
>>> args[8] && STREQ(args[8], "nonpersistent") &&
>>> args[9] == NULL) {
>>>  ignore_value(VIR_STRDUP(*output, iscsiadmSendtargetsOutput));

 here we do some sort of comparison
>>>
>>> What comparison? The only comparison I see is "args[X] && STREQ(args[X],
>>> ...)".
>>>
>>> If you're referring to VIR_STRDUP() that is setting the pretended output
>>> of the iscsiadm command. It's not comparing anything.
>>>
>>
>> Can you just indicate rather than "nada" what the actual deal is here?
> 
> Okay, how about "/* no output */" instead of "nada"?
> 

Not convinced that's better than nada? Mainly because it's not true and
the example below I believe proves that point. As poor of an API to
configure things as iscsiadm is, it can be quite chatty for output.

Replace -T with --targetname and -p with --portal. Not only is there
visual output, but (at least on my host) there's a sequence of clicks
that happens on login and logout.

>> That is, in our mocked environment when running this command we don't
>> expect to get any output from iscsiadm. If this were a real command then
>> the following would be returned:
>>> ...
>>
>> In my saved iSCSI output file, I have for example:
>>
>> iscsiadm -m node -T iqn.2013-12.com.example:iscsi-chap-pool -p
>> 192.168.122.1 --login
>>
>> returning:
>>
>> Logging in to [iface: default, target:
>> iqn.2013-12.com.example:iscsi-chap-pool, portal: 192.168.122.1,3260]
>> (multiple)
>> Login to [iface: default, target:
>> iqn.2013-12.com.example:iscsi-chap-pool, portal: 192.168.122.1,3260]
>> successful.
> 
> But this output is never processed by our code, therefore it doesn't
> make much sense for our 'mock' to produce any.
> 

Not exactly the point compared to the 3 strings already present in the
code that are in a way processed, but I do understand what you're stating.

Remove yourself from the authorship of the code and you don't find it
strange that some commands aren't producing mock output?  Reviewing such
code would you just be satisfied with "/* nada */"? Wouldn't you want to
read the code, try an example or two, and then make a decision how to
proceed. If during your examples you found output, then where do you
stand on "nada" or "no output"?

I really don't think it's that much of an ask or a reach to supply
information that would instantaneously help someone looking at this code
to know what is going on.  Reading "nada" or "no output" doesn't help.

Reading something like:

 /* mocking real environment output is not feasible for [creating |
updating | logging into], example of real environment is:

xxx

 */

I believe is better - it's not difficult to add, but I'm at the point of
not caring right now because this truly has gone on too long.

For the code/logic:

Reviewed-by: John Ferlan 

Do whatever you want for the comments. I disagree that "nada" or "no
output" is good enough, but it's not worth holding this up.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 0/2] Allow for further expected failures when listening on sockets

[Daniel P . Berrangé]

Daniel P. Berrangé (2):
  socket: preserve real errno when socket/bind calls fail
  rpc: treat EADDRNOTAVAIL as non-fatal when listening

 src/rpc/virnetsocket.c | 26 +-
 1 file changed, 13 insertions(+), 13 deletions(-)

-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 10/10] viriscsitest: Extend virISCSIConnectionLogin test

[John Ferlan]

On 07/24/2018 05:48 AM, Michal Privoznik wrote:
> On 07/23/2018 03:10 PM, John Ferlan wrote:
>>
>>
>> On 07/23/2018 04:01 AM, Michal Prívozník wrote:
>>> On 07/17/2018 09:15 PM, John Ferlan wrote:


 On 07/04/2018 05:23 AM, Michal Privoznik wrote:
> Extend this existing test so that a case when IQN is provided is
> tested too. Since a special iSCSI interface is created and its
> name is randomly generated at runtime we need to link with
> virrandommock to have predictable names.
>
> Signed-off-by: Michal Privoznik 
> ---
>  tests/viriscsitest.c | 75 
> ++--
>  1 file changed, 73 insertions(+), 2 deletions(-)
>

 Should testIscsiadmCbData initialization get { false, false } now (and I
 should have mention in patch 9 that :

 +struct testIscsiadmCbData cbData = { 0 };

 should be

 +struct testIscsiadmCbData cbData = { false };>
 I know that 0 and false are synonymous, but syntactical correctness is
 in play here ;-)
>>>
>>> No. So { 0 } is basically a shortcut for:
>>>
>>> struct testIscsiadmCbData cbData;
>>>
>>> memset(, 0, sizeof(cbData));
>>
>> Oh yeah, right  for literal interpretation of data.
>>
>>>
>>> It has nothing to do with the struct having only two bools (for now). It
>>> is used widely in our code, because it has one great advantage: even if
>>> we add/remove/rearrange items in the struct it is always going to be
>>> zeroed out whereas if I initialized it like this:
>>>
>>> struct testIscsiadmCbData cbData = {false, false};
>>>
>>> and then somebody would append yet another member to the struct they
>>> would either:
>>>
>>> a) have to change all the lines where the struct is initialized
>>> (possibly touching functions that has nothing to do with actual change), or
>>> b) forget to initialize it completely.
>>>
>>>
>>> TL;DR It's just coincidence that the first member is a bool.
>>>

 I also think you're doing two things here:

 1. Generating a dryRun output for "existing" test without initiator.iqn

 2. Adding tests to test initiator.iqn

>>>
>>> Sure. That's how login works. Firstly it lists output of iscsi
>>> interfaces, then it creates a new one and then use that to log in. I
>>> don't see a problem here.
>>>
>>
>> OK, right... Maybe it would have been useful from the literal review POV
>> to be reminded of this...  While sometimes it's a pain to document the
>> reason for something - it perhaps prevents this back and forth later on
>> especially when it's "unique case" code paths.  IIQN isn't really well
>> documented by any stretch...
> 
> Well, I usually read the code when trying to understand how something
> works as our comments/documentation is often incorrect.
> 

If the comments/documentation is often incorrect, then that should be
rectified. Help the next poor soul trying to read what someone thought
was self documenting code. To me that almost reaches the level of a
trivial patch.

>>
> diff --git a/tests/viriscsitest.c b/tests/viriscsitest.c
> index c6e0e3b8ff..55889d04a3 100644
> --- a/tests/viriscsitest.c
> +++ b/tests/viriscsitest.c
> @@ -60,8 +60,19 @@ const char *iscsiadmSendtargetsOutput =
>  "10.20.30.40:3260,1 iqn.2008-04.example:example1:iscsi.bar\n"
>  "10.20.30.40:3260,1 iqn.2009-04.example:example1:iscsi.seven\n";
>  
> +const char *iscsiadmIfaceDefaultOutput =
> +"default tcp\n"
> +"iser iser\n";
> +
> +const char *iscsiadmIfaceIfaceOutput =
> +"default tcp\n"
> +"iser iser\n"
> +"libvirt-iface-03020100 
> tcpiqn.2004-06.example:example1:initiator\n";
> +
> +
>  struct testIscsiadmCbData {
>  bool output_version;
> +bool iface_created;
>  };
>  
>  static void testIscsiadmCb(const char *const*args,
> @@ -103,6 +114,62 @@ static void testIscsiadmCb(const char *const*args,
> args[7] && STREQ(args[7], "--login") &&
> args[8] == NULL) {
>  /* nada */

 Is this "nada"
>>>
>>> Yes, this is "nada" ;-)
>>>
>>
>> Maybe the explanation as to why it's nada and not just leaving nada
>> there would have helped.
> 
> As I say in the other reply, how about s/nada/no output/?
> 
>>
>> In some instances we're doing output comparison (eventually) and in
>> others we're not. In the long run it's mainly a matter of being reminded
>> what the processing is and what (if any) the expected output is.
>> Sometimes that expected output only occurs on the next query, IIRC.
> 
> Yes, this is right.
> 
>>
>> Creating IIQN's or iSCSI targets is not something I do all that often -
>> I wonder if you came back to this code 6 months or longer from now
>> whether you'd (instantly) recall what type of output was generated ;-).
>> If you would then your short term memory is better than mine! I have 

[libvirt] [PATCH 1/2] socket: preserve real errno when socket/bind calls fail

[Daniel P . Berrangé]

When reporting socket/bind failures we want to ensure any fatal error
reported is as accurate as possible. We'll prefer reporting a bind()
errno over a socket() errno, because if socket() works but bind() fails
that is a more significant event.

Signed-off-by: Daniel P. Berrangé 
---
 src/rpc/virnetsocket.c | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index fee61ace60..8e04d61e98 100644
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c
@@ -310,8 +310,8 @@ int virNetSocketNewListenTCP(const char *nodename,
 struct addrinfo hints;
 int fd = -1;
 size_t i;
-bool addrInUse = false;
-bool familyNotSupported = false;
+int socketErrno = 0;
+int bindErrno = 0;
 virSocketAddr tmp_addr;
 
 *retsocks = NULL;
@@ -351,7 +351,7 @@ int virNetSocketNewListenTCP(const char *nodename,
 if ((fd = socket(runp->ai_family, runp->ai_socktype,
  runp->ai_protocol)) < 0) {
 if (errno == EAFNOSUPPORT) {
-familyNotSupported = true;
+socketErrno = errno;
 runp = runp->ai_next;
 continue;
 }
@@ -386,7 +386,7 @@ int virNetSocketNewListenTCP(const char *nodename,
 virReportSystemError(errno, "%s", _("Unable to bind to port"));
 goto error;
 }
-addrInUse = true;
+bindErrno = errno;
 VIR_FORCE_CLOSE(fd);
 runp = runp->ai_next;
 continue;
@@ -409,14 +409,14 @@ int virNetSocketNewListenTCP(const char *nodename,
 fd = -1;
 }
 
-if (nsocks == 0 && familyNotSupported) {
-virReportSystemError(EAFNOSUPPORT, "%s", _("Unable to bind to port"));
-goto error;
-}
-
-if (nsocks == 0 &&
-addrInUse) {
-virReportSystemError(EADDRINUSE, "%s", _("Unable to bind to port"));
+if (nsocks == 0) {
+if (bindErrno) {
+virReportSystemError(bindErrno, "%s", _("Unable to bind to port"));
+} else if (socketErrno) {
+virReportSystemError(socketErrno, "%s", _("Unable to create 
socket"));
+} else {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("No addresses to 
bind to"));
+}
 goto error;
 }
 
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 2/2] rpc: treat EADDRNOTAVAIL as non-fatal when listening

[Daniel P . Berrangé]

Consider creating a listener socket from a hostname that resolves to
multiple addresses. It might be the case that the hostname resolves to
both an IPv4 and IPv6 address because it is reachable over both
protocols, but the IPv6 connectivity is provided off-host. In such a
case no local NIC will have IPv6 and so bind() would fail with the
EADDRNOTAVAIL errno. Thus it should be treated as non-fatal as long as
at least one socket was succesfully bound.

Signed-off-by: Daniel P. Berrangé 
---
 src/rpc/virnetsocket.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index 8e04d61e98..044e6d8804 100644
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c
@@ -382,7 +382,7 @@ int virNetSocketNewListenTCP(const char *nodename,
 #endif
 
 if (bind(fd, runp->ai_addr, runp->ai_addrlen) < 0) {
-if (errno != EADDRINUSE) {
+if (errno != EADDRINUSE && errno != EADDRNOTAVAIL) {
 virReportSystemError(errno, "%s", _("Unable to bind to port"));
 goto error;
 }
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] esx storage: Fix typo lsilogic -> lsiLogic

[Marcos Paulo de Souza]

Commit 77298458d027db4d3e082213355e2d792f65158d changed the esx storage
adapter from busLogic to lsilogic, introducing a typo. Changing it back
to lsiLogic (with capital L) solves the issue. With this change, libvirt can now
create volumes in ESX again.

Thanks to Jaroslav Suchanek who figured out what was the issue in the
first place.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1571759
Signed-off-by: Marcos Paulo de Souza 
---
 src/esx/esx_storage_backend_vmfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/esx/esx_storage_backend_vmfs.c 
b/src/esx/esx_storage_backend_vmfs.c
index 630a6aa8c9..bb2de4b69f 100644
--- a/src/esx/esx_storage_backend_vmfs.c
+++ b/src/esx/esx_storage_backend_vmfs.c
@@ -967,9 +967,9 @@ esxStorageVolCreateXML(virStoragePoolPtr pool,
 /*
  * FIXME: The adapter type is a required parameter, but there is no
  * way to let the user specify it in the volume XML config. Therefore,
- * default to 'lsilogic' here.
+ * default to 'lsiLogic' here.
  */
-virtualDiskSpec->adapterType = (char *)"lsilogic";
+virtualDiskSpec->adapterType = (char *)"lsiLogic";
 
 virtualDiskSpec->capacityKb->value =
   VIR_DIV_UP(def->target.capacity, 1024); /* Scale from byte to 
kilobyte */
-- 
2.17.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 RESEND 03/12] conf: Introduce a new PCI address extension flag

[Andrea Bolognani]

On Tue, 2018-07-10 at 16:02 +0800, Yi Min Zhao wrote:
[...]
> +static bool
> +qemuDomainDeviceSupportZPCI(virDomainDeviceDefPtr device)
> +{
> +switch ((virDomainDeviceType) device->type) {
> +case VIR_DOMAIN_DEVICE_CHR:
> +return false;
> +
> +case VIR_DOMAIN_DEVICE_CONTROLLER:
> +case VIR_DOMAIN_DEVICE_NONE:
> +case VIR_DOMAIN_DEVICE_DISK:
> +case VIR_DOMAIN_DEVICE_LEASE:
> +case VIR_DOMAIN_DEVICE_FS:
> +case VIR_DOMAIN_DEVICE_NET:
> +case VIR_DOMAIN_DEVICE_INPUT:
> +case VIR_DOMAIN_DEVICE_SOUND:
> +case VIR_DOMAIN_DEVICE_VIDEO:
> +case VIR_DOMAIN_DEVICE_HOSTDEV:
> +case VIR_DOMAIN_DEVICE_WATCHDOG:
> +case VIR_DOMAIN_DEVICE_GRAPHICS:
> +case VIR_DOMAIN_DEVICE_HUB:
> +case VIR_DOMAIN_DEVICE_REDIRDEV:
> +case VIR_DOMAIN_DEVICE_SMARTCARD:
> +case VIR_DOMAIN_DEVICE_MEMBALLOON:
> +case VIR_DOMAIN_DEVICE_NVRAM:
> +case VIR_DOMAIN_DEVICE_RNG:
> +case VIR_DOMAIN_DEVICE_SHMEM:
> +case VIR_DOMAIN_DEVICE_TPM:
> +case VIR_DOMAIN_DEVICE_PANIC:
> +case VIR_DOMAIN_DEVICE_MEMORY:
> +case VIR_DOMAIN_DEVICE_IOMMU:
> +case VIR_DOMAIN_DEVICE_VSOCK:
> +case VIR_DOMAIN_DEVICE_LAST:
> +break;

Did you validate that all of the above can be used with zPCI?

Either way, at least _NONE and _LAST should definitely result in
an error being reported, as well as the default case which should
be included; use virReportEnumRangeError() for convenience.

[...]
> +static virDomainPCIAddressExtensionFlags
> +qemuDomainDeviceCalculatePCIAddressExtensionFlags(virQEMUCapsPtr qemuCaps,
> +  virDomainDeviceDefPtr dev)
> +{
> +virDomainPCIAddressExtensionFlags extFlags = 0;
> +
> +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI) &&
> +qemuDomainDeviceSupportZPCI(dev))
> +extFlags |= VIR_PCI_ADDRESS_EXTENSION_ZPCI;

The libvirt code style guidelines[1] state that this should be
formatted as

  if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI) &&
  qemuDomainDeviceSupportZPCI(dev)) {
  extFlags |= VIR_PCI_ADDRESS_EXTENSION_ZPCI;
  }

[1] https://libvirt.org/hacking.html
-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 RESEND 07/12] conf: Introduce parser, formatter for uid and fid

[Andrea Bolognani]

On Tue, 2018-07-10 at 16:02 +0800, Yi Min Zhao wrote:
> +  
> +
> +  
> +(0x)?[0-9a-fA-F]{1,8}
> +  
> +  

This should probably be unignedInt instead of int, but other uint*
types defined in the file also use int so if anything changing all
of them would be the job for a follow-up patch.

[...]
> +  
> +
> +  
> +
> +  
> +(0x)?[0-9a-fA-F]{1,4}
> +  
> +  
> +1
> +65535
> +  
> +
> +  

I don't see why you wouldn't just use the existing uint16 type
here. Is that because uid can't be zero? Making sure that's
actually the case is a job for the PostParse() or Validate()
callback anyway, because schema validation is entirely opt-in
and thus can't be relied upon.

[...]
> +static int
> +virZPCIDeviceAddressIsValid(virZPCIDeviceAddressPtr zpci)
> +{
> +if (!zpci->uid_assigned)
> +return 1;
> +
> +if (zpci->zpci_uid > VIR_DOMAIN_DEVICE_ZPCI_MAX_UID ||
> +zpci->zpci_uid == 0) {
> +virReportError(VIR_ERR_XML_ERROR,
> +   _("Invalid PCI address uid='0x%x', "
> + "must be > 0x0 and <= 0x%x"),
> +   zpci->zpci_uid,
> +   VIR_DOMAIN_DEVICE_ZPCI_MAX_UID);
> +return 0;
> +}

fid should be validated as well.

[...]
> +
> +  QEMUGuest1
> +  c7a5fdbd-edaf-9455-926a-d65c16db1809
> +  219136
> +  1
> +  
> +hvm

The correct machine type would be s390-ccw-virtio.

There are a bunch of existing test files that incorrectly use
s390-ccw, feel free to clean that up as well ;)

[...]
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index bbb995656e..e3282e2b98 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -397,6 +397,8 @@ mymain(void)
>  QEMU_CAPS_VIRTIO_SCSI);
>  DO_TEST("disk-virtio-scsi-ioeventfd",
>  QEMU_CAPS_VIRTIO_SCSI);
> +DO_TEST("disk-virtio-s390-zpci",
> +QEMU_CAPS_DEVICE_ZPCI, QEMU_CAPS_CCW);
>  DO_TEST("disk-scsi-megasas",
>  QEMU_CAPS_SCSI_MEGASAS);
>  DO_TEST("disk-scsi-mptsas1068",
> @@ -476,6 +478,7 @@ mymain(void)
>  DO_TEST("hostdev-usb-address", NONE);
>  DO_TEST("hostdev-pci-address", NONE);
>  DO_TEST("hostdev-vfio", NONE);
> +DO_TEST("hostdev-vfio-zpci", QEMU_CAPS_DEVICE_ZPCI, QEMU_CAPS_CCW);

I haven't actually tried that, but you should be able to add the
test cases to qemuxml2argvtest at the same time as you add them
here, for consistency's sake.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [dbus PATCH] connect: fix calls to virtDBusConnectOpen

[Ján Tomko]

Even though the changes are self-explanatory, the commit message could
use some text, e.g.:

The function returns TRUE/FALSE, not 0/-1.

On Tue, Jul 24, 2018 at 09:31:28AM -0400, Anya Harter wrote:

Signed-off-by: Anya Harter 
---
src/domain.c  | 2 +-
src/interface.c   | 2 +-
src/network.c | 2 +-
src/nodedev.c | 2 +-
src/nwfilter.c| 2 +-
src/secret.c  | 2 +-
src/storagepool.c | 2 +-
src/storagevol.c  | 2 +-
8 files changed, 8 insertions(+), 8 deletions(-)



Reviewed-by: Ján Tomko 

Jano


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 RESEND 01/12] conf: Add definitions for 'uid' and 'fid' PCI address attributes

[Andrea Bolognani]

On Tue, 2018-07-10 at 16:02 +0800, Yi Min Zhao wrote:
>  struct _virPCIDeviceAddress {
>  unsigned int domain;
>  unsigned int bus;
>  unsigned int slot;
>  unsigned int function;
>  int multi; /* virTristateSwitch */
> +virZPCIDeviceAddressPtr zpci;

This should probably be an embedded structure rather than a pointer
to a separate, heap allocated structure.

I remember Laine having somewhat strong feelings about the topic,
so I'll leave arguing for or against that to him :)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 07/10] viriscsitest: Test virISCSIConnectionLogin

[Michal Privoznik]

On 07/24/2018 04:08 PM, John Ferlan wrote:
> 
>  > Reading something like:
> 
>  /* mocking real environment output is not feasible for [creating |
> updating | logging into], example of real environment is:
> 
> xxx
> 
>  */
> 
> I believe is better - it's not difficult to add, but I'm at the point of
> not caring right now because this truly has gone on too long.
> 
> For the code/logic:
> 
> Reviewed-by: John Ferlan 
> 
> Do whatever you want for the comments. I disagree that "nada" or "no
> output" is good enough, but it's not worth holding this up.

Thanks. I'll add the real output into comments as you're suggesting.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 06/10] virCommandWait: Propagate dryRunCallback return value properly

[John Ferlan]

Let's just go back to the start on this one as light has dawned on
marblehead and it's now clear what the technicality I was missing on the
reinterpretation of the API docs.

On 07/04/2018 05:23 AM, Michal Privoznik wrote:
> The documentation to virCommandWait() function states that if
> @exitstatus is NULL and command finished with error -1 is
> returned. In other words, if @dryRunCallback is set and returns
> an error (by setting its @status argument to a nonzero value) we
> must propagate this error properly honouring the documentation
> (and also regular run).



The documentation to virCommandWait() functions states "If @exitstatus
is NULL, then the child must exit with status 0 for this to succeed."

Thus if a dryRunCallback environment doesn't pass @exitstatus, then when
@dryRunStatus is not zero we must return -1.

...

So the technicality is that dryRunCallback is always set - whether it's
set to 0 in virCommandRunAsync or (inexplicably at this point) to some
other value in some as yet to be created mocked/fictitious environment.

My first gut instinct on this was why not just return dryRunStatus, but
that conflicts with the exitstatus rules and well I just couldn't get
past that.


> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/util/vircommand.c | 2 ++
>  1 file changed, 2 insertions(+)
> 

Reviewed-by: John Ferlan 

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 RESEND 08/12] conf: Allocate/release 'uid' and 'fid' in PCI address

[Andrea Bolognani]

On Tue, 2018-07-10 at 16:02 +0800, Yi Min Zhao wrote:
> This patch adds new functions for reservation, assignment and release
> to handle the uid/fid. If the uid/fid is defined in the domain XML,
> they will be reserved directly in collecting phase. If any of them is
> not defined, we will find out an available value for it from zPCI
> address hashtable, and reserve it. For hotplug case, there might be or
> not zPCI definition. So allocate and reserve uid/fid for undefined
> case. Assign if needed and reserve uid/fid for defined case. If the user
> define zPCI extension address but zPCI capability doesn't exist, an
> error will be reported.

For this patch I once again didn't look too closely to the
implementation, sorry.

[...]
> +static int
> +virDomainZPCIAddressReserveId(virHashTablePtr set, unsigned int id,
> +  const char *name)

One argument per line, please.

There are more instances in the patch, but I'm not going to
point them all out :)

[...]
> +static int
> +virDomainZPCIAddressAssignUid(virHashTablePtr set, virZPCIDeviceAddressPtr 
> addr)
> +{
> +if (addr->uid_assigned)
> +return 0;
> +
> +addr->uid_assigned = virDomainZPCIAddressAssignId(set, >zpci_uid, 
> 1,
> +   VIR_DOMAIN_DEVICE_ZPCI_MAX_UID, 
> "uid");

Messed up alignment. More instances further down.

[...]
> +static void
> +virDomainZPCIAddressReleaseUid(virHashTablePtr set, virZPCIDeviceAddressPtr 
> addr)
> +{
> +if (!addr->uid_assigned)
> +return;
> +
> +if (virHashRemoveEntry(set, >zpci_uid))
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Release uid %u failed"), addr->zpci_uid);

Curly braces are required here. More instances further down.


Looking at

> +static void
> +virDomainZPCIAddressReleaseFid(virHashTablePtr set, virZPCIDeviceAddressPtr 
> addr)

and

> +static void
> +virDomainZPCIAddressReleaseIds(virDomainZPCIAddressIdsPtr zpciIds,
> +   virPCIDeviceAddressPtr addr)

and

> +static int
> +virDomainZPCIAddressReserveAddr(virDomainPCIAddressSetPtr addrs,
> +virPCIDeviceAddressPtr addr)

you're being awfully inconsistent about the datatypes you're passing
around...

Unless I've missed something that makes doing so impossible, please
try to make it so only the top-level datatypes (DomainPCIAddressSet
and PCIDeviceAddress) are passed around.

[...]
> +static int
> +virDomainZPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs,
> +virZPCIDeviceAddressPtr zpci)
> +{
> +if (!zpci->uid_assigned &&
> +virDomainZPCIAddressReserveNextUid(addrs->zpciIds->uids, zpci))
> +return -1;

Messed up indentation. Also, missing curly braces.

[...]
> +int
> +virDomainPCIAddressExtensionReserveAddr(virDomainPCIAddressSetPtr addrs,
> +virPCIDeviceAddressPtr addr,
> +virDomainPCIAddressExtensionFlags 
> extFlags)
> +{
> +if (extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
> +/* Reserve uid/fid to ZPCI device which has defined uid/fid
> + * in the domain.
> + */

Messed up indentation.

[...]
> +int
> +virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs,
> +virPCIDeviceAddressPtr dev,
> +
> virDomainPCIAddressExtensionFlags extFlags)
> +{
> +if (extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
> +/* Assign and reserve uid/fid to ZPCI device which has not defined 
> uid/fid
> + * in the domain.
> + */

Messed up indentation.

[...]
> +static int
> +virDomainPCIAddressEnsureExtensionAddr(virDomainPCIAddressSetPtr addrs,
> +   virDomainDeviceInfoPtr dev)

This should be virDomainPCIAddressExtensionEnsureAddr() for
consistency's sake.

> @@ -1385,7 +1403,12 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def 
> ATTRIBUTE_UNUSED,
>   * parent, and will have its address collected during the scan
>   * of the parent's device type.
>  */
> -return 0;
> +if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI ||
> +info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
> +return virDomainPCIAddressExtensionReserveAddr(addrs, addr,
> +   
> info->pciAddressExtFlags);
> +else
> +return 0;

This doesn't look right: the comment specifically states that the
PCI address will be handled by the parent device in this case,
why wouldn't the zPCI address not be handled in the same way?

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 RESEND 09/12] qemu: Generate and use zPCI device in QEMU command line

[Andrea Bolognani]

On Tue, 2018-07-10 at 16:02 +0800, Yi Min Zhao wrote:
[...]
> +bool
> +qemuCheckDeviceIsZPCI(virDomainDeviceInfoPtr info)
> +{
> +if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
> +((info->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) ||
> + info->addr.pci.zpci))
> +return true;

Missing curly braces.

Also, do you really need to check both the flags and the presence
of the zPCI address bits? It looks like either one or the other
should be enough or, if that's not the case, it should be made so
because having to check for two separate conditions makes me feel
like it would introduce bugs in the long run.

[...]
> +char *qemuBuildZPCIDevStr(virDomainDeviceInfoPtr dev);
> +
> +bool qemuCheckDeviceIsZPCI(virDomainDeviceInfoPtr info);

Is this really necessary? Can't these two functions be static?

[...]
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -1014,6 +1014,8 @@ mymain(void)
>  QEMU_CAPS_CCW, QEMU_CAPS_VIRTIO_S390);
>  DO_TEST("disk-virtio-scsi-ccw", QEMU_CAPS_VIRTIO_SCSI,
>  QEMU_CAPS_CCW, QEMU_CAPS_VIRTIO_S390);
> +DO_TEST("disk-virtio-s390-zpci", QEMU_CAPS_DEVICE_ZPCI,
> +QEMU_CAPS_CCW, QEMU_CAPS_VIRTIO_S390);
>  DO_TEST("disk-order",
>  QEMU_CAPS_DRIVE_BOOT, QEMU_CAPS_VIRTIO_BLK_SCSI);
>  DO_TEST("disk-virtio-drive-queues",
> @@ -1574,6 +1576,18 @@ mymain(void)
>  QEMU_CAPS_DEVICE_VFIO_PCI);
>  DO_TEST_PARSE_ERROR("hostdev-mdev-invalid-target-address",
>  QEMU_CAPS_DEVICE_VFIO_PCI);
> +DO_TEST("hostdev-vfio-zpci",
> +QEMU_CAPS_DEVICE_VFIO_PCI, QEMU_CAPS_DEVICE_ZPCI);
> +DO_TEST("hostdev-vfio-zpci-multidomain-many",
> +QEMU_CAPS_DEVICE_VFIO_PCI, X_QEMU_CAPS_HOST_PCI_MULTIDOMAIN,

Capabilities with X_QEMU prefix are no longer used, so you should
not list them here.

> +QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_ZPCI);
> +DO_TEST("hostdev-vfio-zpci-autogenerate",
> +QEMU_CAPS_DEVICE_VFIO_PCI, QEMU_CAPS_DEVICE_ZPCI);
> +DO_TEST("hostdev-vfio-zpci-boundaries",
> +QEMU_CAPS_DEVICE_VFIO_PCI, X_QEMU_CAPS_HOST_PCI_MULTIDOMAIN,
> +QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_ZPCI);
> +DO_TEST_FAILURE("hostdev-vfio-zpci",
> +QEMU_CAPS_DEVICE_VFIO_PCI);

Please add these to qemuxml2xmltest at the same time.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v1 01/40] util: cgroup: modify virCgroupFree to take virCgroupPtr

[Sukrit Bhatnagar]

On Tue, 24 Jul 2018 at 11:33, Erik Skultety  wrote:
>
> On Mon, Jul 23, 2018 at 08:19:21PM +0530, Sukrit Bhatnagar wrote:
> > On Mon, 23 Jul 2018 at 16:29, Erik Skultety  wrote:
> > >
> > > On Sat, Jul 21, 2018 at 05:36:33PM +0530, Sukrit Bhatnagar wrote:
> > > > Modify virCgroupFree function signature to take a value of type
> > > > virCgroupPtr instead of virCgroupPtr * as the parameter.
> > > >
> > > > Change the argument type in all calls to virCgroupFree function
> > > > from virCgroupPtr * to virCgroupPtr.
> > >
> > > ^This sentence doesn't add any useful information. Instead, the commit 
> > > message
> > > should add information about why we're performing this change, i.e. in 
> > > order to
> > > enable usage of VIR_AUTOPTR with cgroup module or something alike.
> > > Also, this patch is oddly placed, IMHO it should come before patch 8, 
> > > where the
> > > other work on cgroup module is done.
> > >
> > > With that:
> > > Reviewed-by: Erik Skultety 
> > >
> > > ...
> > >
> > > > @@ -1379,8 +1379,8 @@ virCgroupNewPartition(const char *path,
> > > >  ret = 0;
> > > >   cleanup:
> > > >  if (ret != 0)
> > > > -virCgroupFree(group);
> > > > -virCgroupFree();
> > > > +virCgroupFree(*group);
> > > > +virCgroupFree(parent);
> > >
> > > Since you're already touching the code, I'd appreciate another 
> > > "adjustment"
> > > patch where occurrences like ^this will be replaced by a VIR_STEAL_PTR, 
> > > IOW
> > > where we're passing a reference to a pointer in order to change the 
> > > original
> > > pointer, we should use a VIR_STEAL_PTR just before the cleanup section, 
> > > so that
> > > we don't need an if (ret != 0) or if (ret < 0) check only to 
> > > conditionally do
> > > some cleanup. Feel free to let me know if none of what I just wrote is 
> > > clear.
> >
> > I am assuming that you are referring to `group` variable. If so, then I 
> > cannot
> > apply cleanup attribute to function parameters and `group` is one of them.
>
> I didn't mean using a cleanup attribute. What I meant was making the necessary
> adjustments in order to get rid of the 'if(ret != 0)' check, since you're
> already touching the code I thought why not going one step further...


You mean something like this?

VIR_AUTOPTR(virCgroup) ptr = NULL;
...
return 0;
 cleanup:
VIR_STEAL_PTR(ptr, *group);
return -1;

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: caps: use model "qemu" for s390 tcg cpu-model-expansion

[Cornelia Huck]

On Tue, 24 Jul 2018 18:08:21 +0200
Jiri Denemark  wrote:

> On Mon, Jul 23, 2018 at 17:45:33 -0400, Collin Walling wrote:
> > Use model name "qemu" instead of "max" when calling
> > query-cpu-model-expansion for s390 on tcg.
> > 
> > Signed-off-by: Collin Walling 
> > ---
> >  src/qemu/qemu_capabilities.c | 5 -
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > index 23b4833..e9b44cc 100644
> > --- a/src/qemu/qemu_capabilities.c
> > +++ b/src/qemu/qemu_capabilities.c
> > @@ -2356,7 +2356,10 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps,
> >  
> >  if (tcg || !virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) {
> >  virtType = VIR_DOMAIN_VIRT_QEMU;
> > -model = "max";
> > +if (ARCH_IS_S390(qemuCaps->arch))
> > +model = "qemu";
> > +else
> > +model = "max";  
> 
> I think we should also check if "max" is a supported model
> (qemuCaps->tcgCPUModels is already populated at this point) and only use
> "qemu" on s390 if "max" is not supported. And please, report the issue
> to QEMU developers since one of the reasons behind "max" is its
> universal availability on everywhere CPU model expansion is supported.

Hm, can you point me to that discussion? A quick search through the
QEMU log gives me the addition of the "max" model on i386 as a
replacement to the "host" model for !kvm, but nothing about it being
universal...

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: caps: use model "qemu" for s390 tcg cpu-model-expansion

[Daniel P . Berrangé]

On Tue, Jul 24, 2018 at 06:53:54PM +0200, Cornelia Huck wrote:
> On Tue, 24 Jul 2018 18:08:21 +0200
> Jiri Denemark  wrote:
> 
> > On Mon, Jul 23, 2018 at 17:45:33 -0400, Collin Walling wrote:
> > > Use model name "qemu" instead of "max" when calling
> > > query-cpu-model-expansion for s390 on tcg.
> > > 
> > > Signed-off-by: Collin Walling 
> > > ---
> > >  src/qemu/qemu_capabilities.c | 5 -
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > > index 23b4833..e9b44cc 100644
> > > --- a/src/qemu/qemu_capabilities.c
> > > +++ b/src/qemu/qemu_capabilities.c
> > > @@ -2356,7 +2356,10 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps,
> > >  
> > >  if (tcg || !virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) {
> > >  virtType = VIR_DOMAIN_VIRT_QEMU;
> > > -model = "max";
> > > +if (ARCH_IS_S390(qemuCaps->arch))
> > > +model = "qemu";
> > > +else
> > > +model = "max";  
> > 
> > I think we should also check if "max" is a supported model
> > (qemuCaps->tcgCPUModels is already populated at this point) and only use
> > "qemu" on s390 if "max" is not supported. And please, report the issue
> > to QEMU developers since one of the reasons behind "max" is its
> > universal availability on everywhere CPU model expansion is supported.
> 
> Hm, can you point me to that discussion? A quick search through the
> QEMU log gives me the addition of the "max" model on i386 as a
> replacement to the "host" model for !kvm, but nothing about it being
> universal...

I don't recall the link but "max" is supposed to be the standard shorthand
for "enable all the features supported by the virt type". IOW, 'max' should
work both KVM and TCG ("host" was only well defined for KVM), and across
all architectures.

So having to use a different name on s390 is a bug in QEMU imho.

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 v1 15/40] util: firewall: use VIR_AUTOFREE instead of VIR_FREE for scalar types

[Sukrit Bhatnagar]

On Tue, 24 Jul 2018 at 11:54, Erik Skultety  wrote:
>
> On Mon, Jul 23, 2018 at 11:18:07PM +0530, Sukrit Bhatnagar wrote:
> > On Mon, 23 Jul 2018 at 18:39, Erik Skultety  wrote:
> > >
> > > On Sat, Jul 21, 2018 at 05:36:47PM +0530, Sukrit Bhatnagar wrote:
> > > > By making use of GNU C's cleanup attribute handled by the
> > > > VIR_AUTOFREE macro for declaring scalar variables, majority
> > > > of the VIR_FREE calls can be dropped, which in turn leads to
> > > > getting rid of most of our cleanup sections.
> > > >
> > > > Signed-off-by: Sukrit Bhatnagar 
> > > > ---
> > > >  src/util/virfirewall.c | 16 +---
> > > >  1 file changed, 5 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
> > > > index dfd792f..b4a4d06 100644
> > > > --- a/src/util/virfirewall.c
> > > > +++ b/src/util/virfirewall.c
> > > > @@ -511,7 +511,7 @@ void virFirewallRuleAddArgFormat(virFirewallPtr 
> > > > firewall,
> > > >   virFirewallRulePtr rule,
> > > >   const char *fmt, ...)
> > > >  {
> > > > -char *arg;
> > > > +VIR_AUTOFREE(char *) arg = NULL;
> > > >  va_list list;
> > > >
> > > >  VIR_FIREWALL_RULE_RETURN_IF_ERROR(firewall, rule);
> > > > @@ -525,13 +525,11 @@ void virFirewallRuleAddArgFormat(virFirewallPtr 
> > > > firewall,
> > > >
> > > >  va_end(list);
> > > >
> > > > -VIR_FREE(arg);
> > > >  return;
> > > >
> > > >   no_memory:
> > > >  firewall->err = ENOMEM;
> > > >  va_end(list);
> > > > -VIR_FREE(arg);
> > >
> > > There could be an additional patch replacing the no_memory label with 
> > > 'cleanup'
> > > with the obvious adjustments.
> >
> > There are many functions in virfirewall.c where no_memory is used
> > instead of cleanup.
> > So I should change either all of them, or none of them.
>
> Yep, so it should be all of them.

Also, according to libvirt hacking page, no_memory is a standard label [1].
Should we replace it then?

[1]: https://libvirt.org/hacking.html#goto

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v1 15/40] util: firewall: use VIR_AUTOFREE instead of VIR_FREE for scalar types

[Daniel P . Berrangé]

On Tue, Jul 24, 2018 at 10:46:54PM +0530, Sukrit Bhatnagar wrote:
> On Tue, 24 Jul 2018 at 11:54, Erik Skultety  wrote:
> >
> > On Mon, Jul 23, 2018 at 11:18:07PM +0530, Sukrit Bhatnagar wrote:
> > > On Mon, 23 Jul 2018 at 18:39, Erik Skultety  wrote:
> > > >
> > > > On Sat, Jul 21, 2018 at 05:36:47PM +0530, Sukrit Bhatnagar wrote:
> > > > > By making use of GNU C's cleanup attribute handled by the
> > > > > VIR_AUTOFREE macro for declaring scalar variables, majority
> > > > > of the VIR_FREE calls can be dropped, which in turn leads to
> > > > > getting rid of most of our cleanup sections.
> > > > >
> > > > > Signed-off-by: Sukrit Bhatnagar 
> > > > > ---
> > > > >  src/util/virfirewall.c | 16 +---
> > > > >  1 file changed, 5 insertions(+), 11 deletions(-)
> > > > >
> > > > > diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c
> > > > > index dfd792f..b4a4d06 100644
> > > > > --- a/src/util/virfirewall.c
> > > > > +++ b/src/util/virfirewall.c
> > > > > @@ -511,7 +511,7 @@ void virFirewallRuleAddArgFormat(virFirewallPtr 
> > > > > firewall,
> > > > >   virFirewallRulePtr rule,
> > > > >   const char *fmt, ...)
> > > > >  {
> > > > > -char *arg;
> > > > > +VIR_AUTOFREE(char *) arg = NULL;
> > > > >  va_list list;
> > > > >
> > > > >  VIR_FIREWALL_RULE_RETURN_IF_ERROR(firewall, rule);
> > > > > @@ -525,13 +525,11 @@ void virFirewallRuleAddArgFormat(virFirewallPtr 
> > > > > firewall,
> > > > >
> > > > >  va_end(list);
> > > > >
> > > > > -VIR_FREE(arg);
> > > > >  return;
> > > > >
> > > > >   no_memory:
> > > > >  firewall->err = ENOMEM;
> > > > >  va_end(list);
> > > > > -VIR_FREE(arg);
> > > >
> > > > There could be an additional patch replacing the no_memory label with 
> > > > 'cleanup'
> > > > with the obvious adjustments.
> > >
> > > There are many functions in virfirewall.c where no_memory is used
> > > instead of cleanup.
> > > So I should change either all of them, or none of them.
> >
> > Yep, so it should be all of them.
> 
> Also, according to libvirt hacking page, no_memory is a standard label [1].
> Should we replace it then?

No, the hacking page is correct - 'no_memory' should be used where the
label is specifically related to OOM handling. That is the case in this
code snippet, as the label is explicitly setting "firewall->err = ENOMEM".
Using "cleanup" here would be misleading, suggesting it was generic
error handling.

> 
> [1]: https://libvirt.org/hacking.html#goto

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