Re: [libvirt] [dbus PATCH] AUTHORS: Update maintainers and original authors

[Andrea Bolognani]

On Wed, 2018-07-04 at 07:35 +0200, Pavel Hrdina wrote:
> Create section for authors that started the project and add Katerina
> into the list of primary maintainers.
> 
> Suggested-by: Andrea Bolognani 
> Signed-off-by: Pavel Hrdina 

One day I will learn to check the list before posting patches O:-)

[...]
> -The primary maintainers of libvirt-dbus are:
> +The libvirt-dbus project was initiated by:

"Initiated" doesn't sound quite right in this context, what about
"started" (which, incidentally, you used in the commit message :)
or "created"?

[...]
>  Patches have been received from:
>  
>  #authorslist#

Aside: right at the bottom of the file, we have

  ... send patches to get your name added ...

which seems quite useless to me, given that the list of
contributors is generated from the git log. I suggest dropping it,
but that's of course not in scope for this patch.

With the nit mentioned above fixed,

  Reviewed-by: Andrea Bolognani 

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH] util:Fix with process number and pid file do not match

[杜 博]

Hi， I Have do it in the last git email sending with correct author name.
在 2018-07-04 12:48:21，"杜 博"  写道：
>Bobo Du
>在 2018-07-04 12:44:22，"Michal Prívozník"  写道：
>>On 07/04/2018 04:36 AM, Chen Hanxiao wrote:
>>> 
>>> At 2018-07-03 15:36:19, "Michal Prívozník"  wrote:
 On 07/02/2018 01:08 PM, dubo163 wrote:
> From: dubobo 
>
> the libvirtd pid file is not match the os process pid number
> which is smaller than before.
>
> this would be exist if the libvirtd process coredump or the os
> process was killed which the next pid number is smaller.
>
> you can be also edit the pid file to write the longer number than
> before,then restart the libvirtd service.
>
> Signed-off-by: dubobo 

 I'm sorry, but this has to be your legal name, which I believe dubobo is
 not. Also as I was pointed out earlier, the name of the author of the
 patch has to be legal name.
>>> 
>>> Guess that a space needed between family name and given name.
>>> Such as "du bobo" , "Du Bobo" or "Bobo Du"
>>
>>Ah okay. So let me know which one you prefer and I will fix it before
>>pushing.
>>
>>Miahl

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

Re: [libvirt] [PATCH] util:Fix with process number and pid file do not match

[杜 博]

Bobo Du
在 2018-07-04 12:44:22，"Michal Prívozník"  写道：
>On 07/04/2018 04:36 AM, Chen Hanxiao wrote:
>> 
>> At 2018-07-03 15:36:19, "Michal Prívozník"  wrote:
>>> On 07/02/2018 01:08 PM, dubo163 wrote:
 From: dubobo 

 the libvirtd pid file is not match the os process pid number
 which is smaller than before.

 this would be exist if the libvirtd process coredump or the os
 process was killed which the next pid number is smaller.

 you can be also edit the pid file to write the longer number than
 before,then restart the libvirtd service.

 Signed-off-by: dubobo 
>>>
>>> I'm sorry, but this has to be your legal name, which I believe dubobo is
>>> not. Also as I was pointed out earlier, the name of the author of the
>>> patch has to be legal name.
>> 
>> Guess that a space needed between family name and given name.
>> Such as "du bobo" , "Du Bobo" or "Bobo Du"
>
>Ah okay. So let me know which one you prefer and I will fix it before
>pushing.
>
>Miahl

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

Re: [libvirt] [PATCH] util:Fix with process number and pid file do not match

[Michal Prívozník]

On 07/04/2018 04:36 AM, Chen Hanxiao wrote:
> 
> At 2018-07-03 15:36:19, "Michal Prívozník"  wrote:
>> On 07/02/2018 01:08 PM, dubo163 wrote:
>>> From: dubobo 
>>>
>>> the libvirtd pid file is not match the os process pid number
>>> which is smaller than before.
>>>
>>> this would be exist if the libvirtd process coredump or the os
>>> process was killed which the next pid number is smaller.
>>>
>>> you can be also edit the pid file to write the longer number than
>>> before,then restart the libvirtd service.
>>>
>>> Signed-off-by: dubobo 
>>
>> I'm sorry, but this has to be your legal name, which I believe dubobo is
>> not. Also as I was pointed out earlier, the name of the author of the
>> patch has to be legal name.
> 
> Guess that a space needed between family name and given name.
> Such as "du bobo" , "Du Bobo" or "Bobo Du"

Ah okay. So let me know which one you prefer and I will fix it before
pushing.

Miahl

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

[libvirt] [dbus PATCH] AUTHORS: Update maintainers list

[Andrea Bolognani]

List Katerina, who is currently the top libvirt-dbus
contributor by number of commits, among the maintainers.

Signed-off-by: Andrea Bolognani 
---
 AUTHORS.in | 1 +
 1 file changed, 1 insertion(+)

diff --git a/AUTHORS.in b/AUTHORS.in
index 988dd6a..bb09e0e 100644
--- a/AUTHORS.in
+++ b/AUTHORS.in
@@ -5,6 +5,7 @@ The primary maintainers of libvirt-dbus are:
 
 Lars Karlitski 
 Pavel Hrdina 
+Katerina Koukiou 
 
 Patches have been received from:
 
-- 
2.17.1

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

[libvirt] [dbus PATCH] AUTHORS: Update maintainers and original authors

[Pavel Hrdina]

Create section for authors that started the project and add Katerina
into the list of primary maintainers.

Suggested-by: Andrea Bolognani 
Signed-off-by: Pavel Hrdina 
---
 AUTHORS.in | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/AUTHORS.in b/AUTHORS.in
index 988dd6a..4dcd915 100644
--- a/AUTHORS.in
+++ b/AUTHORS.in
@@ -1,11 +1,16 @@
libvirt-dbus Authors

 
-The primary maintainers of libvirt-dbus are:
+The libvirt-dbus project was initiated by:
 
 Lars Karlitski 
 Pavel Hrdina 
 
+The primary maintainers of libvirt-dbus are:
+
+Katerina Koukiou 
+Pavel Hrdina 
+
 Patches have been received from:
 
 #authorslist#
-- 
2.17.1

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

Re: [libvirt] [PATCH 0/4] domain_addr: make functions static

[Pavel Hrdina]

On Tue, Jul 03, 2018 at 04:19:47PM -0400, Anya Harter wrote:
> which are unused outside domain_addr.c
> 
> The commit where the last external use was removed is linked in each
> commit when it exists
> 
> Anya Harter (4):
>   domain_addr: make virDomainPCIAddressBusIsEmpty static
>   domain_addr: make virDomainCCWAddress funcs static
>   domain_addr: make virDomainVirtioSerialAddr funcs static
>   domain_addr: make virDomainUSBAddressPortFormat static

The patch 3 could be split into two commits, one that move the code and
second that manes function static.

Reviewed-by: Pavel Hrdina  and pushed.


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

Re: [libvirt] [dbus PATCH] connect: fix g_free order in virtDBusConnectFree

[Pavel Hrdina]

On Tue, Jul 03, 2018 at 03:57:29PM -0400, Anya Harter wrote:
> 
> 
> On 07/03/2018 09:03 AM, Ján Tomko wrote:
> > On Tue, Jul 03, 2018 at 02:18:02PM +0200, Pavel Hrdina wrote:
> >> On Mon, Jul 02, 2018 at 11:42:08AM -0400, Anya Harter wrote:
> >>> so that g_free(connect->nodeDevPath) line appears in alphabetical order
> >>> with the rest of the lines
> >>>
> >>> Signed-off-by: Anya Harter 
> >>> ---
> >>>  src/connect.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> In general we don't send/merge patches that just move code without any
> >> functional reason because it pollutes git history but in this case I
> >> would say it's ok.
> >>
> > 
> > What about the gchar *nodeDevPath; declaration in connect.h?
> > It is also in a different order.
> 
> This is a good catch.
> 
> How would you do something that isn't functional as part of a different
> commit? Wouldn't it confuse someone looking at it?

So if you need to move some code in order to introduce some functional
change than the move should be done in separate commit as a preparation
for the actual change.  In that case the move is justified and
desirable, but if the move is just to make things look nice it is not
usually good idea since it pollutes the git history.

Pavel


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

Re: [libvirt] [PATCH] qemu: Added support L2 table cache for qcow2 disk.

[Allen Do]

Thanks, I'll take a look.

On Tue, Jul 3, 2018 at 9:24 PM Peter Krempa  wrote:

> On Tue, Jul 03, 2018 at 20:41:54 +0800, dujiancheng wrote:
> > From: Dujiancheng 
> >
> > dujiancheng (1):
> >   qemu: Added support L2 table cache for qcow2 disk.
> > L2 table cache can be set by the new element diskCache.
>
> > Use the following methods to set the L2 table cache
> > and cache clean interval:
> > 
> >   
> > 10240
> >   
> >   
> > 
>
>
> Since you insist on sending your own version rather than reusing the
> better previously posted version, here are a few things this
> patch is missing:
>
> - capability which allows to check whether qemu supports the new
>   features
> - cache is a property of the image and not the disk, also it needs to be
>   configurable for  so that it's usable for the upcoming
>   blockdev work
> - the documentation does not explain how it's supposed to be used.
> - the RNG schema indentation is wrong
> - the formatter for the XML should not have any logic .. e.g.
>   determining that only L2 cache is printed
> - the code itself is rather strange and does not allow multiple cache
>   levels although the data structure suggests so
> - the name for the tests are wrong since this is not disk cache but
>   image cache
> - there are no checks whether a format supporting the l2 cache is used
>   (qcow2 only)
> - make syntax-check fails:
>
> --- tests/qemuxml2argvdata/disk-cache.args  2018-07-03
> 15:29:18.386122485 +0200
> +++ -   2018-07-03 15:30:36.263098059 +0200
> @@ -22,7 +22,8 @@
>  -no-acpi \
>  -boot c \
>  -usb \
> --drive
> file=/tmp/data.img,format=raw,l2-cache-size=10240,cache-clean-interval=900,if=none,id=drive-virtio-disk0
> \
> +-drive file=/tmp/data.img,format=raw,l2-cache-size=10240,\
> +cache-clean-interval=900,if=none,id=drive-virtio-disk0 \
>  -device virtio-blk-pci,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,\
>  id=virtio-disk0 \
>  -drive file=/tmp/logs.img,format=raw,if=none,id=drive-virtio-disk1 \
> Incorrect line wrapping in tests/qemuxml2argvdata/disk-cache.args
> Use test-wrap-argv.pl to wrap test data files
>
> > Signed-off-by: dujiancheng 
>
> I think it was pointed out that we prefer real names...
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] util:Fix with process number and pid file do not match

[Bobo Du]

the libvirtd pid file is not match the os process pid number
which is smaller than before.

this would be exist if the libvirtd process coredump or the os
process was killed which the next pid number is smaller.

you can be also edit the pid file to write the longer number than
before,then restart the libvirtd service.

Signed-off-by: Bobo Du 
---
 src/util/virpidfile.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c
index 58ab29f..1a85d43 100644
--- a/src/util/virpidfile.c
+++ b/src/util/virpidfile.c
@@ -446,6 +446,14 @@ int virPidFileAcquirePath(const char *path,
 
 snprintf(pidstr, sizeof(pidstr), "%lld", (long long) pid);
 
+if (ftruncate(fd, 0) < 0) {
+virReportSystemError(errno,
+ _("Failed to truncate pid file '%s'"),
+ path);
+VIR_FORCE_CLOSE(fd);
+return -1;
+}
+
 if (safewrite(fd, pidstr, strlen(pidstr)) < 0) {
 virReportSystemError(errno,
  _("Failed to write to pid file '%s'"),
-- 
1.8.3.1

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

[libvirt] [PATCH] util:Fix with process number and pid file do not match

[Bobo Du]

Bobo Du (1):
  util:Fix with process number and pid file do not match

 src/util/virpidfile.c | 8 
 1 file changed, 8 insertions(+)

-- 
1.8.3.1

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

[libvirt] [PATCH 1/2] virsh: usb support for virsh attach-disk --address

[Han Han]

Adding usb bus address support to the optional address parameter of virsh
attach-disk. The address is used as bus:port. e.g.
usb:1:1

Signed-off-by: Han Han 
---
 tools/virsh-domain.c | 38 +-
 tools/virsh.pod  |  2 +-
 2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index e9b88f0013..5a445eff44 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -319,6 +319,7 @@ enum {
 DISK_ADDR_TYPE_SCSI,
 DISK_ADDR_TYPE_IDE,
 DISK_ADDR_TYPE_CCW,
+DISK_ADDR_TYPE_USB,
 };
 
 struct PCIAddress {
@@ -346,6 +347,11 @@ struct CCWAddress {
 unsigned int devno;
 };
 
+struct USBAddress {
+unsigned int bus;
+unsigned int port;
+};
+
 struct DiskAddress {
 int type;
 union {
@@ -353,6 +359,7 @@ struct DiskAddress {
 struct SCSIAddress scsi;
 struct IDEAddress ide;
 struct CCWAddress ccw;
+struct USBAddress usb;
 } addr;
 };
 
@@ -460,10 +467,32 @@ static int str2CCWAddress(const char *str, struct 
CCWAddress *ccwAddr)
 return 0;
 }
 
+static int str2USBAddress(const char *str, struct USBAddress *usbAddr)
+{
+char *bus, *port;
+
+if (!usbAddr)
+return -1;
+if (!str)
+return -1;
+
+bus = (char *)str;
+
+if (virStrToLong_uip(bus, , 10, >bus) != 0)
+return -1;
+
+port++;
+if (virStrToLong_uip(port, NULL, 10, >port) != 0)
+return -1;
+
+return 0;
+}
+
 /* pci address pci:.00.0x0a.0 (domain:bus:slot:function)
  * ide disk address: ide:00.00.0 (controller:bus:unit)
  * scsi disk address: scsi:00.00.0 (controller:bus:unit)
  * ccw disk address: ccw:0xfe.0. (cssid:ssid:devno)
+ * usb disk address: usb:00.00 (bus:port)
  */
 
 static int str2DiskAddress(const char *str, struct DiskAddress *diskAddr)
@@ -492,6 +521,9 @@ static int str2DiskAddress(const char *str, struct 
DiskAddress *diskAddr)
 } else if (STREQLEN(type, "ccw", addr - type)) {
 diskAddr->type = DISK_ADDR_TYPE_CCW;
 return str2CCWAddress(addr + 1, >addr.ccw);
+} else if (STREQLEN(type, "usb", addr - type)) {
+diskAddr->type = DISK_ADDR_TYPE_USB;
+return str2USBAddress(addr + 1, >addr.usb);
 }
 
 return -1;
@@ -648,8 +680,12 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
   " bus='%u' unit='%llu' />\n",
   diskAddr.addr.scsi.controller, 
diskAddr.addr.scsi.bus,
   diskAddr.addr.scsi.unit);
+} else if (diskAddr.type == DISK_ADDR_TYPE_USB) {
+virBufferAsprintf(,
+  "\n",
+  diskAddr.addr.usb.bus, 
diskAddr.addr.usb.port);
 } else {
-vshError(ctl, "%s", _("expecting a scsi:00.00.00 address."));
+vshError(ctl, "%s", _("expecting a scsi:00.00.00 or usb:00.00 
address."));
 goto cleanup;
 }
 } else if (STRPREFIX((const char *)target, "hd")) {
diff --git a/tools/virsh.pod b/tools/virsh.pod
index dc100db9f3..2ca1b8f7a2 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -3060,7 +3060,7 @@ this disk may be attached (QEMU only).
 I is the serial of disk device. I is the wwn of disk device.
 I indicates the disk needs rawio capability.
 I is the address of disk device in the form of 
pci:domain.bus.slot.function,
-scsi:controller.bus.unit, ide:controller.bus.unit or ccw:cssid.ssid.devno.
+scsi:controller.bus.unit, ide:controller.bus.unit, usb:bus:port or 
ccw:cssid.ssid.devno.
 Virtio-ccw devices must have their cssid set to 0xfe.
 I indicates specified pci address is a multifunction pci device
 address.
-- 
2.17.1

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

[libvirt] [PATCH 2/2] virsh: sata support for virsh attach-disk --address

[Han Han]

Adding sata bus address support to the optional address parameter of virsh
attach-disk. The address is used as controller.bus.unit. e.g.
sata:0.0.0

Signed-off-by: Han Han 
---
 tools/virsh-domain.c | 46 +++-
 tools/virsh.pod  |  2 +-
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 5a445eff44..3959b5475b 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -320,6 +320,7 @@ enum {
 DISK_ADDR_TYPE_IDE,
 DISK_ADDR_TYPE_CCW,
 DISK_ADDR_TYPE_USB,
+DISK_ADDR_TYPE_SATA,
 };
 
 struct PCIAddress {
@@ -352,6 +353,12 @@ struct USBAddress {
 unsigned int port;
 };
 
+struct SATAAddress {
+unsigned int controller;
+unsigned int bus;
+unsigned long long unit;
+};
+
 struct DiskAddress {
 int type;
 union {
@@ -360,6 +367,7 @@ struct DiskAddress {
 struct IDEAddress ide;
 struct CCWAddress ccw;
 struct USBAddress usb;
+struct SATAAddress sata;
 } addr;
 };
 
@@ -488,11 +496,37 @@ static int str2USBAddress(const char *str, struct 
USBAddress *usbAddr)
 return 0;
 }
 
+static int str2SATAAddress(const char *str, struct SATAAddress *sataAddr)
+{
+char *controller, *bus, *unit;
+
+if (!sataAddr)
+return -1;
+if (!str)
+return -1;
+
+controller = (char *)str;
+
+if (virStrToLong_uip(controller, , 10, >controller) != 0)
+return -1;
+
+bus++;
+if (virStrToLong_uip(bus, , 10, >bus) != 0)
+return -1;
+
+unit++;
+if (virStrToLong_ullp(unit, NULL, 10, >unit) != 0)
+return -1;
+
+return 0;
+}
+
 /* pci address pci:.00.0x0a.0 (domain:bus:slot:function)
  * ide disk address: ide:00.00.0 (controller:bus:unit)
  * scsi disk address: scsi:00.00.0 (controller:bus:unit)
  * ccw disk address: ccw:0xfe.0. (cssid:ssid:devno)
  * usb disk address: usb:00.00 (bus:port)
+ * sata disk address: sata:00.00.0 (controller:bus:unit)
  */
 
 static int str2DiskAddress(const char *str, struct DiskAddress *diskAddr)
@@ -524,6 +558,9 @@ static int str2DiskAddress(const char *str, struct 
DiskAddress *diskAddr)
 } else if (STREQLEN(type, "usb", addr - type)) {
 diskAddr->type = DISK_ADDR_TYPE_USB;
 return str2USBAddress(addr + 1, >addr.usb);
+} else if (STREQLEN(type, "sata", addr - type)) {
+diskAddr->type = DISK_ADDR_TYPE_SATA;
+return str2SATAAddress(addr + 1, >addr.sata);
 }
 
 return -1;
@@ -684,8 +721,15 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
 virBufferAsprintf(,
   "\n",
   diskAddr.addr.usb.bus, 
diskAddr.addr.usb.port);
+} else if (diskAddr.type == DISK_ADDR_TYPE_SATA) {
+virBufferAsprintf(,
+  "\n",
+  diskAddr.addr.sata.controller, 
diskAddr.addr.sata.bus,
+  diskAddr.addr.sata.unit);
 } else {
-vshError(ctl, "%s", _("expecting a scsi:00.00.00 or usb:00.00 
address."));
+vshError(ctl, "%s",
+_("expecting a scsi:00.00.00 or usb:00.00 or sata:00.00.00 
address."));
 goto cleanup;
 }
 } else if (STRPREFIX((const char *)target, "hd")) {
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 2ca1b8f7a2..9343b0d99f 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -3060,7 +3060,7 @@ this disk may be attached (QEMU only).
 I is the serial of disk device. I is the wwn of disk device.
 I indicates the disk needs rawio capability.
 I is the address of disk device in the form of 
pci:domain.bus.slot.function,
-scsi:controller.bus.unit, ide:controller.bus.unit, usb:bus:port or 
ccw:cssid.ssid.devno.
+scsi:controller.bus.unit, ide:controller.bus.unit, usb:bus:port, 
sata:controller.bus.unit or ccw:cssid.ssid.devno.
 Virtio-ccw devices must have their cssid set to 0xfe.
 I indicates specified pci address is a multifunction pci device
 address.
-- 
2.17.1

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

[libvirt] [PATCH 0/2] Usb and sata for virsh attach-disk --address

[Han Han]

Add usb and sata support for attach-disk of address parameter in virsh.

Han Han (2):
  virsh: usb support for virsh attach-disk --address
  virsh: sata support for virsh attach-disk --address

 tools/virsh-domain.c | 82 +++-
 tools/virsh.pod  |  2 +-
 2 files changed, 82 insertions(+), 2 deletions(-)

-- 
2.17.1

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

Re: [libvirt] [PATCH] util:Fix with process number and pid file do not match

[Chen Hanxiao]

At 2018-07-03 15:36:19, "Michal Prívozník"  wrote:
>On 07/02/2018 01:08 PM, dubo163 wrote:
>> From: dubobo 
>> 
>> the libvirtd pid file is not match the os process pid number
>> which is smaller than before.
>> 
>> this would be exist if the libvirtd process coredump or the os
>> process was killed which the next pid number is smaller.
>> 
>> you can be also edit the pid file to write the longer number than
>> before,then restart the libvirtd service.
>> 
>> Signed-off-by: dubobo 
>
>I'm sorry, but this has to be your legal name, which I believe dubobo is
>not. Also as I was pointed out earlier, the name of the author of the
>patch has to be legal name.

Guess that a space needed between family name and given name.
Such as "du bobo" , "Du Bobo" or "Bobo Du"

As a Chinese, the author's name is  a common given name.
One of my friend had the name with that same pronounciation : )

Regards,
- Chen

>
>> ---
>>  src/util/virpidfile.c | 6 ++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c
>> index 58ab29f..8b0ff99 100644
>> --- a/src/util/virpidfile.c
>> +++ b/src/util/virpidfile.c
>> @@ -445,6 +445,12 @@ int virPidFileAcquirePath(const char *path,
>>  }
>>  
>>  snprintf(pidstr, sizeof(pidstr), "%lld", (long long) pid);
>> +if (ftruncate(fd, 0) < 0) {
>> +VIR_FORCE_CLOSE(fd);
>> +return -1;
>
>So if ftruncate() fails, caller sees -1 but no error message. This is
>not nice because users then have no idea what went wrong. All they see
>is a failed attempt to start libvirtd. We need virReportSystemError() here.
>
>> +}
>> +
>> +lseek(fd, 0, SEEK_SET);
>
>This is pretty useless. Since open() nothing was written to/read from
>the pidfile. So we don't really need to seek in it.
>
>Michal
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list

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

[libvirt] [PATCH -v2 2/2] esx: Use ESX_VI_CHECK_ARG_LIST macro to avoid code duplication

[Marcos Paulo de Souza]

By using this macro we can avoid boilerplate code to check for arrays of
objects from ESX driver. This replacement was done using the coccinelle
script bellow:

@@
identifier ptr;
@@

-if (!ptr || *ptr) { ... }
+ESX_VI_CHECK_ARG_LIST(ptr);

Signed-off-by: Marcos Paulo de Souza 
---

 Changes from v1:
 * Use ESX_VI_CHECK_ARG_LIST macro from previous patch
 * Use coccinelle script to check for all files inside esx directory

 src/esx/esx_driver.c |   5 +-
 src/esx/esx_network_driver.c |  10 +--
 src/esx/esx_util.c   |   5 +-
 src/esx/esx_vi.c | 165 +++
 src/esx/esx_vi_methods.c |  10 +--
 src/esx/esx_vi_types.c   |  51 +++
 6 files changed, 49 insertions(+), 197 deletions(-)

diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
index 01bcc99962..06e1238385 100644
--- a/src/esx/esx_driver.c
+++ b/src/esx/esx_driver.c
@@ -626,10 +626,7 @@ esxConnectToHost(esxPrivate *priv,
 ? esxVI_ProductLine_ESX
 : esxVI_ProductLine_GSX;
 
-if (!vCenterIPAddress || *vCenterIPAddress) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
-return -1;
-}
+ESX_VI_CHECK_ARG_LIST(vCenterIPAddress);
 
 if (esxUtil_ResolveHostname(conn->uri->server, ipAddress, NI_MAXHOST) < 0)
 return -1;
diff --git a/src/esx/esx_network_driver.c b/src/esx/esx_network_driver.c
index b4f7f006d0..04118b4fa6 100644
--- a/src/esx/esx_network_driver.c
+++ b/src/esx/esx_network_driver.c
@@ -220,10 +220,7 @@ esxBandwidthToShapingPolicy(virNetDevBandwidthPtr 
bandwidth,
 {
 int result = -1;
 
-if (!shapingPolicy || *shapingPolicy) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
-return -1;
-}
+ESX_VI_CHECK_ARG_LIST(shapingPolicy);
 
 if (!bandwidth->in || !bandwidth->out ||
 bandwidth->in->average != bandwidth->out->average ||
@@ -589,10 +586,7 @@ static int
 esxShapingPolicyToBandwidth(esxVI_HostNetworkTrafficShapingPolicy 
*shapingPolicy,
 virNetDevBandwidthPtr *bandwidth)
 {
-if (!bandwidth || *bandwidth) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
-return -1;
-}
+ESX_VI_CHECK_ARG_LIST(bandwidth);
 
 if (!shapingPolicy || shapingPolicy->enabled != esxVI_Boolean_True)
 return 0;
diff --git a/src/esx/esx_util.c b/src/esx/esx_util.c
index 2dd9f78569..d7210375fa 100644
--- a/src/esx/esx_util.c
+++ b/src/esx/esx_util.c
@@ -48,10 +48,7 @@ esxUtil_ParseUri(esxUtil_ParsedUri **parsedUri, virURIPtr 
uri)
 int autoAnswer;
 char *tmp;
 
-if (!parsedUri || *parsedUri) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
-return -1;
-}
+ESX_VI_CHECK_ARG_LIST(parsedUri);
 
 if (VIR_ALLOC(*parsedUri) < 0)
 return -1;
diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
index 25fbdc7e44..d3c4f760ba 100644
--- a/src/esx/esx_vi.c
+++ b/src/esx/esx_vi.c
@@ -51,10 +51,7 @@ VIR_LOG_INIT("esx.esx_vi");
 int \
 esxVI_##_type##_Alloc(esxVI_##_type **ptrptr) \
 { \
-if (!ptrptr || *ptrptr) { \
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid 
argument")); \
-return -1; \
-} \
+ESX_VI_CHECK_ARG_LIST(ptrptr); \
  \
 if (VIR_ALLOC(*ptrptr) < 0) \
 return -1; \
@@ -372,10 +369,7 @@ esxVI_CURL_Download(esxVI_CURL *curl, const char *url, 
char **content,
 virBuffer buffer = VIR_BUFFER_INITIALIZER;
 int responseCode = 0;
 
-if (!content || *content) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
-return -1;
-}
+ESX_VI_CHECK_ARG_LIST(content);
 
 if (length && *length > 0) {
 /*
@@ -1762,10 +1756,7 @@ esxVI_List_DeepCopy(esxVI_List **destList, esxVI_List 
*srcList,
 esxVI_List *dest = NULL;
 esxVI_List *src = NULL;
 
-if (!destList || *destList) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
-return -1;
-}
+ESX_VI_CHECK_ARG_LIST(destList);
 
 for (src = srcList; src; src = src->_next) {
 if (deepCopyFunc(, src) < 0 ||
@@ -2170,10 +2161,7 @@ esxVI_LookupObjectContentByType(esxVI_Context *ctx,
 bool propertySpec_isAppended = false;
 esxVI_PropertyFilterSpec *propertyFilterSpec = NULL;
 
-if (!objectContentList || *objectContentList) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
-return -1;
-}
+ESX_VI_CHECK_ARG_LIST(objectContentList);
 
 if (esxVI_ObjectSpec_Alloc() < 0)
 return -1;
@@ -2372,10 +2360,7 @@ esxVI_GetVirtualMachineQuestionInfo
 {
 esxVI_DynamicProperty *dynamicProperty;
 
-if (!questionInfo || *questionInfo) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
-return -1;
-}
+ESX_VI_CHECK_ARG_LIST(questionInfo);
 
 for (dynamicProperty = 

[libvirt] [PATCH -v2 1/2] esx_util.h: Add ESX_VI_CHECK_ARG_LIST macro

[Marcos Paulo de Souza]

This macro avoids code duplication when checking for arrays of objects.

Signed-off-by: Marcos Paulo de Souza 
---

 Changes from v1:
 * Change VIR_ERR_INVALID_ARG to VIR_ERR_INTERNAL_ERROR (Michal)
 * Change esxVI_checkArgList to ESX_VI_CHECK_ARG_LIST (Matthias)

 src/esx/esx_util.h | 8 
 1 file changed, 8 insertions(+)

diff --git a/src/esx/esx_util.h b/src/esx/esx_util.h
index c6f14bb2d9..63602bf3cb 100644
--- a/src/esx/esx_util.h
+++ b/src/esx/esx_util.h
@@ -26,6 +26,14 @@
 # include "internal.h"
 # include "viruri.h"
 
+#define ESX_VI_CHECK_ARG_LIST(val) \
+do { \
+if (!val || *val) { \
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid 
argument")); \
+return -1; \
+} \
+} while (0)
+
 typedef struct _esxUtil_ParsedUri esxUtil_ParsedUri;
 
 struct _esxUtil_ParsedUri {
-- 
2.17.1

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

[libvirt] [PATCH -v2 0/2] esx: use coccinelle to avoid code duplication

[Marcos Paulo de Souza]

Hi guys,

in this second version I address the comments of Michal and Matthias. Changes 
from v1 are placed in each patch.
Please let me know if you have more suggestions for the approach taken.

Thanks,

Marcos Paulo de Souza (2):
  esx_util.h: Add ESX_VI_CHECK_ARG_LIST macro
  esx: Use ESX_VI_CHECK_ARG_LIST macro to avoid code duplication

 src/esx/esx_driver.c |   5 +-
 src/esx/esx_network_driver.c |  10 +--
 src/esx/esx_util.c   |   5 +-
 src/esx/esx_util.h   |   8 ++
 src/esx/esx_vi.c | 165 +++
 src/esx/esx_vi_methods.c |  10 +--
 src/esx/esx_vi_types.c   |  51 +++
 7 files changed, 57 insertions(+), 197 deletions(-)

-- 
2.17.1

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

[libvirt] [PATCH] util:Fix with process number and pid file do not match

[dubobo]

dubobo (1):
  util:Fix with process number and pid file do not match

 src/util/virpidfile.c | 8 
 1 file changed, 8 insertions(+)

-- 
1.8.3.1


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

[libvirt] [PATCH] util:Fix with process number and pid file do not match

[dubobo]

the libvirtd pid file is not match the os process pid number
which is smaller than before.

this would be exist if the libvirtd process coredump or the os
process was killed which the next pid number is smaller.

you can be also edit the pid file to write the longer number than
before,then restart the libvirtd service.

Signed-off-by: dubobo 
---
 src/util/virpidfile.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c
index 58ab29f..1a85d43 100644
--- a/src/util/virpidfile.c
+++ b/src/util/virpidfile.c
@@ -446,6 +446,14 @@ int virPidFileAcquirePath(const char *path,
 
 snprintf(pidstr, sizeof(pidstr), "%lld", (long long) pid);
 
+if (ftruncate(fd, 0) < 0) {
+virReportSystemError(errno,
+ _("Failed to truncate pid file '%s'"),
+ path);
+VIR_FORCE_CLOSE(fd);
+return -1;
+}
+
 if (safewrite(fd, pidstr, strlen(pidstr)) < 0) {
 virReportSystemError(errno,
  _("Failed to write to pid file '%s'"),
-- 
1.8.3.1


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

Re: [libvirt] [PATCH 0/2] esx: Couple of fixes

[Michal Prívozník]

On 07/03/2018 07:08 PM, Matthias Bolte wrote:
> 2018-07-03 17:03 GMT+02:00 Michal Privoznik :
>> *** BLURB HERE ***
>>
>> Michal Privoznik (2):
>>   esx: Report error in esxVI_LookupVirtualMachineByName
>>   esx: De-duplicate @virtualMachine check in esxDomainLookupByName
>>
>>  src/esx/esx_driver.c | 7 +--
>>  src/esx/esx_vi.c | 5 -
>>  2 files changed, 5 insertions(+), 7 deletions(-)
>>
> 
> ACK for both patches.
> 

Thanks, pushed.

Michal

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

[libvirt] [PATCH 4/4] domain_addr: make virDomainUSBAddressPortFormat static

[Anya Harter]

never used outside domain_addr.c

Signed-off-by: Anya Harter 
---
 src/conf/domain_addr.c   | 2 +-
 src/conf/domain_addr.h   | 3 ---
 src/libvirt_private.syms | 1 -
 3 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 33f859697b..16f7ffa928 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -1653,7 +1653,7 @@ virDomainUSBAddressPortFormatBuf(virBufferPtr buf,
 }
 
 
-char *
+static char * ATTRIBUTE_NONNULL(1)
 virDomainUSBAddressPortFormat(unsigned int *port)
 {
 virBuffer buf = VIR_BUFFER_INITIALIZER;
diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
index a94d4bda66..5ad9d8ef3d 100644
--- a/src/conf/domain_addr.h
+++ b/src/conf/domain_addr.h
@@ -234,9 +234,6 @@ void
 virDomainUSBAddressPortFormatBuf(virBufferPtr buf,
  unsigned int *port)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
-char *
-virDomainUSBAddressPortFormat(unsigned int *port)
-ATTRIBUTE_NONNULL(1);
 
 # define VIR_DOMAIN_USB_HUB_PORTS 8
 
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 0f82f3c38a..3e304907b9 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -123,7 +123,6 @@ virDomainPCIControllerModelToConnectType;
 virDomainUSBAddressAssign;
 virDomainUSBAddressCountAllPorts;
 virDomainUSBAddressEnsure;
-virDomainUSBAddressPortFormat;
 virDomainUSBAddressPortFormatBuf;
 virDomainUSBAddressPortIsValid;
 virDomainUSBAddressPresent;
-- 
2.17.1

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

[libvirt] [PATCH 0/4] domain_addr: make functions static

[Anya Harter]

which are unused outside domain_addr.c

The commit where the last external use was removed is linked in each
commit when it exists

Anya Harter (4):
  domain_addr: make virDomainPCIAddressBusIsEmpty static
  domain_addr: make virDomainCCWAddress funcs static
  domain_addr: make virDomainVirtioSerialAddr funcs static
  domain_addr: make virDomainUSBAddressPortFormat static

 src/conf/domain_addr.c   | 197 +++
 src/conf/domain_addr.h   |  39 
 src/libvirt_private.syms |   8 --
 3 files changed, 98 insertions(+), 146 deletions(-)

-- 
2.17.1

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

[libvirt] [PATCH 3/4] domain_addr: make virDomainVirtioSerialAddr funcs static

[Anya Harter]

SetCreate, SetAddControllers, Reserve

last uses of these functions outside domain_addr.c removed in commit:
40c284f0a6b53a182af7b12173bcb5dd3eb0c826

Assign

never used outside domain_addr.c

move Assign and Reserve above their first call within domain_addr.c

Signed-off-by: Anya Harter 
---
 src/conf/domain_addr.c   | 187 +++
 src/conf/domain_addr.h   |  21 -
 src/libvirt_private.syms |   4 -
 3 files changed, 93 insertions(+), 119 deletions(-)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 1922412853..33f859697b 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -1216,7 +1216,7 @@ virDomainCCWAddressSetCreateFromDomain(virDomainDefPtr 
def)
  *
  * Allocates an address set for virtio serial addresses
  */
-virDomainVirtioSerialAddrSetPtr
+static virDomainVirtioSerialAddrSetPtr
 virDomainVirtioSerialAddrSetCreate(void)
 {
 virDomainVirtioSerialAddrSetPtr ret = NULL;
@@ -1318,7 +1318,7 @@ 
virDomainVirtioSerialAddrSetAddController(virDomainVirtioSerialAddrSetPtr addrs,
  * Adds virtio serial ports of controllers present in the domain definition
  * to the address set.
  */
-int
+static int ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
 virDomainVirtioSerialAddrSetAddControllers(virDomainVirtioSerialAddrSetPtr 
addrs,
virDomainDefPtr def)
 {
@@ -1346,6 +1346,65 @@ 
virDomainVirtioSerialAddrSetFree(virDomainVirtioSerialAddrSetPtr addrs)
 }
 }
 
+/* virDomainVirtioSerialAddrReserve
+ *
+ * Reserve the virtio serial address of the device
+ *
+ * For use with virDomainDeviceInfoIterate,
+ * opaque should be the address set
+ */
+static int ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4)
+virDomainVirtioSerialAddrReserve(virDomainDefPtr def ATTRIBUTE_UNUSED,
+ virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED,
+ virDomainDeviceInfoPtr info,
+ void *data)
+{
+virDomainVirtioSerialAddrSetPtr addrs = data;
+char *str = NULL;
+int ret = -1;
+virBitmapPtr map = NULL;
+bool b;
+ssize_t i;
+
+if (!virDomainVirtioSerialAddrIsComplete(info))
+return 0;
+
+VIR_DEBUG("Reserving virtio serial %u %u", info->addr.vioserial.controller,
+  info->addr.vioserial.port);
+
+i = virDomainVirtioSerialAddrFindController(addrs, 
info->addr.vioserial.controller);
+if (i < 0) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("virtio serial controller %u is missing"),
+   info->addr.vioserial.controller);
+goto cleanup;
+}
+
+map = addrs->controllers[i]->ports;
+if (virBitmapGetBit(map, info->addr.vioserial.port, ) < 0) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("virtio serial controller %u does not have port %u"),
+   info->addr.vioserial.controller,
+   info->addr.vioserial.port);
+goto cleanup;
+}
+
+if (b) {
+virReportError(VIR_ERR_XML_ERROR,
+   _("virtio serial port %u on controller %u is already 
occupied"),
+   info->addr.vioserial.port,
+   info->addr.vioserial.controller);
+goto cleanup;
+}
+
+ignore_value(virBitmapSetBit(map, info->addr.vioserial.port));
+
+ret = 0;
+
+ cleanup:
+VIR_FREE(str);
+return ret;
+}
 
 /* virDomainVirtioSerialAddrSetCreateFromDomain
  *
@@ -1487,6 +1546,38 @@ 
virDomainVirtioSerialAddrNextFromController(virDomainVirtioSerialAddrSetPtr addr
 return 0;
 }
 
+static int ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
+virDomainVirtioSerialAddrAssign(virDomainDefPtr def,
+virDomainVirtioSerialAddrSetPtr addrs,
+virDomainDeviceInfoPtr info,
+bool allowZero,
+bool portOnly)
+{
+int ret = -1;
+virDomainDeviceInfo nfo = { 0 };
+virDomainDeviceInfoPtr ptr = allowZero ?  : info;
+
+ptr->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL;
+
+if (portOnly) {
+if (virDomainVirtioSerialAddrNextFromController(addrs,
+>addr.vioserial) 
< 0)
+goto cleanup;
+} else {
+if (virDomainVirtioSerialAddrNext(def, addrs, >addr.vioserial,
+  allowZero) < 0)
+goto cleanup;
+}
+
+if (virDomainVirtioSerialAddrReserve(NULL, NULL, ptr, addrs) < 0)
+goto cleanup;
+
+ret = 0;
+
+ cleanup:
+return ret;
+}
+
 /* virDomainVirtioSerialAddrAutoAssign
  *
  * reserve a virtio serial address of the device (if it has one)
@@ -1528,38 +1619,6 @@ virDomainVirtioSerialAddrAutoAssign(virDomainDefPtr def,
 }
 
 
-int
-virDomainVirtioSerialAddrAssign(virDomainDefPtr def,
-

[libvirt] [PATCH 1/4] domain_addr: make virDomainPCIAddressBusIsEmpty static

[Anya Harter]

never used outside domain_addr.c

Signed-off-by: Anya Harter 
---
 src/conf/domain_addr.c | 2 +-
 src/conf/domain_addr.h | 3 ---
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index eb0784cd2c..ec2dfdbbdc 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -386,7 +386,7 @@ 
virDomainPCIAddressBusIsFullyReserved(virDomainPCIAddressBusPtr bus)
 }
 
 
-bool
+static bool ATTRIBUTE_NONNULL(1)
 virDomainPCIAddressBusIsEmpty(virDomainPCIAddressBusPtr bus)
 {
 size_t i;
diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
index 776399eb63..fc87c14eba 100644
--- a/src/conf/domain_addr.h
+++ b/src/conf/domain_addr.h
@@ -146,9 +146,6 @@ int 
virDomainPCIAddressBusSetModel(virDomainPCIAddressBusPtr bus,
 bool virDomainPCIAddressBusIsFullyReserved(virDomainPCIAddressBusPtr bus)
 ATTRIBUTE_NONNULL(1);
 
-bool virDomainPCIAddressBusIsEmpty(virDomainPCIAddressBusPtr bus)
-ATTRIBUTE_NONNULL(1);
-
 bool virDomainPCIAddressSlotInUse(virDomainPCIAddressSetPtr addrs,
   virPCIDeviceAddressPtr addr)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
-- 
2.17.1

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

[libvirt] [PATCH 2/4] domain_addr: make virDomainCCWAddress funcs static

[Anya Harter]

Allocate, Validate, SetCreate

last uses of these functions outside domain_addr.c removed in commit:
7bdd06b4e103269992122603949f585fc2bfdc6

Signed-off-by: Anya Harter 
---
 src/conf/domain_addr.c   |  6 +++---
 src/conf/domain_addr.h   | 12 
 src/libvirt_private.syms |  3 ---
 3 files changed, 3 insertions(+), 18 deletions(-)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index ec2dfdbbdc..1922412853 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -1134,7 +1134,7 @@ virDomainCCWAddressAssign(virDomainDeviceInfoPtr dev,
 return ret;
 }
 
-int
+static int ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4)
 virDomainCCWAddressAllocate(virDomainDefPtr def ATTRIBUTE_UNUSED,
 virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED,
 virDomainDeviceInfoPtr info,
@@ -1143,7 +1143,7 @@ virDomainCCWAddressAllocate(virDomainDefPtr def 
ATTRIBUTE_UNUSED,
 return virDomainCCWAddressAssign(info, data, true);
 }
 
-int
+static int ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4)
 virDomainCCWAddressValidate(virDomainDefPtr def ATTRIBUTE_UNUSED,
 virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED,
 virDomainDeviceInfoPtr info,
@@ -1161,7 +1161,7 @@ void virDomainCCWAddressSetFree(virDomainCCWAddressSetPtr 
addrs)
 VIR_FREE(addrs);
 }
 
-virDomainCCWAddressSetPtr
+static virDomainCCWAddressSetPtr
 virDomainCCWAddressSetCreate(void)
 {
 virDomainCCWAddressSetPtr addrs = NULL;
diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
index fc87c14eba..fc5ddcf6f1 100644
--- a/src/conf/domain_addr.h
+++ b/src/conf/domain_addr.h
@@ -186,18 +186,6 @@ int virDomainCCWAddressAssign(virDomainDeviceInfoPtr dev,
   bool autoassign)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
 void virDomainCCWAddressSetFree(virDomainCCWAddressSetPtr addrs);
-int virDomainCCWAddressAllocate(virDomainDefPtr def,
-virDomainDeviceDefPtr dev,
-virDomainDeviceInfoPtr info,
-void *data)
-ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4);
-int virDomainCCWAddressValidate(virDomainDefPtr def,
-virDomainDeviceDefPtr dev,
-virDomainDeviceInfoPtr info,
-void *data)
-ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4);
-
-virDomainCCWAddressSetPtr virDomainCCWAddressSetCreate(void);
 
 virDomainCCWAddressSetPtr
 virDomainCCWAddressSetCreateFromDomain(virDomainDefPtr def)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 7000620b41..fef38d29fa 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -104,12 +104,9 @@ virPCIDeviceAddressParseXML;
 
 
 # conf/domain_addr.h
-virDomainCCWAddressAllocate;
 virDomainCCWAddressAssign;
-virDomainCCWAddressSetCreate;
 virDomainCCWAddressSetCreateFromDomain;
 virDomainCCWAddressSetFree;
-virDomainCCWAddressValidate;
 virDomainPCIAddressAsString;
 virDomainPCIAddressBusIsFullyReserved;
 virDomainPCIAddressBusSetModel;
-- 
2.17.1

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

Re: [libvirt] [dbus PATCH] connect: fix g_free order in virtDBusConnectFree

[Anya Harter]

On 07/03/2018 09:03 AM, Ján Tomko wrote:
> On Tue, Jul 03, 2018 at 02:18:02PM +0200, Pavel Hrdina wrote:
>> On Mon, Jul 02, 2018 at 11:42:08AM -0400, Anya Harter wrote:
>>> so that g_free(connect->nodeDevPath) line appears in alphabetical order
>>> with the rest of the lines
>>>
>>> Signed-off-by: Anya Harter 
>>> ---
>>>  src/connect.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> In general we don't send/merge patches that just move code without any
>> functional reason because it pollutes git history but in this case I
>> would say it's ok.
>>
> 
> What about the gchar *nodeDevPath; declaration in connect.h?
> It is also in a different order.

This is a good catch.

How would you do something that isn't functional as part of a different
commit? Wouldn't it confuse someone looking at it?

Thanks,

Anya

> 
> /me runs
> 
> Jano
> 
>> Reviewed-by: Pavel Hrdina  and pushed.
> 
> 
> 
>> -- 
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
> 

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

Re: [libvirt] [PATCH v2] qemu: move qemuDomainCCWAddrSetCreateFromDomain

[John Ferlan]

On 07/03/2018 11:25 AM, Anya Harter wrote:
> from src/qemu/qemu_domain_address.c to src/conf/domain_addr.c
> and rename to virDomainCCWAddressSetCreateFromDomain
> 
> (rename to have Address in full instead of Addr to follow
> the naming convention of other virDomainCCWAddress functions)
> 
> Signed-off-by: Anya Harter 
> ---
> 
> renamed function in v2 patch (reason supplied in commit message)
> 
> original patch email: 
> https://www.redhat.com/archives/libvir-list/2018-June/msg01823.html
> 
>  src/conf/domain_addr.c | 24 
>  src/conf/domain_addr.h |  4 
>  src/libvirt_private.syms   |  1 +
>  src/qemu/qemu_domain_address.c | 26 ++
>  src/qemu/qemu_domain_address.h |  4 
>  src/qemu/qemu_hotplug.c|  4 ++--
>  6 files changed, 33 insertions(+), 30 deletions(-)
> 

Reviewed-by: John Ferlan 

(and pushed)

John

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

Re: [libvirt] [PATCH 0/9] Resolve libvirtd hang on termination with connected long running client

[John Ferlan]

> 
> Is there any update so far? I’m asking because I’m still getting
> segmentation faults and hang-ups on termination of libvirtd (using the
> newest version of libvirt).
> 
> Example for a hang-up:
> ➤  bt
> #0  0x03fffca8df84 in pthread_cond_wait@@GLIBC_2.3.2 () from 
> /lib64/libpthread.so.0
> #1  0x03fffdac29ca in virCondWait (c=, m=) 
> at ../../src/util/virthread.c:154
> #2  0x03fffdac381c in virThreadPoolFree (pool=) at 
> ../../src/util/virthreadpool.c:290
> #3  0x03fffdbb21ae in virNetServerDispose (obj=0x1000cc640) at 
> ../../src/rpc/virnetserver.c:803
> #4  0x03fffda97286 in virObjectUnref (anyobj=) at 
> ../../src/util/virobject.c:350
> #5  0x03fffda97a5a in virObjectFreeHashData (opaque=, 
> name=) at ../../src/util/virobject.c:591
> #6  0x03fffda66576 in virHashFree (table=) at 
> ../../src/util/virhash.c:305
> #7  0x03fffdbaff82 in virNetDaemonDispose (obj=0x1000cc3c0) at 
> ../../src/rpc/virnetdaemon.c:105
> #8  0x03fffda97286 in virObjectUnref (anyobj=) at 
> ../../src/util/virobject.c:350
> #9  0x000100026cd6 in main (argc=, argv=) 
> at ../../src/remote/remote_daemon.c:1487
> 
> And segmentation faults happen for RPC jobs that are still running.
> 

There has been zero of my cycles spent thinking about this. Partially
because I'm busy in other areas, partially because I know Daniel is
planning changes in libvirtd
(https://www.redhat.com/archives/libvir-list/2018-May/msg01307.html),
and partially because I'm not sure I have a {reliable|simple} reproducer
(at least I don't recall).

I do still have various branches in various states of disarray that are
way behind current head (easy to happen it seems lately).

John

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

Re: [libvirt] [PATCH 1/2] virTypedParamsSerialize: minor fixes

[John Ferlan]

On 07/03/2018 06:32 AM, Marc Hartmayer wrote:
> On Thu, Jun 21, 2018 at 03:47 PM +0200, Marc Hartmayer 
>  wrote:
>> On Wed, Jun 13, 2018 at 03:11 PM +0200, John Ferlan  
>> wrote:
>>> On 06/07/2018 08:17 AM, Marc Hartmayer wrote:
 On Wed, May 09, 2018 at 09:51 PM +0200, John Ferlan  
 wrote:
> On 05/07/2018 11:24 AM, Marc Hartmayer wrote:
>> On Mon, Apr 30, 2018 at 03:20 PM +0200, John Ferlan  
>> wrote:
>>> On 04/25/2018 11:55 AM, Marc Hartmayer wrote:
 1. Don't allocate if there is nothing that needs to be
allocated. Especially as the result of calling calloc(0, ...) is
implementation-defined.
>
> uh oh - another memory recollection challenge ;-)
>
>>>
>>> Following VIR_ALLOC_N one finds :
>>>
>>> VIR_ALLOC_N(params_val, nparams)
>>>
>>> which equates to
>>>
>>> # define VIR_ALLOC_N(ptr, count) \
>>>  virAllocN(&(ptr), sizeof(*(ptr)), (count), true, \
>>>VIR_FROM_THIS, __FILE__, __FUNCTION__, __LINE__)
>>>
>>> or
>>>
>>> virAllocN(_val, sizeof(params_val), nparams, true, ...)
>>>
>>> and eventually/essentially
>>>
>>> *params_val = calloc(sizeof(params_val), nparams)
>>>
>>> If the returned value is NULL then we error w/ OOM (4th param=true).
>>>
>>> So, unless @params_val had no elements, it won't be calloc(0,...) and
>>
>> I'm talking about the case that nparams == 0 => calloc(sizeof(…), 0).
>>
>
> And I was thinking is there a specific consumer/caller of
> virTypedParamsSerialize that was passing something incorrectly.
>
>>> thus the question becomes is there a more specific path you are
>>> referencing here?
>>
>> It’s a “generic” serializer so it should work for every (valid)
>> case. What happens, for example, if params == NULL and nparams == 0? In
>> that case I would expect *remote_params_val = NULL and
>> *remote_params_len = 0.
>>
>
> Going through the callers to virTypedParamsSerialize can/does anyone
> pass params == NULL and nparams == 0?  Would that be reasonable and/or
> expected?
>
> My look didn't find any - either some caller checks that the passed
> @params != NULL (usually via virCheckNonNullArgGoto in the external API
> call) or @params is built up on the fly and wouldn't be NULL because
> there'd be an error causing failure beforehand. Although I'll admit the
> migration ones are also crazy to look at.
>
> I could have made a mistake too; hence, the is there a specific instance
> that I missed? Or perhaps this is a result of some branched/private code
> that had that error which I don't have access to?

 It was the result of private code but actually it was intended and no
 error :)

>
> To answer your "What happens, for example, if params == NULL and nparams
> == 0?", well supposedly "VIR_ALLOC_N(params_val, 0)" should return NULL,
> so params_val and thus *remote_params_val == NULL.

 Did you try what 'VIR_ALLOC_N(params_val, 0)' actually returns? At least
 for me, it doesn’t return a NULL pointer.

>>>
>>> I think I already determined that NULL isn't returned; otherwise, we
>>> would have failed w/ OOM. I do recall trying this and seeing a non NULL
>>> value returned.
>>>
>>> IIRC: I found no way into [de]serialize w/ a 0 parameter, so having that
>>> guard didn't seem necessary,
>>
>> I used the libvirt-python APIs for some testing. I called an (own) API
>> with 'None' as argument and this resulted in 0 parameters.
>>
>>> but it's been 2 months since I looked at
>>> this and that level of detail has long been flushed out of main memory
>>> cache. ;-)
>>>
 The man page for calloc-posix reads:

 “If either nelem or elsize is 0, then either a null pointer or a unique
 pointer value that can be successfully passed to free() shall be
 returned.” [1]

 [1] https://www.unix.com/man-page/POSIX/3posix/calloc/

>>>
>>> perhaps then we avoid this conundrum and take Daniel's advice in his
>>> response:
>>>
>>>"If there is any problem with this, then we should just change
>>> bootstrap.conf to use calloc-gnu instead of calloc-posix, which
>>> basically turns calloc(0) into calloc(1) for compat with glibc
>>> behaviour."
>>
>> This would still require this patch, no? At least if we agree that the
>> following example should work:
> 
> Okay, the example wouldn’t even compile… but I hope the overall message
> is clear :)
> 
>>
>> virTypedParameterPtr params;
>> int nparams;
>>
>> virTypedParamsDeserialize(NULL, 0, , );
>> assert(params == NULL && assert nparams == 0);
>>
>> Do we?
>>
> 
> Polite ping.
> 
> […ping]
> 

I hope I can politely say I've completely lost context of all of this in
my short term memory. :-).

Short answer, not sure. At least not without patch 2 of this series and
without checking each 

Re: [libvirt] [PATCH 0/2] esx: Couple of fixes

[Matthias Bolte]

2018-07-03 17:03 GMT+02:00 Michal Privoznik :
> *** BLURB HERE ***
>
> Michal Privoznik (2):
>   esx: Report error in esxVI_LookupVirtualMachineByName
>   esx: De-duplicate @virtualMachine check in esxDomainLookupByName
>
>  src/esx/esx_driver.c | 7 +--
>  src/esx/esx_vi.c | 5 -
>  2 files changed, 5 insertions(+), 7 deletions(-)
>

ACK for both patches.

-- 
Matthias Bolte
http://photron.blogspot.com

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

Re: [libvirt] [PATCH 7/9] rpc: Alter virNetDaemonQuit processing

[Marc Hartmayer]

On Fri, Jan 19, 2018 at 06:23 PM +0100, John Ferlan  wrote:
> When virNetDaemonQuit is called from libvirtd's shutdown
> handler (daemonShutdownHandler) we need to perform the quit
> in multiple steps. The first part is to "request" the quit
> and notify the NetServer's of the impending quit which causes
> the NetServers to Drain their pending queue and tell workers
> to quit and the second is wait for any currently running
> worker jobs to complete. Once the workers are complete, then
> we can cause the quit to occur.

Instead of “polling” for 'daemonServerWorkersDone' we could use a pipe
for signaling. This would also ensure that we do not remain in the
poll() call indefinitely and it avoids the timer hack.

[…snip]

--
Beste Grüße / Kind regards
   Marc Hartmayer

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


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

Re: [libvirt] [PATCH 0/9] Resolve libvirtd hang on termination with connected long running client

[Marc Hartmayer]

On Wed, Feb 14, 2018 at 11:36 PM +0100, John Ferlan  wrote:
> On 01/26/2018 03:47 AM, Nikolay Shirokovskiy wrote:
>>
>>
>> On 19.01.2018 20:23, John Ferlan wrote:
>>> RFC:
>>> https://www.redhat.com/archives/libvir-list/2018-January/msg00318.html
>>>
>>> Adjustments since RFC...
>>>
>>> Patches 1&2: No change, were already R-B'd
>>> Patch 3: Removed code as noted in code review, update commit message
>>> Patch 4: From old series removed, see below for more details
>>> Patch 9: no change
>>> NB: Patches 5-8 and 10 from Nikolay Shirokovskiy 
>>> 
>>> are removed as they seemed to not be necessary
>>>
>>> Replaced the former patch 4 with series of patches to (slowly) provide
>>> support to disable new connections, handle removing waiting jobs, causing
>>> the waiting workers to quit, and allow any running jobs to complete.
>>>
>>> As it turns out, waiting for running jobs to complete cannot be done
>>> from the virNetServerClose callbacks because that means the event loop
>>> processing done during virNetServerRun will not allow any currently
>>> long running worker job thread a means to complete.
>>
>> Hi, John.
>
> Sorry - been busy in other areas lately - trying to get back to this...
>
>>
>> So you suggest a different appoarch. Instead of introducing means to
>> help rpc threads to finish after event loop is finished (stateShutdown hook 
>> in my series)
>> you suggest to block futher new rpc processing and then waiting running
>> rpc calls to finish keeping event loop running for that purpose.
>
> Somewhere along the way in one of the reviews it was suggested to give
> the workers perhaps a few extra cycles to complete anything they might
> be doing. It was also suggested a mechanism to do that - which is what I
> tried to accomplish here.
>
> Based on that and that your mechanism was more of an "immediate
> separation" by IIRC closing the monitor connection and letting that
> close handler do whatever magic happens there.
>
> This not to say your mechanism is the wrong way to go about it, but it
> also hasn't garnered support nor have you actively attempted to champion
> it. So I presented an alternative approach.
>
>>
>> This could lead to libvirtd never finish its termination if one of
>> qemu processes do not respond for example. In case of long running
>> operation such as migration finishing can take much time. On the
>> over hand if we finish rpc threads abruptly as in case of stateShutdown hook
>> we will deal with all possible error paths on daemon finishing. May
>> be good approach is to abort long running operation, then give rpc threads
>> some time to finish as you suggest and only after that finish them abruptly 
>> to handle
>> hanging qemu etc.
>
> True, but it's also easy enough to add something to the last and updated
> patch to add the QuitTimer and force an exit without going through the
> rest of the "friendly" quit (IOW: more or less what Dan has suggested).
>
> There's an incredible amount of cycles being spent for what amounts to
> trying to be nice from a SIGTERM, SIGQUIT, or SIGINT - IOW: eventual death.
>
>
>>
>> Also in this approach we keep event loop running for rpc threads only
>> but there can be other threads that depend on the loop. For example if
>> qemu is rebooted we spawn a thread that executes qemuProcessFakeReboot
>> that sends commands to qemu (i.e. depends on event loop). We need to await
>> such threads finishing too while keep event loop running. In approach of
>> stateShutdown hook we finish all threads that uses qemu monitor by closing
>> the monitor.
>>
>> And hack with timeout in a loop... I think of a different aproach for waiting
>> rpc threads to finish their work. First let's make drain function only flush
>> job queue and take some callback which will be called when all workers will
>> be free from work (let's keep worker threads as Dan suggested). In this 
>> callback we
>> can use same technique as in virNetDaemonSignalHandler. That is make some
>> IO to set some flag in the event loop thread and cause virEventRunDefaultImpl
>> to finish and then test this flag in virNetDaemonRun.
>>
>
> Please feel free to spend as many cycles as you can making adjustments
> to what I have. I'm working through some alterations and posting a v2 of
> what I have and we'll see where it takes me/us.
>
> John

Is there any update so far? I’m asking because I’m still getting
segmentation faults and hang-ups on termination of libvirtd (using the
newest version of libvirt).

Example for a hang-up:
➤  bt
#0  0x03fffca8df84 in pthread_cond_wait@@GLIBC_2.3.2 () from 
/lib64/libpthread.so.0
#1  0x03fffdac29ca in virCondWait (c=, m=) at 
../../src/util/virthread.c:154
#2  0x03fffdac381c in virThreadPoolFree (pool=) at 
../../src/util/virthreadpool.c:290
#3  0x03fffdbb21ae in virNetServerDispose (obj=0x1000cc640) at 
../../src/rpc/virnetserver.c:803
#4  0x03fffda97286 in virObjectUnref (anyobj=) at 
../../src/util/virobject.c:350
#5  0x03fffda97a5a in 

[libvirt] [PATCH] rpm: add new nwfilter RNGs to mingw-libvirt spec file

[Daniel P . Berrangé]

Signed-off-by: Daniel P. Berrangé 
---

Pushed as a build fix

 mingw-libvirt.spec.in | 4 
 1 file changed, 4 insertions(+)

diff --git a/mingw-libvirt.spec.in b/mingw-libvirt.spec.in
index 917d2143d8..cc1e619927 100644
--- a/mingw-libvirt.spec.in
+++ b/mingw-libvirt.spec.in
@@ -248,6 +248,8 @@ rm -rf 
$RPM_BUILD_ROOT%{mingw64_libexecdir}/libvirt-guests.sh
 %{mingw32_datadir}/libvirt/schemas/networkcommon.rng
 %{mingw32_datadir}/libvirt/schemas/nodedev.rng
 %{mingw32_datadir}/libvirt/schemas/nwfilter.rng
+%{mingw32_datadir}/libvirt/schemas/nwfilter_params.rng
+%{mingw32_datadir}/libvirt/schemas/nwfilterbinding.rng
 %{mingw32_datadir}/libvirt/schemas/secret.rng
 %{mingw32_datadir}/libvirt/schemas/storagecommon.rng
 %{mingw32_datadir}/libvirt/schemas/storagepool.rng
@@ -333,6 +335,8 @@ rm -rf 
$RPM_BUILD_ROOT%{mingw64_libexecdir}/libvirt-guests.sh
 %{mingw64_datadir}/libvirt/schemas/networkcommon.rng
 %{mingw64_datadir}/libvirt/schemas/nodedev.rng
 %{mingw64_datadir}/libvirt/schemas/nwfilter.rng
+%{mingw64_datadir}/libvirt/schemas/nwfilter_params.rng
+%{mingw64_datadir}/libvirt/schemas/nwfilterbinding.rng
 %{mingw64_datadir}/libvirt/schemas/secret.rng
 %{mingw64_datadir}/libvirt/schemas/storagecommon.rng
 %{mingw64_datadir}/libvirt/schemas/storagepool.rng
-- 
2.17.1

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

[libvirt] [PATCH v2] qemu: move qemuDomainCCWAddrSetCreateFromDomain

[Anya Harter]

from src/qemu/qemu_domain_address.c to src/conf/domain_addr.c
and rename to virDomainCCWAddressSetCreateFromDomain

(rename to have Address in full instead of Addr to follow
the naming convention of other virDomainCCWAddress functions)

Signed-off-by: Anya Harter 
---

renamed function in v2 patch (reason supplied in commit message)

original patch email: 
https://www.redhat.com/archives/libvir-list/2018-June/msg01823.html

 src/conf/domain_addr.c | 24 
 src/conf/domain_addr.h |  4 
 src/libvirt_private.syms   |  1 +
 src/qemu/qemu_domain_address.c | 26 ++
 src/qemu/qemu_domain_address.h |  4 
 src/qemu/qemu_hotplug.c|  4 ++--
 6 files changed, 33 insertions(+), 30 deletions(-)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index fd66a74e9d..eb0784cd2c 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -1185,6 +1185,30 @@ virDomainCCWAddressSetCreate(void)
 }
 
 
+virDomainCCWAddressSetPtr
+virDomainCCWAddressSetCreateFromDomain(virDomainDefPtr def)
+{
+virDomainCCWAddressSetPtr addrs = NULL;
+
+if (!(addrs = virDomainCCWAddressSetCreate()))
+goto error;
+
+if (virDomainDeviceInfoIterate(def, virDomainCCWAddressValidate,
+   addrs) < 0)
+goto error;
+
+if (virDomainDeviceInfoIterate(def, virDomainCCWAddressAllocate,
+   addrs) < 0)
+goto error;
+
+return addrs;
+
+ error:
+virDomainCCWAddressSetFree(addrs);
+return NULL;
+}
+
+
 #define VIR_DOMAIN_DEFAULT_VIRTIO_SERIAL_PORTS 31
 
 
diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
index 2764070cd2..776399eb63 100644
--- a/src/conf/domain_addr.h
+++ b/src/conf/domain_addr.h
@@ -202,6 +202,10 @@ int virDomainCCWAddressValidate(virDomainDefPtr def,
 
 virDomainCCWAddressSetPtr virDomainCCWAddressSetCreate(void);
 
+virDomainCCWAddressSetPtr
+virDomainCCWAddressSetCreateFromDomain(virDomainDefPtr def)
+ATTRIBUTE_NONNULL(1);
+
 struct _virDomainVirtioSerialController {
 unsigned int idx;
 virBitmapPtr ports;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index ffe5dfd19b..7000620b41 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -107,6 +107,7 @@ virPCIDeviceAddressParseXML;
 virDomainCCWAddressAllocate;
 virDomainCCWAddressAssign;
 virDomainCCWAddressSetCreate;
+virDomainCCWAddressSetCreateFromDomain;
 virDomainCCWAddressSetFree;
 virDomainCCWAddressValidate;
 virDomainPCIAddressAsString;
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index e9f460d77a..eb11a660d7 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -376,28 +376,6 @@ qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def,
 }
 }
 
-virDomainCCWAddressSetPtr
-qemuDomainCCWAddrSetCreateFromDomain(virDomainDefPtr def)
-{
-virDomainCCWAddressSetPtr addrs = NULL;
-
-if (!(addrs = virDomainCCWAddressSetCreate()))
-goto error;
-
-if (virDomainDeviceInfoIterate(def, virDomainCCWAddressValidate,
-   addrs) < 0)
-goto error;
-
-if (virDomainDeviceInfoIterate(def, virDomainCCWAddressAllocate,
-   addrs) < 0)
-goto error;
-
-return addrs;
-
- error:
-virDomainCCWAddressSetFree(addrs);
-return NULL;
-}
 
 /*
  * Three steps populating CCW devnos
@@ -420,7 +398,7 @@ qemuDomainAssignS390Addresses(virDomainDefPtr def,
 qemuDomainPrimeVirtioDeviceAddresses(
 def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW);
 
-if (!(addrs = qemuDomainCCWAddrSetCreateFromDomain(def)))
+if (!(addrs = virDomainCCWAddressSetCreateFromDomain(def)))
 goto cleanup;
 
 } else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_S390)) {
@@ -3027,7 +3005,7 @@ qemuDomainEnsureVirtioAddress(bool *releaseAddr,
 }
 
 if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
-if (!(ccwaddrs = qemuDomainCCWAddrSetCreateFromDomain(vm->def)))
+if (!(ccwaddrs = virDomainCCWAddressSetCreateFromDomain(vm->def)))
 goto cleanup;
 if (virDomainCCWAddressAssign(info, ccwaddrs,
   !info->addr.ccw.assigned) < 0)
diff --git a/src/qemu/qemu_domain_address.h b/src/qemu/qemu_domain_address.h
index 83f8e81cad..89d7a5ac3e 100644
--- a/src/qemu/qemu_domain_address.h
+++ b/src/qemu/qemu_domain_address.h
@@ -59,10 +59,6 @@ void qemuDomainReleaseDeviceAddress(virDomainObjPtr vm,
 virDomainDeviceInfoPtr info,
 const char *devstr);
 
-virDomainCCWAddressSetPtr
-qemuDomainCCWAddrSetCreateFromDomain(virDomainDefPtr def)
-ATTRIBUTE_NONNULL(1);
-
 int qemuDomainAssignMemoryDeviceSlot(virDomainDefPtr def,
  virDomainMemoryDefPtr mem);
 
diff --git a/src/qemu/qemu_hotplug.c 

Re: [libvirt] [PATCH] util:Fix with process number and pid file do not match

[Michal Prívozník]

On 07/03/2018 01:51 PM, dubo163 wrote:
> the libvirtd pid file is not match the os process pid number
> which is smaller than before.
> 
> this would be exist if the libvirtd process coredump or the os
> process was killed which the next pid number is smaller.
> 
> you can be also edit the pid file to write the longer number than
> before,then restart the libvirtd service.
> 
> Signed-off-by: dubo163 

The patch looks good, but you need to use your real name for both patch
authorship and S-o-b line.

Michal

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

Re: [libvirt] [PATCH 1/2] qemu: move qemuDomainCCWAddrSetCreateFromDomain

[Anya Harter]

On 07/02/2018 04:48 PM, John Ferlan wrote:
> 
> 
> On 06/28/2018 04:23 PM, Anya Harter wrote:
>> from src/qemu/qemu_domain_address.[ch] to src/conf/domain_addr.[ch]
>>
>> Signed-off-by: Anya Harter 
>> ---
>>  src/conf/domain_addr.c | 24 
>>  src/conf/domain_addr.h |  4 
>>  src/qemu/qemu_domain_address.c | 22 --
>>  src/qemu/qemu_domain_address.h |  4 
>>  4 files changed, 28 insertions(+), 26 deletions(-)
>>
> 
> While I can appreciate the 2 patch approach for ease of review, patch 1
> and patch 2 should be combined. Once you move a function into src/conf/*
> - give it the vir* prefix.
> 
> Since you now have a function inside src/conf called from a driver, you
> must add the function to src/libvirt_private.syms (otherwise your build
> fails - at least mine did).
> 

Woops! I missed that step.

> Additionally, once patches are combined virDomainCCWAddressSetCreate,
> virDomainCCWAddressValidate, and virDomainCCWAddressAllocate (moved in
> this patch) all become local/static to domain_addr and thus can be
> removed from domain_addr.h and libvirt_private.syms

I'm going to do this as part of a separate patch series which makes
several functions static which are unused elsewhere.

> 
> John
> 
> BTW: The ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) from the .h file that
> would then be "lost" could be added after the "static int" in the .c
> file, although that's not a requirement. We're finding those are really
> only useful if someone pass NULL as an argument and less useful if an
> argument itself is NULL (which isn't the case here).

I will keep this in mind for my other patch series.

Thanks,

Anya

> 
>> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
>> index 39f22b82eb..6c39608e01 100644
>> --- a/src/conf/domain_addr.c
>> +++ b/src/conf/domain_addr.c
>> @@ -1209,6 +1209,30 @@ virDomainCCWAddressSetCreate(void)
>>  }
>>  
>>  
>> +virDomainCCWAddressSetPtr
>> +qemuDomainCCWAddrSetCreateFromDomain(virDomainDefPtr def)
>> +{
>> +virDomainCCWAddressSetPtr addrs = NULL;
>> +
>> +if (!(addrs = virDomainCCWAddressSetCreate()))
>> +goto error;
>> +
>> +if (virDomainDeviceInfoIterate(def, virDomainCCWAddressValidate,
>> +   addrs) < 0)
>> +goto error;
>> +
>> +if (virDomainDeviceInfoIterate(def, virDomainCCWAddressAllocate,
>> +   addrs) < 0)
>> +goto error;
>> +
>> +return addrs;
>> +
>> + error:
>> +virDomainCCWAddressSetFree(addrs);
>> +return NULL;
>> +}
>> +
>> +
>>  #define VIR_DOMAIN_DEFAULT_VIRTIO_SERIAL_PORTS 31
>>  
>>  
>> diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
>> index 3236b7d6de..039efcaaf1 100644
>> --- a/src/conf/domain_addr.h
>> +++ b/src/conf/domain_addr.h
>> @@ -205,6 +205,10 @@ int 
>> virDomainCCWAddressReleaseAddr(virDomainCCWAddressSetPtr addrs,
>>  ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>>  virDomainCCWAddressSetPtr virDomainCCWAddressSetCreate(void);
>>  
>> +virDomainCCWAddressSetPtr
>> +qemuDomainCCWAddrSetCreateFromDomain(virDomainDefPtr def)
>> +ATTRIBUTE_NONNULL(1);
>> +
>>  struct _virDomainVirtioSerialController {
>>  unsigned int idx;
>>  virBitmapPtr ports;
>> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
>> index e9f460d77a..b1aef64d89 100644
>> --- a/src/qemu/qemu_domain_address.c
>> +++ b/src/qemu/qemu_domain_address.c
>> @@ -376,28 +376,6 @@ qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr 
>> def,
>>  }
>>  }
>>  
>> -virDomainCCWAddressSetPtr
>> -qemuDomainCCWAddrSetCreateFromDomain(virDomainDefPtr def)
>> -{
>> -virDomainCCWAddressSetPtr addrs = NULL;
>> -
>> -if (!(addrs = virDomainCCWAddressSetCreate()))
>> -goto error;
>> -
>> -if (virDomainDeviceInfoIterate(def, virDomainCCWAddressValidate,
>> -   addrs) < 0)
>> -goto error;
>> -
>> -if (virDomainDeviceInfoIterate(def, virDomainCCWAddressAllocate,
>> -   addrs) < 0)
>> -goto error;
>> -
>> -return addrs;
>> -
>> - error:
>> -virDomainCCWAddressSetFree(addrs);
>> -return NULL;
>> -}
>>  
>>  /*
>>   * Three steps populating CCW devnos
>> diff --git a/src/qemu/qemu_domain_address.h b/src/qemu/qemu_domain_address.h
>> index 83f8e81cad..89d7a5ac3e 100644
>> --- a/src/qemu/qemu_domain_address.h
>> +++ b/src/qemu/qemu_domain_address.h
>> @@ -59,10 +59,6 @@ void qemuDomainReleaseDeviceAddress(virDomainObjPtr vm,
>>  virDomainDeviceInfoPtr info,
>>  const char *devstr);
>>  
>> -virDomainCCWAddressSetPtr
>> -qemuDomainCCWAddrSetCreateFromDomain(virDomainDefPtr def)
>> -ATTRIBUTE_NONNULL(1);
>> -
>>  int qemuDomainAssignMemoryDeviceSlot(virDomainDefPtr def,
>>   virDomainMemoryDefPtr mem);
>>  
>>

--
libvir-list mailing list

Re: [libvirt] [PATCH] qemu: hotplug: Don't access srcPriv when it's not allocated

[Marc Hartmayer]

On Tue, Jul 03, 2018 at 04:22 PM +0200, Peter Krempa  wrote:
> On Tue, Jul 03, 2018 at 09:34:56 -0400, John Ferlan wrote:
>>
>>
>> On 07/03/2018 08:05 AM, Peter Krempa wrote:
>> > The private data of a virStorageSource which is backing an iSCSI hostdev
>> > may be NULL if no authentication is present. The code handling the
>> > hotplug would attempt to extract the authentication info stored in
>> > 'secinfo' without checking if it is allocated which resulted in a crash.
>> >
>> > Here we opt the easy way to check if srcPriv is not NULL so that we
>> > don't duplicate all the logic which selects whether the disk source has
>> > a secret.
>> >
>> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1597550
>> >
>> > Signed-off-by: Peter Krempa 
>> > ---
>> >  src/qemu/qemu_hotplug.c | 3 ++-
>> >  1 file changed, 2 insertions(+), 1 deletion(-)
>> >
>>
>> Reviewed-by: John Ferlan 
>>
>> John
>>
>> Feels like we've done this before...
>>
>> 97202988  and 6050affb7f
>
> Specifically the former commit was not good enough to cover this case
> despite touching this code.

Would it make sense to always allocate the
qemuDomainStorageSourcePrivate struct? We’d be wasting some memory, but
at the same time the code would be easier to read and less prone to
error. e.g. qemuDomainDeviceDiskDefPostParseRestoreSecAlias would be
simpler.

For the privateData of virConnect, virDomainVsockDef, virDisk,
virDomainVcpu… it’s implemented this way.

Anyway: Reviewed-by: Marc Hartmayer  and thanks
for fixing it!

>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
Beste Grüße / Kind regards
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
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

[libvirt] [PATCH 0/2] esx: Couple of fixes

[Michal Privoznik]

*** BLURB HERE ***

Michal Privoznik (2):
  esx: Report error in esxVI_LookupVirtualMachineByName
  esx: De-duplicate @virtualMachine check in esxDomainLookupByName

 src/esx/esx_driver.c | 7 +--
 src/esx/esx_vi.c | 5 -
 2 files changed, 5 insertions(+), 7 deletions(-)

-- 
2.16.4

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

[libvirt] [PATCH 1/2] esx: Report error in esxVI_LookupVirtualMachineByName

[Michal Privoznik]

When reviewing 00d9edfe2f39f60b40 I've changed proposed patch and
made it to not report error if no domain is found. This is wrong
and the original patch was okay. Thing is, both callers pass
occurrence = OptionalItem so no error message overwriting is done
as I thought initially.

Signed-off-by: Michal Privoznik 
---
 src/esx/esx_vi.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
index 0bdfc5a8be..25fbdc7e44 100644
--- a/src/esx/esx_vi.c
+++ b/src/esx/esx_vi.c
@@ -3014,8 +3014,11 @@ esxVI_LookupVirtualMachineByName(esxVI_Context *ctx, 
const char *name,
 break;
 }
 
-if (!(*virtualMachine) && occurrence != esxVI_Occurrence_OptionalItem)
+if (!(*virtualMachine) && occurrence != esxVI_Occurrence_OptionalItem) {
+virReportError(VIR_ERR_NO_DOMAIN,
+   _("Could not find domain with name '%s'"), name);
 goto cleanup;
+}
 
 result = 0;
 
-- 
2.16.4

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

[libvirt] [PATCH 2/2] esx: De-duplicate @virtualMachine check in esxDomainLookupByName

[Michal Privoznik]

The function call esxVI_LookupVirtualMachineByName(occurrence =
OptionalItem) and then checks if @virtualMachine is NULL. If it
is an error is reported. The same result can be achieved by
setting occurrence to RequiredItem.

Signed-off-by: Michal Privoznik 
---
 src/esx/esx_driver.c | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
index e906d11fa9..01bcc99962 100644
--- a/src/esx/esx_driver.c
+++ b/src/esx/esx_driver.c
@@ -1577,12 +1577,7 @@ esxDomainLookupByName(virConnectPtr conn, const char 
*name)
"config.uuid\0") < 0 ||
 esxVI_LookupVirtualMachineByName(priv->primary, name, propertyNameList,
  ,
- esxVI_Occurrence_OptionalItem) < 0) {
-goto cleanup;
-}
-
-if (!virtualMachine) {
-virReportError(VIR_ERR_NO_DOMAIN, _("No domain with name '%s'"), name);
+ esxVI_Occurrence_RequiredItem) < 0) {
 goto cleanup;
 }
 
-- 
2.16.4

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

Re: [libvirt] [PATCH 1/3] esx_vi.c: Simplify error handling in MachineByName

[Michal Prívozník]

On 07/03/2018 03:43 PM, Matthias Bolte wrote:
> 2018-07-03 11:31 GMT+02:00 Michal Prívozník :
>> On 07/03/2018 04:20 AM, Marcos Paulo de Souza wrote:
>>> The same pattern is used in lots of other places.
>>>
>>> Signed-off-by: Marcos Paulo de Souza 
>>> ---
>>>  src/esx/esx_vi.c | 14 --
>>>  1 file changed, 4 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
>>> index 43ff7ea048..25fbdc7e44 100644
>>> --- a/src/esx/esx_vi.c
>>> +++ b/src/esx/esx_vi.c
>>> @@ -3014,16 +3014,10 @@ esxVI_LookupVirtualMachineByName(esxVI_Context 
>>> *ctx, const char *name,
>>>  break;
>>>  }
>>>
>>> -if (!(*virtualMachine)) {
>>> -if (occurrence == esxVI_Occurrence_OptionalItem) {
>>> -result = 0;
>>> -
>>> -goto cleanup;
>>> -} else {
>>> -virReportError(VIR_ERR_NO_DOMAIN,
>>> -   _("Could not find domain with name '%s'"), 
>>> name);
>>> -goto cleanup;
>>> -}
>>> +if (!(*virtualMachine) && occurrence != esxVI_Occurrence_OptionalItem) 
>>> {
>>> +virReportError(VIR_ERR_NO_DOMAIN,
>>> +   _("Could not find domain with name '%s'"), name);
>>> +goto cleanup;
>>>  }
>>
>> I think this error message should be dropped. Firstly, this function is
>> called from esxDomainDefineXMLFlags() where it is used to check if a
>> domain with the same name as in provided XML does not exist. If it
>> doesn't this function reports an error which is undesired.
>>
>> Secondly, every caller checks for virtualMachine == NULL and reports
>> their own error effectively overwriting this one.
>>
>> Thirdly, this function is supposed to act like
>> virDomainObjListFindByName() which doesn't report error either.
>>
>>
>> ACK with this squashed in:
>>
>> diff --git i/src/esx/esx_vi.c w/src/esx/esx_vi.c
>> index 25fbdc7e44..0bdfc5a8be 100644
>> --- i/src/esx/esx_vi.c
>> +++ w/src/esx/esx_vi.c
>> @@ -3014,11 +3014,8 @@ esxVI_LookupVirtualMachineByName(esxVI_Context *ctx, 
>> const char *name,
>>  break;
>>  }
>>
>> -if (!(*virtualMachine) && occurrence != esxVI_Occurrence_OptionalItem) {
>> -virReportError(VIR_ERR_NO_DOMAIN,
>> -   _("Could not find domain with name '%s'"), name);
>> +if (!(*virtualMachine) && occurrence != esxVI_Occurrence_OptionalItem)
>>  goto cleanup;
>> -}
>>
>>  result = 0;
>>
>>
>> And pushed.
> 
> The original patch was fine, your modification is wrong. The error
> message needs to stay. You didn't follow the logic closely enough. The
> caller of esxVI_LookupVirtualMachineByName specified the expected
> occurrence.
> 
> esxDomainDefineXMLFlags uses this to test if the domain exists and
> calls esxVI_LookupVirtualMachineByName with occurrence OptionalItem.
> If the domain is missing no error is reported by
> esxVI_LookupVirtualMachineByName, because of occurrence OptionalItem.
> 
> esxDomainLookupByName uses this to find a domain with the given name.
> It also uses occurrence OptionalItem. Here you argue that the caller
> checks for virtualMachine == NULL anyway. But no error is overwritten
> here because esxVI_LookupVirtualMachineByName doesn't report an error
> with occurrence OptionalItem. The actual point that could be made here
> is that esxDomainLookupByName should actually use occurrence
> RequiredItem instead of doing the virtualMachine == NULL check itself.
> 
> Also esxVI_LookupVirtualMachineByUuid has the same logic that got
> simplified in the original patch, but again, the error reporting there
> is correct as well and needs to stay.
> 

D'oh. Well, so far all (as in both) callers pass OptionalItem so no real
harm done. But sure, I'll post a patch to fix my mess.

Michal

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

Re: [libvirt] [PATCH] qemu: hotplug: Don't access srcPriv when it's not allocated

[Peter Krempa]

On Tue, Jul 03, 2018 at 09:34:56 -0400, John Ferlan wrote:
> 
> 
> On 07/03/2018 08:05 AM, Peter Krempa wrote:
> > The private data of a virStorageSource which is backing an iSCSI hostdev
> > may be NULL if no authentication is present. The code handling the
> > hotplug would attempt to extract the authentication info stored in
> > 'secinfo' without checking if it is allocated which resulted in a crash.
> > 
> > Here we opt the easy way to check if srcPriv is not NULL so that we
> > don't duplicate all the logic which selects whether the disk source has
> > a secret.
> > 
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1597550
> > 
> > Signed-off-by: Peter Krempa 
> > ---
> >  src/qemu/qemu_hotplug.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> 
> Reviewed-by: John Ferlan 
> 
> John
> 
> Feels like we've done this before...
> 
> 97202988  and 6050affb7f

Specifically the former commit was not good enough to cover this case
despite touching this code.



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

Re: [libvirt] [PATCH 2/3] esx: Add esxVI_checkArgList macro

[Matthias Bolte]

2018-07-03 4:20 GMT+02:00 Marcos Paulo de Souza :
> This macro avoids code duplication when checking for arrays of objects.
>
> Signed-off-by: Marcos Paulo de Souza 
> ---
>  src/esx/esx_vi.c | 189 ---
>  1 file changed, 46 insertions(+), 143 deletions(-)
>
> diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
> index 25fbdc7e44..212300dbff 100644
> --- a/src/esx/esx_vi.c
> +++ b/src/esx/esx_vi.c
> @@ -41,6 +41,16 @@
>
>  VIR_LOG_INIT("esx.esx_vi");
>
> +#define esxVI_checkArgList(val) \
> +do { \
> +if (!val || *val) { \
> +virReportError(VIR_ERR_INVALID_ARG, "%s", _("Invalid 
> argument")); \
> +return -1; \
> +} \
> +} while (0)

As this is a macro I suggest naming it ESX_VI_CHECK_ARG_LIST instead
of esxVI_checkArgList. Because at the moment the ESX code makes a
clear distinction between macros and functions in their spelling. I'd
like to keep it that way, even if the rest of the code base is no
consistent about this.

-- 
Matthias Bolte
http://photron.blogspot.com

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

Re: [libvirt] [PATCH 1/3] esx_vi.c: Simplify error handling in MachineByName

[Matthias Bolte]

2018-07-03 11:31 GMT+02:00 Michal Prívozník :
> On 07/03/2018 04:20 AM, Marcos Paulo de Souza wrote:
>> The same pattern is used in lots of other places.
>>
>> Signed-off-by: Marcos Paulo de Souza 
>> ---
>>  src/esx/esx_vi.c | 14 --
>>  1 file changed, 4 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
>> index 43ff7ea048..25fbdc7e44 100644
>> --- a/src/esx/esx_vi.c
>> +++ b/src/esx/esx_vi.c
>> @@ -3014,16 +3014,10 @@ esxVI_LookupVirtualMachineByName(esxVI_Context *ctx, 
>> const char *name,
>>  break;
>>  }
>>
>> -if (!(*virtualMachine)) {
>> -if (occurrence == esxVI_Occurrence_OptionalItem) {
>> -result = 0;
>> -
>> -goto cleanup;
>> -} else {
>> -virReportError(VIR_ERR_NO_DOMAIN,
>> -   _("Could not find domain with name '%s'"), name);
>> -goto cleanup;
>> -}
>> +if (!(*virtualMachine) && occurrence != esxVI_Occurrence_OptionalItem) {
>> +virReportError(VIR_ERR_NO_DOMAIN,
>> +   _("Could not find domain with name '%s'"), name);
>> +goto cleanup;
>>  }
>
> I think this error message should be dropped. Firstly, this function is
> called from esxDomainDefineXMLFlags() where it is used to check if a
> domain with the same name as in provided XML does not exist. If it
> doesn't this function reports an error which is undesired.
>
> Secondly, every caller checks for virtualMachine == NULL and reports
> their own error effectively overwriting this one.
>
> Thirdly, this function is supposed to act like
> virDomainObjListFindByName() which doesn't report error either.
>
>
> ACK with this squashed in:
>
> diff --git i/src/esx/esx_vi.c w/src/esx/esx_vi.c
> index 25fbdc7e44..0bdfc5a8be 100644
> --- i/src/esx/esx_vi.c
> +++ w/src/esx/esx_vi.c
> @@ -3014,11 +3014,8 @@ esxVI_LookupVirtualMachineByName(esxVI_Context *ctx, 
> const char *name,
>  break;
>  }
>
> -if (!(*virtualMachine) && occurrence != esxVI_Occurrence_OptionalItem) {
> -virReportError(VIR_ERR_NO_DOMAIN,
> -   _("Could not find domain with name '%s'"), name);
> +if (!(*virtualMachine) && occurrence != esxVI_Occurrence_OptionalItem)
>  goto cleanup;
> -}
>
>  result = 0;
>
>
> And pushed.

The original patch was fine, your modification is wrong. The error
message needs to stay. You didn't follow the logic closely enough. The
caller of esxVI_LookupVirtualMachineByName specified the expected
occurrence.

esxDomainDefineXMLFlags uses this to test if the domain exists and
calls esxVI_LookupVirtualMachineByName with occurrence OptionalItem.
If the domain is missing no error is reported by
esxVI_LookupVirtualMachineByName, because of occurrence OptionalItem.

esxDomainLookupByName uses this to find a domain with the given name.
It also uses occurrence OptionalItem. Here you argue that the caller
checks for virtualMachine == NULL anyway. But no error is overwritten
here because esxVI_LookupVirtualMachineByName doesn't report an error
with occurrence OptionalItem. The actual point that could be made here
is that esxDomainLookupByName should actually use occurrence
RequiredItem instead of doing the virtualMachine == NULL check itself.

Also esxVI_LookupVirtualMachineByUuid has the same logic that got
simplified in the original patch, but again, the error reporting there
is correct as well and needs to stay.

-- 
Matthias Bolte
http://photron.blogspot.com

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

Re: [libvirt] [PATCH] qemu: hotplug: Don't access srcPriv when it's not allocated

[John Ferlan]

On 07/03/2018 08:05 AM, Peter Krempa wrote:
> The private data of a virStorageSource which is backing an iSCSI hostdev
> may be NULL if no authentication is present. The code handling the
> hotplug would attempt to extract the authentication info stored in
> 'secinfo' without checking if it is allocated which resulted in a crash.
> 
> Here we opt the easy way to check if srcPriv is not NULL so that we
> don't duplicate all the logic which selects whether the disk source has
> a secret.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1597550
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/qemu/qemu_hotplug.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

Reviewed-by: John Ferlan 

John

Feels like we've done this before...

97202988  and 6050affb7f

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

Re: [libvirt] [PATCH] qemu: Added support L2 table cache for qcow2 disk.

[Peter Krempa]

On Tue, Jul 03, 2018 at 20:41:54 +0800, dujiancheng wrote:
> From: Dujiancheng 
> 
> dujiancheng (1):
>   qemu: Added support L2 table cache for qcow2 disk.   
> L2 table cache can be set by the new element diskCache.   
> Use the following methods to set the L2 table cache   
> and cache clean interval:   
>  
>
> 10240 
>
>  
>


Since you insist on sending your own version rather than reusing the
better previously posted version, here are a few things this
patch is missing:

- capability which allows to check whether qemu supports the new
  features
- cache is a property of the image and not the disk, also it needs to be
  configurable for  so that it's usable for the upcoming
  blockdev work
- the documentation does not explain how it's supposed to be used.
- the RNG schema indentation is wrong
- the formatter for the XML should not have any logic .. e.g.
  determining that only L2 cache is printed
- the code itself is rather strange and does not allow multiple cache
  levels although the data structure suggests so
- the name for the tests are wrong since this is not disk cache but
  image cache
- there are no checks whether a format supporting the l2 cache is used
  (qcow2 only)
- make syntax-check fails:

--- tests/qemuxml2argvdata/disk-cache.args  2018-07-03 15:29:18.386122485 
+0200
+++ -   2018-07-03 15:30:36.263098059 +0200
@@ -22,7 +22,8 @@
 -no-acpi \
 -boot c \
 -usb \
--drive 
file=/tmp/data.img,format=raw,l2-cache-size=10240,cache-clean-interval=900,if=none,id=drive-virtio-disk0
 \
+-drive file=/tmp/data.img,format=raw,l2-cache-size=10240,\
+cache-clean-interval=900,if=none,id=drive-virtio-disk0 \
 -device virtio-blk-pci,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,\
 id=virtio-disk0 \
 -drive file=/tmp/logs.img,format=raw,if=none,id=drive-virtio-disk1 \
Incorrect line wrapping in tests/qemuxml2argvdata/disk-cache.args
Use test-wrap-argv.pl to wrap test data files

> Signed-off-by: dujiancheng 

I think it was pointed out that we prefer real names...



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

Re: [libvirt] [PATCH 4/5] virISCSIScanTargets: Honour iSCSI interface

[Michal Prívozník]

On 07/03/2018 02:38 PM, John Ferlan wrote:
> 
> 
> On 07/03/2018 01:08 AM, Michal Prívozník wrote:
>> On 07/03/2018 01:38 AM, John Ferlan wrote:
>>>
>>>
>>> On 06/29/2018 11:01 AM, Michal Privoznik wrote:
 When scanning for targets, iSCSI might give different results
 depending on the interface used. This is basically just name of
>>>
>>> assuming this means whether the initiatoriqn interface was used
>>
>> Yes.
>>
>>>
 config file under /etc/iscsi/ifaces to use. The file contains
 initiator IQN thus different results claim.

>>>
>>> Strange to have activity here - was there a bz? Or something found by
>>> the (I assume) GSoC project:
>>>
>>> https://www.redhat.com/archives/libvir-list/2018-June/msg00249.html
>>>
>>> Touching something that's been avoided for 8 years is always scary, but
>>> if it's broken, then sure it ought to be fixed.
>>
>> There is no BZ. And yes, the GSoC project got me experimenting (because
>> for libiscsi backend we will have to make IQN required since libiscsi
>> does not parse anything from /etc/iscsi nor initiatorname.iscsi). And
>> while experimenting I've tired to put my own IQN into iscsi pool we
>> already have and found this bug.
>>
> 
> FWIW: To a degree this is restoring the initiatoriqn argument that
> commit 027986f5 removed...

Yes, but I did not think it is worth mentioning anywhere. We change code
all the time, and if we were even to blame which commits caused what ..
it's a endless spiral IMO.

> 
> [...]
> 
 +static int
 +virISCSIScanTargetsInternal(const char *portal,
 +const char *ifacename,
 +size_t *ntargetsret,
 +char ***targetsret);
 +
 +
  struct virISCSISessionData {
  char *session;
  const char *devpath;
 @@ -286,9 +293,10 @@ virISCSIConnection(const char *portal,
   * iscsiadm doesn't let you send commands to the Interface 
 IQN,
   * unless you've first issued a 'sendtargets' command to the
   * portal. Without the sendtargets all that is received is a
 - * "iscsiadm: No records found"
 + * "iscsiadm: No records found". However, we must ensure that
 + * the command is issued over interface name we invented 
 above.
>>>
>>> "invented" - again is this the make sure it's issued over the
>>> initiatoriqn interface?
>>
>> Yes. And we construct the interface name at the beginning of this function.
>>
> 
> Oh right, the virStorageBackendCreateIfaceIQN call which creates that
> libvirt-iface-* name. That's just one of those context things that you
> know when you've been working through the code ;-).
> 
> Still without the next patch, does this work?

Not really because even though targets are refreshed their configs are
not saved and thus login still fails.

Michal

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

Re: [libvirt] [PATCH 5/5] virISCSIScanTargets: Allow making targets persistent

[Michal Prívozník]

On 07/03/2018 02:42 PM, John Ferlan wrote:
> 
> 
> On 07/03/2018 01:08 AM, Michal Prívozník wrote:
>> On 07/03/2018 01:40 AM, John Ferlan wrote:
>>>
>>>
>>> On 06/29/2018 11:01 AM, Michal Privoznik wrote:
 After new iSCSI interface is successfully set up, we issue
>>>
>>> s/new/a new/
>>> s/issue/issue a/
>>>
 sendtargets command. However, after 56057900dc53df490d we don't
 update the host config which in turn makes login fail because
 iscsiadm is unable to find any matching record for the interface.

 Signed-off-by: Michal Privoznik 
 ---
  src/storage/storage_backend_iscsi.c |  1 +
  src/util/viriscsi.c | 21 ++---
  src/util/viriscsi.h |  1 +
  tests/viriscsitest.c|  3 ++-
  4 files changed, 22 insertions(+), 4 deletions(-)

>>>
>>> Like the previous patch - is there a specific bug or something that led
>>> you down this path?  Can you show an example of the existing code that's
>>> creating a bad command line and generating an error and then how this
>>> fixes things.  It's not like we have tests and for this stuff it's
>>> really nice to have plenty of examples.
>>
>> So here is the run without my patches:
>>
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode session
>> iscsiadm: No active sessions.
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal 
>> $PORTAL:3260,1 --targetname $TARGET --op new
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal 
>> $PORTAL:3260,1 --target $TARGET --op update --name 
>> node.session.auth.authmethod --value CHAP
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal 
>> $PORTAL:3260,1 --target $TARGET --op update --name 
>> node.session.auth.username --value $USER
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal 
>> $PORTAL:3260,1 --target $TARGET --op update --name 
>> node.session.auth.password --value $PASS
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface 
>> --interface libvirt-iface-03316143 --op new
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface 
>> --interface libvirt-iface-03316143 --op update --name iface.initiatorname 
>> --value $INITIATOR
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode discovery 
>> --type sendtargets --portal $PORTAL:3260,1 --op nonpersistent
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal 
>> $PORTAL:3260,1 --targetname $TARGET --login --interface 
>> libvirt-iface-03316143
>> error : virCommandWait:2600 : internal error: Child process (iscsiadm --mode 
>> node --portal $PORTAL:3260,1 --targetname $TARGET --login --interface 
>> libvirt-iface-03316143) unexpected exit status 21:
>> iscsiadm: No records found
>> iscsiadm: No records found
>>
>>
>> And with my patches:
>>
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode session
>> iscsiadm: No active sessions.
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal 
>> $PORTAL:3260,1 --targetname $TARGET --op new
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal 
>> $PORTAL:3260,1 --target $TARGET --op update --name 
>> node.session.auth.authmethod --value CHAP
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal 
>> $PORTAL:3260,1 --target $TARGET --op update --name 
>> node.session.auth.username --value $USER
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal 
>> $PORTAL:3260,1 --target $TARGET --op update --name 
>> node.session.auth.password --value $PASS
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface 
>> --interface libvirt-iface-28727243 --op new
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface 
>> --interface libvirt-iface-28727243 --op update --name iface.initiatorname 
>> --value $INITIATOR
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode discovery 
>> --type sendtargets --portal $PORTAL:3260,1 --interface libvirt-iface-28727243
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal 
>> $PORTAL:3260,1 --targetname $TARGET --login --interface 
>> libvirt-iface-28727243
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode session
>> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode session -r 1 
>> -R
>>
>>
> 
> So the example helps me understand - thanks! The difference of discovery
> being:
> 
>   fail: iscsiadm --mode discovery --type sendtargets \
>  --portal $PORTAL:3260,1 \
>  --op 

Re: [libvirt] [PATCH 3/5] virStorageBackendIQNFound: Rework iscsiadm output parsing

[Michal Prívozník]

On 07/03/2018 02:51 PM, John Ferlan wrote:
> 
> 
> On 07/03/2018 01:08 AM, Michal Prívozník wrote:
>> On 07/03/2018 01:18 AM, John Ferlan wrote:
>>>
>>>
>>> On 06/29/2018 11:01 AM, Michal Privoznik wrote:
 Firstly, we can utilize virCommandSetOutputBuffer() API which
 will collect the command output for us. Secondly, sscanf()-ing
 through each line is easier to understand (and more robust) than
 jumping over a string with strchr().

 Signed-off-by: Michal Privoznik 
 ---
  src/util/viriscsi.c | 85 
 +
  1 file changed, 34 insertions(+), 51 deletions(-)

>>>
>>> I assume virCommandSetOutputBuffer didn't exist when this code was created.
>>
>> Perhaps. Let me check git log. .. .. And yes, you are right.
>> virCommandSetOutputBuffer was introduced among with other basic
>> virCommand* APIs by Dan in late 2010 (f16ad06fb2aeb5e5c9974). But this
>> function was introduced by Dave in Jan 2010 so he couldn't have used it.
>>
>>>
>>> Also, why do I have this recollection related to portability of sscanf?
>>> I know we use it elsewhere, but some oddball issue that someone like
>>> Eric may recollect or know about.
>>
>> I don't know about anything. But since we use it in our code pretty
>> freely I assumed there is no problem with it.
>>
>>>
 diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
 index 2e55b3c10b..44788056fd 100644
 --- a/src/util/viriscsi.c
 +++ b/src/util/viriscsi.c
 @@ -108,7 +108,6 @@ virISCSIGetSession(const char *devpath,
  
  
  
 -#define LINE_SIZE 4096
  #define IQN_FOUND 1
  #define IQN_MISSING 0
  #define IQN_ERROR -1
 @@ -117,71 +116,56 @@ static int
  virStorageBackendIQNFound(const char *initiatoriqn,
char **ifacename)
  {
 -int ret = IQN_ERROR, fd = -1;
 -char ebuf[64];
 -FILE *fp = NULL;
 -char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL;
 +int ret = IQN_ERROR;
 +char *outbuf = NULL;
 +char *line = NULL;
 +char *iface = NULL;
 +char *iqn = NULL;
  virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
   "--mode", "iface", NULL);
  
  *ifacename = NULL;
  
 -if (VIR_ALLOC_N(line, LINE_SIZE) != 0) {
 -virReportError(VIR_ERR_INTERNAL_ERROR,
 -   _("Could not allocate memory for output of '%s'"),
 -   ISCSIADM);
 +virCommandSetOutputBuffer(cmd, );
 +if (virCommandRun(cmd, NULL) < 0)
  goto cleanup;
 -}
  
 -memset(line, 0, LINE_SIZE);
 +/* Example of data we are dealing with:
 + * default tcp
 + * iser iser
 + * libvirt-iface-253db048 
 tcpiqn.2017-03.com.user:client
 + */
>>>
>>> Nice to have an example especially since this is a bit of a edge case...
>>>
>>> Another option - virStringSplitCount on the "\n" to get the number of
>>> lines and then on each line again using "," to tear apart the pieces.
>>> This I assume works as I don't have a initiatoriqn set up to test.
>>>
>>> Any thoughts/recollections about sscanf? I assume it'll be felt that
>>> virStringSplit might be overkill, 
>>
>> Indeed.
>>
>>> but at least it's for other purposes
>>> and perhaps less susceptible to the line && *line adjustment.
>>
>> I like sscanf() more because it's fewer lines of code, the variables I
>> need are assigned value immediately and also memory footprint is smaller
>> (I guess) since there's no need to store multiple arrays.
>>
> 
> Understood - it's probably something really early on when I first
> started w/ libvirt and using sscanf was discouraged for something that
> has stuck in my head. Another part for me is readability - similar to
> the various [i]scsi code that uses regex's in order to parse output.
> That format is just a mish-mash of search patterns that causes my eyes
> to complain.

Yup.

> 
> Perhaps it's best to see this along with the combined 4&5 "again".  Also
> add a non initiatoriqn to the mix (even the comment above) just so show
> the various formats.
> 
> Hopefully using libiscsi makes all the personally displeasing regex and
> sscanf searching code disappear.

Unfortunately it will not. The libiscsi GSoC project aims on creating
new type of pool "iscsi-direct". It is going to be completely new pool
type (storage driver backend) which will share some similarities with
iscsi pool we have but in some aspects it will be more like rbd pool,
because it is not going to have any  (=no host representation
of LUNs).

Michal

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

Re: [libvirt] [dbus PATCH] connect: fix g_free order in virtDBusConnectFree

[Ján Tomko]

On Tue, Jul 03, 2018 at 02:18:02PM +0200, Pavel Hrdina wrote:

On Mon, Jul 02, 2018 at 11:42:08AM -0400, Anya Harter wrote:

so that g_free(connect->nodeDevPath) line appears in alphabetical order
with the rest of the lines

Signed-off-by: Anya Harter 
---
 src/connect.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


In general we don't send/merge patches that just move code without any
functional reason because it pollutes git history but in this case I
would say it's ok.



What about the gchar *nodeDevPath; declaration in connect.h?
It is also in a different order.

/me runs

Jano


Reviewed-by: Pavel Hrdina  and pushed.





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




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

Re: [libvirt] [PATCH] qemuOpenFileAs: Lose bypassSecurityDriver

[John Ferlan]

$SUBJ/

util: Remove unused bypassSecurityDriver from qemuOpenFileAs

On 07/03/2018 01:57 AM, Michal Privoznik wrote:
> This argument is not used anymore. The only function that is
> passing non-NULL (qemuDomainSaveMemory) does not actually care
> for the value (after 23087cfdb) and every other caller just
> passes NULL anyway.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_driver.c | 32 +---
>  1 file changed, 9 insertions(+), 23 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH 3/5] virStorageBackendIQNFound: Rework iscsiadm output parsing

[John Ferlan]

On 07/03/2018 01:08 AM, Michal Prívozník wrote:
> On 07/03/2018 01:18 AM, John Ferlan wrote:
>>
>>
>> On 06/29/2018 11:01 AM, Michal Privoznik wrote:
>>> Firstly, we can utilize virCommandSetOutputBuffer() API which
>>> will collect the command output for us. Secondly, sscanf()-ing
>>> through each line is easier to understand (and more robust) than
>>> jumping over a string with strchr().
>>>
>>> Signed-off-by: Michal Privoznik 
>>> ---
>>>  src/util/viriscsi.c | 85 
>>> +
>>>  1 file changed, 34 insertions(+), 51 deletions(-)
>>>
>>
>> I assume virCommandSetOutputBuffer didn't exist when this code was created.
> 
> Perhaps. Let me check git log. .. .. And yes, you are right.
> virCommandSetOutputBuffer was introduced among with other basic
> virCommand* APIs by Dan in late 2010 (f16ad06fb2aeb5e5c9974). But this
> function was introduced by Dave in Jan 2010 so he couldn't have used it.
> 
>>
>> Also, why do I have this recollection related to portability of sscanf?
>> I know we use it elsewhere, but some oddball issue that someone like
>> Eric may recollect or know about.
> 
> I don't know about anything. But since we use it in our code pretty
> freely I assumed there is no problem with it.
> 
>>
>>> diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c
>>> index 2e55b3c10b..44788056fd 100644
>>> --- a/src/util/viriscsi.c
>>> +++ b/src/util/viriscsi.c
>>> @@ -108,7 +108,6 @@ virISCSIGetSession(const char *devpath,
>>>  
>>>  
>>>  
>>> -#define LINE_SIZE 4096
>>>  #define IQN_FOUND 1
>>>  #define IQN_MISSING 0
>>>  #define IQN_ERROR -1
>>> @@ -117,71 +116,56 @@ static int
>>>  virStorageBackendIQNFound(const char *initiatoriqn,
>>>char **ifacename)
>>>  {
>>> -int ret = IQN_ERROR, fd = -1;
>>> -char ebuf[64];
>>> -FILE *fp = NULL;
>>> -char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL;
>>> +int ret = IQN_ERROR;
>>> +char *outbuf = NULL;
>>> +char *line = NULL;
>>> +char *iface = NULL;
>>> +char *iqn = NULL;
>>>  virCommandPtr cmd = virCommandNewArgList(ISCSIADM,
>>>   "--mode", "iface", NULL);
>>>  
>>>  *ifacename = NULL;
>>>  
>>> -if (VIR_ALLOC_N(line, LINE_SIZE) != 0) {
>>> -virReportError(VIR_ERR_INTERNAL_ERROR,
>>> -   _("Could not allocate memory for output of '%s'"),
>>> -   ISCSIADM);
>>> +virCommandSetOutputBuffer(cmd, );
>>> +if (virCommandRun(cmd, NULL) < 0)
>>>  goto cleanup;
>>> -}
>>>  
>>> -memset(line, 0, LINE_SIZE);
>>> +/* Example of data we are dealing with:
>>> + * default tcp
>>> + * iser iser
>>> + * libvirt-iface-253db048 
>>> tcpiqn.2017-03.com.user:client
>>> + */
>>
>> Nice to have an example especially since this is a bit of a edge case...
>>
>> Another option - virStringSplitCount on the "\n" to get the number of
>> lines and then on each line again using "," to tear apart the pieces.
>> This I assume works as I don't have a initiatoriqn set up to test.
>>
>> Any thoughts/recollections about sscanf? I assume it'll be felt that
>> virStringSplit might be overkill, 
> 
> Indeed.
> 
>> but at least it's for other purposes
>> and perhaps less susceptible to the line && *line adjustment.
> 
> I like sscanf() more because it's fewer lines of code, the variables I
> need are assigned value immediately and also memory footprint is smaller
> (I guess) since there's no need to store multiple arrays.
> 

Understood - it's probably something really early on when I first
started w/ libvirt and using sscanf was discouraged for something that
has stuck in my head. Another part for me is readability - similar to
the various [i]scsi code that uses regex's in order to parse output.
That format is just a mish-mash of search patterns that causes my eyes
to complain.

Perhaps it's best to see this along with the combined 4&5 "again".  Also
add a non initiatoriqn to the mix (even the comment above) just so show
the various formats.

Hopefully using libiscsi makes all the personally displeasing regex and
sscanf searching code disappear.

John

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

[libvirt] [PATCH] qemu: Added support L2 table cache for qcow2 disk.

[dujiancheng]

From: Dujiancheng 

dujiancheng (1):
  qemu: Added support L2 table cache for qcow2 disk.   
L2 table cache can be set by the new element diskCache.   
Use the following methods to set the L2 table cache   
and cache clean interval:   
 
   
10240 
   
 
   


Signed-off-by: dujiancheng 
---
 docs/formatdomain.html.in   | 25 +
 docs/schemas/domaincommon.rng   | 26 ++
 src/conf/domain_conf.c  | 63 -
 src/conf/domain_conf.h  |  5 +++
 src/qemu/qemu_command.c |  6 
 src/qemu/qemu_domain.c  |  6 
 tests/qemuxml2argvdata/disk-cache.args  | 30 
 tests/qemuxml2argvdata/disk-cache.xml   | 40 +
 tests/qemuxml2argvtest.c|  1 +
 tests/qemuxml2xmloutdata/disk-cache.xml | 46 
 tests/qemuxml2xmltest.c |  1 +
 11 files changed, 248 insertions(+), 1 deletion(-)
 create mode 100644 tests/qemuxml2argvdata/disk-cache.args
 create mode 100644 tests/qemuxml2argvdata/disk-cache.xml
 create mode 100644 tests/qemuxml2xmloutdata/disk-cache.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 0d68596..2afeb2b 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2719,6 +2719,12 @@
 backingStore type='file'
   format type='qcow2'/
   source file='/var/lib/libvirt/images/snapshot.qcow'/
+  diskCache
+cache level='2'
+size unit='KiB'128/size
+/cache
+clean interval='8'/
+  /diskCache
   backingStore type='block'
 format type='raw'/
 source dev='/dev/mapper/base'/
@@ -3582,6 +3588,25 @@
   
 
   
+  diskCache
+  The diskCache allows setting the L2 cache
+and cache clean interval values of the qcow2 image with
+the following properties.
+Since 2.5(QEMU and KVM)
+
+  cache
+  The level attribute allows you to set the
+level of the cache, temporarily only supports level 2 cache.
+The size allows you to set the size of the cache
+and can specify the unit
+  
+  clean
+  The interval allow setting cache expiration time
+in seconds
+  
+
+
+  
 
 
 Filesystems
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index f46145c..04bc9e1 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -1388,8 +1388,34 @@
   
 
   
+  
+
+  
 
   
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
   
 
   
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b8b5345..9b6f445 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -8894,6 +8894,37 @@ virDomainDiskSourceParse(xmlNodePtr node,
 return 0;
 }
 
+static int
+virDomainDiskCacheParse(xmlNodePtr node,
+xmlXPathContextPtr ctxt,
+virDomainDiskDefPtr def)
+{
+int ret = -1;
+xmlNodePtr saveNode = ctxt->node;
+ctxt->node = node;
+if (virXPathUInt("string(./clean/@interval)", ctxt, 
>disk_cache.clean_interval) < -1) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   _("malformed disk cache clean interval"));
+goto cleanup;
+}
+
+if (virXPathUInt("string(./cache/@level)", ctxt, 
>disk_cache.cache_level) < -1) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   _("malformed disk cache level"));
+goto cleanup;
+}
+
+if (virDomainParseMemory("./cache/size", NULL, ctxt,
+ >disk_cache.cache_size, false, true) < 0)
+goto cleanup;
+
+ret = 0;
+
+ cleanup:
+ctxt->node = saveNode;
+return ret;
+
+}
 
 static int
 virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt,
@@ -9701,6 +9732,10 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
 }
 } else if (virXMLNodeNameEqual(cur, "boot")) {
 /* boot is parsed as part of virDomainDeviceInfoParseXML */
+} else if (virXMLNodeNameEqual(cur, "diskCache")) {
+
+if (virDomainDiskCacheParse(cur, ctxt, def) < 0)
+goto error;
 }
 }
 
@@ -23369,7 +23404,32 @@ virDomainDiskBlockIoDefFormat(virBufferPtr buf,
 virBufferAddLit(buf, "/>\n");
 }
 }
-
+static void

Re: [libvirt] [PATCH 5/5] virISCSIScanTargets: Allow making targets persistent

[John Ferlan]

On 07/03/2018 01:08 AM, Michal Prívozník wrote:
> On 07/03/2018 01:40 AM, John Ferlan wrote:
>>
>>
>> On 06/29/2018 11:01 AM, Michal Privoznik wrote:
>>> After new iSCSI interface is successfully set up, we issue
>>
>> s/new/a new/
>> s/issue/issue a/
>>
>>> sendtargets command. However, after 56057900dc53df490d we don't
>>> update the host config which in turn makes login fail because
>>> iscsiadm is unable to find any matching record for the interface.
>>>
>>> Signed-off-by: Michal Privoznik 
>>> ---
>>>  src/storage/storage_backend_iscsi.c |  1 +
>>>  src/util/viriscsi.c | 21 ++---
>>>  src/util/viriscsi.h |  1 +
>>>  tests/viriscsitest.c|  3 ++-
>>>  4 files changed, 22 insertions(+), 4 deletions(-)
>>>
>>
>> Like the previous patch - is there a specific bug or something that led
>> you down this path?  Can you show an example of the existing code that's
>> creating a bad command line and generating an error and then how this
>> fixes things.  It's not like we have tests and for this stuff it's
>> really nice to have plenty of examples.
> 
> So here is the run without my patches:
> 
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode session
> iscsiadm: No active sessions.
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal 
> $PORTAL:3260,1 --targetname $TARGET --op new
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal 
> $PORTAL:3260,1 --target $TARGET --op update --name 
> node.session.auth.authmethod --value CHAP
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal 
> $PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.username 
> --value $USER
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal 
> $PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.password 
> --value $PASS
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface 
> --interface libvirt-iface-03316143 --op new
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface 
> --interface libvirt-iface-03316143 --op update --name iface.initiatorname 
> --value $INITIATOR
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode discovery 
> --type sendtargets --portal $PORTAL:3260,1 --op nonpersistent
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal 
> $PORTAL:3260,1 --targetname $TARGET --login --interface libvirt-iface-03316143
> error : virCommandWait:2600 : internal error: Child process (iscsiadm --mode 
> node --portal $PORTAL:3260,1 --targetname $TARGET --login --interface 
> libvirt-iface-03316143) unexpected exit status 21:
> iscsiadm: No records found
> iscsiadm: No records found
> 
> 
> And with my patches:
> 
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode session
> iscsiadm: No active sessions.
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal 
> $PORTAL:3260,1 --targetname $TARGET --op new
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal 
> $PORTAL:3260,1 --target $TARGET --op update --name 
> node.session.auth.authmethod --value CHAP
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal 
> $PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.username 
> --value $USER
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal 
> $PORTAL:3260,1 --target $TARGET --op update --name node.session.auth.password 
> --value $PASS
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface 
> --interface libvirt-iface-28727243 --op new
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface 
> --interface libvirt-iface-28727243 --op update --name iface.initiatorname 
> --value $INITIATOR
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode iface
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode discovery 
> --type sendtargets --portal $PORTAL:3260,1 --interface libvirt-iface-28727243
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode node --portal 
> $PORTAL:3260,1 --targetname $TARGET --login --interface libvirt-iface-28727243
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode session
> debug : virCommandRunAsync:2476 : About to run iscsiadm --mode session -r 1 -R
> 
> 

So the example helps me understand - thanks! The difference of discovery
being:

  fail: iscsiadm --mode discovery --type sendtargets \
 --portal $PORTAL:3260,1 \
 --op nonpersistent

  succ: iscsiadm --mode discovery --type sendtargets \
 --portal $PORTAL:3260,1 \
 --interface libvirt-iface-28727243

Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial

[Christian Borntraeger]

On 07/03/2018 01:35 PM, Peter Maydell wrote:
> On 3 July 2018 at 12:32, Kevin Wolf  wrote:
>> Am 03.07.2018 um 13:22 hat Daniel P. Berrangé geschrieben:
>>> Just posted latest version here:
>>>
>>>   https://www.redhat.com/archives/libvir-list/2018-July/msg00130.html
>>>
>>> It will be in the next release on ~ Aug 1st
>>
>> It would have been a lot nicer to have it the July release because this
>> means that we'll have the released libvirt broken during almost the
>> whole rc phase of QEMU 3.0, but the release is planned for Aug 8th the
>> earliest, so I guess we're still okay. People using QEMU from git will
>> just need libvirt from git as well.
> 
> I'm still not clear what we gain from having a QEMU that's dropped
> a feature that is still used by everything except leading-edge
> not-yet-released versions of libvirt. Is there a strong reason we
> can't just revert the deletion of the deprecated feature for a QEMU
> release or two?

+1


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

Re: [libvirt] [PATCH 4/5] virISCSIScanTargets: Honour iSCSI interface

[John Ferlan]

On 07/03/2018 01:08 AM, Michal Prívozník wrote:
> On 07/03/2018 01:38 AM, John Ferlan wrote:
>>
>>
>> On 06/29/2018 11:01 AM, Michal Privoznik wrote:
>>> When scanning for targets, iSCSI might give different results
>>> depending on the interface used. This is basically just name of
>>
>> assuming this means whether the initiatoriqn interface was used
> 
> Yes.
> 
>>
>>> config file under /etc/iscsi/ifaces to use. The file contains
>>> initiator IQN thus different results claim.
>>>
>>
>> Strange to have activity here - was there a bz? Or something found by
>> the (I assume) GSoC project:
>>
>> https://www.redhat.com/archives/libvir-list/2018-June/msg00249.html
>>
>> Touching something that's been avoided for 8 years is always scary, but
>> if it's broken, then sure it ought to be fixed.
> 
> There is no BZ. And yes, the GSoC project got me experimenting (because
> for libiscsi backend we will have to make IQN required since libiscsi
> does not parse anything from /etc/iscsi nor initiatorname.iscsi). And
> while experimenting I've tired to put my own IQN into iscsi pool we
> already have and found this bug.
> 

FWIW: To a degree this is restoring the initiatoriqn argument that
commit 027986f5 removed...

[...]

>>> +static int
>>> +virISCSIScanTargetsInternal(const char *portal,
>>> +const char *ifacename,
>>> +size_t *ntargetsret,
>>> +char ***targetsret);
>>> +
>>> +
>>>  struct virISCSISessionData {
>>>  char *session;
>>>  const char *devpath;
>>> @@ -286,9 +293,10 @@ virISCSIConnection(const char *portal,
>>>   * iscsiadm doesn't let you send commands to the Interface IQN,
>>>   * unless you've first issued a 'sendtargets' command to the
>>>   * portal. Without the sendtargets all that is received is a
>>> - * "iscsiadm: No records found"
>>> + * "iscsiadm: No records found". However, we must ensure that
>>> + * the command is issued over interface name we invented above.
>>
>> "invented" - again is this the make sure it's issued over the
>> initiatoriqn interface?
> 
> Yes. And we construct the interface name at the beginning of this function.
> 

Oh right, the virStorageBackendCreateIfaceIQN call which creates that
libvirt-iface-* name. That's just one of those context things that you
know when you've been working through the code ;-).

Still without the next patch, does this work?

John

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

Re: [libvirt] [PATCH v4] qemu: format serial and geometry on frontend disk device

[Peter Krempa]

On Tue, Jul 03, 2018 at 12:18:54 +0100, Daniel Berrange wrote:
> Currently we format the serial, geometry and error policy on the -drive
> backend argument.
> 
> QEMU added the ability to set serial and geometry on the frontend in
> the 1.2 release deprecating use of -drive, with support being deleted
> from -drive in 3.0.
> 
> We keep formatting error policy on -drive for now, because we don't
> ahve support for that with -device for usb-storage just yet.
> 
> Note that some disk buses (sd) still don't support -device. Although
> QEMU allowed these properties to be set on -drive for if=sd, they
> have been ignored so we now report an error in this case.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---

ACK


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

[libvirt] [PATCH 5/8] qemu: monitor: Add API to help creating 'transaction' arguments

[Peter Krempa]

Add a new helper that will be solely used to create arguments for the
transaction command. Later on this will make it possible to remove the
overloading which was caused by the fact that snapshots were created
without transaction and also will help in blockdevizing of snapshots.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_monitor_json.c | 46 
 src/qemu/qemu_monitor_json.h |  4 
 2 files changed, 50 insertions(+)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 3e90279b71..54fefcb612 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -464,6 +464,52 @@ qemuMonitorJSONHasError(virJSONValuePtr reply,
 }


+/**
+ * qemuMonitorJSONTransactionAdd:
+ * @actions: array of actions for the 'transaction' command
+ * @cmdname: command to add to @actions
+ * @...: arguments for @cmdname (see virJSONValueObjectAddVArgs for formatting)
+ *
+ * Add a new command with arguments to the existing ones. The resulting array
+ * is used as argument for the 'transaction' command.
+ *
+ * Returns 0 on success and -1 on error.
+ */
+int
+qemuMonitorJSONTransactionAdd(virJSONValuePtr actions,
+  const char *cmdname,
+  ...)
+{
+virJSONValuePtr entry = NULL;
+virJSONValuePtr data = NULL;
+va_list args;
+int ret = -1;
+
+va_start(args, cmdname);
+
+if (virJSONValueObjectCreateVArgs(, args) < 0)
+goto cleanup;
+
+if (virJSONValueObjectCreate(,
+ "s:type", cmdname,
+ "A:data", , NULL) < 0)
+goto cleanup;
+
+if (virJSONValueArrayAppend(actions, entry) < 0)
+goto cleanup;
+
+entry = NULL;
+ret = 0;
+
+ cleanup:
+virJSONValueFree(entry);
+virJSONValueFree(data);
+va_end(args);
+
+return ret;
+}
+
+
 /**
  * qemuMonitorJSONMakeCommandInternal:
  * @cmdname: QMP command name
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 6bc0dd3ad2..da6c121d72 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -32,6 +32,10 @@
 # include "cpu/cpu.h"
 # include "util/virgic.h"

+int qemuMonitorJSONTransactionAdd(virJSONValuePtr actions,
+  const char *cmdname,
+  ...);
+
 int qemuMonitorJSONIOProcessLine(qemuMonitorPtr mon,
  const char *line,
  qemuMonitorMessagePtr msg);
-- 
2.16.2

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

[libvirt] [PATCH 2/8] qemu: snapshot: Remove monitor code now that 'transaction' is always used

[Peter Krempa]

Since we now always do the snapshot via the 'transaction' command we can
drop the code which would enter monitor for individual disk snapshots.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_driver.c | 25 +++--
 1 file changed, 3 insertions(+), 22 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4f577e50d2..39b745b1db 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14925,14 +14925,12 @@ 
qemuDomainSnapshotUpdateDiskSources(qemuDomainSnapshotDiskDataPtr dd,
 }


-/* The domain is expected to hold monitor lock.  */
 static int
 qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
  virDomainObjPtr vm,
  qemuDomainSnapshotDiskDataPtr dd,
  virJSONValuePtr actions,
- bool reuse,
- qemuDomainAsyncJob asyncJob)
+ bool reuse)
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
 char *device = NULL;
@@ -14964,23 +14962,10 @@ 
qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,

 dd->prepared = true;

-/* create the actual snapshot */
 formatStr = virStorageFileFormatTypeToString(dd->src->format);

-/* The monitor is only accessed if qemu doesn't support transactions.
- * Otherwise the following monitor command only constructs the command.
- */
-if (!actions &&
-qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
-goto cleanup;
-
 ret = rc = qemuMonitorDiskSnapshot(priv->mon, actions, device, source,
formatStr, reuse);
-if (!actions) {
-if (qemuDomainObjExitMonitor(driver, vm) < 0)
-ret = -1;
-}
-
 virDomainAuditDisk(vm, dd->disk->src, dd->src, "snapshot", rc >= 0);

  cleanup:
@@ -15032,11 +15017,7 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr 
driver,

 ret = qemuDomainSnapshotCreateSingleDiskActive(driver, vm,
[i],
-   actions, reuse, 
asyncJob);
-
-/* without transaction support the change can't be rolled back */
-if (!actions)
-qemuDomainSnapshotUpdateDiskSources([i], );
+   actions, reuse);

 if (ret < 0)
 goto error;
@@ -15044,7 +15025,7 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr 
driver,
 do_transaction = true;
 }

-if (actions && do_transaction) {
+if (do_transaction) {
 if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
 goto cleanup;

-- 
2.16.2

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

[libvirt] [PATCH 1/8] qemu: snapshot: Require support of 'transaction' command for external snapshots

[Peter Krempa]

While qemu supports the 'transaction' command since v1.1.0
(52e7c241ac766406f05fa) and the 'blockdev-snapshot-sync' command since
v0.14.0-rc0 we need to keep the capability bits present since some qemu
downstreams (RHEL/CentOS 7 for example) chose to cripple qemu by
arbitrarily compile out some stuff which was already present at that
time.

To simplify the crazy code just require both commands to be present at
the beginning of a external snapshot so that we can remove the case when
'transaction' would not be supported.

This also allows to drop any logic connected to the
VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC flag since snapshots are atomic with
the 'transaction' command.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_driver.c | 62 +++---
 1 file changed, 13 insertions(+), 49 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 825b2b27e6..4f577e50d2 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14614,18 +14614,9 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm,
 size_t i;
 bool active = virDomainObjIsActive(vm);
 bool reuse = (*flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0;
-bool atomic = (*flags & VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC) != 0;
 bool found_internal = false;
 bool forbid_internal = false;
 int external = 0;
-qemuDomainObjPrivatePtr priv = vm->privateData;
-
-if (def->state == VIR_DOMAIN_DISK_SNAPSHOT &&
-reuse && !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_TRANSACTION)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("reuse is not supported with this QEMU binary"));
-goto cleanup;
-}

 for (i = 0; i < def->ndisks; i++) {
 virDomainSnapshotDiskDefPtr disk = >disks[i];
@@ -14756,18 +14747,6 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm,
 if (external && !active)
 *flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY;

-if (def->state != VIR_DOMAIN_DISK_SNAPSHOT && active) {
-if (external == 1 ||
-virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_TRANSACTION)) {
-*flags |= VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC;
-} else if (atomic && external > 1) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("atomic live snapshot of multiple disks "
- "is unsupported"));
-goto cleanup;
-}
-}
-
 ret = 0;

  cleanup:
@@ -15033,15 +15012,8 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr 
driver,
 if (virDomainObjCheckActive(vm) < 0)
 return -1;

-if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_TRANSACTION)) {
-if (!(actions = virJSONValueNewArray()))
-return -1;
-} else if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DISK_SNAPSHOT)) {
-virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
-   _("live disk snapshot not supported with this "
- "QEMU binary"));
+if (!(actions = virJSONValueNewArray()))
 return -1;
-}

 /* prepare a list of objects to use in the vm definition so that we don't
  * have to roll back later */
@@ -15154,8 +15126,6 @@ qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr 
driver,
 char *xml = NULL;
 bool memory = snap->def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
 bool memory_unlink = false;
-bool atomic = !!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC);
-bool transaction = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_TRANSACTION);
 int thaw = 0; /* 1 if freeze succeeded, -1 if freeze failed */
 bool pmsuspended = false;
 virQEMUDriverConfigPtr cfg = NULL;
@@ -15163,6 +15133,14 @@ 
qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr driver,
 char *compressedpath = NULL;
 virQEMUSaveDataPtr data = NULL;

+if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DISK_SNAPSHOT) ||
+!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_TRANSACTION)) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+   _("live disk snapshot not supported with this "
+ "QEMU binary"));
+return -1;
+}
+
 /* If quiesce was requested, then issue a freeze command, and a
  * counterpart thaw command when it is actually sent to agent.
  * The command will fail if the guest is paused or the guest agent
@@ -15197,20 +15175,11 @@ 
qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr driver,
 } else if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) {
 /* For external checkpoints (those with memory), the guest
  * must pause (either by libvirt up front, or by qemu after
- * _LIVE converges).  For disk-only snapshots with multiple
- * disks, libvirt must pause externally to get all snapshots
- * to be at the same point in time, unless qemu supports
- * transactions.  For a single disk, snapshot is 

[libvirt] [PATCH 7/8] qemu: monitor: Remove old external snapshot code

[Peter Krempa]

Remove the dual mode code which allowed to create snapshots without
support for 'transaction'.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_monitor.c  | 17 -
 src/qemu/qemu_monitor.h  |  6 --
 src/qemu/qemu_monitor_json.c | 37 -
 src/qemu/qemu_monitor_json.h |  8 
 4 files changed, 68 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 6ed475ede0..5d7f6905ae 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3177,23 +3177,6 @@ qemuMonitorDeleteSnapshot(qemuMonitorPtr mon, const char 
*name)
 }


-/* Use the snapshot_blkdev command to convert the existing file for
- * device into a read-only backing file of a new qcow2 image located
- * at file.  */
-int
-qemuMonitorDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions,
-const char *device, const char *file,
-const char *format, bool reuse)
-{
-VIR_DEBUG("actions=%p, device=%s, file=%s, format=%s, reuse=%d",
-  actions, device, file, format, reuse);
-
-QEMU_CHECK_MONITOR(mon);
-
-return qemuMonitorJSONDiskSnapshot(mon, actions, device, file, format, 
reuse);
-}
-
-
 /* Start a drive-mirror block job.  bandwidth is in bytes/sec.  */
 int
 qemuMonitorDriveMirror(qemuMonitorPtr mon,
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index b3d62324b4..e09ca14bfa 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -827,12 +827,6 @@ int qemuMonitorCreateSnapshot(qemuMonitorPtr mon, const 
char *name);
 int qemuMonitorLoadSnapshot(qemuMonitorPtr mon, const char *name);
 int qemuMonitorDeleteSnapshot(qemuMonitorPtr mon, const char *name);

-int qemuMonitorDiskSnapshot(qemuMonitorPtr mon,
-virJSONValuePtr actions,
-const char *device,
-const char *file,
-const char *format,
-bool reuse);
 int qemuMonitorTransaction(qemuMonitorPtr mon, virJSONValuePtr *actions)
 ATTRIBUTE_NONNULL(2);
 int qemuMonitorDriveMirror(qemuMonitorPtr mon,
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 54fefcb612..cf1636d858 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -4101,43 +4101,6 @@ int qemuMonitorJSONDelObject(qemuMonitorPtr mon,
 }


-int
-qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions,
-const char *device, const char *file,
-const char *format, bool reuse)
-{
-int ret = -1;
-virJSONValuePtr cmd;
-virJSONValuePtr reply = NULL;
-
-cmd = qemuMonitorJSONMakeCommandRaw(actions != NULL,
-"blockdev-snapshot-sync",
-"s:device", device,
-"s:snapshot-file", file,
-"s:format", format,
-"S:mode", reuse ? "existing" : NULL,
-NULL);
-if (!cmd)
-return -1;
-
-if (actions) {
-if (virJSONValueArrayAppend(actions, cmd) == 0) {
-ret = 0;
-cmd = NULL;
-}
-} else {
-if ((ret = qemuMonitorJSONCommand(mon, cmd, )) < 0)
-goto cleanup;
-
-ret = qemuMonitorJSONCheckError(cmd, reply);
-}
-
- cleanup:
-virJSONValueFree(cmd);
-virJSONValueFree(reply);
-return ret;
-}
-
 /* speed is in bytes/sec */
 int
 qemuMonitorJSONDriveMirror(qemuMonitorPtr mon,
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index da6c121d72..b61046379c 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -242,14 +242,6 @@ int qemuMonitorJSONAddObject(qemuMonitorPtr mon,
 int qemuMonitorJSONDelObject(qemuMonitorPtr mon,
  const char *objalias);

-int qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon,
-virJSONValuePtr actions,
-const char *device,
-const char *file,
-const char *format,
-bool reuse)
-ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3)
-ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5);
 int qemuMonitorJSONTransaction(qemuMonitorPtr mon, virJSONValuePtr *actions)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
 int qemuMonitorJSONDriveMirror(qemuMonitorPtr mon,
-- 
2.16.2

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

[libvirt] [PATCH 0/8] qemu: Sanitize monitor code to create snapshots

[Peter Krempa]

Require that the 'transaction' command is present for external snapshots
to work and remove the various hacks which were present to make it work
without that.

Peter Krempa (8):
  qemu: snapshot: Require support of 'transaction' command for external
snapshots
  qemu: snapshot: Remove monitor code now that 'transaction' is always
used
  qemu: snapshot: Unify conditions checking whether snapshot needs to be
taken
  qemu: snapshot: Audit actual disk snapshot creation
  qemu: monitor: Add API to help creating 'transaction' arguments
  qemu: block: Create helper for creating data for legacy snapshots
  qemu: monitor: Remove old external snapshot code
  qemu: monitor: Remove old code for dual handling of 'transaction' data

 src/qemu/qemu_block.c|  37 +
 src/qemu/qemu_block.h|   6 ++
 src/qemu/qemu_driver.c   | 128 ---
 src/qemu/qemu_monitor.c  |  17 --
 src/qemu/qemu_monitor.h  |   6 --
 src/qemu/qemu_monitor_json.c | 125 +++---
 src/qemu/qemu_monitor_json.h |  12 ++--
 7 files changed, 138 insertions(+), 193 deletions(-)

-- 
2.16.2

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

[libvirt] [PATCH 6/8] qemu: block: Create helper for creating data for legacy snapshots

[Peter Krempa]

With 'transaction' support we don't need to keep around the multipurpose
code which would create the snapshot if 'transaction' is not supported.

To simplify this add a new helper that just wraps the arguments for
'blockdev-snapshot-sync' operation in 'transaction' and use it instead
of qemuBlockSnapshotAddLegacy.

Additionally this allows to format the arguments prior to creating the
file for simpler cleanup.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_block.c  | 37 +
 src/qemu/qemu_block.h  |  6 ++
 src/qemu/qemu_driver.c | 18 +++---
 3 files changed, 46 insertions(+), 15 deletions(-)

diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 0ebf2d2aff..db1579ca20 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -19,8 +19,10 @@
 #include 

 #include "qemu_block.h"
+#include "qemu_command.h"
 #include "qemu_domain.h"
 #include "qemu_alias.h"
+#include "qemu_monitor_json.h"

 #include "viralloc.h"
 #include "virstring.h"
@@ -1683,3 +1685,38 @@ qemuBlockStorageSourceDetachOneBlockdev(virQEMUDriverPtr 
driver,

 return ret;
 }
+
+
+int
+qemuBlockSnapshotAddLegacy(virJSONValuePtr actions,
+   virDomainDiskDefPtr disk,
+   virStorageSourcePtr newsrc,
+   bool reuse)
+{
+const char *format = virStorageFileFormatTypeToString(newsrc->format);
+char *device = NULL;
+char *source = NULL;
+int ret = -1;
+
+if (!(device = qemuAliasDiskDriveFromDisk(disk)))
+goto cleanup;
+
+if (qemuGetDriveSourceString(newsrc, NULL, ) < 0)
+goto cleanup;
+
+if (qemuMonitorJSONTransactionAdd(actions, "blockdev-snapshot-sync",
+  "s:device", device,
+  "s:snapshot-file", source,
+  "s:format", format,
+  "S:mode", reuse ? "existing" : NULL,
+   NULL) < 0)
+goto cleanup;
+
+ret = 0;
+
+ cleanup:
+VIR_FREE(device);
+VIR_FREE(source);
+
+return ret;
+}
diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h
index 418b5064b5..fd8984e60b 100644
--- a/src/qemu/qemu_block.h
+++ b/src/qemu/qemu_block.h
@@ -117,4 +117,10 @@ qemuBlockStorageSourceDetachOneBlockdev(virQEMUDriverPtr 
driver,
 qemuDomainAsyncJob asyncJob,
 virStorageSourcePtr src);

+int
+qemuBlockSnapshotAddLegacy(virJSONValuePtr actions,
+   virDomainDiskDefPtr disk,
+   virStorageSourcePtr newsrc,
+   bool reuse);
+
 #endif /* __QEMU_BLOCK_H__ */
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ea06e23ff1..2d8af5daaa 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14932,23 +14932,16 @@ 
qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
  virJSONValuePtr actions,
  bool reuse)
 {
-qemuDomainObjPrivatePtr priv = vm->privateData;
-char *device = NULL;
-char *source = NULL;
-const char *formatStr = NULL;
 int ret = -1;

-if (!(device = qemuAliasDiskDriveFromDisk(dd->disk)))
-goto cleanup;
-
-if (qemuGetDriveSourceString(dd->src, NULL, ) < 0)
+if (qemuBlockSnapshotAddLegacy(actions, dd->disk, dd->src, reuse) < 0)
 goto cleanup;

 /* pre-create the image file so that we can label it before handing it to 
qemu */
 if (!reuse && dd->src->type != VIR_STORAGE_TYPE_BLOCK) {
 if (virStorageFileCreate(dd->src) < 0) {
 virReportSystemError(errno, _("failed to create image file '%s'"),
- source);
+ NULLSTR(dd->src->path));
 goto cleanup;
 }
 dd->created = true;
@@ -14962,14 +14955,9 @@ 
qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,

 dd->prepared = true;

-formatStr = virStorageFileFormatTypeToString(dd->src->format);
-
-ret = qemuMonitorDiskSnapshot(priv->mon, actions, device, source,
-  formatStr, reuse);
+ret = 0;

  cleanup:
-VIR_FREE(device);
-VIR_FREE(source);
 return ret;
 }

-- 
2.16.2

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

[libvirt] [PATCH 8/8] qemu: monitor: Remove old code for dual handling of 'transaction' data

[Peter Krempa]

Now that we use only the separate function for creating data for the
'transaction' command we can remove all the boilerplate which was
necessary before.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_monitor_json.c | 42 ++
 1 file changed, 10 insertions(+), 32 deletions(-)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index cf1636d858..87ec496d0d 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -514,48 +514,30 @@ qemuMonitorJSONTransactionAdd(virJSONValuePtr actions,
  * qemuMonitorJSONMakeCommandInternal:
  * @cmdname: QMP command name
  * @arguments: a JSON object containing command arguments or NULL
- * @transaction: format the command as arguments for the 'transaction' command
  *
- * Create a JSON object used on the QMP monitor to call a command. If
- * @transaction is true, the returned object is formatted to be used as a 
member
- * of the 'transaction' command.
+ * Create a JSON object used on the QMP monitor to call a command.
  *
  * Note that @arguments is always consumed and should not be referenced after
  * the call to this function.
  */
 static virJSONValuePtr
 qemuMonitorJSONMakeCommandInternal(const char *cmdname,
-   virJSONValuePtr arguments,
-   bool transaction)
+   virJSONValuePtr arguments)
 {
-virJSONValuePtr cmd = NULL;
 virJSONValuePtr ret = NULL;

-if (!transaction) {
-if (virJSONValueObjectCreate(,
- "s:execute", cmdname,
- "A:arguments", , NULL) < 0)
-goto cleanup;
-} else {
-if (virJSONValueObjectCreate(,
- "s:type", cmdname,
- "A:data", , NULL) < 0)
-goto cleanup;
-}
-
-VIR_STEAL_PTR(ret, cmd);
+ignore_value(virJSONValueObjectCreate(,
+  "s:execute", cmdname,
+  "A:arguments", , NULL));

- cleanup:
 virJSONValueFree(arguments);
-virJSONValueFree(cmd);
 return ret;
 }


 static virJSONValuePtr ATTRIBUTE_SENTINEL
-qemuMonitorJSONMakeCommandRaw(bool transaction,
-  const char *cmdname,
-  ...)
+qemuMonitorJSONMakeCommand(const char *cmdname,
+   ...)
 {
 virJSONValuePtr obj = NULL;
 virJSONValuePtr jargs = NULL;
@@ -566,7 +548,7 @@ qemuMonitorJSONMakeCommandRaw(bool transaction,
 if (virJSONValueObjectCreateVArgs(, args) < 0)
 goto cleanup;

-obj = qemuMonitorJSONMakeCommandInternal(cmdname, jargs, transaction);
+obj = qemuMonitorJSONMakeCommandInternal(cmdname, jargs);

  cleanup:
 va_end(args);
@@ -574,9 +556,6 @@ qemuMonitorJSONMakeCommandRaw(bool transaction,
 return obj;
 }

-#define qemuMonitorJSONMakeCommand(cmdname, ...) \
-qemuMonitorJSONMakeCommandRaw(false, cmdname, __VA_ARGS__)
-

 static virJSONValuePtr
 qemuMonitorJSONKeywordStringToJSON(const char *str, const char *firstkeyword)
@@ -4057,7 +4036,7 @@ qemuMonitorJSONAddObject(qemuMonitorPtr mon,
 virJSONValuePtr cmd;
 virJSONValuePtr reply = NULL;

-if (!(cmd = qemuMonitorJSONMakeCommandInternal("object-add", props, 
false)))
+if (!(cmd = qemuMonitorJSONMakeCommandInternal("object-add", props)))
 goto cleanup;

 if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
@@ -7964,8 +7943,7 @@ qemuMonitorJSONBlockdevAdd(qemuMonitorPtr mon,
 virJSONValuePtr reply = NULL;
 int ret = -1;

-if (!(cmd = qemuMonitorJSONMakeCommandInternal("blockdev-add",
-   props, false)))
+if (!(cmd = qemuMonitorJSONMakeCommandInternal("blockdev-add", props)))
 return -1;

 if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
-- 
2.16.2

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

[libvirt] [PATCH 3/8] qemu: snapshot: Unify conditions checking whether snapshot needs to be taken

[Peter Krempa]

In the cleanup path we already checked whether a snapshot needed to be
taken by looking into the collected data. Use the same approach when
creating the snapshot command data and when commiting the changes to the
domain definition.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_driver.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 39b745b1db..e5005fd829 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15012,7 +15012,7 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr 
driver,
   * VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL with a valid file name and
   * qcow2 format.  */
 for (i = 0; i < snap->def->ndisks; i++) {
-if (snap->def->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE)
+if (!diskdata[i].src)
 continue;

 ret = qemuDomainSnapshotCreateSingleDiskActive(driver, vm,
@@ -15036,8 +15036,14 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr 
driver,
 goto error;
 }

-for (i = 0; i < snap->def->ndisks; i++)
+for (i = 0; i < snap->def->ndisks; i++) {
+qemuDomainSnapshotDiskDataPtr dd = [i];
+
+if (!dd->src)
+continue;
+
 qemuDomainSnapshotUpdateDiskSources([i], );
+}
 }

  error:
-- 
2.16.2

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

[libvirt] [PATCH 4/8] qemu: snapshot: Audit actual disk snapshot creation

[Peter Krempa]

Currently we'd audit that we managed to format the data for the
'transaction' command rather than the (un)successful attempt to create
the snapshot.

Move the auditing code so that it can actually audit the result of the
'transaction' command.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_driver.c | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e5005fd829..ea06e23ff1 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14936,7 +14936,7 @@ 
qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,
 char *device = NULL;
 char *source = NULL;
 const char *formatStr = NULL;
-int ret = -1, rc;
+int ret = -1;

 if (!(device = qemuAliasDiskDriveFromDisk(dd->disk)))
 goto cleanup;
@@ -14964,9 +14964,8 @@ 
qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver,

 formatStr = virStorageFileFormatTypeToString(dd->src->format);

-ret = rc = qemuMonitorDiskSnapshot(priv->mon, actions, device, source,
-   formatStr, reuse);
-virDomainAuditDisk(vm, dd->disk->src, dd->src, "snapshot", rc >= 0);
+ret = qemuMonitorDiskSnapshot(priv->mon, actions, device, source,
+  formatStr, reuse);

  cleanup:
 VIR_FREE(device);
@@ -15031,10 +15030,8 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr 
driver,

 ret = qemuMonitorTransaction(priv->mon, );

-if (qemuDomainObjExitMonitor(driver, vm) < 0 || ret < 0) {
+if (qemuDomainObjExitMonitor(driver, vm) < 0)
 ret = -1;
-goto error;
-}

 for (i = 0; i < snap->def->ndisks; i++) {
 qemuDomainSnapshotDiskDataPtr dd = [i];
@@ -15042,8 +15039,14 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr 
driver,
 if (!dd->src)
 continue;

-qemuDomainSnapshotUpdateDiskSources([i], );
+virDomainAuditDisk(vm, dd->disk->src, dd->src, "snapshot", ret >= 
0);
+
+if (ret == 0)
+qemuDomainSnapshotUpdateDiskSources(dd, );
 }
+
+if (ret < 0)
+goto error;
 }

  error:
-- 
2.16.2

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

Re: [libvirt] [dbus PATCH] connect: fix g_free order in virtDBusConnectFree

[Pavel Hrdina]

On Mon, Jul 02, 2018 at 11:42:08AM -0400, Anya Harter wrote:
> so that g_free(connect->nodeDevPath) line appears in alphabetical order
> with the rest of the lines
> 
> Signed-off-by: Anya Harter 
> ---
>  src/connect.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

In general we don't send/merge patches that just move code without any
functional reason because it pollutes git history but in this case I
would say it's ok.

Reviewed-by: Pavel Hrdina  and pushed.


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

[libvirt] [PATCH] qemu: hotplug: Don't access srcPriv when it's not allocated

[Peter Krempa]

The private data of a virStorageSource which is backing an iSCSI hostdev
may be NULL if no authentication is present. The code handling the
hotplug would attempt to extract the authentication info stored in
'secinfo' without checking if it is allocated which resulted in a crash.

Here we opt the easy way to check if srcPriv is not NULL so that we
don't duplicate all the logic which selects whether the disk source has
a secret.

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

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_hotplug.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index fcd8eb0ffa..075f2fb72e 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2240,7 +2240,8 @@ qemuDomainAttachHostSCSIDevice(virQEMUDriverPtr driver,
 if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) {
 qemuDomainStorageSourcePrivatePtr srcPriv =
 QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(scsisrc->u.iscsi.src);
-secinfo = srcPriv->secinfo;
+if (srcPriv)
+secinfo = srcPriv->secinfo;
 }

 if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) {
-- 
2.16.2

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

[libvirt] [PATCH] util:Fix with process number and pid file do not match

[dubo163]

the libvirtd pid file is not match the os process pid number
which is smaller than before.

this would be exist if the libvirtd process coredump or the os
process was killed which the next pid number is smaller.

you can be also edit the pid file to write the longer number than
before,then restart the libvirtd service.

Signed-off-by: dubo163 
---
 src/util/virpidfile.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c
index 58ab29f..1a85d43 100644
--- a/src/util/virpidfile.c
+++ b/src/util/virpidfile.c
@@ -446,6 +446,14 @@ int virPidFileAcquirePath(const char *path,
 
 snprintf(pidstr, sizeof(pidstr), "%lld", (long long) pid);
 
+if (ftruncate(fd, 0) < 0) {
+virReportSystemError(errno,
+ _("Failed to truncate pid file '%s'"),
+ path);
+VIR_FORCE_CLOSE(fd);
+return -1;
+}
+
 if (safewrite(fd, pidstr, strlen(pidstr)) < 0) {
 virReportSystemError(errno,
  _("Failed to write to pid file '%s'"),
-- 
1.8.3.1

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

[libvirt] [PATCH] util:Fix with process number and pid file do not match

[dubo163]

dubo163 (1):
  util:Fix with process number and pid file do not match

 src/util/virpidfile.c | 8 
 1 file changed, 8 insertions(+)

-- 
1.8.3.1

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

[libvirt] [PATCH v2 2/6] rpc: Initialize a worker pool for max_workers=0 as well

[Marc Hartmayer]

Semantically, there is no difference between an uninitialized worker
pool and an initialized worker pool with zero workers. Let's allow the
worker pool to be initialized for max_workers=0 as well then which
makes the API more symmetric and simplifies code. Validity of the
worker pool is delegated to virThreadPoolGetMaxWorkersLocked instead.

This patch fixes segmentation faults in
virNetServerGetThreadPoolParameters and
virNetServerSetThreadPoolParameters for the case when no worker pool
is actually initialized (max_workers=0).

Signed-off-by: Marc Hartmayer 
Reviewed-by: Boris Fiuczynski 
Reviewed-by: Bjoern Walk 
---
 src/libvirt_private.syms |  4 +++
 src/rpc/virnetserver.c   | 13 -
 src/util/virthreadpool.c | 73 
 src/util/virthreadpool.h |  8 ++
 4 files changed, 73 insertions(+), 25 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index ffe5dfd19b11..aa496ddf8012 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3005,11 +3005,15 @@ virThreadPoolGetCurrentWorkers;
 virThreadPoolGetFreeWorkers;
 virThreadPoolGetJobQueueDepth;
 virThreadPoolGetMaxWorkers;
+virThreadPoolGetMaxWorkersLocked;
 virThreadPoolGetMinWorkers;
 virThreadPoolGetPriorityWorkers;
+virThreadPoolLock;
 virThreadPoolNewFull;
 virThreadPoolSendJob;
+virThreadPoolSendJobLocked;
 virThreadPoolSetParameters;
+virThreadPoolUnlock;
 
 
 # util/virtime.h
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 091e3ef23406..fdee08fee7cd 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -203,7 +203,8 @@ virNetServerDispatchNewMessage(virNetServerClientPtr client,
  * of disposing @srv */
 virObjectUnlock(srv);
 
-if (srv->workers) {
+virThreadPoolLock(srv->workers);
+if (virThreadPoolGetMaxWorkersLocked(srv->workers) > 0)  {
 virNetServerJobPtr job;
 
 if (VIR_ALLOC(job) < 0)
@@ -218,7 +219,7 @@ virNetServerDispatchNewMessage(virNetServerClientPtr client,
 }
 
 virObjectRef(client);
-if (virThreadPoolSendJob(srv->workers, priority, job) < 0) {
+if (virThreadPoolSendJobLocked(srv->workers, priority, job) < 0) {
 virObjectUnref(client);
 VIR_FREE(job);
 virObjectUnref(prog);
@@ -228,10 +229,12 @@ virNetServerDispatchNewMessage(virNetServerClientPtr 
client,
 if (virNetServerProcessMsg(srv, client, prog, msg) < 0)
 goto error;
 }
+virThreadPoolUnlock(srv->workers);
 
 return;
 
  error:
+virThreadPoolUnlock(srv->workers);
 virNetMessageFree(msg);
 virNetServerClientClose(client);
 }
@@ -363,8 +366,7 @@ virNetServerPtr virNetServerNew(const char *name,
 if (!(srv = virObjectLockableNew(virNetServerClass)))
 return NULL;
 
-if (max_workers &&
-!(srv->workers = virThreadPoolNew(min_workers, max_workers,
+if (!(srv->workers = virThreadPoolNew(min_workers, max_workers,
   priority_workers,
   virNetServerHandleJob,
   srv)))
@@ -575,21 +577,18 @@ virJSONValuePtr 
virNetServerPreExecRestart(virNetServerPtr srv)
 goto error;
 
 if (virJSONValueObjectAppendNumberUint(object, "min_workers",
-   srv->workers == NULL ? 0 :

virThreadPoolGetMinWorkers(srv->workers)) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Cannot set min_workers data in JSON document"));
 goto error;
 }
 if (virJSONValueObjectAppendNumberUint(object, "max_workers",
-   srv->workers == NULL ? 0 :

virThreadPoolGetMaxWorkers(srv->workers)) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Cannot set max_workers data in JSON document"));
 goto error;
 }
 if (virJSONValueObjectAppendNumberUint(object, "priority_workers",
-   srv->workers == NULL ? 0 :

virThreadPoolGetPriorityWorkers(srv->workers)) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Cannot set priority_workers data in JSON document"));
diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c
index 10f2bd2c3a03..f18eafb35deb 100644
--- a/src/util/virthreadpool.c
+++ b/src/util/virthreadpool.c
@@ -318,17 +318,27 @@ size_t virThreadPoolGetMinWorkers(virThreadPoolPtr pool)
 return ret;
 }
 
-size_t virThreadPoolGetMaxWorkers(virThreadPoolPtr pool)
+
+size_t
+virThreadPoolGetMaxWorkersLocked(virThreadPoolPtr pool)
+{
+return pool->maxWorkers;
+}
+
+
+size_t
+virThreadPoolGetMaxWorkers(virThreadPoolPtr pool)
 {
 size_t ret;
 
 virMutexLock(>mutex);
-ret = 

[libvirt] [PATCH v2 6/6] rpc: Fix name of include guard

[Marc Hartmayer]

The include guard should match the file name and comment.

Signed-off-by: Marc Hartmayer 
Reviewed-by: Boris Fiuczynski 
---
 src/rpc/virnetserverprogram.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/rpc/virnetserverprogram.h b/src/rpc/virnetserverprogram.h
index c2a5635c959b..03784478de0b 100644
--- a/src/rpc/virnetserverprogram.h
+++ b/src/rpc/virnetserverprogram.h
@@ -21,8 +21,8 @@
  * Author: Daniel P. Berrange 
  */
 
-#ifndef __VIR_NET_PROGRAM_H__
-# define __VIR_NET_PROGRAM_H__
+#ifndef __VIR_NET_SERVER_PROGRAM_H__
+# define __VIR_NET_SERVER_PROGRAM_H__
 
 # include "virnetmessage.h"
 # include "virnetserverclient.h"
-- 
2.13.4

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

[libvirt] [PATCH v2 4/6] daemon: Raise an error if 'max_workers' < 1 in libvirtd.conf

[Marc Hartmayer]

Hypervisor drivers (e.g. QEMU) assume that they run in a separate
thread from the main event loop thread otherwise deadlocks can
occur. Therefore let's report an error if max_workers < 1 is set in
the libvirtd configuration file.

Signed-off-by: Marc Hartmayer 
Reviewed-by: Boris Fiuczynski 
Reviewed-by: Bjoern Walk 
---
 src/remote/remote_daemon_config.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/src/remote/remote_daemon_config.c 
b/src/remote/remote_daemon_config.c
index b1516befb4a6..27e0c635f1be 100644
--- a/src/remote/remote_daemon_config.c
+++ b/src/remote/remote_daemon_config.c
@@ -337,6 +337,11 @@ daemonConfigLoadOptions(struct daemonConfig *data,
 goto error;
 if (virConfGetValueUInt(conf, "max_workers", >max_workers) < 0)
 goto error;
+if (data->max_workers < 1) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("'max_workers' must be greater than 0"));
+goto error;
+}
 if (virConfGetValueUInt(conf, "max_clients", >max_clients) < 0)
 goto error;
 if (virConfGetValueUInt(conf, "max_queued_clients", 
>max_queued_clients) < 0)
-- 
2.13.4

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

[libvirt] [PATCH v2 5/6] virt-admin: Fix two error messages

[Marc Hartmayer]

Signed-off-by: Marc Hartmayer 
Reviewed-by: Boris Fiuczynski 
---
 tools/virt-admin.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/virt-admin.c b/tools/virt-admin.c
index c86b5763a73c..107a1d870df5 100644
--- a/tools/virt-admin.c
+++ b/tools/virt-admin.c
@@ -552,7 +552,8 @@ cmdSrvThreadpoolSet(vshControl *ctl, const vshCmd *cmd)
   VIR_THREADPOOL_WORKERS_MAX, ) &&
 virTypedParamsGetUInt(params, nparams,
   VIR_THREADPOOL_WORKERS_MIN, ) && min > max) {
-vshError(ctl, "%s", _("--min-workers must be less than 
--max-workers"));
+vshError(ctl, "%s", _("--min-workers must be less than or equal to "
+  "--max-workers"));
 goto cleanup;
 }
 
@@ -952,7 +953,7 @@ cmdSrvClientsSet(vshControl *ctl, const vshCmd *cmd)
 virTypedParamsGetUInt(params, nparams,
   VIR_SERVER_CLIENTS_UNAUTH_MAX, _max) &&
 unauth_max > max) {
-vshError(ctl, "%s", _("--max-unauth-clients must be less than "
+vshError(ctl, "%s", _("--max-unauth-clients must be less than or equal 
to "
   "--max-clients"));
 goto cleanup;
 }
-- 
2.13.4

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

[libvirt] [PATCH v2 1/6] rpc: Fix deadlock if there is no worker pool available

[Marc Hartmayer]

@srv must be unlocked for the call virNetServerProcessMsg otherwise a
deadlock can occur.

Since the pointer 'srv->workers' will never be changed after
initialization and the thread pool has it's own locking we can release
the lock of 'srv' earlier. This also fixes the deadlock.

Signed-off-by: Marc Hartmayer 
Reviewed-by: Boris Fiuczynski 
Reviewed-by: Bjoern Walk 
---
 src/rpc/virnetserver.c | 15 ++-
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 5c7f7dd08fe1..091e3ef23406 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -51,6 +51,7 @@ struct _virNetServer {
 
 char *name;
 
+/* Immutable pointer, self-locking APIs */
 virThreadPoolPtr workers;
 
 char *mdnsGroupName;
@@ -177,9 +178,11 @@ static void virNetServerHandleJob(void *jobOpaque, void 
*opaque)
 VIR_FREE(job);
 }
 
-static void virNetServerDispatchNewMessage(virNetServerClientPtr client,
-   virNetMessagePtr msg,
-   void *opaque)
+
+static void
+virNetServerDispatchNewMessage(virNetServerClientPtr client,
+   virNetMessagePtr msg,
+   void *opaque)
 {
 virNetServerPtr srv = opaque;
 virNetServerProgramPtr prog = NULL;
@@ -196,6 +199,9 @@ static void 
virNetServerDispatchNewMessage(virNetServerClientPtr client,
 break;
 }
 }
+/* we can unlock @srv since @prog can only become invalid in case
+ * of disposing @srv */
+virObjectUnlock(srv);
 
 if (srv->workers) {
 virNetServerJobPtr job;
@@ -223,15 +229,14 @@ static void 
virNetServerDispatchNewMessage(virNetServerClientPtr client,
 goto error;
 }
 
-virObjectUnlock(srv);
 return;
 
  error:
 virNetMessageFree(msg);
 virNetServerClientClose(client);
-virObjectUnlock(srv);
 }
 
+
 /**
  * virNetServerCheckLimits:
  * @srv: server to check limits on
-- 
2.13.4

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

[libvirt] [PATCH v2 0/6] Fixes for segfault and deadlock

[Marc Hartmayer]

One way to reproduce the bugs is to set admin_max_workers=0 in
libvirtd.conf, restart libvirtd, and then call:

 $ virt-admin server-threadpool-info admin

Changelog:
 v1->v2:
  - Worked in Daniel Berrangé's comments that:
1. max_workers=0 should not be allowed for the libvirtd (patch 3)
2. it shouldn't be allowed to switch between zero and non-zero
   max_workers (patch 4)
  - Changed the deadlock fix (patch 1)
  - Added two small fixes (patch 5 and 6)  

Marc Hartmayer (6):
  rpc: Fix deadlock if there is no worker pool available
  rpc: Initialize a worker pool for max_workers=0 as well
  virThreadPool: Prevent switching between zero and non-zero maxWorkers
  daemon: Raise an error if 'max_workers' < 1 in libvirtd.conf
  virt-admin: Fix two error messages
  rpc: Fix name of include guard

 src/libvirt_private.syms  |  4 ++
 src/remote/remote_daemon_config.c |  5 +++
 src/rpc/virnetserver.c| 28 --
 src/rpc/virnetserverprogram.h |  4 +-
 src/util/virthreadpool.c  | 81 ++-
 src/util/virthreadpool.h  |  8 
 tools/virt-admin.c|  5 ++-
 7 files changed, 101 insertions(+), 34 deletions(-)

-- 
2.13.4

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

[libvirt] [PATCH v2 3/6] virThreadPool: Prevent switching between zero and non-zero maxWorkers

[Marc Hartmayer]

...since maxWorkers=0 is only intended for virtlockd or virlogd which
must not be multithreaded.

Signed-off-by: Marc Hartmayer 
Reviewed-by: Boris Fiuczynski 
Reviewed-by: Bjoern Walk 
---
 src/util/virthreadpool.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c
index f18eafb35deb..e615c8c07c82 100644
--- a/src/util/virthreadpool.c
+++ b/src/util/virthreadpool.c
@@ -479,6 +479,14 @@ virThreadPoolSetParameters(virThreadPoolPtr pool,
 goto error;
 }
 
+if ((maxWorkers == 0 && pool->maxWorkers > 0) ||
+(maxWorkers > 0 && pool->maxWorkers == 0)) {
+virReportError(VIR_ERR_INVALID_ARG, "%s",
+   _("maxWorkers must not be switched from zero to 
non-zero"
+ " and vice versa"));
+goto error;
+}
+
 if (minWorkers >= 0) {
 if ((size_t) minWorkers > pool->nWorkers &&
 virThreadPoolExpand(pool, minWorkers - pool->nWorkers,
-- 
2.13.4

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

Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial

[Daniel P . Berrangé]

On Tue, Jul 03, 2018 at 01:32:29PM +0200, Kevin Wolf wrote:
> Am 03.07.2018 um 13:22 hat Daniel P. Berrangé geschrieben:
> > On Tue, Jul 03, 2018 at 12:53:44PM +0200, Christian Borntraeger wrote:
> > > 
> > > 
> > > On 07/02/2018 10:04 AM, Kevin Wolf wrote:
> > > > Am 25.06.2018 um 13:45 hat Peter Krempa geschrieben:
> > > >> On Mon, Jun 25, 2018 at 13:41:06 +0200, Kevin Wolf wrote:
> > > >>> Am 25.06.2018 um 11:53 hat Daniel P. Berrangé geschrieben:
> > >  On Fri, Jun 22, 2018 at 03:31:46PM +0100, Daniel P. Berrangé wrote:
> > > > On Fri, Jun 22, 2018 at 04:25:13PM +0200, Kevin Wolf wrote:
> > > >> Am 22.06.2018 um 15:36 hat Christian Borntraeger geschrieben:
> > > >>
> > > >> [...]
> > > >>
> > > >>>
> > > >>> Thanks!
> > > >>>
> > > >>> I'll look into werror/rerror support for usb-storage. It shouldn't be
> > > >>> too hard, though it's strictly speaking a separate problem related to
> > > >>> using -blockdev rather than option deprecation.
> > > >>>
> > > >>> If Peter wants to wait for QEMU support before converting 
> > > >>> werror/rerror
> > > >>
> > > >> Definitely. I don't want to keep around yet another hack that will
> > > >> satisfy one specific case and then add another capability for it. We
> > > >> should then gate the moving of the feature based on the presence of
> > > >> werror for usb-storage.
> > > >>
> > > >>> to -device, maybe it would make sense to split your patch for v2 so 
> > > >>> that
> > > >>> geometry and serial can get fixed right away?
> > > >>
> > > >> Yes this can be done right away.
> > > > 
> > > > Has serial/gemoetry been fixed meanwhile and will it make it into the
> > > > next release?
> > > 
> > > I cannot find an archive that has it, but it is on the libvirt mailing
> > > list as "[libvirt] [PATCH v3] qemu: format serial and geometry on 
> > > frontend disk device".
> > > Review seems done, but it has missed libvirt 4.5 which was released today.
> > 
> > Just posted latest version here:
> > 
> >   https://www.redhat.com/archives/libvir-list/2018-July/msg00130.html
> > 
> > It will be in the next release on ~ Aug 1st
> 
> It would have been a lot nicer to have it the July release because this
> means that we'll have the released libvirt broken during almost the
> whole rc phase of QEMU 3.0, but the release is planned for Aug 8th the
> earliest, so I guess we're still okay. People using QEMU from git will
> just need libvirt from git as well.

Yeah, unfortunately i just missed the window of possibility to get it
into yesterday's release.

Fedora at least will be ok, and when other distros do pick up new QEMU
they'll likely pick up the corresponding libvirt release at same time
anyway.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial

[Peter Maydell]

On 3 July 2018 at 12:32, Kevin Wolf  wrote:
> Am 03.07.2018 um 13:22 hat Daniel P. Berrangé geschrieben:
>> Just posted latest version here:
>>
>>   https://www.redhat.com/archives/libvir-list/2018-July/msg00130.html
>>
>> It will be in the next release on ~ Aug 1st
>
> It would have been a lot nicer to have it the July release because this
> means that we'll have the released libvirt broken during almost the
> whole rc phase of QEMU 3.0, but the release is planned for Aug 8th the
> earliest, so I guess we're still okay. People using QEMU from git will
> just need libvirt from git as well.

I'm still not clear what we gain from having a QEMU that's dropped
a feature that is still used by everything except leading-edge
not-yet-released versions of libvirt. Is there a strong reason we
can't just revert the deletion of the deprecated feature for a QEMU
release or two?

thanks
-- PMM

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

Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial

[Kevin Wolf]

Am 03.07.2018 um 13:22 hat Daniel P. Berrangé geschrieben:
> On Tue, Jul 03, 2018 at 12:53:44PM +0200, Christian Borntraeger wrote:
> > 
> > 
> > On 07/02/2018 10:04 AM, Kevin Wolf wrote:
> > > Am 25.06.2018 um 13:45 hat Peter Krempa geschrieben:
> > >> On Mon, Jun 25, 2018 at 13:41:06 +0200, Kevin Wolf wrote:
> > >>> Am 25.06.2018 um 11:53 hat Daniel P. Berrangé geschrieben:
> >  On Fri, Jun 22, 2018 at 03:31:46PM +0100, Daniel P. Berrangé wrote:
> > > On Fri, Jun 22, 2018 at 04:25:13PM +0200, Kevin Wolf wrote:
> > >> Am 22.06.2018 um 15:36 hat Christian Borntraeger geschrieben:
> > >>
> > >> [...]
> > >>
> > >>>
> > >>> Thanks!
> > >>>
> > >>> I'll look into werror/rerror support for usb-storage. It shouldn't be
> > >>> too hard, though it's strictly speaking a separate problem related to
> > >>> using -blockdev rather than option deprecation.
> > >>>
> > >>> If Peter wants to wait for QEMU support before converting werror/rerror
> > >>
> > >> Definitely. I don't want to keep around yet another hack that will
> > >> satisfy one specific case and then add another capability for it. We
> > >> should then gate the moving of the feature based on the presence of
> > >> werror for usb-storage.
> > >>
> > >>> to -device, maybe it would make sense to split your patch for v2 so that
> > >>> geometry and serial can get fixed right away?
> > >>
> > >> Yes this can be done right away.
> > > 
> > > Has serial/gemoetry been fixed meanwhile and will it make it into the
> > > next release?
> > 
> > I cannot find an archive that has it, but it is on the libvirt mailing
> > list as "[libvirt] [PATCH v3] qemu: format serial and geometry on frontend 
> > disk device".
> > Review seems done, but it has missed libvirt 4.5 which was released today.
> 
> Just posted latest version here:
> 
>   https://www.redhat.com/archives/libvir-list/2018-July/msg00130.html
> 
> It will be in the next release on ~ Aug 1st

It would have been a lot nicer to have it the July release because this
means that we'll have the released libvirt broken during almost the
whole rc phase of QEMU 3.0, but the release is planned for Aug 8th the
earliest, so I guess we're still okay. People using QEMU from git will
just need libvirt from git as well.

Kevin

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

Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial

[Daniel P . Berrangé]

On Tue, Jul 03, 2018 at 12:53:44PM +0200, Christian Borntraeger wrote:
> 
> 
> On 07/02/2018 10:04 AM, Kevin Wolf wrote:
> > Am 25.06.2018 um 13:45 hat Peter Krempa geschrieben:
> >> On Mon, Jun 25, 2018 at 13:41:06 +0200, Kevin Wolf wrote:
> >>> Am 25.06.2018 um 11:53 hat Daniel P. Berrangé geschrieben:
>  On Fri, Jun 22, 2018 at 03:31:46PM +0100, Daniel P. Berrangé wrote:
> > On Fri, Jun 22, 2018 at 04:25:13PM +0200, Kevin Wolf wrote:
> >> Am 22.06.2018 um 15:36 hat Christian Borntraeger geschrieben:
> >>
> >> [...]
> >>
> >>>
> >>> Thanks!
> >>>
> >>> I'll look into werror/rerror support for usb-storage. It shouldn't be
> >>> too hard, though it's strictly speaking a separate problem related to
> >>> using -blockdev rather than option deprecation.
> >>>
> >>> If Peter wants to wait for QEMU support before converting werror/rerror
> >>
> >> Definitely. I don't want to keep around yet another hack that will
> >> satisfy one specific case and then add another capability for it. We
> >> should then gate the moving of the feature based on the presence of
> >> werror for usb-storage.
> >>
> >>> to -device, maybe it would make sense to split your patch for v2 so that
> >>> geometry and serial can get fixed right away?
> >>
> >> Yes this can be done right away.
> > 
> > Has serial/gemoetry been fixed meanwhile and will it make it into the
> > next release?
> 
> I cannot find an archive that has it, but it is on the libvirt mailing
> list as "[libvirt] [PATCH v3] qemu: format serial and geometry on frontend 
> disk device".
> Review seems done, but it has missed libvirt 4.5 which was released today.

Just posted latest version here:

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

It will be in the next release on ~ Aug 1st

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[libvirt] [PATCH v4] qemu: format serial and geometry on frontend disk device

[Daniel P . Berrangé]

Currently we format the serial, geometry and error policy on the -drive
backend argument.

QEMU added the ability to set serial and geometry on the frontend in
the 1.2 release deprecating use of -drive, with support being deleted
from -drive in 3.0.

We keep formatting error policy on -drive for now, because we don't
ahve support for that with -device for usb-storage just yet.

Note that some disk buses (sd) still don't support -device. Although
QEMU allowed these properties to be set on -drive for if=sd, they
have been ignored so we now report an error in this case.

Signed-off-by: Daniel P. Berrangé 
---

Changed in v4:

 - Move error checks to post parse validator

 src/qemu/qemu_command.c   | 13 -
 src/qemu/qemu_domain.c| 27 +++
 .../disk-drive-network-tlsx509.args   | 12 -
 .../disk-drive-network-vxhs.args  |  4 +--
 tests/qemuxml2argvdata/disk-drive-shared.args |  5 ++--
 tests/qemuxml2argvdata/disk-geometry.args |  6 ++---
 tests/qemuxml2argvdata/disk-ide-wwn.args  |  5 ++--
 .../qemuxml2argvdata/disk-scsi-disk-wwn.args  |  4 +--
 tests/qemuxml2argvdata/disk-serial.args   | 10 +++
 tests/qemuxml2argvdata/disk-serial.xml|  1 -
 tests/qemuxml2xmloutdata/disk-serial.xml  |  1 -
 11 files changed, 57 insertions(+), 31 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 70acbde80a..04c5c28438 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1613,7 +1613,7 @@ qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr disk,
   disk->geometry.sectors);
 
 if (disk->geometry.trans != VIR_DOMAIN_DISK_TRANS_DEFAULT)
-virBufferAsprintf(buf, ",trans=%s",
+virBufferAsprintf(buf, ",bios-chs-trans=%s",
   
virDomainDiskGeometryTransTypeToString(disk->geometry.trans));
 }
 
@@ -1621,8 +1621,6 @@ qemuBuildDiskFrontendAttributes(virDomainDiskDefPtr disk,
 virBufferAddLit(buf, ",serial=");
 virBufferEscape(buf, '\\', " ", "%s", disk->serial);
 }
-
-qemuBuildDiskFrontendAttributeErrorPolicy(disk, buf);
 }
 
 
@@ -1664,9 +1662,10 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
 virBufferAsprintf(, ",index=%d", idx);
 }
 
-/* Format attributes for the drive itself (not the storage backing it) 
which
- * we've formatted historically with -drive */
-qemuBuildDiskFrontendAttributes(disk, );
+/* werror/rerror are really frontend attributes, but older
+ * qemu requires them on -drive instead of -device */
+qemuBuildDiskFrontendAttributeErrorPolicy(disk, );
+
 
 /* While this is a frontend attribute, it only makes sense to be used when
  * legacy -drive is used. In modern qemu the 'ide-cd' or 'scsi-cd' are 
used.
@@ -2125,6 +2124,8 @@ qemuBuildDriveDevStr(const virDomainDef *def,
 if (qemuBuildDriveDevCacheStr(disk, , qemuCaps) < 0)
 goto error;
 
+qemuBuildDiskFrontendAttributes(disk, );
+
 if (virBufferCheckError() < 0)
 goto error;
 
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index e671656121..a60bca29ca 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4657,6 +4657,33 @@ qemuDomainDeviceDefValidateDisk(const virDomainDiskDef 
*disk,
 }
 }
 
+if (disk->geometry.cylinders > 0 &&
+disk->geometry.heads > 0 &&
+disk->geometry.sectors > 0) {
+if (disk->bus == VIR_DOMAIN_DISK_BUS_USB ||
+disk->bus == VIR_DOMAIN_DISK_BUS_SD) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("CHS geometry can not be set for '%s' bus"),
+   virDomainDiskBusTypeToString(disk->bus));
+return -1;
+}
+
+if (disk->geometry.trans != VIR_DOMAIN_DISK_TRANS_DEFAULT &&
+disk->bus != VIR_DOMAIN_DISK_BUS_IDE) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("CHS translation mode can only be set for 'ide' 
bus not '%s'"),
+   virDomainDiskBusTypeToString(disk->bus));
+return -1;
+}
+}
+
+if (disk->serial && disk->bus == VIR_DOMAIN_DISK_BUS_SD) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Serial property not supported for drive bus '%s'"),
+   virDomainDiskBusTypeToString(disk->bus));
+return -1;
+}
+
 if (driverName && STRNEQ(driverName, "qemu")) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("unsupported driver name '%s' for disk '%s'"),
diff --git a/tests/qemuxml2argvdata/disk-drive-network-tlsx509.args 
b/tests/qemuxml2argvdata/disk-drive-network-tlsx509.args
index e25f45742c..b5e73064c0 100644
--- a/tests/qemuxml2argvdata/disk-drive-network-tlsx509.args
+++ b/tests/qemuxml2argvdata/disk-drive-network-tlsx509.args
@@ -28,22 +28,22 @@ 

Re: [libvirt] [Qemu-devel] [PULL 25/26] block: Remove deprecated -drive option serial

[Christian Borntraeger]

On 07/02/2018 10:04 AM, Kevin Wolf wrote:
> Am 25.06.2018 um 13:45 hat Peter Krempa geschrieben:
>> On Mon, Jun 25, 2018 at 13:41:06 +0200, Kevin Wolf wrote:
>>> Am 25.06.2018 um 11:53 hat Daniel P. Berrangé geschrieben:
 On Fri, Jun 22, 2018 at 03:31:46PM +0100, Daniel P. Berrangé wrote:
> On Fri, Jun 22, 2018 at 04:25:13PM +0200, Kevin Wolf wrote:
>> Am 22.06.2018 um 15:36 hat Christian Borntraeger geschrieben:
>>
>> [...]
>>
>>>
>>> Thanks!
>>>
>>> I'll look into werror/rerror support for usb-storage. It shouldn't be
>>> too hard, though it's strictly speaking a separate problem related to
>>> using -blockdev rather than option deprecation.
>>>
>>> If Peter wants to wait for QEMU support before converting werror/rerror
>>
>> Definitely. I don't want to keep around yet another hack that will
>> satisfy one specific case and then add another capability for it. We
>> should then gate the moving of the feature based on the presence of
>> werror for usb-storage.
>>
>>> to -device, maybe it would make sense to split your patch for v2 so that
>>> geometry and serial can get fixed right away?
>>
>> Yes this can be done right away.
> 
> Has serial/gemoetry been fixed meanwhile and will it make it into the
> next release?

I cannot find an archive that has it, but it is on the libvirt mailing
list as "[libvirt] [PATCH v3] qemu: format serial and geometry on frontend disk 
device".
Review seems done, but it has missed libvirt 4.5 which was released today. 

Christian

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

Re: [libvirt] [PATCH 1/2] virTypedParamsSerialize: minor fixes

[Marc Hartmayer]

On Thu, Jun 21, 2018 at 03:47 PM +0200, Marc Hartmayer  
wrote:
> On Wed, Jun 13, 2018 at 03:11 PM +0200, John Ferlan  
> wrote:
>> On 06/07/2018 08:17 AM, Marc Hartmayer wrote:
>>> On Wed, May 09, 2018 at 09:51 PM +0200, John Ferlan  
>>> wrote:
 On 05/07/2018 11:24 AM, Marc Hartmayer wrote:
> On Mon, Apr 30, 2018 at 03:20 PM +0200, John Ferlan  
> wrote:
>> On 04/25/2018 11:55 AM, Marc Hartmayer wrote:
>>> 1. Don't allocate if there is nothing that needs to be
>>>allocated. Especially as the result of calling calloc(0, ...) is
>>>implementation-defined.

 uh oh - another memory recollection challenge ;-)

>>
>> Following VIR_ALLOC_N one finds :
>>
>> VIR_ALLOC_N(params_val, nparams)
>>
>> which equates to
>>
>> # define VIR_ALLOC_N(ptr, count) \
>>  virAllocN(&(ptr), sizeof(*(ptr)), (count), true, \
>>VIR_FROM_THIS, __FILE__, __FUNCTION__, __LINE__)
>>
>> or
>>
>> virAllocN(_val, sizeof(params_val), nparams, true, ...)
>>
>> and eventually/essentially
>>
>> *params_val = calloc(sizeof(params_val), nparams)
>>
>> If the returned value is NULL then we error w/ OOM (4th param=true).
>>
>> So, unless @params_val had no elements, it won't be calloc(0,...) and
>
> I'm talking about the case that nparams == 0 => calloc(sizeof(…), 0).
>

 And I was thinking is there a specific consumer/caller of
 virTypedParamsSerialize that was passing something incorrectly.

>> thus the question becomes is there a more specific path you are
>> referencing here?
>
> It’s a “generic” serializer so it should work for every (valid)
> case. What happens, for example, if params == NULL and nparams == 0? In
> that case I would expect *remote_params_val = NULL and
> *remote_params_len = 0.
>

 Going through the callers to virTypedParamsSerialize can/does anyone
 pass params == NULL and nparams == 0?  Would that be reasonable and/or
 expected?

 My look didn't find any - either some caller checks that the passed
 @params != NULL (usually via virCheckNonNullArgGoto in the external API
 call) or @params is built up on the fly and wouldn't be NULL because
 there'd be an error causing failure beforehand. Although I'll admit the
 migration ones are also crazy to look at.

 I could have made a mistake too; hence, the is there a specific instance
 that I missed? Or perhaps this is a result of some branched/private code
 that had that error which I don't have access to?
>>>
>>> It was the result of private code but actually it was intended and no
>>> error :)
>>>

 To answer your "What happens, for example, if params == NULL and nparams
 == 0?", well supposedly "VIR_ALLOC_N(params_val, 0)" should return NULL,
 so params_val and thus *remote_params_val == NULL.
>>>
>>> Did you try what 'VIR_ALLOC_N(params_val, 0)' actually returns? At least
>>> for me, it doesn’t return a NULL pointer.
>>>
>>
>> I think I already determined that NULL isn't returned; otherwise, we
>> would have failed w/ OOM. I do recall trying this and seeing a non NULL
>> value returned.
>>
>> IIRC: I found no way into [de]serialize w/ a 0 parameter, so having that
>> guard didn't seem necessary,
>
> I used the libvirt-python APIs for some testing. I called an (own) API
> with 'None' as argument and this resulted in 0 parameters.
>
>> but it's been 2 months since I looked at
>> this and that level of detail has long been flushed out of main memory
>> cache. ;-)
>>
>>> The man page for calloc-posix reads:
>>>
>>> “If either nelem or elsize is 0, then either a null pointer or a unique
>>> pointer value that can be successfully passed to free() shall be
>>> returned.” [1]
>>>
>>> [1] https://www.unix.com/man-page/POSIX/3posix/calloc/
>>>
>>
>> perhaps then we avoid this conundrum and take Daniel's advice in his
>> response:
>>
>>"If there is any problem with this, then we should just change
>> bootstrap.conf to use calloc-gnu instead of calloc-posix, which
>> basically turns calloc(0) into calloc(1) for compat with glibc
>> behaviour."
>
> This would still require this patch, no? At least if we agree that the
> following example should work:

Okay, the example wouldn’t even compile… but I hope the overall message
is clear :)

>
> virTypedParameterPtr params;
> int nparams;
>
> virTypedParamsDeserialize(NULL, 0, , );
> assert(params == NULL && assert nparams == 0);
>
> Do we?
>

Polite ping.

[…ping]

--
Beste Grüße / Kind regards
   Marc Hartmayer

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


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

Re: [libvirt] [PATCH 2/3] esx: Add esxVI_checkArgList macro

[Marcos Paulo de Souza]

On Tue, Jul 03, 2018 at 11:31:36AM +0200, Michal Prívozník wrote:
> On 07/03/2018 04:20 AM, Marcos Paulo de Souza wrote:
> > This macro avoids code duplication when checking for arrays of objects.
> > 
> > Signed-off-by: Marcos Paulo de Souza 
> > ---
> >  src/esx/esx_vi.c | 189 ---
> >  1 file changed, 46 insertions(+), 143 deletions(-)
> > 
> > diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
> > index 25fbdc7e44..212300dbff 100644
> > --- a/src/esx/esx_vi.c
> > +++ b/src/esx/esx_vi.c
> > @@ -41,6 +41,16 @@
> >  
> >  VIR_LOG_INIT("esx.esx_vi");
> >  
> > +#define esxVI_checkArgList(val) \
> > +do { \
> > +if (!val || *val) { \
> > +virReportError(VIR_ERR_INVALID_ARG, "%s", _("Invalid 
> > argument")); \
> 
> Actually, this is one of the few places where VIR_ERR_INTERNAL_ERROR is
> not misused and makes sense. The only way how this error can be reported
> is if there's a bug in our code, not because of some input user made.
> 
> There are also other occurrences of this pattern. coccinelle helped to
> find other. I used the following spatch:
> 
> @@
> identifier ptr;
> @@
> 
> - (!ptr || *ptr)
> + (esxVI_checkArgList(ptr))
> 
> 
> Mind putting them here too? Otherwise the patch looks good.

No problem. I will take a look into coccinele and resend the patch once all
other places are covered. Thanks for pushing the other patches and fixing the
code style.

> 
> Michal

-- 
Thanks,
Marcos

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

Re: [libvirt] [PATCH 3/3] esx_driver: Use virCheckFlag macro

[Michal Prívozník]

On 07/03/2018 04:21 AM, Marcos Paulo de Souza wrote:
> Instead of duplicating code to do the same checking. Now all functions
> of virHypervisorDriver from esx driver are using this macro.
> 
> Signed-off-by: Marcos Paulo de Souza 
> ---
>  src/esx/esx_driver.c | 10 ++
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
> index 60aa5fc252..705b0d1a41 100644
> --- a/src/esx/esx_driver.c
> +++ b/src/esx/esx_driver.c
> @@ -2488,10 +2488,7 @@ esxDomainSetVcpusFlags(virDomainPtr domain, unsigned 
> int nvcpus,
>  esxVI_TaskInfoState taskInfoState;
>  char *taskInfoErrorMessage = NULL;
>  
> -if (flags != VIR_DOMAIN_AFFECT_LIVE) {
> -virReportError(VIR_ERR_INVALID_ARG, _("unsupported flags: (0x%x)"), 
> flags);
> -return -1;
> -}
> +virCheckFlags(VIR_DOMAIN_AFFECT_LIVE, -1);
>  
>  if (nvcpus < 1) {
>  virReportError(VIR_ERR_INVALID_ARG, "%s",
> @@ -2570,10 +2567,7 @@ esxDomainGetVcpusFlags(virDomainPtr domain, unsigned 
> int flags)
>  esxVI_ObjectContent *hostSystem = NULL;
>  esxVI_DynamicProperty *dynamicProperty = NULL;
>  
> -if (flags != (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_VCPU_MAXIMUM)) {
> -virReportError(VIR_ERR_INVALID_ARG, _("unsupported flags: (0x%x)"), 
> flags);
> -return -1;
> -}
> +virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_VCPU_MAXIMUM, -1);

We tend to put one flag per line as it helps us size down changes needed
when adding new flag (= patches are smaller).

Fixed, ACKed and pushed.

Michal

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

Re: [libvirt] [PATCH 2/3] esx: Add esxVI_checkArgList macro

[Michal Prívozník]

On 07/03/2018 04:20 AM, Marcos Paulo de Souza wrote:
> This macro avoids code duplication when checking for arrays of objects.
> 
> Signed-off-by: Marcos Paulo de Souza 
> ---
>  src/esx/esx_vi.c | 189 ---
>  1 file changed, 46 insertions(+), 143 deletions(-)
> 
> diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
> index 25fbdc7e44..212300dbff 100644
> --- a/src/esx/esx_vi.c
> +++ b/src/esx/esx_vi.c
> @@ -41,6 +41,16 @@
>  
>  VIR_LOG_INIT("esx.esx_vi");
>  
> +#define esxVI_checkArgList(val) \
> +do { \
> +if (!val || *val) { \
> +virReportError(VIR_ERR_INVALID_ARG, "%s", _("Invalid 
> argument")); \

Actually, this is one of the few places where VIR_ERR_INTERNAL_ERROR is
not misused and makes sense. The only way how this error can be reported
is if there's a bug in our code, not because of some input user made.

There are also other occurrences of this pattern. coccinelle helped to
find other. I used the following spatch:

@@
identifier ptr;
@@

- (!ptr || *ptr)
+ (esxVI_checkArgList(ptr))


Mind putting them here too? Otherwise the patch looks good.

Michal

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

Re: [libvirt] [PATCH 1/3] esx_vi.c: Simplify error handling in MachineByName

[Michal Prívozník]

On 07/03/2018 04:20 AM, Marcos Paulo de Souza wrote:
> The same pattern is used in lots of other places.
> 
> Signed-off-by: Marcos Paulo de Souza 
> ---
>  src/esx/esx_vi.c | 14 --
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
> index 43ff7ea048..25fbdc7e44 100644
> --- a/src/esx/esx_vi.c
> +++ b/src/esx/esx_vi.c
> @@ -3014,16 +3014,10 @@ esxVI_LookupVirtualMachineByName(esxVI_Context *ctx, 
> const char *name,
>  break;
>  }
>  
> -if (!(*virtualMachine)) {
> -if (occurrence == esxVI_Occurrence_OptionalItem) {
> -result = 0;
> -
> -goto cleanup;
> -} else {
> -virReportError(VIR_ERR_NO_DOMAIN,
> -   _("Could not find domain with name '%s'"), name);
> -goto cleanup;
> -}
> +if (!(*virtualMachine) && occurrence != esxVI_Occurrence_OptionalItem) {
> +virReportError(VIR_ERR_NO_DOMAIN,
> +   _("Could not find domain with name '%s'"), name);
> +goto cleanup;
>  }

I think this error message should be dropped. Firstly, this function is
called from esxDomainDefineXMLFlags() where it is used to check if a
domain with the same name as in provided XML does not exist. If it
doesn't this function reports an error which is undesired.

Secondly, every caller checks for virtualMachine == NULL and reports
their own error effectively overwriting this one.

Thirdly, this function is supposed to act like
virDomainObjListFindByName() which doesn't report error either.


ACK with this squashed in:

diff --git i/src/esx/esx_vi.c w/src/esx/esx_vi.c
index 25fbdc7e44..0bdfc5a8be 100644
--- i/src/esx/esx_vi.c
+++ w/src/esx/esx_vi.c
@@ -3014,11 +3014,8 @@ esxVI_LookupVirtualMachineByName(esxVI_Context *ctx, 
const char *name,
 break;
 }
 
-if (!(*virtualMachine) && occurrence != esxVI_Occurrence_OptionalItem) {
-virReportError(VIR_ERR_NO_DOMAIN,
-   _("Could not find domain with name '%s'"), name);
+if (!(*virtualMachine) && occurrence != esxVI_Occurrence_OptionalItem)
 goto cleanup;
-}
 
 result = 0;


And pushed.

Michal

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

[libvirt] [PATCH 02/25] qemu_migration: Check for active domain after talking to remote daemon

[dubo163]

From: Jiri Denemark 

Once we called qemuDomainObjEnterRemote to talk to the destination
daemon during a peer to peer migration, the vm lock is released and we
only hold an async job. If the source domain dies at this point the
monitor EOF callback is allowed to do its job and (among other things)
clear all private data irrelevant for stopped domain. Thus when we call
qemuDomainObjExitRemote, the domain may already be gone and we should
avoid touching runtime private data (such as current job info).

In other words after acquiring the lock in qemuDomainObjExitRemote, we
need to check the domain is still alive. Unless we're doing offline
migration.

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

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_domain.c| 14 +++-
 src/qemu/qemu_domain.h|  5 +++--
 src/qemu/qemu_migration.c | 54 ++-
 3 files changed, 41 insertions(+), 32 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index afd572f..4c15d5a 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -7023,11 +7023,23 @@ void qemuDomainObjEnterRemote(virDomainObjPtr obj)
 virObjectUnlock(obj);
 }
 
-void qemuDomainObjExitRemote(virDomainObjPtr obj)
+
+int
+qemuDomainObjExitRemote(virDomainObjPtr obj,
+bool checkActive)
 {
 virObjectLock(obj);
 VIR_DEBUG("Exited remote (vm=%p name=%s)",
   obj, obj->def->name);
+
+if (checkActive && !virDomainObjIsActive(obj)) {
+virReportError(VIR_ERR_OPERATION_FAILED,
+   _("domain '%s' is not running"),
+   obj->def->name);
+return -1;
+}
+
+return 0;
 }
 
 
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 0a9af75..30d186a 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -584,8 +584,9 @@ void qemuDomainObjExitAgent(virDomainObjPtr obj, 
qemuAgentPtr agent)
 
 void qemuDomainObjEnterRemote(virDomainObjPtr obj)
 ATTRIBUTE_NONNULL(1);
-void qemuDomainObjExitRemote(virDomainObjPtr obj)
-ATTRIBUTE_NONNULL(1);
+int qemuDomainObjExitRemote(virDomainObjPtr obj,
+bool checkActive)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
 
 virDomainDefPtr qemuDomainDefCopy(virQEMUDriverPtr driver,
   virDomainDefPtr src,
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index c9aaa38..435cd17 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3920,27 +3920,20 @@ qemuMigrationSrcPerformPeer2Peer2(virQEMUDriverPtr 
driver,
 qemuDomainObjEnterRemote(vm);
 ret = dconn->driver->domainMigratePrepareTunnel
 (dconn, st, destflags, dname, resource, dom_xml);
-qemuDomainObjExitRemote(vm);
+if (qemuDomainObjExitRemote(vm, true) < 0)
+goto cleanup;
 } else {
 qemuDomainObjEnterRemote(vm);
 ret = dconn->driver->domainMigratePrepare2
 (dconn, , , NULL, _out,
  destflags, dname, resource, dom_xml);
-qemuDomainObjExitRemote(vm);
+if (qemuDomainObjExitRemote(vm, true) < 0)
+goto cleanup;
 }
 VIR_FREE(dom_xml);
 if (ret == -1)
 goto cleanup;
 
-/* the domain may have shutdown or crashed while we had the locks dropped
- * in qemuDomainObjEnterRemote, so check again
- */
-if (!virDomainObjIsActive(vm)) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("guest unexpectedly quit"));
-goto cleanup;
-}
-
 if (!(flags & VIR_MIGRATE_TUNNELLED) &&
 (uri_out == NULL)) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -3987,7 +3980,8 @@ qemuMigrationSrcPerformPeer2Peer2(virQEMUDriverPtr driver,
 ddomain = dconn->driver->domainMigrateFinish2
 (dconn, dname, cookie, cookielen,
  uri_out ? uri_out : dconnuri, destflags, cancelled);
-qemuDomainObjExitRemote(vm);
+/* The domain is already gone at this point */
+ignore_value(qemuDomainObjExitRemote(vm, false));
 if (cancelled && ddomain)
 VIR_ERROR(_("finish step ignored that migration was cancelled"));
 
@@ -4052,6 +4046,7 @@ qemuMigrationSrcPerformPeer2Peer3(virQEMUDriverPtr driver,
 int nparams = 0;
 int maxparams = 0;
 size_t i;
+bool offline = !!(flags & VIR_MIGRATE_OFFLINE);
 
 VIR_DEBUG("driver=%p, sconn=%p, dconn=%p, dconnuri=%s, vm=%p, xmlin=%s, "
   "dname=%s, uri=%s, graphicsuri=%s, listenAddress=%s, "
@@ -4145,7 +4140,8 @@ qemuMigrationSrcPerformPeer2Peer3(virQEMUDriverPtr driver,
 (dconn, st, cookiein, cookieinlen, , ,
  destflags, dname, bandwidth, dom_xml);
 }
-qemuDomainObjExitRemote(vm);
+if (qemuDomainObjExitRemote(vm, !offline) < 0)
+goto cleanup;
 } else {
 qemuDomainObjEnterRemote(vm);
 if (useParams) {
@@ 

[libvirt] [PATCH 03/25] news: Update for 4.5.0 release

[dubo163]

From: Andrea Bolognani 

Signed-off-by: Andrea Bolognani 
---
 docs/news.xml | 81 +++
 1 file changed, 81 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml
index 468d340..7348838 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -61,6 +61,16 @@
   Support specifying extended TSEG size for SMM in QEMU.
 
   
+  
+
+  qemu: Add support for SEV guests
+
+
+  SEV (Secure Encrypted Virtualization) is a feature available on AMD
+  CPUs that encrypts the guest memory and makes it inaccessible even
+  to the host OS.
+
+  
 
 
   
@@ -76,6 +86,24 @@
   secret objects, but that support was never added to libvirt.
 
   
+  
+
+  Make GnuTLS mandatory
+
+
+  Building without GnuTLS is no longer possible.
+
+  
+  
+
+  qemu: Remove allow_disk_format_probing configuration option
+
+
+  The option represented a security risk when used with malicious
+  disk images, so users were recommended against enabling it; with
+  this release, it's been removed altogether.
+
+  
 
 
   
@@ -130,6 +158,39 @@
   or virStorageVolCreateXMLFrom.
 
   
+  
+
+  qemu: Add support for vsock hot (un)plug and cold (un)plug
+
+  
+  
+
+  qemu: Add support for NBD over TLS
+
+
+  NBD volumes can now be accessed securely.
+
+  
+  
+
+  qemu: Implement FD passing for Unix sockets
+
+
+  Instead of having QEMU open the socket and then connecting to it,
+  which is inherently racy, starting with QEMU 2.12 we can open the
+  socket ourselves and pass it to QEMU, avoiding race conditions.
+
+  
+  
+
+  virsh: Introduce --nowait option for domstat command
+
+
+  When this option is specified, virsh will try to fetch the guest
+  stats but abort instead of stalling if they can't be retrieved right
+  away.
+
+  
 
 
   
@@ -143,6 +204,26 @@
   us getting to the worker pool initialization.
 
   
+  
+
+  qemu: Fix domain resume after failed migration
+
+
+  Recent versions of QEMU activate block devices before the guest CPU
+  has been started, which makes it impossible to roll back a failed
+  migration. Use the late-block-activate migration
+  capability if supported to avoid the issue.
+
+  
+  
+
+  vmx: Permit guests to have an odd number of vCPUs
+
+
+  An odd number of vCPUs greater than 1 was forbidden in the past,
+  but current versions of ESXi have lifted that restriction.
+
+  
 
   
   
-- 
1.8.3.1


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

[libvirt] [PATCH 01/25] qemu_migration: Rename 'offline' variable in SrcPerformPeer2Peer

[dubo163]

From: Jiri Denemark 

The variable is used to store the offline migration capability of the
destination daemon. Let's call it 'dstOffline' so that we can later use
'offline' to indicate whether we were asked to do offline migration.

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_migration.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 9107660..c9aaa38 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -4399,7 +4399,7 @@ qemuMigrationSrcPerformPeer2Peer(virQEMUDriverPtr driver,
 virConnectPtr dconn = NULL;
 bool p2p;
 virErrorPtr orig_err = NULL;
-bool offline = false;
+bool dstOffline = false;
 virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 bool useParams;
 
@@ -4469,8 +4469,8 @@ qemuMigrationSrcPerformPeer2Peer(virQEMUDriverPtr driver,
 useParams = VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn,
  VIR_DRV_FEATURE_MIGRATION_PARAMS);
 if (flags & VIR_MIGRATE_OFFLINE)
-offline = VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn,
-   VIR_DRV_FEATURE_MIGRATION_OFFLINE);
+dstOffline = VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn,
+  
VIR_DRV_FEATURE_MIGRATION_OFFLINE);
 qemuDomainObjExitRemote(vm);
 
 if (!p2p) {
@@ -4488,7 +4488,7 @@ qemuMigrationSrcPerformPeer2Peer(virQEMUDriverPtr driver,
 goto cleanup;
 }
 
-if (flags & VIR_MIGRATE_OFFLINE && !offline) {
+if (flags & VIR_MIGRATE_OFFLINE && !dstOffline) {
 virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
_("offline migration is not supported by "
  "the destination host"));
-- 
1.8.3.1


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

[libvirt] [PATCH 04/25] Release of libvirt-4.5.0

[dubo163]

From: Daniel Veillard 

- docs/news.xml: updated for the release

Signed-off-by: Daniel Veillard 
---
 docs/news.xml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/news.xml b/docs/news.xml
index 7348838..33824dd 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -33,7 +33,7 @@
  -->
 
 
-  
+  
 
   
 
-- 
1.8.3.1


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

[libvirt] [PATCH 08/25] lxc: Remove FORCE flag from lxcDomainUpdateDeviceFlags

[dubo163]

From: John Ferlan 

Force would be used to force eject a cdrom live, since the code
doesn't support live update, remove the flag.

Signed-off-by: John Ferlan 
ACKed-by: Michal Prívozník 
---
 src/lxc/lxc_driver.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index f9794e0..aea48e5 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -4832,8 +4832,7 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom,
 virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver);
 
 virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
-  VIR_DOMAIN_AFFECT_CONFIG |
-  VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1);
+  VIR_DOMAIN_AFFECT_CONFIG, -1);
 
 if (!(vm = lxcDomObjFromDomain(dom)))
 goto cleanup;
-- 
1.8.3.1


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

[libvirt] [PATCH 09/25] lxc: Rearrange order in lxcDomainUpdateDeviceFlags

[dubo163]

From: John Ferlan 

Although commit e3497f3f noted that the LIVE option doesn't
matter and removed the call to virDomainDefCompatibleDevice,
it didn't go quite far enough and change the order of the checks
and rework the code to just handle the config change causing
a failure after virDomainObjUpdateModificationImpact updates
the @flags. Since we only support config a lot of previously
conditional code is now just inlined.

Signed-off-by: John Ferlan 
ACKed-by: Michal Prívozník 
---
 src/lxc/lxc_driver.c | 63 ++--
 1 file changed, 21 insertions(+), 42 deletions(-)

diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index aea48e5..1fc6c6a 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -4827,7 +4827,7 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom,
 virCapsPtr caps = NULL;
 virDomainObjPtr vm = NULL;
 virDomainDefPtr vmdef = NULL;
-virDomainDeviceDefPtr dev = NULL, dev_copy = NULL;
+virDomainDeviceDefPtr dev = NULL;
 int ret = -1;
 virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver);
 
@@ -4846,61 +4846,40 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom,
 if (virDomainObjUpdateModificationImpact(vm, ) < 0)
 goto endjob;
 
-if (!(caps = virLXCDriverGetCapabilities(driver, false)))
+if (flags & VIR_DOMAIN_AFFECT_LIVE) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+   _("Unable to modify live devices"));
 goto endjob;
+}
 
-dev = dev_copy = virDomainDeviceDefParse(xml, vm->def,
- caps, driver->xmlopt,
- VIR_DOMAIN_DEF_PARSE_INACTIVE);
-if (dev == NULL)
+if (!(caps = virLXCDriverGetCapabilities(driver, false)))
 goto endjob;
 
-if (flags & VIR_DOMAIN_AFFECT_CONFIG &&
-flags & VIR_DOMAIN_AFFECT_LIVE) {
-/* If we are affecting both CONFIG and LIVE
- * create a deep copy of device as adding
- * to CONFIG takes one instance.
- */
-dev_copy = virDomainDeviceDefCopy(dev, vm->def,
-  caps, driver->xmlopt);
-if (!dev_copy)
-goto endjob;
-}
-
-if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
-/* Make a copy for updated domain. */
-vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt);
-if (!vmdef)
-goto endjob;
+if (!(dev = virDomainDeviceDefParse(xml, vm->def, caps, driver->xmlopt,
+VIR_DOMAIN_DEF_PARSE_INACTIVE)))
+goto endjob;
 
-/* virDomainDefCompatibleDevice call is delayed until we know the
- * device we're going to update. */
-if ((ret = lxcDomainUpdateDeviceConfig(vmdef, dev)) < 0)
-goto endjob;
-}
+/* Make a copy for updated domain. */
+if (!(vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt)))
+goto endjob;
 
-if (flags & VIR_DOMAIN_AFFECT_LIVE) {
-virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
-   _("Unable to modify live devices"));
+/* virDomainDefCompatibleDevice call is delayed until we know the
+ * device we're going to update. */
+if (lxcDomainUpdateDeviceConfig(vmdef, dev) < 0)
+goto endjob;
 
+if (virDomainSaveConfig(cfg->configDir, driver->caps, vmdef) < 0)
 goto endjob;
-}
 
-/* Finally, if no error until here, we can save config. */
-if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
-ret = virDomainSaveConfig(cfg->configDir, driver->caps, vmdef);
-if (!ret) {
-virDomainObjAssignDef(vm, vmdef, false, NULL);
-vmdef = NULL;
-}
-}
+virDomainObjAssignDef(vm, vmdef, false, NULL);
+vmdef = NULL;
+ret = 0;
+
  endjob:
 virLXCDomainObjEndJob(driver, vm);
 
  cleanup:
 virDomainDefFree(vmdef);
-if (dev != dev_copy)
-virDomainDeviceDefFree(dev_copy);
 virDomainDeviceDefFree(dev);
 virDomainObjEndAPI();
 virObjectUnref(caps);
-- 
1.8.3.1


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

[libvirt] [PATCH 10/25] util: new function virNetDevOpenvswitchInterfaceGetMaster()

[dubo163]

From: Laine Stump 

This function retrieves the name of the OVS bridge that the given
netdev is attached to. This separate function is necessary because OVS
set the IFLA_MASTER attribute to "ovs-system" for all netdevs that are
attached to an OVS bridge, so the standard method of retrieving the
master can't be used.

Signed-off-by: Laine Stump 
ACKed-by: Michal Privoznik 
---
 src/libvirt_private.syms|  1 +
 src/util/virnetdevopenvswitch.c | 56 +
 src/util/virnetdevopenvswitch.h |  6 +
 3 files changed, 63 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index b87e813..ffe5dfd 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2389,6 +2389,7 @@ virNetDevMidonetUnbindPort;
 virNetDevOpenvswitchAddPort;
 virNetDevOpenvswitchGetMigrateData;
 virNetDevOpenvswitchGetVhostuserIfname;
+virNetDevOpenvswitchInterfaceGetMaster;
 virNetDevOpenvswitchInterfaceStats;
 virNetDevOpenvswitchRemovePort;
 virNetDevOpenvswitchSetMigrateData;
diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
index f86f698..d1c5cf4 100644
--- a/src/util/virnetdevopenvswitch.c
+++ b/src/util/virnetdevopenvswitch.c
@@ -404,6 +404,62 @@ virNetDevOpenvswitchInterfaceStats(const char *ifname,
 return ret;
 }
 
+
+/**
+ * virNetDeOpenvswitchGetMaster:
+ * @ifname: name of interface we're interested in
+ * @master: used to return a string containing the name of @ifname's "master"
+ *  (this is the bridge or bond device that this device is attached to)
+ *
+ * Returns 0 on success, -1 on failure (if @ifname has no master
+ * @master will be NULL, but return value will still be 0 (success)).
+ *
+ * NB: This function is needed because the IFLA_MASTER attribute of an
+ * interface in a netlink dump (see virNetDevGetMaster()) will always
+ * return "ovs-system" for any interface that is attached to an OVS
+ * switch. When that happens, virNetDevOpenvswitchInterfaceGetMaster()
+ * must be called to get the "real" master of the interface.
+ */
+int
+virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char **master)
+{
+virCommandPtr cmd = NULL;
+int ret = -1;
+int exitstatus;
+
+*master = NULL;
+
+cmd = virCommandNew(OVSVSCTL);
+virNetDevOpenvswitchAddTimeout(cmd);
+virCommandAddArgList(cmd, "iface-to-br", ifname, NULL);
+virCommandSetOutputBuffer(cmd, master);
+
+if (virCommandRun(cmd, ) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Unable to run command to get OVS master for "
+ "interface %s"), ifname);
+goto cleanup;
+}
+
+/* non-0 exit code just means that the interface has no master in OVS */
+if (exitstatus != 0)
+VIR_FREE(*master);
+
+if (*master) {
+/* truncate at the first newline */
+char *nl = strchr(*master, '\n');
+if (nl)
+*nl = '\0';
+}
+
+VIR_DEBUG("OVS master for %s is %s", ifname, *master ? *master : "(none)");
+
+ret = 0;
+ cleanup:
+return ret;
+}
+
+
 /**
  * virNetDevOpenvswitchVhostuserGetIfname:
  * @path: the path of the unix socket
diff --git a/src/util/virnetdevopenvswitch.h b/src/util/virnetdevopenvswitch.h
index 6f6e620..331e483 100644
--- a/src/util/virnetdevopenvswitch.h
+++ b/src/util/virnetdevopenvswitch.h
@@ -47,6 +47,9 @@ int virNetDevOpenvswitchAddPort(const char *brname,
 int virNetDevOpenvswitchRemovePort(const char *brname, const char *ifname)
 ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
 
+int virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char **master)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
+
 int virNetDevOpenvswitchGetMigrateData(char **migrate, const char *ifname)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
 
@@ -57,6 +60,9 @@ int virNetDevOpenvswitchInterfaceStats(const char *ifname,
virDomainInterfaceStatsPtr stats)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
 
+int virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char **master)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
+
 int virNetDevOpenvswitchGetVhostuserIfname(const char *path,
char **ifname)
 ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NOINLINE;
-- 
1.8.3.1


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

[libvirt] [PATCH 07/25] domain_addr: delete virDomainVirtioSerialAddrRelease

[dubo163]

From: Anya Harter 

the last use of this function was deleted in commit
19a148b7c8353d5c214bed699f8fe983317baf93

Signed-off-by: Anya Harter 
---
 src/conf/domain_addr.c   | 44 
 src/conf/domain_addr.h   |  5 -
 src/libvirt_private.syms |  1 -
 3 files changed, 50 deletions(-)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 5589935..fd66a74 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -1607,50 +1607,6 @@ virDomainVirtioSerialAddrReserve(virDomainDefPtr def 
ATTRIBUTE_UNUSED,
 return ret;
 }
 
-/* virDomainVirtioSerialAddrRelease
- *
- * Release the virtio serial address of the device
- */
-int
-virDomainVirtioSerialAddrRelease(virDomainVirtioSerialAddrSetPtr addrs,
- virDomainDeviceInfoPtr info)
-{
-virBitmapPtr map;
-char *str = NULL;
-int ret = -1;
-ssize_t i;
-
-if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL ||
-info->addr.vioserial.port == 0)
-return 0;
-
-VIR_DEBUG("Releasing virtio serial %u %u", info->addr.vioserial.controller,
-  info->addr.vioserial.port);
-
-i = virDomainVirtioSerialAddrFindController(addrs, 
info->addr.vioserial.controller);
-if (i < 0) {
-virReportError(VIR_ERR_XML_ERROR,
-   _("virtio serial controller %u is missing"),
-   info->addr.vioserial.controller);
-goto cleanup;
-}
-
-map = addrs->controllers[i]->ports;
-if (virBitmapClearBit(map, info->addr.vioserial.port) < 0) {
-virReportError(VIR_ERR_XML_ERROR,
-   _("virtio serial controller %u does not have port %u"),
-   info->addr.vioserial.controller,
-   info->addr.vioserial.port);
-goto cleanup;
-}
-
-ret = 0;
-
- cleanup:
-VIR_FREE(str);
-return ret;
-}
-
 
 bool
 virDomainUSBAddressPortIsValid(unsigned int *port)
diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
index cbf04bf..2764070 100644
--- a/src/conf/domain_addr.h
+++ b/src/conf/domain_addr.h
@@ -258,11 +258,6 @@ virDomainVirtioSerialAddrReserve(virDomainDefPtr def,
  void *data)
 ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4);
 
-int
-virDomainVirtioSerialAddrRelease(virDomainVirtioSerialAddrSetPtr addrs,
- virDomainDeviceInfoPtr info)
-ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
-
 bool
 virDomainUSBAddressPortIsValid(unsigned int *port)
 ATTRIBUTE_NONNULL(1);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index cd2e428..b87e813 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -139,7 +139,6 @@ virDomainVirtioSerialAddrAssign;
 virDomainVirtioSerialAddrAutoAssign;
 virDomainVirtioSerialAddrAutoAssignFromCache;
 virDomainVirtioSerialAddrIsComplete;
-virDomainVirtioSerialAddrRelease;
 virDomainVirtioSerialAddrReserve;
 virDomainVirtioSerialAddrSetAddControllers;
 virDomainVirtioSerialAddrSetCreate;
-- 
1.8.3.1


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

[libvirt] [PATCH 05/25] Post-release version bump to 4.6.0

[dubo163]

From: John Ferlan 

Signed-off-by: John Ferlan 
---
 configure.ac  | 2 +-
 docs/news.xml | 8 
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index e25bf0a..59d2d09 100644
--- a/configure.ac
+++ b/configure.ac
@@ -16,7 +16,7 @@ dnl You should have received a copy of the GNU Lesser General 
Public
 dnl License along with this library.  If not, see
 dnl .
 
-AC_INIT([libvirt], [4.5.0], [libvir-list@redhat.com], [], 
[https://libvirt.org])
+AC_INIT([libvirt], [4.6.0], [libvir-list@redhat.com], [], 
[https://libvirt.org])
 AC_CONFIG_SRCDIR([src/libvirt.c])
 AC_CONFIG_AUX_DIR([build-aux])
 AC_CONFIG_HEADERS([config.h])
diff --git a/docs/news.xml b/docs/news.xml
index 33824dd..1ddfbd1 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -33,6 +33,14 @@
  -->
 
 
+  
+
+
+
+
+
+
+  
   
 
   
-- 
1.8.3.1


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

[libvirt] [PATCH 00/25] util:Fix with process number and pid file do not match

[dubo163]

Andrea Bolognani (5):
  news: Update for 4.5.0 release
  qemu: Add capability for the HTM pSeries feature
  conf: Parse and format the HTM pSeries feature
  qemu: Format the HTM pSeries feature
  news: Update for the HTM pSeries feature

Anya Harter (2):
  domain_addr: delete virDomainCCWAddressReleaseAddr
  domain_addr: delete virDomainVirtioSerialAddrRelease

Daniel Veillard (1):
  Release of libvirt-4.5.0

Jiri Denemark (2):
  qemu_migration: Rename 'offline' variable in SrcPerformPeer2Peer
  qemu_migration: Check for active domain after talking to remote daemon

John Ferlan (3):
  Post-release version bump to 4.6.0
  lxc: Remove FORCE flag from lxcDomainUpdateDeviceFlags
  lxc: Rearrange order in lxcDomainUpdateDeviceFlags

Julio Faracco (2):
  lxc: moving 'type' argument to avoid issues with mount() syscall.
  util: moving 'type' argument to avoid issues with mount() syscall.

Laine Stump (3):
  util: new function virNetDevOpenvswitchInterfaceGetMaster()
  util: add some debug log to virNetDevGetMaster
  network: properly check for taps that are connected to an OVS bridge

Marcos Paulo de Souza (1):
  esx_driver: Simplify IsEncrypted and IsSecure

Michal Privoznik (2):
  qemuDomainDeviceDefValidateNetwork: Check for range only if IP prefix
set
  virsh: Provide completer for detach-device-alias

Peter Krempa (3):
  qemu: domain: update only newly detected images in
qemuDomainDetermineDiskChain
  tests: qemumonitorjson: Add only required replies for blockstats test
  tests: qemumonitorjson: Fix name and call apropriate API

dubo163 (1):
  util:Fix with process number and pid file do not match

 configure.ac |   2 +-
 docs/formatdomain.html.in|   8 ++
 docs/news.xml| 100 ++-
 docs/schemas/domaincommon.rng|   5 ++
 src/conf/domain_addr.c   |  68 ---
 src/conf/domain_addr.h   |   8 --
 src/conf/domain_conf.c   |  19 +
 src/conf/domain_conf.h   |   1 +
 src/esx/esx_driver.c |  12 +--
 src/libvirt_private.syms |   3 +-
 src/lxc/lxc_container.c  |  26 +++---
 src/lxc/lxc_driver.c |  66 +--
 src/network/bridge_driver.c  |  21 -
 src/qemu/qemu_capabilities.c |   2 +
 src/qemu/qemu_capabilities.h |   1 +
 src/qemu/qemu_command.c  |  20 +
 src/qemu/qemu_domain.c   |  32 +++-
 src/qemu/qemu_domain.h   |   5 +-
 src/qemu/qemu_migration.c|  60 +++---
 src/util/vircgroup.c |   2 +-
 src/util/virnetdev.c |   1 +
 src/util/virnetdevopenvswitch.c  |  56 +
 src/util/virnetdevopenvswitch.h  |   6 ++
 src/util/virpidfile.c|   8 ++
 tests/qemucapabilitiesdata/caps_2.12.0.ppc64.xml |   1 +
 tests/qemucapabilitiesdata/caps_3.0.0.ppc64.xml  |   1 +
 tests/qemumonitorjsontest.c  |  19 ++---
 tests/qemuxml2argvdata/pseries-features.args |   2 +-
 tests/qemuxml2argvdata/pseries-features.xml  |   1 +
 tests/qemuxml2argvtest.c |   1 +
 tests/qemuxml2xmloutdata/pseries-features.xml|   1 +
 tests/qemuxml2xmltest.c  |   1 +
 tools/virsh-completer.c  |  48 +++
 tools/virsh-completer.h  |   4 +
 tools/virsh-domain.c |   1 +
 35 files changed, 414 insertions(+), 198 deletions(-)

-- 
1.8.3.1


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

Re: [libvirt] [PATCH] util:Fix with process number and pid file do not match

[Michal Prívozník]

On 07/02/2018 01:08 PM, dubo163 wrote:
> From: dubobo 
> 
> the libvirtd pid file is not match the os process pid number
> which is smaller than before.
> 
> this would be exist if the libvirtd process coredump or the os
> process was killed which the next pid number is smaller.
> 
> you can be also edit the pid file to write the longer number than
> before,then restart the libvirtd service.
> 
> Signed-off-by: dubobo 

I'm sorry, but this has to be your legal name, which I believe dubobo is
not. Also as I was pointed out earlier, the name of the author of the
patch has to be legal name.

> ---
>  src/util/virpidfile.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c
> index 58ab29f..8b0ff99 100644
> --- a/src/util/virpidfile.c
> +++ b/src/util/virpidfile.c
> @@ -445,6 +445,12 @@ int virPidFileAcquirePath(const char *path,
>  }
>  
>  snprintf(pidstr, sizeof(pidstr), "%lld", (long long) pid);
> +if (ftruncate(fd, 0) < 0) {
> +VIR_FORCE_CLOSE(fd);
> +return -1;

So if ftruncate() fails, caller sees -1 but no error message. This is
not nice because users then have no idea what went wrong. All they see
is a failed attempt to start libvirtd. We need virReportSystemError() here.

> +}
> +
> +lseek(fd, 0, SEEK_SET);

This is pretty useless. Since open() nothing was written to/read from
the pidfile. So we don't really need to seek in it.

Michal

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