[libvirt] [test-API][PATCH] Add connection_security_model test case

2015-03-16 Thread lcheng
The connection_security_model.py uses getSecurityModel() to validate new API 
virNodeGetSecurityModel of libvirt.
---
 cases/linux_domain.conf|   4 ++
 repos/virconn/connection_security_model.py | 101 +
 2 files changed, 105 insertions(+)
 create mode 100644 repos/virconn/connection_security_model.py

diff --git a/cases/linux_domain.conf b/cases/linux_domain.conf
index 903fdb5..a7015f0 100644
--- a/cases/linux_domain.conf
+++ b/cases/linux_domain.conf
@@ -233,6 +233,10 @@ domain:domain_fsthaw
 guestname
 $defaultname
 
+virconn:connection_security_model
+guestname
+$defaultname
+
 domain:destroy
 guestname
 $defaultname
diff --git a/repos/virconn/connection_security_model.py 
b/repos/virconn/connection_security_model.py
new file mode 100644
index 000..b44d78c
--- /dev/null
+++ b/repos/virconn/connection_security_model.py
@@ -0,0 +1,101 @@
+#!/usr/bin/env python
+# To test "getSecurityModel"
+
+import libvirt
+
+from xml.dom import minidom
+from libvirt import libvirtError
+from src import sharedmod
+from utils import utils
+
+required_params = ('guestname',)
+optional_params = {}
+
+def get_security_driver(logger):
+"""get security driver from /etc/libvirt/qemu.conf"""
+
+cmds = "grep \"^security_driver\" /etc/libvirt/qemu.conf"
+(ret, conf) = utils.exec_cmd(cmds, shell=True)
+if ret:
+cmds = "getenforce"
+(ret, policy) = utils.exec_cmd(cmds, shell=True)
+
+if policy[0] == "Disabled":
+return "none"
+else:
+return "selinux"
+
+tmp = conf[0].split(' = ')
+if len(tmp[1].split(', ')) > 1:
+driver = tmp[1].split(', ')
+return (filter(str.isalpha, driver[0]))
+else:
+cmds = "echo '%s' | awk -F '\"' '{print $2}'" % conf[0]
+(ret, driver) = utils.exec_cmd(cmds, shell=True)
+
+if driver[0] == "selinux":
+return "selinux"
+elif driver[0] == "none":
+return "none"
+elif driver[0] == "apparmor":
+return "apparmor"
+elif driver[0] == "stack":
+return "stack"
+else:
+return ""
+
+def get_security_model(logger, domname):
+"""get security model from process"""
+
+PID = "ps aux | grep -v grep | grep %s | awk '{print $2}'" % domname
+ret, pid = utils.exec_cmd(PID, shell=True)
+if ret:
+logger.error("get domain pid failed.")
+return ""
+
+LABEL = "ls -nZd /proc/%s" % pid[0]
+ret, label = utils.exec_cmd(LABEL, shell=True)
+if ret:
+logger.error("get domain process's label failed.")
+return ""
+
+if "system_u:system_r:svirt_t:s0" in label[0]:
+return "selinux"
+else:
+return "none"
+
+def check_security_model(logger, domname, model):
+""" check security model"""
+
+dommodel = get_security_model(logger, domname)
+driver = get_security_driver(logger)
+
+logger.info("domain security model is %s." % dommodel)
+logger.info("get security driver is %s." % driver)
+logger.info("get security model is %s." % model)
+
+if driver == dommodel and dommodel == model:
+return True
+else:
+return False
+
+def connection_security_model(params):
+"""test API for getSecurityModel"""
+
+logger = params['logger']
+domname = params['guestname']
+conn = sharedmod.libvirtobj['conn']
+
+try:
+model = conn.getSecurityModel()
+
+if not check_security_model(logger, domname, model[0]):
+logger.error("Fail : get a error security model.")
+return 1
+else:
+logger.info("Pass : get security model successful.")
+return 0
+except libvirtError, e:
+logger.error("API error message: %s" % e.message)
+return 1
+
-- 
1.8.3.1

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


[libvirt] [PATCH] qemu: fix some wrong indentation or types

2015-03-16 Thread Luyao Huang
---
 src/qemu/qemu_command.c | 23 +++
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index aa7a928..02105c3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1683,8 +1683,8 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def,
  *
  */
 if (nbuses > 0)
-   virDomainPCIAddressBusSetModel(&addrs->buses[0],
-  VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT);
+virDomainPCIAddressBusSetModel(&addrs->buses[0],
+   VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT);
 for (i = 1; i < nbuses; i++) {
 virDomainPCIAddressBusSetModel(&addrs->buses[i],
VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE);
@@ -4761,7 +4761,7 @@ qemuBuildMemoryBackendStr(unsigned long long size,
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("this qemu doesn't support the "
  "memory-backend-ram object"));
-goto cleanup;
+goto cleanup;
 }
 
 /* report back that using the new backend is not necessary to achieve
@@ -4967,7 +4967,7 @@ qemuBuildNicDevStr(virDomainDefPtr def,
 if (qemuBuildDeviceAddressStr(&buf, def, &net->info, qemuCaps) < 0)
 goto error;
 if (qemuBuildRomStr(&buf, &net->info, qemuCaps) < 0)
-   goto error;
+goto error;
 if (bootindex && virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOTINDEX))
 virBufferAsprintf(&buf, ",bootindex=%d", bootindex);
 
@@ -5512,7 +5512,7 @@ qemuBuildPCIHostdevDevStr(virDomainDefPtr def,
 if (qemuBuildDeviceAddressStr(&buf, def, dev->info, qemuCaps) < 0)
 goto error;
 if (qemuBuildRomStr(&buf, dev->info, qemuCaps) < 0)
-   goto error;
+goto error;
 
 if (virBufferCheckError(&buf) < 0)
 goto error;
@@ -5577,8 +5577,7 @@ qemuBuildRedirdevDevStr(virDomainDefPtr def,
 }
 
 virBufferAsprintf(&buf, "usb-redir,chardev=char%s,id=%s",
-  dev->info.alias,
-  dev->info.alias);
+  dev->info.alias, dev->info.alias);
 
 if (redirfilter && redirfilter->nusbdevs) {
 if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_USB_REDIR_FILTER)) {
@@ -9610,7 +9609,7 @@ qemuBuildCommandLine(virConnectPtr conn,
 } else {
 virCommandAddArg(cmd, "-parallel");
 if (!(devstr = qemuBuildChrArgStr(¶llel->source, NULL)))
-  goto error;
+goto error;
 virCommandAddArg(cmd, devstr);
 VIR_FREE(devstr);
 }
@@ -10558,7 +10557,7 @@ qemuBuildSerialChrDeviceStr(char **deviceStr,
 case VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA:
 if (serial->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("no addresses are suported for isa-serial"));
+   _("no addresses are supported for isa-serial"));
 goto error;
 }
 break;
@@ -10611,7 +10610,7 @@ qemuBuildChannelChrDeviceStr(char **deviceStr,
 
 case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO:
 if (!(*deviceStr = qemuBuildVirtioSerialPortDevStr(chr, qemuCaps)))
-goto cleanup;
+goto cleanup;
 break;
 
 case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_NONE:
@@ -11467,7 +11466,7 @@ qemuParseCommandLinePCI(const char *val)
 virDomainHostdevDefPtr def = virDomainHostdevDefAlloc();
 
 if (!def)
-   goto error;
+goto error;
 
 if (!STRPREFIX(val, "host=")) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -11521,7 +11520,7 @@ qemuParseCommandLineUSB(const char *val)
 char *end;
 
 if (!def)
-   goto error;
+goto error;
 usbsrc = &def->source.subsys.u.usb;
 
 if (!STRPREFIX(val, "host:")) {
-- 
1.8.3.1

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


[libvirt] [PATCH v3] qemu: read backing chain names from qemu

2015-03-16 Thread Eric Blake
https://bugzilla.redhat.com/show_bug.cgi?id=1199182 documents that
after a series of disk snapshots into existing destination images,
followed by active commits of the top image, it is possible for
qemu 2.2 and earlier to end up tracking a different name for the
image than what it would have had when opening the chain afresh.
That is, when starting with the chain 'a <- b <- c', the name
associated with 'b' is how it was spelled in the metadata of 'c',
but when starting with 'a', taking two snapshots into 'a <- b <- c',
then committing 'c' back into 'b', the name associated with 'b' is
now the name used when taking the first snapshot.

Sadly, older qemu doesn't know how to treat different spellings of
the same filename as identical files (it uses strcmp() instead of
checking for the same inode), which means libvirt's attempt to
commit an image using solely the names learned from qcow2 metadata
fails with a cryptic:

error: internal error: unable to execute QEMU command 'block-commit': Top image 
file /tmp/images/c/../b/b not found

even though the file exists.  Trying to teach libvirt the rules on
which name qemu will expect is not worth the effort (besides, we'd
have to remember it across libvirtd restarts, and track whether a
file was opened via metadata or via snapshot creation for a given
qemu process); it is easier to just always directly ask qemu what
string it expects to see in the first place.

As a safety valve, we validate that any name returned by qemu
still maps to the same local file as we have tracked it, so that
a compromised qemu cannot accidentally cause us to act on an
incorrect file.

* src/qemu/qemu_monitor.h (qemuMonitorDiskNameLookup): New
prototype.
* src/qemu/qemu_monitor_json.h (qemuMonitorJSONDiskNameLookup):
Likewise.
* src/qemu/qemu_monitor.c (qemuMonitorDiskNameLookup): New function.
* src/qemu/qemu_monitor_json.c (qemuMonitorJSONDiskNameLookup)
(qemuMonitorJSONDiskNameLookupOne): Likewise.
* src/qemu/qemu_driver.c (qemuDomainBlockCommit)
(qemuDomainBlockJobImpl): Use it.

Signed-off-by: Eric Blake 
---

v3: better sanity checks in qemu_monitor_json, rebase
and retested atop Peter's fixes for interlocking block jobs

 src/qemu/qemu_driver.c   |  28 ++--
 src/qemu/qemu_monitor.c  |  20 -
 src/qemu/qemu_monitor.h  |   8 +++-
 src/qemu/qemu_monitor_json.c | 102 ++-
 src/qemu/qemu_monitor_json.h |   9 +++-
 5 files changed, 149 insertions(+), 18 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f4b8dab..9129531 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16283,9 +16283,6 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
 goto endjob;

 if (baseSource) {
-if (qemuGetDriveSourceString(baseSource, NULL, &basePath) < 0)
-goto endjob;
-
 if (flags & VIR_DOMAIN_BLOCK_REBASE_RELATIVE) {
 if (!virQEMUCapsGet(priv->qemuCaps, 
QEMU_CAPS_CHANGE_BACKING_FILE)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
@@ -16323,8 +16320,12 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
 }

 qemuDomainObjEnterMonitor(driver, vm);
-ret = qemuMonitorBlockJob(priv->mon, device, basePath, backingPath,
-  speed, mode, async);
+if (baseSource)
+basePath = qemuMonitorDiskNameLookup(priv->mon, device, disk->src,
+ baseSource);
+if (!baseSource || basePath)
+ret = qemuMonitorBlockJob(priv->mon, device, basePath, backingPath,
+  speed, mode, async);
 if (qemuDomainObjExitMonitor(driver, vm) < 0)
 ret = -1;
 if (ret < 0) {
@@ -17050,12 +17051,6 @@ qemuDomainBlockCommit(virDomainPtr dom,
VIR_DISK_CHAIN_READ_WRITE) < 0))
 goto endjob;

-if (qemuGetDriveSourceString(topSource, NULL, &topPath) < 0)
-goto endjob;
-
-if (qemuGetDriveSourceString(baseSource, NULL, &basePath) < 0)
-goto endjob;
-
 if (flags & VIR_DOMAIN_BLOCK_COMMIT_RELATIVE &&
 topSource != disk->src) {
 if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_CHANGE_BACKING_FILE)) {
@@ -17086,9 +17081,14 @@ qemuDomainBlockCommit(virDomainPtr dom,
 disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT;
 }
 qemuDomainObjEnterMonitor(driver, vm);
-ret = qemuMonitorBlockCommit(priv->mon, device,
- topPath, basePath, backingPath,
- speed);
+basePath = qemuMonitorDiskNameLookup(priv->mon, device, disk->src,
+ baseSource);
+topPath = qemuMonitorDiskNameLookup(priv->mon, device, disk->src,
+topSource);
+if (basePath && topPath)
+ret = qemuMonitorBlockCommit(priv->mon, device,
+ topPath, basePath, backingPath,
+  

Re: [libvirt] [PATCH] libxl: fix regression introduced by commit 4ab8cd77

2015-03-16 Thread Jim Fehlig
Daniel P. Berrange wrote:
> On Fri, Mar 13, 2015 at 06:38:24PM -0600, Jim Fehlig wrote:
>   
>> Commit 4ab8cd77 added a check requiring input devices to have
>> a bus type of VIR_DOMAIN_INPUT_BUS_USB, failing to start the
>> domain otherwise.  But virDomainDefParseXML adds implicit mouse
>> and keyboard if a graphics device is configured.  See calls to
>> virDomainDefMaybeAddInput.
>>
>> The regression is fixed by removing the check requiring USB input
>> devices, and skipping non-USB input devices when populating USB
>> 'usbdevice' in libxl_domain_build_info struct.
>> 
>
>
> So IIUC the problem is that we're adding an mouse + keyboard
> with input bus  == Xen (paravirt) or bus == ps2. (HVM). With
> libxl though, these are implicitly present by default when
> you have graphics enabled, so you don't want libxlMakeDomBuildInfo
> to see these input devices.

Yes.  FWIW, the legacy xen driver behaves similarly.  Only USB input
devices are processed.

>  You only want to be looking at the
> USB input devices, as they're the only ones that neeed extra
> config settings procssed, is that right ?
>   

Correct.

>
>   
>> Signed-off-by: Jim Fehlig 
>> ---
>>  src/libxl/libxl_conf.c | 11 ---
>>  1 file changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>> index 50ef9d8..2b57d0b 100644
>> --- a/src/libxl/libxl_conf.c
>> +++ b/src/libxl/libxl_conf.c
>> @@ -750,13 +750,6 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
>>  libxl_defbool_set(&b_info->u.hvm.sdl.enable, 0);
>>  
>>  if (def->ninputs) {
>> -for (i = 0; i < def->ninputs; i++) {
>> -if (def->inputs[i]->bus != VIR_DOMAIN_INPUT_BUS_USB) {
>> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> -_("libxenlight supports only USB input"));
>> -return -1;
>> -}
>> -}
>>  #ifdef LIBXL_HAVE_BUILDINFO_USBDEVICE_LIST
>>  if (VIR_ALLOC_N(b_info->u.hvm.usbdevice_list, def->ninputs+1) < 
>> 0)
>>  return -1;
>> @@ -769,6 +762,10 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
>>  #endif
>>  for (i = 0; i < def->ninputs; i++) {
>>  char **usbdevice;
>> +
>> +if (def->inputs[i]->bus != VIR_DOMAIN_INPUT_BUS_USB)
>> +continue;
>> +
>>  #ifdef LIBXL_HAVE_BUILDINFO_USBDEVICE_LIST
>>  usbdevice = &b_info->u.hvm.usbdevice_list[i];
>>  #else
>> 
>
> ACK
>   

Thanks!  Will push shortly.

Regards,
Jim

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


Re: [libvirt] [PATCH] libxl: fix regression introduced by commit 4ab8cd77

2015-03-16 Thread Daniel P. Berrange
On Fri, Mar 13, 2015 at 06:38:24PM -0600, Jim Fehlig wrote:
> Commit 4ab8cd77 added a check requiring input devices to have
> a bus type of VIR_DOMAIN_INPUT_BUS_USB, failing to start the
> domain otherwise.  But virDomainDefParseXML adds implicit mouse
> and keyboard if a graphics device is configured.  See calls to
> virDomainDefMaybeAddInput.
> 
> The regression is fixed by removing the check requiring USB input
> devices, and skipping non-USB input devices when populating USB
> 'usbdevice' in libxl_domain_build_info struct.


So IIUC the problem is that we're adding an mouse + keyboard
with input bus  == Xen (paravirt) or bus == ps2. (HVM). With
libxl though, these are implicitly present by default when
you have graphics enabled, so you don't want libxlMakeDomBuildInfo
to see these input devices. You only want to be looking at the
USB input devices, as they're the only ones that neeed extra
config settings procssed, is that right ?


> 
> Signed-off-by: Jim Fehlig 
> ---
>  src/libxl/libxl_conf.c | 11 ---
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 50ef9d8..2b57d0b 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -750,13 +750,6 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
>  libxl_defbool_set(&b_info->u.hvm.sdl.enable, 0);
>  
>  if (def->ninputs) {
> -for (i = 0; i < def->ninputs; i++) {
> -if (def->inputs[i]->bus != VIR_DOMAIN_INPUT_BUS_USB) {
> -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -_("libxenlight supports only USB input"));
> -return -1;
> -}
> -}
>  #ifdef LIBXL_HAVE_BUILDINFO_USBDEVICE_LIST
>  if (VIR_ALLOC_N(b_info->u.hvm.usbdevice_list, def->ninputs+1) < 
> 0)
>  return -1;
> @@ -769,6 +762,10 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
>  #endif
>  for (i = 0; i < def->ninputs; i++) {
>  char **usbdevice;
> +
> +if (def->inputs[i]->bus != VIR_DOMAIN_INPUT_BUS_USB)
> +continue;
> +
>  #ifdef LIBXL_HAVE_BUILDINFO_USBDEVICE_LIST
>  usbdevice = &b_info->u.hvm.usbdevice_list[i];
>  #else

ACK

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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


[libvirt] [PATCH 1/3] networkStateInitialize: Don't lock network driver

2015-03-16 Thread Michal Privoznik
There's no need to lock the network driver, as network driver
initialization is done prior accepting any client. There's nobody
to hop in and do something over partially initialized driver. Nor
qemu driver is doing that.

==30532== Observed (incorrect) order is: acquisition of lock at 0x1439EF50
==30532==at 0x4C31A26: pthread_mutex_lock (in 
/usr/lib64/valgrind/vgpreload_helgrind-amd64-linux.so)
==30532==by 0x5324895: virMutexLock (virthread.c:88)
==30532==by 0x5307E86: virObjectLock (virobject.c:323)
==30532==by 0x5396440: virNetworkObjListForEach (network_conf.c:4511)
==30532==by 0x19B29308: networkStateInitialize (bridge_driver.c:686)
==30532==by 0x53E1CCC: virStateInitialize (libvirt.c:777)
==30532==by 0x11DEB7: daemonRunStateInit (libvirtd.c:906)
==30532==by 0x5324B6A: virThreadHelper (virthread.c:197)
==30532==by 0x4C30456: ??? (in 
/usr/lib64/valgrind/vgpreload_helgrind-amd64-linux.so)
==30532==by 0xA1EC1F2: start_thread (in /lib64/libpthread-2.19.so)
==30532==by 0xA4EDC8C: clone (in /lib64/libc-2.19.so)
==30532==
==30532==  followed by a later acquisition of lock at 0x1439CD60
==30532==at 0x4C31A26: pthread_mutex_lock (in 
/usr/lib64/valgrind/vgpreload_helgrind-amd64-linux.so)
==30532==by 0x5324895: virMutexLock (virthread.c:88)
==30532==by 0x19B27B2C: networkDriverLock (bridge_driver.c:102)
==30532==by 0x19B27B60: networkGetDnsmasqCaps (bridge_driver.c:113)
==30532==by 0x19B2856A: networkUpdateState (bridge_driver.c:389)
==30532==by 0x53963E9: virNetworkObjListForEachHelper (network_conf.c:4488)
==30532==by 0x52E2224: virHashForEach (virhash.c:521)
==30532==by 0x539645B: virNetworkObjListForEach (network_conf.c:4512)
==30532==by 0x19B29308: networkStateInitialize (bridge_driver.c:686)
==30532==by 0x53E1CCC: virStateInitialize (libvirt.c:777)
==30532==by 0x11DEB7: daemonRunStateInit (libvirtd.c:906)
==30532==by 0x5324B6A: virThreadHelper (virthread.c:197)
==30532==
==30532== Required order was established by acquisition of lock at 0x1439CD60
==30532==at 0x4C31A26: pthread_mutex_lock (in 
/usr/lib64/valgrind/vgpreload_helgrind-amd64-linux.so)
==30532==by 0x5324895: virMutexLock (virthread.c:88)
==30532==by 0x19B27B2C: networkDriverLock (bridge_driver.c:102)
==30532==by 0x19B28DF9: networkStateInitialize (bridge_driver.c:609)
==30532==by 0x53E1CCC: virStateInitialize (libvirt.c:777)
==30532==by 0x11DEB7: daemonRunStateInit (libvirtd.c:906)
==30532==by 0x5324B6A: virThreadHelper (virthread.c:197)
==30532==by 0x4C30456: ??? (in 
/usr/lib64/valgrind/vgpreload_helgrind-amd64-linux.so)
==30532==by 0xA1EC1F2: start_thread (in /lib64/libpthread-2.19.so)
==30532==by 0xA4EDC8C: clone (in /lib64/libc-2.19.so)
==30532==
==30532==  followed by a later acquisition of lock at 0x1439EF50
==30532==at 0x4C31A26: pthread_mutex_lock (in 
/usr/lib64/valgrind/vgpreload_helgrind-amd64-linux.so)
==30532==by 0x5324895: virMutexLock (virthread.c:88)
==30532==by 0x5307E86: virObjectLock (virobject.c:323)
==30532==by 0x538A09C: virNetworkAssignDef (network_conf.c:527)
==30532==by 0x5391EB2: virNetworkLoadState (network_conf.c:3008)
==30532==by 0x53922D4: virNetworkLoadAllState (network_conf.c:3128)
==30532==by 0x19B2929A: networkStateInitialize (bridge_driver.c:671)
==30532==by 0x53E1CCC: virStateInitialize (libvirt.c:777)
==30532==by 0x11DEB7: daemonRunStateInit (libvirtd.c:906)
==30532==by 0x5324B6A: virThreadHelper (virthread.c:197)
==30532==by 0x4C30456: ??? (in 
/usr/lib64/valgrind/vgpreload_helgrind-amd64-linux.so)
==30532==by 0xA1EC1F2: start_thread (in /lib64/libpthread-2.19.so)

Signed-off-by: Michal Privoznik 
---
 src/network/bridge_driver.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index a007388..b3dcadc 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -606,7 +606,6 @@ networkStateInitialize(bool privileged,
 VIR_FREE(network_driver);
 goto error;
 }
-networkDriverLock(network_driver);
 
 /* configuration/state paths are one of
  * ~/.config/libvirt/... (session/unprivileged)
@@ -677,8 +676,6 @@ networkStateInitialize(bool privileged,
  network_driver->networkAutostartDir) < 0)
 goto error;
 
-networkDriverUnlock(network_driver);
-
 /* Update the internal status of all allegedly active
  * networks according to external conditions on the host
  * (i.e. anything that isn't stored directly in each
-- 
2.0.5

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


[libvirt] [PATCH 2/3] objecteventtest: Check for virNetwork* return values

2015-03-16 Thread Michal Privoznik
Lets not give a bad example and check for return values of
virNetwork* APIs called within the test. Even though it's
unlikely that any API will fail, it can happen. We're connected
to the test driver after all, and our API sequence is correct. So
test driver should fail only in case of bug or OOM.

Signed-off-by: Michal Privoznik 
---
 tests/objecteventtest.c | 29 ++---
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/tests/objecteventtest.c b/tests/objecteventtest.c
index 052dbe5..95fbfec 100644
--- a/tests/objecteventtest.c
+++ b/tests/objecteventtest.c
@@ -417,7 +417,7 @@ testNetworkCreateXML(const void *data)
&counter, NULL);
 net = virNetworkCreateXML(test->conn, networkDef);
 
-if (virEventRunDefaultImpl() < 0) {
+if (!net || virEventRunDefaultImpl() < 0) {
 ret = -1;
 goto cleanup;
 }
@@ -429,10 +429,10 @@ testNetworkCreateXML(const void *data)
 
  cleanup:
 virConnectNetworkEventDeregisterAny(test->conn, id);
-virNetworkDestroy(net);
-
-virNetworkFree(net);
-
+if (net) {
+virNetworkDestroy(net);
+virNetworkFree(net);
+}
 return ret;
 }
 
@@ -455,7 +455,7 @@ testNetworkDefine(const void *data)
 /* Make sure the define event is triggered */
 net = virNetworkDefineXML(test->conn, networkDef);
 
-if (virEventRunDefaultImpl() < 0) {
+if (!net || virEventRunDefaultImpl() < 0) {
 ret = -1;
 goto cleanup;
 }
@@ -481,7 +481,8 @@ testNetworkDefine(const void *data)
 
  cleanup:
 virConnectNetworkEventDeregisterAny(test->conn, id);
-virNetworkFree(net);
+if (net)
+virNetworkFree(net);
 
 return ret;
 }
@@ -494,6 +495,9 @@ testNetworkStartStopEvent(const void *data)
 int id;
 int ret = 0;
 
+if (!test->net)
+return -1;
+
 lifecycleEventCounter_reset(&counter);
 
 id = virConnectNetworkEventRegisterAny(test->conn, test->net,
@@ -509,7 +513,7 @@ testNetworkStartStopEvent(const void *data)
 }
 
 if (counter.startEvents != 1 || counter.stopEvents != 1 ||
-counter.unexpectedEvents > 0) {
+counter.unexpectedEvents > 0) {
 ret = -1;
 goto cleanup;
 }
@@ -567,13 +571,16 @@ mymain(void)
 ret = EXIT_FAILURE;
 
 /* Define a test network */
-test.net = virNetworkDefineXML(test.conn, networkDef);
+if (!(test.net = virNetworkDefineXML(test.conn, networkDef)))
+ret = EXIT_FAILURE;
 if (virtTestRun("Network start stop events ", testNetworkStartStopEvent, 
&test) < 0)
 ret = EXIT_FAILURE;
 
 /* Cleanup */
-virNetworkUndefine(test.net);
-virNetworkFree(test.net);
+if (test.net) {
+virNetworkUndefine(test.net);
+virNetworkFree(test.net);
+}
 virConnectClose(test.conn);
 virEventRemoveTimeout(timer);
 
-- 
2.0.5

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


[libvirt] [PATCH 3/3] network_conf: Drop virNetworkObjIsDuplicate

2015-03-16 Thread Michal Privoznik
This function does not make any sense now, that network driver is
(almost) dropped. I mean, previously, when threads were
serialized, this function was there to check, if no other network
with the same name or UUID exists. However, nowadays that threads
can run more in parallel, this function is useless, in fact it
gives misleading return values. Consider the following scenario.
Two threads, both trying to define networks with same name but
different UUID (e.g. because it was generated during XML parsing
phase, whatever). Lets assume that both threads are about to call
networkValidate() which immediately calls
virNetworkObjIsDuplicate().

T1: calls virNetworkObjIsDuplicate() and since no network with
given name or UUID exist, success is returned.
T2: calls virNetworkObjIsDuplicate() and since no network with
given name or UUID exist, success is returned.

T1: calls virNetworkAssignDef() and successfully places its
network into the virNetworkObjList.
T2: calls virNetworkAssignDef() and since network with the same
name exists, the network definition is replaced.

Okay, this is mainly because virNetworkAssignDef() does not check
whether name and UUID matches. Well, lets make it so! And drop
useless function too.

Signed-off-by: Michal Privoznik 
---
 src/conf/network_conf.c   | 171 ++
 src/conf/network_conf.h   |  10 +--
 src/libvirt_private.syms  |   1 -
 src/network/bridge_driver.c   |  17 ++--
 src/parallels/parallels_network.c |   4 +-
 src/test/test_driver.c|  10 ++-
 6 files changed, 99 insertions(+), 114 deletions(-)

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index d7c5dec..1fb06ef 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -504,6 +504,81 @@ virNetworkObjAssignDef(virNetworkObjPtr network,
 }
 
 /*
+ * If flags & VIR_NETWORK_OBJ_LIST_ADD_CHECK_LIVE then this will
+ * refuse updating an existing def if the current def is Live
+ *
+ * If flags & VIR_NETWORK_OBJ_LIST_ADD_LIVE then the @def being
+ * added is assumed to represent a live config, not a future
+ * inactive config
+ */
+static virNetworkObjPtr
+virNetworkAssignDefLocked(virNetworkObjListPtr nets,
+  virNetworkDefPtr def,
+  unsigned int flags)
+{
+virNetworkObjPtr network;
+virNetworkObjPtr ret = NULL;
+char uuidstr[VIR_UUID_STRING_BUFLEN];
+
+/* See if a network with matching UUID already exists */
+if ((network = virNetworkObjFindByUUIDLocked(nets, def->uuid))) {
+virObjectLock(network);
+/* UUID matches, but if names don't match, refuse it */
+if (STRNEQ(network->def->name, def->name)) {
+virUUIDFormat(network->def->uuid, uuidstr);
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _("network '%s' is already defined with uuid %s"),
+   network->def->name, uuidstr);
+goto cleanup;
+}
+
+if (flags & VIR_NETWORK_OBJ_LIST_ADD_CHECK_LIVE) {
+/* UUID & name match, but if network is already active, refuse it 
*/
+if (virNetworkObjIsActive(network)) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+   _("network is already active as '%s'"),
+   network->def->name);
+goto cleanup;
+}
+}
+
+virNetworkObjAssignDef(network,
+   def,
+   !!(flags & VIR_NETWORK_OBJ_LIST_ADD_LIVE));
+} else {
+/* UUID does not match, but if a name matches, refuse it */
+if ((network = virNetworkObjFindByNameLocked(nets, def->name))) {
+virObjectLock(network);
+virUUIDFormat(network->def->uuid, uuidstr);
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _("network '%s' already exists with uuid %s"),
+   def->name, uuidstr);
+goto cleanup;
+}
+
+if (!(network = virNetworkObjNew()))
+  goto cleanup;
+
+virObjectLock(network);
+
+virUUIDFormat(def->uuid, uuidstr);
+if (virHashAddEntry(nets->objs, uuidstr, network) < 0)
+goto cleanup;
+
+network->def = def;
+network->persistent = !(flags & VIR_NETWORK_OBJ_LIST_ADD_LIVE);
+virObjectRef(network);
+}
+
+ret = network;
+network = NULL;
+
+ cleanup:
+virNetworkObjEndAPI(&network);
+return ret;
+}
+
+/*
  * virNetworkAssignDef:
  * @nets: list of all networks
  * @def: the new NetworkDef (will be consumed by this function iff successful)
@@ -519,40 +594,14 @@ virNetworkObjAssignDef(virNetworkObjPtr network,
 virNetworkObjPtr
 virNetworkAssignDef(virNetworkObjListPtr nets,
 virNetworkDefPtr def,
-bool live)
+unsigned int flags)
 {
 virNetworkObjPtr network;
-   

[libvirt] [PATCH 0/3] Yet a couple of network cleanups

2015-03-16 Thread Michal Privoznik
I've noticed these while testing the code.

Michal Privoznik (3):
  networkStateInitialize: Don't lock network driver
  objecteventtest: Check for virNetwork* return values
  network_conf: Drop virNetworkObjIsDuplicate

 src/conf/network_conf.c   | 171 ++
 src/conf/network_conf.h   |  10 +--
 src/libvirt_private.syms  |   1 -
 src/network/bridge_driver.c   |  20 ++---
 src/parallels/parallels_network.c |   4 +-
 src/test/test_driver.c|  10 ++-
 tests/objecteventtest.c   |  29 ---
 7 files changed, 117 insertions(+), 128 deletions(-)

-- 
2.0.5

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


Re: [libvirt] [PATCH] qemu: block-commit: Mark disk in block jobs only on successful command

2015-03-16 Thread Peter Krempa
On Mon, Mar 16, 2015 at 10:09:22 -0600, Eric Blake wrote:
> On 03/16/2015 09:55 AM, Peter Krempa wrote:
> > Patch 51f9f03a4ca50b070c0fbfb29748d49f583e15e1 introduces a regression
> > where if a blockCommit operation fails the disk is still marked as being
> > part of a block job but can't be unmarked later.
> > ---
> >  src/qemu/qemu_driver.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> ACK. Serves me right for reviewing but not thoroughly testing the
> earlier patches.

Pushed; Thanks.

Peter


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

Re: [libvirt] [PATCH v2 3/4] qemu: Don't duplicate errors when settings stats period

2015-03-16 Thread Martin Kletzander

On Mon, Mar 16, 2015 at 02:09:39PM +0100, Erik Skultety wrote:



On 03/13/2015 05:17 PM, Martin Kletzander wrote:

In order not to leave old error messages set, this patch refactors the
code so the error is reported only when acted upon.  The only such place
already rewrites any error, so cleaning up all the error reporting in
qemuMonitorSetMemoryStatsPeriod() is enough.

+/**
+ * qemuMonitorSetMemoryStatsPeriod:
+ *
+ * This function sets balloon stats update period.
+ *
+ * Returns 0 on success and -1 on error, but does *not* set an error.
+ */
 int qemuMonitorSetMemoryStatsPeriod(qemuMonitorPtr mon,
 int period)
 {
 int ret = -1;
 VIR_DEBUG("mon=%p period=%d", mon, period);

-if (!mon) {
-virReportError(VIR_ERR_INVALID_ARG, "%s",
-   _("monitor must not be NULL"));
+if (!mon)
 return -1;
-}

-if (!mon->json) {
-virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
-   _("JSON monitor is required"));
+if (!mon->json)
+return -1;
+
+if (period < 0)
 return -1;
-}


Hmm. It is a nice idea, but I guess you might have forgotten to check
the actual return value in qemuProcessStart (there are actually 2
appearances in qemu_process.c) like we do in most cases.
I found a piece of code (see below) where we check this correctly (so
it's great you refactored this, otherwise reporting 2 identical messages
would be sort of confusing)



This function is called from three places.  When starting a domain,
when attaching to a domain and from an API that requests change to
this particular value.  First two calls are intentionally non-fatal
and hence not acted upon, since such minor issue as setting the
statistics gathering period shouldn't make domains non-startable.


src/qemu/qemu_driver.c
r = qemuMonitorSetMemoryStatsPeriod(priv->mon, period);
if (qemuDomainObjExitMonitor(driver, vm) < 0)
   goto endjob;
if (r < 0) {
   virReportError(VIR_ERR_OPERATION_INVALID, "%s",
  _("unable to set balloon driver collection period"));
   goto endjob;


 if (qemuMonitorFindBalloonObjectPath(mon, "/") == 0) {
 ret = qemuMonitorJSONSetMemoryStatsPeriod(mon, mon->balloonpath,
   period);
+
+/*
+ * Most of the calls to this function are supposed to be
+ * non-fatal and the only one that should be fatal wants its
+ * own error message.  More details for debugging will be in
+ * the log file.
+ */
+if (ret < 0)
+virResetLastError();
 }
 mon->ballooninit = true;
 return ret;



Everything else looks fine to me, though I think that other monitor
calls should meet a similar fate sometime in the future.
Erik


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

Re: [libvirt] [PATCH] qemu: block-commit: Mark disk in block jobs only on successful command

2015-03-16 Thread Eric Blake
On 03/16/2015 09:55 AM, Peter Krempa wrote:
> Patch 51f9f03a4ca50b070c0fbfb29748d49f583e15e1 introduces a regression
> where if a blockCommit operation fails the disk is still marked as being
> part of a block job but can't be unmarked later.
> ---
>  src/qemu/qemu_driver.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

ACK. Serves me right for reviewing but not thoroughly testing the
earlier patches.

> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 7ca993d..4b8e104 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -17086,7 +17086,8 @@ qemuDomainBlockCommit(virDomainPtr dom,
>  goto endjob;
>  }
> 
> -disk->blockjob = true;
> +if (ret == 0)
> +disk->blockjob = true;
> 
>  if (mirror) {
>  if (ret == 0) {
> 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



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

Re: [libvirt] [PATCHv2.5 06/10] qemu: migration: Forbid migration with memory modules lacking info

2015-03-16 Thread Peter Krempa
On Fri, Mar 13, 2015 at 10:38:57 -0400, John Ferlan wrote:
> 
> 
> On 03/04/2015 11:24 AM, Peter Krempa wrote:
> > Make sure that libvirt has all vital information needed to reliably
> > represent configuration of guest's memory devices in case of a
> > migration.
> > 
> > This patch forbids migration in case the required slot number and module
> > base address are not present (failed to be loaded from qemu via
> > monitor).
> > ---
> >  src/qemu/qemu_migration.c | 14 ++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > index 20e40aa..a31ce9a 100644
> > --- a/src/qemu/qemu_migration.c
> > +++ b/src/qemu/qemu_migration.c
> > @@ -2016,6 +2016,20 @@ qemuMigrationIsAllowed(virQEMUDriverPtr driver, 
> > virDomainObjPtr vm,
> >  }
> >  }
> > 
> > +/* Verify that memory device config can be transferred reliably */
> > +for (i = 0; i < def->nmems; i++) {
> > +virDomainMemoryDefPtr mem = def->mems[i];
> > +
> > +if (mem->model == VIR_DOMAIN_MEMORY_MODEL_DIMM &&
> > +mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) {
> > +virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> > +   _("domain's dimm info lacks slot ID "
> > + "or base address"));
> > +
> > +return false;
> > +}
> > +}
> > +
> >  return true;
> >  }
> > 
> Would this only be possibly true for an offline migration?  It would
> seem an online/live migration that guest startup and continued running
> of libvirtd would seemingly fill in the values.  Then if libvirtd is
> restarted, the cached copy of the guest with the addresses is read. If
> the qemuProcessAttach code is implemented, then we have an address.

This is to prevent online migration in case where qemu doesn't due to a
failure report the addresses that were used for the module.

> 
> Could this be because we 'ignore' the -2 failures in patch 5?  However,
> if we do, then we've never "really" added support for this
> functionality.  Of course if the target of the migration does have it,
> then there's issues.

The actual hotplug code adds another place that may potentialy fail to
fill the info on a failure but will not inhibit/fail the hotplug
operation as it's improbable that we could recover from that due to the
fact that at the point the device was exposed to the guest.

> 
> While what's being checked is valid and safely protects us against some
> unintended mutilation and thus I'd say ACK for just the safety reasons,
> I'm mostly curious as to the why.  Other checks in the API seem to be
> valid you are not allowed to migrate because we said you couldn't
> migrate with snapshots, block job running non-USB host devices, or with
> a specific CPU feature enabled. But, this seems to be - something went
> wrong and we decided to ignore it, so you cannot migrate this guest. Is
> there "anything" we could do to possible fill in the values so that we
> don't cause failure because of some decision point in libvirt? Of course
> we couldn't have already gone through this in previous reviews, but my
> "short term memory" would have been unplugged ;-)
> 
> John

Peter


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

[libvirt] [PATCH] daemon: avoid memleak when ListAll returns nothing

2015-03-16 Thread Eric Blake
Commit 4f25146 (v1.2.8) managed to silence Coverity, but at the
cost of a memory leak detected by valgrind:
==24129== 40 bytes in 5 blocks are definitely lost in loss record 355 of 637
==24129==at 0x4A08B1C: realloc (in 
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==24129==by 0x5084B8E: virReallocN (viralloc.c:245)
==24129==by 0x514D5AA: virDomainObjListExport (domain_conf.c:22200)
==24129==by 0x201227DB: qemuConnectListAllDomains (qemu_driver.c:18042)
==24129==by 0x51CC1B6: virConnectListAllDomains (libvirt-domain.c:6797)
==24129==by 0x14173D: remoteDispatchConnectListAllDomains (remote.c:1580)
==24129==by 0x121BE1: remoteDispatchConnectListAllDomainsHelper 
(remote_dispatch.h:1072)

In short, every time a client calls a ListAll variant and asks
for the resulting list, but there are 0 elements to return, we
end up leaking the 1-entry array that holds the NULL terminator.

What's worse, a read-only client can access these functions in a
tight loop to cause libvirtd to eventually run out of memory; and
this can be considered a denial of service attack against more
privileged clients.  Thankfully, the leak is so small (8 bytes per
call) that you would already have some other denial of service with
any guest calling the API that frequently, so an out-of-memory
crash is unlikely enough that this did not warrant a CVE.

* daemon/remote.c (remoteDispatchConnectListAllDomains)
(remoteDispatchDomainListAllSnapshots)
(remoteDispatchDomainSnapshotListAllChildren)
(remoteDispatchConnectListAllStoragePools)
(remoteDispatchStoragePoolListAllVolumes)
(remoteDispatchConnectListAllNetworks)
(remoteDispatchConnectListAllInterfaces)
(remoteDispatchConnectListAllNodeDevices)
(remoteDispatchConnectListAllNWFilters)
(remoteDispatchConnectListAllSecrets)
(remoteDispatchNetworkGetDHCPLeases): Plug leak.

Signed-off-by: Eric Blake 
---

I'm pushing this now, and backporting to stable maintenance
branches, based on review on the libvirt-security list (where
we decided it was not severe enough to need a CVE).  I'll be
sending a Libvirt Security Notice about the issue later.

 daemon/remote.c | 55 ++-
 1 file changed, 22 insertions(+), 33 deletions(-)

diff --git a/daemon/remote.c b/daemon/remote.c
index ff64eeb..fc2237d 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -1609,11 +1609,10 @@ remoteDispatchConnectListAllDomains(virNetServerPtr 
server ATTRIBUTE_UNUSED,
  cleanup:
 if (rv < 0)
 virNetMessageSaveError(rerr);
-if (doms && ndomains > 0) {
+if (doms && ndomains > 0)
 for (i = 0; i < ndomains; i++)
 virObjectUnref(doms[i]);
-VIR_FREE(doms);
-}
+VIR_FREE(doms);
 return rv;
 }

@@ -4605,11 +4604,10 @@ remoteDispatchDomainListAllSnapshots(virNetServerPtr 
server ATTRIBUTE_UNUSED,
 if (rv < 0)
 virNetMessageSaveError(rerr);
 virObjectUnref(dom);
-if (snaps && nsnaps > 0) {
+if (snaps && nsnaps > 0)
 for (i = 0; i < nsnaps; i++)
 virObjectUnref(snaps[i]);
-VIR_FREE(snaps);
-}
+VIR_FREE(snaps);
 return rv;
 }

@@ -4674,11 +4672,10 @@ 
remoteDispatchDomainSnapshotListAllChildren(virNetServerPtr server ATTRIBUTE_UNU
 virNetMessageSaveError(rerr);
 virObjectUnref(snapshot);
 virObjectUnref(dom);
-if (snaps && nsnaps > 0) {
+if (snaps && nsnaps > 0)
 for (i = 0; i < nsnaps; i++)
 virObjectUnref(snaps[i]);
-VIR_FREE(snaps);
-}
+VIR_FREE(snaps);
 return rv;
 }

@@ -4733,11 +4730,10 @@ 
remoteDispatchConnectListAllStoragePools(virNetServerPtr server ATTRIBUTE_UNUSED
  cleanup:
 if (rv < 0)
 virNetMessageSaveError(rerr);
-if (pools && npools > 0) {
+if (pools && npools > 0)
 for (i = 0; i < npools; i++)
 virObjectUnref(pools[i]);
-VIR_FREE(pools);
-}
+VIR_FREE(pools);
 return rv;
 }

@@ -4796,11 +4792,10 @@ remoteDispatchStoragePoolListAllVolumes(virNetServerPtr 
server ATTRIBUTE_UNUSED,
  cleanup:
 if (rv < 0)
 virNetMessageSaveError(rerr);
-if (vols && nvols > 0) {
+if (vols && nvols > 0)
 for (i = 0; i < nvols; i++)
 virObjectUnref(vols[i]);
-VIR_FREE(vols);
-}
+VIR_FREE(vols);
 virObjectUnref(pool);
 return rv;
 }
@@ -4856,11 +4851,10 @@ remoteDispatchConnectListAllNetworks(virNetServerPtr 
server ATTRIBUTE_UNUSED,
  cleanup:
 if (rv < 0)
 virNetMessageSaveError(rerr);
-if (nets && nnets > 0) {
+if (nets && nnets > 0)
 for (i = 0; i < nnets; i++)
 virObjectUnref(nets[i]);
-VIR_FREE(nets);
-}
+VIR_FREE(nets);
 return rv;
 }

@@ -4915,11 +4909,10 @@ remoteDispatchConnectListAllInterfaces(virNetServerPtr 
server ATTRIBUTE_UNUSED,
  cleanup:
 if (rv < 0)
 virNetMessageSaveError(rerr);
-if (ifaces && nifaces > 0) {
+if (ifaces && nifaces > 0)
 for (i = 

Re: [libvirt] [PATCH 00/12] More cleanup from IOThreads changes

2015-03-16 Thread John Ferlan


On 03/13/2015 11:11 PM, John Ferlan wrote:
> During the review process a few things were pointed at as perhaps
> needing some adjustments based on what was done for IOThreads.
> Specifically a memory leak in PinVcpuFlags since PinIOThreads was
> just a copy of the Vcpu code and secondarily since the IOThreads
> code "reused" the virDomainVcpuPin* data structures and API's, those
> should change names to be more generic.
> 
> 
> John Ferlan (12):
>   qemu: Fix possible memory leak in qemuDomainPinVcpuFlags
>   Convert virDomainVcpuPinDefPtr to virDomainPinDefPtr
>   Convert virDomainPinDefPtr->vcpuid to virDomainPinDefPtr->id
>   Convert virDomainVcpuPinDefFree to virDomainPinDefFree
>   Convert virDomainVcpuPinDefArrayFree to virDomainPinDefArrayFree
>   Convert virDomainVcpuPinDefCopy into virDomainPinDefCopy
>   Convert virDomainVcpuPinIsDuplicate into virDomainPinIsDuplicate
>   Convert virDomainVcpuPinFindByVcpu into virDomainPinFindByVcpu
>   Replace virDomainVcpuPinAdd with virDomainPinAdd
>   Replace virDomainIOThreadsPinAdd with virDomainPinAdd
>   Replace virDomainVcpuPinDel with virDomainPinDel
>   Remove virDomainIOThreadsPinDel
> 
>  src/conf/domain_conf.c   | 234 
> +--
>  src/conf/domain_conf.h   |  58 +---
>  src/libvirt_private.syms |  16 ++--
>  src/libxl/libxl_domain.c |   2 +-
>  src/libxl/libxl_driver.c |  18 ++--
>  src/qemu/qemu_cgroup.c   |  12 +--
>  src/qemu/qemu_cgroup.h   |   4 +-
>  src/qemu/qemu_driver.c   | 104 +++--
>  src/qemu/qemu_process.c  |  16 ++--
>  src/xen/xend_internal.c  |  10 +-
>  10 files changed, 204 insertions(+), 270 deletions(-)
> 

Cleaned up patch 8 (virDomainVcpuPinFindByVcpu into
virDomainPinFindByVcpu) to be virDomainVcpuPinFindByVcpu into
virDomainPinFind

Adjusted the terse commit comments a bit and pushed

Thanks -

John

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


[libvirt] [PATCH] qemu: block-commit: Mark disk in block jobs only on successful command

2015-03-16 Thread Peter Krempa
Patch 51f9f03a4ca50b070c0fbfb29748d49f583e15e1 introduces a regression
where if a blockCommit operation fails the disk is still marked as being
part of a block job but can't be unmarked later.
---
 src/qemu/qemu_driver.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7ca993d..4b8e104 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17086,7 +17086,8 @@ qemuDomainBlockCommit(virDomainPtr dom,
 goto endjob;
 }

-disk->blockjob = true;
+if (ret == 0)
+disk->blockjob = true;

 if (mirror) {
 if (ret == 0) {
-- 
2.2.2

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


Re: [libvirt] [PATCHv2.5 08/10] qemu: conf: Add support for memory device cold(un)plug

2015-03-16 Thread Peter Krempa
On Fri, Mar 13, 2015 at 10:39:05 -0400, John Ferlan wrote:
> On 03/04/2015 11:24 AM, Peter Krempa wrote:
> > Add a few helpers that allow to operate with memory device definitions
> > on the domain config and use them to implement memory device coldplug in
> > the qemu driver.
> > ---
> >  src/conf/domain_conf.c   | 100 
> > +++
> >  src/conf/domain_conf.h   |  10 +
> >  src/libvirt_private.syms |   4 ++
> >  src/qemu/qemu_driver.c   |  15 ++-
> >  4 files changed, 127 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 4f20aa6..0f6058b 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -12687,6 +12687,106 @@ virDomainRNGRemove(virDomainDefPtr def,
> >  }
> > 
> > 
> > +static int
> > +virDomainMemoryFindByDefInternal(virDomainDefPtr def,
> > + virDomainMemoryDefPtr mem,
> > + bool allowAddressFallback)
> > +{
> > +size_t i;
> > +
> > +for (i = 0; i < def->nmems; i++) {
> > +virDomainMemoryDefPtr tmp = def->mems[i];
> > +
> > +/* address, if present */
> > +if (allowAddressFallback) {
> > +if (tmp->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
> > +continue;
> 
> So this path says - we want address fallback and this device has either
> DIMM or LAST (oy!) as a type, so go to the next one and ignore this one

or any other type as PCI, USB ... basically the check is here as if the
device had any address assigned it should have been rejected already by
previous call to this function with @allowAddressFallback equal to
false.

> 
> > +} else {
> > +if (mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
> > +!virDomainDeviceInfoAddressIsEqual(&tmp->info, &mem->info))
> > +continue;
> 
> This path says - we don't want address fallback and as long as we're
> DIMM or LAST (oy!), then compare

... or any other supported address type which translates to "the address
is set"

> 
> What happens when we add a new address type? It would seem the more
> straightforward way would be
> 
> if (type == DIMM && !Equal)
> if (!allowAddressFallback)
> continue'

No, it would actually make it less straightforward. This code is shared
so it has to be device address type agnostic.

> 
> Otherwise we're falling through to alias, target, etc. checks
> 
> > +}
> > +
> > +/* alias, if present */
> > +if (mem->info.alias &&
> > +STRNEQ_NULLABLE(tmp->info.alias, mem->info.alias))
> > +continue;
> 
> I thought the NULLABLE checks both elems for NULL...

It does indeed, but in this case the logic of the lookup requires the
extra check as if the user doesn't specify the alias of the memory
device to find, we still do might want do find a device definition that
already has the alias filled but the user didn't specify it explicitly.

> 
> > +
> > +/* target info -> always present */
> > +if (tmp->model != mem->model ||
> > +tmp->targetNode != mem->targetNode ||
> > +tmp->size != mem->size)
> > +continue;
> > +
> > +/* source stuff -> match with device */
> > +if (tmp->pagesize != mem->pagesize)
> > +continue;
> > +
> > +if (!virBitmapEqual(tmp->sourceNodes, mem->sourceNodes))
> > +continue;
> > +
> > +break;
> > +}
> > +
> > +if (i == def->nmems)
> > +return -1;
> > +
> > +return i;
> > +}
> > +
> > +
> > +int
> > +virDomainMemoryFindByDef(virDomainDefPtr def,
> > + virDomainMemoryDefPtr mem)
> > +{
> > +return virDomainMemoryFindByDefInternal(def, mem, false);
> > +}
> > +
> > +
> > +int
> > +virDomainMemoryFindInactiveByDef(virDomainDefPtr def,
> > + virDomainMemoryDefPtr mem)
> > +{
> > +int ret;
> > +
> > +if ((ret = virDomainMemoryFindByDefInternal(def, mem, false)) < 0)
> > +ret = virDomainMemoryFindByDefInternal(def, mem, true);
> 
> I would seem Inactive would probably not have the dimm address set, so
> we're incurring a 2X penalty for perhaps no reason... Unless perhaps the
> incoming mem def being checked has an address...

The address will be set if and only if the user provided it when adding
the device. Ohterwise the address will be empty. In that case we want to
find the correct device first and ignore addresses in case the user
provided the XML from the running device but is expecting the inactive
configuration (which does not have an address set) to be removed too.

> 
> That is if my incoming has an address, then don't allow fallback;
> otherwise, well best match.

That exactly wouldn't work in my previous example, where you take the
live XML (with the addres auto-assigned) the code would not match it's
original definition that did not

Re: [libvirt] [PATCHv2.5 09/10] qemu: Implement memory device hotplug

2015-03-16 Thread Peter Krempa
On Fri, Mar 13, 2015 at 20:48:39 -0400, John Ferlan wrote:
> 
> 
> On 03/04/2015 11:25 AM, Peter Krempa wrote:
> > Add code to hot-add memory devices to running qemu instances.
> > ---
> >  src/qemu/qemu_command.c |  4 +--
> >  src/qemu/qemu_command.h | 15 +
> >  src/qemu/qemu_driver.c  |  5 ++-
> >  src/qemu/qemu_hotplug.c | 85 
> > +
> >  src/qemu/qemu_hotplug.h |  3 ++
> >  5 files changed, 109 insertions(+), 3 deletions(-)

...

> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 6b10e96..b917d76 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -7095,8 +7095,11 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm,
> >  dev->data.rng = NULL;
> >  break;
> > 
> > -/*TODO: implement later */
> >  case VIR_DOMAIN_DEVICE_MEMORY:
> > +ret = qemuDomainAttachMemory(driver, vm,
> > + dev->data.memory);
> 
> Shouldn't there be a :
> 
>if (!ret)
> 
> prior to the following line (like the other cases in the switch)

No as qemuDomainAttachMemory always consumes the memory device
definition. I'll add a comment in the next version to clarify that.

> 
> > +dev->data.memory = NULL;
> > +break;
> > 
> >  case VIR_DOMAIN_DEVICE_NONE:
> >  case VIR_DOMAIN_DEVICE_FS:
> > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> > index cb2270e..967b678 100644
> > --- a/src/qemu/qemu_hotplug.c
> > +++ b/src/qemu/qemu_hotplug.c
> > @@ -1672,6 +1672,91 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver,
> >  }
> > 
> > 
> > +int
> > +qemuDomainAttachMemory(virQEMUDriverPtr driver,
> > +   virDomainObjPtr vm,
> > +   virDomainMemoryDefPtr mem)
> > +{
> > +qemuDomainObjPrivatePtr priv = vm->privateData;
> > +virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> > +char *devstr = NULL;
> > +char *objalias = NULL;
> > +const char *backendType;
> > +virJSONValuePtr props = NULL;
> > +int id;
> > +int ret = -1;
> > +
> > +if (virAsprintf(&mem->info.alias, "dimm%zu", vm->def->nmems) < 0)
> > +goto cleanup;
> > +
> > +if (virAsprintf(&objalias, "mem%s", mem->info.alias) < 0)
> > +goto cleanup;
> > +
> > +if (!(devstr = qemuBuildMemoryDeviceStr(mem, priv->qemuCaps)))
> > +goto cleanup;
> > +
> > +qemuDomainMemoryDeviceAlignSize(mem);
> > +
> > +if (qemuBuildMemoryBackendStr(mem->size, mem->pagesize,
> > +  mem->targetNode, mem->sourceNodes, NULL,
> > +  vm->def, priv->qemuCaps, cfg,
> > +  &backendType, &props, true) < 0)
> > +goto cleanup;
> > +
> > +if (virDomainMemoryInsert(vm->def, mem) < 0) {
> > +virJSONValueFree(props);
> > +goto cleanup;
> > +}
> > +
> > +qemuDomainObjEnterMonitor(driver, vm);
> > +if (qemuMonitorAddObject(priv->mon, backendType, objalias, props) < 0)
> 
> I see from the AddObject comments, props is consumed upon success, but
> if we hit the else of mon->json, then we don't free it which we should -
> not related to this particular code, but something that should be fixed...

I'll fixed that as a separate patch that will be part of the next
posting.

> > +goto removedef;
> > +
> > +if (qemuMonitorAddDevice(priv->mon, devstr) < 0) {
> > +virErrorPtr err = virSaveLastError();
> > +ignore_value(qemuMonitorDelObject(priv->mon, objalias));
> > +virSetError(err);
> > +virFreeError(err);
> > +goto removedef;
> > +}
> > +
> > +if (qemuDomainObjExitMonitor(driver, vm) < 0) {
> > +/* we shouldn't touch mem now, as the def might be freed */
> > +mem = NULL;
> > +goto cleanup;
> > +}
> > +
> > +/* mem is consumed by vm->def */
> > +mem = NULL;
> 
> Since both the Exit error path and this path set mem = NULL, why not
> just set it before the Exit and comment that it'll either consumed by
> vm->def on success or might be freed on failure...

It's always consumed, either freed or added to the definition. In some
cases it's necessary to track it separately as the @vm was unlocked
during monitor access.

Peter


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

[libvirt] [PATCH python v10] Expose virDomainInterfacesAddresses to python binding

2015-03-16 Thread Daniel P. Berrange
From: Nehal J Wani 

examples/Makefile.am:
  * Add new file domipaddrs.py

examples/README:
  * Add documentation for the python example

libvirt-override-api.xml:
  * Add new symbol for virDomainInterfacesAddresses

libvirt-override.c:
  * Hand written python api

Example:
  $ python examples/domipaddrs.py qemu:///system f18
Interface  MAC address  Protocol Address
lo 00:00:00:00:00:00ipv4 127.0.0.1/8
lo 00:00:00:00:00:00ipv6 ::1/128
eth3   52:54:00:20:70:3dipv4 192.168.105.240/16
eth3   52:54:00:20:70:3dipv6 fe80::5054:ff:fe20:703d/64
eth2   52:54:00:36:2a:e5N/A  N/A
eth1   52:54:00:b1:70:19ipv4 192.168.105.201/16
eth1   52:54:00:b1:70:19ipv4 192.168.201.195/16
eth1   52:54:00:b1:70:19ipv6 fe80::5054:ff:feb1:7019/64
eth0   52:54:00:2e:45:ceipv4 10.1.33.188/24
eth0   52:54:00:2e:45:ceipv6 2001:db8:0:f101::2/64
eth0   52:54:00:2e:45:ceipv6 fe80::5054:ff:fe2e:45ce/64
---
 MANIFEST.in  |   1 +
 examples/README  |   1 +
 examples/domipaddrs.py   |  57 +
 generator.py |   2 +
 libvirt-override-api.xml |   9 +++-
 libvirt-override.c   | 105 +++
 sanitytest.py|   3 ++
 7 files changed, 177 insertions(+), 1 deletion(-)
 create mode 100755 examples/domipaddrs.py

diff --git a/MANIFEST.in b/MANIFEST.in
index d7bc545..dd05221 100644
--- a/MANIFEST.in
+++ b/MANIFEST.in
@@ -4,6 +4,7 @@ include COPYING
 include COPYING.LESSER
 include ChangeLog
 include examples/consolecallback.py
+include examples/domipaddrs.py
 include examples/dominfo.py
 include examples/domrestore.py
 include examples/domsave.py
diff --git a/examples/README b/examples/README
index 5b5d405..1d4b425 100644
--- a/examples/README
+++ b/examples/README
@@ -11,6 +11,7 @@ domrestore.py - restore domU's from their saved files in a 
directory
 esxlist.py  - list active domains of an VMware ESX host and print some info.
   also demonstrates how to use the libvirt.openAuth() method
 dhcpleases.py - list dhcp leases for a given virtual network
+domipaddrs.py - list IP addresses for guest domains
 
 The XML files in this directory are examples of the XML format that libvirt
 expects, and will have to be adapted for your setup. They are only needed
diff --git a/examples/domipaddrs.py b/examples/domipaddrs.py
new file mode 100755
index 000..d6d5cac
--- /dev/null
+++ b/examples/domipaddrs.py
@@ -0,0 +1,57 @@
+#!/usr/bin/env python
+# domipaddrs - print domain interfaces along with their MAC and IP addresses
+
+import libvirt
+import sys
+
+def usage():
+print "Usage: %s [URI] DOMAIN" % sys.argv[0]
+print "Print domain interfaces along with their MAC and IP 
addresses"
+
+uri = None
+name = None
+args = len(sys.argv)
+
+if args == 2:
+name = sys.argv[1]
+elif args == 3:
+uri = sys.argv[1]
+name = sys.argv[2]
+else:
+usage()
+sys.exit(2)
+
+conn = libvirt.open(uri)
+if conn == None:
+print "Unable to open connection to libvirt"
+sys.exit(1)
+
+try:
+dom = conn.lookupByName(name)
+except libvirt.libvirtError:
+print "Domain %s not found" % name
+sys.exit(0)
+
+ifaces = 
dom.interfaceAddresses(libvirt.VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LEASE);
+if (ifaces == None):
+print "Failed to get domain interfaces"
+sys.exit(0)
+
+print " {0:10} {1:20} {2:12} {3}".format("Interface", "MAC address", 
"Protocol", "Address")
+
+def toIPAddrType(addrType):
+if addrType == libvirt.VIR_IP_ADDR_TYPE_IPV4:
+return "ipv4"
+elif addrType == libvirt.VIR_IP_ADDR_TYPE_IPV6:
+return "ipv6"
+
+for (name, val) in ifaces.iteritems():
+if val['addrs']:
+for addr in val['addrs']:
+   print " {0:10} {1:19}".format(name, val['hwaddr']),
+   print " {0:12} {1}/{2} ".format(toIPAddrType(addr['type']), 
addr['addr'], addr['prefix']),
+   print
+else:
+print " {0:10} {1:19}".format(name, val['hwaddr']),
+print " {0:12} {1}".format("N/A", "N/A"),
+print
diff --git a/generator.py b/generator.py
index df7a74d..8c1c48e 100755
--- a/generator.py
+++ b/generator.py
@@ -483,6 +483,7 @@ skip_impl = (
 'virDomainBlockCopy',
 'virNodeAllocPages',
 'virDomainGetFSInfo',
+'virDomainInterfaceAddresses',
 )
 
 lxc_skip_impl = (
@@ -595,6 +596,7 @@ skip_function = (
 'virDomainStatsRecordListFree', # only useful in C, python uses dict
 'virDomainFSInfoFree', # only useful in C, python code uses list
 'virDomainIOThreadsInfoFree', # only useful in C, python code uses list
+'virDomainInterfaceFree', # only useful in C, python code uses list
 )
 
 lxc_skip_function = (
diff --git a/libvirt-override-api.xml b/libvirt-override-api.xml
index 4660c9f..b197639 100644
--- a/l

Re: [libvirt] [PATCH v10 3/4] domifaddr: Implement the API for qemu

2015-03-16 Thread Daniel P. Berrange
On Mon, Mar 16, 2015 at 08:42:49AM -0400, John Ferlan wrote:
> 
> 
> On 03/11/2015 07:09 AM, Daniel P. Berrange wrote:
> > From: Nehal J Wani 
> > 
> > By querying the qemu guest agent with the QMP command
> > "guest-network-get-interfaces" and converting the received JSON
> > output to structured objects.
> > 
> > Although "ifconfig" is deprecated, IP aliases created by "ifconfig"
> > are supported by this API. The legacy syntax of an IP alias is:
> > ":". Since we want all aliases to be clubbed
> > under parent interface, simply stripping ":" suffices.
> > Note that IP aliases formed by "ip" aren't visible to "ifconfig",
> > and aliases created by "ip" do not have any specific name. But
> > we are lucky, as qemu guest agent detects aliases created by both.
> > 
> > src/qemu/qemu_agent.h:
> >   * Define qemuAgentGetInterfaces
> > 
> > src/qemu/qemu_agent.c:
> >   * Implement qemuAgentGetInterface
> > 
> > src/qemu/qemu_driver.c:
> >   * New function qemuGetDHCPInterfaces
> >   * New function qemuDomainInterfaceAddresses
> > 
> > src/remote_protocol-sructs:
> >   * Define new structs
> > 
> > tests/qemuagenttest.c:
> >   * Add new test: testQemuAgentGetInterfaces
> > Test cases for IP aliases, 0 or multiple ipv4/ipv6 address(es)
> > 
> > Signed-off-by: Nehal J Wani 
> > ---
> >  src/qemu/qemu_agent.c  | 204 
> > +
> >  src/qemu/qemu_agent.h  |   4 +
> >  src/qemu/qemu_driver.c | 175 ++
> >  tests/qemuagenttest.c  | 188 +
> >  4 files changed, 571 insertions(+)
> > 
> 
> ACK - there's a NIT and a comment that follows, I'll let you decide how
> to handle.
> 
> John
> > diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
> > index 5fcc40f..8155006 100644
> > --- a/src/qemu/qemu_agent.c
> > +++ b/src/qemu/qemu_agent.c
> > @@ -1953,3 +1953,207 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, 
> > virDomainFSInfoPtr **info,
> >  virJSONValueFree(reply);
> >  return ret;
> >  }
> > +
> > +/*
> > + * qemuAgentGetInterfaces:
> > + * @mon: Agent monitor
> > + * @ifaces: pointer to an array of pointers pointing to interface objects
> > + *
> > + * Issue guest-network-get-interfaces to guest agent, which returns a
> > + * list of interfaces of a running domain along with their IP and MAC
> > + * addresses.
> > + *
> > + * Returns: number of interfaces on success, -1 on error.
> > + */
> > +int
> > +qemuAgentGetInterfaces(qemuAgentPtr mon,
> > +   virDomainInterfacePtr **ifaces)
> > +{
> > +int ret = -1;
> > +size_t i, j;
> > +int size = -1;
> > +virJSONValuePtr cmd = NULL;
> > +virJSONValuePtr reply = NULL;
> > +virJSONValuePtr ret_array = NULL;
> > +size_t ifaces_count = 0;
> > +size_t addrs_count = 0;
> > +virDomainInterfacePtr *ifaces_ret = NULL;
> > +virHashTablePtr ifaces_store = NULL;
> > +char **ifname = NULL;
> > +
> > +/* Hash table to handle the interface alias */
> > +if (!(ifaces_store = virHashCreate(ifaces_count, NULL))) {
> > +virHashFree(ifaces_store);
> > +return -1;
> > +}
> > +
> > +if (!(cmd = qemuAgentMakeCommand("guest-network-get-interfaces", 
> > NULL)))
> > +goto cleanup;
> > +
> > +if (qemuAgentCommand(mon, cmd, &reply, false, 
> > VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0 ||
> > +qemuAgentCheckError(cmd, reply) < 0) {
> > +goto cleanup;
> > +}
> > +
> > +if (!(ret_array = virJSONValueObjectGet(reply, "return"))) {
> > +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +   _("qemu agent didn't provide 'return' field"));
> > +goto cleanup;
> > +}
> > +
> > +if ((size = virJSONValueArraySize(ret_array)) < 0) {
> > +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +   _("qemu agent didn't return an array of 
> > interfaces"));
> > +goto cleanup;
> > +}
> > +
> > +for (i = 0; i < size; i++) {
> > +virJSONValuePtr tmp_iface = virJSONValueArrayGet(ret_array, i);
> > +virJSONValuePtr ip_addr_arr = NULL;
> > +const char *hwaddr, *ifname_s, *name = NULL;
> > +int ip_addr_arr_size;
> > +virDomainInterfacePtr iface = NULL;
> > +
> > +/* Shouldn't happen but doesn't hurt to check neither */
> > +if (!tmp_iface) {
> > +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +   _("something has went really wrong"));
> > +goto error;
> > +}
> > +
> > +/* interface name is required to be presented */
> > +name = virJSONValueObjectGetString(tmp_iface, "name");
> > +if (!name) {
> > +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +   _("qemu agent didn't provide 'name' field"));
> > +goto error;
> > +}
> > +
> > +/* Handle interface alias (:) */
> > +ifname

Re: [libvirt] [PATCH v10 1/4] domifaddr: Implement the public APIs

2015-03-16 Thread Daniel P. Berrange
On Mon, Mar 16, 2015 at 08:42:18AM -0400, John Ferlan wrote:
> 
> 
> On 03/11/2015 07:09 AM, Daniel P. Berrange wrote:
> > From: Nehal J Wani 
> > 
> > Define helper function virDomainInterfaceFree, which allows
> > the upper layer application to free the domain interface object
> > conveniently.
> > 
> > The API is going to provide multiple methods by flags, e.g.
> >   * Query guest agent
> >   * Parse DHCP lease file
> > 
> > include/libvirt/libvirt-domain.h
> >   * Define virDomainInterfaceAddresses, virDomainInterfaceFree
> >   * Define structs virDomainInterface, virDomainIPAddress
> > 
> > src/driver-hypervisor.h:
> >   * Define domainInterfaceAddresses
> > 
> > src/libvirt-domain.c:
> >   * Implement virDomainInterfaceAddresses
> >   * Implement virDomainInterfaceFree
> > 
> > src/libvirt_public.syms:
> >   * Export the new symbols
> > 
> > Signed-off-by: Nehal J Wani 
> > ---
> >  include/libvirt/libvirt-domain.h |  32 ++
> >  src/driver-hypervisor.h  |   6 ++
> >  src/libvirt-domain.c | 123 
> > +++
> >  src/libvirt_public.syms  |   2 +
> >  4 files changed, 163 insertions(+)
> > 


> > + *  for (i = 0; i < ifaces_count; i++)
> > + *  virDomainInterfaceFree(ifaces[i]);
> > + *  free(ifaces);
> > + *
> > + * Returns the number of interfaces on success, -1 in case of error.
> > + */
> > +int
> > +virDomainInterfaceAddresses(virDomainPtr dom,
> > +virDomainInterfacePtr **ifaces,
> > +unsigned int source,
> > +unsigned int flags)
> > +{
> > +virConnectPtr conn;
> > +
> > +VIR_DOMAIN_DEBUG(dom, "ifaces=%p, source=%d, flags=%x", ifaces, 
> > source, flags);
> > +
> > +virResetLastError();
> > +
> > +conn = dom->conn;
> 
> If 'dom' is NULL, then there's a pointer deref (considering check below)
> 
> > +
> > +if (ifaces)
> > +*ifaces = NULL;
> > +virCheckDomainReturn(dom, -1);
> > +virCheckNonNullArgGoto(ifaces, error);
> > +virCheckReadOnlyGoto(conn->flags, error);
> 
> I think I ran into this when I was adding the libvirt-perl code - since
> this is a get interface - does it really matter if it's readonly?

In this case it matters because we need to block access to the QEMU
guest agent.

> 
> Also, I'd move both conn = dom->conn and "if (ifaces) *ifaces = NULL;"
> to here

I'll just delete the 'conn' var - it is fairly pointless and just causes
problems.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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


Re: [libvirt] [PATCHv3.5 0/4] Automaticaly fill element for NUMA enabled guests

2015-03-16 Thread Peter Krempa
On Thu, Mar 12, 2015 at 10:07:04 -0400, John Ferlan wrote:
> On 03/06/2015 09:40 AM, Peter Krempa wrote:
> > Pavel's series changed few same places thus the previous version no longer 
> > applies.
> > 
> > Peter Krempa (4):
> >   conf: Replace access to def->mem.max_balloon with accessor functions
> >   qemu: command: Add helper to align memory sizes
> >   conf: Automatically use NUMA memory size in case NUMA is enabled
> >   conf: Make specifying  optional
> > 
> >  docs/schemas/domaincommon.rng  | 18 ++---
> >  src/conf/domain_conf.c | 81 
> > +++---
> >  src/conf/domain_conf.h |  4 ++
> >  src/hyperv/hyperv_driver.c |  2 +-
> >  src/libvirt_private.syms   |  3 +
> >  src/libxl/libxl_conf.c |  2 +-
> >  src/libxl/libxl_driver.c   |  8 +--
> >  src/lxc/lxc_cgroup.c   |  2 +-
> >  src/lxc/lxc_driver.c   | 12 ++--
> >  src/lxc/lxc_fuse.c |  4 +-
> >  src/lxc/lxc_native.c   |  4 +-
> >  src/openvz/openvz_driver.c |  2 +-
> >  src/parallels/parallels_driver.c   |  2 +-
> >  src/parallels/parallels_sdk.c  | 12 ++--
> >  src/phyp/phyp_driver.c | 11 +--
> >  src/qemu/qemu_command.c| 23 +++---
> >  src/qemu/qemu_domain.c | 21 ++
> >  src/qemu/qemu_domain.h |  2 +
> >  src/qemu/qemu_driver.c | 21 +++---
> >  src/qemu/qemu_hotplug.c|  8 ++-
> >  src/qemu/qemu_process.c|  2 +-
> >  src/test/test_driver.c |  8 +--
> >  src/uml/uml_driver.c   |  8 +--
> >  src/vbox/vbox_common.c |  4 +-
> >  src/vmware/vmware_driver.c |  2 +-
> >  src/vmx/vmx.c  | 12 ++--
> >  src/xen/xm_internal.c  | 14 ++--
> >  src/xenapi/xenapi_driver.c |  2 +-
> >  src/xenapi/xenapi_utils.c  |  4 +-
> >  src/xenconfig/xen_common.c |  8 ++-
> >  src/xenconfig/xen_sxpr.c   |  9 +--
> >  .../qemuxml2argv-cpu-numa-no-memory-element.args   |  7 ++
> >  .../qemuxml2argv-cpu-numa-no-memory-element.xml| 24 +++
> >  .../qemuxml2argv-minimal-no-memory.xml | 25 +++
> >  .../qemuxml2argv-numatune-memnode.args |  2 +-
> >  tests/qemuxml2argvtest.c   |  2 +
> >  .../qemuxml2xmlout-cpu-numa-no-memory-element.xml  | 28 
> >  tests/qemuxml2xmltest.c|  1 +
> >  38 files changed, 299 insertions(+), 105 deletions(-)
> >  create mode 100644 
> > tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-no-memory-element.args
> >  create mode 100644 
> > tests/qemuxml2argvdata/qemuxml2argv-cpu-numa-no-memory-element.xml
> >  create mode 100644 
> > tests/qemuxml2argvdata/qemuxml2argv-minimal-no-memory.xml
> >  create mode 100644 
> > tests/qemuxml2xmloutdata/qemuxml2xmlout-cpu-numa-no-memory-element.xml
> > 
> 
> Since this is the order of your repo on git://pipo.sk/pipo/libvirt.git,
> I'll start with these.
> 
> Couple of general comments...
> 
>  - I think the new function names are better
> 
>  - I think one can tell that the "KiB" or "MiB" was realized at some
> point in time after "KB" and "MB" - as there's stray comments and
> variables using the (I guess) now incorrect terminology.  Not that I'm
> asking for them to be changed, just an "observation".
> 
>  - Whether they snuck in after you started this - there are a few direct
> references to mem.max_balloon in the bhyve_command.c.  Should they be
> replaced by the new accessors?

I actually missed fixing the bhyve driver. I've done it now. 

> 
>  - Because it became apparent in patch 4... In virDomainDefParseXML,
> just to be "complete" shouldn't the:
> 
> /* Extract domain memory */
> if (virDomainParseMemory("./memory[1]", NULL, ctxt,
>  &def->mem.max_balloon, false, true) < 0)
> 
> be replaced with something like:
> 
> unsigned long long memory_value;
> ...
> /* Extract domain memory */
> if (virDomainParseMemory("./memory[1]", NULL, ctxt,
>  &memory_value, false, true) < 0) {
> ...
> }
> virDomainDefSetMemoryInitial(def, memory_value);
> 
> 
> Although it seems like overkill - it's just making sure no one tries to
> cut-copy-paste in the future.

In the config the implementation is actually "private" and since it's
only a single place we should be okay here.

> 
>  - With having virDomainDefGetMemoryInitial u

Re: [libvirt] [PATCH] rpm-build: use pkg-config to detect wireshark presence

2015-03-16 Thread Daniel P. Berrange
On Mon, Mar 16, 2015 at 02:38:40PM +0100, Pavel Hrdina wrote:
> On Mon, Mar 16, 2015 at 12:46:04PM +, Daniel P. Berrange wrote:
> > On Mon, Mar 16, 2015 at 01:41:15PM +0100, Pavel Hrdina wrote:
> > > Wireshark supports pkg-config since 1.11.3.  Right now we build
> > > wireshark-dissectior tool as default trough rpm build only on
> > > fedora >= 21 and there is newer wireshark that supports pkg-config.
> > > If someone wants to build libvirt with wireshark-dissector against older
> > > wireshark, they should specify the location by hand.
> > > 
> > > This patch is mainly to fix wrong dependency on wireshark binary as it
> > > doesn't make sense to require that binary file to just get version info
> > > of that package in makefile.
> > > 
> > > Signed-off-by: Pavel Hrdina 
> > 
> > Be nice to go one step further and switch over to using LIBVIRT_CHECK_PKG
> > and having the code in m4/virt-wireshark.m4. Just copy virt-fuse.m4 as a
> > equivalently simple module
> > 
> 
> At first I thought that I'll be nice, but this is not that simple module.
> Wireshark requires glib-2.0 and also there is an optional argument
> --with-ws-plugindir.  This is actually the only thing I tried to avoid, but
> you're right that if I'm touching this code that it would be nice to make a
> separate m4 module.  I'll send a v2.

You shouldn't need to check for glib2.0 yourself - if that dependancy is
declared in the wireshark.pc (as it should be), then pkg-config itself
will check that for you.

Moving the --with-ws-plugindir arg declaration into the virt-wireshark.m4
file is pretty straightforward - we do that in a number of cases already
so just checkout some more existing examples.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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


Re: [libvirt] [PATCH] rpm-build: use pkg-config to detect wireshark presence

2015-03-16 Thread Pavel Hrdina
On Mon, Mar 16, 2015 at 12:46:04PM +, Daniel P. Berrange wrote:
> On Mon, Mar 16, 2015 at 01:41:15PM +0100, Pavel Hrdina wrote:
> > Wireshark supports pkg-config since 1.11.3.  Right now we build
> > wireshark-dissectior tool as default trough rpm build only on
> > fedora >= 21 and there is newer wireshark that supports pkg-config.
> > If someone wants to build libvirt with wireshark-dissector against older
> > wireshark, they should specify the location by hand.
> > 
> > This patch is mainly to fix wrong dependency on wireshark binary as it
> > doesn't make sense to require that binary file to just get version info
> > of that package in makefile.
> > 
> > Signed-off-by: Pavel Hrdina 
> 
> Be nice to go one step further and switch over to using LIBVIRT_CHECK_PKG
> and having the code in m4/virt-wireshark.m4. Just copy virt-fuse.m4 as a
> equivalently simple module
> 

At first I thought that I'll be nice, but this is not that simple module.
Wireshark requires glib-2.0 and also there is an optional argument
--with-ws-plugindir.  This is actually the only thing I tried to avoid, but
you're right that if I'm touching this code that it would be nice to make a
separate m4 module.  I'll send a v2.

Thanks for review.

Pavel

> 
> Regards,
> Daniel
> -- 
> |: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org  -o- http://virt-manager.org :|
> |: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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


Re: [libvirt] AUTHORS.in: added myself into commiters list

2015-03-16 Thread Erik Skultety


On 03/16/2015 01:38 PM, Peter Krempa wrote:
> On Mon, Mar 16, 2015 at 09:34:56 +0100, Erik Skultety wrote:
> 
> The actual patch is missing :)
> 
> Peter
> 
> 
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 
Oops, sent a regular patch to list.
Erik

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


[libvirt] [PATCH] AUTHORS: add myself to commiters list

2015-03-16 Thread Erik Skultety
---
 AUTHORS.in | 1 +
 1 file changed, 1 insertion(+)

diff --git a/AUTHORS.in b/AUTHORS.in
index 329924f..6673b3f 100644
--- a/AUTHORS.in
+++ b/AUTHORS.in
@@ -16,6 +16,7 @@ Daniel Berrange 
 Daniel Veillard 
 Doug Goldstein 
 Eric Blake 
+Erik Skultety 
 Gao Feng 
 Guido G??nther 
 J??n Tomko 
-- 
1.9.3

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

Re: [libvirt] [PATCH] AUTHORS: add myself to commiters list

2015-03-16 Thread Erik Skultety


On 03/16/2015 02:19 PM, Erik Skultety wrote:
> ---
>  AUTHORS.in | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/AUTHORS.in b/AUTHORS.in
> index 329924f..6673b3f 100644
> --- a/AUTHORS.in
> +++ b/AUTHORS.in
> @@ -16,6 +16,7 @@ Daniel Berrange 
>  Daniel Veillard 
>  Doug Goldstein 
>  Eric Blake 
> +Erik Skultety 
>  Gao Feng 
>  Guido Günther 
>  Ján Tomko 
> 
> 
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 
Now pushed as trivial.
Erik

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

[libvirt] [PATCH] network: Resolve Coverity FORWARD_NULL

2015-03-16 Thread John Ferlan
The following is a long winded way to say this patch is avoiding a
false positive.

Coverity complains that calling networkPlugBandwidth() could eventually
end up with a NULL dereference on iface->bandwidth because in the
networkAllocateActualDevice there's a check of 'iface->bandwidth'
before deciding to try to use the 'portgroup' if it exists or to not
perferm the virNetDevBandwidthCopy if 'bandwidth' is not NULL.

Later in networkPlugBandwidth the 'iface->bandwidth' is sourced from
virDomainNetGetActualBandwidth - which would be either iface->bandwidth
or (preferably) iface->data.network.actual->bandwidth which would have
been filled in from either 'iface->bandwidth' or 'portgroup->bandwidth'
back in networkAllocateActualDevice

There *is* a check in networkCheckBandwidth for the result of the
virDomainNetGetActualBandwidth being NULL and a return 1 based on
that which would cause networkPlugBandwidth to exit properly and thus
never hit the condition that Coverity complains about.

However, since Coverity checks all paths - it somehow believes that
a return of 0 by networkCheckBandwidth in this condition would end
up causing the possible NULL dereference. The "fix" to silence Coverity
is to not have networkCheckBandwidth also call virDomainNetGetActualBandwidth
in order to get the ifaceBand, but rather have it accept it as an argument
which causes Coverity to "see" that it's the exit condition of 1 that won't
have the possible NULL dereference.  Since we're passing that, I added the
passing of iface->mac rather than passing iface as well. This just hopefully
makes sure someone doesn't undo this in the future...

Signed-off-by: John Ferlan 
---
 src/network/bridge_driver.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index a007388..d885aa9 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -3820,7 +3820,7 @@ networkAllocateActualDevice(virDomainDefPtr dom,
 (netdef->forward.type == VIR_NETWORK_FORWARD_NAT) ||
 (netdef->forward.type == VIR_NETWORK_FORWARD_ROUTE)) {
 /* for these forward types, the actual net type really *is*
- *NETWORK; we just keep the info from the portgroup in
+ * NETWORK; we just keep the info from the portgroup in
  * iface->data.network.actual
  */
 iface->data.network.actual->type = VIR_DOMAIN_NET_TYPE_NETWORK;
@@ -4593,17 +4593,17 @@ networkGetNetworkAddress(const char *netname, char 
**netaddr)
  */
 static int
 networkCheckBandwidth(virNetworkObjPtr net,
-  virDomainNetDefPtr iface,
+  virNetDevBandwidthPtr ifaceBand,
+  virMacAddr ifaceMac,
   unsigned long long *new_rate)
 {
 int ret = -1;
 virNetDevBandwidthPtr netBand = net->def->bandwidth;
-virNetDevBandwidthPtr ifaceBand = virDomainNetGetActualBandwidth(iface);
 unsigned long long tmp_floor_sum = net->floor_sum;
 unsigned long long tmp_new_rate = 0;
 char ifmac[VIR_MAC_STRING_BUFLEN];
 
-virMacAddrFormat(&iface->mac, ifmac);
+virMacAddrFormat(&ifaceMac, ifmac);
 
 if (ifaceBand && ifaceBand->in && ifaceBand->in->floor &&
 !(netBand && netBand->in)) {
@@ -4689,7 +4689,8 @@ networkPlugBandwidth(virNetworkObjPtr net,
 char ifmac[VIR_MAC_STRING_BUFLEN];
 virNetDevBandwidthPtr ifaceBand = virDomainNetGetActualBandwidth(iface);
 
-if ((plug_ret = networkCheckBandwidth(net, iface, &new_rate)) < 0) {
+if ((plug_ret = networkCheckBandwidth(net, ifaceBand,
+  iface->mac, &new_rate)) < 0) {
 /* helper reported error */
 goto cleanup;
 }
-- 
2.1.0

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


Re: [libvirt] [PATCH v2 4/4] conf: Use correct type for balloon stats period

2015-03-16 Thread Erik Skultety


On 03/13/2015 05:17 PM, Martin Kletzander wrote:
> We're parsing memballoon status period as unsigned int, but when we're
> trying to set it, both we and qemu use signed int.  That means large
> values will get wrapped around to negative one resulting in error.
> Basically the same problem as commit e3a7b874 was dealing with when
> updating live domain.
> 
> QEMU changed the accepted value to int64 in commit 1f9296b5, but even
> values as INT_MAX don't make sense since the value passed means seconds.
> Hence adding capability flag for this change isn't worth it.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1140958
> 
> Signed-off-by: Luyao Huang 
> Signed-off-by: Martin Kletzander 
> ---
>  docs/formatdomain.html.in | 2 ++
>  src/conf/domain_conf.c| 9 +++--
>  src/conf/domain_conf.h| 2 +-
>  src/qemu/qemu_process.c   | 2 +-
>  4 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 40e2b29..7a11cc7 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -5630,6 +5630,8 @@ qemu-kvm -net nic,model=? /dev/null
>only be made to the active guest.
>If the QEMU driver is not at the right
>revision, the attempt to set the period will fail.
> +  Large values might be ignored, but this only affects
> +  non-sensical numbers (i.e. many years).
>Since 1.1.1, requires QEMU 1.5
>  
>

Just a nitpick, I'd probably avoid word construction non-sensical in our
docs (it's not even correct --> nonsensical) and simplify this to "Large
values (i.e. many years) might be ignored."

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index e010040..b3d 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -10432,6 +10432,7 @@ virDomainMemballoonDefParseXML(xmlNodePtr node,
>  char *model;
>  virDomainMemballoonDefPtr def;
>  xmlNodePtr save = ctxt->node;
> +unsigned int period = 0;
> 
>  if (VIR_ALLOC(def) < 0)
>  return NULL;
> @@ -10450,12 +10451,16 @@ virDomainMemballoonDefParseXML(xmlNodePtr node,
>  }
> 
>  ctxt->node = node;
> -if (virXPathUInt("string(./stats/@period)", ctxt, &def->period) < -1) {
> +if (virXPathUInt("string(./stats/@period)", ctxt, &period) < -1) {
>  virReportError(VIR_ERR_XML_ERROR, "%s",
> _("invalid statistics collection period"));
>  goto error;
>  }
> 
> +def->period = period;
> +if (def->period < 0)
> +def->period = 0;
> +
>  if (def->model == VIR_DOMAIN_MEMBALLOON_MODEL_NONE)
>  VIR_DEBUG("Ignoring device address for none model Memballoon");
>  else if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0)
> @@ -18823,7 +18828,7 @@ virDomainMemballoonDefFormat(virBufferPtr buf,
>  virBufferAdjustIndent(&childrenBuf, indent + 2);
> 
>  if (def->period)
> -virBufferAsprintf(&childrenBuf, "\n", 
> def->period);
> +virBufferAsprintf(&childrenBuf, "\n", 
> def->period);
> 
>  if (virDomainDeviceInfoNeedsFormat(&def->info, flags) &&
>  virDomainDeviceInfoFormat(&childrenBuf, &def->info, flags) < 0) {
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index ea463cb..ee0f5fd 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1556,7 +1556,7 @@ enum {
>  struct _virDomainMemballoonDef {
>  int model;
>  virDomainDeviceInfo info;
> -unsigned int period; /* seconds between collections */
> +int period; /* seconds between collections */
>  };
> 
>  struct _virDomainNVRAMDef {
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index d1f089d..0f357d5 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -4390,7 +4390,7 @@ int qemuProcessStart(virConnectPtr conn,
>  virCommandPtr cmd = NULL;
>  struct qemuProcessHookData hookData;
>  unsigned long cur_balloon;
> -unsigned int period = 0;
> +int period = 0;
>  size_t i;
>  bool rawio_set = false;
>  char *nodeset = NULL;
> --
> 2.3.2
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 

ACK with the nitpick fixed.
Erik

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


Re: [libvirt] [PATCH v2 3/4] qemu: Don't duplicate errors when settings stats period

2015-03-16 Thread Erik Skultety


On 03/13/2015 05:17 PM, Martin Kletzander wrote:
> In order not to leave old error messages set, this patch refactors the
> code so the error is reported only when acted upon.  The only such place
> already rewrites any error, so cleaning up all the error reporting in
> qemuMonitorSetMemoryStatsPeriod() is enough.
> 
> +/**
> + * qemuMonitorSetMemoryStatsPeriod:
> + *
> + * This function sets balloon stats update period.
> + *
> + * Returns 0 on success and -1 on error, but does *not* set an error.
> + */
>  int qemuMonitorSetMemoryStatsPeriod(qemuMonitorPtr mon,
>  int period)
>  {
>  int ret = -1;
>  VIR_DEBUG("mon=%p period=%d", mon, period);
> 
> -if (!mon) {
> -virReportError(VIR_ERR_INVALID_ARG, "%s",
> -   _("monitor must not be NULL"));
> +if (!mon)
>  return -1;
> -}
> 
> -if (!mon->json) {
> -virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> -   _("JSON monitor is required"));
> +if (!mon->json)
> +return -1;
> +
> +if (period < 0)
>  return -1;
> -}

Hmm. It is a nice idea, but I guess you might have forgotten to check
the actual return value in qemuProcessStart (there are actually 2
appearances in qemu_process.c) like we do in most cases.
I found a piece of code (see below) where we check this correctly (so
it's great you refactored this, otherwise reporting 2 identical messages
would be sort of confusing)

src/qemu/qemu_driver.c
r = qemuMonitorSetMemoryStatsPeriod(priv->mon, period);
if (qemuDomainObjExitMonitor(driver, vm) < 0)
goto endjob;
if (r < 0) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
   _("unable to set balloon driver collection period"));
goto endjob;

>  if (qemuMonitorFindBalloonObjectPath(mon, "/") == 0) {
>  ret = qemuMonitorJSONSetMemoryStatsPeriod(mon, mon->balloonpath,
>period);
> +
> +/*
> + * Most of the calls to this function are supposed to be
> + * non-fatal and the only one that should be fatal wants its
> + * own error message.  More details for debugging will be in
> + * the log file.
> + */
> +if (ret < 0)
> +virResetLastError();
>  }
>  mon->ballooninit = true;
>  return ret;


Everything else looks fine to me, though I think that other monitor
calls should meet a similar fate sometime in the future.
Erik

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


Re: [libvirt] [PATCH v2 2/4] conf: Reorder elements inside memballoon

2015-03-16 Thread Erik Skultety


On 03/13/2015 05:17 PM, Martin Kletzander wrote:
> All the devices we have format their address as its last sub-element, so
> let's change memballoon to follow suit.  Also adjust RNG to allow any
> order of them so 'virsh edit' doesn't shout at us.
> 
> Signed-off-by: Martin Kletzander 
> ---
>  docs/schemas/domaincommon.rng  | 28 ++--
>  src/conf/domain_conf.c | 30 
> ++
>  .../qemuxml2xmlout-balloon-device-period.xml   | 30 
> ++
>  tests/qemuxml2xmltest.c|  1 +
>  4 files changed, 60 insertions(+), 29 deletions(-)
>  create mode 100644 
> tests/qemuxml2xmloutdata/qemuxml2xmlout-balloon-device-period.xml
> 
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index b1d883f..b9d430a 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -3438,19 +3438,21 @@
>none
>  
>
> -  
> -
> -  
> -  
> -
> -  
> -  
> -
> -  
> -
> -  
> -
> -  
> +  
> +
> +  
> +
> +
> +  
> +
> +
> +  
> +
> +  
> +
> +  
> +
> +  
>  
>
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index ae8688e..e010040 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -18810,7 +18810,8 @@ virDomainMemballoonDefFormat(virBufferPtr buf,
>   unsigned int flags)
>  {
>  const char *model = virDomainMemballoonModelTypeToString(def->model);
> -bool noopts = true;
> +virBuffer childrenBuf = VIR_BUFFER_INITIALIZER;
> +int indent = virBufferGetIndent(buf, false);
> 
>  if (!model) {
>  virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -18819,27 +18820,24 @@ virDomainMemballoonDefFormat(virBufferPtr buf,
>  }
> 
>  virBufferAsprintf(buf, " -virBufferAdjustIndent(buf, 2);
> +virBufferAdjustIndent(&childrenBuf, indent + 2);
> 
> -if (virDomainDeviceInfoNeedsFormat(&def->info, flags)) {
> -virBufferAddLit(buf, ">\n");
> -if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0)
> -return -1;
> -noopts = false;
> -}
> +if (def->period)
> +virBufferAsprintf(&childrenBuf, "\n", 
> def->period);
> 
> -if (def->period) {
> -if (noopts)
> -virBufferAddLit(buf, ">\n");
> -virBufferAsprintf(buf, "\n", def->period);
> -noopts = false;
> +if (virDomainDeviceInfoNeedsFormat(&def->info, flags) &&
> +virDomainDeviceInfoFormat(&childrenBuf, &def->info, flags) < 0) {
> +virBufferFreeAndReset(&childrenBuf);
> +return -1;
>  }
> 
> -virBufferAdjustIndent(buf, -2);
> -if (noopts)
> +if (!virBufferUse(&childrenBuf)) {
>  virBufferAddLit(buf, "/>\n");
> -else
> +} else {
> +virBufferAddLit(buf, ">\n");
> +virBufferAddBuffer(buf, &childrenBuf);
>  virBufferAddLit(buf, "\n");
> +}
> 
>  return 0;
>  }
> diff --git 
> a/tests/qemuxml2xmloutdata/qemuxml2xmlout-balloon-device-period.xml 
> b/tests/qemuxml2xmloutdata/qemuxml2xmlout-balloon-device-period.xml
> new file mode 100644
> index 000..79e465a
> --- /dev/null
> +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-balloon-device-period.xml
> @@ -0,0 +1,30 @@
> +
> +  QEMUGuest1
> +  c7a5fdbd-edaf-9455-926a-d65c16db1809
> +  219136
> +  219136
> +  1
> +  
> +hvm
> +
> +  
> +  
> +  destroy
> +  restart
> +  destroy
> +  
> +/usr/bin/qemu
> +
> +  
> +  
> +  
> +
> +
> +
> +
> +
> +  
> +   function='0x0'/>
> +
> +  
> +
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index 8e12e84..9e4b3a2 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -354,6 +354,7 @@ mymain(void)
> 
>  /* These tests generate different XML */
>  DO_TEST_DIFFERENT("balloon-device-auto");
> +DO_TEST_DIFFERENT("balloon-device-period");
>  DO_TEST_DIFFERENT("channel-virtio-auto");
>  DO_TEST_DIFFERENT("console-compat-auto");
>  DO_TEST_DIFFERENT("disk-scsi-device-auto");
> --
> 2.3.2
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 
Looks good, ACK.
Erik

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


Re: [libvirt] [PATCH v2 1/4] util: Make sure the comment about virBufferAddBuffer is true

2015-03-16 Thread Erik Skultety


On 03/13/2015 05:17 PM, Martin Kletzander wrote:
> Change it so it really *always* eats the @toadd buffer.
> 
> Signed-off-by: Martin Kletzander 
> ---
>  src/util/virbuffer.c | 14 +-
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c
> index 96a0f16..0089d1b 100644
> --- a/src/util/virbuffer.c
> +++ b/src/util/virbuffer.c
> @@ -1,7 +1,7 @@
>  /*
>   * virbuffer.c: buffers for libvirt
>   *
> - * Copyright (C) 2005-2008, 2010-2014 Red Hat, Inc.
> + * Copyright (C) 2005-2008, 2010-2015 Red Hat, Inc.
>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Lesser General Public
> @@ -188,23 +188,27 @@ virBufferAddBuffer(virBufferPtr buf, virBufferPtr toadd)
>  {
>  unsigned int needSize;
> 
> -if (!buf || !toadd)
> +if (!toadd)
>  return;
> 
> +if (!buf)
> +goto done;
> +
>  if (buf->error || toadd->error) {
>  if (!buf->error)
>  buf->error = toadd->error;
> -virBufferFreeAndReset(toadd);
> -return;
> +goto done;
>  }
> 
>  needSize = buf->use + toadd->use;
>  if (virBufferGrow(buf, needSize - buf->use) < 0)
> -return;
> +goto done;
> 
>  memcpy(&buf->content[buf->use], toadd->content, toadd->use);
>  buf->use += toadd->use;
>  buf->content[buf->use] = '\0';
> +
> + done:
>  virBufferFreeAndReset(toadd);
>  }
> 
> --
> 2.3.2
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 
ACK,
Erik

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


Re: [libvirt] [PATCH 08/12] Convert virDomainVcpuPinFindByVcpu into virDomainPinFindByVcpu

2015-03-16 Thread Ján Tomko
On Fri, Mar 13, 2015 at 11:11:52PM -0400, John Ferlan wrote:
> Make common between Vcpu and IOThreads

I feel like this sentence is missing something.

> 
> Signed-off-by: John Ferlan 
> ---
>  src/conf/domain_conf.c   | 24 
>  src/conf/domain_conf.h   |  6 +++---
>  src/libvirt_private.syms |  2 +-
>  src/qemu/qemu_driver.c   | 12 ++--
>  src/qemu/qemu_process.c  | 12 ++--
>  5 files changed, 28 insertions(+), 28 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index efc01bd..75d75bc 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -16693,17 +16693,17 @@ virDomainPinIsDuplicate(virDomainPinDefPtr *def,
>  }
>  
>  virDomainPinDefPtr
> -virDomainVcpuPinFindByVcpu(virDomainPinDefPtr *def,
> -   int nvcpupin,
> -   int vcpu)
> +virDomainPinFindByVcpu(virDomainPinDefPtr *def,
> +   int npin,
> +   int id)

The second reference to Vcpu should be removed as well:
virDomainPinFindById
or even simpler:
virDomainPinFind
since I can't imagine doing a find according to any other attribute.

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 00/12] More cleanup from IOThreads changes

2015-03-16 Thread Ján Tomko
On Fri, Mar 13, 2015 at 11:11:44PM -0400, John Ferlan wrote:
> During the review process a few things were pointed at as perhaps
> needing some adjustments based on what was done for IOThreads.
> Specifically a memory leak in PinVcpuFlags since PinIOThreads was
> just a copy of the Vcpu code and secondarily since the IOThreads
> code "reused" the virDomainVcpuPin* data structures and API's, those
> should change names to be more generic.
> 
> 
> John Ferlan (12):
>   qemu: Fix possible memory leak in qemuDomainPinVcpuFlags
>   Convert virDomainVcpuPinDefPtr to virDomainPinDefPtr
>   Convert virDomainPinDefPtr->vcpuid to virDomainPinDefPtr->id
>   Convert virDomainVcpuPinDefFree to virDomainPinDefFree
>   Convert virDomainVcpuPinDefArrayFree to virDomainPinDefArrayFree
>   Convert virDomainVcpuPinDefCopy into virDomainPinDefCopy
>   Convert virDomainVcpuPinIsDuplicate into virDomainPinIsDuplicate
>   Convert virDomainVcpuPinFindByVcpu into virDomainPinFindByVcpu
>   Replace virDomainVcpuPinAdd with virDomainPinAdd
>   Replace virDomainIOThreadsPinAdd with virDomainPinAdd
>   Replace virDomainVcpuPinDel with virDomainPinDel
>   Remove virDomainIOThreadsPinDel
> 
>  src/conf/domain_conf.c   | 234 
> +--
>  src/conf/domain_conf.h   |  58 +---
>  src/libvirt_private.syms |  16 ++--
>  src/libxl/libxl_domain.c |   2 +-
>  src/libxl/libxl_driver.c |  18 ++--
>  src/qemu/qemu_cgroup.c   |  12 +--
>  src/qemu/qemu_cgroup.h   |   4 +-
>  src/qemu/qemu_driver.c   | 104 +++--
>  src/qemu/qemu_process.c  |  16 ++--
>  src/xen/xend_internal.c  |  10 +-
>  10 files changed, 204 insertions(+), 270 deletions(-)
> 

ACK series.

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 v10 4/4] domifaddr: Add virsh support

2015-03-16 Thread Daniel P. Berrange
On Mon, Mar 16, 2015 at 08:43:32AM -0400, John Ferlan wrote:
> 
> 
> On 03/11/2015 07:09 AM, Daniel P. Berrange wrote:
> > From: Nehal J Wani 
> > 
> > tools/virsh-domain-monitor.c
> >* Introduce new command : domifaddr
> >  Usage: domifaddr  [interface] [--full] [--source lease|agent]
> > 
> >  Example outputs:
> >  virsh # domifaddr f20
> >  Name   MAC address  Protocol Address
> >  
> > ---
> >  lo 00:00:00:00:00:00ipv4 127.0.0.1/8
> >  -  -ipv6 ::1/128
> >  eth0   52:54:00:2e:45:ceipv4 10.1.33.188/24
> >  -  -ipv6 2001:db8:0:f101::2/64
> >  -  -ipv6 fe80::5054:ff:fe2e:45ce/64
> >  eth1   52:54:00:b1:70:19ipv4 192.168.105.201/16
> >  -  -ipv4 192.168.201.195/16
> >  -  -ipv6 fe80::5054:ff:feb1:7019/64
> >  eth2   52:54:00:36:2a:e5N/A  N/A
> >  eth3   52:54:00:20:70:3dipv4 192.168.105.240/16
> >  -  -ipv6 fe80::5054:ff:fe20:703d/64
> > 
> 
> Does the above list now change with the default --source of lease?

Yeah, it should be using  vnet0, vnet1, etc instead of eth0, eth1, etc
at the very least.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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


Re: [libvirt] [PATCH] rpm-build: use pkg-config to detect wireshark presence

2015-03-16 Thread Daniel P. Berrange
On Mon, Mar 16, 2015 at 01:41:15PM +0100, Pavel Hrdina wrote:
> Wireshark supports pkg-config since 1.11.3.  Right now we build
> wireshark-dissectior tool as default trough rpm build only on
> fedora >= 21 and there is newer wireshark that supports pkg-config.
> If someone wants to build libvirt with wireshark-dissector against older
> wireshark, they should specify the location by hand.
> 
> This patch is mainly to fix wrong dependency on wireshark binary as it
> doesn't make sense to require that binary file to just get version info
> of that package in makefile.
> 
> Signed-off-by: Pavel Hrdina 

Be nice to go one step further and switch over to using LIBVIRT_CHECK_PKG
and having the code in m4/virt-wireshark.m4. Just copy virt-fuse.m4 as a
equivalently simple module


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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


Re: [libvirt] [PATCH v10 1/4] domifaddr: Implement the public APIs

2015-03-16 Thread John Ferlan


On 03/11/2015 07:09 AM, Daniel P. Berrange wrote:
> From: Nehal J Wani 
> 
> Define helper function virDomainInterfaceFree, which allows
> the upper layer application to free the domain interface object
> conveniently.
> 
> The API is going to provide multiple methods by flags, e.g.
>   * Query guest agent
>   * Parse DHCP lease file
> 
> include/libvirt/libvirt-domain.h
>   * Define virDomainInterfaceAddresses, virDomainInterfaceFree
>   * Define structs virDomainInterface, virDomainIPAddress
> 
> src/driver-hypervisor.h:
>   * Define domainInterfaceAddresses
> 
> src/libvirt-domain.c:
>   * Implement virDomainInterfaceAddresses
>   * Implement virDomainInterfaceFree
> 
> src/libvirt_public.syms:
>   * Export the new symbols
> 
> Signed-off-by: Nehal J Wani 
> ---
>  include/libvirt/libvirt-domain.h |  32 ++
>  src/driver-hypervisor.h  |   6 ++
>  src/libvirt-domain.c | 123 
> +++
>  src/libvirt_public.syms  |   2 +
>  4 files changed, 163 insertions(+)
> 
> diff --git a/include/libvirt/libvirt-domain.h 
> b/include/libvirt/libvirt-domain.h
> index 9487b80..8cc7fe8 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -3725,5 +3725,37 @@ typedef struct _virTypedParameter virMemoryParameter;
>   */
>  typedef virMemoryParameter *virMemoryParameterPtr;
>  
> +typedef enum {
> +VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LEASE = 0, /* Parse DHCP lease file */
> +VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT = 1, /* Query qemu guest agent 
> */
> +
> +# ifdef VIR_ENUM_SENTINELS
> +VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LAST
> +# endif
> +} virDomainInterfaceAddressesSource;
> +
> +typedef struct _virDomainInterfaceIPAddress virDomainIPAddress;
> +typedef virDomainIPAddress *virDomainIPAddressPtr;
> +struct _virDomainInterfaceIPAddress {
> +int type;/* virIPAddrType */
> +char *addr;  /* IP address */
> +unsigned int prefix; /* IP address prefix */
> +};
> +
> +typedef struct _virDomainInterface virDomainInterface;
> +typedef virDomainInterface *virDomainInterfacePtr;
> +struct _virDomainInterface {
> +char *name; /* interface name */
> +char *hwaddr;   /* hardware address */
> +unsigned int naddrs;/* number of items in @addrs */
> +virDomainIPAddressPtr addrs;/* array of IP addresses */
> +};
> +
> +int virDomainInterfaceAddresses(virDomainPtr dom,
> +virDomainInterfacePtr **ifaces,
> +unsigned int source,
> +unsigned int flags);
> +
> +void virDomainInterfaceFree(virDomainInterfacePtr iface);
>  
>  #endif /* __VIR_LIBVIRT_DOMAIN_H__ */
> diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h
> index c2b4cd8..0e729c4 100644
> --- a/src/driver-hypervisor.h
> +++ b/src/driver-hypervisor.h
> @@ -1178,6 +1178,11 @@ typedef int
>  unsigned int cellCount,
>  unsigned int flags);
>  
> +typedef int
> +(*virDrvDomainInterfaceAddresses)(virDomainPtr dom,
> +  virDomainInterfacePtr **ifaces,
> +  unsigned int source,
> +  unsigned int flags);
>  
>  typedef struct _virHypervisorDriver virHypervisorDriver;
>  typedef virHypervisorDriver *virHypervisorDriverPtr;
> @@ -1404,6 +1409,7 @@ struct _virHypervisorDriver {
>  virDrvConnectGetAllDomainStats connectGetAllDomainStats;
>  virDrvNodeAllocPages nodeAllocPages;
>  virDrvDomainGetFSInfo domainGetFSInfo;
> +virDrvDomainInterfaceAddresses domainInterfaceAddresses;
>  };
>  
>  
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 04545fd..cf36d30 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -11338,3 +11338,126 @@ virDomainFSInfoFree(virDomainFSInfoPtr info)
>  VIR_FREE(info->devAlias[i]);
>  VIR_FREE(info->devAlias);
>  }
> +
> +/**
> + * virDomainInterfaceAddresses:
> + * @dom: domain object
> + * @ifaces: pointer to an array of pointers pointing to interface objects
> + * @source: one of the virDomainInterfaceAddressesSource constants
> + * @flags: currently unused, pass zero
> + *
> + * Return a pointer to the allocated array of pointers to interfaces
> + * present in given domain along with their IP and MAC addresses. Note that
> + * single interface can have multiple or even 0 IP addresses.
> + *
> + * This API dynamically allocates the virDomainInterfacePtr struct based on
> + * how many interfaces domain @dom has, usually there's 1:1 correlation. The
> + * count of the interfaces is returned as the return value.
> + *
> + * If @source is VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LEASE, the DHCP lease
> + * file associated with any virtual networks will be examined to obtain
> + * the interface addresses. This only returns data for interfaces whic

Re: [libvirt] [PATCH v10 2/4] domifaddr: Implement the remote protocol

2015-03-16 Thread John Ferlan


On 03/11/2015 07:09 AM, Daniel P. Berrange wrote:
> From: Nehal J Wani 
> 
> daemon/remote.c
>* Define remoteSerializeDomainInterface, 
> remoteDispatchDomainInterfaceAddresses
> 
> src/remote/remote_driver.c
>* Define remoteDomainInterfaceAddresses
> 
> src/remote/remote_protocol.x
>* New RPC procedure: REMOTE_PROC_DOMAIN_INTERFACE_ADDRESSES
>* Define structs remote_domain_ip_addr, remote_domain_interface,
>  remote_domain_interfaces_addresse_args, 
> remote_domain_interface_addresses_ret
>* Introduce upper bounds (to handle DoS attacks):
>  REMOTE_DOMAIN_INTERFACE_MAX = 2048
>  REMOTE_DOMAIN_IP_ADDR_MAX = 2048
>  Restrictions on the maximum number of aliases per interface were
>  removed after kernel v2.0, and theoretically, at present, there
>  are no upper limits on number of interfaces per virtual machine
>  and on the number of IP addresses per interface.
> 
> src/remote_protocol-structs
>* New structs added
> 
> Signed-off-by: Nehal J Wani 
> ---
>  daemon/remote.c  | 124 
> +++
>  src/remote/remote_driver.c   | 103 +++
>  src/remote/remote_protocol.x |  37 -
>  src/remote_protocol-structs  |  25 +
>  4 files changed, 288 insertions(+), 1 deletion(-)
> 
> diff --git a/daemon/remote.c b/daemon/remote.c
> index ff64eeb..f2b970a 100644
> --- a/daemon/remote.c
> +++ b/daemon/remote.c
> @@ -6510,6 +6510,130 @@ remoteDispatchDomainGetFSInfo(virNetServerPtr server 
> ATTRIBUTE_UNUSED,
>  }
>  
>  
> +static int
> +remoteSerializeDomainInterface(virDomainInterfacePtr *ifaces,
> +   unsigned int ifaces_count,
> +   remote_domain_interface_addresses_ret *ret)
> +{
> +size_t i, j;
> +
> +if (ifaces_count > REMOTE_DOMAIN_INTERFACE_MAX) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Number of interfaces, %d exceeds the max limit: 
> %d"),
> +   ifaces_count, REMOTE_DOMAIN_INTERFACE_MAX);
> +return -1;
> +}
> +
> +if (VIR_ALLOC_N(ret->ifaces.ifaces_val, ifaces_count) < 0)
> +return -1;
> +
> +ret->ifaces.ifaces_len = ifaces_count;
> +
> +for (i = 0; i < ifaces_count; i++) {
> +virDomainInterfacePtr iface = ifaces[i];
> +remote_domain_interface *iface_ret = &(ret->ifaces.ifaces_val[i]);
> +
> +if ((VIR_STRDUP(iface_ret->name, iface->name)) < 0)
> +goto cleanup;
> +
> +if ((VIR_STRDUP(iface_ret->hwaddr, iface->hwaddr)) < 0)
> +goto cleanup;
> +
> +if (iface->naddrs > REMOTE_DOMAIN_IP_ADDR_MAX) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Number of interfaces, %d exceeds the max 
> limit: %d"),
> +   iface->naddrs, REMOTE_DOMAIN_IP_ADDR_MAX);
> +goto cleanup;
> +}
> +
> +if (VIR_ALLOC_N(iface_ret->addrs.addrs_val,
> +iface->naddrs) < 0)
> +goto cleanup;
> +
> +iface_ret->addrs.addrs_len = iface->naddrs;
> +
> +for (j = 0; j < iface->naddrs; j++) {
> +virDomainIPAddressPtr ip_addr = &(iface->addrs[j]);
> +remote_domain_ip_addr *ip_addr_ret =
> +&(iface_ret->addrs.addrs_val[j]);
> +
> +if (VIR_STRDUP(ip_addr_ret->addr, ip_addr->addr) < 0)
> +goto cleanup;
> +
> +ip_addr_ret->prefix = ip_addr->prefix;
> +ip_addr_ret->type = ip_addr->type;
> +}
> +}
> +
> +return 0;
> +
> + cleanup:
> +if (ret->ifaces.ifaces_val) {
> +for (i = 0; i < ifaces_count; i++) {
> +remote_domain_interface *iface_ret = 
> &(ret->ifaces.ifaces_val[i]);
> +VIR_FREE(iface_ret->name);
> +VIR_FREE(iface_ret->hwaddr);
> +for (j = 0; j < iface_ret->addrs.addrs_len; j++) {
> +remote_domain_ip_addr *ip_addr =
> +&(iface_ret->addrs.addrs_val[j]);
> +VIR_FREE(ip_addr->addr);
> +}
> +}
> +VIR_FREE(ret->ifaces.ifaces_val);
> +}
> +
> +return -1;
> +}
> +
> +
> +static int
> +remoteDispatchDomainInterfaceAddresses(virNetServerPtr server 
> ATTRIBUTE_UNUSED,
> +   virNetServerClientPtr client,
> +   virNetMessagePtr msg ATTRIBUTE_UNUSED,
> +   virNetMessageErrorPtr rerr,
> +   
> remote_domain_interface_addresses_args *args,
> +   remote_domain_interface_addresses_ret 
> *ret)
> +{
> +size_t i;
> +int rv = -1;
> +virDomainPtr dom = NULL;
> +virDomainInterfacePtr *ifaces = NULL;
> +int ifaces_count = 0;
> +struct daemonClientPrivate *priv =
> +virNetServerClientGetPrivateData(client);
> +
> +

[libvirt] [PATCH] rpm-build: use pkg-config to detect wireshark presence

2015-03-16 Thread Pavel Hrdina
Wireshark supports pkg-config since 1.11.3.  Right now we build
wireshark-dissectior tool as default trough rpm build only on
fedora >= 21 and there is newer wireshark that supports pkg-config.
If someone wants to build libvirt with wireshark-dissector against older
wireshark, they should specify the location by hand.

This patch is mainly to fix wrong dependency on wireshark binary as it
doesn't make sense to require that binary file to just get version info
of that package in makefile.

Signed-off-by: Pavel Hrdina 
---
 configure.ac | 26 ++
 1 file changed, 6 insertions(+), 20 deletions(-)

diff --git a/configure.ac b/configure.ac
index 2fedd1a..75a0688 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2669,25 +2669,12 @@ if test "$with_wireshark_dissector" != "no"; then
 ])
 
 dnl Search for wireshark(or tshark) command
-AC_PATH_PROG([WIRESHARK], [wireshark])
-AC_PATH_PROG([WIRESHARK], [tshark])
-if test -z "$WIRESHARK"; then
-LIBVIRT_WS_HANDLE_ERROR([command not found wireshark or tshark])
-else
-dnl Check for wireshark headers
-save_CPPFLAGS="$CPPFLAGS"
-WS_DISSECTOR_CPPFLAGS="$WS_DISSECTOR_CPPFLAGS -I`dirname 
$WIRESHARK`/../include/wireshark"
-CPPFLAGS="$CPPFLAGS $WS_DISSECTOR_CPPFLAGS"
-AC_CHECK_HEADERS([wireshark/config.h],, [
-LIBVIRT_WS_HANDLE_ERROR([wireshark/config.h is required for 
wireshark-dissector support])
-])
-AC_CHECK_HEADERS([wireshark/epan/packet.h 
wireshark/epan/dissectors/packet-tcp.h],, [
-LIBVIRT_WS_HANDLE_ERROR([wireshark/epan/{packet,packet-tcp}.h are 
required for wireshark-dissector support])
-], [
-  #include 
-])
-CPPFLAGS="$save_CPPFLAGS"
-fi
+PKG_CHECK_MODULES([WIRESHARK], [wireshark], [
+  WS_DISSECTOR_CPPFLAGS="$WS_DISSECTOR_CPPFLAGS `$PKG_CONFIG --cflags 
wireshark`"
+  ws_version=`$PKG_CONFIG --modversion wireshark`
+], [
+  LIBVIRT_WS_HANDLE_ERROR([pkg-config 'wireshark' is required for 
wireshark-dissector support])
+])
 if test "$with_wireshark_dissector" != "no"; then
 with_wireshark_dissector=yes
 fi
@@ -2701,7 +2688,6 @@ AC_ARG_WITH([ws-plugindir],
   [ws_plugindir=$withval])
 
 if test "$with_wireshark_dissector" != "no" && test -z "$ws_plugindir"; then
-ws_version=`$WIRESHARK -v | head -1 | cut -f 2 -d' '`
 ws_plugindir="$libdir/wireshark/plugins/$ws_version"
 fi
 AC_SUBST([ws_plugindir])
-- 
2.0.5

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


Re: [libvirt] [PATCH v10 4/4] domifaddr: Add virsh support

2015-03-16 Thread John Ferlan


On 03/11/2015 07:09 AM, Daniel P. Berrange wrote:
> From: Nehal J Wani 
> 
> tools/virsh-domain-monitor.c
>* Introduce new command : domifaddr
>  Usage: domifaddr  [interface] [--full] [--source lease|agent]
> 
>  Example outputs:
>  virsh # domifaddr f20
>  Name   MAC address  Protocol Address
>  
> ---
>  lo 00:00:00:00:00:00ipv4 127.0.0.1/8
>  -  -ipv6 ::1/128
>  eth0   52:54:00:2e:45:ceipv4 10.1.33.188/24
>  -  -ipv6 2001:db8:0:f101::2/64
>  -  -ipv6 fe80::5054:ff:fe2e:45ce/64
>  eth1   52:54:00:b1:70:19ipv4 192.168.105.201/16
>  -  -ipv4 192.168.201.195/16
>  -  -ipv6 fe80::5054:ff:feb1:7019/64
>  eth2   52:54:00:36:2a:e5N/A  N/A
>  eth3   52:54:00:20:70:3dipv4 192.168.105.240/16
>  -  -ipv6 fe80::5054:ff:fe20:703d/64
> 

Does the above list now change with the default --source of lease?

Since the subsequent example lists --source agent, I suspect the above
may not be "correct" for eth0.

>  virsh # domifaddr f20 eth1 --source lease
>  Name   MAC address  Protocol Address
>  
> ---
>  eth1   52:54:00:b1:70:19ipv4 192.168.105.201/16
>  -  -ipv4 192.168.201.195/16
>  -  -ipv6 fe80::5054:ff:feb1:7019/64
> 
>  virsh # domifaddr f20 eth0 --source agent --full
>  Name   MAC address  Protocol Address
>  
> ---
>  eth0   52:54:00:2e:45:ceipv4 10.1.33.188/24
>  eth0   52:54:00:2e:45:ceipv6 2001:db8:0:f101::2/64
>  eth0   52:54:00:2e:45:ceipv6 fe80::5054:ff:fe2e:45ce/64
> 
> tools/virsh.pod
>* Document new command
> 
> Signed-off-by: Nehal J Wani 
> ---
>  tools/virsh-domain-monitor.c | 146 
> +++
>  tools/virsh.pod  |  16 +
>  2 files changed, 162 insertions(+)
> 
> diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
> index 464ac11..92ce152 100644
> --- a/tools/virsh-domain-monitor.c
> +++ b/tools/virsh-domain-monitor.c
> @@ -2197,6 +2197,146 @@ cmdDomstats(vshControl *ctl, const vshCmd *cmd)
>  return ret;
>  }
>  
> +/* "domifaddr" command
> + */
> +static const vshCmdInfo info_domifaddr[] = {
> +{"help", N_("Get network interfaces' addresses for a running domain")},
> +{"desc", N_("Get network interfaces' addresses for a running domain")},
> +{NULL, NULL}
> +};
> +
> +static const vshCmdOptDef opts_domifaddr[] = {
> +{.name = "domain",
> + .type = VSH_OT_DATA,
> + .flags = VSH_OFLAG_REQ,
> + .help = N_("domain name, id or uuid")},
> +{.name = "interface",
> + .type = VSH_OT_STRING,
> + .flags = VSH_OFLAG_NONE,
> + .help = N_("network interface name")},
> +{.name = "full",
> + .type = VSH_OT_BOOL,
> + .flags = VSH_OFLAG_NONE,
> + .help = N_("display full fields")},
> +{.name = "source",
> + .type = VSH_OT_STRING,
> + .flags = VSH_OFLAG_NONE,
> + .help = N_("address source: 'lease' or 'agent'")},
> +{.name = NULL}
> +};
> +
> +static bool
> +cmdDomIfAddr(vshControl *ctl, const vshCmd *cmd)
> +{
> +virDomainPtr dom = NULL;
> +const char *interface = NULL;
> +virDomainInterfacePtr *ifaces = NULL;
> +size_t i, j;
> +int ifaces_count = 0;
> +bool ret = false;
> +bool full = vshCommandOptBool(cmd, "full");
> +const char *sourcestr = NULL;
> +int source = VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LEASE;
> +
> +if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
> +return false;
> +
> +if (vshCommandOptString(cmd, "interface", &interface) < 0)
> +goto cleanup;
> +if (vshCommandOptString(cmd, "source", &sourcestr) < 0)
> +goto cleanup;
> +
> +if (sourcestr) {
> +if (STREQ(sourcestr, "lease")) {
> +source = VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LEASE;
> +} else if (STREQ(sourcestr, "agent")) {
> +source = VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT;
> +} else {
> +vshError(ctl, _("Unknown data source '%s'"), sourcestr);
> +goto cleanup;
> +}
> +}
> +
> +if ((ifaces_count = virDomainInterfaceAddresses(dom, &ifaces, source, 
> 0)) < 0) {
> +vshError(ctl, _("Failed to query for interfaces addresses"));
> +goto cleanup;
> +}
> +
> +vshPrintExtra(ctl, " %-10s %-20s %-8s %

Re: [libvirt] AUTHORS.in: added myself into commiters list

2015-03-16 Thread Peter Krempa
On Mon, Mar 16, 2015 at 09:34:56 +0100, Erik Skultety wrote:

The actual patch is missing :)

Peter


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

Re: [libvirt] [PATCH v10 3/4] domifaddr: Implement the API for qemu

2015-03-16 Thread John Ferlan


On 03/11/2015 07:09 AM, Daniel P. Berrange wrote:
> From: Nehal J Wani 
> 
> By querying the qemu guest agent with the QMP command
> "guest-network-get-interfaces" and converting the received JSON
> output to structured objects.
> 
> Although "ifconfig" is deprecated, IP aliases created by "ifconfig"
> are supported by this API. The legacy syntax of an IP alias is:
> ":". Since we want all aliases to be clubbed
> under parent interface, simply stripping ":" suffices.
> Note that IP aliases formed by "ip" aren't visible to "ifconfig",
> and aliases created by "ip" do not have any specific name. But
> we are lucky, as qemu guest agent detects aliases created by both.
> 
> src/qemu/qemu_agent.h:
>   * Define qemuAgentGetInterfaces
> 
> src/qemu/qemu_agent.c:
>   * Implement qemuAgentGetInterface
> 
> src/qemu/qemu_driver.c:
>   * New function qemuGetDHCPInterfaces
>   * New function qemuDomainInterfaceAddresses
> 
> src/remote_protocol-sructs:
>   * Define new structs
> 
> tests/qemuagenttest.c:
>   * Add new test: testQemuAgentGetInterfaces
> Test cases for IP aliases, 0 or multiple ipv4/ipv6 address(es)
> 
> Signed-off-by: Nehal J Wani 
> ---
>  src/qemu/qemu_agent.c  | 204 
> +
>  src/qemu/qemu_agent.h  |   4 +
>  src/qemu/qemu_driver.c | 175 ++
>  tests/qemuagenttest.c  | 188 +
>  4 files changed, 571 insertions(+)
> 

ACK - there's a NIT and a comment that follows, I'll let you decide how
to handle.

John
> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
> index 5fcc40f..8155006 100644
> --- a/src/qemu/qemu_agent.c
> +++ b/src/qemu/qemu_agent.c
> @@ -1953,3 +1953,207 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, 
> virDomainFSInfoPtr **info,
>  virJSONValueFree(reply);
>  return ret;
>  }
> +
> +/*
> + * qemuAgentGetInterfaces:
> + * @mon: Agent monitor
> + * @ifaces: pointer to an array of pointers pointing to interface objects
> + *
> + * Issue guest-network-get-interfaces to guest agent, which returns a
> + * list of interfaces of a running domain along with their IP and MAC
> + * addresses.
> + *
> + * Returns: number of interfaces on success, -1 on error.
> + */
> +int
> +qemuAgentGetInterfaces(qemuAgentPtr mon,
> +   virDomainInterfacePtr **ifaces)
> +{
> +int ret = -1;
> +size_t i, j;
> +int size = -1;
> +virJSONValuePtr cmd = NULL;
> +virJSONValuePtr reply = NULL;
> +virJSONValuePtr ret_array = NULL;
> +size_t ifaces_count = 0;
> +size_t addrs_count = 0;
> +virDomainInterfacePtr *ifaces_ret = NULL;
> +virHashTablePtr ifaces_store = NULL;
> +char **ifname = NULL;
> +
> +/* Hash table to handle the interface alias */
> +if (!(ifaces_store = virHashCreate(ifaces_count, NULL))) {
> +virHashFree(ifaces_store);
> +return -1;
> +}
> +
> +if (!(cmd = qemuAgentMakeCommand("guest-network-get-interfaces", NULL)))
> +goto cleanup;
> +
> +if (qemuAgentCommand(mon, cmd, &reply, false, 
> VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0 ||
> +qemuAgentCheckError(cmd, reply) < 0) {
> +goto cleanup;
> +}
> +
> +if (!(ret_array = virJSONValueObjectGet(reply, "return"))) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("qemu agent didn't provide 'return' field"));
> +goto cleanup;
> +}
> +
> +if ((size = virJSONValueArraySize(ret_array)) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("qemu agent didn't return an array of interfaces"));
> +goto cleanup;
> +}
> +
> +for (i = 0; i < size; i++) {
> +virJSONValuePtr tmp_iface = virJSONValueArrayGet(ret_array, i);
> +virJSONValuePtr ip_addr_arr = NULL;
> +const char *hwaddr, *ifname_s, *name = NULL;
> +int ip_addr_arr_size;
> +virDomainInterfacePtr iface = NULL;
> +
> +/* Shouldn't happen but doesn't hurt to check neither */
> +if (!tmp_iface) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("something has went really wrong"));
> +goto error;
> +}
> +
> +/* interface name is required to be presented */
> +name = virJSONValueObjectGetString(tmp_iface, "name");
> +if (!name) {
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("qemu agent didn't provide 'name' field"));
> +goto error;
> +}
> +
> +/* Handle interface alias (:) */
> +ifname = virStringSplit(name, ":", 2);
> +ifname_s = ifname[0];
> +
> +iface = virHashLookup(ifaces_store, ifname_s);
> +
> +/* If the hash table doesn't contain this iface, add it */
> +if (!iface) {
> +if (VIR_EXPAND_N(ifaces_ret, ifaces_count, 1) < 0)
> +goto error;
> +
> +

Re: [libvirt] [libvirt-perl PATCHv2 1/2] Add virDomainGetIOThreads and virDomainPinIOThread bindings

2015-03-16 Thread Daniel P. Berrange
On Tue, Mar 10, 2015 at 11:04:00AM -0400, John Ferlan wrote:
> Test results in the following output:
> 
> $ perl examples/iothreadsinfo.pl
> Addr
> VMM type: QEMU
> ...
> Domain: {
>   ID: 2 'f18iothr'
>   UUID: fb9f7826-b5d7-4f74-b962-7181ef3fc9ec
>   IOThread: {
> affinity: 0010
> number: 1
>   }
>   IOThread: {
> affinity: 0001
> number: 2
>   }
>   IOThread: {
> affinity: 1100
> number: 3
>   }
> }
> 
> Signed-off-by: John Ferlan 
> ---
>  Changes   |  2 +-
>  Virt.xs   | 42 ++
>  examples/iothreadsinfo.pl | 33 +
>  lib/Sys/Virt/Domain.pm| 17 +
>  4 files changed, 93 insertions(+), 1 deletion(-)
>  create mode 100644 examples/iothreadsinfo.pl
> 
> diff --git a/Changes b/Changes
> index b62ee24..88f648c 100644
> --- a/Changes
> +++ b/Changes
> @@ -2,7 +2,7 @@ Revision history for perl module Sys::Virt
>  
>  1.2.14 2015-00-00
>  
> - - XXX
> + - Add virDomainGetIOThreads and virDomainPinIOThread API bindings
>  
>  1.2.13 2015-03-05
>  
> diff --git a/Virt.xs b/Virt.xs
> index f9ec7a4..56e143d 100644
> --- a/Virt.xs
> +++ b/Virt.xs
> @@ -5014,6 +5014,48 @@ get_emulator_pin_info(dom, flags=0)
>RETVAL
>  
>  
> +void
> +get_iothread_info(dom, flags=0)
> +  virDomainPtr dom;
> +  unsigned int flags;
> + PREINIT:
> +  virDomainIOThreadInfoPtr *iothrinfo;
> +  int niothreads;
> +  int i;
> +   PPCODE:
> +  if ((niothreads = virDomainGetIOThreadsInfo(dom, &iothrinfo,
> +  flags)) < 0)
> +  _croak_error();
> +
> +  EXTEND(SP, niothreads);
> +  for (i = 0 ; i < niothreads ; i++) {
> +  HV *rec = newHV();
> +  (void)hv_store(rec, "number", 6,
> + newSViv(iothrinfo[i]->iothread_id), 0);
> +  (void)hv_store(rec, "affinity", 8,
> + newSVpvn((char*)iothrinfo[i]->cpumap,
> +  iothrinfo[i]->cpumaplen), 0);
> +  PUSHs(newRV_noinc((SV *)rec));
> +  }
> +
> +  Safefree(iothrinfo);

Opps, we should have been calling  virDomainIOThreadsInfoFree on
each element of iothrinfo, and then using free() rather than
Safefree(), since the memory was allocated by Libvirt rather
than by Perl.

> +void
> +pin_iothread(dom, iothread_id, mask, flags=0)
> + virDomainPtr dom;
> + unsigned int iothread_id;
> + SV *mask;
> + unsigned int flags;
> +PREINIT:
> + STRLEN masklen;
> + unsigned char *maps;
> + PPCODE:
> + maps = (unsigned char *)SvPV(mask, masklen);
> + if (virDomainPinVcpuFlags(dom, iothread_id, maps, masklen, flags) < 0)
> + _croak_error();

And s/VcpuFlags/IOThreads/ here.

BTW, I noticed these mistakes by running the API coverage test suite

  make test TEST_MAINTAINER=1

which reports anything in libvirt*.h that is not used from the Perl
code. I've pushed the obvious fixes.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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


[libvirt] [PATCH] PowerPC : Do not allow an empty model spec for 'host-model'

2015-03-16 Thread Prerna Saxena

[PATCH] PowerPC : Do not allow an empty model spec for 'host-model'

On PowerPC, a guest VM having CPU mode as 'host-model'
represents a 'compat' mode VM. This cannot have a NULL
CPU model.
This commit forbids such a guest definition.

Signed-off-by: Prerna Saxena 
---
 src/cpu/cpu_powerpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c
index c77374c..62ad92b 100644
--- a/src/cpu/cpu_powerpc.c
+++ b/src/cpu/cpu_powerpc.c
@@ -549,12 +549,12 @@ ppcUpdate(virCPUDefPtr guest,
   const virCPUDef *host)
 {
 switch ((virCPUMode) guest->mode) {
-case VIR_CPU_MODE_HOST_MODEL:
 case VIR_CPU_MODE_HOST_PASSTHROUGH:
 guest->match = VIR_CPU_MATCH_EXACT;
 virCPUDefFreeModel(guest);
 return virCPUDefCopyModel(guest, host, true);
 
+case VIR_CPU_MODE_HOST_MODEL:
 case VIR_CPU_MODE_CUSTOM:
 return 0;
 
-- 
1.8.3.1

-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India

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


Re: [libvirt] [PATCH 0/3] qemu: fix broken block job handling

2015-03-16 Thread Peter Krempa
On Fri, Mar 13, 2015 at 11:24:25 -0600, Eric Blake wrote:
> On 03/13/2015 10:25 AM, Peter Krempa wrote:
> > Block job handling violates our usage of domain jobs and changes disk source
> > definition behind our back.
> > 
> > Peter Krempa (3):
> >   qemu: process: Export qemuProcessFindDomainDiskByAlias
> >   qemu: event: Don't fiddle with disk backing trees without a job
> >   qemu: Disallow concurrent block jobs on a single disk
> 
> On description alone, this should help solve the crash that Shanzhi
> observed:
> https://www.redhat.com/archives/libvir-list/2015-March/msg00656.html
> 

I've pushed this series. While my suggestion to check also the length of
the backing chain reported by qemu would fix that crash as a symptom,
this fixes the reason why it happened.

Peter


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/2] target-i386: Haswell-noTSX and Broadwell-noTSX

2015-03-16 Thread Daniel P. Berrange
On Fri, Mar 13, 2015 at 04:09:57PM -0300, Eduardo Habkost wrote:
> With the Intel microcode update that removed HLE and RTM, there will be
> different kinds of Haswell and Broadwell CPUs out there: some that still
> have the HLE and RTM features, and some that don't have the HLE and RTM
> features. On both cases people may be willing to use the pc-*-2.3
> machine-types.
> 
> So, to cover both cases, introduce Haswell-noTSX and Broadwell-noTSX CPU
> models, for hosts that have Haswell and Broadwell CPUs without TSX support.
> 
> Signed-off-by: Eduardo Habkost 

The addition of Haswell-noTSX looks good to me.

I'm unclear on whether we truely need Broadwell-noTSX though. Did
Intel actually ship any Broadwell production silicon in which the
microcode disables this feature, or was it only a problem on
pre-production samples of Broadwell ? If the latter, I'd say we
don't need to have a Broadwell-noTSX model added. Perhaps Jun/Don
can confirm from Intel's side.


> ---
>  target-i386/cpu.c | 69 
> +++
>  1 file changed, 69 insertions(+)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index de3cdce..b693bab 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1073,6 +1073,39 @@ static X86CPUDefinition builtin_x86_defs[] = {
>  .model_id = "Intel Xeon E3-12xx v2 (Ivy Bridge)",
>  },
>  {
> +.name = "Haswell-noTSX",
> +.level = 0xd,
> +.vendor = CPUID_VENDOR_INTEL,
> +.family = 6,
> +.model = 60,
> +.stepping = 1,
> +.features[FEAT_1_EDX] =
> +CPUID_VME | CPUID_SSE2 | CPUID_SSE | CPUID_FXSR | CPUID_MMX |
> +CPUID_CLFLUSH | CPUID_PSE36 | CPUID_PAT | CPUID_CMOV | CPUID_MCA 
> |
> +CPUID_PGE | CPUID_MTRR | CPUID_SEP | CPUID_APIC | CPUID_CX8 |
> +CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE |
> +CPUID_DE | CPUID_FP87,
> +.features[FEAT_1_ECX] =
> +CPUID_EXT_AVX | CPUID_EXT_XSAVE | CPUID_EXT_AES |
> +CPUID_EXT_POPCNT | CPUID_EXT_X2APIC | CPUID_EXT_SSE42 |
> +CPUID_EXT_SSE41 | CPUID_EXT_CX16 | CPUID_EXT_SSSE3 |
> +CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSE3 |
> +CPUID_EXT_TSC_DEADLINE_TIMER | CPUID_EXT_FMA | CPUID_EXT_MOVBE |
> +CPUID_EXT_PCID | CPUID_EXT_F16C | CPUID_EXT_RDRAND,
> +.features[FEAT_8000_0001_EDX] =
> +CPUID_EXT2_LM | CPUID_EXT2_RDTSCP | CPUID_EXT2_NX |
> +CPUID_EXT2_SYSCALL,
> +.features[FEAT_8000_0001_ECX] =
> +CPUID_EXT3_LAHF_LM,
> +.features[FEAT_7_0_EBX] =
> +CPUID_7_0_EBX_FSGSBASE | CPUID_7_0_EBX_BMI1 |
> +CPUID_7_0_EBX_AVX2 | CPUID_7_0_EBX_SMEP |
> +CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_ERMS | CPUID_7_0_EBX_INVPCID,
> +.features[FEAT_XSAVE] =
> +CPUID_XSAVE_XSAVEOPT,
> +.xlevel = 0x800A,
> +.model_id = "Intel Core Processor (Haswell, no TSX)",
> +},{
>  .name = "Haswell",
>  .level = 0xd,
>  .vendor = CPUID_VENDOR_INTEL,
> @@ -1108,6 +1141,42 @@ static X86CPUDefinition builtin_x86_defs[] = {
>  .model_id = "Intel Core Processor (Haswell)",
>  },
>  {
> +.name = "Broadwell-noTSX",
> +.level = 0xd,
> +.vendor = CPUID_VENDOR_INTEL,
> +.family = 6,
> +.model = 61,
> +.stepping = 2,
> +.features[FEAT_1_EDX] =
> +CPUID_VME | CPUID_SSE2 | CPUID_SSE | CPUID_FXSR | CPUID_MMX |
> +CPUID_CLFLUSH | CPUID_PSE36 | CPUID_PAT | CPUID_CMOV | CPUID_MCA 
> |
> +CPUID_PGE | CPUID_MTRR | CPUID_SEP | CPUID_APIC | CPUID_CX8 |
> +CPUID_MCE | CPUID_PAE | CPUID_MSR | CPUID_TSC | CPUID_PSE |
> +CPUID_DE | CPUID_FP87,
> +.features[FEAT_1_ECX] =
> +CPUID_EXT_AVX | CPUID_EXT_XSAVE | CPUID_EXT_AES |
> +CPUID_EXT_POPCNT | CPUID_EXT_X2APIC | CPUID_EXT_SSE42 |
> +CPUID_EXT_SSE41 | CPUID_EXT_CX16 | CPUID_EXT_SSSE3 |
> +CPUID_EXT_PCLMULQDQ | CPUID_EXT_SSE3 |
> +CPUID_EXT_TSC_DEADLINE_TIMER | CPUID_EXT_FMA | CPUID_EXT_MOVBE |
> +CPUID_EXT_PCID | CPUID_EXT_F16C | CPUID_EXT_RDRAND,
> +.features[FEAT_8000_0001_EDX] =
> +CPUID_EXT2_LM | CPUID_EXT2_RDTSCP | CPUID_EXT2_NX |
> +CPUID_EXT2_SYSCALL,
> +.features[FEAT_8000_0001_ECX] =
> +CPUID_EXT3_LAHF_LM | CPUID_EXT3_3DNOWPREFETCH,
> +.features[FEAT_7_0_EBX] =
> +CPUID_7_0_EBX_FSGSBASE | CPUID_7_0_EBX_BMI1 |
> +CPUID_7_0_EBX_AVX2 | CPUID_7_0_EBX_SMEP |
> +CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_ERMS | CPUID_7_0_EBX_INVPCID |
> +CPUID_7_0_EBX_RDSEED | CPUID_7_0_EBX_ADX |
> +CPUID_7_0_EBX_SMAP,
> +.features[FEAT_XSAVE] =
> +CPUID_XSAVE_XSAVEOPT,
> +.xlevel = 0x800A,
> +.model_id = "Intel

Re: [libvirt] [PATCH 3/3] qemu: Disallow concurrent block jobs on a single disk

2015-03-16 Thread Peter Krempa
On Fri, Mar 13, 2015 at 13:36:13 -0600, Eric Blake wrote:
> On 03/13/2015 10:25 AM, Peter Krempa wrote:
> > While qemu may be prepared to do this libvirt is not. Forbid the block
> > ops until we fix our code.
> > ---
> >  src/conf/domain_conf.h |  1 +
> >  src/qemu/qemu_domain.c | 23 +++
> >  src/qemu/qemu_domain.h |  2 ++
> >  src/qemu/qemu_driver.c | 28 +---
> >  4 files changed, 39 insertions(+), 15 deletions(-)
> > 
> > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> > index ea463cb..6f2df46 100644
> > --- a/src/conf/domain_conf.h
> > +++ b/src/conf/domain_conf.h
> > @@ -664,6 +664,7 @@ struct _virDomainDiskDef {
> >  int tray_status; /* enum virDomainDiskTray */
> >  int removable; /* enum virTristateSwitch */
> > 
> > +bool blockjob;
> 
> Might be worth a FIXME comment acknowledging that a bool will be
> insufficient when we update our code to support parallel jobs.

Ok, I'll add a note.

> 
> > +
> > +bool
> > +qemuDomainDiskBlockJobIsActive(virDomainDiskDefPtr disk)
> > +{
> > +if (disk->mirror) {
> > +virReportError(VIR_ERR_BLOCK_COPY_ACTIVE,
> > +   _("disk '%s' already in active block job"),
> > +   disk->dst);
> > +
> > +return true;
> > +}
> > +
> > +if (disk->blockjob) {
> > +virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> > +   _("disk '%s' already in active block job"),
> > +   disk->dst);
> > +return true;
> > +}
> 
> Shorter as 'if (disk->mirror || disk->blockjob)', up to you if you want
> to compress the common code.

The errors have different error codes and I wanted to avoid a ternary
operator.

> 
> ACK.  Looks like you have correctly locked out blockpull, blockcopy, and
> blockcommit as the three jobs that are all mutually exclusive according
> to our current abilities.

Peter


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

Re: [libvirt] [PATCH 1/2] Revert "target-i386: Disable HLE and RTM on Haswell & Broadwell"

2015-03-16 Thread Daniel P. Berrange
On Fri, Mar 13, 2015 at 04:09:56PM -0300, Eduardo Habkost wrote:
> This reverts commit 13704e4c455770d500d6b87b117e32f0d01252c9.
> 
> With the Intel microcode update that removed HLE and RTM, there will be
> different kinds of Haswell and Broadwell CPUs out there: some that still
> have the HLE and RTM features, and some that don't have the HLE and RTM
> features. On both cases people may be willing to use the pc-*-2.3
> machine-types.
> 
> So instead of making the CPU model results confusing by making it depend
> on the machine-type, keep HLE and RTM on the existing Haswell and
> Broadwell CPU models. The plan is to introduce "Haswell-noTSX" and
> "Broadwell-noTSX" CPU models later, for people who have CPUs that don't
> have TSX feature available.
> 
> Signed-off-by: Eduardo Habkost 

Yep, in this situation we need to support both "models" of CPU, so changing
based on machine type is inappropriate in this scenario.

Reviewed-by: Daniel P. Berrange 

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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


Re: [libvirt] [PATCH] qemu: Check for negative port values in network drive configuration

2015-03-16 Thread Erik Skultety


On 02/20/2015 06:09 PM, Peter Krempa wrote:

> I think Erik's patch is actually fine in the context of the code.
> 
> Peter

OK, now pushed, thanks.

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


[libvirt] AUTHORS.in: added myself into commiters list

2015-03-16 Thread Erik Skultety

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