[libvirt] [PATCH] news: mention 'ramfb' mdev attribute
Signed-off-by: Jonathon Jongsma --- docs/news.xml | 13 + 1 file changed, 13 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index c362bf3a15..046314ef78 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -53,6 +53,19 @@ which were introduced in QEMU 4.2.0. + + + qemu: support boot display for GPU mediated devices + + + Until now, GPU mediated devices generally did not show any output + until the guest OS had initialized the vgpu. By specifying the + ramfb attribute, qemu can be configured to use ramfb as + a boot display for the device. This allows for display of firmware + messages, boot loader menu, and other output before the guest OS has + initialized the vgpu. + + -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [patch 1/1] virt-aa-helper: Add support for smartcard host-certificates
On 10/24/19 4:57 AM, Arnaud Patard wrote:
> When emulating smartcard with host certificates, qemu needs to
> be able to read the certificates files, which is denied by apparmor.
> Add necessary code to add the smartcard certificates related directory
> to the apparmor profile.
>
> This code supports only this case smartcard 'host' and 'passthrough'
> settings are not supported, as I can't test them.
>
> Signed-off-by: Arnaud Patard
> Index: libvirt-5.0.0/src/security/virt-aa-helper.c
> ===
> --- libvirt-5.0.0.orig/src/security/virt-aa-helper.c
> +++ libvirt-5.0.0/src/security/virt-aa-helper.c
> @@ -1251,6 +1251,26 @@ get_files(vahControl * ctl)
> }
> }
>
> +for (i = 0; i < ctl->def->nsmartcards; i++) {
> +virDomainSmartcardDefPtr sc = ctl->def->smartcards[i];
> +virDomainSmartcardType sc_type = sc->type;
> +char *sc_db = (char *)VIR_DOMAIN_SMARTCARD_DEFAULT_DATABASE;
> +if (sc->data.cert.database)
> +sc_db = sc->data.cert.database;
> +switch(sc_type) {
Add a space after 'switch'. 'make syntax-check' will catch this. libvirt
style is typically to not indent the 'case' keyword either, but this
file is inconsistent on that front. With those fixed:
Reviewed-by: Cole Robinson
This matches what is done for the selinux driver AFAICT
CCing apparmor maintainers, I'll defer to them to commit
- Cole
> +case VIR_DOMAIN_SMARTCARD_TYPE_HOST:
> +break;
> +case VIR_DOMAIN_SMARTCARD_TYPE_HOST_CERTIFICATES:
> +virBufferAsprintf(&buf, " \"%s/\" rk,\n", sc_db);
> +virBufferAsprintf(&buf, " \"%s/*\" rk,\n", sc_db);
> +break;
> +case VIR_DOMAIN_SMARTCARD_TYPE_PASSTHROUGH:
> +break;
> +case VIR_DOMAIN_SMARTCARD_TYPE_LAST:
> +break;
> +}
> +}
> +
> if (ctl->def->virtType == VIR_DOMAIN_VIRT_KVM) {
> for (i = 0; i < ctl->def->nnets; i++) {
> virDomainNetDefPtr net = ctl->def->nets[i];
>
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/7] conf: add hypervisor agnostic, domain start-time, validation function for NetDef
On 10/22/19 9:24 PM, Laine Stump wrote:
> devices (virDomainNetDef) are a bit different from other
> types of devices in that their actual type may come from a network (in
> the form of a port connection), and that doesn't happen until the
> domain is started. This means that any validation of an at
> parse time needs to be a bit liberal in what it accepts - when
> type='network', you could think that something is/isn't allowed, but
> once the domain is started and a port is created by the configured
> network, the opposite might be true.
>
> To solve this problem hypervisor drivers need to do an extra
> validation step when the domain is being started. I recently (commit
> 3cff23f7, libvirt 5.7.0) added a function to peform such validation
> for all interfaces to the QEMU driver -
> qemuDomainValidateActualNetDef() - but while that function is a good
> single point to call for the multiple places that need to "start" an
> interface (domain startup, device hotplug, device update), it can't be
> called by the other hypervisor drivers, since 1) it's in the QEMU
> driver, and 2) it contains some checks specific to QEMU. For
> validation that applies to network devices on *all* hypervisors, we
> need yet another interface validation function that can be called by
> any hypervisor driver (not just QEMU) right after its network port has
> been created during domain startup or hotplug. This patch adds that
> function - virDomainNetDefRuntimeValidate(), in the conf directory,
> and calls it in appropriate places in the QEMU, lxc, and libxl
> drivers.
>
> The new function, virDomainNetDefRuntimeValidate(), should contain
> network device validation that 1) is hypervisor agnostic, and 2) can't
> be done until we know the "actual type" of an interface.
>
> There is no framework for validation at domain startup as there is for
> post-parse validation, but I don't want to create a whole elaborate
> system that will only be used by one type of device. For that reason,
> I just made a single function that should be called directly from the
> hypervisors, when they are initializing interfaces to start a domain,
> right after conditionally allocating the network port (and regardless
> of whether or not that was actually needed). In the case of the QEMU
> driver, qemuDomainValidateActualNetDef() is already called in all the
> appropriate places, so we can just call the new function from
> there. In the case of the other hypervisors, we search for
> virDomainNetAllocateActualDevice() (which is the hypervisor-agnostic
> function that calls virNetworkPortCreateXML()), and add the call to our
> new function right after that.
>
> The new function itself could be plunked down into many places in the
> code, but we already have 3 validation functions for network devices
> in 2 different places (not counting any basic validation done in
> virDomainNetDefParseXML() itself):
>
> 1) post-parse hypervisor-agnostic
>(virDomainNetDefValidate() - domain_conf.c:6145)
> 2) post-parse hypervisor-specific
>(qemuDomainDeviceDefValidateNetwork() - qemu_domain.c:5498)
> 3) domain-start hypervisor-specific
>(qemuDomainValidateActualNetDef() - qemu_domain.c:5390)
>
> I placed (3) right next to (2) when I added it, specifically to avoid
> spreading validation all over the code. For the same reason, I decided
> to put this new function right next to (1) - this way if someone needs
> to add validation specific to qemu, they go to one location, and if
> they need to add validation applying to everyone, they go to the
> other. It looks a bit strange to have a public function in between a
> bunch of statics, but I think it's better than the alternative of
> further fragmentation. (I'm open to other ideas though, of course.)
>
> Signed-off-by: Laine Stump
> ---
> src/conf/domain_conf.c | 22 ++
> src/conf/domain_conf.h | 3 +++
> src/libvirt_private.syms | 1 +
> src/libxl/libxl_domain.c | 4
> src/libxl/libxl_driver.c | 4
> src/lxc/lxc_driver.c | 4
> src/lxc/lxc_process.c| 4
> src/qemu/qemu_domain.c | 6 ++
> 8 files changed, 48 insertions(+)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 01a1a74946..c063f40a4c 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -6094,6 +6094,28 @@ virDomainRedirdevDefValidate(const virDomainDef *def,
> }
>
>
> +int
> +virDomainNetDefRuntimeValidate(const virDomainNetDef *net G_GNUC_UNUSED)
> +{
> +/* Unlike virDomainNetDefValidate(), which is a static function
> + * called internally to this file, virDomainActualNetDefValidate()
> + * is a public function that can be called from a hypervisor after
> + * it has completely setup the NetDef for use by a domain,
> + * including possibly allocating a port from the network driver
> + * (which could change the effective/"actual" type of the NetDef,
> + * thus changing what should/shouldn't be allowed by va
[libvirt] [PATCH v3 3/4] conf: report errors when parsing video acceleration
Since this function is now only called when an 'acceleration' element is
present in the xml, any failure to parse the element will be considered
an error.
Previously, we detected some types of errors, but we would only log an
error (virReportError()), but still return a partially-specified accel
object to the caller. This patch returns NULL for all parsing errors and
reports that error back up to the caller.
Signed-off-by: Jonathon Jongsma
---
src/conf/domain_conf.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b0599c7318..c0f20c928f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15269,8 +15269,11 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
accel2d = virXMLPropString(node, "accel2d");
rendernode = virXMLPropString(node, "rendernode");
-if (!accel3d && !accel2d && !rendernode)
+if (!accel3d && !accel2d && !rendernode) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("missing values for acceleration"));
return NULL;
+}
def = g_new0(virDomainVideoAccelDef, 1);
@@ -15278,7 +15281,7 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
if ((val = virTristateBoolTypeFromString(accel3d)) <= 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("unknown accel3d value '%s'"), accel3d);
-goto cleanup;
+return NULL;
}
def->accel3d = val;
}
@@ -15287,7 +15290,7 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
if ((val = virTristateBoolTypeFromString(accel2d)) <= 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("unknown accel2d value '%s'"), accel2d);
-goto cleanup;
+return NULL;
}
def->accel2d = val;
}
@@ -15295,7 +15298,6 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
if (rendernode)
def->rendernode = virFileSanitizePath(rendernode);
- cleanup:
return g_steal_pointer(&def);
}
@@ -15414,8 +15416,10 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
while (child != NULL) {
if (child->type == XML_ELEMENT_NODE) {
if (def->accel == NULL &&
-virXMLNodeNameEqual(child, "acceleration"))
-def->accel = virDomainVideoAccelDefParseXML(child);
+virXMLNodeNameEqual(child, "acceleration")) {
+if ((def->accel =
virDomainVideoAccelDefParseXML(child)) == NULL)
+goto error;
+}
if (def->res == NULL &&
virXMLNodeNameEqual(child, "resolution")) {
if ((def->res =
virDomainVideoResolutionDefParseXML(child)) == NULL)
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 2/4] conf: report errors when parsing video resolution
The current code doesn't properly handle errors when parsing a video
device's resolution. We were returning a NULL structure for the case
where 'x' or 'y' were missing. But for the other error cases, we were
logging an error (virReportError()), but still returning an
under-specified structure. That under-specified structure was used by
the calling function rather than properly reporting an error.
This patch changes the parse function to return NULL on any parsing
error and changes the calling function to report an error when NULL is
returned.
Signed-off-by: Jonathon Jongsma
---
src/conf/domain_conf.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5c86729258..b0599c7318 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15309,24 +15309,26 @@ virDomainVideoResolutionDefParseXML(xmlNodePtr node)
x = virXMLPropString(node, "x");
y = virXMLPropString(node, "y");
-if (!x || !y)
+if (!x || !y) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("missing values for resolution"));
return NULL;
+}
def = g_new0(virDomainVideoResolutionDef, 1);
if (virStrToLong_uip(x, NULL, 10, &def->x) < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("cannot parse video x-resolution '%s'"), x);
-goto cleanup;
+return NULL;
}
if (virStrToLong_uip(y, NULL, 10, &def->y) < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("cannot parse video y-resolution '%s'"), y);
-goto cleanup;
+return NULL;
}
- cleanup:
return g_steal_pointer(&def);
}
@@ -15415,8 +15417,10 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
virXMLNodeNameEqual(child, "acceleration"))
def->accel = virDomainVideoAccelDefParseXML(child);
if (def->res == NULL &&
-virXMLNodeNameEqual(child, "resolution"))
-def->res =
virDomainVideoResolutionDefParseXML(child);
+virXMLNodeNameEqual(child, "resolution")) {
+if ((def->res =
virDomainVideoResolutionDefParseXML(child)) == NULL)
+goto error;
+}
}
child = child->next;
}
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 1/4] conf: iterate video model children in parent function
Previously, we were passing the video "model" node to the "acceleration"
and "resolution" parsing functions and requiring them to iterate over
the children to discover and parse the appropriate node. It makes more
sense to move this responsibility up to the parent function and just
pass these functions the node that needs to be parsed.
Signed-off-by: Jonathon Jongsma
---
src/conf/domain_conf.c | 45 +-
1 file changed, 18 insertions(+), 27 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 561e25ff6e..5c86729258 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15259,25 +15259,15 @@ virDomainVideoDefaultType(const virDomainDef *def)
static virDomainVideoAccelDefPtr
virDomainVideoAccelDefParseXML(xmlNodePtr node)
{
-xmlNodePtr cur;
g_autofree virDomainVideoAccelDefPtr def = NULL;
int val;
g_autofree char *accel2d = NULL;
g_autofree char *accel3d = NULL;
g_autofree char *rendernode = NULL;
-cur = node->children;
-while (cur != NULL) {
-if (cur->type == XML_ELEMENT_NODE) {
-if (!accel3d && !accel2d &&
-virXMLNodeNameEqual(cur, "acceleration")) {
-accel3d = virXMLPropString(cur, "accel3d");
-accel2d = virXMLPropString(cur, "accel2d");
-rendernode = virXMLPropString(cur, "rendernode");
-}
-}
-cur = cur->next;
-}
+accel3d = virXMLPropString(node, "accel3d");
+accel2d = virXMLPropString(node, "accel2d");
+rendernode = virXMLPropString(node, "rendernode");
if (!accel3d && !accel2d && !rendernode)
return NULL;
@@ -15312,22 +15302,12 @@ virDomainVideoAccelDefParseXML(xmlNodePtr node)
static virDomainVideoResolutionDefPtr
virDomainVideoResolutionDefParseXML(xmlNodePtr node)
{
-xmlNodePtr cur;
g_autofree virDomainVideoResolutionDefPtr def = NULL;
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;
-}
+x = virXMLPropString(node, "x");
+y = virXMLPropString(node, "y");
if (!x || !y)
return NULL;
@@ -15415,6 +15395,7 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
if (cur->type == XML_ELEMENT_NODE) {
if (!type && !vram && !ram && !heads &&
virXMLNodeNameEqual(cur, "model")) {
+xmlNodePtr child;
type = virXMLPropString(cur, "type");
ram = virXMLPropString(cur, "ram");
vram = virXMLPropString(cur, "vram");
@@ -15427,8 +15408,18 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
VIR_FREE(primary);
}
-def->accel = virDomainVideoAccelDefParseXML(cur);
-def->res = virDomainVideoResolutionDefParseXML(cur);
+child = cur->children;
+while (child != NULL) {
+if (child->type == XML_ELEMENT_NODE) {
+if (def->accel == NULL &&
+virXMLNodeNameEqual(child, "acceleration"))
+def->accel = virDomainVideoAccelDefParseXML(child);
+if (def->res == NULL &&
+virXMLNodeNameEqual(child, "resolution"))
+def->res =
virDomainVideoResolutionDefParseXML(child);
+}
+child = child->next;
+}
}
if (virXMLNodeNameEqual(cur, "driver")) {
if (virDomainVirtioOptionsParseXML(cur, &def->virtio) < 0)
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 0/4] Fix up some issues from x and y resolution patches
This is the third version of this patch series after a review from Cole. The main changes to this version of the patch series are: - several patches have been pushed upstream (and therefore dropped from this series - The iteration over child nodes was moved up to the caller due to a suggestion from Cole (much better, thanks) - Because the parse functions are now only called when the appropriate XML element is present, the parse functions no longer need to return "successful" NULL for the case where the element is not specified in the XML. Therefore we do not need to change the function signatures and we can simply treat a NULL return value as an error. - the resolution validation is moved to virDomainVideoDefValidate(). Jonathon Jongsma (4): conf: iterate video model children in parent function conf: report errors when parsing video resolution conf: report errors when parsing video acceleration conf: validate video resolution src/conf/domain_conf.c | 75 ++ 1 file changed, 40 insertions(+), 35 deletions(-) -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v3 4/4] conf: validate video resolution
Ensure that both x and y are non-zero when resolution is specified for a
video device.
Signed-off-by: Jonathon Jongsma
---
src/conf/domain_conf.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c0f20c928f..54d6ae297e 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6319,6 +6319,12 @@ virDomainVideoDefValidate(const virDomainVideoDef *video,
return -1;
}
+if (video->res && (video->res->x == 0 || video->res->y == 0)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("video resolution values must be greater than 0"));
+return -1;
+}
+
return 0;
}
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/3] util: Remove unnecessary check in virFileRewrite
Since g_strdup_printf will abort, we know @newfile won't be NULL. Found by Coverity Signed-off-by: John Ferlan --- src/util/virfile.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index cb6cfc0fe7..fca7ff9d35 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -543,8 +543,7 @@ virFileRewrite(const char *path, cleanup: VIR_FORCE_CLOSE(fd); -if (newfile) -unlink(newfile); +unlink(newfile); return ret; } -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/3] network: Check for QOS before blindly using it
If networkAllocatePort calls networkPlugBandwidth eventually the
port->bandwidth would be passed to virNetDevBandwidthPlug which
requires that the parameter is non-NULL. Coverity additionally
notes that since (!port->bandwidth) is checked earlier in the
networkAllocatePort method that the subsequent call to blindly
use if for a function that requires it needs to check.
Signed-off-by: John Ferlan
---
src/network/bridge_driver.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 68bb916501..9c49c70564 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -4567,6 +4567,13 @@ networkAllocatePort(virNetworkObjPtr obj,
return -1;
}
+if (!port->bandwidth) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("QOS must be defined for network '%s'"),
+ netdef->name);
+return -1;
+}
+
if (networkPlugBandwidth(obj, &port->mac, port->bandwidth,
&port->class_id) < 0)
return -1;
break;
@@ -4633,6 +4640,13 @@ networkAllocatePort(virNetworkObjPtr obj,
}
}
+if (!port->bandwidth) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("QOS must be defined for network '%s'"),
+ netdef->name);
+return -1;
+}
+
if (networkPlugBandwidth(obj, &port->mac, port->bandwidth,
&port->class_id) < 0)
return -1;
break;
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/3] A few more Coverity patches
Finally got Coverity to build with the recent build env changes. With doing that Coverity re-examines everything and bridge_driver generated a couple of longer term things. The virFileRewrite is from more recent changes. John Ferlan (3): util: Remove unnecessary check in virFileRewrite network: Use local variables in networkUpdatePortBandwidth network: Check for QOS before blindly using it src/network/bridge_driver.c | 31 ++- src/util/virfile.c | 3 +-- 2 files changed, 23 insertions(+), 11 deletions(-) -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/3] network: Use local variables in networkUpdatePortBandwidth
We go through the trouble of checking {old|new}Bandwidth[->in] and
storing the result in local @old_floor and @new_floor, but then
we don't use them. Instead we make derefs to the longer name. This
caused Coverity to note dereferencing newBandwidth->in without first
checking @newBandwidth like was done for new_floor could cause a
NULL dereference.
Signed-off-by: John Ferlan
---
src/network/bridge_driver.c | 17 -
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 7ee5d7ee53..68bb916501 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -5380,19 +5380,18 @@ networkUpdatePortBandwidth(virNetworkObjPtr obj,
/* Okay, there are three possible scenarios: */
-if (oldBandwidth && oldBandwidth->in && oldBandwidth->in->floor &&
-newBandwidth->in && newBandwidth->in->floor) {
+if (old_floor > 0 && new_floor > 0) {
/* Either we just need to update @floor .. */
if (virNetDevBandwidthUpdateRate(def->bridge,
*class_id,
def->bandwidth,
- newBandwidth->in->floor) < 0)
+ new_floor) < 0)
return -1;
tmp_floor_sum = virNetworkObjGetFloorSum(obj);
-tmp_floor_sum -= oldBandwidth->in->floor;
-tmp_floor_sum += newBandwidth->in->floor;
+tmp_floor_sum -= old_floor;
+tmp_floor_sum += new_floor;
virNetworkObjSetFloorSum(obj, tmp_floor_sum);
new_rate -= tmp_floor_sum;
@@ -5401,17 +5400,17 @@ networkUpdatePortBandwidth(virNetworkObjPtr obj,
virNetworkObjSaveStatus(driver->stateDir,
obj, network_driver->xmlopt) < 0) {
/* Ouch, rollback */
-tmp_floor_sum -= newBandwidth->in->floor;
-tmp_floor_sum += oldBandwidth->in->floor;
+tmp_floor_sum -= new_floor;
+tmp_floor_sum += old_floor;
virNetworkObjSetFloorSum(obj, tmp_floor_sum);
ignore_value(virNetDevBandwidthUpdateRate(def->bridge,
*class_id,
def->bandwidth,
-
oldBandwidth->in->floor));
+ old_floor));
return -1;
}
-} else if (newBandwidth->in && newBandwidth->in->floor) {
+} else if (new_floor > 0) {
/* .. or we need to plug in new .. */
if (networkPlugBandwidthImpl(obj, mac, newBandwidth,
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [ruby PATCH] Fix default values for node_cpu_stats() and node_memory_stats()
On 10/22/19 11:47 AM, Stefano Garzarella wrote:
> ruby_libvirt_value_to_int() returns 0 if the optional value is
> not defined, but in node_cpu_stats() and node_memory_stats()
> the default value of cpuNum and cellNum is -1.
>
I know nothing about ruby or the libvirt bindings, but what you describe
makes sense and matches my reading of the code. -1 is
VIR_NODE_CPU_STATS_ALL_CPUS and VIR_NODE_MEMORY_STATS_ALL_CELLS so it
makes sense that would be the default.
Reviewed-by: Cole Robinson
CCing clalance who is the primary ruby-libvirt maintainer, but if he
doesn't get to it by next week then I'll figure out how to build test
it, and push
- Cole
> Reported-by: Charlie Smurthwaite
> Signed-off-by: Stefano Garzarella
> ---
> ext/libvirt/connect.c | 14 --
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/ext/libvirt/connect.c b/ext/libvirt/connect.c
> index 5932535..b2d041b 100644
> --- a/ext/libvirt/connect.c
> +++ b/ext/libvirt/connect.c
> @@ -2079,7 +2079,12 @@ static VALUE libvirt_connect_node_cpu_stats(int argc,
> VALUE *argv, VALUE c)
>
> rb_scan_args(argc, argv, "02", &intparam, &flags);
>
> -tmp = ruby_libvirt_value_to_int(intparam);
> +if (NIL_P(intparam)) {
> +tmp = -1;
> +}
> +else {
> +tmp = ruby_libvirt_value_to_int(intparam);
> +}
>
> return ruby_libvirt_get_parameters(c, ruby_libvirt_value_to_uint(flags),
> (void *)&tmp, sizeof(virNodeCPUStats),
> @@ -2139,7 +2144,12 @@ static VALUE libvirt_connect_node_memory_stats(int
> argc, VALUE *argv, VALUE c)
>
> rb_scan_args(argc, argv, "02", &intparam, &flags);
>
> -tmp = ruby_libvirt_value_to_int(intparam);
> +if (NIL_P(intparam)) {
> +tmp = -1;
> +}
> +else {
> +tmp = ruby_libvirt_value_to_int(intparam);
> +}
>
> return ruby_libvirt_get_parameters(c, ruby_libvirt_value_to_uint(flags),
> (void *)&tmp,
> sizeof(virNodeMemoryStats),
>
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] lxc: Fix 'domblkstat' error with attached disk devices.
On 10/20/19 11:54 PM, jcfara...@gmail.com wrote:
> From: Julio Faracco
>
I think if you set your gitconfig correctly, you can avoid this 'From:'
showing up. I have:
[sendemail]
from = "Cole Robinson "
[user]
name = Cole Robinson
email = crobi...@redhat.com
> LXC was not working when you attached a new disk that points to block
> device. See: https://bugzilla.redhat.com/show_bug.cgi?id=1697115.
> Command line from virsh was showing problems with alias first (this
> feature is not supported) and after, problems to query block device.
If you are fixing two distinct issues, this should at be at least two
separate patches. Otherwise review is more difficult among other things
It
> was happening because extra disks were not being included into
> cgroup blkio.throttle properties and libvirt could not query this info.
> Applying those devices into 'allowed' list is not enough, libvirt should
> reset blkio.throttle as a workaround to include disks into container's
> cgroup directory.
>
> Before:
>
> virsh # domblkstat CentOS hda
> error: Failed to get block stats for domain 'CentOS' device 'hda'
> error: internal error: domain stats query failed
>
> Now:
>
> virsh # domblkstat CentOS hda
> hda rd_req 0
> hda rd_bytes 0
> hda wr_req 0
> hda wr_bytes 0
>
> Signed-off-by: Julio Faracco
> ---
> src/lxc/lxc_cgroup.c | 29 -
> src/lxc/lxc_cgroup.h | 4
> src/lxc/lxc_driver.c | 8
> 3 files changed, 36 insertions(+), 5 deletions(-)
>
> diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c
> index 5efb495b56..de6d892521 100644
> --- a/src/lxc/lxc_cgroup.c
> +++ b/src/lxc/lxc_cgroup.c
> @@ -302,6 +302,24 @@ virLXCTeardownHostUSBDeviceCgroup(virUSBDevicePtr dev
> G_GNUC_UNUSED,
> return 0;
> }
>
> +int
> +virLXCCgroupResetBlkioDeviceThrottle(virCgroupPtr cgroup,
> + const char *path)
> +{
> +if (virCgroupSetBlkioDeviceReadBps(cgroup, path, 0) < 0)
> +return -1;
> +
> +if (virCgroupSetBlkioDeviceWriteBps(cgroup, path, 0) < 0)
> +return -1;
> +
> +if (virCgroupSetBlkioDeviceReadIops(cgroup, path, 0) < 0)
> +return -1;
> +
> +if (virCgroupSetBlkioDeviceWriteIops(cgroup, path, 0) < 0)
> +return -1;
> +
> +return 0;
> +}
>
> static int virLXCCgroupSetupDeviceACL(virDomainDefPtr def,
>virCgroupPtr cgroup)
> @@ -309,6 +327,7 @@ static int virLXCCgroupSetupDeviceACL(virDomainDefPtr def,
> int capMknod = def->caps_features[VIR_DOMAIN_CAPS_FEATURE_MKNOD];
> int ret = -1;
> size_t i;
> +const char *src_path = NULL;
> static virLXCCgroupDevicePolicy devices[] = {
> {'c', LXC_DEV_MAJ_MEMORY, LXC_DEV_MIN_NULL},
> {'c', LXC_DEV_MAJ_MEMORY, LXC_DEV_MIN_ZERO},
> @@ -346,8 +365,16 @@ static int virLXCCgroupSetupDeviceACL(virDomainDefPtr
> def,
> !virStorageSourceIsBlockLocal(def->disks[i]->src))
> continue;
>
> +/* Workaround to include disks into blkio.throttle.
> + * To include it, we need to reset any feature of
> + * blkio.throttle.* */
> +src_path = virDomainDiskGetSource(def->disks[i]);
> +if (virLXCCgroupResetBlkioDeviceThrottle(cgroup,
> + src_path) < 0)
> +goto cleanup;
> +
I'll admit I don't really know how cgroups work here. But it seems odd.
Why exactly is this needed? How does blkstats fail without this, after
the alias piece is fixed? Is something similar needed for the qemu
driver, and if not, why the difference?
Also FWIW, not sure if you have fedora 31 installed anywhere, but seems
like the lxc driver is completely broken with cgroupv2:
https://bugzilla.redhat.com/show_bug.cgi?id=1770763
The suggestion in the bug sounds simple though. If you wanted to
separately take a stab at that I'm motivated to test and review. Just a
thought
> if (virCgroupAllowDevicePath(cgroup,
> - virDomainDiskGetSource(def->disks[i]),
> + src_path,
> (def->disks[i]->src->readonly ?
>VIR_CGROUP_DEVICE_READ :
>VIR_CGROUP_DEVICE_RW) |
> diff --git a/src/lxc/lxc_cgroup.h b/src/lxc/lxc_cgroup.h
> index 63e9e837b0..3d643a4fea 100644
> --- a/src/lxc/lxc_cgroup.h
> +++ b/src/lxc/lxc_cgroup.h
> @@ -46,3 +46,7 @@ int
> virLXCTeardownHostUSBDeviceCgroup(virUSBDevicePtr dev,
>const char *path,
>void *opaque);
> +
> +int
> +virLXCCgroupResetBlkioDeviceThrottle(virCgroupPtr cgroup,
> + const char *path);
> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> index 204a3ed522..87da55f308 100644
> --- a/src/lxc/lxc_driver.c
> +++ b/src/lxc/l
Re: [libvirt] [PATCH v3 0/9] Enable ramfb parameter for mediated devices
On 11/14/19 11:40 AM, Cole Robinson wrote: > On 10/18/19 11:30 AM, Jonathon Jongsma wrote: >> This is the third version of a patch series that adds support for a boot >> display for mediated vgpu devices using the 'ramfb' parameter. This version >> fixes a bunch of test failures that I had inadvertently introduced in the >> last >> series. It also splits the 4th patch up into several separate patches as >> suggested by Cole to make things more readable. >> >> Jonathon Jongsma (9): >> qemu: fix domain device validation >> qemu: use g_autoptr in qemuDomainDeviceDefValidate() >> qemu: Set capabilities properly for tests >> qemu: set domain capability for ramfb device >> qemu: set domain capability for video type "none" >> qemu: validate vhost-user video backend in qemu_domain.c >> qemu: move validation of video accel to qemu_domain.c >> qemu: use domain caps to validate video device model >> qemu: add 'ramfb' attribute for mediated devices >> > > Pushed with some fixups: > > * rebased and resolved conflicts > * fixed function spacing in the last two patches, to preserve existing 2 > line spacing (this was a bit ambiguous in the second to last patch) > * adjusted docs version in the last patch to mention 5.10.0 Also, ramfb addition warrants a news.xml update too Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] Add API to change qemu agent response timeout
On 11/13/19 11:06 PM, Jonathon Jongsma wrote:
> Some layered products such as oVirt have requested a way to avoid being
> blocked by guest agent commands when querying a loaded vm. For example,
> many guest agent commands are polled periodically to monitor changes,
> and rather than blocking the calling process, they'd prefer to simply
> time out when an agent query is taking too long.
>
> This patch adds a way for the user to specify a custom agent timeout
> that is applied to all agent commands.
>
> One special case to note here is the 'guest-sync' command. 'guest-sync'
> is issued internally prior to calling any other command. (For example,
> when libvirt wants to call 'guest-get-fsinfo', we first call
> 'guest-sync' and then call 'guest-get-fsinfo').
>
> Previously, the 'guest-sync' command used a 5-second timeout
> (VIR_DOMAIN_QEMU_AGENT_COMMAND_DEFAULT), whereas the actual command that
> followed always blocked indefinitely
> (VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK). As part of this patch, if a
> custom timeout is specified that is shorter than
> 5 seconds, this new timeout is also used for 'guest-sync'. If there is
> no custom timeout or if the custom timeout is longer than 5 seconds, we
> will continue to use the 5-second timeout.
>
> Signed-off-by: Jonathon Jongsma
> ---
> Changes in v3:
> - Changed enum names per Daniel's suggestion
> - incorporated Michal's review and fixup patches
> - As discussed on the mailing list, this patch does not acquire any jobs, but
>simply uses mutex locking for data integrity
> - libvirt release versions updated
> - format and parse private agentTimeout data in status xml
>
> include/libvirt/libvirt-domain.h | 10 +++
> include/libvirt/libvirt-qemu.h| 8 +--
> src/driver-hypervisor.h | 6 ++
> src/libvirt-domain.c | 49 +
> src/libvirt_public.syms | 5 ++
> src/qemu/qemu_agent.c | 70 ++-
> src/qemu/qemu_agent.h | 3 +
> src/qemu/qemu_domain.c| 6 ++
> src/qemu/qemu_domain.h| 1 +
> src/qemu/qemu_driver.c| 47 +
> src/remote/remote_driver.c| 1 +
> src/remote/remote_protocol.x | 18 -
> src/remote_protocol-structs | 9 +++
> .../blockjob-blockdev-in.xml | 1 +
> .../blockjob-mirror-in.xml| 1 +
> .../disk-secinfo-upgrade-out.xml | 1 +
> .../migration-in-params-in.xml| 1 +
> .../migration-out-nbd-out.xml | 1 +
> .../migration-out-nbd-tls-out.xml | 1 +
> .../migration-out-params-in.xml | 1 +
> tests/qemustatusxml2xmldata/modern-in.xml | 1 +
> .../qemustatusxml2xmldata/vcpus-multi-in.xml | 1 +
> tools/virsh-domain.c | 52 ++
> 23 files changed, 257 insertions(+), 37 deletions(-)
Close enough :-)
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 54dde34f15..86358341d5 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2093,6 +2093,8 @@ qemuDomainObjPrivateAlloc(void *opaque)
> if (!(priv->dbusVMStates = virHashCreate(5, dbusVMStateHashFree)))
> goto error;
>
> +/* agent commands block by default, user can choose different behavior */
> +priv->agentTimeout = VIR_DOMAIN_AGENT_RESPONSE_TIMEOUT_BLOCK;
> priv->migMaxBandwidth = QEMU_DOMAIN_MIG_BANDWIDTH_MAX;
> priv->driver = opaque;
>
> @@ -2875,6 +2877,8 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf,
> if (qemuDomainObjPrivateXMLFormatSlirp(buf, vm) < 0)
> return -1;
>
> +virBufferAsprintf(buf, "%i\n",
> priv->agentTimeout);
> +
> return 0;
> }
>
> @@ -3672,6 +3676,8 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt,
>
> priv->memPrealloc = virXPathBoolean("boolean(./memPrealloc)", ctxt) == 1;
>
> +virXPathInt("string(./agentTimeout)", ctxt, &priv->agentTimeout);
This is not safe. If the value in status XML is not valid number,
ignoring the error is not good.
> +
> return 0;
>
> error:
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index ab00b25789..98a9540275 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -293,6 +293,7 @@ struct _qemuDomainObjPrivate {
> virDomainChrSourceDefPtr monConfig;
> bool monError;
> unsigned long long monStart;
> +int agentTimeout;
>
> qemuAgentPtr agent;
> bool agentError;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index c969a3d463..77a26ccf2e 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -22666,6 +22666,52 @@ qemuDomainGetGuestInfo(virDomainPtr dom,
> return ret;
> }
>
> +
> +static int
> +qemuDomainAgentSetResponseTimeout(vi
Re: [libvirt] [PATCH 1/3] Use g_mkstemp_full instead of mkostemp(s)
On Thu, Nov 14, 2019 at 05:37:26PM +0100, Peter Krempa wrote:
On Thu, Nov 14, 2019 at 14:48:04 +0100, Ján Tomko wrote:
With g_mkstemp_full, there is no need to distinguish between
mkostemp and mkostemps (no suffix vs. a suffix of a fixed length),
because the GLib function looks for the XX pattern everywhere
in the string.
Use S_IRUSR | S_IWUSR for the permissions and do not pass O_RDWR
in flags since it's implied.
Signed-off-by: Ján Tomko
---
src/qemu/qemu_driver.c | 8
src/storage/storage_driver.c | 2 +-
src/storage/storage_util.c | 2 +-
src/util/virlog.c| 8 +---
src/vbox/vbox_common.c | 4 ++--
tests/virfiletest.c | 2 +-
tools/vsh.c | 4 ++--
7 files changed, 12 insertions(+), 18 deletions(-)
[...]
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 04e4abcd6a..d8355d3c3c 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -2825,7 +2825,7 @@ virStoragePoolObjFindPoolByUUID(const unsigned char *uuid)
*
* Generate a name for a temporary file using the driver stateDir
* as a path, the pool name, and the volume name to be used as input
- * for a mkostemp
+ * for mkstemp
Shouldn't we mention g_mkstemp_full?
Well, the XX template is not really g_mkstemp-specific
*
* Returns a string pointer on success, NULL on failure
*/
[...]
diff --git a/tools/vsh.c b/tools/vsh.c
index 000cf6a009..e851303e69 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -2400,9 +2400,9 @@ vshEditWriteToTempFile(vshControl *ctl, const char *doc)
tmpdir = getenv("TMPDIR");
if (!tmpdir) tmpdir = "/tmp";
ret = g_strdup_printf("%s/virshXX.xml", tmpdir);
-fd = mkostemps(ret, 4, O_CLOEXEC);
+fd = g_mkstemp_full(ret, O_CLOEXEC, S_IRUSR | S_IWUSR);
if (fd == -1) {
-vshError(ctl, _("mkostemps: failed to create temporary file: %s"),
+vshError(ctl, _("g_mkstemp: failed to create temporary file: %s"),
_full
Fixed.
Jano
virStrerror(errno, ebuf, sizeof(ebuf)));
VIR_FREE(ret);
return NULL;
Reviewed-by: Peter Krempa
--
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
[libvirt] [PATCH v4 2/7] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback
The commit 'close callback: move it to driver' (88f09b75eb99) moved
the responsibility for the close callback to the driver. But if the
driver doesn't support the connectRegisterCloseCallback API this
function does nothing, even no unsupported error report. This behavior
may lead to problems, for example memory leaks, as the caller cannot
differentiate whether the close callback was 'really' registered or
not.
Therefore call directly @freecb if the connectRegisterCloseCallback
API is not supported by the driver used by the connection and update
the documentation.
Signed-off-by: Marc Hartmayer
---
src/libvirt-host.c | 16 +++-
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/src/libvirt-host.c b/src/libvirt-host.c
index 221a1b7a4353..94383ed449a9 100644
--- a/src/libvirt-host.c
+++ b/src/libvirt-host.c
@@ -1398,7 +1398,7 @@ virConnectIsAlive(virConnectPtr conn)
* @conn: pointer to connection object
* @cb: callback to invoke upon close
* @opaque: user data to pass to @cb
- * @freecb: callback to free @opaque
+ * @freecb: callback to free @opaque when not used anymore
*
* Registers a callback to be invoked when the connection
* is closed. This callback is invoked when there is any
@@ -1412,7 +1412,9 @@ virConnectIsAlive(virConnectPtr conn)
*
* The @freecb must not invoke any other libvirt public
* APIs, since it is not called from a re-entrant safe
- * context.
+ * context. Only if virConnectRegisterCloseCallback is
+ * successful, @freecb will be called, otherwise the
+ * caller is responsible to free @opaque.
*
* Returns 0 on success, -1 on error
*/
@@ -1428,9 +1430,13 @@ virConnectRegisterCloseCallback(virConnectPtr conn,
virCheckConnectReturn(conn, -1);
virCheckNonNullArgGoto(cb, error);
-if (conn->driver->connectRegisterCloseCallback &&
-conn->driver->connectRegisterCloseCallback(conn, cb, opaque, freecb) <
0)
-goto error;
+if (conn->driver->connectRegisterCloseCallback) {
+if (conn->driver->connectRegisterCloseCallback(conn, cb, opaque,
freecb) < 0)
+goto error;
+} else {
+if (freecb)
+freecb(opaque);
+}
return 0;
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 3/7] remote: Save reference to program in daemonClientEventCallback
As a result, you can later determine during the callback which program
was used. This makes it easier to refactor the code in the future and
is less prone to error.
Signed-off-by: Marc Hartmayer
Reviewed-by: Pavel Hrdina
---
src/remote/remote_daemon_dispatch.c | 108 +++-
1 file changed, 59 insertions(+), 49 deletions(-)
diff --git a/src/remote/remote_daemon_dispatch.c
b/src/remote/remote_daemon_dispatch.c
index f369ffb02a65..7b53c2241c05 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -78,6 +78,7 @@ VIR_LOG_INIT("daemon.remote");
struct daemonClientEventCallback {
virNetServerClientPtr client;
+virNetServerProgramPtr program;
int eventID;
int callbackID;
bool legacy;
@@ -149,6 +150,7 @@ remoteEventCallbackFree(void *opaque)
daemonClientEventCallbackPtr callback = opaque;
if (!callback)
return;
+virObjectUnref(callback->program);
virObjectUnref(callback->client);
VIR_FREE(callback);
}
@@ -334,7 +336,7 @@ remoteRelayDomainEventLifecycle(virConnectPtr conn,
data.detail = detail;
if (callback->legacy) {
-remoteDispatchObjectEventSend(callback->client, remoteProgram,
+remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_LIFECYCLE,
(xdrproc_t)xdr_remote_domain_event_lifecycle_msg,
&data);
@@ -342,7 +344,7 @@ remoteRelayDomainEventLifecycle(virConnectPtr conn,
remote_domain_event_callback_lifecycle_msg msg = {
callback->callbackID,
data };
-remoteDispatchObjectEventSend(callback->client, remoteProgram,
+remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_LIFECYCLE,
(xdrproc_t)xdr_remote_domain_event_callback_lifecycle_msg,
&msg);
@@ -371,14 +373,14 @@ remoteRelayDomainEventReboot(virConnectPtr conn,
make_nonnull_domain(&data.dom, dom);
if (callback->legacy) {
-remoteDispatchObjectEventSend(callback->client, remoteProgram,
+remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_REBOOT,
(xdrproc_t)xdr_remote_domain_event_reboot_msg, &data);
} else {
remote_domain_event_callback_reboot_msg msg = { callback->callbackID,
data };
-remoteDispatchObjectEventSend(callback->client, remoteProgram,
+remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_REBOOT,
(xdrproc_t)xdr_remote_domain_event_callback_reboot_msg, &msg);
}
@@ -410,14 +412,14 @@ remoteRelayDomainEventRTCChange(virConnectPtr conn,
data.offset = offset;
if (callback->legacy) {
-remoteDispatchObjectEventSend(callback->client, remoteProgram,
+remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_RTC_CHANGE,
(xdrproc_t)xdr_remote_domain_event_rtc_change_msg, &data);
} else {
remote_domain_event_callback_rtc_change_msg msg = {
callback->callbackID,
data };
-remoteDispatchObjectEventSend(callback->client, remoteProgram,
+remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_RTC_CHANGE,
(xdrproc_t)xdr_remote_domain_event_callback_rtc_change_msg, &msg);
}
@@ -448,14 +450,14 @@ remoteRelayDomainEventWatchdog(virConnectPtr conn,
data.action = action;
if (callback->legacy) {
-remoteDispatchObjectEventSend(callback->client, remoteProgram,
+remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_WATCHDOG,
(xdrproc_t)xdr_remote_domain_event_watchdog_msg, &data);
} else {
remote_domain_event_callback_watchdog_msg msg = { callback->callbackID,
data };
-remoteDispatchObjectEventSend(callback->client, remoteProgram,
+remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_WATCHDOG,
(xdrproc_t)xdr_remote_domain_event_callback_watchdog_ms
[libvirt] [PATCH v4 5/7] rpc: Introduce virNetServerGetProgramLocked helper function
This patch introduces virNetServerGetProgramLocked. It's a function to
determine which program has to be used for a given @msg. This function
will be reused in the next patch.
Signed-off-by: Marc Hartmayer
Reviewed-by: Pavel Hrdina
---
src/rpc/virnetserver.c | 28 +---
1 file changed, 21 insertions(+), 7 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 673bb7c10c86..41226368058f 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -166,6 +166,26 @@ static void virNetServerHandleJob(void *jobOpaque, void
*opaque)
VIR_FREE(job);
}
+/**
+ * virNetServerGetProgramLocked:
+ * @srv: server (must be locked by the caller)
+ * @msg: message
+ *
+ * Searches @srv for the right program for a given message @msg.
+ *
+ * Returns a pointer to the server program or NULL if not found.
+ */
+static virNetServerProgramPtr
+virNetServerGetProgramLocked(virNetServerPtr srv,
+ virNetMessagePtr msg)
+{
+size_t i;
+for (i = 0; i < srv->nprograms; i++) {
+if (virNetServerProgramMatches(srv->programs[i], msg))
+return srv->programs[i];
+}
+return NULL;
+}
static void
virNetServerDispatchNewMessage(virNetServerClientPtr client,
@@ -175,18 +195,12 @@ virNetServerDispatchNewMessage(virNetServerClientPtr
client,
virNetServerPtr srv = opaque;
virNetServerProgramPtr prog = NULL;
unsigned int priority = 0;
-size_t i;
VIR_DEBUG("server=%p client=%p message=%p",
srv, client, msg);
virObjectLock(srv);
-for (i = 0; i < srv->nprograms; i++) {
-if (virNetServerProgramMatches(srv->programs[i], msg)) {
-prog = srv->programs[i];
-break;
-}
-}
+prog = virNetServerGetProgramLocked(srv, msg);
/* we can unlock @srv since @prog can only become invalid in case
* of disposing @srv, but let's grab a ref first to ensure nothing
* disposes of it before we use it. */
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 4/7] remote: Use domainClientEventCallbacks for remoteReplayConnectionClosedEvent
This allows us later to get rid of another usage of the global
variable `remoteProgram`.
Signed-off-by: Marc Hartmayer
Reviewed-by: Pavel Hrdina
---
src/remote/remote_daemon_dispatch.c | 19 +++
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/src/remote/remote_daemon_dispatch.c
b/src/remote/remote_daemon_dispatch.c
index 7b53c2241c05..9dc2083d715a 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -1620,12 +1620,12 @@ remoteRelayDomainQemuMonitorEvent(virConnectPtr conn,
static
void remoteRelayConnectionClosedEvent(virConnectPtr conn G_GNUC_UNUSED, int
reason, void *opaque)
{
-virNetServerClientPtr client = opaque;
+daemonClientEventCallbackPtr callback = opaque;
VIR_DEBUG("Relaying connection closed event, reason %d", reason);
remote_connect_event_connection_closed_msg msg = { reason };
-remoteDispatchObjectEventSend(client, remoteProgram,
+remoteDispatchObjectEventSend(callback->client, callback->program,
REMOTE_PROC_CONNECT_EVENT_CONNECTION_CLOSED,
(xdrproc_t)xdr_remote_connect_event_connection_closed_msg,
&msg);
@@ -4170,6 +4170,7 @@
remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server G_GNUC_UNUSED,
virNetMessageErrorPtr rerr)
{
int rv = -1;
+daemonClientEventCallbackPtr callback = NULL;
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
virConnectPtr conn = remoteGetHypervisorConn(client);
@@ -4179,9 +4180,17 @@
remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server G_GNUC_UNUSED,
if (!conn)
goto cleanup;
+if (VIR_ALLOC(callback) < 0)
+goto cleanup;
+
+callback->client = virObjectRef(client);
+callback->program = virObjectRef(remoteProgram);
+/* eventID, callbackID, and legacy are not used */
+callback->eventID = -1;
+callback->callbackID = -1;
if (virConnectRegisterCloseCallback(conn,
remoteRelayConnectionClosedEvent,
-client, NULL) < 0)
+callback, remoteEventCallbackFree) < 0)
goto cleanup;
priv->closeRegistered = true;
@@ -4189,8 +4198,10 @@
remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server G_GNUC_UNUSED,
cleanup:
virMutexUnlock(&priv->lock);
-if (rv < 0)
+if (rv < 0) {
+remoteEventCallbackFree(callback);
virNetMessageSaveError(rerr);
+}
return rv;
}
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 0/7] Fix virConnectRegisterCloseCallback and get rid of global variables
The second patch of this patch series fixes the behavior of virConnectRegisterCloseCallback. The subsequent patches remove the need to have the global variables 'qemuProgram' and 'remoteProgram' in libvirtd.[ch]. They only work in combination with the fixed behavior of virConnectRegisterCloseCallback. Changelog: + v3->v4: - Rebased to current master - Added Pavel's r-bs - Worked in Pavel's comments - Added patch "remote: shrink the critical sections" in preparation for patch "remote/rpc: Use virNetServerGetProgram() to determine the program" + v2->v3: - Rebased to current master - Added Johns r-b to the first patch, all other r-b's I have dropped as there were to many changes in the meantime - Removed accepted patches - Dropped patches 8 and 9 + v1->v2: - Removed accepted patches - Removed NACKed patches - Added r-b to patch 5 - Worked in comments - Rebased - Added patches 7-9 Marc Hartmayer (7): rpc: use the return value of virObjectRef directly virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback remote: Save reference to program in daemonClientEventCallback remote: Use domainClientEventCallbacks for remoteReplayConnectionClosedEvent rpc: Introduce virNetServerGetProgramLocked helper function remote: shrink the critical sections remote/rpc: Use virNetServerGetProgram() to determine the program src/libvirt-host.c | 16 +- src/libvirt_remote.syms | 1 + src/remote/remote_daemon.c | 4 +- src/remote/remote_daemon.h | 2 - src/remote/remote_daemon_dispatch.c | 363 +--- src/rpc/gendispatch.pl | 6 + src/rpc/virnetserver.c | 53 +++- src/rpc/virnetserver.h | 2 + 8 files changed, 293 insertions(+), 154 deletions(-) -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 6/7] remote: shrink the critical sections
To free the structs and save the error, it is not necessary to hold @priv->lock, therefore move these parts after the mutex unlock. Signed-off-by: Marc Hartmayer --- src/remote/remote_daemon_dispatch.c | 32 ++--- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 9dc2083d715a..6ece51c2889d 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -4293,10 +4293,10 @@ remoteDispatchConnectDomainEventRegister(virNetServerPtr server G_GNUC_UNUSED, rv = 0; cleanup: +virMutexUnlock(&priv->lock); remoteEventCallbackFree(callback); if (rv < 0) virNetMessageSaveError(rerr); -virMutexUnlock(&priv->lock); return rv; } @@ -4342,9 +4342,9 @@ remoteDispatchConnectDomainEventDeregister(virNetServerPtr server G_GNUC_UNUSED, rv = 0; cleanup: +virMutexUnlock(&priv->lock); if (rv < 0) virNetMessageSaveError(rerr); -virMutexUnlock(&priv->lock); return rv; } @@ -4522,10 +4522,10 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server G_GNUC_UNUSED rv = 0; cleanup: +virMutexUnlock(&priv->lock); remoteEventCallbackFree(callback); if (rv < 0) virNetMessageSaveError(rerr); -virMutexUnlock(&priv->lock); return rv; } @@ -4598,11 +4598,11 @@ remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr server G_GNU rv = 0; cleanup: +virMutexUnlock(&priv->lock); remoteEventCallbackFree(callback); if (rv < 0) virNetMessageSaveError(rerr); virObjectUnref(dom); -virMutexUnlock(&priv->lock); return rv; } @@ -4657,9 +4657,9 @@ remoteDispatchConnectDomainEventDeregisterAny(virNetServerPtr server G_GNUC_UNUS rv = 0; cleanup: +virMutexUnlock(&priv->lock); if (rv < 0) virNetMessageSaveError(rerr); -virMutexUnlock(&priv->lock); return rv; } @@ -4702,9 +4702,9 @@ remoteDispatchConnectDomainEventCallbackDeregisterAny(virNetServerPtr server G_G rv = 0; cleanup: +virMutexUnlock(&priv->lock); if (rv < 0) virNetMessageSaveError(rerr); -virMutexUnlock(&priv->lock); return rv; } @@ -6081,11 +6081,11 @@ remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server G_GNUC_UNUSE rv = 0; cleanup: +virMutexUnlock(&priv->lock); remoteEventCallbackFree(callback); if (rv < 0) virNetMessageSaveError(rerr); virObjectUnref(net); -virMutexUnlock(&priv->lock); return rv; } @@ -6128,9 +6128,9 @@ remoteDispatchConnectNetworkEventDeregisterAny(virNetServerPtr server G_GNUC_UNU rv = 0; cleanup: +virMutexUnlock(&priv->lock); if (rv < 0) virNetMessageSaveError(rerr); -virMutexUnlock(&priv->lock); return rv; } @@ -6202,11 +6202,11 @@ remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr server G_GNUC_U rv = 0; cleanup: +virMutexUnlock(&priv->lock); remoteEventCallbackFree(callback); if (rv < 0) virNetMessageSaveError(rerr); virObjectUnref(pool); -virMutexUnlock(&priv->lock); return rv; } @@ -6248,9 +6248,9 @@ remoteDispatchConnectStoragePoolEventDeregisterAny(virNetServerPtr server G_GNUC rv = 0; cleanup: +virMutexUnlock(&priv->lock); if (rv < 0) virNetMessageSaveError(rerr); -virMutexUnlock(&priv->lock); return rv; } @@ -6322,11 +6322,11 @@ remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr server G_GNUC_UN rv = 0; cleanup: +virMutexUnlock(&priv->lock); remoteEventCallbackFree(callback); if (rv < 0) virNetMessageSaveError(rerr); virObjectUnref(dev); -virMutexUnlock(&priv->lock); return rv; } @@ -6368,9 +6368,9 @@ remoteDispatchConnectNodeDeviceEventDeregisterAny(virNetServerPtr server G_GNUC_ rv = 0; cleanup: +virMutexUnlock(&priv->lock); if (rv < 0) virNetMessageSaveError(rerr); -virMutexUnlock(&priv->lock); return rv; } @@ -6442,11 +6442,11 @@ remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server G_GNUC_UNUSED rv = 0; cleanup: +virMutexUnlock(&priv->lock); remoteEventCallbackFree(callback); if (rv < 0) virNetMessageSaveError(rerr); virObjectUnref(secret); -virMutexUnlock(&priv->lock); return rv; } @@ -6488,9 +6488,9 @@ remoteDispatchConnectSecretEventDeregisterAny(virNetServerPtr server G_GNUC_UNUS rv = 0; cleanup: +virMutexUnlock(&priv->lock); if (rv < 0) virNetMessageSaveError(rerr); -virMutexUnlock(&priv->lock); return rv; } @@ -6558,11 +6558,11 @@ qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server G_GNUC_UNUS rv = 0; cleanup: +virMutexUnlock(&priv->lock); remoteEventCallbackFree(callback); if (rv <
[libvirt] [PATCH v4 1/7] rpc: use the return value of virObjectRef directly
Use the return value of virObjectRef directly. This way, it's easier
for another reader to identify the reason why the additional reference
is required.
Signed-off-by: Marc Hartmayer
Reviewed-by: John Ferlan
Reviewed-by: Pavel Hrdina
---
src/rpc/virnetserver.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 590e780b641e..673bb7c10c86 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -199,7 +199,7 @@ virNetServerDispatchNewMessage(virNetServerClientPtr client,
if (VIR_ALLOC(job) < 0)
goto error;
-job->client = client;
+job->client = virObjectRef(client);
job->msg = msg;
if (prog) {
@@ -207,7 +207,6 @@ virNetServerDispatchNewMessage(virNetServerClientPtr client,
priority = virNetServerProgramGetPriority(prog, msg->header.proc);
}
-virObjectRef(client);
if (virThreadPoolSendJob(srv->workers, priority, job) < 0) {
virObjectUnref(client);
VIR_FREE(job);
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v4 7/7] remote/rpc: Use virNetServerGetProgram() to determine the program
Use virNetServerGetProgram() to determine the virNetServerProgram
instead of using hard coded global variables. This allows us to remove
the global variables @remoteProgram and @qemuProgram as they're now no
longer necessary.
Signed-off-by: Marc Hartmayer
---
src/libvirt_remote.syms | 1 +
src/remote/remote_daemon.c | 4 +-
src/remote/remote_daemon.h | 2 -
src/remote/remote_daemon_dispatch.c | 238 ++--
src/rpc/gendispatch.pl | 6 +
src/rpc/virnetserver.c | 22 +++
src/rpc/virnetserver.h | 2 +
7 files changed, 187 insertions(+), 88 deletions(-)
diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index 0493467f4603..a6883f373608 100644
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -124,6 +124,7 @@ virNetServerGetCurrentUnauthClients;
virNetServerGetMaxClients;
virNetServerGetMaxUnauthClients;
virNetServerGetName;
+virNetServerGetProgram;
virNetServerGetThreadPoolParameters;
virNetServerHasClients;
virNetServerNeedsAuth;
diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
index b400b1dd1059..a36b5449d5ae 100644
--- a/src/remote/remote_daemon.c
+++ b/src/remote/remote_daemon.c
@@ -73,8 +73,6 @@ VIR_LOG_INIT("daemon." DAEMON_NAME);
#if WITH_SASL
virNetSASLContextPtr saslCtxt = NULL;
#endif
-virNetServerProgramPtr remoteProgram = NULL;
-virNetServerProgramPtr qemuProgram = NULL;
volatile bool driversInitialized = false;
@@ -996,6 +994,8 @@ int main(int argc, char **argv) {
virNetServerPtr srv = NULL;
virNetServerPtr srvAdm = NULL;
virNetServerProgramPtr adminProgram = NULL;
+virNetServerProgramPtr qemuProgram = NULL;
+virNetServerProgramPtr remoteProgram = NULL;
virNetServerProgramPtr lxcProgram = NULL;
char *remote_config_file = NULL;
int statuswrite = -1;
diff --git a/src/remote/remote_daemon.h b/src/remote/remote_daemon.h
index a2d9af403619..a3d6a220f868 100644
--- a/src/remote/remote_daemon.h
+++ b/src/remote/remote_daemon.h
@@ -97,5 +97,3 @@ struct daemonClientPrivate {
#if WITH_SASL
extern virNetSASLContextPtr saslCtxt;
#endif
-extern virNetServerProgramPtr remoteProgram;
-extern virNetServerProgramPtr qemuProgram;
diff --git a/src/remote/remote_daemon_dispatch.c
b/src/remote/remote_daemon_dispatch.c
index 6ece51c2889d..7ebb97f49f3b 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -4164,9 +4164,9 @@ remoteDispatchNodeDeviceGetParent(virNetServerPtr server
G_GNUC_UNUSED,
}
static int
-remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server
G_GNUC_UNUSED,
+remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg G_GNUC_UNUSED,
+ virNetMessagePtr msg,
virNetMessageErrorPtr rerr)
{
int rv = -1;
@@ -4174,30 +4174,37 @@
remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server G_GNUC_UNUSED,
struct daemonClientPrivate *priv =
virNetServerClientGetPrivateData(client);
virConnectPtr conn = remoteGetHypervisorConn(client);
+virNetServerProgramPtr program;
+
+if (!(program = virNetServerGetProgram(server, msg))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching program
found"));
+goto cleanup;
+}
virMutexLock(&priv->lock);
if (!conn)
-goto cleanup;
+goto cleanup_unlock;
if (VIR_ALLOC(callback) < 0)
-goto cleanup;
+goto cleanup_unlock;
callback->client = virObjectRef(client);
-callback->program = virObjectRef(remoteProgram);
+callback->program = virObjectRef(program);
/* eventID, callbackID, and legacy are not used */
callback->eventID = -1;
callback->callbackID = -1;
if (virConnectRegisterCloseCallback(conn,
remoteRelayConnectionClosedEvent,
callback, remoteEventCallbackFree) < 0)
-goto cleanup;
+goto cleanup_unlock;
priv->closeRegistered = true;
rv = 0;
- cleanup:
+ cleanup_unlock:
virMutexUnlock(&priv->lock);
+ cleanup:
if (rv < 0) {
remoteEventCallbackFree(callback);
virNetMessageSaveError(rerr);
@@ -4236,9 +4243,9 @@
remoteDispatchConnectUnregisterCloseCallback(virNetServerPtr server G_GNUC_UNUSE
}
static int
-remoteDispatchConnectDomainEventRegister(virNetServerPtr server G_GNUC_UNUSED,
+remoteDispatchConnectDomainEventRegister(virNetServerPtr server,
virNetServerClientPtr client,
- virNetMessagePtr msg G_GNUC_UNUSED,
+ virNetMessagePtr msg,
virNetM
[libvirt] [PATCH] spec: Remove build-time list of edk2 firmwares
Fedora now advertises supported firmwares via descriptor files.
Since the upstream spec file assumes recent Fedora, remove the
build-time list of firmwares, which can produce a warning after
commit 75597f022a.
Signed-off-by: Jim Fehlig
---
libvirt.spec.in | 29 -
1 file changed, 29 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index 2374668722..a6219da604 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1131,34 +1131,6 @@ exit 1
%define arg_selinux_mount --with-selinux-mount="/sys/fs/selinux"
-%if 0%{?fedora}
-# Nightly edk2.git-ovmf-x64
-
LOADERS="/usr/share/edk2.git/ovmf-x64/OVMF_CODE-pure-efi.fd:/usr/share/edk2.git/ovmf-x64/OVMF_VARS-pure-efi.fd"
-# Nightly edk2.git-ovmf-ia32
-
LOADERS="$LOADERS:/usr/share/edk2.git/ovmf-ia32/OVMF_CODE-pure-efi.fd:/usr/share/edk2.git/ovmf-ia32/OVMF_VARS-pure-efi.fd"
-# Nightly edk2.git-aarch64
-
LOADERS="$LOADERS:/usr/share/edk2.git/aarch64/QEMU_EFI-pflash.raw:/usr/share/edk2.git/aarch64/vars-template-pflash.raw"
-# Nightly edk2.git-arm
-
LOADERS="$LOADERS:/usr/share/edk2.git/arm/QEMU_EFI-pflash.raw:/usr/share/edk2.git/arm/vars-template-pflash.raw"
-
-# Fedora edk2-ovmf, x86_64
-
LOADERS="$LOADERS:/usr/share/edk2/ovmf/OVMF_CODE.fd:/usr/share/edk2/ovmf/OVMF_VARS.fd"
-# Fedora edk2-ovmf, x86_64, with Secure Boot
-
LOADERS="$LOADERS:/usr/share/edk2/ovmf/OVMF_CODE.secboot.fd:/usr/share/edk2/ovmf/OVMF_VARS.secboot.fd"
-# Fedora edk2-ovmf-ia32
-
LOADERS="$LOADERS:/usr/share/edk2/ovmf-ia32/OVMF_CODE.fd:/usr/share/edk2/ovmf-ia32/OVMF_VARS.fd"
-# Fedora edk2-ovmf-ia32, with Secure Boot. (NB: Unlike x86_64, for
-# 'ia32', there is no secboot-variant "VARS" file (NVRAM template).
-# So the NVRAM template for 'ovmf-ia32/OVMF_CODE.secboot.fd' is the
-# same as the one for the non-secboot variant.)
-
LOADERS="$LOADERS:/usr/share/edk2/ovmf-ia32/OVMF_CODE.secboot.fd:/usr/share/edk2/ovmf-ia32/OVMF_VARS.fd"
-# Fedora edk2-aarch64
-
LOADERS="$LOADERS:/usr/share/edk2/aarch64/QEMU_EFI-pflash.raw:/usr/share/edk2/aarch64/vars-template-pflash.raw"
-# Fedora edk2-arm
-
LOADERS="$LOADERS:/usr/share/edk2/arm/QEMU_EFI-pflash.raw:/usr/share/edk2/arm/vars-template-pflash.raw"
-%define arg_loader_nvram --with-loader-nvram="$LOADERS"
-%endif
-
# place macros above and build commands below this comment
export SOURCE_DATE_EPOCH=$(stat --printf='%Y' %{_specdir}/%{name}.spec)
@@ -1231,7 +1203,6 @@ cd %{_vpath_builddir}
--with-qemu-user=%{qemu_user} \
--with-qemu-group=%{qemu_group} \
--with-tls-priority=%{tls_priority} \
- %{?arg_loader_nvram} \
%{?enable_werror} \
--enable-expensive-tests \
--with-init-script=systemd \
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] syntax-check: prefer g_mkstemp_full and g_mkdtemp
On Thu, Nov 14, 2019 at 14:48:06 +0100, Ján Tomko wrote: > Signed-off-by: Ján Tomko > --- > build-aux/syntax-check.mk | 13 ++--- > 1 file changed, 10 insertions(+), 3 deletions(-) Reviewed-by: Peter Krempa -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] Use g_mkdtemp instead of mkdtemp
On Thu, Nov 14, 2019 at 14:48:05 +0100, Ján Tomko wrote: > Prefer the GLib version to the one from gnulib. > > Signed-off-by: Ján Tomko > --- > src/qemu/qemu_process.c | 2 +- > tests/fdstreamtest.c | 2 +- > tests/qemuhotplugtest.c | 2 +- > tests/qemumemlocktest.c | 2 +- > tests/qemumonitortestutils.c | 2 +- > tests/qemuxml2argvtest.c | 2 +- > tests/qemuxml2xmltest.c | 2 +- > tests/scsihosttest.c | 2 +- > tests/testutilsqemu.c| 4 ++-- > tests/vircgrouptest.c| 2 +- > tests/virhostdevtest.c | 2 +- > tests/virnetsockettest.c | 4 ++-- > tests/virpcitest.c | 2 +- > tests/virscsitest.c | 2 +- > 14 files changed, 16 insertions(+), 16 deletions(-) Reviewed-by: Peter Krempa -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 0/9] Enable ramfb parameter for mediated devices
On 10/18/19 11:30 AM, Jonathon Jongsma wrote: > This is the third version of a patch series that adds support for a boot > display for mediated vgpu devices using the 'ramfb' parameter. This version > fixes a bunch of test failures that I had inadvertently introduced in the last > series. It also splits the 4th patch up into several separate patches as > suggested by Cole to make things more readable. > > Jonathon Jongsma (9): > qemu: fix domain device validation > qemu: use g_autoptr in qemuDomainDeviceDefValidate() > qemu: Set capabilities properly for tests > qemu: set domain capability for ramfb device > qemu: set domain capability for video type "none" > qemu: validate vhost-user video backend in qemu_domain.c > qemu: move validation of video accel to qemu_domain.c > qemu: use domain caps to validate video device model > qemu: add 'ramfb' attribute for mediated devices > Pushed with some fixups: * rebased and resolved conflicts * fixed function spacing in the last two patches, to preserve existing 2 line spacing (this was a bit ambiguous in the second to last patch) * adjusted docs version in the last patch to mention 5.10.0 Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/3] Use g_mkstemp_full instead of mkostemp(s)
On Thu, Nov 14, 2019 at 14:48:04 +0100, Ján Tomko wrote:
> With g_mkstemp_full, there is no need to distinguish between
> mkostemp and mkostemps (no suffix vs. a suffix of a fixed length),
> because the GLib function looks for the XX pattern everywhere
> in the string.
>
> Use S_IRUSR | S_IWUSR for the permissions and do not pass O_RDWR
> in flags since it's implied.
>
> Signed-off-by: Ján Tomko
> ---
> src/qemu/qemu_driver.c | 8
> src/storage/storage_driver.c | 2 +-
> src/storage/storage_util.c | 2 +-
> src/util/virlog.c| 8 +---
> src/vbox/vbox_common.c | 4 ++--
> tests/virfiletest.c | 2 +-
> tools/vsh.c | 4 ++--
> 7 files changed, 12 insertions(+), 18 deletions(-)
[...]
> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index 04e4abcd6a..d8355d3c3c 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -2825,7 +2825,7 @@ virStoragePoolObjFindPoolByUUID(const unsigned char
> *uuid)
> *
> * Generate a name for a temporary file using the driver stateDir
> * as a path, the pool name, and the volume name to be used as input
> - * for a mkostemp
> + * for mkstemp
Shouldn't we mention g_mkstemp_full?
> *
> * Returns a string pointer on success, NULL on failure
> */
[...]
> diff --git a/tools/vsh.c b/tools/vsh.c
> index 000cf6a009..e851303e69 100644
> --- a/tools/vsh.c
> +++ b/tools/vsh.c
> @@ -2400,9 +2400,9 @@ vshEditWriteToTempFile(vshControl *ctl, const char *doc)
> tmpdir = getenv("TMPDIR");
> if (!tmpdir) tmpdir = "/tmp";
> ret = g_strdup_printf("%s/virshXX.xml", tmpdir);
> -fd = mkostemps(ret, 4, O_CLOEXEC);
> +fd = g_mkstemp_full(ret, O_CLOEXEC, S_IRUSR | S_IWUSR);
> if (fd == -1) {
> -vshError(ctl, _("mkostemps: failed to create temporary file: %s"),
> +vshError(ctl, _("g_mkstemp: failed to create temporary file: %s"),
_full
> virStrerror(errno, ebuf, sizeof(ebuf)));
> VIR_FREE(ret);
> return NULL;
Reviewed-by: Peter Krempa
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 1/4] replace use of gnulib snprintf by g_snprintf
On Wed, Nov 13, 2019 at 17:33:24 +0100, Pavel Hrdina wrote: > Glib implementation follows the ISO C99 standard so it's safe to replace > the gnulib implementation. > > Signed-off-by: Pavel Hrdina > --- > > changes in v2: > - don't replace snprintf in tools/virt-login-shell.c Reviewed-by: Peter Krempa -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 12/12] tests: use GRegex in vboxsnapshotxmltest
On Wed, Nov 13, 2019 at 16:48:53 +0100, Ján Tomko wrote: > Signed-off-by: Ján Tomko > --- > tests/vboxsnapshotxmltest.c | 20 > 1 file changed, 8 insertions(+), 12 deletions(-) Reviewed-by: Peter Krempa -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 11/12] util: use GRegex in virStringMatch
On Wed, Nov 13, 2019 at 16:48:52 +0100, Ján Tomko wrote:
> Signed-off-by: Ján Tomko
> ---
> src/util/virstring.c | 22 --
> 1 file changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/src/util/virstring.c b/src/util/virstring.c
> index 3da793c87a..6add2278d8 100644
> --- a/src/util/virstring.c
> +++ b/src/util/virstring.c
> @@ -19,7 +19,6 @@
> #include
>
> #include
> -#include
> #include
>
> #include "c-ctype.h"
> @@ -1089,24 +1088,19 @@ bool
> virStringMatch(const char *str,
> const char *regexp)
> {
> -regex_t re;
> -int rv;
> +g_autoptr(GRegex) regex = NULL;
> +g_autoptr(GError) err = NULL;
>
> VIR_DEBUG("match '%s' for '%s'", str, regexp);
>
> -if ((rv = regcomp(&re, regexp, REG_EXTENDED | REG_NOSUB)) != 0) {
> -char error[100];
> -regerror(rv, &re, error, sizeof(error));
> -VIR_WARN("error while compiling regular expression '%s': %s",
> - regexp, error);
> -return false;
> +regex = g_regex_new(regexp, 0, 0, &err);
> +if (!regex) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to compile regex %s"), err->message);
> +return -1;
The function returns bool.
Additionally there is one non-test use of this function. Do you think we
should open-code it instead?
> }
>
> -rv = regexec(&re, str, 0, NULL, 0);
> -
> -regfree(&re);
> -
> -return rv == 0;
> +return g_regex_match(regex, str, 0, NULL);
> }
With the return value/error case sorted out:
Reviewed-by: Peter Krempa
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 10/12] util: use GRegex in virStringSearch
On Wed, Nov 13, 2019 at 16:48:51 +0100, Ján Tomko wrote:
> Signed-off-by: Ján Tomko
> ---
> src/util/virstring.c | 31 ++-
> 1 file changed, 14 insertions(+), 17 deletions(-)
>
> diff --git a/src/util/virstring.c b/src/util/virstring.c
> index 283cf8c8d8..3da793c87a 100644
> --- a/src/util/virstring.c
> +++ b/src/util/virstring.c
> @@ -1017,29 +1017,27 @@ virStringSearch(const char *str,
> size_t max_matches,
> char ***matches)
> {
> -regex_t re;
> -regmatch_t rem;
> +g_autoptr(GRegex) regex = NULL;
> +g_autoptr(GError) err = NULL;
> +g_autoptr(GMatchInfo) info = NULL;
> size_t nmatches = 0;
> ssize_t ret = -1;
> -int rv = -1;
>
> *matches = NULL;
>
> VIR_DEBUG("search '%s' for '%s'", str, regexp);
>
> -if ((rv = regcomp(&re, regexp, REG_EXTENDED)) != 0) {
> -char error[100];
> -regerror(rv, &re, error, sizeof(error));
> +regex = g_regex_new(regexp, 0, 0, &err);
> +if (!regex) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Error while compiling regular expression '%s':
> %s"),
> - regexp, error);
> + _("Failed to compile regex %s"), err->message);
> return -1;
> }
>
> -if (re.re_nsub != 1) {
> +if (g_regex_get_capture_count(regex) != 1) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Regular expression '%s' must have exactly 1 match
> group, not %zu"),
> - regexp, re.re_nsub);
> + _("Regular expression '%s' must have exactly 1 match
> group, not %d"),
> + regexp, g_regex_get_capture_count(regex));
> goto cleanup;
> }
>
> @@ -1051,28 +1049,27 @@ virStringSearch(const char *str,
>
> while ((nmatches - 1) < max_matches) {
> char *match;
> +int endpos;
>
> -if (regexec(&re, str, 1, &rem, 0) != 0)
> +if (!g_regex_match(regex, str, 0, &info))
info is overwritten and leaked
> break;
>
> if (VIR_EXPAND_N(*matches, nmatches, 1) < 0)
> goto cleanup;
>
> -if (VIR_STRNDUP(match, str + rem.rm_so,
> -rem.rm_eo - rem.rm_so) < 0)
> -goto cleanup;
> +match = g_match_info_fetch(info, 1);
>
> VIR_DEBUG("Got '%s'", match);
>
> (*matches)[nmatches-2] = match;
>
> -str = str + rem.rm_eo;
> +g_match_info_fetch_pos(info, 1, NULL, &endpos);
> +str += endpos;
> }
>
> ret = nmatches - 1; /* don't count the trailing null */
>
> cleanup:
> -regfree(&re);
> if (ret < 0) {
> virStringListFree(*matches);
> *matches = NULL;
With the memleak resolved:
Reviewed-by: Peter Krempa
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 09/12] util: use GRegex for virLogRegex
On Wed, Nov 13, 2019 at 16:48:50 +0100, Ján Tomko wrote: > Signed-off-by: Ján Tomko > --- > src/util/virlog.c | 15 +-- > 1 file changed, 5 insertions(+), 10 deletions(-) Reviewed-by: Peter Krempa -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 08/12] util: use GRegex in virCommandRunRegex
On Wed, Nov 13, 2019 at 16:48:49 +0100, Ján Tomko wrote:
> This saves us from allocating vars upfront, since GLib deals with
> that for us.
>
> Signed-off-by: Ján Tomko
> ---
> src/util/vircommand.c | 37 +
> 1 file changed, 13 insertions(+), 24 deletions(-)
>
> diff --git a/src/util/vircommand.c b/src/util/vircommand.c
> index 49432ddfcb..1ab3dbc819 100644
> --- a/src/util/vircommand.c
> +++ b/src/util/vircommand.c
> @@ -22,7 +22,6 @@
> #include
>
> #include
> -#include
> #include
> #include
> #include
> @@ -3077,11 +3076,10 @@ virCommandRunRegex(virCommandPtr cmd,
> const char *prefix,
> int *exitstatus)
> {
> -int err;
> -regex_t *reg;
> -g_autofree regmatch_t *vars = NULL;
> +GRegex **reg = NULL;
> +g_autoptr(GMatchInfo) info = NULL;
This g_autoptr ...
> size_t i, j, k;
> -int totgroups = 0, ngroup = 0, maxvars = 0;
> +int totgroups = 0, ngroup = 0;
> char **groups;
> g_autofree char *outbuf = NULL;
> VIR_AUTOSTRINGLIST lines = NULL;
> @@ -3092,29 +3090,23 @@ virCommandRunRegex(virCommandPtr cmd,
> return -1;
>
> for (i = 0; i < nregex; i++) {
> -err = regcomp(®[i], regex[i], REG_EXTENDED);
> -if (err != 0) {
> -char error[100];
> -regerror(err, ®[i], error, sizeof(error));
> +g_autoptr(GError) err = NULL;
> +reg[i] = g_regex_new(regex[i], G_REGEX_OPTIMIZE, 0, &err);
> +if (!reg[i]) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Failed to compile regex %s"), error);
> + _("Failed to compile regex %s"), err->message);
> for (j = 0; j < i; j++)
> -regfree(®[j]);
> +g_regex_unref(reg[j]);
> VIR_FREE(reg);
> return -1;
> }
>
> totgroups += nvars[i];
> -if (nvars[i] > maxvars)
> -maxvars = nvars[i];
> -
> }
>
> /* Storage for matched variables */
> if (VIR_ALLOC_N(groups, totgroups) < 0)
> goto cleanup;
> -if (VIR_ALLOC_N(vars, maxvars+1) < 0)
> -goto cleanup;
>
> virCommandSetOutputBuffer(cmd, &outbuf);
> if (virCommandRun(cmd, exitstatus) < 0)
> @@ -3140,15 +3132,12 @@ virCommandRunRegex(virCommandPtr cmd,
>
> ngroup = 0;
> for (i = 0; i < nregex; i++) {
> -if (regexec(®[i], p, nvars[i]+1, vars, 0) != 0)
> +if (!(g_regex_match(reg[i], p, 0, &info)))
... will not free the pointer returned here overwritten by the
iterating. The best will be to declare it just above.
> break;
>
> -/* NB vars[0] is the full pattern, so we offset j by 1 */
> -for (j = 1; j <= nvars[i]; j++) {
> -if (VIR_STRNDUP(groups[ngroup++], p + vars[j].rm_so,
> -vars[j].rm_eo - vars[j].rm_so) < 0)
> -goto cleanup;
> -}
> +/* NB match #0 is the full pattern, so we offset j by 1 */
> +for (j = 1; j <= nvars[i]; j++)
> +groups[ngroup++] = g_match_info_fetch(info, j);
>
> }
> /* We've matched on the last regex, so callback time */
> @@ -3170,7 +3159,7 @@ virCommandRunRegex(virCommandPtr cmd,
> }
>
> for (i = 0; i < nregex; i++)
> -regfree(®[i]);
> +g_regex_unref(reg[i]);
>
> VIR_FREE(reg);
> return ret;
with the memleak fixed:
Reviewed-by: Peter Krempa
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [tck PATCH 3/3] hooks: Use internal variable holding the state of the libvirtd service
On Tue, Nov 12, 2019 at 04:10:20PM +0100, Erik Skultety wrote: > Rather than assuming the existence of the 'service' command, use the > object variable holding the current state of the libvirtd service which > got populated right after we performed the tested action on the service. > --- > scripts/hooks/051-daemon-hook.t | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/2] a few cleanups in qemu_hotplug.c
On 10/17/19 11:44 AM, Daniel Henrique Barboza wrote: > There is not much to do regarding cleanup and g_auto* in this > file. Here's a couple of simple fixes I've found. > > > changes from v1: > - added a new patch to remove unused cleanup labels > > > v1: https://www.redhat.com/archives/libvir-list/2019-September/msg01463.html > > Daniel Henrique Barboza (2): > qemu_hotplug.c: use g_autoptr() with virConnectPtr > qemu_process: remove unused cleanup labels > > src/qemu/qemu_hotplug.c | 72 + > 1 file changed, 29 insertions(+), 43 deletions(-) > I tweaked the patch subjects, to use 'qemu: hotplug:' prefix, the X.c format isn't typically used. Patch 2 incorrectly mentioned qemu_process, so I fixed that too Reviewed-by: Cole Robinson and pushed - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 07/12] storage: use GRegex virStorageBackendLogicalParseVolExtents
On Thu, Nov 14, 2019 at 13:14:33 +0100, Ján Tomko wrote: > On Thu, Nov 14, 2019 at 08:11:46AM +0100, Peter Krempa wrote: > > On Wed, Nov 13, 2019 at 16:48:48 +0100, Ján Tomko wrote: > > > Using GRegex simplifies the code since g_match_info_fetch will > > > copy the matched substring for us. > > > > > > Signed-off-by: Ján Tomko > > > --- > > > src/storage/storage_backend_logical.c | 49 +++ > > > 1 file changed, 13 insertions(+), 36 deletions(-) > > > > I'm getting a build failure with this patch: > > > > ../../src/storage/storage_backend_logical.c:128:11: error: variable 'p' set > > but not used [-Werror=unused-but-set-variable] > > 128 | char *p = NULL; > > Oops, another case where GCC reports a slightly different set of > warnings than CLang. > > Consider the following squashed in. Jano > > diff --git a/src/storage/storage_backend_logical.c > b/src/storage/storage_backend_logical.c > index d6c81e25b1..42dec05ba0 100644 > --- a/src/storage/storage_backend_logical.c > +++ b/src/storage/storage_backend_logical.c > @@ -125,7 +125,6 @@ > virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol, > g_autoptr(GMatchInfo) info = NULL; > int nextents, ret = -1; > const char *regex_unit = "(\\S+)\\((\\S+)\\)"; > -char *p = NULL; > size_t i; > unsigned long long offset, size, length; > virStorageVolSourceExtent extent; > @@ -186,8 +185,6 @@ > virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol, > goto cleanup; > } > > -p = groups[3]; > - > /* Each extent has a "path:offset" pair, and match #0 > * is the whole matched string. > */ > Reviewed-by: Peter Krempa > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 6/6] remote/rpc: Use virNetServerGetProgram() to determine the program
On Thu, Nov 14, 2019 at 09:20 AM +0100, Pavel Hrdina wrote:
> On Wed, Nov 13, 2019 at 07:12:34PM +0100, Marc Hartmayer wrote:
>> On Wed, Nov 13, 2019 at 09:52 AM +0100, Pavel Hrdina
>> wrote:
>> > On Fri, Nov 01, 2019 at 06:35:48PM +0100, Marc Hartmayer wrote:
>> >> Use virNetServerGetProgram() to determine the virNetServerProgram
>> >> instead of using hard coded global variables. This allows us to remove
>> >> the global variables @remoteProgram and @qemuProgram as they're now no
>> >> longer necessary.
>> >>
>> >> Signed-off-by: Marc Hartmayer
>>
>> […snip…]
>>
>> >> virNetMessageErrorPtr rerr)
>> >> {
>> >> int rv = -1;
>> >> @@ -4180,6 +4180,12 @@
>> >> remoteDispatchConnectRegisterCloseCallback(virNetServerPtr server
>> >> G_GNUC_UNUSED,
>> >> struct daemonClientPrivate *priv =
>> >> virNetServerClientGetPrivateData(client);
>> >> virConnectPtr conn = remoteGetHypervisorConn(client);
>> >> +virNetServerProgramPtr program;
>> >> +
>> >> +if (!(program = virNetServerGetProgram(server, msg))) {
>> >> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no matching
>> >> program found"));
>> >> +goto cleanup;
>> >> +}
>> >
>> > This doesn't look right. If the function fails we will jump to cleanup
>> > where we will try to unlock &priv->lock. This has to happen after we
>> > acquire that lock.
>> >
>> > Pavel
>>
>> Yep, will fix that as well. Shall I directly return in the error case or
>> jump to another label (e.g. 'cleanup_unlock')?
>
> Returning directly would not work properly as well, we call
> virNetMessageSaveError() in case of error in the cleanup section.
>
> Another label would work.
>
>> Or do see any reason why we should hold the priv->lock during the
>> virNetServerGetProgram call?
>
> We don't have to hold the lock for virNetServerGetProgram(), it just
> makes the function easier to follow as there will be only one cleanup
> and I don't see any harm of holding the lock for that function call as
> well if the function will later wait for the same lock.
This would enforce the lock order 'server -> priv->lock' (not sure if
this is already the case) + it will become harder to identify what we’re
trying to protect with the lock. So if you have no strong opinion about
that I will introduce a 'cleanup_unlock' label. Fine with you?
Thanks.
>
> Pavel
--
Kind regards / Beste Grüße
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/4] qemu: Use host-model CPU on s390 by default
On s390 machines host-passthrough and host-model CPUs result in the same
guest ABI (with QEMU new enough to be able to tell us what "host" CPU is
expanded to, which was implemented around 2.9.0). So instead of using
host-passthrough CPU when there's no CPU specified in a domain XML we
can safely use host-model and benefit from CPU compatibility checks
during migration, snapshot restore and similar operations.
Signed-off-by: Jiri Denemark
---
src/qemu/qemu_domain.c| 33 ---
...t-cpu-kvm-ccw-virtio-4.2.s390x-latest.args | 4 ++-
...lt-cpu-kvm-ccw-virtio-4.2.s390x-latest.xml | 2 +-
3 files changed, 26 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 8bd05f4058..e5269e6300 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4440,6 +4440,7 @@ qemuDomainDefVcpusPostParse(virDomainDefPtr def)
static int
qemuDomainDefSetDefaultCPU(virDomainDefPtr def,
+ virCapsPtr caps,
virQEMUCapsPtr qemuCaps)
{
const char *model;
@@ -4470,26 +4471,36 @@ qemuDomainDefSetDefaultCPU(virDomainDefPtr def,
return -1;
}
-VIR_DEBUG("Setting default CPU model for domain '%s' to %s",
- def->name, model);
-
if (!def->cpu)
def->cpu = g_new0(virCPUDef, 1);
-/* We need to turn off all CPU checks when the domain is started because
- * the default CPU (e.g., qemu64) may not be runnable on any host. QEMU
- * will just disable the unavailable features and we will update the CPU
- * definition accordingly and set check to FULL when starting the domain.
*/
def->cpu->type = VIR_CPU_TYPE_GUEST;
-def->cpu->check = VIR_CPU_CHECK_NONE;
if (STREQ(model, "host")) {
-def->cpu->mode = VIR_CPU_MODE_HOST_PASSTHROUGH;
+if (ARCH_IS_S390(def->os.arch) &&
+virQEMUCapsIsCPUModeSupported(qemuCaps, caps, def->virtType,
+ VIR_CPU_MODE_HOST_MODEL)) {
+def->cpu->mode = VIR_CPU_MODE_HOST_MODEL;
+} else {
+def->cpu->mode = VIR_CPU_MODE_HOST_PASSTHROUGH;
+}
+
+VIR_DEBUG("Setting default CPU mode for domain '%s' to %s",
+ def->name, virCPUModeTypeToString(def->cpu->mode));
} else {
+/* We need to turn off all CPU checks when the domain is started
+ * because the default CPU (e.g., qemu64) may not be runnable on any
+ * host. QEMU will just disable the unavailable features and we will
+ * update the CPU definition accordingly and set check to FULL when
+ * starting the domain. */
+def->cpu->check = VIR_CPU_CHECK_NONE;
def->cpu->mode = VIR_CPU_MODE_CUSTOM;
def->cpu->match = VIR_CPU_MATCH_EXACT;
def->cpu->fallback = VIR_CPU_FALLBACK_FORBID;
def->cpu->model = g_strdup(model);
+
+VIR_DEBUG("Setting default CPU model for domain '%s' to %s",
+ def->name, model);
}
return 0;
@@ -4674,7 +4685,7 @@ qemuDomainDefPostParseBasic(virDomainDefPtr def,
static int
qemuDomainDefPostParse(virDomainDefPtr def,
- virCapsPtr caps G_GNUC_UNUSED,
+ virCapsPtr caps,
unsigned int parseFlags,
void *opaque,
void *parseOpaque)
@@ -4706,7 +4717,7 @@ qemuDomainDefPostParse(virDomainDefPtr def,
if (qemuCanonicalizeMachine(def, qemuCaps) < 0)
return -1;
-if (qemuDomainDefSetDefaultCPU(def, qemuCaps) < 0)
+if (qemuDomainDefSetDefaultCPU(def, caps, qemuCaps) < 0)
return -1;
qemuDomainDefEnableDefaultFeatures(def, qemuCaps);
diff --git
a/tests/qemuxml2argvdata/s390-default-cpu-kvm-ccw-virtio-4.2.s390x-latest.args
b/tests/qemuxml2argvdata/s390-default-cpu-kvm-ccw-virtio-4.2.s390x-latest.args
index 4b7345630b..0386019418 100644
---
a/tests/qemuxml2argvdata/s390-default-cpu-kvm-ccw-virtio-4.2.s390x-latest.args
+++
b/tests/qemuxml2argvdata/s390-default-cpu-kvm-ccw-virtio-4.2.s390x-latest.args
@@ -13,7 +13,9 @@ QEMU_AUDIO_DRV=none \
-object secret,id=masterKey0,format=raw,\
file=/tmp/lib/domain--1-test/master-key.aes \
-machine s390-ccw-virtio-4.2,accel=kvm,usb=off,dump-guest-core=off \
--cpu host \
+-cpu z13.2-base,aen=on,aefsi=on,msa5=on,msa4=on,msa3=on,msa2=on,msa1=on,\
+sthyi=on,edat=on,ri=on,edat2=on,vx=on,ipter=on,ap=on,esop=on,apft=on,apqci=on,\
+cte=on,bpb=on,ppa15=on,zpci=on,sea_esop2=on,te=on,cmm=on \
-m 256 \
-overcommit mem-lock=off \
-smp 1,sockets=1,cores=1,threads=1 \
diff --git
a/tests/qemuxml2xmloutdata/s390-default-cpu-kvm-ccw-virtio-4.2.s390x-latest.xml
b/tests/qemuxml2xmloutdata/s390-default-cpu-kvm-ccw-virtio-4.2.s390x-latest.xml
index eec5051934..1cc2edd893 100644
---
a/tests/qemuxml2xmloutdata/s390-default-cpu-kvm-ccw-virtio-4.2.s390x-latest.xml
+++
b/tests/qemuxml2xmloutdata/s390-default-cpu-kvm-ccw-virtio-4.2
[libvirt] [PATCH 0/4] qemu: Use host-model CPU on s390 by default
On s390 machines host-passthrough and host-model CPUs result in the same guest ABI (with QEMU new enough to be able to tell us what "host" CPU is expanded to, which was implemented around 2.9.0). So instead of using host-passthrough CPU when there's no CPU specified in a domain XML we can safely use host-model and benefit from CPU compatibility checks during migration, snapshot restore and similar operations. This series applies on top of "qemu: Store default CPU in domain XML" which is already acked, but it's waiting for a QEMU patch to be applied to 4.2.0. You can fetch both series at once using git fetch https://gitlab.com/jirkade/libvirt cpu-default-type Jiri Denemark (4): cpu_conf: Fix default value for CPU match attribute cpu_conf: Don't format empty model for host-model CPUs cpu_s390: Don't check match attribute for host-model CPUs qemu: Use host-model CPU on s390 by default src/conf/cpu_conf.c | 25 -- src/conf/cpu_conf.h | 2 +- src/cpu/cpu_s390.c| 18 +- src/qemu/qemu_domain.c| 33 --- .../ppc64-host+guest-compat-none.xml | 4 +-- ...t-cpu-kvm-ccw-virtio-4.2.s390x-latest.args | 4 ++- .../cpu-check-default-partial.xml | 4 +-- .../cpu-host-model-features.xml | 1 - ...lt-cpu-kvm-ccw-virtio-4.2.s390x-latest.xml | 2 +- 9 files changed, 45 insertions(+), 48 deletions(-) -- 2.24.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/4] cpu_s390: Don't check match attribute for host-model CPUs
The match attribute is only relevant for custom mode CPUs. Reporting
failure when match == 'minimum' regardless on CPU mode can cause
unexpected failures. We should only report the error for custom CPUs. In
fact, calling virCPUs390Update on a custom mode CPU should always report
an error as optional features are not supported on s390 either.
Signed-off-by: Jiri Denemark
---
src/cpu/cpu_s390.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/src/cpu/cpu_s390.c b/src/cpu/cpu_s390.c
index a4a381f4b8..dd030c5a11 100644
--- a/src/cpu/cpu_s390.c
+++ b/src/cpu/cpu_s390.c
@@ -49,15 +49,15 @@ virCPUs390Update(virCPUDefPtr guest,
int ret = -1;
size_t i;
-if (guest->match == VIR_CPU_MATCH_MINIMUM) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("match mode %s not supported"),
- virCPUMatchTypeToString(guest->match));
-goto cleanup;
-}
-
-if (guest->mode != VIR_CPU_MODE_HOST_MODEL) {
-ret = 0;
+if (guest->mode == VIR_CPU_MODE_CUSTOM) {
+if (guest->match == VIR_CPU_MATCH_MINIMUM) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("match mode %s not supported"),
+ virCPUMatchTypeToString(guest->match));
+} else {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("optional CPU features are not supported"));
+}
goto cleanup;
}
--
2.24.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/4] cpu_conf: Fix default value for CPU match attribute
Commit v0.8.4-66-g95ff6b18ec (9 years ago) changed the default value for
the cpu/@match attribute to 'exact' in a rather complicated way. It did
so only if subelement was present and set -1 otherwise (which is
not expected to ever happen). Thus the following two equivalent XML
elements:
and
would be parsed differently. The former would end up with match == -1
while the latter would have match == 1 ('exact'). This is not a big deal
since the match attribute is ignored for host-model CPUs, but we can
simplify the code and make it a little bit saner anyway.
Signed-off-by: Jiri Denemark
---
Notes:
Well, thanks to a bug (fixed in another patch in this series) the
s390 CPU driver actually looks at the match attribute even for
host-model CPUs.
src/conf/cpu_conf.c | 9 ++---
src/conf/cpu_conf.h | 2 +-
2 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
index 2a1bd7c9b2..3641b5ef4c 100644
--- a/src/conf/cpu_conf.c
+++ b/src/conf/cpu_conf.c
@@ -46,8 +46,8 @@ VIR_ENUM_IMPL(virCPUMode,
VIR_ENUM_IMPL(virCPUMatch,
VIR_CPU_MATCH_LAST,
- "minimum",
"exact",
+ "minimum",
"strict",
);
@@ -388,12 +388,7 @@ virCPUDefParseXML(xmlXPathContextPtr ctxt,
char *match = virXMLPropString(ctxt->node, "match");
char *check;
-if (!match) {
-if (virXPathBoolean("boolean(./model)", ctxt))
-def->match = VIR_CPU_MATCH_EXACT;
-else
-def->match = -1;
-} else {
+if (match) {
def->match = virCPUMatchTypeFromString(match);
VIR_FREE(match);
diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h
index e41d47d1ae..96fda3e6b3 100644
--- a/src/conf/cpu_conf.h
+++ b/src/conf/cpu_conf.h
@@ -52,8 +52,8 @@ typedef enum {
VIR_ENUM_DECL(virCPUMode);
typedef enum {
-VIR_CPU_MATCH_MINIMUM,
VIR_CPU_MATCH_EXACT,
+VIR_CPU_MATCH_MINIMUM,
VIR_CPU_MATCH_STRICT,
VIR_CPU_MATCH_LAST
--
2.24.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/4] cpu_conf: Don't format empty model for host-model CPUs
Most likely for historical reasons our CPU def formatting code is
happily adding useless for host-model CPUs. We
can just drop it.
Signed-off-by: Jiri Denemark
---
src/conf/cpu_conf.c | 16 +---
.../cputestdata/ppc64-host+guest-compat-none.xml | 4 +---
.../cpu-check-default-partial.xml| 4 +---
.../cpu-host-model-features.xml | 1 -
4 files changed, 7 insertions(+), 18 deletions(-)
diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
index 3641b5ef4c..4542bcb7bd 100644
--- a/src/conf/cpu_conf.c
+++ b/src/conf/cpu_conf.c
@@ -751,16 +751,12 @@ virCPUDefFormatBuf(virBufferPtr buf,
{
size_t i;
bool formatModel;
-bool formatFallback;
if (!def)
return 0;
formatModel = (def->mode == VIR_CPU_MODE_CUSTOM ||
def->mode == VIR_CPU_MODE_HOST_MODEL);
-formatFallback = (def->type == VIR_CPU_TYPE_GUEST &&
- (def->mode == VIR_CPU_MODE_HOST_MODEL ||
- (def->mode == VIR_CPU_MODE_CUSTOM && def->model)));
if (!def->model && def->mode == VIR_CPU_MODE_CUSTOM && def->nfeatures) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -768,9 +764,10 @@ virCPUDefFormatBuf(virBufferPtr buf,
return -1;
}
-if ((formatModel && def->model) || formatFallback) {
+if (formatModel && def->model) {
virBufferAddLit(buf, "type == VIR_CPU_TYPE_GUEST) {
const char *fallback;
fallback = virCPUFallbackTypeToString(def->fallback);
@@ -784,11 +781,8 @@ virCPUDefFormatBuf(virBufferPtr buf,
if (def->vendor_id)
virBufferEscapeString(buf, " vendor_id='%s'", def->vendor_id);
}
-if (formatModel && def->model) {
-virBufferEscapeString(buf, ">%s\n", def->model);
-} else {
-virBufferAddLit(buf, "/>\n");
-}
+
+virBufferEscapeString(buf, ">%s\n", def->model);
}
if (formatModel && def->vendor)
diff --git a/tests/cputestdata/ppc64-host+guest-compat-none.xml
b/tests/cputestdata/ppc64-host+guest-compat-none.xml
index 188ebebb72..fd50c03a79 100644
--- a/tests/cputestdata/ppc64-host+guest-compat-none.xml
+++ b/tests/cputestdata/ppc64-host+guest-compat-none.xml
@@ -1,3 +1 @@
-
-
-
+
diff --git a/tests/qemuxml2xmloutdata/cpu-check-default-partial.xml
b/tests/qemuxml2xmloutdata/cpu-check-default-partial.xml
index 4e5fa44832..b64a1f0ef7 100644
--- a/tests/qemuxml2xmloutdata/cpu-check-default-partial.xml
+++ b/tests/qemuxml2xmloutdata/cpu-check-default-partial.xml
@@ -8,9 +8,7 @@
hvm
-
-
-
+
destroy
restart
diff --git a/tests/qemuxml2xmloutdata/cpu-host-model-features.xml
b/tests/qemuxml2xmloutdata/cpu-host-model-features.xml
index a5de9ea38d..6480bd5494 100644
--- a/tests/qemuxml2xmloutdata/cpu-host-model-features.xml
+++ b/tests/qemuxml2xmloutdata/cpu-host-model-features.xml
@@ -14,7 +14,6 @@
-
--
2.24.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH] virhostuptime: Wrap virHostGetBootTimeProcfs() call in an ifdef
The virHostGetBootTimeProcfs() function is defined only for Linux and therefore it's only call should also be done if we're on Linux. Signed-off-by: Michal Privoznik --- Pushed under trivial rule. src/util/virhostuptime.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/util/virhostuptime.c b/src/util/virhostuptime.c index 056b5c591a..8c49c3d40e 100644 --- a/src/util/virhostuptime.c +++ b/src/util/virhostuptime.c @@ -99,8 +99,10 @@ virHostGetBootTimeOnceInit(void) endutxent(); # endif /* HAVE_GETUTXID */ +# ifdef __linux__ if (bootTimeErrno != 0 || bootTime == 0) bootTimeErrno = -virHostGetBootTimeProcfs(&bootTime); +# endif /* __linux__ */ } #else /* !defined(HAVE_GETUTXID) && !defined(__linux__) */ -- 2.23.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 06/12] libxl: remove 'ret' from xenParseSxprVifRate
On Wed, Nov 13, 2019 at 16:48:47 +0100, Ján Tomko wrote: > Now that the cleanup section is empty, the ret variable is no longer > necessary. > > Signed-off-by: Ján Tomko > --- > src/libxl/xen_common.c | 10 +++--- > 1 file changed, 3 insertions(+), 7 deletions(-) Reviewed-by: Peter Krempa -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 05/12] libxl: use GRegex in xenParseSxprVifRate
On Wed, Nov 13, 2019 at 16:48:46 +0100, Ján Tomko wrote: > Use GRegex from GLib instead of regcomp. > > Signed-off-by: Ján Tomko > --- > src/libxl/xen_common.c | 20 +++- > 1 file changed, 7 insertions(+), 13 deletions(-) Reviewed-by: Peter Krempa -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/3] virt-aa-helper: drop pointer checks in get_files
On 11/14/19 6:20 AM, Christian Ehrhardt wrote: > It was mentioned that the pointers in loops like: > for (i = 0; i < ctl->def->nserials; i++) > if (ctl->def->serials[i] ... > will always be !NULL or we would have a much more serious problem. > > Simplify the if chains in get_files by dropping these checks. > > Signed-off-by: Christian Ehrhardt > --- > src/security/virt-aa-helper.c | 135 -- > 1 file changed, 63 insertions(+), 72 deletions(-) > Reviewed-by: Cole Robinson - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 04/12] libxl: use g_autofree in xenParseSxprVifRate
On Wed, Nov 13, 2019 at 16:48:45 +0100, Ján Tomko wrote: > Signed-off-by: Ján Tomko > --- > src/libxl/xen_common.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Reviewed-by: Peter Krempa -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 03/12] libxl: use GRegex in libxlGetAutoballoonConf
On Wed, Nov 13, 2019 at 16:48:44 +0100, Ján Tomko wrote: > Replace the use of regcomp with GRegex. > > Signed-off-by: Ján Tomko > --- > src/libxl/libxl_conf.c | 20 +++- > 1 file changed, 7 insertions(+), 13 deletions(-) Reviewed-by: Peter Krempa -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 02/12] remove unused regex.h includes
On Wed, Nov 13, 2019 at 16:48:43 +0100, Ján Tomko wrote: > The code using regexes got moved, but the include stayed. > > Signed-off-by: Ján Tomko > --- > src/libxl/libxl_driver.c | 1 - > src/storage/storage_util.c | 1 - > src/util/viriscsi.c| 2 -- > src/util/virnetdevtap.c| 1 - > tests/testutils.c | 1 - > 5 files changed, 6 deletions(-) Reviewed-by: Peter Krempa -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Warn verbosely if using old loader:nvram pairs
On Thu, Nov 14, 2019 at 03:26:33PM +0100, Michal Privoznik wrote: > On 11/13/19 7:59 PM, Jim Fehlig wrote: > > On 11/13/19 9:20 AM, Michal Privoznik wrote: > > > On 11/12/19 11:17 PM, Jim Fehlig wrote: > > > > On 11/11/19 9:42 AM, Michal Privoznik wrote: > > > > > There are two ways for specifying loader:nvram pairs: > > > > > > > > > > 1) --with-loader-nvram configure option > > > > > 2) nvram variable in qemu.conf > > > > > > > > > > Since we have FW descriptors, using this old style is > > > > > discouraged, but not as strong as one would expect. Produce more > > > > > warnings: > > > > > > > > Oh, I didn't know the old style was discouraged when using FW > > > > descriptors. > > > > Thanks for mentioning it! > > > > > > > > > > > > > > 1) produce a warning if somebody tries the configure option > > > > > 2) produce a warning if somebody sets nvram variable and at > > > > > least on FW descriptor was found > > > > > > > > > > The reason for producing warning in case 1) is that package > > > > > maintainers, who set the configure option in the first place > > > > > should start moving towards FW descriptors and abandon the > > > > > configure option. After all, the warning is printed into config > > > > > output only in this case. > > > > > > > > Should the configure option be removed from the upstream spec file? > > > > > > Definitely, Fedora ships FW descriptors and so does RHEL. What's the > > > state in > > > OpenSUSE? But I bet you have your own spec file anyway, don't you? Just > > > like we > > > have for Fedora and RHEL. Will post a patch tomorrow (unless you beat me > > > to it). > > > > I was attempting to beat you to it, but removing $LOADERS from the spec file > > means removing use of --with-loader-nvram configure option. Does that mean > > we > > should remove associated code from configure.ac, nuke > > m4/virt-loader-nvram.m4, > > remove DEFAULT_LOADER_NVRAM from qemu and libxl drivers, ...? It seems a bit > > early for that since you are just now adding the deprecation warning to > > virt-loader-nvram.m4 :-). Should we wait until completely removing support > > for > > the build-time FW list before nuking from the spec file? > > I think it's safe to remove the configure argument from the spec file, but > at this point I'd rather wait and give distros a while to adapt to FW > descriptors before removing the argument even from configure script. I mean, > I can see us removing the argument and nvram=[] from qemu.conf at the same > time, but not right now. As long as we aim to support QEMU versions (aka our QEMU min version in caps probing) that pre-date the FW descriptor spec, we shouldn't remove support for this from our configure script. Removing it from the RPM spec is fine since thd spec assumes recent Fedora or RHEL only which both have FW descriptor support. 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 01/12] libxl: do not use G_REGEX_EXTENDED
On Wed, Nov 13, 2019 at 16:48:42 +0100, Ján Tomko wrote: > This flag is not needed to use extended regular expression syntax > with GRegex and it makes GRegex ignore whitespace in the regex. > > Remove the unintended usage, even though it should not matter in this > case. > > Signed-off-by: Ján Tomko > --- > src/libxl/libxl_capabilities.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Peter Krempa -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Warn verbosely if using old loader:nvram pairs
On 11/13/19 7:59 PM, Jim Fehlig wrote: On 11/13/19 9:20 AM, Michal Privoznik wrote: On 11/12/19 11:17 PM, Jim Fehlig wrote: On 11/11/19 9:42 AM, Michal Privoznik wrote: There are two ways for specifying loader:nvram pairs: 1) --with-loader-nvram configure option 2) nvram variable in qemu.conf Since we have FW descriptors, using this old style is discouraged, but not as strong as one would expect. Produce more warnings: Oh, I didn't know the old style was discouraged when using FW descriptors. Thanks for mentioning it! 1) produce a warning if somebody tries the configure option 2) produce a warning if somebody sets nvram variable and at least on FW descriptor was found The reason for producing warning in case 1) is that package maintainers, who set the configure option in the first place should start moving towards FW descriptors and abandon the configure option. After all, the warning is printed into config output only in this case. Should the configure option be removed from the upstream spec file? Definitely, Fedora ships FW descriptors and so does RHEL. What's the state in OpenSUSE? But I bet you have your own spec file anyway, don't you? Just like we have for Fedora and RHEL. Will post a patch tomorrow (unless you beat me to it). I was attempting to beat you to it, but removing $LOADERS from the spec file means removing use of --with-loader-nvram configure option. Does that mean we should remove associated code from configure.ac, nuke m4/virt-loader-nvram.m4, remove DEFAULT_LOADER_NVRAM from qemu and libxl drivers, ...? It seems a bit early for that since you are just now adding the deprecation warning to virt-loader-nvram.m4 :-). Should we wait until completely removing support for the build-time FW list before nuking from the spec file? I think it's safe to remove the configure argument from the spec file, but at this point I'd rather wait and give distros a while to adapt to FW descriptors before removing the argument even from configure script. I mean, I can see us removing the argument and nvram=[] from qemu.conf at the same time, but not right now. Anyway, I'll let you write the patch since it looks like you already have it. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] docs: mention lifted vCPUs restriction for esx
On Wed, Nov 13, 2019 at 01:57:34PM +0100, Pino Toscano wrote: It was lifted with c92b6023e8eb670e01571e299a85e9da9bd4844c. Signed-off-by: Pino Toscano --- docs/drvesx.html.in | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) Reviewed-by: Ján Tomko and pushed 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 2/2] qemu: cold-unplug of sound
On 10/15/19 2:41 AM, Jidong Xia wrote:
> With this patch users can cold unplug some sound devices.
> use "virsh detach-device vm sound.xml --config" command.
>
I reviewed and pushed patch #1
Some comments below
> Signed-off-by: Jidong Xia
> ---
> src/conf/domain_conf.c | 61
>
> src/conf/domain_conf.h | 2 ++
> src/libvirt_private.syms | 2 ++
> src/qemu/qemu_driver.c | 9 ++-
> 4 files changed, 73 insertions(+), 1 deletion(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index c1705a0..4083b7c 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2754,6 +2754,15 @@ void
> virDomainSmartcardDefFree(virDomainSmartcardDefPtr def)
> VIR_FREE(def);
> }
>
> +virDomainSoundDefPtr
> +virDomainSoundDefRemove(virDomainDefPtr def, size_t idx)
> +{
> +virDomainSoundDefPtr ret = def->sounds[idx];
> +VIR_DELETE_ELEMENT(def->sounds, idx, def->nsounds);
> +return ret;
> + }
> +
> +
> void virDomainSoundCodecDefFree(virDomainSoundCodecDefPtr def)
> {
> if (!def)
> @@ -17211,6 +17220,58 @@ virDomainNetUpdate(virDomainDefPtr def,
> return 0;
> }
>
> +/**
> + * virDomainSoundFindIdx:
> + * @def: domain definition
> + * @sound: sound definition
> + *
> + * Lookup domain's sound interface based on passed @sound
> + * definition.
> + *
> + * Return: index of match if unique match found,
> + * -1 otherwise and an error is logged.
> + */
> +
> +int
> +virDomainSoundFindIdx(virDomainDefPtr def, virDomainSoundDefPtr sound)
> +{
> +size_t i;
> +int matchidx = -1;
> +bool PCIAddrSpecified = virDomainDeviceAddressIsValid(&sound->info,
> +
> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI);
> +
> +for (i = 0; i < def->nsounds; i++) {
> +
> +if (PCIAddrSpecified &&
> +!virPCIDeviceAddressEqual(&def->sounds[i]->info.addr.pci,
> + &sound->info.addr.pci))
> +continue;
> +
> +if (matchidx >= 0) {
> +virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> + _("multiple matching devices found"));
> +
> +return -1;
> +}
> +
> +matchidx = i;
> +}
> +
> +if (matchidx < 0) {
> +if (PCIAddrSpecified) {
> +virReportError(VIR_ERR_DEVICE_MISSING,
> + _("no device found on %.4x:%.2x:%.2x.%.1x"),
> + sound->info.addr.pci.domain,
> + sound->info.addr.pci.bus,
> + sound->info.addr.pci.slot,
> + sound->info.addr.pci.function);
> +} else {
> +virReportError(VIR_ERR_DEVICE_MISSING, "%s",
> + _("no matching device found"));
> +}
> +}
> +return matchidx;
> +}
>
I think the model to follow should be virDomainInputDefFind +
virDomainInputDefEquals
* Put all the matching logic in the Equals function
* It should match ->model as well. This way a use could detach just an
XML with without requiring the block
* Use virDomainDeviceInfoAddressIsEqual instead of restricting it to
just PCI address. Some day maybe we support usb-audio which would have a
different address type for example
You can CC me on v2 and I'll review it
Thanks,
Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/5] util: file: Use more obvious logic in virFindFileInPath
On Thu, Nov 14, 2019 at 02:29:20PM +0100, Peter Krempa wrote: Make it more obvious that the function will return NULL if the file is not executable and stop reusing variables. Signed-off-by: Peter Krempa --- v2: - fixed logic to do the same as it did before - rewrote commit message to accomodate change src/util/virfile.c | 18 +++--- 1 file changed, 11 insertions(+), 7 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
[libvirt] [PATCH 3/3] syntax-check: prefer g_mkstemp_full and g_mkdtemp
Signed-off-by: Ján Tomko --- build-aux/syntax-check.mk | 13 ++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index 74d0d5f6d4..7d1ac4dbfc 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -402,10 +402,17 @@ sc_prohibit_fork_wrappers: halt='use virCommand for child processes' \ $(_sc_search_regexp) -# Prefer mkostemp with O_CLOEXEC. +# Prefer g_mkostemp_full with O_CLOEXEC. sc_prohibit_mkstemp: - @prohibit='[^"]\https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/3] Use g_mkdtemp instead of mkdtemp
Prefer the GLib version to the one from gnulib.
Signed-off-by: Ján Tomko
---
src/qemu/qemu_process.c | 2 +-
tests/fdstreamtest.c | 2 +-
tests/qemuhotplugtest.c | 2 +-
tests/qemumemlocktest.c | 2 +-
tests/qemumonitortestutils.c | 2 +-
tests/qemuxml2argvtest.c | 2 +-
tests/qemuxml2xmltest.c | 2 +-
tests/scsihosttest.c | 2 +-
tests/testutilsqemu.c| 4 ++--
tests/vircgrouptest.c| 2 +-
tests/virhostdevtest.c | 2 +-
tests/virnetsockettest.c | 4 ++--
tests/virpcitest.c | 2 +-
tests/virscsitest.c | 2 +-
14 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 4cf4069d50..a669fc608f 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -8508,7 +8508,7 @@ qemuProcessQMPInit(qemuProcessQMPPtr proc)
template = g_strdup_printf("%s/qmp-XX", proc->libDir);
-if (!(proc->uniqDir = mkdtemp(template))) {
+if (!(proc->uniqDir = g_mkdtemp(template))) {
virReportSystemError(errno,
_("Failed to create unique directory with "
"template '%s' for probing QEMU"),
diff --git a/tests/fdstreamtest.c b/tests/fdstreamtest.c
index f4d38d5a20..03f5520fa6 100644
--- a/tests/fdstreamtest.c
+++ b/tests/fdstreamtest.c
@@ -319,7 +319,7 @@ mymain(void)
char scratchdir[] = SCRATCHDIRTEMPLATE;
int ret = 0;
-if (!mkdtemp(scratchdir)) {
+if (!g_mkdtemp(scratchdir)) {
virFilePrintf(stderr, "Cannot create fdstreamdir");
abort();
}
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index a0a579f29a..5356785fa4 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -593,7 +593,7 @@ mymain(void)
fakerootdir = g_strdup(FAKEROOTDIRTEMPLATE);
-if (!mkdtemp(fakerootdir)) {
+if (!g_mkdtemp(fakerootdir)) {
fprintf(stderr, "Cannot create fakerootdir");
abort();
}
diff --git a/tests/qemumemlocktest.c b/tests/qemumemlocktest.c
index d5a4fb1268..52cd9f9f26 100644
--- a/tests/qemumemlocktest.c
+++ b/tests/qemumemlocktest.c
@@ -62,7 +62,7 @@ mymain(void)
fakerootdir = g_strdup(FAKEROOTDIRTEMPLATE);
-if (!mkdtemp(fakerootdir)) {
+if (!g_mkdtemp(fakerootdir)) {
fprintf(stderr, "Cannot create fakerootdir");
abort();
}
diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c
index d7ecdb2ef9..617a2997d1 100644
--- a/tests/qemumonitortestutils.c
+++ b/tests/qemumonitortestutils.c
@@ -1045,7 +1045,7 @@ qemuMonitorCommonTestNew(virDomainXMLOptionPtr xmlopt,
tmpdir_template = g_strdup("/tmp/libvirt_XX");
-if (!(test->tmpdir = mkdtemp(tmpdir_template))) {
+if (!(test->tmpdir = g_mkdtemp(tmpdir_template))) {
virReportSystemError(errno, "%s",
"Failed to create temporary directory");
goto error;
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index f0405866e1..5e1d6a45c5 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -609,7 +609,7 @@ mymain(void)
fakerootdir = g_strdup(FAKEROOTDIRTEMPLATE);
-if (!mkdtemp(fakerootdir)) {
+if (!g_mkdtemp(fakerootdir)) {
fprintf(stderr, "Cannot create fakerootdir");
abort();
}
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 8b256b9b19..08c85d3b73 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -142,7 +142,7 @@ mymain(void)
fakerootdir = g_strdup(FAKEROOTDIRTEMPLATE);
-if (!mkdtemp(fakerootdir)) {
+if (!g_mkdtemp(fakerootdir)) {
fprintf(stderr, "Cannot create fakerootdir");
abort();
}
diff --git a/tests/scsihosttest.c b/tests/scsihosttest.c
index b35ec6980a..e8193836ff 100644
--- a/tests/scsihosttest.c
+++ b/tests/scsihosttest.c
@@ -250,7 +250,7 @@ mymain(void)
fakerootdir = g_strdup(FAKEROOTDIRTEMPLATE);
-if (!mkdtemp(fakerootdir)) {
+if (!g_mkdtemp(fakerootdir)) {
fprintf(stderr, "Cannot create fakerootdir");
goto cleanup;
}
diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
index 6fe385e545..5a7011478b 100644
--- a/tests/testutilsqemu.c
+++ b/tests/testutilsqemu.c
@@ -709,14 +709,14 @@ int qemuTestDriverInit(virQEMUDriver *driver)
driver->config->libDir = g_strdup("/tmp/lib");
driver->config->channelTargetDir = g_strdup("/tmp/channel");
-if (!mkdtemp(statedir)) {
+if (!g_mkdtemp(statedir)) {
virFilePrintf(stderr, "Cannot create fake stateDir");
goto error;
}
driver->config->stateDir = g_strdup(statedir);
-if (!mkdtemp(configdir)) {
+if (!g_mkdtemp(configdir)) {
virFilePrintf(stderr, "Cannot create fake configDir");
goto error;
}
diff --git a/tests/vircgrouptest.c b/tests/vircgrouptest.c
index e1770426d7..c952676c25 100644
--- a/tests/vircgrouptest.c
+++ b/
[libvirt] [PATCH 0/3] Prefer g_mkstemp_full and g_mkdtemp (glib chronicles)
Remove the dependency on three more gnulib modules: mkdtemp mkostemp mkostemps Note: this series is not affiliated with the kill-a-gnulib-module-a-day initiative Ján Tomko (3): Use g_mkstemp_full instead of mkostemp(s) Use g_mkdtemp instead of mkdtemp syntax-check: prefer g_mkstemp_full and g_mkdtemp build-aux/syntax-check.mk| 13 ++--- src/qemu/qemu_driver.c | 8 src/qemu/qemu_process.c | 2 +- src/storage/storage_driver.c | 2 +- src/storage/storage_util.c | 2 +- src/util/virlog.c| 8 +--- src/vbox/vbox_common.c | 4 ++-- tests/fdstreamtest.c | 2 +- tests/qemuhotplugtest.c | 2 +- tests/qemumemlocktest.c | 2 +- tests/qemumonitortestutils.c | 2 +- tests/qemuxml2argvtest.c | 2 +- tests/qemuxml2xmltest.c | 2 +- tests/scsihosttest.c | 2 +- tests/testutilsqemu.c| 4 ++-- tests/vircgrouptest.c| 2 +- tests/virfiletest.c | 2 +- tests/virhostdevtest.c | 2 +- tests/virnetsockettest.c | 4 ++-- tests/virpcitest.c | 2 +- tests/virscsitest.c | 2 +- tools/vsh.c | 4 ++-- 22 files changed, 38 insertions(+), 37 deletions(-) -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/3] Use g_mkstemp_full instead of mkostemp(s)
With g_mkstemp_full, there is no need to distinguish between
mkostemp and mkostemps (no suffix vs. a suffix of a fixed length),
because the GLib function looks for the XX pattern everywhere
in the string.
Use S_IRUSR | S_IWUSR for the permissions and do not pass O_RDWR
in flags since it's implied.
Signed-off-by: Ján Tomko
---
src/qemu/qemu_driver.c | 8
src/storage/storage_driver.c | 2 +-
src/storage/storage_util.c | 2 +-
src/util/virlog.c| 8 +---
src/vbox/vbox_common.c | 4 ++--
tests/virfiletest.c | 2 +-
tools/vsh.c | 4 ++--
7 files changed, 12 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 159a6dc464..a9364dc7e3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4024,8 +4024,8 @@ qemuDomainScreenshot(virDomainPtr dom,
if (!(tmp = g_strdup_printf("%s/qemu.screendump.XX", cfg->cacheDir)))
goto endjob;
-if ((tmp_fd = mkostemp(tmp, O_CLOEXEC)) == -1) {
-virReportSystemError(errno, _("mkostemp(\"%s\") failed"), tmp);
+if ((tmp_fd = g_mkstemp_full(tmp, O_CLOEXEC, S_IRUSR | S_IWUSR)) == -1) {
+virReportSystemError(errno, _("g_mkstemp(\"%s\") failed"), tmp);
goto endjob;
}
unlink_tmp = true;
@@ -11963,9 +11963,9 @@ qemuDomainMemoryPeek(virDomainPtr dom,
goto endjob;
/* Create a temporary filename. */
-if ((fd = mkostemp(tmp, O_CLOEXEC)) == -1) {
+if ((fd = g_mkstemp_full(tmp, O_CLOEXEC, S_IRUSR | S_IWUSR)) == -1) {
virReportSystemError(errno,
- _("mkostemp(\"%s\") failed"), tmp);
+ _("g_mkstemp(\"%s\") failed"), tmp);
goto endjob;
}
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 04e4abcd6a..d8355d3c3c 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -2825,7 +2825,7 @@ virStoragePoolObjFindPoolByUUID(const unsigned char *uuid)
*
* Generate a name for a temporary file using the driver stateDir
* as a path, the pool name, and the volume name to be used as input
- * for a mkostemp
+ * for mkstemp
*
* Returns a string pointer on success, NULL on failure
*/
diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index f91c2c64ee..8cc308e12d 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -1216,7 +1216,7 @@
storageBackendCreateQemuImgSecretPath(virStoragePoolObjPtr pool,
if (!(secretPath = virStoragePoolObjBuildTempFilePath(pool, vol)))
goto cleanup;
-if ((fd = mkostemp(secretPath, O_CLOEXEC)) < 0) {
+if ((fd = g_mkstemp_full(secretPath, O_CLOEXEC, S_IRUSR | S_IWUSR)) < 0) {
virReportSystemError(errno, "%s",
_("failed to open secret file for write"));
goto error;
diff --git a/src/util/virlog.c b/src/util/virlog.c
index b3460d85fe..dcb287f146 100644
--- a/src/util/virlog.c
+++ b/src/util/virlog.c
@@ -996,13 +996,7 @@ virLogOutputToJournald(virLogSourcePtr source,
* and pass an FD to the journal
*/
-/* NB: mkostemp is not declared async signal safe by
- * POSIX, but this is Linux only code and the GLibc
- * impl is safe enough, only using open() and inline
- * asm to read a timestamp (falling back to gettimeofday
- * on some arches
- */
-if ((buffd = mkostemp(path, O_CLOEXEC|O_RDWR)) < 0)
+if ((buffd = g_mkstemp_full(path, O_CLOEXEC, S_IRUSR | S_IWUSR)) < 0)
return;
if (unlink(path) < 0)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 0bd47e3ddb..043c26b9f6 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -7385,8 +7385,8 @@ vboxDomainScreenshot(virDomainPtr dom,
tmp = g_strdup_printf("%s/vbox.screendump.XX", cacheDir);
-if ((tmp_fd = mkostemp(tmp, O_CLOEXEC)) == -1) {
-virReportSystemError(errno, _("mkostemp(\"%s\") failed"), tmp);
+if ((tmp_fd = g_mkstemp_full(tmp, O_CLOEXEC, S_IRUSR | S_IWUSR)) == -1) {
+virReportSystemError(errno, _("g_mkstemp(\"%s\") failed"), tmp);
VIR_FREE(tmp);
VBOX_RELEASE(machine);
return NULL;
diff --git a/tests/virfiletest.c b/tests/virfiletest.c
index c7d5f6abeb..193c5bedd4 100644
--- a/tests/virfiletest.c
+++ b/tests/virfiletest.c
@@ -133,7 +133,7 @@ makeSparseFile(const off_t offsets[],
off_t len = 0;
size_t i;
-if ((fd = mkostemp(path, O_CLOEXEC|O_RDWR)) < 0)
+if ((fd = g_mkstemp_full(path, O_CLOEXEC, S_IRUSR | S_IWUSR)) < 0)
goto error;
if (unlink(path) < 0)
diff --git a/tools/vsh.c b/tools/vsh.c
index 000cf6a009..e851303e69 100644
--- a/tools/vsh.c
+++ b/tools/vsh.c
@@ -2400,9 +2400,9 @@ vshEditWriteToTempFile(vshControl *ctl, const char *doc)
tmpdir = getenv("TMPDIR");
if (!tmpdir) tmpdir = "/tmp";
ret = g_strdup_printf("%s/virshXX.xml", tmpdir);
-fd = mkostemps(ret, 4,
[libvirt] [PATCH v2 3/5] util: file: Use more obvious logic in virFindFileInPath
Make it more obvious that the function will return NULL if the file is
not executable and stop reusing variables.
Signed-off-by: Peter Krempa
---
v2:
- fixed logic to do the same as it did before
- rewrote commit message to accomodate change
src/util/virfile.c | 18 +++---
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 072a299b39..c7620e49d5 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -1645,19 +1645,23 @@ virFindFileInPath(const char *file)
* copy of that path, after validating that it is executable
*/
if (IS_ABSOLUTE_FILE_NAME(file)) {
-char *ret = NULL;
-if (virFileIsExecutable(file))
-ret = g_strdup(file);
-return ret;
+if (!virFileIsExecutable(file))
+return NULL;
+
+return g_strdup(file);
}
/* If we are passed an anchored path (containing a /), then there
* is no path search - it must exist in the current directory
*/
if (strchr(file, '/')) {
-if (virFileIsExecutable(file))
-ignore_value(virFileAbsPath(file, &path));
-return path;
+char *abspath = NULL;
+
+if (!virFileIsExecutable(file))
+return NULL;
+
+ignore_value(virFileAbsPath(file, &abspath));
+return abspath;
}
/* copy PATH env so we can tweak it */
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] libxl: Fix lock manager lock ordering
On 10/14/19 5:39 PM, Jim Fehlig wrote: > The ordering of lock manager locks in the libxl driver has a flaw that was > uncovered by a migration error path. In the perform phase of migration, the > source host calls virDomainLockProcessPause to release the lock before > sending the VM to the destination host. If the send fails an attempt is made > to reacquire the lock with virDomainLockProcessResume, but that too can fail > if the destination host has not finished cleaning up the failed VM and > releasing the lock it acquired when starting to receive the VM. > > This change delays calling virDomainLockProcessResume in libxlDomainStart > until the VM is successfully created, but before it is unpaused. A similar > approach is used by the qemu driver, avoiding the need to release the lock > if VM creation fails. In the migration perform phase, releasing the lock > with virDomainLockProcessPause is delayed until the VM is successfully > sent to the destination, which avoids reacquiring the lock if the send > fails. > > Signed-off-by: Jim Fehlig > --- > src/libxl/libxl_domain.c| 14 +++--- > src/libxl/libxl_migration.c | 14 +- > 2 files changed, 12 insertions(+), 16 deletions(-) Reviewed-by: Cole Robinson - Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 0/3] use virStringParseYesNo helper
On 10/16/19 11:19 PM, Mao Zhongyi wrote: > 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(-) > Reviewed-by: Cole Robinson And pushed Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/8] conf: domaincaps: Add 'iothreads' to the features section
On Thu, Nov 14, 2019 at 12:59:00 +0100, Peter Krempa wrote: > On Thu, Nov 14, 2019 at 12:55:40 +0100, Ján Tomko wrote: > > On Wed, Nov 13, 2019 at 05:05:22PM +0100, Peter Krempa wrote: > > > Historically iothreads were the first feature and thus didn't have it's > > > own section. Add them to for consistency with other features. > > > > > > > I'm not convinced duplicating this is a good idea - apps looking into > > the element for iothread support would only find it with > > libvirt new enough to have it there and miss out on iothread support > > with older libvirt. > > I certainly can keep 'iothreads' separate. I'm definitely not going to > add an special case into the formatter to avoid formatting it there. > > Also to be fair I'm not sure whether it's worth caring about the > outlined scenario. If an APP looks for iothreads in it should > not be used with older libvirt than the one where it was added. > > At any rate, I can drop this patch and keep the iothreads stuff > separate. Also any new app will have to write two lookup XPaths to get all the data as it's now rather than having everything in one place. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 4/5] conf: report errors when parsing video resolution
On 11/13/19 5:41 PM, Jonathon Jongsma wrote:
> On Wed, 2019-11-13 at 13:54 -0500, Cole Robinson wrote:
>> I pushed the first three patches. Comments below
>>
>> On 10/23/19 1:46 PM, Jonathon Jongsma wrote:
>>> The current code doesn't properly handle errors when parsing a
>>> video
>>> device's resolution.
>>>
>>> This patch changes the parse function signature to return an error
>>> when we're missing an 'x' or 'y' parameter or when the 'x' or 'y'
>>> parameters are not positive integers. No error is returned when no
>>> 'resolution' element is found.
>>>
>>> Previously we were returning a NULL structure for the case where
>>> 'x' or
>>> 'y' were missing, but were returning an under-specified structure
>>> for
>>> the other error cases.
>>>
>>> Signed-off-by: Jonathon Jongsma
>>> ---
>>> src/conf/domain_conf.c | 44
>>> --
>>> 1 file changed, 30 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index 269a6ec2c3..b3f32d0b63 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -15338,45 +15338,57 @@ virDomainVideoAccelDefParseXML(xmlNodePtr
>>> node)
>>> return g_steal_pointer(&def);
>>> }
>>>
>>> -static virDomainVideoResolutionDefPtr
>>> -virDomainVideoResolutionDefParseXML(xmlNodePtr node)
>>> +static int
>>> +virDomainVideoResolutionDefParseXML(xmlNodePtr node,
>>> +virDomainVideoResolutionDefPtr
>>> *res)
>>> {
>>> xmlNodePtr cur;
>>> g_autofree virDomainVideoResolutionDefPtr def = NULL;
>>> g_autofree char *x = NULL;
>>> g_autofree char *y = NULL;
>>>
>>> +*res = 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");
>>> -}
>>> +if ((cur->type == XML_ELEMENT_NODE) &&
>>> +virXMLNodeNameEqual(cur, "resolution")) {
>>> +x = virXMLPropString(cur, "x");
>>> +y = virXMLPropString(cur, "y");
>>> +break;
>>> }
>>> cur = cur->next;
>>> }
>>>
>>
>> This loop can be removed if you move the virXMLNodeNameEqual to the
>> parent function, like how it is handled for parent call of
>> virDomainVirtioOptionsParseXML
>>
>>> +/* resolution not specified */
>>> +if (cur == NULL)
>>> +return 0;
>>> +
>>> +/* resolution specified, but x or y is missing. report error
>>> */
>>> if (!x || !y)
>>> -return NULL;
>>> +return -1;
>>>
>>> def = g_new0(virDomainVideoResolutionDef, 1);
>>>
>>> if (virStrToLong_uip(x, NULL, 10, &def->x) < 0) {
>>> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>>> _("cannot parse video x-resolution '%s'"),
>>> x);
>>> -goto cleanup;
>>> +return -1;
>>> }
>>>
>>> if (virStrToLong_uip(y, NULL, 10, &def->y) < 0) {
>>> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>>> _("cannot parse video y-resolution '%s'"),
>>> y);
>>> -goto cleanup;
>>> +return -1;
>>> }
>>>
>>> - cleanup:
>>> -return g_steal_pointer(&def);
>>> +if (def->x == 0 || def->y == 0) {
>>> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> + _("resolution values must be greater than
>>> 0"));
>>> +return -1;
>>> +}
>>> +
>>> +*res = g_steal_pointer(&def);
>>> +return 0;
>>> }
>>>
>>
>> This patch is doing two things: converting to the more common error
>> handling pattern, but also adding this error check. Separating them
>> will
>> help review.
>>
>> It's borderline pedantic but this type of error should actually be in
>> the Validate callback machinery instead. Some drivers will fill in a
>> virDomainDef manually in code (like virtualbox). If they accidentally
>> set an x or y value of 0, Validate won't catch it as an error, but
>> the
>> XML parser will catch it later. Generally the XML parser should only
>> throw errors about
>
> It seems that this sentence is unfinished?
>
I meant to delete that unformed thought because it was going to turn
into more than a sentence and would distract from this patch :)
Generally the XML parser should only raise errors that are specific to
the XML input. Anything that's validating a value after it has already
been parsed is better served in the Validate callbacks. The existing
code is not the best guide for this though, it's just the goal we should
shoot for IMO
- Cole
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 07/12] storage: use GRegex virStorageBackendLogicalParseVolExtents
On Thu, Nov 14, 2019 at 08:11:46AM +0100, Peter Krempa wrote: On Wed, Nov 13, 2019 at 16:48:48 +0100, Ján Tomko wrote: Using GRegex simplifies the code since g_match_info_fetch will copy the matched substring for us. Signed-off-by: Ján Tomko --- src/storage/storage_backend_logical.c | 49 +++ 1 file changed, 13 insertions(+), 36 deletions(-) I'm getting a build failure with this patch: ../../src/storage/storage_backend_logical.c:128:11: error: variable 'p' set but not used [-Werror=unused-but-set-variable] 128 | char *p = NULL; Oops, another case where GCC reports a slightly different set of warnings than CLang. Consider the following squashed in. Jano diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index d6c81e25b1..42dec05ba0 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -125,7 +125,6 @@ virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol, g_autoptr(GMatchInfo) info = NULL; int nextents, ret = -1; const char *regex_unit = "(\\S+)\\((\\S+)\\)"; -char *p = NULL; size_t i; unsigned long long offset, size, length; virStorageVolSourceExtent extent; @@ -186,8 +185,6 @@ virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol, goto cleanup; } -p = groups[3]; - /* Each extent has a "path:offset" pair, and match #0 * is the whole matched string. */ signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] util: file: Simplify logic of shortuct cases in virFindFileInPath
On Thu, Nov 14, 2019 at 12:51:57 +0100, Ján Tomko wrote:
> On Thu, Nov 14, 2019 at 11:43:31AM +0100, Peter Krempa wrote:
> > Combine the terms in the two sets of nested conditionals an don't reuse
> > a variable for a more obvious control flow.
> >
> > Signed-off-by: Peter Krempa
> > ---
> > src/util/virfile.c | 16 ++--
> > 1 file changed, 6 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/util/virfile.c b/src/util/virfile.c
> > index 072a299b39..c077639c1c 100644
> > --- a/src/util/virfile.c
> > +++ b/src/util/virfile.c
> > @@ -1644,20 +1644,16 @@ virFindFileInPath(const char *file)
> > /* if we are passed an absolute path (starting with /), return a
> > * copy of that path, after validating that it is executable
> > */
> > -if (IS_ABSOLUTE_FILE_NAME(file)) {
> > -char *ret = NULL;
> > -if (virFileIsExecutable(file))
> > -ret = g_strdup(file);
> > -return ret;
> > -}
> > +if (IS_ABSOLUTE_FILE_NAME(file) && virFileIsExecutable(file))
> > +return g_strdup(file);
> >
> > /* If we are passed an anchored path (containing a /), then there
> > * is no path search - it must exist in the current directory
> > */
> > -if (strchr(file, '/')) {
> > -if (virFileIsExecutable(file))
> > -ignore_value(virFileAbsPath(file, &path));
> > -return path;
> > +if (strchr(file, '/') && virFileIsExecutable(file)) {
> > +char *abspath = NULL;
> > +ignore_value(virFileAbsPath(file, &abspath));
> > +return abspath;
>
> Before, if either the filename was absolute, or it contained a slash,
> NULL would be returned right away if the file was not executable.
>
> After this patch, those cases will fall through to the iteration over
> $PATH.
Hmm, right. I'll send a v2.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 8/8] qemu: domcaps: Simplify adding new domaincaps based on qemu caps
On Wed, Nov 13, 2019 at 05:05:26PM +0100, Peter Krempa wrote:
Add a helper which converts qemu emulator capabilities to the domain
capability XML. This will simplify future additions of new features.
Signed-off-by: Peter Krempa
---
src/qemu/qemu_capabilities.c | 39 ++--
1 file changed, 28 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index a1fbf0da34..483c3fcf0f 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -5281,12 +5281,35 @@ virQEMUCapsFillDomainCPUCaps(virCapsPtr caps,
}
+struct virQEMUCapsDomainFeatureCapabilityTuple {
+virDomainCapsFeature domcap;
+virQEMUCapsFlags qemucap;
+};
+
+/**
+ * This maps the qemu features to the entries in of the domain
+ * capability XML. Use QEMU_CAPS_LAST as qemucap to always enable the feature.
+ */
+static const struct virQEMUCapsDomainFeatureCapabilityTuple domCapsTuples[] = {
+{ VIR_DOMAIN_CAPS_FEATURE_IOTHREADS, QEMU_CAPS_OBJECT_IOTHREAD },
+{ VIR_DOMAIN_CAPS_FEATURE_VMCOREINFO, QEMU_CAPS_DEVICE_VMCOREINFO },
+{ VIR_DOMAIN_CAPS_FEATURE_GENID, QEMU_CAPS_DEVICE_VMGENID },
+};
+
+
static void
-virQEMUCapsFillDomainIOThreadCaps(virQEMUCapsPtr qemuCaps,
- virDomainCapsPtr domCaps)
+virQEMUCapsFillDomainFeaturesFromQEMUCaps(virQEMUCapsPtr qemuCaps,
+ virDomainCapsPtr domCaps)
{
-domCaps->features[VIR_DOMAIN_CAPS_FEATURE_IOTHREADS] =
virTristateBoolFromBool(
-virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD));
+size_t i;
+
+virDomainCapsFeaturesInitUnsupported(domCaps);
+
+for (i = 0; i < G_N_ELEMENTS(domCapsTuples); i++) {
+if (domCapsTuples[i].qemucap == QEMU_CAPS_LAST ||
Dropping this special value and using a separate array like
domCapsAlwaysOn would be more readable. Especially since you don't
mandate an entry for each existing DOMAIN_CAPS_FEATURE
+virQEMUCapsGet(qemuCaps, domCapsTuples[i].qemucap))
+domCaps->features[domCapsTuples[i].domcap] = VIR_TRISTATE_BOOL_YES;
+}
}
@@ -5572,6 +5595,7 @@ virQEMUCapsFillDomainCaps(virCapsPtr caps,
virDomainCapsDeviceRNGPtr rng = &domCaps->rng;
virDomainCapsFeaturesInitUnsupported(domCaps);
+virQEMUCapsFillDomainFeaturesFromQEMUCaps(qemuCaps, domCaps);
domCaps->maxvcpus = virQEMUCapsGetMachineMaxCpus(qemuCaps,
domCaps->machine);
@@ -5584,12 +5608,6 @@ virQEMUCapsFillDomainCaps(virCapsPtr caps,
domCaps->maxvcpus = MIN(domCaps->maxvcpus, hostmaxvcpus);
}
-domCaps->features[VIR_DOMAIN_CAPS_FEATURE_VMCOREINFO] =
virTristateBoolFromBool(
-virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VMCOREINFO));
-
-domCaps->features[VIR_DOMAIN_CAPS_FEATURE_GENID] = virTristateBoolFromBool(
-virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VMGENID));
-
if (virQEMUCapsFillDomainOSCaps(os,
domCaps->machine,
domCaps->arch,
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 7/8] domaincaps: Store domain capability features in an array
On Wed, Nov 13, 2019 at 05:05:25PM +0100, Peter Krempa wrote:
Declare the capabilities as enum values and store them in an array. This
makes adding new features more straightforward and simplifies the
formatter which now doesn't require changing.
Signed-off-by: Peter Krempa
---
src/conf/domain_capabilities.c | 30 +++---
src/conf/domain_capabilities.h | 13 ++---
src/qemu/qemu_capabilities.c | 6 +++---
3 files changed, 36 insertions(+), 13 deletions(-)
diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
index 39acad00f1..1993a22cc5 100644
--- a/src/conf/domain_capabilities.c
+++ b/src/conf/domain_capabilities.c
@@ -33,6 +33,15 @@ VIR_ENUM_IMPL(virDomainCapsCPUUsable,
"unknown", "yes", "no",
);
+
+VIR_ENUM_DECL(virDomainCapsFeature);
+VIR_ENUM_IMPL(virDomainCapsFeature,
+ VIR_DOMAIN_CAPS_FEATURE_LAST,
+ "iothreads",
+ "vmcoreinfo",
+ "genid",
+);
+
static virClassPtr virDomainCapsClass;
static virClassPtr virDomainCapsCPUModelsClass;
@@ -324,9 +333,10 @@ virDomainCapsEnumClear(virDomainCapsEnumPtr capsEnum)
void
virDomainCapsFeaturesInitUnsupported(virDomainCapsPtr caps)
{
-caps->iothreads = VIR_TRISTATE_BOOL_NO;
-caps->vmcoreinfo = VIR_TRISTATE_BOOL_NO;
-caps->genid = VIR_TRISTATE_BOOL_NO;
+size_t i;
+
+for (i = 0; i < VIR_DOMAIN_CAPS_FEATURE_LAST; i++)
+caps->features[i] = VIR_TRISTATE_BOOL_NO;
}
@@ -619,11 +629,16 @@ virDomainCapsFormatFeatures(const virDomainCaps *caps,
virBufferPtr buf)
{
g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf);
+size_t i;
virDomainCapsFeatureGICFormat(&childBuf, &caps->gic);
-qemuDomainCapsFeatureFormatSimple(&childBuf, "iothreads", caps->iothreads);
-qemuDomainCapsFeatureFormatSimple(&childBuf, "vmcoreinfo",
caps->vmcoreinfo);
-qemuDomainCapsFeatureFormatSimple(&childBuf, "genid", caps->genid);
+
+for (i = 0; i < VIR_DOMAIN_CAPS_FEATURE_LAST; i++) {
if (i == VIR_DOMAIN_CAPS_FEATURE_IOTHREADS)
continue;
or leave the iothreads feature stored separately (unless it's really
okay to output the feature in two places)
+qemuDomainCapsFeatureFormatSimple(&childBuf,
+ virDomainCapsFeatureTypeToString(i),
+ caps->features[i]);
+}
+
virDomainCapsFeatureSEVFormat(&childBuf, caps->sev);
virXMLFormatElement(buf, "features", NULL, &childBuf);
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 4/8] conf: domaincaps: Add 'iothreads' to the features section
On Thu, Nov 14, 2019 at 12:55:40 +0100, Ján Tomko wrote: > On Wed, Nov 13, 2019 at 05:05:22PM +0100, Peter Krempa wrote: > > Historically iothreads were the first feature and thus didn't have it's > > own section. Add them to for consistency with other features. > > > > I'm not convinced duplicating this is a good idea - apps looking into > the element for iothread support would only find it with > libvirt new enough to have it there and miss out on iothread support > with older libvirt. I certainly can keep 'iothreads' separate. I'm definitely not going to add an special case into the formatter to avoid formatting it there. Also to be fair I'm not sure whether it's worth caring about the outlined scenario. If an APP looks for iothreads in it should not be used with older libvirt than the one where it was added. At any rate, I can drop this patch and keep the iothreads stuff separate. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 6/8] qemu: domcaps: Initialize all features
On Wed, Nov 13, 2019 at 05:05:24PM +0100, Peter Krempa wrote: While the qemu driver currently implements all domain capability features, we should initialize all features using the helper similarly to how we do it in drivers which don't support any. Signed-off-by: Peter Krempa --- src/qemu/qemu_capabilities.c | 2 ++ 1 file changed, 2 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 5/8] domcaps: Add function for initializing domain caps as unsupported
On Wed, Nov 13, 2019 at 05:05:23PM +0100, Peter Krempa wrote: For future extensions of the domain caps it's useful to have a single point that initializes all capabilities as unsupported by a driver. The driver then can enable specific ones. Signed-off-by: Peter Krempa --- src/bhyve/bhyve_capabilities.c | 4 +--- src/conf/domain_capabilities.c | 14 ++ src/conf/domain_capabilities.h | 2 ++ src/libvirt_private.syms | 1 + src/libxl/libxl_capabilities.c | 5 ++--- 5 files changed, 20 insertions(+), 6 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 3/8] conf: domaincaps: Use virXMLFormatElement in virDomainCapsFormatFeatures
On Wed, Nov 13, 2019 at 05:05:21PM +0100, Peter Krempa wrote: Signed-off-by: Peter Krempa --- src/conf/domain_capabilities.c | 14 ++ 1 file changed, 6 insertions(+), 8 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 2/8] conf: domaincaps: Extract formatting of the subelement
On Wed, Nov 13, 2019 at 05:05:20PM +0100, Peter Krempa wrote: Extract it to virDomainCapsFormatFeatures so that the main function does not get so bloated over time. Signed-off-by: Peter Krempa --- src/conf/domain_capabilities.c | 28 ++-- 1 file changed, 18 insertions(+), 10 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 1/8] conf: domaincaps: Replace FORMAT_SINGLE macro by a function
On Wed, Nov 13, 2019 at 05:05:19PM +0100, Peter Krempa wrote: Introduce qemuDomainCapsFeatureFormatSimple which does exactly the same thing but it's a function. Signed-off-by: Peter Krempa --- src/conf/domain_capabilities.c | 27 --- 1 file changed, 16 insertions(+), 11 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 4/8] conf: domaincaps: Add 'iothreads' to the features section
On Wed, Nov 13, 2019 at 05:05:22PM +0100, Peter Krempa wrote: Historically iothreads were the first feature and thus didn't have it's own section. Add them to for consistency with other features. I'm not convinced duplicating this is a good idea - apps looking into the element for iothread support would only find it with libvirt new enough to have it there and miss out on iothread support with older libvirt. Jano Unfortunately we must keep the original one in place. Signed-off-by: Peter Krempa --- docs/schemas/domaincaps.rng | 3 +++ src/conf/domain_capabilities.c| 1 + tests/domaincapsdata/libxl-xenfv.xml | 1 + tests/domaincapsdata/libxl-xenpv.xml | 1 + signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] gnulib: Remove use of 'strsep' module
On Thu, Nov 14, 2019 at 11:43:33AM +0100, Peter Krempa wrote: We don't use strsep any more. Signed-off-by: Peter Krempa --- bootstrap.conf | 1 - 1 file changed, 1 deletion(-) 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 4/5] util: file: Replace use of 'strsep' with virStringSplit
On Thu, Nov 14, 2019 at 11:43:32AM +0100, Peter Krempa wrote: Use our helper instead of the gnulib one. Signed-off-by: Peter Krempa --- src/util/virfile.c | 15 --- 1 file changed, 8 insertions(+), 7 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 3/5] util: file: Simplify logic of shortuct cases in virFindFileInPath
On Thu, Nov 14, 2019 at 11:43:31AM +0100, Peter Krempa wrote:
Combine the terms in the two sets of nested conditionals an don't reuse
a variable for a more obvious control flow.
Signed-off-by: Peter Krempa
---
src/util/virfile.c | 16 ++--
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 072a299b39..c077639c1c 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -1644,20 +1644,16 @@ virFindFileInPath(const char *file)
/* if we are passed an absolute path (starting with /), return a
* copy of that path, after validating that it is executable
*/
-if (IS_ABSOLUTE_FILE_NAME(file)) {
-char *ret = NULL;
-if (virFileIsExecutable(file))
-ret = g_strdup(file);
-return ret;
-}
+if (IS_ABSOLUTE_FILE_NAME(file) && virFileIsExecutable(file))
+return g_strdup(file);
/* If we are passed an anchored path (containing a /), then there
* is no path search - it must exist in the current directory
*/
-if (strchr(file, '/')) {
-if (virFileIsExecutable(file))
-ignore_value(virFileAbsPath(file, &path));
-return path;
+if (strchr(file, '/') && virFileIsExecutable(file)) {
+char *abspath = NULL;
+ignore_value(virFileAbsPath(file, &abspath));
+return abspath;
Before, if either the filename was absolute, or it contained a slash,
NULL would be returned right away if the file was not executable.
After this patch, those cases will fall through to the iteration over
$PATH.
Jano
}
/* copy PATH env so we can tweak it */
--
2.23.0
--
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 2/5] util: file: Use g_autofree in virFindFileInPath
On Thu, Nov 14, 2019 at 11:43:30AM +0100, Peter Krempa wrote: Simplify the final lookup loop by freeing memory automatically and thus being able to directly return the result. Signed-off-by: Peter Krempa --- src/util/virfile.c | 11 --- 1 file changed, 4 insertions(+), 7 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 1/5] rpc: use virStringSplit instead of strsep
On Thu, Nov 14, 2019 at 11:43:29AM +0100, Peter Krempa wrote: When parsing allowed authentication methods for the native ssh lib trasnports we used strsep. Since we have virStringSplit helper let's use that one. Signed-off-by: Peter Krempa --- src/rpc/virnetsocket.c | 28 1 file changed, 12 insertions(+), 16 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 0/8] Remove use of 'areadlink' gnulib module (kill-a-gnulib-module-a-day initiative)
On Wed, Nov 13, 2019 at 02:07:01PM +0100, Peter Krempa wrote: This removes use of 'areadlink' by using 'g_file_read_link' and fixes a few insane uses. Note that I'll post the actual module deletion from bootstrap.conf separately as touching that file results in lengthy rebuilds. Peter Krempa (8): qemu: domain: Use g_file_read_link instead of virFileReadLink util: file: Remove virFileReadLink qemu: tpm: Use g_autofree in qemuTPMEmulatorGetPid qemu: tpm: Sanitize error values in qemuTPMEmulatorGetPid qemu: gpu: Sanitize error values in qemuVhostUserGPUGetPid util: pidfile: Sanitize return values of virPidFileReadIfAlive util: pidfile: Sanitize return values of virPidFileReadPathIfAlive util: pidfile: Replace 'areadlink' by 'g_file_read_link' src/libvirt_private.syms | 1 - src/qemu/qemu_domain.c | 18 +- src/qemu/qemu_tpm.c| 16 - src/qemu/qemu_vhost_user_gpu.c | 12 --- src/util/virfile.c | 13 --- src/util/virfile.h | 3 -- src/util/virpidfile.c | 64 +- 7 files changed, 56 insertions(+), 71 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 5/5] gnulib: Remove use of 'strsep' module
On Thu, Nov 14, 2019 at 11:43:33AM +0100, Peter Krempa wrote: > We don't use strsep any more. > > Signed-off-by: Peter Krempa > --- > bootstrap.conf | 1 - > 1 file changed, 1 deletion(-) We should probably introduce syntax-check rule to forbid strsep. Reviewed-by: Pavel Hrdina signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/5] util: file: Replace use of 'strsep' with virStringSplit
On Thu, Nov 14, 2019 at 11:43:32AM +0100, Peter Krempa wrote: > Use our helper instead of the gnulib one. > > Signed-off-by: Peter Krempa > --- > src/util/virfile.c | 15 --- > 1 file changed, 8 insertions(+), 7 deletions(-) Reviewed-by: Pavel Hrdina signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/5] util: file: Simplify logic of shortuct cases in virFindFileInPath
On Thu, Nov 14, 2019 at 11:43:31AM +0100, Peter Krempa wrote: > Combine the terms in the two sets of nested conditionals an don't reuse > a variable for a more obvious control flow. > > Signed-off-by: Peter Krempa > --- > src/util/virfile.c | 16 ++-- > 1 file changed, 6 insertions(+), 10 deletions(-) Reviewed-by: Pavel Hrdina signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/2] qemu: block: Fix two bugs related to usage of blockdev (blockdev-add saga)
On Wed, Nov 13, 2019 at 03:22:10PM +0100, Peter Krempa wrote: Peter Krempa (2): qemu: blockjob: Transfer 'readonly' state of images after active layer block commit qemu: snapshot: Fix inactive external snapshots when backing chain is present src/qemu/qemu_blockjob.c | 2 ++ src/qemu/qemu_driver.c | 25 +++-- 2 files changed, 21 insertions(+), 6 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 2/5] util: file: Use g_autofree in virFindFileInPath
On Thu, Nov 14, 2019 at 11:43:30AM +0100, Peter Krempa wrote: > Simplify the final lookup loop by freeing memory automatically and thus > being able to directly return the result. > > Signed-off-by: Peter Krempa > --- > src/util/virfile.c | 11 --- > 1 file changed, 4 insertions(+), 7 deletions(-) Reviewed-by: Pavel Hrdina signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 0/4] apparmor fixes triggered by multi disk snapshots
On Thu, Nov 14, 2019 at 1:23 AM Cole Robinson wrote: > > On 10/16/19 10:27 AM, Christian Ehrhardt wrote: > > 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(-) > > > > Not runtime tested, but: > > Reviewed-by: Cole Robinson Thank you, I added the tag in my local series, but that is not worth a v2 submission. Before pushing I'm still waiting for someone with apparmor experience to take a look, just to be somewhat on the safe side. > - Cole > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/5] rpc: use virStringSplit instead of strsep
On Thu, Nov 14, 2019 at 11:43:29AM +0100, Peter Krempa wrote: > When parsing allowed authentication methods for the native ssh lib > trasnports we used strsep. Since we have virStringSplit helper let's use s/trasnports/transports/ > that one. > > Signed-off-by: Peter Krempa > --- > src/rpc/virnetsocket.c | 28 > 1 file changed, 12 insertions(+), 16 deletions(-) Reviewed-by: Pavel Hrdina signature.asc Description: PGP signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 3/3] virt-aa-helper: drop pointer checks in get_files
It was mentioned that the pointers in loops like:
for (i = 0; i < ctl->def->nserials; i++)
if (ctl->def->serials[i] ...
will always be !NULL or we would have a much more serious problem.
Simplify the if chains in get_files by dropping these checks.
Signed-off-by: Christian Ehrhardt
---
src/security/virt-aa-helper.c | 135 --
1 file changed, 63 insertions(+), 72 deletions(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index c6c4bb9bd0..17f49a6259 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -965,8 +965,7 @@ get_files(vahControl * ctl)
}
for (i = 0; i < ctl->def->nserials; i++)
-if (ctl->def->serials[i] &&
-(ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
+if ((ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV ||
ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE ||
ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX ||
@@ -980,8 +979,7 @@ get_files(vahControl * ctl)
goto cleanup;
for (i = 0; i < ctl->def->nconsoles; i++)
-if (ctl->def->consoles[i] &&
-(ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
+if ((ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV ||
ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE ||
ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX ||
@@ -993,8 +991,7 @@ get_files(vahControl * ctl)
goto cleanup;
for (i = 0; i < ctl->def->nparallels; i++)
-if (ctl->def->parallels[i] &&
-(ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
+if ((ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV ||
ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE
||
ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX
||
@@ -1008,8 +1005,7 @@ get_files(vahControl * ctl)
goto cleanup;
for (i = 0; i < ctl->def->nchannels; i++)
-if (ctl->def->channels[i] &&
-(ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
+if ((ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV ||
ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE ||
ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX ||
@@ -1082,76 +1078,74 @@ get_files(vahControl * ctl)
"r") != 0)
goto cleanup;
-for (i = 0; i < ctl->def->nhostdevs; i++)
-if (ctl->def->hostdevs[i]) {
-virDomainHostdevDefPtr dev = ctl->def->hostdevs[i];
-virDomainHostdevSubsysUSBPtr usbsrc = &dev->source.subsys.u.usb;
-switch (dev->source.subsys.type) {
-case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: {
-virUSBDevicePtr usb =
-virUSBDeviceNew(usbsrc->bus, usbsrc->device, NULL);
+for (i = 0; i < ctl->def->nhostdevs; i++) {
+virDomainHostdevDefPtr dev = ctl->def->hostdevs[i];
+virDomainHostdevSubsysUSBPtr usbsrc = &dev->source.subsys.u.usb;
+switch (dev->source.subsys.type) {
+case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: {
+virUSBDevicePtr usb =
+virUSBDeviceNew(usbsrc->bus, usbsrc->device, NULL);
-if (usb == NULL)
-continue;
-
-if (virHostdevFindUSBDevice(dev, true, &usb) < 0)
-continue;
+if (usb == NULL)
+continue;
-rc = virUSBDeviceFileIterate(usb, file_iterate_hostdev_cb,
&buf);
-virUSBDeviceFree(usb);
-if (rc != 0)
-goto cleanup;
-break;
-}
+if (virHostdevFindUSBDevice(dev, true, &usb) < 0)
+continue;
-case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: {
-virDomainHostdevSubsysMediatedDevPtr mdevsrc =
&dev->source.subsys.u.mdev;
-switch ((virMediatedDeviceModelType) mdevsrc->model) {
-case VIR_MDEV_MODEL_TYPE_VFIO_PCI:
-case VIR_MDEV_MODEL_TYPE_VFIO_AP:
-case VIR_MDEV_MODEL_TYPE_VFIO_CCW:
-needsVfio = true;
-break;
-case VIR_MDEV_MODEL_TYPE_LAST:
-default:
-virReportEnumRangeError(virMediatedDeviceModelType,
-
[libvirt] [PATCH v2 1/3] virt-aa-helper: add rules for shmem devices
Shared memory devices need qemu to be able to access certain paths
either for the shared memory directly (mostly ivshmem-plain) or for a
socket (mostly ivshmem-doorbell).
Add logic to virt-aa-helper to render those apparmor rules based
on the domain configuration.
https://bugzilla.redhat.com/show_bug.cgi?id=1761645
Reviewed-by: Cole Robinson
Signed-off-by: Christian Ehrhardt
---
src/security/virt-aa-helper.c | 31 +++
1 file changed, 31 insertions(+)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 5ac9a9eeb8..c6c4bb9bd0 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -929,6 +929,7 @@ get_files(vahControl * ctl)
int rc = -1;
size_t i;
char *uuid;
+char *mem_path = NULL;
char uuidstr[VIR_UUID_STRING_BUFLEN];
bool needsVfio = false, needsvhost = false, needsgl = false;
@@ -1192,6 +1193,35 @@ get_files(vahControl * ctl)
}
}
+for (i = 0; i < ctl->def->nshmems; i++) {
+virDomainShmemDef *shmem = ctl->def->shmems[i];
+/* server path can be on any type and overwrites defaults */
+if (shmem->server.enabled &&
+shmem->server.chr.data.nix.path) {
+if (vah_add_file(&buf, shmem->server.chr.data.nix.path,
+"rw") != 0)
+goto cleanup;
+} else {
+switch (shmem->model) {
+case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN:
+/* until exposed, recreate qemuBuildShmemBackendMemProps */
+mem_path = g_strdup_printf("/dev/shm/%s", shmem->name);
+break;
+case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL:
+case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM:
+ /* until exposed, recreate qemuDomainPrepareShmemChardev */
+mem_path = g_strdup_printf("/var/lib/libvirt/shmem-%s-sock",
+ shmem->name);
+break;
+}
+if (mem_path != NULL) {
+if (vah_add_file(&buf, mem_path, "rw") != 0)
+goto cleanup;
+}
+}
+}
+
+
if (ctl->def->tpm) {
char *shortName = NULL;
const char *tpmpath = NULL;
@@ -1286,6 +1316,7 @@ get_files(vahControl * ctl)
ctl->files = virBufferContentAndReset(&buf);
cleanup:
+VIR_FREE(mem_path);
VIR_FREE(uuid);
return rc;
}
--
2.24.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 2/3] virt-aa-helper: testcase for shmem devices
Adding build time self tests for basic (deprecated), doorbell and plain mode. Reviewed-by: Cole Robinson Signed-off-by: Christian Ehrhardt --- tests/virt-aa-helper-test | 15 +++ 1 file changed, 15 insertions(+) diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test index 6e674bfe5c..6a6703ecf5 100755 --- a/tests/virt-aa-helper-test +++ b/tests/virt-aa-helper-test @@ -384,6 +384,21 @@ testme "0" "dri egl" "-r -u $valid_uuid" "$test_xml" "/dev/dri/testegl1.*rw,$" sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,,,g" "$template_xml" > "$test_xml" testme "0" "dri spice" "-r -u $valid_uuid" "$test_xml" "/dev/dri/testegl2.*rw,$" +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,,4,g" "$template_xml" > "$test_xml" +testme "0" "shmem" "-r -u $valid_uuid" "$test_xml" "\"/var/lib/libvirt/shmem-my_shmem0-sock\"\s*rw,$" + +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,,4,g" "$template_xml" > "$test_xml" +testme "0" "shmem serverpath" "-r -u $valid_uuid" "$test_xml" "\"/var/lib/libvirt/ivshmem_socket\"\s*rw,$" + +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,,4,g" "$template_xml" > "$test_xml" +testme "0" "shmem plain" "-r -u $valid_uuid" "$test_xml" "\"/dev/shm/my_shmem0\"\s*rw,$" + +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,,,g" "$template_xml" > "$test_xml" +testme "0" "shmem doorbell" "-r -u $valid_uuid" "$test_xml" "\"/var/lib/libvirt/shmem-shmem_server-sock\"\s*rw,$" + +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,,,g" "$template_xml" > "$test_xml" +testme "0" "shmem doorbell serverpath" "-r -u $valid_uuid" "$test_xml" "\"/var/lib/libvirt/ivshmem_socket\"\s*rw,$" + testme "0" "help" "-h" echo "" >$output -- 2.24.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2 0/3] virt-aa-helper for shmem device
Cole was recently adding a few of the usual apparmor suspects to BZ 1761645 and I was taking a look at the low hanging fruits of it today. It isn't perfect, but would resolve the reported issue - so I'd appreciate a review. Limitations: - One could break the path creating elements in qemuBuildShmemBackendMemProps and qemuDomainPrepareShmemChardev into extra functions and then use those from virt-aa-helper. But I haven't done so yet and unless it is strictly required consider it too much for what we want/need to achieve here. - I haven't covered hotplug of shmem devices yet, it seems there is no infrastructure for their labels yet and I wasn't sure how important shmem-hotplug would even be to anyone. Updates in v2: - rebase latest master - drop checking shmems[i] - switch to use g_strdup_printf - added an extra patch removing checks like ctl->def->mems[i] - add reviewed-by tag Christian Ehrhardt (3): virt-aa-helper: add rules for shmem devices virt-aa-helper: testcase for shmem devices virt-aa-helper: drop pointer checks in get_files src/security/virt-aa-helper.c | 166 +++--- tests/virt-aa-helper-test | 15 +++ 2 files changed, 109 insertions(+), 72 deletions(-) -- 2.24.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/2] virt-aa-helper: add rules for shmem devices
On Thu, Nov 14, 2019 at 1:14 AM Cole Robinson wrote:
>
> On 10/22/19 8:18 AM, Christian Ehrhardt wrote:
> > Shared memory devices need qemu to be able to access certain paths
> > either for the shared memory directly (mostly ivshmem-plain) or for a
> > socket (mostly ivshmem-doorbell).
> >
> > Add logic to virt-aa-helper to render those apparmor rules based
> > on the domain configuration.
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=1761645
> >
> > Signed-off-by: Christian Ehrhardt
> > ---
> > src/security/virt-aa-helper.c | 35 +++
> > 1 file changed, 35 insertions(+)
> >
> > diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> > index 7d7262ca39..8c261f0010 100644
> > --- a/src/security/virt-aa-helper.c
> > +++ b/src/security/virt-aa-helper.c
> > @@ -958,6 +958,7 @@ get_files(vahControl * ctl)
> > int rc = -1;
> > size_t i;
> > char *uuid;
> > +char *mem_path = NULL;
> > char uuidstr[VIR_UUID_STRING_BUFLEN];
> > bool needsVfio = false, needsvhost = false, needsgl = false;
> >
> > @@ -1224,6 +1225,39 @@ get_files(vahControl * ctl)
> > }
> > }
> >
> > +for (i = 0; i < ctl->def->nshmems; i++) {
> > +if (ctl->def->shmems[i]) {
>
> shmems[i] should never be NULL here except in the case of a serious and
> obvious bug elsewhere in libvirt. So this should be removed IMO. Same
> goes for all other device list handling in this function, but that's a
> separate issue.
Hi Cole and thanks for the review!
I continued the pattern which existed for ages, but I agree and thank
you for pointing this one out!
I have reworked it and will append a patch that generally removes
those as I agree these days this should be safe since this is no interim
structure but a freshly parsed definition.
OTOH you could say that is defensive programming, we will see if
there is different feedback on the explicit patch about it.
> Also style comment, IMO in cases where this type of pattern _is_
> relevant, it's nicer to do
>
> if (!condition)
> continue;
>
> Which saves a lot of indenting
>
> > +virDomainShmemDef *shmem = ctl->def->shmems[i];
> > +/* server path can be on any type and overwrites defaults */
> > +if (shmem->server.enabled &&
> > +shmem->server.chr.data.nix.path) {
> > +if (vah_add_file(&buf, shmem->server.chr.data.nix.path,
> > +"rw") != 0)
> > +goto cleanup;
> > +} else {
> > +switch (shmem->model) {
> > +case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN:
> > +/* until exposed, recreate
> > qemuBuildShmemBackendMemProps */
> > +if (virAsprintf(&mem_path, "/dev/shm/%s", shmem->name)
> > < 0)
> > +goto cleanup;
>
> virAsprintf is gone in git, you can use g_strdup_printf, which also
> means dropping the error checking. See the other conversions patches
Yeah I knew this was coming, but wanted to wait for general reviews to
not proliferate this series with v1..v99 :-)
Done in v2
> With those changes, for the series:
>
> Reviewed-by: Cole Robinson
>
> > +break;
> > +case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL:
> > +case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM:
> > + /* until exposed, recreate
> > qemuDomainPrepareShmemChardev */
> > +if (virAsprintf(&mem_path,
> > "/var/lib/libvirt/shmem-%s-sock",
> > +shmem->name) < 0)
> > +goto cleanup;
> > +break;
> > +}
> > +if (mem_path != NULL) {
> > +if (vah_add_file(&buf, mem_path, "rw") != 0)
> > +goto cleanup;
> > +}
> > +}
> > +}
> > +}
> > +
> > +
> > if (ctl->def->tpm) {
> > char *shortName = NULL;
> > const char *tpmpath = NULL;
> > @@ -1324,6 +1358,7 @@ get_files(vahControl * ctl)
> > ctl->files = virBufferContentAndReset(&buf);
> >
> > cleanup:
> > +VIR_FREE(mem_path);
> > VIR_FREE(uuid);
> > return rc;
> > }
> >
>
>
> - Cole
>
--
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 5/5] gnulib: Remove use of 'strsep' module
We don't use strsep any more. Signed-off-by: Peter Krempa --- bootstrap.conf | 1 - 1 file changed, 1 deletion(-) diff --git a/bootstrap.conf b/bootstrap.conf index f13fcae11a..e519c69d30 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -75,7 +75,6 @@ socket stat-time strchrnul strptime -strsep strtok_r sys_stat sys_wait -- 2.23.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/5] util: file: Replace use of 'strsep' with virStringSplit
Use our helper instead of the gnulib one.
Signed-off-by: Peter Krempa
---
src/util/virfile.c | 15 ---
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index c077639c1c..55705c8463 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -1634,9 +1634,8 @@ char *
virFindFileInPath(const char *file)
{
const char *origpath = NULL;
-g_autofree char *path = NULL;
-char *pathiter;
-char *pathseg;
+VIR_AUTOSTRINGLIST paths = NULL;
+char **pathiter;
if (file == NULL)
return NULL;
@@ -1660,14 +1659,16 @@ virFindFileInPath(const char *file)
origpath = getenv("PATH");
if (!origpath)
origpath = "/bin:/usr/bin";
-path = g_strdup(origpath);
/* for each path segment, append the file to search for and test for
* it. return it if found.
*/
-pathiter = path;
-while ((pathseg = strsep(&pathiter, ":")) != NULL) {
-g_autofree char *fullpath = g_strdup_printf("%s/%s", pathseg, file);
+
+if (!(paths = virStringSplit(origpath, ":", 0)))
+return NULL;
+
+for (pathiter = paths; *pathiter; pathiter++) {
+g_autofree char *fullpath = g_strdup_printf("%s/%s", *pathiter, file);
if (virFileIsExecutable(fullpath))
return g_steal_pointer(&fullpath);
}
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/5] util: file: Use g_autofree in virFindFileInPath
Simplify the final lookup loop by freeing memory automatically and thus
being able to directly return the result.
Signed-off-by: Peter Krempa
---
src/util/virfile.c | 11 ---
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index b1aeaa5851..072a299b39 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -1634,10 +1634,9 @@ char *
virFindFileInPath(const char *file)
{
const char *origpath = NULL;
-char *path = NULL;
+g_autofree char *path = NULL;
char *pathiter;
char *pathseg;
-char *fullpath = NULL;
if (file == NULL)
return NULL;
@@ -1672,14 +1671,12 @@ virFindFileInPath(const char *file)
*/
pathiter = path;
while ((pathseg = strsep(&pathiter, ":")) != NULL) {
-fullpath = g_strdup_printf("%s/%s", pathseg, file);
+g_autofree char *fullpath = g_strdup_printf("%s/%s", pathseg, file);
if (virFileIsExecutable(fullpath))
-break;
-VIR_FREE(fullpath);
+return g_steal_pointer(&fullpath);
}
-VIR_FREE(path);
-return fullpath;
+return NULL;
}
--
2.23.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/5] PATCH 0/8] Remove use of 'strsep' gnulib module (kill-a-gnulib-module-a-day initiative)
Peter Krempa (5): rpc: use virStringSplit instead of strsep util: file: Use g_autofree in virFindFileInPath util: file: Simplify logic of shortuct cases in virFindFileInPath util: file: Replace use of 'strsep' with virStringSplit gnulib: Remove use of 'strsep' module bootstrap.conf | 1 - src/rpc/virnetsocket.c | 28 src/util/virfile.c | 38 -- 3 files changed, 28 insertions(+), 39 deletions(-) -- 2.23.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
