Re: [libvirt] [RFC] Accepting PRs/MRs for libvirt on GitHub/GitLab

[Peter Krempa]

On Wed, Oct 16, 2019 at 17:08:20 +0100, Daniel Berrange wrote:
> On Wed, Oct 16, 2019 at 05:03:31PM +0200, Ján Tomko wrote:
> > On Wed, Oct 16, 2019 at 02:22:56PM +0200, Andrea Bolognani wrote:

[...]

> > Here it deviates from the usual mailing list workflow where the patch
> > has (in theory) a chance to be seen by all the developers.
> > 
> > But given that the requests will probably
> > a) be close to trivial
> > b) seen by a group of developers, not just one
> 
> I wouldn't expect the changes to be trivial. Current stuff
> is trivial largely because we tell people not to open merge
> requests. If we adopt use of web based review, then expect

I'd still want the message we'll put out to encourage them using e-mail.

> people to submit non-trivial patches. I would do so myself
> for example. Thus I think we must make a clean switchover
> from email to a single web based tool.

I disagree. There is nothing really appealing to me in any of the web
based frontends for git.

The user interface of them is designed to be flashy but that really
hurts usability of git. We get cool icons but in return we must pay with
always-online connection, loading bars if you click anywhere and the
general necessity to interact with the browser which requires a lot of
mousing around.

The commenting interface on individual patches is very poor given what
email allows you and in many cases it's hard to access older versions
after a pull-request is force-pushed.

Also we then get to the discussion whether to use the most popular web
based git hosting site, which is the most terrible to use of them all or
one of the less popular and less bad ones. Since this idea is sold as a
way to attract outside contributors, it might lead to sacrificing
usability for popularity ...

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

[libvirt] [PATCH v4 2/2] qemu: Generate 'xres' and 'yres' for QEMU video devices

[jcfaracco]

From: Julio Faracco 

This commit let QEMU command line define 'xres' and 'yres' properties
if XML contains both properties from video model: based on resolution
fields 'x' and 'y'. There is a conditional structure inside
qemuDomainDeviceDefValidateVideo() that validates if video model
supports this feature. This commit includes the necessary changes to
cover resolution for 'video-qxl-resolution' test cases too.

Signed-off-by: Julio Faracco 
---
 src/qemu/qemu_command.c   |  5 +
 src/qemu/qemu_domain.c| 11 +++
 tests/qemuxml2argvdata/video-qxl-resolution.args  |  2 +-
 tests/qemuxml2argvdata/video-qxl-resolution.xml   |  4 +++-
 tests/qemuxml2xmloutdata/video-qxl-resolution.xml |  4 +++-
 5 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index e98195b1d7..b579b994f3 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4598,6 +4598,11 @@ qemuBuildDeviceVideoStr(const virDomainDef *def,
 virBufferAsprintf(, ",vgamem=%uk", video->vram);
 }
 
+if (video->res && video->res->x && video->res->y) {
+/* QEMU accepts resolution xres and yres. */
+virBufferAsprintf(, ",xres=%u,yres=%u", video->res->x, 
video->res->y);
+}
+
 if (qemuBuildDeviceAddressStr(, def, >info, qemuCaps) < 0)
 return NULL;
 
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 09b6c9a570..a97bf65e7f 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5774,6 +5774,17 @@ qemuDomainDeviceDefValidateVideo(const virDomainVideoDef 
*video)
 }
 }
 
+if (video->type != VIR_DOMAIN_VIDEO_TYPE_VGA &&
+video->type != VIR_DOMAIN_VIDEO_TYPE_QXL &&
+video->type != VIR_DOMAIN_VIDEO_TYPE_VIRTIO &&
+video->type != VIR_DOMAIN_VIDEO_TYPE_BOCHS) {
+if (video->res) {
+virReportError(VIR_ERR_XML_ERROR, "%s",
+   _("model resolution is not supported"));
+return -1;
+}
+}
+
 if (video->type == VIR_DOMAIN_VIDEO_TYPE_VGA ||
 video->type == VIR_DOMAIN_VIDEO_TYPE_VMVGA) {
 if (video->vram && video->vram < 1024) {
diff --git a/tests/qemuxml2argvdata/video-qxl-resolution.args 
b/tests/qemuxml2argvdata/video-qxl-resolution.args
index 1dbcd660f1..ed2e4422da 100644
--- a/tests/qemuxml2argvdata/video-qxl-resolution.args
+++ b/tests/qemuxml2argvdata/video-qxl-resolution.args
@@ -28,5 +28,5 @@ server,nowait \
 -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
 -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \
 -device qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,vgamem_mb=8,\
-bus=pci.0,addr=0x2 \
+xres=1280,yres=720,bus=pci.0,addr=0x2 \
 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git a/tests/qemuxml2argvdata/video-qxl-resolution.xml 
b/tests/qemuxml2argvdata/video-qxl-resolution.xml
index 6ba2817002..1bc415f3f8 100644
--- a/tests/qemuxml2argvdata/video-qxl-resolution.xml
+++ b/tests/qemuxml2argvdata/video-qxl-resolution.xml
@@ -30,7 +30,9 @@
 
 
 
-  
+  
+
+  
   
 
 
diff --git a/tests/qemuxml2xmloutdata/video-qxl-resolution.xml 
b/tests/qemuxml2xmloutdata/video-qxl-resolution.xml
index 6ba2817002..1bc415f3f8 100644
--- a/tests/qemuxml2xmloutdata/video-qxl-resolution.xml
+++ b/tests/qemuxml2xmloutdata/video-qxl-resolution.xml
@@ -30,7 +30,9 @@
 
 
 
-  
+  
+
+  
   
 
 
-- 
2.20.1

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

[libvirt] [PATCH v4 0/2] Add resolution properties for QEMU video devices

[jcfaracco]

From: Julio Faracco 

This serie adds resolution ('x' and 'y') properties into XML definition
for QEMU video devices to specify a default resolution. This specific
case is not considering those attributes as a QEMU capabilities due to
complexity of code versus test complexity versus a real gain. This
skeleton would work very well initially. After, it should be possible to
include them as a capabilities without changing this serie.

v1-v2: Adds suggestions of multiple members.
v2-v3: Adds Cole's suggestions.
v3-v4: Adds Cole's suggestions again.

Julio Faracco (2):
  conf: Add 'x' and 'y' resolution into video XML definition
  qemu: Generate 'xres' and 'yres' for QEMU video devices

 docs/formatdomain.html.in |  5 +-
 docs/schemas/domaincommon.rng | 10 +++
 src/conf/domain_conf.c| 63 ++-
 src/conf/domain_conf.h|  5 ++
 src/conf/virconftypes.h   |  3 +
 src/qemu/qemu_command.c   |  5 ++
 src/qemu/qemu_domain.c| 11 
 .../video-qxl-resolution.args | 32 ++
 .../qemuxml2argvdata/video-qxl-resolution.xml | 42 +
 tests/qemuxml2argvtest.c  |  4 ++
 .../video-qxl-resolution.xml  | 42 +
 tests/qemuxml2xmltest.c   |  1 +
 12 files changed, 221 insertions(+), 2 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/video-qxl-resolution.args
 create mode 100644 tests/qemuxml2argvdata/video-qxl-resolution.xml
 create mode 100644 tests/qemuxml2xmloutdata/video-qxl-resolution.xml

-- 
2.20.1

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

[libvirt] [PATCH v4 1/2] conf: Add 'x' and 'y' resolution into video XML definition

[jcfaracco]

From: Julio Faracco 

This commit adds resolution element with parameters 'x' and 'y' into video
XML domain group definition. Both, properties were added into an element
called 'resolution' and it was added inside 'model' element. They are set
as optional. This element does not follow QEMU properties 'xres' and
'yres' format. Both HTML documentation and schema were changed too. This
commit includes a simple test case to cover resolution for QEMU video
models. The new XML format for resolution looks like:


  


Signed-off-by: Julio Faracco 
---
 docs/formatdomain.html.in |  5 +-
 docs/schemas/domaincommon.rng | 10 +++
 src/conf/domain_conf.c| 63 ++-
 src/conf/domain_conf.h|  5 ++
 src/conf/virconftypes.h   |  3 +
 .../video-qxl-resolution.args | 32 ++
 .../qemuxml2argvdata/video-qxl-resolution.xml | 40 
 tests/qemuxml2argvtest.c  |  4 ++
 .../video-qxl-resolution.xml  | 40 
 tests/qemuxml2xmltest.c   |  1 +
 10 files changed, 201 insertions(+), 2 deletions(-)
 create mode 100644 tests/qemuxml2argvdata/video-qxl-resolution.args
 create mode 100644 tests/qemuxml2argvdata/video-qxl-resolution.xml
 create mode 100644 tests/qemuxml2xmloutdata/video-qxl-resolution.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 500f114f41..962766b792 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -7077,7 +7077,10 @@ qemu-kvm -net nic,model=? /dev/null
   vgamem (since 1.2.11) to set
   the size of VGA framebuffer for fallback mode of QXL device.
   Attribute vram64 (since 
1.3.3)
-  extends secondary bar and makes it addressable as 64bit memory.
+  extends secondary bar and makes it addressable as 64bit memory. For
+  resolution settings, there are x and y
+  (since 5.9.0) optional attributes to set
+  minimum resolution for model.
 
   
 
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index ead5a25068..e06f892da3 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3656,6 +3656,16 @@
   
 
   
+  
+
+  
+
+  
+  
+
+  
+
+  
 
   
   
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 2e6a113de3..88e93f6fb8 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15345,6 +15345,52 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
 return def;
 }
 
+static virDomainVideoResolutionDefPtr
+virDomainVideoResolutionDefParseXML(xmlNodePtr node)
+{
+xmlNodePtr cur;
+virDomainVideoResolutionDefPtr def;
+g_autofree char *x = NULL;
+g_autofree char *y = NULL;
+
+cur = node->children;
+while (cur != NULL) {
+if (cur->type == XML_ELEMENT_NODE) {
+if (!x && !y &&
+virXMLNodeNameEqual(cur, "resolution")) {
+x = virXMLPropString(cur, "x");
+y = virXMLPropString(cur, "y");
+}
+}
+cur = cur->next;
+}
+
+if (!x || !y)
+return NULL;
+
+if (VIR_ALLOC(def) < 0)
+goto cleanup;
+
+if (x) {
+if (virStrToLong_uip(x, NULL, 10, >x) < 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("cannot parse video x-resolution '%s'"), x);
+goto cleanup;
+}
+}
+
+if (y) {
+if (virStrToLong_uip(y, NULL, 10, >y) < 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("cannot parse video y-resolution '%s'"), y);
+goto cleanup;
+}
+}
+
+ cleanup:
+return def;
+}
+
 static virDomainVideoDriverDefPtr
 virDomainVideoDriverDefParseXML(xmlNodePtr node)
 {
@@ -15424,6 +15470,7 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
 }
 
 def->accel = virDomainVideoAccelDefParseXML(cur);
+def->res = virDomainVideoResolutionDefParseXML(cur);
 }
 if (virXMLNodeNameEqual(cur, "driver")) {
 if (virDomainVirtioOptionsParseXML(cur, >virtio) < 0)
@@ -26515,6 +26562,18 @@ virDomainVideoAccelDefFormat(virBufferPtr buf,
 virBufferAddLit(buf, "/>\n");
 }
 
+static void
+virDomainVideoResolutionDefFormat(virBufferPtr buf,
+  virDomainVideoResolutionDefPtr def)
+{
+virBufferAddLit(buf, "x && def->y) {
+virBufferAsprintf(buf, " x='%u' y='%u'",
+  def->x, def->y);
+}
+virBufferAddLit(buf, "/>\n");
+}
+
 static int
 virDomainVideoDefFormat(virBufferPtr buf,
 virDomainVideoDefPtr def,
@@ 

[libvirt] [PATCH] Apparmor: Support Xen scripts in libexec

[Jim Fehlig]

Upstream Xen has traditionally installed various hotplug and
utility scripts in /etc/xen/scripts/. openSUSE is slowly moving
all distribution provided configuration files and scripts from
/etc to /usr. In the case of the Xen scripts provided under
/etc/xen/scripts/, they will be moving to /usr/lib/xen/scripts/.
Adjust the libvirtd Apparmor profile to allow executing scripts
from this location.

Signed-off-by: Jim Fehlig 
---

If this is deemed too distro-specific I'm happy to maintain a
downstream patch.

 src/security/apparmor/usr.sbin.libvirtd | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/security/apparmor/usr.sbin.libvirtd 
b/src/security/apparmor/usr.sbin.libvirtd
index 29f9936ad9..b0d23c80f3 100644
--- a/src/security/apparmor/usr.sbin.libvirtd
+++ b/src/security/apparmor/usr.sbin.libvirtd
@@ -104,6 +104,7 @@ profile libvirtd /usr/sbin/libvirtd 
flags=(attach_disconnected) {
   /usr/{lib,lib64}/libvirt/libvirt_iohelper ix,
   /etc/libvirt/hooks/** rmix,
   /etc/xen/scripts/** rmix,
+  /usr/{lib,lib64}/xen/scripts/** rmix,
 
   # allow changing to our UUID-based named profiles
   change_profile -> 
@{LIBVIRT}-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*,
-- 
2.23.0


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

[libvirt] [PATCH v2 2/3] conf/network_conf: use virStringParseYesNo helper

[Mao Zhongyi]

A function virStringParseYesNo was added to convert
string 'yes' to true and 'no' to false, so use this
helper to replace 'STREQ(.*, \"yes\")' and
'STREQ(.*, \"no\")' as it allows us to drop several
repetitive if-then-else string->bool conversion blocks.

Signed-off-by: Mao Zhongyi 
Signed-off-by: Zhang Shengju 
Acked-by: Michal Privoznik 
---
 src/conf/network_conf.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 75ec5ccc27..9954c3d25f 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -1682,9 +1682,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt,
  */
 ipv6nogwStr = virXPathString("string(./@ipv6)", ctxt);
 if (ipv6nogwStr) {
-if (STREQ(ipv6nogwStr, "yes")) {
-def->ipv6nogw = true;
-} else if (STRNEQ(ipv6nogwStr, "no")) {
+if (virStringParseYesNo(ipv6nogwStr, >ipv6nogw) < 0) {
 virReportError(VIR_ERR_XML_ERROR,
_("Invalid ipv6 setting '%s' in network '%s'"),
ipv6nogwStr, def->name);
-- 
2.17.1



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

[libvirt] [PATCH v2 1/3] conf/domain_conf: use virStringParseYesNo helper

[Mao Zhongyi]

This helper performs a conversion from a "yes|no" string
to a corresponding boolean, and several conversions were
already done, but there are still some omissions.

For most of the remaining usages in domain_conf.c only
"yes" is explicitly checked for. This means all other
values are implicitly handled as 'false'. In this case,
use virStringParseYesNo to handle the conversion and
reserve the original logic of not raise an error, so
ignore the return value of helper.

Signed-off-by: Mao Zhongyi 
Signed-off-by: Zhang Shengju 
---
 src/conf/domain_conf.c | 35 +--
 1 file changed, 13 insertions(+), 22 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 10d6bf0eea..370e2840eb 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7600,10 +7600,8 @@ virDomainHostdevSubsysUSBDefParseXML(xmlNodePtr node,
 }
 }
 
-if ((autoAddress = virXMLPropString(node, "autoAddress"))) {
-if (STREQ(autoAddress, "yes"))
-usbsrc->autoAddress = true;
-}
+if ((autoAddress = virXMLPropString(node, "autoAddress")))
+ignore_value(virStringParseYesNo(autoAddress, >autoAddress));
 
 /* Product can validly be 0, so we need some extra help to determine
  * if it is uninitialized*/
@@ -8167,10 +8165,8 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
  * element that might be (pure hostdev, or higher level device
  * (e.g. ) with type='hostdev')
  */
-if ((managed = virXMLPropString(node, "managed")) != NULL) {
-if (STREQ(managed, "yes"))
-def->managed = true;
-}
+if ((managed = virXMLPropString(node, "managed")) != NULL)
+ignore_value(virStringParseYesNo(managed, >managed));
 
 sgio = virXMLPropString(node, "sgio");
 rawio = virXMLPropString(node, "rawio");
@@ -13807,9 +13803,7 @@ 
virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr def,
 
 if (autoGenerated &&
 flags & VIR_DOMAIN_DEF_PARSE_STATUS) {
-if (STREQ(autoGenerated, "yes")) {
-def->autoGenerated = true;
-} else if (STRNEQ(autoGenerated, "no")) {
+if (virStringParseYesNo(autoGenerated, >autoGenerated) < 0) {
 virReportError(VIR_ERR_XML_ERROR,
_("Invalid autoGenerated value: %s"),
autoGenerated);
@@ -13939,13 +13933,10 @@ 
virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def,
 }
 
 if (autoport) {
-if (STREQ(autoport, "yes")) {
-if (flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)
-def->data.vnc.port = 0;
-def->data.vnc.autoport = true;
-} else {
-def->data.vnc.autoport = false;
-}
+ignore_value(virStringParseYesNo(autoport, >data.vnc.autoport));
+
+if (def->data.vnc.autoport && flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)
+def->data.vnc.port = 0;
 }
 
 if (websocket) {
@@ -13958,8 +13949,9 @@ virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr 
def,
 }
 }
 
-if (websocketGenerated && STREQ(websocketGenerated, "yes"))
-def->data.vnc.websocketGenerated = true;
+if (websocketGenerated)
+ignore_value(virStringParseYesNo(websocketGenerated,
+ >data.vnc.websocketGenerated));
 
 if (sharePolicy) {
 int policy =
@@ -15479,8 +15471,7 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
 heads = virXMLPropString(cur, "heads");
 
 if ((primary = virXMLPropString(cur, "primary")) != NULL) {
-if (STREQ(primary, "yes"))
-def->primary = true;
+ignore_value(virStringParseYesNo(primary, >primary));
 VIR_FREE(primary);
 }
 
-- 
2.17.1



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

[libvirt] [PATCH v2 0/3] use virStringParseYesNo helper

[Mao Zhongyi]

A function virStringParseYesNo was added to convert
string 'yes' to true and 'no' to false, so use this
helper to replace 'STREQ(.*, \"yes\")' and
'STREQ(.*, \"no\")' as it allows us to drop several
repetitive if-then-else string->bool conversion blocks.

v2->v1

p1:
- ignore the return value of virStringParseYesNo.
- update the commit message.   [Michal Privoznik]

p2:
- add the Acked-by tag.

p3:
- pass return value of helper to rc directly.
  [Michal Privoznik]

Mao Zhongyi (3):
  conf/domain_conf: use virStringParseYesNo helper
  conf/network_conf: use virStringParseYesNo helper
  qemu/qemu_migration_params: use virStringParseYesNo helper

 src/conf/domain_conf.c   | 35 
 src/conf/network_conf.c  |  4 +---
 src/qemu/qemu_migration_params.c |  7 +--
 3 files changed, 15 insertions(+), 31 deletions(-)

-- 
2.17.1



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

[libvirt] [PATCH v2 3/3] qemu/qemu_migration_params: use virStringParseYesNo helper

[Mao Zhongyi]

A function virStringParseYesNo was added to convert
string 'yes' to true and 'no' to false, so use this
helper to replace 'STREQ(.*, \"yes\")' and
'STREQ(.*, \"no\")' as it allows us to drop several
repetitive if-then-else string->bool conversion blocks.

Signed-off-by: Mao Zhongyi 
Signed-off-by: Zhang Shengju 
---
 src/qemu/qemu_migration_params.c | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c
index 85fa8f8de5..f5bc8596f4 100644
--- a/src/qemu/qemu_migration_params.c
+++ b/src/qemu/qemu_migration_params.c
@@ -1326,12 +1326,7 @@ qemuMigrationParamsParse(xmlXPathContextPtr ctxt,
 break;
 
 case QEMU_MIGRATION_PARAM_TYPE_BOOL:
-if (STREQ(value, "yes"))
-pv->value.b = true;
-else if (STREQ(value, "no"))
-pv->value.b = false;
-else
-rc = -1;
+rc = virStringParseYesNo(value, >value.b);
 break;
 
 case QEMU_MIGRATION_PARAM_TYPE_STRING:
-- 
2.17.1



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

Re: [libvirt] [PATCH 1/3] conf/domain_conf: usevirStringParseYesNohelper

[maozy]

On 10/16/19 7:02 PM, Michal Privoznik wrote:

On 10/16/19 12:10 PM, maozy wrote:

Hi, Michal

On 10/16/19 4:38 PM, Michal Privoznik wrote:

On 10/16/19 4:39 AM, Mao Zhongyi wrote:

This helper performs a conversion from a "yes|no" string
to a corresponding boolean, and several conversions were
already done, but there are still some omissions.

For most of the remaining usages in domain_conf.c only
"yes" is explicitly checked for. This means all other
values are implicitly handled as 'false'. In this case,
use virStringParseYesNo, but if it returns -1, don't
raise an error but set the bool value to false.

Cc: crobi...@redhat.com
Cc: berra...@redhat.com
Cc: abolo...@redhat.com
Cc: la...@laine.org
Cc: phrd...@redhat.com
Cc: mklet...@redhat.com
Cc: g.sho1...@gmail.com

Signed-off-by: Mao Zhongyi 
Signed-off-by: Zhang Shengju 
---
  src/conf/domain_conf.c | 30 ++
  1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 10d6bf0eea..7420658726 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7601,8 +7601,8 @@ 
virDomainHostdevSubsysUSBDefParseXML(xmlNodePtr node,

  }
  if ((autoAddress = virXMLPropString(node, "autoAddress"))) {
-    if (STREQ(autoAddress, "yes"))
-    usbsrc->autoAddress = true;
+    if (virStringParseYesNo(autoAddress, >autoAddress) 
< 0)

+    usbsrc->autoAddress = false;


I think we should either report a proper error here or don't touch 
this line (because this attribute is generated by libvirt itself, it 
doesn't come from an user).


This patch replaces all STREQs that explicitly handle 'yes' to true,
reserving the original implicitly handling of other values to false,
so not report an error, which is also the recommendation of Cole
Robinson in BiteSizedtasks. Therefor, I'm not very sure whether it
is better to report an error in this case? If indeed, will fix it in v2.


Alright then. But what we can use then is:

ignore_value(virStringParseYesNo(autoAddress, >autoAddress));

which is equivallent to existing code and also shorter.






  }
  /* Product can validly be 0, so we need some extra help to 
determine
@@ -8168,8 +8168,8 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr 
node,

   * (e.g. ) with type='hostdev')
   */
  if ((managed = virXMLPropString(node, "managed")) != NULL) {
-    if (STREQ(managed, "yes"))
-    def->managed = true;
+    if (virStringParseYesNo(managed, >managed) < 0)
+    def->managed = false;


Here we need to report a proper error because "managed" attribute 
comes from user, therefore risk of an user providing invalid value is 
high compared to the first hunk. But if we want to be consistent we 
should report error in both cases.



  }
  sgio = virXMLPropString(node, "sgio");
@@ -13807,9 +13807,7 @@ 
virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr def,

  if (autoGenerated &&
  flags & VIR_DOMAIN_DEF_PARSE_STATUS) {
-    if (STREQ(autoGenerated, "yes")) {
-    def->autoGenerated = true;
-    } else if (STRNEQ(autoGenerated, "no")) {
+    if (virStringParseYesNo(autoGenerated, >autoGenerated) 
< 0) {

  virReportError(VIR_ERR_XML_ERROR,
 _("Invalid autoGenerated value: %s"),
 autoGenerated);


This one is correct.

@@ -13939,12 +13937,11 @@ 
virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def,

  }
  if (autoport) {
-    if (STREQ(autoport, "yes")) {
-    if (flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)
-    def->data.vnc.port = 0;
-    def->data.vnc.autoport = true;
-    } else {
+    if (virStringParseYesNo(autoport, >data.vnc.autoport) 
< 0) {

  def->data.vnc.autoport = false;
+    } else {
+    if (def->data.vnc.autoport && flags & 
VIR_DOMAIN_DEF_PARSE_INACTIVE)

+    def->data.vnc.port = 0;


This doesn't need to go under else branch really.


right



  }
  }
@@ -13958,8 +13955,9 @@ 
virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def,

  }
  }
-    if (websocketGenerated && STREQ(websocketGenerated, "yes"))
-    def->data.vnc.websocketGenerated = true;
+    if (websocketGenerated &&
+    virStringParseYesNo(websocketGenerated, 
>data.vnc.websocketGenerated))


Ooops, you've missed '< 0' comparison.


virStringParseYesNo return 0 on succeed and -1 on error, and
here is && operation, so, no explicit comparison '< 0'.


The comparison is required because even though the function doesn't 
return any positive value now it might at some point in the future. In 
libvirt, there's the following semantics for integer return values:


< 0 : an error state,
   0 : success,
 > 0 : success, but also something is signalized to the caller

There's plenty of examples, but let me pick just one: The retvals for 

Re: [libvirt] [PATCH v2 0/3] qemu_process: use VIR_AUTO*/g_auto* all around

[Daniel Henrique Barboza]

Please nevermind this patch series. It is deprecated after
all the changes going in current master. I'll resend it
using GLib macros only and splitting the patch series like
it was suggested by Jano in the qemu_driver cleanup.


Thanks,

DHB

On 10/15/19 7:47 PM, Daniel Henrique Barboza wrote:

The usual AUTOFREE() and AUTOUNREF() changes that allows
for a bit of cleanup. A new patch was added to convert
most of the VIR_AUTO* macros to GLib g_auto* ones.


v1: https://www.redhat.com/archives/libvir-list/2019-September/msg01465.html

Daniel Henrique Barboza (3):
   qemu_process: use VIR_AUTOFREE()
   qemu_process: use VIR_AUTOUNREF()
   qemu_process.c: use GLib macros

  src/qemu/qemu_process.c | 444 +++-
  1 file changed, 161 insertions(+), 283 deletions(-)



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

[libvirt] [PATCH v4 5/5] qemu_driver: use g_strdup_printf

[Daniel Henrique Barboza]

This patch changes all virAsprintf calls to use the GLib API
g_strdup_printf in qemu_driver.c

Signed-off-by: Daniel Henrique Barboza 
---
 src/qemu/qemu_driver.c | 38 +-
 1 file changed, 17 insertions(+), 21 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a263393626..c9b3ed877f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -402,7 +402,7 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
 
 priv = vm->privateData;
 
-if (virAsprintf(, "%s/%s", baseDir, vm->def->name) < 0) {
+if (!(snapDir = g_strdup_printf("%s/%s", baseDir, vm->def->name))) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to allocate memory for "
"snapshot directory for domain %s"),
@@ -427,7 +427,7 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
kill the whole process */
 VIR_INFO("Loading snapshot file '%s'", entry->d_name);
 
-if (virAsprintf(, "%s/%s", snapDir, entry->d_name) < 0) {
+if (!(fullpath = g_strdup_printf("%s/%s", snapDir, entry->d_name))) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Failed to allocate memory for path"));
 continue;
@@ -514,7 +514,7 @@ qemuDomainCheckpointLoad(virDomainObjPtr vm,
 virObjectLock(vm);
 priv = vm->privateData;
 
-if (virAsprintf(, "%s/%s", baseDir, vm->def->name) < 0) {
+if (!(chkDir = g_strdup_printf("%s/%s", baseDir, vm->def->name))) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to allocate memory for "
"checkpoint directory for domain %s"),
@@ -539,7 +539,7 @@ qemuDomainCheckpointLoad(virDomainObjPtr vm,
kill the whole process */
 VIR_INFO("Loading checkpoint file '%s'", entry->d_name);
 
-if (virAsprintf(, "%s/%s", chkDir, entry->d_name) < 0) {
+if (!(fullpath = g_strdup_printf("%s/%s", chkDir, entry->d_name))) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Failed to allocate memory for path"));
 continue;
@@ -690,7 +690,7 @@ qemuStateInitialize(bool privileged,
 if (!(qemu_driver->config = cfg = virQEMUDriverConfigNew(privileged)))
 goto error;
 
-if (virAsprintf(, "%s/qemu.conf", cfg->configBaseDir) < 0)
+if (!(driverConf = g_strdup_printf("%s/qemu.conf", cfg->configBaseDir)))
 goto error;
 
 if (virQEMUDriverConfigLoadFile(cfg, driverConf, privileged) < 0)
@@ -1359,10 +1359,10 @@ qemuGetSchedInfo(unsigned long long *cpuWait,
 /* In general, we cannot assume pid_t fits in int; but /proc parsing
  * is specific to Linux where int works fine.  */
 if (tid)
-ret = virAsprintf(, "/proc/%d/task/%d/sched", (int)pid, (int)tid);
+proc = g_strdup_printf("/proc/%d/task/%d/sched", (int)pid, (int)tid);
 else
-ret = virAsprintf(, "/proc/%d/sched", (int)pid);
-if (ret < 0)
+proc = g_strdup_printf("/proc/%d/sched", (int)pid);
+if (!proc)
 goto cleanup;
 ret = -1;
 
@@ -1426,15 +1426,14 @@ qemuGetProcessInfo(unsigned long long *cpuTime, int 
*lastCpu, long *vm_rss,
 unsigned long long usertime = 0, systime = 0;
 long rss = 0;
 int cpu = 0;
-int ret;
 
 /* In general, we cannot assume pid_t fits in int; but /proc parsing
  * is specific to Linux where int works fine.  */
 if (tid)
-ret = virAsprintf(, "/proc/%d/task/%d/stat", (int)pid, tid);
+proc = g_strdup_printf("/proc/%d/task/%d/stat", (int)pid, tid);
 else
-ret = virAsprintf(, "/proc/%d/stat", (int)pid);
-if (ret < 0)
+proc = g_strdup_printf("/proc/%d/stat", (int)pid);
+if (!proc)
 return -1;
 
 pidinfo = fopen(proc, "r");
@@ -3521,7 +3520,7 @@ qemuDomainManagedSavePath(virQEMUDriverPtr driver, 
virDomainObjPtr vm)
 char *ret;
 g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
 
-if (virAsprintf(, "%s/%s.save", cfg->saveDir, vm->def->name) < 0)
+if (!(ret = g_strdup_printf("%s/%s.save", cfg->saveDir, vm->def->name)))
 return NULL;
 
 return ret;
@@ -4032,7 +4031,7 @@ qemuDomainScreenshot(virDomainPtr dom,
 }
 }
 
-if (virAsprintf(, "%s/qemu.screendump.XX", cfg->cacheDir) < 0)
+if (!(tmp = g_strdup_printf("%s/qemu.screendump.XX", cfg->cacheDir)))
 goto endjob;
 
 if ((tmp_fd = mkostemp(tmp, O_CLOEXEC)) == -1) {
@@ -4095,10 +4094,7 @@ getAutoDumpPath(virQEMUDriverPtr driver,
 localtime_r(, _info);
 strftime(timestr, sizeof(timestr), "%Y-%m-%d-%H:%M:%S", _info);
 
-ignore_value(virAsprintf(, "%s/%s-%s",
- cfg->autoDumpPath,
- domname,
- timestr));
+dumpfile = g_strdup_printf("%s/%s-%s", cfg->autoDumpPath, domname, 
timestr);
 
 return dumpfile;
 }
@@ -5883,7 +5879,7 @@ 

[libvirt] [PATCH v4 4/5] qemu_driver: remove unused 'cleanup' labels after g_auto*() changes

[Daniel Henrique Barboza]

The g_auto*() changes made by the previous patches made a lot
of 'cleanup' labels obsolete. Let's remove them.

Suggested-by: Ján Tomko 
Signed-off-by: Daniel Henrique Barboza 
---
 src/qemu/qemu_driver.c | 160 +++--
 1 file changed, 56 insertions(+), 104 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d824992c16..a263393626 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1057,7 +1057,7 @@ qemuStateReload(void)
 return 0;
 
 if (!(caps = virQEMUDriverGetCapabilities(qemu_driver, false)))
-goto cleanup;
+return 0;
 
 cfg = virQEMUDriverGetConfig(qemu_driver);
 virDomainObjListLoadAllConfigs(qemu_driver->domains,
@@ -1065,7 +1065,6 @@ qemuStateReload(void)
cfg->autostartDir, false,
caps, qemu_driver->xmlopt,
qemuNotifyLoadDomain, qemu_driver);
- cleanup:
 return 0;
 }
 
@@ -1181,18 +1180,15 @@ static int
 qemuConnectURIProbe(char **uri)
 {
 g_autoptr(virQEMUDriverConfig) cfg = NULL;
-int ret = -1;
 
 if (qemu_driver == NULL)
 return 0;
 
 cfg = virQEMUDriverGetConfig(qemu_driver);
 if (VIR_STRDUP(*uri, cfg->uri) < 0)
-goto cleanup;
+return -1;
 
-ret = 0;
- cleanup:
-return ret;
+return 0;
 }
 
 static virDrvOpenStatus qemuConnectOpen(virConnectPtr conn,
@@ -1335,20 +1331,15 @@ qemuConnectGetMaxVcpus(virConnectPtr conn 
G_GNUC_UNUSED, const char *type)
 
 static char *qemuConnectGetCapabilities(virConnectPtr conn) {
 virQEMUDriverPtr driver = conn->privateData;
-char *xml = NULL;
 g_autoptr(virCaps) caps = NULL;
 
 if (virConnectGetCapabilitiesEnsureACL(conn) < 0)
 return NULL;
 
 if (!(caps = virQEMUDriverGetCapabilities(driver, true)))
-goto cleanup;
-
-xml = virCapabilitiesFormatXML(caps);
-
- cleanup:
+return NULL;
 
-return xml;
+return virCapabilitiesFormatXML(caps);
 }
 
 
@@ -1691,7 +1682,6 @@ static int qemuDomainIsUpdated(virDomainPtr dom)
 static int qemuConnectGetVersion(virConnectPtr conn, unsigned long *version)
 {
 virQEMUDriverPtr driver = conn->privateData;
-int ret = -1;
 unsigned int qemuVersion = 0;
 g_autoptr(virCaps) caps = NULL;
 
@@ -1699,18 +1689,15 @@ static int qemuConnectGetVersion(virConnectPtr conn, 
unsigned long *version)
 return -1;
 
 if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
-goto cleanup;
+return -1;
 
 if (virQEMUCapsGetDefaultVersion(caps,
  driver->qemuCapsCache,
  ) < 0)
-goto cleanup;
+return -1;
 
 *version = qemuVersion;
-ret = 0;
-
- cleanup:
-return ret;
+return 0;
 }
 
 
@@ -2902,7 +2889,6 @@ virQEMUSaveDataWrite(virQEMUSaveDataPtr data,
 size_t len;
 size_t xml_len;
 size_t cookie_len = 0;
-int ret = -1;
 size_t zerosLen = 0;
 g_autofree char *zeros = NULL;
 
@@ -2916,12 +2902,12 @@ virQEMUSaveDataWrite(virQEMUSaveDataPtr data,
 if (len > header->data_len) {
 virReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("new xml too large to fit in file"));
-goto cleanup;
+return -1;
 }
 
 zerosLen = header->data_len - len;
 if (VIR_ALLOC_N(zeros, zerosLen) < 0)
-goto cleanup;
+return -1;
 } else {
 header->data_len = len;
 }
@@ -2933,14 +2919,14 @@ virQEMUSaveDataWrite(virQEMUSaveDataPtr data,
 virReportSystemError(errno,
  _("failed to write header to domain save file 
'%s'"),
  path);
-goto cleanup;
+return -1;
 }
 
 if (safewrite(fd, data->xml, xml_len) != xml_len) {
 virReportSystemError(errno,
  _("failed to write domain xml to '%s'"),
  path);
-goto cleanup;
+return -1;
 }
 
 if (data->cookie &&
@@ -2948,20 +2934,17 @@ virQEMUSaveDataWrite(virQEMUSaveDataPtr data,
 virReportSystemError(errno,
  _("failed to write cookie to '%s'"),
  path);
-goto cleanup;
+return -1;
 }
 
 if (safewrite(fd, zeros, zerosLen) != zerosLen) {
 virReportSystemError(errno,
  _("failed to write padding to '%s'"),
  path);
-goto cleanup;
+return -1;
 }
 
-ret = 0;
-
- cleanup:
-return ret;
+return 0;
 }
 
 
@@ -3031,7 +3014,6 @@ qemuOpenFile(virQEMUDriverPtr driver,
  int oflags,
  bool *needUnlink)
 {
-int ret = -1;
 g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
 uid_t user = cfg->user;
 gid_t group = 

[libvirt] [PATCH v4 1/5] qemu_driver: use g_auto* in some functions

[Daniel Henrique Barboza]

This patch changes qemuDomainSnapshotLoad, qemuDomainCheckpointLoad and
qemuStateInitialize to use g_autoptr() and g_autofree, cleaning up
some virObjectUnref() and VIR_FREE() calls on each.

The reason this is being sent separately is because these are not
trivial search/replace cases. In all these functions some strings
declarations are moved inside local loops, where they are in fact
used, allowing us to erase VIR_FREE() calls that were made inside
the loop and in 'cleanup' labels.

Following patches with tackle more trivial cases of g_auto* usage
in all qemu_driver.c file.

Suggested-by: Ján Tomko 
Signed-off-by: Daniel Henrique Barboza 
---
 src/qemu/qemu_driver.c | 51 +-
 1 file changed, 15 insertions(+), 36 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 86d7647628..3772c71a51 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -386,11 +386,9 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
void *data)
 {
 char *baseDir = (char *)data;
-char *snapDir = NULL;
+g_autofree char *snapDir = NULL;
 DIR *dir = NULL;
 struct dirent *entry;
-char *xmlStr;
-char *fullpath;
 virDomainSnapshotDefPtr def = NULL;
 virDomainMomentObjPtr snap = NULL;
 virDomainMomentObjPtr current = NULL;
@@ -399,7 +397,7 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
   VIR_DOMAIN_SNAPSHOT_PARSE_DISKS |
   VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL);
 int ret = -1;
-virCapsPtr caps = NULL;
+g_autoptr(virCaps) caps = NULL;
 int direrr;
 qemuDomainObjPrivatePtr priv;
 
@@ -425,6 +423,9 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
 goto cleanup;
 
 while ((direrr = virDirRead(dir, , NULL)) > 0) {
+g_autofree char *xmlStr = NULL;
+g_autofree char *fullpath = NULL;
+
 /* NB: ignoring errors, so one malformed config doesn't
kill the whole process */
 VIR_INFO("Loading snapshot file '%s'", entry->d_name);
@@ -440,7 +441,6 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
 virReportSystemError(errno,
  _("Failed to read snapshot file %s"),
  fullpath);
-VIR_FREE(fullpath);
 continue;
 }
 
@@ -453,8 +453,6 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to parse snapshot XML from file '%s'"),
fullpath);
-VIR_FREE(fullpath);
-VIR_FREE(xmlStr);
 continue;
 }
 
@@ -468,9 +466,6 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
vm->def->name);
 current = snap;
 }
-
-VIR_FREE(fullpath);
-VIR_FREE(xmlStr);
 }
 if (direrr < 0)
 virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -497,8 +492,6 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
 ret = 0;
  cleanup:
 VIR_DIR_CLOSE(dir);
-VIR_FREE(snapDir);
-virObjectUnref(caps);
 virObjectUnlock(vm);
 return ret;
 }
@@ -509,17 +502,15 @@ qemuDomainCheckpointLoad(virDomainObjPtr vm,
  void *data)
 {
 char *baseDir = (char *)data;
-char *chkDir = NULL;
+g_autofree char *chkDir = NULL;
 DIR *dir = NULL;
 struct dirent *entry;
-char *xmlStr;
-char *fullpath;
 virDomainCheckpointDefPtr def = NULL;
 virDomainMomentObjPtr chk = NULL;
 virDomainMomentObjPtr current = NULL;
 unsigned int flags = VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE;
 int ret = -1;
-virCapsPtr caps = NULL;
+g_autoptr(virCaps) caps = NULL;
 int direrr;
 qemuDomainObjPrivatePtr priv;
 
@@ -544,6 +535,9 @@ qemuDomainCheckpointLoad(virDomainObjPtr vm,
 goto cleanup;
 
 while ((direrr = virDirRead(dir, , NULL)) > 0) {
+g_autofree char *xmlStr = NULL;
+g_autofree char *fullpath = NULL;
+
 /* NB: ignoring errors, so one malformed config doesn't
kill the whole process */
 VIR_INFO("Loading checkpoint file '%s'", entry->d_name);
@@ -559,7 +553,6 @@ qemuDomainCheckpointLoad(virDomainObjPtr vm,
 virReportSystemError(errno,
  _("Failed to read checkpoint file %s"),
  fullpath);
-VIR_FREE(fullpath);
 continue;
 }
 
@@ -572,8 +565,6 @@ qemuDomainCheckpointLoad(virDomainObjPtr vm,
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to parse checkpoint XML from file '%s'"),
fullpath);
-VIR_FREE(fullpath);
-VIR_FREE(xmlStr);
 virObjectUnref(def);
 continue;
 }
@@ -581,9 +572,6 @@ qemuDomainCheckpointLoad(virDomainObjPtr vm,
 chk = virDomainCheckpointAssignDef(vm->checkpoints, 

[libvirt] [PATCH v4 0/5] qemu_driver: GLib powered cleanup

[Daniel Henrique Barboza]

changes from v3:
- only use the new Glib macros in all patches
- different patch split, as suggested by Jano in v3 review
- patch 5 (new): g_strdup_printf conversion

changes from v2:
- rebased with newer master (67e72053c1)
- added an extra patch to convert the existing VIR_AUTO* macros
to g_auto* ones, all at once, to avoid the case where a method
will have both VIR_AUTO* and g_auto* macros at the same time.

changes from v1:
- addressed review concerns made by Erik
- added more cleanups, as suggested by Erik

v3: https://www.redhat.com/archives/libvir-list/2019-October/msg00918.html
v2: https://www.redhat.com/archives/libvir-list/2019-September/msg01452.html
v1: https://www.redhat.com/archives/libvir-list/2019-September/msg00719.html

Daniel Henrique Barboza (5):
  qemu_driver: use g_auto* in some functions
  qemu_driver: use g_autoptr() when possible
  qemu_driver: use g_autofree when possible
  qemu_driver: remove unused 'cleanup' labels after g_auto*() changes
  qemu_driver: use g_strdup_printf

 src/qemu/qemu_driver.c | 721 ++---
 1 file changed, 242 insertions(+), 479 deletions(-)

-- 
2.21.0

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

[libvirt] [PATCH v4 2/5] qemu_driver: use g_autoptr() when possible

[Daniel Henrique Barboza]

Several pointer types can be auto-unref for the great majority
of the uses made in qemu_driver, sparing us a virObjectUnref()
call.

This patch uses g_autoptr() in the following pointer types inside
qemu_driver.c, whenever possible:

- qemuBlockJobDataPtr
- virCapsPtr
- virConnect
- virDomainCapsPtr
- virNetworkPtr
- virQEMUDriverConfigPtr

Suggested-by: Erik Skultety 
Signed-off-by: Daniel Henrique Barboza 
---
 src/qemu/qemu_driver.c | 271 +
 1 file changed, 87 insertions(+), 184 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 3772c71a51..dc342734b1 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -202,7 +202,7 @@ qemuAutostartDomain(virDomainObjPtr vm,
 {
 virQEMUDriverPtr driver = opaque;
 int flags = 0;
-virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
 int ret = -1;
 
 if (cfg->autoStartBypassCache)
@@ -234,7 +234,6 @@ qemuAutostartDomain(virDomainObjPtr vm,
 ret = 0;
  cleanup:
 virDomainObjEndAPI();
-virObjectUnref(cfg);
 return ret;
 }
 
@@ -308,7 +307,7 @@ qemuSecurityInit(virQEMUDriverPtr driver)
 char **names;
 virSecurityManagerPtr mgr = NULL;
 virSecurityManagerPtr stack = NULL;
-virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
 unsigned int flags = 0;
 
 if (cfg->securityDefaultConfined)
@@ -368,7 +367,6 @@ qemuSecurityInit(virQEMUDriverPtr driver)
 }
 
 driver->securityManager = stack;
-virObjectUnref(cfg);
 return 0;
 
  error:
@@ -376,7 +374,6 @@ qemuSecurityInit(virQEMUDriverPtr driver)
_("Failed to initialize security drivers"));
 virObjectUnref(stack);
 virObjectUnref(mgr);
-virObjectUnref(cfg);
 return -1;
 }
 
@@ -1053,8 +1050,8 @@ static void qemuNotifyLoadDomain(virDomainObjPtr vm, int 
newVM, void *opaque)
 static int
 qemuStateReload(void)
 {
-virQEMUDriverConfigPtr cfg = NULL;
-virCapsPtr caps = NULL;
+g_autoptr(virQEMUDriverConfig) cfg = NULL;
+g_autoptr(virCaps) caps = NULL;
 
 if (!qemu_driver)
 return 0;
@@ -1069,8 +1066,6 @@ qemuStateReload(void)
caps, qemu_driver->xmlopt,
qemuNotifyLoadDomain, qemu_driver);
  cleanup:
-virObjectUnref(cfg);
-virObjectUnref(caps);
 return 0;
 }
 
@@ -1085,13 +1080,13 @@ static int
 qemuStateStop(void)
 {
 int ret = -1;
-virConnectPtr conn;
+g_autoptr(virConnect) conn = NULL;
 int numDomains = 0;
 size_t i;
 int state;
 virDomainPtr *domains = NULL;
 unsigned int *flags = NULL;
-virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(qemu_driver);
+g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(qemu_driver);
 
 if (!(conn = virConnectOpen(cfg->uri)))
 goto cleanup;
@@ -1129,8 +1124,6 @@ qemuStateStop(void)
 VIR_FREE(domains);
 }
 VIR_FREE(flags);
-virObjectUnref(conn);
-virObjectUnref(cfg);
 
 return ret;
 }
@@ -1188,7 +1181,7 @@ qemuStateCleanup(void)
 static int
 qemuConnectURIProbe(char **uri)
 {
-virQEMUDriverConfigPtr cfg = NULL;
+g_autoptr(virQEMUDriverConfig) cfg = NULL;
 int ret = -1;
 
 if (qemu_driver == NULL)
@@ -1200,7 +1193,6 @@ qemuConnectURIProbe(char **uri)
 
 ret = 0;
  cleanup:
-virObjectUnref(cfg);
 return ret;
 }
 
@@ -1344,8 +1336,8 @@ qemuConnectGetMaxVcpus(virConnectPtr conn G_GNUC_UNUSED, 
const char *type)
 
 static char *qemuConnectGetCapabilities(virConnectPtr conn) {
 virQEMUDriverPtr driver = conn->privateData;
-virCapsPtr caps = NULL;
 char *xml = NULL;
+g_autoptr(virCaps) caps = NULL;
 
 if (virConnectGetCapabilitiesEnsureACL(conn) < 0)
 return NULL;
@@ -1354,7 +1346,6 @@ static char *qemuConnectGetCapabilities(virConnectPtr 
conn) {
 goto cleanup;
 
 xml = virCapabilitiesFormatXML(caps);
-virObjectUnref(caps);
 
  cleanup:
 
@@ -1706,7 +1697,7 @@ static int qemuConnectGetVersion(virConnectPtr conn, 
unsigned long *version)
 virQEMUDriverPtr driver = conn->privateData;
 int ret = -1;
 unsigned int qemuVersion = 0;
-virCapsPtr caps = NULL;
+g_autoptr(virCaps) caps = NULL;
 
 if (virConnectGetVersionEnsureACL(conn) < 0)
 return -1;
@@ -1723,7 +1714,6 @@ static int qemuConnectGetVersion(virConnectPtr conn, 
unsigned long *version)
 ret = 0;
 
  cleanup:
-virObjectUnref(caps);
 return ret;
 }
 
@@ -1777,7 +1767,7 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr 
conn,
 virObjectEventPtr event = NULL;
 virObjectEventPtr event2 = NULL;
 unsigned int start_flags = VIR_QEMU_PROCESS_START_COLD;
-virCapsPtr caps = NULL;
+g_autoptr(virCaps) caps = NULL;
 unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE 

[libvirt] [PATCH v4 3/5] qemu_driver: use g_autofree when possible

[Daniel Henrique Barboza]

String and other scalar pointers an be auto-unref, sparing us
a VIR_FREE() call.

This patch uses g_autofree whenever possible with strings and
other scalar pointer types.

Suggested-by: Erik Skultety 
Signed-off-by: Daniel Henrique Barboza 
---
 src/qemu/qemu_driver.c | 203 ++---
 1 file changed, 68 insertions(+), 135 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index dc342734b1..d824992c16 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1085,7 +1085,7 @@ qemuStateStop(void)
 size_t i;
 int state;
 virDomainPtr *domains = NULL;
-unsigned int *flags = NULL;
+g_autofree unsigned int *flags = NULL;
 g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(qemu_driver);
 
 if (!(conn = virConnectOpen(cfg->uri)))
@@ -1123,7 +1123,6 @@ qemuStateStop(void)
 virObjectUnref(domains[i]);
 VIR_FREE(domains);
 }
-VIR_FREE(flags);
 
 return ret;
 }
@@ -1357,8 +1356,8 @@ static int
 qemuGetSchedInfo(unsigned long long *cpuWait,
  pid_t pid, pid_t tid)
 {
-char *proc = NULL;
-char *data = NULL;
+g_autofree char *proc = NULL;
+g_autofree char *data = NULL;
 char **lines = NULL;
 size_t i;
 int ret = -1;
@@ -1422,8 +1421,6 @@ qemuGetSchedInfo(unsigned long long *cpuWait,
 ret = 0;
 
  cleanup:
-VIR_FREE(data);
-VIR_FREE(proc);
 virStringListFree(lines);
 return ret;
 }
@@ -1433,7 +1430,7 @@ static int
 qemuGetProcessInfo(unsigned long long *cpuTime, int *lastCpu, long *vm_rss,
pid_t pid, int tid)
 {
-char *proc;
+g_autofree char *proc = NULL;
 FILE *pidinfo;
 unsigned long long usertime = 0, systime = 0;
 long rss = 0;
@@ -1450,7 +1447,6 @@ qemuGetProcessInfo(unsigned long long *cpuTime, int 
*lastCpu, long *vm_rss,
 return -1;
 
 pidinfo = fopen(proc, "r");
-VIR_FREE(proc);
 
 /* See 'man proc' for information about what all these fields are. We're
  * only interested in a very few of them */
@@ -2908,7 +2904,7 @@ virQEMUSaveDataWrite(virQEMUSaveDataPtr data,
 size_t cookie_len = 0;
 int ret = -1;
 size_t zerosLen = 0;
-char *zeros = NULL;
+g_autofree char *zeros = NULL;
 
 xml_len = strlen(data->xml) + 1;
 if (data->cookie)
@@ -2965,7 +2961,6 @@ virQEMUSaveDataWrite(virQEMUSaveDataPtr data,
 ret = 0;
 
  cleanup:
-VIR_FREE(zeros);
 return ret;
 }
 
@@ -3300,7 +3295,7 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver,
int compressed, const char *compressedpath,
const char *xmlin, unsigned int flags)
 {
-char *xml = NULL;
+g_autofree char *xml = NULL;
 bool was_running = false;
 int ret = -1;
 virObjectEventPtr event = NULL;
@@ -3381,7 +3376,6 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver,
 if (!(data = virQEMUSaveDataNew(xml, cookie, was_running, compressed,
 driver->xmlopt)))
 goto endjob;
-xml = NULL;
 
 ret = qemuDomainSaveMemory(driver, vm, path, data, compressedpath,
flags, QEMU_ASYNC_JOB_SAVE);
@@ -3417,7 +3411,6 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver,
 
  cleanup:
 virObjectUnref(cookie);
-VIR_FREE(xml);
 virQEMUSaveDataFree(data);
 virObjectEventStateQueue(driver->domainEventState, event);
 return ret;
@@ -3505,7 +3498,7 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, 
const char *dxml,
 {
 virQEMUDriverPtr driver = dom->conn->privateData;
 int compressed;
-char *compressedpath = NULL;
+g_autofree char *compressedpath = NULL;
 int ret = -1;
 virDomainObjPtr vm = NULL;
 g_autoptr(virQEMUDriverConfig) cfg = NULL;
@@ -3534,7 +3527,6 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, 
const char *dxml,
 
  cleanup:
 virDomainObjEndAPI();
-VIR_FREE(compressedpath);
 return ret;
 }
 
@@ -3562,9 +3554,9 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int 
flags)
 virQEMUDriverPtr driver = dom->conn->privateData;
 g_autoptr(virQEMUDriverConfig) cfg = NULL;
 int compressed;
-char *compressedpath = NULL;
+g_autofree char *compressedpath = NULL;
 virDomainObjPtr vm;
-char *name = NULL;
+g_autofree char *name = NULL;
 int ret = -1;
 
 virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE |
@@ -3604,8 +3596,6 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int 
flags)
 
  cleanup:
 virDomainObjEndAPI();
-VIR_FREE(name);
-VIR_FREE(compressedpath);
 
 return ret;
 }
@@ -3615,7 +3605,7 @@ qemuDomainManagedSaveLoad(virDomainObjPtr vm,
   void *opaque)
 {
 virQEMUDriverPtr driver = opaque;
-char *name;
+g_autofree char *name = NULL;
 int ret = -1;
 
 virObjectLock(vm);
@@ -3628,7 +3618,6 @@ qemuDomainManagedSaveLoad(virDomainObjPtr vm,
 ret = 0;
  cleanup:
 

[libvirt] [PATCH v1] qemu_driver: improve comparison/baseline error reporting

[Collin Walling]

Providing an erroneous CPU definition in the XML file provided to the
hypervisor-cpu-compare/baseline command will result in a verbose
internal error. Let's add some sanity checking before executing the QMP
commands to provide a cleaner message if something is wrong with the
CPU model or features.

An internal error will still appear for other messages originating from
QEMU e.g. if a newer feature is not available for an older CPU model.

Signed-off-by: Collin Walling 
---
 src/qemu/qemu_driver.c | 57 ++
 1 file changed, 57 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2e422b5..a4f916b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -13645,6 +13645,52 @@ qemuConnectCompareCPU(virConnectPtr conn,
 }
 
 
+static int
+qemuConnectCheckCPUModel(qemuMonitorPtr mon,
+ virCPUDefPtr cpu)
+{
+qemuMonitorCPUModelInfoPtr modelInfo = NULL;
+qemuMonitorCPUModelExpansionType type;
+size_t i, j;
+int ret = -1;
+
+/* Collect CPU model name and features known by QEMU */
+type = QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL;
+if (qemuMonitorGetCPUModelExpansion(mon, type, cpu, true,
+false, ) < 0)
+goto cleanup;
+
+/* Sanity check CPU model */
+if (!modelInfo) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Unknown CPU model: %s"), cpu->model);
+goto cleanup;
+}
+
+/* Sanity check CPU features */
+for (i = 0; i < cpu->nfeatures; i++) {
+const char *feat = cpu->features[i].name;
+
+for (j = 0; j < modelInfo->nprops; j++) {
+const char *prop = modelInfo->props[j].name;
+if (STREQ(feat, prop))
+break;
+}
+
+if (j == modelInfo->nprops) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("Unknown CPU feature: %s"), feat);
+goto cleanup;
+}
+}
+ret = 0;
+
+ cleanup:
+qemuMonitorCPUModelInfoFree(modelInfo);
+return ret;
+}
+
+
 static virCPUCompareResult
 qemuConnectCPUModelComparison(virQEMUCapsPtr qemuCaps,
   const char *libDir,
@@ -13665,6 +13711,11 @@ qemuConnectCPUModelComparison(virQEMUCapsPtr qemuCaps,
 if (qemuProcessQMPStart(proc) < 0)
 goto cleanup;
 
+if (qemuConnectCheckCPUModel(proc->mon, cpu_a) < 0 ||
+qemuConnectCheckCPUModel(proc->mon, cpu_b) < 0) {
+goto cleanup;
+}
+
 if (qemuMonitorGetCPUModelComparison(proc->mon, cpu_a, cpu_b, ) < 0)
 goto cleanup;
 
@@ -13863,6 +13914,9 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps,
 if (qemuProcessQMPStart(proc) < 0)
 goto cleanup;
 
+if (qemuConnectCheckCPUModel(proc->mon, cpus[0]) < 0)
+goto cleanup;
+
 if (VIR_ALLOC(baseline) < 0)
 goto cleanup;
 
@@ -13870,6 +13924,9 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps,
 goto cleanup;
 
 for (i = 1; i < ncpus; i++) {
+if (qemuConnectCheckCPUModel(proc->mon, cpus[i]) < 0)
+goto cleanup;
+
 if (qemuMonitorGetCPUModelBaseline(proc->mon, baseline,
cpus[i], ) < 0)
 goto cleanup;
-- 
2.7.4

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

Re: [libvirt] [RFC] Accepting PRs/MRs for libvirt on GitHub/GitLab

[Daniel Henrique Barboza]

On 10/16/19 9:22 AM, Andrea Bolognani wrote:

As we look to make the libvirt project easier to contribute to, one
fact that certainly comes to mind is that we only accept patches via
the mailing list. While the core developers are comfortable with the
email-based workflow and swear by it, many perspective contributors
are used to the PR/MR workflow common to many Open Source projects
these days, and similarly swear by it.

If we look at the PRs submitted on GitHub against the libvirt mirror
repository[1], there's just three of them per year on average, which
is arguably not a lot; however, it should be noted that each
repository also carries a fairly loud "PULL REQUESTS ARE IGNORED"
message right at the top, and it's not unlikely that a number of
perspective contributors simply lost interest after seeing it.

As a way to make contributions easier without forcing core developers
to give up their current workflow or making the project dependent on
a third-party provider, I suggest we adopt a hybrid approach.

First of all, we'd remove the ominous message from GitHub mirror
repositories (interestingly, the same is not present on GitLab).

Then, we'd starting accepting PRs/MRs. The way we'd handle them is
that, when one comes in, one among the handful of core developers who
volunteered to do so would review the patches on the respective
platform, working with the submitter to get it into shape just like
they would do on the mailing list; once the series is finally ready
to be merged, the core developer would update the PR/MR as necessary,
for example picking up R-bs or fixing the kind of trivial issues that
usually don't warrant a repost, and then push to master as usual.


This would mean that there will be patches that will get accepted
without the ML being aware of. The core developer (or the author
itself) would need to send the revised patches to the ML before
pushing to make people aware of it before pushing to master, IMO.

I have a 2 year experience maintaining a now defunct project in
Github (a KVM web management tool called Kimchi) in which we used a
hybrid approach like I think you're suggesting, with mailing list, people
opening bugs in Github + Github PRs. It was annoying - people will 
inevitably

discuss issues using the Github tracking system, which means that you'll
have to subscribe via email to Github notifications if you didn't want 
to miss

out. It was common for people that used the ML to start discussions that
were already happening in the Web, and vice-versa.

All that to make a case against Libvirt having additional ways of
communication. I don't mind pull requests from Github/Gitlab as long as the
ML is made aware of, before the patches are accepted. But people bringing
up discussions in the ML and Github/Gitlab scales poorly, in my experience.


DHB




More in detail: GitHub and GitLab have a feature that allows project
members to update PRs/MRs: basically the way it works is that, if the
PR/MR was created by the user 'foo' from their branch 'bar', the
libvirt developer is allowed to (force-)push to the foo/libvirt/bar
branch, and doing so updates the corresponding PR/MR; after that, if
the updated branch is locally merged into master and master is pushed
to the libvirt.org repo, once it gets mirrored GitHub/GitLab will
recognize that the commit IDs match and automatically mark the PR/MR
as merged. I have tested this behavior on both platforms (minus the
mirroring part) with Martin's help.

One last important bit. In the spirit of not requiring core
developers to alter their workflow, the developer who reviewed the
patches on GitHub/GitLab will also be responsible to format them to
the mailing list after merging them: this way, even those who don't
have a GitHub/GitLab account will get a chance to take notice of the
code changes and weigh in. Unlike what we're used to, this feedback
will come after the fact, but assuming that issues are spotted only
at that point we can either push the relevant fixes as follow-up
commits or even revert the series outright, so I don't feel like it
would be a massive problem overall.

Thoughts?


[1] https://github.com/libvirt/libvirt/pulls?q=is:pr+is:closed


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

Re: [libvirt] [RFC] Accepting PRs/MRs for libvirt on GitHub/GitLab

[Daniel P . Berrangé]

On Wed, Oct 16, 2019 at 05:03:31PM +0200, Ján Tomko wrote:
> On Wed, Oct 16, 2019 at 02:22:56PM +0200, Andrea Bolognani wrote:
> > First of all, we'd remove the ominous message from GitHub mirror
> > repositories (interestingly, the same is not present on GitLab).
> > 
> 
> Well if you're using GitLab then you're already aware of the fact that
> not everything is hosted on GitHub.
> 
> > Then, we'd starting accepting PRs/MRs. The way we'd handle them is
> > that, when one comes in, one among the handful of core developers who
> > volunteered to do so would review the patches on the respective
> > platform, working with the submitter to get it into shape just like
> > they would do on the mailing list; once the series is finally ready
> > to be merged, the core developer would update the PR/MR as necessary,
> > for example picking up R-bs or fixing the kind of trivial issues that
> > usually don't warrant a repost, and then push to master as usual.
> > 
> > More in detail: GitHub and GitLab have a feature that allows project
> > members to update PRs/MRs: basically the way it works is that, if the
> > PR/MR was created by the user 'foo' from their branch 'bar', the
> > libvirt developer is allowed to (force-)push to the foo/libvirt/bar
> > branch, and doing so updates the corresponding PR/MR; after that, if
> > the updated branch is locally merged into master and master is pushed
> > to the libvirt.org repo, once it gets mirrored GitHub/GitLab will
> > recognize that the commit IDs match and automatically mark the PR/MR
> > as merged. I have tested this behavior on both platforms (minus the
> > mirroring part) with Martin's help.
> > 
> > One last important bit. In the spirit of not requiring core
> > developers to alter their workflow, the developer who reviewed the
> > patches on GitHub/GitLab will also be responsible to format them to
> > the mailing list after merging them: this way, even those who don't
> > have a GitHub/GitLab account will get a chance to take notice of the
> > code changes and weigh in. Unlike what we're used to, this feedback
> > will come after the fact, but assuming that issues are spotted only
> > at that point we can either push the relevant fixes as follow-up
> > commits or even revert the series outright, so I don't feel like it
> > would be a massive problem overall.
> > 
> 
> Here it deviates from the usual mailing list workflow where the patch
> has (in theory) a chance to be seen by all the developers.
> 
> But given that the requests will probably
> a) be close to trivial
> b) seen by a group of developers, not just one

I wouldn't expect the changes to be trivial. Current stuff
is trivial largely because we tell people not to open merge
requests. If we adopt use of web based review, then expect
people to submit non-trivial patches. I would do so myself
for example. Thus I think we must make a clean switchover
from email to a single web based tool.
 
> sending it to the mailing list after it's pushed is better treatment
> than our language bindings get.

The situation with our language bindings has never felt very
comfortable to me. Except for Python, they rarely get posted
for review. Using a web based tool merge requests for language
bindings would be a step forward for transparency in what
we're submitting for them. If nothing else, it will ensure
we can wire up CI to run gating pushes to master to avoid
broken builds.

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

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

Re: [libvirt] [PATCH] qemu: caps: Use unique key for domCaps caching

[Jonathon Jongsma]

On Wed, 2019-10-16 at 09:12 -0300, Daniel Henrique Barboza wrote:
> 
> On 10/15/19 1:01 PM, Cole Robinson wrote:
> > When searching qemuCaps->domCapsCache for existing domCaps data,
> > we check for a matching pair of arch+virttype+machine+emulator.
> > However
> > for the hash table key we only use the machine string. So if the
> > cache already contains:
> > 
> >x86_64 + kvm + pc + /usr/bin/qemu-kvm
> > 
> > But a new VM is defined with
> > 
> >x86_64 + qemu + pc + /usr/bin/qemu-kvm
> > 
> > We correctly fail to find matching cached domCaps, but then attempt
> > to use a colliding key with virHashAddEntry
> > 
> > Fix this by building a hash key from the 4 values, not just machine
> > 
> > Signed-off-by: Cole Robinson 
> > ---
> > qemu_domain.c validation should be affected, but it is covered up
> > by another bug, fixed here:
> > https://www.redhat.com/archives/libvir-list/2019-October/msg00708.html
> > 
> >   src/qemu/qemu_conf.c | 11 ++-
> >   1 file changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> > index 08cd784054..64ac8cbdd3 100644
> > --- a/src/qemu/qemu_conf.c
> > +++ b/src/qemu/qemu_conf.c
> > @@ -1396,6 +1396,8 @@
> > virQEMUDriverGetDomainCapabilities(virQEMUDriverPtr driver,
> >   domCaps = virHashSearch(domCapsCache,
> >   virQEMUDriverSearchDomcaps, ,
> > NULL);
> >   if (!domCaps) {
> > +VIR_AUTOFREE(char *) key = NULL;
> > +
> 
> "g_autofree char *key = NULL;" instead of VIR_AUTOFREE for extra
> karma
> points. Everything else LGTM.
> 
> 
> 
> Reviewed-by: Daniel Henrique Barboza 
> 


Looks good to me as well. But in keeping with the theme, we could also
use g_strdup_printf() below instead of virAsprintf(). That is one of
the suggestions made in the recent updates to our hacking documentation
from Daniel's glib patch series.

Reviewed-by: Jonathon Jongsma 


> 
> 
> >   /* hash miss, build new domcaps */
> >   if (!(domCaps = virDomainCapsNew(data.path, data.machine,
> >data.arch,
> > data.virttype)))
> > @@ -1406,7 +1408,14 @@
> > virQEMUDriverGetDomainCapabilities(virQEMUDriverPtr driver,
> > cfg->firmwares, cfg-
> > >nfirmwares) < 0)
> >   return NULL;
> >   
> > -if (virHashAddEntry(domCapsCache, machine, domCaps) < 0)
> > +if (virAsprintf(, "%d:%d:%s:%s",
> > +data.arch,
> > +data.virttype,
> > +NULLSTR(data.machine),
> > +NULLSTR(data.path)) < 0)
> > +return NULL;
> > +
> > +if (virHashAddEntry(domCapsCache, key, domCaps) < 0)
> >   return NULL;
> >   }
> >   

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

Re: [libvirt] [RFC] Accepting PRs/MRs for libvirt on GitHub/GitLab

[Daniel P . Berrangé]

On Wed, Oct 16, 2019 at 02:22:56PM +0200, Andrea Bolognani wrote:
> As we look to make the libvirt project easier to contribute to, one
> fact that certainly comes to mind is that we only accept patches via
> the mailing list. While the core developers are comfortable with the
> email-based workflow and swear by it, many perspective contributors
> are used to the PR/MR workflow common to many Open Source projects
> these days, and similarly swear by it.

I still think email is a more productive workflow in many respects,
but there's no denying the fact that web based workflow has come to
dominate the open source development world. Projects that are as
large, or larger, than libvirt are successfully using web based
workflows, so it is not credible to claim it won't work for libvirt
anymore.

The challenges are all around the human factors. We have 15 years
of using a email workflow for libvirt, so that's what our current
regular contributors are all used to, and have optimized our daily
routine around. Changing people's daily routines is always disruptive
and met with resistance.

If we're to reduce the on-ramp / learning curve to make libvirt more
accessible for new contributors I think switching libvirt to use an
web based tool is inevitable & desirable.

Beyond that there are a number of ways it would benefit existing
contributors too. Over the past 5 years there have been many times
when mailman has just given up and stopped sending mails to the
libvirt list. Some of these outages have lasted as long as an
entire week & been quite risruptive to us. Getting anyone to care
to fix the outages is challenging as few other people are impacted
to the same degree as libvirt is with this redhat.com mailman.

All to often patches from contributors get lost in the torrent.
This can happen in web based tools too. The difference is that
the web tools have much better facilities for identifying these
missed patches & reporting on these problems, or helping organize
patches to minimized missed stuff.


> If we look at the PRs submitted on GitHub against the libvirt mirror
> repository[1], there's just three of them per year on average, which
> is arguably not a lot; however, it should be noted that each
> repository also carries a fairly loud "PULL REQUESTS ARE IGNORED"
> message right at the top, and it's not unlikely that a number of
> perspective contributors simply lost interest after seeing it.

Yep, that's certainly not an encouraging first impression. 

> As a way to make contributions easier without forcing core developers
> to give up their current workflow or making the project dependent on
> a third-party provider, I suggest we adopt a hybrid approach.

> First of all, we'd remove the ominous message from GitHub mirror
> repositories (interestingly, the same is not present on GitLab).

That's an accident in GitLab config.

> 
> Then, we'd starting accepting PRs/MRs. The way we'd handle them is
> that, when one comes in, one among the handful of core developers who
> volunteered to do so would review the patches on the respective
> platform, working with the submitter to get it into shape just like
> they would do on the mailing list; once the series is finally ready
> to be merged, the core developer would update the PR/MR as necessary,
> for example picking up R-bs or fixing the kind of trivial issues that
> usually don't warrant a repost, and then push to master as usual.

I really don't like this hybrid approach for several reasons.

Splitting attention between the web based tool and email list, with
only a subset of people volunteering to use the web tool is really
not attractive. Anyone who only pays attention to one of the two
places is going to be missing out on what's being submitted and
merging until it has already merged.

I think it is fundamental principal that whatever workflow we use
for patch submission & review *must* be one that is universally
used by everyone.

There's two tools being discussed here - both GitHub and GitLab.
Splitting attention between email and a web based tool is bad,
but splitting attention between email and two web based tools
is even worse.

Finally I have general desire to *NOT* designate GitHub as an
official part of the libvirt workflow on the basis that it is
a closed source tool. Yes, we are already using it, but it is
largely an ancillary service working as a passive backup service
for our git repos, not a core part of our workflow. I don't want
to elevate it to be a core part.

> One last important bit. In the spirit of not requiring core
> developers to alter their workflow, the developer who reviewed the
> patches on GitHub/GitLab will also be responsible to format them to
> the mailing list after merging them: this way, even those who don't
> have a GitHub/GitLab account will get a chance to take notice of the
> code changes and weigh in. Unlike what we're used to, this feedback
> will come after the fact, but assuming that issues are spotted only
> at that 

Re: [libvirt] [PATCH v2 11/31] qemu: Flatten qemuMonitorCPUDefs.cpus

[Jiri Denemark]

On Wed, Oct 16, 2019 at 13:15:19 +0200, Ján Tomko wrote:
> On Tue, Oct 15, 2019 at 05:34:47PM +0200, Jiri Denemark wrote:
> >Let's store qemuMonitorCPUDefInfo directly in the array of CPUs in
> >qemuMonitorCPUDefs rather then using an array of pointers.
> >
> >Signed-off-by: Jiri Denemark 
> >Reviewed-by: Ján Tomko 
> >---
> >
> >Notes:
> >Version 2:
> >- trivial rebase
> >
> > src/qemu/qemu_capabilities.c | 14 +++---
> > src/qemu/qemu_monitor.c  |  5 ++---
> > src/qemu/qemu_monitor.h  |  2 +-
> > src/qemu/qemu_monitor_json.c |  7 +--
> > tests/qemumonitorjsontest.c  |  8 
> > 5 files changed, 15 insertions(+), 21 deletions(-)
> >
> 
> Reviewed-by: Ján Tomko 
> 
> @@ -3573,7 +3572,7 @@ qemuMonitorCPUDefsNew(size_t count)
>  g_autoptr(qemuMonitorCPUDefs) defs = NULL;
> 
>  defs = g_new0(qemuMonitorCPUDefs, 1);
> -defs->cpus = g_new0(qemuMonitorCPUDefInfoPtr, count);
> +defs->cpus = g_new0(qemuMonitorCPUDefInfo, count);
>  defs->ncpus = count;
>  return g_steal_pointer();
>  }

Looks like a change in something that doesn't exist yet :-)


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

Re: [libvirt] [PATCH v2 31/31] qemu: Store default CPU in domain XML

[Ján Tomko]

On Tue, Oct 15, 2019 at 05:35:07PM +0200, Jiri Denemark wrote:

When starting a domain without a CPU model specified in the domain XML,
QEMU will choose a default one. Which is fine unless the domain gets
migrated to another host because libvirt doesn't perform any CPU ABI
checks and the virtual CPU provided by QEMU on the destination host can
differ from the one on the source host.

With QEMU 4.2.0 we can probe for the default CPU model used by QEMU for
a particular machine type and store it in the domain XML. This way the
chosen CPU model is more visible to users and libvirt will make sure
the guest will see the exact same CPU after migration.

Architecture specific notes
- aarch64: We only set the default CPU for TCG domains as KVM requires
 -cpu host to work.

- ppc64: (to be checked with QEMU developers) Default CPU data reported
 by QEMU is unusable for KVM. QEMU would effectively use -cpu host by
 default and it even rewrites typename of the current host CPU model to
 host-powerpc64-cpu. The default CPU type reported by query-machines is
 still power*-powerpc64-cpu, which may not even exist due to the
 changed typename. For example, on a Power8 host, power8 CPU model is
 of host-powerpc64-cpu type (according to query-cpu-definitions), while
 the default CPU type name for pseries-3.1 machine type is reported as
 power8_v2.0-powerpc64-cpu. What's even worse, pseries-4.2 says the
 default CPU type is power9_v2.0-powerpc64-cpu, which cannot be started
 on older host at all. So again, we only set the default CPU for TCG
 domains.

- s390x: (to be checked with QEMU developers) The default CPU is said to
 be "qemu", which works fine for TCG domains, but it doesn't work on
 KVM because QEMU complains that some features requested in the CPU
 model are not available.

- x86_64: The default CPU model (qemu64) is not runnable on any host
 with KVM, but in contrast to s390x QEMU just disables unavailable
 features and starts happily.

https://bugzilla.redhat.com/show_bug.cgi?id=1598151
https://bugzilla.redhat.com/show_bug.cgi?id=1598162
---

Notes:
   This patch should not be merged yet as we need to confirm what to do
   on s390x and ppc64 architectures.

   Version 2:
   - new test cases

src/qemu/qemu_domain.c| 56 +++
...fault-cpu-tcg-virt-4.2.aarch64-latest.args |  1 +
.../disk-cache.x86_64-latest.args |  1 +
.../disk-cdrom-network.x86_64-latest.args |  1 +
.../disk-cdrom-tray.x86_64-latest.args|  1 +
.../disk-copy_on_read.x86_64-latest.args  |  1 +
.../disk-detect-zeroes.x86_64-latest.args |  1 +
.../disk-floppy-q35-2_11.x86_64-latest.args   |  1 +
.../disk-floppy-q35-2_9.x86_64-latest.args|  1 +
.../os-firmware-bios.x86_64-latest.args   |  1 +
...os-firmware-efi-secboot.x86_64-latest.args |  1 +
.../os-firmware-efi.x86_64-latest.args|  1 +
...ault-cpu-tcg-pseries-2.7.ppc64-latest.args |  1 +
...ault-cpu-tcg-pseries-3.1.ppc64-latest.args |  1 +
...ault-cpu-tcg-pseries-4.2.ppc64-latest.args |  1 +
...t-cpu-tcg-ccw-virtio-4.2.s390x-latest.args |  1 +
.../tpm-emulator-tpm2-enc.x86_64-latest.args  |  1 +
.../tpm-emulator-tpm2.x86_64-latest.args  |  1 +
.../tpm-emulator.x86_64-latest.args   |  1 +
.../tseg-explicit-size.x86_64-latest.args |  1 +
.../vhost-vsock-auto.x86_64-latest.args   |  1 +
.../vhost-vsock.x86_64-latest.args|  1 +
...-default-cpu-kvm-pc-4.2.x86_64-latest.args |  1 +
...default-cpu-kvm-q35-4.2.x86_64-latest.args |  1 +
...-default-cpu-tcg-pc-4.2.x86_64-latest.args |  1 +
...default-cpu-tcg-q35-4.2.x86_64-latest.args |  1 +
...efault-cpu-tcg-virt-4.2.aarch64-latest.xml |  3 +
.../os-firmware-bios.x86_64-latest.xml|  3 +
.../os-firmware-efi-secboot.x86_64-latest.xml |  3 +
.../os-firmware-efi.x86_64-latest.xml |  3 +
...fault-cpu-tcg-pseries-2.7.ppc64-latest.xml |  3 +
...fault-cpu-tcg-pseries-3.1.ppc64-latest.xml |  3 +
...fault-cpu-tcg-pseries-4.2.ppc64-latest.xml |  3 +
...lt-cpu-tcg-ccw-virtio-4.2.s390x-latest.xml |  3 +
.../tpm-emulator-tpm2-enc.x86_64-latest.xml   |  3 +
.../tpm-emulator-tpm2.x86_64-latest.xml   |  3 +
.../tpm-emulator.x86_64-latest.xml|  3 +
.../tpm-passthrough-crb.x86_64-latest.xml |  3 +
.../tpm-passthrough.x86_64-latest.xml |  3 +
...4-default-cpu-kvm-pc-4.2.x86_64-latest.xml |  3 +
...-default-cpu-kvm-q35-4.2.x86_64-latest.xml |  3 +
...4-default-cpu-tcg-pc-4.2.x86_64-latest.xml |  3 +
...-default-cpu-tcg-q35-4.2.x86_64-latest.xml |  3 +
43 files changed, 132 insertions(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 9dcba4ef38..438f003186 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4470,6 +4470,59 @@ qemuDomainDefVcpusPostParse(virDomainDefPtr def)
}


+static int
+qemuDomainDefSetDefaultCPU(virDomainDefPtr def,
+   virQEMUCapsPtr qemuCaps)
+{
+VIR_AUTOPTR(virCPUDef) newCPU = NULL;


g_autoptr


+virCPUDefPtr cpu = def->cpu;

Re: [libvirt] [PATCH v2 11/31] qemu: Flatten qemuMonitorCPUDefs.cpus

[Ján Tomko]

On Wed, Oct 16, 2019 at 05:46:24PM +0200, Jiri Denemark wrote:

On Wed, Oct 16, 2019 at 13:15:19 +0200, Ján Tomko wrote:

On Tue, Oct 15, 2019 at 05:34:47PM +0200, Jiri Denemark wrote:
>Let's store qemuMonitorCPUDefInfo directly in the array of CPUs in
>qemuMonitorCPUDefs rather then using an array of pointers.
>
>Signed-off-by: Jiri Denemark 
>Reviewed-by: Ján Tomko 
>---
>
>Notes:
>Version 2:
>- trivial rebase
>
> src/qemu/qemu_capabilities.c | 14 +++---
> src/qemu/qemu_monitor.c  |  5 ++---
> src/qemu/qemu_monitor.h  |  2 +-
> src/qemu/qemu_monitor_json.c |  7 +--
> tests/qemumonitorjsontest.c  |  8 
> 5 files changed, 15 insertions(+), 21 deletions(-)
>

Reviewed-by: Ján Tomko 

@@ -3573,7 +3572,7 @@ qemuMonitorCPUDefsNew(size_t count)
 g_autoptr(qemuMonitorCPUDefs) defs = NULL;

 defs = g_new0(qemuMonitorCPUDefs, 1);
-defs->cpus = g_new0(qemuMonitorCPUDefInfoPtr, count);
+defs->cpus = g_new0(qemuMonitorCPUDefInfo, count);
 defs->ncpus = count;
 return g_steal_pointer();
 }


Looks like a change in something that doesn't exist yet :-)



Yes, it's exactly what it looks like! :)

Jano


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

Re: [libvirt] [PATCH v2 25/31] qemu: Use VIR_AUTOUNREF in qemuDomainDefPostParse

[Ján Tomko]

On Tue, Oct 15, 2019 at 05:35:01PM +0200, Jiri Denemark wrote:

Signed-off-by: Jiri Denemark 
Reviewed-by: Ján Tomko 
---

Notes:
   Version 2:
   - no change

src/qemu/qemu_domain.c | 28 
1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c638077aa8..9dcba4ef38 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4617,55 +4617,51 @@ qemuDomainDefPostParse(virDomainDefPtr def,
   void *parseOpaque)
{
virQEMUDriverPtr driver = opaque;
-virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver);


g_autoptr

Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v2 21/31] qemu: Store typename from query-cpu-definitions in qemuCaps

[Ján Tomko]

On Tue, Oct 15, 2019 at 05:34:57PM +0200, Jiri Denemark wrote:

We need to create a mapping between CPU model names and their
corresponding QOM types.

Signed-off-by: Jiri Denemark 
---

Notes:
   Version 2:
   - ignore empty typename strings



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v2 26/31] conf: Define VIR_AUTOPTR for virCPUDef

[Ján Tomko]

On Tue, Oct 15, 2019 at 05:35:02PM +0200, Jiri Denemark wrote:

Signed-off-by: Jiri Denemark 
---

Notes:
   Version 2:
   - no change

src/conf/cpu_conf.h | 1 +
1 file changed, 1 insertion(+)

diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h
index 30904fab95..230e75f077 100644
--- a/src/conf/cpu_conf.h
+++ b/src/conf/cpu_conf.h
@@ -151,6 +151,7 @@ virCPUDefFreeModel(virCPUDefPtr def);

void
virCPUDefFree(virCPUDefPtr def);
+VIR_DEFINE_AUTOPTR_FUNC(virCPUDef, virCPUDefFree);


G_DEFINE_AUTOPTR_CLEANUP_FUNC

Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v2 28/31] qemuxml2*test: Add test cases for default CPU models on ppc64

[Ján Tomko]

On Tue, Oct 15, 2019 at 05:35:04PM +0200, Jiri Denemark wrote:

Signed-off-by: Jiri Denemark 
---

Notes:
   Version 2:
   - new patch

...ault-cpu-kvm-pseries-2.7.ppc64-latest.args | 38 
.../ppc64-default-cpu-kvm-pseries-2.7.xml | 22 +
...ault-cpu-kvm-pseries-3.1.ppc64-latest.args | 38 
.../ppc64-default-cpu-kvm-pseries-3.1.xml | 22 +
...ault-cpu-kvm-pseries-4.2.ppc64-latest.args | 38 
.../ppc64-default-cpu-kvm-pseries-4.2.xml | 22 +
...ault-cpu-tcg-pseries-2.7.ppc64-latest.args | 38 
.../ppc64-default-cpu-tcg-pseries-2.7.xml | 22 +
...ault-cpu-tcg-pseries-3.1.ppc64-latest.args | 38 
.../ppc64-default-cpu-tcg-pseries-3.1.xml | 22 +
...ault-cpu-tcg-pseries-4.2.ppc64-latest.args | 38 
.../ppc64-default-cpu-tcg-pseries-4.2.xml | 22 +
tests/qemuxml2argvtest.c  |  6 +++
...fault-cpu-kvm-pseries-2.7.ppc64-latest.xml | 45 +++
...fault-cpu-kvm-pseries-3.1.ppc64-latest.xml | 45 +++
...fault-cpu-kvm-pseries-4.2.ppc64-latest.xml | 45 +++
...fault-cpu-tcg-pseries-2.7.ppc64-latest.xml | 45 +++
...fault-cpu-tcg-pseries-3.1.ppc64-latest.xml | 45 +++
...fault-cpu-tcg-pseries-4.2.ppc64-latest.xml | 45 +++
tests/qemuxml2xmltest.c   |  6 +++
20 files changed, 642 insertions(+)
create mode 100644 
tests/qemuxml2argvdata/ppc64-default-cpu-kvm-pseries-2.7.ppc64-latest.args
create mode 100644 tests/qemuxml2argvdata/ppc64-default-cpu-kvm-pseries-2.7.xml
create mode 100644 
tests/qemuxml2argvdata/ppc64-default-cpu-kvm-pseries-3.1.ppc64-latest.args
create mode 100644 tests/qemuxml2argvdata/ppc64-default-cpu-kvm-pseries-3.1.xml
create mode 100644 
tests/qemuxml2argvdata/ppc64-default-cpu-kvm-pseries-4.2.ppc64-latest.args
create mode 100644 tests/qemuxml2argvdata/ppc64-default-cpu-kvm-pseries-4.2.xml
create mode 100644 
tests/qemuxml2argvdata/ppc64-default-cpu-tcg-pseries-2.7.ppc64-latest.args
create mode 100644 tests/qemuxml2argvdata/ppc64-default-cpu-tcg-pseries-2.7.xml
create mode 100644 
tests/qemuxml2argvdata/ppc64-default-cpu-tcg-pseries-3.1.ppc64-latest.args
create mode 100644 tests/qemuxml2argvdata/ppc64-default-cpu-tcg-pseries-3.1.xml
create mode 100644 
tests/qemuxml2argvdata/ppc64-default-cpu-tcg-pseries-4.2.ppc64-latest.args
create mode 100644 tests/qemuxml2argvdata/ppc64-default-cpu-tcg-pseries-4.2.xml
create mode 100644 
tests/qemuxml2xmloutdata/ppc64-default-cpu-kvm-pseries-2.7.ppc64-latest.xml
create mode 100644 
tests/qemuxml2xmloutdata/ppc64-default-cpu-kvm-pseries-3.1.ppc64-latest.xml
create mode 100644 
tests/qemuxml2xmloutdata/ppc64-default-cpu-kvm-pseries-4.2.ppc64-latest.xml
create mode 100644 
tests/qemuxml2xmloutdata/ppc64-default-cpu-tcg-pseries-2.7.ppc64-latest.xml
create mode 100644 
tests/qemuxml2xmloutdata/ppc64-default-cpu-tcg-pseries-3.1.ppc64-latest.xml
create mode 100644 
tests/qemuxml2xmloutdata/ppc64-default-cpu-tcg-pseries-4.2.ppc64-latest.xml


Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v2 27/31] qemuxml2*test: Add test cases for default CPU models on aarch64

[Ján Tomko]

On Tue, Oct 15, 2019 at 05:35:03PM +0200, Jiri Denemark wrote:

Signed-off-by: Jiri Denemark 
---

Notes:
   Version 2:
   - new patch

...fault-cpu-kvm-virt-4.2.aarch64-latest.args | 38 +++
.../aarch64-default-cpu-kvm-virt-4.2.xml  | 20 ++
...fault-cpu-tcg-virt-4.2.aarch64-latest.args | 38 +++
.../aarch64-default-cpu-tcg-virt-4.2.xml  | 20 ++
tests/qemuxml2argvtest.c  |  3 ++
...efault-cpu-kvm-virt-4.2.aarch64-latest.xml | 38 +++
...efault-cpu-tcg-virt-4.2.aarch64-latest.xml | 38 +++
tests/qemuxml2xmltest.c   |  3 ++
8 files changed, 198 insertions(+)
create mode 100644 
tests/qemuxml2argvdata/aarch64-default-cpu-kvm-virt-4.2.aarch64-latest.args
create mode 100644 tests/qemuxml2argvdata/aarch64-default-cpu-kvm-virt-4.2.xml
create mode 100644 
tests/qemuxml2argvdata/aarch64-default-cpu-tcg-virt-4.2.aarch64-latest.args
create mode 100644 tests/qemuxml2argvdata/aarch64-default-cpu-tcg-virt-4.2.xml
create mode 100644 
tests/qemuxml2xmloutdata/aarch64-default-cpu-kvm-virt-4.2.aarch64-latest.xml
create mode 100644 
tests/qemuxml2xmloutdata/aarch64-default-cpu-tcg-virt-4.2.aarch64-latest.xml



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v2 29/31] qemuxml2*test: Add test cases for default CPU models on s390x

[Ján Tomko]

On Tue, Oct 15, 2019 at 05:35:05PM +0200, Jiri Denemark wrote:

Signed-off-by: Jiri Denemark 
---

Notes:
   Version 2:
   - new patch

...t-cpu-kvm-ccw-virtio-4.2.s390x-latest.args | 31 +++
.../s390-default-cpu-kvm-ccw-virtio-4.2.xml   | 16 ++
...t-cpu-tcg-ccw-virtio-4.2.s390x-latest.args | 31 +++
.../s390-default-cpu-tcg-ccw-virtio-4.2.xml   | 16 ++
tests/qemuxml2argvtest.c  |  2 ++
...lt-cpu-kvm-ccw-virtio-4.2.s390x-latest.xml | 23 ++
...lt-cpu-tcg-ccw-virtio-4.2.s390x-latest.xml | 23 ++
tests/qemuxml2xmltest.c   |  2 ++
8 files changed, 144 insertions(+)
create mode 100644 
tests/qemuxml2argvdata/s390-default-cpu-kvm-ccw-virtio-4.2.s390x-latest.args
create mode 100644 
tests/qemuxml2argvdata/s390-default-cpu-kvm-ccw-virtio-4.2.xml
create mode 100644 
tests/qemuxml2argvdata/s390-default-cpu-tcg-ccw-virtio-4.2.s390x-latest.args
create mode 100644 
tests/qemuxml2argvdata/s390-default-cpu-tcg-ccw-virtio-4.2.xml
create mode 100644 
tests/qemuxml2xmloutdata/s390-default-cpu-kvm-ccw-virtio-4.2.s390x-latest.xml
create mode 100644 
tests/qemuxml2xmloutdata/s390-default-cpu-tcg-ccw-virtio-4.2.s390x-latest.xml



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v2 22/31] qemu: Probe for default CPU types

[Ján Tomko]

On Tue, Oct 15, 2019 at 05:34:58PM +0200, Jiri Denemark wrote:

QEMU 4.2.0 will report default CPU types used by each machine type and
we will want to start using it.

Signed-off-by: Jiri Denemark 
---

Notes:
   Version 2:
   - no change except for updated test results

src/qemu/qemu_capabilities.c  | 15 ++-
src/qemu/qemu_monitor.c   |  1 +
src/qemu/qemu_monitor.h   |  1 +
src/qemu/qemu_monitor_json.c  | 12 +++
.../caps_4.2.0.aarch64.xml| 94 +--
.../qemucapabilitiesdata/caps_4.2.0.ppc64.xml | 58 ++--
.../qemucapabilitiesdata/caps_4.2.0.s390x.xml | 28 +++---
.../caps_4.2.0.x86_64.xml | 92 +-
8 files changed, 163 insertions(+), 138 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v2 30/31] qemuxml2*test: Add test cases for default CPU models on x86_64

[Ján Tomko]

On Tue, Oct 15, 2019 at 05:35:06PM +0200, Jiri Denemark wrote:

Signed-off-by: Jiri Denemark 
---

Notes:
   Version 2:
   - new patch

...-default-cpu-kvm-pc-4.2.x86_64-latest.args | 36 +++
.../x86_64-default-cpu-kvm-pc-4.2.xml | 24 
...default-cpu-kvm-q35-4.2.x86_64-latest.args | 41 +
.../x86_64-default-cpu-kvm-q35-4.2.xml| 24 
...-default-cpu-tcg-pc-4.2.x86_64-latest.args | 36 +++
.../x86_64-default-cpu-tcg-pc-4.2.xml | 24 
...default-cpu-tcg-q35-4.2.x86_64-latest.args | 41 +
.../x86_64-default-cpu-tcg-q35-4.2.xml| 24 
tests/qemuxml2argvtest.c  |  4 ++
...4-default-cpu-kvm-pc-4.2.x86_64-latest.xml | 37 
...-default-cpu-kvm-q35-4.2.x86_64-latest.xml | 60 +++
...4-default-cpu-tcg-pc-4.2.x86_64-latest.xml | 37 
...-default-cpu-tcg-q35-4.2.x86_64-latest.xml | 60 +++
tests/qemuxml2xmltest.c   |  4 ++
14 files changed, 452 insertions(+)
create mode 100644 
tests/qemuxml2argvdata/x86_64-default-cpu-kvm-pc-4.2.x86_64-latest.args
create mode 100644 tests/qemuxml2argvdata/x86_64-default-cpu-kvm-pc-4.2.xml
create mode 100644 
tests/qemuxml2argvdata/x86_64-default-cpu-kvm-q35-4.2.x86_64-latest.args
create mode 100644 tests/qemuxml2argvdata/x86_64-default-cpu-kvm-q35-4.2.xml
create mode 100644 
tests/qemuxml2argvdata/x86_64-default-cpu-tcg-pc-4.2.x86_64-latest.args
create mode 100644 tests/qemuxml2argvdata/x86_64-default-cpu-tcg-pc-4.2.xml
create mode 100644 
tests/qemuxml2argvdata/x86_64-default-cpu-tcg-q35-4.2.x86_64-latest.args
create mode 100644 tests/qemuxml2argvdata/x86_64-default-cpu-tcg-q35-4.2.xml
create mode 100644 
tests/qemuxml2xmloutdata/x86_64-default-cpu-kvm-pc-4.2.x86_64-latest.xml
create mode 100644 
tests/qemuxml2xmloutdata/x86_64-default-cpu-kvm-q35-4.2.x86_64-latest.xml
create mode 100644 
tests/qemuxml2xmloutdata/x86_64-default-cpu-tcg-pc-4.2.x86_64-latest.xml
create mode 100644 
tests/qemuxml2xmloutdata/x86_64-default-cpu-tcg-q35-4.2.x86_64-latest.xml



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v2 23/31] qemu: Introduce virQEMUCapsGetMachineDefaultCPU

[Ján Tomko]

On Tue, Oct 15, 2019 at 05:34:59PM +0200, Jiri Denemark wrote:

Signed-off-by: Jiri Denemark 
---

Notes:
   Version 2:
   - fix crash with CPUs without type

src/qemu/qemu_capabilities.c | 36 
src/qemu/qemu_capabilities.h |  3 +++
2 files changed, 39 insertions(+)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v2 24/31] qemu: Drop unused virQEMUCapsGetDefaultMachine

[Ján Tomko]

On Tue, Oct 15, 2019 at 05:35:00PM +0200, Jiri Denemark wrote:

Signed-off-by: Jiri Denemark 
---

Notes:
   Version 2:
   - no change

src/qemu/qemu_capabilities.c | 12 
src/qemu/qemu_capabilities.h |  1 -
2 files changed, 13 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v2 20/31] conf: Drop virDomainCapsCPUModelsAddSteal

[Ján Tomko]

On Tue, Oct 15, 2019 at 05:34:56PM +0200, Jiri Denemark wrote:

Both virDomainCapsCPUModelsAdd and virDomainCapsCPUModelsAddSteal are so
simple we can just squash the code in a single function.

Signed-off-by: Jiri Denemark 
---

Notes:
   Version 2:
   - no change

src/conf/domain_capabilities.c | 33 ++---
src/conf/domain_capabilities.h |  4 
src/libvirt_private.syms   |  1 -
3 files changed, 10 insertions(+), 28 deletions(-)

diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
index a13463a6e7..dadb2a1183 100644
--- a/src/conf/domain_capabilities.c
+++ b/src/conf/domain_capabilities.c
@@ -185,27 +185,6 @@ virDomainCapsCPUModelsCopy(virDomainCapsCPUModelsPtr old)
}


-int
-virDomainCapsCPUModelsAddSteal(virDomainCapsCPUModelsPtr cpuModels,
-   char **name,
-   virDomainCapsCPUUsable usable,
-   char ***blockers)
-{
-if (VIR_RESIZE_N(cpuModels->models, cpuModels->nmodels_max,
- cpuModels->nmodels, 1) < 0)
-return -1;
-
-cpuModels->models[cpuModels->nmodels].usable = usable;
-VIR_STEAL_PTR(cpuModels->models[cpuModels->nmodels].name, *name);
-
-if (blockers)
-VIR_STEAL_PTR(cpuModels->models[cpuModels->nmodels].blockers, 
*blockers);
-
-cpuModels->nmodels++;
-return 0;
-}
-
-
int
virDomainCapsCPUModelsAdd(virDomainCapsCPUModelsPtr cpuModels,
  const char *name,
@@ -214,6 +193,7 @@ virDomainCapsCPUModelsAdd(virDomainCapsCPUModelsPtr 
cpuModels,
{
VIR_AUTOFREE(char *) nameCopy = NULL;
VIR_AUTOSTRINGLIST blockersCopy = NULL;
+virDomainCapsCPUModelPtr cpu;

if (VIR_STRDUP(nameCopy, name) < 0)
return -1;
@@ -221,10 +201,17 @@ virDomainCapsCPUModelsAdd(virDomainCapsCPUModelsPtr 
cpuModels,
if (virStringListCopy(, (const char **)blockers) < 0)
return -1;

-if (virDomainCapsCPUModelsAddSteal(cpuModels, ,
-   usable, ) < 0)
+if (VIR_RESIZE_N(cpuModels->models, cpuModels->nmodels_max,
+ cpuModels->nmodels, 1) < 0)
return -1;

+cpu = cpuModels->models + cpuModels->nmodels;
+cpuModels->nmodels++;
+
+cpu->usable = usable;
+VIR_STEAL_PTR(cpu->name, nameCopy);
+VIR_STEAL_PTR(cpu->blockers, blockersCopy);


g_steal_pointer


+
return 0;
}



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v2 19/31] conf: Drop unused virDomainCapsCPUModelsFilter

[Ján Tomko]

On Tue, Oct 15, 2019 at 05:34:55PM +0200, Jiri Denemark wrote:

Signed-off-by: Jiri Denemark 
---

Notes:
   Version 2:
   - no change

src/conf/domain_capabilities.c | 33 -
src/conf/domain_capabilities.h |  3 ---
src/libvirt_private.syms   |  1 -
3 files changed, 37 deletions(-)



Beautiful :')

Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v2 18/31] qemu: Switch qemuCaps to use qemuMonitorCPUDefs

[Ján Tomko]

On Tue, Oct 15, 2019 at 05:34:54PM +0200, Jiri Denemark wrote:

We will need to keep some QEMU-specific data for each CPU model
supported by a QEMU binary. Instead of complicating the generic
virDomainCapsCPUModelsPtr, we can just directly store
qemuMonitorCPUDefsPtr returned by the capabilities probing code.

Signed-off-by: Jiri Denemark 
---

Notes:
   Version 2:
   - adapted to changes made by the new patches

src/qemu/qemu_capabilities.c | 109 +--
1 file changed, 53 insertions(+), 56 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 436d65f578..a274cef120 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -607,8 +607,8 @@ struct _virQEMUCaps {
virArch arch;

virHashTablePtr domCapsCache;
-virDomainCapsCPUModelsPtr kvmCPUModels;
-virDomainCapsCPUModelsPtr tcgCPUModels;
+qemuMonitorCPUDefsPtr kvmCPUModels;
+qemuMonitorCPUDefsPtr tcgCPUModels;

size_t nmachineTypes;
struct virQEMUCapsMachineType *machineTypes;
@@ -1625,17 +1625,9 @@ virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr 
qemuCaps)

ret->arch = qemuCaps->arch;

-if (qemuCaps->kvmCPUModels) {
-ret->kvmCPUModels = virDomainCapsCPUModelsCopy(qemuCaps->kvmCPUModels);
-if (!ret->kvmCPUModels)
-goto error;
-}
-
-if (qemuCaps->tcgCPUModels) {
-ret->tcgCPUModels = virDomainCapsCPUModelsCopy(qemuCaps->tcgCPUModels);
-if (!ret->tcgCPUModels)
-goto error;
-}
+if (qemuMonitorCPUDefsCopy(>kvmCPUModels, qemuCaps->kvmCPUModels) < 0 
||
+qemuMonitorCPUDefsCopy(>tcgCPUModels, qemuCaps->tcgCPUModels) < 0)
+goto error;

if (virQEMUCapsHostCPUDataCopy(>kvmCPU, >kvmCPU) < 0 ||
virQEMUCapsHostCPUDataCopy(>tcgCPU, >tcgCPU) < 0)
@@ -1861,25 +1853,36 @@ virQEMUCapsAddCPUDefinitions(virQEMUCapsPtr qemuCaps,
 virDomainCapsCPUUsable usable)
{
size_t i;
-virDomainCapsCPUModelsPtr cpus = NULL;
+size_t start;
+qemuMonitorCPUDefsPtr defs = NULL;

if (type == VIR_DOMAIN_VIRT_KVM && qemuCaps->kvmCPUModels)
-cpus = qemuCaps->kvmCPUModels;
+defs = qemuCaps->kvmCPUModels;
else if (type == VIR_DOMAIN_VIRT_QEMU && qemuCaps->tcgCPUModels)
-cpus = qemuCaps->tcgCPUModels;
+defs = qemuCaps->tcgCPUModels;
+
+if (defs) {
+start = defs->ncpus;

-if (!cpus) {
-if (!(cpus = virDomainCapsCPUModelsNew(count)))
+if (VIR_EXPAND_N(defs->cpus, defs->ncpus, count) < 0)
+return -1;
+} else {
+start = 0;
+
+if (!(defs = qemuMonitorCPUDefsNew(count)))
return -1;

if (type == VIR_DOMAIN_VIRT_KVM)
-qemuCaps->kvmCPUModels = cpus;
+qemuCaps->kvmCPUModels = defs;
else
-qemuCaps->tcgCPUModels = cpus;
+qemuCaps->tcgCPUModels = defs;
}

for (i = 0; i < count; i++) {
-if (virDomainCapsCPUModelsAdd(cpus, name[i], usable, NULL) < 0)
+qemuMonitorCPUDefInfoPtr cpu = defs->cpus + start + i;
+
+cpu->usable = usable;


This fails to compile with my CLang:
   qemu/qemu_capabilities.c:1884:23: error: implicit conversion from 
enumeration type 'virDomainCapsCPUUsable' to different enumeration type 
'virTristateBool' [-Werror,-Wenum-conversion]
   cpu->usable = usable;
   ~ ^~

An explicit cast works:
 cpu->usable = (virTristateBool)usable;
but I forgot what is our preferred spacing for casts again.


+if (VIR_STRDUP(cpu->name, name[i]) < 0)


consider g_strdup


return -1;
}

@@ -3503,7 +3503,7 @@ virQEMUCapsLoadCPUModels(virQEMUCapsPtr qemuCaps,
 xmlXPathContextPtr ctxt,
 virDomainVirtType type)
{
-virDomainCapsCPUModelsPtr cpus = NULL;
+VIR_AUTOPTR(qemuMonitorCPUDefs) defs = NULL;


g_autoptr


VIR_AUTOFREE(xmlNodePtr *) nodes = NULL;
size_t i;
int n;
@@ -3579,11 +3574,13 @@ virQEMUCapsLoadCPUModels(virQEMUCapsPtr qemuCaps,
}
}
}
-
-if (virDomainCapsCPUModelsAddSteal(cpus, , usable, ) < 0)
-return -1;
}

+if (type == VIR_DOMAIN_VIRT_KVM)
+VIR_STEAL_PTR(qemuCaps->kvmCPUModels, defs);
+else
+VIR_STEAL_PTR(qemuCaps->tcgCPUModels, defs);


g_steal_pointer


+
return 0;
}



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v2 15/31] qemu: Introduce virQEMUCapsCPUDefsToModels

[Ján Tomko]

On Tue, Oct 15, 2019 at 05:34:51PM +0200, Jiri Denemark wrote:

The function translates qemuMonitorCPUDefsPtr (used by QEMU caps probing
code) into virDomainCapsCPUModelsPtr used by domain capabilities.

Signed-off-by: Jiri Denemark 
---

Notes:
   Version 2:
   - trivial rebase

src/qemu/qemu_capabilities.c | 71 +---
1 file changed, 42 insertions(+), 29 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index e38ad03ab5..87ac9bacdc 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -1887,6 +1887,41 @@ virQEMUCapsAddCPUDefinitions(virQEMUCapsPtr qemuCaps,
}


+static virDomainCapsCPUModelsPtr
+virQEMUCapsCPUDefsToModels(qemuMonitorCPUDefsPtr defs,
+   const char **modelWhitelist,
+   const char **modelBlacklist)
+{
+VIR_AUTOUNREF(virDomainCapsCPUModelsPtr) cpuModels = NULL;


g_autoptr


+size_t i;
+
+if (!(cpuModels = virDomainCapsCPUModelsNew(defs->ncpus)))
+return NULL;
+
+for (i = 0; i < defs->ncpus; i++) {
+qemuMonitorCPUDefInfoPtr cpu = defs->cpus + i;
+virDomainCapsCPUUsable usable = VIR_DOMCAPS_CPU_USABLE_UNKNOWN;
+
+if (modelWhitelist && !virStringListHasString(modelWhitelist, 
cpu->name))
+continue;
+
+if (modelBlacklist && virStringListHasString(modelBlacklist, 
cpu->name))
+continue;
+
+if (cpu->usable == VIR_TRISTATE_BOOL_YES)
+usable = VIR_DOMCAPS_CPU_USABLE_YES;
+else if (cpu->usable == VIR_TRISTATE_BOOL_NO)
+usable = VIR_DOMCAPS_CPU_USABLE_NO;
+
+if (virDomainCapsCPUModelsAdd(cpuModels, cpu->name,
+  usable, cpu->blockers) < 0)
+return NULL;
+}
+
+VIR_RETURN_PTR(cpuModels);


return g_steal_pointer();

Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v2 17/31] qemu: Split virQEMUCapsFetchCPUModels

[Ján Tomko]

On Tue, Oct 15, 2019 at 05:34:53PM +0200, Jiri Denemark wrote:

Most of the code moved to a new virQEMUCapsFetchCPUDefinitions function
and the existing virQEMUCapsFetchCPUModels just becomes a small wrapper
around virQEMUCapsFetchCPUDefinitions and virQEMUCapsCPUDefsToModels.

Signed-off-by: Jiri Denemark 
---

Notes:
   Version 2:
   - new patch

src/qemu/qemu_capabilities.c | 29 +++--
1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index cdc3a2d4b0..436d65f578 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -2487,15 +2487,15 @@ virQEMUCapsProbeQMPMachineProps(virQEMUCapsPtr qemuCaps,
}


-int
-virQEMUCapsFetchCPUModels(qemuMonitorPtr mon,
-  virArch arch,
-  virDomainCapsCPUModelsPtr *cpuModels)
+static int
+virQEMUCapsFetchCPUDefinitions(qemuMonitorPtr mon,
+   virArch arch,
+   qemuMonitorCPUDefsPtr *cpuDefs)
{
VIR_AUTOPTR(qemuMonitorCPUDefs) defs = NULL;
size_t i;

-*cpuModels = NULL;
+*cpuDefs = NULL;

if (qemuMonitorGetCPUDefinitions(mon, ) < 0)
return -1;
@@ -2524,7 +2524,24 @@ virQEMUCapsFetchCPUModels(qemuMonitorPtr mon,
}
}

-if (!(*cpuModels = virQEMUCapsCPUDefsToModels(defs, NULL, NULL)))
+VIR_STEAL_PTR(*cpuDefs, defs);


g_steal_pointer


+return 0;
+}
+
+
+int
+virQEMUCapsFetchCPUModels(qemuMonitorPtr mon,
+  virArch arch,
+  virDomainCapsCPUModelsPtr *cpuModels)
+{
+VIR_AUTOPTR(qemuMonitorCPUDefs) defs = NULL;


g_autoptr


+
+*cpuModels = NULL;
+
+if (virQEMUCapsFetchCPUDefinitions(mon, arch, ) < 0)
+return -1;
+
+if (defs && !(*cpuModels = virQEMUCapsCPUDefsToModels(defs, NULL, NULL)))
return -1;



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v2 16/31] qemu: Rename virQEMUCaps{Get, Fetch}CPUDefinitions

[Ján Tomko]

On Tue, Oct 15, 2019 at 05:34:52PM +0200, Jiri Denemark wrote:

The functions return virDomainCapsCPUModelsPtr and thus they should be
called *CPUModels for consistency. Functions called *CPUDefinitions will
work on qemuMonitorCPUDefsPtr.

Signed-off-by: Jiri Denemark 
---

Notes:
   Version 2:
   - new patch

src/qemu/qemu_capabilities.c | 28 ++--
src/qemu/qemu_capabilities.h | 14 +++---
src/qemu/qemu_driver.c   |  2 +-
src/qemu/qemu_process.c  |  4 ++--
tests/cputest.c  |  2 +-
5 files changed, 25 insertions(+), 25 deletions(-)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH] build: Update .gitignore for build-aux files

[Daniel P . Berrangé]

On Wed, Oct 16, 2019 at 10:10:48AM -0500, Eric Blake wrote:
> On 10/16/19 9:23 AM, Daniel P. Berrangé wrote:
> > On Wed, Oct 16, 2019 at 10:17:16AM -0400, John Ferlan wrote:
> > > 
> > > 
> > > On 10/16/19 10:08 AM, Daniel P. Berrangé wrote:
> > > > On Wed, Oct 16, 2019 at 08:54:26AM -0400, John Ferlan wrote:
> > > > > Recent changes moved some files to build-aux and created at least
> > > > > one more.
> > > > 
> > > > Really ? I wasn't aware that we'd moved the files listed in
> > > > this change.
> > > > 
> > > 
> > > Well that's where they now show up... I dunno. I had to clean my
> > > environment more than once due to recent changes and these files now
> > > show up in the noted/changed locations.  I also use a clean environment
> > > for my coverity builds every day and this is where they show up.
> > 
> > Oh in fact I see them in the same place build-aux, but git is not
> > reporting them despite not being listed in gitignore.
> > 
> > I wonder if it has some default ignore list its using ?
> 
> When using gnulib's bootstrap, gnulib creates or augments .gitignore as
> needed to cover files that it copies in or which autoconf/automake will
> install.  But as we have been moving away from that, we have to pick up the
> slack and ignore the files ourselves.

I think we're fine to continue relyuing on bootstrap to create this
.gitignore file until we switch to meson, at which point it won't
matter anyway, as we'll isolate everything gnulib does in its own
subdirectory away from the root.

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

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

Re: [libvirt] [PATCH] build: Update .gitignore for build-aux files

[Eric Blake]

On 10/16/19 9:23 AM, Daniel P. Berrangé wrote:

On Wed, Oct 16, 2019 at 10:17:16AM -0400, John Ferlan wrote:



On 10/16/19 10:08 AM, Daniel P. Berrangé wrote:

On Wed, Oct 16, 2019 at 08:54:26AM -0400, John Ferlan wrote:

Recent changes moved some files to build-aux and created at least
one more.


Really ? I wasn't aware that we'd moved the files listed in
this change.



Well that's where they now show up... I dunno. I had to clean my
environment more than once due to recent changes and these files now
show up in the noted/changed locations.  I also use a clean environment
for my coverity builds every day and this is where they show up.


Oh in fact I see them in the same place build-aux, but git is not
reporting them despite not being listed in gitignore.

I wonder if it has some default ignore list its using ?


When using gnulib's bootstrap, gnulib creates or augments .gitignore as 
needed to cover files that it copies in or which autoconf/automake will 
install.  But as we have been moving away from that, we have to pick up 
the slack and ignore the files ourselves.


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

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

Re: [libvirt] [PATCH] build: Update .gitignore for build-aux files

[John Ferlan]

On 10/16/19 10:52 AM, Ján Tomko wrote:
> On Wed, Oct 16, 2019 at 03:23:41PM +0100, Daniel P. Berrangé wrote:
>> On Wed, Oct 16, 2019 at 10:17:16AM -0400, John Ferlan wrote:
>>>
>>>
>>> On 10/16/19 10:08 AM, Daniel P. Berrangé wrote:
>>> > On Wed, Oct 16, 2019 at 08:54:26AM -0400, John Ferlan wrote:
>>> >> Recent changes moved some files to build-aux and created at least
>>> >> one more.
>>> >
>>> > Really ? I wasn't aware that we'd moved the files listed in
>>> > this change.
>>> >
>>>
>>> Well that's where they now show up... I dunno. I had to clean my
>>> environment more than once due to recent changes and these files now
>>> show up in the noted/changed locations.  I also use a clean environment
>>> for my coverity builds every day and this is where they show up.
>>
>> Oh in fact I see them in the same place build-aux, but git is not
>> reporting them despite not being listed in gitignore.
>>
> 
> Note that there is another .gitignore in build-aux that lists the
> filenames mentioned in the patch
> 
> Jano
> 

$ cat build-aux/.gitignore
/config.rpath
/useless-if-before-free
/vc-list-files
$

hmm...

commit 144c06d4ee4cdf9c2035b9912844ab42bcd4dd9a
Author: Eric Blake 
Date:   Tue Nov 16 12:29:09 2010 -0700

maint: update to latest gnulib

...

There's an 'interesting' change to build-aux/.gitignore

John

>> I wonder if it has some default ignore list its using ?
>>
>>
>> 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

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

Re: [libvirt] [RFC] Accepting PRs/MRs for libvirt on GitHub/GitLab

[Ján Tomko]

On Wed, Oct 16, 2019 at 02:22:56PM +0200, Andrea Bolognani wrote:

First of all, we'd remove the ominous message from GitHub mirror
repositories (interestingly, the same is not present on GitLab).



Well if you're using GitLab then you're already aware of the fact that
not everything is hosted on GitHub.


Then, we'd starting accepting PRs/MRs. The way we'd handle them is
that, when one comes in, one among the handful of core developers who
volunteered to do so would review the patches on the respective
platform, working with the submitter to get it into shape just like
they would do on the mailing list; once the series is finally ready
to be merged, the core developer would update the PR/MR as necessary,
for example picking up R-bs or fixing the kind of trivial issues that
usually don't warrant a repost, and then push to master as usual.

More in detail: GitHub and GitLab have a feature that allows project
members to update PRs/MRs: basically the way it works is that, if the
PR/MR was created by the user 'foo' from their branch 'bar', the
libvirt developer is allowed to (force-)push to the foo/libvirt/bar
branch, and doing so updates the corresponding PR/MR; after that, if
the updated branch is locally merged into master and master is pushed
to the libvirt.org repo, once it gets mirrored GitHub/GitLab will
recognize that the commit IDs match and automatically mark the PR/MR
as merged. I have tested this behavior on both platforms (minus the
mirroring part) with Martin's help.

One last important bit. In the spirit of not requiring core
developers to alter their workflow, the developer who reviewed the
patches on GitHub/GitLab will also be responsible to format them to
the mailing list after merging them: this way, even those who don't
have a GitHub/GitLab account will get a chance to take notice of the
code changes and weigh in. Unlike what we're used to, this feedback
will come after the fact, but assuming that issues are spotted only
at that point we can either push the relevant fixes as follow-up
commits or even revert the series outright, so I don't feel like it
would be a massive problem overall.



Here it deviates from the usual mailing list workflow where the patch
has (in theory) a chance to be seen by all the developers.

But given that the requests will probably
a) be close to trivial
b) seen by a group of developers, not just one

sending it to the mailing list after it's pushed is better treatment
than our language bindings get.

Jano


Thoughts?


[1] https://github.com/libvirt/libvirt/pulls?q=is:pr+is:closed
--
Andrea Bolognani / Red Hat / Virtualization

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


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

Re: [libvirt] [PATCH v3 06/10] qemu_driver: use VIR_AUTOFREE() with strings 1/3

[Daniel Henrique Barboza]

On 10/16/19 11:28 AM, Ján Tomko wrote:

On Wed, Oct 16, 2019 at 11:14:05AM -0300, Daniel Henrique Barboza wrote:



On 10/16/19 10:49 AM, Ján Tomko wrote:
On Tue, Oct 15, 2019 at 05:08:48PM -0300, Daniel Henrique Barboza 
wrote:

Using VIR_AUTOFREE() in all strings of qemu_driver.c make the code
a bit tidier and smaller, sparing VIR_FREE() calls and sometimes a
whole 'cleanup' label.


These patches would look much better split by:
* individual functions (in case you do rework multiple things at once)


Do you mean sending an individual patch for any function that might
have, say, 2+ changes in it? For example, if the same function
was changed to use g_autoptr and g_autofree and perhaps
that causes a label to be dropped, this can be an individual patch?


Yes, but if you convert a lot of functions, that would result in a lot
of patches.


Just checked. There's just a couple of functions I messed around in a
non-trivial way (moved string declarations inside loops to avoid a
VIR_FREE call, lots of VIR_FREE calls being removed and so on).

I can send them in separated patches (perhaps both in the same patch,
I think the functions are related), then proceed with the more trivial
changes of inserting g_autoptr, g_autofree and removing labels in
separated patches each.


DHB




Sending one patch per function is more viable for the cases where you
need to refactor it in order to add some functionality later (see
Jirka's series for example). For mass conversion for the sake of
conversion, one patch per change is better.

Jano





* individual changes, i.e.
 * g_autofree for scalars
 * g_autoptr for pointers and unref
 * possible removal of cleanup labels


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

Re: [libvirt] [PATCH] build: Update .gitignore for build-aux files

[Ján Tomko]

On Wed, Oct 16, 2019 at 03:23:41PM +0100, Daniel P. Berrangé wrote:

On Wed, Oct 16, 2019 at 10:17:16AM -0400, John Ferlan wrote:



On 10/16/19 10:08 AM, Daniel P. Berrangé wrote:
> On Wed, Oct 16, 2019 at 08:54:26AM -0400, John Ferlan wrote:
>> Recent changes moved some files to build-aux and created at least
>> one more.
>
> Really ? I wasn't aware that we'd moved the files listed in
> this change.
>

Well that's where they now show up... I dunno. I had to clean my
environment more than once due to recent changes and these files now
show up in the noted/changed locations.  I also use a clean environment
for my coverity builds every day and this is where they show up.


Oh in fact I see them in the same place build-aux, but git is not
reporting them despite not being listed in gitignore.



Note that there is another .gitignore in build-aux that lists the
filenames mentioned in the patch

Jano


I wonder if it has some default ignore list its using ?


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


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

Re: [libvirt] [PATCH v3] util: Block SIGPIPE until execve in child process

[Daniel P . Berrangé]

On Wed, Oct 16, 2019 at 09:33:59AM -0500, Eric Blake wrote:
> On 10/16/19 9:04 AM, Daniel P. Berrangé wrote:
> > On Wed, Oct 16, 2019 at 06:50:33AM -0500, Eric Blake wrote:
> > > On 10/16/19 4:02 AM, Daniel P. Berrangé wrote:
> > > 
> > > > 
> > > > The challenge here is that we're in between fork + execve and want 
> > > > signal
> > > > handlers back to their defaults at time of execve.
> > > > 
> > > > If we set SIGPIPE to SIG_IGN and then execve() will that get reset back
> > > > to SIG_DFL automatically ?
> > > 
> > > Sadly, no.  execve() does not change whether a signal is ignored or masked
> > > (ignored is more common - a number of CI systems have had issues where the
> > > child inherits SIGPIPE ignored because the parent forgot to reset it, but
> > > the child wasn't expecting it; but inheriting a signal masked is also a 
> > > real
> > > issue), with the lone exception of SIGCHLD. However, execve() _does_ 
> > > change
> > > a signal that is being caught in the parent into SIG_DFL post-exec.
> > > 
> > > That does mean, however, that it is viable to install a no-op SIGPIPE
> > > handler (SIGPIPE is generated but ignored, I/O gets the EPIPE as desired),
> > > then post-exec the new process will have SIG_DFL.
> > 
> > Yeah, that's workable.
> > 
> > So we need virFork() to install a dummy SIGPIPE handler function that
> > is a no-op, *before* it unmasks signals.
> 
> Why mask signals at all? You either mask the signal before I/O, install the
> dummy handler, then unmask (and any intermediate SIGPIPE is now ignored by
> no-op), or you can merely install the dummy handler before I/O (any SIGPIPE
> is ignored by no-op).  That is, by the time you identify a a safe place to
> install a mask (ie. no I/O between fork() and that point, but where there
> will be potential I/O between that point and exec), with plans to release it
> later, that same place is just as good for changing from SIG_IGN to a no-op
> handler without messing with masks.

The fork'd child process inherits all signal handlers from the parent.
These handlers may no longer be valid since we are mass-closing all
file descriptors. We thus need to kill off all the signal handlers
in the forkd child. 

There's a window between fork & sigaction where signals can still
get delivered to the child & run the undesirable handlers. So we must
mask all signals immediately before fork, only unmasking them after
we have set all signal handlers back to their defaults.

We just need to set SIGPIPE to a dummy no-op handler before this
unmask

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 4/4] apparmor: let AppArmorSetSecurityImageLabel append rules

[Christian Ehrhardt]

There are currently broken use cases, e.g. snapshotting more than one disk at
once like:
 $ virsh snapshot-create-as --domain eoan --disk-only --atomic
   --diskspec vda,snapshot=no  --diskspec vdb,snapshot=no
   --diskspec vdc,file=/test/disk1.snapshot1.qcow,snapshot=external
   --diskspec vdd,file=/test/disk2.snapshot1.qcow,snapshot=external
The command above will iterate from qemuDomainSnapshotCreateDiskActive and
eventually add /test/disk1.snapshot1.qcow first (appears in the rules)
to then later add /test/disk2.snapshot1.qcow and while doing so throwing
away the former rule causing it to fail.

All other calls to (re)load_profile already use append=true when adding
rules append=false is only used when restoring rules [1].

Fix this by letting AppArmorSetSecurityImageLabel use append=true as well.

Bugs:
https://bugs.launchpad.net/libvirt/+bug/1845506
https://bugzilla.redhat.com/show_bug.cgi?id=1746684

[1]: https://bugs.launchpad.net/libvirt/+bug/1845506/comments/13

Signed-off-by: Christian Ehrhardt 
---
 src/security/security_apparmor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index 320d69e52a..4dd7ba20b4 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -812,7 +812,7 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
 return -1;
 }
 
-return reload_profile(mgr, def, src->path, false);
+return reload_profile(mgr, def, src->path, true);
 }
 
 static int
-- 
2.23.0

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

Re: [libvirt] [PATCH v3] util: Block SIGPIPE until execve in child process

[Eric Blake]

On 10/16/19 9:04 AM, Daniel P. Berrangé wrote:

On Wed, Oct 16, 2019 at 06:50:33AM -0500, Eric Blake wrote:

On 10/16/19 4:02 AM, Daniel P. Berrangé wrote:



The challenge here is that we're in between fork + execve and want signal
handlers back to their defaults at time of execve.

If we set SIGPIPE to SIG_IGN and then execve() will that get reset back
to SIG_DFL automatically ?


Sadly, no.  execve() does not change whether a signal is ignored or masked
(ignored is more common - a number of CI systems have had issues where the
child inherits SIGPIPE ignored because the parent forgot to reset it, but
the child wasn't expecting it; but inheriting a signal masked is also a real
issue), with the lone exception of SIGCHLD. However, execve() _does_ change
a signal that is being caught in the parent into SIG_DFL post-exec.

That does mean, however, that it is viable to install a no-op SIGPIPE
handler (SIGPIPE is generated but ignored, I/O gets the EPIPE as desired),
then post-exec the new process will have SIG_DFL.


Yeah, that's workable.

So we need virFork() to install a dummy SIGPIPE handler function that
is a no-op, *before* it unmasks signals.


Why mask signals at all? You either mask the signal before I/O, install 
the dummy handler, then unmask (and any intermediate SIGPIPE is now 
ignored by no-op), or you can merely install the dummy handler before 
I/O (any SIGPIPE is ignored by no-op).  That is, by the time you 
identify a a safe place to install a mask (ie. no I/O between fork() and 
that point, but where there will be potential I/O between that point and 
exec), with plans to release it later, that same place is just as good 
for changing from SIG_IGN to a no-op handler without messing with masks.


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

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

[libvirt] [PATCH 1/4] virt-aa-helper: clarify command line options

[Christian Ehrhardt]

While only used internally from libvirt the options still are misleading
enough to cause issues every now and then.
Group modes, options and an adding extra file and extend the wording of
the latter which had the biggest lack of clarity.
Both add a file to the end of the rules, but one re-generates the
rules from XML and the other keeps the existing rules as-is not
considering the XML content.

Signed-off-by: Christian Ehrhardt 
---
 src/security/virt-aa-helper.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 9157411133..e0c72e1b9c 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -95,18 +95,20 @@ vahDeinit(vahControl * ctl)
 static void
 vah_usage(void)
 {
-printf(_("\n%s [options] [< def.xml]\n\n"
-"  Options:\n"
+printf(_("\n%s mode [options] [extra file] [< def.xml]\n\n"
+"  Modes:\n"
 "-a | --add load profile\n"
 "-c | --create  create profile from template\n"
-"-d | --dryrun  dry run\n"
 "-D | --delete  unload and delete profile\n"
-"-f | --add-file  add file to profile\n"
-"-F | --append-file   append file to profile\n"
 "-r | --replace reload profile\n"
 "-R | --remove  unload profile\n"
-"-h | --helpthis help\n"
+"  Options:\n"
+"-d | --dryrun  dry run\n"
 "-u | --uuid  uuid (profile name)\n"
+"-h | --helpthis help\n"
+"  Extra File:\n"
+"-f | --add-file  add file to a profile 
generated from XML\n"
+"-F | --append-file   append file to an existing 
profile\n"
 "\n"), progname);
 
 puts(_("This command is intended to be used by libvirtd "
-- 
2.23.0

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

[libvirt] [PATCH 0/4] apparmor fixes triggered by multi disk snapshots

[Christian Ehrhardt]

Hi,
the bugs [1][2] that made me debug into this actually only need the
last patch (one line), but while coming along I found several
opportunities for minor improvements of the apparmor code in libvirt.
But that way it became a 4 patch series around apparmor.

[1]: https://bugzilla.redhat.com/show_bug.cgi?id=1746684
[2]: https://bugs.launchpad.net/libvirt/+bug/1845506

Christian Ehrhardt (4):
  virt-aa-helper: clarify command line options
  apparmor: drop useless call to get_profile_name
  apparmor: refactor AppArmorSetSecurityImageLabel
  apparmor: let AppArmorSetSecurityImageLabel append rules

 src/security/security_apparmor.c | 52 +++-
 src/security/virt-aa-helper.c| 14 +
 2 files changed, 19 insertions(+), 47 deletions(-)

-- 
2.23.0

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

Re: [libvirt] [PATCH v3 06/10] qemu_driver: use VIR_AUTOFREE() with strings 1/3

[Ján Tomko]

On Wed, Oct 16, 2019 at 11:14:05AM -0300, Daniel Henrique Barboza wrote:



On 10/16/19 10:49 AM, Ján Tomko wrote:

On Tue, Oct 15, 2019 at 05:08:48PM -0300, Daniel Henrique Barboza wrote:

Using VIR_AUTOFREE() in all strings of qemu_driver.c make the code
a bit tidier and smaller, sparing VIR_FREE() calls and sometimes a
whole 'cleanup' label.


These patches would look much better split by:
* individual functions (in case you do rework multiple things at once)


Do you mean sending an individual patch for any function that might
have, say, 2+ changes in it? For example, if the same function
was changed to use g_autoptr and g_autofree and perhaps
that causes a label to be dropped, this can be an individual patch?


Yes, but if you convert a lot of functions, that would result in a lot
of patches.

Sending one patch per function is more viable for the cases where you
need to refactor it in order to add some functionality later (see
Jirka's series for example). For mass conversion for the sake of
conversion, one patch per change is better.

Jano





* individual changes, i.e.
 * g_autofree for scalars
 * g_autoptr for pointers and unref
 * possible removal of cleanup labels


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

[libvirt] [PATCH 0/3] docs: hacking: document removed macro replacements (glib chronicles)

[Ján Tomko]

Make the GLib section more readable.

Ján Tomko (3):
  docs: hacking: separate section about already deleted macros
  docs: hacking: use  for functions/names
  docs: hacking: add a conversion table for removed libvirt macros

 docs/hacking.html.in | 71 +---
 1 file changed, 47 insertions(+), 24 deletions(-)

-- 
2.21.0

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

[libvirt] [PATCH 2/4] apparmor: drop useless call to get_profile_name

[Christian Ehrhardt]

reload_profile calls get_profile_name for no particular gain, lets
remove that call. The string isn't used in that function later on
and not registered/passed anywhere.

It can only fail if it either can't allocate or if the
virDomainDefPtr would have no uuid set (which isn't allowed).

Thereby the only "check" it really provides is if it can allocate the
string to then free it again.

This was initially added in [1] when the code was still in
AppArmorRestoreSecurityImageLabel (later moved) and even back then had
no further effect than described above.

[1]: 
https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/security/security_apparmor.c;h=16de0f26f41689e0c50481120d9f8a59ba1f4073;hb=bbaecd6a8f15345bc822ab4b79eb0955986bb2fd#l487

Signed-off-by: Christian Ehrhardt 
---
 src/security/security_apparmor.c | 14 ++
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index 75203cc43a..691833eb4b 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -282,17 +282,12 @@ reload_profile(virSecurityManagerPtr mgr,
const char *fn,
bool append)
 {
-int rc = -1;
-char *profile_name = NULL;
 virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(
 def, SECURITY_APPARMOR_NAME);
 
 if (!secdef || !secdef->relabel)
 return 0;
 
-if ((profile_name = get_profile_name(def)) == NULL)
-return rc;
-
 /* Update the profile only if it is loaded */
 if (profile_loaded(secdef->imagelabel) >= 0) {
 if (load_profile(mgr, secdef->imagelabel, def, fn, append) < 0) {
@@ -300,15 +295,10 @@ reload_profile(virSecurityManagerPtr mgr,
_("cannot update AppArmor profile "
  "\'%s\'"),
secdef->imagelabel);
-goto cleanup;
+return -1;
 }
 }
-
-rc = 0;
- cleanup:
-VIR_FREE(profile_name);
-
-return rc;
+return 0;
 }
 
 static int
-- 
2.23.0

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

[libvirt] [PATCH 3/4] apparmor: refactor AppArmorSetSecurityImageLabel

[Christian Ehrhardt]

A lot of the code in AppArmorSetSecurityImageLabel is a duplicate of
what is in reload_profile, this refactors AppArmorSetSecurityImageLabel
to use reload_profile instead.

Signed-off-by: Christian Ehrhardt 
---
 src/security/security_apparmor.c | 38 
 1 file changed, 9 insertions(+), 29 deletions(-)

diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index 691833eb4b..320d69e52a 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -792,8 +792,6 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
   virStorageSourcePtr src,
   virSecurityDomainImageLabelFlags flags 
G_GNUC_UNUSED)
 {
-int rc = -1;
-char *profile_name = NULL;
 virSecurityLabelDefPtr secdef;
 
 if (!src->path || !virStorageSourceIsLocalStorage(src))
@@ -803,36 +801,18 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
 if (!secdef || !secdef->relabel)
 return 0;
 
-if (secdef->imagelabel) {
-/* if the device doesn't exist, error out */
-if (!virFileExists(src->path)) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("\'%s\' does not exist"),
-   src->path);
-return -1;
-}
-
-if ((profile_name = get_profile_name(def)) == NULL)
-return -1;
+if (!secdef->imagelabel)
+return 0;
 
-/* update the profile only if it is loaded */
-if (profile_loaded(secdef->imagelabel) >= 0) {
-if (load_profile(mgr, secdef->imagelabel, def,
- src->path, false) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("cannot update AppArmor profile "
- "\'%s\'"),
-   secdef->imagelabel);
-goto cleanup;
-}
-}
+/* if the device doesn't exist, error out */
+if (!virFileExists(src->path)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("\'%s\' does not exist"),
+   src->path);
+return -1;
 }
-rc = 0;
 
- cleanup:
-VIR_FREE(profile_name);
-
-return rc;
+return reload_profile(mgr, def, src->path, false);
 }
 
 static int
-- 
2.23.0

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

[libvirt] [PATCH 1/3] docs: hacking: separate section about already deleted macros

[Ján Tomko]

Move the recently deleted libvirt macros into a separate section.

Signed-off-by: Ján Tomko 
---
 docs/hacking.html.in | 13 +
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/docs/hacking.html.in b/docs/hacking.html.in
index de450b7cde..92826c5d44 100644
--- a/docs/hacking.html.in
+++ b/docs/hacking.html.in
@@ -1029,6 +1029,15 @@ BAD:
 instead. Don't use g_vasprintf unless having the string length
 returned is unavoidable.
 
+  virStrerror
+  The GLib g_strerror() function should be used instead,
+which has a simpler calling convention as an added benefit.
+
+
+
+  The following libvirt APIs have been deleted already:
+
+
   VIR_AUTOPTR, VIR_AUTOCLEAN, VIR_AUTOFREE
   The GLib macros g_autoptr, g_auto and g_autofree must be used
 instead in all new code. In existing code, the GLib macros must
@@ -1051,10 +1060,6 @@ BAD:
   The GLib macros g_autoptr and G_DEFINE_AUTOPTR_CLEANUP_FUNC
 should be used to manage autoclean of virObject classes.
 This matches usage with GObject classes.
-
-  virStrerror
-  The GLib g_strerror() function should be used instead,
-which has a simpler calling convention as an added benefit.
 
 
 File handling
-- 
2.21.0

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

Re: [libvirt] [PATCH] build: Update .gitignore for build-aux files

[Daniel P . Berrangé]

On Wed, Oct 16, 2019 at 10:17:16AM -0400, John Ferlan wrote:
> 
> 
> On 10/16/19 10:08 AM, Daniel P. Berrangé wrote:
> > On Wed, Oct 16, 2019 at 08:54:26AM -0400, John Ferlan wrote:
> >> Recent changes moved some files to build-aux and created at least
> >> one more.
> > 
> > Really ? I wasn't aware that we'd moved the files listed in
> > this change.
> > 
> 
> Well that's where they now show up... I dunno. I had to clean my
> environment more than once due to recent changes and these files now
> show up in the noted/changed locations.  I also use a clean environment
> for my coverity builds every day and this is where they show up.

Oh in fact I see them in the same place build-aux, but git is not
reporting them despite not being listed in gitignore.

I wonder if it has some default ignore list its using ?


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

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

Re: [libvirt] [PATCH] build: Update .gitignore for build-aux files

[John Ferlan]

On 10/16/19 10:08 AM, Daniel P. Berrangé wrote:
> On Wed, Oct 16, 2019 at 08:54:26AM -0400, John Ferlan wrote:
>> Recent changes moved some files to build-aux and created at least
>> one more.
> 
> Really ? I wasn't aware that we'd moved the files listed in
> this change.
> 

Well that's where they now show up... I dunno. I had to clean my
environment more than once due to recent changes and these files now
show up in the noted/changed locations.  I also use a clean environment
for my coverity builds every day and this is where they show up.

If that's not right, then maybe someone else can figure out why - I was
just going for the "clean" look of/for git status. If I need to change
the commit message that works too. I didn't think too long about it.

John

>>
>> Signed-off-by: John Ferlan 
>> ---
>>  .gitignore | 7 ---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/.gitignore b/.gitignore
>> index 85ead5c907..30610a37c9 100644
>> --- a/.gitignore
>> +++ b/.gitignore
>> @@ -43,20 +43,22 @@
>>  /autom4te.cache
>>  /build-aux/.gitignore
>>  /build-aux/compile
>> +/build-aux/config.guess
>> +/build-aux/config.sub
>>  /build-aux/depcomp
>> +/build-aux/install-sh
>> +/build-aux/ltmain.sh
>>  /build-aux/missing
>>  /build-aux/test-driver
>>  /build/
>>  /ci/scratch/
>>  /confdefs.h
>>  /config.cache
>> -/config.guess
>>  /config.h
>>  /config.h.in
>>  /config.log
>>  /config.rpath
>>  /config.status
>> -/config.sub
>>  /configure
>>  /configure.lineno
>>  /conftest.*
>> @@ -96,7 +98,6 @@
>>  /libvirt*.pc
>>  /libvirt.spec
>>  /ltconfig
>> -/ltmain.sh
>>  /m4/*
>>  /mingw-libvirt.spec
>>  /mkinstalldirs
>> -- 
>> 2.20.1
>>
>> --
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
> 
> Regards,
> Daniel
> 

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

[libvirt] [PATCH 3/3] docs: hacking: add a conversion table for removed libvirt macros

[Ján Tomko]

Signed-off-by: Ján Tomko 
---
 docs/hacking.html.in | 13 +
 1 file changed, 13 insertions(+)

diff --git a/docs/hacking.html.in b/docs/hacking.html.in
index ecf52ffc17..3b66c16761 100644
--- a/docs/hacking.html.in
+++ b/docs/hacking.html.in
@@ -1066,6 +1066,19 @@ BAD:
 should be used to manage autoclean of virObject classes.
 This matches usage with GObject classes.
 
+
+deleted versionGLib versionNotes
+
VIR_AUTOPTRg_autoptr
+
VIR_AUTOCLEANg_auto
+
VIR_AUTOFREEg_autofreeThe 
GLib version does not use parentheses
+
VIR_AUTOUNREFg_autoptrThe 
cleanup function needs to be defined
+
VIR_DEFINE_AUTOPTR_FUNCG_DEFINE_AUTOPTR_CLEANUP_FUNC
+
VIR_DEFINE_AUTOCLEAN_FUNCG_DEFINE_AUTO_CLEANUP_CLEAR_FUNC
+
VIR_STEAL_PTRg_steal_pointer
+a = f(b) instead of f(a, 
b)
+VIR_RETURN_PTRreturn 
g_steal_pointer
+
+
 
 File handling
 
-- 
2.21.0

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

[libvirt] [PATCH 2/3] docs: hacking: use for functions/names

[Ján Tomko]

Use the  element more in the GLib section.

Signed-off-by: Ján Tomko 
---
 docs/hacking.html.in | 47 
 1 file changed, 26 insertions(+), 21 deletions(-)

diff --git a/docs/hacking.html.in b/docs/hacking.html.in
index 92826c5d44..ecf52ffc17 100644
--- a/docs/hacking.html.in
+++ b/docs/hacking.html.in
@@ -1008,28 +1008,32 @@ BAD:
 
 
 
-  VIR_ALLOC, VIR_REALLOC, VIR_RESIZE_N, VIR_EXPAND_N,
-VIR_SHRINK_N, VIR_FREE, VIR_APPEND_ELEMENT, VIR_INSERT_ELEMENT,
-VIR_DELETE_ELEMENT
-  Prefer the GLib APIs g_new0/g_renew/g_free in most cases.
-There should rarely be a need to use g_malloc/g_realloc.
+  VIR_ALLOC, VIR_REALLOC,
+VIR_RESIZE_N, VIR_EXPAND_N,
+VIR_SHRINK_N, VIR_FREE,
+VIR_APPEND_ELEMENT, VIR_INSERT_ELEMENT,
+VIR_DELETE_ELEMENT
+Prefer the GLib APIs g_new0/g_renew/
+g_free in most cases. There should rarely be a need
+to use g_malloc/g_realloc.
 Instead of using plain C arrays, it is preferrable to use
-one of the GLib types, GArray, GPtrArray or GByteArray. These
+one of the GLib types, GArray, GPtrArray
+or GByteArray. These
 all use a struct to track the array memory and size together
 and efficiently resize. NEVER MIX use of the
 classic libvirt memory allocation APIs and GLib APIs within
 a single method. Keep the style consistent, converting existing
 code to GLib style in a separate, prior commit.
 
-  VIR_STRDUP, VIR_STRNDUP
-  Prefer the GLib APIs g_strdup and g_strndup.
+  VIR_STRDUP, VIR_STRNDUP
+  Prefer the GLib APIs g_strdup and 
g_strndup.
 
-  virAsprintf, virVasprintf
-  The GLib APIs g_strdup_printf / g_strdup_vprint should be used
-instead. Don't use g_vasprintf unless having the string length
+  virAsprintf, virVasprintf
+  The GLib APIs g_strdup_printf / 
g_strdup_vprint should be used
+instead. Don't use g_vasprintf unless having the string 
length
 returned is unavoidable.
 
-  virStrerror
+  virStrerror
   The GLib g_strerror() function should be used instead,
 which has a simpler calling convention as an added benefit.
 
@@ -1038,26 +1042,27 @@ BAD:
   The following libvirt APIs have been deleted already:
 
 
-  VIR_AUTOPTR, VIR_AUTOCLEAN, VIR_AUTOFREE
-  The GLib macros g_autoptr, g_auto and g_autofree must be used
+  VIR_AUTOPTR, VIR_AUTOCLEAN, 
VIR_AUTOFREE
+  The GLib macros g_autoptr, g_auto and
+g_autofree must be used
 instead in all new code. In existing code, the GLib macros must
 never be mixed with libvirt macros within a method, nor should
-they be mixed with VIR_FREE. If introducing GLib macros to an
+they be mixed with VIR_FREE. If introducing GLib macros 
to an
 existing method, any use of libvirt macros must be converted
 in an independent commit.
   
 
-  VIR_DEFINE_AUTOPTR_FUNC, VIR_DEFINE_AUTOCLEAN_FUNC
-  The GLib macros G_DEFINE_AUTOPTR_CLEANUP_FUNC and
-G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC must be used in all
+  VIR_DEFINE_AUTOPTR_FUNC, 
VIR_DEFINE_AUTOCLEAN_FUNC
+  The GLib macros G_DEFINE_AUTOPTR_CLEANUP_FUNC and
+G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC must be used in all
 new code. Existing code should be converted to the
 new macros where relevant. It is permissible to use
-g_autoptr, g_auto on an object whose cleanup function
+g_autoptr, g_auto on an object whose cleanup 
function
 is declared with the libvirt macros and vice-verca.
   
 
-  VIR_AUTOUNREF
-  The GLib macros g_autoptr and G_DEFINE_AUTOPTR_CLEANUP_FUNC
+  VIR_AUTOUNREF
+  The GLib macros g_autoptr and 
G_DEFINE_AUTOPTR_CLEANUP_FUNC
 should be used to manage autoclean of virObject classes.
 This matches usage with GObject classes.
 
-- 
2.21.0

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

Re: [libvirt] [PATCH v3 06/10] qemu_driver: use VIR_AUTOFREE() with strings 1/3

[Daniel Henrique Barboza]

On 10/16/19 10:49 AM, Ján Tomko wrote:

On Tue, Oct 15, 2019 at 05:08:48PM -0300, Daniel Henrique Barboza wrote:

Using VIR_AUTOFREE() in all strings of qemu_driver.c make the code
a bit tidier and smaller, sparing VIR_FREE() calls and sometimes a
whole 'cleanup' label.


These patches would look much better split by:
* individual functions (in case you do rework multiple things at once)


Do you mean sending an individual patch for any function that might
have, say, 2+ changes in it? For example, if the same function
was changed to use g_autoptr and g_autofree and perhaps
that causes a label to be dropped, this can be an individual patch?



* individual changes, i.e.
 * g_autofree for scalars
 * g_autoptr for pointers and unref
 * possible removal of cleanup labels




Got it. I'll reorganize the changes and send it this way.





Especially splitting the goto -> return change makes the patches
much more easier to read, since it makes it obvious that you don't
change the exit points of the function while adding the autofree
attributes.

Jano



This is a huge change due to the amount of char * declared in
this file, thus let's split it in 3. This is the first part.

Signed-off-by: Daniel Henrique Barboza 
---
src/qemu/qemu_driver.c | 103 +
1 file changed, 33 insertions(+), 70 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d95c5c5b81..f887a79ecd 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -381,11 +381,9 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
   void *data)
{
    char *baseDir = (char *)data;
-    char *snapDir = NULL;
+    VIR_AUTOFREE(char *) snapDir = NULL;
    DIR *dir = NULL;
    struct dirent *entry;
-    char *xmlStr;
-    char *fullpath;
    virDomainSnapshotDefPtr def = NULL;
    virDomainMomentObjPtr snap = NULL;
    virDomainMomentObjPtr current = NULL;
@@ -420,6 +418,9 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
    goto cleanup;

    while ((direrr = virDirRead(dir, , NULL)) > 0) {
+    VIR_AUTOFREE(char *) xmlStr = NULL;
+    VIR_AUTOFREE(char *) fullpath = NULL;
+
    /* NB: ignoring errors, so one malformed config doesn't
   kill the whole process */
    VIR_INFO("Loading snapshot file '%s'", entry->d_name);
@@ -435,7 +436,6 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
    virReportSystemError(errno,
 _("Failed to read snapshot file %s"),
 fullpath);
-    VIR_FREE(fullpath);
    continue;
    }

@@ -448,8 +448,6 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
    virReportError(VIR_ERR_INTERNAL_ERROR,
   _("Failed to parse snapshot XML from file 
'%s'"),

   fullpath);
-    VIR_FREE(fullpath);
-    VIR_FREE(xmlStr);
    continue;
    }

@@ -463,9 +461,6 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
   vm->def->name);
    current = snap;
    }
-
-    VIR_FREE(fullpath);
-    VIR_FREE(xmlStr);
    }
    if (direrr < 0)
    virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -492,7 +487,6 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
    ret = 0;
 cleanup:
    VIR_DIR_CLOSE(dir);
-    VIR_FREE(snapDir);
    virObjectUnlock(vm);
    return ret;
}
@@ -503,11 +497,9 @@ qemuDomainCheckpointLoad(virDomainObjPtr vm,
 void *data)
{
    char *baseDir = (char *)data;
-    char *chkDir = NULL;
+    VIR_AUTOFREE(char *) chkDir = NULL;
    DIR *dir = NULL;
    struct dirent *entry;
-    char *xmlStr;
-    char *fullpath;
    virDomainCheckpointDefPtr def = NULL;
    virDomainMomentObjPtr chk = NULL;
    virDomainMomentObjPtr current = NULL;
@@ -538,6 +530,9 @@ qemuDomainCheckpointLoad(virDomainObjPtr vm,
    goto cleanup;

    while ((direrr = virDirRead(dir, , NULL)) > 0) {
+    VIR_AUTOFREE(char *) xmlStr = NULL;
+    VIR_AUTOFREE(char *) fullpath = NULL;
+
    /* NB: ignoring errors, so one malformed config doesn't
   kill the whole process */
    VIR_INFO("Loading checkpoint file '%s'", entry->d_name);
@@ -553,7 +548,6 @@ qemuDomainCheckpointLoad(virDomainObjPtr vm,
    virReportSystemError(errno,
 _("Failed to read checkpoint file %s"),
 fullpath);
-    VIR_FREE(fullpath);
    continue;
    }

@@ -566,8 +560,6 @@ qemuDomainCheckpointLoad(virDomainObjPtr vm,
    virReportError(VIR_ERR_INTERNAL_ERROR,
   _("Failed to parse checkpoint XML from 
file '%s'"),

   fullpath);
-    VIR_FREE(fullpath);
-    VIR_FREE(xmlStr);
    virObjectUnref(def);
    continue;
    }
@@ -575,9 +567,6 @@ qemuDomainCheckpointLoad(virDomainObjPtr vm,
    chk = virDomainCheckpointAssignDef(vm->checkpoints, def);
    if 

Re: [libvirt] [PATCH] build: Update .gitignore for build-aux files

[Daniel P . Berrangé]

On Wed, Oct 16, 2019 at 08:54:26AM -0400, John Ferlan wrote:
> Recent changes moved some files to build-aux and created at least
> one more.

Really ? I wasn't aware that we'd moved the files listed in
this change.

> 
> Signed-off-by: John Ferlan 
> ---
>  .gitignore | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/.gitignore b/.gitignore
> index 85ead5c907..30610a37c9 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -43,20 +43,22 @@
>  /autom4te.cache
>  /build-aux/.gitignore
>  /build-aux/compile
> +/build-aux/config.guess
> +/build-aux/config.sub
>  /build-aux/depcomp
> +/build-aux/install-sh
> +/build-aux/ltmain.sh
>  /build-aux/missing
>  /build-aux/test-driver
>  /build/
>  /ci/scratch/
>  /confdefs.h
>  /config.cache
> -/config.guess
>  /config.h
>  /config.h.in
>  /config.log
>  /config.rpath
>  /config.status
> -/config.sub
>  /configure
>  /configure.lineno
>  /conftest.*
> @@ -96,7 +98,6 @@
>  /libvirt*.pc
>  /libvirt.spec
>  /ltconfig
> -/ltmain.sh
>  /m4/*
>  /mingw-libvirt.spec
>  /mkinstalldirs
> -- 
> 2.20.1
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

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

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

[libvirt] [PATCH 0/2] Fix build with musl libc

[casantos]

From: Carlos Santos 

- Do not use 'stderr' as a field name, since it's a preprocessor macro
  in stdio.h.
- Include paths.h for _PATH_MOUNTED, if it's not defined in mntent.h.

Found while porting libvirt to Buildroot (https://buildroot.org/).

Carlos Santos (2):
  qemu: fix build with musl libc
  storage: fix build with musl libc

 src/qemu/qemu_process.c| 8 
 src/qemu/qemu_process.h| 2 +-
 src/storage/storage_backend_fs.c   | 3 +++
 src/storage/storage_backend_vstorage.c | 3 +++
 4 files changed, 11 insertions(+), 5 deletions(-)

-- 
2.18.1

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

[libvirt] [PATCH 1/2] qemu: fix build with musl libc

[casantos]

From: Carlos Santos 

On musl libc "stderr" is a preprocessor macro whose expansion leads to
compilation errors:

In file included from qemu/qemu_process.c:66:
qemu/qemu_process.c: In function 'qemuProcessQMPFree':
qemu/qemu_process.c:8418:21: error: expected identifier before '(' token
 VIR_FREE((proc->stderr));
 ^~

Prevent this by renaming the homonymous field in the _qemuProcessQMP
struct to "stdErr".

Signed-off-by: Carlos Santos 
---
 src/qemu/qemu_process.c | 8 
 src/qemu/qemu_process.h | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 9c50c4a1d8..e7a8d74455 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -8448,7 +8448,7 @@ qemuProcessQMPFree(qemuProcessQMPPtr proc)
 VIR_FREE(proc->monpath);
 VIR_FREE(proc->monarg);
 VIR_FREE(proc->pidfile);
-VIR_FREE(proc->stderr);
+VIR_FREE(proc->stdErr);
 VIR_FREE(proc);
 }
 
@@ -8601,7 +8601,7 @@ qemuProcessQMPLaunch(qemuProcessQMPPtr proc)
 virCommandSetGID(proc->cmd, proc->runGid);
 virCommandSetUID(proc->cmd, proc->runUid);
 
-virCommandSetErrorBuffer(proc->cmd, &(proc->stderr));
+virCommandSetErrorBuffer(proc->cmd, &(proc->stdErr));
 
 if (virCommandRun(proc->cmd, ) < 0)
 goto cleanup;
@@ -8611,7 +8611,7 @@ qemuProcessQMPLaunch(qemuProcessQMPPtr proc)
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to start QEMU binary %s for probing: %s"),
proc->binary,
-   proc->stderr ? proc->stderr : _("unknown error"));
+   proc->stdErr ? proc->stdErr : _("unknown error"));
 goto cleanup;
 }
 
@@ -8690,7 +8690,7 @@ qemuProcessQMPConnectMonitor(qemuProcessQMPPtr proc)
  *   ** Send QMP Queries to QEMU using monitor (proc->mon) **
  *   qemuProcessQMPFree(proc);
  *
- * Process error output (proc->stderr) remains available in qemuProcessQMP
+ * Process error output (proc->stdErr) remains available in qemuProcessQMP
  * struct until qemuProcessQMPFree is called.
  */
 int
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index 1d62319092..9af9f967fd 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -212,7 +212,7 @@ struct _qemuProcessQMP {
 char *libDir;
 uid_t runUid;
 gid_t runGid;
-char *stderr;
+char *stdErr;
 char *monarg;
 char *monpath;
 char *pidfile;
-- 
2.18.1

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

[libvirt] [PATCH 2/2] storage: fix build with musl libc

[casantos]

From: Carlos Santos 

On musl _PATH_MOUNTED is defined in paths.h, not in mntent.h, which
causes compilation errors:

storage/storage_backend_fs.c: In function 
'virStorageBackendFileSystemIsMounted':
storage/storage_backend_fs.c:255:23: error: '_PATH_MOUNTED' undeclared (first 
use in this function); did you mean 'XPATH_POINT'?
 if ((mtab = fopen(_PATH_MOUNTED, "r")) == NULL) {
   ^
   XPATH_POINT

Fix this including paths.h if _PATH_MOUNTED is still not defined after
including mntent.h. This also works with glibc and uClibc-ng.

Signed-off-by: Carlos Santos 
---
 src/storage/storage_backend_fs.c   | 3 +++
 src/storage/storage_backend_vstorage.c | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index ed677058ed..fafe2745cc 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -42,6 +42,9 @@ VIR_LOG_INIT("storage.storage_backend_fs");
 #if WITH_STORAGE_FS
 
 # include 
+#ifndef _PATH_MOUNTED
+# include 
+#endif
 
 struct _virNetfsDiscoverState {
 const char *host;
diff --git a/src/storage/storage_backend_vstorage.c 
b/src/storage/storage_backend_vstorage.c
index cec21dccbf..685f78a22f 100644
--- a/src/storage/storage_backend_vstorage.c
+++ b/src/storage/storage_backend_vstorage.c
@@ -7,6 +7,9 @@
 #include "virlog.h"
 #include "virstring.h"
 #include 
+#ifndef _PATH_MOUNTED
+#include 
+#endif
 #include 
 #include 
 #include "storage_util.h"
-- 
2.18.1

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

Re: [libvirt] Fwd: libvirtd failing on MacOS in setgroups

[Marcus Furlong]

On Mon, 30 Sep 2019 at 21:05, Bruno Haible  wrote:
>
> Daniel P. Berrangé wrote:
> > > For what purpose is libvirt or QEMU using setgroups()? What goes wrong if
> > > setgroups() fails?

On macOS, as far as I can see, everything works as expected without it.

So not sure if it's actually needed?

> > QEMU potentially needs access to files owned by a supplementary group.
> > On Linux for example, /dev/kvm is often owned by 'kvm' group, but the
> > 'qemu' user on Fedora has 'qemu' group as its primary group. So QEMU
> > would be unable to open /dev/kvm without the setgroups call to set up
> > supplementary groups.
>
> Ah, it's libvirt which calls setgroups and qemu which needs the groups.
> Then my suggested workaround that consists of overriding setgroups() and
> open() won't work.
>
> > >   - Is using the first 16 groups and ignoring the extra ones an acceptable
> > > solution?
> >
> > Certainly that's better than just ignoring groups entirely, as it will
> > work for many more cases, even if not perfect.
>
> Hmm. If the group of /dev/kvm comes at 17th group, it will still not work.
> I.e. it will be unreliable.
>
> Then, how about if libvirt collects the set of groups that qemu might need
> for accessing devices (surely less than 16), then fills up the remaining
> up to 16 slots with secondary groups? Admittedly it makes qemu less
> self-contained. But given that setgroups() works only for root on macOS [1]
> I see no better way.

Note that /dev/kvm is for linux and does not exist on macOS.

Unless we identify specific devices on macOS that qemu requires access
to, then something like the following might work?

https://github.com/furlongm/libvirt/commit/01a1d3d0e37c7f81a04da2e9707ac1c39f4642b9

Seems to work correctly for me (virsh capabilities now returns the
correct output, and VMs run).

-- 
Marcus Furlong

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

Re: [libvirt] [PATCH v3] util: Block SIGPIPE until execve in child process

[Daniel P . Berrangé]

On Wed, Oct 16, 2019 at 06:50:33AM -0500, Eric Blake wrote:
> On 10/16/19 4:02 AM, Daniel P. Berrangé wrote:
> 
> > 
> > The challenge here is that we're in between fork + execve and want signal
> > handlers back to their defaults at time of execve.
> > 
> > If we set SIGPIPE to SIG_IGN and then execve() will that get reset back
> > to SIG_DFL automatically ?
> 
> Sadly, no.  execve() does not change whether a signal is ignored or masked
> (ignored is more common - a number of CI systems have had issues where the
> child inherits SIGPIPE ignored because the parent forgot to reset it, but
> the child wasn't expecting it; but inheriting a signal masked is also a real
> issue), with the lone exception of SIGCHLD. However, execve() _does_ change
> a signal that is being caught in the parent into SIG_DFL post-exec.
> 
> That does mean, however, that it is viable to install a no-op SIGPIPE
> handler (SIGPIPE is generated but ignored, I/O gets the EPIPE as desired),
> then post-exec the new process will have SIG_DFL.

Yeah, that's workable.

So we need virFork() to install a dummy SIGPIPE handler function that
is a no-op, *before* it unmasks signals.


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

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

Re: [libvirt] [PATCH 0/9] GLib macros and where to find them (glib chronicles)

[Michal Privoznik]

On 10/16/19 2:09 PM, Ján Tomko wrote:

Ján Tomko (9):
   util: delete VIR_AUTOFREE
   Remove all usage of VIR_RETURN_PTR
   internal: delete VIR_RETURN_PTR
   conf: use g_steal_pointer instead of VIR_STEAL_PTR
   qemu: use g_steal_pointer instead of VIR_STEAL_PTR
   tools: use g_steal_pointer instead of VIR_STEAL_PTR
   util: use g_steal_pointer instead of VIR_STEAL_PTR
   Use g_steal_pointer instead of VIR_STEAL_PTR everywhere
   internal: delete VIR_STEAL_PTR



Reviewed-by: Michal Privoznik 

Michal

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

Re: [libvirt] [PATCH 1/9] util: delete VIR_AUTOFREE

[Michal Privoznik]

On 10/16/19 2:09 PM, Ján Tomko wrote:

Commit 1e2ae2e311c7453e7894e93688f8785736aa0618 deleted the last use
of VIR_AUTOFREE but forgot to delete the macro definition.

Signed-off-by: Ján Tomko 
---
  src/util/viralloc.h | 14 --
  1 file changed, 14 deletions(-)

diff --git a/src/util/viralloc.h b/src/util/viralloc.h
index c503bbe19b..d7862d6127 100644
--- a/src/util/viralloc.h
+++ b/src/util/viralloc.h
@@ -488,17 +488,3 @@ void virDisposeString(char **strptr)
   */
  #define VIR_DISPOSE(ptr) virDispose(1 ? (void *) &(ptr) : (ptr), 1, \
  sizeof(*(ptr)), NULL)
-
-
-/**
- * VIR_AUTOFREE:
- * @type: type of the variable to be freed automatically
- *
- * DEPRECATED: use g_autofree for new code. See HACKING
- * for further guidance.
- *
- * Macro to automatically free the memory allocated to
- * the variable declared with it by calling virFree
- * when the variable goes out of scope.
- */
-#define VIR_AUTOFREE(type) g_autofree type



Now that all VIR_AUTOPTR, VIR_AUTOCLEAN, VIR_AUTOFREE are gone, should 
we remove corresponding paragraph from docs/hacking.html.in:1032 too?


Any new code will not compile if it uses either of those macros.

Michal

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

Re: [libvirt] [PATCH v3 00/10] use VIR_AUTO*/g_auto* all around qemu_driver.c

[Ján Tomko]

On Wed, Oct 16, 2019 at 09:24:13AM -0300, Daniel Henrique Barboza wrote:

I just realized by reading a patch from Jano that VIR_AUTOFREE
is simply an alias for g_autofree. This means that it is not worth
to add more VIR_AUTOFREE() macros, and that it is ok to mix
g_autofree with other VIR_ macros such as VIR_AUTOUNREF().


I deleted VIR_AUTOUNREF already. All usage of VIR_AUTOFREE was also
removed, but I missed that the macro is definied in viralloc.h,
patch removing it is pending.

Per our HACKING style:
https://libvirt.org/hacking.html#glib
while the alloc/free functions should be interchangeable, it is bad
style to mix the VIR_ and g_ allocation macros in a single function.

I converted the VIR_AUTO_* group that had a GLib alternative,
leaving VIR_AUTOSTRINGLIST and VIR_AUTOCLOSE behind.

But to convert VIR_APPEND_ELEMENT usage a more fine grained approach
will be needed, since it might involve using the GLib container types.


Jano is
also removing VIR_AUTOFREE() from the code in his series, which
will make this whole series obsolete.

I'll re-send this series to not add more VIR_AUTOFREE() in any
step of the way.


DHB


On 10/15/19 5:08 PM, Daniel Henrique Barboza wrote:

changes from v2:
- rebased with newer master (67e72053c1)
- added an extra patch to convert the existing VIR_AUTO* macros
to g_auto* ones, all at once, to avoid the case where a method
will have both VIR_AUTO* and g_auto* macros at the same time.


Note: the conversion in patch 10 wasn't 100% due to a handful of
methods that I was unable to use g_autoptr. Take for example
the method qemuDomainSaveInternal:
--
qemuDomainObjPrivatePtr priv = vm->privateData;
VIR_AUTOUNREF(virCapsPtr) caps = NULL;
virQEMUSaveDataPtr data = NULL;
VIR_AUTOUNREF(qemuDomainSaveCookiePtr) cookie = NULL;
--

Changing the 'cookie' variable to use g_autoptr() causes an error:

make[3]: Entering directory '/home/danielhb/kvm-project/libvirt/src'
  CC   qemu/libvirt_driver_qemu_impl_la-qemu_driver.lo
In file included from /usr/lib64/glib-2.0/include/glibconfig.h:9,
 from /usr/include/glib-2.0/glib/gtypes.h:32,
 from /usr/include/glib-2.0/glib/galloca.h:32,
 from /usr/include/glib-2.0/glib.h:30,
 from ./internal.h:31,
 from qemu/qemu_agent.h:24,
 from qemu/qemu_driver.c:40:
qemu/qemu_driver.c: In function 'qemuDomainSaveInternal':
qemu/qemu_driver.c:3282:15: error: unknown type name 
'qemuDomainSaveCookie_autoptr'
 3282 | g_autoptr(qemuDomainSaveCookie) cookie = NULL;



IIUC the original plan was to use VIR_AUTOPTR for the types needing
unref so the cleanup function was defined for them, but then
VIR_AUTOUNREF was introduced which always used virObjectUnref.

To use g_autoptr with other types, you need to define the CLEANUP
function, see:

commit df4986b51bc88b65ae7f453814963e4340b168f3
 Define G_DEFINE_AUTOPTR_CLEANUP_FUNC for virDomainCheckpointDef

My objective was to get rid of the VIR_AUTOUNREF usage, not sure
whether introducing g_autoptr for already existing objects until
we convert them to GObject.



I tried doing it with the 'Ptr' in the name, same error:


  CC   qemu/libvirt_driver_qemu_impl_la-qemu_driver.lo
In file included from /usr/lib64/glib-2.0/include/glibconfig.h:9,
 from /usr/include/glib-2.0/glib/gtypes.h:32,
 from /usr/include/glib-2.0/glib/galloca.h:32,
 from /usr/include/glib-2.0/glib.h:30,
 from ./internal.h:31,
 from qemu/qemu_agent.h:24,
 from qemu/qemu_driver.c:40:
qemu/qemu_driver.c: In function 'qemuDomainSaveInternal':
qemu/qemu_driver.c:3282:15: error: unknown type name 
'qemuDomainSaveCookiePtr_autoptr'
 3282 | g_autoptr(qemuDomainSaveCookiePtr) cookie = NULL;


Similar situation happens with qemuDomainSaveImageStartVM and with
qemuSecurityInit methods. They are mentioned in the commit message
of patch 10 for reference. These methods are still using VIR_AUTO*
macros.


changes from v1:
- addressed review concerns made by Erik
- added more cleanups, as suggested by Erik

v2: https://www.redhat.com/archives/libvir-list/2019-September/msg01452.html
v1: https://www.redhat.com/archives/libvir-list/2019-September/msg00719.html

Daniel Henrique Barboza (10):
  qemu_driver: use VIR_AUTOUNREF() with virQEMUDriverConfigPtr 1/3
  qemu_driver: use VIR_AUTOUNREF() with virQEMUDriverConfigPtr 2/3
  qemu_driver: use VIR_AUTOUNREF() with virQEMUDriverConfigPtr 3/3
  qemu_driver: use VIR_AUTOUNREF() with virCapsPtr
  qemu_driver: use VIR_AUTOUNREF() with more pointer types
  qemu_driver: use VIR_AUTOFREE() with strings 1/3
  qemu_driver: use VIR_AUTOFREE() with strings 2/3
  qemu_driver: use VIR_AUTOFREE() with strings 3/3
  qemu_driver: VIR_AUTOFREE on other scalar pointers



  qemu_driver.c: use GLib macros


Fixing newly added lines seems odd, especially since you'll be replacing
the exact text that you added earlier.

Re: [libvirt] [PATCH v3 06/10] qemu_driver: use VIR_AUTOFREE() with strings 1/3

[Ján Tomko]

On Tue, Oct 15, 2019 at 05:08:48PM -0300, Daniel Henrique Barboza wrote:

Using VIR_AUTOFREE() in all strings of qemu_driver.c make the code
a bit tidier and smaller, sparing VIR_FREE() calls and sometimes a
whole 'cleanup' label.


These patches would look much better split by:
* individual functions (in case you do rework multiple things at once)
* individual changes, i.e.
 * g_autofree for scalars
 * g_autoptr for pointers and unref
 * possible removal of cleanup labels

Especially splitting the goto -> return change makes the patches
much more easier to read, since it makes it obvious that you don't
change the exit points of the function while adding the autofree
attributes.

Jano



This is a huge change due to the amount of char * declared in
this file, thus let's split it in 3. This is the first part.

Signed-off-by: Daniel Henrique Barboza 
---
src/qemu/qemu_driver.c | 103 +
1 file changed, 33 insertions(+), 70 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d95c5c5b81..f887a79ecd 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -381,11 +381,9 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
   void *data)
{
char *baseDir = (char *)data;
-char *snapDir = NULL;
+VIR_AUTOFREE(char *) snapDir = NULL;
DIR *dir = NULL;
struct dirent *entry;
-char *xmlStr;
-char *fullpath;
virDomainSnapshotDefPtr def = NULL;
virDomainMomentObjPtr snap = NULL;
virDomainMomentObjPtr current = NULL;
@@ -420,6 +418,9 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
goto cleanup;

while ((direrr = virDirRead(dir, , NULL)) > 0) {
+VIR_AUTOFREE(char *) xmlStr = NULL;
+VIR_AUTOFREE(char *) fullpath = NULL;
+
/* NB: ignoring errors, so one malformed config doesn't
   kill the whole process */
VIR_INFO("Loading snapshot file '%s'", entry->d_name);
@@ -435,7 +436,6 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
virReportSystemError(errno,
 _("Failed to read snapshot file %s"),
 fullpath);
-VIR_FREE(fullpath);
continue;
}

@@ -448,8 +448,6 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
virReportError(VIR_ERR_INTERNAL_ERROR,
   _("Failed to parse snapshot XML from file '%s'"),
   fullpath);
-VIR_FREE(fullpath);
-VIR_FREE(xmlStr);
continue;
}

@@ -463,9 +461,6 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
   vm->def->name);
current = snap;
}
-
-VIR_FREE(fullpath);
-VIR_FREE(xmlStr);
}
if (direrr < 0)
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -492,7 +487,6 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
ret = 0;
 cleanup:
VIR_DIR_CLOSE(dir);
-VIR_FREE(snapDir);
virObjectUnlock(vm);
return ret;
}
@@ -503,11 +497,9 @@ qemuDomainCheckpointLoad(virDomainObjPtr vm,
 void *data)
{
char *baseDir = (char *)data;
-char *chkDir = NULL;
+VIR_AUTOFREE(char *) chkDir = NULL;
DIR *dir = NULL;
struct dirent *entry;
-char *xmlStr;
-char *fullpath;
virDomainCheckpointDefPtr def = NULL;
virDomainMomentObjPtr chk = NULL;
virDomainMomentObjPtr current = NULL;
@@ -538,6 +530,9 @@ qemuDomainCheckpointLoad(virDomainObjPtr vm,
goto cleanup;

while ((direrr = virDirRead(dir, , NULL)) > 0) {
+VIR_AUTOFREE(char *) xmlStr = NULL;
+VIR_AUTOFREE(char *) fullpath = NULL;
+
/* NB: ignoring errors, so one malformed config doesn't
   kill the whole process */
VIR_INFO("Loading checkpoint file '%s'", entry->d_name);
@@ -553,7 +548,6 @@ qemuDomainCheckpointLoad(virDomainObjPtr vm,
virReportSystemError(errno,
 _("Failed to read checkpoint file %s"),
 fullpath);
-VIR_FREE(fullpath);
continue;
}

@@ -566,8 +560,6 @@ qemuDomainCheckpointLoad(virDomainObjPtr vm,
virReportError(VIR_ERR_INTERNAL_ERROR,
   _("Failed to parse checkpoint XML from file '%s'"),
   fullpath);
-VIR_FREE(fullpath);
-VIR_FREE(xmlStr);
virObjectUnref(def);
continue;
}
@@ -575,9 +567,6 @@ qemuDomainCheckpointLoad(virDomainObjPtr vm,
chk = virDomainCheckpointAssignDef(vm->checkpoints, def);
if (chk == NULL)
virObjectUnref(def);
-
-VIR_FREE(fullpath);
-VIR_FREE(xmlStr);
}
if (direrr < 0)
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -602,7 +591,6 @@ qemuDomainCheckpointLoad(virDomainObjPtr vm,
ret = 0;
 cleanup:
VIR_DIR_CLOSE(dir);
-VIR_FREE(chkDir);
virObjectUnlock(vm);
return ret;
}
@@ -660,12 

Re: [libvirt] [PATCH 00/10] Clean up error preservation/restoration

[Michal Privoznik]

On 10/15/19 7:37 PM, John Ferlan wrote:

I was going through some old git trees and found this one started before
I 'altered' my Red Hat career path. Seeing as no one has picked it up from
https://wiki.libvirt.org/page/BiteSizedTasks - figured I may as well post
it rather than throw it away ;-)

The change essentially alters virSaveLastError ... virSetError and
virFreeError paths to use virErrorPreserveLast & virErrorRestore.
  


John Ferlan (10):
   conf: Use consistent error preservation and restoration calls
   src: Use consistent error preservation and restoration calls
   libxl: Use consistent error preservation and restoration calls
   lxc: Use consistent error preservation and restoration calls
   qemu: Use consistent error preservation and restoration calls
   remote: Use consistent error preservation and restoration calls
   storage: Use consistent error preservation and restoration calls
   util: Use consistent error preservation and restoration calls
   vz: Use consistent error preservation and restoration calls
   tools: Use consistent error preservation and restoration calls

  src/conf/domain_conf.c|  7 +--
  src/libvirt-stream.c  | 36 +---
  src/libxl/libxl_migration.c   | 18 ++
  src/lxc/lxc_driver.c  |  9 ++-
  src/lxc/lxc_process.c |  7 +--
  src/qemu/qemu_cgroup.c| 14 ++---
  src/qemu/qemu_command.c   | 12 ++--
  src/qemu/qemu_domain.c|  7 +--
  src/qemu/qemu_driver.c| 36 +---
  src/qemu/qemu_migration.c | 85 +--
  src/qemu/qemu_migration_params.c  |  9 ++-
  src/qemu/qemu_monitor.c   | 19 +++---
  src/qemu/qemu_process.c   |  7 +--
  src/remote/remote_daemon_stream.c | 17 +++---
  src/storage/storage_backend_disk.c|  5 +-
  src/storage/storage_backend_logical.c |  5 +-
  src/storage/storage_driver.c  |  8 +--
  src/util/vircgroup.c  | 18 +++---
  src/util/virfirewall.c|  6 +-
  src/vz/vz_driver.c|  9 +--
  tools/virsh.c |  7 +--
  tools/virt-admin.c|  7 +--
  22 files changed, 137 insertions(+), 211 deletions(-)



Reviewed-by: Michal Privoznik 

Michal

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

[libvirt] [PATCH] build: Update .gitignore for build-aux files

[John Ferlan]

Recent changes moved some files to build-aux and created at least
one more.

Signed-off-by: John Ferlan 
---
 .gitignore | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/.gitignore b/.gitignore
index 85ead5c907..30610a37c9 100644
--- a/.gitignore
+++ b/.gitignore
@@ -43,20 +43,22 @@
 /autom4te.cache
 /build-aux/.gitignore
 /build-aux/compile
+/build-aux/config.guess
+/build-aux/config.sub
 /build-aux/depcomp
+/build-aux/install-sh
+/build-aux/ltmain.sh
 /build-aux/missing
 /build-aux/test-driver
 /build/
 /ci/scratch/
 /confdefs.h
 /config.cache
-/config.guess
 /config.h
 /config.h.in
 /config.log
 /config.rpath
 /config.status
-/config.sub
 /configure
 /configure.lineno
 /conftest.*
@@ -96,7 +98,6 @@
 /libvirt*.pc
 /libvirt.spec
 /ltconfig
-/ltmain.sh
 /m4/*
 /mingw-libvirt.spec
 /mkinstalldirs
-- 
2.20.1

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

Re: [libvirt] [PATCH v3 00/10] use VIR_AUTO*/g_auto* all around qemu_driver.c

[Daniel Henrique Barboza]

I just realized by reading a patch from Jano that VIR_AUTOFREE
is simply an alias for g_autofree. This means that it is not worth
to add more VIR_AUTOFREE() macros, and that it is ok to mix
g_autofree with other VIR_ macros such as VIR_AUTOUNREF(). Jano is
also removing VIR_AUTOFREE() from the code in his series, which
will make this whole series obsolete.

I'll re-send this series to not add more VIR_AUTOFREE() in any
step of the way.


DHB


On 10/15/19 5:08 PM, Daniel Henrique Barboza wrote:

changes from v2:
- rebased with newer master (67e72053c1)
- added an extra patch to convert the existing VIR_AUTO* macros
to g_auto* ones, all at once, to avoid the case where a method
will have both VIR_AUTO* and g_auto* macros at the same time.


Note: the conversion in patch 10 wasn't 100% due to a handful of
methods that I was unable to use g_autoptr. Take for example
the method qemuDomainSaveInternal:
--
 qemuDomainObjPrivatePtr priv = vm->privateData;
 VIR_AUTOUNREF(virCapsPtr) caps = NULL;
 virQEMUSaveDataPtr data = NULL;
 VIR_AUTOUNREF(qemuDomainSaveCookiePtr) cookie = NULL;
--

Changing the 'cookie' variable to use g_autoptr() causes an error:

make[3]: Entering directory '/home/danielhb/kvm-project/libvirt/src'
   CC   qemu/libvirt_driver_qemu_impl_la-qemu_driver.lo
In file included from /usr/lib64/glib-2.0/include/glibconfig.h:9,
  from /usr/include/glib-2.0/glib/gtypes.h:32,
  from /usr/include/glib-2.0/glib/galloca.h:32,
  from /usr/include/glib-2.0/glib.h:30,
  from ./internal.h:31,
  from qemu/qemu_agent.h:24,
  from qemu/qemu_driver.c:40:
qemu/qemu_driver.c: In function 'qemuDomainSaveInternal':
qemu/qemu_driver.c:3282:15: error: unknown type name 
'qemuDomainSaveCookie_autoptr'
  3282 | g_autoptr(qemuDomainSaveCookie) cookie = NULL;


I tried doing it with the 'Ptr' in the name, same error:


   CC   qemu/libvirt_driver_qemu_impl_la-qemu_driver.lo
In file included from /usr/lib64/glib-2.0/include/glibconfig.h:9,
  from /usr/include/glib-2.0/glib/gtypes.h:32,
  from /usr/include/glib-2.0/glib/galloca.h:32,
  from /usr/include/glib-2.0/glib.h:30,
  from ./internal.h:31,
  from qemu/qemu_agent.h:24,
  from qemu/qemu_driver.c:40:
qemu/qemu_driver.c: In function 'qemuDomainSaveInternal':
qemu/qemu_driver.c:3282:15: error: unknown type name 
'qemuDomainSaveCookiePtr_autoptr'
  3282 | g_autoptr(qemuDomainSaveCookiePtr) cookie = NULL;


Similar situation happens with qemuDomainSaveImageStartVM and with
qemuSecurityInit methods. They are mentioned in the commit message
of patch 10 for reference. These methods are still using VIR_AUTO*
macros.


changes from v1:
- addressed review concerns made by Erik
- added more cleanups, as suggested by Erik

v2: https://www.redhat.com/archives/libvir-list/2019-September/msg01452.html
v1: https://www.redhat.com/archives/libvir-list/2019-September/msg00719.html

Daniel Henrique Barboza (10):
   qemu_driver: use VIR_AUTOUNREF() with virQEMUDriverConfigPtr 1/3
   qemu_driver: use VIR_AUTOUNREF() with virQEMUDriverConfigPtr 2/3
   qemu_driver: use VIR_AUTOUNREF() with virQEMUDriverConfigPtr 3/3
   qemu_driver: use VIR_AUTOUNREF() with virCapsPtr
   qemu_driver: use VIR_AUTOUNREF() with more pointer types
   qemu_driver: use VIR_AUTOFREE() with strings 1/3
   qemu_driver: use VIR_AUTOFREE() with strings 2/3
   qemu_driver: use VIR_AUTOFREE() with strings 3/3
   qemu_driver: VIR_AUTOFREE on other scalar pointers
   qemu_driver.c: use GLib macros

  src/qemu/qemu_driver.c | 755 ++---
  1 file changed, 259 insertions(+), 496 deletions(-)



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

[libvirt] [RFC] Accepting PRs/MRs for libvirt on GitHub/GitLab

[Andrea Bolognani]

As we look to make the libvirt project easier to contribute to, one
fact that certainly comes to mind is that we only accept patches via
the mailing list. While the core developers are comfortable with the
email-based workflow and swear by it, many perspective contributors
are used to the PR/MR workflow common to many Open Source projects
these days, and similarly swear by it.

If we look at the PRs submitted on GitHub against the libvirt mirror
repository[1], there's just three of them per year on average, which
is arguably not a lot; however, it should be noted that each
repository also carries a fairly loud "PULL REQUESTS ARE IGNORED"
message right at the top, and it's not unlikely that a number of
perspective contributors simply lost interest after seeing it.

As a way to make contributions easier without forcing core developers
to give up their current workflow or making the project dependent on
a third-party provider, I suggest we adopt a hybrid approach.

First of all, we'd remove the ominous message from GitHub mirror
repositories (interestingly, the same is not present on GitLab).

Then, we'd starting accepting PRs/MRs. The way we'd handle them is
that, when one comes in, one among the handful of core developers who
volunteered to do so would review the patches on the respective
platform, working with the submitter to get it into shape just like
they would do on the mailing list; once the series is finally ready
to be merged, the core developer would update the PR/MR as necessary,
for example picking up R-bs or fixing the kind of trivial issues that
usually don't warrant a repost, and then push to master as usual.

More in detail: GitHub and GitLab have a feature that allows project
members to update PRs/MRs: basically the way it works is that, if the
PR/MR was created by the user 'foo' from their branch 'bar', the
libvirt developer is allowed to (force-)push to the foo/libvirt/bar
branch, and doing so updates the corresponding PR/MR; after that, if
the updated branch is locally merged into master and master is pushed
to the libvirt.org repo, once it gets mirrored GitHub/GitLab will
recognize that the commit IDs match and automatically mark the PR/MR
as merged. I have tested this behavior on both platforms (minus the
mirroring part) with Martin's help.

One last important bit. In the spirit of not requiring core
developers to alter their workflow, the developer who reviewed the
patches on GitHub/GitLab will also be responsible to format them to
the mailing list after merging them: this way, even those who don't
have a GitHub/GitLab account will get a chance to take notice of the
code changes and weigh in. Unlike what we're used to, this feedback
will come after the fact, but assuming that issues are spotted only
at that point we can either push the relevant fixes as follow-up
commits or even revert the series outright, so I don't feel like it
would be a massive problem overall.

Thoughts?


[1] https://github.com/libvirt/libvirt/pulls?q=is:pr+is:closed
-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH] qemu: caps: Use unique key for domCaps caching

[Daniel Henrique Barboza]

On 10/15/19 1:01 PM, Cole Robinson wrote:

When searching qemuCaps->domCapsCache for existing domCaps data,
we check for a matching pair of arch+virttype+machine+emulator. However
for the hash table key we only use the machine string. So if the
cache already contains:

   x86_64 + kvm + pc + /usr/bin/qemu-kvm

But a new VM is defined with

   x86_64 + qemu + pc + /usr/bin/qemu-kvm

We correctly fail to find matching cached domCaps, but then attempt
to use a colliding key with virHashAddEntry

Fix this by building a hash key from the 4 values, not just machine

Signed-off-by: Cole Robinson 
---
qemu_domain.c validation should be affected, but it is covered up
by another bug, fixed here:
https://www.redhat.com/archives/libvir-list/2019-October/msg00708.html

  src/qemu/qemu_conf.c | 11 ++-
  1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 08cd784054..64ac8cbdd3 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -1396,6 +1396,8 @@ virQEMUDriverGetDomainCapabilities(virQEMUDriverPtr 
driver,
  domCaps = virHashSearch(domCapsCache,
  virQEMUDriverSearchDomcaps, , NULL);
  if (!domCaps) {
+VIR_AUTOFREE(char *) key = NULL;
+


"g_autofree char *key = NULL;" instead of VIR_AUTOFREE for extra karma
points. Everything else LGTM.



Reviewed-by: Daniel Henrique Barboza 





  /* hash miss, build new domcaps */
  if (!(domCaps = virDomainCapsNew(data.path, data.machine,
   data.arch, data.virttype)))
@@ -1406,7 +1408,14 @@ virQEMUDriverGetDomainCapabilities(virQEMUDriverPtr 
driver,
cfg->firmwares, cfg->nfirmwares) < 0)
  return NULL;
  
-if (virHashAddEntry(domCapsCache, machine, domCaps) < 0)

+if (virAsprintf(, "%d:%d:%s:%s",
+data.arch,
+data.virttype,
+NULLSTR(data.machine),
+NULLSTR(data.path)) < 0)
+return NULL;
+
+if (virHashAddEntry(domCapsCache, key, domCaps) < 0)
  return NULL;
  }
  


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

[libvirt] [PATCH 8/9] Use g_steal_pointer instead of VIR_STEAL_PTR everywhere

[Ján Tomko]

Signed-off-by: Ján Tomko 
---
 src/admin/admin_remote.c   |  2 +-
 src/admin/admin_server_dispatch.c  |  2 +-
 src/bhyve/bhyve_domain.c   |  2 +-
 src/cpu/cpu_x86.c  |  4 ++--
 src/libvirt-domain.c   |  8 
 src/libxl/xen_common.c |  2 +-
 src/locking/lock_driver_lockd.c|  6 +++---
 src/network/bridge_driver.c|  2 +-
 src/phyp/phyp_driver.c |  2 +-
 src/remote/remote_driver.c |  8 
 src/rpc/virnetlibsshsession.c  |  2 +-
 src/secret/secret_driver.c |  4 ++--
 src/security/security_manager.c|  4 ++--
 src/security/virt-aa-helper.c  |  4 ++--
 src/storage/storage_backend_fs.c   |  2 +-
 src/storage/storage_backend_gluster.c  |  4 ++--
 src/storage/storage_backend_iscsi_direct.c |  4 ++--
 src/storage/storage_backend_logical.c  |  4 ++--
 src/storage/storage_backend_rbd.c  |  6 +++---
 src/storage/storage_backend_scsi.c |  2 +-
 src/storage/storage_driver.c   |  4 ++--
 src/storage/storage_file_gluster.c |  2 +-
 src/storage/storage_util.c |  8 
 src/test/test_driver.c | 20 ++--
 src/vbox/vbox_common.c |  6 +++---
 src/vmx/vmx.c  |  2 +-
 src/vz/vz_driver.c |  8 
 src/vz/vz_sdk.c|  4 ++--
 tests/networkxml2conftest.c|  2 +-
 tests/qemuagenttest.c  |  4 ++--
 tests/qemublocktest.c  |  2 +-
 tests/qemusecuritytest.c   |  2 +-
 tests/testutilsqemu.c  |  2 +-
 tests/virstoragetest.c |  2 +-
 34 files changed, 71 insertions(+), 71 deletions(-)

diff --git a/src/admin/admin_remote.c b/src/admin/admin_remote.c
index 8e0752746c..ca5e0c9fe4 100644
--- a/src/admin/admin_remote.c
+++ b/src/admin/admin_remote.c
@@ -451,7 +451,7 @@ remoteAdminConnectGetLoggingOutputs(virAdmConnectPtr conn,
 goto done;
 
 if (outputs)
-VIR_STEAL_PTR(*outputs, ret.outputs);
+*outputs = g_steal_pointer();
 
 rv = ret.noutputs;
 xdr_free((xdrproc_t) xdr_admin_connect_get_logging_outputs_ret, (char *) 
);
diff --git a/src/admin/admin_server_dispatch.c 
b/src/admin/admin_server_dispatch.c
index 4cc41428ed..e56da3ba9f 100644
--- a/src/admin/admin_server_dispatch.c
+++ b/src/admin/admin_server_dispatch.c
@@ -490,7 +490,7 @@ adminDispatchConnectGetLoggingOutputs(virNetServerPtr 
server G_GNUC_UNUSED,
 return -1;
 }
 
-VIR_STEAL_PTR(ret->outputs, outputs);
+ret->outputs = g_steal_pointer();
 ret->noutputs = noutputs;
 
 return 0;
diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c
index bc506bad66..7d24bb602f 100644
--- a/src/bhyve/bhyve_domain.c
+++ b/src/bhyve/bhyve_domain.c
@@ -223,7 +223,7 @@ bhyveDomainDefNamespaceParse(xmlXPathContextPtr ctxt,
 cmd->num_args++;
 }
 
-VIR_STEAL_PTR(*data, cmd);
+*data = g_steal_pointer();
 ret = 0;
 
  cleanup:
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
index 7a52ab5d4f..3de99ca057 100644
--- a/src/cpu/cpu_x86.c
+++ b/src/cpu/cpu_x86.c
@@ -2218,8 +2218,8 @@ x86Decode(virCPUDefPtr cpu,
 VIR_DEBUG("Using CPU model %s (signatures %s) for CPU with signature 
%06lx",
   model->name, NULLSTR(sigs), (unsigned long)signature);
 
-VIR_STEAL_PTR(cpu->model, cpuModel->model);
-VIR_STEAL_PTR(cpu->features, cpuModel->features);
+cpu->model = g_steal_pointer(>model);
+cpu->features = g_steal_pointer(>features);
 cpu->nfeatures = cpuModel->nfeatures;
 cpuModel->nfeatures = 0;
 cpu->nfeatures_max = cpuModel->nfeatures_max;
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 68e83c..dcab179e6e 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -3047,7 +3047,7 @@ virDomainMigrateVersion3Full(virDomainPtr domain,
   VIR_MIGRATE_AUTO_CONVERGE);
 
 VIR_DEBUG("Prepare3 %p flags=0x%x", dconn, destflags);
-VIR_STEAL_PTR(cookiein, cookieout);
+cookiein = g_steal_pointer();
 cookieinlen = cookieoutlen;
 cookieoutlen = 0;
 if (useParams) {
@@ -3111,7 +3111,7 @@ virDomainMigrateVersion3Full(virDomainPtr domain,
  */
 VIR_DEBUG("Perform3 %p uri=%s", domain->conn, uri);
 VIR_FREE(cookiein);
-VIR_STEAL_PTR(cookiein, cookieout);
+cookiein = g_steal_pointer();
 cookieinlen = cookieoutlen;
 cookieoutlen = 0;
 /* dconnuri not relevant in non-P2P modes, so left NULL here */
@@ -3149,7 +3149,7 @@ virDomainMigrateVersion3Full(virDomainPtr domain,
  */
 VIR_DEBUG("Finish3 %p ret=%d", dconn, ret);
 VIR_FREE(cookiein);
-VIR_STEAL_PTR(cookiein, cookieout);
+cookiein = g_steal_pointer();
 cookieinlen = cookieoutlen;

[libvirt] [PATCH 9/9] internal: delete VIR_STEAL_PTR

[Ján Tomko]

Delete the macro to prevent its usage in new code.

The GLib version should be used instead:
p = g_steal_pointer();

Signed-off-by: Ján Tomko 
---
 src/internal.h | 12 
 1 file changed, 12 deletions(-)

diff --git a/src/internal.h b/src/internal.h
index 722cdbc57c..fb17b87baa 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -213,18 +213,6 @@
 (a) = (a) ^ (b); \
 } while (0)
 
-/**
- * VIR_STEAL_PTR:
- *
- * Steals pointer passed as second argument into the first argument. Second
- * argument must not have side effects.
- */
-#define VIR_STEAL_PTR(a, b) \
-do { \
-(a) = (b); \
-(b) = NULL; \
-} while (0)
-
 /**
  * virCheckFlags:
  * @supported: an OR'ed set of supported flags
-- 
2.19.2

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

[libvirt] [PATCH 3/9] internal: delete VIR_RETURN_PTR

[Ján Tomko]

Remove the macro definition to prevent its usage in new code.

Signed-off-by: Ján Tomko 
---
 src/internal.h | 15 ---
 1 file changed, 15 deletions(-)

diff --git a/src/internal.h b/src/internal.h
index 25df0560af..722cdbc57c 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -225,21 +225,6 @@
 (b) = NULL; \
 } while (0)
 
-/**
- * VIR_RETURN_PTR:
- * @ret: pointer to return
- *
- * Returns value of @ret while clearing @ret. This ensures that pointers
- * freed by using VIR_AUTOPTR can be easily passed back to the caller without
- * any temporary variable. @ptr is evaluated more than once.
- */
-#define VIR_RETURN_PTR(ptr) \
-do { \
-typeof(ptr) virTemporaryReturnPointer = (ptr); \
-(ptr) = NULL; \
-return virTemporaryReturnPointer; \
-} while (0)
-
 /**
  * virCheckFlags:
  * @supported: an OR'ed set of supported flags
-- 
2.19.2

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

[libvirt] [PATCH 6/9] tools: use g_steal_pointer instead of VIR_STEAL_PTR

[Ján Tomko]

Signed-off-by: Ján Tomko 
---
 tools/virsh-completer-domain.c| 12 ++--
 tools/virsh-completer-host.c  |  4 ++--
 tools/virsh-completer-interface.c |  2 +-
 tools/virsh-completer-network.c   |  4 ++--
 tools/virsh-completer-nodedev.c   |  4 ++--
 tools/virsh-completer-nwfilter.c  |  4 ++--
 tools/virsh-completer-pool.c  |  4 ++--
 tools/virsh-completer-secret.c|  4 ++--
 tools/virsh-completer-snapshot.c  |  2 +-
 tools/virsh-completer-volume.c|  2 +-
 tools/virsh-domain.c  |  2 +-
 tools/virsh-snapshot.c|  2 +-
 tools/vsh.c   |  2 +-
 13 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/tools/virsh-completer-domain.c b/tools/virsh-completer-domain.c
index 16c209ab36..bb06f468d7 100644
--- a/tools/virsh-completer-domain.c
+++ b/tools/virsh-completer-domain.c
@@ -66,7 +66,7 @@ virshDomainNameCompleter(vshControl *ctl,
 goto cleanup;
 }
 
-VIR_STEAL_PTR(ret, tmp);
+ret = g_steal_pointer();
 
  cleanup:
 for (i = 0; i < ndomains; i++)
@@ -122,7 +122,7 @@ virshDomainInterfaceCompleter(vshControl *ctl,
 return NULL;
 }
 
-VIR_STEAL_PTR(ret, tmp);
+ret = g_steal_pointer();
 return ret;
 }
 
@@ -162,7 +162,7 @@ virshDomainDiskTargetCompleter(vshControl *ctl,
 return NULL;
 }
 
-VIR_STEAL_PTR(ret, tmp);
+ret = g_steal_pointer();
 return ret;
 }
 
@@ -186,7 +186,7 @@ virshDomainEventNameCompleter(vshControl *ctl G_GNUC_UNUSED,
 return NULL;
 }
 
-VIR_STEAL_PTR(ret, tmp);
+ret = g_steal_pointer();
 return ret;
 }
 
@@ -249,7 +249,7 @@ virshDomainInterfaceStateCompleter(vshControl *ctl,
 return NULL;
 }
 
-VIR_STEAL_PTR(ret, tmp);
+ret = g_steal_pointer();
 return ret;
 }
 
@@ -292,7 +292,7 @@ virshDomainDeviceAliasCompleter(vshControl *ctl,
 return NULL;
 }
 
-VIR_STEAL_PTR(ret, tmp);
+ret = g_steal_pointer();
 return ret;
 }
 
diff --git a/tools/virsh-completer-host.c b/tools/virsh-completer-host.c
index ce43818be3..42e59a6656 100644
--- a/tools/virsh-completer-host.c
+++ b/tools/virsh-completer-host.c
@@ -100,7 +100,7 @@ virshAllocpagesPagesizeCompleter(vshControl *ctl,
 return NULL;
 }
 
-VIR_STEAL_PTR(ret, tmp);
+ret = g_steal_pointer();
 return ret;
 }
 
@@ -143,6 +143,6 @@ virshCellnoCompleter(vshControl *ctl,
 return NULL;
 }
 
-VIR_STEAL_PTR(ret, tmp);
+ret = g_steal_pointer();
 return ret;
 }
diff --git a/tools/virsh-completer-interface.c 
b/tools/virsh-completer-interface.c
index 24a69d30b9..6fae0f6584 100644
--- a/tools/virsh-completer-interface.c
+++ b/tools/virsh-completer-interface.c
@@ -57,7 +57,7 @@ virshInterfaceNameCompleter(vshControl *ctl,
 goto cleanup;
 }
 
-VIR_STEAL_PTR(ret, tmp);
+ret = g_steal_pointer();
 
  cleanup:
 for (i = 0; i < nifaces; i++)
diff --git a/tools/virsh-completer-network.c b/tools/virsh-completer-network.c
index 404260b20c..a3d8b71d19 100644
--- a/tools/virsh-completer-network.c
+++ b/tools/virsh-completer-network.c
@@ -59,7 +59,7 @@ virshNetworkNameCompleter(vshControl *ctl,
 goto cleanup;
 }
 
-VIR_STEAL_PTR(ret, tmp);
+ret = g_steal_pointer();
 
  cleanup:
 for (i = 0; i < nnets; i++)
@@ -88,7 +88,7 @@ virshNetworkEventNameCompleter(vshControl *ctl G_GNUC_UNUSED,
 goto cleanup;
 }
 
-VIR_STEAL_PTR(ret, tmp);
+ret = g_steal_pointer();
 
  cleanup:
 return ret;
diff --git a/tools/virsh-completer-nodedev.c b/tools/virsh-completer-nodedev.c
index 9bd1d20945..899c199902 100644
--- a/tools/virsh-completer-nodedev.c
+++ b/tools/virsh-completer-nodedev.c
@@ -57,7 +57,7 @@ virshNodeDeviceNameCompleter(vshControl *ctl,
 goto cleanup;
 }
 
-VIR_STEAL_PTR(ret, tmp);
+ret = g_steal_pointer();
 
  cleanup:
 for (i = 0; i < ndevs; i++)
@@ -86,7 +86,7 @@ virshNodeDeviceEventNameCompleter(vshControl *ctl 
G_GNUC_UNUSED,
 return NULL;
 }
 
-VIR_STEAL_PTR(ret, tmp);
+ret = g_steal_pointer();
 return ret;
 }
 
diff --git a/tools/virsh-completer-nwfilter.c b/tools/virsh-completer-nwfilter.c
index 5a86602101..5029becf09 100644
--- a/tools/virsh-completer-nwfilter.c
+++ b/tools/virsh-completer-nwfilter.c
@@ -55,7 +55,7 @@ virshNWFilterNameCompleter(vshControl *ctl,
 goto cleanup;
 }
 
-VIR_STEAL_PTR(ret, tmp);
+ret = g_steal_pointer();
 
  cleanup:
 for (i = 0; i < nnwfilters; i++)
@@ -95,7 +95,7 @@ virshNWFilterBindingNameCompleter(vshControl *ctl,
 goto cleanup;
 }
 
-VIR_STEAL_PTR(ret, tmp);
+ret = g_steal_pointer();
 
  cleanup:
 for (i = 0; i < nbindings; i++)
diff --git a/tools/virsh-completer-pool.c b/tools/virsh-completer-pool.c
index e646d07e65..ceb73eed06 100644
--- a/tools/virsh-completer-pool.c
+++ b/tools/virsh-completer-pool.c
@@ -60,7 +60,7 @@ 

[libvirt] [PATCH 1/9] util: delete VIR_AUTOFREE

[Ján Tomko]

Commit 1e2ae2e311c7453e7894e93688f8785736aa0618 deleted the last use
of VIR_AUTOFREE but forgot to delete the macro definition.

Signed-off-by: Ján Tomko 
---
 src/util/viralloc.h | 14 --
 1 file changed, 14 deletions(-)

diff --git a/src/util/viralloc.h b/src/util/viralloc.h
index c503bbe19b..d7862d6127 100644
--- a/src/util/viralloc.h
+++ b/src/util/viralloc.h
@@ -488,17 +488,3 @@ void virDisposeString(char **strptr)
  */
 #define VIR_DISPOSE(ptr) virDispose(1 ? (void *) &(ptr) : (ptr), 1, \
 sizeof(*(ptr)), NULL)
-
-
-/**
- * VIR_AUTOFREE:
- * @type: type of the variable to be freed automatically
- *
- * DEPRECATED: use g_autofree for new code. See HACKING
- * for further guidance.
- *
- * Macro to automatically free the memory allocated to
- * the variable declared with it by calling virFree
- * when the variable goes out of scope.
- */
-#define VIR_AUTOFREE(type) g_autofree type
-- 
2.19.2

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

[libvirt] [PATCH 5/9] qemu: use g_steal_pointer instead of VIR_STEAL_PTR

[Ján Tomko]

Signed-off-by: Ján Tomko 
---
 src/qemu/qemu_agent.c|  6 ++--
 src/qemu/qemu_block.c| 20 ++--
 src/qemu/qemu_blockjob.c |  6 ++--
 src/qemu/qemu_capabilities.c | 14 -
 src/qemu/qemu_cgroup.c   |  2 +-
 src/qemu/qemu_command.c  |  2 +-
 src/qemu/qemu_domain.c   | 26 
 src/qemu/qemu_driver.c   | 52 
 src/qemu/qemu_firmware.c | 16 +-
 src/qemu/qemu_hotplug.c  |  4 +--
 src/qemu/qemu_migration.c| 14 -
 src/qemu/qemu_migration_cookie.c |  2 +-
 src/qemu/qemu_migration_params.c |  4 +--
 src/qemu/qemu_monitor.c  | 16 +-
 src/qemu/qemu_monitor_json.c | 16 +-
 src/qemu/qemu_process.c  | 10 +++---
 src/qemu/qemu_vhost_user.c   |  8 ++---
 17 files changed, 109 insertions(+), 109 deletions(-)

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index ccd076d540..bd5b8035ca 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -2163,7 +2163,7 @@ qemuAgentGetFSInfoInternal(qemuAgentPtr mon,
 goto cleanup;
 }
 
-VIR_STEAL_PTR(*info, info_ret);
+*info = g_steal_pointer(_ret);
 ret = ndata;
 
  cleanup:
@@ -2200,7 +2200,7 @@ qemuAgentGetFSInfo(qemuAgentPtr mon,
 goto cleanup;
 }
 
-VIR_STEAL_PTR(*info, info_ret);
+*info = g_steal_pointer(_ret);
 ret = nfs;
 
  cleanup:
@@ -2485,7 +2485,7 @@ qemuAgentGetInterfaces(qemuAgentPtr mon,
 iface->naddrs = addrs_count;
 }
 
-VIR_STEAL_PTR(*ifaces, ifaces_ret);
+*ifaces = g_steal_pointer(_ret);
 ret = ifaces_count;
 
  cleanup:
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 8841006c96..4dc4f2922d 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -184,8 +184,8 @@ qemuBlockNodeNameGetBackingChainBacking(virJSONValuePtr 
next,
 ) < 0)
 return -1;
 
-VIR_STEAL_PTR(data->backing, backingdata);
-VIR_STEAL_PTR(*nodenamedata, data);
+data->backing = g_steal_pointer();
+*nodenamedata = g_steal_pointer();
 
 return 0;
 }
@@ -1675,7 +1675,7 @@ qemuBlockStorageSourceDetachPrepare(virStorageSourcePtr 
src,
 goto cleanup;
 
 if (driveAlias) {
-VIR_STEAL_PTR(data->driveAlias, driveAlias);
+data->driveAlias = g_steal_pointer();
 data->driveAdded = true;
 } else {
 data->formatNodeName = src->nodeformat;
@@ -1704,7 +1704,7 @@ qemuBlockStorageSourceDetachPrepare(virStorageSourcePtr 
src,
 goto cleanup;
 }
 
-VIR_STEAL_PTR(ret, data);
+ret = g_steal_pointer();
 
  cleanup:
 VIR_FREE(driveAlias);
@@ -2035,7 +2035,7 @@ 
qemuBlockStorageSourceCreateGetFormatPropsGeneric(virStorageSourcePtr src,
 qemuBlockStorageSourceCreateAddBacking(backing, props, false) < 0)
 return -1;
 
-VIR_STEAL_PTR(*retprops, props);
+*retprops = g_steal_pointer();
 return 0;
 }
 
@@ -2076,7 +2076,7 @@ 
qemuBlockStorageSourceCreateGetEncryptionLUKS(virStorageSourcePtr src,
 return -1;
 }
 
-VIR_STEAL_PTR(*luksProps, props);
+*luksProps = g_steal_pointer();
 return 0;
 }
 
@@ -2097,7 +2097,7 @@ 
qemuBlockStorageSourceCreateGetFormatPropsLUKS(virStorageSourcePtr src,
   NULL) < 0)
 return -1;
 
-VIR_STEAL_PTR(*props, luksprops);
+*props = g_steal_pointer();
 return 0;
 }
 
@@ -2155,7 +2155,7 @@ 
qemuBlockStorageSourceCreateGetFormatPropsQcow2(virStorageSourcePtr src,
 qemuBlockStorageSourceCreateAddEncryptionQcow(src, qcow2props) < 0)
 return -1;
 
-VIR_STEAL_PTR(*props, qcow2props);
+*props = g_steal_pointer();
 return 0;
 }
 
@@ -2178,7 +2178,7 @@ 
qemuBlockStorageSourceCreateGetFormatPropsQcow(virStorageSourcePtr src,
 qemuBlockStorageSourceCreateAddEncryptionQcow(src, qcowprops) < 0)
 return -1;
 
-VIR_STEAL_PTR(*props, qcowprops);
+*props = g_steal_pointer();
 return 0;
 }
 
@@ -2200,7 +2200,7 @@ 
qemuBlockStorageSourceCreateGetFormatPropsQed(virStorageSourcePtr src,
 if (qemuBlockStorageSourceCreateAddBacking(backing, qedprops, true) < 0)
 return -1;
 
-VIR_STEAL_PTR(*props, qedprops);
+*props = g_steal_pointer();
 return 0;
 }
 
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 6b2c370d9f..3506fa165b 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -627,7 +627,7 @@ qemuBlockJobRewriteConfigDiskSource(virDomainObjPtr vm,
 }
 
 virObjectUnref(persistDisk->src);
-VIR_STEAL_PTR(persistDisk->src, copy);
+persistDisk->src = g_steal_pointer();
 }
 
 
@@ -1148,13 +1148,13 @@ 
qemuBlockJobProcessEventConcludedCopyPivot(virQEMUDriverPtr driver,
  * inherit the rest of the chain */
 if (job->data.copy.shallownew &&
 !virStorageSourceIsBacking(job->disk->mirror->backingStore))
-

[libvirt] [PATCH 2/9] Remove all usage of VIR_RETURN_PTR

[Ján Tomko]

Prefer:
return g_steal_pointer();

Signed-off-by: Ján Tomko 
---
 src/conf/domain_conf.c   |  8 
 src/conf/secret_conf.c   |  2 +-
 src/conf/storage_conf.c  |  2 +-
 src/conf/virnetworkportdef.c |  2 +-
 src/libxl/xen_xl.c   |  2 +-
 src/libxl/xen_xm.c   |  2 +-
 src/qemu/qemu_block.c| 30 +++---
 src/qemu/qemu_blockjob.c | 12 ++--
 src/qemu/qemu_command.c  | 10 +-
 src/qemu/qemu_conf.c |  6 +++---
 src/qemu/qemu_dbus.c |  2 +-
 src/qemu/qemu_domain.c   |  2 +-
 src/qemu/qemu_interface.c|  2 +-
 src/qemu/qemu_monitor_json.c |  2 +-
 src/qemu/qemu_qapi.c |  2 +-
 src/qemu/qemu_slirp.c|  4 ++--
 src/util/virhostdev.c|  4 ++--
 src/util/virpci.c|  2 +-
 tools/virsh-completer.c  |  2 +-
 19 files changed, 49 insertions(+), 49 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 8616f820fc..8b56ff3458 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -16560,7 +16560,7 @@ virDomainDeviceDefParse(const char *xmlStr,
 if (virDomainDeviceDefValidate(dev, def, flags, xmlopt) < 0)
 return NULL;
 
-VIR_RETURN_PTR(dev);
+return g_steal_pointer();
 }
 
 
@@ -21681,7 +21681,7 @@ virDomainDefParseNode(xmlDocPtr xml,
 if (virDomainDefValidate(def, caps, flags, xmlopt) < 0)
 return NULL;
 
-VIR_RETURN_PTR(def);
+return g_steal_pointer();
 }
 
 
@@ -30672,7 +30672,7 @@ virDomainNetDefToNetworkPort(virDomainDefPtr dom,
 
 port->trustGuestRxFilters = iface->trustGuestRxFilters;
 
-VIR_RETURN_PTR(port);
+return g_steal_pointer();
 }
 
 int
@@ -30925,7 +30925,7 @@ virDomainNetDefActualToNetworkPort(virDomainDefPtr dom,
 port->class_id = actual->class_id;
 port->trustGuestRxFilters = actual->trustGuestRxFilters;
 
-VIR_RETURN_PTR(port);
+return g_steal_pointer();
 }
 
 
diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c
index 6ee9315933..3716d5731f 100644
--- a/src/conf/secret_conf.c
+++ b/src/conf/secret_conf.c
@@ -193,7 +193,7 @@ secretXMLParseNode(xmlDocPtr xml, xmlNodePtr root)
 && virSecretDefParseUsage(ctxt, def) < 0)
 return NULL;
 
-VIR_RETURN_PTR(def);
+return g_steal_pointer();
 }
 
 static virSecretDefPtr
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 4c7e7b3f4d..bddede0934 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -718,7 +718,7 @@ virStoragePoolDefParseSourceString(const char *srcSpec,
  node) < 0)
 return NULL;
 
-VIR_RETURN_PTR(def);
+return g_steal_pointer();
 }
 
 
diff --git a/src/conf/virnetworkportdef.c b/src/conf/virnetworkportdef.c
index 32834e41d4..459384611c 100644
--- a/src/conf/virnetworkportdef.c
+++ b/src/conf/virnetworkportdef.c
@@ -253,7 +253,7 @@ virNetworkPortDefParseXML(xmlXPathContextPtr ctxt)
 return NULL;
 }
 
-VIR_RETURN_PTR(def);
+return g_steal_pointer();
 }
 
 
diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c
index 8ae0dbbbd5..cbd3364fe9 100644
--- a/src/libxl/xen_xl.c
+++ b/src/libxl/xen_xl.c
@@ -2247,5 +2247,5 @@ xenFormatXL(virDomainDefPtr def, virConnectPtr conn)
 if (xenFormatXLDomainChannels(conf, def) < 0)
 return NULL;
 
-VIR_RETURN_PTR(conf);
+return g_steal_pointer();
 }
diff --git a/src/libxl/xen_xm.c b/src/libxl/xen_xm.c
index 65882b7c0b..e368482cac 100644
--- a/src/libxl/xen_xm.c
+++ b/src/libxl/xen_xm.c
@@ -616,5 +616,5 @@ xenFormatXM(virConnectPtr conn,
 if (xenFormatXMInputDevs(conf, def) < 0)
 return NULL;
 
-VIR_RETURN_PTR(conf);
+return g_steal_pointer();
 }
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 46584f2f1b..8841006c96 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -255,7 +255,7 @@ qemuBlockNodeNameGetBackingChain(virJSONValuePtr namednodes,
   ) < 0)
 return NULL;
 
-VIR_RETURN_PTR(disks);
+return g_steal_pointer();
 }
 
 
@@ -380,7 +380,7 @@ qemuBlockGetNodeData(virJSONValuePtr data)
   qemuBlockNamedNodesArrayToHash, 
nodedata) < 0)
 return NULL;
 
-VIR_RETURN_PTR(nodedata);
+return g_steal_pointer();
 }
 
 
@@ -449,7 +449,7 @@ qemuBlockStorageSourceGetURI(virStorageSourcePtr src)
 if (VIR_STRDUP(uri->server, src->hosts->name) < 0)
 return NULL;
 
-VIR_RETURN_PTR(uri);
+return g_steal_pointer();
 }
 
 
@@ -514,7 +514,7 @@ 
qemuBlockStorageSourceBuildJSONSocketAddress(virStorageNetHostDefPtr host,
 return NULL;
 }
 
-VIR_RETURN_PTR(server);
+return g_steal_pointer();
 }
 
 
@@ -550,7 +550,7 @@ 
qemuBlockStorageSourceBuildHostsJSONSocketAddress(virStorageSourcePtr src,
 server = NULL;
 }
 
-VIR_RETURN_PTR(servers);
+return g_steal_pointer();
 }
 
 
@@ -617,7 +617,7 @@ 

[libvirt] [PATCH 7/9] util: use g_steal_pointer instead of VIR_STEAL_PTR

[Ján Tomko]

Signed-off-by: Ján Tomko 
---
 src/util/vircgroupv1.c  |  4 ++--
 src/util/vircgroupv2.c  |  2 +-
 src/util/virdbus.c  | 12 +--
 src/util/virdevmapper.c |  2 +-
 src/util/virerror.c |  2 +-
 src/util/virfile.c  |  2 +-
 src/util/virfilecache.c |  2 +-
 src/util/virhostdev.c   |  2 +-
 src/util/viriscsi.c |  4 ++--
 src/util/virjson.c  |  4 ++--
 src/util/virmdev.c  |  6 +++---
 src/util/virnetdevtap.c |  2 +-
 src/util/virnetlink.c   |  6 +++---
 src/util/virnuma.c  |  4 ++--
 src/util/virpci.c   |  4 ++--
 src/util/virresctrl.c   | 10 -
 src/util/virrotatingfile.c  |  2 +-
 src/util/virscsi.c  |  2 +-
 src/util/virscsivhost.c |  2 +-
 src/util/virstorageencryption.c |  2 +-
 src/util/virstoragefile.c   | 36 -
 src/util/virtypedparam.c|  2 +-
 22 files changed, 57 insertions(+), 57 deletions(-)

diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c
index d23045331e..6ab79a1897 100644
--- a/src/util/vircgroupv1.c
+++ b/src/util/vircgroupv1.c
@@ -264,7 +264,7 @@ virCgroupV1ResolveMountLink(const char *mntDir,
 VIR_WARN("Expecting a symlink at %s for controller %s",
  linkSrc, typeStr);
 } else {
-VIR_STEAL_PTR(controller->linkPoint, linkSrc);
+controller->linkPoint = g_steal_pointer();
 }
 }
 
@@ -412,7 +412,7 @@ virCgroupV1StealPlacement(virCgroupPtr group)
 {
 char *ret = NULL;
 
-VIR_STEAL_PTR(ret, group->legacy[VIR_CGROUP_CONTROLLER_SYSTEMD].placement);
+ret = 
g_steal_pointer(>legacy[VIR_CGROUP_CONTROLLER_SYSTEMD].placement);
 
 return ret;
 }
diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c
index 403b3b145e..621ab71eb5 100644
--- a/src/util/vircgroupv2.c
+++ b/src/util/vircgroupv2.c
@@ -244,7 +244,7 @@ virCgroupV2StealPlacement(virCgroupPtr group)
 {
 char *ret;
 
-VIR_STEAL_PTR(ret, group->unified.placement);
+ret = g_steal_pointer(>unified.placement);
 
 return ret;
 }
diff --git a/src/util/virdbus.c b/src/util/virdbus.c
index f423305e11..f54c917c03 100644
--- a/src/util/virdbus.c
+++ b/src/util/virdbus.c
@@ -762,7 +762,7 @@ virDBusMessageIterEncode(DBusMessageIter *rootiter,
 goto cleanup;
 }
 VIR_FREE(contsig);
-VIR_STEAL_PTR(iter, newiter);
+iter = g_steal_pointer();
 types = t + 1;
 nstruct = skiplen;
 narray = (size_t)va_arg(args, int);
@@ -788,7 +788,7 @@ virDBusMessageIterEncode(DBusMessageIter *rootiter,
 VIR_FREE(newiter);
 goto cleanup;
 }
-VIR_STEAL_PTR(iter, newiter);
+iter = g_steal_pointer();
 types = vsig;
 nstruct = strlen(types);
 narray = (size_t)-1;
@@ -819,7 +819,7 @@ virDBusMessageIterEncode(DBusMessageIter *rootiter,
 goto cleanup;
 }
 VIR_FREE(contsig);
-VIR_STEAL_PTR(iter, newiter);
+iter = g_steal_pointer();
 types = t + 1;
 nstruct = skiplen - 2;
 narray = (size_t)-1;
@@ -1056,7 +1056,7 @@ virDBusMessageIterDecode(DBusMessageIter *rootiter,
  nstruct, narray) < 0)
 goto cleanup;
 VIR_FREE(contsig);
-VIR_STEAL_PTR(iter, newiter);
+iter = g_steal_pointer();
 types = t + 1;
 nstruct = skiplen;
 if (arrayref) {
@@ -1086,7 +1086,7 @@ virDBusMessageIterDecode(DBusMessageIter *rootiter,
 VIR_DEBUG("Push failed");
 goto cleanup;
 }
-VIR_STEAL_PTR(iter, newiter);
+iter = g_steal_pointer();
 types = vsig;
 nstruct = strlen(types);
 narray = (size_t)-1;
@@ -1113,7 +1113,7 @@ virDBusMessageIterDecode(DBusMessageIter *rootiter,
  nstruct, narray) < 0)
 goto cleanup;
 VIR_FREE(contsig);
-VIR_STEAL_PTR(iter, newiter);
+iter = g_steal_pointer();
 types = t + 1;
 nstruct = skiplen - 2;
 narray = (size_t)-1;
diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c
index 1abd8044e7..a9996b067c 100644
--- a/src/util/virdevmapper.c
+++ b/src/util/virdevmapper.c
@@ -150,7 +150,7 @@ virDevMapperGetTargetsImpl(const char *path,
 if (virStringListMerge(, ) < 0)
 goto cleanup;
 
-VIR_STEAL_PTR(*devPaths_ret, devPaths);
+*devPaths_ret = g_steal_pointer();
 ret = 0;
  cleanup:
 virStringListFree(recursiveDevPaths);
diff --git a/src/util/virerror.c b/src/util/virerror.c
index f02d6f3b8a..b45c53b7df 100644
--- a/src/util/virerror.c
+++ b/src/util/virerror.c
@@ 

[libvirt] [PATCH 0/9] GLib macros and where to find them (glib chronicles)

[Ján Tomko]

Ján Tomko (9):
  util: delete VIR_AUTOFREE
  Remove all usage of VIR_RETURN_PTR
  internal: delete VIR_RETURN_PTR
  conf: use g_steal_pointer instead of VIR_STEAL_PTR
  qemu: use g_steal_pointer instead of VIR_STEAL_PTR
  tools: use g_steal_pointer instead of VIR_STEAL_PTR
  util: use g_steal_pointer instead of VIR_STEAL_PTR
  Use g_steal_pointer instead of VIR_STEAL_PTR everywhere
  internal: delete VIR_STEAL_PTR

 src/admin/admin_remote.c   |   2 +-
 src/admin/admin_server_dispatch.c  |   2 +-
 src/bhyve/bhyve_domain.c   |   2 +-
 src/conf/checkpoint_conf.c |   2 +-
 src/conf/cpu_conf.c|  18 +--
 src/conf/domain_addr.c |   4 +-
 src/conf/domain_capabilities.c |   4 +-
 src/conf/domain_conf.c | 146 ++---
 src/conf/node_device_conf.c|  12 +-
 src/conf/secret_conf.c |   2 +-
 src/conf/snapshot_conf.c   |   8 +-
 src/conf/storage_conf.c|  10 +-
 src/conf/virnetworkobj.c   |   2 +-
 src/conf/virnetworkportdef.c   |   2 +-
 src/conf/virnwfilterbindingobj.c   |   2 +-
 src/conf/virsecretobj.c|   2 +-
 src/conf/virstorageobj.c   |   4 +-
 src/cpu/cpu_x86.c  |   4 +-
 src/internal.h |  27 
 src/libvirt-domain.c   |   8 +-
 src/libxl/xen_common.c |   2 +-
 src/libxl/xen_xl.c |   2 +-
 src/libxl/xen_xm.c |   2 +-
 src/locking/lock_driver_lockd.c|   6 +-
 src/network/bridge_driver.c|   2 +-
 src/phyp/phyp_driver.c |   2 +-
 src/qemu/qemu_agent.c  |   6 +-
 src/qemu/qemu_block.c  |  50 +++
 src/qemu/qemu_blockjob.c   |  18 +--
 src/qemu/qemu_capabilities.c   |  14 +-
 src/qemu/qemu_cgroup.c |   2 +-
 src/qemu/qemu_command.c|  12 +-
 src/qemu/qemu_conf.c   |   6 +-
 src/qemu/qemu_dbus.c   |   2 +-
 src/qemu/qemu_domain.c |  28 ++--
 src/qemu/qemu_driver.c |  52 
 src/qemu/qemu_firmware.c   |  16 +--
 src/qemu/qemu_hotplug.c|   4 +-
 src/qemu/qemu_interface.c  |   2 +-
 src/qemu/qemu_migration.c  |  14 +-
 src/qemu/qemu_migration_cookie.c   |   2 +-
 src/qemu/qemu_migration_params.c   |   4 +-
 src/qemu/qemu_monitor.c|  16 +--
 src/qemu/qemu_monitor_json.c   |  18 +--
 src/qemu/qemu_process.c|  10 +-
 src/qemu/qemu_qapi.c   |   2 +-
 src/qemu/qemu_slirp.c  |   4 +-
 src/qemu/qemu_vhost_user.c |   8 +-
 src/remote/remote_driver.c |   8 +-
 src/rpc/virnetlibsshsession.c  |   2 +-
 src/secret/secret_driver.c |   4 +-
 src/security/security_manager.c|   4 +-
 src/security/virt-aa-helper.c  |   4 +-
 src/storage/storage_backend_fs.c   |   2 +-
 src/storage/storage_backend_gluster.c  |   4 +-
 src/storage/storage_backend_iscsi_direct.c |   4 +-
 src/storage/storage_backend_logical.c  |   4 +-
 src/storage/storage_backend_rbd.c  |   6 +-
 src/storage/storage_backend_scsi.c |   2 +-
 src/storage/storage_driver.c   |   4 +-
 src/storage/storage_file_gluster.c |   2 +-
 src/storage/storage_util.c |   8 +-
 src/test/test_driver.c |  20 +--
 src/util/viralloc.h|  14 --
 src/util/vircgroupv1.c |   4 +-
 src/util/vircgroupv2.c |   2 +-
 src/util/virdbus.c |  12 +-
 src/util/virdevmapper.c|   2 +-
 src/util/virerror.c|   2 +-
 src/util/virfile.c |   2 +-
 src/util/virfilecache.c|   2 +-
 src/util/virhostdev.c  |   6 +-
 src/util/viriscsi.c|   4 +-
 src/util/virjson.c |   4 +-
 src/util/virmdev.c |   6 +-
 src/util/virnetdevtap.c|   2 +-
 src/util/virnetlink.c  |   6 +-
 src/util/virnuma.c |   4 +-
 src/util/virpci.c  |   6 +-
 src/util/virresctrl.c  |  10 +-
 src/util/virrotatingfile.c |   2 +-
 src/util/virscsi.c |   2 +-
 src/util/virscsivhost.c|   2 +-
 src/util/virstorageencryption.c|   2 +-
 src/util/virstoragefile.c  |  36 ++---
 src/util/virtypedparam.c   

[libvirt] [PATCH 4/9] conf: use g_steal_pointer instead of VIR_STEAL_PTR

[Ján Tomko]

Signed-off-by: Ján Tomko 
---
 src/conf/checkpoint_conf.c   |   2 +-
 src/conf/cpu_conf.c  |  18 ++--
 src/conf/domain_addr.c   |   4 +-
 src/conf/domain_capabilities.c   |   4 +-
 src/conf/domain_conf.c   | 138 +++
 src/conf/node_device_conf.c  |  12 +--
 src/conf/snapshot_conf.c |   8 +-
 src/conf/storage_conf.c  |   8 +-
 src/conf/virnetworkobj.c |   2 +-
 src/conf/virnwfilterbindingobj.c |   2 +-
 src/conf/virsecretobj.c  |   2 +-
 src/conf/virstorageobj.c |   4 +-
 12 files changed, 102 insertions(+), 102 deletions(-)

diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c
index e7b204a4d2..b254dce7fd 100644
--- a/src/conf/checkpoint_conf.c
+++ b/src/conf/checkpoint_conf.c
@@ -197,7 +197,7 @@ virDomainCheckpointDefParse(xmlXPathContextPtr ctxt,
 return NULL;
 }
 
-VIR_STEAL_PTR(ret, def);
+ret = g_steal_pointer();
 return ret;
 }
 
diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
index a6bb9ea263..80b7687b86 100644
--- a/src/conf/cpu_conf.c
+++ b/src/conf/cpu_conf.c
@@ -185,14 +185,14 @@ virCPUDefStealModel(virCPUDefPtr dst,
 char *vendor_id = NULL;
 
 if (keepVendor) {
-VIR_STEAL_PTR(vendor, dst->vendor);
-VIR_STEAL_PTR(vendor_id, dst->vendor_id);
+vendor = g_steal_pointer(>vendor);
+vendor_id = g_steal_pointer(>vendor_id);
 }
 
 virCPUDefFreeModel(dst);
 
-VIR_STEAL_PTR(dst->model, src->model);
-VIR_STEAL_PTR(dst->features, src->features);
+dst->model = g_steal_pointer(>model);
+dst->features = g_steal_pointer(>features);
 dst->microcodeVersion = src->microcodeVersion;
 dst->nfeatures_max = src->nfeatures_max;
 src->nfeatures_max = 0;
@@ -203,8 +203,8 @@ virCPUDefStealModel(virCPUDefPtr dst,
 dst->vendor = vendor;
 dst->vendor_id = vendor_id;
 } else {
-VIR_STEAL_PTR(dst->vendor, src->vendor);
-VIR_STEAL_PTR(dst->vendor_id, src->vendor_id);
+dst->vendor = g_steal_pointer(>vendor);
+dst->vendor_id = g_steal_pointer(>vendor_id);
 }
 }
 
@@ -465,7 +465,7 @@ virCPUDefParseXML(xmlXPathContextPtr ctxt,
 tsc->scaling = scaling;
 }
 
-VIR_STEAL_PTR(def->tsc, tsc);
+def->tsc = g_steal_pointer();
 }
 }
 
@@ -644,7 +644,7 @@ virCPUDefParseXML(xmlXPathContextPtr ctxt,
 def->cache->mode = mode;
 }
 
-VIR_STEAL_PTR(*cpu, def);
+*cpu = g_steal_pointer();
 ret = 0;
 
  cleanup:
@@ -987,7 +987,7 @@ virCPUDefCheckFeatures(virCPUDefPtr cpu,
 }
 }
 
-VIR_STEAL_PTR(*features, list);
+*features = g_steal_pointer();
 return n;
 }
 
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index fc612ee6a2..d0026942aa 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -1757,7 +1757,7 @@ 
virDomainVirtioSerialAddrSetCreateFromDomain(virDomainDefPtr def)
addrs) < 0)
 goto cleanup;
 
-VIR_STEAL_PTR(ret, addrs);
+ret = g_steal_pointer();
  cleanup:
 virDomainVirtioSerialAddrSetFree(addrs);
 return ret;
@@ -2094,7 +2094,7 @@ virDomainUSBAddressHubNew(size_t nports)
 goto cleanup;
 hub->nports = nports;
 
-VIR_STEAL_PTR(ret, hub);
+ret = g_steal_pointer();
  cleanup:
 virDomainUSBAddressHubFree(hub);
 return ret;
diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
index b0fdd15d6c..43a778a505 100644
--- a/src/conf/domain_capabilities.c
+++ b/src/conf/domain_capabilities.c
@@ -229,10 +229,10 @@ virDomainCapsCPUModelsAddSteal(virDomainCapsCPUModelsPtr 
cpuModels,
 return -1;
 
 cpuModels->models[cpuModels->nmodels].usable = usable;
-VIR_STEAL_PTR(cpuModels->models[cpuModels->nmodels].name, *name);
+cpuModels->models[cpuModels->nmodels].name = g_steal_pointer(&*name);
 
 if (blockers)
-VIR_STEAL_PTR(cpuModels->models[cpuModels->nmodels].blockers, 
*blockers);
+cpuModels->models[cpuModels->nmodels].blockers = 
g_steal_pointer(&*blockers);
 
 cpuModels->nmodels++;
 return 0;
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 8b56ff3458..69464a3345 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1379,7 +1379,7 @@ virDomainKeyWrapDefParseXML(virDomainDefPtr def, 
xmlXPathContextPtr ctxt)
 }
 
 if (keywrap->aes || keywrap->dea)
-VIR_STEAL_PTR(def->keywrap, keywrap);
+def->keywrap = g_steal_pointer();
 
 return 0;
 }
@@ -1794,7 +1794,7 @@ virDomainVcpuDefNew(virDomainXMLOptionPtr xmlopt)
 if (VIR_ALLOC(ret) < 0)
 return NULL;
 
-VIR_STEAL_PTR(ret->privateData, priv);
+ret->privateData = g_steal_pointer();
 
 return ret;
 }
@@ -2362,7 +2362,7 @@ virDomainVsockDefNew(virDomainXMLOptionPtr xmlopt)
 !(vsock->privateData = xmlopt->privateData.vsockNew()))
 goto cleanup;
 

Re: [libvirt] [PATCH v3] util: Block SIGPIPE until execve in child process

[Eric Blake]

On 10/16/19 4:02 AM, Daniel P. Berrangé wrote:



The challenge here is that we're in between fork + execve and want signal
handlers back to their defaults at time of execve.

If we set SIGPIPE to SIG_IGN and then execve() will that get reset back
to SIG_DFL automatically ?


Sadly, no.  execve() does not change whether a signal is ignored or 
masked (ignored is more common - a number of CI systems have had issues 
where the child inherits SIGPIPE ignored because the parent forgot to 
reset it, but the child wasn't expecting it; but inheriting a signal 
masked is also a real issue), with the lone exception of SIGCHLD. 
However, execve() _does_ change a signal that is being caught in the 
parent into SIG_DFL post-exec.


That does mean, however, that it is viable to install a no-op SIGPIPE 
handler (SIGPIPE is generated but ignored, I/O gets the EPIPE as 
desired), then post-exec the new process will have SIG_DFL.


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

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

Re: [libvirt] [PATCH 2/2] libvirt-rust Fix Stream::send

[Sahid Orentino Ferdjaoui]

On Fri, Sep 20, 2019 at 10:02:47AM +0200, Martin Kletzander wrote:
> On Thu, Sep 19, 2019 at 05:48:49AM +, Linus Färnstrand wrote:
> > * Handle the -2 error case
> > * Allow sending arbitrary byte array, not just UTF-8 strings
> > * Fix FFI declaration, takes size_t, not c_uint
> > * Return usize to be more idiomatic (type used for slice indexing)
> > 
> > Signed-off-by: Linus Färnstrand 
> > ---
> > src/stream.rs | 21 ++---
> > 1 file changed, 10 insertions(+), 11 deletions(-)
> > 
> > diff --git a/src/stream.rs b/src/stream.rs
> > index af6c8ec..2ca5d0b 100644
> > --- a/src/stream.rs
> > +++ b/src/stream.rs
> > @@ -34,7 +34,7 @@ pub mod sys {
> > extern "C" {
> > fn virStreamSend(c: sys::virStreamPtr,
> >  data: *const libc::c_char,
> > - nbytes: libc::c_uint)
> > + nbytes: libc::size_t)
> >  -> libc::c_int;
> > fn virStreamRecv(c: sys::virStreamPtr,
> >  data: *mut libc::c_char,
> > @@ -105,16 +105,15 @@ impl Stream {
> > }
> > }
> > 
> > -pub fn send(, data: ) -> Result {
> > -unsafe {
> > -let ret = virStreamSend(self.as_ptr(),
> > -string_to_c_chars!(data),
> > -data.len() as libc::c_uint);
> > -if ret == -1 {
> > -return Err(Error::new());
> > -}
> > -return Ok(ret as u32);
> > -}
> > +pub fn send(, data: &[u8]) -> Result {
> > +let ret = unsafe {
> > +virStreamSend(
> > +self.as_ptr(),
> > +data.as_ptr() as *mut libc::c_char,
> > +data.len()
> > +)
> > +};
> > +usize::try_from(ret).map_err(|_| Error::new())
> 
> I guess same as the comment form the first patch should be applied here, but 
> we
> might need to even change the return value so that it is properly 
> distinguished
> as here it has yet another meaning.  Are you basing this on some of your work 
> on
> top of libvirt-rust?  Is this the easy to use (semi-)properly?  If yes, we 
> might
> just as well keep it this way as it is still way better than what we have now.

I think the patch is good as it is now. Of-course distinguish between
-1 and -2 would be the ideal but even previously that was not the
case. We could still improve it in the future.

Thank you Linus for your patch.

I will push it Martin if you don't have objection.

Thanks,
s.

> > }
> > 
> > pub fn recv(, buf:  [u8]) -> Result {
> > --
> > 2.21.0
> > 
> > 
> > --
> > 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

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

[libvirt] [PATCH 2/2] storage: fix build with musl libc

[casantos]

From: Carlos Santos 

On musl _PATH_MOUNTED is defined in paths.h, not in mntent.h, which
causes compilation errors:

storage/storage_backend_fs.c: In function 
'virStorageBackendFileSystemIsMounted':
storage/storage_backend_fs.c:255:23: error: '_PATH_MOUNTED' undeclared (first 
use in this function); did you mean 'XPATH_POINT'?
 if ((mtab = fopen(_PATH_MOUNTED, "r")) == NULL) {
   ^
   XPATH_POINT

Fix this including paths.h if _PATH_MOUNTED is still not defined after
including mntent.h. This also works with glibc and uClibc-ng.

Signed-off-by: Carlos Santos 
---
 src/storage/storage_backend_fs.c   | 3 +++
 src/storage/storage_backend_vstorage.c | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index ed677058ed..fafe2745cc 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -42,6 +42,9 @@ VIR_LOG_INIT("storage.storage_backend_fs");
 #if WITH_STORAGE_FS
 
 # include 
+#ifndef _PATH_MOUNTED
+# include 
+#endif
 
 struct _virNetfsDiscoverState {
 const char *host;
diff --git a/src/storage/storage_backend_vstorage.c 
b/src/storage/storage_backend_vstorage.c
index cec21dccbf..685f78a22f 100644
--- a/src/storage/storage_backend_vstorage.c
+++ b/src/storage/storage_backend_vstorage.c
@@ -7,6 +7,9 @@
 #include "virlog.h"
 #include "virstring.h"
 #include 
+#ifndef _PATH_MOUNTED
+#include 
+#endif
 #include 
 #include 
 #include "storage_util.h"
-- 
2.18.1

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

[libvirt] [PATCH 0/2] Fix build with musl libc

[casantos]

From: Carlos Santos 

- Do not use 'stderr' as a field name, since it's a preprocessor macro
  in stdio.h.
- Include paths.h for _PATH_MOUNTED, if it's not defined in mntent.h.

Found while porting libvirt to Buildroot (https://buildroot.org/).

Carlos Santos (2):
  qemu: fix build with musl libc
  storage: fix build with musl libc

 src/qemu/qemu_process.c| 8 
 src/qemu/qemu_process.h| 2 +-
 src/storage/storage_backend_fs.c   | 3 +++
 src/storage/storage_backend_vstorage.c | 3 +++
 4 files changed, 11 insertions(+), 5 deletions(-)

-- 
2.18.1

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

[libvirt] [PATCH 1/2] qemu: fix build with musl libc

[casantos]

From: Carlos Santos 

On musl libc "stderr" is a preprocessor macro whose expansion leads to
compilation errors:

In file included from qemu/qemu_process.c:66:
qemu/qemu_process.c: In function 'qemuProcessQMPFree':
qemu/qemu_process.c:8418:21: error: expected identifier before '(' token
 VIR_FREE((proc->stderr));
 ^~

Prevent this by renaming the homonymous field in the _qemuProcessQMP
struct to "stdErr".

Signed-off-by: Carlos Santos 
---
 src/qemu/qemu_process.c | 8 
 src/qemu/qemu_process.h | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 9c50c4a1d8..e7a8d74455 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -8448,7 +8448,7 @@ qemuProcessQMPFree(qemuProcessQMPPtr proc)
 VIR_FREE(proc->monpath);
 VIR_FREE(proc->monarg);
 VIR_FREE(proc->pidfile);
-VIR_FREE(proc->stderr);
+VIR_FREE(proc->stdErr);
 VIR_FREE(proc);
 }
 
@@ -8601,7 +8601,7 @@ qemuProcessQMPLaunch(qemuProcessQMPPtr proc)
 virCommandSetGID(proc->cmd, proc->runGid);
 virCommandSetUID(proc->cmd, proc->runUid);
 
-virCommandSetErrorBuffer(proc->cmd, &(proc->stderr));
+virCommandSetErrorBuffer(proc->cmd, &(proc->stdErr));
 
 if (virCommandRun(proc->cmd, ) < 0)
 goto cleanup;
@@ -8611,7 +8611,7 @@ qemuProcessQMPLaunch(qemuProcessQMPPtr proc)
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to start QEMU binary %s for probing: %s"),
proc->binary,
-   proc->stderr ? proc->stderr : _("unknown error"));
+   proc->stdErr ? proc->stdErr : _("unknown error"));
 goto cleanup;
 }
 
@@ -8690,7 +8690,7 @@ qemuProcessQMPConnectMonitor(qemuProcessQMPPtr proc)
  *   ** Send QMP Queries to QEMU using monitor (proc->mon) **
  *   qemuProcessQMPFree(proc);
  *
- * Process error output (proc->stderr) remains available in qemuProcessQMP
+ * Process error output (proc->stdErr) remains available in qemuProcessQMP
  * struct until qemuProcessQMPFree is called.
  */
 int
diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h
index 1d62319092..9af9f967fd 100644
--- a/src/qemu/qemu_process.h
+++ b/src/qemu/qemu_process.h
@@ -212,7 +212,7 @@ struct _qemuProcessQMP {
 char *libDir;
 uid_t runUid;
 gid_t runGid;
-char *stderr;
+char *stdErr;
 char *monarg;
 char *monpath;
 char *pidfile;
-- 
2.18.1

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

Re: [libvirt] [PATCH v2 12/31] build: Export virStringListCopy internal API

[Ján Tomko]

On Tue, Oct 15, 2019 at 05:34:48PM +0200, Jiri Denemark wrote:

Signed-off-by: Jiri Denemark 
---

Notes:
   Version 2:
   - no change

src/libvirt_private.syms | 1 +
1 file changed, 1 insertion(+)



Reviewed-by: Ján Tomko 

Jano


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

Re: [libvirt] [PATCH v2 13/31] qemu: Add qemuMonitorCPUDefsCopy

[Ján Tomko]

On Tue, Oct 15, 2019 at 05:34:49PM +0200, Jiri Denemark wrote:

Signed-off-by: Jiri Denemark 
---

Notes:
   Version 2:
   - no change

src/qemu/qemu_monitor.c | 33 +
src/qemu/qemu_monitor.h |  2 ++
2 files changed, 35 insertions(+)



Reviewed-by: Ján Tomko 

Jano

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index adb6befe4a..614fee98c1 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3582,7 +3582,7 @@ int
qemuMonitorCPUDefsCopy(qemuMonitorCPUDefsPtr *dst,
   qemuMonitorCPUDefsPtr src)
{
-VIR_AUTOPTR(qemuMonitorCPUDefs) defs = NULL;
+g_autoptr(qemuMonitorCPUDefs) defs = NULL;
size_t i;

if (!src) {
@@ -3599,14 +3599,11 @@ qemuMonitorCPUDefsCopy(qemuMonitorCPUDefsPtr *dst,
qemuMonitorCPUDefInfoPtr cpuSrc = src->cpus + i;

cpuDst->usable = cpuSrc->usable;
-
-if (VIR_STRDUP(cpuDst->name, cpuSrc->name) < 0 ||
-virStringListCopy(>blockers,
-  (const char **)cpuSrc->blockers) < 0)
-return -1;
+cpuDst->name = g_strdup(cpuSrc->name);
+cpuDst->blockers = g_strdupv(cpuSrc->blockers);
}

-VIR_STEAL_PTR(*dst, defs);
+*dst = g_steal_pointer();
return 0;
}




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

Re: [libvirt] [PATCH v2 11/31] qemu: Flatten qemuMonitorCPUDefs.cpus

[Ján Tomko]

On Tue, Oct 15, 2019 at 05:34:47PM +0200, Jiri Denemark wrote:

Let's store qemuMonitorCPUDefInfo directly in the array of CPUs in
qemuMonitorCPUDefs rather then using an array of pointers.

Signed-off-by: Jiri Denemark 
Reviewed-by: Ján Tomko 
---

Notes:
   Version 2:
   - trivial rebase

src/qemu/qemu_capabilities.c | 14 +++---
src/qemu/qemu_monitor.c  |  5 ++---
src/qemu/qemu_monitor.h  |  2 +-
src/qemu/qemu_monitor_json.c |  7 +--
tests/qemumonitorjsontest.c  |  8 
5 files changed, 15 insertions(+), 21 deletions(-)



Reviewed-by: Ján Tomko 

@@ -3573,7 +3572,7 @@ qemuMonitorCPUDefsNew(size_t count)
g_autoptr(qemuMonitorCPUDefs) defs = NULL;

defs = g_new0(qemuMonitorCPUDefs, 1);
-defs->cpus = g_new0(qemuMonitorCPUDefInfoPtr, count);
+defs->cpus = g_new0(qemuMonitorCPUDefInfo, count);
defs->ncpus = count;
return g_steal_pointer();
}



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

Re: [libvirt] [libvirt-rust PATCH 1/1] Make creating safe wrapper from raw pointer unsafe

[Sahid Orentino Ferdjaoui]

On Mon, Sep 30, 2019 at 12:10:49PM +0200, Martin Kletzander wrote:
> On Wed, Sep 25, 2019 at 04:36:16PM +, Linus Färnstrand wrote:
> > Giving an invalid pointer to the safe wrapper types causes
> > undefined behavior when methods are later called on said wrapper
> > 
> > Properly document safety contract of using unsafe constructor
> > ---
> 

> Ideally it should not be exposed at all because there already is a
> method for getting the proper structure.  And that method (or those
> methods) already use ::new() so with this patch it just fails.
> Tests, examples, etc.  Instead of fixing that I would, probably,
> just not expose them.  I don't see a reason for them being public.

Yes I'm agree, they should just not be public.

s.

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

Re: [libvirt] [PATCH v2 10/31] qemu: Introduce qemuMonitorCPUDefs struct

[Ján Tomko]

On Tue, Oct 15, 2019 at 05:34:46PM +0200, Jiri Denemark wrote:

It is a container for a CPU models list (qemuMonitorCPUDefInfo) and a
number of elements in this list.

Signed-off-by: Jiri Denemark 
Reviewed-by: Ján Tomko 
---

Notes:
   Version 2:
   - v1 reviewed by Ján Tomko, but the patch had to be changed because
 of the previous patch

src/qemu/qemu_capabilities.c | 30 +--
src/qemu/qemu_monitor.c  | 39 +++--
src/qemu/qemu_monitor.h  | 14 +++--
src/qemu/qemu_monitor_json.c | 56 ++--
src/qemu/qemu_monitor_json.h |  2 +-
tests/qemumonitorjsontest.c  | 38 +---
6 files changed, 93 insertions(+), 86 deletions(-)



Reviewed-by: Ján Tomko 

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 3384b538d0..e4e8d2ed0c 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -2457,7 +2457,7 @@ virQEMUCapsFetchCPUDefinitions(qemuMonitorPtr mon,
   virArch arch,
   virDomainCapsCPUModelsPtr *cpuModels)
{
-VIR_AUTOPTR(qemuMonitorCPUDefs) defs = NULL;
+g_autoptr(qemuMonitorCPUDefs) defs = NULL;
virDomainCapsCPUModelsPtr models = NULL;
size_t i;
int ret = -1;
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 2cce9af929..d1a53f82cb 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3557,29 +3557,25 @@ qemuMonitorCPUDefsFree(qemuMonitorCPUDefsPtr defs)
return;

for (i = 0; i < defs->ncpus; i++) {
-virStringListFree(defs->cpus[i]->blockers);
-VIR_FREE(defs->cpus[i]->name);
-VIR_FREE(defs->cpus[i]);
+g_strfreev(defs->cpus[i]->blockers);
+g_free(defs->cpus[i]->name);
+g_free(defs->cpus[i]);
}

-VIR_FREE(defs->cpus);
-VIR_FREE(defs);
+g_free(defs->cpus);
+g_free(defs);
}


qemuMonitorCPUDefsPtr
qemuMonitorCPUDefsNew(size_t count)
{
-VIR_AUTOPTR(qemuMonitorCPUDefs) defs = NULL;
-
-if (VIR_ALLOC(defs) < 0)
-return NULL;
-
-if (count > 0 && VIR_ALLOC_N(defs->cpus, count) < 0)
-return NULL;
+g_autoptr(qemuMonitorCPUDefs) defs = NULL;

+defs = g_new0(qemuMonitorCPUDefs, 1);
+defs->cpus = g_new0(qemuMonitorCPUDefInfoPtr, count);
defs->ncpus = count;
-VIR_RETURN_PTR(defs);
+return g_steal_pointer();
}


diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 3bce9a971d..4fed575fd3 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -1112,7 +1112,7 @@ int qemuMonitorGetCPUDefinitions(qemuMonitorPtr mon,
 qemuMonitorCPUDefsPtr *cpuDefs);
qemuMonitorCPUDefsPtr qemuMonitorCPUDefsNew(size_t count);
void qemuMonitorCPUDefsFree(qemuMonitorCPUDefsPtr defs);
-VIR_DEFINE_AUTOPTR_FUNC(qemuMonitorCPUDefs, qemuMonitorCPUDefsFree);
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuMonitorCPUDefs, qemuMonitorCPUDefsFree);


typedef enum {
diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
index 104d3a9fc0..b114fabbe0 100644
--- a/tests/qemumonitorjsontest.c
+++ b/tests/qemumonitorjsontest.c
@@ -428,7 +428,7 @@ testQemuMonitorJSONGetCPUDefinitions(const void *opaque)
{
const testGenericData *data = opaque;
virDomainXMLOptionPtr xmlopt = data->xmlopt;
-VIR_AUTOPTR(qemuMonitorCPUDefs) defs = NULL;
+g_autoptr(qemuMonitorCPUDefs) defs = NULL;
g_autoptr(qemuMonitorTest) test = NULL;

if (!(test = qemuMonitorTestNewSchema(xmlopt, data->schema)))



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

Re: [libvirt] [PATCH 1/3] conf/domain_conf: use virStringParseYesNohelper

[Michal Privoznik]

On 10/16/19 12:10 PM, maozy wrote:

Hi, Michal

On 10/16/19 4:38 PM, Michal Privoznik wrote:

On 10/16/19 4:39 AM, Mao Zhongyi wrote:

This helper performs a conversion from a "yes|no" string
to a corresponding boolean, and several conversions were
already done, but there are still some omissions.

For most of the remaining usages in domain_conf.c only
"yes" is explicitly checked for. This means all other
values are implicitly handled as 'false'. In this case,
use virStringParseYesNo, but if it returns -1, don't
raise an error but set the bool value to false.

Cc: crobi...@redhat.com
Cc: berra...@redhat.com
Cc: abolo...@redhat.com
Cc: la...@laine.org
Cc: phrd...@redhat.com
Cc: mklet...@redhat.com
Cc: g.sho1...@gmail.com

Signed-off-by: Mao Zhongyi 
Signed-off-by: Zhang Shengju 
---
  src/conf/domain_conf.c | 30 ++
  1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 10d6bf0eea..7420658726 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7601,8 +7601,8 @@ virDomainHostdevSubsysUSBDefParseXML(xmlNodePtr 
node,

  }
  if ((autoAddress = virXMLPropString(node, "autoAddress"))) {
-    if (STREQ(autoAddress, "yes"))
-    usbsrc->autoAddress = true;
+    if (virStringParseYesNo(autoAddress, >autoAddress) < 0)
+    usbsrc->autoAddress = false;


I think we should either report a proper error here or don't touch 
this line (because this attribute is generated by libvirt itself, it 
doesn't come from an user).


This patch replaces all STREQs that explicitly handle 'yes' to true,
reserving the original implicitly handling of other values to false,
so not report an error, which is also the recommendation of Cole
Robinson in BiteSizedtasks. Therefor, I'm not very sure whether it
is better to report an error in this case? If indeed, will fix it in v2.


Alright then. But what we can use then is:

ignore_value(virStringParseYesNo(autoAddress, >autoAddress));

which is equivallent to existing code and also shorter.






  }
  /* Product can validly be 0, so we need some extra help to 
determine

@@ -8168,8 +8168,8 @@ virDomainHostdevDefParseXMLSubsys(xmlNodePtr node,
   * (e.g. ) with type='hostdev')
   */
  if ((managed = virXMLPropString(node, "managed")) != NULL) {
-    if (STREQ(managed, "yes"))
-    def->managed = true;
+    if (virStringParseYesNo(managed, >managed) < 0)
+    def->managed = false;


Here we need to report a proper error because "managed" attribute 
comes from user, therefore risk of an user providing invalid value is 
high compared to the first hunk. But if we want to be consistent we 
should report error in both cases.



  }
  sgio = virXMLPropString(node, "sgio");
@@ -13807,9 +13807,7 @@ 
virDomainGraphicsListenDefParseXML(virDomainGraphicsListenDefPtr def,

  if (autoGenerated &&
  flags & VIR_DOMAIN_DEF_PARSE_STATUS) {
-    if (STREQ(autoGenerated, "yes")) {
-    def->autoGenerated = true;
-    } else if (STRNEQ(autoGenerated, "no")) {
+    if (virStringParseYesNo(autoGenerated, >autoGenerated) 
< 0) {

  virReportError(VIR_ERR_XML_ERROR,
 _("Invalid autoGenerated value: %s"),
 autoGenerated);


This one is correct.

@@ -13939,12 +13937,11 @@ 
virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def,

  }
  if (autoport) {
-    if (STREQ(autoport, "yes")) {
-    if (flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)
-    def->data.vnc.port = 0;
-    def->data.vnc.autoport = true;
-    } else {
+    if (virStringParseYesNo(autoport, >data.vnc.autoport) < 
0) {

  def->data.vnc.autoport = false;
+    } else {
+    if (def->data.vnc.autoport && flags & 
VIR_DOMAIN_DEF_PARSE_INACTIVE)

+    def->data.vnc.port = 0;


This doesn't need to go under else branch really.


right



  }
  }
@@ -13958,8 +13955,9 @@ 
virDomainGraphicsDefParseXMLVNC(virDomainGraphicsDefPtr def,

  }
  }
-    if (websocketGenerated && STREQ(websocketGenerated, "yes"))
-    def->data.vnc.websocketGenerated = true;
+    if (websocketGenerated &&
+    virStringParseYesNo(websocketGenerated, 
>data.vnc.websocketGenerated))


Ooops, you've missed '< 0' comparison.


virStringParseYesNo return 0 on succeed and -1 on error, and
here is && operation, so, no explicit comparison '< 0'.


The comparison is required because even though the function doesn't 
return any positive value now it might at some point in the future. In 
libvirt, there's the following semantics for integer return values:


< 0 : an error state,
  0 : success,
> 0 : success, but also something is signalized to the caller

There's plenty of examples, but let me pick just one: The retvals for 
virConfGetValueBool() are defined as follows:


-1 on error,
 0 if 

Re: [libvirt] [PATCH v2 09/31] qemu: Change return type of virQEMUCapsFetchCPUDefinitions

[Ján Tomko]

On Tue, Oct 15, 2019 at 05:34:45PM +0200, Jiri Denemark wrote:

The function would return a valid virDomainCapsCPUModelsPtr with empty
CPU models list if query-cpu-definitions exists in QEMU, but returns
GenericError meaning it's not in fact implemented. This behaviour is a
bit strange especially after such virDomainCapsCPUModels structure is
stored in capabilities XML and parsed back, which will result in NULL
virDomainCapsCPUModelsPtr rather than a structure containing nothing.

Let's just keep virDomainCapsCPUModelsPtr NULL if the QMP command is not
implemented and change the return value to int so that callers can
easily check for failure or success.

Signed-off-by: Jiri Denemark 
---

Notes:
   Version 2:
   - new patch

src/qemu/qemu_capabilities.c | 34 +-
src/qemu/qemu_capabilities.h |  5 +++--
src/qemu/qemu_process.c  | 17 ++---
3 files changed, 34 insertions(+), 22 deletions(-)



Reviewed-by: Ján Tomko 

Your call whether you consider squashing in the following, since
VIR_STEAL_PTR still hasn't been eliminated:

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index c8b3b65aad..8e0fae54d7 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -2510,7 +2510,7 @@ virQEMUCapsFetchCPUDefinitions(qemuMonitorPtr mon,
goto cleanup;
}

-VIR_STEAL_PTR(*cpuModels, models);
+*cpuModels = g_steal_pointer();
ret = 0;

 cleanup:



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

Re: [libvirt] [PATCH v2 05/31] qemu: Copy CPU models in virQEMUCapsGetCPUDefinitions

[Ján Tomko]

On Tue, Oct 15, 2019 at 05:34:41PM +0200, Jiri Denemark wrote:

Rather than returning a direct pointer the list stored in qemuCaps the
function now creates a new copy of the CPU models list.

The main purpose of this seemingly useless change is to update callers
to free the result returned by virQEMUCapsGetCPUDefinitions because the
internals of this function will change significantly in the following
patches.

Signed-off-by: Jiri Denemark 
Reviewed-by: Ján Tomko 
---

Notes:
   Version 2:
   - no change

src/qemu/qemu_capabilities.c | 23 ++-
src/qemu/qemu_driver.c   |  2 +-
src/qemu/qemu_process.c  |  7 +--
tests/cputest.c  |  1 -
4 files changed, 24 insertions(+), 9 deletions(-)



Reviewed-by: Ján Tomko 
with the following squashed in:

diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h
index 264c7fc429..36500435fd 100644
--- a/src/conf/domain_capabilities.h
+++ b/src/conf/domain_capabilities.h
@@ -139,6 +139,8 @@ struct _virDomainCapsCPUModels {
virDomainCapsCPUModelPtr models;
};

+G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainCapsCPUModels, virObjectUnref);
+
typedef struct _virDomainCapsCPU virDomainCapsCPU;
typedef virDomainCapsCPU *virDomainCapsCPUPtr;
struct _virDomainCapsCPU {
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index a875896a73..e1750751b4 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -3132,7 +3132,7 @@ virQEMUCapsInitCPUModelX86(virQEMUCapsPtr qemuCaps,
   virCPUDefPtr cpu,
   bool migratable)
{
-VIR_AUTOUNREF(virDomainCapsCPUModelsPtr) cpuModels = NULL;
+g_autoptr(virDomainCapsCPUModels) cpuModels = NULL;
virCPUDataPtr data = NULL;
int ret = -1;

@@ -3227,7 +3227,7 @@ virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps,
if ((rc = virQEMUCapsInitCPUModel(qemuCaps, type, cpu, false)) < 0) {
goto error;
} else if (rc == 1) {
-VIR_AUTOUNREF(virDomainCapsCPUModelsPtr) cpuModels = NULL;
+g_autoptr(virDomainCapsCPUModels) cpuModels = NULL;

VIR_DEBUG("No host CPU model info from QEMU; probing host CPU 
directly");

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 78ba1f6f04..d95bf92cfd 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -13913,7 +13913,7 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,
virQEMUCapsPtr qemuCaps = NULL;
virArch arch;
virDomainVirtType virttype;
-VIR_AUTOUNREF(virDomainCapsCPUModelsPtr) cpuModels = NULL;
+g_autoptr(virDomainCapsCPUModels) cpuModels = NULL;
bool migratable;
virCPUDefPtr cpu = NULL;
char *cpustr = NULL;
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 539ab7ae33..9135a90a0d 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6122,7 +6122,7 @@ qemuProcessUpdateGuestCPU(virDomainDefPtr def,

/* nothing to update for host-passthrough */
if (def->cpu->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) {
-VIR_AUTOUNREF(virDomainCapsCPUModelsPtr) cpuModels = NULL;
+g_autoptr(virDomainCapsCPUModels) cpuModels = NULL;

if (def->cpu->check == VIR_CPU_CHECK_PARTIAL &&
virCPUCompare(caps->host.arch,



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