Re: [libvirt] [Qemu-devel] change x86 default machine type to Q35?

[Gerd Hoffmann]

  Hi,

> > I think simply not having a default
> > machine type (as already suggested elsewhere in this thread) is the
> > best way to deal with this.
> 
> I would absolutely hate this. One of the nice things about qemu has
> always been that 'qemu disk.img' is enough to start a simple VM.

Well, not really.  There is no "qemu" any more, and there are other
defaults like default memory size which need tweaks, so the minimum
command line isn't that short any more and looks more like this:

qemu-system-x86_64 -enable-kvm -m 1G disk.img

cheers,
  Gerd

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

[libvirt] [PATCH] qemu: hotplug virtio_scsi over lsilogic when sicsi controller is not present

[Liang Yan]

It may be better to check virtio-scsi controller first since it is
supported better in qemu level.


Signed-off-by: Liang Yan
---
 src/qemu/qemu_domain_address.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index b5b863fe4..9bec0790f 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -90,10 +90,10 @@ qemuDomainSetSCSIControllerModel(const virDomainDef *def,
 } else {
 if (qemuDomainIsPSeries(def)) {
 *model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI;
-} else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_LSI)) {
-*model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC;
 } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_SCSI)) {
 *model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI;
+} else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_LSI)) {
+*model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC;
 } else {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Unable to determine model for scsi controller"));
-- 
2.13.2

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

Re: [libvirt] [PATCH v4 3/3] Add TLS support for Veritas HyperScale (VxHS) block device protocol

[ashish mittal]

On Fri, Jun 30, 2017 at 2:21 PM, John Ferlan  wrote:
> [...]
>
>>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>>> index 8e00782..99bc94f 100644
>>> --- a/src/qemu/qemu_command.c
>>> +++ b/src/qemu/qemu_command.c
>>> @@ -931,6 +931,68 @@ qemuBuildGlusterDriveJSON(virStorageSourcePtr src)
>>>  return ret;
>>>  }
>>>
>>> +/* qemuBuildDiskVxHSTLSinfoCommandLine:
>>> + * @cmd: Pointer to the command string
>>> + * @cfg: Pointer to the qemu driver config
>>> + * @disk: The disk we are processing
>>> + * @qemuCaps: qemu capabilities object
>>> + *
>>> + * Check if the VxHS disk meets all the criteria to enable TLS.
>>> + * If yes, add a new TLS object and mention it's ID on the disk
>>> + * command line.
>>> + *
>>> + * Returns 0 on success, -1 w/ error on some sort of failure.
>>> + */
>>> +static int
>>> +qemuBuildDiskVxHSTLSinfoCommandLine(virCommandPtr cmd,
>>> +virQEMUDriverConfigPtr cfg,
>>> +virDomainDiskDefPtr disk,
>>> +virQEMUCapsPtr qemuCaps)
>>> +{
>>> +int ret = 0;
>>> +
>>> +if (cfg->vxhsTLS  == true && disk->src->haveTLS != 
>>> VIR_TRISTATE_BOOL_NO) {
>>> +disk->src->addTLS = true;
>>> +ret = qemuBuildTLSx509CommandLine(cmd, cfg->vxhsTLSx509certdir,
>>> +  false,
>>> +  true,
>>> +  false,
>>> +  "vxhs",
>>
>> This argument can't be a constant. This is the alias for the certificate
>> object.
>>
>> Otherwise you'd had to make sure that there's only one such object,
>> which obviously would make sense here, since (if you don't hotplug disks
>> after libvirtd restart) the TLS credentials are the same for this disk.
>>
>> In case of hotplug though you need to make sure that the TLS object is
>> removed with the last disk and added if any other disk needing TLS is
>> added.
>>
>
> So at least the conversation we had last week now makes a bit more sense
> w/r/t the call to qemuBuildTLSx509CommandLine for chardevTLSx509certdir.
> As I look at that code now quickly, although having multiple
> tls-cert-x509 objects for each chardev isn't necessary, it does "work"
> or start qemu because each would have a different alias (@charAlias is
> uniquely generated via qemuAliasChardevFromDevAlias). Theoretically
> speaking two objects wouldn't be required, except for the problem that
> hotunplug would have to be made aware and we'd have to keep track of
> when the last one was removed. So leaving with each having their own
> object is the way the code will stay.
>
> So given all that - your alias creation is going to have to contain that
> uuid or you are going to have to figure out a way to just have one
> object which each disk uses. You'll have to scan the disks looking to
> determine if any of the vxhs ones have tls and if so, break to add the
> object.  Then add the 'tls-creds=$object_alias_id'.
>

Hi John, Peter,

The problem statement - Alias for the TLS certificate can't be a
constant. Two TLS objects cannot have the same ID/alias.

This was pointed out by both of you as something that needs to be
fixed. To that end, I have made some changes to use the block device
domain alias (e.g. virtio-disk0) as a unique identifier for each TLS
object. This is similar to how char devices create their TLS aliases.

I was hoping to get some feedback on whether this diff would be
acceptable to fix the said issue. I will reply to/fix all the other
comments. Just thought I'd tackle this one first as this appears to be
one of the bigger ones...

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 99bc94f..fc58236 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -952,13 +952,19 @@ qemuBuildDiskVxHSTLSinfoCommandLine(virCommandPtr cmd,
 int ret = 0;

 if (cfg->vxhsTLS  == true && disk->src->haveTLS != VIR_TRISTATE_BOOL_NO) {
-disk->src->addTLS = true;
-ret = qemuBuildTLSx509CommandLine(cmd, cfg->vxhsTLSx509certdir,
-  false,
-  true,
-  false,
-  "vxhs",
-  qemuCaps);
+if (virAsprintf(>src->aliasTLS,
+"vxhs_%s", disk->info.alias) < 0) {
+ret = -1;
+goto cleanup;
+}
+
+disk->src->addTLS = true;
+ret = qemuBuildTLSx509CommandLine(cmd, cfg->vxhsTLSx509certdir,
+  false,
+  true,
+  false,
+  disk->src->aliasTLS,
+  

Re: [libvirt] [PATCH v4 3/3] Add TLS support for Veritas HyperScale (VxHS) block device protocol

[ashish mittal]

On Fri, Jun 30, 2017 at 2:58 PM, ashish mittal  wrote:
> On Fri, Jun 30, 2017 at 2:21 PM, John Ferlan  wrote:
>> [...]
>>
 diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
 index 8e00782..99bc94f 100644
 --- a/src/qemu/qemu_command.c
 +++ b/src/qemu/qemu_command.c
 @@ -931,6 +931,68 @@ qemuBuildGlusterDriveJSON(virStorageSourcePtr src)
  return ret;
  }

 +/* qemuBuildDiskVxHSTLSinfoCommandLine:
 + * @cmd: Pointer to the command string
 + * @cfg: Pointer to the qemu driver config
 + * @disk: The disk we are processing
 + * @qemuCaps: qemu capabilities object
 + *
 + * Check if the VxHS disk meets all the criteria to enable TLS.
 + * If yes, add a new TLS object and mention it's ID on the disk
 + * command line.
 + *
 + * Returns 0 on success, -1 w/ error on some sort of failure.
 + */
 +static int
 +qemuBuildDiskVxHSTLSinfoCommandLine(virCommandPtr cmd,
 +virQEMUDriverConfigPtr cfg,
 +virDomainDiskDefPtr disk,
 +virQEMUCapsPtr qemuCaps)
 +{
 +int ret = 0;
 +
 +if (cfg->vxhsTLS  == true && disk->src->haveTLS != 
 VIR_TRISTATE_BOOL_NO) {
 +disk->src->addTLS = true;
 +ret = qemuBuildTLSx509CommandLine(cmd, 
 cfg->vxhsTLSx509certdir,
 +  false,
 +  true,
 +  false,
 +  "vxhs",
>>>
>>> This argument can't be a constant. This is the alias for the certificate
>>> object.
>>>
>>> Otherwise you'd had to make sure that there's only one such object,
>>> which obviously would make sense here, since (if you don't hotplug disks
>>> after libvirtd restart) the TLS credentials are the same for this disk.
>>>
>>> In case of hotplug though you need to make sure that the TLS object is
>>> removed with the last disk and added if any other disk needing TLS is
>>> added.
>>>
>>
>> So at least the conversation we had last week now makes a bit more sense
>> w/r/t the call to qemuBuildTLSx509CommandLine for chardevTLSx509certdir.
>> As I look at that code now quickly, although having multiple
>> tls-cert-x509 objects for each chardev isn't necessary, it does "work"
>> or start qemu because each would have a different alias (@charAlias is
>> uniquely generated via qemuAliasChardevFromDevAlias). Theoretically
>> speaking two objects wouldn't be required, except for the problem that
>> hotunplug would have to be made aware and we'd have to keep track of
>> when the last one was removed. So leaving with each having their own
>> object is the way the code will stay.
>>
>> So given all that - your alias creation is going to have to contain that
>> uuid or you are going to have to figure out a way to just have one
>> object which each disk uses. You'll have to scan the disks looking to
>> determine if any of the vxhs ones have tls and if so, break to add the
>> object.  Then add the 'tls-creds=$object_alias_id'.
>>
>
> This makes sense. Will get back with the diff that could achieve this.
>
>> BTW: if you haven't already, read Dan Berrange's blog on TLS:
>>
>> https://www.berrange.com/posts/2016/04/01/improving-qemu-security-part-2-generic-tls-support/
>>
>> that's a link to page 2, read/scan the remaining 5 blogs as well. Some
>> of the exact qemu syntax has changed since the blog was written, but the
>> description is really what helps the frame of reference.
>>
>
> Will do. Thanks!
>
 +  qemuCaps);
 +} else if (cfg->vxhsTLS  == false &&
 +   disk->src->haveTLS == VIR_TRISTATE_BOOL_YES) {
 +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
 +   _("Please enable VxHS specific TLS options in the 
 qemu "
 + "conf file before using TLS in VxHS device 
 domain "
 + "specification"));
 +ret = -1;
 +}
 +
 +return ret;
 +}
>>
>> [...]
>>
 diff --git 
 a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args
  
 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args
 new file mode 100644
 index 000..960960d
 --- /dev/null
 +++ 
 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args
>>>
>>> [this file has same mistake as the one below]
>>>
 diff --git 
 a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args.new
  
 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args.new
 new file mode 100644
 index 000..960960d

[libvirt] [PATCH v2] network: allow to specify buffer size for netlink socket

[ZhiPeng Lu]

This patchs allow to set the buffer size for netlink socket in
the libvirtd configuration file. The default buffer size remain
as before at 128k.

Signed-off-by: Zhipeng Lu 
---
 daemon/libvirtd-config.c|  6 ++
 daemon/libvirtd-config.h|  2 ++
 daemon/libvirtd.aug |  1 +
 daemon/libvirtd.c   | 12 
 daemon/libvirtd.conf|  3 +++
 daemon/test_libvirtd.aug.in |  1 +
 src/libvirt_private.syms|  1 +
 src/util/virnetlink.c   | 19 ++-
 src/util/virnetlink.h   |  7 ++-
 9 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c
index 6c0f00e..b2bda28 100644
--- a/daemon/libvirtd-config.c
+++ b/daemon/libvirtd-config.c
@@ -33,6 +33,7 @@
 #include "remote/remote_protocol.h"
 #include "remote/remote_driver.h"
 #include "util/virnetdevopenvswitch.h"
+#include "util/virnetlink.h"
 #include "virstring.h"
 #include "virutil.h"
 
@@ -172,6 +173,8 @@ daemonConfigNew(bool privileged ATTRIBUTE_UNUSED)
 data->admin_keepalive_count = 5;
 
 data->ovs_timeout = VIR_NETDEV_OVS_DEFAULT_TIMEOUT;
+
+data->netlink_sock_buffer_size = VIRT_NETLINK_SOCK_BUFFER_SIZE;
 
 localhost = virGetHostname();
 if (localhost == NULL) {
@@ -394,6 +397,9 @@ daemonConfigLoadOptions(struct daemonConfig *data,
 if (virConfGetValueUInt(conf, "ovs_timeout", >ovs_timeout) < 0)
 goto error;
 
+if (virConfGetValueUInt(conf, "netlink_sock_buffer_size", 
>netlink_sock_buffer_size) < 0)
+goto error;
+
 return 0;
 
  error:
diff --git a/daemon/libvirtd-config.h b/daemon/libvirtd-config.h
index 1edf5fa..22befd1 100644
--- a/daemon/libvirtd-config.h
+++ b/daemon/libvirtd-config.h
@@ -94,6 +94,8 @@ struct daemonConfig {
 unsigned int admin_keepalive_count;
 
 unsigned int ovs_timeout;
+
+unsigned int netlink_sock_buffer_size;
 };
 
 
diff --git a/daemon/libvirtd.aug b/daemon/libvirtd.aug
index 24fdf44..84ea00a 100644
--- a/daemon/libvirtd.aug
+++ b/daemon/libvirtd.aug
@@ -89,6 +89,7 @@ module Libvirtd =
let misc_entry = str_entry "host_uuid"
   | str_entry "host_uuid_source"
   | int_entry "ovs_timeout"
+  | int_entry "netlink_sock_buffer_size"
 
(* Each enty in the config is one of the following three ... *)
let entry = network_entry
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index a558458..34db23a 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -648,6 +648,16 @@ daemonSetupNetDevOpenvswitch(struct daemonConfig *config)
 
 
 /*
+ * Set up the netlink socket  buffer size
+ */
+static void
+daemonSetupNetLink(struct daemonConfig *config)
+{
+virNetLinkSetBufferSize(config->netlink_sock_buffer_size);
+}
+
+
+/*
  * Set up the logging environment
  * By default if daemonized all errors go to the logfile libvirtd.log,
  * but if verbose or error debugging is asked for then also output
@@ -1257,6 +1267,8 @@ int main(int argc, char **argv) {
 exit(EXIT_FAILURE);
 }
 
+daemonSetupNetLink(config);
+
 daemonSetupNetDevOpenvswitch(config);
 
 if (daemonSetupAccessManager(config) < 0) {
diff --git a/daemon/libvirtd.conf b/daemon/libvirtd.conf
index e83e9a1..b174767 100644
--- a/daemon/libvirtd.conf
+++ b/daemon/libvirtd.conf
@@ -476,3 +476,6 @@
 # potential infinite waits blocking libvirt.
 #
 #ovs_timeout = 5
+
+# This allow to specify buffer size for netlink socket.
+#netlink_sock_buffer_size = 131072
diff --git a/daemon/test_libvirtd.aug.in b/daemon/test_libvirtd.aug.in
index 1200952..0a1207f 100644
--- a/daemon/test_libvirtd.aug.in
+++ b/daemon/test_libvirtd.aug.in
@@ -64,3 +64,4 @@ module Test_libvirtd =
 { "admin_keepalive_interval" = "5" }
 { "admin_keepalive_count" = "5" }
 { "ovs_timeout" = "5" }
+{ "netlink_sock_buffer_size" = "131072" }
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 888412a..83be39d 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2248,6 +2248,7 @@ virNetlinkEventServiceStart;
 virNetlinkEventServiceStop;
 virNetlinkEventServiceStopAll;
 virNetlinkGetErrorCode;
+virNetLinkSetBufferSize;
 virNetlinkShutdown;
 virNetlinkStartup;
 
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
index d732fe8..0a549b7 100644
--- a/src/util/virnetlink.c
+++ b/src/util/virnetlink.c
@@ -49,6 +49,7 @@ VIR_LOG_INIT("util.netlink");
 
 #define NETLINK_ACK_TIMEOUT_S  (2*1000)
 
+
 #if defined(__linux__) && defined(HAVE_LIBNL)
 /* State for a single netlink event handle */
 struct virNetlinkEventHandle {
@@ -104,6 +105,22 @@ static int nextWatch = 1;
 static virNetlinkEventSrvPrivatePtr server[MAX_LINKS] = {NULL};
 static virNetlinkHandle *placeholder_nlhandle;
 
+/*
+ * Set netlink  default buffer size
+ */
+static unsigned int virNetLinkBufferSize = VIRT_NETLINK_SOCK_BUFFER_SIZE; 
+
+/**
+ * virNetLinkSetBufferSize:
+ * @size: the buffer size
+ *
+ * Set 

Re: [libvirt] [PATCH v2 5/5] Prevent more compiler optimization of mockable functions

[Marc-André Lureau]

On Wed, Jul 12, 2017 at 1:54 AM Marc-André Lureau <
marcandre.lur...@gmail.com> wrote:

> Hi
>
> On Mon, Jul 10, 2017 at 1:14 PM Peter Krempa  wrote:
>
>> On Wed, Jul 05, 2017 at 12:58:51 +0100, Daniel Berrange wrote:
>> > Currently all mockable functions are annotated with the 'noinline'
>> > attribute. This is insufficient to guarantee that a function can
>> > be reliably mocked with an LD_PRELOAD. The C language spec allows
>> > the compiler to assume there is only a single implementation of
>> > each function. It can thus do things like propagating constant
>> > return values into the caller at compile time, or creating
>> > multiple specialized copies of the function body each optimized
>> > for a different caller. To prevent these optimizations we must
>> > also set the 'noclone' and 'weak' attributes.
>> >
>> > This fixes the test suite when libvirt.so is built with CLang
>> > with optimization enabled.
>> >
>> > Signed-off-by: Daniel P. Berrange 
>>
>
> This patch makes virtlogd crash:
>
>  (gdb) bt
> #0  0x in ?? ()
> #1  0x555a8084 in virHashCreateFull (size=size@entry=5,
> dataFree=0x555b0930 , 
> keyCode=keyCode@entry=0x555a7c30
> , keyEqual=keyEqual@entry=0x555a7c10
> , keyCopy=keyCopy@entry=0x555a7bb0 ,
> keyFree=keyFree@entry=0x555a7b90 ) at
> util/virhash.c:167
> #2  0x555a8151 in virHashCreate (size=size@entry=5,
> dataFree=) at util/virhash.c:196
> #3  0x555779f0 in virNetDaemonNew () at rpc/virnetdaemon.c:137
> #4  0x555708ec in virLogDaemonNew (privileged=false,
> config=0x55820940) at logging/log_daemon.c:163
> #5  main (argc=, argv=0x7fffd888) at
> logging/log_daemon.c:1069
>
> any idea?
>
>
(btw, this is on f25,  gcc (GCC) 6.3.1 20161221 (Red Hat 6.3.1-1))

> ---
>> >  build-aux/mock-noinline.pl  |  2 +-
>> >  src/check-symfile.pl|  2 +-
>> >  src/internal.h  | 23 ++-
>> >  src/qemu/qemu_capspriv.h|  2 +-
>> >  src/rpc/virnetsocket.h  |  4 ++--
>> >  src/util/vircommand.h   |  2 +-
>> >  src/util/vircrypto.h|  2 +-
>> >  src/util/virfile.h  |  2 +-
>> >  src/util/virhostcpu.h   |  4 ++--
>> >  src/util/virmacaddr.h   |  2 +-
>> >  src/util/virnetdev.h|  8 
>> >  src/util/virnetdevip.h  |  2 +-
>> >  src/util/virnetdevopenvswitch.h |  2 +-
>> >  src/util/virnetdevtap.h |  6 +++---
>> >  src/util/virnuma.h  | 16 
>> >  src/util/virrandom.h|  6 +++---
>> >  src/util/virscsi.h  |  2 +-
>> >  src/util/virscsivhost.h |  2 +-
>> >  src/util/virtpm.h   |  2 +-
>> >  src/util/virutil.h  | 10 +-
>> >  src/util/viruuid.h  |  2 +-
>> >  21 files changed, 58 insertions(+), 45 deletions(-)
>>
>> ACK
>> --
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>
> --
> Marc-André Lureau
>
-- 
Marc-André Lureau
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 5/5] Prevent more compiler optimization of mockable functions

[Marc-André Lureau]

Hi

On Mon, Jul 10, 2017 at 1:14 PM Peter Krempa  wrote:

> On Wed, Jul 05, 2017 at 12:58:51 +0100, Daniel Berrange wrote:
> > Currently all mockable functions are annotated with the 'noinline'
> > attribute. This is insufficient to guarantee that a function can
> > be reliably mocked with an LD_PRELOAD. The C language spec allows
> > the compiler to assume there is only a single implementation of
> > each function. It can thus do things like propagating constant
> > return values into the caller at compile time, or creating
> > multiple specialized copies of the function body each optimized
> > for a different caller. To prevent these optimizations we must
> > also set the 'noclone' and 'weak' attributes.
> >
> > This fixes the test suite when libvirt.so is built with CLang
> > with optimization enabled.
> >
> > Signed-off-by: Daniel P. Berrange 
>

This patch makes virtlogd crash:

 (gdb) bt
#0  0x in ?? ()
#1  0x555a8084 in virHashCreateFull (size=size@entry=5,
dataFree=0x555b0930 ,
keyCode=keyCode@entry=0x555a7c30
, keyEqual=keyEqual@entry=0x555a7c10 ,
keyCopy=keyCopy@entry=0x555a7bb0 ,
keyFree=keyFree@entry=0x555a7b90
) at util/virhash.c:167
#2  0x555a8151 in virHashCreate (size=size@entry=5,
dataFree=) at util/virhash.c:196
#3  0x555779f0 in virNetDaemonNew () at rpc/virnetdaemon.c:137
#4  0x555708ec in virLogDaemonNew (privileged=false,
config=0x55820940) at logging/log_daemon.c:163
#5  main (argc=, argv=0x7fffd888) at
logging/log_daemon.c:1069

any idea?

> ---
> >  build-aux/mock-noinline.pl  |  2 +-
> >  src/check-symfile.pl|  2 +-
> >  src/internal.h  | 23 ++-
> >  src/qemu/qemu_capspriv.h|  2 +-
> >  src/rpc/virnetsocket.h  |  4 ++--
> >  src/util/vircommand.h   |  2 +-
> >  src/util/vircrypto.h|  2 +-
> >  src/util/virfile.h  |  2 +-
> >  src/util/virhostcpu.h   |  4 ++--
> >  src/util/virmacaddr.h   |  2 +-
> >  src/util/virnetdev.h|  8 
> >  src/util/virnetdevip.h  |  2 +-
> >  src/util/virnetdevopenvswitch.h |  2 +-
> >  src/util/virnetdevtap.h |  6 +++---
> >  src/util/virnuma.h  | 16 
> >  src/util/virrandom.h|  6 +++---
> >  src/util/virscsi.h  |  2 +-
> >  src/util/virscsivhost.h |  2 +-
> >  src/util/virtpm.h   |  2 +-
> >  src/util/virutil.h  | 10 +-
> >  src/util/viruuid.h  |  2 +-
> >  21 files changed, 58 insertions(+), 45 deletions(-)
>
> ACK
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

-- 
Marc-André Lureau
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] qemu: hotplug virtio_scsi over lsilogic when sicsi controller is not present?

[Liang Yan]

Hi,

We hit some problems when we attached some lun devices in our vm, turns
out that libvirt created lsilogic scsi controller automatically for
these scsi devices, however these device works well under virtio_scsi
controller.

the current code logic is check lsilogic first, if qemu could not
support it, then check virtio_scsi, however is it better to check
virtio_scsi earlier? since it is supported better in qemu level?

I am wondering which solution is better?
1. simple switch sequence and check virtio_scsi first

2. add extra option for "virsh attach-disk" to choose specific
controller type 

Thanks,
Liang



Code part is like below

qemu_hotplug.c  qemuDomainFindOrCreateSCSIDiskController

/* No SCSI controller present, for backward compatibility we
 * now hotplug a controller */
if (VIR_ALLOC(cont) < 0)
return NULL;
cont->type = VIR_DOMAIN_CONTROLLER_TYPE_SCSI;
cont->idx = controller;
cont->model = -1;

VIR_INFO("No SCSI controller present, hotplugging one");
if (qemuDomainAttachControllerDevice(driver, 



qemu_domain_address.c  qemuDomainSetSCSIControllerModel

if (qemuDomainIsPSeries(def)) {
*model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI;
} else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SCSI_LSI)) {
*model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC;
} else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_SCSI)) {
*model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI;
} else {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
   _("Unable to determine model for scsi
controller"));
return -1;


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

Re: [libvirt] [PATCH 03/16] network: Perform some formatting cleanup in bridge_driver

[Pavel Hrdina]

On Fri, May 19, 2017 at 09:03:11AM -0400, John Ferlan wrote:
> Modify code to have two spaces between functions, follow function more
> recent function formatting w/r/t args per line and function return type
> and name on separate lines.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/network/bridge_driver.c | 228 
> +---
>  1 file changed, 174 insertions(+), 54 deletions(-)

Reviewed-by: Pavel Hrdina 


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

Re: [libvirt] [PATCH 01/16] test: Fix up formatting in network test API's

[Pavel Hrdina]

On Fri, May 19, 2017 at 09:03:09AM -0400, John Ferlan wrote:
> Fix some spacing/formatting in the network test driver code.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/test/test_driver.c | 87 
> +-
>  1 file changed, 64 insertions(+), 23 deletions(-)

Reviewed-by: Pavel Hrdina 


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

Re: [libvirt] [PATCH] qemu: Provide non-linux stub for qemuDomainAttachDeviceMknodRecursive

[Daniel P. Berrange]

On Tue, Jul 11, 2017 at 06:42:29PM +0200, Michal Privoznik wrote:
> The way we create devices under /dev is highly linux specific.
> For instance we do mknod(), mount(), umount(), etc. Some
> platforms are even missing some of these functions. Then again,
> as declared in qemuDomainNamespaceAvailable(): namespaces are
> linux only. Therefore, to avoid obfuscating the code by trying to
> make it compile on weird platforms, just provide a non-linux stub
> for qemuDomainAttachDeviceMknodRecursive(). At the same time,
> qemuDomainAttachDeviceMknodHelper() which actually calls the
> non-existent functions is moved under ifdef __linux__ block since
> its only caller is in that block too.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_domain.c | 38 ++
>  1 file changed, 30 insertions(+), 8 deletions(-)

Reviewed-by: Daniel P. Berrange 


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

[libvirt] [PATCH] qemu: Provide non-linux stub for qemuDomainAttachDeviceMknodRecursive

[Michal Privoznik]

The way we create devices under /dev is highly linux specific.
For instance we do mknod(), mount(), umount(), etc. Some
platforms are even missing some of these functions. Then again,
as declared in qemuDomainNamespaceAvailable(): namespaces are
linux only. Therefore, to avoid obfuscating the code by trying to
make it compile on weird platforms, just provide a non-linux stub
for qemuDomainAttachDeviceMknodRecursive(). At the same time,
qemuDomainAttachDeviceMknodHelper() which actually calls the
non-existent functions is moved under ifdef __linux__ block since
its only caller is in that block too.

Signed-off-by: Michal Privoznik 
---
 src/qemu/qemu_domain.c | 38 ++
 1 file changed, 30 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 080ff336e..eb1a9794b 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8541,6 +8541,8 @@ struct qemuDomainAttachDeviceMknodData {
 };
 
 
+/* Our way of creating devices is highly linux specific */
+#if defined(__linux__)
 static int
 qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED,
   void *opaque)
@@ -8638,7 +8640,7 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid 
ATTRIBUTE_UNUSED,
 goto cleanup;
 }
 
-#ifdef WITH_SELINUX
+# ifdef WITH_SELINUX
 if (data->tcon &&
 lsetfilecon_raw(data->file, (VIR_SELINUX_CTX_CONST char *) data->tcon) 
< 0) {
 VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR
@@ -8650,7 +8652,7 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid 
ATTRIBUTE_UNUSED,
 goto cleanup;
 }
 }
-#endif
+# endif
 
 /* Finish mount process started earlier. */
 if (isReg &&
@@ -8661,9 +8663,9 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid 
ATTRIBUTE_UNUSED,
  cleanup:
 if (ret < 0 && delDevice)
 unlink(data->file);
-#ifdef WITH_SELINUX
+# ifdef WITH_SELINUX
 freecon(data->tcon);
-#endif
+# endif
 virFileFreeACLs(>acl);
 return ret;
 }
@@ -8754,14 +8756,14 @@ qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr 
driver,
 goto cleanup;
 }
 
-#ifdef WITH_SELINUX
+# ifdef WITH_SELINUX
 if (lgetfilecon_raw(file, ) < 0 &&
 (errno != ENOTSUP && errno != ENODATA)) {
 virReportSystemError(errno,
  _("Unable to get SELinux label from %s"), file);
 goto cleanup;
 }
-#endif
+# endif
 
 if (STRPREFIX(file, DEVPREFIX)) {
 size_t i;
@@ -8798,9 +8800,9 @@ qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr 
driver,
 
 ret = 0;
  cleanup:
-#ifdef WITH_SELINUX
+# ifdef WITH_SELINUX
 freecon(data.tcon);
-#endif
+# endif
 virFileFreeACLs();
 if (isReg && target)
 umount(target);
@@ -8810,6 +8812,26 @@ qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr 
driver,
 }
 
 
+#else /* !defined(__linux__) */
+
+
+static int
+qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
+ virDomainObjPtr vm ATTRIBUTE_UNUSED,
+ const char *file ATTRIBUTE_UNUSED,
+ char * const *devMountsPath 
ATTRIBUTE_UNUSED,
+ size_t ndevMountsPath ATTRIBUTE_UNUSED,
+ unsigned int ttl ATTRIBUTE_UNUSED)
+{
+virReportSystemError(ENOSYS, "%s",
+ _("Namespaces are not supported on this platform."));
+return -1;
+}
+
+
+#endif /* !defined(__linux__) */
+
+
 static int
 qemuDomainAttachDeviceMknod(virQEMUDriverPtr driver,
 virDomainObjPtr vm,
-- 
2.13.0

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

Re: [libvirt] [PATCH 13/19] storage: Move autostartLink deletion to virstorageobj

[Pavel Hrdina]

On Tue, May 09, 2017 at 11:30:20AM -0400, John Ferlan wrote:
> The virStoragePoolObjDeleteDef already dealt with the configFile - just
> add in the autostartLink as well. If there is no autostartLink defined,
> then no need to attempt unlink - which also allows removal of one errno
> check in moved function.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/conf/virstorageobj.c | 11 +++
>  src/storage/storage_driver.c | 11 ---
>  2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
> index 9ce3840..69ed66d 100644
> --- a/src/conf/virstorageobj.c
> +++ b/src/conf/virstorageobj.c
> @@ -696,6 +696,17 @@ virStoragePoolObjDeleteDef(virStoragePoolObjPtr obj)
> obj->def->name);
>  return -1;
>  }
> +VIR_FREE(obj->configFile);
> +
> +if (!obj->autostartLink)
> +return 0;
> +
> +if (unlink(obj->autostartLink) < 0 && errno != ENOTDIR) {
> +char ebuf[1024];
> +VIR_ERROR(_("Failed to delete autostart link '%s': %s"),
> +  obj->autostartLink, virStrerror(errno, ebuf, 
> sizeof(ebuf)));
> +}
> +VIR_FREE(obj->autostartLink);
>  
>  return 0;
>  }
> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index c4e4e7b..c4650cd 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -841,17 +841,6 @@ storagePoolUndefine(virStoragePoolPtr pool)
>  if (virStoragePoolObjDeleteDef(obj) < 0)
>  goto cleanup;
>  
> -if (unlink(obj->autostartLink) < 0 &&
> -errno != ENOENT &&
> -errno != ENOTDIR) {
> -char ebuf[1024];
> -VIR_ERROR(_("Failed to delete autostart link '%s': %s"),
> -  obj->autostartLink, virStrerror(errno, ebuf, 
> sizeof(ebuf)));
> -}

The same as for the previous patch, keep the unlink in storage driver
and use the new virStoragePoolObjGetAutostartLink().

> -
> -VIR_FREE(obj->configFile);
> -VIR_FREE(obj->autostartLink);
> -

This can be safely removed since it's handled by virStoragePoolObjFree()
which is called in virStoragePoolObjRemove().

>  event = virStoragePoolEventLifecycleNew(obj->def->name,
>  obj->def->uuid,
>  VIR_STORAGE_POOL_EVENT_UNDEFINED,
> -- 
> 2.9.3
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list


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

Re: [libvirt] [PATCH 12/19] storage: Introduce virStoragePoolObj{Get|Set}Autostart

[Pavel Hrdina]

On Tue, May 09, 2017 at 11:30:19AM -0400, John Ferlan wrote:
> Rather than have the logic in the storage driver, move it to virstorageobj.
> 
> NB: virStoragePoolObjGetAutostart can take liberties with not needing the
> same if...else construct.  Also pass a NULL for testStoragePoolSetAutostart
> to avoid the autostartLink setup using the autostartDir for the test driver.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/conf/virstorageobj.c | 57 
> 
>  src/conf/virstorageobj.h |  8 +++
>  src/libvirt_private.syms |  2 ++
>  src/storage/storage_driver.c | 41 +++
>  src/test/test_driver.c   | 13 ++
>  5 files changed, 72 insertions(+), 49 deletions(-)
> 
> diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
> index 0079472..9ce3840 100644
> --- a/src/conf/virstorageobj.c
> +++ b/src/conf/virstorageobj.c
> @@ -111,6 +111,63 @@ virStoragePoolObjSetActive(virStoragePoolObjPtr obj,
>  }
>  
>  
> +int
> +virStoragePoolObjGetAutostart(virStoragePoolObjPtr obj)
> +{
> +if (!obj->configFile)
> +return 0;
> +
> +return obj->autostart;
> +}

The getter is correct.

> +
> +
> +int
> +virStoragePoolObjSetAutostart(virStoragePoolObjPtr obj,
> +  const char *autostartDir,
> +  int autostart)
> +{
> +obj->autostart = autostart;
> +

However, the setter does a lot more than it should be doing.

> +if (!obj->configFile) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   "%s", _("pool has no config file"));
> +return -1;
> +}
> +
> +autostart = (autostart != 0);
> +
> +if (obj->autostart != autostart) {
> +if (autostart && autostartDir) {
> +if (virFileMakePath(autostartDir) < 0) {
> +virReportSystemError(errno,
> + _("cannot create autostart directory 
> %s"),
> + autostartDir);
> +return -1;
> +}

The autostartDir is owned by the storage driver so it is up to the
storage driver to create the directory and create/remove the symlink.

> +
> +if (symlink(obj->configFile, obj->autostartLink) < 0) {
> +virReportSystemError(errno,
> + _("Failed to create symlink '%s' to 
> '%s'"),
> + obj->autostartLink, obj->configFile);
> +return -1;
> +}
> +} else {
> +if (unlink(obj->autostartLink) < 0 &&
> +errno != ENOENT && errno != ENOTDIR) {
> +virReportSystemError(errno,
> + _("Failed to delete symlink '%s'"),
> + obj->autostartLink);
> +return -1;
> +}
> +}
> +
> +obj->autostart = autostart;

This is duplicated the beginning of the function.  The one at the
beginning of the function is wrong, the @autostart should be changed
only if we actually creates the symlink.  I see that you've probably
added it at the beginning of the function to reuse it in the test driver
but that's wrong, it may cause issues for real storage driver.

> +}
> +
> +return 0;
> +}
> +
> +
>  unsigned int
>  virStoragePoolObjGetAsyncjobs(virStoragePoolObjPtr obj)
>  {
> diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h
> index d47b233..3b6e395 100644
> --- a/src/conf/virstorageobj.h
> +++ b/src/conf/virstorageobj.h
> @@ -93,6 +93,14 @@ void
>  virStoragePoolObjSetActive(virStoragePoolObjPtr obj,
> bool active);
>  
> +int
> +virStoragePoolObjGetAutostart(virStoragePoolObjPtr obj);
> +
> +int
> +virStoragePoolObjSetAutostart(virStoragePoolObjPtr obj,
> +  const char *autostartDir,
> +  int autostart);
> +
>  unsigned int
>  virStoragePoolObjGetAsyncjobs(virStoragePoolObjPtr obj);
>  
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index edd3174..e8b4eda 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1014,6 +1014,7 @@ virStoragePoolObjDeleteDef;
>  virStoragePoolObjFindByName;
>  virStoragePoolObjFindByUUID;
>  virStoragePoolObjGetAsyncjobs;
> +virStoragePoolObjGetAutostart;
>  virStoragePoolObjGetConfigFile;
>  virStoragePoolObjGetDef;
>  virStoragePoolObjGetNames;
> @@ -1031,6 +1032,7 @@ virStoragePoolObjNumOfVolumes;
>  virStoragePoolObjRemove;
>  virStoragePoolObjSaveDef;
>  virStoragePoolObjSetActive;
> +virStoragePoolObjSetAutostart;
>  virStoragePoolObjSetConfigFile;
>  virStoragePoolObjSourceFindDuplicate;
>  virStoragePoolObjStealNewDef;
> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index 6289314..c4e4e7b 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ 

Re: [libvirt] [PATCH 4/8] secret: Clean up virSecretObjListAdd processing

[Michal Privoznik]

On 06/03/2017 03:27 PM, John Ferlan wrote:
> Make use of an error: label to handle the failure and need to call
> virSecretObjEndAPI for the object to set it to NULL for return.
> 
> Also rather than an if/else processing - have the initial "if" which
> is just replacing the @newdef into obj->def goto cleanup, thus allowing
> the remaining code to be extracted from the else and appear to more inline.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/conf/virsecretobj.c | 74 
> -
>  1 file changed, 37 insertions(+), 37 deletions(-)
> 
> diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
> index e3bcbe5..1bafd0b 100644
> --- a/src/conf/virsecretobj.c
> +++ b/src/conf/virsecretobj.c
> @@ -333,7 +333,6 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>  {
>  virSecretObjPtr obj;
>  virSecretDefPtr objdef;
> -virSecretObjPtr ret = NULL;
>  char uuidstr[VIR_UUID_STRING_BUFLEN];
>  char *configFile = NULL, *base64File = NULL;
>  
> @@ -354,13 +353,13 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
> _("a secret with UUID %s is already defined for "
>   "use with %s"),
> uuidstr, objdef->usage_id);
> -goto cleanup;
> +goto error;
>  }
>  
>  if (objdef->isprivate && !newdef->isprivate) {
>  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("cannot change private flag on existing 
> secret"));
> -goto cleanup;
> +goto error;
>  }
>  
>  if (oldDef)
> @@ -368,50 +367,51 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>  else
>  virSecretDefFree(objdef);
>  obj->def = newdef;
> -} else {
> -/* No existing secret with same UUID,
> - * try look for matching usage instead */
> -if ((obj = virSecretObjListFindByUsageLocked(secrets,
> - newdef->usage_type,
> - newdef->usage_id))) {
> -virObjectLock(obj);
> -objdef = obj->def;
> -virUUIDFormat(objdef->uuid, uuidstr);
> -virReportError(VIR_ERR_INTERNAL_ERROR,
> -   _("a secret with UUID %s already defined for "
> - "use with %s"),
> -   uuidstr, newdef->usage_id);
> -goto cleanup;
> -}
> +goto cleanup;
> +}
>  
> -/* Generate the possible configFile and base64File strings
> - * using the configDir, uuidstr, and appropriate suffix
> - */
> -if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) ||
> -!(base64File = virFileBuildPath(configDir, uuidstr, ".base64")))
> -goto cleanup;
> +/* No existing secret with same UUID,
> + * try to look for matching usage instead */
> +if ((obj = virSecretObjListFindByUsageLocked(secrets,
> + newdef->usage_type,
> + newdef->usage_id))) {
> +virObjectLock(obj);
> +objdef = obj->def;
> +virUUIDFormat(objdef->uuid, uuidstr);
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("a secret with UUID %s already defined for "
> + "use with %s"),
> +   uuidstr, newdef->usage_id);
> +goto error;
> +}
>  
> -if (!(obj = virSecretObjNew()))
> -goto cleanup;
> +/* Generate the possible configFile and base64File strings
> + * using the configDir, uuidstr, and appropriate suffix
> + */
> +if (!(configFile = virFileBuildPath(configDir, uuidstr, ".xml")) ||
> +!(base64File = virFileBuildPath(configDir, uuidstr, ".base64")))
> +goto cleanup;
>  
> -if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0)
> -goto cleanup;
> +if (!(obj = virSecretObjNew()))
> +goto cleanup;
>  
> -obj->def = newdef;
> -VIR_STEAL_PTR(obj->configFile, configFile);
> -VIR_STEAL_PTR(obj->base64File, base64File);
> -virObjectRef(obj);
> -}
> +if (virHashAddEntry(secrets->objs, uuidstr, obj) < 0)
> +goto error;

Frankly, I find this very confusing. While reading the commit message, I
understand why you sometimes jump to cleanup and other times to error
label. But if I were to just look at the code alone, it's completely
random to me. I think I'd much rather see the pattern that's here.
However, if you really dislike if-else branching (dislike also as in
prepare for upcoming patches), I suggest changing just that.

Michal

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

Re: [libvirt] [PATCH 1/8] secret: Whitespace modification for secret_driver

[Michal Privoznik]

On 06/03/2017 03:27 PM, John Ferlan wrote:
> Ensure two empty lines between functions.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/secret/secret_driver.c | 13 -
>  1 file changed, 12 insertions(+), 1 deletion(-)

ACK

Michal

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

Re: [libvirt] [PATCH 6/8] secret: Have virSecretObjNew consume newdef

[Michal Privoznik]

On 06/03/2017 03:27 PM, John Ferlan wrote:
> Move the consumption of @newdef into virSecretObjNew and then handle that
> in the calling path.  Because on error the calling code expects to free
> @newdef, unset obj->def for the creation failure error paths.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/conf/virsecretobj.c | 14 +-
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c
> index c0bcfab..ca13cf8 100644
> --- a/src/conf/virsecretobj.c
> +++ b/src/conf/virsecretobj.c
> @@ -87,7 +87,7 @@ virSecretObjOnceInit(void)
>  VIR_ONCE_GLOBAL_INIT(virSecretObj)
>  
>  static virSecretObjPtr
> -virSecretObjNew(void)
> +virSecretObjNew(virSecretDefPtr def)
>  {
>  virSecretObjPtr obj;
>  
> @@ -98,6 +98,7 @@ virSecretObjNew(void)
>  return NULL;
>  
>  virObjectLock(obj);
> +obj->def = def;
>  
>  return obj;
>  }
> @@ -384,20 +385,23 @@ virSecretObjListAdd(virSecretObjListPtr secrets,
>  goto error;
>  }
>  
> -if (!(obj = virSecretObjNew()))
> +if (!(obj = virSecretObjNew(newdef)))
>  goto cleanup;
>  
>  /* Generate the possible configFile and base64File strings
>   * using the configDir, uuidstr, and appropriate suffix
>   */
>  if (!(obj->configFile = virFileBuildPath(configDir, uuidstr, ".xml")) ||
> -!(obj->base64File = virFileBuildPath(configDir, uuidstr, ".base64")))
> +!(obj->base64File = virFileBuildPath(configDir, uuidstr, 
> ".base64"))) {
> +obj->def = NULL;
>  goto error;
> +}

I don't quite see the value of this patch, esp. because you have to
manually unset the ->def in each error path.

Michal

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

Re: [libvirt] [PATCH 2/8] secret: Alter FindByUUID to expect the formatted uuidstr

[Michal Privoznik]

On 06/03/2017 03:27 PM, John Ferlan wrote:
> Since we're storing a virUUIDFormat'd string in our Hash Table, let's
> modify the Lookup API to receive a formatted string as well.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/conf/virsecretobj.c| 18 +++---
>  src/conf/virsecretobj.h|  2 +-
>  src/secret/secret_driver.c | 10 +-
>  3 files changed, 13 insertions(+), 17 deletions(-)

ACK

Michal

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

Re: [libvirt] [PATCH 8/8] secret: Handle object list removal and deletion properly

[Michal Privoznik]

On 06/03/2017 03:27 PM, John Ferlan wrote:
> Rather than rely on virSecretObjEndAPI to make the final virObjectUnref
> after the call virSecretObjListRemove, be more explicit by calling
> virObjectUnref and setting @obj to NULL for secretUndefine and in
> the error path of secretDefineXML.
> 
> This also fixes a leak during virSecretLoad if the virSecretLoadValue
> fails the code jumps to cleanup without setting @ret = obj, thus calling
> virSecretObjListRemove which only accounts for the object reference
> related to adding the object to the list during virSecretObjListAdd,
> but does not account for the reference to the object itself as the
> return of @ret would be NULL so the caller wouldn't call virSecretObjEndAPI
> on the object recently added thus reducing the refcnt to zero. Thus
> cleaning up the virSecretLoadValue error path to make it clearer what
> needs to be done on failure.

I think the real reason is that we cannot call virSecretObjEndAPI()
because that *unlocks* the secret object. However, the object is already
unlocked at that point by virSecretObjListRemove() and thus we would
unlock twice while locking just once. Honestly, I'd rather see that
explanation in the commit message. But it's your call.

> 
> Signed-off-by: John Ferlan 
> ---
>  src/conf/virsecretobj.c| 14 ++
>  src/secret/secret_driver.c |  9 +++--
>  2 files changed, 13 insertions(+), 10 deletions(-)

ACK

Michal

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

Re: [libvirt] [PATCH 3/8] secret: Rename variable in virSecretObjListAdd

[Michal Privoznik]

On 06/03/2017 03:27 PM, John Ferlan wrote:
> Rename @def to @objdef - it'll make future patches easier.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/conf/virsecretobj.c | 18 +-
>  1 file changed, 9 insertions(+), 9 deletions(-)

ACK

Michal

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

Re: [libvirt] [PATCH 7/8] secret: Properly handle @def after virSecretObjAdd in driver

[Michal Privoznik]

On 06/03/2017 03:27 PM, John Ferlan wrote:
> Since the virSecretObjListAdd technically consumes @def on success,
> the secretDefineXML should fetch the @def from the object afterwards
> and manage as an @objdef and set @def = NULL immediately.

Really? virSecretObjListAdd sets ->def pointer in the object, but it
doesn't touch the definition otherwise. So I think a caller is safe to
continue using the pointer.

Michal

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

Re: [libvirt] [PATCH 5/8] secret: Remove need for local configFile and base64File in ObjectAdd

[Michal Privoznik]

On 06/03/2017 03:27 PM, John Ferlan wrote:
> Rather than assign to a local variable, let's just assign directly to the
> object using the error path for cleanup.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/conf/virsecretobj.c | 17 ++---
>  1 file changed, 6 insertions(+), 11 deletions(-)

ACK

Michal

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

[libvirt] [PATCH 0/5] qemu: Modernize storage file access for block copy

[Peter Krempa]

Refactor access of the block copy target to use the storage driver APIs.

Peter Krempa (5):
  qemu: driver: Split out access to VIR_DOMAIN_BLOCK_COPY_REUSE_EXT
  qemu: blockcopy: Explicitly assert 'reuse' for block devices
  qemu: blockcopy: reuse storage driver APIs to pre-create copy target
  qemu: blockcopy: Split out checking of the target image file
  qemu: blockcopy: Refactor logic checking the target storage file

 src/qemu/qemu_driver.c | 120 -
 1 file changed, 79 insertions(+), 41 deletions(-)

-- 
2.12.2

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

[libvirt] [PATCH 4/5] qemu: blockcopy: Split out checking of the target image file

[Peter Krempa]

Move the code into a separate function so that the flow of creating the
copy is more obvious and split into logical pieces.
---
 src/qemu/qemu_driver.c | 79 +-
 1 file changed, 46 insertions(+), 33 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 48dc5e5cc..5beb14e64 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16685,6 +16685,50 @@ qemuDomainBlockJobSetSpeed(virDomainPtr dom,
 }


+static int
+qemuDomainBlockCopyValidateMirror(virStorageSourcePtr mirror,
+  const char *dst,
+  bool *reuse)
+{
+int desttype = virStorageSourceGetActualType(mirror);
+struct stat st;
+
+if (stat(mirror->path, ) < 0) {
+if (errno != ENOENT) {
+virReportSystemError(errno, _("unable to stat for disk %s: %s"),
+ dst, mirror->path);
+return -1;
+} else if (*reuse || desttype == VIR_STORAGE_TYPE_BLOCK) {
+virReportSystemError(errno,
+ _("missing destination file for disk %s: %s"),
+ dst, mirror->path);
+return -1;
+}
+} else if (!S_ISBLK(st.st_mode)) {
+if (st.st_size && !(*reuse)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("external destination file for disk %s already "
+ "exists and is not a block device: %s"),
+   dst, mirror->path);
+return -1;
+}
+if (desttype == VIR_STORAGE_TYPE_BLOCK) {
+virReportError(VIR_ERR_INVALID_ARG,
+   _("blockdev flag requested for disk %s, but file "
+ "'%s' is not a block device"),
+   dst, mirror->path);
+return -1;
+}
+} else {
+/* if the target is a block device, assume that we are reusing it, so
+ * there are no attempts to create it */
+*reuse = true;
+}
+
+return 0;
+}
+
+
 /* bandwidth in bytes/s.  Caller must lock vm beforehand, and not
  * access mirror afterwards.  */
 static int
@@ -16703,11 +16747,9 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
 char *device = NULL;
 virDomainDiskDefPtr disk = NULL;
 int ret = -1;
-struct stat st;
 bool need_unlink = false;
 virQEMUDriverConfigPtr cfg = NULL;
 const char *format = NULL;
-int desttype = virStorageSourceGetActualType(mirror);
 virErrorPtr monitor_error = NULL;
 bool reuse = !!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT);

@@ -16790,37 +16832,8 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
 if (qemuDomainStorageFileInit(driver, vm, mirror) < 0)
 goto endjob;

-if (stat(mirror->path, ) < 0) {
-if (errno != ENOENT) {
-virReportSystemError(errno, _("unable to stat for disk %s: %s"),
- disk->dst, mirror->path);
-goto endjob;
-} else if (reuse || desttype == VIR_STORAGE_TYPE_BLOCK) {
-virReportSystemError(errno,
- _("missing destination file for disk %s: %s"),
- disk->dst, mirror->path);
-goto endjob;
-}
-} else if (!S_ISBLK(st.st_mode)) {
-if (st.st_size && !reuse) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("external destination file for disk %s already "
- "exists and is not a block device: %s"),
-   disk->dst, mirror->path);
-goto endjob;
-}
-if (desttype == VIR_STORAGE_TYPE_BLOCK) {
-virReportError(VIR_ERR_INVALID_ARG,
-   _("blockdev flag requested for disk %s, but file "
- "'%s' is not a block device"),
-   disk->dst, mirror->path);
-goto endjob;
-}
-} else {
-/* if the target is a block device, assume that we are reusing it, so
- * there are no attempts to create it */
-reuse = true;
-}
+if (qemuDomainBlockCopyValidateMirror(mirror, disk->dst, ) < 0)
+goto endjob;

 if (!mirror->format) {
 if (!reuse) {
-- 
2.12.2

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

[libvirt] [PATCH 1/5] qemu: driver: Split out access to VIR_DOMAIN_BLOCK_COPY_REUSE_EXT

[Peter Krempa]

Extract the presence of the flag into a boolean to simplify conditions
and allow further manipulation of the state of the flag.
---
 src/qemu/qemu_driver.c | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a7019c53c..d03a9dbc3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16709,6 +16709,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
 const char *format = NULL;
 int desttype = virStorageSourceGetActualType(mirror);
 virErrorPtr monitor_error = NULL;
+bool reuse = !!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT);

 /* Preliminaries: find the disk we are editing, sanity checks */
 virCheckFlags(VIR_DOMAIN_BLOCK_COPY_SHALLOW |
@@ -16769,8 +16770,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,

 /* unless the user provides a pre-created file, shallow copy into a raw
  * file is not possible */
-if ((flags & VIR_DOMAIN_BLOCK_COPY_SHALLOW) &&
-!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT) &&
+if ((flags & VIR_DOMAIN_BLOCK_COPY_SHALLOW) && !reuse &&
 mirror->format == VIR_STORAGE_FILE_RAW) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("shallow copy of disk '%s' into a raw file "
@@ -16791,15 +16791,14 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
 virReportSystemError(errno, _("unable to stat for disk %s: %s"),
  disk->dst, mirror->path);
 goto endjob;
-} else if (flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT ||
-   desttype == VIR_STORAGE_TYPE_BLOCK) {
+} else if (reuse || desttype == VIR_STORAGE_TYPE_BLOCK) {
 virReportSystemError(errno,
  _("missing destination file for disk %s: %s"),
  disk->dst, mirror->path);
 goto endjob;
 }
 } else if (!S_ISBLK(st.st_mode)) {
-if (st.st_size && !(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT)) {
+if (st.st_size && !reuse) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("external destination file for disk %s already "
  "exists and is not a block device: %s"),
@@ -16816,7 +16815,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
 }

 if (!mirror->format) {
-if (!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT)) {
+if (!reuse) {
 mirror->format = disk->src->format;
 } else {
 /* If the user passed the REUSE_EXT flag, then either they
@@ -16829,7 +16828,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
 }

 /* pre-create the image file */
-if (!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT)) {
+if (!reuse) {
 int fd = qemuOpenFile(driver, vm, mirror->path,
   O_WRONLY | O_TRUNC | O_CREAT,
   _unlink, NULL);
-- 
2.12.2

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

[libvirt] [PATCH 2/5] qemu: blockcopy: Explicitly assert 'reuse' for block devices

[Peter Krempa]

When copying to a block device, the block device will already exist. To
allow users using a block device without any preparation, they need to
use the block copy without VIR_DOMAIN_BLOCK_COPY_REUSE_EXT.

This means that if the target is an existing block device we don't need
to prepare it, but we can't reject it as being existing.

To avoid breaking this feature, explicitly assume that existing block
devices will be reused even without that flag explicitly specified,
while skipping attempts to create it.

qemuMonitorDriveMirror still needs to honor the flag as specified by the
user, since qemu overwrites the metadata otherwise.
---
 src/qemu/qemu_driver.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d03a9dbc3..d00166f23 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16812,6 +16812,10 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
disk->dst, mirror->path);
 goto endjob;
 }
+} else {
+/* if the target is a block device, assume that we are reusing it, so
+ * there are no attempts to create it */
+reuse = true;
 }

 if (!mirror->format) {
@@ -16851,6 +16855,8 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,

 /* Actually start the mirroring */
 qemuDomainObjEnterMonitor(driver, vm);
+/* qemuMonitorDriveMirror needs to honor the REUSE_EXT flag as specified
+ * by the user regardless of how @reuse was modified */
 ret = qemuMonitorDriveMirror(priv->mon, device, mirror->path, format,
  bandwidth, granularity, buf_size, flags);
 virDomainAuditDisk(vm, NULL, mirror, "mirror", ret >= 0);
-- 
2.12.2

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

[libvirt] [PATCH 5/5] qemu: blockcopy: Refactor logic checking the target storage file

[Peter Krempa]

Use virStorageSource accessors to check the file and call
virStorageFileAccess before even attempting to stat the target. This
will be helpful once we try to add network destinations for block copy,
since there will be no need to stat them.
---
 src/qemu/qemu_driver.c | 53 +++---
 1 file changed, 33 insertions(+), 20 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5beb14e64..f93c5cb21 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16693,36 +16693,49 @@ qemuDomainBlockCopyValidateMirror(virStorageSourcePtr 
mirror,
 int desttype = virStorageSourceGetActualType(mirror);
 struct stat st;

-if (stat(mirror->path, ) < 0) {
+if (virStorageFileAccess(mirror, F_OK) < 0) {
 if (errno != ENOENT) {
-virReportSystemError(errno, _("unable to stat for disk %s: %s"),
- dst, mirror->path);
+virReportSystemError(errno, "%s",
+ _("unable to verify existance of "
+   "block copy target"));
 return -1;
-} else if (*reuse || desttype == VIR_STORAGE_TYPE_BLOCK) {
+}
+
+if (*reuse || desttype == VIR_STORAGE_TYPE_BLOCK) {
 virReportSystemError(errno,
  _("missing destination file for disk %s: %s"),
  dst, mirror->path);
 return -1;
 }
-} else if (!S_ISBLK(st.st_mode)) {
-if (st.st_size && !(*reuse)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("external destination file for disk %s already "
- "exists and is not a block device: %s"),
-   dst, mirror->path);
+} else {
+if (virStorageFileStat(mirror, ) < 0) {
+virReportSystemError(errno,
+ _("unable to stat block copy target '%s'"),
+ mirror->path);
 return -1;
 }
-if (desttype == VIR_STORAGE_TYPE_BLOCK) {
-virReportError(VIR_ERR_INVALID_ARG,
-   _("blockdev flag requested for disk %s, but file "
- "'%s' is not a block device"),
-   dst, mirror->path);
-return -1;
+
+if (S_ISBLK(st.st_mode)) {
+/* if the target is a block device, assume that we are reusing it,
+ * so there are no attempts to create it */
+*reuse = true;
+} else {
+if (st.st_size && !(*reuse)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("external destination file for disk %s 
already "
+ "exists and is not a block device: %s"),
+   dst, mirror->path);
+return -1;
+}
+
+if (desttype == VIR_STORAGE_TYPE_BLOCK) {
+virReportError(VIR_ERR_INVALID_ARG,
+   _("blockdev flag requested for disk %s, but 
file "
+ "'%s' is not a block device"),
+   dst, mirror->path);
+return -1;
+}
 }
-} else {
-/* if the target is a block device, assume that we are reusing it, so
- * there are no attempts to create it */
-*reuse = true;
 }

 return 0;
-- 
2.12.2

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

[libvirt] [PATCH 3/5] qemu: blockcopy: reuse storage driver APIs to pre-create copy target

[Peter Krempa]

Rather than using the local-file only implementation 'qemuOpenFile'
switch to the imagelabel aware storage driver implementation.
---
 src/qemu/qemu_driver.c | 21 ++---
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d00166f23..48dc5e5cc 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16786,6 +16786,10 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
_("non-file destination not supported yet"));
 goto endjob;
 }
+
+if (qemuDomainStorageFileInit(driver, vm, mirror) < 0)
+goto endjob;
+
 if (stat(mirror->path, ) < 0) {
 if (errno != ENOENT) {
 virReportSystemError(errno, _("unable to stat for disk %s: %s"),
@@ -16833,12 +16837,12 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,

 /* pre-create the image file */
 if (!reuse) {
-int fd = qemuOpenFile(driver, vm, mirror->path,
-  O_WRONLY | O_TRUNC | O_CREAT,
-  _unlink, NULL);
-if (fd < 0)
+if (virStorageFileCreate(mirror) < 0) {
+virReportSystemError(errno, "%s", _("failed to create copy 
target"));
 goto endjob;
-VIR_FORCE_CLOSE(fd);
+}
+
+need_unlink = true;
 }

 if (mirror->format > 0)
@@ -16870,6 +16874,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,

 /* Update vm in place to match changes.  */
 need_unlink = false;
+virStorageFileDeinit(mirror);
 disk->mirror = mirror;
 mirror = NULL;
 disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_COPY;
@@ -16880,8 +16885,10 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
  vm->def->name);

  endjob:
-if (need_unlink && unlink(mirror->path))
-VIR_WARN("unable to unlink just-created %s", mirror->path);
+if (need_unlink && virStorageFileUnlink(mirror))
+VIR_WARN("%s", _("unable to remove just-created copy target"));
+virStorageFileDeinit(mirror);
+virStorageSourceFree(mirror);
 qemuDomainObjEndJob(driver, vm);
 if (monitor_error) {
 virSetError(monitor_error);
-- 
2.12.2

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

Re: [libvirt] [PATCH 11/19] storage: Introduce virStoragePoolObjNew

[Pavel Hrdina]

On Tue, May 09, 2017 at 11:30:18AM -0400, John Ferlan wrote:
> Create/use a helper to perform object allocation.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/conf/virstorageobj.c | 34 +++---
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
> index 7d6b311..0079472 100644
> --- a/src/conf/virstorageobj.c
> +++ b/src/conf/virstorageobj.c
> @@ -37,6 +37,27 @@
>  VIR_LOG_INIT("conf.virstorageobj");
>  
>  
> +static virStoragePoolObjPtr
> +virStoragePoolObjNew(virStoragePoolDefPtr def)
> +{
> +virStoragePoolObjPtr obj;
> +
> +if (VIR_ALLOC(obj) < 0)
> +return NULL;
> +
> +if (virMutexInit(>lock) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("cannot initialize mutex"));
> +VIR_FREE(obj);
> +return NULL;
> +}
> +virStoragePoolObjLock(obj);
> +obj->active = 0;
> +obj->def = def;
> +return obj;
> +}
> +
> +
>  virStoragePoolDefPtr
>  virStoragePoolObjGetDef(virStoragePoolObjPtr obj)
>  {
> @@ -386,24 +407,15 @@ virStoragePoolObjAssignDef(virStoragePoolObjListPtr 
> pools,
>  return obj;
>  }
>  
> -if (VIR_ALLOC(obj) < 0)
> +if (!(obj = virStoragePoolObjNew(def)))
>  return NULL;

The new virStoragePoolObjNew() doesn't have to take @def and ...

>  
> -if (virMutexInit(>lock) < 0) {
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -   _("cannot initialize mutex"));
> -VIR_FREE(obj);
> -return NULL;
> -}
> -virStoragePoolObjLock(obj);
> -obj->active = 0;
> -
>  if (VIR_APPEND_ELEMENT_COPY(pools->objs, pools->count, obj) < 0) {
> +obj->def = NULL;

... need to set the @obj->def to NULL and ...

>  virStoragePoolObjUnlock(obj);
>  virStoragePoolObjFree(obj);
>  return NULL;
>  }
> -obj->def = def;

... just keep this line as it is.

>  
>  return obj;
>  }
> -- 
> 2.9.3
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list


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

Re: [libvirt] [PATCH 4/6] storage: Make virStorageFileReadHeader more universal

[Peter Krempa]

On Sat, Jul 01, 2017 at 10:43:09 -0400, John Ferlan wrote:
> 
> 
> On 06/23/2017 09:33 AM, Peter Krempa wrote:
> > Allow specifying offset to read an arbitrary position in the file. This
> > warrants a rename to virStorageFileRead.
> > ---
> >  src/qemu/qemu_driver.c|  3 +--
> >  src/storage/storage_backend.h |  9 +
> >  src/storage/storage_backend_fs.c  | 20 +--
> >  src/storage/storage_backend_gluster.c | 37 
> > ++-
> >  src/storage/storage_source.c  | 30 +++-
> >  src/storage/storage_source.h  |  7 ---
> >  6 files changed, 63 insertions(+), 43 deletions(-)
> > 
> 
> [...]
> 
> > diff --git a/src/storage/storage_backend_gluster.c 
> > b/src/storage/storage_backend_gluster.c
> > index 93dce4042..8ea7e603c 100644
> > --- a/src/storage/storage_backend_gluster.c
> > +++ b/src/storage/storage_backend_gluster.c

[...]

> > @@ -292,7 +292,7 @@ 
> > virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state,
> >  goto cleanup;
> >  }
> > 
> 
> Should :
> 
> ssize_t len = VIR_STORAGE_MAX_HEADER;
> 
> change to size_t and a new variable "ssize_t read_len" be created and
> used for the following and subsequent virStorageFileGetMetadataFromBuf
> call?  (although that also takes a size_t for the 3rd param).

I actually just used VIR_STORAGE_MAX_HEADER constant directly in the
call of virStorageBackendGlusterRead. The original variable will thus be
used only for the return value, which is signed.


> 
> > -if ((len = virStorageBackendGlusterReadHeader(fd, name, len, )) 
> > < 0)
> > +if ((len = virStorageBackendGlusterRead(fd, name, len, )) < 0)
> >  goto cleanup;
> > 
> >  if (!(meta = virStorageFileGetMetadataFromBuf(name, header, len,


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

Re: [libvirt] [Qemu-devel] change x86 default machine type to Q35?

[Eduardo Habkost]

On Tue, Jul 11, 2017 at 10:13:05AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > Concerning QEMU, could we maybe simply emit a warning a la
> > 
> >  "you did not specify a machine type with the -M option, so you are
> >   currently running the the 'pc' machine type. Please note that
> > future
> >   versions of QEMU might use the 'q35' machine type instead. If you
> >   require the 'pc' machine type for your setting, then please specify
> >   it with the -M option."
> 
> Warnings tend to get ignored until things are actually break, so I
> don't think this helps much.  I think simply not having a default
> machine type (as already suggested elsewhere in this thread) is the
> best way to deal with this.  That way we don't silently change
> behavior.  It also is in line with what we have on arm where we already
> require the user to explicitly pick a machine type.

If we do that, we probably should wait for libvirt to adapt and
choose its own default.  Current libvirt would pick an arbitrary
machine-type (the first one in the query-machines list) as the
default.

-- 
Eduardo

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

Re: [libvirt] [Qemu-devel] change x86 default machine type to Q35?

[Paolo Bonzini]

On 11/07/2017 16:42, Kevin Wolf wrote:
>>>  Concerning QEMU, could we maybe simply emit a warning a la
>>>
>>>  "you did not specify a machine type with the -M option, so you are
>>>   currently running the the 'pc' machine type. Please note that
>>> future
>>>   versions of QEMU might use the 'q35' machine type instead. If you
>>>   require the 'pc' machine type for your setting, then please specify
>>>   it with the -M option."
>> Warnings tend to get ignored until things are actually break, so I
>> don't think this helps much.  I think simply not having a default
>> machine type (as already suggested elsewhere in this thread) is the
>> best way to deal with this.
> I would absolutely hate this. One of the nice things about qemu has
> always been that 'qemu disk.img' is enough to start a simple VM. You
> only need to touch any other options for things you care about. I
> wouldn't want to give this up.

I agree.  Don't change anything, leave "-M pc" aside, and let libosinfo
pick q35 for newer guests.

Paolo

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

Re: [libvirt] [PATCH 3/6] storage: Split out virStorageSource accessors to separate file

[Peter Krempa]

On Sat, Jul 01, 2017 at 10:42:56 -0400, John Ferlan wrote:
> 
> 
> On 06/23/2017 09:33 AM, Peter Krempa wrote:
> > The helper methods for actually accessing the storage objects don't
> > really belong to the main storage driver implementation file. Split them
> > out.
> > ---
> >  po/POTFILES.in|   1 +
> >  src/Makefile.am   |   1 +
> >  src/qemu/qemu_domain.c|   1 +
> >  src/qemu/qemu_driver.c|   1 +
> >  src/security/virt-aa-helper.c |   2 +-
> >  src/storage/storage_driver.c  | 551 +--
> >  src/storage/storage_driver.h  |  28 --
> >  src/storage/storage_source.c  | 585 
> > ++
> >  src/storage/storage_source.h  |  53 
> >  tests/virstoragetest.c|   1 +
> >  10 files changed, 645 insertions(+), 579 deletions(-)
> >  create mode 100644 src/storage/storage_source.c
> >  create mode 100644 src/storage/storage_source.h
> > 
> 
> Since all of the helpers being moved are prefixed with virStorageFile
> why not "storage_file.{c,h}"? I realize the helpers are all operating on
> virStorageSourcePtr.  Is it perhaps because being too close to
> virstoragefile.{c,h}?

The thing is that the virStorageSource does not necessarily need to be a
file so I'd eventually like to rename those helpers, so I'll stick with
this filename for now.


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

Re: [libvirt] [Qemu-devel] change x86 default machine type to Q35?

[Kevin Wolf]

Am 11.07.2017 um 10:13 hat Gerd Hoffmann geschrieben:
>   Hi,
> 
> > Concerning QEMU, could we maybe simply emit a warning a la
> > 
> >  "you did not specify a machine type with the -M option, so you are
> >   currently running the the 'pc' machine type. Please note that
> > future
> >   versions of QEMU might use the 'q35' machine type instead. If you
> >   require the 'pc' machine type for your setting, then please specify
> >   it with the -M option."
> 
> Warnings tend to get ignored until things are actually break, so I
> don't think this helps much.  I think simply not having a default
> machine type (as already suggested elsewhere in this thread) is the
> best way to deal with this.

I would absolutely hate this. One of the nice things about qemu has
always been that 'qemu disk.img' is enough to start a simple VM. You
only need to touch any other options for things you care about. I
wouldn't want to give this up.

Kevin

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

Re: [libvirt] [PATCH v4 12/14] nodedev: Convert virNodeDeviceObj to use virObjectLockable

[Erik Skultety]

On Mon, Jul 03, 2017 at 05:25:28PM -0400, John Ferlan wrote:
> Now that we have a bit more control, let's convert our object into
> a lockable object and let that magic handle the create and lock/unlock.
>
> This also involves creating a virNodeDeviceEndAPI in order to handle
> the object cleaup for API's that use the Add or Find API's in order

s/cleaup/cleanup


[...]
>
>   cleanup:
> -virNodeDeviceObjUnlock(obj);
> +virNodeDeviceObjEndAPI();
>  if (ret == -1) {
>  --ncaps;
>  while (--ncaps >= 0)
> @@ -613,8 +613,7 @@ nodeDeviceDestroy(virNodeDevicePtr device)
>   * to be taken, so grab the object def which will have the various
>   * fields used to search (name, parent, parent_wwnn, parent_wwpn,
>   * or parent_fabric_wwn) and drop the object lock. */

^This commentary got me thinking. There's a deadlock because of how the locks
are acquired earlier in this function. Patch 14 solves the deadlock in exchange
for a race, so see my comment in patch 14.

[...]
>   cleanup:
> -if (obj)
> -virNodeDeviceObjUnlock(obj);
> +virNodeDeviceObjEndAPI();
>  testDriverUnlock(driver);
>  virNodeDeviceDefFree(def);
>  virObjectUnref(dev);
> @@ -5596,13 +5595,13 @@ testNodeDeviceDestroy(virNodeDevicePtr dev)
>   * taken, so we have to dup the parent's name and drop the lock
>   * before calling it.  We don't need the reference to the object
>   * any more once we have the parent's name.  */

Not that this patch would be touching it directly, but ^this commentary is
pretty much useless now, since @parent_name is useless in this function,
nodeDeviceDestroy doesn't work that way anymore.

Erik

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

Re: [libvirt] [PATCH v4 14/14] nodedev: Remove driver locks around object list mgmt code

[Erik Skultety]

On Mon, Jul 03, 2017 at 05:25:30PM -0400, John Ferlan wrote:
> Since virnodedeviceobj now has a self-lockable hash table, there's no
> need to lock the table from the driver for processing. Thus remove the
> locks from the driver for NodeDeviceObjList mgmt.
>
> Signed-off-by: John Ferlan 

[..]

> @@ -601,8 +565,6 @@ nodeDeviceDestroy(virNodeDevicePtr device)
>  return -1;
>  def = virNodeDeviceObjGetDef(obj);
>
> -nodeDeviceLock();
> -

Consider the following scenario handling the same device:

Thread 1 (udev event handler callback) | Thread 2 (nodeDeviceDestroy)
===|=
   | # attempt to destroy a device
   | obj = nodeDeviceObjFindByName()
   |
   | # @obj is locked
   | def = virNodeDeviceObjGetDef(obj)
   |
   | virNodeDeviceObjEndAPI()
   | # @obj is unlocked
  <--
# change event |
udevAddOneDevice() |
   |
  obj = virNodeDeviceObjListFindByName |
  # @obj locked|
  new_device = false   |
  # @obj unlocked  |
   |
  obj = virNodeDeviceObjListAssignDef  |
  # @obj locked|
  virNodeDeviceObjEndAPI() |
  # @obj unlocked and @def changed |
  -->
   | virNodeDeviceObjListGetParentHost()
   |if (def->parent) ===> SIGSEGV

In patch 12/14 this would have been a deadlock because you first locked the
@obj, then nodedev driver while udevAddOneDevice did in the reverse order. I
don't know about NPIV so I'm not sure whether there is another way of removing
a device and updating the parent device tree, might require an updated model,
but for now, you need to make a deep copy of @def. I can see that the chance of
getting an 'change' event from udev on a vHBA device is low, but I'm trying to
look at it from a higher perspective, as we want to be able to remove mdevs
this way, possibly other devices in the future.

The rest of the patch looks okay, but I want to try it first.

Erik

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

[libvirt] [PATCH] internal: don't use weak symbols for Win32 platform

[Daniel P. Berrange]

The Win32 platform will fail to link if you use weak symbols
because it is incompatible with exporting symbols in a DLL:

Cannot export virRandomGenerateWWN: symbol wrong type (2 vs 3)

We only need weak symbols for our test suite to do LD_PRELOAD
and this doesn't work on Win32, so we can just drop the hack
for Win32

Signed-off-by: Daniel P. Berrange 
---

Pushed as a build fix for Win32

 src/internal.h | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/src/internal.h b/src/internal.h
index 00edd4f..edc3587 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -128,10 +128,14 @@
  *
  */
 # ifndef ATTRIBUTE_MOCKABLE
-#  if __GNUC_PREREQ(4, 5)
-#   define ATTRIBUTE_MOCKABLE __attribute__((__noinline__, __noclone__, 
__weak__))
+#  if defined(WIN32)
+#   define ATTRIBUTE_MOCKABLE
 #  else
-#   define ATTRIBUTE_MOCKABLE __attribute__((__noinline__, __weak__))
+#   if __GNUC_PREREQ(4, 5)
+#define ATTRIBUTE_MOCKABLE __attribute__((__noinline__, __noclone__, 
__weak__))
+#   else
+#define ATTRIBUTE_MOCKABLE __attribute__((__noinline__, __weak__))
+#   endif
 #  endif
 # endif
 
-- 
2.9.4

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

[libvirt] Xen device section defaults miss name='qemu'

[Christian Ehrhardt]

Hi,
we have found an issue caused by 321a28c6 "libxl: set default disk format
in device post-parse" (and maybe more changes I couldn't identify yet).

TL;DR: The auto-added driver section in xen changed and now causes issues.

An example to to trigger:
1. Create a  XEN xml with a disk device of type cdrom, but do not add a
driver section, like.

  
  

2. on virsh define if that XML
2b. You could also "virsh edit" any working xml file, remove the
 which will trigger the same
2c. You could also define via virt-manager as it does not explicitly
specify the device section

What happens is that before the changes this auto-added a driver section
like:
   
But now it does only add
   
Which fails to verify like:


Interestingly the same is not true for KVM, there the section is as it was
in the past which made it more likely the post-parse step might be involved.

Also rather expected, if one adds a full 
in the XML libvirt doesn't have to provide (broken) defaults and things
work as they should - yet from a user with formerly working XMLs or using
virt-manager this is a regression.

I have beg your pardon I seem to not be experienced enough on this part of
libvirt to provide a valid fix - I tried but nothing good came out so far.
For now I reverted 321a28c6 which also affects a test later added by
4cd3f241. With those changes it will not add anything which lets us start
Xen VMs for now, but it clearly isn't a solution.

I wanted to ask you if you could take a look into this?

-- 
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: handle missing bind host/service on chardev hotplug

[Ján Tomko]

On Mon, Jun 12, 2017 at 07:08:50AM -0400, John Ferlan wrote:

Also, it seems connectHost could be NULL too, how does that affect the
qemuMonitorJSONBuildInetSocketAddress a few lines earlier?

FWIW: Questions are from reading the qemuBuildChrChardevStr /
qemuBuildChrArgStr code and the libxl_conf.c implementation...



Missing connectHost is not addressed by this patch.
It seems QEMU can automatically fill in "localhost", when the host
is missing, but currently this does not work on a fresh domain start
etiher:

https://bugzilla.redhat.com/show_bug.cgi?id=1454671

Jan


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

Re: [libvirt] [PATCH] tests: add further XML namespace test

[Andrea Bolognani]

On Tue, 2017-07-11 at 14:00 +0100, Daniel P. Berrange wrote:
> > You can get rid of  and  too, then add
> >  and
> >  to avoid unnecessary devices
> > popping up in the output file.
> 
> FYI, this XML file is identical to the pre-existing qemuxml2argv-qemu-ns.xml
> file, so I'm inclined to leave it as is

I'm aware of that, that's why I said that my R-b would stand
whether or not you decide to address these points.

That said, I'd really prefer it if we started being more
to the point with our test data and really only include the
relevant information rather than copy the same pointless
bits over and over again.

Here's a counter-offer: if you agree to follow my advice
above and use a minimal input file, I'll post a patch that
strips the other qemu-ns*.xml from unnecessary cruft. Does
that sound fair? :)

> > > +++ b/tests/qemuxml2argvtest.c
> > > @@ -1505,6 +1505,7 @@ mymain(void)
> > >   
> > >   DO_TEST("qemu-ns", NONE);
> > >   DO_TEST("qemu-ns-no-env", NONE);
> > > +DO_TEST("qemu-ns-alt", NONE);
> > 
> > You'll want to add this to qemuxml2xmltest too.
> 
> This wont round trip - it'll end up outputing the canonical xmlns syntax
> instead, for which we already have output tests.

Hence proving that the alternative syntax and the canonical
one result in the same output file. Why wouldn't we want to
test that?

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 09/19] storage: Alter volume num, name, and export API's to just take obj

[Pavel Hrdina]

On Tue, May 09, 2017 at 11:30:16AM -0400, John Ferlan wrote:
> Alter the virStoragePoolObjNumOfVolumes, virStoragePoolObjVolumeGetNames,
> and virStoragePoolObjVolumeListExport APIs to take a virStoragePoolObjPtr
> instead of the >volumes and obj->def.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/conf/virstorageobj.c | 15 +--
>  src/conf/virstorageobj.h |  9 +++--
>  src/storage/storage_driver.c |  7 +++
>  src/test/test_driver.c   |  9 +++--
>  4 files changed, 18 insertions(+), 22 deletions(-)

Reviewed-by: Pavel Hrdina 


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

Re: [libvirt] [PATCH] qemu: Fix qemuDomainGetBlockInfo allocation value setting

[Michal Privoznik]

On 07/06/2017 10:02 PM, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1467826
> 
> Commit id 'b9b1aa639' was supposed to add logic to set the allocation
> for sparse files when wr_highest_offset was zero; however, an unconditional
> setting was done just prior. For block devices, this means allocation is
> always returning 0 since 'actual-size' will be zero.
> 
> Remove the unconditional setting and add the note about it being possible
> to still be zero for block devices. As soon as the guest starts writing to
> the volume, the allocation value will then be obtainable from qemu via
> the wr_highest_offset.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/qemu/qemu_driver.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

ACK

Michal

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

Re: [libvirt] [PATCH 5/6] qemu: Use storage driver APIs in qemuDomainBlockPeek

[Peter Krempa]

On Sat, Jul 01, 2017 at 10:43:23 -0400, John Ferlan wrote:
> 
> 
> On 06/23/2017 09:33 AM, Peter Krempa wrote:
> > Refactor the access to storage driver usage along with
> > qemuDomainStorageFileInit which ensures that we access the file with
> > correct DAC uid/gid.
> > ---
> >  src/qemu/qemu_driver.c | 25 -
> >  1 file changed, 8 insertions(+), 17 deletions(-)
> > 
> 
> Irony is the comment at the end of qemuOpenFile:
> 
>  * This function should not be used on storage sources. Use
>  * qemuDomainStorageFileInit and storage driver APIs if possible.

This message was added by:

commit f7105d0e4a485bf7d9e878fd17e675d7f9d29f9f
Author: Peter Krempa 
Date:   Wed May 10 12:28:38 2017 +0200

the usage of qemuOpenFile (prior to that, open() was used directly) was
added in:

commit b4a40dd92dc7e6f110b13f2353cb5343d1147227
Author: Martin Kletzander 
Date:   Fri May 24 18:26:26 2013 +0200

I was just lazy and did not bother fixing all places right away and
added the warning that it shouldn't be reused more.


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

Re: [libvirt] [PATCH] tests: add further XML namespace test

[Daniel P. Berrange]

On Mon, Jul 10, 2017 at 12:52:32PM +0200, Andrea Bolognani wrote:
> On Mon, 2017-07-10 at 11:14 +0100, Daniel P. Berrange wrote:
> > +++ b/tests/qemuxml2argvdata/qemuxml2argv-qemu-ns-alt.xml
> > @@ -0,0 +1,30 @@
> > +
> > +  QEMUGuest1
> > +  c7a5fdbd-edaf-9455-926a-d65c16db1809
> > +  219136
> > +  219136
> 
>  can be left out.
> 
> > +  1
> > +  
> > +hvm
> > +
> 
>  is unnecessary.
> 
> > +  
> > +  
> > +  destroy
> > +  restart
> > +  destroy
> 
>  and  can be removed as well.
> 
> > +  
> > +/usr/bin/qemu-system-i686
> > +
> > +  
> > +  
> > +  
> > +
> > +
> 
> You can get rid of  and  too, then add
>  and
>  to avoid unnecessary devices
> popping up in the output file.

FYI, this XML file is identical to the pre-existing qemuxml2argv-qemu-ns.xml
file, so I'm inclined to leave it as is

> 
> > +++ b/tests/qemuxml2argvtest.c
> > @@ -1505,6 +1505,7 @@ mymain(void)
> >  
> >  DO_TEST("qemu-ns", NONE);
> >  DO_TEST("qemu-ns-no-env", NONE);
> > +DO_TEST("qemu-ns-alt", NONE);
> 
> You'll want to add this to qemuxml2xmltest too.

This wont round trip - it'll end up outputing the canonical xmlns syntax
instead, for which we already have output tests.

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

[libvirt] [PATCH v2 2/4] tests: virjson: Remove spaces from 'very-hard' parsing example

[Peter Krempa]

The example is rather long and upcomming patch will check whether the
string can be formatted back. As the formatted string lacks spaces and
adding the 'expect' string with spaces would be rather long, just drop
spaces from this test case.

There are other test cases which do contain spaces.
---
 tests/virjsontest.c | 48 
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/tests/virjsontest.c b/tests/virjsontest.c
index a5ffc4707..029a580f4 100644
--- a/tests/virjsontest.c
+++ b/tests/virjsontest.c
@@ -386,30 +386,30 @@ mymain(void)
   "\"label\": \"charmonitor\"}, {\"filename\": 
\"pty:/dev/pts/158\","
   "\"label\": \"charserial0\"}], \"id\": \"libvirt-3\"}");

-DO_TEST_PARSE("VeryHard", "{\"return\": [{\"name\": \"quit\"}, {\"name\":"
-  "\"eject\"}, {\"name\": \"change\"}, {\"name\": 
\"screendump\"},"
-  "{\"name\": \"stop\"}, {\"name\": \"cont\"}, {\"name\": "
-  "\"system_reset\"}, {\"name\": \"system_powerdown\"}, "
-  "{\"name\": \"device_add\"}, {\"name\": \"device_del\"}, "
-  "{\"name\": \"cpu\"}, {\"name\": \"memsave\"}, {\"name\": "
-  "\"pmemsave\"}, {\"name\": \"migrate\"}, {\"name\": "
-  "\"migrate_cancel\"}, {\"name\": \"migrate_set_speed\"},"
-  "{\"name\": \"client_migrate_info\"}, {\"name\": "
-  "\"migrate_set_downtime\"}, {\"name\": \"netdev_add\"}, "
-  "{\"name\": \"netdev_del\"}, {\"name\": \"block_resize\"},"
-  "{\"name\": \"balloon\"}, {\"name\": \"set_link\"}, 
{\"name\":"
-  "\"getfd\"}, {\"name\": \"closefd\"}, {\"name\": 
\"block_passwd\"},"
-  "{\"name\": \"set_password\"}, {\"name\": 
\"expire_password\"},"
-  "{\"name\": \"qmp_capabilities\"}, {\"name\": "
-  "\"human-monitor-command\"}, {\"name\": \"query-version\"},"
-  "{\"name\": \"query-commands\"}, {\"name\": 
\"query-chardev\"},"
-  "{\"name\": \"query-block\"}, {\"name\": 
\"query-blockstats\"}, "
-  "{\"name\": \"query-cpus\"}, {\"name\": \"query-pci\"}, 
{\"name\":"
-  "\"query-kvm\"}, {\"name\": \"query-status\"}, {\"name\": "
-  "\"query-mice\"}, {\"name\": \"query-vnc\"}, {\"name\": "
-  "\"query-spice\"}, {\"name\": \"query-name\"}, {\"name\": "
-  "\"query-uuid\"}, {\"name\": \"query-migrate\"}, {\"name\": "
-  "\"query-balloon\"}], \"id\": \"libvirt-2\"}");
+DO_TEST_PARSE("VeryHard", "{\"return\":[{\"name\":\"quit\"},{\"name\":"
+  "\"eject\"},{\"name\":\"change\"},{\"name\":\"screendump\"},"
+  "{\"name\":\"stop\"},{\"name\":\"cont\"},{\"name\":"
+  "\"system_reset\"},{\"name\":\"system_powerdown\"},"
+  "{\"name\":\"device_add\"},{\"name\":\"device_del\"},"
+  "{\"name\":\"cpu\"},{\"name\":\"memsave\"},{\"name\":"
+  "\"pmemsave\"},{\"name\":\"migrate\"},{\"name\":"
+  "\"migrate_cancel\"},{\"name\":\"migrate_set_speed\"},"
+  "{\"name\":\"client_migrate_info\"},{\"name\":"
+  "\"migrate_set_downtime\"},{\"name\":\"netdev_add\"},"
+  "{\"name\":\"netdev_del\"},{\"name\":\"block_resize\"},"
+  "{\"name\":\"balloon\"},{\"name\":\"set_link\"},{\"name\":"
+  
"\"getfd\"},{\"name\":\"closefd\"},{\"name\":\"block_passwd\"},"
+  "{\"name\":\"set_password\"},{\"name\":\"expire_password\"},"
+  "{\"name\":\"qmp_capabilities\"},{\"name\":"
+  "\"human-monitor-command\"},{\"name\":\"query-version\"},"
+  "{\"name\":\"query-commands\"},{\"name\":\"query-chardev\"},"
+  "{\"name\":\"query-block\"},{\"name\":\"query-blockstats\"},"
+  
"{\"name\":\"query-cpus\"},{\"name\":\"query-pci\"},{\"name\":"
+  "\"query-kvm\"},{\"name\":\"query-status\"},{\"name\":"
+  "\"query-mice\"},{\"name\":\"query-vnc\"},{\"name\":"
+  "\"query-spice\"},{\"name\":\"query-name\"},{\"name\":"
+  "\"query-uuid\"},{\"name\":\"query-migrate\"},{\"name\":"
+  "\"query-balloon\"}],\"id\":\"libvirt-2\"}");

 DO_TEST_FULL("add and remove", AddRemove,
  "{\"name\": \"sample\", \"value\": true}",
-- 
2.12.2

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

[libvirt] [PATCH v2 4/4] tests: virjson: Test string escaping

[Peter Krempa]

Make sure that JSON strings can contain characters which need to be
escaped (double quotes, backslashes, tabs, etc.) and that JSON objects
formatted into strings can be nested into strings.
---
 tests/virjsontest.c | 66 +
 1 file changed, 66 insertions(+)

diff --git a/tests/virjsontest.c b/tests/virjsontest.c
index a6e158179..30457d118 100644
--- a/tests/virjsontest.c
+++ b/tests/virjsontest.c
@@ -365,7 +365,67 @@ testJSONDeflatten(const void *data)
 VIR_FREE(actual);

 return ret;
+}
+
+
+static int
+testJSONEscapeObj(const void *data ATTRIBUTE_UNUSED)
+{
+virJSONValuePtr json = NULL;
+virJSONValuePtr nestjson = NULL;
+virJSONValuePtr parsejson = NULL;
+char *neststr = NULL;
+char *result = NULL;
+const char *parsednestedstr;
+int ret = -1;
+
+if (virJSONValueObjectCreate(,
+ "s:stringkey", "stringvalue",
+ "i:numberkey", 1234,
+ "b:booleankey", false, NULL) < 0) {
+VIR_TEST_VERBOSE("failed to create nested json object");
+goto cleanup;
+}
+
+if (!(neststr = virJSONValueToString(nestjson, false))) {
+VIR_TEST_VERBOSE("failed to format nested json object");
+goto cleanup;
+}
+
+if (virJSONValueObjectCreate(, "s:test", neststr, NULL) < 0) {
+VIR_TEST_VERBOSE("Failed to create json object");
+goto cleanup;
+}
+
+if (!(result = virJSONValueToString(json, false))) {
+VIR_TEST_VERBOSE("Failed to format json object");
+goto cleanup;
+}
+
+if (!(parsejson = virJSONValueFromString(result))) {
+VIR_TEST_VERBOSE("Failed to parse JSON with nested JSON in string");
+goto cleanup;
+}

+if (!(parsednestedstr = virJSONValueObjectGetString(parsejson, "test"))) {
+VIR_TEST_VERBOSE("Failed to retrieve string containing nested json");
+goto cleanup;
+}
+
+if (STRNEQ(parsednestedstr, neststr)) {
+virTestDifference(stderr, neststr, parsednestedstr);
+goto cleanup;
+}
+
+ret = 0;
+
+ cleanup:
+VIR_FREE(neststr);
+VIR_FREE(result);
+virJSONValueFree(json);
+virJSONValueFree(nestjson);
+virJSONValueFree(parsejson);
+return ret;
 }


@@ -490,6 +550,10 @@ mymain(void)
 DO_TEST_PARSE("integer", "1", NULL);
 DO_TEST_PARSE("boolean", "true", NULL);
 DO_TEST_PARSE("null", "null", NULL);
+
+DO_TEST_PARSE("escaping symbols", "[\"\\\"\\t\\n\"]", NULL);
+DO_TEST_PARSE("escaped strings", "[\"{\\\"blurb\\\":\\\"test\\\"}\"]", 
NULL);
+
 DO_TEST_PARSE_FAIL("incomplete keyword", "tr");
 DO_TEST_PARSE_FAIL("overdone keyword", "[ truest ]");
 DO_TEST_PARSE_FAIL("unknown keyword", "huh");
@@ -522,6 +586,8 @@ mymain(void)
 DO_TEST_FULL("lookup with correct type", Lookup,
  "{ \"a\": {}, \"b\": 1, \"c\": \"str\", \"d\": [] }",
  NULL, true);
+DO_TEST_FULL("create object with nested json in attribute", EscapeObj,
+ NULL, NULL, true);

 #define DO_TEST_DEFLATTEN(name, pass) \
 DO_TEST_FULL(name, Deflatten, name, NULL, pass)
-- 
2.12.2

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

[libvirt] [PATCH v2 1/4] tests: virjson: Modify logic in testJSONFromString

[Peter Krempa]

To allow better testing in case where the string was parsed, modify the
logic so that the regular code path is not included in a conditional
block.
---
 tests/virjsontest.c | 22 +++---
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/tests/virjsontest.c b/tests/virjsontest.c
index b9c210620..a5ffc4707 100644
--- a/tests/virjsontest.c
+++ b/tests/virjsontest.c
@@ -27,24 +27,24 @@ testJSONFromString(const void *data)

 json = virJSONValueFromString(info->doc);

-if (info->pass) {
-if (!json) {
+if (!json) {
+if (info->pass) {
 VIR_TEST_VERBOSE("Fail to parse %s\n", info->doc);
-ret = -1;
-goto cleanup;
-} else {
-VIR_TEST_DEBUG("Parsed %s\n", info->doc);
-}
-} else {
-if (json) {
-VIR_TEST_VERBOSE("Should not have parsed %s\n", info->doc);
-ret = -1;
 goto cleanup;
 } else {
 VIR_TEST_DEBUG("Fail to parse %s\n", info->doc);
+ret = 0;
+goto cleanup;
 }
 }

+if (!info->pass) {
+VIR_TEST_VERBOSE("Should not have parsed %s\n", info->doc);
+goto cleanup;
+}
+
+VIR_TEST_DEBUG("Parsed %s\n", info->doc);
+
 ret = 0;

  cleanup:
-- 
2.12.2

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

[libvirt] [PATCH v2 0/4] tests: json: Improve testing of parsing and formatting

[Peter Krempa]

Now rebased to master after changes to the virjsontest.

Purpose of this series was to test json nested in json. While it won't be used,
it may be worth to test that our parser and formatter handles escaping/nesting
properly.

Peter Krempa (4):
  tests: virjson: Modify logic in testJSONFromString
  tests: virjson: Remove spaces from 'very-hard' parsing example
  tests: virjson: Test formatting along with parsing of JSON objects
  tests: virjson: Test string escaping

 tests/virjsontest.c | 194 +++-
 1 file changed, 145 insertions(+), 49 deletions(-)

-- 
2.12.2

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

[libvirt] [PATCH v2 3/4] tests: virjson: Test formatting along with parsing of JSON objects

[Peter Krempa]

Format the parsed string back and compare it to the original (or
modified) string for back and forth comparison.
---
 tests/virjsontest.c | 58 -
 1 file changed, 44 insertions(+), 14 deletions(-)

diff --git a/tests/virjsontest.c b/tests/virjsontest.c
index 029a580f4..a6e158179 100644
--- a/tests/virjsontest.c
+++ b/tests/virjsontest.c
@@ -23,6 +23,8 @@ testJSONFromString(const void *data)
 {
 const struct testInfo *info = data;
 virJSONValuePtr json;
+const char *expectstr = info->expect ? info->expect : info->doc;
+char *formatted = NULL;
 int ret = -1;

 json = virJSONValueFromString(info->doc);
@@ -45,9 +47,20 @@ testJSONFromString(const void *data)

 VIR_TEST_DEBUG("Parsed %s\n", info->doc);

+if (!(formatted = virJSONValueToString(json, false))) {
+VIR_TEST_VERBOSE("Failed to format json data\n");
+goto cleanup;
+}
+
+if (STRNEQ(expectstr, formatted)) {
+virTestDifference(stderr, expectstr, formatted);
+goto cleanup;
+}
+
 ret = 0;

  cleanup:
+VIR_FREE(formatted);
 virJSONValueFree(json);
 return ret;
 }
@@ -368,23 +381,39 @@ mymain(void)
 ret = -1;   \
 } while (0)

-#define DO_TEST_PARSE(name, doc)\
-DO_TEST_FULL(name, FromString, doc, NULL, true)
+/**
+ * DO_TEST_PARSE:
+ * @name: test name
+ * @doc: source JSON string
+ * @expect: expected output JSON formatted from parsed @doc
+ *
+ * Parses @doc and formats it back. If @expect is NULL the result has to be
+ * identical to @doc.
+ */
+#define DO_TEST_PARSE(name, doc, expect)\
+DO_TEST_FULL(name, FromString, doc, expect, true)

 #define DO_TEST_PARSE_FAIL(name, doc)   \
 DO_TEST_FULL(name, FromString, doc, NULL, false)


-DO_TEST_PARSE("Simple", "{\"return\": {}, \"id\": \"libvirt-1\"}");
+DO_TEST_PARSE("Simple", "{\"return\": {}, \"id\": \"libvirt-1\"}",
+  "{\"return\":{},\"id\":\"libvirt-1\"}");
 DO_TEST_PARSE("NotSoSimple", "{\"QMP\": {\"version\": {\"qemu\":"
   "{\"micro\": 91, \"minor\": 13, \"major\": 0},"
-  "\"package\": \" (qemu-kvm-devel)\"}, \"capabilities\": 
[]}}");
-
+  "\"package\": \" (qemu-kvm-devel)\"}, \"capabilities\": 
[]}}",
+  "{\"QMP\":{\"version\":{\"qemu\":"
+  "{\"micro\":91,\"minor\":13,\"major\":0},"
+  "\"package\":\" (qemu-kvm-devel)\"},\"capabilities\":[]}}");

 DO_TEST_PARSE("Harder", "{\"return\": [{\"filename\": "
   
"\"unix:/home/berrange/.libvirt/qemu/lib/tck.monitor,server\","
   "\"label\": \"charmonitor\"}, {\"filename\": 
\"pty:/dev/pts/158\","
-  "\"label\": \"charserial0\"}], \"id\": \"libvirt-3\"}");
+  "\"label\": \"charserial0\"}], \"id\": \"libvirt-3\"}",
+  "{\"return\":[{\"filename\":"
+  
"\"unix:/home/berrange/.libvirt/qemu/lib/tck.monitor,server\","
+  
"\"label\":\"charmonitor\"},{\"filename\":\"pty:/dev/pts/158\","
+  "\"label\":\"charserial0\"}],\"id\":\"libvirt-3\"}");

 DO_TEST_PARSE("VeryHard", "{\"return\":[{\"name\":\"quit\"},{\"name\":"
   "\"eject\"},{\"name\":\"change\"},{\"name\":\"screendump\"},"
@@ -409,7 +438,7 @@ mymain(void)
   "\"query-mice\"},{\"name\":\"query-vnc\"},{\"name\":"
   "\"query-spice\"},{\"name\":\"query-name\"},{\"name\":"
   "\"query-uuid\"},{\"name\":\"query-migrate\"},{\"name\":"
-  "\"query-balloon\"}],\"id\":\"libvirt-2\"}");
+  "\"query-balloon\"}],\"id\":\"libvirt-2\"}", NULL);

 DO_TEST_FULL("add and remove", AddRemove,
  "{\"name\": \"sample\", \"value\": true}",
@@ -445,21 +474,22 @@ mymain(void)
  "\"query-balloon\"}], \"id\": \"libvirt-2\"}", NULL, true);


-DO_TEST_PARSE("almost nothing", "[]");
+DO_TEST_PARSE("almost nothing", "[]", NULL);
 DO_TEST_PARSE_FAIL("nothing", "");

-DO_TEST_PARSE("number without garbage", "[ 234545 ]");
+DO_TEST_PARSE("number without garbage", "[ 234545 ]", "[234545]");
 DO_TEST_PARSE_FAIL("number with garbage", "[ 2345b45 ]");

-DO_TEST_PARSE("float without garbage", "[ 0.0314159e+100 ]");
+DO_TEST_PARSE("float without garbage", "[ 0.0314159e+100 ]", 
"[0.0314159e+100]");
 DO_TEST_PARSE_FAIL("float with garbage", "[ 0.0314159ee+100 ]");

-DO_TEST_PARSE("string", "[ \"The meaning of life\" ]");
+DO_TEST_PARSE("string", "[ \"The meaning of life\" ]",
+  "[\"The meaning of life\"]");
 DO_TEST_PARSE_FAIL("unterminated string", "[ \"The meaning of lif ]");

-DO_TEST_PARSE("integer", "1");
-DO_TEST_PARSE("boolean", "true");
-DO_TEST_PARSE("null", "null");
+DO_TEST_PARSE("integer", "1", NULL);
+

Re: [libvirt] [PATCH v2] vz: allow to start vz driver without host cache info

[Martin Kletzander]

On Tue, Jul 11, 2017 at 03:20:38PM +0300, Mikhail Feoktistov wrote:


On 11.07.2017 12:22, Martin Kletzander wrote:

On Tue, Jul 11, 2017 at 04:59:05AM -0400, Mikhail Feoktistov wrote:

Show warning message instead of fail operation.
It happens if kernel or cpu doesn't support reporting cpu cache info.
In case of Virtuozzo file "id" doesn't exist.


What is your kernel version?


 3.10.0-514.16.1.vz7.30.10

Is there another way of getting the information about the cache ID?
Maybe we need to parse the name of the cache directory 'index2' would be
id 2 maybe?  If there is no other way, then this fix is fine (as most
drivers do the same thing), but I would rather fix it if that's
possible.  Unfortunately the cache information is structured stupidly
compared to other kernel-provided topology-related information.

Only kernel 4.11 reports cache ID (maybe 4.10, but not checked)
4.9 doesn't report ID


Oh, so that is just older kernel.  OK, then.

Reviewed-by: Martin Kletzander 


ID doesn't match to indexN



Yeah, in vanilla it is number per level and easily deductible, but not
really something we want to add into the code.


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

Re: [libvirt] How to make gic_version=3 as defailt to qemu on arm64

[Andrea Bolognani]

On Tue, 2017-07-11 at 14:44 +0530, Vishnu Pajjuri wrote:
> Hi
> 
>I'm running Openstack which is installed by using
> devstack. But it is not launching VMs.

libvir-list is for the *development* of libvirt: for
questions related to *running* libvirt, please use the
libvirt-users mailing list instead.

> From command line with gic_version=3 option it is running.
> But openstack glance doesn't have any privilege to specify
> gic version.
> 
> On my ARM64 board gicv2 is not supported, so i want to make
> gicv3 as default one to pass to qemu.
> Kindly suggest any specific version of vibvirt or patch
> such that libvirt should pass gicv3 as default one.

If you're running libvirt >= 1.3.5 and QEMU >= 2.6.0, both
released about a year ago, you should get GICv3 out of the
box because of the capabilities of your hardware.

That said, aarch64 support has been steadily improving
throughout the stack, so I would strongly recommend you
don't stop there and install the very latest upstream
releases of all components instead.

-- 
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 3/5] Remove network constants out of internal.h

[Peter Krempa]

On Tue, Jul 11, 2017 at 13:38:56 +0100, Daniel Berrange wrote:
> On Tue, Jul 11, 2017 at 02:27:10PM +0200, Peter Krempa wrote:
> > On Mon, Jul 10, 2017 at 16:07:08 +0100, Daniel Berrange wrote:
> > > On Mon, Jul 10, 2017 at 01:01:26PM +0200, Peter Krempa wrote:
> > > > On Wed, Jul 05, 2017 at 12:58:49 +0100, Daniel Berrange wrote:

[...]

> > > > > +# ifndef HOST_NAME_MAX
> > > > > +#  define HOST_NAME_MAX 256
> > > > 
> > > > I'm not entirely convinced that this semantically belongs to
> > > > virsocketaddr.h
> > > > 
> > > > At least in the virlog.c usage case it is not used in any way in regards
> > > > to network communication.
> > > 
> > > The constant is used to define a hostname string buffer, which is then
> > > used with the gethostname() system call. That's network related IMHO.
> > 
> > I find it weird, that it gets defined in virsocketaddr.h, but the only
> > usage is in src/util/virutil.c thus it needs cross-inclusions which were
> > not in place prior to the move.
> > 
> > Should perhaps virGetHostnameImpl and friends be moved too in that case?
> 
> Lets just move HOST_NAME_MAX out of the headers entirely and just pyut it
> in virutil.c at the virGetHostname method definition.

Yep, that's reasonable. ACK to that.


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

[libvirt] [PATCH] tests: virjson: Reuse VIR_TEST_VERBOSE in testJSONCopy

[Peter Krempa]

Use VIR_TEST_VERBOSE instead of calling virTestGetVerbose and
conditionally fprintf. Additionally remove redundant setting of 'ret' to
-1.
---

Pushed as trivial.

 tests/virjsontest.c | 26 ++
 1 file changed, 6 insertions(+), 20 deletions(-)

diff --git a/tests/virjsontest.c b/tests/virjsontest.c
index b3a230a02..b9c210620 100644
--- a/tests/virjsontest.c
+++ b/tests/virjsontest.c
@@ -240,40 +240,31 @@ testJSONCopy(const void *data)

 json = virJSONValueFromString(info->doc);
 if (!json) {
-if (virTestGetVerbose())
-fprintf(stderr, "Failed to parse %s\n", info->doc);
-ret = -1;
+VIR_TEST_VERBOSE("Failed to parse %s\n", info->doc);
 goto cleanup;
 }

 jsonCopy = virJSONValueCopy(json);
 if (!jsonCopy) {
-if (virTestGetVerbose())
-fprintf(stderr, "Failed to copy JSON data\n");
-ret = -1;
+VIR_TEST_VERBOSE("Failed to copy JSON data\n");
 goto cleanup;
 }

 result = virJSONValueToString(json, false);
 if (!result) {
-if (virTestGetVerbose())
-fprintf(stderr, "Failed to format original JSON data\n");
-ret = -1;
+VIR_TEST_VERBOSE("Failed to format original JSON data\n");
 goto cleanup;
 }

 resultCopy = virJSONValueToString(json, false);
 if (!resultCopy) {
-if (virTestGetVerbose())
-fprintf(stderr, "Failed to format copied JSON data\n");
-ret = -1;
+VIR_TEST_VERBOSE("Failed to format copied JSON data\n");
 goto cleanup;
 }

 if (STRNEQ(result, resultCopy)) {
 if (virTestGetVerbose())
 virTestDifference(stderr, result, resultCopy);
-ret = -1;
 goto cleanup;
 }

@@ -282,24 +273,19 @@ testJSONCopy(const void *data)

 result = virJSONValueToString(json, true);
 if (!result) {
-if (virTestGetVerbose())
-fprintf(stderr, "Failed to format original JSON data\n");
-ret = -1;
+VIR_TEST_VERBOSE("Failed to format original JSON data\n");
 goto cleanup;
 }

 resultCopy = virJSONValueToString(json, true);
 if (!resultCopy) {
-if (virTestGetVerbose())
-fprintf(stderr, "Failed to format copied JSON data\n");
-ret = -1;
+VIR_TEST_VERBOSE("Failed to format copied JSON data\n");
 goto cleanup;
 }

 if (STRNEQ(result, resultCopy)) {
 if (virTestGetVerbose())
 virTestDifference(stderr, result, resultCopy);
-ret = -1;
 goto cleanup;
 }

-- 
2.12.2

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

Re: [libvirt] [PATCH v2 3/5] Remove network constants out of internal.h

[Daniel P. Berrange]

On Tue, Jul 11, 2017 at 02:27:10PM +0200, Peter Krempa wrote:
> On Mon, Jul 10, 2017 at 16:07:08 +0100, Daniel Berrange wrote:
> > On Mon, Jul 10, 2017 at 01:01:26PM +0200, Peter Krempa wrote:
> > > On Wed, Jul 05, 2017 at 12:58:49 +0100, Daniel Berrange wrote:
> > > > The HOST_NAME_MAX, INET_ADDRSTRLEN and VIR_LOOPBACK_IPV4_ADDR
> > > > constants are only used by a handful of files, so are better
> > > > kept in virsocketaddr.h
> > > > 
> > > > Signed-off-by: Daniel P. Berrange 
> > > > ---
> > > >  src/internal.h | 16 
> > > >  src/libxl/libxl_conf.c |  1 +
> > > >  src/nwfilter/nwfilter_dhcpsnoop.c  |  1 +
> > > >  src/nwfilter/nwfilter_gentech_driver.c |  1 +
> > > >  src/qemu/qemu_conf.c   |  1 +
> > > >  src/util/virsocketaddr.h   | 16 
> > > >  src/util/virutil.c |  1 +
> > > >  src/vz/vz_sdk.c|  1 +
> > > >  8 files changed, 22 insertions(+), 16 deletions(-)
> > > > 
> > > 
> > > [...]
> > > 
> > > > diff --git a/src/util/virsocketaddr.h b/src/util/virsocketaddr.h
> > > > index 43a3706..ae76166 100644
> > > > --- a/src/util/virsocketaddr.h
> > > > +++ b/src/util/virsocketaddr.h
> > > > @@ -32,6 +32,22 @@
> > > >  #  include 
> > > >  # endif
> > > >  
> > > > +/* On architectures which lack these limits, define them (ie. Cygwin).
> > > > + * Note that the libvirt code should be robust enough to handle the
> > > > + * case where actual value is longer than these limits (eg. by setting
> > > > + * length correctly in second argument to gethostname and by always
> > > > + * using strncpy instead of strcpy).
> > > > + */
> > > > +# ifndef HOST_NAME_MAX
> > > > +#  define HOST_NAME_MAX 256
> > > 
> > > I'm not entirely convinced that this semantically belongs to
> > > virsocketaddr.h
> > > 
> > > At least in the virlog.c usage case it is not used in any way in regards
> > > to network communication.
> > 
> > The constant is used to define a hostname string buffer, which is then
> > used with the gethostname() system call. That's network related IMHO.
> 
> I find it weird, that it gets defined in virsocketaddr.h, but the only
> usage is in src/util/virutil.c thus it needs cross-inclusions which were
> not in place prior to the move.
> 
> Should perhaps virGetHostnameImpl and friends be moved too in that case?

Lets just move HOST_NAME_MAX out of the headers entirely and just pyut it
in virutil.c at the virGetHostname method definition.

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 9/9] tests: Validate that JSON deflattening fixed nested json pseudo-protocol strings

[Peter Krempa]

On Mon, Jul 10, 2017 at 10:01:30 -0400, John Ferlan wrote:
> 
> 
> On 06/27/2017 08:46 AM, Peter Krempa wrote:
> > Sheepdog and possibly others use nested objects for network server and
> > thus could be specified in a way that libvirt would not parse.
> > 
> > Validates that https://bugzilla.redhat.com/show_bug.cgi?id=1464821
> > is fixed properly.
> > ---
> >  tests/virjsondata/deflatten-qemu-sheepdog-in.json  | 11 +++
> >  tests/virjsondata/deflatten-qemu-sheepdog-out.json | 13 +
> >  tests/virjsontest.c|  1 +
> >  tests/virstoragetest.c | 10 ++
> >  4 files changed, 35 insertions(+)
> >  create mode 100644 tests/virjsondata/deflatten-qemu-sheepdog-in.json
> >  create mode 100644 tests/virjsondata/deflatten-qemu-sheepdog-out.json
> > 
> 
> Reviewed-by: John Ferlan 

Thanks for the review, I've implemented your suggestions and pushed the
patches.


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

Re: [libvirt] [PATCH 0/5] tests: json: Improve testing of parsing and formatting

[Peter Krempa]

On Tue, Jul 04, 2017 at 12:53:23 +0200, Peter Krempa wrote:
> For an experiment I was doing I needed to nest JSON string into an attribute 
> of
> a different JSON string, so I wrote some tests for that.
> 
> The experiment failed, but the tests may make sense to have in libvirt.
> 

This fails to apply now, so I'll post a rebased version soon.


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 3/5] Remove network constants out of internal.h

[Peter Krempa]

On Mon, Jul 10, 2017 at 16:07:08 +0100, Daniel Berrange wrote:
> On Mon, Jul 10, 2017 at 01:01:26PM +0200, Peter Krempa wrote:
> > On Wed, Jul 05, 2017 at 12:58:49 +0100, Daniel Berrange wrote:
> > > The HOST_NAME_MAX, INET_ADDRSTRLEN and VIR_LOOPBACK_IPV4_ADDR
> > > constants are only used by a handful of files, so are better
> > > kept in virsocketaddr.h
> > > 
> > > Signed-off-by: Daniel P. Berrange 
> > > ---
> > >  src/internal.h | 16 
> > >  src/libxl/libxl_conf.c |  1 +
> > >  src/nwfilter/nwfilter_dhcpsnoop.c  |  1 +
> > >  src/nwfilter/nwfilter_gentech_driver.c |  1 +
> > >  src/qemu/qemu_conf.c   |  1 +
> > >  src/util/virsocketaddr.h   | 16 
> > >  src/util/virutil.c |  1 +
> > >  src/vz/vz_sdk.c|  1 +
> > >  8 files changed, 22 insertions(+), 16 deletions(-)
> > > 
> > 
> > [...]
> > 
> > > diff --git a/src/util/virsocketaddr.h b/src/util/virsocketaddr.h
> > > index 43a3706..ae76166 100644
> > > --- a/src/util/virsocketaddr.h
> > > +++ b/src/util/virsocketaddr.h
> > > @@ -32,6 +32,22 @@
> > >  #  include 
> > >  # endif
> > >  
> > > +/* On architectures which lack these limits, define them (ie. Cygwin).
> > > + * Note that the libvirt code should be robust enough to handle the
> > > + * case where actual value is longer than these limits (eg. by setting
> > > + * length correctly in second argument to gethostname and by always
> > > + * using strncpy instead of strcpy).
> > > + */
> > > +# ifndef HOST_NAME_MAX
> > > +#  define HOST_NAME_MAX 256
> > 
> > I'm not entirely convinced that this semantically belongs to
> > virsocketaddr.h
> > 
> > At least in the virlog.c usage case it is not used in any way in regards
> > to network communication.
> 
> The constant is used to define a hostname string buffer, which is then
> used with the gethostname() system call. That's network related IMHO.

I find it weird, that it gets defined in virsocketaddr.h, but the only
usage is in src/util/virutil.c thus it needs cross-inclusions which were
not in place prior to the move.

Should perhaps virGetHostnameImpl and friends be moved too in that case?


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] vz: allow to start vz driver without host cache info

[Mikhail Feoktistov]

On 11.07.2017 12:22, Martin Kletzander wrote:

On Tue, Jul 11, 2017 at 04:59:05AM -0400, Mikhail Feoktistov wrote:

Show warning message instead of fail operation.
It happens if kernel or cpu doesn't support reporting cpu cache info.
In case of Virtuozzo file "id" doesn't exist.


What is your kernel version?


 3.10.0-514.16.1.vz7.30.10

Is there another way of getting the information about the cache ID?
Maybe we need to parse the name of the cache directory 'index2' would be
id 2 maybe?  If there is no other way, then this fix is fine (as most
drivers do the same thing), but I would rather fix it if that's
possible.  Unfortunately the cache information is structured stupidly
compared to other kernel-provided topology-related information.

Only kernel 4.11 reports cache ID (maybe 4.10, but not checked)
4.9 doesn't report ID
ID doesn't match to indexN

[liveuser@localhost-live ~]$ uname -a
Linux localhost-live 4.11.0-2.fc26.x86_64 #1 SMP Tue May 9 15:24:49 UTC 
2017 x86_64 x86_64 x86_64 GNU/Linux
[liveuser@localhost-live ~]$ cat 
/sys/devices/system/cpu/cpu1/cache/index0/id

0
[liveuser@localhost-live ~]$ cat 
/sys/devices/system/cpu/cpu1/cache/index1/id

0
[liveuser@localhost-live ~]$ cat 
/sys/devices/system/cpu/cpu1/cache/index2/id

0
[liveuser@localhost-live ~]$ cat 
/sys/devices/system/cpu/cpu0/cache/index2/id

0
[liveuser@localhost-live ~]$ cat 
/sys/devices/system/cpu/cpu0/cache/index0/id

0


---
src/vz/vz_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index 6f4aee3..eb97e54 100644
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -119,7 +119,7 @@ vzBuildCapabilities(void)
goto error;

if (virCapabilitiesInitCaches(caps) < 0)
-goto error;
+VIR_WARN("Failed to get host CPU cache info");

verify(ARRAY_CARDINALITY(archs) == ARRAY_CARDINALITY(emulators));

--
1.8.3.1

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


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

Re: [libvirt] [PATCH 06/19] storage: Fix return value checks for virAsprintf

[Pavel Hrdina]

On Tue, May 09, 2017 at 11:30:13AM -0400, John Ferlan wrote:
> Use the < 0 rather than == -1 (consistently) for virAsprintf errors.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/storage/storage_backend_logical.c  | 2 +-
>  src/storage/storage_backend_rbd.c  | 8 
>  src/storage/storage_backend_sheepdog.c | 4 ++--
>  src/storage/storage_backend_zfs.c  | 4 ++--
>  src/storage/storage_util.c | 6 +++---
>  src/test/test_driver.c | 9 +++--
>  6 files changed, 15 insertions(+), 18 deletions(-)

Reviewed-by: Pavel Hrdina 


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

Re: [libvirt] [PATCH 05/19] test: Add testStorageVolDefFindByName for storage volume tests

[Pavel Hrdina]

On Tue, May 09, 2017 at 11:30:12AM -0400, John Ferlan wrote:
> Remove repetitive code, replace with common function.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/test/test_driver.c | 56 
> +++---
>  1 file changed, 21 insertions(+), 35 deletions(-)

Reviewed-by: Pavel Hrdina 


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

[libvirt] 答复: Re: [PATCH] util: increase libnl buffer size to 1M

[lu.zhipeng]

发件人： 
收件人： 
抄送人：芦志朋10108272 
日 期 ：2017年07月11日 17:11
主 题 ：Re: [libvirt] [PATCH] util: increase libnl buffer size to 1M






>>On Mon, Jul 10, 2017 at 02:51:34PM -0400, John Ferlan wrote:
>>
> >
>> On 06/29/2017 02:05 PM, ZhiPeng Lu wrote:
>> > nl_recv() returns the error "No buffer space available"
>> > when using virsh destroy domain with 240 or more
>> > passhthrough network interfaces.
>> 
>> pass-through
>> 
>> > The patch increases libnl sock receive buffer size to 1M,
>> > and nl_recv() doesn't return error when destroying domain
>> > with 512 network interfaces.
>> > 
>> > Signed-off-by: ZhiPeng Lu 
>> > ---
>> >  src/util/virnetlink.c | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> > 
>> 
>> This feels like something that perhaps should be configurable - that is
>> some /etc/libvirt/libvirtd.conf variable otherwise, we'll keep hitting
>> some conflated maximum based on the size of something.

>1 MB matches what systemed/udevd uses, so if we hit that limit, then  the
>system as a whole is going to struggle already. So I don't think we need
>make it configurable.




--1M is just an experience value, and I feel that setting up a 
configuration item


 can accommodate the needs of different users. 


In addition, this is only the netlink socket receive buffer size that will not 
affect the entire system.


The default value can still be set to 128K.














芦志朋 luzhipeng






IT开发工程师 IT Development
Engineer
操作系统产品部/中心研究院/系统产品 OS Product Dept./Central R＆D Institute/System Product









深圳市南山区科技南路55号中兴通讯研发大楼33楼 
33/F, R Building, ZTE
Corporation Hi-tech Road South, 
Hi-tech
Industrial Park Nanshan District, Shenzhen, P.R.China, 518057 
T: +86 755  F:+86 755  
M: +86 xxx 
E: lu.zhip...@zte.com.cn 
www.zte.com.cn






原始邮件



发件人： 
收件人： 
抄送人：芦志朋10108272 
日 期 ：2017年07月11日 17:11
主 题 ：Re: [libvirt] [PATCH] util: increase libnl buffer size to 1M





On Mon, Jul 10, 2017 at 02:51:34PM -0400, John Ferlan wrote:
> 
> 
> On 06/29/2017 02:05 PM, ZhiPeng Lu wrote:
> > nl_recv() returns the error "No buffer space available"
> > when using virsh destroy domain with 240 or more
> > passhthrough network interfaces.
> 
> pass-through
> 
> > The patch increases libnl sock receive buffer size to 1M,
> > and nl_recv() doesn't return error when destroying domain
> > with 512 network interfaces.
> > 
> > Signed-off-by: ZhiPeng Lu 
> > ---
> >  src/util/virnetlink.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> 
> This feels like something that perhaps should be configurable - that is
> some /etc/libvirt/libvirtd.conf variable otherwise, we'll keep hitting
> some conflated maximum based on the size of something.

1 MB matches what systemed/udevd uses, so if we hit that limit, then  the
system as a whole is going to struggle already. So I don't think we need
make it configurable.


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] [Qemu-devel] change x86 default machine type to Q35?

[Thomas Huth]

On 10.07.2017 19:47, Eduardo Habkost wrote:
> (CCing libvir-list)
> 
> On Mon, Jul 10, 2017 at 07:45:54PM +0300, Michael S. Tsirkin wrote:
>> On Mon, Jul 10, 2017 at 10:59:43AM -0300, Eduardo Habkost wrote:
>>> On Mon, Jul 10, 2017 at 10:42:26AM +0300, Marcel Apfelbaum wrote:
 On 07/07/2017 21:03, Eduardo Habkost wrote:
> On Fri, Jul 07, 2017 at 06:17:57PM +0300, Michael S. Tsirkin wrote:
>> On Fri, Jul 07, 2017 at 10:39:49AM -0300, Eduardo Habkost wrote:
>>> On Wed, Jul 05, 2017 at 12:32:10PM +0300, Marcel Apfelbaum wrote:
>>> [...]

 The upper layers should manage the defaults by themselves so
 are not supposed to be affected.
>>>
>>> But they would be.  libvirt uses the default machine-type from
>>> QEMU.
>>
>> How about extending the command for supported machines with a
>> recommended machine type, and teaching libvirt to use that?
>
> I don't think QEMU has enough information to decide if it should
> recommend "q35" or "pc".

 We don't really need a complicated rule set, we would just recommend q35
 by default. Libvirt will try to create the default machine and if fails
 for some reason (what would it be?) it can switch to PC.

 The advanced logic would be "old systems should use PC", where old
 means Windows XP and before and so on. But this logic should appear
 in management layers above.
>>>
>>> In this case, is there any difference between "changing the
>>> default to q35" and "recommending q35", for libvirt users?
>>>
>>> -- 
>>> Eduardo
>>
>> No but libvirt users do not manage e.g. pci slots manually.
>> They do not use the -cdrom flag.
>> Etc.
>> So changing the default is unlikely to break things for them.
>>
> 
> I see.  If this part is really true (can libvirt developers
> confirm that?), then the proposed end result makes sense (not
> having a default for running QEMU directly, but changing default
> to "q35" for people using libvirt).
> 
> But I don't see why we would need a new mechanism to make QEMU
> recommend a machine-type for libvirt, if libvirt could simply
> choose its own default (or maybe also refuse to pick a default,
> if libvirt developers decide that's the best solution).

Agreed, it does not make much sense to invent a new mechanism here. I
guess the default should rather be switched in the the tools that create
the XML for libvirt, i.e. virt-install and friends?

Concerning QEMU, could we maybe simply emit a warning a la

 "you did not specify a machine type with the -M option, so you are
  currently running the the 'pc' machine type. Please note that future
  versions of QEMU might use the 'q35' machine type instead. If you
  require the 'pc' machine type for your setting, then please specify
  it with the -M option."

for a couple of releases, so that other people have a chance to update
their scripts, and then finally switch to q35 ?

 Thomas

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

[libvirt] How to make gic_version=3 as defailt to qemu on arm64

[Vishnu Pajjuri]

Hi

   I'm running Openstack which is installed by using devstack. But it is
not launching VMs.
>From command line with gic_version=3 option it is running. But openstack
glance doesn't have any privilege to specify gic version.

On my ARM64 board gicv2 is not supported, so i want to make gicv3 as
default one to pass to qemu.
Kindly suggest any specific version of vibvirt or patch such that libvirt
should pass gicv3 as default one.

Thanks in Advance
-Vishnu.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 04/19] test: Add helpers to fetch active/inactive storage pool by name

[Pavel Hrdina]

On Tue, May 09, 2017 at 11:30:11AM -0400, John Ferlan wrote:
> Rather than have repetitive code - create/use a couple of helpers:
> 
> testStoragePoolObjFindActiveByName and testStoragePoolObjFindInactiveByName

I would wrap this line, it's too long for no reason.

> This will also allow for the reduction of some cleanup path logic.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/test/test_driver.c | 256 
> +
>  1 file changed, 86 insertions(+), 170 deletions(-)
> 
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index efa54ff..9918df6 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -4030,6 +4030,46 @@ testStoragePoolObjFindByName(testDriverPtr privconn,
>  
>  
>  static virStoragePoolObjPtr
> +testStoragePoolObjFindActiveByName(testDriverPtr privconn,
> +   const char *name)
> +{
> +virStoragePoolObjPtr obj;
> +
> +if (!(obj = testStoragePoolObjFindByName(privconn, name)))
> +return NULL;
> +
> +if (!virStoragePoolObjIsActive(obj)) {
> +virReportError(VIR_ERR_OPERATION_INVALID,
> +   _("storage pool '%s' is not active"), name);
> +virStoragePoolObjUnlock(obj);
> +return NULL;
> +}
> +
> +return obj;
> +}
> +
> +
> +static virStoragePoolObjPtr
> +testStoragePoolObjFindInactiveByName(testDriverPtr privconn,
> + const char *name)
> +{
> +virStoragePoolObjPtr obj;
> +
> +if (!(obj = testStoragePoolObjFindByName(privconn, name)))
> +return NULL;
> +
> +if (virStoragePoolObjIsActive(obj)) {
> +virReportError(VIR_ERR_OPERATION_INVALID,
> +   _("storage pool '%s' is already active"), name);

I would remove the "already" for the error message.  Since this is only
test driver I'll leave it up to you.  The reason is that for some APIs
like "Undefine" the error message doesn't make sense.

Reviewed-by: Pavel Hrdina 


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

Re: [libvirt] [PATCH 2/8] qemuDomainBuildNamespace: Handle special file mount points

[John Ferlan]

On 07/11/2017 01:14 AM, Michal Privoznik wrote:
> On 06/27/2017 11:37 PM, John Ferlan wrote:
>>
>>
>> On 06/22/2017 12:18 PM, Michal Privoznik wrote:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1459592
>>>
>>> In 290a00e41d I've tried to fix the process of building a
>>> qemu namespace when dealing with file mount points. What I
>>> haven't realized then is that we might be dealing not with just
>>> regular files but also special files (like sockets). Indeed, try
>>> the following:
>>>
>>> 1) socat unix-listen:/tmp/soket stdio
>>> 2) touch /dev/socket
>>> 3) mount --bind /tmp/socket /dev/socket
>>> 4) virsh start anyDomain
>>>
>>> Problem with my previous approach is that I wasn't creating the
>>> temporary location (where mount points under /dev are moved) for
>>> anything but directories and regular files.
>>>
>>> Signed-off-by: Michal Privoznik 
>>> ---
>>>  src/qemu/qemu_domain.c | 6 --
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>>> index 8e7404da6..212717c80 100644
>>> --- a/src/qemu/qemu_domain.c
>>> +++ b/src/qemu/qemu_domain.c
>>> @@ -8356,9 +8356,11 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,
>>>  goto cleanup;
>>>  }
>>>  
>>> -/* At this point, devMountsPath is either a regular file or a 
>>> directory. */
>>> +/* At this point, devMountsPath is either:
>>> + * a file (regular or special), or
>>> + * a directory. */
>>>  if ((S_ISDIR(sb.st_mode) && virFileMakePath(devMountsSavePath[i]) 
>>> < 0) ||
>>> -(S_ISREG(sb.st_mode) && virFileTouch(devMountsSavePath[i], 
>>> sb.st_mode) < 0)) {
>>> +(!S_ISDIR(sb.st_mode) && virFileTouch(devMountsSavePath[i], 
>>> sb.st_mode) < 0)) {
>>
>> It would seem to me that this would open Pandora's box to all different
>> types of things (BLK, CHR, FIFO, LNK, NAM, MPB, MPC, NWK) - some of
>> which it may not be so popular to perform a touch on.
>>
>> I think you should keep it specific...  Perhaps use the list from
>> qemuDomainCreateDeviceRecursive:
>>
>>isReg = S_ISREG(sb.st_mode) || S_ISFIFO(sb.st_mode) ||
>> S_ISSOCK(sb.st_mode);
> 
> Not really. mount --bind makes the target to be the correct type. IOW:
> 

OK - I wasn't sure what all those other things were and going with
!IS_DIR just seemed to open the door to unsurety for me.  Since there
was other code that was more restrictive to decide "ifReg", I figured
using that same list would be fine, but I'm not sure if I ever check
where in the scheme of the path the other check is make.

If you're fine with how things are, then fine consider it...

Reviewed-by: John Ferlan 

John

> # create a regular file
> touch /tmp/blah
> 
> # here, assume /source/socket is a socket
> mount --bind /source/socket /tmp/blah
> 
> # now, /tmp/blah will be type of socket too
> 
> The only problem here is that for file based 'devices' (or things in
> general) we have to create the file whereas for directories we have to
> create directories. Just like in the example.
> 
> BTW, what type of file are NAM, MPB, MPC, NWK?
> 
> Michal
> 

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

Re: [libvirt] [PATCH go-xml] Support WWN tag in disks

[Daniel P. Berrange]

On Mon, Jul 10, 2017 at 04:12:06PM +0200, Thomas Hipp wrote:
> Support the WWN (World Wide Name) tag in disks, and add test code.
> 
> Signed-off-by: Thomas Hipp 
> ---
>  domain.go  | 1 +
>  domain_test.go | 2 ++
>  2 files changed, 3 insertions(+)

ACK, will push to git.


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 go-xml v3] Add support for QEMU

[Daniel P. Berrange]

On Mon, Jul 10, 2017 at 12:28:23PM +0200, Thomas Hipp wrote:
> Add support for QEMU, and add test code.
> 
> Signed-off-by: Thomas Hipp 
> ---
> Changes since v2:
>   - set default ns in commandline subtree
>   - remove ns prefix "qemu:"
> 
> ---
>  domain.go  | 50 +-
>  domain_test.go | 27 +++
>  2 files changed, 60 insertions(+), 17 deletions(-)

ACK, looks good now, will push to git.

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 go-xml] Add support for domain clock and test code

[Daniel P. Berrange]

On Mon, Jul 10, 2017 at 11:35:23AM +0800, zhenwei.pi wrote:
> Signed-off-by: zhenwei.pi 
> ---
>  domain.go  | 7 +++
>  domain_test.go | 6 ++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/domain.go b/domain.go
> index b9b0f77..1d020aa 100644
> --- a/domain.go
> +++ b/domain.go
> @@ -532,6 +532,12 @@ type DomainCPU struct {
>   Features []DomainCPUFeature `xml:"feature"`
>  }
>  
> +type DomainClock struct {
> + Offset string `xml:"offset,attr,omitempty"`
> + Basic  string `xml:"basic,attr,omitempty"`

This should be 'basis' not 'basic'. I'll change this when applying to
git so no need to resubmit.

> + Adjustment int`xml:"adjustment,attr,omitempty"`
> +}
> +
>  type DomainFeature struct {
>  }
>  
> @@ -607,6 +613,7 @@ type Domain struct {
>   OS*DomainOS  `xml:"os"`
>   Features  *DomainFeatureList `xml:"features"`
>   CPU   *DomainCPU `xml:"cpu"`
> + Clock *DomainClock   `xml:"clock,omitempty"`
>   OnPoweroffstring `xml:"on_poweroff,omitempty"`
>   OnReboot  string `xml:"on_reboot,omitempty"`
>   OnCrash   string `xml:"on_crash,omitempty"`
> diff --git a/domain_test.go b/domain_test.go
> index 47e5e26..536b94c 100644
> --- a/domain_test.go
> +++ b/domain_test.go
> @@ -491,6 +491,11 @@ var domainTestData = []struct {
>   },
>   },
>   },
> + Clock: {
> + Offset: "variable",
> + Basic:  "utc",
> + Adjustment: 28794,
> + },
>   },
>   Expected: []string{
>   ``,
> @@ -524,6 +529,7 @@ var domainTestData = []struct {
>   `--unit`,
>   `emergency.service`,
>   `  `,
> + `   adjustment="28794">`,
>   ``,
>   },
>   },
> -- 
> 2.7.4
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

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 v2] vz: allow to start vz driver without host cache info

[Martin Kletzander]

On Tue, Jul 11, 2017 at 04:59:05AM -0400, Mikhail Feoktistov wrote:

Show warning message instead of fail operation.
It happens if kernel or cpu doesn't support reporting cpu cache info.
In case of Virtuozzo file "id" doesn't exist.


What is your kernel version?

Is there another way of getting the information about the cache ID?
Maybe we need to parse the name of the cache directory 'index2' would be
id 2 maybe?  If there is no other way, then this fix is fine (as most
drivers do the same thing), but I would rather fix it if that's
possible.  Unfortunately the cache information is structured stupidly
compared to other kernel-provided topology-related information.


---
src/vz/vz_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index 6f4aee3..eb97e54 100644
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -119,7 +119,7 @@ vzBuildCapabilities(void)
goto error;

if (virCapabilitiesInitCaches(caps) < 0)
-goto error;
+VIR_WARN("Failed to get host CPU cache info");

verify(ARRAY_CARDINALITY(archs) == ARRAY_CARDINALITY(emulators));

--
1.8.3.1

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


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

Re: [libvirt] [PATCH] util: increase libnl buffer size to 1M

[Daniel P. Berrange]

On Mon, Jul 10, 2017 at 02:51:34PM -0400, John Ferlan wrote:
> 
> 
> On 06/29/2017 02:05 PM, ZhiPeng Lu wrote:
> > nl_recv() returns the error "No buffer space available"
> > when using virsh destroy domain with 240 or more
> > passhthrough network interfaces.
> 
> pass-through
> 
> > The patch increases libnl sock receive buffer size to 1M,
> > and nl_recv() doesn't return error when destroying domain
> > with 512 network interfaces.
> > 
> > Signed-off-by: ZhiPeng Lu 
> > ---
> >  src/util/virnetlink.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> 
> This feels like something that perhaps should be configurable - that is
> some /etc/libvirt/libvirtd.conf variable; otherwise, we'll keep hitting
> some conflated maximum based on the size of something.

1 MB matches what systemed/udevd uses, so if we hit that limit, then  the
system as a whole is going to struggle already. So I don't think we need
make it configurable.


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

[libvirt] [PATCH] virFileInData: Report an error if unable to reposition file

[Michal Privoznik]

The purpose of this function is to tell if the current position
in given FD is in data section or a hole and how much bytes there
is remaining until the end of the section. This is achieved by
couple of lseeks(). The most important part is that we reposition
the FD back, so that the position is unchanged from the caller
POV. And until now the final lseek() back to the original
position was done with no check for errors. And I was convinced
that that's okay since nothing can go wrong. However, John
persuaded me, that it's better to be safe than sorry. Therefore,
lets check if the final lseek() succeeded and if it doesn't
report an error.

Signed-off-by: Michal Privoznik 
---
 src/util/virfile.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/util/virfile.c b/src/util/virfile.c
index d444b32f8..2f28e83f4 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -3900,8 +3900,12 @@ virFileInData(int fd,
 ret = 0;
  cleanup:
 /* At any rate, reposition back to where we started. */
-if (cur != (off_t) -1)
-ignore_value(lseek(fd, cur, SEEK_SET));
+if (cur != (off_t) -1 &&
+lseek(fd, cur, SEEK_SET) == (off_t) -1) {
+virReportSystemError(errno, "%s",
+ _("unable to restore position in file"));
+ret = -1;
+}
 return ret;
 }
 
-- 
2.13.0

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

[libvirt] [PATCH v2] vz: allow to start vz driver without host cache info

[Mikhail Feoktistov]

Show warning message instead of fail operation.
It happens if kernel or cpu doesn't support reporting cpu cache info.
In case of Virtuozzo file "id" doesn't exist.
---
 src/vz/vz_driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index 6f4aee3..eb97e54 100644
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -119,7 +119,7 @@ vzBuildCapabilities(void)
 goto error;
 
 if (virCapabilitiesInitCaches(caps) < 0)
-goto error;
+VIR_WARN("Failed to get host CPU cache info");
 
 verify(ARRAY_CARDINALITY(archs) == ARRAY_CARDINALITY(emulators));
 
-- 
1.8.3.1

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

Re: [libvirt] [PATCH 03/19] test: Cleanup exit/failure paths of some storage pool APIs

[Pavel Hrdina]

On Tue, May 09, 2017 at 11:30:10AM -0400, John Ferlan wrote:
> Rework some of the test driver API's to remove the need to return failure
> when testStoragePoolObjFindByName returns NULL rather than going to cleanup.
> This removes the need for check for "if (obj)" and in some instances the
> need to for a cleanup label and a local ret variable.

The recommended wrapping of commit message body is 72 columns.

> Signed-off-by: John Ferlan 
> ---
>  src/test/test_driver.c | 49 -
>  1 file changed, 16 insertions(+), 33 deletions(-)

Reviewed-by: Pavel Hrdina 


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

Re: [libvirt] [PATCH] vz: allow to start vz driver without host cache info

[Peter Krempa]

On Tue, Jul 11, 2017 at 10:41:15 +0300, Mikhail Feoktistov wrote:
> Error occurs in src/conf/capabilities.c line 1736
> 
> int virCapabilitiesInitCaches(virCapsPtr caps)
> ...
>if (virFileReadValueUint(>id,
> "%s/cpu/cpu%zd/cache/%s/id",
>  SYSFS_SYSTEM_PATH, pos,
> ent->d_name) < 0)
>   goto cleanup;
> ...
> 
> file "id" doesn't exist
> 
> ls -al /sys/devices/system/cpu/cpu0/cache/index0/
> total 0
> drwxr-xr-x 2 root root0 Jun 30 06:03 .
> drwxr-xr-x 6 root root0 Jun 29 04:42 ..
> -r--r--r-- 1 root root 4096 Jul 10 06:44 coherency_line_size
> -r--r--r-- 1 root root 4096 Jul  5 11:33 level
> -r--r--r-- 1 root root 4096 Jul 10 06:44 number_of_sets
> -r--r--r-- 1 root root 4096 Jul 10 06:44 physical_line_partition
> -r--r--r-- 1 root root 4096 Jul 10 06:44 shared_cpu_list
> -r--r--r-- 1 root root 4096 Jul 10 06:44 shared_cpu_map
> -r--r--r-- 1 root root 4096 Jul 10 06:44 size
> -r--r--r-- 1 root root 4096 Jul 10 06:44 type
> -r--r--r-- 1 root root 4096 Jul 10 06:44 ways_of_associativity

So your system (cpu/kernel) does not support reporting the cache data
required to populate the capabilities.

So the fix is correct, since other callers of virCapabilitiesInitCaches
ignore the error as well.

Please put the justification into the commit message as it's usually
done in commits to libvirt.

> On 10.07.2017 15:48, Peter Krempa wrote:
> > On Mon, Jul 10, 2017 at 07:54:03 -0400, Mikhail Feoktistov wrote:
> > 
> > Any justification or explanation when this happens?
> > 
> > > ---
> > >   src/vz/vz_driver.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list


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

Re: [libvirt] [PATCH] autogen.sh: tell user the correct make command

[Martin Kletzander]

On Wed, Jul 05, 2017 at 06:32:07PM +0200, Andrea Bolognani wrote:

On Tue, 2017-07-04 at 16:22 +0100, Daniel P. Berrange wrote:

When autogen.sh finishes it helpfully prints
 
   "Now type 'make' to compile libvirt."
 
which is fine if on a host with GNU make, but on *BSD running
'make' will end in tears. We should tell users to run 'gmake'
on these platforms. If 'gmake' doesn't exist then we should
report an error too
 
   "GNU make is required to build libvirt"
 
Signed-off-by: Daniel P. Berrange 
---
  autogen.sh | 19 ++-
  1 file changed, 18 insertions(+), 1 deletion(-)
 
diff --git a/autogen.sh b/autogen.sh
index d5d836a..1e99ce8 100755
--- a/autogen.sh
+++ b/autogen.sh
@@ -193,4 +193,21 @@ else
  fi
  
  echo


I'd move this 'echo' to the bottom as well.


-echo "Now type 'make' to compile libvirt."
+
+# Make sure we can find GNU make and tell the user
+# the right command to run
+make -v | grep "GNU Make" 1>/dev/null 2>&1


This doesn't catch stderr for the make invocation, and
FreeBSD's make doesn't support the -v flag so you'll
end up with a bunch of spurious output. You can use

  make -v 2>&1 | grep -q "GNU Make"

instead.


+if test $? = 0


You're not using $? for anything else, so you can just
have the command above as condition for 'if'.


+then


The rest of the file puts 'then' on the same line as
'if', please keep it consistent.


+MAKE=make
+else
+which gmake 1>/dev/null 2>&1


also I believe `which` is not POSIX-guaranteed, you should use `type` instead.


+if test $? = 0
+then


Same comments as above. Additionally, you don't need
to mention fd 1 explicitly when redirecting stdout,
just '>/dev/null' is enough and looks less weird.

-- 
Andrea Bolognani / Red Hat / Virtualization

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


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

Re: [libvirt] [PATCH 02/19] test: Use consistent variable names for storage test driver APIs

[Pavel Hrdina]

On Tue, May 09, 2017 at 11:30:09AM -0400, John Ferlan wrote:
> A virStoragePoolObjPtr will be an 'obj'.
> 
> A virStoragePoolPtr will be a 'pool'.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/test/test_driver.c | 443 
> -
>  1 file changed, 219 insertions(+), 224 deletions(-)
> 
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 548f318..c0e46af 100644
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c

[...]

> @@ -4057,18 +4056,18 @@ testStoragePoolLookupByUUID(virConnectPtr conn,
>  const unsigned char *uuid)
>  {
>  testDriverPtr privconn = conn->privateData;
> -virStoragePoolObjPtr pool;
> +virStoragePoolObjPtr obj;
>  virStoragePoolPtr ret = NULL;

There you didn't changed the "ret" to "pool".

> -if (!(pool = testStoragePoolObjFindByUUID(privconn, uuid)))
> +if (!(obj = testStoragePoolObjFindByUUID(privconn, uuid)))
>  goto cleanup;
>  
> -ret = virGetStoragePool(conn, pool->def->name, pool->def->uuid,
> +ret = virGetStoragePool(conn, obj->def->name, obj->def->uuid,
>  NULL, NULL);
>  
>   cleanup:
> -if (pool)
> -virStoragePoolObjUnlock(pool);
> +if (obj)
> +virStoragePoolObjUnlock(obj);
>  return ret;
>  }
>  
> @@ -4078,18 +4077,18 @@ testStoragePoolLookupByName(virConnectPtr conn,
>  const char *name)
>  {
>  testDriverPtr privconn = conn->privateData;
> -virStoragePoolObjPtr pool;
> +virStoragePoolObjPtr obj;
>  virStoragePoolPtr ret = NULL;

Same here.

> -if (!(pool = testStoragePoolObjFindByName(privconn, name)))
> +if (!(obj = testStoragePoolObjFindByName(privconn, name)))
>  goto cleanup;
>  
> -ret = virGetStoragePool(conn, pool->def->name, pool->def->uuid,
> +ret = virGetStoragePool(conn, obj->def->name, obj->def->uuid,
>  NULL, NULL);
>  
>   cleanup:
> -if (pool)
> -virStoragePoolObjUnlock(pool);
> +if (obj)
> +virStoragePoolObjUnlock(obj);
>  return ret;
>  }
>  

[...]

> @@ -4345,7 +4344,7 @@ testStoragePoolCreateXML(virConnectPtr conn,
>  {
>  testDriverPtr privconn = conn->privateData;
>  virStoragePoolDefPtr def;
> -virStoragePoolObjPtr pool = NULL;
> +virStoragePoolObjPtr obj = NULL;
>  virStoragePoolPtr ret = NULL;

And here.

>  virObjectEventPtr event = NULL;
>  

[...]

> @@ -4419,7 +4418,7 @@ testStoragePoolDefineXML(virConnectPtr conn,
>  {
>  testDriverPtr privconn = conn->privateData;
>  virStoragePoolDefPtr def;
> -virStoragePoolObjPtr pool = NULL;
> +virStoragePoolObjPtr obj = NULL;
>  virStoragePoolPtr ret = NULL;

And here

>  virObjectEventPtr event = NULL;
>

[...]

I don't like these type of patches.  The value to noise ration is poor.
I'm hesitating to give this patch an ACK even though I probably done
that in the past.

Pavel


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

Re: [libvirt] [Qemu-devel] change x86 default machine type to Q35?

[Gerd Hoffmann]

  Hi,

> Concerning QEMU, could we maybe simply emit a warning a la
> 
>  "you did not specify a machine type with the -M option, so you are
>   currently running the the 'pc' machine type. Please note that
> future
>   versions of QEMU might use the 'q35' machine type instead. If you
>   require the 'pc' machine type for your setting, then please specify
>   it with the -M option."

Warnings tend to get ignored until things are actually break, so I
don't think this helps much.  I think simply not having a default
machine type (as already suggested elsewhere in this thread) is the
best way to deal with this.  That way we don't silently change
behavior.  It also is in line with what we have on arm where we already
require the user to explicitly pick a machine type.

We probably want a more verbose error message on x86 though, suggesting
to pick 'pc' for compatibility with old qemu versions and for old
guests, and q35 otherwise.

cheers,
  Gerd

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

Re: [libvirt] [Qemu-devel] change x86 default machine type to Q35?

[Marcel Apfelbaum]

On 11/07/2017 10:48, Thomas Huth wrote:

On 10.07.2017 19:47, Eduardo Habkost wrote:

(CCing libvir-list)

On Mon, Jul 10, 2017 at 07:45:54PM +0300, Michael S. Tsirkin wrote:

On Mon, Jul 10, 2017 at 10:59:43AM -0300, Eduardo Habkost wrote:

On Mon, Jul 10, 2017 at 10:42:26AM +0300, Marcel Apfelbaum wrote:

On 07/07/2017 21:03, Eduardo Habkost wrote:

On Fri, Jul 07, 2017 at 06:17:57PM +0300, Michael S. Tsirkin wrote:

On Fri, Jul 07, 2017 at 10:39:49AM -0300, Eduardo Habkost wrote:

On Wed, Jul 05, 2017 at 12:32:10PM +0300, Marcel Apfelbaum wrote:

[...]


The upper layers should manage the defaults by themselves so
are not supposed to be affected.


But they would be.  libvirt uses the default machine-type from
QEMU.


How about extending the command for supported machines with a
recommended machine type, and teaching libvirt to use that?


I don't think QEMU has enough information to decide if it should
recommend "q35" or "pc".


We don't really need a complicated rule set, we would just recommend q35
by default. Libvirt will try to create the default machine and if fails
for some reason (what would it be?) it can switch to PC.

The advanced logic would be "old systems should use PC", where old
means Windows XP and before and so on. But this logic should appear
in management layers above.


In this case, is there any difference between "changing the
default to q35" and "recommending q35", for libvirt users?

--
Eduardo


No but libvirt users do not manage e.g. pci slots manually.
They do not use the -cdrom flag.
Etc.
So changing the default is unlikely to break things for them.



I see.  If this part is really true (can libvirt developers
confirm that?), then the proposed end result makes sense (not
having a default for running QEMU directly, but changing default
to "q35" for people using libvirt).

But I don't see why we would need a new mechanism to make QEMU
recommend a machine-type for libvirt, if libvirt could simply
choose its own default (or maybe also refuse to pick a default,
if libvirt developers decide that's the best solution).




Hi Thomas,


Agreed, it does not make much sense to invent a new mechanism here. I
guess the default should rather be switched in the the tools that create
the XML for libvirt, i.e. virt-install and friends?

Concerning QEMU, could we maybe simply emit a warning a la

  "you did not specify a machine type with the -M option, so you are
   currently running the the 'pc' machine type. Please note that future
   versions of QEMU might use the 'q35' machine type instead. If you
   require the 'pc' machine type for your setting, then please specify
   it with the -M option."

for a couple of releases, so that other people have a chance to update
their scripts, and then finally switch to q35 ?



Sounds like a plan, adding Laine (libvirt) to confirm (or not)
if makes sense.

Is a pity to loose the "QEMU 3.0" release, but is nicer indeed
to let people know in advance.

Thanks,
Marcel


  Thomas



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

Re: [libvirt] [PATCH v4 5/6] virFileInData: preserve errno in cleanup path

[Michal Privoznik]

On 07/08/2017 02:52 PM, John Ferlan wrote:
> 
> 
> On 06/22/2017 08:30 AM, Michal Privoznik wrote:
>> Callers might be interested in the original value of errno. Let's
>> not overwrite it with lseek() done in cleanup path.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  src/util/virfile.c | 5 -
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
> 
> Looks like I did a lot of writing in my response to the previous
> version.  Still not clear what value this is as it's been pointed out
> previously that lseek shouldn't fail and it had to have succeeded at
> least once (since "cur != -1") and maybe failed at least once (each of
> those getting a ReportSystemError).
> 
> So what value is there in saving errno which may not even be used when
> ret = 0? I don't even see it used when ret = -1. The one thing that does
> happen is saving the errmsg from patch 2, but nothing w/ errno.
> 
> IMO, I think failing that last lseek() should cause failure since the
> API hasn't truthfully represented what it does ("Upon its return, the
> position in the @fd is left unchanged,..."). If that last lseek() fails
> and we return 0, then the caller would be assuming we're back at the
> same position we started, but we're not.  But I think I stated that a
> long time ago when the code was first being added and you convinced me
> otherwise!

Okay, I'll post a patch that does that.

Meanwhile I've pushed the rest modulo this patch. Thanks!

Michal

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

Re: [libvirt] [PATCH 01/19] test: Fix up formatting in storage test API's

[Pavel Hrdina]

On Tue, May 09, 2017 at 11:30:08AM -0400, John Ferlan wrote:
> Fix some spacing/formatting in the storage pool/vol test driver code.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/test/test_driver.c | 37 +++--
>  1 file changed, 31 insertions(+), 6 deletions(-)

Reviewed-by: Pavel Hrdina 


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

Re: [libvirt] [PATCH v4 6/6] virStream*All: Report error if a callback fails

[Michal Privoznik]

On 07/08/2017 02:52 PM, John Ferlan wrote:
> 
> 
> On 06/22/2017 08:30 AM, Michal Privoznik wrote:
>> All of these four functions (virStreamRecvAll, virStreamSendAll,
>> virStreamSparseRecvAll, virStreamSparseSendAll) take one or more
>> callback that handle various aspects of streams. However, if any
> 
> (same as previous review)
> 
> s/callback/callback functions/
> 
>> of them fails no error is reported therefore caller does not know
>> what went wrong.
>>
>> At the same time, we silently presumed callbacks to set errno on
>> failure. With this change we should document it explicitly as the
>> error is not properly reported.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>  include/libvirt/libvirt-stream.h | 17 +-
>>  src/libvirt-stream.c | 50 
>> +---
>>  2 files changed, 58 insertions(+), 9 deletions(-)
>>
> 
> I think you could move all those "errno = 0;" settings outside the "for
> (;;) {" loop beginnings. Unless it's felt some called function would
> change errno to something and return 0. They're not wrong where they
> are, so it's your call.

Yeah I can. I just wanted to be safe. What if a function touches errno
(e.g. resets it) without returning error? Yes, such function is flawed,
but it doesn't mean we can't work with it. On the other hand, the worst
thing that can happen is we report EIO instead of real error. Also,
setting errno in each loop is overkill, right? ;-) I'll move the lines
in front of the loops.

> 
> You could also alter each of the new comments requesting the setting of
> errno to something to indicate failure to set errno will cause libvirt
> to default to using EIO.

I'm slightly hesitant to do this. What if we want to change the default
at some point? I'd say leave the comments as they are.

> 
> Reviewed-by: John Ferlan 

Thanks.

Michal

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

Re: [libvirt] [PATCH] vz: allow to start vz driver without host cache info

[Mikhail Feoktistov]

Error occurs in src/conf/capabilities.c line 1736

int virCapabilitiesInitCaches(virCapsPtr caps)
...
   if (virFileReadValueUint(>id,
"%s/cpu/cpu%zd/cache/%s/id",
 SYSFS_SYSTEM_PATH, pos, 
ent->d_name) < 0)

  goto cleanup;
...

file "id" doesn't exist

ls -al /sys/devices/system/cpu/cpu0/cache/index0/
total 0
drwxr-xr-x 2 root root0 Jun 30 06:03 .
drwxr-xr-x 6 root root0 Jun 29 04:42 ..
-r--r--r-- 1 root root 4096 Jul 10 06:44 coherency_line_size
-r--r--r-- 1 root root 4096 Jul  5 11:33 level
-r--r--r-- 1 root root 4096 Jul 10 06:44 number_of_sets
-r--r--r-- 1 root root 4096 Jul 10 06:44 physical_line_partition
-r--r--r-- 1 root root 4096 Jul 10 06:44 shared_cpu_list
-r--r--r-- 1 root root 4096 Jul 10 06:44 shared_cpu_map
-r--r--r-- 1 root root 4096 Jul 10 06:44 size
-r--r--r-- 1 root root 4096 Jul 10 06:44 type
-r--r--r-- 1 root root 4096 Jul 10 06:44 ways_of_associativity

On 10.07.2017 15:48, Peter Krempa wrote:

On Mon, Jul 10, 2017 at 07:54:03 -0400, Mikhail Feoktistov wrote:

Any justification or explanation when this happens?


---
  src/vz/vz_driver.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


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

Re: [libvirt] [PATCH] cpu_x86: Properly disable unknown CPU features

[Jiri Denemark]

On Mon, Jun 19, 2017 at 15:09:04 +0200, Jiri Denemark wrote:
> CPU features unknown to a hypervisor will not be present in dataDisabled
> even though the feature won't naturally be enabled because it is
> unknown. Thus any features we asked for which are not in dataEnabled
> should be considered disabled.
> 
> Signed-off-by: Jiri Denemark 

ping

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

Re: [libvirt] [PATCH] Avoid hidden cgroup mount points

[Martin Kletzander]

On Thu, Jul 06, 2017 at 05:03:31PM +0200, Juan Hernandez wrote:

Currently the scan of the /proc/mounts file used to find cgroup mount
points doesn't take into account that mount points may hidden by other
mount points. For, example in certain Kubernetes environments the
/proc/mounts contains the following lines:

 cgroup /sys/fs/cgroup/net_prio,net_cls cgroup ...
 tmpfs /sys/fs/cgroup tmpfs ...
 cgroup /sys/fs/cgroup/net_cls,net_prio cgroup ...

In this particular environment the first mount point is hidden by the
second one. The correct mount point is the third one, but libvirt will
never process it because it only checks the first mount point for each
controller (net_cls in this case). So libvirt will try to use the first
mount point, which doesn't actually exist, and the complete detection
process will fail.

To avoid that issue this patch changes the virCgroupDetectMountsFromFile
function so that when there are duplicates it takes the information from
the last line in /proc/mounts. This requires removing the previous
explicit condition to skip duplicates, and adding code to free the
memory used by the processing of duplicated lines.

Related-To: https://bugzilla.redhat.com/1468214
Related-To: https://github.com/kubevirt/libvirt/issues/4
Signed-off-by: Juan Hernandez 
---
src/util/vircgroup.c| 23 ++-
tests/vircgroupdata/kubevirt.mounts | 36 
tests/vircgroupdata/kubevirt.parsed | 10 ++
tests/vircgrouptest.c   |  1 +
4 files changed, 61 insertions(+), 9 deletions(-)
create mode 100644 tests/vircgroupdata/kubevirt.mounts
create mode 100644 tests/vircgroupdata/kubevirt.parsed

diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 5aa1db5..41d90e7 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -397,6 +397,7 @@ virCgroupDetectMountsFromFile(virCgroupPtr group,
const char *typestr = virCgroupControllerTypeToString(i);
int typelen = strlen(typestr);
char *tmp = entry.mnt_opts;
+struct virCgroupController *controller = >controllers[i];
while (tmp) {
char *next = strchr(tmp, ',');
int len;
@@ -406,18 +407,21 @@ virCgroupDetectMountsFromFile(virCgroupPtr group,
} else {
len = strlen(tmp);
}
-/* NB, the same controller can appear >1 time in mount list
- * due to bind mounts from one location to another. Pick the
- * first entry only
- */
-if (typelen == len && STREQLEN(typestr, tmp, len) &&
-!group->controllers[i].mountPoint) {
+
+if (typelen == len && STREQLEN(typestr, tmp, len)) {
char *linksrc;
struct stat sb;
char *tmp2;

-if (VIR_STRDUP(group->controllers[i].mountPoint,
-   entry.mnt_dir) < 0)
+/* Note that the lines in /proc/mounts have the same
+ * order than the mount operations, and that there may
+ * be duplicates due to bind mounts. This means
+ * that the same mount point may be processed more than
+ * once. We need to save the results of the last one,
+ * and we need to be careful to release the memory used
+ * by previous processing. */
+VIR_FREE(controller->mountPoint);


You need to free the linkPoint here as well as it is no longer valid if
we are throwing out the previous entry.

There's also pre-existing leak with linksrc in this function.


+if (VIR_STRDUP(controller->mountPoint, entry.mnt_dir) < 0)
goto error;

tmp2 = strrchr(entry.mnt_dir, '/');
@@ -453,7 +457,8 @@ virCgroupDetectMountsFromFile(virCgroupPtr group,
VIR_WARN("Expecting a symlink at %s for controller 
%s",
 linksrc, typestr);
} else {
-group->controllers[i].linkPoint = linksrc;
+VIR_FREE(controller->linkPoint);
+controller->linkPoint = linksrc;
}
}
}
diff --git a/tests/vircgroupdata/kubevirt.mounts 
b/tests/vircgroupdata/kubevirt.mounts
new file mode 100644
index 000..b0d31bb
--- /dev/null
+++ b/tests/vircgroupdata/kubevirt.mounts
@@ -0,0 +1,36 @@
+tmpfs /sys/fs/cgroup tmpfs rw,nosuid,nodev,noexec,relatime,mode=755 0 0
+cgroup /sys/fs/cgroup/systemd cgroup 
rw,nosuid,nodev,noexec,relatime,xattr,release_agent=/usr/lib/systemd/systemd-cgroups-agent,name=systemd
 0 0
+cgroup /sys/fs/cgroup/cpuacct,cpu cgroup 

Re: [libvirt] Potential problem with /proc/mounts items that don't exist inside Kubernetes

[Juan Hernández]

On 07/06/2017 01:07 PM, Juan Hernández wrote:

On 07/06/2017 12:11 PM, Daniel P. Berrange wrote:

On Thu, Jul 06, 2017 at 11:48:43AM +0200, Juan Hernández wrote:

On 07/06/2017 11:33 AM, Daniel P. Berrange wrote:

On Thu, Jul 06, 2017 at 11:26:58AM +0200, Juan Hernández wrote:

On 07/06/2017 11:18 AM, Daniel P. Berrange wrote:

On Thu, Jul 06, 2017 at 11:11:03AM +0200, Juan Hernández wrote:

Hello all,

This is my first mail to this list, so let me introduce myself. 
My name is
Juan Hernandez, and I work in the oVirt team. Currently I am 
experimenting

with the integration between ManageIQ and KubeVirt.

I recently detected a potential issue when running libvirt inside
Kubernetes, as part of KubeVirt. There are entries in 
/proc/mounts that
don't exist, and libvirt can't start virtual machines because of 
that. This
is specific to this enviroment, but I think it may be worth 
addressing it in

libvirt itself. See the following issue for details:

 Libvirt fails when there are hidden cgroup mount points in 
`/proc/mounts`

 https://github.com/kubevirt/libvirt/issues/4

I suggested a possible fix there, which seems simple, but it 
makes all tests
fail. I'd be happy to fix the tests as well, but I would need 
some guidance

on how to do so. Any suggestion is welcome.


The root cause problem will be the code that parse /proc/mounts. 
It needs
to pick the last entry in the mounts file, since the earlier ones 
can be
hidden. For some reason virCgroupDetectMountsFromFile instead 
picks the

first entry, so that function needs updating todo the reverse.



Is the order of /proc/mounts guaranteed? It may be, but I'd suggest 
to not
rely on that. Instead of that libvirt could check if the mount 
point does

actually exist, and skip if it it doesn't. That is the fix I proposed:


The order of /proc/mounts reflects the order in which the mounts were
performed. IOW, later entries will override earlier entries if there
is path overlap.



diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 5aa1db5..021a3f2 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -393,6 +393,14 @@ virCgroupDetectMountsFromFile(virCgroupPtr group,
   if (STRNEQ(entry.mnt_type, "cgroup"))
   continue;

+/* Some mount points in the /proc/mounts file may be
+ * hidden by others, and may not actually exist from
+ * the point of the view of the process, so we need
+ * to skip them.
+ */
+   if (!virFileExists(entry.mnt_dir))
+continue;


This is fragile because it is possible for the mount point to
still exist, but for its contents to have been replaced or
hidden. So we really do want to explicitly take only the last
entry, instead of doing this check.



That makes sense to me. Should I open a BZ to request that change?


Yes, its worth tracking this in BZ.

If you wanted to try to write a patch too, that'd be awesome :-)



I created the following BZ:

   Don't use cgroup mount points from /proc/mounts that are hidden
   https://bugzilla.redhat.com/1468214

I will assign it to myself, and I will try to create a fix. Should I 
fail, I will ask for help.




The patch is here:

  https://www.redhat.com/archives/libvir-list/2017-July/msg00141.html

I'd appreciate any review.




+
   for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) {
   const char *typestr = 
virCgroupControllerTypeToString(i);

   int typelen = strlen(typestr);

That fix makes things work, for it doesn't pass the tests.




Regards,
Daniel





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

Re: [libvirt] [PATCH] news: qemu platform serial devices now use -chardev

[Andrea Bolognani]

On Mon, 2017-07-10 at 17:38 -0400, Cole Robinson wrote:
> Signed-off-by: Cole Robinson 
> ---
>  docs/news.xml | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/docs/news.xml b/docs/news.xml
> index f44d676c1..10faad673 100644
> --- a/docs/news.xml
> +++ b/docs/news.xml
> @@ -42,6 +42,17 @@
>
>  
>  
> +  
> +
> +  qemu: arm/aarch64 serial devices can now use chardev features

I would replace "arm/aarch64" with "platform".

> +
> +
> +  qemu VMs that depend on platform serial devices can now use
> +  qemu's -chardev option, which enables access to advanced features

Please add  around -chardev so that it will look
nicer on the website, and spell out QEMU all caps in
the description.


Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH] qemu: process: Remove unused qemuCaps

[Andrea Bolognani]

On Mon, 2017-07-10 at 17:37 -0400, Cole Robinson wrote:
> After 405c0f07f5

The commit that changed qemuProcessLookupPTYs() is actually
426dc5eb28bade109bf27bdd10d7305a040b4a3e, isn't it?

> qemuCaps is unused here, remove it from the call
> stack

@def has become ATTRIBUTE_UNUSED as of that commit as well,
so you should mention both.


Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

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