Re: [libvirt] [PATCH] libxl: fix unused functions

2016-08-15 Thread Roman Bogorodskiy 
  Michal Privoznik wrote:

> On 14.08.2016 04:37, Roman Bogorodskiy wrote:
> > Commit eee7bd4e introduced two functions: libxlDiskPathToID and
> > libxlDiskSectorSize.
> > 
> > However, as they're used only by code under #ifdef __linux__,
> > on non-Linux platforms it results in errors similar to this:
> > 
> >  CC   libxl/libvirt_driver_libxl_impl_la-libxl_driver.lo
> > libxl/libxl_driver.c:5263:1: error: unused function 'libxlDiskPathToID' 
> > [-Werror,-Wunused-function]
> > libxlDiskPathToID(const char *virtpath)
> > ^
> > libxl/libxl_driver.c:5312:1: error: unused function 'libxlDiskSectorSize' 
> > [-Werror,-Wunused-function]
> > libxlDiskSectorSize(int domid, int devno)
> > ^
> > 2 errors generated.
> > 
> > Fix that by moving these functions under the #ifdef __linux__ block.
> > ---
> >  src/libxl/libxl_driver.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> > index 4957072..1170062 100644
> > --- a/src/libxl/libxl_driver.c
> > +++ b/src/libxl/libxl_driver.c
> 
> ACK

Pushed, thanks!

> Michal

Roman Bogorodskiy


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

Re: [libvirt] [PATCH] tests: fix domaincapstest linking for libxl

2016-08-15 Thread Roman Bogorodskiy 
  Michal Privoznik wrote:

> On 15.08.2016 01:25, Roman Bogorodskiy wrote:
> > Commit 11567cf added some libxl tests into domaincapstest and
> > added libvirt_driver_libxl_impl.la to domaincapstest_LDADD.
> > 
> > This causes link fail on systems without GNU regex implementation:
> > 
> > gmake[2]: Entering directory '/usr/home/novel/code/libvirt/tests'
> >   CCLD domaincapstest
> >   
> > ../src/.libs/libvirt_driver_libxl_impl.a(libvirt_driver_libxl_impl_la-libxl_capabilities.o):
> >   In function `libxlMakeCapabilities':
> >   libxl/libxl_capabilities.c:(.text+0x6b2): undefined reference to
> >   `rpl_regcomp'
> >   libxl/libxl_capabilities.c:(.text+0x6d0): undefined reference to
> >   `rpl_regerror'
> >   libxl/libxl_capabilities.c:(.text+0x803): undefined reference to
> >   `rpl_regexec'
> >   libxl/libxl_capabilities.c:(.text+0xa58): undefined reference to
> >   `rpl_regfree'
> >   clang-3.8: error: linker command failed with exit code 1 (use -v to
> >   see invocation)
> > 
> > This happens because on these system it tries to use gnulib's builtin
> > regex implementation, but doesn't link to gnulib.
> > 
> > Fix by adding $(GNULIB_LIBS) along with libvirt_driver_libxl_impl.la to
> > domaincapstest_LDADD.
> > ---
> 
> ACK

Pushed, thanks!

> Michal

Roman Bogorodskiy


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

Re: [libvirt] problem with rbd auth after switch to secret objects

2016-08-15 Thread Jim Fehlig 
On 08/15/2016 03:38 PM, John Ferlan wrote:
>
> On 08/10/2016 06:01 PM, Jim Fehlig wrote:
>> Hi John,
>>
>> I've been having problems with rbd auth since the change to using qemu's 
>> secret
>> objects. E.g. when hotplugging disk config
>>
>> 
>>   
>>   > name="volumes/volume-f9c33a0a-5313-44fc-9624-c3b09ed21a57">
>> 
>>   
>>   
>> 
>>   
>>   
>> 
>>
>> libvirt issues the following monitor commands
>>
>> 2016-08-08 16:13:41.720+: 27504: info : qemuMonitorSend:1006 :
>> QEMU_MONITOR_SEND_MSG: mon=0x7f55c4000f50
>> msg={"execute":"object-add","arguments":{"qom-type":"secret","id":"virtio-disk1-secret0","props":{"data":"w6x17STyqO9tMEOpAJy9Mnx+B5R1qrsJBXZZn/uZCKU=","keyid":"masterKey0","iv":"ZAE6WkKf+jDIl9lJkXGsnQ==","format":"base64"}},"id":"libvirt-12"}
>> 2016-08-08 16:13:41.722+: 27504: debug : 
>> qemuMonitorJSONCommandWithFd:296 :
>> Send command
>> '{"execute":"human-monitor-command","arguments":{"command-line":"drive_add 
>> dummy
>> file=rbd:volumes/volume-f9c33a0a-5313-44fc-9624-c3b09ed21a57:id=cinder:auth_supported=cephx\\;none:mon_host=xxx.xx.xxx.xxx\\:6789,password-secret=virtio-disk1-secret0,format=raw,if=none,id=drive-virtio-disk1,cache=none"},"id":"libvirt-13"}'
>>
>> The latter fails with
>>
>> 2016-08-08 16:13:41.733+: 27499: debug : virJSONValueFromString:1604 :
>> string={"return": "error connecting\r\n", "id": "libvirt-13"}
>>
>> Debugging in the qemu rbd code, I found that
>>
>> secretid = qemu_opt_get(opts, "password-secret");
>>
>> in $qemu-src/block/rbd.c:qemu_rbd_create() returns NULL. The NULL secretid is
>> later passed to qemu_rbd_set_auth(), which silently returns success when
>> secretid==NULL. Later, rados_connect() fails with "error connecting" since 
>> the
>> secret was not configured.
>>
>> I'm not familiar with qemu option parsing, but it seems the
>> ...,password-secret=xxx,... associates the password-secret option parsing 
>> with
>> the drive object, whereas it needs to be associated with the rbd "file" 
>> object?
>> As a quick hack test, I made the following change in libvirt and then was 
>> able
>> to successfully attach the disk
>>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 55df23d..eb478fb 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -1287,7 +1287,7 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
>>  virBufferAddLit(buf, ",");
>>  
>>  if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) {
>> -virBufferAsprintf(buf, "password-secret=%s,",
>> +virBufferAsprintf(buf, "file.password-secret=%s,",
>>secinfo->s.aes.alias);
>>  }
>>
>> I suspect others (including yourself) have done this successfully without 
>> that
>> hack, so I'm not quite sure what the problem might be in my configuration. 
>> I'm
>> using libvirt.git master and qemu 2.6, but I didn't notice any post-2.6 
>> patches
>> that would help on the qemu side.
>>
> Was away on a beach last week - checked out!

Nice!

>   So still trying to catch
> up...  I'm CC'd Daniel since he made the qemu adjustments for this.
>
> So one question I'd have is do things work if the disk was in the domain
> at startup?

No, same problem whether hotplugging a disk or starting the domain with already
existing disk.

>   The libvirt code essentially reuses the same qemu command
> for initial startup and hotplug. The usage of "file.password-secret"
> seems to indicate to me something's amiss.

I mentioned this issue to Bruce (cc'd) and after peeking at the qemu option
parsing code, he thought networks disks using curl protocol would suffer the
same issue, but iscsi protocol would not. If so, my hack would need to be a bit
smarter.

Regards,
Jim

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

Re: [libvirt] problem with rbd auth after switch to secret objects

2016-08-15 Thread John Ferlan 


On 08/10/2016 06:01 PM, Jim Fehlig wrote:
> Hi John,
> 
> I've been having problems with rbd auth since the change to using qemu's 
> secret
> objects. E.g. when hotplugging disk config
> 
> 
>   
>name="volumes/volume-f9c33a0a-5313-44fc-9624-c3b09ed21a57">
> 
>   
>   
> 
>   
>   
> 
> 
> libvirt issues the following monitor commands
> 
> 2016-08-08 16:13:41.720+: 27504: info : qemuMonitorSend:1006 :
> QEMU_MONITOR_SEND_MSG: mon=0x7f55c4000f50
> msg={"execute":"object-add","arguments":{"qom-type":"secret","id":"virtio-disk1-secret0","props":{"data":"w6x17STyqO9tMEOpAJy9Mnx+B5R1qrsJBXZZn/uZCKU=","keyid":"masterKey0","iv":"ZAE6WkKf+jDIl9lJkXGsnQ==","format":"base64"}},"id":"libvirt-12"}
> 2016-08-08 16:13:41.722+: 27504: debug : qemuMonitorJSONCommandWithFd:296 
> :
> Send command
> '{"execute":"human-monitor-command","arguments":{"command-line":"drive_add 
> dummy
> file=rbd:volumes/volume-f9c33a0a-5313-44fc-9624-c3b09ed21a57:id=cinder:auth_supported=cephx\\;none:mon_host=xxx.xx.xxx.xxx\\:6789,password-secret=virtio-disk1-secret0,format=raw,if=none,id=drive-virtio-disk1,cache=none"},"id":"libvirt-13"}'
> 
> The latter fails with
> 
> 2016-08-08 16:13:41.733+: 27499: debug : virJSONValueFromString:1604 :
> string={"return": "error connecting\r\n", "id": "libvirt-13"}
> 
> Debugging in the qemu rbd code, I found that
> 
> secretid = qemu_opt_get(opts, "password-secret");
> 
> in $qemu-src/block/rbd.c:qemu_rbd_create() returns NULL. The NULL secretid is
> later passed to qemu_rbd_set_auth(), which silently returns success when
> secretid==NULL. Later, rados_connect() fails with "error connecting" since the
> secret was not configured.
> 
> I'm not familiar with qemu option parsing, but it seems the
> ...,password-secret=xxx,... associates the password-secret option parsing with
> the drive object, whereas it needs to be associated with the rbd "file" 
> object?
> As a quick hack test, I made the following change in libvirt and then was able
> to successfully attach the disk
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 55df23d..eb478fb 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1287,7 +1287,7 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
>  virBufferAddLit(buf, ",");
>  
>  if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) {
> -virBufferAsprintf(buf, "password-secret=%s,",
> +virBufferAsprintf(buf, "file.password-secret=%s,",
>secinfo->s.aes.alias);
>  }
> 
> I suspect others (including yourself) have done this successfully without that
> hack, so I'm not quite sure what the problem might be in my configuration. I'm
> using libvirt.git master and qemu 2.6, but I didn't notice any post-2.6 
> patches
> that would help on the qemu side.
> 

Was away on a beach last week - checked out!  So still trying to catch
up...  I'm CC'd Daniel since he made the qemu adjustments for this.

So one question I'd have is do things work if the disk was in the domain
at startup?  The libvirt code essentially reuses the same qemu command
for initial startup and hotplug. The usage of "file.password-secret"
seems to indicate to me something's amiss.

John

> Thanks for any suggestions.
> 
> Regards,
> Jim
> 

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

[libvirt] [PATCH] vz: getting bus type for containers

2016-08-15 Thread Mikhail Feoktistov 
We should query bus type for containers too, like for VM.
In openstack we add volume disk like SCSI, so we can't
hardcode SATA bus.
---
 src/vz/vz_sdk.c | 32 +---
 1 file changed, 13 insertions(+), 19 deletions(-)

diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index f81b320..c4a1c3d 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -47,7 +47,7 @@ VIR_LOG_INIT("parallels.sdk");
 static PRL_HANDLE
 prlsdkFindNetByMAC(PRL_HANDLE sdkdom, virMacAddrPtr mac);
 static PRL_HANDLE
-prlsdkGetDisk(PRL_HANDLE sdkdom, virDomainDiskDefPtr disk, bool isCt);
+prlsdkGetDisk(PRL_HANDLE sdkdom, virDomainDiskDefPtr disk);
 
 /*
  * Log error description
@@ -547,7 +547,7 @@ prlsdkAddDomainVideoInfoVm(PRL_HANDLE sdkdom, 
virDomainDefPtr def)
 }
 
 static int
-prlsdkGetDiskId(PRL_HANDLE disk, bool isCt, int *bus, char **dst)
+prlsdkGetDiskId(PRL_HANDLE disk, int *bus, char **dst)
 {
 PRL_RESULT pret;
 PRL_UINT32 pos, ifType;
@@ -555,13 +555,8 @@ prlsdkGetDiskId(PRL_HANDLE disk, bool isCt, int *bus, char 
**dst)
 pret = PrlVmDev_GetStackIndex(disk, );
 prlsdkCheckRetExit(pret, -1);
 
-/* Let physical devices added to CT look like SATA disks */
-if (isCt) {
-ifType = PMS_SATA_DEVICE;
-} else {
-pret = PrlVmDev_GetIfaceType(disk, );
-prlsdkCheckRetExit(pret, -1);
-}
+pret = PrlVmDev_GetIfaceType(disk, );
+prlsdkCheckRetExit(pret, -1);
 
 switch (ifType) {
 case PMS_IDE_DEVICE:
@@ -632,7 +627,7 @@ prlsdkGetDiskInfo(vzDriverPtr driver,
 if (*buf != '\0' && virDomainDiskSetSource(disk, buf) < 0)
 goto cleanup;
 
-if (prlsdkGetDiskId(prldisk, isCt, >bus, >dst) < 0)
+if (prlsdkGetDiskId(prldisk, >bus, >dst) < 0)
 goto cleanup;
 
 if (virDiskNameToBusDeviceIndex(disk, , ) < 0)
@@ -1598,7 +1593,7 @@ prlsdkBootOrderCheck(PRL_HANDLE sdkdom, PRL_DEVICE_TYPE 
sdkType, int sdkIndex,
 goto cleanup;
 }
 
-if (prlsdkGetDiskId(dev, false, , ) < 0)
+if (prlsdkGetDiskId(dev, , ) < 0)
 goto cleanup;
 
 if (!(bus == disk->bus && STREQ(disk->dst, dst)))
@@ -3409,7 +3404,6 @@ prlsdkFindNetByMAC(PRL_HANDLE sdkdom, virMacAddrPtr mac)
 static int prlsdkConfigureDisk(vzDriverPtr driver,
PRL_HANDLE sdkdom,
virDomainDiskDefPtr disk,
-   bool isCt,
bool create)
 {
 PRL_RESULT pret;
@@ -3432,7 +3426,7 @@ static int prlsdkConfigureDisk(vzDriverPtr driver,
 pret = PrlVmCfg_CreateVmDev(sdkdom, devType, );
 prlsdkCheckRetGoto(pret, cleanup);
 } else {
-sdkdisk = prlsdkGetDisk(sdkdom, disk, isCt);
+sdkdisk = prlsdkGetDisk(sdkdom, disk);
 if (sdkdisk == PRL_INVALID_HANDLE)
 return -1;
 }
@@ -3499,7 +3493,7 @@ static int prlsdkConfigureDisk(vzDriverPtr driver,
 }
 
 static PRL_HANDLE
-prlsdkGetDisk(PRL_HANDLE sdkdom, virDomainDiskDefPtr disk, bool isCt)
+prlsdkGetDisk(PRL_HANDLE sdkdom, virDomainDiskDefPtr disk)
 {
 PRL_RESULT pret;
 PRL_UINT32 num;
@@ -3521,7 +3515,7 @@ prlsdkGetDisk(PRL_HANDLE sdkdom, virDomainDiskDefPtr 
disk, bool isCt)
 pret = PrlVmCfg_GetDevByType(sdkdom, devType, i, );
 prlsdkCheckRetGoto(pret, error);
 
-if (prlsdkGetDiskId(sdkdisk, isCt, , ) < 0)
+if (prlsdkGetDiskId(sdkdisk, , ) < 0)
 goto error;
 
 if (disk->bus == bus && STREQ(disk->dst, dst)) {
@@ -3560,7 +3554,7 @@ prlsdkAttachDevice(vzDriverPtr driver,
 switch (dev->type) {
 case VIR_DOMAIN_DEVICE_DISK:
 if (prlsdkConfigureDisk(driver, privdom->sdkdom,
-dev->data.disk, IS_CT(dom->def), true) < 0)
+dev->data.disk, true) < 0)
 return -1;
 
 break;
@@ -3618,7 +3612,7 @@ prlsdkDetachDevice(vzDriverPtr driver,
 
 switch (dev->type) {
 case VIR_DOMAIN_DEVICE_DISK:
-sdkdev = prlsdkGetDisk(privdom->sdkdom, dev->data.disk, 
IS_CT(dom->def));
+sdkdev = prlsdkGetDisk(privdom->sdkdom, dev->data.disk);
 if (sdkdev == PRL_INVALID_HANDLE)
 goto cleanup;
 
@@ -3688,7 +3682,7 @@ prlsdkUpdateDevice(vzDriverPtr driver,
 switch (dev->type) {
 case VIR_DOMAIN_DEVICE_DISK:
 if (prlsdkConfigureDisk(driver, privdom->sdkdom, dev->data.disk,
-IS_CT(dom->def), false) < 0)
+false) < 0)
 return -1;
 
 break;
@@ -3981,7 +3975,7 @@ prlsdkDoApplyConfig(vzDriverPtr driver,
 
 for (i = 0; i < def->ndisks; i++) {
 if (prlsdkConfigureDisk(driver, sdkdom, def->disks[i],
-IS_CT(def), true) < 0)
+true) < 0)
 goto error;
 }
 
-- 
1.8.3.1

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

[libvirt] [PATCH 1/2] conf: free the ports array of a USB hub

2016-08-15 Thread Ján Tomko 
The array needs to be freed too, not just its members.

https://bugzilla.redhat.com/show_bug.cgi?id=1366097
---
 src/conf/domain_addr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index c533edb..c9cddac 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -1360,6 +1360,7 @@ virDomainUSBAddressHubFree(virDomainUSBAddressHubPtr hub)
 
 for (i = 0; i < hub->nports; i++)
 virDomainUSBAddressHubFree(hub->ports[i]);
+VIR_FREE(hub->ports);
 virBitmapFree(hub->portmap);
 VIR_FREE(hub);
 }
-- 
2.7.3

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

[libvirt] [PATCH 2/2] conf: report an error message for non-existing USB hubs

2016-08-15 Thread Ján Tomko 
If any of the devices referenced a USB hub that does not exist,
defining the domain would either fail with:
error: An error occurred, but the cause is unknown
(if only the last hub in the path is missing)
or crash.

Return a proper error instead of crashing.

https://bugzilla.redhat.com/show_bug.cgi?id=1367130
---
 src/conf/domain_addr.c|  8 
 .../qemuxml2argv-usb-hub-nonexistent.xml  | 19 +++
 tests/qemuxml2argvtest.c  |  3 +++
 3 files changed, 30 insertions(+)
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-usb-hub-nonexistent.xml

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index c9cddac..c1b5580 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -1539,6 +1539,14 @@ virDomainUSBAddressFindPort(virDomainUSBAddressSetPtr 
addrs,
 return NULL;
 }
 hub = hub->ports[portIdx];
+if (!hub) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("there is no hub at port %u in USB address bus: 
%u port: %s"),
+   info->addr.usb.port[i],
+   info->addr.usb.bus,
+   portStr);
+return NULL;
+}
 }
 
 *targetIdx = info->addr.usb.port[lastIdx] - 1;
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-usb-hub-nonexistent.xml 
b/tests/qemuxml2argvdata/qemuxml2argv-usb-hub-nonexistent.xml
new file mode 100644
index 000..2090319
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-hub-nonexistent.xml
@@ -0,0 +1,19 @@
+
+  QEMUGuest1
+  c7a5fdbd-edaf-9455-926a-d65c16db1809
+  219136
+  219136
+  1
+  
+hvm
+
+  
+  
+/usr/bin/qemu
+
+
+
+  
+
+  
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index ad0693f..e8b8cb4 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1204,6 +1204,9 @@ mymain(void)
 DO_TEST_PARSE_ERROR("usb-hub-conflict",
 QEMU_CAPS_CHARDEV, QEMU_CAPS_USB_HUB,
 QEMU_CAPS_NODEFCONFIG);
+DO_TEST_PARSE_ERROR("usb-hub-nonexistent",
+QEMU_CAPS_CHARDEV, QEMU_CAPS_USB_HUB,
+QEMU_CAPS_NODEFCONFIG);
 DO_TEST("usb-port-missing",
 QEMU_CAPS_CHARDEV, QEMU_CAPS_USB_HUB,
 QEMU_CAPS_NODEFCONFIG);
-- 
2.7.3

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

[libvirt] [PATCH 0/2] USB address assignment fixes

2016-08-15 Thread Ján Tomko 
Ján Tomko (2):
  conf: free the ports array of a USB hub
  conf: report an error message for non-existing USB hubs

 src/conf/domain_addr.c|  9 +
 .../qemuxml2argv-usb-hub-nonexistent.xml  | 19 +++
 tests/qemuxml2argvtest.c  |  3 +++
 3 files changed, 31 insertions(+)
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-usb-hub-nonexistent.xml

-- 
2.7.3

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

Re: [libvirt] [PATCH 1/2] conf: Provide error on undefined iothreadsched entry

2016-08-15 Thread John Ferlan 


On 08/15/2016 11:07 AM, Peter Krempa wrote:
> On Mon, Aug 15, 2016 at 10:55:33 -0400, John Ferlan wrote:
>> When commit id '6dfb4507' refactored where the iothreadsched data was
>> stored, the error message for when the virDomainIOThreadIDFind failed
>> to find an iothreadid ("iothreadsched attribute 'iothreads' uses
>> undefined iothread ids") was lost. This led to the possibility that
>> someone would try to use it, but receive the generic message "An error
>> occurred, but the cause is unknown".
>>
>> This patch adds the error message back so that someone will know that
>> they have an invalid configuration.
>>
>> Signed-off-by: John Ferlan 
>> ---
>>  src/conf/domain_conf.c | 6 +-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> Same as for 2/2. Code paths that care should report the error in place
> since we would report duplicate errors in cases where it can be ignored
> or is overwritten.
> 

Unlike 2/2 the error here is being checked where appropriate unless you
are advocating a generic error message in virDomainDefParseXML after
calling virDomainIOThreadSchedParse and returning a < 0 value, but no
error message set, then create error message (which doesn't seem right)
e.g.:

for (i = 0; i < n; i++) {
if (virDomainIOThreadSchedParse(nodes[i], def) < 0) {
if (!virGetLastError()) {
virReportError(..., "some error occurred"...);
}
goto error;
}


For virDomainDefGetIOThreadSched the only callers that care are Parse
and Format. NB: All callers of virDomainIOThreadIDFind care to check if
the ID was valid or not and that's where I believe the check should be.

As for patch 2/2, fair enough I can move the error message to
virDomainDefGetVcpuSched... Of course that if for some unknown reason in
the future virDomainDefGetVcpu adds a new reason for NULL return the
assumption could be lost or the error message overwritten.  Does that
then address your concern?


John

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

Re: [libvirt] [PATCH 1/2] conf: Provide error on undefined iothreadsched entry

2016-08-15 Thread Peter Krempa 
On Mon, Aug 15, 2016 at 10:55:33 -0400, John Ferlan wrote:
> When commit id '6dfb4507' refactored where the iothreadsched data was
> stored, the error message for when the virDomainIOThreadIDFind failed
> to find an iothreadid ("iothreadsched attribute 'iothreads' uses
> undefined iothread ids") was lost. This led to the possibility that
> someone would try to use it, but receive the generic message "An error
> occurred, but the cause is unknown".
> 
> This patch adds the error message back so that someone will know that
> they have an invalid configuration.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/conf/domain_conf.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)

Same as for 2/2. Code paths that care should report the error in place
since we would report duplicate errors in cases where it can be ignored
or is overwritten.

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

Re: [libvirt] [PATCH 2/2] conf: Provide error on undefined vcpusched entry

2016-08-15 Thread Peter Krempa 
On Mon, Aug 15, 2016 at 10:55:34 -0400, John Ferlan wrote:
> Commit id '9cc931f0b' removed the error message; however, it was reachable
> via the virsh edit of a domain and attempting to add a vcpusched element
> for "undefined" vcpu values.  Without the error, the generic message
> "An error occurred, but the cause is unknown" is displayed.
> 
> This is similar to bz 1366484 for the iothreadsched element.
> 
> Signed-off-by: John Ferlan 
> ---
>  src/conf/domain_conf.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 9037304..caebe99 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1467,8 +1467,12 @@ virDomainVcpuDefPtr
>  virDomainDefGetVcpu(virDomainDefPtr def,
>  unsigned int vcpu)
>  {
> -if (vcpu >= def->maxvcpus)
> +if (vcpu >= def->maxvcpus) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +   _("vCPU '%u' is not present in domain definition"),
> +   vcpu);
>  return NULL;
> +}

This error would be overwritten by the many callers that care if it's
not found and report bogus error in cases where failure is okay.

The specific code path that cares should report the error.

NACK.

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

Re: [libvirt] [PATCH] lxc: don't try to reference NULL when mounting filesystems

2016-08-15 Thread Peter Krempa 
On Mon, Aug 15, 2016 at 12:38:30 +0100, Daniel Berrange wrote:
>   
> 
> 
>   
> 
> would lead to lxcContainerMountAllFS calling STRPREFIX
> on a NLL pointer because it failed to check if fs->src->path
> was non-NULL. This is a regression caused by
> 
>   commit da665fbd4858890fbb3bbf5da2a7b6ca37bb3220
>   Author: Olga Krishtal 
>   Date:   Thu Jul 14 16:52:38 2016 +0300
> 
> filesystem: adds possibility to use storage pool as fs source
> 
> Signed-off-by: Olga Krishtal 
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  src/lxc/lxc_container.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

ACK

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

[libvirt] [PATCH 2/2] conf: Provide error on undefined vcpusched entry

2016-08-15 Thread John Ferlan 
Commit id '9cc931f0b' removed the error message; however, it was reachable
via the virsh edit of a domain and attempting to add a vcpusched element
for "undefined" vcpu values.  Without the error, the generic message
"An error occurred, but the cause is unknown" is displayed.

This is similar to bz 1366484 for the iothreadsched element.

Signed-off-by: John Ferlan 
---
 src/conf/domain_conf.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9037304..caebe99 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1467,8 +1467,12 @@ virDomainVcpuDefPtr
 virDomainDefGetVcpu(virDomainDefPtr def,
 unsigned int vcpu)
 {
-if (vcpu >= def->maxvcpus)
+if (vcpu >= def->maxvcpus) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("vCPU '%u' is not present in domain definition"),
+   vcpu);
 return NULL;
+}
 
 return def->vcpus[vcpu];
 }
-- 
2.7.4

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

[libvirt] [PATCH 0/2] Restore error messages for {iothread|vcpu}sched id out of range.

2016-08-15 Thread John Ferlan 
These patches will restore error messages indicating the vcpu or iothread
range(s) for vcpusched or iothreadsched are invalid rather than just quietly
ignoring. If someone for some unknown reason had hand edited their domain
prior to starting libvirt, they'll at least get a message indicating that
"some" domain has an invaliv vcpu/iothread value.

NB: Removal of error message occurred in the following releases:

iothreadsched: Removed in v1.3.2
vcpusched: Removed in V2.1

John Ferlan (2):
  conf: Provide error on undefined iothreadsched entry
  conf: Provide error on undefined vcpusched entry

 src/conf/domain_conf.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

-- 
2.7.4

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

[libvirt] [PATCH 1/2] conf: Provide error on undefined iothreadsched entry

2016-08-15 Thread John Ferlan 
When commit id '6dfb4507' refactored where the iothreadsched data was
stored, the error message for when the virDomainIOThreadIDFind failed
to find an iothreadid ("iothreadsched attribute 'iothreads' uses
undefined iothread ids") was lost. This led to the possibility that
someone would try to use it, but receive the generic message "An error
occurred, but the cause is unknown".

This patch adds the error message back so that someone will know that
they have an invalid configuration.

Signed-off-by: John Ferlan 
---
 src/conf/domain_conf.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 82876f3..9037304 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15490,8 +15490,12 @@ virDomainDefGetIOThreadSched(virDomainDefPtr def,
 {
 virDomainIOThreadIDDefPtr iothrinfo;
 
-if (!(iothrinfo = virDomainIOThreadIDFind(def, iothread)))
+if (!(iothrinfo = virDomainIOThreadIDFind(def, iothread))) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Cannot find 'iothread' : %u"),
+   iothread);
 return NULL;
+}
 
 return >sched;
 }
-- 
2.7.4

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

Re: [libvirt] [PATCH 1/3] Introduce node device update event as top level event

2016-08-15 Thread Cole Robinson 
On 08/11/2016 11:15 AM, Jovanka Gulicoska wrote:
> Also includes node device update event implementation for udev
> backend.
> 

Hmm, because of the way nodedev-event and event-test.c are implemented, looks
like this throws some errors via the verify() and VIR_ENUM_IMPL sanity
checking. So looks like they all need to be updated at once

> Node device Update API:
> virConnectNodeDeviceEventUpdateCallback

Actually it doesn't add this, it just uses GenericCallback. So this reference
here should be dropped from the commit message.

> ---
>  daemon/remote.c| 29 
>  include/libvirt/libvirt-nodedev.h  |  1 +
>  src/conf/node_device_event.c   | 55 
> ++
>  src/conf/node_device_event.h   |  3 +++
>  src/libvirt_private.syms   |  1 +
>  src/node_device/node_device_udev.c |  2 ++
>  src/remote/remote_driver.c | 30 +
>  src/remote/remote_protocol.x   | 13 -
>  src/remote_protocol-structs|  5 
>  9 files changed, 138 insertions(+), 1 deletion(-)
> 
> diff --git a/daemon/remote.c b/daemon/remote.c
> index 9e75472..155e9b0 100644
> --- a/daemon/remote.c
> +++ b/daemon/remote.c
> @@ -1388,8 +1388,37 @@ remoteRelayNodeDeviceEventLifecycle(virConnectPtr conn,
>  return 0;
>  }
>  
> +static int
> +remoteRelayNodeDeviceEventUpdate(virConnectPtr conn,
> + virNodeDevicePtr dev,
> + void *opaque)
> +{
> +daemonClientEventCallbackPtr callback = opaque;
> +remote_node_device_event_update_msg data;
> +
> +if (callback->callbackID < 0 ||
> +!remoteRelayNodeDeviceEventCheckACL(callback->client, conn, dev))
> +return -1;
> +
> +VIR_DEBUG("Relaying node device update event callback %d",
> +  callback->callbackID);
> +
> +/* build return data */
> +memset(, 0, sizeof(data));
> +make_nonnull_node_device(, dev);
> +data.callbackID = callback->callbackID;
> +
> +remoteDispatchObjectEventSend(callback->client, remoteProgram,
> +  REMOTE_PROC_NODE_DEVICE_EVENT_UPDATE,
> +  
> (xdrproc_t)xdr_remote_node_device_event_update_msg,
> +  );
> +
> +return 0;
> +}
> +
>  static virConnectNodeDeviceEventGenericCallback nodeDeviceEventCallbacks[] = 
> {
>  VIR_NODE_DEVICE_EVENT_CALLBACK(remoteRelayNodeDeviceEventLifecycle),
> +VIR_NODE_DEVICE_EVENT_CALLBACK(remoteRelayNodeDeviceEventUpdate),
>  };
>  
>  verify(ARRAY_CARDINALITY(nodeDeviceEventCallbacks) == 
> VIR_NODE_DEVICE_EVENT_ID_LAST);
> diff --git a/include/libvirt/libvirt-nodedev.h 
> b/include/libvirt/libvirt-nodedev.h
> index 4ab6917..4ff8b41 100644
> --- a/include/libvirt/libvirt-nodedev.h
> +++ b/include/libvirt/libvirt-nodedev.h
> @@ -138,6 +138,7 @@ int virNodeDeviceDestroy
> (virNodeDevicePtr dev);
>   */
>  typedef enum {
>  VIR_NODE_DEVICE_EVENT_ID_LIFECYCLE = 0, /* 
> virConnectNodeDeviceEventLifecycleCallback */
> +VIR_NODE_DEVICE_EVENT_ID_UPDATE = 1, /* 
> virConnectNodeDeviceEventUpdateCallback */
>  

This reference here in the comment should also be changed to GenericCallback

Otherwise the code looks good to me, so I made those adjustments, squashed
these patches together, tweaked the commit message a bit, and pushed.

Thanks,
Cole

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

Re: [libvirt] [PATCH] virsh: Fix core for cmdSecretGetValue

2016-08-15 Thread Michal Privoznik 
On 15.08.2016 14:02, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1366611
> 
> When commit id 'cb2e3e50' reworked the cmdSecretGetValue call to use
> VIR_DISPOSE_STRING for base64, it neglected to initialize the base64
> value to NULL since the cleanup: label could be reached prior to the
> base64 value being set or not.  This resulted in a core dump, adding
> the initialization will avoid the issue.
> 
> Signed-off-by: John Ferlan 
> ---
>  tools/virsh-secret.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c
> index 8588f4c..537bdd7 100644
> --- a/tools/virsh-secret.c
> +++ b/tools/virsh-secret.c
> @@ -253,7 +253,7 @@ static bool
>  cmdSecretGetValue(vshControl *ctl, const vshCmd *cmd)
>  {
>  virSecretPtr secret;
> -char *base64;
> +char *base64 = NULL;
>  unsigned char *value;
>  size_t value_size;
>  bool ret = false;
> 

ACK

Michal

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

[libvirt] [PATCH] virsh: Fix core for cmdSecretGetValue

2016-08-15 Thread John Ferlan 
https://bugzilla.redhat.com/show_bug.cgi?id=1366611

When commit id 'cb2e3e50' reworked the cmdSecretGetValue call to use
VIR_DISPOSE_STRING for base64, it neglected to initialize the base64
value to NULL since the cleanup: label could be reached prior to the
base64 value being set or not.  This resulted in a core dump, adding
the initialization will avoid the issue.

Signed-off-by: John Ferlan 
---
 tools/virsh-secret.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c
index 8588f4c..537bdd7 100644
--- a/tools/virsh-secret.c
+++ b/tools/virsh-secret.c
@@ -253,7 +253,7 @@ static bool
 cmdSecretGetValue(vshControl *ctl, const vshCmd *cmd)
 {
 virSecretPtr secret;
-char *base64;
+char *base64 = NULL;
 unsigned char *value;
 size_t value_size;
 bool ret = false;
-- 
2.7.4

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

Re: [libvirt] Drop support for old libvirt versions?

2016-08-15 Thread Andrea Bolognani 
On Mon, 2016-08-15 at 09:39 +0100, Daniel P. Berrange wrote:
> > My interpretation was that Andrea is suggesting two things:
> > 
> > 1) remove any code within libvirt that maintains compatibility when a
> > contemporary libvirtd (or application using contemporary libvirt client
> > library) is communicating with a version of libvirtd older than the support
> > cutoff. This would include things like removing certain XML during
> > migration, and (in virsh) falling back to an older deprecated API when a
> > newer API isn't available (e.g. falling back to virDomainDefineXML() when
> > virDomainDefineXMLFlags() isn't available).
> 
> I'm not in favour of dropping the virsh compat support - IMHO it is pretty
> beneficial and not really a significant maint burden to deal with it.
> 
> The migration compat doesn't seem like a particularly significnt burden
> either to be honest. This is quite different from the QEMU code where
> our command line generator has huge amounts of complexity for dealing
> with different QEMU syntaxes, particularly around the -device introduction.

It's not necessarily a huge burden; that said, I believe
there's some value in having a well-defined and explicit
cut-off point for libvirt versions like we already have for
qemu versions, especially when such a cut-off point is
defined against a moving target (eg. "libvirt and qemu
versions shipped in supported Linux distributions").

That's not to say we should necessarily go through the code
with a fine-toothed comb and get rid of all compatibility
stuff right away; however, if your changes are at odds with
some code that's kinda hacky and its only purpose is to
support migration to libvirt versions that are hardly
relevant anymore, I think it would be better to drop the
compatibility code rather than waste time updating it.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH] libxl: fix unused functions

2016-08-15 Thread Michal Privoznik 
On 14.08.2016 04:37, Roman Bogorodskiy wrote:
> Commit eee7bd4e introduced two functions: libxlDiskPathToID and
> libxlDiskSectorSize.
> 
> However, as they're used only by code under #ifdef __linux__,
> on non-Linux platforms it results in errors similar to this:
> 
>  CC   libxl/libvirt_driver_libxl_impl_la-libxl_driver.lo
> libxl/libxl_driver.c:5263:1: error: unused function 'libxlDiskPathToID' 
> [-Werror,-Wunused-function]
> libxlDiskPathToID(const char *virtpath)
> ^
> libxl/libxl_driver.c:5312:1: error: unused function 'libxlDiskSectorSize' 
> [-Werror,-Wunused-function]
> libxlDiskSectorSize(int domid, int devno)
> ^
> 2 errors generated.
> 
> Fix that by moving these functions under the #ifdef __linux__ block.
> ---
>  src/libxl/libxl_driver.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 4957072..1170062 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c

ACK

Michal

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

Re: [libvirt] [PATCH] tests: fix domaincapstest linking for libxl

2016-08-15 Thread Michal Privoznik 
On 15.08.2016 01:25, Roman Bogorodskiy wrote:
> Commit 11567cf added some libxl tests into domaincapstest and
> added libvirt_driver_libxl_impl.la to domaincapstest_LDADD.
> 
> This causes link fail on systems without GNU regex implementation:
> 
> gmake[2]: Entering directory '/usr/home/novel/code/libvirt/tests'
>   CCLD domaincapstest
>   
> ../src/.libs/libvirt_driver_libxl_impl.a(libvirt_driver_libxl_impl_la-libxl_capabilities.o):
>   In function `libxlMakeCapabilities':
>   libxl/libxl_capabilities.c:(.text+0x6b2): undefined reference to
>   `rpl_regcomp'
>   libxl/libxl_capabilities.c:(.text+0x6d0): undefined reference to
>   `rpl_regerror'
>   libxl/libxl_capabilities.c:(.text+0x803): undefined reference to
>   `rpl_regexec'
>   libxl/libxl_capabilities.c:(.text+0xa58): undefined reference to
>   `rpl_regfree'
>   clang-3.8: error: linker command failed with exit code 1 (use -v to
>   see invocation)
> 
> This happens because on these system it tries to use gnulib's builtin
> regex implementation, but doesn't link to gnulib.
> 
> Fix by adding $(GNULIB_LIBS) along with libvirt_driver_libxl_impl.la to
> domaincapstest_LDADD.
> ---

ACK

Michal

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

[libvirt] [PATCH] lxc: don't try to reference NULL when mounting filesystems

2016-08-15 Thread Daniel P. Berrange 
  


  

would lead to lxcContainerMountAllFS calling STRPREFIX
on a NLL pointer because it failed to check if fs->src->path
was non-NULL. This is a regression caused by

  commit da665fbd4858890fbb3bbf5da2a7b6ca37bb3220
  Author: Olga Krishtal 
  Date:   Thu Jul 14 16:52:38 2016 +0300

filesystem: adds possibility to use storage pool as fs source

Signed-off-by: Olga Krishtal 

Signed-off-by: Daniel P. Berrange 
---
 src/lxc/lxc_container.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index e1eb434..5357df4 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -1627,8 +1627,7 @@ static int lxcContainerMountAllFS(virDomainDefPtr vmDef,
 if (lxcContainerResolveSymlinks(vmDef->fss[i], false) < 0)
 return -1;
 
-
-if (!(vmDef->fss[i]->src &&
+if (!(vmDef->fss[i]->src && vmDef->fss[i]->src->path &&
   STRPREFIX(vmDef->fss[i]->src->path, vmDef->fss[i]->dst)) &&
 lxcContainerUnmountSubtree(vmDef->fss[i]->dst, false) < 0)
 return -1;
-- 
2.7.4

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

[libvirt] [PATCH 4/5] qemu_process: graphics: reserve port only if listen type is address or network

2016-08-15 Thread Pavel Hrdina 
Signed-off-by: Pavel Hrdina 
---
 src/qemu/qemu_process.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index d61f704..b3c6400 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4009,6 +4009,17 @@ static int
 qemuProcessGraphicsReservePorts(virQEMUDriverPtr driver,
 virDomainGraphicsDefPtr graphics)
 {
+virDomainGraphicsListenDefPtr glisten;
+
+if (graphics->nListens <= 0)
+return 0;
+
+glisten = >listens[0];
+
+if (glisten->type != VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS &&
+glisten->type != VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK)
+return 0;
+
 if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
 !graphics->data.vnc.autoport) {
 if (virPortAllocatorSetUsed(driver->remotePorts,
-- 
2.9.2

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

[libvirt] [PATCH 5/5] qemu_process: graphics: setup listen types before ports are reserved/allocated

2016-08-15 Thread Pavel Hrdina 
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1364843

Signed-off-by: Pavel Hrdina 
---
 src/qemu/qemu_process.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index b3c6400..2b857bb 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4213,6 +4213,13 @@ qemuProcessSetupGraphics(virQEMUDriverPtr driver,
 size_t i;
 int ret = -1;
 
+for (i = 0; i < vm->def->ngraphics; i++) {
+graphics = vm->def->graphics[i];
+
+if (qemuProcessGraphicsSetupListen(driver, graphics, vm) < 0)
+goto cleanup;
+}
+
 if (allocate) {
 for (i = 0; i < vm->def->ngraphics; i++) {
 graphics = vm->def->graphics[i];
@@ -4227,9 +4234,6 @@ qemuProcessSetupGraphics(virQEMUDriverPtr driver,
 
 if (qemuProcessGraphicsAllocatePorts(driver, graphics, allocate) < 0)
 goto cleanup;
-
-if (qemuProcessGraphicsSetupListen(driver, graphics, vm) < 0)
-goto cleanup;
 }
 
 ret = 0;
-- 
2.9.2

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

[libvirt] [PATCH 1/5] qemu_process: graphics: ref driver config only in function where it is used

2016-08-15 Thread Pavel Hrdina 
Signed-off-by: Pavel Hrdina data.spice.defaultMode;
+int ret = -1;
 
 bool needTLSPort = false;
 bool needPort = false;
@@ -3598,12 +3599,13 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver,
 if (needTLSPort || graphics->data.spice.tlsPort == -1)
 graphics->data.spice.tlsPort = 5902;
 
-return 0;
+ret = 0;
+goto cleanup;
 }
 
 if (needPort || graphics->data.spice.port == -1) {
 if (virPortAllocatorAcquire(driver->remotePorts, ) < 0)
-goto error;
+goto cleanup;
 
 graphics->data.spice.port = port;
 
@@ -3616,11 +3618,11 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver,
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Auto allocation of spice TLS port requested "
  "but spice TLS is disabled in qemu.conf"));
-goto error;
+goto cleanup;
 }
 
 if (virPortAllocatorAcquire(driver->remotePorts, ) < 0)
-goto error;
+goto cleanup;
 
 graphics->data.spice.tlsPort = tlsPort;
 
@@ -3628,11 +3630,12 @@ qemuProcessSPICEAllocatePorts(virQEMUDriverPtr driver,
 graphics->data.spice.tlsPortReserved = true;
 }
 
-return 0;
+ret = 0;
 
- error:
+ cleanup:
 virPortAllocatorRelease(driver->remotePorts, port);
-return -1;
+virObjectUnref(cfg);
+return ret;
 }
 
 
@@ -4070,15 +4073,17 @@ 
qemuProcessGraphicsSetupNetworkAddress(virDomainGraphicsListenDefPtr glisten,
 
 
 static int
-qemuProcessGraphicsSetupListen(virQEMUDriverConfigPtr cfg,
+qemuProcessGraphicsSetupListen(virQEMUDriverPtr driver,
virDomainGraphicsDefPtr graphics,
virDomainObjPtr vm)
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
+virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 const char *type = virDomainGraphicsTypeToString(graphics->type);
 char *listenAddr = NULL;
 bool useSocket = false;
 size_t i;
+int ret = -1;
 
 switch (graphics->type) {
 case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
@@ -4111,12 +4116,12 @@ qemuProcessGraphicsSetupListen(virQEMUDriverConfigPtr 
cfg,
 memset(glisten, 0, sizeof(virDomainGraphicsListenDef));
 if (virAsprintf(>socket, "%s/%s.sock",
 priv->libDir, type) < 0)
-return -1;
+goto cleanup;
 glisten->fromConfig = true;
 glisten->type = VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET;
 } else if (listenAddr) {
 if (VIR_STRDUP(glisten->address, listenAddr) < 0)
-return -1;
+goto cleanup;
 glisten->fromConfig = true;
 }
 }
@@ -4128,14 +4133,14 @@ qemuProcessGraphicsSetupListen(virQEMUDriverConfigPtr 
cfg,
 
 if (qemuProcessGraphicsSetupNetworkAddress(glisten,
listenAddr) < 0)
-return -1;
+goto cleanup;
 break;
 
 case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET:
 if (!glisten->socket) {
 if (virAsprintf(>socket, "%s/%s.sock",
 priv->libDir, type) < 0)
-return -1;
+goto cleanup;
 glisten->autoGenerated = true;
 }
 break;
@@ -4146,7 +4151,11 @@ qemuProcessGraphicsSetupListen(virQEMUDriverConfigPtr 
cfg,
 }
 }
 
-return 0;
+ret = 0;
+
+ cleanup:
+virObjectUnref(cfg);
+return ret;
 }
 
 
@@ -4155,7 +4164,6 @@ qemuProcessSetupGraphics(virQEMUDriverPtr driver,
  virDomainObjPtr vm,
  unsigned int flags)
 {
-virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 bool allocate = !(flags & VIR_QEMU_PROCESS_START_PRETEND);
 size_t i;
 int ret = -1;
@@ -4176,8 +4184,7 @@ qemuProcessSetupGraphics(virQEMUDriverPtr driver,

[libvirt] [PATCH 3/5] qemu_process: graphics: extract for loop out of qemuProcessGraphicsReservePorts

2016-08-15 Thread Pavel Hrdina 
Signed-off-by: Pavel Hrdina 
---
 src/qemu/qemu_process.c | 58 +
 1 file changed, 30 insertions(+), 28 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index f813bae..d61f704 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4007,37 +4007,32 @@ qemuProcessStartHook(virQEMUDriverPtr driver,
 
 static int
 qemuProcessGraphicsReservePorts(virQEMUDriverPtr driver,
-virDomainObjPtr vm)
+virDomainGraphicsDefPtr graphics)
 {
-size_t i;
+if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
+!graphics->data.vnc.autoport) {
+if (virPortAllocatorSetUsed(driver->remotePorts,
+graphics->data.vnc.port,
+true) < 0)
+return -1;
+graphics->data.vnc.portReserved = true;
 
-for (i = 0; i < vm->def->ngraphics; ++i) {
-virDomainGraphicsDefPtr graphics = vm->def->graphics[i];
-if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC &&
-!graphics->data.vnc.autoport) {
+} else if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
+   !graphics->data.spice.autoport) {
+if (graphics->data.spice.port > 0) {
 if (virPortAllocatorSetUsed(driver->remotePorts,
-graphics->data.vnc.port,
+graphics->data.spice.port,
 true) < 0)
 return -1;
-graphics->data.vnc.portReserved = true;
-
-} else if (graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE &&
-   !graphics->data.spice.autoport) {
-if (graphics->data.spice.port > 0) {
-if (virPortAllocatorSetUsed(driver->remotePorts,
-graphics->data.spice.port,
-true) < 0)
-return -1;
-graphics->data.spice.portReserved = true;
-}
+graphics->data.spice.portReserved = true;
+}
 
-if (graphics->data.spice.tlsPort > 0) {
-if (virPortAllocatorSetUsed(driver->remotePorts,
-graphics->data.spice.tlsPort,
-true) < 0)
-return -1;
-graphics->data.spice.tlsPortReserved = true;
-}
+if (graphics->data.spice.tlsPort > 0) {
+if (virPortAllocatorSetUsed(driver->remotePorts,
+graphics->data.spice.tlsPort,
+true) < 0)
+return -1;
+graphics->data.spice.tlsPortReserved = true;
 }
 }
 
@@ -4202,15 +4197,22 @@ qemuProcessSetupGraphics(virQEMUDriverPtr driver,
  virDomainObjPtr vm,
  unsigned int flags)
 {
+virDomainGraphicsDefPtr graphics;
 bool allocate = !(flags & VIR_QEMU_PROCESS_START_PRETEND);
 size_t i;
 int ret = -1;
 
-if (allocate && qemuProcessGraphicsReservePorts(driver, vm) < 0)
-goto cleanup;
+if (allocate) {
+for (i = 0; i < vm->def->ngraphics; i++) {
+graphics = vm->def->graphics[i];
+
+if (qemuProcessGraphicsReservePorts(driver, graphics) < 0)
+goto cleanup;
+}
+}
 
 for (i = 0; i < vm->def->ngraphics; ++i) {
-virDomainGraphicsDefPtr graphics = vm->def->graphics[i];
+graphics = vm->def->graphics[i];
 
 if (qemuProcessGraphicsAllocatePorts(driver, graphics, allocate) < 0)
 goto cleanup;
-- 
2.9.2

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

[libvirt] [PATCH 2/5] qemu_process: graphics: extract port allocation into function

2016-08-15 Thread Pavel Hrdina 
Signed-off-by: Pavel Hrdina 
---
 src/qemu/qemu_process.c | 61 -
 1 file changed, 40 insertions(+), 21 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 26aef5e..f813bae 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -4046,6 +4046,44 @@ qemuProcessGraphicsReservePorts(virQEMUDriverPtr driver,
 
 
 static int
+qemuProcessGraphicsAllocatePorts(virQEMUDriverPtr driver,
+ virDomainGraphicsDefPtr graphics,
+ bool allocate)
+{
+virDomainGraphicsListenDefPtr glisten;
+
+if (graphics->nListens <= 0)
+return 0;
+
+glisten = >listens[0];
+
+if (glisten->type != VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS &&
+glisten->type != VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK)
+return 0;
+
+switch (graphics->type) {
+case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
+if (qemuProcessVNCAllocatePorts(driver, graphics, allocate) < 0)
+return -1;
+break;
+
+case VIR_DOMAIN_GRAPHICS_TYPE_SPICE:
+if (qemuProcessSPICEAllocatePorts(driver, graphics, allocate) < 0)
+return -1;
+break;
+
+case VIR_DOMAIN_GRAPHICS_TYPE_SDL:
+case VIR_DOMAIN_GRAPHICS_TYPE_RDP:
+case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP:
+case VIR_DOMAIN_GRAPHICS_TYPE_LAST:
+break;
+}
+
+return 0;
+}
+
+
+static int
 qemuProcessGraphicsSetupNetworkAddress(virDomainGraphicsListenDefPtr glisten,
const char *listenAddr)
 {
@@ -4174,27 +4212,8 @@ qemuProcessSetupGraphics(virQEMUDriverPtr driver,
 for (i = 0; i < vm->def->ngraphics; ++i) {
 virDomainGraphicsDefPtr graphics = vm->def->graphics[i];
 
-if (graphics->nListens > 0 &&
-(graphics->listens[0].type == 
VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS ||
- graphics->listens[0].type == 
VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK)) {
-switch (graphics->type) {
-case VIR_DOMAIN_GRAPHICS_TYPE_VNC:
-if (qemuProcessVNCAllocatePorts(driver, graphics, allocate) < 
0)
-goto cleanup;
-break;
-
-case VIR_DOMAIN_GRAPHICS_TYPE_SPICE:
-if (qemuProcessSPICEAllocatePorts(driver, graphics, allocate) 
< 0)
-goto cleanup;
-break;
-
-case VIR_DOMAIN_GRAPHICS_TYPE_SDL:
-case VIR_DOMAIN_GRAPHICS_TYPE_RDP:
-case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP:
-case VIR_DOMAIN_GRAPHICS_TYPE_LAST:
-break;
-}
-}
+if (qemuProcessGraphicsAllocatePorts(driver, graphics, allocate) < 0)
+goto cleanup;
 
 if (qemuProcessGraphicsSetupListen(driver, graphics, vm) < 0)
 goto cleanup;
-- 
2.9.2

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

[libvirt] [PATCH 0/5] graphics cleanup and bug fix

2016-08-15 Thread Pavel Hrdina 
Pavel Hrdina (5):
  qemu_process: graphics: ref driver config only in function where it is
used
  qemu_process: graphics: extract port allocation into function
  qemu_process: graphics: extract for loop out of
qemuProcessGraphicsReservePorts
  qemu_process: graphics: reserve port only if listen type is address or
network
  qemu_process: graphics: setup listen types before ports are
reserved/allocated

 src/qemu/qemu_process.c | 174 ++--
 1 file changed, 108 insertions(+), 66 deletions(-)

-- 
2.9.2

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

Re: [libvirt] [PATCHv3 04/11] Add virtio revision attribute to controllers

2016-08-15 Thread Boris Fiuczynski 

On 08/12/2016 04:59 PM, Ján Tomko wrote:

On Fri, Aug 12, 2016 at 10:47:13AM +0200, Cornelia Huck wrote:

On Thu, 11 Aug 2016 16:17:10 +0200
Ján Tomko  wrote:


On Thu, Aug 11, 2016 at 02:31:55PM +0100, Daniel P. Berrange wrote:
>On Thu, Aug 11, 2016 at 03:25:53PM +0200, Ján Tomko wrote:
>> On Thu, Aug 11, 2016 at 01:00:08PM +0100, Daniel P. Berrange wrote:
>> > On Wed, Aug 10, 2016 at 03:27:15PM +0200, Ján Tomko wrote:
>> > > 
>> > > 
>> >
>> > I'm wondering about generalizing this. eg what if there are
>> > other device models where we want the ability to set a
>> > revision. We don't really want to invent a new sub-elment
>> > named after each device model
>>
>> Not even a new attribute? :)
>>  
>>
>> How about:
>>  
>
>Both of those are quite repetative - we already know its virtio.
>

I guess one device having s of different types is unlikely.

>Most devices we have alrady include a  or  sub-element,
>so we should really just add a revision= attrbute to those existing

What I liked about having it as a separate element is that it can be
repeated, e.g.:
  
  

for a device with both 0.9 and 1.0.

I could not come up with a nice way to represent that in a single
attribute:
* '0.9+1.0' feels like the two values should rather be separated
  at the XML level
* 'all' will not be true if there happens to be another virtio
  revision in the future


[not a libvirt developer, but let me comment from the qemu virtio
perspective]

I don't think you are expressing the concept of virtio (standard)
revisions (more like releases!) here correctly. Let me elaborate:

- The disable-legacy/disable-modern attributes are virtio-pci only.
Moreover, they don't express 'compliant to virtio-1.0' or so: They do
exactly What It Says On The Tin. A device with both disable attributes
off is in fact virtio-1.0 compliant (transitional devices are
compliant), as is a device with disable-legacy off. But it might also
be virtio-1.1 compliant! (That's the most likely release of the
standard in the near future.)

- virtio-ccw does not have the concept of these disable switches.
Instead, there are virtio-ccw specific 'revisions' which count upwards
and may be limited by the 'max_revision' attribute. However, this is
not an attribute that is supposed to be set by the user, but for
backwards compatibility only. Unlike pci, ccw has nothing to gain by
disabling legacy support.

- We may actually want to add some kind of versioning scheme to virtio
devices in future versions of the standard. But that's just a very
vague idea right now.

Am I right in assuming that you simply want to be able to control
whether your virtio-pci devices are legacy, transitional or modern?


Yes.


Then I think you'd be best off adding these as virtio-pci attributes
only and leave the concept of a 'virtio revision' for the future when
we might introduce it in the standard.



So XML like this:

or

(which could possibly be reused for other hypervisors with a similar
concept, not just QEMU and virtio)
Sorry to be a pain in the bud... but... both above proposals are 
virtio-PCI only.





was what we needed all along, but I misunderstood their purpose.


That's good to know :)

Jan


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




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

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

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

Re: [libvirt] Drop support for old libvirt versions?

2016-08-15 Thread Daniel P. Berrange 
On Sat, Aug 13, 2016 at 02:03:39PM -0400, Laine Stump wrote:
> On 08/13/2016 09:24 AM, Martin Kletzander wrote:
> > On Sat, Aug 13, 2016 at 10:16:03AM +0200, Michal Privoznik wrote:
> > > On 12.08.2016 18:25, Andrea Bolognani wrote:
> > > > 
> > > > 
> > > 
> > > I like the idea. I mean, the less versions we have to take care of the
> > > better. But before we go any further - what do you mean by dropping
> > > support? For instance, what I think of is when formatting migration XML
> > > we won't remove devices (like USB hubs or what is it) just because the
> > > other side is too old.
> > > 
> > > Or you even mean deprecating APIs?
> > > 
> > 
> > My question exactly.  What support are you talking about?  Forward
> > compatible migration support (migrating back to older libvirt versions)?
> > I don't think that's very much supported at all.  Talking about old APIs
> > (e.g. superseded and obsoleted ones)?  I don't think we can do that.
> 
> Correct. In order to remove anything from the API, we would need to
> increment the shared library .so version, which libvirt has promised to
> never do. We can only ever add to APIs (and XML).
> 
> My interpretation was that Andrea is suggesting two things:
> 
> 1) remove any code within libvirt that maintains compatibility when a
> contemporary libvirtd (or application using contemporary libvirt client
> library) is communicating with a version of libvirtd older than the support
> cutoff. This would include things like removing certain XML during
> migration, and (in virsh) falling back to an older deprecated API when a
> newer API isn't available (e.g. falling back to virDomainDefineXML() when
> virDomainDefineXMLFlags() isn't available).

I'm not in favour of dropping the virsh compat support - IMHO it is pretty
beneficial and not really a significant maint burden to deal with it.

The migration compat doesn't seem like a particularly significnt burden
either to be honest. This is quite different from the QEMU code where
our command line generator has huge amounts of complexity for dealing
with different QEMU syntaxes, particularly around the -device introduction.

> 2) stop maintaining bugfixes (including CVE fixes?) on -maint branches older
> than the support cutoff.

Well we already don't officially do releases to all old maint branches, and
we've never guaranteed we'll backport CVE to all old maint branches. Typically
I'll backport the patches far as they'll cherry pick easily and perhaps do a
few manual fixes. When the backport starts getting "hard" though I'll stop
and leave the branch unfixed.

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