Re: [libvirt] Matching the type of mediated devices in the migration

[Alex Williamson]

On Thu, 23 Aug 2018 04:02:43 +
"Tian, Kevin"  wrote:

> > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > Sent: Thursday, August 23, 2018 11:47 AM
> > 
> > On Wed, 22 Aug 2018 02:30:12 +
> > "Tian, Kevin"  wrote:
> >   
> > > > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > > > Sent: Wednesday, August 22, 2018 10:08 AM
> > > >
> > > > On Wed, 22 Aug 2018 01:27:05 +
> > > > "Tian, Kevin"  wrote:
> > > >  
> > > > > > From: Wang, Zhi A
> > > > > > Sent: Wednesday, August 22, 2018 2:43 AM  
> > > > > > >
> > > > > > > Are there any suggestions how we can deal with security issues?
> > > > > > > Allowing userspace to provide a data stream representing the  
> > internal  
> > > > > > > state of a virtual device model living within the kernel seems
> > > > > > > troublesome.  If we need to trust the data stream, do we need to
> > > > > > > somehow make the operation more privileged than what a vfio  
> > user  
> > > > > > might  
> > > > > > > have otherwise?  Does the data stream need to be somehow  
> > signed  
> > > > and  
> > > > > > how  
> > > > > > > might we do that?  How can we build in protection against an  
> > > > untrusted  
> > > > > > > restore image?  Thanks,  
> > > > >
> > > > > imo it is not necessary. restoring mdev state should be handled as if
> > > > > guest is programming the mdev.  
> > > >
> > > > To me this suggests that a state save/restore is just an algorithm
> > > > executed by userspace using the existing vfio device accesses.  This is
> > > > not at all what we've been discussing for migration.  I believe the  
> > >
> > > not algorithm by userspace. It's kernel driver to apply the audit when
> > > receiving opaque state data.  
> > 
> > And a kernel driver receiving and processing opaque state date from a
> > user doesn't raise security concerns for you?  
> 
> opaque is from userspace p.o.v. kernel driver understands the actual
> format and thus can audit when restoring the state.

Which only means that we risk having untold security issues within each
separate mdev vendor driver.

> > > > interface we've been hashing out exposes opaque device state through  
> > a  
> > > > vfio region.  We therefore must assume that that opaque data contains
> > > > not only device state, but also emulation state, similar to what we see
> > > > for any QEMU device.  Not only is there internal emulation state, but
> > > > we have no guarantee that the device state goes through the same
> > > > auditing as it does through the vfio interface.  Since this device and
> > > > emulation state live inside the kernel and not just within the user's
> > > > own process, a malicious user can do far more than shoot themselves.  
> > It  
> > > > would be one thing devices were IOMMU isolated, but they're not,
> > > > they're isolated through vendor and device specific mechanism, and for
> > > > all we know the parameters of that isolation are included in the
> > > > restore state.  I don't see how we can say this is not an issue.  
> > >
> > > I didn't quite get this. My understanding is that isolation configuration
> > > is completed when a mdev is created on DEST machine given a type
> > > definition. The state image contains just runtime data reflecting what
> > > guest driver does on SRC machine. Restoring such state shouldn't
> > > change the isolation policy.  
> > 
> > Let's invent an example where the mdev vendor driver has a set of
> > pinned pages which are the current working set for the device at the
> > time of migration.  Information about that pinning might be included in
> > the opaque migration state.  If a malicious user discovers this, they
> > can potentially also craft a modified state which can exploit the host
> > kernel isolation.  
> 
> pinned pages may be not a good example. the pin knowledge could be
> reconstructed when restoring the state (e.g. in GVT-g pinning is triggered
> by shadowing GPU page table which has to be recreated on DEST). 

There are always ways for vendor drivers to do this correctly, but
again, one vendor doing it correctly doesn't prevent this from being a
gaping security issue with unending vulnerabilities for other vendors.

> > > > > Then all the audits/security checks
> > > > > enforced in normal emulation path should still apply. vendor driver
> > > > > may choose to audit every state restore operation one-by-one, and
> > > > > do it altoghter at a synchronization point (e.g. when the mdev is re-
> > > > > scheduled, similar to what we did before VMENTRY).  
> > > >
> > > > Giving the vendor driver the choice of whether to be secure or not is
> > > > exactly what I'm trying to propose we spend some time thinking about.
> > > > For instance, what if instead of allowing the user to load device state
> > > > through a region, the kernel could side load it using sometime similar
> > > > to the firmware loading path.  The user could be provided with a file
> > > > name token that they push through the vfio 

[libvirt] [PATCH] Add virNetlinkNewLink for simplifying virNetDev*Create

[Shi Lei]

This patch adds virNetlinkNewLink for simplifying virNetDevMacVLanCreate
and virNetDevBridgeCreate.

Signed-off-by: Shi Lei 
---
 src/libvirt_private.syms|   1 +
 src/util/virnetdevbridge.c  |  73 +++
 src/util/virnetdevmacvlan.c | 137 ++--
 src/util/virnetlink.c   | 104 +
 src/util/virnetlink.h   |   8 +++
 5 files changed, 164 insertions(+), 159 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 47ea35f..23931ba 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2439,6 +2439,7 @@ virNetlinkEventServiceStop;
 virNetlinkEventServiceStopAll;
 virNetlinkGetErrorCode;
 virNetlinkGetNeighbor;
+virNetlinkNewLink;
 virNetlinkShutdown;
 virNetlinkStartup;
 
diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
index bc377b5..1f5b37e 100644
--- a/src/util/virnetdevbridge.c
+++ b/src/util/virnetdevbridge.c
@@ -417,77 +417,22 @@ virNetDevBridgeCreate(const char *brname)
 {
 /* use a netlink RTM_NEWLINK message to create the bridge */
 const char *type = "bridge";
-struct nlmsgerr *err;
-struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC };
-unsigned int recvbuflen;
-struct nlattr *linkinfo;
-VIR_AUTOPTR(virNetlinkMsg) nl_msg = NULL;
-VIR_AUTOFREE(struct nlmsghdr *) resp = NULL;
+int error = 0;
 
-nl_msg = nlmsg_alloc_simple(RTM_NEWLINK,
-NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL);
-if (!nl_msg) {
-virReportOOMError();
-return -1;
-}
-
-if (nlmsg_append(nl_msg,  , sizeof(ifinfo), NLMSG_ALIGNTO) < 0)
-goto buffer_too_small;
-if (nla_put(nl_msg, IFLA_IFNAME, strlen(brname)+1, brname) < 0)
-goto buffer_too_small;
-if (!(linkinfo = nla_nest_start(nl_msg, IFLA_LINKINFO)))
-goto buffer_too_small;
-if (nla_put(nl_msg, IFLA_INFO_KIND, strlen(type), type) < 0)
-goto buffer_too_small;
-nla_nest_end(nl_msg, linkinfo);
-
-if (virNetlinkCommand(nl_msg, , , 0, 0,
-  NETLINK_ROUTE, 0) < 0) {
-return -1;
-}
-
-if (recvbuflen < NLMSG_LENGTH(0) || resp == NULL)
-goto malformed_resp;
-
-switch (resp->nlmsg_type) {
-case NLMSG_ERROR:
-err = (struct nlmsgerr *)NLMSG_DATA(resp);
-if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
-goto malformed_resp;
-
-if (err->error < 0) {
+if (virNetlinkNewLink(NULL, brname, NULL, type, NULL, NULL, ) < 0) {
 # if defined(HAVE_STRUCT_IFREQ) && defined(SIOCBRADDBR)
-if (err->error == -EOPNOTSUPP) {
-/* fallback to ioctl if netlink doesn't support creating
- * bridges
- */
-return virNetDevBridgeCreateWithIoctl(brname);
-}
-# endif
-
-virReportSystemError(-err->error,
- _("error creating bridge interface %s"),
- brname);
-return -1;
+if (error == -EOPNOTSUPP) {
+/* fallback to ioctl if netlink doesn't support creating bridges */
+return virNetDevBridgeCreateWithIoctl(brname);
 }
-break;
+# endif
+virReportSystemError(-error, _("error creating bridge interface %s"),
+ brname);
 
-case NLMSG_DONE:
-break;
-default:
-goto malformed_resp;
+return -1;
 }
 
 return 0;
-
- malformed_resp:
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("malformed netlink response message"));
-return -1;
- buffer_too_small:
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("allocated netlink buffer is too small"));
-return -1;
 }
 
 
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index 2035b1f..1629add 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -283,6 +283,37 @@ virNetDevMacVLanReleaseName(const char *name)
 }
 
 
+static int
+virNetDevMacVLanCreateCallback(struct nl_msg *nl_msg, const void *opaque)
+{
+const uint32_t *mode = (const uint32_t *) opaque;
+if (!nl_msg || !opaque) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("nl_msg %p or opaque %p is NULL"),
+   nl_msg, opaque);
+return -1;
+}
+
+if (*mode > 0) {
+struct nlattr *info_data;
+if (!(info_data = nla_nest_start(nl_msg, IFLA_INFO_DATA)))
+goto buffer_too_small;
+
+if (nla_put(nl_msg, IFLA_MACVLAN_MODE, sizeof(*mode), mode) < 0)
+goto buffer_too_small;
+
+nla_nest_end(nl_msg, info_data);
+}
+
+return 0;
+
+ buffer_too_small:
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("allocated netlink buffer is too small"));
+return -1;
+}
+
+
 /**
  * virNetDevMacVLanCreate:
  *
@@ -307,113 

Re: [libvirt] Matching the type of mediated devices in the migration

[Tian, Kevin]

> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Thursday, August 23, 2018 11:47 AM
> 
> On Wed, 22 Aug 2018 02:30:12 +
> "Tian, Kevin"  wrote:
> 
> > > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > > Sent: Wednesday, August 22, 2018 10:08 AM
> > >
> > > On Wed, 22 Aug 2018 01:27:05 +
> > > "Tian, Kevin"  wrote:
> > >
> > > > > From: Wang, Zhi A
> > > > > Sent: Wednesday, August 22, 2018 2:43 AM
> > > > > >
> > > > > > Are there any suggestions how we can deal with security issues?
> > > > > > Allowing userspace to provide a data stream representing the
> internal
> > > > > > state of a virtual device model living within the kernel seems
> > > > > > troublesome.  If we need to trust the data stream, do we need to
> > > > > > somehow make the operation more privileged than what a vfio
> user
> > > > > might
> > > > > > have otherwise?  Does the data stream need to be somehow
> signed
> > > and
> > > > > how
> > > > > > might we do that?  How can we build in protection against an
> > > untrusted
> > > > > > restore image?  Thanks,
> > > >
> > > > imo it is not necessary. restoring mdev state should be handled as if
> > > > guest is programming the mdev.
> > >
> > > To me this suggests that a state save/restore is just an algorithm
> > > executed by userspace using the existing vfio device accesses.  This is
> > > not at all what we've been discussing for migration.  I believe the
> >
> > not algorithm by userspace. It's kernel driver to apply the audit when
> > receiving opaque state data.
> 
> And a kernel driver receiving and processing opaque state date from a
> user doesn't raise security concerns for you?

opaque is from userspace p.o.v. kernel driver understands the actual
format and thus can audit when restoring the state.

> 
> > > interface we've been hashing out exposes opaque device state through
> a
> > > vfio region.  We therefore must assume that that opaque data contains
> > > not only device state, but also emulation state, similar to what we see
> > > for any QEMU device.  Not only is there internal emulation state, but
> > > we have no guarantee that the device state goes through the same
> > > auditing as it does through the vfio interface.  Since this device and
> > > emulation state live inside the kernel and not just within the user's
> > > own process, a malicious user can do far more than shoot themselves.
> It
> > > would be one thing devices were IOMMU isolated, but they're not,
> > > they're isolated through vendor and device specific mechanism, and for
> > > all we know the parameters of that isolation are included in the
> > > restore state.  I don't see how we can say this is not an issue.
> >
> > I didn't quite get this. My understanding is that isolation configuration
> > is completed when a mdev is created on DEST machine given a type
> > definition. The state image contains just runtime data reflecting what
> > guest driver does on SRC machine. Restoring such state shouldn't
> > change the isolation policy.
> 
> Let's invent an example where the mdev vendor driver has a set of
> pinned pages which are the current working set for the device at the
> time of migration.  Information about that pinning might be included in
> the opaque migration state.  If a malicious user discovers this, they
> can potentially also craft a modified state which can exploit the host
> kernel isolation.

pinned pages may be not a good example. the pin knowledge could be
reconstructed when restoring the state (e.g. in GVT-g pinning is triggered
by shadowing GPU page table which has to be recreated on DEST). 

> 
> > > > Then all the audits/security checks
> > > > enforced in normal emulation path should still apply. vendor driver
> > > > may choose to audit every state restore operation one-by-one, and
> > > > do it altoghter at a synchronization point (e.g. when the mdev is re-
> > > > scheduled, similar to what we did before VMENTRY).
> > >
> > > Giving the vendor driver the choice of whether to be secure or not is
> > > exactly what I'm trying to propose we spend some time thinking about.
> > > For instance, what if instead of allowing the user to load device state
> > > through a region, the kernel could side load it using sometime similar
> > > to the firmware loading path.  The user could be provided with a file
> > > name token that they push through the vfio interface to trigger the
> > > state loading from a location with proper file level ACLs such that the
> > > image can be considered trusted.  Unfortunately the collateral is that
> > > libvirt would need to become the secure delivery entity, somehow
> > > stripping this section of the migration stream into a file and
> > > providing a token for the user to ask the kernel to load it.  What are
> > > some other options?  Could save/restore be done simply as an
> > > algorithmic script matched to stack of data, as I read into your first
> > > statement above?  I have doubts that we can achieve 

Re: [libvirt] Matching the type of mediated devices in the migration

[Alex Williamson]

On Wed, 22 Aug 2018 02:30:12 +
"Tian, Kevin"  wrote:

> > From: Alex Williamson [mailto:alex.william...@redhat.com]
> > Sent: Wednesday, August 22, 2018 10:08 AM
> > 
> > On Wed, 22 Aug 2018 01:27:05 +
> > "Tian, Kevin"  wrote:
> >   
> > > > From: Wang, Zhi A
> > > > Sent: Wednesday, August 22, 2018 2:43 AM  
> > > > >
> > > > > Are there any suggestions how we can deal with security issues?
> > > > > Allowing userspace to provide a data stream representing the internal
> > > > > state of a virtual device model living within the kernel seems
> > > > > troublesome.  If we need to trust the data stream, do we need to
> > > > > somehow make the operation more privileged than what a vfio user  
> > > > might  
> > > > > have otherwise?  Does the data stream need to be somehow signed  
> > and  
> > > > how  
> > > > > might we do that?  How can we build in protection against an  
> > untrusted  
> > > > > restore image?  Thanks,  
> > >
> > > imo it is not necessary. restoring mdev state should be handled as if
> > > guest is programming the mdev.  
> > 
> > To me this suggests that a state save/restore is just an algorithm
> > executed by userspace using the existing vfio device accesses.  This is
> > not at all what we've been discussing for migration.  I believe the  
> 
> not algorithm by userspace. It's kernel driver to apply the audit when
> receiving opaque state data.

And a kernel driver receiving and processing opaque state date from a
user doesn't raise security concerns for you?

> > interface we've been hashing out exposes opaque device state through a
> > vfio region.  We therefore must assume that that opaque data contains
> > not only device state, but also emulation state, similar to what we see
> > for any QEMU device.  Not only is there internal emulation state, but
> > we have no guarantee that the device state goes through the same
> > auditing as it does through the vfio interface.  Since this device and
> > emulation state live inside the kernel and not just within the user's
> > own process, a malicious user can do far more than shoot themselves.  It
> > would be one thing devices were IOMMU isolated, but they're not,
> > they're isolated through vendor and device specific mechanism, and for
> > all we know the parameters of that isolation are included in the
> > restore state.  I don't see how we can say this is not an issue.  
> 
> I didn't quite get this. My understanding is that isolation configuration
> is completed when a mdev is created on DEST machine given a type
> definition. The state image contains just runtime data reflecting what
> guest driver does on SRC machine. Restoring such state shouldn't
> change the isolation policy.

Let's invent an example where the mdev vendor driver has a set of
pinned pages which are the current working set for the device at the
time of migration.  Information about that pinning might be included in
the opaque migration state.  If a malicious user discovers this, they
can potentially also craft a modified state which can exploit the host
kernel isolation.

> > > Then all the audits/security checks
> > > enforced in normal emulation path should still apply. vendor driver
> > > may choose to audit every state restore operation one-by-one, and
> > > do it altoghter at a synchronization point (e.g. when the mdev is re-
> > > scheduled, similar to what we did before VMENTRY).  
> > 
> > Giving the vendor driver the choice of whether to be secure or not is
> > exactly what I'm trying to propose we spend some time thinking about.
> > For instance, what if instead of allowing the user to load device state
> > through a region, the kernel could side load it using sometime similar
> > to the firmware loading path.  The user could be provided with a file
> > name token that they push through the vfio interface to trigger the
> > state loading from a location with proper file level ACLs such that the
> > image can be considered trusted.  Unfortunately the collateral is that
> > libvirt would need to become the secure delivery entity, somehow
> > stripping this section of the migration stream into a file and
> > providing a token for the user to ask the kernel to load it.  What are
> > some other options?  Could save/restore be done simply as an
> > algorithmic script matched to stack of data, as I read into your first
> > statement above?  I have doubts that we can achieve the internal state
> > we need, or maybe even the performance we need using such a process.
> > Thanks,
> >   
> 
> for GVT-g I think we invoke common functions as used in emulation path
> to recover vGPU state, e.g. gtt rw handler, etc. Zhi can correct me if
> I'm wrong.

One example of migration state being restored in a secure manner does
not prove that such an interface is universally secure or a good idea.

> Can you elaborate the difference between device state and emulation
> state which you mentioned earlier? We may need look at some concrete
> example to understand 

Re: [libvirt] [PATCH] Take DryRun code out of virCommand* code flow

[石磊]

>On Wed, Aug 22, 2018 at 11:37:34AM +0800, Shi Lei wrote:
>> Since dry-run of cmd is just for tests now, it would be better to remove
>> dry-run code and move them to testutils. Then the code flow in virCommand*
>> could be more general. There are 3 steps in this patch:
>> 1. Introduce a new global hook (of virExecHook type) which will be called
>>    in code flow just before the cmd->hook. The global hook is also called in
>>    child process. If it returns 1, the child process will exit with status
>>    in advance and the parent will process io and wait for the child normally.
>>    It prepares for registering dry-run and anything else.
>>    The virCommandSetPreExecHook is modified for registering both types of 
>>hooks.
>> 2. Implement virTestSetDryRun with dry-run code in "testutils.c".
>>    It substitutes for virCommandSetDryRun. The virTestSetDryRun invokes
>>    virCommandSetPreExecHook to register a function on the global hook which
>>    will fill in cmdline buffer and call callback for tests.
>> 3. Remove all dryrun code in "vircommand.c" and remove virCommandSetDryRun 
>> API.
>>
>> Diffs from old dry-run:
>> The new global hook is called in child process. So dryrun-callback for tests
>> should write to stdout/stderr when they need output something.
>>
>> Now the opaque argument for dryrun-callback cannot be inout. In 
>> "tests/viriscsitest.c",
>> iface_created in opaque of callback is an inout argument. There's a bit
>> complicated to transfer it between the child and its parent. So this patch 
>> use
>> a temporary file to do that.
>>   
>> Signed-off-by: Shi Lei 
>> ---
>[snip]
>
>>  void
>>  virCommandSetPreExecHook(virCommandPtr cmd, virExecHook hook, void *opaque)
>>  {
>> -    if (!cmd || cmd->has_error)
>> +    if (!cmd) {
>> +    /* Global hook */
>> +    preExecHook = hook;
>> +    preExecOpaque = opaque;
>> +    return;
>> +    }
>
>I don't think this is an improvement.
>
>With the virCommandSetDryRun() approach there is no way that the dry run
>code can be accidentally triggered in production scenarios, as we can be
>sure nothing will accidentally call virCommandSetDryRun().
>
>Changing the semantics of virCommandSetPreExecHook() so that it sets a
>global hook when 'cmd' is NULL introduces significant risk. The virCommand
>APIs are designed to fail-safe in face of memory exhaustion or errors from
>the caller. IOW passing a NULL for 'cmd' is an expected scenario in a
>production environment, and this change breaks handling of that.
>
>Personally I don't think the stated problem needs solving at all. The
>virCommandSetDryRun() works reliably and doesn't need rewriting IMHO.
>
>Regards,
>Daniel 

Okay. Thanks for your comments.

Regards,
ShiLei

>--
>|: 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] util: eventpoll: Survive EBADF on macOS

[Roman Bolshakov]

On Wed, Aug 22, 2018 at 10:41:18AM +0100, Daniel P. Berrangé wrote:
> I don't think we should go through the dispatch code when
> we get EBADF, as the contents of the 'revents' fields are
> undefined after we get an error.
> 
> We should jump straight to the end where we have
> "eventLoop.running = 0;"
> 
> If any FDs were in fact ready, we'll get them on the next iteration of
> the loop anyway.
> 

Alright, but the thread won't hold evenLoop.lock after such jump and
virMutexUnlock may fail with ENOPERM. I suppose that could confuse
valgrind & friends.
What if we change eventLoop.running to 0 under the eventLoop.lock right in
the macOS EBADF handler and then return?

--
Roman

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

[libvirt] [PATCH] nwfilter: Add extra verbiage for binding create/delete

[John Ferlan]

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

Add some cautionary words related to the create and delete
NWFilter Binding use cases and possible issues that may result
to the virsh nwfilter-binding-{create|delete} descriptions
and the virNWFilterBinding{CreateXML|Delete) API descriptions.

Essentially summarizing commit 2d9318b6c without using the
shoot yourself in the foot wording.

Signed-off-by: John Ferlan 
---
 Perhaps not the exact "answer" for the bz; however, using the bz as the
 excuse to say it was a bit confusing and let's try to clarify the wording
 a bit more.

 src/libvirt-nwfilter.c | 16 ++--
 tools/virsh.pod| 14 --
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/src/libvirt-nwfilter.c b/src/libvirt-nwfilter.c
index e572d46c18..3da85adc9e 100644
--- a/src/libvirt-nwfilter.c
+++ b/src/libvirt-nwfilter.c
@@ -678,7 +678,14 @@ virNWFilterBindingGetFilterName(virNWFilterBindingPtr 
binding)
  * @flags: currently unused, pass 0
  *
  * Define a new network filter, based on an XML description
- * similar to the one returned by virNWFilterGetXMLDesc()
+ * similar to the one returned by virNWFilterGetXMLDesc(). This
+ * API may be used to associate a filter with a currently running
+ * guest that does not have a filter defined for a specific network
+ * port. Since the bindings are generally automatically managed by
+ * the hypervisor, using this command to define a filter for a network
+ * port and then starting the guest afterwards may prevent the guest
+ * from starting if it attempts to use the network port and finds a
+ * filter already defined.
  *
  * virNWFilterFree should be used to free the resources after the
  * binding object is no longer needed.
@@ -717,7 +724,12 @@ virNWFilterBindingCreateXML(virConnectPtr conn, const char 
*xml, unsigned int fl
  * @binding: a binding object
  *
  * Delete the binding object. This does not free the
- * associated virNWFilterBindingPtr object.
+ * associated virNWFilterBindingPtr object. This API
+ * may be used to remove the network port binding filter
+ * currently in use for the guest while the guest is
+ * running without needing to restart the guest. Restoring
+ * the network port binding filter for the running guest
+ * would be accomplished by using virNWFilterBindingCreateXML.
  *
  * Returns 0 in case of success and -1 in case of failure.
  */
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 4e118851f8..86c041d575 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -4836,13 +4836,23 @@ of the network filters directly.
 =item B I
 
 Associate a network port with a network filter. The network filter backend
-will immediately attempt to instantiate the filter rules on the port.
+will immediately attempt to instantiate the filter rules on the port. This
+command may be used to associate a filter with a currently running guest
+that does not have a filter defined for a specific network port. Since the
+bindings are generally automatically managed by the hypervisor, using this
+command to define a filter for a network port and then starting the guest
+afterwards may prevent the guest from starting if it attempts to use the
+network port and finds a filter already defined.
 
 =item B I
 
 Disassociate a network port from a network filter. The network filter
 backend will immediately tear down the filter rules that exist on the
-port.
+port. This command may be used to remove the network port binding for
+a filter currently in use for the guest while the guest is running
+without needing to restart the guest. Restoring the network port binding
+filter for the running guest would be accomplished by using
+I.
 
 =item B
 
-- 
2.17.1

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

[libvirt] [PATCH] nwfilter: Handle libvirtd restart if nwfilter binding deleted

[John Ferlan]

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

It's stated that if the admin wants to shoot themselves in
the foot by removing the nwfilter binding while the domain
is running we will certainly allow that.  However, in doing
so we also run the risk that a libvirtd restart will cause
the domain to be shutdown, which isn't a good thing.

So add another boolean to virDomainConfNWFilterInstantiate
which allows us to recover somewhat gracefully in the event
the virNWFilterBindingCreateXML fails when we come from
qemuProcessReconnect and we determine that the filter has
been deleted. It was there at some point (it had to be), but
if it's missing, then we don't want to cause the guest to
stop running, so issue a warning and continue on.

Signed-off-by: John Ferlan 
---
 src/conf/domain_nwfilter.c | 33 -
 src/conf/domain_nwfilter.h |  3 ++-
 src/lxc/lxc_process.c  |  3 ++-
 src/qemu/qemu_hotplug.c|  7 ---
 src/qemu/qemu_interface.c  |  6 --
 src/qemu/qemu_process.c| 10 +++---
 src/uml/uml_conf.c |  3 ++-
 7 files changed, 49 insertions(+), 16 deletions(-)

diff --git a/src/conf/domain_nwfilter.c b/src/conf/domain_nwfilter.c
index f39c8a1f9b..3e6e462def 100644
--- a/src/conf/domain_nwfilter.c
+++ b/src/conf/domain_nwfilter.c
@@ -85,16 +85,19 @@ int
 virDomainConfNWFilterInstantiate(const char *vmname,
  const unsigned char *vmuuid,
  virDomainNetDefPtr net,
- bool ignoreExists)
+ bool ignoreExists,
+ bool ignoreDeleted)
 {
 virConnectPtr conn = virGetConnectNWFilter();
 virNWFilterBindingDefPtr def = NULL;
 virNWFilterBindingPtr binding = NULL;
+virNWFilterPtr nwfilter = NULL;
 char *xml = NULL;
 int ret = -1;
 
-VIR_DEBUG("vmname=%s portdev=%s filter=%s ignoreExists=%d",
-  vmname, NULLSTR(net->ifname), NULLSTR(net->filter), 
ignoreExists);
+VIR_DEBUG("vmname=%s portdev=%s filter=%s ignoreExists=%d 
ignoreDeleted=%d",
+  vmname, NULLSTR(net->ifname), NULLSTR(net->filter),
+  ignoreExists, ignoreDeleted);
 
 if (!conn)
 goto cleanup;
@@ -113,14 +116,34 @@ virDomainConfNWFilterInstantiate(const char *vmname,
 if (!(xml = virNWFilterBindingDefFormat(def)))
 goto cleanup;
 
-if (!(binding = virNWFilterBindingCreateXML(conn, xml, 0)))
-goto cleanup;
+if (!(binding = virNWFilterBindingCreateXML(conn, xml, 0))) {
+virErrorPtr orig_err;
+
+if (!ignoreDeleted)
+goto cleanup;
+
+/* Let's determine if the error was because the filter was deleted.
+ * Save the orig_err just in case it's not a failure to find the
+ * filter by name. */
+orig_err = virSaveLastError();
+nwfilter = virNWFilterLookupByName(conn, def->filter);
+virSetError(orig_err);
+virFreeError(orig_err);
+if (nwfilter)
+goto cleanup;
+
+VIR_WARN("filter '%s' for binding '%s' has been deleted while the "
+ "guest was running, ignoring for restart processing",
+ def->filter, def->portdevname);
+virResetLastError();
+}
 
 ret = 0;
 
  cleanup:
 VIR_FREE(xml);
 virNWFilterBindingDefFree(def);
+virObjectUnref(nwfilter);
 virObjectUnref(binding);
 virObjectUnref(conn);
 return ret;
diff --git a/src/conf/domain_nwfilter.h b/src/conf/domain_nwfilter.h
index 6bda228fc8..e3a2f7a7f2 100644
--- a/src/conf/domain_nwfilter.h
+++ b/src/conf/domain_nwfilter.h
@@ -26,7 +26,8 @@
 int virDomainConfNWFilterInstantiate(const char *vmname,
  const unsigned char *vmuuid,
  virDomainNetDefPtr net,
- bool ignoreExists);
+ bool ignoreExists,
+ bool ignoreDeleted);
 void virDomainConfNWFilterTeardown(virDomainNetDefPtr net);
 void virDomainConfVMNWFilterTeardown(virDomainObjPtr vm);
 
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index 33c806630b..b8b014ca72 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -303,7 +303,8 @@ virLXCProcessSetupInterfaceTap(virDomainDefPtr vm,
 }
 
 if (net->filter &&
-virDomainConfNWFilterInstantiate(vm->name, vm->uuid, net, false) < 0)
+virDomainConfNWFilterInstantiate(vm->name, vm->uuid, net,
+ false, false) < 0)
 goto cleanup;
 
 ret = containerVeth;
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 0b84a503bb..11b10cbe14 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3434,8 +3434,8 @@ qemuDomainChangeNetFilter(virDomainObjPtr vm,
 virDomainConfNWFilterTeardown(olddev);
 
 if (newdev->filter &&
-

[libvirt] [PATCH] qemu: fix error message on directory creation

[Eric Blake]

Minor copy-and-paste bug present since commit 462c74c3, in Apr 2010.

Signed-off-by: Eric Blake 
---
Pushing this under the trivial rule.

 src/qemu/qemu_driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 21e9e87ddd..da8c4e8991 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -656,7 +656,7 @@ qemuStateInitialize(bool privileged,
 goto error;
 }
 if (virFileMakePath(cfg->snapshotDir) < 0) {
-virReportSystemError(errno, _("Failed to create save dir %s"),
+virReportSystemError(errno, _("Failed to create snapshot dir %s"),
  cfg->snapshotDir);
 goto error;
 }
-- 
2.17.1

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

[libvirt] proposal: file renames for consistency

[Eric Blake]

We have an inconsistent mix of multi-word file names that use dash vs. 
underscore to separate those words.


$ git ls-files '**/*_*' | wc
   24972497  146485
$ git ls-files '**/*-*' | wc
   81478147  26

Some of those are overlaps (meaning we can't make up our minds), such as:
docs/api_extension/0001-add-to-xml.patch

However, my personal preference is to use dash everywhere (or at least 
on all the .[ch] files), for at least several reasons:


1. dash is easier to type (at least on US keyboards), as it does not 
require the shift key [I find it easier to edit files in tools/ than in 
src/qemu/ for example, because I don't have to worry about shift]


2. include/ uses only dash, not underscore; if our public interface 
favors that spelling, our internal files should too


3. underscore is easier to miss visually when other markup like 
underline is being applied (for example, when viewing the git repository 
online and hovering over a filename). Granted, you can still often tell 
that an underscore is present vs. a space (or in general, realize that 
we avoid spaces in file names in our git tree, so it must be an 
underscore); but I still find dash much easier to pick out.


Since git rename detection does a good job at tracking history by 
content over file renames, a mass rename wouldn't even be that invasive 
to git archaeology or patch backporting.


If you're worried about finger memory typing the wrong form, but you use 
bash, it's possible to configure bash to treat '-' and '_' 
interchangeably during TAB-completion (similar to case insensitive tab 
completion), which reduces some of the pain, but it doesn't solve 
everything.


So, would anyone like a beginner's project of consistently renaming 
files, and/or would anyone object if I submitted some patches along 
those lines?


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

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

[libvirt] libvirt and virt-manager - Unable to complete install: 'internal error: unsupported input bus usb'

[Eduardo Lúcio Amorim Costa]

I'm trying create a virtual machine using Xen as hypervisor and
virt-manager (libvirt) as the management module. When trying to create the
virtual machine I am getting the following error:

"
Unable to complete install: 'internal error: unsupported input bus usb'
"

ERROR DETAILS:

Unable to complete install: 'internal error: unsupported input bus usb'

Traceback (most recent call last):
  File "/usr/share/virt-manager/virtManager/asyncjob.py", line 75, in
cb_wrapper
callback(asyncjob, *args, **kwargs)
  File "/usr/share/virt-manager/virtManager/create.py", line 2276, in
_do_async_install
guest.start_install(meter=meter)
  File "/usr/share/virt-manager/virtinst/guest.py", line 461, in
start_install
doboot, transient)
  File "/usr/share/virt-manager/virtinst/guest.py", line 397, in
_create_guest
domain = self.conn.createXML(install_xml or final_xml, 0)
  File "/usr/lib/python3.7/site-packages/libvirt.py", line 3717, in
createXML
if ret is None:raise libvirtError('virDomainCreateXML() failed',
conn=self)
libvirt.libvirtError: internal error: unsupported input bus usb

NOTE I: The deploy process follows the instructions here
https://www.youtube.com/watch?v=BwkmDM-Gpzc and here
https://wiki.centos.org/HowTos/Xen/Xen4QuickStart .
NOTE II: Xen Hypervisor uses a CentOS 7 as "domu".

MORE INFORMATION ABOUT THE PROBLEM HERE:
https://unix.stackexchange.com/q/464186/61742

Thanks! =D
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v4 3/3] vsh: Added tests

[Simon Kobyda]

For now, there are 9 test cases
- testVshTableNew: Creating table with empty header
- testVshTableHeader: Printing table with/without header
- testVshTableRowAppend: Appending row with various number of cells.
  Only row with same number of cells as in header is accepted.
- testUnicode: Printing table with unicode characters.
  Checking correct alignment.
- testUnicodeArabic: test opposite (right to left) writing
- testUnicodeZeroWidthChar
- testUnicodeCombiningChar
- testUnicodeNonPrintableChar,
- testNTables: Create and print varios types of tables - one column,
  one row table, table without content, standart table...

Signed-off-by: Simon Kobyda 
---
 tests/Makefile.am|   8 +
 tests/vshtabletest.c | 393 +++
 2 files changed, 401 insertions(+)
 create mode 100644 tests/vshtabletest.c

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 21a6c823d9..136fe16f71 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -206,6 +206,7 @@ test_programs = virshtest sockettest \
virhostdevtest \
virnetdevtest \
virtypedparamtest \
+   vshtabletest \
$(NULL)
 
 test_libraries = libshunload.la \
@@ -938,6 +939,13 @@ metadatatest_SOURCES = \
testutils.c testutils.h
 metadatatest_LDADD = $(LDADDS) $(LIBXML_LIBS)
 
+vshtabletest_SOURCES = \
+   vshtabletest.c \
+   testutils.c testutils.h
+vshtabletest_LDADD = \
+   $(LDADDS) \
+   ../tools/libvirt_shell.la
+
 virshtest_SOURCES = \
virshtest.c \
testutils.c testutils.h
diff --git a/tests/vshtabletest.c b/tests/vshtabletest.c
new file mode 100644
index 00..48553057d4
--- /dev/null
+++ b/tests/vshtabletest.c
@@ -0,0 +1,393 @@
+/*
+ * Copyright (C) 2018 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ */
+
+#include 
+
+#include 
+#include 
+#include 
+
+#include "internal.h"
+#include "testutils.h"
+#include "viralloc.h"
+#include "../tools/vsh-table.h"
+
+static int
+testVshTableNew(const void *opaque ATTRIBUTE_UNUSED)
+{
+if (vshTableNew(NULL)) {
+fprintf(stderr, "expected failure when passing null to vshTableNew\n");
+return -1;
+}
+
+return 0;
+}
+
+static int
+testVshTableHeader(const void *opaque ATTRIBUTE_UNUSED)
+{
+int ret = 0;
+char *act = NULL;
+const char *exp =
+" 1   fedora28   running  \n"
+" 2   rhel7.5running  \n";
+const char *exp2 =
+" Id   Name   State\n"
+"--\n"
+" 1fedora28   running  \n"
+" 2rhel7.5running  \n";
+
+vshTablePtr table = vshTableNew("Id", "Name", "State",
+NULL); //to ask about return
+if (!table)
+goto cleanup;
+
+vshTableRowAppend(table, "1", "fedora28", "running", NULL);
+vshTableRowAppend(table, "2", "rhel7.5", "running",
+  NULL);
+
+act = vshTablePrintToString(table, false);
+if (virTestCompareToString(exp, act) < 0)
+ret = -1;
+
+VIR_FREE(act);
+act = vshTablePrintToString(table, true);
+if (virTestCompareToString(exp2, act) < 0)
+ret = -1;
+
+ cleanup:
+VIR_FREE(act);
+vshTableFree(table);
+return ret;
+}
+
+static int
+testVshTableRowAppend(const void *opaque ATTRIBUTE_UNUSED)
+{
+int ret = 0;
+
+vshTablePtr table = vshTableNew("Id", "Name", NULL);
+if (!table)
+goto cleanup;
+
+if (vshTableRowAppend(table, NULL) >= 0) {
+fprintf(stderr, "Appending NULL shouldn't work\n");
+ret = -1;
+}
+
+if (vshTableRowAppend(table, "2", NULL) >= 0) {
+fprintf(stderr, "Appending less items than in header\n");
+ret = -1;
+}
+
+if (vshTableRowAppend(table, "2", "rhel7.5", "running",
+  NULL) >= 0) {
+fprintf(stderr, "Appending more items than in header\n");
+ret = -1;
+}
+
+if (vshTableRowAppend(table, "2", "rhel7.5", NULL) < 0) {
+fprintf(stderr, "Appending same number of items as in header"
+" should not return NULL\n");
+ret = -1;
+}
+
+ cleanup:
+vshTableFree(table);
+return ret;
+}
+
+static int
+testUnicode(const void *opaque ATTRIBUTE_UNUSED)
+{
+
+int ret = 0;
+char *act = NULL;
+
+char *locale = setlocale(LC_CTYPE, NULL);
+if (!setlocale(LC_CTYPE, 

[libvirt] [PATCH v4 1/3] vsh: Add API for printing tables.

[Simon Kobyda]

It solves problems with alignment of columns. Width of each column
is calculated by its biggest cell. Should solve unicode bug.
In future, it may be implemented in virsh, virt-admin...

This API has 5 public functions:
- vshTableNew - adds new table and defines its header
- vshTableRowAppend - appends new row (for same number of columns as in
header)
- vshTablePrintToStdout
- vshTablePrintToString
- vshTableFree

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

Signed-off-by: Simon Kobyda 
---
 tools/Makefile.am |   4 +-
 tools/vsh-table.c | 483 ++
 tools/vsh-table.h |  42 
 3 files changed, 528 insertions(+), 1 deletion(-)
 create mode 100644 tools/vsh-table.c
 create mode 100644 tools/vsh-table.h

diff --git a/tools/Makefile.am b/tools/Makefile.am
index 1452d984a0..f069167acc 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -144,7 +144,9 @@ libvirt_shell_la_LIBADD = \
$(READLINE_LIBS) \
../gnulib/lib/libgnu.la \
$(NULL)
-libvirt_shell_la_SOURCES = vsh.c vsh.h
+libvirt_shell_la_SOURCES = \
+   vsh.c vsh.h \
+   vsh-table.c vsh-table.h
 
 virt_host_validate_SOURCES = \
virt-host-validate.c \
diff --git a/tools/vsh-table.c b/tools/vsh-table.c
new file mode 100644
index 00..6e1793e4e3
--- /dev/null
+++ b/tools/vsh-table.c
@@ -0,0 +1,483 @@
+/*
+ * vsh-table.c: table printing helper
+ *
+ * Copyright (C) 2018 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ *
+ * Authors:
+ *   Simon Kobyda 
+ *
+ */
+
+#include 
+#include "vsh-table.h"
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "c-ctype.h"
+
+#include "viralloc.h"
+#include "virbuffer.h"
+#include "virstring.h"
+#include "virsh-util.h"
+
+#define HEX_ENCODE_LENGTH 4 /* represents length of '\xNN' */
+
+struct _vshTableRow {
+char **cells;
+size_t ncells;
+};
+
+struct _vshTable {
+vshTableRowPtr *rows;
+size_t nrows;
+};
+
+static void
+vshTableRowFree(vshTableRowPtr row)
+{
+size_t i;
+
+if (!row)
+return;
+
+for (i = 0; i < row->ncells; i++)
+VIR_FREE(row->cells[i]);
+
+VIR_FREE(row->cells);
+VIR_FREE(row);
+}
+
+void
+vshTableFree(vshTablePtr table)
+{
+size_t i;
+
+if (!table)
+return;
+
+for (i = 0; i < table->nrows; i++)
+vshTableRowFree(table->rows[i]);
+VIR_FREE(table->rows);
+VIR_FREE(table);
+}
+
+/**
+ * vshTableRowNew:
+ * @arg: the first argument.
+ * @ap: list of variadic arguments
+ *
+ * Create a new row in the table. Each argument passed
+ * represents a cell in the row.
+ * Return: pointer to vshTableRowPtr row or NULL.
+ */
+static vshTableRowPtr
+vshTableRowNew(const char *arg, va_list ap)
+{
+vshTableRowPtr row = NULL;
+char *tmp = NULL;
+
+if (!arg) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+_("Table row cannot be empty"));
+goto error;
+}
+
+if (VIR_ALLOC(row) < 0)
+goto error;
+
+while (arg) {
+if (VIR_STRDUP(tmp, arg) < 0)
+goto error;
+
+if (VIR_APPEND_ELEMENT(row->cells, row->ncells, tmp) < 0)
+goto error;
+
+arg = va_arg(ap, const char *);
+}
+
+return row;
+
+ error:
+vshTableRowFree(row);
+return NULL;
+}
+
+/**
+ * vshTableNew:
+ * @arg: List of column names (NULL terminated)
+ *
+ * Create a new table.
+ *
+ * Returns: pointer to table or NULL.
+ */
+vshTablePtr
+vshTableNew(const char *arg, ...)
+{
+vshTablePtr table;
+vshTableRowPtr header = NULL;
+va_list ap;
+
+if (VIR_ALLOC(table) < 0)
+goto error;
+
+va_start(ap, arg);
+header = vshTableRowNew(arg, ap);
+va_end(ap);
+
+if (!header)
+goto error;
+
+if (VIR_APPEND_ELEMENT(table->rows, table->nrows, header) < 0)
+goto error;
+
+return table;
+ error:
+vshTableRowFree(header);
+vshTableFree(table);
+return NULL;
+}
+
+/**
+ * vshTableRowAppend:
+ * @table: table to append to
+ * @arg: cells of the row (NULL terminated)
+ *
+ * Append new row into the @table. The number of cells in the row has
+ * to be equal to the number of cells in the table header.
+ *
+ * Returns: 0 if succeeded, -1 if 

[libvirt] [PATCH v4 2/3] virsh: Implement new table API for virsh list

[Simon Kobyda]

Instead of printing it straight in virsh, it creates table struct
which is filled with header and rows(domains). It allows us to know
more about table before printing to calculate alignment right.

Signed-off-by: Simon Kobyda 
---
 tests/virshtest.c| 14 ++--
 tools/virsh-domain-monitor.c | 43 
 2 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/tests/virshtest.c b/tests/virshtest.c
index 94548a82d1..10cd0d356b 100644
--- a/tests/virshtest.c
+++ b/tests/virshtest.c
@@ -98,9 +98,9 @@ static int testCompareListDefault(const void *data 
ATTRIBUTE_UNUSED)
 {
   const char *const argv[] = { VIRSH_DEFAULT, "list", NULL };
   const char *exp = "\
- IdName   State\n\
-\n\
- 1 test   running\n\
+ Id   Name   State\n\
+--\n\
+ 1test   running  \n\
 \n";
   return testCompareOutputLit(exp, NULL, argv);
 }
@@ -109,10 +109,10 @@ static int testCompareListCustom(const void *data 
ATTRIBUTE_UNUSED)
 {
   const char *const argv[] = { VIRSH_CUSTOM, "list", NULL };
   const char *exp = "\
- IdName   State\n\
-\n\
- 1 fv0running\n\
- 2 fc4running\n\
+ Id   Name   State\n\
+--\n\
+ 1fv0running  \n\
+ 2fc4running  \n\
 \n";
   return testCompareOutputLit(exp, NULL, argv);
 }
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index b9b4f9739b..adc5bb1a7a 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -39,6 +39,7 @@
 #include "virmacaddr.h"
 #include "virxml.h"
 #include "virstring.h"
+#include "vsh-table.h"
 
 VIR_ENUM_DECL(virshDomainIOError)
 VIR_ENUM_IMPL(virshDomainIOError,
@@ -1901,6 +1902,7 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
 char id_buf[INT_BUFSIZE_BOUND(unsigned int)];
 unsigned int id;
 unsigned int flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE;
+vshTablePtr table = NULL;
 
 /* construct filter flags */
 if (vshCommandOptBool(cmd, "inactive") ||
@@ -1940,15 +1942,12 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
 /* print table header in legacy mode */
 if (optTable) {
 if (optTitle)
-vshPrintExtra(ctl, " %-5s %-30s %-10s %-20s\n%s\n",
-  _("Id"), _("Name"), _("State"), _("Title"),
-  "-"
-  "-");
+table = vshTableNew("Id", "Name", "State", "Title", NULL);
 else
-vshPrintExtra(ctl, " %-5s %-30s %s\n%s\n",
-  _("Id"), _("Name"), _("State"),
-  "-"
-  "---");
+table = vshTableNew("Id", "Name", "State", NULL);
+
+if (!table)
+goto cleanup;
 }
 
 for (i = 0; i < list->ndomains; i++) {
@@ -1973,20 +1972,22 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
 if (optTitle) {
 if (!(title = virshGetDomainDescription(ctl, dom, true, 0)))
 goto cleanup;
-
-vshPrint(ctl, " %-5s %-30s %-10s %-20s\n", id_buf,
- virDomainGetName(dom),
- state == -2 ? _("saved")
- : virshDomainStateToString(state),
- title);
-
+if (vshTableRowAppend(table, id_buf,
+  virDomainGetName(dom),
+  state == -2 ? _("saved")
+  : virshDomainStateToString(state),
+  title, NULL) < 0)
+goto cleanup;
 VIR_FREE(title);
 } else {
-vshPrint(ctl, " %-5s %-30s %s\n", id_buf,
- virDomainGetName(dom),
- state == -2 ? _("saved")
- : virshDomainStateToString(state));
+if (vshTableRowAppend(table, id_buf,
+  virDomainGetName(dom),
+  state == -2 ? _("saved")
+  : virshDomainStateToString(state),
+  NULL) < 0)
+goto cleanup;
 }
+
 } else if (optUUID && optName) {
 if (virDomainGetUUIDString(dom, uuid) < 0) {
 vshError(ctl, "%s", _("Failed to get domain's UUID"));
@@ -2004,8 +2005,12 @@ cmdList(vshControl *ctl, const vshCmd *cmd)
 }
 }
 
+if (optTable)
+vshTablePrintToStdout(table, ctl);
+
 ret = true;
  cleanup:
+vshTableFree(table);
 

[libvirt] [PATCH v4 0/3] vsh: Introduce new API for printing tables

[Simon Kobyda]

Created new API for priting tables, mainly to solve alignment problems.
Implemented these test to virsh list. In the future, API may be
everywhere in virsh and virt-admin.
Also wrote basic tests for the new API, and corrected tests in virshtest
which are influenced by implementation of the API in virsh list.

Changes in v4:
- fixed width calculation for zero-width, nonprintable and combined
character. (pulled some code from linux-util)
- added tests for cases mentioned above
- changed usage of vshControl variables. From now on PrintToStdout calls
PrintToString and then prints returned string to stdout

Changes in v3:
- changed encoding of 3/3 patch, otherwise it cannot be applied

Changes in v2:
- added tests
- fixed alignment for unicode character which span more spaces
- moved ncolumns check to vshTableRowAppend
- changed arguments for functions vshTablePrint, vshTablePrintToStdout,
vshTablePrintToString

Simon Kobyda (3):
  vsh: Add API for printing tables.
  virsh: Implement new table API for virsh list
  vsh: Added tests

 tests/Makefile.am|   8 +
 tests/virshtest.c|  14 +-
 tests/vshtabletest.c | 393 
 tools/Makefile.am|   4 +-
 tools/virsh-domain-monitor.c |  43 ++--
 tools/vsh-table.c| 483 +++
 tools/vsh-table.h|  42 +++
 7 files changed, 960 insertions(+), 27 deletions(-)
 create mode 100644 tests/vshtabletest.c
 create mode 100644 tools/vsh-table.c
 create mode 100644 tools/vsh-table.h

-- 
2.17.1

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

Re: [libvirt] [Qemu-devel] clean/simple Q35 support in libvirt+QEMU for guest OSes that don't support virtio-1.0

[Daniel P . Berrangé]

On Wed, Aug 22, 2018 at 12:49:48PM -0300, Eduardo Habkost wrote:

> > The thing that has really tipped my mind this way is that even
> > if we provide new device models, mgmt apps will be loathe to
> > actually use them because it will prevent live migration of
> > those guests to hosts with older libvirt.
> 
> This might be an issue for some apps, but is it going to happen
> in practice?  Don't they all need mechanisms to flip a switch and
> enable features that require newer host software, already?

That is true, but most features that get added to virt these days
are things which are opt-in for specific use cases. eg when we
added mdev support if a guest gets given an mdev VGPU it obviously
won't be migratable to older libvirt lacking mdev support. The
mitigation is that only $TINY % of guests will be using mdev, so
the compat problem won't widely affect things.

With the alternative virtio models we're discussing here, the idea
is that they'd be used by default for all new guest deployments,
so the impact will be felt on every guest.

> > So my feeling is we should do the work to enable use of Q35
> > by default in mgmt apps, for guest OS where it is known to
> > work correctly today. Every other OS should just stick with
> > i440fx as we already know that works for them today and Q35
> > doesn't offer legacy OS compelling enough benefits to switch.
> 
> I'd still prefer if libvirt provided a saner configuration
> mechanism, and let libosinfo and management apps decide what
> works best for them.
> 
> If it helps, I can send QEMU patches to make
> virtio-0.9/virtio-1.0-non-transitional/virtio-1.0-transitional
> appear as different device types.  libvirt would then be able to
> check if the device type implements "pci-express-device" or
> "conventional-pci-device", instead of adding device-specific
> placement logic.

I don't think it makes a big difference from pov of the libvirt
impl, as its just the difference between "-device virtio-net,modern=off"
and "-device virtio-net-0.9".  It still has the same amount of extra
work and complexity rippling out from libvirt to mgmt apps, to address
a problem (make old RHEL6 use Q35 instad of i440fx) that shouldn't
really need to exist.

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

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

Re: [libvirt] [Qemu-devel] clean/simple Q35 support in libvirt+QEMU for guest OSes that don't support virtio-1.0

[Eduardo Habkost]

On Wed, Aug 22, 2018 at 03:57:20PM +0100, Daniel P. Berrangé wrote:
> On Wed, Aug 22, 2018 at 11:18:28AM -0300, Eduardo Habkost wrote:
> > On Wed, Aug 22, 2018 at 02:44:40PM +0100, Daniel P. Berrangé wrote:
[...]
> > > An explicit virtio-transitional device is still two separate
> > > devices pretending to be the same thing, but magically changing
> > > their identity at runtime. We've already got that situation with
> > > existing device models, and I'm loathe to see us add 2nd device
> > > model with that same behaviour, just for sake of having a slightly
> > > different PCI bus placement strategy to support outdated guest OS.
> > 
> > Your seem to be describing what the current "virtio" device is:
> > it becomes a non-transitional (modern-only) Virtio device on some
> > cases, and becomes a transitional Virtio device on other cases.
> > It is two devices pretending to be the same thing.  That's
> > exactly what I would like applications to get rid of.
> > 
> > Now, the above is really not an accurate description of
> > transitional Virtio devices.  A transitional Virtio device is
> > something clearly specified in the Virtio spec, and it just means
> > a device that supports two types of drivers.  It's not different
> > from a x86_64 CPU that can run 32-bit OSes.
> > 
> > See:
> > http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-60001
> > http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-3090004
> 
> When I say a device pretending to be 2 different devices, I'm
> generally referring to the fact that a single QEMU device model
> can expose two different PCI device IDs depending on how it is
> configured and/or placed.

Understood.  Then you are not describing transitional Virtio, you
are just describing QEMU's disable-legacy=auto behavior (which is
the default).


> 
> > > > > Honestly though, the longer this discussion goes on, the more I think
> > > > > the answer is just "do nothing". All this time spent on discussion,
> > > > > and future time spent on implementing new logic in apps, is merely
> > > > > to support running RHEL-6 on Q35.  I think we should just say that
> > > > > RHEL-6 should use i440fx forever and be done with it.
> > > > 
> > > > I'm not sure if you are saying that we (Red Hat) shouldn't spend
> > > > time implementing it, or that the libvirt upstream project should
> > > > reject the patches if somebody implements it.  I would understand
> > > > the former, but not the latter.
> > > 
> > > Even if someone is willing to implement it in libvirt, we have to
> > > consider the cost of supporting it in both libvirt and applications
> > > using libvirt and the complexity it adds to our story about the
> > > docs / best practices for configuring guests.
> > > 
> > > Even though I do kind of like the virtio-0.9/virtio-1.0 device model
> > > as concepts, I'm yet to be convinced that implementing them in libvirt
> > > and then also in all the downstream applications (oVirt, OpenStack,
> > > virt-manager, cockpit, etc) is actually worth the cost.
> > > 
> > > There's little compelling reason to care about running outdated OS
> > > like RHEL-6 on Q35 in general. The motivation behind it is just
> > > coming from an artifically created problem downstream, by wishing
> > > to drop the i440fx machine at some still undeteremined point in the
> > > future. By the time that future comes, RHEL-6 may well even be
> > > end of life making the entire exercise a pointless.
> > 
> > I'm all for making a cost/benefit analysis, but I don't think you
> > are taking into account the costs of keeping the confusing
> > semantics of existing "virtio" devices.
> > 
> > If you still want to refuse to provide a sane way to configure
> > transitional Virtio devices, I really won't care.  But I believe
> > the interface you are trying to keep is actually the one you are
> > criticizing ("two separate devices pretending to be the same
> > thing, but magically changing their identity at runtime").
> 
> Yeah, I guess I should make a distinction between what I would
> do if it was a clean slate, and what we should do given our existing
> practice.
> 
> If we had a clean slate I would not like to see our current impl
> done.  Given that it already exists, however, we're stuck with
> that forever. So the question is whether implementing any of
> the alternative options is a net benefit for libvirt & mgmt apps
> overall. My gut feeling is that despite the downsides of the
> current impl, it is not worth trying todo something different.

Fair enough.

> 
> The thing that has really tipped my mind this way is that even
> if we provide new device models, mgmt apps will be loathe to
> actually use them because it will prevent live migration of
> those guests to hosts with older libvirt.

This might be an issue for some apps, but is it going to happen
in practice?  Don't they all need mechanisms to flip a switch and
enable features that require newer host software, already?

Re: [libvirt] [Qemu-devel] clean/simple Q35 support in libvirt+QEMU for guest OSes that don't support virtio-1.0

[Daniel P . Berrangé]

On Wed, Aug 22, 2018 at 10:37:12AM -0400, Laine Stump wrote:
> On 08/22/2018 09:44 AM, Daniel P. Berrangé wrote:
> > Even if someone is willing to implement it in libvirt, we have to
> > consider the cost of supporting it in both libvirt and applications
> > using libvirt and the complexity it adds to our story about the
> > docs / best practices for configuring guests.
> >
> > Even though I do kind of like the virtio-0.9/virtio-1.0 device model
> > as concepts, I'm yet to be convinced that implementing them in libvirt
> > and then also in all the downstream applications (oVirt, OpenStack,
> > virt-manager, cockpit, etc) is actually worth the cost.
> 
> I'm starting to lean towards this opinion too - I was thinking about
> this over the weekend, and it does seem like the code in the management
> apps will be convoluted/complex/your favorite adjective. Going into this
> I had the naive impression that a simple bit of logic in the management
> application could just take the union of supported devices from
> libosinfo(guestOS) and domaincapabilities(qemu), then pick the top model
> name from the list. It's unfortunately not that simple, so we're going
> to end up with a bunch of extra code in the management application
> (multiplied by the number of management apps, multiplied by the number
> of different virtio devices) and that code will need to be maintained.
> 
> In the meantime, the only advantages over just giving up and using 440fx
> for RHEL6 would be 1) consistent support for using Q35 on all
> "supported" guest OSes, 2) the possibility of doing SecureBoot, and 2)
> being able to someday in the future eliminate all 440fx-specific code
> from the set of code that needs to be tested/maintained by downstream
> maintainers. (1) is nice and clean, but the value is dubious if it's
> achieved by "unclean" code elsewhere. (2) is a non-feature for almost
> everyone, and (3) isn't going to happen anyway, since any existing
> guests have already been setup using 440fx as the machinetype, and we
> can't force people to change the machinetype of an existing guest (as
> Dan says, over the amount of time needed to make that an acceptable
> requirement, the guest OSes in question could likely reach EOL anyway).

Two notes

 - The notion of "supported" guest OS is an invention of downstream
   distro vendors.

 - RHEL-6 doesn't support SecureBoot at all, even on bare metal.
   IOW the possibility of SecureBoot with KVM for RHEL-6 doesn't
   even arise to begin with.

> (NB: of course if we want to require 440fx for RHEL6, we'll still need
> to do the work on libosinfo to report "supported machinetype", and on
> all the management applications to honor that information)

Yes, we still have plenty of work todo across the mgmt apps just to get
Q35 used by default in the first place.

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

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

Re: [libvirt] [Qemu-devel] clean/simple Q35 support in libvirt+QEMU for guest OSes that don't support virtio-1.0

[Daniel P . Berrangé]

On Wed, Aug 22, 2018 at 11:18:28AM -0300, Eduardo Habkost wrote:
> On Wed, Aug 22, 2018 at 02:44:40PM +0100, Daniel P. Berrangé wrote:
> > On Wed, Aug 22, 2018 at 09:54:55AM -0300, Eduardo Habkost wrote:
> > > On Wed, Aug 22, 2018 at 01:26:01PM +0100, Daniel P. Berrangé wrote:
> > > > On Wed, Aug 22, 2018 at 09:01:35AM -0300, Eduardo Habkost wrote:
> > > > > On Wed, Aug 22, 2018 at 12:36:27PM +0200, Andrea Bolognani wrote:
> > > > > > On Tue, 2018-08-21 at 14:21 -0400, Laine Stump wrote:
> > > > > > > On 08/17/2018 06:35 AM, Andrea Bolognani wrote:
> > > > > > > > If we decide we want to explicitly spell out the options instead
> > > > > > > > of relying on QEMU changing behavior based on the slot type, 
> > > > > > > > which
> > > > > > > > is probably a good idea anyway, I think we should have
> > > > > > > > 
> > > > > > > >   virtio-0.9 => disable-legacy=no,disable-modern=no
> > > > > > > >   virtio-1.0 => disable-legacy=yes,disable-modern=no
> > > > > > > > 
> > > > > > > > There's basically no reason to have a device legacy-only rather
> > > > > > > > than transitional, and spelling out both options instead of only
> > > > > > > > one of them just seems more robust.
> > > > > > > 
> > > > > > > I agree with both of those, but the counter-argument is that 
> > > > > > > "virtio"
> > > > > > > already describes a transitional device like your proposal for
> > > > > > > virtio-0.9 (at least today), and it makes the versioned models 
> > > > > > > less
> > > > > > > orthogonal. In the end, I could go either way...
> > > > > > 
> > > > > > Yeah, Dan already made that argument and convinced me that we
> > > > > > should use virtio-0.9 for legacy only, virtio-1.0 for modern only
> > > > > > and plain virtio for no enforced behavior / transitional.
> > > > > 
> > > > > I don't understand why we are optimizing the new system for the
> > > > > less useful use cases:
> > > > > 
> > > > > I don't see a use case where virtio-0.9 (legacy-only) would be
> > > > > more useful than virtio-transitional.  I don't see why anybody
> > > > > would prefer a legacy-only device instead of a transitional
> > > > > device.  Even if your guest has only legacy drivers, it might be
> > > > > upgraded and get new drivers in the future.
> > > > > 
> > > > > I don't see a use case where virtio-1.0 (modern-only) would be
> > > > > more useful than "virtio".  If you are running i440fx, you get a
> > > > > transitional device with "virtio", and I don't see why anybody
> > > > > would prefer a modern-only device.  If you are running Q35, you
> > > > > already get a modern-only device with "virtio".
> > > > > 
> > > > > The most useful feature users need is the ability to ask for a
> > > > > transitional virtio device on Q35, and this use case is
> > > > > explicitly being left out of the proposal.  Why?
> > > > 
> > > > You can already get a transitional device on Q35, albeit with manual
> > > > placement. Adding flags for magic placement for the existing devices
> > > > is not something that is suitable for the XML. The ability to get
> > > > legacy-only, or modern-only doesn't exist today in any way, so that
> > > > would be a valid new feature.
> > > 
> > > Transitional devices and modern-only devices are different kinds
> > > of devices.  Making the guest see a different type of device
> > > depending on where it's plugged is why we got into this mess, why
> > > would we recommend applications to rely on this behavior?
> > > 
> > > That's why I like your virtio-0.9/virtio-1.0 proposal.  I just
> > > don't see why you think virtio-transitional should be out of it.
> > 
> > An explicit virtio-transitional device is still two separate
> > devices pretending to be the same thing, but magically changing
> > their identity at runtime. We've already got that situation with
> > existing device models, and I'm loathe to see us add 2nd device
> > model with that same behaviour, just for sake of having a slightly
> > different PCI bus placement strategy to support outdated guest OS.
> 
> Your seem to be describing what the current "virtio" device is:
> it becomes a non-transitional (modern-only) Virtio device on some
> cases, and becomes a transitional Virtio device on other cases.
> It is two devices pretending to be the same thing.  That's
> exactly what I would like applications to get rid of.
> 
> Now, the above is really not an accurate description of
> transitional Virtio devices.  A transitional Virtio device is
> something clearly specified in the Virtio spec, and it just means
> a device that supports two types of drivers.  It's not different
> from a x86_64 CPU that can run 32-bit OSes.
> 
> See:
> http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-60001
> http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-3090004

When I say a device pretending to be 2 different devices, I'm
generally referring to the fact that a single QEMU device model
can expose two different PCI device IDs 

Re: [libvirt] [Qemu-devel] clean/simple Q35 support in libvirt+QEMU for guest OSes that don't support virtio-1.0

[Laine Stump]

On 08/22/2018 09:44 AM, Daniel P. Berrangé wrote:
> On Wed, Aug 22, 2018 at 09:54:55AM -0300, Eduardo Habkost wrote:
>> On Wed, Aug 22, 2018 at 01:26:01PM +0100, Daniel P. Berrangé wrote:
>>> On Wed, Aug 22, 2018 at 09:01:35AM -0300, Eduardo Habkost wrote:
 On Wed, Aug 22, 2018 at 12:36:27PM +0200, Andrea Bolognani wrote:
> On Tue, 2018-08-21 at 14:21 -0400, Laine Stump wrote:
>> On 08/17/2018 06:35 AM, Andrea Bolognani wrote:
>>> If we decide we want to explicitly spell out the options instead
>>> of relying on QEMU changing behavior based on the slot type, which
>>> is probably a good idea anyway, I think we should have
>>>
>>>   virtio-0.9 => disable-legacy=no,disable-modern=no
>>>   virtio-1.0 => disable-legacy=yes,disable-modern=no
>>>
>>> There's basically no reason to have a device legacy-only rather
>>> than transitional, and spelling out both options instead of only
>>> one of them just seems more robust.
>> I agree with both of those, but the counter-argument is that "virtio"
>> already describes a transitional device like your proposal for
>> virtio-0.9 (at least today), and it makes the versioned models less
>> orthogonal. In the end, I could go either way...
> Yeah, Dan already made that argument and convinced me that we
> should use virtio-0.9 for legacy only, virtio-1.0 for modern only
> and plain virtio for no enforced behavior / transitional.
 I don't understand why we are optimizing the new system for the
 less useful use cases:

 I don't see a use case where virtio-0.9 (legacy-only) would be
 more useful than virtio-transitional.  I don't see why anybody
 would prefer a legacy-only device instead of a transitional
 device.  Even if your guest has only legacy drivers, it might be
 upgraded and get new drivers in the future.

 I don't see a use case where virtio-1.0 (modern-only) would be
 more useful than "virtio".  If you are running i440fx, you get a
 transitional device with "virtio", and I don't see why anybody
 would prefer a modern-only device.  If you are running Q35, you
 already get a modern-only device with "virtio".

 The most useful feature users need is the ability to ask for a
 transitional virtio device on Q35, and this use case is
 explicitly being left out of the proposal.  Why?
>>> You can already get a transitional device on Q35, albeit with manual
>>> placement. Adding flags for magic placement for the existing devices
>>> is not something that is suitable for the XML. The ability to get
>>> legacy-only, or modern-only doesn't exist today in any way, so that
>>> would be a valid new feature.
>> Transitional devices and modern-only devices are different kinds
>> of devices.  Making the guest see a different type of device
>> depending on where it's plugged is why we got into this mess, why
>> would we recommend applications to rely on this behavior?
>>
>> That's why I like your virtio-0.9/virtio-1.0 proposal.  I just
>> don't see why you think virtio-transitional should be out of it.
> An explicit virtio-transitional device is still two separate
> devices pretending to be the same thing, but magically changing
> their identity at runtime. We've already got that situation with
> existing device models, and I'm loathe to see us add 2nd device
> model with that same behaviour, just for sake of having a slightly
> different PCI bus placement strategy to support outdated guest OS.
>
>>> Honestly though, the longer this discussion goes on, the more I think
>>> the answer is just "do nothing". All this time spent on discussion,
>>> and future time spent on implementing new logic in apps, is merely
>>> to support running RHEL-6 on Q35.  I think we should just say that
>>> RHEL-6 should use i440fx forever and be done with it.
>> I'm not sure if you are saying that we (Red Hat) shouldn't spend
>> time implementing it, or that the libvirt upstream project should
>> reject the patches if somebody implements it.  I would understand
>> the former, but not the latter.
> Even if someone is willing to implement it in libvirt, we have to
> consider the cost of supporting it in both libvirt and applications
> using libvirt and the complexity it adds to our story about the
> docs / best practices for configuring guests.
>
> Even though I do kind of like the virtio-0.9/virtio-1.0 device model
> as concepts, I'm yet to be convinced that implementing them in libvirt
> and then also in all the downstream applications (oVirt, OpenStack,
> virt-manager, cockpit, etc) is actually worth the cost.

I'm starting to lean towards this opinion too - I was thinking about
this over the weekend, and it does seem like the code in the management
apps will be convoluted/complex/your favorite adjective. Going into this
I had the naive impression that a simple bit of logic in the management
application could just take the union of supported devices from

Re: [libvirt] [Qemu-devel] clean/simple Q35 support in libvirt+QEMU for guest OSes that don't support virtio-1.0

[Eduardo Habkost]

On Wed, Aug 22, 2018 at 02:44:40PM +0100, Daniel P. Berrangé wrote:
> On Wed, Aug 22, 2018 at 09:54:55AM -0300, Eduardo Habkost wrote:
> > On Wed, Aug 22, 2018 at 01:26:01PM +0100, Daniel P. Berrangé wrote:
> > > On Wed, Aug 22, 2018 at 09:01:35AM -0300, Eduardo Habkost wrote:
> > > > On Wed, Aug 22, 2018 at 12:36:27PM +0200, Andrea Bolognani wrote:
> > > > > On Tue, 2018-08-21 at 14:21 -0400, Laine Stump wrote:
> > > > > > On 08/17/2018 06:35 AM, Andrea Bolognani wrote:
> > > > > > > If we decide we want to explicitly spell out the options instead
> > > > > > > of relying on QEMU changing behavior based on the slot type, which
> > > > > > > is probably a good idea anyway, I think we should have
> > > > > > > 
> > > > > > >   virtio-0.9 => disable-legacy=no,disable-modern=no
> > > > > > >   virtio-1.0 => disable-legacy=yes,disable-modern=no
> > > > > > > 
> > > > > > > There's basically no reason to have a device legacy-only rather
> > > > > > > than transitional, and spelling out both options instead of only
> > > > > > > one of them just seems more robust.
> > > > > > 
> > > > > > I agree with both of those, but the counter-argument is that 
> > > > > > "virtio"
> > > > > > already describes a transitional device like your proposal for
> > > > > > virtio-0.9 (at least today), and it makes the versioned models less
> > > > > > orthogonal. In the end, I could go either way...
> > > > > 
> > > > > Yeah, Dan already made that argument and convinced me that we
> > > > > should use virtio-0.9 for legacy only, virtio-1.0 for modern only
> > > > > and plain virtio for no enforced behavior / transitional.
> > > > 
> > > > I don't understand why we are optimizing the new system for the
> > > > less useful use cases:
> > > > 
> > > > I don't see a use case where virtio-0.9 (legacy-only) would be
> > > > more useful than virtio-transitional.  I don't see why anybody
> > > > would prefer a legacy-only device instead of a transitional
> > > > device.  Even if your guest has only legacy drivers, it might be
> > > > upgraded and get new drivers in the future.
> > > > 
> > > > I don't see a use case where virtio-1.0 (modern-only) would be
> > > > more useful than "virtio".  If you are running i440fx, you get a
> > > > transitional device with "virtio", and I don't see why anybody
> > > > would prefer a modern-only device.  If you are running Q35, you
> > > > already get a modern-only device with "virtio".
> > > > 
> > > > The most useful feature users need is the ability to ask for a
> > > > transitional virtio device on Q35, and this use case is
> > > > explicitly being left out of the proposal.  Why?
> > > 
> > > You can already get a transitional device on Q35, albeit with manual
> > > placement. Adding flags for magic placement for the existing devices
> > > is not something that is suitable for the XML. The ability to get
> > > legacy-only, or modern-only doesn't exist today in any way, so that
> > > would be a valid new feature.
> > 
> > Transitional devices and modern-only devices are different kinds
> > of devices.  Making the guest see a different type of device
> > depending on where it's plugged is why we got into this mess, why
> > would we recommend applications to rely on this behavior?
> > 
> > That's why I like your virtio-0.9/virtio-1.0 proposal.  I just
> > don't see why you think virtio-transitional should be out of it.
> 
> An explicit virtio-transitional device is still two separate
> devices pretending to be the same thing, but magically changing
> their identity at runtime. We've already got that situation with
> existing device models, and I'm loathe to see us add 2nd device
> model with that same behaviour, just for sake of having a slightly
> different PCI bus placement strategy to support outdated guest OS.

Your seem to be describing what the current "virtio" device is:
it becomes a non-transitional (modern-only) Virtio device on some
cases, and becomes a transitional Virtio device on other cases.
It is two devices pretending to be the same thing.  That's
exactly what I would like applications to get rid of.

Now, the above is really not an accurate description of
transitional Virtio devices.  A transitional Virtio device is
something clearly specified in the Virtio spec, and it just means
a device that supports two types of drivers.  It's not different
from a x86_64 CPU that can run 32-bit OSes.

See:
http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-60001
http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-3090004


> 
> > > Honestly though, the longer this discussion goes on, the more I think
> > > the answer is just "do nothing". All this time spent on discussion,
> > > and future time spent on implementing new logic in apps, is merely
> > > to support running RHEL-6 on Q35.  I think we should just say that
> > > RHEL-6 should use i440fx forever and be done with it.
> > 
> > I'm not sure if you are saying that we (Red Hat) shouldn't spend

Re: [libvirt] [Qemu-devel] clean/simple Q35 support in libvirt+QEMU for guest OSes that don't support virtio-1.0

[Daniel P . Berrangé]

On Wed, Aug 22, 2018 at 09:54:55AM -0300, Eduardo Habkost wrote:
> On Wed, Aug 22, 2018 at 01:26:01PM +0100, Daniel P. Berrangé wrote:
> > On Wed, Aug 22, 2018 at 09:01:35AM -0300, Eduardo Habkost wrote:
> > > On Wed, Aug 22, 2018 at 12:36:27PM +0200, Andrea Bolognani wrote:
> > > > On Tue, 2018-08-21 at 14:21 -0400, Laine Stump wrote:
> > > > > On 08/17/2018 06:35 AM, Andrea Bolognani wrote:
> > > > > > If we decide we want to explicitly spell out the options instead
> > > > > > of relying on QEMU changing behavior based on the slot type, which
> > > > > > is probably a good idea anyway, I think we should have
> > > > > > 
> > > > > >   virtio-0.9 => disable-legacy=no,disable-modern=no
> > > > > >   virtio-1.0 => disable-legacy=yes,disable-modern=no
> > > > > > 
> > > > > > There's basically no reason to have a device legacy-only rather
> > > > > > than transitional, and spelling out both options instead of only
> > > > > > one of them just seems more robust.
> > > > > 
> > > > > I agree with both of those, but the counter-argument is that "virtio"
> > > > > already describes a transitional device like your proposal for
> > > > > virtio-0.9 (at least today), and it makes the versioned models less
> > > > > orthogonal. In the end, I could go either way...
> > > > 
> > > > Yeah, Dan already made that argument and convinced me that we
> > > > should use virtio-0.9 for legacy only, virtio-1.0 for modern only
> > > > and plain virtio for no enforced behavior / transitional.
> > > 
> > > I don't understand why we are optimizing the new system for the
> > > less useful use cases:
> > > 
> > > I don't see a use case where virtio-0.9 (legacy-only) would be
> > > more useful than virtio-transitional.  I don't see why anybody
> > > would prefer a legacy-only device instead of a transitional
> > > device.  Even if your guest has only legacy drivers, it might be
> > > upgraded and get new drivers in the future.
> > > 
> > > I don't see a use case where virtio-1.0 (modern-only) would be
> > > more useful than "virtio".  If you are running i440fx, you get a
> > > transitional device with "virtio", and I don't see why anybody
> > > would prefer a modern-only device.  If you are running Q35, you
> > > already get a modern-only device with "virtio".
> > > 
> > > The most useful feature users need is the ability to ask for a
> > > transitional virtio device on Q35, and this use case is
> > > explicitly being left out of the proposal.  Why?
> > 
> > You can already get a transitional device on Q35, albeit with manual
> > placement. Adding flags for magic placement for the existing devices
> > is not something that is suitable for the XML. The ability to get
> > legacy-only, or modern-only doesn't exist today in any way, so that
> > would be a valid new feature.
> 
> Transitional devices and modern-only devices are different kinds
> of devices.  Making the guest see a different type of device
> depending on where it's plugged is why we got into this mess, why
> would we recommend applications to rely on this behavior?
> 
> That's why I like your virtio-0.9/virtio-1.0 proposal.  I just
> don't see why you think virtio-transitional should be out of it.

An explicit virtio-transitional device is still two separate
devices pretending to be the same thing, but magically changing
their identity at runtime. We've already got that situation with
existing device models, and I'm loathe to see us add 2nd device
model with that same behaviour, just for sake of having a slightly
different PCI bus placement strategy to support outdated guest OS.

> > Honestly though, the longer this discussion goes on, the more I think
> > the answer is just "do nothing". All this time spent on discussion,
> > and future time spent on implementing new logic in apps, is merely
> > to support running RHEL-6 on Q35.  I think we should just say that
> > RHEL-6 should use i440fx forever and be done with it.
> 
> I'm not sure if you are saying that we (Red Hat) shouldn't spend
> time implementing it, or that the libvirt upstream project should
> reject the patches if somebody implements it.  I would understand
> the former, but not the latter.

Even if someone is willing to implement it in libvirt, we have to
consider the cost of supporting it in both libvirt and applications
using libvirt and the complexity it adds to our story about the
docs / best practices for configuring guests.

Even though I do kind of like the virtio-0.9/virtio-1.0 device model
as concepts, I'm yet to be convinced that implementing them in libvirt
and then also in all the downstream applications (oVirt, OpenStack,
virt-manager, cockpit, etc) is actually worth the cost.

There's little compelling reason to care about running outdated OS
like RHEL-6 on Q35 in general. The motivation behind it is just
coming from an artifically created problem downstream, by wishing
to drop the i440fx machine at some still undeteremined point in the
future. By the time that future comes, RHEL-6 may well 

[libvirt] [PATCH v2 1/3] ui: remove support for GTK2 in favour of GTK3

[Daniel P . Berrangé]

GTK2 was deprecated in the 2.12.0 release with:

  commit b7715af2b31f47060cc5b4be930d16c13be93fa9
  Author: Daniel P. Berrange 
  Date:   Tue Dec 12 11:34:40 2017 +

ui: deprecate use of GTK 2.x in favour of 3.x series

The GTK 3.0 release was made in Feb, 2011:

  https://blog.gtk.org/2011/02/10/gtk-3-0-released/

That will soon be 7 years ago, which is enough time to consider
the 3.x series widely supported.

Thus we deprecate the GTK 2.x support, which will allow us to
delete it in the last release of 2018. By this time, GTK 3.x
will be almost 8 years old.

Signed-off-by: Daniel P. Berrange 
Message-id: 20171212113440.16483-1-berra...@redhat.com
Signed-off-by: Gerd Hoffmann 

It is thus able to be removed in the 3.1.0 release.

Signed-off-by: Daniel P. Berrangé 
---
 configure|  51 ++--
 include/ui/gtk.h |   9 --
 qemu-deprecated.texi |   7 --
 ui/gtk-egl.c |  10 +--
 ui/gtk.c | 192 ---
 5 files changed, 26 insertions(+), 243 deletions(-)

diff --git a/configure b/configure
index e7bddc04b0..f982d8bd61 100755
--- a/configure
+++ b/configure
@@ -453,7 +453,6 @@ glusterfs_discard="no"
 glusterfs_fallocate="no"
 glusterfs_zerofill="no"
 gtk=""
-gtkabi=""
 gtk_gl="no"
 tls_priority="NORMAL"
 gnutls=""
@@ -1369,8 +1368,6 @@ for opt do
   ;;
   --disable-pvrdma) pvrdma="no"
   ;;
-  --with-gtkabi=*) gtkabi="$optarg"
-  ;;
   --disable-vte) vte="no"
   ;;
   --enable-vte) vte="yes"
@@ -1658,7 +1655,6 @@ disabled with --disable-FEATURE, default is enabled if 
available:
   sdl SDL UI
   --with-sdlabi select preferred SDL ABI 1.2 or 2.0
   gtk gtk UI
-  --with-gtkabi select preferred GTK ABI 2.0 or 3.0
   vte vte support for the gtk UI
   curses  curses UI
   vnc VNC UI support
@@ -2648,24 +2644,9 @@ fi
 # GTK probe
 
 if test "$gtk" != "no"; then
-if test "$gtkabi" = ""; then
-# The GTK ABI was not specified explicitly, so try whether 3.0 is 
available.
-# Use 2.0 as a fallback if that is available.
-if $pkg_config --exists "gtk+-3.0 >= 3.0.0"; then
-gtkabi=3.0
-elif $pkg_config --exists "gtk+-2.0 >= 2.18.0"; then
-gtkabi=2.0
-else
-gtkabi=3.0
-fi
-fi
-gtkpackage="gtk+-$gtkabi"
-gtkx11package="gtk+-x11-$gtkabi"
-if test "$gtkabi" = "3.0" ; then
-  gtkversion="3.0.0"
-else
-  gtkversion="2.18.0"
-fi
+gtkpackage="gtk+-3.0"
+gtkx11package="gtk+-x11-3.0"
+gtkversion="3.0.0"
 if $pkg_config --exists "$gtkpackage >= $gtkversion"; then
 gtk_cflags=$($pkg_config --cflags $gtkpackage)
 gtk_libs=$($pkg_config --libs $gtkpackage)
@@ -2909,16 +2890,11 @@ fi
 # VTE probe
 
 if test "$vte" != "no"; then
-if test "$gtkabi" = "3.0"; then
-  vteminversion="0.32.0"
-  if $pkg_config --exists "vte-2.91"; then
-vtepackage="vte-2.91"
-  else
-vtepackage="vte-2.90"
-  fi
+vteminversion="0.32.0"
+if $pkg_config --exists "vte-2.91"; then
+  vtepackage="vte-2.91"
 else
-  vtepackage="vte"
-  vteminversion="0.24.0"
+  vtepackage="vte-2.90"
 fi
 if $pkg_config --exists "$vtepackage >= $vteminversion"; then
 vte_cflags=$($pkg_config --cflags $vtepackage)
@@ -2926,11 +2902,7 @@ if test "$vte" != "no"; then
 vteversion=$($pkg_config --modversion $vtepackage)
 vte="yes"
 elif test "$vte" = "yes"; then
-if test "$gtkabi" = "3.0"; then
-feature_not_found "vte" "Install libvte-2.90/2.91 devel"
-else
-feature_not_found "vte" "Install libvte devel"
-fi
+feature_not_found "vte" "Install libvte-2.90/2.91 devel"
 else
 vte="no"
 fi
@@ -6089,12 +6061,6 @@ if test "$sdl_too_old" = "yes"; then
 echo "-> Your SDL version is too old - please upgrade to have SDL support"
 fi
 
-if test "$gtkabi" = "2.0"; then
-echo
-echo "WARNING: Use of GTK 2.0 is deprecated and will be removed in"
-echo "WARNING: future releases. Please switch to using GTK 3.0"
-fi
-
 if test "$sdlabi" = "1.2"; then
 echo
 echo "WARNING: Use of SDL 1.2 is deprecated and will be removed in"
@@ -6414,7 +6380,6 @@ if test "$glib_subprocess" = "yes" ; then
 fi
 if test "$gtk" = "yes" ; then
   echo "CONFIG_GTK=m" >> $config_host_mak
-  echo "CONFIG_GTKABI=$gtkabi" >> $config_host_mak
   echo "GTK_CFLAGS=$gtk_cflags" >> $config_host_mak
   echo "GTK_LIBS=$gtk_libs" >> $config_host_mak
   if test "$gtk_gl" = "yes" ; then
diff --git a/include/ui/gtk.h b/include/ui/gtk.h
index a79780afc7..99edd3c085 100644
--- a/include/ui/gtk.h
+++ b/include/ui/gtk.h
@@ -27,15 +27,6 @@
 #include "ui/egl-context.h"
 #endif
 
-/* Compatibility define to let us build on both Gtk2 and Gtk3 */
-#if GTK_CHECK_VERSION(3, 0, 0)
-static inline void gdk_drawable_get_size(GdkWindow *w, 

[libvirt] [PATCH v2 3/3] ui: remove support for SDL1.2 in favour of SDL2

[Daniel P . Berrangé]

SDL1.2 was deprecated in the 2.12.0 release with:

  commit e52c6ba34149b4f39c3fd60e59ee32b809db2bfa
  Author: Daniel P. Berrange 
  Date:   Mon Jan 15 14:25:33 2018 +

ui: deprecate use of SDL 1.2 in favour of 2.0 series

The SDL 2.0 release was made in Aug, 2013:

  https://www.libsdl.org/release/

That will soon be 4 + 1/2 years ago, which is enough time to consider
the 2.0 series widely supported.

Thus we deprecate the SDL 1.2 support, which will allow us to delete it
in the last release of 2018. By this time, SDL 2.0 will be more than 5
years old.

Signed-off-by: Daniel P. Berrange 
Reviewed-by: Marc-André Lureau 
Message-id: 20180115142533.24585-1-berra...@redhat.com
Signed-off-by: Gerd Hoffmann 

It is thus able to be removed in the 3.1.0 release.

Signed-off-by: Daniel P. Berrangé 
---
 configure  |   60 +--
 qemu-deprecated.texi   |9 -
 ui/Makefile.objs   |5 -
 ui/sdl.c   | 1027 
 ui/sdl_zoom.c  |   93 
 ui/sdl_zoom.h  |   25 -
 ui/sdl_zoom_template.h |  219 -
 7 files changed, 7 insertions(+), 1431 deletions(-)
 delete mode 100644 ui/sdl.c
 delete mode 100644 ui/sdl_zoom.c
 delete mode 100644 ui/sdl_zoom.h
 delete mode 100644 ui/sdl_zoom_template.h

diff --git a/configure b/configure
index eca30885d6..c7728e9355 100755
--- a/configure
+++ b/configure
@@ -344,7 +344,6 @@ docs=""
 fdt=""
 netmap="no"
 sdl=""
-sdlabi=""
 virtfs=""
 mpath=""
 vnc="yes"
@@ -565,7 +564,6 @@ query_pkg_config() {
 "${pkg_config_exe}" ${QEMU_PKG_CONFIG_FLAGS} "$@"
 }
 pkg_config=query_pkg_config
-sdl_config="${SDL_CONFIG-${cross_prefix}sdl-config}"
 sdl2_config="${SDL2_CONFIG-${cross_prefix}sdl2-config}"
 
 # If the user hasn't specified ARFLAGS, default to 'rv', just as make does.
@@ -1028,8 +1026,6 @@ for opt do
   ;;
   --enable-sdl) sdl="yes"
   ;;
-  --with-sdlabi=*) sdlabi="$optarg"
-  ;;
   --disable-qom-cast-debug) qom_cast_debug="no"
   ;;
   --enable-qom-cast-debug) qom_cast_debug="yes"
@@ -1653,7 +1649,6 @@ disabled with --disable-FEATURE, default is enabled if 
available:
   nettle  nettle cryptography support
   gcrypt  libgcrypt cryptography support
   sdl SDL UI
-  --with-sdlabi select preferred SDL ABI 1.2 or 2.0
   gtk gtk UI
   vte vte support for the gtk UI
   curses  curses UI
@@ -2916,37 +2911,11 @@ fi
 
 sdl_probe ()
 {
-  sdl_too_old=no
-  if test "$sdlabi" = ""; then
-  if $pkg_config --exists "sdl2"; then
-  sdlabi=2.0
-  elif $pkg_config --exists "sdl"; then
-  sdlabi=1.2
-  else
-  sdlabi=2.0
-  fi
-  fi
-
-  if test $sdlabi = "2.0"; then
-  sdl_config=$sdl2_config
-  sdlname=sdl2
-  sdlconfigname=sdl2_config
-  elif test $sdlabi = "1.2"; then
-  sdlname=sdl
-  sdlconfigname=sdl_config
-  else
-  error_exit "Unknown sdlabi $sdlabi, must be 1.2 or 2.0"
-  fi
-
-  if test "$(basename $sdl_config)" != $sdlconfigname && ! has ${sdl_config}; 
then
-sdl_config=$sdlconfigname
-  fi
-
-  if $pkg_config $sdlname --exists; then
-sdlconfig="$pkg_config $sdlname"
+  if $pkg_config sdl2 --exists; then
+sdlconfig="$pkg_config sdl2"
 sdlversion=$($sdlconfig --modversion 2>/dev/null)
   elif has ${sdl_config}; then
-sdlconfig="$sdl_config"
+sdlconfig="$sdl2_config"
 sdlversion=$($sdlconfig --version)
   else
 if test "$sdl" = "yes" ; then
@@ -2968,8 +2937,8 @@ EOF
   sdl_cflags=$($sdlconfig --cflags 2>/dev/null)
   sdl_cflags="$sdl_cflags -Wno-undef"  # workaround 2.0.8 bug
   if test "$static" = "yes" ; then
-if $pkg_config $sdlname --exists; then
-  sdl_libs=$($pkg_config $sdlname --static --libs 2>/dev/null)
+if $pkg_config sdl2 --exists; then
+  sdl_libs=$($pkg_config sdl2 --static --libs 2>/dev/null)
 else
   sdl_libs=$($sdlconfig --static-libs 2>/dev/null)
 fi
@@ -2977,11 +2946,7 @@ EOF
 sdl_libs=$($sdlconfig --libs 2>/dev/null)
   fi
   if compile_prog "$sdl_cflags" "$sdl_libs" ; then
-if test $(echo $sdlversion | sed 's/[^0-9]//g') -lt 121 ; then
-  sdl_too_old=yes
-else
-  sdl=yes
-fi
+sdl=yes
 
 # static link with sdl ? (note: sdl.pc's --static --libs is broken)
 if test "$sdl" = "yes" -a "$static" = "yes" ; then
@@ -2997,7 +2962,7 @@ EOF
 fi # static link
   else # sdl not found
 if test "$sdl" = "yes" ; then
-  feature_not_found "sdl" "Install SDL devel"
+  feature_not_found "sdl" "Install SDL2 devel"
 fi
 sdl=no
   fi # sdl compile test
@@ -6057,16 +6022,6 @@ echo "capstone  $capstone"
 echo "docker$docker"
 echo "libpmem support   $libpmem"
 
-if test "$sdl_too_old" = "yes"; then
-echo "-> Your SDL version is too old - please upgrade to have SDL support"
-fi
-
-if test "$sdlabi" = "1.2"; then
-echo
-echo "WARNING: Use of SDL 1.2 is deprecated and will be removed in"
- 

[libvirt] [PATCH v2 2/3] ui: increase min required GTK3 version to 3.14.0

[Daniel P . Berrangé]

Per supported platforms doc[1], the various min GTK3 on relevant distros is:

  RHEL-7.0: 3.8.8
  RHEL-7.2: 3.14.13
  RHEL-7.4: 3.22.10
  RHEL-7.5: 3.22.26
  Debian (Stretch): 3.22.11
  Debian (Jessie): 3.14.5
  OpenBSD (Ports): 3.22.30
  FreeBSD (Ports): 3.22.29
  OpenSUSE Leap 15: 3.22.30
  SLE12-SP2: Unknown
  Ubuntu (Xenial): 3.18.9
  macOS (Homebrew): 3.22.30

This suggests that a minimum GTK3 of 3.14.0 is a reasonable target,
as users are unlikely to be stuck on RHEL-7.0/7.1 still

[1] https://qemu.weilnetz.de/doc/qemu-doc.html#Supported-build-platforms

Signed-off-by: Daniel P. Berrangé 
---
 configure |  2 +-
 ui/gtk.c  | 10 --
 2 files changed, 1 insertion(+), 11 deletions(-)

diff --git a/configure b/configure
index f982d8bd61..eca30885d6 100755
--- a/configure
+++ b/configure
@@ -2646,7 +2646,7 @@ fi
 if test "$gtk" != "no"; then
 gtkpackage="gtk+-3.0"
 gtkx11package="gtk+-x11-3.0"
-gtkversion="3.0.0"
+gtkversion="3.14.0"
 if $pkg_config --exists "$gtkpackage >= $gtkversion"; then
 gtk_cflags=$($pkg_config --cflags $gtkpackage)
 gtk_libs=$($pkg_config --libs $gtkpackage)
diff --git a/ui/gtk.c b/ui/gtk.c
index 9d09bc9e57..6d57558084 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -998,7 +998,6 @@ static gboolean gd_scroll_event(GtkWidget *widget, 
GdkEventScroll *scroll,
 btn = INPUT_BUTTON_WHEEL_UP;
 } else if (scroll->direction == GDK_SCROLL_DOWN) {
 btn = INPUT_BUTTON_WHEEL_DOWN;
-#if GTK_CHECK_VERSION(3, 4, 0)
 } else if (scroll->direction == GDK_SCROLL_SMOOTH) {
 gdouble delta_x, delta_y;
 if (!gdk_event_get_scroll_deltas((GdkEvent *)scroll,
@@ -1010,7 +1009,6 @@ static gboolean gd_scroll_event(GtkWidget *widget, 
GdkEventScroll *scroll,
 } else {
 btn = INPUT_BUTTON_WHEEL_UP;
 }
-#endif
 } else {
 return TRUE;
 }
@@ -1672,11 +1670,9 @@ static GSList *gd_vc_menu_init(GtkDisplayState *s, 
VirtualConsole *vc,
 gtk_accel_group_connect(s->accel_group, GDK_KEY_1 + idx,
 HOTKEY_MODIFIERS, 0,
 g_cclosure_new_swap(G_CALLBACK(gd_accel_switch_vc), vc, NULL));
-#if GTK_CHECK_VERSION(3, 8, 0)
 gtk_accel_label_set_accel(
 GTK_ACCEL_LABEL(gtk_bin_get_child(GTK_BIN(vc->menu_item))),
 GDK_KEY_1 + idx, HOTKEY_MODIFIERS);
-#endif
 
 g_signal_connect(vc->menu_item, "activate",
  G_CALLBACK(gd_menu_switch_vc), s);
@@ -2167,11 +2163,9 @@ static GtkWidget *gd_create_menu_view(GtkDisplayState *s)
TRUE);
 gtk_accel_group_connect(s->accel_group, GDK_KEY_m, HOTKEY_MODIFIERS, 0,
 g_cclosure_new_swap(G_CALLBACK(gd_accel_show_menubar), s, NULL));
-#if GTK_CHECK_VERSION(3, 8, 0)
 gtk_accel_label_set_accel(
 GTK_ACCEL_LABEL(gtk_bin_get_child(GTK_BIN(s->show_menubar_item))),
 GDK_KEY_m, HOTKEY_MODIFIERS);
-#endif
 gtk_menu_shell_append(GTK_MENU_SHELL(view_menu), s->show_menubar_item);
 
 return view_menu;
@@ -2221,11 +2215,7 @@ static void gtk_display_init(DisplayState *ds, 
DisplayOptions *opts)
 s->opts = opts;
 
 s->window = gtk_window_new(GTK_WINDOW_TOPLEVEL);
-#if GTK_CHECK_VERSION(3, 2, 0)
 s->vbox = gtk_box_new(GTK_ORIENTATION_VERTICAL, 0);
-#else
-s->vbox = gtk_vbox_new(FALSE, 0);
-#endif
 s->notebook = gtk_notebook_new();
 s->menu_bar = gtk_menu_bar_new();
 
-- 
2.17.1

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

[libvirt] [PATCH v2 0/3] ui: remove deprecated UI frontends

[Daniel P . Berrangé]

We deprecated GTK2 and SDL1.2 in the 2.12.0 release, so they are able to
be removed entirely in the 3.1.0 release. The min GTK3 version can also
be bumped up based the distros we aim to support.

Note that before this can merge, the openbsd VM test image needs to be
updated to have SDL2, as openbsd build mandates SDL availability.

The freebsd & netbsd images should also be updated, but they are not
build blockers as they automatically disable SDL

Changed in v2:

 - Rebased to resolve conflicts

Daniel P. Berrangé (3):
  ui: remove support for GTK2 in favour of GTK3
  ui: increase min required GTK3 version to 3.14.0
  ui: remove support for SDL1.2 in favour of SDL2

 configure  |  111 +
 include/ui/gtk.h   |9 -
 qemu-deprecated.texi   |   16 -
 ui/Makefile.objs   |5 -
 ui/gtk-egl.c   |   10 +-
 ui/gtk.c   |  202 +---
 ui/sdl.c   | 1027 
 ui/sdl_zoom.c  |   93 
 ui/sdl_zoom.h  |   25 -
 ui/sdl_zoom_template.h |  219 -
 10 files changed, 33 insertions(+), 1684 deletions(-)
 delete mode 100644 ui/sdl.c
 delete mode 100644 ui/sdl_zoom.c
 delete mode 100644 ui/sdl_zoom.h
 delete mode 100644 ui/sdl_zoom_template.h

-- 
2.17.1

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

Re: [libvirt] [Qemu-devel] clean/simple Q35 support in libvirt+QEMU for guest OSes that don't support virtio-1.0

[Eduardo Habkost]

On Wed, Aug 22, 2018 at 01:26:01PM +0100, Daniel P. Berrangé wrote:
> On Wed, Aug 22, 2018 at 09:01:35AM -0300, Eduardo Habkost wrote:
> > On Wed, Aug 22, 2018 at 12:36:27PM +0200, Andrea Bolognani wrote:
> > > On Tue, 2018-08-21 at 14:21 -0400, Laine Stump wrote:
> > > > On 08/17/2018 06:35 AM, Andrea Bolognani wrote:
> > > > > If we decide we want to explicitly spell out the options instead
> > > > > of relying on QEMU changing behavior based on the slot type, which
> > > > > is probably a good idea anyway, I think we should have
> > > > > 
> > > > >   virtio-0.9 => disable-legacy=no,disable-modern=no
> > > > >   virtio-1.0 => disable-legacy=yes,disable-modern=no
> > > > > 
> > > > > There's basically no reason to have a device legacy-only rather
> > > > > than transitional, and spelling out both options instead of only
> > > > > one of them just seems more robust.
> > > > 
> > > > I agree with both of those, but the counter-argument is that "virtio"
> > > > already describes a transitional device like your proposal for
> > > > virtio-0.9 (at least today), and it makes the versioned models less
> > > > orthogonal. In the end, I could go either way...
> > > 
> > > Yeah, Dan already made that argument and convinced me that we
> > > should use virtio-0.9 for legacy only, virtio-1.0 for modern only
> > > and plain virtio for no enforced behavior / transitional.
> > 
> > I don't understand why we are optimizing the new system for the
> > less useful use cases:
> > 
> > I don't see a use case where virtio-0.9 (legacy-only) would be
> > more useful than virtio-transitional.  I don't see why anybody
> > would prefer a legacy-only device instead of a transitional
> > device.  Even if your guest has only legacy drivers, it might be
> > upgraded and get new drivers in the future.
> > 
> > I don't see a use case where virtio-1.0 (modern-only) would be
> > more useful than "virtio".  If you are running i440fx, you get a
> > transitional device with "virtio", and I don't see why anybody
> > would prefer a modern-only device.  If you are running Q35, you
> > already get a modern-only device with "virtio".
> > 
> > The most useful feature users need is the ability to ask for a
> > transitional virtio device on Q35, and this use case is
> > explicitly being left out of the proposal.  Why?
> 
> You can already get a transitional device on Q35, albeit with manual
> placement. Adding flags for magic placement for the existing devices
> is not something that is suitable for the XML. The ability to get
> legacy-only, or modern-only doesn't exist today in any way, so that
> would be a valid new feature.

Transitional devices and modern-only devices are different kinds
of devices.  Making the guest see a different type of device
depending on where it's plugged is why we got into this mess, why
would we recommend applications to rely on this behavior?

That's why I like your virtio-0.9/virtio-1.0 proposal.  I just
don't see why you think virtio-transitional should be out of it.

> 
> Honestly though, the longer this discussion goes on, the more I think
> the answer is just "do nothing". All this time spent on discussion,
> and future time spent on implementing new logic in apps, is merely
> to support running RHEL-6 on Q35.  I think we should just say that
> RHEL-6 should use i440fx forever and be done with it.

I'm not sure if you are saying that we (Red Hat) shouldn't spend
time implementing it, or that the libvirt upstream project should
reject the patches if somebody implements it.  I would understand
the former, but not the latter.

-- 
Eduardo

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

Re: [libvirt] [jenkins-ci PATCH v3 02/12] jobs: Remove archive_format from defaults

[Erik Skultety]

On Wed, Aug 22, 2018 at 01:12:58PM +0200, Andrea Bolognani wrote:
> On Wed, 2018-08-22 at 12:37 +0200, Erik Skultety wrote:
> > On Wed, Aug 22, 2018 at 11:44:17AM +0200, Andrea Bolognani wrote:
> > > Instead of defining a default and overriding it on a
> > > case-by-case basis, always define archive_format at the
> > > project level.
> > >
> > > This adds a bit of duplication, but it's consistent with
> > > how we define other metadata for projects and it will
> > > help us out later.
> >
> > How is it going to help us later?
>
> Welp, I should definitely have expanded on those "will help us out
> later" bits O:-)
>
> > So here you move something which is perfectly
> > fine to have in defaults and override it only when necessary and in the
> > following patch you're making a preparation changes for essentially the
> > opposite of this - moving autogen_args and similar to defaults (not that any
> > template needs to override these at the moment, but I'm getting confused by 
> > the
> > consistency you talk about).
>
> The ultimate goal is to translate the JJB jobs definitions to
> Ansible tasks while keeping the two as close as possible so that
> further changes can be applied pretty much verbatim to both and
> not make maintainance a nightmare.
>
> Problem is, some JJB constructs are awfully difficult to translate
> without adding extra boilerplate, so in those cases I opted for
> moving to a different construct instead.
>
> So for example autogen_args can be defined as a default because
> every time we need to override it we can use
>
>   - autotools-build-job:
>   autogen_args: --enable-gtk-doc
>
> which translates quite naturally to
>
>   - include: '{{ playbook_base }}/jobs/autotools-build-job.yml'
> vars:
>   autogen_args: --enable-gtk-doc
>
> since variables passed to an included task are limited in scope
> to the included task and don't affect any other module call, but
> for archive_format we can't do the same because we want to
> translate
>
>   - project:
>   name: libvirt-dbus
>   archive_format: xz
>
> to
>
>   - set_fact:
>   name: libvirt-dbus
>   archive_format: xz
>
> and set_fact affects the *global* state, which means that it will
> overwrite the default every time it is called and subsequent tasks
> might break depending on the order they're called in.

Understood.

>
> I guess I could create a task that takes care of stashing the

Nah, that would just cause more confusion. I'd appreciate if you managed to
put a much condensed version of your explanation into the commit message for
future generations :).

Reviewed-by: Erik Skultety 

> existing defaults before overriding them with the
> project-appropriate ones, and another one that restores the
> previous values after running all tasks for a project, but as I
> said that's extra boilerplate that we can avoid thanks to this
> patch.
>
> (As an aside, I actually like having archive_format spelled out
>  explicitly along with other project metadata such as name, title
>  and git_url: it makes sense to me to have that kind of information
>  all in one place. But the above is the actual reason why this
>  patch is needed.)
>
> --
> Andrea Bolognani / Red Hat / Virtualization
>

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

Re: [libvirt] [Qemu-devel] clean/simple Q35 support in libvirt+QEMU for guest OSes that don't support virtio-1.0

[Daniel P . Berrangé]

On Wed, Aug 22, 2018 at 09:01:35AM -0300, Eduardo Habkost wrote:
> On Wed, Aug 22, 2018 at 12:36:27PM +0200, Andrea Bolognani wrote:
> > On Tue, 2018-08-21 at 14:21 -0400, Laine Stump wrote:
> > > On 08/17/2018 06:35 AM, Andrea Bolognani wrote:
> > > > If we decide we want to explicitly spell out the options instead
> > > > of relying on QEMU changing behavior based on the slot type, which
> > > > is probably a good idea anyway, I think we should have
> > > > 
> > > >   virtio-0.9 => disable-legacy=no,disable-modern=no
> > > >   virtio-1.0 => disable-legacy=yes,disable-modern=no
> > > > 
> > > > There's basically no reason to have a device legacy-only rather
> > > > than transitional, and spelling out both options instead of only
> > > > one of them just seems more robust.
> > > 
> > > I agree with both of those, but the counter-argument is that "virtio"
> > > already describes a transitional device like your proposal for
> > > virtio-0.9 (at least today), and it makes the versioned models less
> > > orthogonal. In the end, I could go either way...
> > 
> > Yeah, Dan already made that argument and convinced me that we
> > should use virtio-0.9 for legacy only, virtio-1.0 for modern only
> > and plain virtio for no enforced behavior / transitional.
> 
> I don't understand why we are optimizing the new system for the
> less useful use cases:
> 
> I don't see a use case where virtio-0.9 (legacy-only) would be
> more useful than virtio-transitional.  I don't see why anybody
> would prefer a legacy-only device instead of a transitional
> device.  Even if your guest has only legacy drivers, it might be
> upgraded and get new drivers in the future.
> 
> I don't see a use case where virtio-1.0 (modern-only) would be
> more useful than "virtio".  If you are running i440fx, you get a
> transitional device with "virtio", and I don't see why anybody
> would prefer a modern-only device.  If you are running Q35, you
> already get a modern-only device with "virtio".
> 
> The most useful feature users need is the ability to ask for a
> transitional virtio device on Q35, and this use case is
> explicitly being left out of the proposal.  Why?

You can already get a transitional device on Q35, albeit with manual
placement. Adding flags for magic placement for the existing devices
is not something that is suitable for the XML. The ability to get
legacy-only, or modern-only doesn't exist today in any way, so that
would be a valid new feature.

Honestly though, the longer this discussion goes on, the more I think
the answer is just "do nothing". All this time spent on discussion,
and future time spent on implementing new logic in apps, is merely
to support running RHEL-6 on Q35.  I think we should just say that
RHEL-6 should use i440fx forever and be done with it.

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

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

Re: [libvirt] [Qemu-devel] clean/simple Q35 support in libvirt+QEMU for guest OSes that don't support virtio-1.0

[Eduardo Habkost]

On Wed, Aug 22, 2018 at 12:36:27PM +0200, Andrea Bolognani wrote:
> On Tue, 2018-08-21 at 14:21 -0400, Laine Stump wrote:
> > On 08/17/2018 06:35 AM, Andrea Bolognani wrote:
> > > If we decide we want to explicitly spell out the options instead
> > > of relying on QEMU changing behavior based on the slot type, which
> > > is probably a good idea anyway, I think we should have
> > > 
> > >   virtio-0.9 => disable-legacy=no,disable-modern=no
> > >   virtio-1.0 => disable-legacy=yes,disable-modern=no
> > > 
> > > There's basically no reason to have a device legacy-only rather
> > > than transitional, and spelling out both options instead of only
> > > one of them just seems more robust.
> > 
> > I agree with both of those, but the counter-argument is that "virtio"
> > already describes a transitional device like your proposal for
> > virtio-0.9 (at least today), and it makes the versioned models less
> > orthogonal. In the end, I could go either way...
> 
> Yeah, Dan already made that argument and convinced me that we
> should use virtio-0.9 for legacy only, virtio-1.0 for modern only
> and plain virtio for no enforced behavior / transitional.

I don't understand why we are optimizing the new system for the
less useful use cases:

I don't see a use case where virtio-0.9 (legacy-only) would be
more useful than virtio-transitional.  I don't see why anybody
would prefer a legacy-only device instead of a transitional
device.  Even if your guest has only legacy drivers, it might be
upgraded and get new drivers in the future.

I don't see a use case where virtio-1.0 (modern-only) would be
more useful than "virtio".  If you are running i440fx, you get a
transitional device with "virtio", and I don't see why anybody
would prefer a modern-only device.  If you are running Q35, you
already get a modern-only device with "virtio".

The most useful feature users need is the ability to ask for a
transitional virtio device on Q35, and this use case is
explicitly being left out of the proposal.  Why?

-- 
Eduardo

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

Re: [libvirt] [PATCH 0/3] qemu: Fix few issues from the -blockdev series

[John Ferlan]

On 08/22/2018 07:27 AM, Peter Krempa wrote:
> All three should appease coverity.

That they do...

> 
> Peter Krempa (3):
>   qemu: hotplug: Don't generate alias when detaching controllers
>   qemu: hotplug: Don't leak 'nodename' in qemuDomainChangeMediaBlockdev
>   qemu: monitor: Fix device matching in
> qemuMonitorJSONBlockIoThrottleInfo
> 
>  src/qemu/qemu_hotplug.c  | 6 +-
>  src/qemu/qemu_monitor_json.c | 4 ++--
>  2 files changed, 3 insertions(+), 7 deletions(-)
> 

Reviewed-by: John Ferlan 

John

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

Re: [libvirt] [PATCH] ppc: Remove deprecated ppcemb target

[David Gibson]

On Tue, Aug 21, 2018 at 01:27:48PM +0200, Thomas Huth wrote:
> There is no known available OS for ppc around anymore that uses page
> sizes below 4k, so it does not make much sense that we keep wasting
> our time on building and testing the ppcemb-softmmu target. It has
> been deprecated since two releases, and nobody complained, so let's
> remove this now.
> 
> Signed-off-by: Thomas Huth 

Applied, thanks.

> ---
>  configure  | 13 +++--
>  cpus.c |  1 -
>  default-configs/ppcemb-softmmu.mak | 23 ---
>  hw/ppc/ppc405_boards.c | 14 --
>  hw/ppc/ppc440_bamboo.c |  7 ---
>  hw/ppc/sam460ex.c  |  7 ---
>  hw/ppc/virtex_ml507.c  |  7 ---
>  include/exec/poison.h  |  1 -
>  qapi/common.json   |  2 +-
>  qemu-deprecated.texi   |  6 --
>  target/ppc/cpu-qom.h   |  2 --
>  target/ppc/cpu.h   | 16 
>  target/ppc/kvm.c   |  4 +---
>  target/ppc/mmu_helper.c|  6 +++---
>  target/ppc/translate_init.inc.c| 35 +--
>  tests/machine-none-test.c  |  1 -
>  16 files changed, 9 insertions(+), 136 deletions(-)
>  delete mode 100644 default-configs/ppcemb-softmmu.mak
> 
> diff --git a/configure b/configure
> index e7bddc0..0fc55eb 100755
> --- a/configure
> +++ b/configure
> @@ -195,8 +195,7 @@ supported_kvm_target() {
>  i386:i386 | i386:x86_64 | i386:x32 | \
>  x86_64:i386 | x86_64:x86_64 | x86_64:x32 | \
>  mips:mips | mipsel:mips | \
> -ppc:ppc | ppcemb:ppc | ppc64:ppc | \
> -ppc:ppc64 | ppcemb:ppc64 | ppc64:ppc64 | \
> +ppc:ppc | ppc64:ppc | ppc:ppc64 | ppc64:ppc64 | \
>  s390x:s390x)
>  return 0
>  ;;
> @@ -6935,7 +6934,7 @@ if test "$linux" = "yes" ; then
>i386|x86_64|x32)
>  linux_arch=x86
>  ;;
> -  ppcemb|ppc|ppc64)
> +  ppc|ppc64)
>  linux_arch=powerpc
>  ;;
>s390x)
> @@ -6965,7 +6964,7 @@ target_name=$(echo $target | cut -d '-' -f 1)
>  target_bigendian="no"
>  
>  case "$target_name" in
> -  
> armeb|aarch64_be|hppa|lm32|m68k|microblaze|mips|mipsn32|mips64|moxie|or1k|ppc|ppcemb|ppc64|ppc64abi32|s390x|sh4eb|sparc|sparc64|sparc32plus|xtensaeb)
> +  
> armeb|aarch64_be|hppa|lm32|m68k|microblaze|mips|mipsn32|mips64|moxie|or1k|ppc|ppc64|ppc64abi32|s390x|sh4eb|sparc|sparc64|sparc32plus|xtensaeb)
>target_bigendian=yes
>;;
>  esac
> @@ -7093,12 +7092,6 @@ case "$target_name" in
>  gdb_xml_files="power-core.xml power-fpu.xml power-altivec.xml 
> power-spe.xml"
>  target_compiler=$cross_cc_powerpc
>;;
> -  ppcemb)
> -TARGET_BASE_ARCH=ppc
> -TARGET_ABI_DIR=ppc
> -gdb_xml_files="power-core.xml power-fpu.xml power-altivec.xml 
> power-spe.xml"
> -target_compiler=$cross_cc_ppcemb
> -  ;;
>ppc64)
>  TARGET_BASE_ARCH=ppc
>  TARGET_ABI_DIR=ppc
> diff --git a/cpus.c b/cpus.c
> index b5844b7..80ca683 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -2203,7 +2203,6 @@ static CpuInfoArch 
> sysemu_target_to_cpuinfo_arch(SysEmuTarget target)
>  return CPU_INFO_ARCH_X86;
>  
>  case SYS_EMU_TARGET_PPC:
> -case SYS_EMU_TARGET_PPCEMB:
>  case SYS_EMU_TARGET_PPC64:
>  return CPU_INFO_ARCH_PPC;
>  
> diff --git a/default-configs/ppcemb-softmmu.mak 
> b/default-configs/ppcemb-softmmu.mak
> deleted file mode 100644
> index ac44f15..000
> --- a/default-configs/ppcemb-softmmu.mak
> +++ /dev/null
> @@ -1,23 +0,0 @@
> -# Default configuration for ppcemb-softmmu
> -
> -include pci.mak
> -include sound.mak
> -include usb.mak
> -CONFIG_PPC4XX=y
> -CONFIG_M48T59=y
> -CONFIG_SERIAL=y
> -CONFIG_SERIAL_ISA=y
> -CONFIG_I8257=y
> -CONFIG_OPENPIC=y
> -CONFIG_PFLASH_CFI01=y
> -CONFIG_PFLASH_CFI02=y
> -CONFIG_PTIMER=y
> -CONFIG_I8259=y
> -CONFIG_XILINX=y
> -CONFIG_XILINX_ETHLITE=y
> -CONFIG_USB_EHCI_SYSBUS=y
> -CONFIG_SM501=y
> -CONFIG_DDC=y
> -CONFIG_IDE_SII3112=y
> -CONFIG_I2C=y
> -CONFIG_BITBANG_I2C=y
> diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
> index 7011107..3a54d52 100644
> --- a/hw/ppc/ppc405_boards.c
> +++ b/hw/ppc/ppc405_boards.c
> @@ -202,13 +202,6 @@ static void ref405ep_init(MachineState *machine)
>  DriveInfo *dinfo;
>  MemoryRegion *sysmem = get_system_memory();
>  
> -#ifdef TARGET_PPCEMB
> -if (!qtest_enabled()) {
> -warn_report("qemu-system-ppcemb is deprecated, "
> -"please use qemu-system-ppc instead.");
> -}
> -#endif
> -
>  /* XXX: fix this */
>  memory_region_allocate_system_memory(_memories[0], NULL, 
> "ef405ep.ram",
>   0x0800);
> @@ -503,13 +496,6 @@ static void taihu_405ep_init(MachineState *machine)
>  int fl_idx, fl_sectors;
>  DriveInfo *dinfo;
>  
> -#ifdef TARGET_PPCEMB
> -if (!qtest_enabled()) {
> -

[libvirt] [PATCH 3/3] qemu: monitor: Fix device matching in qemuMonitorJSONBlockIoThrottleInfo

[Peter Krempa]

We should compare the alias/qdev id only when it was provided by the
caller and when it was found in the reply. Otherwise we could
dereference a NULL pointer. STRNEQ_NULLABLE is not appropriate since
it would return 'true' if the string was not present in the JSON output.

Found by Coverity.

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

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 5cd38710e5..ddfa261743 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -5001,8 +5001,8 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr 
io_throttle,
 goto cleanup;
 }

-if ((drivealias && STRNEQ(current_drive, drivealias)) ||
-(qdevid && STRNEQ(current_qdev, qdevid)))
+if ((drivealias && current_drive && STRNEQ(current_drive, drivealias)) 
||
+(qdevid && current_qdev && STRNEQ(current_qdev, qdevid)))
 continue;

 found = true;
-- 
2.16.2

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

[libvirt] [PATCH 1/3] qemu: hotplug: Don't generate alias when detaching controllers

[Peter Krempa]

qemuDomainDetachControllerDevice contained code which implied that alias
might be NULL when detaching the disk and tried to generate it. This is
no longer possible so we can remove the code.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_hotplug.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index bd1942efe7..1a9a9e1a32 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5281,11 +5281,6 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr 
driver,
 goto cleanup;
 }

-if (!detach->info.alias) {
-if (qemuAssignDeviceControllerAlias(vm->def, priv->qemuCaps, detach) < 
0)
-goto cleanup;
-}
-
 if (!async)
 qemuDomainMarkDeviceForRemoval(vm, >info);

-- 
2.16.2

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

[libvirt] [PATCH 2/3] qemu: hotplug: Don't leak 'nodename' in qemuDomainChangeMediaBlockdev

[Peter Krempa]

qemuDomainDiskGetBackendAlias allocates a copy of the nodename string so
we need to free it at the end.

Found by Coverity.

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

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 1a9a9e1a32..0b84a503bb 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -700,6 +700,7 @@ qemuDomainChangeMediaBlockdev(virQEMUDriverPtr driver,
  cleanup:
 qemuHotplugDiskSourceDataFree(newbackend);
 qemuHotplugDiskSourceDataFree(oldbackend);
+VIR_FREE(nodename);
 /* caller handles correct exchange of sources */
 disk->src = oldsrc;
 return ret;
-- 
2.16.2

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

[libvirt] [PATCH 0/3] qemu: Fix few issues from the -blockdev series

[Peter Krempa]

All three should appease coverity.

Peter Krempa (3):
  qemu: hotplug: Don't generate alias when detaching controllers
  qemu: hotplug: Don't leak 'nodename' in qemuDomainChangeMediaBlockdev
  qemu: monitor: Fix device matching in
qemuMonitorJSONBlockIoThrottleInfo

 src/qemu/qemu_hotplug.c  | 6 +-
 src/qemu/qemu_monitor_json.c | 4 ++--
 2 files changed, 3 insertions(+), 7 deletions(-)

-- 
2.16.2

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

Re: [libvirt] [jenkins-ci PATCH v3 02/12] jobs: Remove archive_format from defaults

[Andrea Bolognani]

On Wed, 2018-08-22 at 12:37 +0200, Erik Skultety wrote:
> On Wed, Aug 22, 2018 at 11:44:17AM +0200, Andrea Bolognani wrote:
> > Instead of defining a default and overriding it on a
> > case-by-case basis, always define archive_format at the
> > project level.
> > 
> > This adds a bit of duplication, but it's consistent with
> > how we define other metadata for projects and it will
> > help us out later.
> 
> How is it going to help us later?

Welp, I should definitely have expanded on those "will help us out
later" bits O:-)

> So here you move something which is perfectly
> fine to have in defaults and override it only when necessary and in the
> following patch you're making a preparation changes for essentially the
> opposite of this - moving autogen_args and similar to defaults (not that any
> template needs to override these at the moment, but I'm getting confused by 
> the
> consistency you talk about).

The ultimate goal is to translate the JJB jobs definitions to
Ansible tasks while keeping the two as close as possible so that
further changes can be applied pretty much verbatim to both and
not make maintainance a nightmare.

Problem is, some JJB constructs are awfully difficult to translate
without adding extra boilerplate, so in those cases I opted for
moving to a different construct instead.

So for example autogen_args can be defined as a default because
every time we need to override it we can use

  - autotools-build-job:
  autogen_args: --enable-gtk-doc

which translates quite naturally to

  - include: '{{ playbook_base }}/jobs/autotools-build-job.yml'
vars:
  autogen_args: --enable-gtk-doc

since variables passed to an included task are limited in scope
to the included task and don't affect any other module call, but
for archive_format we can't do the same because we want to
translate

  - project:
  name: libvirt-dbus
  archive_format: xz

to

  - set_fact:
  name: libvirt-dbus
  archive_format: xz

and set_fact affects the *global* state, which means that it will
overwrite the default every time it is called and subsequent tasks
might break depending on the order they're called in.

I guess I could create a task that takes care of stashing the
existing defaults before overriding them with the
project-appropriate ones, and another one that restores the
previous values after running all tasks for a project, but as I
said that's extra boilerplate that we can avoid thanks to this
patch.

(As an aside, I actually like having archive_format spelled out
 explicitly along with other project metadata such as name, title
 and git_url: it makes sense to me to have that kind of information
 all in one place. But the above is the actual reason why this
 patch is needed.)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCHv2 12/62] qemu: monitor: Allow using 'qdev' instead of 'device' for getting disk throttling

[Peter Krempa]

On Wed, Aug 22, 2018 at 06:47:45 -0400, John Ferlan wrote:
> [...]
> 
> > --- a/src/qemu/qemu_monitor_json.c
> > +++ b/src/qemu/qemu_monitor_json.c
> > @@ -4818,7 +4818,8 @@ int qemuMonitorJSONOpenGraphics(qemuMonitorPtr mon,
> >  }
> >  static int
> >  qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr io_throttle,
> > -   const char *device,
> > +   const char *drivealias,
> > +   const char *qdevid,
> > virDomainBlockIoTuneInfoPtr reply)
> >  {
> >  int ret = -1;
> > @@ -4828,7 +4829,8 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr 
> > io_throttle,
> >  for (i = 0; i < virJSONValueArraySize(io_throttle); i++) {
> >  virJSONValuePtr temp_dev = virJSONValueArrayGet(io_throttle, i);
> >  virJSONValuePtr inserted;
> > -const char *current_dev;
> > +const char *current_drive;
> > +const char *current_qdev;
> > 
> >  if (!temp_dev || virJSONValueGetType(temp_dev) != 
> > VIR_JSON_TYPE_OBJECT) {
> >  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > @@ -4837,14 +4839,18 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr 
> > io_throttle,
> >  goto cleanup;
> >  }
> > 
> > -if (!(current_dev = virJSONValueObjectGetString(temp_dev, 
> > "device"))) {
> > +current_qdev = virJSONValueObjectGetString(temp_dev, "qdev");
> > +current_drive = virJSONValueObjectGetString(temp_dev, "device");
> > +
> > +if (!current_drive && !current_qdev) {
> 
> Is this supposed to be || ?

No that would break old qemus

> 
> >  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > _("block_io_throttle device entry "
> >   "was not in expected format"));
> >  goto cleanup;
> >  }
> > 
> > -if (STRNEQ(current_dev, device))
> > +if ((drivealias && STRNEQ(current_drive, drivealias)) ||
> > +(qdevid && STRNEQ(current_qdev, qdevid)))
> 
> Because Coverity complains here that if one or the other is == NULL
> while the other != NULL, then strcmp is not going to be happy.

Ah, yes. I'll change these to the NULLABLE variant.


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

Re: [libvirt] [jenkins-ci PATCH v3 01/12] jobs: Rename git-url -> git_url

[Erik Skultety]

On Wed, Aug 22, 2018 at 12:45:19PM +0200, Andrea Bolognani wrote:
> On Wed, 2018-08-22 at 12:19 +0200, Erik Skultety wrote:
> > On Wed, Aug 22, 2018 at 11:44:16AM +0200, Andrea Bolognani wrote:
> > > Other user-defined variables use underscores as
> > > separator instead of dashes; this change will also
> > > help us out later.
> >
> > There are variables like child-workspace, block-upstream, days-to-keep, 
> > etc. in
> > most of the templates under jobs/ which would fancy a similar care since 
> > you're
> > striving for consistency in variable naming here. Anyway, I'm quite 
> > ambivalent
> > about the patch, I don't mind having it in, but I don't feel like this is 
> > the
> > kind of change we want to merge. Having said that though, there already are
> > commits like that, so my argument is invalid.
> >
> > If you show some love to the other variables which suffer from the same
> > "plague" too:
> >
> > Reviewed-by: Erik Skultety 
>
> Note "user-defined" above: all the other names you mentioned are
> part of JJB's own grammar for defining jobs, so we couldn't change
> them even if we wanted to.
>
> Perhaps I should have expanded the "help us out later" bit in the
> commit message to explain that this is not a gratuitous change
> made just to satisfy my craving for consistency[1]: while JJB is
> okay with user variables having dashes is them, Ansible is very
> much not, and if you look ahead to patch 06/12 you'll see why

Ah, good to know.

Thanks,
Erik

> keeping the existing name would just not work - hence this patch.
>
>
> [1] Though I'll admit I'm absolutely not above doing that ;)
> --
> Andrea Bolognani / Red Hat / Virtualization
>
> --
> 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] [Qemu-devel] clean/simple Q35 support in libvirt+QEMU for guest OSes that don't support virtio-1.0

[Daniel P . Berrangé]

On Wed, Aug 22, 2018 at 12:36:27PM +0200, Andrea Bolognani wrote:
> On Tue, 2018-08-21 at 14:21 -0400, Laine Stump wrote:
> > On 08/17/2018 06:35 AM, Andrea Bolognani wrote:
> > > If we decide we want to explicitly spell out the options instead
> > > of relying on QEMU changing behavior based on the slot type, which
> > > is probably a good idea anyway, I think we should have
> > > 
> > >   virtio-0.9 => disable-legacy=no,disable-modern=no
> > >   virtio-1.0 => disable-legacy=yes,disable-modern=no
> > > 
> > > There's basically no reason to have a device legacy-only rather
> > > than transitional, and spelling out both options instead of only
> > > one of them just seems more robust.
> > 
> > I agree with both of those, but the counter-argument is that "virtio"
> > already describes a transitional device like your proposal for
> > virtio-0.9 (at least today), and it makes the versioned models less
> > orthogonal. In the end, I could go either way...
> 
> Yeah, Dan already made that argument and convinced me that we
> should use virtio-0.9 for legacy only, virtio-1.0 for modern only
> and plain virtio for no enforced behavior / transitional.
> 
> > The problem I can see with the virtio-1.0 model name is that if
> > management applications start putting that into their XML (even though
> > "virtio" would yield a working guest), the guests will be unable to
> > migrate to another machine that has the same version of qemu, but an
> > older libvirt that doesn't understand the virtio-1.0 model number. If
> > that's acceptable, then management apps can being always specifying the
> > version for virtio whether it's old or new. If not, then they should
> > continue to use plain "virtio" unless they specifically need to force
> > virtio-0.9.
> 
> Well, even using virtio-0.9 could be considered problematic,
> because at least from the QEMU point of view there's nothing
> preventing the guest from working correctly as long as the version
> is recent enough that disable-legacy/disable-modern are available.
> 
> AFAIK management applications such as oVirt and OpenStack usually
> require specific, reasonably recent versions of QEMU and libvirt,
> so they could make sure virtio-0.9 and virtio-1.0 are understood
> by all nodes in the cluster that way.

Of course this is not a new scenario - any time an app makes use of a new
feature exposed in libvirt there's a chance that guests using that feature
will not be migratable to older libvirt. The apps and/or administrators
deploying them, have to decide on the cost/benefit tradeoff.

I think this will have an impact on ability of apps to adopt use of the
virtio-0.9/1.0 device model variants though. Both oVirt and OpenStack
do care about live migration to older versions, at least at certain
periods in time. For example, during a live upgrade scenario, 

This dovetails into Laine querying about the domain capabilities not
currently reporting info on the available device models. In the case
that migration to older verisons is needed, the dom capabilities info
won't be looked at anyway, as they don't wish to blindly use a feature
that just happens to exist in the current version.

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 v3 1/3] vsh: Add API for printing tables.

[Daniel P . Berrangé]

On Wed, Aug 22, 2018 at 12:50:12PM +0200, Simon Kobyda wrote:
> On Tue, 2018-08-21 at 11:46 +0100, Daniel P. Berrangé wrote:
> > On Tue, Aug 21, 2018 at 12:27:34PM +0200, Michal Privoznik wrote:
> > > On 08/21/2018 11:18 AM, Simon Kobyda wrote:
> > > > On Thu, 2018-08-16 at 12:28 +0100, Daniel P. Berrangé wrote:
> > > > > On Thu, Aug 16, 2018 at 12:56:24PM +0200, Simon Kobyda wrote:
> > > > > > 
> > > > > 
> > > > > After asking around I have found the right solution that we
> > > > > need to
> > > > > use
> > > > > for measuring string width.  mbstowcs()/wcswidth() will get the
> > > > > answer
> > > > > wrong wrt zero-width characters, combining characters, non-
> > > > > printable
> > > > > characters, etc. We need to use the libunistring library:
> > > > > 
> > > > >   
> > > > > 
> https://www.gnu.org/software/libunistring/manual/libunistring.html#uniwidth_002eh
> > > > > 
> > > > > 
> > > > 
> > > > I've tried what you've suggested, but it seems that it doesn't
> > > > work
> > > > well with all unicode characters. I'm looking into the code of
> > > > the
> > > > library, and each function uN_strwidth calls function uN_width,
> > > > and
> > > > that function calls uc_width for calculation of width of
> > > > characters.
> > > > And if we look into the code of uc_width here:
> > > > 
> > > > 
> http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/uniwidth/width.c;h=269cfc77f50a3b9802e5fb5620ff8bcf95e05e40;hb=HEAD#l415
> > > > it seems that this library is limited only to certain unicodes,
> > > > e.g.:
> > > > hangul characters, angle brackets, CJK characters... But it
> > > > doesn't
> > > > cover all multiple-width characters. Example: I try to throw any
> > > > emoji
> > > > (e.g. , 呂, ), it returns width of 1 column for each charact
> > > > er, nevertheless these characters have width of 2 columns on
> > > > terminal.
> > > > 
> > > > BTW, it seems unistring library imports those funcions from
> > > > gnulib.
> > > 
> > > I guess the only option then is to try smartcols [1]. If it is good
> > > for
> > > util-linux it's going to be good for us too. Although, I'd prefer
> > > to
> > > have our own wrappers over their API.
> > > 
> > > https://github.com/karelzak/util-linux/tree/master/libsmartcols
> > 
> > The util-linux code does something that uses mbstowcs / wcwidth to
> > convert the characters and count their width, sort of like the
> > original
> > version of this patch. They have further code that decides to convert
> > certain unicode characters into "\xNN" escaped sequences, which
> > avoids
> > the problems I raised wrt non-printable strings.
> > 
> >https://github.com/karelzak/util-linux/blob/master/lib/mbsalign.c
> > 
> > So we could pull that helper API into our code, since its LGPL
> > loicensed.
> > I'm unclear if this correctly handles all the cases or not though as
> > there's no unit tests for it in util-linux AFACT.
> > 
> > Really the only way for us to be sure is to provide a unit test which
> > stresses our the code with a variety of unicode input strings.
> 
> About unit tests. Right now i've got tests for non-pritnable, zero-
> width, combining characters and opposite (rigth to left) writing.
> Anybody got any idea what else could be problematic with
> mbstowcs()/wcswidth(), and therefore tested?

I think that sounds reasonable enough for now - passing such tests would
already be massively better than the code  that exists today with strlen()

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] [PATCHv2 45/62] qemu: hotplug: Implement removable media change for -blockdev

[John Ferlan]

[...]

> +static int
> +qemuDomainChangeMediaBlockdev(virQEMUDriverPtr driver,
> +  virDomainObjPtr vm,
> +  virDomainDiskDefPtr disk,
> +  virStorageSourcePtr newsrc,
> +  bool force)
> +{
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
> +qemuHotplugDiskSourceDataPtr newbackend = NULL;
> +qemuHotplugDiskSourceDataPtr oldbackend = NULL;
> +virStorageSourcePtr oldsrc = disk->src;
> +char *nodename = NULL;
> +int rc;
> +int ret = -1;
> +
> +if (!virStorageSourceIsEmpty(disk->src) &&
> +!(oldbackend = qemuHotplugDiskSourceRemovePrepare(disk, 
> priv->qemuCaps)))
> +goto cleanup;
> +
> +disk->src = newsrc;
> +if (!virStorageSourceIsEmpty(disk->src)) {
> +if (!(newbackend = qemuHotplugDiskSourceAttachPrepare(disk,
> +  
> priv->qemuCaps)))
> +goto cleanup;
> +
> +if (qemuDomainDiskGetBackendAlias(disk, priv->qemuCaps, ) < 
> 0)

Coverity notes @nodename is leaked at cleanup:

John

BTW: Only 2 Coverity whines for a large series and you got rid of one of
my false positives by removing the detach->info.alias creation done in
qemuDomainDetachVirtioDiskDevice. Something similar I suppose could be
done for qemuDomainDetachControllerDevice using the same reasoning.


> +goto cleanup;
> +}
> +
> +if (diskPriv->tray && disk->tray_status != VIR_DOMAIN_DISK_TRAY_OPEN) {
> +qemuDomainObjEnterMonitor(driver, vm);
> +rc = qemuMonitorBlockdevTrayOpen(priv->mon, 
> diskPriv->backendQomName, force);
> +if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0)
> +goto cleanup;
> +
> +if (!force && qemuHotplugWaitForTrayEject(vm, disk) < 0)
> +goto cleanup;
> +}
> +
> +qemuDomainObjEnterMonitor(driver, vm);
> +
> +rc = qemuMonitorBlockdevMediumRemove(priv->mon, 
> diskPriv->backendQomName);
> +
> +if (rc == 0 && oldbackend)
> +qemuHotplugDiskSourceRemove(priv->mon, oldbackend);
> +
> +if (newbackend && nodename) {
> +if (rc == 0)
> +rc = qemuHotplugDiskSourceAttach(priv->mon, newbackend);
> +
> +if (rc == 0)
> +rc = qemuMonitorBlockdevMediumInsert(priv->mon,
> + diskPriv->backendQomName,
> + nodename);
> +}
> +
> +if (rc == 0)
> +rc = qemuMonitorBlockdevTrayClose(priv->mon, 
> diskPriv->backendQomName);
> +
> +if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0)
> +goto cleanup;
> +
> +ret = 0;
> +
> + cleanup:
> +qemuHotplugDiskSourceDataFree(newbackend);
> +qemuHotplugDiskSourceDataFree(oldbackend);
> +/* caller handles correct exchange of sources */
> +disk->src = oldsrc;
> +return ret;
> +}
> +
> +

[...]

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

Re: [libvirt] [PATCH v3 1/3] vsh: Add API for printing tables.

[Simon Kobyda]

On Tue, 2018-08-21 at 11:46 +0100, Daniel P. Berrangé wrote:
> On Tue, Aug 21, 2018 at 12:27:34PM +0200, Michal Privoznik wrote:
> > On 08/21/2018 11:18 AM, Simon Kobyda wrote:
> > > On Thu, 2018-08-16 at 12:28 +0100, Daniel P. Berrangé wrote:
> > > > On Thu, Aug 16, 2018 at 12:56:24PM +0200, Simon Kobyda wrote:
> > > > > 
> > > > 
> > > > After asking around I have found the right solution that we
> > > > need to
> > > > use
> > > > for measuring string width.  mbstowcs()/wcswidth() will get the
> > > > answer
> > > > wrong wrt zero-width characters, combining characters, non-
> > > > printable
> > > > characters, etc. We need to use the libunistring library:
> > > > 
> > > >   
> > > > 
https://www.gnu.org/software/libunistring/manual/libunistring.html#uniwidth_002eh
> > > > 
> > > > 
> > > 
> > > I've tried what you've suggested, but it seems that it doesn't
> > > work
> > > well with all unicode characters. I'm looking into the code of
> > > the
> > > library, and each function uN_strwidth calls function uN_width,
> > > and
> > > that function calls uc_width for calculation of width of
> > > characters.
> > > And if we look into the code of uc_width here:
> > > 
> > > 
http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/uniwidth/width.c;h=269cfc77f50a3b9802e5fb5620ff8bcf95e05e40;hb=HEAD#l415
> > > it seems that this library is limited only to certain unicodes,
> > > e.g.:
> > > hangul characters, angle brackets, CJK characters... But it
> > > doesn't
> > > cover all multiple-width characters. Example: I try to throw any
> > > emoji
> > > (e.g. , 呂, ), it returns width of 1 column for each charact
> > > er, nevertheless these characters have width of 2 columns on
> > > terminal.
> > > 
> > > BTW, it seems unistring library imports those funcions from
> > > gnulib.
> > 
> > I guess the only option then is to try smartcols [1]. If it is good
> > for
> > util-linux it's going to be good for us too. Although, I'd prefer
> > to
> > have our own wrappers over their API.
> > 
> > https://github.com/karelzak/util-linux/tree/master/libsmartcols
> 
> The util-linux code does something that uses mbstowcs / wcwidth to
> convert the characters and count their width, sort of like the
> original
> version of this patch. They have further code that decides to convert
> certain unicode characters into "\xNN" escaped sequences, which
> avoids
> the problems I raised wrt non-printable strings.
> 
>https://github.com/karelzak/util-linux/blob/master/lib/mbsalign.c
> 
> So we could pull that helper API into our code, since its LGPL
> loicensed.
> I'm unclear if this correctly handles all the cases or not though as
> there's no unit tests for it in util-linux AFACT.
> 
> Really the only way for us to be sure is to provide a unit test which
> stresses our the code with a variety of unicode input strings.

About unit tests. Right now i've got tests for non-pritnable, zero-
width, combining characters and opposite (rigth to left) writing.
Anybody got any idea what else could be problematic with
mbstowcs()/wcswidth(), and therefore tested?

Simon Kobyda.

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

Re: [libvirt] [PATCHv2 12/62] qemu: monitor: Allow using 'qdev' instead of 'device' for getting disk throttling

[John Ferlan]

[...]

> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -4818,7 +4818,8 @@ int qemuMonitorJSONOpenGraphics(qemuMonitorPtr mon,
>  }
>  static int
>  qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr io_throttle,
> -   const char *device,
> +   const char *drivealias,
> +   const char *qdevid,
> virDomainBlockIoTuneInfoPtr reply)
>  {
>  int ret = -1;
> @@ -4828,7 +4829,8 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr 
> io_throttle,
>  for (i = 0; i < virJSONValueArraySize(io_throttle); i++) {
>  virJSONValuePtr temp_dev = virJSONValueArrayGet(io_throttle, i);
>  virJSONValuePtr inserted;
> -const char *current_dev;
> +const char *current_drive;
> +const char *current_qdev;
> 
>  if (!temp_dev || virJSONValueGetType(temp_dev) != 
> VIR_JSON_TYPE_OBJECT) {
>  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> @@ -4837,14 +4839,18 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr 
> io_throttle,
>  goto cleanup;
>  }
> 
> -if (!(current_dev = virJSONValueObjectGetString(temp_dev, 
> "device"))) {
> +current_qdev = virJSONValueObjectGetString(temp_dev, "qdev");
> +current_drive = virJSONValueObjectGetString(temp_dev, "device");
> +
> +if (!current_drive && !current_qdev) {

Is this supposed to be || ?

>  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("block_io_throttle device entry "
>   "was not in expected format"));
>  goto cleanup;
>  }
> 
> -if (STRNEQ(current_dev, device))
> +if ((drivealias && STRNEQ(current_drive, drivealias)) ||
> +(qdevid && STRNEQ(current_qdev, qdevid)))

Because Coverity complains here that if one or the other is == NULL
while the other != NULL, then strcmp is not going to be happy.

John

>  continue;
> 
>  found = true;
> @@ -4885,7 +4891,7 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr 
> io_throttle,
>  if (!found) {
>  virReportError(VIR_ERR_INTERNAL_ERROR,
> _("cannot find throttling info for device '%s'"),
> -   device);
> +   drivealias ? drivealias : qdevid);
>  goto cleanup;
>  }
>  ret = 0;
> @@ -4996,7 +5002,8 @@ int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr 
> mon,
>  }

[...]

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

Re: [libvirt] [jenkins-ci PATCH v3 01/12] jobs: Rename git-url -> git_url

[Andrea Bolognani]

On Wed, 2018-08-22 at 12:19 +0200, Erik Skultety wrote:
> On Wed, Aug 22, 2018 at 11:44:16AM +0200, Andrea Bolognani wrote:
> > Other user-defined variables use underscores as
> > separator instead of dashes; this change will also
> > help us out later.
> 
> There are variables like child-workspace, block-upstream, days-to-keep, etc. 
> in
> most of the templates under jobs/ which would fancy a similar care since 
> you're
> striving for consistency in variable naming here. Anyway, I'm quite ambivalent
> about the patch, I don't mind having it in, but I don't feel like this is the
> kind of change we want to merge. Having said that though, there already are
> commits like that, so my argument is invalid.
> 
> If you show some love to the other variables which suffer from the same
> "plague" too:
> 
> Reviewed-by: Erik Skultety 

Note "user-defined" above: all the other names you mentioned are
part of JJB's own grammar for defining jobs, so we couldn't change
them even if we wanted to.

Perhaps I should have expanded the "help us out later" bit in the
commit message to explain that this is not a gratuitous change
made just to satisfy my craving for consistency[1]: while JJB is
okay with user variables having dashes is them, Ansible is very
much not, and if you look ahead to patch 06/12 you'll see why
keeping the existing name would just not work - hence this patch.


[1] Though I'll admit I'm absolutely not above doing that ;)
-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [jenkins-ci PATCH v3 04/12] jobs: Declare empty values consistently

[Erik Skultety]

On Wed, Aug 22, 2018 at 11:44:19AM +0200, Andrea Bolognani wrote:
> The pipe syntax is intended for multi-line preformatted
> text, and is abused here. Use a saner syntax instead.
>
> Signed-off-by: Andrea Bolognani 
> ---
Reviewed-by: Erik Skultety 

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

Re: [libvirt] [jenkins-ci PATCH v3 03/12] jobs: Move some parameters from jobs to defaults

[Erik Skultety]

On Wed, Aug 22, 2018 at 11:44:18AM +0200, Andrea Bolognani wrote:
> The affected parameters are autogen_args, command and
> command_pre_build.
>
> Moving their default value next to the other overridable
> defaults makes sense, and it will help us out later.
>
> Signed-off-by: Andrea Bolognani 
> ---
Reviewed-by: Erik Skultety 

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

Re: [libvirt] [jenkins-ci PATCH v3 02/12] jobs: Remove archive_format from defaults

[Erik Skultety]

On Wed, Aug 22, 2018 at 12:37:27PM +0200, Erik Skultety wrote:
> On Wed, Aug 22, 2018 at 11:44:17AM +0200, Andrea Bolognani wrote:
> > Instead of defining a default and overriding it on a
> > case-by-case basis, always define archive_format at the
> > project level.
> >
> > This adds a bit of duplication, but it's consistent with
> > how we define other metadata for projects and it will
> > help us out later.
>
> How is it going to help us later? So here you move something which is 
> perfectly
> fine to have in defaults and override it only when necessary and in the
> following patch you're making a preparation changes for essentially the
> opposite of this - moving autogen_args and similar to defaults (not that any
> template needs to override these at the moment, but I'm getting confused by 
> the

Actually there are templates overriding those ones, but the point stays.

Erik

> consistency you talk about).
> I think this patch should be dropped.
>
> Erik

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

Re: [libvirt] [jenkins-ci PATCH v3 02/12] jobs: Remove archive_format from defaults

[Erik Skultety]

On Wed, Aug 22, 2018 at 11:44:17AM +0200, Andrea Bolognani wrote:
> Instead of defining a default and overriding it on a
> case-by-case basis, always define archive_format at the
> project level.
>
> This adds a bit of duplication, but it's consistent with
> how we define other metadata for projects and it will
> help us out later.

How is it going to help us later? So here you move something which is perfectly
fine to have in defaults and override it only when necessary and in the
following patch you're making a preparation changes for essentially the
opposite of this - moving autogen_args and similar to defaults (not that any
template needs to override these at the moment, but I'm getting confused by the
consistency you talk about).
I think this patch should be dropped.

Erik

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

Re: [libvirt] [Qemu-devel] clean/simple Q35 support in libvirt+QEMU for guest OSes that don't support virtio-1.0

[Andrea Bolognani]

On Tue, 2018-08-21 at 14:21 -0400, Laine Stump wrote:
> On 08/17/2018 06:35 AM, Andrea Bolognani wrote:
> > If we decide we want to explicitly spell out the options instead
> > of relying on QEMU changing behavior based on the slot type, which
> > is probably a good idea anyway, I think we should have
> > 
> >   virtio-0.9 => disable-legacy=no,disable-modern=no
> >   virtio-1.0 => disable-legacy=yes,disable-modern=no
> > 
> > There's basically no reason to have a device legacy-only rather
> > than transitional, and spelling out both options instead of only
> > one of them just seems more robust.
> 
> I agree with both of those, but the counter-argument is that "virtio"
> already describes a transitional device like your proposal for
> virtio-0.9 (at least today), and it makes the versioned models less
> orthogonal. In the end, I could go either way...

Yeah, Dan already made that argument and convinced me that we
should use virtio-0.9 for legacy only, virtio-1.0 for modern only
and plain virtio for no enforced behavior / transitional.

> The problem I can see with the virtio-1.0 model name is that if
> management applications start putting that into their XML (even though
> "virtio" would yield a working guest), the guests will be unable to
> migrate to another machine that has the same version of qemu, but an
> older libvirt that doesn't understand the virtio-1.0 model number. If
> that's acceptable, then management apps can being always specifying the
> version for virtio whether it's old or new. If not, then they should
> continue to use plain "virtio" unless they specifically need to force
> virtio-0.9.

Well, even using virtio-0.9 could be considered problematic,
because at least from the QEMU point of view there's nothing
preventing the guest from working correctly as long as the version
is recent enough that disable-legacy/disable-modern are available.

AFAIK management applications such as oVirt and OpenStack usually
require specific, reasonably recent versions of QEMU and libvirt,
so they could make sure virtio-0.9 and virtio-1.0 are understood
by all nodes in the cluster that way.

For something like virt-manager where the coupling is loose
perhaps it would make sense to use virtio-0.9 only on q35 when
the OS requires it and plain virtio everywhere else for the time
being, then switch to always using virtio-0.9 and virtio-1.0
after a Reasonable Amount of Time™ has passed.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [jenkins-ci PATCH v3 01/12] jobs: Rename git-url -> git_url

[Erik Skultety]

On Wed, Aug 22, 2018 at 11:44:16AM +0200, Andrea Bolognani wrote:
> Other user-defined variables use underscores as
> separator instead of dashes; this change will also
> help us out later.

There are variables like child-workspace, block-upstream, days-to-keep, etc. in
most of the templates under jobs/ which would fancy a similar care since you're
striving for consistency in variable naming here. Anyway, I'm quite ambivalent
about the patch, I don't mind having it in, but I don't feel like this is the
kind of change we want to merge. Having said that though, there already are
commits like that, so my argument is invalid.

If you show some love to the other variables which suffer from the same
"plague" too:

Reviewed-by: Erik Skultety 

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

Re: [libvirt] [PATCH v3 01/12] conf: Add definitions for 'uid' and 'fid' PCI address attributes

[Andrea Bolognani]

On Wed, 2018-08-22 at 17:39 +0800, Yi Min Zhao wrote:
> 在 2018/8/21 下午9:19, Andrea Bolognani 写道:
> > On Tue, 2018-08-21 at 19:55 +0800, Yi Min Zhao wrote:
> > > > > For zPCI address, if we use the same strategy as PCI part and user
> > > > > wants to assign fid=0 to the specific device, he might encounter
> > > > > assignment failure. At least, according to the doc, 0 shoud be a valid
> > > > > value to assign to fid.
> > > > 
> > > > If the user wants to use a specific zPCI address they can simply
> > > > specify both attributes, eg. uid=1,fid=0 will work just fine with
> > > > the proposed approach and won't produce errors or cause a new zPCI
> > > > address to be automatically assigned.
> > > > 
> > > > If the user only specified fid=0 while leaving uid unspecified,
> > > > well, an address is going to be picked for them.
> > > 
> > > This would be an empty zpci address while parsing XML so that we might
> > > pick a zpci address with non-zero fid. For example:
> > > 
> > > dev1: fid=0 (this address would be treated as an unassigned zpci address)
> > > dev2 (no definition for both fid and uid)
> > > 
> > > Then the process of assigning addresses will iterate devices as type by
> > > type.
> > > If dev2 is processed before dev1, dev2's fid would be assigned by 0.
> > > This causes
> > > dev1's fid=1. The result doesn't match what the user wants.
> > 
> > This would be the same as specifying a partial PCI address such as
> > 
> >
> > 
> > and getting an address with slot != 0x00 back: surprising, perhaps,
> > but that's the way it's been with PCI addresses forever so you can
> > assume users are familiar with it by now.
> > 
> > For zPCI addresses to be inconsistent with this behavior would be
> > utterly confusing; moreover, if the user really needs the uid and
> > fid to have certain values, all they have to do is specify both
> > and libvirt will not attempt to override them.
> 
> I tried as your idea. It makes everything complicated, especially 
> alloc/reserve/release
> zpci address. If the user defines uid=1 and fid=0, we don't know whether 
> should
> reserve fid. (uid=1 fid=0) is the same with (uid=1).

You should reserve it. The user can either

  * not specify zPCI information at all: automatic assignment will
be performed, no error should be reported;

  * specify a *full* zPCI address: manual assignment, will result
in either that exact address being used or an error;

  * specify partial information: missing properties will default
to zero, which will probably cause errors eg. if only the uid
is specified for a bunch of devices.

The last option is less predictable so it should probably never be
used. All of the above is consistent with how the PCI part works.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH v3 09/12] qemu: Generate and use zPCI device in QEMU command line

[Yi Min Zhao]

在 2018/8/22 下午4:42, Andrea Bolognani 写道:

On Wed, 2018-08-22 at 11:06 +0800, Yi Min Zhao wrote:

在 2018/8/20 下午7:14, Andrea Bolognani 写道:

So which is unique: uid, fid, or the combination of the two?
Could I have

-device zpci,uid=1,fid=1
-device zpci,uid=1,fid=2

or would that not work? What about

-device zpci,uid=1,fid=1
-device zpci,uid=2,fid=1

would that be okay?

Both can't work. FID must be unique and UID must be also unique.
They are independent.

Got it, thanks.


My point is that a funtion called fooIsBaz() should only ever
check whether the foo in question is indeed a baz, without taking
any other action such as reporting errors. Leave that to the
caller, or give the function a different name.

I think moving to the caller is proper.


qemuDomainDeviceDefValidate() looks like a reasonable entry point
for checks such as "you specified a zPCI address but this is not
an s390 guest" or "your configuration requires zPCI but the QEMU
binary doesn't support that feature". Both of the cases above
should have proper test suite coverage, by the way.

How about my idea mentioned above?

You mean moving the check to the caller?

Yes.


I think qemuDomainDeviceDefValidate() is a better choice because
it should allow you to implement the checks once, potentially with
more context such as information about the domain and QEMU
capabilities, and have them performed regardless of whether the
device is being processed eg. during regular parsing or hotplug.


IIUC, in qemuDomainDeviceDefValidate(), we have to directly access
device_info from DeviceDef to check zpci. I think direct accessing is
insecure and we have to process those device types that has device_info.
In addition, these is a special case that address is not defined and pci
address type will be chosen by default.

I found there are a lot of caps check code while building command line.

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

Re: [libvirt] [PATCH v3 08/12] conf: Allocate/release 'uid' and 'fid' in PCI address

[Andrea Bolognani]

On Wed, 2018-08-22 at 11:00 +0800, Yi Min Zhao wrote:
> 在 2018/8/17 上午12:06, Andrea Bolognani 写道:
> > On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
> > [...]
> > > +static inline bool
> > > +virDeviceInfoPCIAddressExtensionPresent(const virDomainDeviceInfo *info)
> > > +{
> > > +return info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
> > > +info->addr.pci.zpci;
> > > +}
> > 
> > This should be called virDeviceInfoPCIAddressExtensionIsPresent()
> > since it's a predicate. I know the corresponding PCI function gets
> > the naming wrong, but that doesn't mean you should too :)
> > 
> > In the same vein, I don't think this (or the PCI version, for that
> > matter) need to be inline... I'd rather have them both as regular
> > functions in the .c file. I'll probably cook up a patch cleaning
> > up the PCI part later, see what the feedback is.
> 
> Got it. Thank!

The patches I said I'd write are now on the list:

  https://www.redhat.com/archives/libvir-list/2018-August/msg01105.html

No reviews yet, we'll see whether they get ACKed or NACKed :)

> > [...]
> > > +static int
> > > +virDomainZPCIAddressReserveNextAddr(virDomainZPCIAddressIdsPtr zpciIds,
> > > +virZPCIDeviceAddressPtr addr)
> > > +{
> > > +if (!addr->uid_assigned &&
> > > +virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr)) {
> > > +return -1;
> > > +}
> > > +
> > > +if (!addr->fid_assigned &&
> > > +virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr)) {
> > > +virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
> > > +return -1;
> > > +}
> > 
> > Not sure I get the logic here... ReserveNextAddress() is supposed
> > to pick the next available address and reserve it, but IIUC this
> > will skip picking either id based on whether they were assigned.
> 
> If uid/fid is assigned, we call ***Reserve***().
> If uid/fid is unassigned, we call ***ReserveNext***().
> 
> But I'm not very clear about your concern.

That in ReserveNext() you're checking whether addr->*id_assigned
when you know that ids have not been assigned or you wouldn't have
called ReserveNext() in the first place... It seems unnecessary
and confusing to me, so unless I've missed something that makes it
necessary please just drop those checks.

> > [...]
> > > +static int
> > > +virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs,
> > > +   virDomainDeviceInfoPtr dev)
> > > +{
> > 
> > It's weird that this function doesn't get extFlags as an argument,
> > unlike the other ones you've introduced. Better make it consistent.
> 
> We have to pass DeviceInfo which already has extFlags. If we pass 
> extFlags again,
> isn't it redundant? This function is only called in one place for 
> hotplug case.

I wanted the API to be more consistent but I realize now you have
to pass either virPCIDeviceAddress or virDomainDeviceInfo depending
on the context, so it doesn't really matter, you can leave it as it
is. The signatures for the corresponding PCI functions are not
entirely consistent either :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [Qemu-devel] [PATCH 3/3] ui: remove support for SDL1.2 in favour of SDL2

[Daniel P . Berrangé]

On Wed, Aug 08, 2018 at 02:51:01PM +0100, Peter Maydell wrote:
> On 8 August 2018 at 11:49, Daniel P. Berrangé  wrote:
> > SDL1.2 was deprecated in the 2.12.0 release with:
> >
> >   commit e52c6ba34149b4f39c3fd60e59ee32b809db2bfa
> >   Author: Daniel P. Berrange 
> >   Date:   Mon Jan 15 14:25:33 2018 +
> >
> > ui: deprecate use of SDL 1.2 in favour of 2.0 series
> >
> > The SDL 2.0 release was made in Aug, 2013:
> >
> >   https://www.libsdl.org/release/
> >
> > That will soon be 4 + 1/2 years ago, which is enough time to consider
> > the 2.0 series widely supported.
> >
> > Thus we deprecate the SDL 1.2 support, which will allow us to delete it
> > in the last release of 2018. By this time, SDL 2.0 will be more than 5
> > years old.
> >
> > Signed-off-by: Daniel P. Berrange 
> > Reviewed-by: Marc-André Lureau 
> > Message-id: 20180115142533.24585-1-berra...@redhat.com
> > Signed-off-by: Gerd Hoffmann 
> >
> > It is thus able to be removed in the 3.1.0 release.
> 
> At least one of the BSD VMs in tests/vm/ is still using SDL1.2.
> I think we should update that VM before we drop SDL1.2 support.

I confirmed that the freebsd & netbsd images still work as SDL just gets
automatically disabled by configure. The openbsd image breaks hard though
because SDL is the only available audio backend on openbsd, effectively
making SDL a mandatory requirement.

So we definitely need to update the openbsd image to have SDL2.


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

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

Re: [libvirt] [PATCH] qemu: Start domain on a node without cpu affinity

[Daniel P . Berrangé]

On Wed, Aug 22, 2018 at 09:48:10AM +0200, Peter Krempa wrote:
> On Wed, Aug 22, 2018 at 00:31:29 +0300, Roman Bolshakov wrote:
> > libvirt affinity wrappers don't support macOS Thread Affinity API:
> > https://developer.apple.com/library/archive/releasenotes/Performance/RN-AffinityAPI/index.html
> > 
> > virProcessSetAffinity stub prevents libvirt from starting a qemu domain
> > on macOS:
> > 
> > $ virsh start vm
> > error: Failed to start domain vm
> > error: Process CPU affinity is not supported on this platform: Function not 
> > implemented
> > 
> > With the patch a VM can be started on macOS but some affinity-related
> > commands will return errors:
> > 
> > $ virsh vcpuinfo vm
> > error: Requested operation is not valid: cpu affinity is not supported
> > 
> > $ virsh vcpupin vm
> > VCPU: CPU Affinity
> > --
> >0: 0-7
> > 
> > $ virsh vcpupin vm --live --vcpu 0 --cpulist 7
> > error: operation failed: Virt type 'qemu' does not support vCPU pinning
> > 
> > $ virsh emulatorpin vm
> > emulator: CPU Affinity
> > --
> >*: 0-7
> > 
> > $ virsh emulatorpin vm --live --cpulist 7
> > error: Requested operation is not valid: cpu affinity is not supported
> > 
> > The patch also fixes virmacmaptest on macOS
> > 
> > Signed-off-by: Roman Bolshakov 
> > ---
> >  src/qemu/qemu_driver.c | 6 ++
> >  src/util/virprocess.c  | 2 +-
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 21e9e87ddd..2e225b1ede 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -5245,6 +5245,12 @@ qemuDomainPinEmulator(virDomainPtr dom,
> >  if (virDomainPinEmulatorEnsureACL(dom->conn, vm->def, flags) < 0)
> >  goto cleanup;
> >  
> > +if (!qemuDomainHasVcpuPids(vm)) {
> > +virReportError(VIR_ERR_OPERATION_INVALID,
> > +   "%s", _("cpu affinity is not supported"));
> > +goto cleanup;
> > +}
> > +
> >  if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
> >  goto cleanup;
> >  
> > diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> > index 3988f5546c..7eaafd29f2 100644
> > --- a/src/util/virprocess.c
> > +++ b/src/util/virprocess.c
> > @@ -598,7 +598,7 @@ int virProcessSetAffinity(pid_t pid ATTRIBUTE_UNUSED,
> >  {
> >  virReportSystemError(ENOSYS, "%s",
> >   _("Process CPU affinity is not supported on this 
> > platform"));
> > -return -1;
> > +return 0;
> 
> This is not a good idea. The caller should make sure that it does not
> call this function if we need to support such a code path.

Specifically I think you just need to turn qemuProcessInitCpuAffinity
into a no-op on macOS builds.

Of course if its possible to actually implement virProcessSetAffinity
on macOS that's an option too :-)

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

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

[libvirt] [PATCH v2] virsh: Don't break loop of domblkinfo for disks

[Han Han]

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

--all option is added to cmdDomblkinfo since commit 62c39193 allowing to
show all block devices info. Reset error when empty source in case error
breaks the loop of domblkinfo for disks.

Signed-off-by: Han Han 
---
 tools/virsh-domain-monitor.c | 13 +
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index b9b4f9739b..576610f005 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -475,6 +475,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
 int ndisks;
 size_t i;
 xmlNodePtr *disks = NULL;
+char *source = NULL;
 char *target = NULL;
 char *protocol = NULL;
 
@@ -505,16 +506,18 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
 
 for (i = 0; i < ndisks; i++) {
 ctxt->node = disks[i];
+source = virXPathString("string(./source)", ctxt);
 protocol = virXPathString("string(./source/@protocol)", ctxt);
 target = virXPathString("string(./target/@dev)", ctxt);
 
 rc = virDomainGetBlockInfo(dom, target, , 0);
 
 if (rc < 0) {
-/* If protocol is present that's an indication of a networked
- * storage device which cannot provide statistics, so generate
- * 0 based data and get the next disk. */
-if (protocol && !active &&
+/* For the case of empty cdrom, networked disk which cannot
+ * provide statistics, generate 0 based data and get the next
+ * disk.
+ */
+if (!source && protocol && !active &&
 virGetLastErrorCode() == VIR_ERR_INTERNAL_ERROR &&
 virGetLastErrorDomain() == VIR_FROM_STORAGE) {
 memset(, 0, sizeof(info));
@@ -526,6 +529,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
 
 cmdDomblkinfoPrint(ctl, , target, human, false);
 
+VIR_FREE(source);
 VIR_FREE(target);
 VIR_FREE(protocol);
 }
@@ -540,6 +544,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
 
  cleanup:
 virshDomainFree(dom);
+VIR_FREE(source);
 VIR_FREE(target);
 VIR_FREE(protocol);
 VIR_FREE(disks);
-- 
2.18.0

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

[libvirt] [jenkins-ci PATCH v3 12/12] lcitool: Document build action

[Andrea Bolognani]

Provide some examples and scenarios.

Signed-off-by: Andrea Bolognani 
---
 guests/README.markdown | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/guests/README.markdown b/guests/README.markdown
index ddf0149..870619e 100644
--- a/guests/README.markdown
+++ b/guests/README.markdown
@@ -46,6 +46,25 @@ related projects, while running
 will update all hosts and prepare them to build libvirt both as a native
 library and, where supported, as a Windows library using MinGW.
 
+Once hosts have been prepared following the steps above, you can use
+`lcitool` to perform builds as well: for example, running
+
+lcitool -a build -h '*debian*' -p libvirt-python -b master
+
+will fetch libvirt-python's `master` branch and build it on all Debian
+hosts.
+
+This feature can be used to validate a series you've been working on
+will not introduce any CI failures when merged: assuming you have a
+personal clone of `$project` hosted somewhere and your changes are in
+the `feature` branch, you can tweak the value of `git_url` in
+`playbooks/build/projects/$project.yml` so that it points to your own
+repository and then run
+
+lcitool -a build -h all -p $project -b feature
+
+to spot issues early.
+
 
 Host setup
 --
-- 
2.17.1

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

[libvirt] [jenkins-ci PATCH v3 11/12] guests: Support building on more targets

[Andrea Bolognani]

The Jenkins build jobs can only run on the subset of
guests that are available on CentOS CI, but when we're
running build jobs through lcitool we don't have that
limitation and we can build on more targets.

Signed-off-by: Andrea Bolognani 
---
 guests/playbooks/build/jobs/defaults.yml |  4 
 guests/playbooks/build/projects/libvirt-dbus.yml | 16 ++--
 .../playbooks/build/projects/libvirt-sandbox.yml |  3 +++
 guests/playbooks/build/projects/libvirt-tck.yml  |  4 
 guests/playbooks/build/projects/libvirt.yml  |  3 +++
 guests/playbooks/build/projects/virt-manager.yml |  9 -
 projects/libvirt-dbus.yaml   |  6 --
 projects/virt-manager.yaml   |  4 +++-
 8 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/guests/playbooks/build/jobs/defaults.yml 
b/guests/playbooks/build/jobs/defaults.yml
index b4e9343..c07475c 100644
--- a/guests/playbooks/build/jobs/defaults.yml
+++ b/guests/playbooks/build/jobs/defaults.yml
@@ -4,11 +4,15 @@ all_machines:
   - libvirt-centos-7
   - libvirt-debian-8
   - libvirt-debian-9
+  - libvirt-debian-sid
   - libvirt-fedora-27
   - libvirt-fedora-28
   - libvirt-fedora-rawhide
   - libvirt-freebsd-10
   - libvirt-freebsd-11
+  - libvirt-freebsd-current
+  - libvirt-ubuntu-16
+  - libvirt-ubuntu-18
 rpm_machines:
   - libvirt-centos-7
   - libvirt-fedora-27
diff --git a/guests/playbooks/build/projects/libvirt-dbus.yml 
b/guests/playbooks/build/projects/libvirt-dbus.yml
index 2d8f77c..d9f5d4a 100644
--- a/guests/playbooks/build/projects/libvirt-dbus.yml
+++ b/guests/playbooks/build/projects/libvirt-dbus.yml
@@ -5,11 +5,15 @@
 machines:
   - libvirt-centos-7
   - libvirt-debian-9
+  - libvirt-debian-sid
   - libvirt-fedora-27
   - libvirt-fedora-28
   - libvirt-fedora-rawhide
   - libvirt-freebsd-10
   - libvirt-freebsd-11
+  - libvirt-freebsd-current
+  - libvirt-ubuntu-16
+  - libvirt-ubuntu-18
 archive_format: xz
 git_url: https://github.com/libvirt/libvirt-dbus.git
 
@@ -17,24 +21,32 @@
 - include: '{{ playbook_base }}/jobs/autotools-build-job.yml'
 - include: '{{ playbook_base }}/jobs/autotools-syntax-check-job.yml'
   vars:
-# syntax-check uses Python 3, which CentOS 7 doesn't include
+# CentOS 7 doesn't include Python 3 and the version of pyflakes
+# in FreeBSD CURRENT is too new to be used by flake8
 machines:
   - libvirt-debian-9
+  - libvirt-debian-sid
   - libvirt-fedora-27
   - libvirt-fedora-28
   - libvirt-fedora-rawhide
   - libvirt-freebsd-10
   - libvirt-freebsd-11
+  - libvirt-ubuntu-16
+  - libvirt-ubuntu-18
 - include: '{{ playbook_base }}/jobs/autotools-check-job.yml'
   vars:
-# The test suite uses Python 3, which CentOS 7 doesn't include
+# CentOS 7 doesn't include Python 3 and the version in Ubuntu
+# 16.04 is too old
 machines:
   - libvirt-debian-9
+  - libvirt-debian-sid
   - libvirt-fedora-27
   - libvirt-fedora-28
   - libvirt-fedora-rawhide
   - libvirt-freebsd-10
   - libvirt-freebsd-11
+  - libvirt-freebsd-current
+  - libvirt-ubuntu-18
 - include: '{{ playbook_base }}/jobs/autotools-rpm-job.yml'
   vars:
 machines: '{{ rpm_machines }}'
diff --git a/guests/playbooks/build/projects/libvirt-sandbox.yml 
b/guests/playbooks/build/projects/libvirt-sandbox.yml
index 23be1f3..411cfc7 100644
--- a/guests/playbooks/build/projects/libvirt-sandbox.yml
+++ b/guests/playbooks/build/projects/libvirt-sandbox.yml
@@ -7,9 +7,12 @@
 machines:
   - libvirt-debian-8
   - libvirt-debian-9
+  - libvirt-debian-sid
   - libvirt-fedora-27
   - libvirt-fedora-28
   - libvirt-fedora-rawhide
+  - libvirt-ubuntu-16
+  - libvirt-ubuntu-18
 archive_format: gz
 git_url: https://github.com/libvirt/libvirt-sandbox.git
 
diff --git a/guests/playbooks/build/projects/libvirt-tck.yml 
b/guests/playbooks/build/projects/libvirt-tck.yml
index 13e63f4..fa16d26 100644
--- a/guests/playbooks/build/projects/libvirt-tck.yml
+++ b/guests/playbooks/build/projects/libvirt-tck.yml
@@ -4,11 +4,15 @@
 machines:
   - libvirt-debian-8
   - libvirt-debian-9
+  - libvirt-debian-sid
   - libvirt-fedora-27
   - libvirt-fedora-28
   - libvirt-fedora-rawhide
   - libvirt-freebsd-10
   - libvirt-freebsd-11
+  - libvirt-freebsd-current
+  - libvirt-ubuntu-16
+  - libvirt-ubuntu-18
 archive_format: gz
 git_url: https://github.com/libvirt/libvirt-tck.git
 
diff --git a/guests/playbooks/build/projects/libvirt.yml 
b/guests/playbooks/build/projects/libvirt.yml
index 23ef13c..bb3e53f 100644
--- a/guests/playbooks/build/projects/libvirt.yml
+++ b/guests/playbooks/build/projects/libvirt.yml
@@ -15,9 +15,12 @@
   - libvirt-centos-7
   - libvirt-debian-8
   - libvirt-debian-9
+  - libvirt-debian-sid
   - libvirt-fedora-27
   - libvirt-fedora-28
   

[libvirt] [jenkins-ci PATCH v3 08/12] lcitool: Make playbook execution generic

[Andrea Bolognani]

Signed-off-by: Andrea Bolognani 
---
 guests/lcitool | 87 ++
 1 file changed, 45 insertions(+), 42 deletions(-)

diff --git a/guests/lcitool b/guests/lcitool
index f28199d..e0410f3 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -351,6 +351,50 @@ class Application:
 help="list of projects to consider",
 )
 
+def _execute_playbook(self, playbook, hosts, projects):
+base = Util.get_base()
+
+flavor = self._config.get_flavor()
+vault_pass_file = self._config.get_vault_password_file()
+root_pass_file = self._config.get_root_password_file()
+
+ansible_hosts = ",".join(self._inventory.expand_pattern(hosts))
+selected_projects = self._projects.expand_pattern(projects)
+
+ansible_cfg_path = os.path.join(base, "ansible.cfg")
+playbook_base = os.path.join(base, "playbooks", playbook)
+playbook_path = os.path.join(playbook_base, "main.yml")
+
+extra_vars = json.dumps({
+"base": base,
+"playbook_base": playbook_base,
+"root_password_file": root_pass_file,
+"flavor": flavor,
+"selected_projects": selected_projects,
+})
+
+cmd = [
+"ansible-playbook",
+"--limit", ansible_hosts,
+"--extra-vars", extra_vars,
+]
+
+# Provide the vault password if available
+if vault_pass_file is not None:
+cmd += ["--vault-password-file", vault_pass_file]
+
+cmd += [playbook_path]
+
+# We need to point Ansible to the correct configuration file,
+# and for some reason this has to be done using the environment
+# rather than through the command line
+os.environ["ANSIBLE_CONFIG"] = ansible_cfg_path
+
+try:
+subprocess.check_call(cmd)
+except Exception:
+raise Error("Failed to run {} on '{}'".format(playbook, hosts))
+
 def _action_hosts(self, _hosts, _projects):
 for host in self._inventory.expand_pattern("all"):
 print(host)
@@ -431,48 +475,7 @@ class Application:
 raise Error("Failed to install '{}'".format(host))
 
 def _action_update(self, hosts, projects):
-base = Util.get_base()
-
-flavor = self._config.get_flavor()
-vault_pass_file = self._config.get_vault_password_file()
-root_pass_file = self._config.get_root_password_file()
-
-ansible_hosts = ",".join(self._inventory.expand_pattern(hosts))
-selected_projects = self._projects.expand_pattern(projects)
-
-ansible_cfg_path = os.path.join(base, "ansible.cfg")
-playbook_base = os.path.join(base, "playbooks", "update")
-playbook_path = os.path.join(playbook_base, "main.yml")
-
-extra_vars = json.dumps({
-"base": base,
-"playbook_base": playbook_base,
-"root_password_file": root_pass_file,
-"flavor": flavor,
-"selected_projects": selected_projects,
-})
-
-cmd = [
-"ansible-playbook",
-"--limit", ansible_hosts,
-"--extra-vars", extra_vars,
-]
-
-# Provide the vault password if available
-if vault_pass_file is not None:
-cmd += ["--vault-password-file", vault_pass_file]
-
-cmd += [playbook_path]
-
-# We need to point Ansible to the correct configuration file,
-# and for some reason this has to be done using the environment
-# rather than through the command line
-os.environ["ANSIBLE_CONFIG"] = ansible_cfg_path
-
-try:
-subprocess.check_call(cmd)
-except Exception:
-raise Error("Failed to update '{}'".format(hosts))
+self._execute_playbook("update", hosts, projects)
 
 def _action_dockerfile(self, hosts, projects):
 mappings = self._projects.get_mappings()
-- 
2.17.1

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

[libvirt] [jenkins-ci PATCH v3 06/12] guests: Add build projects

[Andrea Bolognani]

These tasks mirror the Jenkins projects contained in the
top-level projects/ directory.

Signed-off-by: Andrea Bolognani 
---
 guests/playbooks/build/projects/libosinfo.yml | 36 +
 .../playbooks/build/projects/libvirt-cim.yml  | 10 
 .../playbooks/build/projects/libvirt-dbus.yml | 40 ++
 .../playbooks/build/projects/libvirt-glib.yml | 38 +
 .../build/projects/libvirt-go-xml.yml | 13 +
 .../playbooks/build/projects/libvirt-go.yml   | 13 +
 .../playbooks/build/projects/libvirt-perl.yml | 19 +++
 .../build/projects/libvirt-python.yml | 13 +
 .../build/projects/libvirt-sandbox.yml| 27 ++
 .../playbooks/build/projects/libvirt-tck.yml  | 23 
 guests/playbooks/build/projects/libvirt.yml   | 54 +++
 .../build/projects/osinfo-db-tools.yml| 36 +
 guests/playbooks/build/projects/osinfo-db.yml | 23 
 .../playbooks/build/projects/virt-manager.yml | 35 
 .../playbooks/build/projects/virt-viewer.yml  | 40 ++
 15 files changed, 420 insertions(+)
 create mode 100644 guests/playbooks/build/projects/libosinfo.yml
 create mode 100644 guests/playbooks/build/projects/libvirt-cim.yml
 create mode 100644 guests/playbooks/build/projects/libvirt-dbus.yml
 create mode 100644 guests/playbooks/build/projects/libvirt-glib.yml
 create mode 100644 guests/playbooks/build/projects/libvirt-go-xml.yml
 create mode 100644 guests/playbooks/build/projects/libvirt-go.yml
 create mode 100644 guests/playbooks/build/projects/libvirt-perl.yml
 create mode 100644 guests/playbooks/build/projects/libvirt-python.yml
 create mode 100644 guests/playbooks/build/projects/libvirt-sandbox.yml
 create mode 100644 guests/playbooks/build/projects/libvirt-tck.yml
 create mode 100644 guests/playbooks/build/projects/libvirt.yml
 create mode 100644 guests/playbooks/build/projects/osinfo-db-tools.yml
 create mode 100644 guests/playbooks/build/projects/osinfo-db.yml
 create mode 100644 guests/playbooks/build/projects/virt-manager.yml
 create mode 100644 guests/playbooks/build/projects/virt-viewer.yml

diff --git a/guests/playbooks/build/projects/libosinfo.yml 
b/guests/playbooks/build/projects/libosinfo.yml
new file mode 100644
index 000..c29053b
--- /dev/null
+++ b/guests/playbooks/build/projects/libosinfo.yml
@@ -0,0 +1,36 @@
+---
+- set_fact:
+name: libosinfo
+machines: '{{ all_machines }}'
+archive_format: gz
+git_url: https://gitlab.com/libosinfo/libosinfo.git
+
+- include: '{{ playbook_base }}/jobs/prepare.yml'
+- include: '{{ playbook_base }}/jobs/autotools-build-job.yml'
+- include: '{{ playbook_base }}/jobs/autotools-syntax-check-job.yml'
+- include: '{{ playbook_base }}/jobs/autotools-check-job.yml'
+- include: '{{ playbook_base }}/jobs/autotools-rpm-job.yml'
+  vars:
+machines: '{{ rpm_machines }}'
+
+- include: '{{ playbook_base }}/jobs/prepare.yml'
+  vars:
+variant: -mingw32
+machines: '{{ mingw_machines }}'
+- include: '{{ playbook_base }}/jobs/autotools-build-job.yml'
+  vars:
+variant: -mingw32
+local_env: '{{ mingw32_local_env }}'
+autogen_args: '{{ mingw32_autogen_args }}'
+machines: '{{ mingw_machines }}'
+
+- include: '{{ playbook_base }}/jobs/prepare.yml'
+  vars:
+variant: -mingw64
+machines: '{{ mingw_machines }}'
+- include: '{{ playbook_base }}/jobs/autotools-build-job.yml'
+  vars:
+variant: -mingw64
+local_env: '{{ mingw64_local_env }}'
+autogen_args: '{{ mingw64_autogen_args }}'
+machines: '{{ mingw_machines }}'
diff --git a/guests/playbooks/build/projects/libvirt-cim.yml 
b/guests/playbooks/build/projects/libvirt-cim.yml
new file mode 100644
index 000..f959bf6
--- /dev/null
+++ b/guests/playbooks/build/projects/libvirt-cim.yml
@@ -0,0 +1,10 @@
+---
+- set_fact:
+name: libvirt-cim
+machines: '{{ rpm_machines }}'
+archive_format: gz
+git_url: https://github.com/libvirt/libvirt-cim.git
+
+- include: '{{ playbook_base }}/jobs/prepare.yml'
+- include: '{{ playbook_base }}/jobs/autotools-build-job.yml'
+- include: '{{ playbook_base }}/jobs/autotools-rpm-job.yml'
diff --git a/guests/playbooks/build/projects/libvirt-dbus.yml 
b/guests/playbooks/build/projects/libvirt-dbus.yml
new file mode 100644
index 000..2d8f77c
--- /dev/null
+++ b/guests/playbooks/build/projects/libvirt-dbus.yml
@@ -0,0 +1,40 @@
+---
+- set_fact:
+name: libvirt-dbus
+# Debian 8 doesn't have a recent enough GLib
+machines:
+  - libvirt-centos-7
+  - libvirt-debian-9
+  - libvirt-fedora-27
+  - libvirt-fedora-28
+  - libvirt-fedora-rawhide
+  - libvirt-freebsd-10
+  - libvirt-freebsd-11
+archive_format: xz
+git_url: https://github.com/libvirt/libvirt-dbus.git
+
+- include: '{{ playbook_base }}/jobs/prepare.yml'
+- include: '{{ playbook_base }}/jobs/autotools-build-job.yml'
+- include: '{{ playbook_base }}/jobs/autotools-syntax-check-job.yml'
+  vars:
+# syntax-check uses 

[libvirt] [jenkins-ci PATCH v3 10/12] lcitool: Support building arbitrary branches

[Andrea Bolognani]

Building master is useful for CI purposes and to debug CI
failures locally, but when developing we want to be able
to build a personal branch to validate in-progress changes.

Signed-off-by: Andrea Bolognani 
---
 guests/lcitool   | 30 
 guests/playbooks/build/jobs/defaults.yml |  1 -
 2 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/guests/lcitool b/guests/lcitool
index 2901a92..ec81448 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -351,8 +351,13 @@ class Application:
 metavar="PROJECTS",
 help="list of projects to consider",
 )
+self._parser.add_argument(
+"-b",
+metavar="BRANCH",
+help="git branch to use when performing builds",
+)
 
-def _execute_playbook(self, playbook, hosts, projects):
+def _execute_playbook(self, playbook, hosts, projects, branch):
 base = Util.get_base()
 
 flavor = self._config.get_flavor()
@@ -372,6 +377,7 @@ class Application:
 "root_password_file": root_pass_file,
 "flavor": flavor,
 "selected_projects": selected_projects,
+"branch": branch,
 })
 
 cmd = [
@@ -396,15 +402,15 @@ class Application:
 except Exception:
 raise Error("Failed to run {} on '{}'".format(playbook, hosts))
 
-def _action_hosts(self, _hosts, _projects):
+def _action_hosts(self, _hosts, _projects, _branch):
 for host in self._inventory.expand_pattern("all"):
 print(host)
 
-def _action_projects(self, _hosts, _projects):
+def _action_projects(self, _hosts, _projects, _branch):
 for project in self._projects.expand_pattern("all"):
 print(project)
 
-def _action_install(self, hosts, _projects):
+def _action_install(self, hosts, _projects, _branch):
 base = Util.get_base()
 
 flavor = self._config.get_flavor()
@@ -475,13 +481,16 @@ class Application:
 except Exception:
 raise Error("Failed to install '{}'".format(host))
 
-def _action_update(self, hosts, projects):
-self._execute_playbook("update", hosts, projects)
+def _action_update(self, hosts, projects, _branch):
+self._execute_playbook("update", hosts, projects, None)
+
+def _action_build(self, hosts, projects, branch):
+if branch is None:
+raise Error("Missing branch")
 
-def _action_build(self, hosts, projects):
-self._execute_playbook("build", hosts, projects)
+self._execute_playbook("build", hosts, projects, branch)
 
-def _action_dockerfile(self, hosts, projects):
+def _action_dockerfile(self, hosts, projects, _branch):
 mappings = self._projects.get_mappings()
 
 hosts = self._inventory.expand_pattern(hosts)
@@ -553,11 +562,12 @@ class Application:
 action = cmdline.a
 hosts = cmdline.h
 projects = cmdline.p
+branch = cmdline.b
 
 method = "_action_{}".format(action.replace("-", "_"))
 
 if hasattr(self, method):
-getattr(self, method).__call__(hosts, projects)
+getattr(self, method).__call__(hosts, projects, branch)
 else:
 raise Error("Invalid action '{}'".format(action))
 
diff --git a/guests/playbooks/build/jobs/defaults.yml 
b/guests/playbooks/build/jobs/defaults.yml
index ce49f5a..b4e9343 100644
--- a/guests/playbooks/build/jobs/defaults.yml
+++ b/guests/playbooks/build/jobs/defaults.yml
@@ -1,5 +1,4 @@
 ---
-branch: master
 variant: ''
 all_machines:
   - libvirt-centos-7
-- 
2.17.1

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

[libvirt] [jenkins-ci PATCH v3 03/12] jobs: Move some parameters from jobs to defaults

[Andrea Bolognani]

The affected parameters are autogen_args, command and
command_pre_build.

Moving their default value next to the other overridable
defaults makes sense, and it will help us out later.

Signed-off-by: Andrea Bolognani 
---
 jobs/autotools.yaml| 1 -
 jobs/defaults.yaml | 3 +++
 jobs/generic.yaml  | 1 -
 jobs/go.yaml   | 1 -
 jobs/perl-modulebuild.yaml | 1 -
 jobs/python-distutils.yaml | 1 -
 6 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/jobs/autotools.yaml b/jobs/autotools.yaml
index 1ceb8a1..4232d0e 100644
--- a/jobs/autotools.yaml
+++ b/jobs/autotools.yaml
@@ -4,7 +4,6 @@
 name: '{name}-{branch}-build{variant}'
 project-type: matrix
 description: '{title} Build'
-autogen_args: ''
 workspace: '{name}-{branch}{variant}'
 child-workspace: '.'
 block-downstream: true
diff --git a/jobs/defaults.yaml b/jobs/defaults.yaml
index 5927ce3..19b711f 100644
--- a/jobs/defaults.yaml
+++ b/jobs/defaults.yaml
@@ -22,6 +22,9 @@
   - libvirt-fedora-rawhide
 global_env: |
 local_env: |
+autogen_args: ''
+command: ''
+command_pre_build: ''
 strip_buildrequires: |
   sed -i -e 's/BuildRequires: *libvirt.*//' *.spec*
   sed -i -e 's/BuildRequires: *osinfo-db.*//' *.spec*
diff --git a/jobs/generic.yaml b/jobs/generic.yaml
index 48bec23..805b1d6 100644
--- a/jobs/generic.yaml
+++ b/jobs/generic.yaml
@@ -4,7 +4,6 @@
 name: '{name}-{branch}-build{variant}'
 project-type: matrix
 description: '{title} Build'
-autogen_args: ''
 workspace: '{name}-{branch}{variant}'
 child-workspace: '.'
 block-downstream: true
diff --git a/jobs/go.yaml b/jobs/go.yaml
index 77c931b..b460658 100644
--- a/jobs/go.yaml
+++ b/jobs/go.yaml
@@ -4,7 +4,6 @@
 name: '{name}-{branch}-build{variant}'
 project-type: matrix
 description: '{title} Build'
-autogen_args: ''
 workspace: '{name}-{branch}{variant}'
 child-workspace: '.'
 block-downstream: true
diff --git a/jobs/perl-modulebuild.yaml b/jobs/perl-modulebuild.yaml
index 8d0d87f..01247a3 100644
--- a/jobs/perl-modulebuild.yaml
+++ b/jobs/perl-modulebuild.yaml
@@ -4,7 +4,6 @@
 name: '{name}-{branch}-build{variant}'
 project-type: matrix
 description: '{title} Build'
-autogen_args: ''
 workspace: '{name}-{branch}{variant}'
 child-workspace: '.'
 block-downstream: true
diff --git a/jobs/python-distutils.yaml b/jobs/python-distutils.yaml
index d5be99f..1227b8c 100644
--- a/jobs/python-distutils.yaml
+++ b/jobs/python-distutils.yaml
@@ -4,7 +4,6 @@
 name: '{name}-{branch}-build{variant}'
 project-type: matrix
 description: '{title} Build'
-command_pre_build: ''
 workspace: '{name}-{branch}{variant}'
 child-workspace: '.'
 block-downstream: true
-- 
2.17.1

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

[libvirt] [jenkins-ci PATCH v3 05/12] guests: Add build jobs

[Andrea Bolognani]

These tasks mirror the Jenkins jobs contained in the
top-level jobs/ directory.

Signed-off-by: Andrea Bolognani 
---
 .../build/jobs/autotools-build-job.yml| 15 +++
 .../build/jobs/autotools-check-job.yml| 16 
 .../build/jobs/autotools-rpm-job.yml  | 15 +++
 .../build/jobs/autotools-syntax-check-job.yml | 12 ++
 guests/playbooks/build/jobs/defaults.yml  | 39 +++
 .../build/jobs/generic-build-job.yml  | 11 ++
 .../build/jobs/generic-check-job.yml  | 11 ++
 .../playbooks/build/jobs/generic-rpm-job.yml  | 11 ++
 .../build/jobs/generic-syntax-check-job.yml   | 11 ++
 guests/playbooks/build/jobs/go-build-job.yml  | 11 ++
 guests/playbooks/build/jobs/go-check-job.yml  | 11 ++
 .../build/jobs/perl-modulebuild-build-job.yml | 13 +++
 .../build/jobs/perl-modulebuild-check-job.yml | 11 ++
 .../build/jobs/perl-modulebuild-rpm-job.yml   | 14 +++
 guests/playbooks/build/jobs/prepare.yml   | 19 +
 .../build/jobs/python-distutils-build-job.yml | 13 +++
 .../build/jobs/python-distutils-check-job.yml | 11 ++
 .../build/jobs/python-distutils-rpm-job.yml   | 14 +++
 18 files changed, 258 insertions(+)
 create mode 100644 guests/playbooks/build/jobs/autotools-build-job.yml
 create mode 100644 guests/playbooks/build/jobs/autotools-check-job.yml
 create mode 100644 guests/playbooks/build/jobs/autotools-rpm-job.yml
 create mode 100644 guests/playbooks/build/jobs/autotools-syntax-check-job.yml
 create mode 100644 guests/playbooks/build/jobs/defaults.yml
 create mode 100644 guests/playbooks/build/jobs/generic-build-job.yml
 create mode 100644 guests/playbooks/build/jobs/generic-check-job.yml
 create mode 100644 guests/playbooks/build/jobs/generic-rpm-job.yml
 create mode 100644 guests/playbooks/build/jobs/generic-syntax-check-job.yml
 create mode 100644 guests/playbooks/build/jobs/go-build-job.yml
 create mode 100644 guests/playbooks/build/jobs/go-check-job.yml
 create mode 100644 guests/playbooks/build/jobs/perl-modulebuild-build-job.yml
 create mode 100644 guests/playbooks/build/jobs/perl-modulebuild-check-job.yml
 create mode 100644 guests/playbooks/build/jobs/perl-modulebuild-rpm-job.yml
 create mode 100644 guests/playbooks/build/jobs/prepare.yml
 create mode 100644 guests/playbooks/build/jobs/python-distutils-build-job.yml
 create mode 100644 guests/playbooks/build/jobs/python-distutils-check-job.yml
 create mode 100644 guests/playbooks/build/jobs/python-distutils-rpm-job.yml

diff --git a/guests/playbooks/build/jobs/autotools-build-job.yml 
b/guests/playbooks/build/jobs/autotools-build-job.yml
new file mode 100644
index 000..bb621a1
--- /dev/null
+++ b/guests/playbooks/build/jobs/autotools-build-job.yml
@@ -0,0 +1,15 @@
+---
+- name: '{{ name }}-{{ branch }}-build{{ variant }}'
+  shell: |
+set -e
+cd {{ name }}-{{ branch }}{{ variant }}
+
+{{ global_env }}
+{{ local_env }}
+mkdir build
+cd build
+../autogen.sh --prefix=$VIRT_PREFIX {{ autogen_args }}
+$MAKE
+$MAKE install
+  when:
+- inventory_hostname in machines
diff --git a/guests/playbooks/build/jobs/autotools-check-job.yml 
b/guests/playbooks/build/jobs/autotools-check-job.yml
new file mode 100644
index 000..50024ae
--- /dev/null
+++ b/guests/playbooks/build/jobs/autotools-check-job.yml
@@ -0,0 +1,16 @@
+---
+- name: '{{ name }}-{{ branch }}-check{{ variant }}'
+  shell: |
+set -e
+cd {{ name }}-{{ branch }}{{ variant }}
+
+{{ global_env }}
+{{ local_env }}
+cd build
+if ! $MAKE check
+then
+cat tests/test-suite.log || true
+exit 1
+fi
+  when:
+- inventory_hostname in machines
diff --git a/guests/playbooks/build/jobs/autotools-rpm-job.yml 
b/guests/playbooks/build/jobs/autotools-rpm-job.yml
new file mode 100644
index 000..c8babdf
--- /dev/null
+++ b/guests/playbooks/build/jobs/autotools-rpm-job.yml
@@ -0,0 +1,15 @@
+---
+- name: '{{ name }}-{{ branch }}-rpm{{ variant }}'
+  shell: |
+set -e
+cd {{ name }}-{{ branch }}{{ variant }}
+
+{{ global_env }}
+{{ local_env }}
+cd build
+{{ strip_buildrequires }}
+rm -f *.tar.{{ archive_format }}
+$MAKE dist
+rpmbuild --clean --define "_topdir `pwd`/rpmbuild" -ta *.tar.{{ 
archive_format }}
+  when:
+- inventory_hostname in machines
diff --git a/guests/playbooks/build/jobs/autotools-syntax-check-job.yml 
b/guests/playbooks/build/jobs/autotools-syntax-check-job.yml
new file mode 100644
index 000..bbbd240
--- /dev/null
+++ b/guests/playbooks/build/jobs/autotools-syntax-check-job.yml
@@ -0,0 +1,12 @@
+---
+- name: '{{ name }}-{{ branch }}-syntax-check{{ variant }}'
+  shell: |
+set -e
+cd {{ name }}-{{ branch }}{{ variant }}
+
+{{ global_env }}
+{{ local_env }}
+cd build
+$MAKE syntax-check
+  when:
+- inventory_hostname in machines
diff --git a/guests/playbooks/build/jobs/defaults.yml 

[libvirt] [jenkins-ci PATCH v3 00/12] lcitool: Add 'build' action

[Andrea Bolognani]

Changes from [v2]:

  * rebase on top of master (dbc2de85f775) and integrate recent
changes to build rules on the Jenkins side;

  * drop a commit that had already been merged in the meantime.

Changes from [v1]:

  * rebase on top of master (985ab833be9b) and integrate recent
changes to build rules on the Jenkins side;

  * build on more targets.

[v2] https://www.redhat.com/archives/libvir-list/2018-August/msg01109.html
[v1] https://www.redhat.com/archives/libvir-list/2018-August/msg00393.html

Andrea Bolognani (12):
  jobs: Rename git-url -> git_url
  jobs: Remove archive_format from defaults
  jobs: Move some parameters from jobs to defaults
  jobs: Declare empty values consistently
  guests: Add build jobs
  guests: Add build projects
  guests: Add build playbook
  lcitool: Make playbook execution generic
  lcitool: Add 'build' action
  lcitool: Support building arbitrary branches
  guests: Support building on more targets
  lcitool: Document build action

 guests/README.markdown|  19 +++
 guests/lcitool| 109 ++
 .../build/jobs/autotools-build-job.yml|  15 +++
 .../build/jobs/autotools-check-job.yml|  16 +++
 .../build/jobs/autotools-rpm-job.yml  |  15 +++
 .../build/jobs/autotools-syntax-check-job.yml |  12 ++
 guests/playbooks/build/jobs/defaults.yml  |  42 +++
 .../build/jobs/generic-build-job.yml  |  11 ++
 .../build/jobs/generic-check-job.yml  |  11 ++
 .../playbooks/build/jobs/generic-rpm-job.yml  |  11 ++
 .../build/jobs/generic-syntax-check-job.yml   |  11 ++
 guests/playbooks/build/jobs/go-build-job.yml  |  11 ++
 guests/playbooks/build/jobs/go-check-job.yml  |  11 ++
 .../build/jobs/perl-modulebuild-build-job.yml |  13 +++
 .../build/jobs/perl-modulebuild-check-job.yml |  11 ++
 .../build/jobs/perl-modulebuild-rpm-job.yml   |  14 +++
 guests/playbooks/build/jobs/prepare.yml   |  19 +++
 .../build/jobs/python-distutils-build-job.yml |  13 +++
 .../build/jobs/python-distutils-check-job.yml |  11 ++
 .../build/jobs/python-distutils-rpm-job.yml   |  14 +++
 guests/playbooks/build/main.yml   |  16 +++
 guests/playbooks/build/projects/libosinfo.yml |  36 ++
 .../playbooks/build/projects/libvirt-cim.yml  |  10 ++
 .../playbooks/build/projects/libvirt-dbus.yml |  52 +
 .../playbooks/build/projects/libvirt-glib.yml |  38 ++
 .../build/projects/libvirt-go-xml.yml |  13 +++
 .../playbooks/build/projects/libvirt-go.yml   |  13 +++
 .../playbooks/build/projects/libvirt-perl.yml |  19 +++
 .../build/projects/libvirt-python.yml |  13 +++
 .../build/projects/libvirt-sandbox.yml|  30 +
 .../playbooks/build/projects/libvirt-tck.yml  |  27 +
 guests/playbooks/build/projects/libvirt.yml   |  57 +
 .../build/projects/osinfo-db-tools.yml|  36 ++
 guests/playbooks/build/projects/osinfo-db.yml |  23 
 .../playbooks/build/projects/virt-manager.yml |  42 +++
 .../playbooks/build/projects/virt-viewer.yml  |  40 +++
 jobs/autotools.yaml   |   3 +-
 jobs/defaults.yaml|   8 +-
 jobs/generic.yaml |   3 +-
 jobs/go.yaml  |   3 +-
 jobs/perl-modulebuild.yaml|   3 +-
 jobs/python-distutils.yaml|   3 +-
 projects/libosinfo.yaml   |   3 +-
 projects/libvirt-cim.yaml |   3 +-
 projects/libvirt-dbus.yaml|   8 +-
 projects/libvirt-glib.yaml|   3 +-
 projects/libvirt-go-xml.yaml  |   3 +-
 projects/libvirt-go.yaml  |   3 +-
 projects/libvirt-perl.yaml|   3 +-
 projects/libvirt-python.yaml  |   3 +-
 projects/libvirt-sandbox.yaml |   3 +-
 projects/libvirt-tck.yaml |   3 +-
 projects/libvirt.yaml |   2 +-
 projects/osinfo-db-tools.yaml |   3 +-
 projects/osinfo-db.yaml   |   2 +-
 projects/virt-manager.yaml|   7 +-
 projects/virt-viewer.yaml |   3 +-
 57 files changed, 852 insertions(+), 77 deletions(-)
 create mode 100644 guests/playbooks/build/jobs/autotools-build-job.yml
 create mode 100644 guests/playbooks/build/jobs/autotools-check-job.yml
 create mode 100644 guests/playbooks/build/jobs/autotools-rpm-job.yml
 create mode 100644 guests/playbooks/build/jobs/autotools-syntax-check-job.yml
 create mode 100644 guests/playbooks/build/jobs/defaults.yml
 create mode 100644 guests/playbooks/build/jobs/generic-build-job.yml
 create mode 100644 guests/playbooks/build/jobs/generic-check-job.yml
 create mode 100644 guests/playbooks/build/jobs/generic-rpm-job.yml
 create mode 100644 guests/playbooks/build/jobs/generic-syntax-check-job.yml
 create mode 100644 

[libvirt] [jenkins-ci PATCH v3 07/12] guests: Add build playbook

[Andrea Bolognani]

This playbook represent the entry point for automated
builds, and follows the same calling conventions as the
existing update playbook.

Signed-off-by: Andrea Bolognani 
---
 guests/playbooks/build/main.yml | 16 
 1 file changed, 16 insertions(+)
 create mode 100644 guests/playbooks/build/main.yml

diff --git a/guests/playbooks/build/main.yml b/guests/playbooks/build/main.yml
new file mode 100644
index 000..8abda67
--- /dev/null
+++ b/guests/playbooks/build/main.yml
@@ -0,0 +1,16 @@
+---
+- hosts: all
+  remote_user: '{{ flavor }}'
+
+  vars_files:
+- '{{ playbook_base }}/jobs/defaults.yml'
+
+  tasks:
+
+- include: '{{ playbook_base }}/projects/{{ project }}.yml'
+  with_items:
+'{{ selected_projects }}'
+  loop_control:
+loop_var: project
+  when:
+- project in projects
-- 
2.17.1

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

[libvirt] [jenkins-ci PATCH v3 04/12] jobs: Declare empty values consistently

[Andrea Bolognani]

The pipe syntax is intended for multi-line preformatted
text, and is abused here. Use a saner syntax instead.

Signed-off-by: Andrea Bolognani 
---
 jobs/defaults.yaml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/jobs/defaults.yaml b/jobs/defaults.yaml
index 19b711f..8f11860 100644
--- a/jobs/defaults.yaml
+++ b/jobs/defaults.yaml
@@ -20,8 +20,8 @@
   - libvirt-fedora-rawhide
 mingw_machines:
   - libvirt-fedora-rawhide
-global_env: |
-local_env: |
+global_env: ''
+local_env: ''
 autogen_args: ''
 command: ''
 command_pre_build: ''
-- 
2.17.1

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

[libvirt] [jenkins-ci PATCH v3 09/12] lcitool: Add 'build' action

[Andrea Bolognani]

Signed-off-by: Andrea Bolognani 
---
 guests/lcitool | 4 
 1 file changed, 4 insertions(+)

diff --git a/guests/lcitool b/guests/lcitool
index e0410f3..2901a92 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -323,6 +323,7 @@ class Application:
 common actions:
   install  perform unattended host installation
   update   prepare hosts and keep them updated
+  buildbuild projects on hosts
 
 informational actions:
   hosts list all known hosts
@@ -477,6 +478,9 @@ class Application:
 def _action_update(self, hosts, projects):
 self._execute_playbook("update", hosts, projects)
 
+def _action_build(self, hosts, projects):
+self._execute_playbook("build", hosts, projects)
+
 def _action_dockerfile(self, hosts, projects):
 mappings = self._projects.get_mappings()
 
-- 
2.17.1

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

[libvirt] [jenkins-ci PATCH v3 01/12] jobs: Rename git-url -> git_url

[Andrea Bolognani]

Other user-defined variables use underscores as
separator instead of dashes; this change will also
help us out later.

Signed-off-by: Andrea Bolognani 
---
 jobs/autotools.yaml   | 2 +-
 jobs/generic.yaml | 2 +-
 jobs/go.yaml  | 2 +-
 jobs/perl-modulebuild.yaml| 2 +-
 jobs/python-distutils.yaml| 2 +-
 projects/libosinfo.yaml   | 2 +-
 projects/libvirt-cim.yaml | 2 +-
 projects/libvirt-dbus.yaml| 2 +-
 projects/libvirt-glib.yaml| 2 +-
 projects/libvirt-go-xml.yaml  | 2 +-
 projects/libvirt-go.yaml  | 2 +-
 projects/libvirt-perl.yaml| 2 +-
 projects/libvirt-python.yaml  | 2 +-
 projects/libvirt-sandbox.yaml | 2 +-
 projects/libvirt-tck.yaml | 2 +-
 projects/libvirt.yaml | 2 +-
 projects/osinfo-db-tools.yaml | 2 +-
 projects/osinfo-db.yaml   | 2 +-
 projects/virt-manager.yaml| 2 +-
 projects/virt-viewer.yaml | 2 +-
 20 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/jobs/autotools.yaml b/jobs/autotools.yaml
index f8a7e87..1ceb8a1 100644
--- a/jobs/autotools.yaml
+++ b/jobs/autotools.yaml
@@ -21,7 +21,7 @@
   num-to-keep: 1000
 scm:
   - git:
-  url: '{git-url}'
+  url: '{git_url}'
   branches:
 - origin/{branch}
   clean:
diff --git a/jobs/generic.yaml b/jobs/generic.yaml
index 6c59c63..48bec23 100644
--- a/jobs/generic.yaml
+++ b/jobs/generic.yaml
@@ -21,7 +21,7 @@
   num-to-keep: 1000
 scm:
   - git:
-  url: '{git-url}'
+  url: '{git_url}'
   branches:
 - origin/{branch}
   clean:
diff --git a/jobs/go.yaml b/jobs/go.yaml
index 10518c4..77c931b 100644
--- a/jobs/go.yaml
+++ b/jobs/go.yaml
@@ -21,7 +21,7 @@
   num-to-keep: 1000
 scm:
   - git:
-  url: '{git-url}'
+  url: '{git_url}'
   branches:
 - origin/{branch}
   clean:
diff --git a/jobs/perl-modulebuild.yaml b/jobs/perl-modulebuild.yaml
index 8b30d7e..8d0d87f 100644
--- a/jobs/perl-modulebuild.yaml
+++ b/jobs/perl-modulebuild.yaml
@@ -21,7 +21,7 @@
   num-to-keep: 1000
 scm:
   - git:
-  url: '{git-url}'
+  url: '{git_url}'
   branches:
 - origin/{branch}
   clean:
diff --git a/jobs/python-distutils.yaml b/jobs/python-distutils.yaml
index 7fcc2c6..d5be99f 100644
--- a/jobs/python-distutils.yaml
+++ b/jobs/python-distutils.yaml
@@ -21,7 +21,7 @@
   num-to-keep: 1000
 scm:
   - git:
-  url: '{git-url}'
+  url: '{git_url}'
   branches:
 - origin/{branch}
   clean:
diff --git a/projects/libosinfo.yaml b/projects/libosinfo.yaml
index 22c957e..614eb63 100644
--- a/projects/libosinfo.yaml
+++ b/projects/libosinfo.yaml
@@ -3,7 +3,7 @@
 name: libosinfo
 machines: '{all_machines}'
 title: libosinfo
-git-url: https://gitlab.com/libosinfo/libosinfo.git
+git_url: https://gitlab.com/libosinfo/libosinfo.git
 jobs:
   - autotools-build-job:
   parent_jobs: 'osinfo-db-master-build'
diff --git a/projects/libvirt-cim.yaml b/projects/libvirt-cim.yaml
index c6a7a6d..027c31d 100644
--- a/projects/libvirt-cim.yaml
+++ b/projects/libvirt-cim.yaml
@@ -3,7 +3,7 @@
 name: libvirt-cim
 machines: '{rpm_machines}'
 title: libvirt CIM
-git-url: https://github.com/libvirt/libvirt-cim.git
+git_url: https://github.com/libvirt/libvirt-cim.git
 jobs:
   - autotools-build-job:
   parent_jobs: 'libvirt-master-build'
diff --git a/projects/libvirt-dbus.yaml b/projects/libvirt-dbus.yaml
index 74163b7..43c6bed 100644
--- a/projects/libvirt-dbus.yaml
+++ b/projects/libvirt-dbus.yaml
@@ -12,7 +12,7 @@
   - libvirt-freebsd-11
 title: Libvirt D-Bus
 archive_format: xz
-git-url: https://github.com/libvirt/libvirt-dbus.git
+git_url: https://github.com/libvirt/libvirt-dbus.git
 jobs:
   - autotools-build-job:
   parent_jobs: 'libvirt-glib-master-build'
diff --git a/projects/libvirt-glib.yaml b/projects/libvirt-glib.yaml
index f61ca1e..dd2ba0c 100644
--- a/projects/libvirt-glib.yaml
+++ b/projects/libvirt-glib.yaml
@@ -3,7 +3,7 @@
 name: libvirt-glib
 machines: '{all_machines}'
 title: Libvirt GLib
-git-url: https://github.com/libvirt/libvirt-glib.git
+git_url: https://github.com/libvirt/libvirt-glib.git
 jobs:
   - autotools-build-job:
   parent_jobs: 'libvirt-master-build'
diff --git a/projects/libvirt-go-xml.yaml b/projects/libvirt-go-xml.yaml
index 6eb7ef0..4e44561 100644
--- a/projects/libvirt-go-xml.yaml
+++ b/projects/libvirt-go-xml.yaml
@@ -3,7 +3,7 @@
 name: libvirt-go-xml
 machines: '{all_machines}'
 title: Libvirt Go XML
-git-url: https://github.com/libvirt/libvirt-go-xml.git
+git_url: https://github.com/libvirt/libvirt-go-xml.git
 jobs:
   - go-build-job:
   parent_jobs: 'libvirt-master-build'
diff --git 

[libvirt] [jenkins-ci PATCH v3 02/12] jobs: Remove archive_format from defaults

[Andrea Bolognani]

Instead of defining a default and overriding it on a
case-by-case basis, always define archive_format at the
project level.

This adds a bit of duplication, but it's consistent with
how we define other metadata for projects and it will
help us out later.

Signed-off-by: Andrea Bolognani 
---
 jobs/defaults.yaml| 1 -
 projects/libosinfo.yaml   | 1 +
 projects/libvirt-cim.yaml | 1 +
 projects/libvirt-glib.yaml| 1 +
 projects/libvirt-go-xml.yaml  | 1 +
 projects/libvirt-go.yaml  | 1 +
 projects/libvirt-perl.yaml| 1 +
 projects/libvirt-python.yaml  | 1 +
 projects/libvirt-sandbox.yaml | 1 +
 projects/libvirt-tck.yaml | 1 +
 projects/osinfo-db-tools.yaml | 1 +
 projects/virt-manager.yaml| 1 +
 projects/virt-viewer.yaml | 1 +
 13 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/jobs/defaults.yaml b/jobs/defaults.yaml
index bab5bc4..5927ce3 100644
--- a/jobs/defaults.yaml
+++ b/jobs/defaults.yaml
@@ -20,7 +20,6 @@
   - libvirt-fedora-rawhide
 mingw_machines:
   - libvirt-fedora-rawhide
-archive_format: gz
 global_env: |
 local_env: |
 strip_buildrequires: |
diff --git a/projects/libosinfo.yaml b/projects/libosinfo.yaml
index 614eb63..55a4817 100644
--- a/projects/libosinfo.yaml
+++ b/projects/libosinfo.yaml
@@ -3,6 +3,7 @@
 name: libosinfo
 machines: '{all_machines}'
 title: libosinfo
+archive_format: gz
 git_url: https://gitlab.com/libosinfo/libosinfo.git
 jobs:
   - autotools-build-job:
diff --git a/projects/libvirt-cim.yaml b/projects/libvirt-cim.yaml
index 027c31d..6d524df 100644
--- a/projects/libvirt-cim.yaml
+++ b/projects/libvirt-cim.yaml
@@ -3,6 +3,7 @@
 name: libvirt-cim
 machines: '{rpm_machines}'
 title: libvirt CIM
+archive_format: gz
 git_url: https://github.com/libvirt/libvirt-cim.git
 jobs:
   - autotools-build-job:
diff --git a/projects/libvirt-glib.yaml b/projects/libvirt-glib.yaml
index dd2ba0c..993024a 100644
--- a/projects/libvirt-glib.yaml
+++ b/projects/libvirt-glib.yaml
@@ -3,6 +3,7 @@
 name: libvirt-glib
 machines: '{all_machines}'
 title: Libvirt GLib
+archive_format: gz
 git_url: https://github.com/libvirt/libvirt-glib.git
 jobs:
   - autotools-build-job:
diff --git a/projects/libvirt-go-xml.yaml b/projects/libvirt-go-xml.yaml
index 4e44561..7e6e090 100644
--- a/projects/libvirt-go-xml.yaml
+++ b/projects/libvirt-go-xml.yaml
@@ -3,6 +3,7 @@
 name: libvirt-go-xml
 machines: '{all_machines}'
 title: Libvirt Go XML
+archive_format: gz
 git_url: https://github.com/libvirt/libvirt-go-xml.git
 jobs:
   - go-build-job:
diff --git a/projects/libvirt-go.yaml b/projects/libvirt-go.yaml
index 7046dab..d90339a 100644
--- a/projects/libvirt-go.yaml
+++ b/projects/libvirt-go.yaml
@@ -3,6 +3,7 @@
 name: libvirt-go
 machines: '{all_machines}'
 title: Libvirt Go
+archive_format: gz
 git_url: https://github.com/libvirt/libvirt-go.git
 jobs:
   - go-build-job:
diff --git a/projects/libvirt-perl.yaml b/projects/libvirt-perl.yaml
index 8426a67..dbb6caf 100644
--- a/projects/libvirt-perl.yaml
+++ b/projects/libvirt-perl.yaml
@@ -3,6 +3,7 @@
 name: libvirt-perl
 machines: '{all_machines}'
 title: Libvirt Perl
+archive_format: gz
 git_url: https://github.com/libvirt/libvirt-perl.git
 jobs:
   - perl-modulebuild-build-job:
diff --git a/projects/libvirt-python.yaml b/projects/libvirt-python.yaml
index 69c1e17..05eea41 100644
--- a/projects/libvirt-python.yaml
+++ b/projects/libvirt-python.yaml
@@ -3,6 +3,7 @@
 name: libvirt-python
 machines: '{all_machines}'
 title: Libvirt Python
+archive_format: gz
 git_url: https://github.com/libvirt/libvirt-python.git
 jobs:
   - python-distutils-build-job:
diff --git a/projects/libvirt-sandbox.yaml b/projects/libvirt-sandbox.yaml
index fc9203a..0831896 100644
--- a/projects/libvirt-sandbox.yaml
+++ b/projects/libvirt-sandbox.yaml
@@ -11,6 +11,7 @@
   - libvirt-fedora-28
   - libvirt-fedora-rawhide
 title: Libvirt Sandbox
+archive_format: gz
 git_url: https://github.com/libvirt/libvirt-sandbox.git
 jobs:
   - autotools-build-job:
diff --git a/projects/libvirt-tck.yaml b/projects/libvirt-tck.yaml
index b98fd0a..3c8adfd 100644
--- a/projects/libvirt-tck.yaml
+++ b/projects/libvirt-tck.yaml
@@ -12,6 +12,7 @@
   - libvirt-freebsd-10
   - libvirt-freebsd-11
 title: Libvirt TCK
+archive_format: gz
 git_url: https://github.com/libvirt/libvirt-tck.git
 jobs:
   - perl-modulebuild-build-job:
diff --git a/projects/osinfo-db-tools.yaml b/projects/osinfo-db-tools.yaml
index ccc960b..bcf9e0a 100644
--- a/projects/osinfo-db-tools.yaml
+++ b/projects/osinfo-db-tools.yaml
@@ -3,6 +3,7 @@
 name: osinfo-db-tools
 machines: '{all_machines}'
 title: osinfo database tools
+archive_format: gz
 git_url: 

Re: [libvirt] [PATCH] util: eventpoll: Survive EBADF on macOS

[Daniel P . Berrangé]

On Wed, Aug 22, 2018 at 12:12:03AM +0300, Roman Bolshakov wrote:
> Fixes:
> https://www.redhat.com/archives/libvir-list/2017-January/msg00978.html
> 
> QEMU is probed through monitor fd to check capabilities during libvirtd init.
> The monitor fd is closed after probing by virQEMUCapsInitQMPCommandFree
> that calls virQEMUCapsInitQMPCommandAbort that calls qemuMonitorClose,
> the latter one notifies the event loop via an interrupt handle in
> qemuMonitorUnregister and after then closes monitor fd.
> 
> There could be a case when interrupt is sent after eventLoop is unlocked
> but before virEventPollRunOnce blocks in poll, shortly before file
> descriptor is closed by qemuMonitorClose. Then poll receives closed monitor
> fd in fdset and returns EBADF.
> 
> EBADF is not mentioned as a valid errno on macOS poll man-page but such
> behaviour can appear release-to-release, according to cpython:
> https://github.com/python/cpython/blob/master/Modules/selectmodule.c#L1161

Yeah, this really does look like broken macOS behaviour, as it should
be returning POLLNVAL instead.

> 
> The change also fixes the issue in qemucapabilitiestest. It returns
> Bad file descriptor message 25 times without the fix.
> 
> Signed-off-by: Roman Bolshakov 
> ---
>  src/util/vireventpoll.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/src/util/vireventpoll.c b/src/util/vireventpoll.c
> index 13d278df13..6d19374c52 100644
> --- a/src/util/vireventpoll.c
> +++ b/src/util/vireventpoll.c
> @@ -643,10 +643,18 @@ int virEventPollRunOnce(void)
>  EVENT_DEBUG("Poll got error event %d", errno);
>  if (errno == EINTR || errno == EAGAIN)
>  goto retry;
> +#ifdef __APPLE__
> +if (errno == EBADF) {
> +ret = 0;
> +goto dispatch;
> +}
> +#endif
>  virReportSystemError(errno, "%s",
>   _("Unable to poll on file handles"));
>  return -1;
>  }
> +
> + dispatch:

I don't think we should go through the dispatch code when
we get EBADF, as the contents of the 'revents' fields are
undefined after we get an error.

We should jump straight to the end where we have
"eventLoop.running = 0;"

If any FDs were in fact ready, we'll get them on the next iteration of
the loop anyway.

>  EVENT_DEBUG("Poll got %d event(s)", ret);
>  
>  virMutexLock();


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 v3 01/12] conf: Add definitions for 'uid' and 'fid' PCI address attributes

[Yi Min Zhao]

在 2018/8/21 下午9:19, Andrea Bolognani 写道:

On Tue, 2018-08-21 at 19:55 +0800, Yi Min Zhao wrote:

在 2018/8/21 下午7:00, Andrea Bolognani 写道:

On Tue, 2018-08-21 at 17:35 +0800, Yi Min Zhao wrote:

I want to ask a question. For pci address, any pci device can't use slot 0.
Is that a reason why PCI part could treat all zeros as empty address?

A PCI address where all attributes are zero can't be used, so
there's no ambiguity there; same for a zPCI address where all
attributes are zero, which also can't be used.

Yes, uid must be non-zero value. But the user could only define fid like
you mentioned below. Validation check happens during parsing xml.
So if the user only defines fid=0, zpci address (fid=0, uid=0 although the
user doesn't specify uid value) is going to be invalid as PCI address
check logic.

In the context of PCI addresses, empty implies invalid but during
XML parsing the address is only validated if it's not empty. If
that was not the case, an empty address would never have a chance
to be automatically filled in by libvirt :)


UID and FID must be unique separately, but we can't treat
them as a combination. This doesn't like PCI
address(domain:bus:slot:function).

I asked a question about this in reply to patch 09/12 yesterday
by the way, and it would be great if you could answer it.

I answered you in the morning.



For zPCI address, if we use the same strategy as PCI part and user
wants to assign fid=0 to the specific device, he might encounter
assignment failure. At least, according to the doc, 0 shoud be a valid
value to assign to fid.

If the user wants to use a specific zPCI address they can simply
specify both attributes, eg. uid=1,fid=0 will work just fine with
the proposed approach and won't produce errors or cause a new zPCI
address to be automatically assigned.

If the user only specified fid=0 while leaving uid unspecified,
well, an address is going to be picked for them.

This would be an empty zpci address while parsing XML so that we might
pick a zpci address with non-zero fid. For example:

dev1: fid=0 (this address would be treated as an unassigned zpci address)
dev2 (no definition for both fid and uid)

Then the process of assigning addresses will iterate devices as type by
type.
If dev2 is processed before dev1, dev2's fid would be assigned by 0.
This causes
dev1's fid=1. The result doesn't match what the user wants.

This would be the same as specifying a partial PCI address such as

   

and getting an address with slot != 0x00 back: surprising, perhaps,
but that's the way it's been with PCI addresses forever so you can
assume users are familiar with it by now.

For zPCI addresses to be inconsistent with this behavior would be
utterly confusing; moreover, if the user really needs the uid and
fid to have certain values, all they have to do is specify both
and libvirt will not attempt to override them.

I tried as your idea. It makes everything complicated, especially 
alloc/reserve/release
zpci address. If the user defines uid=1 and fid=0, we don't know whether 
should

reserve fid. (uid=1 fid=0) is the same with (uid=1).

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

[libvirt] [PATCH v3 0/9] RISC-V Guest Support

[Lubomir Rintel]

Hi.

Third iteration of RISC-V guest support. I believe I addressed all
points of Andrea's review. Tested with: images from [1]:

  virt-install \
--import --name riscv64 \
--arch riscv64 --machine virt \
--memory 2048 \
--rng /dev/urandom \
--disk /var/lib/libvirt/images/stage4-disk.img,bus=virtio  \
--boot kernel=/var/lib/libvirt/images/bbl,kernel_args="console=ttyS0 ro 
root=/dev/vda"

[1] https://fedorapeople.org/groups/risc-v/disk-images/

Note that the large test suite changes that touch the '*.replies' files
seem to upset the mail server, thus you're unlikely to receive them from
the list. You can get them straight from my repository instead:

  git pull https://github.com/lkundrak/libvirt.git lr/riscv-v3

Changes since v2:
 * Regenerated cap test data with QEMU 3.0.0
 * Squashed the tests together
 * Folded "docs/schemas: add RISC-V architectures" into "util: add RISC-V 
architectures"
 * Streamlined the Virtio MMIO address assignment
 * Added qemuDomainIsRISCVVirt() and qemuDomainMachineIsRISCVVirt()
 * Cosmetic changes to "util: add RISC-V architectures"
 * New commits:
qemu: add qemuDomainAssignVirtioMMIOAddresses()
qemu: add qemuDomainIsRISCVVirt() and qemuDomainMachineIsRISCVVirt()
qemu: RISC-V machines have no PCI
news: Add a mention of RISC-V guest support
 * Dropped:
qemu: no USB by default on RISC-V machines
(not sure why I thought this was needed)

Lubomir Rintel (9):
  qemu: rename qemuDomainMachineIsVirt()
  qemu: rename qemuDomainArmVirt()
  util: add RISC-V architectures
  qemu: RISC-V machines have no PCI
  qemu: add qemuDomainIsRISCVVirt() and qemuDomainMachineIsRISCVVirt()
  qemu: add qemuDomainAssignVirtioMMIOAddresses()
  qemu: assign addresses to virtio devices on RISC-V
  tests: Add RISC-V architectures
  news: Add a mention of RISC-V guest support

 docs/news.xml | 8 +
 docs/schemas/basictypes.rng   | 2 +
 src/qemu/qemu_capabilities.c  | 6 +-
 src/qemu/qemu_command.c   | 4 +-
 src/qemu/qemu_domain.c|46 +-
 src/qemu/qemu_domain.h| 9 +-
 src/qemu/qemu_domain_address.c|34 +-
 src/util/virarch.c| 5 +-
 src/util/virarch.h| 8 +-
 tests/capabilityschemadata/caps-qemu-kvm.xml  |36 +
 .../caps_3.0.0.riscv32.replies| 14819 
 .../caps_3.0.0.riscv32.xml|   118 +
 .../caps_3.0.0.riscv64.replies| 14819 
 .../caps_3.0.0.riscv64.xml|   118 +
 tests/qemucapabilitiestest.c  | 2 +
 tests/qemuxml2argvdata/riscv64-virt.args  |30 +
 tests/qemuxml2argvdata/riscv64-virt.xml   |32 +
 tests/qemuxml2argvtest.c  | 3 +
 .../riscv64-virt.xml  |42 +
 tests/qemuxml2xmloutdata/riscv64-virt.xml |36 +
 tests/qemuxml2xmltest.c   | 2 +
 tests/testutilsqemu.c |72 +
 tests/vircapstest.c   | 6 +
 23 files changed, 30232 insertions(+), 25 deletions(-)
 create mode 100644 tests/qemucapabilitiesdata/caps_3.0.0.riscv32.replies
 create mode 100644 tests/qemucapabilitiesdata/caps_3.0.0.riscv32.xml
 create mode 100644 tests/qemucapabilitiesdata/caps_3.0.0.riscv64.replies
 create mode 100644 tests/qemucapabilitiesdata/caps_3.0.0.riscv64.xml
 create mode 100644 tests/qemuxml2argvdata/riscv64-virt.args
 create mode 100644 tests/qemuxml2argvdata/riscv64-virt.xml
 create mode 100644 tests/qemuxml2startupxmloutdata/riscv64-virt.xml
 create mode 100644 tests/qemuxml2xmloutdata/riscv64-virt.xml

-- 
2.17.1

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

[libvirt] [PATCH v3 4/9] qemu: RISC-V machines have no PCI

[Lubomir Rintel]

Signed-off-by: Lubomir Rintel 
---
 src/qemu/qemu_domain_address.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 6c540a8094..551883e989 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -2157,7 +2157,9 @@ static bool
 qemuDomainSupportsPCI(virDomainDefPtr def,
   virQEMUCapsPtr qemuCaps)
 {
-if ((def->os.arch != VIR_ARCH_ARMV7L) && (def->os.arch != 
VIR_ARCH_AARCH64))
+if ((def->os.arch != VIR_ARCH_ARMV7L) &&
+(def->os.arch != VIR_ARCH_AARCH64) &&
+!ARCH_IS_RISCV(def->os.arch))
 return true;
 
 if (STREQ(def->os.machine, "versatilepb"))
-- 
2.17.1

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

[libvirt] [PATCH v3 7/9] qemu: assign addresses to virtio devices on RISC-V

[Lubomir Rintel]

Signed-off-by: Lubomir Rintel 
---
 src/qemu/qemu_domain_address.c | 16 
 1 file changed, 16 insertions(+)

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 317f280f92..29a1def24c 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -485,11 +485,27 @@ qemuDomainAssignARMVirtioMMIOAddresses(virDomainDefPtr 
def,
 }
 
 
+static void
+qemuDomainAssignRISCVVirtioMMIOAddresses(virDomainDefPtr def,
+ virQEMUCapsPtr qemuCaps)
+{
+if (!qemuDomainIsRISCVVirt(def))
+return;
+
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_MMIO)) {
+qemuDomainPrimeVirtioDeviceAddresses(def,
+ 
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO);
+}
+}
+
+
 static void
 qemuDomainAssignVirtioMMIOAddresses(virDomainDefPtr def,
 virQEMUCapsPtr qemuCaps)
 {
 qemuDomainAssignARMVirtioMMIOAddresses(def, qemuCaps);
+
+qemuDomainAssignRISCVVirtioMMIOAddresses(def, qemuCaps);
 }
 
 
-- 
2.17.1

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

[libvirt] [PATCH v3 3/9] util: add RISC-V architectures

[Lubomir Rintel]

Signed-off-by: Lubomir Rintel 
Reviewed-by: Andrea Bolognani 
---
 docs/schemas/basictypes.rng | 2 ++
 src/qemu/qemu_domain.c  | 2 ++
 src/util/virarch.c  | 5 -
 src/util/virarch.h  | 8 +++-
 4 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng
index 1a18cd31b1..14a3670e5c 100644
--- a/docs/schemas/basictypes.rng
+++ b/docs/schemas/basictypes.rng
@@ -398,6 +398,8 @@
   ppc64
   ppc64le
   ppcemb
+  riscv32
+  riscv64
   s390
   s390x
   sh4
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index fcb37d45e6..60b4a9b412 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3249,6 +3249,8 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def,
 case VIR_ARCH_OR32:
 case VIR_ARCH_PARISC:
 case VIR_ARCH_PARISC64:
+case VIR_ARCH_RISCV32:
+case VIR_ARCH_RISCV64:
 case VIR_ARCH_PPCLE:
 case VIR_ARCH_UNICORE32:
 case VIR_ARCH_XTENSA:
diff --git a/src/util/virarch.c b/src/util/virarch.c
index be48bcfb89..3f5efde8e2 100644
--- a/src/util/virarch.c
+++ b/src/util/virarch.c
@@ -65,15 +65,18 @@ static const struct virArchData {
 { "ppc64le",  64, VIR_ARCH_LITTLE_ENDIAN },
 { "ppcemb",   32, VIR_ARCH_BIG_ENDIAN },
 
+{ "riscv32",  32, VIR_ARCH_LITTLE_ENDIAN },
+{ "riscv64",  64, VIR_ARCH_LITTLE_ENDIAN },
 { "s390", 32, VIR_ARCH_BIG_ENDIAN },
 { "s390x",64, VIR_ARCH_BIG_ENDIAN },
 { "sh4",  32, VIR_ARCH_LITTLE_ENDIAN },
+
 { "sh4eb",64, VIR_ARCH_BIG_ENDIAN },
 { "sparc",32, VIR_ARCH_BIG_ENDIAN },
-
 { "sparc64",  64, VIR_ARCH_BIG_ENDIAN },
 { "unicore32",32, VIR_ARCH_LITTLE_ENDIAN },
 { "x86_64",   64, VIR_ARCH_LITTLE_ENDIAN },
+
 { "xtensa",   32, VIR_ARCH_LITTLE_ENDIAN },
 { "xtensaeb", 32, VIR_ARCH_BIG_ENDIAN },
 };
diff --git a/src/util/virarch.h b/src/util/virarch.h
index af5ff83528..a52de7f52c 100644
--- a/src/util/virarch.h
+++ b/src/util/virarch.h
@@ -55,15 +55,18 @@ typedef enum {
 VIR_ARCH_PPC64LE,  /* PowerPC 64 LE 
http://en.wikipedia.org/wiki/PowerPC */
 VIR_ARCH_PPCEMB,   /* PowerPC 32 BE 
http://en.wikipedia.org/wiki/PowerPC */
 
+VIR_ARCH_RISCV32,  /* RISC-V  64 LE 
http://en.wikipedia.org/wiki/RISC-V */
+VIR_ARCH_RISCV64,  /* RISC-V  32 BE 
http://en.wikipedia.org/wiki/RISC-V */
 VIR_ARCH_S390, /* S39032 BE 
http://en.wikipedia.org/wiki/S390 */
 VIR_ARCH_S390X,/* S39064 BE 
http://en.wikipedia.org/wiki/S390x */
 VIR_ARCH_SH4,  /* SuperH4 32 LE 
http://en.wikipedia.org/wiki/SuperH */
+
 VIR_ARCH_SH4EB,/* SuperH4 32 BE 
http://en.wikipedia.org/wiki/SuperH */
 VIR_ARCH_SPARC,/* Sparc   32 BE 
http://en.wikipedia.org/wiki/Sparc */
-
 VIR_ARCH_SPARC64,  /* Sparc   64 BE 
http://en.wikipedia.org/wiki/Sparc */
 VIR_ARCH_UNICORE32,/* UniCore 32 LE 
http://en.wikipedia.org/wiki/Unicore*/
 VIR_ARCH_X86_64,   /* x86 64 LE 
http://en.wikipedia.org/wiki/X86 */
+
 VIR_ARCH_XTENSA,   /* XTensa  32 LE 
http://en.wikipedia.org/wiki/Xtensa#Processor_Cores */
 VIR_ARCH_XTENSAEB, /* XTensa  32 BE 
http://en.wikipedia.org/wiki/Xtensa#Processor_Cores */
 
@@ -87,6 +90,9 @@ typedef enum {
  (arch) == VIR_ARCH_ARMV7B ||\
  (arch) == VIR_ARCH_AARCH64)
 
+# define ARCH_IS_RISCV(arch) ((arch) == VIR_ARCH_RISCV32 ||\
+  (arch) == VIR_ARCH_RISCV64)
+
 # define ARCH_IS_S390(arch) ((arch) == VIR_ARCH_S390 ||\
  (arch) == VIR_ARCH_S390X)
 
-- 
2.17.1

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

[libvirt] [PATCH v3 5/9] qemu: add qemuDomainIsRISCVVirt() and qemuDomainMachineIsRISCVVirt()

[Lubomir Rintel]

Signed-off-by: Lubomir Rintel 
---
 src/qemu/qemu_domain.c | 22 ++
 src/qemu/qemu_domain.h |  3 +++
 2 files changed, 25 insertions(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 60b4a9b412..b43e9b76d4 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -9477,6 +9477,28 @@ qemuDomainMachineIsARMVirt(const char *machine,
 }
 
 
+bool
+qemuDomainIsRISCVVirt(const virDomainDef *def)
+{
+return qemuDomainMachineIsRISCVVirt(def->os.machine, def->os.arch);
+}
+
+
+bool
+qemuDomainMachineIsRISCVVirt(const char *machine,
+ const virArch arch)
+{
+if (!ARCH_IS_RISCV(arch))
+return false;
+
+if (STRNEQ(machine, "virt") &&
+!STRPREFIX(machine, "virt-"))
+return false;
+
+return true;
+}
+
+
 bool
 qemuDomainIsPSeries(const virDomainDef *def)
 {
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 898b5b4479..be9c4b7d61 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -810,6 +810,7 @@ bool qemuDomainHasPCIeRoot(const virDomainDef *def);
 bool qemuDomainNeedsFDC(const virDomainDef *def);
 bool qemuDomainIsS390CCW(const virDomainDef *def);
 bool qemuDomainIsARMVirt(const virDomainDef *def);
+bool qemuDomainIsRISCVVirt(const virDomainDef *def);
 bool qemuDomainIsPSeries(const virDomainDef *def);
 bool qemuDomainHasBuiltinIDE(const virDomainDef *def);
 
@@ -819,6 +820,8 @@ bool qemuDomainMachineNeedsFDC(const char *machine);
 bool qemuDomainMachineIsS390CCW(const char *machine);
 bool qemuDomainMachineIsARMVirt(const char *machine,
 const virArch arch);
+bool qemuDomainMachineIsRISCVVirt(const char *machine,
+  const virArch arch);
 bool qemuDomainMachineIsPSeries(const char *machine,
 const virArch arch);
 bool qemuDomainMachineHasBuiltinIDE(const char *machine);
-- 
2.17.1

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

[libvirt] [PATCH v3 1/9] qemu: rename qemuDomainMachineIsVirt()

[Lubomir Rintel]

It's ARM specific.

Signed-off-by: Lubomir Rintel 
---
 src/qemu/qemu_capabilities.c | 2 +-
 src/qemu/qemu_domain.c   | 6 +++---
 src/qemu/qemu_domain.h   | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 9edb4ca4d4..d9b1089c59 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -5335,7 +5335,7 @@ virQEMUCapsFillDomainFeatureGICCaps(virQEMUCapsPtr 
qemuCaps,
 virDomainCapsFeatureGICPtr gic = >gic;
 virGICVersion version;
 
-if (!qemuDomainMachineIsVirt(domCaps->machine, domCaps->arch))
+if (!qemuDomainMachineIsARMVirt(domCaps->machine, domCaps->arch))
 return 0;
 
 for (version = VIR_GIC_VERSION_LAST - 1;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index f570081e3a..ad7f4091f6 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -9455,13 +9455,13 @@ qemuDomainMachineIsS390CCW(const char *machine)
 bool
 qemuDomainIsVirt(const virDomainDef *def)
 {
-return qemuDomainMachineIsVirt(def->os.machine, def->os.arch);
+return qemuDomainMachineIsARMVirt(def->os.machine, def->os.arch);
 }
 
 
 bool
-qemuDomainMachineIsVirt(const char *machine,
-const virArch arch)
+qemuDomainMachineIsARMVirt(const char *machine,
+   const virArch arch)
 {
 if (arch != VIR_ARCH_ARMV7L &&
 arch != VIR_ARCH_AARCH64)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index bff293fc0a..f883780215 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -817,8 +817,8 @@ bool qemuDomainMachineIsQ35(const char *machine);
 bool qemuDomainMachineIsI440FX(const char *machine);
 bool qemuDomainMachineNeedsFDC(const char *machine);
 bool qemuDomainMachineIsS390CCW(const char *machine);
-bool qemuDomainMachineIsVirt(const char *machine,
- const virArch arch);
+bool qemuDomainMachineIsARMVirt(const char *machine,
+const virArch arch);
 bool qemuDomainMachineIsPSeries(const char *machine,
 const virArch arch);
 bool qemuDomainMachineHasBuiltinIDE(const char *machine);
-- 
2.17.1

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

[libvirt] [PATCH v3 6/9] qemu: add qemuDomainAssignVirtioMMIOAddresses()

[Lubomir Rintel]

Signed-off-by: Lubomir Rintel 
---
 src/qemu/qemu_domain_address.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 551883e989..317f280f92 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -485,6 +485,14 @@ qemuDomainAssignARMVirtioMMIOAddresses(virDomainDefPtr def,
 }
 
 
+static void
+qemuDomainAssignVirtioMMIOAddresses(virDomainDefPtr def,
+virQEMUCapsPtr qemuCaps)
+{
+qemuDomainAssignARMVirtioMMIOAddresses(def, qemuCaps);
+}
+
+
 /**
  * qemuDomainDeviceCalculatePCIConnectFlags:
  *
@@ -2923,7 +2931,7 @@ qemuDomainAssignAddresses(virDomainDefPtr def,
 if (qemuDomainAssignS390Addresses(def, qemuCaps) < 0)
 return -1;
 
-qemuDomainAssignARMVirtioMMIOAddresses(def, qemuCaps);
+qemuDomainAssignVirtioMMIOAddresses(def, qemuCaps);
 
 if (qemuDomainAssignPCIAddresses(def, qemuCaps, driver, obj) < 0)
 return -1;
-- 
2.17.1

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

[libvirt] [PATCH v3 2/9] qemu: rename qemuDomainArmVirt()

[Lubomir Rintel]

It's ARM specific.

Signed-off-by: Lubomir Rintel 
---
 src/qemu/qemu_capabilities.c   |  4 ++--
 src/qemu/qemu_command.c|  4 ++--
 src/qemu/qemu_domain.c | 16 
 src/qemu/qemu_domain.h |  2 +-
 src/qemu/qemu_domain_address.c |  4 ++--
 5 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index d9b1089c59..d2f7e9efa6 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -1787,10 +1787,10 @@ bool virQEMUCapsHasPCIMultiBus(virQEMUCapsPtr qemuCaps,
 return false;
 }
 
-/* If 'virt' supports PCI, it supports multibus.
+/* If ARM 'virt' supports PCI, it supports multibus.
  * No extra conditions here for simplicity.
  */
-if (qemuDomainIsVirt(def))
+if (qemuDomainIsARMVirt(def))
 return true;
 
 return false;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ec46db5c36..334f14913a 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2948,7 +2948,7 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd,
 if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB &&
 cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT &&
 !qemuDomainIsQ35(def) &&
-!qemuDomainIsVirt(def)) {
+!qemuDomainIsARMVirt(def)) {
 
 /* An appropriate default USB controller model should already
  * have been selected in qemuDomainDeviceDefPostParse(); if
@@ -2986,7 +2986,7 @@ qemuBuildControllerDevCommandLine(virCommandPtr cmd,
 
 if (usbcontroller == 0 &&
 !qemuDomainIsQ35(def) &&
-!qemuDomainIsVirt(def) &&
+!qemuDomainIsARMVirt(def) &&
 !ARCH_IS_S390(def->os.arch)) {
 /* We haven't added any USB controller yet, but we haven't been asked
  * not to add one either. Add a legacy USB controller, unless we're
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ad7f4091f6..fcb37d45e6 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3199,7 +3199,7 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def,
 case VIR_ARCH_AARCH64:
 addDefaultUSB = false;
 addDefaultMemballoon = false;
-if (qemuDomainIsVirt(def))
+if (qemuDomainIsARMVirt(def))
 addPCIeRoot = virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_GPEX);
 break;
 
@@ -3380,7 +3380,7 @@ qemuDomainDefEnableDefaultFeatures(virDomainDefPtr def,
  * was not included in the domain XML, we need to choose a suitable
  * GIC version ourselves */
 if ((def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ABSENT &&
- qemuDomainIsVirt(def)) ||
+ qemuDomainIsARMVirt(def)) ||
 (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON &&
  def->gic_version == VIR_GIC_VERSION_NONE)) {
 virGICVersion version;
@@ -3832,7 +3832,7 @@ qemuDomainDefValidateFeatures(const virDomainDef *def,
 
 case VIR_DOMAIN_FEATURE_GIC:
 if (def->features[i] == VIR_TRISTATE_SWITCH_ON &&
-!qemuDomainIsVirt(def)) {
+!qemuDomainIsARMVirt(def)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("The '%s' feature is not supported for "
  "architecture '%s' or machine type '%s'"),
@@ -4245,7 +4245,7 @@ qemuDomainChrDefValidate(const virDomainChrDef *dev,
 isCompatible = false;
 }
 
-if (!qemuDomainIsVirt(def) &&
+if (!qemuDomainIsARMVirt(def) &&
 (dev->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SYSTEM ||
  dev->targetModel == VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_PL011)) {
 isCompatible = false;
@@ -5770,7 +5770,7 @@ qemuDomainDefaultNetModel(const virDomainDef *def,
 if (STREQ(def->os.machine, "versatilepb"))
 return "smc91c111";
 
-if (qemuDomainIsVirt(def))
+if (qemuDomainIsARMVirt(def))
 return "virtio";
 
 /* Incomplete. vexpress (and a few others) use this, but not all
@@ -6070,7 +6070,7 @@ qemuDomainChrDefPostParse(virDomainChrDefPtr chr,
 chr->targetType = VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_ISA;
 } else if (qemuDomainIsPSeries(def)) {
 chr->targetType = VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SPAPR_VIO;
-} else if (qemuDomainIsVirt(def)) {
+} else if (qemuDomainIsARMVirt(def)) {
 chr->targetType = VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SYSTEM;
 } else if (ARCH_IS_S390(def->os.arch)) {
 chr->targetType = VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_SCLP;
@@ -6264,7 +6264,7 @@ qemuDomainDeviceVideoDefPostParse(virDomainVideoDefPtr 
video,
 if (video->type == VIR_DOMAIN_VIDEO_TYPE_DEFAULT) {
 if (ARCH_IS_PPC64(def->os.arch))
 video->type = VIR_DOMAIN_VIDEO_TYPE_VGA;
-else if 

[libvirt] [PATCH v3 9/9] news: Add a mention of RISC-V guest support

[Lubomir Rintel]

Signed-off-by: Lubomir Rintel 
---
 docs/news.xml | 8 
 1 file changed, 8 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml
index c6d03f5556..2b6d08dc02 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -53,6 +53,14 @@
   bandwidth by using the memorytune element in 
cputune.
 
   
+  
+
+  qemu: Add support for RISC-V guests
+
+
+  riscv32 and riscv64 guest architectures are now supported.
+
+  
 
 
   
-- 
2.17.1

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

Re: [libvirt] [jenkins-ci PATCH v2 00/13] lcitool: Add 'build' action

[Andrea Bolognani]

On Wed, 2018-08-22 at 10:38 +0200, Erik Skultety wrote:
> On Fri, Aug 17, 2018 at 04:18:16PM +0200, Andrea Bolognani wrote:
> > Changes from [v1]:
> > 
> >   * rebase on top of master (985ab833be9b) and integrate recent
> > changes to build rules on the Jenkins side;
> > 
> >   * build on more targets.
> > 
> > [v1] https://www.redhat.com/archives/libvir-list/2018-August/msg00393.html
> 
> Since the first commit was pushed as part of a different series, it causes
> issues when applying this one (I have to apply most of them manually and even
> fix some failed hunks). Would you mind rebase the series and resend?

Sure, I was planning to do that anyway in order to sync up with
the latest changes made to the Jenkins side. It'll be on the list
in a bit :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH] virsh: Don't break loop of domblkinfo for disks

[Han Han]

On Tue, Aug 21, 2018 at 11:26 PM, Peter Krempa  wrote:

> On Tue, Aug 21, 2018 at 21:23:42 +0800, Han Han wrote:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1619625
> >
> > --all option is added to cmdDomblkinfo since commit 62c39193 allowing to
> > show all block devices info. Remove its 'goto cleanup' part in case it
> breaks
> > the loop of domblkinfo for all disks. Remove unnecessary variables and
> the
> > condition part after virDomainGetBlockInfo returning fail.
> >
> > Signed-off-by: Han Han 
> > ---
> >  tools/virsh-domain-monitor.c | 18 ++
> >  1 file changed, 2 insertions(+), 16 deletions(-)
> >
> > diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
> > index b9b4f9739b..ee926baae8 100644
> > --- a/tools/virsh-domain-monitor.c
> > +++ b/tools/virsh-domain-monitor.c
> > @@ -476,7 +476,6 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
> >  size_t i;
> >  xmlNodePtr *disks = NULL;
> >  char *target = NULL;
> > -char *protocol = NULL;
> >
> >  if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
> >  return false;
> > @@ -490,7 +489,6 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
> >  human = vshCommandOptBool(cmd, "human");
> >
> >  if (all) {
> > -bool active = virDomainIsActive(dom) == 1;
> >  int rc;
> >
> >  if (virshDomainGetXML(ctl, cmd, 0, , ) < 0)
> > @@ -505,29 +503,18 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
> >
> >  for (i = 0; i < ndisks; i++) {
> >  ctxt->node = disks[i];
> > -protocol = virXPathString("string(./source/@protocol)",
> ctxt);
> >  target = virXPathString("string(./target/@dev)", ctxt);
>
> Well, here you can check also whether the  element is present
>
Agree. However, I think it is better not to skip virDomainGetBlockInfo for
empty cdrom.
I expect the empty cdrom output will be:

Target CapacityAllocation  Physical
-
sda  -  - -

> ...
>
> >
> >  rc = virDomainGetBlockInfo(dom, target, , 0);
>
> ... and skip this.
>
> >
> >  if (rc < 0) {
> > -/* If protocol is present that's an indication of a
> networked
> > - * storage device which cannot provide statistics, so
> generate
> > - * 0 based data and get the next disk. */
> > -if (protocol && !active &&
> > -virGetLastErrorCode() == VIR_ERR_INTERNAL_ERROR &&
> > -virGetLastErrorDomain() == VIR_FROM_STORAGE) {
>
I will add one checking of source element here to skip the error from
cdrom.

> > -memset(, 0, sizeof(info));
> > -vshResetLibvirtError();
> > -} else {
> > -goto cleanup;
> > -}
> > +memset(, 0, sizeof(info));
> > +vshResetLibvirtError();
>
> Rather than skipping errors.
>
> This solution may skip other errors as well. Depends whether we are okay
> with that in such case.
>
> On the other hand getting rid of the code above looks good.
>
> >  }
> >
> >  cmdDomblkinfoPrint(ctl, , target, human, false);
> >
> >  VIR_FREE(target);
> > -VIR_FREE(protocol);
> >  }
> >  } else {
> >  if (virDomainGetBlockInfo(dom, device, , 0) < 0)
> > @@ -541,7 +528,6 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
> >   cleanup:
> >  virshDomainFree(dom);
> >  VIR_FREE(target);
> > -VIR_FREE(protocol);
> >  VIR_FREE(disks);
> >  xmlXPathFreeContext(ctxt);
> >  xmlFreeDoc(xmldoc);
> > --
> > 2.18.0
> >
> > --
> > libvir-list mailing list
> > libvir-list@redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list
>



-- 
Best regards,
---
Han Han
Quality Engineer
Redhat.

Email: h...@redhat.com
Phone: +861065339333
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Qemu-devel] [PATCH] Remove deprecated -balloon option

[David Hildenbrand]

On 21.08.2018 12:05, Thomas Huth wrote:
> The "-balloon" option has been replaced by "-device virtio-balloon".
> It's been marked as deprecated since two releases, and nobody
> complained, so let's remove it now.
> 
> Signed-off-by: Thomas Huth 
> ---
>  docs/virtio-balloon-stats.txt |  6 +++---
>  qemu-deprecated.texi  |  5 -
>  qemu-options.hx   | 10 --
>  vl.c  | 36 
>  4 files changed, 3 insertions(+), 54 deletions(-)
> 
> diff --git a/docs/virtio-balloon-stats.txt b/docs/virtio-balloon-stats.txt
> index 9985e1d..1732cc8 100644
> --- a/docs/virtio-balloon-stats.txt
> +++ b/docs/virtio-balloon-stats.txt
> @@ -61,9 +61,9 @@ It's also important to note the following:
> respond to the request the timer will never be re-armed, which has
> the same effect as disabling polling
>  
> -Here are a few examples. QEMU is started with '-balloon virtio', which
> -generates '/machine/peripheral-anon/device[1]' as the QOM path for the
> -balloon device.
> +Here are a few examples. QEMU is started with '-device virtio-balloon',
> +which generates '/machine/peripheral-anon/device[1]' as the QOM path for
> +the balloon device.
>  
>  Enable polling with 2 seconds interval:
>  
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 67b7211..0714017 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -106,11 +106,6 @@ enabled via the ``-machine usb=on'' argument.
>  
>  The ``-nodefconfig`` argument is a synonym for ``-no-user-config``.
>  
> -@subsection -balloon (since 2.12.0)
> -
> -The @option{--balloon virtio} argument has been superseded by
> -@option{--device virtio-balloon}.
> -
>  @subsection -fsdev handle (since 2.12.0)
>  
>  The ``handle'' fsdev backend does not support symlinks and causes the 9p
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 4efdedf..47c6b92 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -454,16 +454,6 @@ modprobe i810_audio clocking=48000
>  @end example
>  ETEXI
>  
> -DEF("balloon", HAS_ARG, QEMU_OPTION_balloon,
> -"-balloon virtio[,addr=str]\n"
> -"enable virtio balloon device (deprecated)\n", 
> QEMU_ARCH_ALL)
> -STEXI
> -@item -balloon virtio[,addr=@var{addr}]
> -@findex -balloon
> -Enable virtio balloon device, optionally with PCI address @var{addr}. This
> -option is deprecated, use @option{-device virtio-balloon} instead.
> -ETEXI
> -
>  DEF("device", HAS_ARG, QEMU_OPTION_device,
>  "-device driver[,prop[=value][,...]]\n"
>  "add device (based on driver)\n"
> diff --git a/vl.c b/vl.c
> index 16b913f..f952f01 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2127,36 +2127,6 @@ static void parse_display(const char *p)
>  }
>  }
>  
> -static int balloon_parse(const char *arg)
> -{
> -QemuOpts *opts;
> -
> -warn_report("This option is deprecated. "
> -"Use '--device virtio-balloon' to enable the balloon 
> device.");
> -
> -if (strcmp(arg, "none") == 0) {
> -return 0;
> -}
> -
> -if (!strncmp(arg, "virtio", 6)) {
> -if (arg[6] == ',') {
> -/* have params -> parse them */
> -opts = qemu_opts_parse_noisily(qemu_find_opts("device"), arg + 7,
> -   false);
> -if (!opts)
> -return  -1;
> -} else {
> -/* create empty opts */
> -opts = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
> -_abort);
> -}
> -qemu_opt_set(opts, "driver", "virtio-balloon", _abort);
> -return 0;
> -}
> -
> -return -1;
> -}
> -
>  char *qemu_find_file(int type, const char *name)
>  {
>  int i;
> @@ -3659,12 +3629,6 @@ int main(int argc, char **argv, char **envp)
>  case QEMU_OPTION_no_hpet:
>  no_hpet = 1;
>  break;
> -case QEMU_OPTION_balloon:
> -if (balloon_parse(optarg) < 0) {
> -error_report("unknown -balloon argument %s", optarg);
> -exit(1);
> -}
> -break;
>  case QEMU_OPTION_no_reboot:
>  no_reboot = 1;
>  break;
> 

Nice to see this go.

Reviewed-by: David Hildenbrand 

-- 

Thanks,

David / dhildenb

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

Re: [libvirt] [PATCH v3 09/12] qemu: Generate and use zPCI device in QEMU command line

[Andrea Bolognani]

On Wed, 2018-08-22 at 11:06 +0800, Yi Min Zhao wrote:
> 在 2018/8/20 下午7:14, Andrea Bolognani 写道:
> > So which is unique: uid, fid, or the combination of the two?
> > Could I have
> > 
> >-device zpci,uid=1,fid=1
> >-device zpci,uid=1,fid=2
> > 
> > or would that not work? What about
> > 
> >-device zpci,uid=1,fid=1
> >-device zpci,uid=2,fid=1
> > 
> > would that be okay?
> 
> Both can't work. FID must be unique and UID must be also unique.
> They are independent.

Got it, thanks.

> > My point is that a funtion called fooIsBaz() should only ever
> > check whether the foo in question is indeed a baz, without taking
> > any other action such as reporting errors. Leave that to the
> > caller, or give the function a different name.
> 
> I think moving to the caller is proper.
> 
> > qemuDomainDeviceDefValidate() looks like a reasonable entry point
> > for checks such as "you specified a zPCI address but this is not
> > an s390 guest" or "your configuration requires zPCI but the QEMU
> > binary doesn't support that feature". Both of the cases above
> > should have proper test suite coverage, by the way.
> 
> How about my idea mentioned above?

You mean moving the check to the caller?

I think qemuDomainDeviceDefValidate() is a better choice because
it should allow you to implement the checks once, potentially with
more context such as information about the domain and QEMU
capabilities, and have them performed regardless of whether the
device is being processed eg. during regular parsing or hotplug.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [jenkins-ci PATCH v2 00/13] lcitool: Add 'build' action

[Erik Skultety]

On Fri, Aug 17, 2018 at 04:18:16PM +0200, Andrea Bolognani wrote:
> Changes from [v1]:
>
>   * rebase on top of master (985ab833be9b) and integrate recent
> changes to build rules on the Jenkins side;
>
>   * build on more targets.
>
> [v1] https://www.redhat.com/archives/libvir-list/2018-August/msg00393.html

Since the first commit was pushed as part of a different series, it causes
issues when applying this one (I have to apply most of them manually and even
fix some failed hunks). Would you mind rebase the series and resend?

Erik

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

Re: [libvirt] [PATCH] qemu: Start domain on a node without cpu affinity

[Peter Krempa]

On Wed, Aug 22, 2018 at 00:31:29 +0300, Roman Bolshakov wrote:
> libvirt affinity wrappers don't support macOS Thread Affinity API:
> https://developer.apple.com/library/archive/releasenotes/Performance/RN-AffinityAPI/index.html
> 
> virProcessSetAffinity stub prevents libvirt from starting a qemu domain
> on macOS:
> 
> $ virsh start vm
> error: Failed to start domain vm
> error: Process CPU affinity is not supported on this platform: Function not 
> implemented
> 
> With the patch a VM can be started on macOS but some affinity-related
> commands will return errors:
> 
> $ virsh vcpuinfo vm
> error: Requested operation is not valid: cpu affinity is not supported
> 
> $ virsh vcpupin vm
> VCPU: CPU Affinity
> --
>0: 0-7
> 
> $ virsh vcpupin vm --live --vcpu 0 --cpulist 7
> error: operation failed: Virt type 'qemu' does not support vCPU pinning
> 
> $ virsh emulatorpin vm
> emulator: CPU Affinity
> --
>*: 0-7
> 
> $ virsh emulatorpin vm --live --cpulist 7
> error: Requested operation is not valid: cpu affinity is not supported
> 
> The patch also fixes virmacmaptest on macOS
> 
> Signed-off-by: Roman Bolshakov 
> ---
>  src/qemu/qemu_driver.c | 6 ++
>  src/util/virprocess.c  | 2 +-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 21e9e87ddd..2e225b1ede 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5245,6 +5245,12 @@ qemuDomainPinEmulator(virDomainPtr dom,
>  if (virDomainPinEmulatorEnsureACL(dom->conn, vm->def, flags) < 0)
>  goto cleanup;
>  
> +if (!qemuDomainHasVcpuPids(vm)) {
> +virReportError(VIR_ERR_OPERATION_INVALID,
> +   "%s", _("cpu affinity is not supported"));
> +goto cleanup;
> +}
> +
>  if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
>  goto cleanup;
>  
> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> index 3988f5546c..7eaafd29f2 100644
> --- a/src/util/virprocess.c
> +++ b/src/util/virprocess.c
> @@ -598,7 +598,7 @@ int virProcessSetAffinity(pid_t pid ATTRIBUTE_UNUSED,
>  {
>  virReportSystemError(ENOSYS, "%s",
>   _("Process CPU affinity is not supported on this 
> platform"));
> -return -1;
> +return 0;

This is not a good idea. The caller should make sure that it does not
call this function if we need to support such a code path.

If you blindly return 0 here other callers might get confused.

Also setting an error and returning success is generally wrong.


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

Re: [libvirt] [PATCH] doc: better explanation for virStoragePoolSetAutostart

[Erik Skultety]

On Wed, Aug 22, 2018 at 08:43:32AM +0300, Dan Kenigsberg wrote:
> The former documentation was an unhelpful tautology. The suggested doc
> mimicks^Wcopies the one of virDomainSetAutostart.
>
> Signed-off-by: Dan Kenigsberg 
> ---
>  src/libvirt-storage.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/libvirt-storage.c b/src/libvirt-storage.c
> index 1879c6bd60..ed305b9b5e 100644
> --- a/src/libvirt-storage.c
> +++ b/src/libvirt-storage.c
> @@ -1127,7 +1127,8 @@ virStoragePoolGetAutostart(virStoragePoolPtr pool,
>   * @pool: pointer to storage pool
>   * @autostart: new flag setting
>   *
> - * Sets the autostart flag
> + * Configure the storage pool to be automatically started
> + * when the host machine boots.

I rewrote the commit message a tiny bit and also changed the description of
@autostart argument to be consistent with the other uses too.

Reviewed-by: Erik Skultety 

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