[libvirt] [PATCH] qemu: add a check for node set when build memory device cmd

[Luyao Huang]

When we set a host not exist nodemask in memory device and then
start the vm, qemu will report error.

 # virsh start test3
 error: Failed to start domain test3
 error: internal error: process exited while connecting to monitor:
 2015-03-25T01:12:17.205913Z qemu-kvm: -object memory-backend-ram,id=memdimm0
 ,size=536870912,host-nodes=1-3,policy=bind: cannot bind memory to host NUMA 
nodes:
 Invalid argument

We have some function to check this, and add a check when build
memory cmd line will report error more early and clearly. And the
check will be done when we start a vm have memory device and hotplug
a memory device. The error will be:

 # virsh start test3
 error: Failed to start domain test3
 error: internal error: NUMA node 1 is unavailable

Signed-off-by: Luyao Huang 
---
 src/qemu/qemu_command.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 99a19d6..04c8df7 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4757,6 +4757,8 @@ qemuBuildMemoryBackendStr(unsigned long long size,
 }
 
 if (nodemask) {
+if (!virNumaNodesetIsAvailable(nodemask))
+goto cleanup;
 if (virJSONValueObjectAdd(props,
   "m:host-nodes", nodemask,
   "S:policy", qemuNumaPolicyTypeToString(mode),
-- 
1.8.3.1

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

[libvirt] [PATCH 0/2] fix domain documentation error

[Chen Fan]

Chen Fan (2):
  docs: no 'via' attribute in route element
  docs: route element must specify network address

 docs/formatdomain.html.in | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

-- 
1.9.3

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

[libvirt] [PATCH 1/2] docs: no 'via' attribute in route element

[Chen Fan]

via -> gateway

Signed-off-by: Chen Fan 
---
 docs/formatdomain.html.in | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 82aa14f..3b3d2d9 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -4471,8 +4471,8 @@ qemu-kvm -net nic,model=? /dev/null
   
   
   
-  
-  
+  
+  
 
 ...
 
@@ -4480,8 +4480,8 @@ qemu-kvm -net nic,model=? /dev/null
 eth0
   
   
-  
-  
+  
+  
 
 
   
-- 
1.9.3

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

[libvirt] [PATCH 2/2] docs: route element must specify network address

[Chen Fan]

because network address is required by route, so
here we should add one avoid user misunderstand.

Signed-off-by: Chen Fan 
---
 docs/formatdomain.html.in | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 3b3d2d9..d7fe942 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -4472,7 +4472,7 @@ qemu-kvm -net nic,model=? /dev/null
   
   
   
-  
+  
 
 ...
 
@@ -4481,7 +4481,7 @@ qemu-kvm -net nic,model=? /dev/null
   
   
   
-  
+  
 
 
   
-- 
1.9.3

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

Re: [libvirt] [PATCH 00/10] libxl: switch driver to use a single libxl_ctx

[Jim Fehlig]

Michal Privoznik wrote:
> On 18.02.2015 04:56, Jim Fehlig wrote:
>   
>> This series is a follow up to
>>
>> https://www.redhat.com/archives/libvir-list/2015-February/msg00024.html
>>
>> It goes a step further and changes the libxl driver to use one,
>> driver-wide libxl_ctx.  Currently the libxl driver has one driver-wide
>> ctx for operations that are not domain-specific and a ctx for each
>> domain.  This approach was necessary back in the old Xen4.1 libxl days,
>> but with the newer libxl it is more of a hinderance than benefit.
>> Ian Jackson suggested moving to a single ctx while discussing some
>> deadlocks and assertions encountered in the libxl driver when under
>> load from tests such as OpenStack Tempest.
>>
>> Making such a change involves quite a bit of code movement.  I've tried
>> to split that up into a reviewable series,  the result of which are the
>> 9 patches that follow.  I've ran this through all of my automated tests
>> as well as some hacky tests I created to reproduce failures revealed by
>> Tempest.
>>
>> One downside of moving to a single ctx is losing the per-domain log
>> files.  Currently, a single log stream can be associated with ctx, hence
>> all logging from libxl will go to a single file.  Ian is going to
>> investigate possibilities to accommodate per-domain log files in libxl,
>> but in the meantime folks using Xen are accustomed to a single
>> log file from the xend days.
>>
>> I've been testing this series on xen-unstable and Xen 4.4.1 + commits
>> 2ffeb5d7, 4b9143e4, 5a968257, 60ce518a, 66bff9fd, 77a1bf37, f49f9b41,
>> 6b5a5bba, 93699882d, f1335f0d, and 8bc64413.  Results are much better
>> than before applying the series, but I do notice a stuck hypercall
>> after many (hundreds) concurrent domain create/destroy operations.
>> The single libxl_ctx is locked in the callpath, essentially deadlocking
>> the driver.
>>
>> Thread 1 (Thread 0x7f0649a198c0 (LWP 2235)):
>>  0  0x7f0645272397 in ioctl () from /lib64/libc.so.6
>>  1  0x7f0645d8e353 in linux_privcmd_hypercall (xch=,
>> h=, hypercall=) at xc_linux_osdep.c:134
>>  2  0x7f0645d854b8 in do_xen_hypercall (xch=xch@entry=0x7f0630039390, 
>> hypercall=hypercall@entry=0x7fffd53f80e0) at xc_private.c:249
>>  3  0x7f0645d86aa4 in do_sysctl (sysctl=sysctl@entry=0x7fffd53f8080, 
>> xch=xch@entry=0x7f0630039390) at xc_private.h:281
>>  4  xc_sysctl (xch=xch@entry=0x7f0630039390,
>> sysctl=sysctl@entry=0x7fffd53f8170) at xc_private.c:656
>>  5  0x7f0645d7bfbf in xc_domain_getinfolist (xch=0x7f0630039390, 
>> first_domain=first_domain@entry=119, max_domains=max_domains@entry=1, 
>> info=info@entry=0x7fffd53f8260) at xc_domain.c:382
>>  6  0x7f0645fabca6 in domain_death_xswatch_callback
>> (egc=0x7fffd53f83f0, w=, wpath=,
>> epath=) at libxl.c:1041
>>  7  0x7f0645fd75a8 in watchfd_callback (egc=0x7fffd53f83f0,
>> ev=, fd=, events=,
>> revents=) at libxl_event.c:515
>>  8  0x7f0645fd8ac3 in libxl_osevent_occurred_fd (ctx=, 
>> for_libxl=, fd=,
>> events_ign=, revents_ign=) at
>> libxl_event.c:1259
>>  9  0x7f063a23402c in libxlFDEventCallback (watch=454, fd=33,
>> vir_events=1, fd_info=0x7f0608007e70) at libxl/libxl_driver.c:123
>>
>> There is no hint in any logs or dmesg suggesting a cause for the stuck
>> hypercall.  Any suggestions for further debugging tips appreciated.
>> 

FYI, this was not a hung hypercall, but looping clear back in frame 6
that I overlooked.  It was fixed in libxl by the following commit

http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=4783c99aab866f470bd59368cfbf5ad5f677b0ec

>> Jim Fehlig (10):
>>   libxl: remove redundant calls to libxl_evdisable_domain_death
>>   libxl: use libxl_ctx passed to libxlConsoleCallback
>>   libxl: use driver-wide ctx in fd and timer event handling
>>   libxl: Move setup of child processing code to driver initialization
>>   libxl: move event registration to driver initialization
>>   libxl: use global libxl_ctx in event handler
>>   libxl: remove unnecessary libxlDomainEventsRegister
>>   libxl: make libxlDomainFreeMem static
>>   libxl: remove per-domain libxl_ctx
>>   libxl: change libxl log stream to ERROR log level
>>
>>  src/libxl/libxl_conf.c  |   2 +-
>>  src/libxl/libxl_domain.c| 438 ++-
>>  src/libxl/libxl_domain.h|  27 +--
>>  src/libxl/libxl_driver.c| 484 
>> +++-
>>  src/libxl/libxl_migration.c |  17 +-
>>  5 files changed, 426 insertions(+), 542 deletions(-)
>>
>> 
>
> ACK series
>   

Thanks!  1 and 2 were pushed earlier as part of this trivial series

https://www.redhat.com/archives/libvir-list/2015-March/msg00102.html

I've now pushed 3-9, but held off on pushing 10 since it removes the
possibility to get debug level messages from libxl.  I think a better
approach would be to introduce /etc/libvirt/libxl.conf with a
'log_level' setting, giving users the ability t

Re: [libvirt] [PATCH 4/4] util: use netlink to create bridge devices

[John Ferlan]

On 03/23/2015 03:43 PM, Laine Stump wrote:
> Just as it is possible to delete a bridge device with the netlink
> RTM_DELLINK message, one can be created with the RTM_NEWLINK
> message. Because of differences in the format of the message, it's not
> as straightforward as with virNetlinkDelLink() to create a single
> utility function that can be used to create any type of interface, so
> the new netlink version of virNetDevBridgeCreate() does its own
> construction of the netlink message and calls virNetlinkCommand()
> itself.
> 
> This doesn't provide any extra functionality, just provides symmetry
> with the previous commit.
> 
> NB: We *could* alter the API of virNetDevBridgeCreate() to take a MAC
> address, and directly program that mac address into the bridge (by
> adding an IFLA_ADDRESS attribute, as is done in
> virNetDevMacVLanCreate()) rather than separately creating the "dummy
> tap" (e.g. virbr0-nic) to maintain a fixed mac address on the bridge,
> but the commit history of virnetdevbridge.c shows that the presence of
> this dummy tap is essential in some older versions of the kernel
> (between 2.6.39 and 3.1 or 3.2, possibly?) to proper operation of IPv6
> DAD, and I don't want to take the chance of breaking something that I
> don't have the time/setup to test (my RHEL6 box is at kernel
> 2.6.32-544, and the next lowest kernel I have is 3.17)
> ---
>  src/util/virnetdevbridge.c | 78 
> +-
>  1 file changed, 77 insertions(+), 1 deletion(-)
> 
What's here seems reasonable - I don't have "history" on my side to
answer the older versions of the kernel query...

One other difference between this and virNetDevMacVLanCreate is the
error path that handles *retry if the name is already in use...  Whether
that's a worthy addition here or not is your call...

ACK -

John

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

Re: [libvirt] [PATCH 3/4] util: use netlink to delete bridge devices

[John Ferlan]

On 03/23/2015 03:43 PM, Laine Stump wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1125755
> 
> reported that a stray bridge device was left on the system when a
> libvirt network failed to start due to an illegal iptables rule caused
> by bad config. Apparently the reason this was happening was that
> NetworkManager was noticing immediately when the bridge device was
> created and automatically setting it IFF_UP. libvirt would then try to
> setup the iptables rules, get an error back, and since libvirt had
> never IFF_UPed the bridge, it didn't expect that it needed to set it
> ~IFF_UP before deleting it during the cleanup process. But the
> ioctl(SIOCBRDELBR) ioctl will fail to delete a bridge if it is IFF_UP.
> 
> Since that bug was reported, NetworkManager has gotten a bit more
> polite in this respect, but just in case something similar happens in
> the future, this patch switches to using the netlink RTM_DELLINK
> message to delete the bridge - unlike SIOCBRDELBR, it will delete the
> requested bridge no matter what the setting of IFF_UP.
> ---
>  src/util/virnetdevbridge.c | 12 ++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 

ACK -

John

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

Re: [libvirt] [PATCH 2/4] util: replace body of virNetDevMacVLanDelete() with virNetlinkDelLink()

[John Ferlan]

On 03/23/2015 03:43 PM, Laine Stump wrote:
> These two functions are identical, so no sense in having the
> duplication. I resisted the temptation to replace calls to
> virNetDevMacVLanDelete() with calls to virNetlinkDelLink() just in
> case some mythical future platform has macvtap devices that aren't
> managed with netlink (or in case we some day need to do more than just
> tell the kernel to delete the device).
> ---
>  src/util/virnetdevmacvlan.c | 67 
> ++---
>  1 file changed, 2 insertions(+), 65 deletions(-)
> 


hmm interesting... Started reading the next patch and noticed
something... Perhaps I was quick on the trigger finger for this one...

This module was compiled with "if WITH_MACVTAP", but the API being
replaced/called uses "#if defined(__linux__) && defined(HAVE_LIBNL)"
thus this module never cared about linux & HAVE_LIBNL, but now depends
on an API that does. My reading comprehension of the Makefile in order
to determine whether anything matters is "limited"...

Of course this module has libnl calls in it already so perhaps either
WITH_MACVTAP implies __linux__ and HAVE_LIBNL or perhaps that something
that needs to be adjusted.  Not my specialty!

John



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

Re: [libvirt] [PATCH 1/4] util: netlink function to delete any network device

[John Ferlan]

On 03/23/2015 03:43 PM, Laine Stump wrote:
> libvirt has always used the netlink RTM_DELLINK message to delete
> macvtap/macvlan devices, but it can actually be used to delete other
> types of network devices, such as bonds and bridges. This patch makes
> virNetDevMacVLanDelete() available as a generic function so it can
> intelligibly be called to delete these other types of interfaces.
> ---
>  src/libvirt_private.syms |  1 +
>  src/util/virnetlink.c| 89 
> 
>  src/util/virnetlink.h|  3 +-
>  3 files changed, 92 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index ca3520d..631edf3 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1805,6 +1805,7 @@ virNetDevVPortProfileOpTypeToString;
>  
>  # util/virnetlink.h
>  virNetlinkCommand;
> +virNetlinkDelLink;
>  virNetlinkEventAddClient;
>  virNetlinkEventRemoveClient;
>  virNetlinkEventServiceIsRunning;
> diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
> index d52f66a..86c9c9c 100644
> --- a/src/util/virnetlink.c
> +++ b/src/util/virnetlink.c
> @@ -277,6 +277,87 @@ int virNetlinkCommand(struct nl_msg *nl_msg,
>  }
>  
>  
> +/**
> + * virNetlinkDelLink:
> + *
> + * @ifname: Name of the link
> + *
> + * delete a network "link" (aka interface aka device) with the given
> + * name. This works for many different types of network devices,
> + * including macvtap and bridges.
> + *
> + * Returns 0 on success, -1 on fatal error.
> + */
> +int
> +virNetlinkDelLink(const char *ifname)
> +{
> +int rc = -1;
> +struct nlmsghdr *resp = NULL;
> +struct nlmsgerr *err;
> +struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC };
> +unsigned int recvbuflen;
> +struct nl_msg *nl_msg;
> +
> +nl_msg = nlmsg_alloc_simple(RTM_DELLINK,
> +NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL);
> +if (!nl_msg) {
> +virReportOOMError();
> +return -1;
> +}
> +
> +if (nlmsg_append(nl_msg,  &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0)
> +goto buffer_too_small;
> +
> +if (nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0)
> +goto buffer_too_small;
> +
> +if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0,
> +  NETLINK_ROUTE, 0) < 0) {
> +goto cleanup;
> +}
> +
> +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) {
> +virReportSystemError(-err->error,
> + _("error destroying network device %s"),
> + ifname);
> +goto cleanup;
> +}
> +break;
> +
> +case NLMSG_DONE:
> +break;
> +
> +default:
> +goto malformed_resp;
> +}
> +
> +rc = 0;
> + cleanup:
> +nlmsg_free(nl_msg);
> +VIR_FREE(resp);
> +return rc;
> +
> + malformed_resp:
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("malformed netlink response message"));
> +goto cleanup;
> +
> + buffer_too_small:
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +   _("allocated netlink buffer is too small"));
> +goto cleanup;
> +}
> +
> +
>  int
>  virNetlinkGetErrorCode(struct nlmsghdr *resp, unsigned int recvbuflen)
>  {
> @@ -803,6 +884,14 @@ int virNetlinkCommand(struct nl_msg *nl_msg 
> ATTRIBUTE_UNUSED,
>  return -1;
>  }
>  
> +
> +int
> +virNetlinkDelLink(const char *ifname ATTRIBUTE_UNSUPPORTED)
> +{
> +virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported));
> +return -1;
> +}
> +
>  /**
>   * stopNetlinkEventServer: stop the monitor to receive netlink
>   * messages for libvirtd
> diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h
> index 1a3e06d..06c3cd0 100644
> --- a/src/util/virnetlink.h
> +++ b/src/util/virnetlink.h
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (C) 2010-2013 Red Hat, Inc.
> + * Copyright (C) 2010-2013, 2015 Red Hat, Inc.
>   * Copyright (C) 2010-2012 IBM Corporation
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -51,6 +51,7 @@ int virNetlinkCommand(struct nl_msg *nl_msg,
>struct nlmsghdr **resp, unsigned int *respbuflen,
>uint32_t src_pid, uint32_t dst_pid,
>unsigned int protocol, unsigned int groups);
> +int virNetlinkDelLink(const char *ifname);

Like the virNetDevMacVLanDelete which this is essentially "replacing" -
add a :

   ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;


ACK with the change

John
>  
>  int virNetlinkGetErrorCode(struct nlmsghdr *resp, unsigned int recvbuflen);
>  
> 

--
libvir-list mailing list
libvir-l

Re: [libvirt] [PATCH 2/4] util: replace body of virNetDevMacVLanDelete() with virNetlinkDelLink()

[John Ferlan]

On 03/23/2015 03:43 PM, Laine Stump wrote:
> These two functions are identical, so no sense in having the
> duplication. I resisted the temptation to replace calls to
> virNetDevMacVLanDelete() with calls to virNetlinkDelLink() just in
> case some mythical future platform has macvtap devices that aren't
> managed with netlink (or in case we some day need to do more than just
> tell the kernel to delete the device).
> ---
>  src/util/virnetdevmacvlan.c | 67 
> ++---
>  1 file changed, 2 insertions(+), 65 deletions(-)
> 

ACK -

John

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

Re: [libvirt] [PATCH V2] libxl: fix dom0 balloon logic

[Stefano Stabellini]

On Tue, 24 Mar 2015, Jim Fehlig wrote:
> Recent testing on large memory systems revealed a bug in the Xen xl
> tool's freemem() function.  When autoballooning is enabled, freemem()
> is used to ensure enough memory is available to start a domain,
> ballooning dom0 if necessary.  When ballooning large amounts of memory
> from dom0, freemem() would exceed its self-imposed wait time and
> return an error.  Meanwhile, dom0 continued to balloon.  Starting the
> domain later, after sufficient memory was ballooned from dom0, would
> succeed.  The libvirt implementation in libxlDomainFreeMem() suffers
> the same bug since it is modeled after freemem().
> 
> In the end, the best place to fix the bug on the Xen side was to
> slightly change the behavior of libxl_wait_for_memory_target().
> Instead of failing after caller-provided wait_sec, the function now
> blocks as long as dom0 memory ballooning is progressing.  It will return
> failure only when more memory is needed to reach the target and wait_sec
> have expired with no progress being made.  See xen.git commit fd3aa246.
> There was a dicussion on how this would affect other libxl apps like
> libvirt
> 
> http://lists.xen.org/archives/html/xen-devel/2015-03/msg00739.html
> 
> If libvirt containing this patch was build against a Xen containing
> the old libxl_wait_for_memory_target() behavior, libxlDomainFreeMem()
> will fail after 30 sec and domain creation will be terminated.
> Without this patch and with old libxl_wait_for_memory_target() behavior,
> libxlDomainFreeMem() does not succeed after 30 sec, but returns success
> anyway.  Domain creation continues resulting in all sorts of fun stuff
> like cpu soft lockups in the guest OS.  It was decided to properly fix
> libxl_wait_for_memory_target(), and if anything improve the default
> behavior of apps using the freemem reference impl in xl.
> 
> xl was patched to accommodate the change in libxl_wait_for_memory_target()
> with xen.git commit 883b30a0.  This patch does the same in the libxl
> driver.  While at it, I changed the logic to essentially match
> freemem() in $xensrc/tools/libxl/xl_cmdimpl.c.  It was a bit cleaner
> IMO and will make it easier to spot future, potentially interesting
> divergences.
> 
> Signed-off-by: Jim Fehlig 

Reviewed-by: Stefano Stabellini 


> V2: Actually use libxl_wait_for_memory_target(), instead of
> libxl_wait_for_free_memory()
> 
>  src/libxl/libxl_domain.c | 55 
> +++-
>  1 file changed, 26 insertions(+), 29 deletions(-)
> 
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index 407a9bd..a1739aa 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -1121,38 +1121,39 @@ libxlDomainFreeMem(libxlDomainObjPrivatePtr priv, 
> libxl_domain_config *d_config)
>  {
>  uint32_t needed_mem;
>  uint32_t free_mem;
> -size_t i;
> -int ret = -1;
> +int ret;
>  int tries = 3;
>  int wait_secs = 10;
>  
> -if ((ret = libxl_domain_need_memory(priv->ctx, &d_config->b_info,
> -&needed_mem)) >= 0) {
> -for (i = 0; i < tries; ++i) {
> -if ((ret = libxl_get_free_memory(priv->ctx, &free_mem)) < 0)
> -break;
> +ret = libxl_domain_need_memory(priv->ctx, &d_config->b_info, 
> &needed_mem);
> +if (ret < 0)
> +goto error;
>  
> -if (free_mem >= needed_mem) {
> -ret = 0;
> -break;
> -}
> +do {
> +ret = libxl_get_free_memory(priv->ctx, &free_mem);
> +if (ret < 0)
> +goto error;
>  
> -if ((ret = libxl_set_memory_target(priv->ctx, 0,
> -   free_mem - needed_mem,
> -   /* relative */ 1, 0)) < 0)
> -break;
> +if (free_mem >= needed_mem)
> +return 0;
>  
> -ret = libxl_wait_for_free_memory(priv->ctx, 0, needed_mem,
> - wait_secs);
> -if (ret == 0 || ret != ERROR_NOMEM)
> -break;
> +ret = libxl_set_memory_target(priv->ctx, 0,
> +  free_mem - needed_mem,
> +  /* relative */ 1, 0);
> +if (ret < 0)
> +goto error;
>  
> -if ((ret = libxl_wait_for_memory_target(priv->ctx, 0, 1)) < 0)
> -break;
> -}
> -}
> +ret = libxl_wait_for_memory_target(priv->ctx, 0, wait_secs);
> +if (ret < 0)
> +goto error;
>  
> -return ret;
> +tries--;
> +} while (tries > 0);
> +
> + error:
> +virReportSystemError(ret, "%s",
> + _("Failed to balloon domain0 memory"));
> +return -1;
>  }
>  
>  static void
> @@ -1271,12 +1272,8 @@ libxlDomainStart(libxlDriverPrivatePtr driver, 
> virDomainObjPtr vm,
> 

Re: [libvirt] [PATCH] Document that USB hostdevs do not need nodeDettach

[John Ferlan]

On 03/23/2015 01:16 PM, Ján Tomko wrote:
> The virNodeDeviceDettach API only works on PCI devices.
> 
> Originally added by commit 10d3272e, but the API never
> supported USB devices.
> 
> Reported by: Martin Polednik 
> ---
>  docs/formatdomain.html.in | 19 +--
>  tools/virsh.pod   | 17 -
>  2 files changed, 17 insertions(+), 19 deletions(-)
> 

ACK -

Should we say to call virNodeDeviceDetachFlags instead of
virNodeDeviceDettach?


John

> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 82aa14f..d6abe17 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -3114,21 +3114,20 @@
>  with additional attributes noted.
>  
>usb
> -  For USB devices, the user is responsible to call
> -virNodeDeviceDettach (or
> -virsh nodedev-detach) before starting the guest
> -or hot-plugging the device and virNodeDeviceReAttach
> -(or virsh nodedev-reattach) after hot-unplug or
> -stopping the guest.
> +  USB devices are detached from the host on guest startup
> +and reattached after the guest exits or the device is
> +hot-unplugged.
>
>pci
>For PCI devices, when managed is "yes" it is
>  detached from the host before being passed on to the guest
>  and reattached to the host after the guest exits. If
> -managed is omitted or "no", follow the steps
> -described for a USB device to detach before starting the
> -guest or hot-plugging and reattach after stopping the guest
> -or hot-unplug.
> +managed is omitted or "no", the user is
> +responsible to call virNodeDeviceDettach
> +(or virsh nodedev-detach before starting the guest
> +or hot-plugging the device and virNodeDeviceReAttach
> +(or virsh nodedev-reattach) after hot-unplug or
> +stopping the guest.
>
>scsi
>For SCSI devices, user is responsible to make sure the device
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 8262a45..4d825c1 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -2385,7 +2385,7 @@ attach taking effect the next time libvirt starts the 
> domain.
>  For cdrom and floppy devices, this command only replaces the media
>  within an existing device; consider using B for this
>  usage.  For passthrough host devices, see also B,
> -needed if the device does not use managed mode.
> +needed if the PCI device does not use managed mode.
>  
>  If I<--live> is specified, affect a running domain.
>  If I<--config> is specified, affect the next startup of a persistent domain.
> @@ -2646,15 +2646,14 @@ L.
>  
>  Passthrough devices cannot be simultaneously used by the host and its
>  guest domains, nor by multiple active guests at once.  If the
> - description includes the attribute B, and the
> -hypervisor driver supports it, then the device is in managed mode, and
> + description of a PCI device includes the attribute 
> B,
> +and the hypervisor driver supports it, then the device is in managed mode, 
> and
>  attempts to use that passthrough device in an active guest will
>  automatically behave as if B (guest start, device
>  hot-plug) and B (guest stop, device hot-unplug) were
> -called at the right points (currently, qemu does this for PCI devices,
> -but not USB).  If a device is not marked as managed, then it must
> -manually be detached before guests can use it, and manually reattached
> -to be returned to the host.  Also, if a device is manually detached,
> +called at the right points.  If a PCI device is not marked as managed,
> +then it must manually be detached before guests can use it, and manually
> +reattached to be returned to the host.  Also, if a device is manually 
> detached,
>  then the host does not regain control of the device without a matching
>  reattach, even if the guests use the device in managed mode.
>  
> @@ -2712,8 +2711,8 @@ I and I<--tree> are mutually exclusive.
>  
>  Declare that I is no longer in use by any guests, and that
>  the host can resume normal use of the device.  This is done
> -automatically for devices in managed mode, but must be done explicitly
> -to match any explicit B.
> +automatically for PCI devices in managed mode and USB devices, but
> +must be done explicitly to match any explicit B.
>  
>  =item B I
>  
> 

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

[libvirt] [PATCH V2] libxl: fix dom0 balloon logic

[Jim Fehlig]

Recent testing on large memory systems revealed a bug in the Xen xl
tool's freemem() function.  When autoballooning is enabled, freemem()
is used to ensure enough memory is available to start a domain,
ballooning dom0 if necessary.  When ballooning large amounts of memory
from dom0, freemem() would exceed its self-imposed wait time and
return an error.  Meanwhile, dom0 continued to balloon.  Starting the
domain later, after sufficient memory was ballooned from dom0, would
succeed.  The libvirt implementation in libxlDomainFreeMem() suffers
the same bug since it is modeled after freemem().

In the end, the best place to fix the bug on the Xen side was to
slightly change the behavior of libxl_wait_for_memory_target().
Instead of failing after caller-provided wait_sec, the function now
blocks as long as dom0 memory ballooning is progressing.  It will return
failure only when more memory is needed to reach the target and wait_sec
have expired with no progress being made.  See xen.git commit fd3aa246.
There was a dicussion on how this would affect other libxl apps like
libvirt

http://lists.xen.org/archives/html/xen-devel/2015-03/msg00739.html

If libvirt containing this patch was build against a Xen containing
the old libxl_wait_for_memory_target() behavior, libxlDomainFreeMem()
will fail after 30 sec and domain creation will be terminated.
Without this patch and with old libxl_wait_for_memory_target() behavior,
libxlDomainFreeMem() does not succeed after 30 sec, but returns success
anyway.  Domain creation continues resulting in all sorts of fun stuff
like cpu soft lockups in the guest OS.  It was decided to properly fix
libxl_wait_for_memory_target(), and if anything improve the default
behavior of apps using the freemem reference impl in xl.

xl was patched to accommodate the change in libxl_wait_for_memory_target()
with xen.git commit 883b30a0.  This patch does the same in the libxl
driver.  While at it, I changed the logic to essentially match
freemem() in $xensrc/tools/libxl/xl_cmdimpl.c.  It was a bit cleaner
IMO and will make it easier to spot future, potentially interesting
divergences.

Signed-off-by: Jim Fehlig 
---

V2: Actually use libxl_wait_for_memory_target(), instead of
libxl_wait_for_free_memory()

 src/libxl/libxl_domain.c | 55 +++-
 1 file changed, 26 insertions(+), 29 deletions(-)

diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 407a9bd..a1739aa 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -1121,38 +1121,39 @@ libxlDomainFreeMem(libxlDomainObjPrivatePtr priv, 
libxl_domain_config *d_config)
 {
 uint32_t needed_mem;
 uint32_t free_mem;
-size_t i;
-int ret = -1;
+int ret;
 int tries = 3;
 int wait_secs = 10;
 
-if ((ret = libxl_domain_need_memory(priv->ctx, &d_config->b_info,
-&needed_mem)) >= 0) {
-for (i = 0; i < tries; ++i) {
-if ((ret = libxl_get_free_memory(priv->ctx, &free_mem)) < 0)
-break;
+ret = libxl_domain_need_memory(priv->ctx, &d_config->b_info, &needed_mem);
+if (ret < 0)
+goto error;
 
-if (free_mem >= needed_mem) {
-ret = 0;
-break;
-}
+do {
+ret = libxl_get_free_memory(priv->ctx, &free_mem);
+if (ret < 0)
+goto error;
 
-if ((ret = libxl_set_memory_target(priv->ctx, 0,
-   free_mem - needed_mem,
-   /* relative */ 1, 0)) < 0)
-break;
+if (free_mem >= needed_mem)
+return 0;
 
-ret = libxl_wait_for_free_memory(priv->ctx, 0, needed_mem,
- wait_secs);
-if (ret == 0 || ret != ERROR_NOMEM)
-break;
+ret = libxl_set_memory_target(priv->ctx, 0,
+  free_mem - needed_mem,
+  /* relative */ 1, 0);
+if (ret < 0)
+goto error;
 
-if ((ret = libxl_wait_for_memory_target(priv->ctx, 0, 1)) < 0)
-break;
-}
-}
+ret = libxl_wait_for_memory_target(priv->ctx, 0, wait_secs);
+if (ret < 0)
+goto error;
 
-return ret;
+tries--;
+} while (tries > 0);
+
+ error:
+virReportSystemError(ret, "%s",
+ _("Failed to balloon domain0 memory"));
+return -1;
 }
 
 static void
@@ -1271,12 +1272,8 @@ libxlDomainStart(libxlDriverPrivatePtr driver, 
virDomainObjPtr vm,
priv->ctx, &d_config) < 0)
 goto endjob;
 
-if (cfg->autoballoon && libxlDomainFreeMem(priv, &d_config) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("libxenlight failed to get free memory for domain 
'%s'"),
-   d_config.c

Re: [libvirt] [Xen-devel] [PATCH] libxl: fix dom0 balloon logic

[Jim Fehlig]

Stefano Stabellini wrote:
> On Mon, 23 Mar 2015, Ian Campbell wrote:
>   
>> (just ccing the other tools maintainers, in particular Stefano who knows
>> what this stuff is supposed to do...)
>>
>> On Fri, 2015-03-20 at 17:10 -0600, Jim Fehlig wrote:
>> 
>>> Recent testing on large memory systems revealed a bug in the Xen xl
>>> tool's freemem() function.  When autoballooning is enabled, freemem()
>>> is used to ensure enough memory is available to start a domain,
>>> ballooning dom0 if necessary.  When ballooning large amounts of memory
>>> from dom0, freemem() would exceed its self-imposed wait time and
>>> return an error.  Meanwhile, dom0 continued to balloon.  Starting the
>>> domain later, after sufficient memory was ballooned from dom0, would
>>> succeed.  The libvirt implementation in libxlDomainFreeMem() suffers
>>> the same bug since it is modeled after freemem().
>>>
>>> In the end, the best place to fix the bug on the Xen side was to
>>> slightly change the behavior of libxl_wait_for_memory_target().
>>> Instead of failing after caller-provided wait_sec, the function now
>>> blocks as long as dom0 memory ballooning is progressing.  It will return
>>> failure only when more memory is needed to reach the target and wait_sec
>>> have expired with no progress being made.  See xen.git commit fd3aa246.
>>> There was a dicussion on how this would affect other libxl apps like
>>> libvirt
>>>
>>> http://lists.xen.org/archives/html/xen-devel/2015-03/msg00739.html
>>>
>>> If libvirt containing this patch was build against a Xen containing
>>> the old libxl_wait_for_memory_target() behavior, libxlDomainFreeMem()
>>> will fail after 30 sec and domain creation will be terminated.
>>> Without this patch and with old libxl_wait_for_memory_target() behavior,
>>> libxlDomainFreeMem() does not succeed after 30 sec, but returns success
>>> anyway.  Domain creation continues resulting in all sorts of fun stuff
>>> like cpu soft lockups in the guest OS.  It was decided to properly fix
>>> libxl_wait_for_memory_target(), and if anything improve the default
>>> behavior of apps using the freemem reference impl in xl.
>>>
>>> xl was patched to accommodate the change in libxl_wait_for_memory_target()
>>> with xen.git commit 883b30a0.  This patch does the same in the libxl
>>> driver.  While at it, I changed the logic to essentially match
>>> freemem() in $xensrc/tools/libxl/xl_cmdimpl.c.  It was a bit cleaner
>>> IMO and will make it easier to spot future, potentially interesting
>>> divergences.
>>>
>>> Signed-off-by: Jim Fehlig 
>>> ---
>>>  src/libxl/libxl_domain.c | 57 
>>> 
>>>  1 file changed, 28 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
>>> index 407a9bd..ff78133 100644
>>> --- a/src/libxl/libxl_domain.c
>>> +++ b/src/libxl/libxl_domain.c
>>> @@ -1121,38 +1121,41 @@ libxlDomainFreeMem(libxlDomainObjPrivatePtr priv, 
>>> libxl_domain_config *d_config)
>>>  {
>>>  uint32_t needed_mem;
>>>  uint32_t free_mem;
>>> -size_t i;
>>> -int ret = -1;
>>> +int ret;
>>>  int tries = 3;
>>>  int wait_secs = 10;
>>>  
>>> -if ((ret = libxl_domain_need_memory(priv->ctx, &d_config->b_info,
>>> -&needed_mem)) >= 0) {
>>> -for (i = 0; i < tries; ++i) {
>>> -if ((ret = libxl_get_free_memory(priv->ctx, &free_mem)) < 0)
>>> -break;
>>> +ret = libxl_domain_need_memory(priv->ctx, &d_config->b_info,
>>> +   &needed_mem);
>>> +if (ret < 0)
>>> +goto error;
>>>  
>>> -if (free_mem >= needed_mem) {
>>> -ret = 0;
>>> -break;
>>> -}
>>> +do {
>>> +ret = libxl_get_free_memory(priv->ctx, &free_mem);
>>> +if (ret < 0)
>>> +goto error;
>>>  
>>> -if ((ret = libxl_set_memory_target(priv->ctx, 0,
>>> -   free_mem - needed_mem,
>>> -   /* relative */ 1, 0)) < 0)
>>> -break;
>>> +if (free_mem >= needed_mem)
>>> +return 0;
>>>  
>>> -ret = libxl_wait_for_free_memory(priv->ctx, 0, needed_mem,
>>> - wait_secs);
>>> -if (ret == 0 || ret != ERROR_NOMEM)
>>> -break;
>>> +ret = libxl_set_memory_target(priv->ctx, 0,
>>> +  free_mem - needed_mem,
>>> +  /* relative */ 1, 0);
>>> +if (ret < 0)
>>> +goto error;
>>>  
>>> -if ((ret = libxl_wait_for_memory_target(priv->ctx, 0, 1)) < 0)
>>> -break;
>>> -}
>>> -}
>>> +ret = libxl_wait_for_free_memory(priv->ctx, 0, needed_mem,
>>> + wait_secs);
>>> +if (ret 

Re: [libvirt] [Xen-devel] [PATCH] libxl: fix dom0 balloon logic

[Stefano Stabellini]

On Mon, 23 Mar 2015, Ian Campbell wrote:
> (just ccing the other tools maintainers, in particular Stefano who knows
> what this stuff is supposed to do...)
> 
> On Fri, 2015-03-20 at 17:10 -0600, Jim Fehlig wrote:
> > Recent testing on large memory systems revealed a bug in the Xen xl
> > tool's freemem() function.  When autoballooning is enabled, freemem()
> > is used to ensure enough memory is available to start a domain,
> > ballooning dom0 if necessary.  When ballooning large amounts of memory
> > from dom0, freemem() would exceed its self-imposed wait time and
> > return an error.  Meanwhile, dom0 continued to balloon.  Starting the
> > domain later, after sufficient memory was ballooned from dom0, would
> > succeed.  The libvirt implementation in libxlDomainFreeMem() suffers
> > the same bug since it is modeled after freemem().
> > 
> > In the end, the best place to fix the bug on the Xen side was to
> > slightly change the behavior of libxl_wait_for_memory_target().
> > Instead of failing after caller-provided wait_sec, the function now
> > blocks as long as dom0 memory ballooning is progressing.  It will return
> > failure only when more memory is needed to reach the target and wait_sec
> > have expired with no progress being made.  See xen.git commit fd3aa246.
> > There was a dicussion on how this would affect other libxl apps like
> > libvirt
> > 
> > http://lists.xen.org/archives/html/xen-devel/2015-03/msg00739.html
> > 
> > If libvirt containing this patch was build against a Xen containing
> > the old libxl_wait_for_memory_target() behavior, libxlDomainFreeMem()
> > will fail after 30 sec and domain creation will be terminated.
> > Without this patch and with old libxl_wait_for_memory_target() behavior,
> > libxlDomainFreeMem() does not succeed after 30 sec, but returns success
> > anyway.  Domain creation continues resulting in all sorts of fun stuff
> > like cpu soft lockups in the guest OS.  It was decided to properly fix
> > libxl_wait_for_memory_target(), and if anything improve the default
> > behavior of apps using the freemem reference impl in xl.
> > 
> > xl was patched to accommodate the change in libxl_wait_for_memory_target()
> > with xen.git commit 883b30a0.  This patch does the same in the libxl
> > driver.  While at it, I changed the logic to essentially match
> > freemem() in $xensrc/tools/libxl/xl_cmdimpl.c.  It was a bit cleaner
> > IMO and will make it easier to spot future, potentially interesting
> > divergences.
> >
> > Signed-off-by: Jim Fehlig 
> > ---
> >  src/libxl/libxl_domain.c | 57 
> > 
> >  1 file changed, 28 insertions(+), 29 deletions(-)
> > 
> > diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> > index 407a9bd..ff78133 100644
> > --- a/src/libxl/libxl_domain.c
> > +++ b/src/libxl/libxl_domain.c
> > @@ -1121,38 +1121,41 @@ libxlDomainFreeMem(libxlDomainObjPrivatePtr priv, 
> > libxl_domain_config *d_config)
> >  {
> >  uint32_t needed_mem;
> >  uint32_t free_mem;
> > -size_t i;
> > -int ret = -1;
> > +int ret;
> >  int tries = 3;
> >  int wait_secs = 10;
> >  
> > -if ((ret = libxl_domain_need_memory(priv->ctx, &d_config->b_info,
> > -&needed_mem)) >= 0) {
> > -for (i = 0; i < tries; ++i) {
> > -if ((ret = libxl_get_free_memory(priv->ctx, &free_mem)) < 0)
> > -break;
> > +ret = libxl_domain_need_memory(priv->ctx, &d_config->b_info,
> > +   &needed_mem);
> > +if (ret < 0)
> > +goto error;
> >  
> > -if (free_mem >= needed_mem) {
> > -ret = 0;
> > -break;
> > -}
> > +do {
> > +ret = libxl_get_free_memory(priv->ctx, &free_mem);
> > +if (ret < 0)
> > +goto error;
> >  
> > -if ((ret = libxl_set_memory_target(priv->ctx, 0,
> > -   free_mem - needed_mem,
> > -   /* relative */ 1, 0)) < 0)
> > -break;
> > +if (free_mem >= needed_mem)
> > +return 0;
> >  
> > -ret = libxl_wait_for_free_memory(priv->ctx, 0, needed_mem,
> > - wait_secs);
> > -if (ret == 0 || ret != ERROR_NOMEM)
> > -break;
> > +ret = libxl_set_memory_target(priv->ctx, 0,
> > +  free_mem - needed_mem,
> > +  /* relative */ 1, 0);
> > +if (ret < 0)
> > +goto error;
> >  
> > -if ((ret = libxl_wait_for_memory_target(priv->ctx, 0, 1)) < 0)
> > -break;
> > -}
> > -}
> > +ret = libxl_wait_for_free_memory(priv->ctx, 0, needed_mem,
> > + wait_secs);
> > +if (ret < 0)
> > +goto error;

Shou

Re: [libvirt] [PATCH 0/8] Make debugging of "cannot acquire state change lock" easier

[John Ferlan]

On 03/23/2015 09:25 AM, Jiri Denemark wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=853839
> 
> Jiri Denemark (8):
>   POTFILES.in: Sort
>   Add support for tracking thread jobs
>   Force usage of virThreadCreate
>   virThread: Set thread job
>   virThreadPool: Set thread worker name
>   Set thread job for every RPC call
>   qemu: Track the API which started the current job
>   qemu: Add timing to domain jobs
> 
>  cfg.mk  |   9 +++
>  daemon/remote.c |   1 +
>  include/libvirt/virterror.h |   1 +
>  po/POTFILES.in  |   5 +-
>  src/Makefile.am |   2 +
>  src/libvirt_private.syms|  11 +++-
>  src/locking/lock_daemon_dispatch.c  |   1 +
>  src/nwfilter/nwfilter_learnipaddr.c |  15 ++---
>  src/nwfilter/nwfilter_learnipaddr.h |   1 -
>  src/qemu/qemu_domain.c  |  60 ++---
>  src/qemu/qemu_domain.h  |   4 ++
>  src/rpc/gendispatch.pl  |   6 +-
>  src/util/virerror.c |   1 +
>  src/util/virthread.c|  25 +--
>  src/util/virthread.h|  13 ++--
>  src/util/virthreadjob.c | 126 
> 
>  src/util/virthreadjob.h |  33 ++
>  src/util/virthreadpool.c|  44 -
>  src/util/virthreadpool.h|  14 ++--
>  19 files changed, 317 insertions(+), 55 deletions(-)
>  create mode 100644 src/util/virthreadjob.c
>  create mode 100644 src/util/virthreadjob.h
> 

Looks fine to me... Couple of 'overall' comments...

 * Clean run through my Coverity checker.

 * New modules add copyright for 2013-2015 - shouldn't that just be 2015

 * Other modules touched haven't had their copyrights adjusted... I
don't always remember either - perhaps everyone needs eblake's emacs
macro to auto update when you edit a file.

ACK series

John

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

Re: [libvirt] [PATCH v3 1/2] cleanup conf/device_conf.h inclusion from util/virnetdev.h

[Laine Stump]

On 03/24/2015 05:57 AM, Shivaprasad G Bhat wrote:
> Move the struct and enum definitions from device_conf.h to
> virnetdev.h thus avoiding the file inclusion.
>
> The wrong reference of conf files from util/ is allowed
> in Makefile.am for now. The reason being,
> The change in Makefile.am for libvirt_util_la_CFLAGS to remove
> conf from the utils would require corresponding inclusion in util files
> to specify the paths explicitly. This would result in syntax-check failures
> (prohibit_cross_inclusion) which need to be worked around until
> there are patches reworking the incorrect inclusion.

I agree that we should eliminate this #include of something from conf in
util (and definitely that is a more egregious problem than calling a
platform specific function from conf), but disagree with using this
patch as the method of assuring that virNetDevExists() is declared for
network_conf.c (so that patch 2/2 of this series compiles).

if virnetdev.h is required by something in network_conf.c, then it
should be #included by network_conf.c. (yes, I understand that the code
movement in this patch makes it necessary to #include virnetdev.h in
device_conf.h anyway)

I see that there are a few other cases of *_conf.h files being included
from .h files in the util directory. Is this patch perhaps part of a
patch series to fix that?

(BTW, the util directory was clean of any #includes from the conf
directory until last July, when management of "close callbacks" was
moved from the qemu driver into util/virclosecallbacks.h. The problem is
that one of the args to the a callback is a virDomainObj. So I think the
proper solution to this problem would require making that arg to all the
virCloseCallback functions an opaque pointer, show real nature would be
known only to the callback function itself.)

>
> The syntax-check failure workaround seems dangerous as that might mask some
> easily resolvable failures. So dont touch the Makefile.am for now. Resolve
> the wrong inclusions in separate patches.

Maybe that's what you're referring to here?

>
> Signed-off-by: Shivaprasad G Bhat 
> ---
>  src/conf/device_conf.h |   38 +-
>  src/util/virnetdev.h   |   38 +-
>  2 files changed, 38 insertions(+), 38 deletions(-)
>
> diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
> index 7ea90f6..a650189 100644
> --- a/src/conf/device_conf.h
> +++ b/src/conf/device_conf.h
> @@ -31,19 +31,7 @@
>  # include "virutil.h"
>  # include "virthread.h"
>  # include "virbuffer.h"
> -
> -typedef enum {
> -VIR_INTERFACE_STATE_UNKNOWN = 1,
> -VIR_INTERFACE_STATE_NOT_PRESENT,
> -VIR_INTERFACE_STATE_DOWN,
> -VIR_INTERFACE_STATE_LOWER_LAYER_DOWN,
> -VIR_INTERFACE_STATE_TESTING,
> -VIR_INTERFACE_STATE_DORMANT,
> -VIR_INTERFACE_STATE_UP,
> -VIR_INTERFACE_STATE_LAST
> -} virInterfaceState;
> -
> -VIR_ENUM_DECL(virInterfaceState)
> +# include "virnetdev.h"
>  
>  typedef struct _virDevicePCIAddress virDevicePCIAddress;
>  typedef virDevicePCIAddress *virDevicePCIAddressPtr;
> @@ -55,30 +43,6 @@ struct _virDevicePCIAddress {
>  int  multi;  /* virTristateSwitch */
>  };
>  
> -typedef struct _virInterfaceLink virInterfaceLink;
> -typedef virInterfaceLink *virInterfaceLinkPtr;
> -struct _virInterfaceLink {
> -virInterfaceState state; /* link state */
> -unsigned int speed;  /* link speed in Mbits per second */
> -};
> -
> -typedef enum {
> -VIR_NET_DEV_FEAT_GRXCSUM,
> -VIR_NET_DEV_FEAT_GTXCSUM,
> -VIR_NET_DEV_FEAT_GSG,
> -VIR_NET_DEV_FEAT_GTSO,
> -VIR_NET_DEV_FEAT_GGSO,
> -VIR_NET_DEV_FEAT_GGRO,
> -VIR_NET_DEV_FEAT_LRO,
> -VIR_NET_DEV_FEAT_RXVLAN,
> -VIR_NET_DEV_FEAT_TXVLAN,
> -VIR_NET_DEV_FEAT_NTUPLE,
> -VIR_NET_DEV_FEAT_RXHASH,
> -VIR_NET_DEV_FEAT_LAST
> -} virNetDevFeature;
> -
> -VIR_ENUM_DECL(virNetDevFeature)
> -
>  int virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr);
>  
>  int virDevicePCIAddressParseXML(xmlNodePtr node,
> diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
> index 856127b..daba0bb 100644
> --- a/src/util/virnetdev.h
> +++ b/src/util/virnetdev.h
> @@ -30,7 +30,6 @@
>  # include "virnetlink.h"
>  # include "virmacaddr.h"
>  # include "virpci.h"
> -# include "device_conf.h"
>  
>  # ifdef HAVE_STRUCT_IFREQ
>  typedef struct ifreq virIfreq;
> @@ -47,6 +46,43 @@ typedef enum {
>  } virNetDevRxFilterMode;
>  VIR_ENUM_DECL(virNetDevRxFilterMode)
>  
> +typedef enum {
> +VIR_INTERFACE_STATE_UNKNOWN = 1,
> +VIR_INTERFACE_STATE_NOT_PRESENT,
> +VIR_INTERFACE_STATE_DOWN,
> +VIR_INTERFACE_STATE_LOWER_LAYER_DOWN,
> +VIR_INTERFACE_STATE_TESTING,
> +VIR_INTERFACE_STATE_DORMANT,
> +VIR_INTERFACE_STATE_UP,
> +VIR_INTERFACE_STATE_LAST
> +} virInterfaceState;
> +
> +VIR_ENUM_DECL(virInterfaceState)
> +
> +typedef struct _virInterfaceLink virInterfaceLink;
> +typedef virInterfaceLink *virInterfaceLinkPtr;
> +struct _virInterfaceL

Re: [libvirt] [PATCH 6/6] qemu: fix set vcpus on host without NUMA

[Pavel Hrdina]

On Tue, Mar 24, 2015 at 05:38:37PM +0100, Peter Krempa wrote:
> On Fri, Mar 20, 2015 at 15:39:04 +0100, Pavel Hrdina wrote:
> > We don't have to modify cpuset.mems on hosts without NUMA.  It also
> > fixes an error message that you get instead of success if you trying
> > update vcpus of a guest on a host without NUMA.
> 
> Could you add example of the error you are fixing here?

Sure

> 
> > 
> > Signed-off-by: Pavel Hrdina 
> > ---
> >  src/qemu/qemu_driver.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index c4d96bd..eb86d68 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -4889,7 +4889,8 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned 
> > int nvcpus,
> >  }
> >  }
> >  
> > -if (flags & VIR_DOMAIN_AFFECT_LIVE && !(flags & 
> > VIR_DOMAIN_VCPU_GUEST)) {
> > +if (flags & VIR_DOMAIN_AFFECT_LIVE && !(flags & VIR_DOMAIN_VCPU_GUEST) 
> > &&
> > +virNumaIsAvailable()) {
> >  if (virCgroupNewEmulator(priv->cgroup, false, &cgroup_temp) < 0)
> >  goto endjob;
> 
> ACK. 
> 
> Peter

Thanks, will push it shortly.

Pavel

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

Re: [libvirt] [PATCH] Document behavior of compat when creating qcow2 volumes

[Ján Tomko]

On Tue, Mar 24, 2015 at 05:43:41PM +0100, Peter Krempa wrote:
> On Tue, Mar 24, 2015 at 17:21:26 +0100, Ján Tomko wrote:
> > Commit bab2eda changed the behavior for missing compat attribute,
> > but failed to update the documentation.
> > 
> > Before, the option was omitted from qemu-img command line and the
> > qemu-img default was used. Now we always specify the compat value
> > and the default is 0.10.
> > 
> > Reported by Christophe Fergeau
> > https://bugzilla.gnome.org/show_bug.cgi?id=746660#c4
> > ---
> >  docs/formatstorage.html.in | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in
> > index d2e2bb8..9c7b1bd 100644
> > --- a/docs/formatstorage.html.in
> > +++ b/docs/formatstorage.html.in
> > @@ -588,8 +588,9 @@
> >  type='qcow2' volumes. Valid values are 
> > 0.10
> >  and 1.1 so far, specifying QEMU version the images 
> > should
> >  be compatible with. If the feature element is present,
> > -1.1 is used. If omitted, qemu-img default is used.
> > -Since 1.1.0
> > +1.1 is used.
> > +Since 1.1.0 If omitted, 0.10 is used.
> > +Since 1.1.2
> 
> You could explicitly state that previously the qemu-img default was
> used.

I didn't want to clutter the documentation by mentioning behavior that
was only there for two releases over a year ago. The version gap there
should be enough for someone using libvirt 1.1.0 that the behavior with
the missing attribute is unspecified.

> 
> >
> >nocow
> >Turn off COW of the newly created volume. So far, this is only 
> > valida
> 
> 
> ACK,

Thanks, pushed.

Jan



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

Re: [libvirt] qemu_migration: Precreate missing storage breaks with network drives

[Noel Burton-Krahn]

In our case the network disk is available on the dest before we call
migration.  It's an openstack volume mounted via ceph.  If we disable the
copy-check in qemu_migration.c the migration works fine.

On Mon, Mar 23, 2015 at 12:17 AM, Michal Privoznik 
wrote:

> On 20.03.2015 20:23, Noel Burton-Krahn wrote:
> > Hi Michal,
> >
> > I think issuing a libvirt migrate to a host where the network disks
> > don't already exist would be a prequisite failure.  Libvirt can never
> > copy a network disk, but it shouldn't fail trying to migrate an existing
> > domain that contains a network disk.  If a libvirt user wishes to
> > migrate a domain that contains a network disk, it's their responsibility
> > to ensure the disk exists before calling migrate.
>
> That's how it was back in the old days. Then libvirt introduced storage
> migration, but requiring users to pre-create storage on destination
> themselves. And just recently I taught libvirt to automatically
> pre-create the storage. However, not for all disk types.
>
> If a disk on destination is missing, it's likely broken guest ABI
> libvirt should have not allowed migration in the first place. How come
> the migration was allowed?
>
> Michal
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCHv3 2/6] Add functions to track virtio-serial addresses

[Ján Tomko]

Create a sorted array of virtio-serial controllers.
Each of the elements contains the controller index
and a bitmap of available ports.

Buses are not tracked, because they aren't supported by QEMU.
---
 src/conf/domain_addr.c   | 398 +++
 src/conf/domain_addr.h   |  59 +++
 src/libvirt_private.syms |  10 ++
 3 files changed, 467 insertions(+)

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index fb4a76f..49d28be 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -718,3 +718,401 @@ virDomainCCWAddressSetCreate(void)
 virDomainCCWAddressSetFree(addrs);
 return NULL;
 }
+
+
+#define VIR_DOMAIN_DEFAULT_VIRTIO_SERIAL_PORTS 31
+
+
+/* virDomainVirtioSerialAddrSetCreate
+ *
+ * Allocates an address set for virtio serial addresses
+ */
+virDomainVirtioSerialAddrSetPtr
+virDomainVirtioSerialAddrSetCreate(void)
+{
+virDomainVirtioSerialAddrSetPtr ret = NULL;
+
+if (VIR_ALLOC(ret) < 0)
+return NULL;
+
+return ret;
+}
+
+static void
+virDomainVirtioSerialControllerFree(virDomainVirtioSerialControllerPtr cont)
+{
+if (cont) {
+virBitmapFree(cont->ports);
+VIR_FREE(cont);
+}
+}
+
+static ssize_t
+virDomainVirtioSerialAddrPlaceController(virDomainVirtioSerialAddrSetPtr addrs,
+ virDomainVirtioSerialControllerPtr 
cont)
+{
+size_t i;
+
+for (i = 0; i < addrs->ncontrollers; i++) {
+if (addrs->controllers[i]->idx == cont->idx) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("virtio serial controller with index %u already 
exists"
+ " in the address set"),
+   cont->idx);
+return -2;
+}
+if (addrs->controllers[i]->idx > cont->idx)
+return i;
+}
+return -1;
+}
+
+static ssize_t
+virDomainVirtioSerialAddrFindController(virDomainVirtioSerialAddrSetPtr addrs,
+unsigned int idx)
+{
+size_t i;
+
+for (i = 0; i < addrs->ncontrollers; i++) {
+if (addrs->controllers[i]->idx == idx)
+return i;
+}
+return -1;
+}
+
+/* virDomainVirtioSerialAddrSetAddController
+ *
+ * Adds virtio serial ports of the existing controller
+ * to the address set.
+ */
+int
+virDomainVirtioSerialAddrSetAddController(virDomainVirtioSerialAddrSetPtr 
addrs,
+  virDomainControllerDefPtr cont)
+{
+int ret = -1;
+int ports;
+virDomainVirtioSerialControllerPtr cnt = NULL;
+ssize_t insertAt;
+
+if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL)
+return 0;
+
+ports = cont->opts.vioserial.ports;
+if (ports == -1)
+ports = VIR_DOMAIN_DEFAULT_VIRTIO_SERIAL_PORTS;
+
+VIR_DEBUG("Adding virtio serial controller index %u with %d"
+  " ports to the address set", cont->idx, ports);
+
+if (VIR_ALLOC(cnt) < 0)
+goto cleanup;
+
+if (!(cnt->ports = virBitmapNew(ports)))
+goto cleanup;
+cnt->idx = cont->idx;
+
+if ((insertAt = virDomainVirtioSerialAddrPlaceController(addrs, cnt)) < -1)
+goto cleanup;
+if (VIR_INSERT_ELEMENT(addrs->controllers, insertAt,
+   addrs->ncontrollers, cnt) < 0)
+goto cleanup;
+
+ret = 0;
+
+ cleanup:
+virDomainVirtioSerialControllerFree(cnt);
+return ret;
+}
+
+/* virDomainVirtioSerialAddrSetAddControllers
+ *
+ * Adds virtio serial ports of controllers present in the domain definition
+ * to the address set.
+ */
+int
+virDomainVirtioSerialAddrSetAddControllers(virDomainVirtioSerialAddrSetPtr 
addrs,
+   virDomainDefPtr def)
+{
+size_t i;
+
+for (i = 0; i < def->ncontrollers; i++) {
+if (virDomainVirtioSerialAddrSetAddController(addrs,
+  def->controllers[i]) < 0)
+return -1;
+}
+
+return 0;
+}
+
+/* virDomainVirtioSerialAddrSetRemoveController
+ *
+ * Removes a virtio serial controller from the address set.
+ */
+void
+virDomainVirtioSerialAddrSetRemoveController(virDomainVirtioSerialAddrSetPtr 
addrs,
+ virDomainControllerDefPtr cont)
+{
+ssize_t pos;
+
+if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL)
+return;
+
+VIR_DEBUG("Removing virtio serial controller index %u "
+  "from the address set", cont->idx);
+
+pos = virDomainVirtioSerialAddrFindController(addrs, cont->idx);
+
+if (pos >= 0)
+VIR_DELETE_ELEMENT(addrs->controllers, pos, addrs->ncontrollers);
+}
+
+void
+virDomainVirtioSerialAddrSetFree(virDomainVirtioSerialAddrSetPtr addrs)
+{
+size_t i;
+if (addrs) {
+for (i = 0; i < addrs->ncontrollers; i++)
+virDomainVirtioSerialControllerFree(addrs->controllers[i]);
+VIR_FREE(addrs);
+}
+}
+
+stat

[libvirt] [PATCHv3 0/6] Allocate virtio-serial addresses

[Ján Tomko]

Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1076708
https://bugzilla.redhat.com/show_bug.cgi?id=890606

New in v3:
* preserve the behavior of honoring the controller if a partial address is 
specified:
  
* automatically add a controller if we're out of free ports
  and add a test for it

Ján Tomko (6):
  Add test for virtio serial port assignment
  Add functions to track virtio-serial addresses
  Allocate virtio-serial addresses when starting a domain
  Expand the address set when attaching a virtio-serial controller
  Assign an address when hotplugging a virtio-serial device
  Auto add virtio-serial controllers

 src/conf/domain_addr.c | 435 +
 src/conf/domain_addr.h |  61 +++
 src/conf/domain_conf.c |  48 +--
 src/conf/domain_conf.h |   1 +
 src/libvirt_private.syms   |  11 +
 src/qemu/qemu_command.c|  63 +++
 src/qemu/qemu_domain.c |   1 +
 src/qemu/qemu_domain.h |   1 +
 src/qemu/qemu_hotplug.c|  32 +-
 src/qemu/qemu_process.c|   2 +
 tests/qemuhotplugtest.c|   2 +-
 .../qemuxml2argv-channel-virtio-auto.args  |   2 +-
 .../qemuxml2argv-channel-virtio-autoadd.args   |  33 ++
 .../qemuxml2argv-channel-virtio-autoadd.xml|  45 +++
 .../qemuxml2argv-channel-virtio-autoassign.args|  20 +
 .../qemuxml2argv-channel-virtio-autoassign.xml |  50 +++
 tests/qemuxml2argvtest.c   |   4 +
 .../qemuxml2xmlout-channel-virtio-auto.xml |   9 +-
 18 files changed, 777 insertions(+), 43 deletions(-)
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoadd.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoadd.xml
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoassign.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoassign.xml

-- 
2.0.5

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

[libvirt] [PATCHv3 1/6] Add test for virtio serial port assignment

[Ján Tomko]

Add a test to demonstrate the effect of automatic virtio-serial
address assignment.
---
 .../qemuxml2argv-channel-virtio-autoassign.args| 20 +
 .../qemuxml2argv-channel-virtio-autoassign.xml | 50 ++
 tests/qemuxml2argvtest.c   |  2 +
 3 files changed, 72 insertions(+)
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoassign.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoassign.xml

diff --git a/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoassign.args 
b/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoassign.args
new file mode 100644
index 000..f7f7b8d
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoassign.args
@@ -0,0 +1,20 @@
+LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \
+/usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults \
+-chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c \
+-device virtio-serial-pci,id=virtio-serial0,max_ports=4,vectors=4,bus=pci.0\
+,addr=0x3 -device virtio-serial-pci,id=virtio-serial1,bus=pci.0,addr=0xa \
+-usb -hda /dev/HostVG/QEMUGuest1 \
+-chardev pty,id=charchannel0 -device virtserialport,bus=virtio-serial0.0,nr=1,\
+chardev=charchannel0,id=channel0,name=org.linux-kvm.port.0 \
+-chardev pty,id=charchannel1 -device virtserialport,bus=virtio-serial0.2,nr=1,\
+chardev=charchannel1,id=channel1,name=org.linux-kvm.port.foo \
+-chardev pty,id=charchannel2 -device virtserialport,bus=virtio-serial0.0,nr=1,\
+chardev=charchannel2,id=channel2,name=org.linux-kvm.port.bar \
+-chardev pty,id=charchannel3 -device virtserialport,bus=virtio-serial0.0,nr=2,\
+chardev=charchannel3,id=channel3,name=org.linux-kvm.port.wizz \
+-chardev pty,id=charchannel4 -device virtserialport,bus=virtio-serial0.0,nr=3,\
+chardev=charchannel4,id=channel4,name=org.linux-kvm.port.ooh \
+-chardev pty,id=charchannel5 -device virtserialport,bus=virtio-serial0.0,nr=4,\
+chardev=charchannel5,id=channel5,name=org.linux-kvm.port.lla \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoassign.xml 
b/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoassign.xml
new file mode 100644
index 000..5592c8f
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoassign.xml
@@ -0,0 +1,50 @@
+
+  QEMUGuest1
+  c7a5fdbd-edaf-9455-926a-d65c16db1809
+  219100
+  219100
+  1
+  
+hvm
+
+  
+  
+  destroy
+  restart
+  destroy
+  
+/usr/bin/qemu
+
+  
+  
+  
+
+
+
+
+
+  
+
+
+  
+
+
+  
+  
+
+
+  
+  
+
+
+  
+
+
+  
+
+
+  
+
+
+  
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 08f374e..00e608c 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1070,6 +1070,8 @@ mymain(void)
 QEMU_CAPS_DEVICE, QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG);
 DO_TEST("channel-virtio-auto",
 QEMU_CAPS_DEVICE, QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG);
+DO_TEST("channel-virtio-autoassign",
+QEMU_CAPS_DEVICE, QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG);
 DO_TEST("console-virtio",
 QEMU_CAPS_DEVICE, QEMU_CAPS_CHARDEV, QEMU_CAPS_NODEFCONFIG);
 DO_TEST("console-virtio-many",
-- 
2.0.5

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

[libvirt] [PATCHv3 5/6] Assign an address when hotplugging a virtio-serial device

[Ján Tomko]

---
 src/qemu/qemu_hotplug.c | 21 +++--
 tests/qemuhotplugtest.c |  2 +-
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index ef2f9b0..560a164 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1541,6 +1541,8 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
 virDomainDefPtr vmdef = vm->def;
 char *devstr = NULL;
 char *charAlias = NULL;
+bool need_release = false;
+bool allowZero = false;
 
 if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
 virReportError(VIR_ERR_OPERATION_INVALID, "%s",
@@ -1551,6 +1553,16 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
 if (qemuAssignDeviceChrAlias(vmdef, chr, -1) < 0)
 goto cleanup;
 
+if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE &&
+chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO)
+allowZero = true;
+
+if (virDomainVirtioSerialAddrAutoAssign(priv->vioserialaddrs,
+&chr->info,
+allowZero) < 0)
+goto cleanup;
+need_release = true;
+
 if (qemuBuildChrDeviceStr(&devstr, vm->def, chr, priv->qemuCaps) < 0)
 goto cleanup;
 
@@ -1582,6 +1594,8 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver,
  cleanup:
 if (ret < 0 && virDomainObjIsActive(vm))
 qemuDomainChrInsertPreAllocCleanup(vm->def, chr);
+if (ret < 0 && need_release)
+virDomainVirtioSerialAddrRelease(priv->vioserialaddrs, &chr->info);
 VIR_FREE(charAlias);
 VIR_FREE(devstr);
 return ret;
@@ -4120,10 +4134,13 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver,
 goto cleanup;
 
 rc = qemuDomainWaitForDeviceRemoval(vm);
-if (rc == 0 || rc == 1)
+if (rc == 0 || rc == 1) {
+virDomainVirtioSerialAddrRelease(priv->vioserialaddrs, &tmpChr->info);
 ret = qemuDomainRemoveChrDevice(driver, vm, tmpChr);
-else
+} else {
 ret = 0;
+}
+
 
  cleanup:
 qemuDomainResetDeviceRemoval(vm);
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index 12a7f71..ea2cf77 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -86,7 +86,7 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt,
 if (event)
 virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_DEVICE_DEL_EVENT);
 
-if (qemuDomainAssignPCIAddresses((*vm)->def, priv->qemuCaps, *vm) < 0)
+if (qemuDomainAssignAddresses((*vm)->def, priv->qemuCaps, *vm) < 0)
 goto cleanup;
 
 if (qemuAssignDeviceAliases((*vm)->def, priv->qemuCaps) < 0)
-- 
2.0.5

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

[libvirt] [PATCHv3 6/6] Auto add virtio-serial controllers

[Ján Tomko]

In virDomainVirtioSerialAddrNext, add another controller
if we've exhausted all ports of the existing controllers.

https://bugzilla.redhat.com/show_bug.cgi?id=1076708
---
 src/conf/domain_addr.c | 47 +++---
 src/conf/domain_addr.h | 10 +++--
 src/qemu/qemu_command.c|  4 +-
 src/qemu/qemu_hotplug.c|  3 +-
 .../qemuxml2argv-channel-virtio-autoadd.args   | 33 +++
 .../qemuxml2argv-channel-virtio-autoadd.xml| 45 +
 tests/qemuxml2argvtest.c   |  2 +
 7 files changed, 132 insertions(+), 12 deletions(-)
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoadd.args
 create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-channel-virtio-autoadd.xml

diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index 49d28be..27a237d 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -878,7 +878,28 @@ 
virDomainVirtioSerialAddrSetFree(virDomainVirtioSerialAddrSetPtr addrs)
 }
 
 static int
-virDomainVirtioSerialAddrNext(virDomainVirtioSerialAddrSetPtr addrs,
+virDomainVirtioSerialAddrSetAutoaddController(virDomainDefPtr def,
+  virDomainVirtioSerialAddrSetPtr 
addrs,
+  unsigned int idx)
+{
+int contidx;
+
+if (virDomainDefMaybeAddController(def,
+   
VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL,
+   idx, -1) < 0)
+return -1;
+
+contidx = virDomainControllerFind(def, 
VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL, idx);
+
+if (virDomainVirtioSerialAddrSetAddController(addrs, 
def->controllers[contidx]) < 0)
+return -1;
+
+return 0;
+}
+
+static int
+virDomainVirtioSerialAddrNext(virDomainDefPtr def,
+  virDomainVirtioSerialAddrSetPtr addrs,
   virDomainDeviceVirtioSerialAddress *addr,
   bool allowZero)
 {
@@ -905,6 +926,20 @@ 
virDomainVirtioSerialAddrNext(virDomainVirtioSerialAddrSetPtr addrs,
 }
 }
 
+if (def) {
+for (i = 0; i < INT_MAX; i++) {
+int idx = virDomainControllerFind(def, 
VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL, i);
+
+if (idx == -1) {
+if (virDomainVirtioSerialAddrSetAutoaddController(def, addrs, 
i) < 0)
+goto cleanup;
+controller = i;
+port = startPort + 1;
+goto success;
+}
+}
+}
+
 virReportError(VIR_ERR_XML_ERROR, "%s",
_("Unable to find a free virtio-serial port"));
 
@@ -958,7 +993,8 @@ 
virDomainVirtioSerialAddrNextFromController(virDomainVirtioSerialAddrSetPtr addr
  * or assign a virtio serial address to the device
  */
 int
-virDomainVirtioSerialAddrAutoAssign(virDomainVirtioSerialAddrSetPtr addrs,
+virDomainVirtioSerialAddrAutoAssign(virDomainDefPtr def,
+virDomainVirtioSerialAddrSetPtr addrs,
 virDomainDeviceInfoPtr info,
 bool allowZero)
 {
@@ -967,12 +1003,13 @@ 
virDomainVirtioSerialAddrAutoAssign(virDomainVirtioSerialAddrSetPtr addrs,
 info->addr.vioserial.port)
 return virDomainVirtioSerialAddrReserve(NULL, NULL, info, addrs);
 else
-return virDomainVirtioSerialAddrAssign(addrs, info, allowZero, 
portOnly);
+return virDomainVirtioSerialAddrAssign(def, addrs, info, allowZero, 
portOnly);
 }
 
 
 int
-virDomainVirtioSerialAddrAssign(virDomainVirtioSerialAddrSetPtr addrs,
+virDomainVirtioSerialAddrAssign(virDomainDefPtr def,
+virDomainVirtioSerialAddrSetPtr addrs,
 virDomainDeviceInfoPtr info,
 bool allowZero,
 bool portOnly)
@@ -988,7 +1025,7 @@ 
virDomainVirtioSerialAddrAssign(virDomainVirtioSerialAddrSetPtr addrs,
 &ptr->addr.vioserial) 
< 0)
 goto cleanup;
 } else {
-if (virDomainVirtioSerialAddrNext(addrs, &ptr->addr.vioserial,
+if (virDomainVirtioSerialAddrNext(def, addrs, &ptr->addr.vioserial,
   allowZero) < 0)
 goto cleanup;
 }
diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
index 14ffd17..c18e130 100644
--- a/src/conf/domain_addr.h
+++ b/src/conf/domain_addr.h
@@ -208,17 +208,19 @@ 
virDomainVirtioSerialAddrSetFree(virDomainVirtioSerialAddrSetPtr addrs);
 bool
 virDomainVirtioSerialAddrIsComplete(virDomainDeviceInfoPtr info);
 int
-virDomainVirtioSerialAddrAutoAssign(virDomainVirtioSerialAddrSetPtr addrs,
+virDomainVirtioSerialAddrAutoAssign(virDomainDefPtr def,
+

[libvirt] [PATCHv3 4/6] Expand the address set when attaching a virtio-serial controller

[Ján Tomko]

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

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 9b8d11b..ef2f9b0 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -437,6 +437,7 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr 
driver,
 char *devstr = NULL;
 qemuDomainObjPrivatePtr priv = vm->privateData;
 bool releaseaddr = false;
+bool addedToAddrSet = false;
 
 if (virDomainControllerFind(vm->def, controller->type, controller->idx) >= 
0) {
 virReportError(VIR_ERR_OPERATION_FAILED,
@@ -475,6 +476,12 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr 
driver,
 goto cleanup;
 }
 
+if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL &&
+virDomainVirtioSerialAddrSetAddController(priv->vioserialaddrs,
+  controller) < 0)
+goto cleanup;
+addedToAddrSet = true;
+
 if (!(devstr = qemuBuildControllerDevStr(vm->def, controller, 
priv->qemuCaps, NULL)))
 goto cleanup;
 }
@@ -503,6 +510,9 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr 
driver,
 }
 
  cleanup:
+if (ret != 0 && addedToAddrSet)
+virDomainVirtioSerialAddrSetRemoveController(priv->vioserialaddrs,
+ controller);
 if (ret != 0 && releaseaddr)
 qemuDomainReleaseDeviceAddress(vm, &controller->info, NULL);
 
-- 
2.0.5

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

[libvirt] [PATCHv3 3/6] Allocate virtio-serial addresses when starting a domain

[Ján Tomko]

Instead of always using controller 0 and incrementing port number,
respect the maximum port numbers of controllers and use all of them.

Ports for virtio consoles are quietly reserved, but not formatted
(neither in XML nor on QEMU command line).

Also rejects duplicate virtio-serial addresses.
https://bugzilla.redhat.com/show_bug.cgi?id=890606
https://bugzilla.redhat.com/show_bug.cgi?id=1076708

Test changes:
* virtio-auto.args
  Filling out the port when just the controller is specified.
  switched from using
maxport + 1
  to:
first free port on the controller
* virtio-autoassign.args
  Filling out the address when no  is specified.
  Started using all the controllers instead of 0, also discards
  the bus value.
* xml -> xml output of virtio-auto
  The port assignment is no longer done as a part of XML parsing,
  so the unspecified values stay 0.
---
 src/conf/domain_conf.c | 48 +
 src/conf/domain_conf.h |  1 +
 src/libvirt_private.syms   |  1 +
 src/qemu/qemu_command.c| 63 ++
 src/qemu/qemu_domain.c |  1 +
 src/qemu/qemu_domain.h |  1 +
 src/qemu/qemu_process.c|  2 +
 .../qemuxml2argv-channel-virtio-auto.args  |  2 +-
 .../qemuxml2argv-channel-virtio-autoassign.args| 10 ++--
 .../qemuxml2xmlout-channel-virtio-auto.xml |  9 ++--
 10 files changed, 93 insertions(+), 45 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d633f93..e777f5f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3498,21 +3498,6 @@ 
virDomainDeviceDefPostParseInternal(virDomainDeviceDefPtr dev,
 
 chr->target.port = maxport + 1;
 }
-
-if (chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL &&
-chr->info.addr.vioserial.port == 0) {
-int maxport = 0;
-
-for (i = 0; i < cnt; i++) {
-const virDomainChrDef *thischr = arrPtr[i];
-if (thischr->info.type == 
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL &&
-thischr->info.addr.vioserial.controller == 
chr->info.addr.vioserial.controller &&
-thischr->info.addr.vioserial.bus == 
chr->info.addr.vioserial.bus &&
-(int)thischr->info.addr.vioserial.port > maxport)
-maxport = thischr->info.addr.vioserial.port;
-}
-chr->info.addr.vioserial.port = maxport + 1;
-}
 }
 
 /* set default path for virtio-rng "random" backend to /dev/random */
@@ -12471,6 +12456,20 @@ virDomainControllerFind(virDomainDefPtr def,
 }
 
 int
+virDomainControllerFindByType(virDomainDefPtr def,
+  int type)
+{
+size_t i;
+
+for (i = 0; i < def->ncontrollers; i++) {
+if (def->controllers[i]->type == type)
+return i;
+}
+
+return -1;
+}
+
+int
 virDomainControllerFindByPCIAddress(virDomainDefPtr def,
 virDevicePCIAddressPtr addr)
 {
@@ -14906,25 +14905,6 @@ virDomainDefParseXML(xmlDocPtr xml,
 goto error;
 
 def->channels[def->nchannels++] = chr;
-
-if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
-chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO &&
-chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
-chr->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL;
-
-if (chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL &&
-chr->info.addr.vioserial.port == 0) {
-int maxport = 0;
-for (j = 0; j < i; j++) {
-virDomainChrDefPtr thischr = def->channels[j];
-if (thischr->info.type == 
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL &&
-thischr->info.addr.vioserial.controller == 
chr->info.addr.vioserial.controller &&
-thischr->info.addr.vioserial.bus == 
chr->info.addr.vioserial.bus &&
-(int)thischr->info.addr.vioserial.port > maxport)
-maxport = thischr->info.addr.vioserial.port;
-}
-chr->info.addr.vioserial.port = maxport + 1;
-}
 }
 VIR_FREE(nodes);
 
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index bceb2d7..b756b40 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2701,6 +2701,7 @@ int virDomainControllerInsert(virDomainDefPtr def,
 void virDomainControllerInsertPreAlloced(virDomainDefPtr def,
  virDomainControllerDefPtr controller);
 int virDomainControllerFind(virDomainDefPtr def, int type, int idx);
+int virDomainControllerFindByType(virDomainDefPtr def, int type);
 int virDomainControllerFindByPCIAddress(virDomainDefPtr def,
  

Re: [libvirt] [PATCH 5/6] tools: fix the wrong check when use virsh setvcpus --maximum

[Pavel Hrdina]

On Tue, Mar 24, 2015 at 05:34:31PM +0100, Peter Krempa wrote:
> On Fri, Mar 20, 2015 at 15:39:03 +0100, Pavel Hrdina wrote:
> > From: Luyao Huang 
> > 
> > We will ignore --maximum option when only use setvcpus with
> > this option, like this (this error is another issue):
> > 
> >  # virsh setvcpus test3 --maximum 10
> >  error: Failed to create controller cpu for group: No such file or directory
> > 
> > this is because we do not set it in flags before we check if there is
> > a flags set.
> > 
> > Refactor these code and fix the logic.
> > 
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1204033
> > 
> > Signed-off-by: Luyao Huang 
> > Signed-off-by: Pavel Hrdina 
> > ---
> >  tools/virsh-domain.c | 30 ++
> >  1 file changed, 6 insertions(+), 24 deletions(-)
> > 
> > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> > index 1d8225c..9430ad9 100644
> > --- a/tools/virsh-domain.c
> > +++ b/tools/virsh-domain.c
> > @@ -6735,6 +6735,8 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
> >  VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
> >  VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
> >  VSH_EXCLUSIVE_OPTIONS_VAR(guest, config);
> > +VSH_EXCLUSIVE_OPTIONS_VAR(guest, maximum);
> > +VSH_EXCLUSIVE_OPTIONS_VAR(config, maximum);
> 
> As Yanbing pointed out, you want to make live and maximum exclusive.
> 
> Additionally this changes semantics, as currently --maximum was possible
> if and only if --config was specified, which would make it exclusive
> with --current too. This is also implied in the man page.
> 
> We have the following options:
> 
> 1) Make --maximum imply --config and document that properly
> 2) Make --maximum mutualy exclusive with --current too
> 3) Allow --maximum and --current and document that it will fail for
>online domains
> 
> I'm fine with either of those options

There is also 4th option: specify that maximum requires config instead of make
it mutually exclusive with current.  This flag requirements are used by some of
the other libvirt APIs.  I'll create another set of macros to tell that some
flags are required and send v2.

Thanks for review.

Pavel

> 
> Peter


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

Re: [libvirt] [PATCH 3/6] qemu: cleanup setvcpus

[Pavel Hrdina]

On Tue, Mar 24, 2015 at 05:01:09PM +0100, Peter Krempa wrote:
> On Fri, Mar 20, 2015 at 15:39:01 +0100, Pavel Hrdina wrote:
> > Remove unnecessary maximum variable and also exclusive flags check as
> > they are now tested by upper layer for all drivers.
> 
> The part about the exclusive flag check is not true.
> 
> > 
> > Signed-off-by: Pavel Hrdina 
> > ---
> >  src/qemu/qemu_driver.c | 16 ++--
> >  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> ACK with commit message fixed.
> 
> Peter

Thanks, I'll push it shortly.

Pavel

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

Re: [libvirt] [PATCH 1/6] internal: introduce macro helpers to reject exclusive flags

[Pavel Hrdina]

On Tue, Mar 24, 2015 at 04:40:17PM +0100, Peter Krempa wrote:
> On Fri, Mar 20, 2015 at 15:38:59 +0100, Pavel Hrdina wrote:
> > Inspired by commit 7e437ee7 that introduced similar macros for virsh
> > commands so we don't have to repeat the same code all over.
> > 
> > Signed-off-by: Pavel Hrdina 
> > ---
> >  src/internal.h | 80 
> > ++
> >  1 file changed, 80 insertions(+)
> > 
> > diff --git a/src/internal.h b/src/internal.h
> > index 4d473af..5885f03 100644
> > --- a/src/internal.h
> > +++ b/src/internal.h
> > @@ -327,6 +327,86 @@
> >  }   \
> >  } while (0)
> >  
> > +/* Macros to help dealing with mutually exclusive flags. */
> > +
> > +/**
> > + * VIR_EXCLUSIVE_FLAGS_EXPR_RET:
> > + *
> > + * @NAME1: String containing the name of the flag.
> > + * @EXPR1: Expression to validate the flag.
> > + * @NAME2: String containing the name of the flag.
> > + * @EXPR2: Expression to validate the flag.
> > + *
> > + * Reject mutually exclusive API flags.  Use the provided expression
> > + * to check the flags.
> > + *
> > + * This helper does an early return and therefore it has to be called
> > + * before anything that would require cleanup.
> > + */
> > +# define VIR_EXCLUSIVE_FLAGS_EXPR_RET(NAME1, EXPR1, NAME2, EXPR2)  
> >  \
> > +if ((EXPR1) && (EXPR2)) {  
> >  \
> > +virReportInvalidArg(ctl,   
> >  \
> > +_("Flags '%s' and '%s' are mutually 
> > exclusive"),\
> > +NAME1, NAME2); 
> >  \
> > +return -1; 
> >  \
> > +}
> > +
> 
> While in virsh the above variant makes sense, because in some cases the
> variables have different names than the corresponding flags, in case of
> this code having it doesn't make sense.

You're right. I've only used the other macros.

> 
> 
> > +/**
> > + * VIR_EXCLUSIVE_FLAGS_VAR_RET:
> > + *
> > + * @FLAG1: First flag to be checked
> > + * @FLAG2: Second flag to be checked
> 
> A third argument @RET should be added here so that this can be used in
> places that also return NULL or any other possible variable.
> 
> In virsh the assumption is that the flags are checked at first and thus
> returning false makes sense, as every virsh command does that.

I'll add the @RET argument.

> 
> > + *
> > + * Reject mutually exclusive API flags.  The checked flags are compared
> > + * with flags variable.
> > + *
> > + * This helper does an early return and therefore it has to be called
> > + * before anything that would require cleanup.
> > + */
> > +# define VIR_EXCLUSIVE_FLAGS_RET(FLAG1, FLAG2) 
> >  \
> > +VIR_EXCLUSIVE_FLAGS_EXPR_RET(#FLAG1, flags & FLAG1, #FLAG2, flags & 
> > FLAG2)
> > +
> > +/**
> > + * VIR_EXCLUSIVE_FLAGS_EXPR_GOTO:
> > + *
> > + * @NAME1: String containing the name of the flag.
> > + * @EXPR1: Expression to validate the flag.
> > + * @NAME2: String containing the name of the flag.
> > + * @EXPR2: Expression to validate the flag.
> > + * @LABEL: label to jump to.
> > + *
> > + * Reject mutually exclusive API flags.  Use the provided expression
> > + * to check the flag.
> > + *
> > + * Returns nothing.  Jumps to a label if unsupported flags were
> > + * passed to it.
> > + */
> > +# define VIR_EXCLUSIVE_FLAGS_EXPR_GOTO(NAME1, EXPR1, NAME2, EXPR2, LABEL)  
> >  \
> > +if ((EXPR1) && (EXPR2)) {  
> >  \
> > +virReportInvalidArg(ctl,   
> >  \
> > +_("Flags '%s' and '%s' are mutually 
> > exclusive"),\
> > +NAME1, NAME2); 
> >  \
> > +goto LABEL;
> >  \
> > +}
> > +
> 
> Again, this function does not make sense in the library code.
> 
> > +/**
> > + * VIR_EXCLUSIVE_FLAGS_VAR_GOTO:
> > + *
> > + * @FLAG1: First flag to be checked.
> > + * @FLAG2: Second flag to be checked.
> > + * @LABEL: label to jump to.
> > + *
> > + * Reject mutually exclusive API flags.  The checked flags are compared
> > + * with flags variable.
> > + *
> > + * Returns nothing.  Jumps to a label if unsupported flags were
> > + * passed to it.
> > + */
> > +# define VIR_EXCLUSIVE_FLAGS_GOTO(FLAG1, FLAG2, LABEL) 
> >  \
> > +VIR_EXCLUSIVE_FLAGS_EXPR_GOTO(#FLAG1, flags & FLAG1,   
> >  \
> > +  #FLAG2, flags & FLAG2, LABEL)
> > +
> > +
> >  # define virCheckNonNullArgReturn(argname, retval)  \
> >  do {\
> >  if (argname == NULL) {  \
> 
> While this is okay.
> 
> Peter

Thanks for review, I'l

Re: [libvirt] [PATCH 7/6] qemu: move virDomainLiveConfigHelperMethod right after BeginJob

[Pavel Hrdina]

On Tue, Mar 24, 2015 at 05:13:07PM +0100, Peter Krempa wrote:
> On Fri, Mar 20, 2015 at 15:58:56 +0100, Pavel Hrdina wrote:
> > We should call virDomainLiveConfigHelperMethod ASAP because this
> > function transfers VIR_DOMAIN_AFFECT_CURRENT to VIR_DOMAIN_AFFECT_LIVE
> > or VIR_DOMAIN_AFFECT_CONFIG.  All other additional checks for those two
> > flags should consider that the user give us VIR_DOMAIN_AFFECT_CURRENT.
> > 
> > Signed-off-by: Pavel Hrdina 
> > ---
> >  src/qemu/qemu_driver.c | 8 
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> ACK, although this should be 0.5/6 or 1.5/6. Basically just push this
> first.
> 
> Peter

Thanks for review, I'll push it shortly.

Pavel

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

Re: [libvirt] [PATCH 2/2] qemu: add a max_core setting to qemu.conf for core dump size

[Peter Krempa]

On Tue, Mar 24, 2015 at 08:26:12 +0100, Ján Tomko wrote:
> On Mon, Mar 23, 2015 at 08:43:31PM -0400, John Ferlan wrote:
> > 
> > 
> > On 03/18/2015 08:36 AM, Daniel P. Berrange wrote:
> > > Currently the QEMU processes inherit their core dump rlimit
> > > from libvirtd, which is really suboptimal. This change allows
> > > their limit to be directly controller from qemu.conf instead.
> > > ---
> > >  src/libvirt_private.syms   |  2 ++
> > >  src/qemu/libvirtd_qemu.aug |  1 +
> > >  src/qemu/qemu.conf | 12 
> > >  src/qemu/qemu_conf.c   |  3 +++
> > >  src/qemu/qemu_conf.h   |  2 ++
> > >  src/qemu/qemu_process.c|  2 ++
> > >  src/qemu/test_libvirtd_qemu.aug.in |  1 +
> > >  src/util/vircommand.c  | 14 ++
> > >  src/util/vircommand.h  |  1 +
> > >  src/util/virprocess.c  | 35 
> > > +++
> > >  src/util/virprocess.h  |  1 +
> > >  11 files changed, 74 insertions(+)
> > > 

...

> > > diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> > > index 1c589a2..12e4326 100644
> > > --- a/src/qemu/qemu.conf
> > > +++ b/src/qemu/qemu.conf
> > > @@ -390,6 +390,18 @@
> > >  #max_processes = 0
> > >  #max_files = 0
> > >  
> > > +# If max_core is set to a positive integer, then QEMU will be
> > > +# permitted to create core dumps when it crashes, provided its
> > > +# RAM size is smaller than the limit set. Be warned that the
> > > +# core dump will include a full copy of the guest RAM, so if
> > > +# the largest guest is 32 GB in size, the max_core limit will
> > > +# have to be at least 33/34 GB to allow enough overhead.
> > > +#
> > > +# By default it will inherit core limit from libvirtd, which
> > > +# is usually set to 0 by systemd/init.
> > > +#
> > > +# Size is in bytes
> > > +#max_core = 0
> > 
> > It's too bad it cannot be a "sized" value... Reading 20G in a file for
> > me is so much easier than the comparable byte value say nothing if we
> > get to 128G, 1T, etc. (some day).
> > 
> 
> Having the option as a string would also allow non-integer values, like
> "unlimited".

I definitely vote for an option to set unlimited rather than having to
specify a large number to achieve the same effect.

> 
> Jan

Peter


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

Re: [libvirt] [PATCH] Document behavior of compat when creating qcow2 volumes

[Peter Krempa]

On Tue, Mar 24, 2015 at 17:21:26 +0100, Ján Tomko wrote:
> Commit bab2eda changed the behavior for missing compat attribute,
> but failed to update the documentation.
> 
> Before, the option was omitted from qemu-img command line and the
> qemu-img default was used. Now we always specify the compat value
> and the default is 0.10.
> 
> Reported by Christophe Fergeau
> https://bugzilla.gnome.org/show_bug.cgi?id=746660#c4
> ---
>  docs/formatstorage.html.in | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in
> index d2e2bb8..9c7b1bd 100644
> --- a/docs/formatstorage.html.in
> +++ b/docs/formatstorage.html.in
> @@ -588,8 +588,9 @@
>  type='qcow2' volumes. Valid values are 0.10
>  and 1.1 so far, specifying QEMU version the images 
> should
>  be compatible with. If the feature element is present,
> -1.1 is used. If omitted, qemu-img default is used.
> -Since 1.1.0
> +1.1 is used.
> +Since 1.1.0 If omitted, 0.10 is used.
> +Since 1.1.2

You could explicitly state that previously the qemu-img default was
used.

>
>nocow
>Turn off COW of the newly created volume. So far, this is only 
> valida


ACK,

Peter


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

Re: [libvirt] [PATCH 6/6] qemu: fix set vcpus on host without NUMA

[Peter Krempa]

On Fri, Mar 20, 2015 at 15:39:04 +0100, Pavel Hrdina wrote:
> We don't have to modify cpuset.mems on hosts without NUMA.  It also
> fixes an error message that you get instead of success if you trying
> update vcpus of a guest on a host without NUMA.

Could you add example of the error you are fixing here?

> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/qemu/qemu_driver.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index c4d96bd..eb86d68 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4889,7 +4889,8 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int 
> nvcpus,
>  }
>  }
>  
> -if (flags & VIR_DOMAIN_AFFECT_LIVE && !(flags & VIR_DOMAIN_VCPU_GUEST)) {
> +if (flags & VIR_DOMAIN_AFFECT_LIVE && !(flags & VIR_DOMAIN_VCPU_GUEST) &&
> +virNumaIsAvailable()) {
>  if (virCgroupNewEmulator(priv->cgroup, false, &cgroup_temp) < 0)
>  goto endjob;

ACK. 

Peter


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

Re: [libvirt] [PATCH] qemu: change accidental VIR_WARNING back to VIR_DEBUG

[Laine Stump]

On 03/24/2015 11:09 AM, John Ferlan wrote:
>
> On 03/24/2015 10:49 AM, Laine Stump wrote:
>> While debugging the support for responding to qemu RX_FILTER_CHANGED
>> events, I had changed the "ignoring this event" log message from
>> VIR_DEBUG to VIR_WARN, but forgot to change it back before
>> pushing. Since many guest OSes make enough changes to multicast lists
>> and/or promiscuous mode settings to trigger this message, it's
>> starting to show up as a red herring in bug reports.
>> ---
>>  src/qemu/qemu_driver.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
> ACK -
>
> Ironically I had just started noticing this in a debug session and began
> wondering...

At least you have the presence of mind to wonder, rather than blindly
filing a bug report :-P

Thanks for the ACK, I've pushed it.

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

Re: [libvirt] [PATCH 5/6] tools: fix the wrong check when use virsh setvcpus --maximum

[Peter Krempa]

On Fri, Mar 20, 2015 at 15:39:03 +0100, Pavel Hrdina wrote:
> From: Luyao Huang 
> 
> We will ignore --maximum option when only use setvcpus with
> this option, like this (this error is another issue):
> 
>  # virsh setvcpus test3 --maximum 10
>  error: Failed to create controller cpu for group: No such file or directory
> 
> this is because we do not set it in flags before we check if there is
> a flags set.
> 
> Refactor these code and fix the logic.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1204033
> 
> Signed-off-by: Luyao Huang 
> Signed-off-by: Pavel Hrdina 
> ---
>  tools/virsh-domain.c | 30 ++
>  1 file changed, 6 insertions(+), 24 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 1d8225c..9430ad9 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -6735,6 +6735,8 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
>  VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
>  VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
>  VSH_EXCLUSIVE_OPTIONS_VAR(guest, config);
> +VSH_EXCLUSIVE_OPTIONS_VAR(guest, maximum);
> +VSH_EXCLUSIVE_OPTIONS_VAR(config, maximum);

As Yanbing pointed out, you want to make live and maximum exclusive.

Additionally this changes semantics, as currently --maximum was possible
if and only if --config was specified, which would make it exclusive
with --current too. This is also implied in the man page.

We have the following options:

1) Make --maximum imply --config and document that properly
2) Make --maximum mutualy exclusive with --current too
3) Allow --maximum and --current and document that it will fail for
   online domains

I'm fine with either of those options

Peter


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

Re: [libvirt] [PATCH 2/6] use new macro helpers to check exclusive flags

[Peter Krempa]

On Fri, Mar 20, 2015 at 15:39:00 +0100, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina 
> ---
>  src/libvirt-domain-snapshot.c  |  45 ++
>  src/libvirt-domain.c   | 288 
> +++--
>  src/qemu/qemu_driver.c |   9 +-
>  src/storage/storage_backend_disk.c |  10 +-
>  src/storage/storage_backend_fs.c   |  11 +-
>  5 files changed, 106 insertions(+), 257 deletions(-)
> 

> @@ -7340,14 +7252,21 @@ virDomainSetVcpusFlags(virDomainPtr domain, unsigned 
> int nvcpus,
>  virCheckDomainReturn(domain, -1);
>  virCheckReadOnlyGoto(domain->conn->flags, error);
>  
> -if (flags & VIR_DOMAIN_VCPU_GUEST &&
> -flags & VIR_DOMAIN_VCPU_MAXIMUM) {
> -virReportInvalidArg(flags,
> -_("flags 'VIR_DOMAIN_VCPU_MAXIMUM' and "
> -  "'VIR_DOMAIN_VCPU_GUEST' in '%s' are mutually "
> -  "exclusive"), __FUNCTION__);
> -goto error;
> -}
> +VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_AFFECT_CURRENT,
> + VIR_DOMAIN_AFFECT_LIVE,
> + error);
> +VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_AFFECT_CURRENT,
> + VIR_DOMAIN_AFFECT_CONFIG,
> + error);
> +VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_VCPU_GUEST,
> + VIR_DOMAIN_AFFECT_CONFIG,
> + error);
> +VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_VCPU_GUEST,
> + VIR_DOMAIN_VCPU_MAXIMUM,
> + error);
> +VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_AFFECT_CONFIG,
> + VIR_DOMAIN_VCPU_MAXIMUM,
> + error);

By the way, this check here is not enough to check that MAXIMUM is not
actually used with _AFFECT_LIVE.

If you use VIR_DOMAIN_AFFECT_CURRENT and the guest is online, this check
is bypassed as the state of the domain is not known at this point.

This unfortunately needs to be checked after the
virDomainLiveConfigHelperMethod in the actual code.

>  
>  virCheckNonZeroArgGoto(nvcpus, error);
>  

Peter


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

[libvirt] [PATCH] Document behavior of compat when creating qcow2 volumes

[Ján Tomko]

Commit bab2eda changed the behavior for missing compat attribute,
but failed to update the documentation.

Before, the option was omitted from qemu-img command line and the
qemu-img default was used. Now we always specify the compat value
and the default is 0.10.

Reported by Christophe Fergeau
https://bugzilla.gnome.org/show_bug.cgi?id=746660#c4
---
 docs/formatstorage.html.in | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in
index d2e2bb8..9c7b1bd 100644
--- a/docs/formatstorage.html.in
+++ b/docs/formatstorage.html.in
@@ -588,8 +588,9 @@
 type='qcow2' volumes. Valid values are 0.10
 and 1.1 so far, specifying QEMU version the images should
 be compatible with. If the feature element is present,
-1.1 is used. If omitted, qemu-img default is used.
-Since 1.1.0
+1.1 is used.
+Since 1.1.0 If omitted, 0.10 is used.
+Since 1.1.2
   
   nocow
   Turn off COW of the newly created volume. So far, this is only valid
-- 
2.0.5

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

Re: [libvirt] [PATCH 4/6] qemu: check if domain is really active when do setvcpus with --live

[Peter Krempa]

On Fri, Mar 20, 2015 at 15:39:02 +0100, Pavel Hrdina wrote:
> From: Luyao Huang 
> 
> Commit e105dc9 fix setting vcpus for offline domain, but forget check
> if vm is active when pass VIR_DOMAIN_AFFECT_LIVE flags.
> 
>  # virsh setvcpus test3 4 --live
>  error: Failed to create controller cpu for group: No such file or directory
> 
> Add a check if we pass VIR_DOMAIN_AFFECT_LIVE flags.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1204006
> 
> Signed-off-by: Luyao Huang 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/qemu/qemu_driver.c | 14 --
>  1 file changed, 8 insertions(+), 6 deletions(-)

NACK, after the virDomainLiveConfigHelperMethod gets moved, this is not
logner necessary as it contains the same code.

Peter


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

Re: [libvirt] [PATCH 7/6] qemu: move virDomainLiveConfigHelperMethod right after BeginJob

[Peter Krempa]

On Fri, Mar 20, 2015 at 15:58:56 +0100, Pavel Hrdina wrote:
> We should call virDomainLiveConfigHelperMethod ASAP because this
> function transfers VIR_DOMAIN_AFFECT_CURRENT to VIR_DOMAIN_AFFECT_LIVE
> or VIR_DOMAIN_AFFECT_CONFIG.  All other additional checks for those two
> flags should consider that the user give us VIR_DOMAIN_AFFECT_CURRENT.
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/qemu/qemu_driver.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)

ACK, although this should be 0.5/6 or 1.5/6. Basically just push this
first.

Peter


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

Re: [libvirt] [PATCH 3/6] qemu: cleanup setvcpus

[Peter Krempa]

On Fri, Mar 20, 2015 at 15:39:01 +0100, Pavel Hrdina wrote:
> Remove unnecessary maximum variable and also exclusive flags check as
> they are now tested by upper layer for all drivers.

The part about the exclusive flag check is not true.

> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/qemu/qemu_driver.c | 16 ++--
>  1 file changed, 6 insertions(+), 10 deletions(-)

ACK with commit message fixed.

Peter


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

Re: [libvirt] [PATCH 2/6] use new macro helpers to check exclusive flags

[Peter Krempa]

On Fri, Mar 20, 2015 at 15:39:00 +0100, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina 
> ---
>  src/libvirt-domain-snapshot.c  |  45 ++
>  src/libvirt-domain.c   | 288 
> +++--
>  src/qemu/qemu_driver.c |   9 +-
>  src/storage/storage_backend_disk.c |  10 +-
>  src/storage/storage_backend_fs.c   |  11 +-
>  5 files changed, 106 insertions(+), 257 deletions(-)

I don't object to this change, but as this changes a lot of error
messages I'd like to have a second opinion on this.

Peter


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

Re: [libvirt] [PATCH v3 2/2] network_conf: check if bridge exists on host for user created bridges

[Laine Stump]

On 03/24/2015 11:12 AM, Laine Stump wrote:
> On 03/24/2015 05:59 AM, Shivaprasad G Bhat wrote:
>> virNetworkBridgeInUse() doesn't check if the bridge is manually created
>> outside of libvirt. Check if the bridge actually exist on host using the
>> virNetDevExists().
>>
>> Signed-off-by: Shivaprasad G Bhat 
>> ---
>>  src/conf/network_conf.c |   15 ---
>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
>> index d7c5dec..c3ae2e4 100644
>> --- a/src/conf/network_conf.c
>> +++ b/src/conf/network_conf.c
>> @@ -3227,13 +3227,22 @@ virNetworkBridgeInUseHelper(const void *payload,
>>  int ret = 0;
>>  virNetworkObjPtr net = (virNetworkObjPtr) payload;
>>  const struct virNetworkBridgeInUseHelperData *data = opaque;
>> +bool defined_bridge = false;
>>  
>>  virObjectLock(net);
>>  if (net->def->bridge &&
>> -STREQ(net->def->bridge, data->bridge) &&
>> -!(data->skipname && STREQ(net->def->name, data->skipname)))
>> -ret = 1;
>> +STREQ(net->def->bridge, data->bridge)) {
>> +defined_bridge = true;
>> +if (!(data->skipname && STREQ(net->def->name, data->skipname)))
>> + ret = 1;
>> +}
>> +
>>  virObjectUnlock(net);
>> +
>> +/* Bridge might have been created by a user manually outside libvirt */
>> +if (!defined_bridge && !ret)
>> +ret = virNetDevExists(data->bridge) ? 1 : 0;
>> +
>>  return ret;
>>  }
> This function is intended to be called once for each defined network on
> the host, with data->bridge being the same each time, but
> net->def->bridge being different, so adding the check for data->bridge
> existence here may work, but it's a bit convoluted.
>
> Instead, you should leave this function alone, and just add the extra
> check directly in the function virNetworkBridgeInUse() (either before
> locking, or after unlocking "nets").

You know, there's another problem with this - adding this call to
virNetDevExists() would be the first case of anything in the conf
directory (which is supposed to be platform-independent
parsing/formatting of XML and maintenance of lists of config objects)
that calls a virNetDev*() function which is dependent on the current
platform (and having root privileges). For the current uses of
virNetworkBridgeInUse() it ends up not being a problem, because the only
caller of virNetworkBridgeInUse() will already be verified as running on
a platform that supports the virNetDev* functions and having root
privileges (and/or certain to fail if we made this call on return), but
it still leaves a bad taste in my mouth to be calling a device ioctl
from a function in the conf directory.

The trouble of course is that doing it differently turns this into a
much bigger problem - as it is network_conf.c mostly isolates the
network driver (bridge_driver.c) from the idea that a network could be
defined without a bridge name specified, or that there might be a
conflicting bridge name, by calling virNetworkSetBridgeName() which is
in network_conf.c (it also calls virNetworkSetBridgeName() on its own
when loading the network configs from disk, thus not even giving the
network driver a chance to intervene).

So I don't want to be the one to NACK based on this cross pollination,
but thought I should point it out in case anyone with a stronger opinion
did (and even if not, so that we see that even if we accept a patch like
the current one to fix the bug now, we may still want to plan a
different fix in the long run. Or maybe not - maybe I'm being too pedantic).

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

Re: [libvirt] [PATCH 1/6] internal: introduce macro helpers to reject exclusive flags

[Peter Krempa]

On Fri, Mar 20, 2015 at 15:38:59 +0100, Pavel Hrdina wrote:
> Inspired by commit 7e437ee7 that introduced similar macros for virsh
> commands so we don't have to repeat the same code all over.
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/internal.h | 80 
> ++
>  1 file changed, 80 insertions(+)
> 
> diff --git a/src/internal.h b/src/internal.h
> index 4d473af..5885f03 100644
> --- a/src/internal.h
> +++ b/src/internal.h
> @@ -327,6 +327,86 @@
>  }   \
>  } while (0)
>  
> +/* Macros to help dealing with mutually exclusive flags. */
> +
> +/**
> + * VIR_EXCLUSIVE_FLAGS_EXPR_RET:
> + *
> + * @NAME1: String containing the name of the flag.
> + * @EXPR1: Expression to validate the flag.
> + * @NAME2: String containing the name of the flag.
> + * @EXPR2: Expression to validate the flag.
> + *
> + * Reject mutually exclusive API flags.  Use the provided expression
> + * to check the flags.
> + *
> + * This helper does an early return and therefore it has to be called
> + * before anything that would require cleanup.
> + */
> +# define VIR_EXCLUSIVE_FLAGS_EXPR_RET(NAME1, EXPR1, NAME2, EXPR2)   \
> +if ((EXPR1) && (EXPR2)) {   \
> +virReportInvalidArg(ctl,\
> +_("Flags '%s' and '%s' are mutually exclusive"),\
> +NAME1, NAME2);  \
> +return -1;  \
> +}
> +

While in virsh the above variant makes sense, because in some cases the
variables have different names than the corresponding flags, in case of
this code having it doesn't make sense.


> +/**
> + * VIR_EXCLUSIVE_FLAGS_VAR_RET:
> + *
> + * @FLAG1: First flag to be checked
> + * @FLAG2: Second flag to be checked

A third argument @RET should be added here so that this can be used in
places that also return NULL or any other possible variable.

In virsh the assumption is that the flags are checked at first and thus
returning false makes sense, as every virsh command does that.

> + *
> + * Reject mutually exclusive API flags.  The checked flags are compared
> + * with flags variable.
> + *
> + * This helper does an early return and therefore it has to be called
> + * before anything that would require cleanup.
> + */
> +# define VIR_EXCLUSIVE_FLAGS_RET(FLAG1, FLAG2)  \
> +VIR_EXCLUSIVE_FLAGS_EXPR_RET(#FLAG1, flags & FLAG1, #FLAG2, flags & 
> FLAG2)
> +
> +/**
> + * VIR_EXCLUSIVE_FLAGS_EXPR_GOTO:
> + *
> + * @NAME1: String containing the name of the flag.
> + * @EXPR1: Expression to validate the flag.
> + * @NAME2: String containing the name of the flag.
> + * @EXPR2: Expression to validate the flag.
> + * @LABEL: label to jump to.
> + *
> + * Reject mutually exclusive API flags.  Use the provided expression
> + * to check the flag.
> + *
> + * Returns nothing.  Jumps to a label if unsupported flags were
> + * passed to it.
> + */
> +# define VIR_EXCLUSIVE_FLAGS_EXPR_GOTO(NAME1, EXPR1, NAME2, EXPR2, LABEL)   \
> +if ((EXPR1) && (EXPR2)) {   \
> +virReportInvalidArg(ctl,\
> +_("Flags '%s' and '%s' are mutually exclusive"),\
> +NAME1, NAME2);  \
> +goto LABEL; \
> +}
> +

Again, this function does not make sense in the library code.

> +/**
> + * VIR_EXCLUSIVE_FLAGS_VAR_GOTO:
> + *
> + * @FLAG1: First flag to be checked.
> + * @FLAG2: Second flag to be checked.
> + * @LABEL: label to jump to.
> + *
> + * Reject mutually exclusive API flags.  The checked flags are compared
> + * with flags variable.
> + *
> + * Returns nothing.  Jumps to a label if unsupported flags were
> + * passed to it.
> + */
> +# define VIR_EXCLUSIVE_FLAGS_GOTO(FLAG1, FLAG2, LABEL)  \
> +VIR_EXCLUSIVE_FLAGS_EXPR_GOTO(#FLAG1, flags & FLAG1,\
> +  #FLAG2, flags & FLAG2, LABEL)
> +
> +
>  # define virCheckNonNullArgReturn(argname, retval)  \
>  do {\
>  if (argname == NULL) {  \

While this is okay.

Peter


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

Re: [libvirt] [PATCH] qemucaps2xmltest: fix the test to correspond to new domain formatting

[Pavel Hrdina]

On Tue, Mar 24, 2015 at 04:36:09PM +0100, Peter Krempa wrote:
> On Tue, Mar 24, 2015 at 15:31:09 +0100, Pavel Hrdina wrote:
> > Commit 2360fe5d updated formating of  element but forget to
> 
> s/forget/forgot/
> 
> > update qemucaps2xmldata xml files.  In addition the test code was broken
> > too.  Update the xml files and return -1 if testCompareXMLToXML fails
> > together with indentation fix.
> > 
> > Signed-off-by: Pavel Hrdina 
> > ---
> >  tests/qemucaps2xmldata/all_1.6.0-1.xml| 3 +--
> >  tests/qemucaps2xmldata/nodisksnapshot_1.6.0-1.xml | 3 +--
> >  tests/qemucaps2xmltest.c  | 4 ++--
> >  3 files changed, 4 insertions(+), 6 deletions(-)
> > 
> > diff --git a/tests/qemucaps2xmldata/all_1.6.0-1.xml 
> > b/tests/qemucaps2xmldata/all_1.6.0-1.xml
> > index 425b22e..2489f49 100644
> > --- a/tests/qemucaps2xmldata/all_1.6.0-1.xml
> > +++ b/tests/qemucaps2xmldata/all_1.6.0-1.xml
> > @@ -12,8 +12,7 @@
> >  
> >32
> >/usr/bin/qemu-system-i386
> > -  
> > -  
> > +  
> >
> >  /usr/bin/qemu-system-i386
> >
> > diff --git a/tests/qemucaps2xmldata/nodisksnapshot_1.6.0-1.xml 
> > b/tests/qemucaps2xmldata/nodisksnapshot_1.6.0-1.xml
> > index cd19814..281fab0 100644
> > --- a/tests/qemucaps2xmldata/nodisksnapshot_1.6.0-1.xml
> > +++ b/tests/qemucaps2xmldata/nodisksnapshot_1.6.0-1.xml
> > @@ -12,8 +12,7 @@
> >  
> >32
> >/usr/bin/qemu-system-i386
> > -  
> > -  
> > +  
> >
> >  /usr/bin/qemu-system-i386
> >
> 
> I'd split this patch at this point, as the hunks above fix a test data
> failure, while the hunks below fix a bug in the test code.
> 
> > diff --git a/tests/qemucaps2xmltest.c b/tests/qemucaps2xmltest.c
> > index 3529acb..b3975b4 100644
> > --- a/tests/qemucaps2xmltest.c
> > +++ b/tests/qemucaps2xmltest.c
> > @@ -31,7 +31,7 @@
> >  static int
> >  testCompareXMLToXML(const char *inxmldata, const char *outxmldata)
> >  {
> > -int ret = 1;
> > +int ret = -1;
> >  
> >  if (STRNEQ(outxmldata, inxmldata)) {
> >  virtTestDifference(stderr, outxmldata, inxmldata);
> > @@ -143,7 +143,7 @@ testQemuCapsXML(const void *opaque)
> >  char *capsXml = NULL;
> >  virCapsPtr capsProvided = NULL;
> >  
> > -   if (virAsprintf(&xmlFile, "%s/qemucaps2xmldata/%s.xml",
> > +if (virAsprintf(&xmlFile, "%s/qemucaps2xmldata/%s.xml",
> >  abs_srcdir, data->base) < 0)
> >  goto cleanup;
> 
> ACK as is. It's not worth splitting/respinning.
> 
> Peter

Thanks, pushed now.

Pavel

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

Re: [libvirt] [PATCH] qemucaps2xmltest: fix the test to correspond to new domain formatting

[Peter Krempa]

On Tue, Mar 24, 2015 at 15:31:09 +0100, Pavel Hrdina wrote:
> Commit 2360fe5d updated formating of  element but forget to

s/forget/forgot/

> update qemucaps2xmldata xml files.  In addition the test code was broken
> too.  Update the xml files and return -1 if testCompareXMLToXML fails
> together with indentation fix.
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  tests/qemucaps2xmldata/all_1.6.0-1.xml| 3 +--
>  tests/qemucaps2xmldata/nodisksnapshot_1.6.0-1.xml | 3 +--
>  tests/qemucaps2xmltest.c  | 4 ++--
>  3 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/qemucaps2xmldata/all_1.6.0-1.xml 
> b/tests/qemucaps2xmldata/all_1.6.0-1.xml
> index 425b22e..2489f49 100644
> --- a/tests/qemucaps2xmldata/all_1.6.0-1.xml
> +++ b/tests/qemucaps2xmldata/all_1.6.0-1.xml
> @@ -12,8 +12,7 @@
>  
>32
>/usr/bin/qemu-system-i386
> -  
> -  
> +  
>
>  /usr/bin/qemu-system-i386
>
> diff --git a/tests/qemucaps2xmldata/nodisksnapshot_1.6.0-1.xml 
> b/tests/qemucaps2xmldata/nodisksnapshot_1.6.0-1.xml
> index cd19814..281fab0 100644
> --- a/tests/qemucaps2xmldata/nodisksnapshot_1.6.0-1.xml
> +++ b/tests/qemucaps2xmldata/nodisksnapshot_1.6.0-1.xml
> @@ -12,8 +12,7 @@
>  
>32
>/usr/bin/qemu-system-i386
> -  
> -  
> +  
>
>  /usr/bin/qemu-system-i386
>

I'd split this patch at this point, as the hunks above fix a test data
failure, while the hunks below fix a bug in the test code.

> diff --git a/tests/qemucaps2xmltest.c b/tests/qemucaps2xmltest.c
> index 3529acb..b3975b4 100644
> --- a/tests/qemucaps2xmltest.c
> +++ b/tests/qemucaps2xmltest.c
> @@ -31,7 +31,7 @@
>  static int
>  testCompareXMLToXML(const char *inxmldata, const char *outxmldata)
>  {
> -int ret = 1;
> +int ret = -1;
>  
>  if (STRNEQ(outxmldata, inxmldata)) {
>  virtTestDifference(stderr, outxmldata, inxmldata);
> @@ -143,7 +143,7 @@ testQemuCapsXML(const void *opaque)
>  char *capsXml = NULL;
>  virCapsPtr capsProvided = NULL;
>  
> -   if (virAsprintf(&xmlFile, "%s/qemucaps2xmldata/%s.xml",
> +if (virAsprintf(&xmlFile, "%s/qemucaps2xmldata/%s.xml",
>  abs_srcdir, data->base) < 0)
>  goto cleanup;

ACK as is. It's not worth splitting/respinning.

Peter


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

[libvirt] [PATCH 1/2] qemu: command: Report error when formatting network source with protocol _NONE

[Peter Krempa]

The function that formats the string for network drives would return
error code but did not set the error message when called on storage
source with VIR_STORAGE_NET_PROTOCOL_LAST or _NONE.

Report an error in this case if it would ever be called in that way.
---
 src/qemu/qemu_command.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 7e6b95c..8dd7a76 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3263,6 +3263,9 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src,

 case VIR_STORAGE_NET_PROTOCOL_LAST:
 case VIR_STORAGE_NET_PROTOCOL_NONE:
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Unexpected network protocol '%s'"),
+   virStorageNetProtocolTypeToString(src->protocol));
 goto cleanup;
 }

-- 
2.2.2

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

[libvirt] [PATCH 0/2] Fix "unknown error" when starting qemu VM with empty network cdrom

[Peter Krempa]

Peter Krempa (2):
  qemu: command: Report error when formatting network source with
protocol _NONE
  qemu: command: Check for empty network source when formatting drive
cmd

 src/qemu/qemu_command.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

-- 
2.2.2

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

[libvirt] [PATCH 2/2] qemu: command: Check for empty network source when formatting drive cmd

[Peter Krempa]

Use the virStorageSourceIsEmpty helper to determine whether the drive
source is empty rather than checking for src->path. This will fix start
of VM with empty network cdrom that would not report any error.
---
 src/qemu/qemu_command.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 8dd7a76..43eecf8 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3289,6 +3289,10 @@ qemuGetDriveSourceString(virStorageSourcePtr src,

 *source = NULL;

+/* return 1 for empty sources */
+if (virStorageSourceIsEmpty(src))
+return 1;
+
 if (conn) {
 if (actualType == VIR_STORAGE_TYPE_NETWORK &&
 src->auth &&
@@ -3318,11 +3322,6 @@ qemuGetDriveSourceString(virStorageSourcePtr src,
 case VIR_STORAGE_TYPE_BLOCK:
 case VIR_STORAGE_TYPE_FILE:
 case VIR_STORAGE_TYPE_DIR:
-if (!src->path) {
-ret = 1;
-goto cleanup;
-}
-
 if (VIR_STRDUP(*source, src->path) < 0)
 goto cleanup;

-- 
2.2.2

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

Re: [libvirt] [PATCH v3 2/2] network_conf: check if bridge exists on host for user created bridges

[Laine Stump]

On 03/24/2015 05:59 AM, Shivaprasad G Bhat wrote:
> virNetworkBridgeInUse() doesn't check if the bridge is manually created
> outside of libvirt. Check if the bridge actually exist on host using the
> virNetDevExists().
>
> Signed-off-by: Shivaprasad G Bhat 
> ---
>  src/conf/network_conf.c |   15 ---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index d7c5dec..c3ae2e4 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -3227,13 +3227,22 @@ virNetworkBridgeInUseHelper(const void *payload,
>  int ret = 0;
>  virNetworkObjPtr net = (virNetworkObjPtr) payload;
>  const struct virNetworkBridgeInUseHelperData *data = opaque;
> +bool defined_bridge = false;
>  
>  virObjectLock(net);
>  if (net->def->bridge &&
> -STREQ(net->def->bridge, data->bridge) &&
> -!(data->skipname && STREQ(net->def->name, data->skipname)))
> -ret = 1;
> +STREQ(net->def->bridge, data->bridge)) {
> +defined_bridge = true;
> +if (!(data->skipname && STREQ(net->def->name, data->skipname)))
> + ret = 1;
> +}
> +
>  virObjectUnlock(net);
> +
> +/* Bridge might have been created by a user manually outside libvirt */
> +if (!defined_bridge && !ret)
> +ret = virNetDevExists(data->bridge) ? 1 : 0;
> +
>  return ret;
>  }

This function is intended to be called once for each defined network on
the host, with data->bridge being the same each time, but
net->def->bridge being different, so adding the check for data->bridge
existence here may work, but it's a bit convoluted.

Instead, you should leave this function alone, and just add the extra
check directly in the function virNetworkBridgeInUse() (either before
locking, or after unlocking "nets").

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

Re: [libvirt] [PATCH] qemu: change accidental VIR_WARNING back to VIR_DEBUG

[John Ferlan]

On 03/24/2015 10:49 AM, Laine Stump wrote:
> While debugging the support for responding to qemu RX_FILTER_CHANGED
> events, I had changed the "ignoring this event" log message from
> VIR_DEBUG to VIR_WARN, but forgot to change it back before
> pushing. Since many guest OSes make enough changes to multicast lists
> and/or promiscuous mode settings to trigger this message, it's
> starting to show up as a red herring in bug reports.
> ---
>  src/qemu/qemu_driver.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

ACK -

Ironically I had just started noticing this in a debug session and began
wondering...

John

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

[libvirt] [PATCH] qemu: change accidental VIR_WARNING back to VIR_DEBUG

[Laine Stump]

While debugging the support for responding to qemu RX_FILTER_CHANGED
events, I had changed the "ignoring this event" log message from
VIR_DEBUG to VIR_WARN, but forgot to change it back before
pushing. Since many guest OSes make enough changes to multicast lists
and/or promiscuous mode settings to trigger this message, it's
starting to show up as a red herring in bug reports.
---
 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 6d9217b..2bac4a9 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4340,7 +4340,7 @@ processNicRxFilterChangedEvent(virQEMUDriverPtr driver,
 def = dev.data.net;
 
 if (!virDomainNetGetActualTrustGuestRxFilters(def)) {
-VIR_WARN("ignore NIC_RX_FILTER_CHANGED event for network "
+VIR_DEBUG("ignore NIC_RX_FILTER_CHANGED event for network "
   "device %s in domain %s",
   def->info.alias, vm->def->name);
 /* not sending "query-rx-filter" will also suppress any
-- 
2.1.0

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

Re: [libvirt] [PATCH 2/7] conf: Add support for storage state directory

[Peter Krempa]

On Tue, Mar 24, 2015 at 10:16:25 -0400, John Ferlan wrote:
> 
> 
> On 03/24/2015 06:06 AM, Erik Skultety wrote:
> > Before introducing necessary changes to storage_driver.c, first prepare
> > our structures for storage state XML support.
> > 
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733
> > ---
> >  src/conf/storage_conf.h  | 1 +
> >  src/storage/storage_driver.c | 1 +
> >  2 files changed, 2 insertions(+)
> > 
> 
> I had started down this path before for a different bug/issue:
> 
> http://www.redhat.com/archives/libvir-list/2015-January/msg00465.html
> 
> but abandoned the stateDir because it was felt it wasn't necessary.  I
> recall running into a few "interesting" issues with stateDir including
> fixing one issue seen during testing that didn't hit the list.  Good

The patches above are for status XMLs for storage volumes while this
series does the same for storage pools. While volumes can theoretically
be reloaded from existing pools this series fixes storage pools that
vanish during start of libvirt.

> news is I still have the patches in a branch somewhere if you're
> interested.  1 & 2 are on list... The 3rd one in the archive was a
> solution to the particular problem - that was rejected and a different
> solution was pushed.
> 
> In any case, I do suggest looking at 1 & 2, plus I can send you an
> adjustment to 1 to resolve some condition I ran into, but cannot recall
> the details.

Peter



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

[libvirt] [PATCH] qemucaps2xmltest: fix the test to correspond to new domain formatting

[Pavel Hrdina]

Commit 2360fe5d updated formating of  element but forget to
update qemucaps2xmldata xml files.  In addition the test code was broken
too.  Update the xml files and return -1 if testCompareXMLToXML fails
together with indentation fix.

Signed-off-by: Pavel Hrdina 
---
 tests/qemucaps2xmldata/all_1.6.0-1.xml| 3 +--
 tests/qemucaps2xmldata/nodisksnapshot_1.6.0-1.xml | 3 +--
 tests/qemucaps2xmltest.c  | 4 ++--
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/tests/qemucaps2xmldata/all_1.6.0-1.xml 
b/tests/qemucaps2xmldata/all_1.6.0-1.xml
index 425b22e..2489f49 100644
--- a/tests/qemucaps2xmldata/all_1.6.0-1.xml
+++ b/tests/qemucaps2xmldata/all_1.6.0-1.xml
@@ -12,8 +12,7 @@
 
   32
   /usr/bin/qemu-system-i386
-  
-  
+  
   
 /usr/bin/qemu-system-i386
   
diff --git a/tests/qemucaps2xmldata/nodisksnapshot_1.6.0-1.xml 
b/tests/qemucaps2xmldata/nodisksnapshot_1.6.0-1.xml
index cd19814..281fab0 100644
--- a/tests/qemucaps2xmldata/nodisksnapshot_1.6.0-1.xml
+++ b/tests/qemucaps2xmldata/nodisksnapshot_1.6.0-1.xml
@@ -12,8 +12,7 @@
 
   32
   /usr/bin/qemu-system-i386
-  
-  
+  
   
 /usr/bin/qemu-system-i386
   
diff --git a/tests/qemucaps2xmltest.c b/tests/qemucaps2xmltest.c
index 3529acb..b3975b4 100644
--- a/tests/qemucaps2xmltest.c
+++ b/tests/qemucaps2xmltest.c
@@ -31,7 +31,7 @@
 static int
 testCompareXMLToXML(const char *inxmldata, const char *outxmldata)
 {
-int ret = 1;
+int ret = -1;
 
 if (STRNEQ(outxmldata, inxmldata)) {
 virtTestDifference(stderr, outxmldata, inxmldata);
@@ -143,7 +143,7 @@ testQemuCapsXML(const void *opaque)
 char *capsXml = NULL;
 virCapsPtr capsProvided = NULL;
 
-   if (virAsprintf(&xmlFile, "%s/qemucaps2xmldata/%s.xml",
+if (virAsprintf(&xmlFile, "%s/qemucaps2xmldata/%s.xml",
 abs_srcdir, data->base) < 0)
 goto cleanup;
 
-- 
2.0.5

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

Re: [libvirt] [Xen-devel] libvirt/libxl implemetation of get_online_cpu / virNodeGetCPUMap?

[Manish Jaggi]

On Tuesday 24 February 2015 08:30 PM, Anthony PERARD wrote:

On Tue, Feb 24, 2015 at 01:22:19PM +, Daniel P. Berrange wrote:

On Tue, Feb 24, 2015 at 01:15:57PM +, Anthony PERARD wrote:

On Tue, Feb 24, 2015 at 12:46:44PM +, Daniel P. Berrange wrote:

On Tue, Feb 24, 2015 at 12:41:01PM +, Anthony PERARD wrote:

Hi,

A recent OpenStack nova commit make use of virNodeGetCPUMap to get the list
of online cpu of a host. But this API is not implemented for the libvirt
xen driver.

The commit:
   Add handling for offlined CPUs to the nova libvirt driver.
https://review.openstack.org/gitweb?p=openstack/nova.git;a=commitdiff;h=0696a5cd5f0fdc08951a074961bb8ce0c3310086

FWIW, this should not impact Xen based on my understanding. The code
path in question should only be used when Nova is setup todo NUMA
pinning support, and that is not supported with Xen in OpenStack,
only KVM.  Did it actually cause failures for you, or are you simply
keeping track of all used APIs in Nova as a sanity check ?

It prevent nova from starting. I do the setup with DevStack.

The error:
libvirtError: this function is not supported by the connection driver: 
virNodeGetCPUMap

And a part of the traceback:
   File "/opt/stack/nova/nova/openstack/common/service.py", line 491, in 
run_service
 service.start()
   File "/opt/stack/nova/nova/service.py", line 181, in start
 self.manager.pre_start_hook()
   File "/opt/stack/nova/nova/compute/manager.py", line 1188, in pre_start_hook
 self.update_available_resource(nova.context.get_admin_context())
   File "/opt/stack/nova/nova/compute/manager.py", line 6062, in 
update_available_resource
 rt.update_available_resource(context)
   File "/opt/stack/nova/nova/compute/resource_tracker.py", line 315, in 
update_available_resource
 resources = self.driver.get_available_resource(self.nodename)
   File "/opt/stack/nova/nova/virt/libvirt/driver.py", line 4896, in 
get_available_resource
 numa_topology = self._get_host_numa_topology()
   File "/opt/stack/nova/nova/virt/libvirt/driver.py", line 4749, in 
_get_host_numa_topology
 online_cpus = self._host.get_online_cpus()
   File "/opt/stack/nova/nova/virt/libvirt/host.py", line 599, in 
get_online_cpus
 (cpus, cpu_map, online) = self.get_connection().getCPUMap()

I'll look into why nova is going through NUMA code paths then.

Oh damn, yes, I understand why now. Please file a bug against Nova for
this, as we must fix it as a high pripority. It was certainly not my
intention to break Xen when I approved this change
I applied the patch, not getting this python libvirtError, but libvirt 
deamon is throwing error.
2015-03-24 08:46:31.169+: 1030: error : virNodeGetCPUMap:1342 : this 
function is not supported by the connection driver: virNodeGetCPUMap



Here is the bug report: https://bugs.launchpad.net/nova/+bug/1425115

Regards,



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

Re: [libvirt] [PATCH 2/7] conf: Add support for storage state directory

[John Ferlan]

On 03/24/2015 06:06 AM, Erik Skultety wrote:
> Before introducing necessary changes to storage_driver.c, first prepare
> our structures for storage state XML support.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733
> ---
>  src/conf/storage_conf.h  | 1 +
>  src/storage/storage_driver.c | 1 +
>  2 files changed, 2 insertions(+)
> 

I had started down this path before for a different bug/issue:

http://www.redhat.com/archives/libvir-list/2015-January/msg00465.html

but abandoned the stateDir because it was felt it wasn't necessary.  I
recall running into a few "interesting" issues with stateDir including
fixing one issue seen during testing that didn't hit the list.  Good
news is I still have the patches in a branch somewhere if you're
interested.  1 & 2 are on list... The 3rd one in the archive was a
solution to the particular problem - that was rejected and a different
solution was pushed.

In any case, I do suggest looking at 1 & 2, plus I can send you an
adjustment to 1 to resolve some condition I ran into, but cannot recall
the details.

John
> diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h
> index 4584075..8ccc947 100644
> --- a/src/conf/storage_conf.h
> +++ b/src/conf/storage_conf.h
> @@ -293,6 +293,7 @@ struct _virStorageDriverState {
>  
>  char *configDir;
>  char *autostartDir;
> +char *stateDir;
>  bool privileged;
>  };
>  
> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index 64ea770..e088ffa 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -261,6 +261,7 @@ storageStateCleanup(void)
>  
>  VIR_FREE(driver->configDir);
>  VIR_FREE(driver->autostartDir);
> +VIR_FREE(driver->stateDir);
>  storageDriverUnlock();
>  virMutexDestroy(&driver->lock);
>  VIR_FREE(driver);
> 

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

[libvirt] [PATCH] qemu: fix can use setmaxmem to change the maximum memory biger than max memory

[Luyao Huang]

When call qemuDomainSetMemoryFlags() to change maximum memory size, we
do not check if the maximum memory is biger than the max memory, this will
make vm disappear after libvirtd restart if we set a big maximum memory.

Add a check for this, also fix a typos issue.

Signed-off-by: Luyao Huang 
---
 src/conf/domain_conf.c | 2 +-
 src/qemu/qemu_driver.c | 7 +++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d633f93..0d4b165 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -16646,7 +16646,7 @@ virDomainDefCheckABIStability(virDomainDefPtr src,
 
 if (src->mem.memory_slots != dst->mem.memory_slots) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("Target domain memory slots count '%u' doesn't match 
source '%u"),
+   _("Target domain memory slots count '%u' doesn't match 
source '%u'"),
dst->mem.memory_slots, src->mem.memory_slots);
 goto error;
 }
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 3518dec..86b87af 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2324,6 +2324,13 @@ static int qemuDomainSetMemoryFlags(virDomainPtr dom, 
unsigned long newmem,
  "nodes cannot be modified with this API"));
 goto endjob;
 }
+if (persistentDef->mem.max_memory &&
+persistentDef->mem.max_memory < newmem) {
+virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+   _("cannot set maximum memory size biger than "
+ "the max memory size"));
+goto endjob;
+}
 
 virDomainDefSetMemoryInitial(persistentDef, newmem);
 
-- 
1.8.3.1

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

[libvirt] [PATCH 1/4] qemu: domain: Don't leak device alias list

[Peter Krempa]

While adding tests for status XML parsing and formatting I've noticed
that the device alias list is leaked.

==763001== 81 (48 direct, 33 indirect) bytes in 1 blocks are definitely lost in 
loss record 414 of 514
==763001==at 0x4C2B8F0: calloc (vg_replace_malloc.c:623)
==763001==by 0x6ACF70F: virAllocN (viralloc.c:191)
==763001==by 0x447B64: qemuDomainObjPrivateXMLParse (qemu_domain.c:727)
==763001==by 0x6B848F9: virDomainObjParseXML (domain_conf.c:15491)
==763001==by 0x6B84CAC: virDomainObjParseNode (domain_conf.c:15608)
---
 src/qemu/qemu_domain.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 655afb9..1cf1aee 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -439,6 +439,7 @@ qemuDomainObjPrivateFree(void *data)
 VIR_FREE(priv->origname);

 virCondDestroy(&priv->unplugFinished);
+virStringFreeList(priv->qemuDevices);
 virChrdevFree(priv->devs);

 /* This should never be non-NULL if we get here, but just in case... */
-- 
2.2.2

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

[libvirt] [PATCH 0/4] Test status XML formatting and parsing

[Peter Krempa]

A recent bug showed that the status XML parsing is not tested. Add test cases
based on existing tests in the XML-2-XML test to excercise the parser.

Additionally this series fixes also a memleak of the domain device alias list.

Peter Krempa (4):
  qemu: domain: Don't leak device alias list
  util: buffer: Add support for adding text blocks with indentation
  tests: xml2xml: Refactor the qemu xml 2 xml test
  test: qemuxml2xml: Test status XML formatting and parsing

 src/conf/domain_conf.c   |   4 +-
 src/conf/domain_conf.h   |   9 ++
 src/libvirt_private.syms |   3 +
 src/qemu/qemu_domain.c   |   1 +
 src/util/virbuffer.c |  38 ++
 src/util/virbuffer.h |   1 +
 tests/qemuxml2xmltest.c  | 309 ---
 tests/virbuftest.c   |  50 
 8 files changed, 345 insertions(+), 70 deletions(-)

-- 
2.2.2

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

[libvirt] [PATCH 4/4] test: qemuxml2xml: Test status XML formatting and parsing

[Peter Krempa]

Recently we've fixed a bug where the status XML could not be parsed as
the parser used absolute path XPath queries. This test enhancement tests
all XML files used in the qemu-xml-2-xml test as a part of a status XML
snippet to see whether they are parsed correctly. The status XML-2-XML is
currently tested in 223 cases with this patch.
---
 src/conf/domain_conf.c   |   4 +-
 src/conf/domain_conf.h   |   9 
 src/libvirt_private.syms |   2 +
 tests/qemuxml2xmltest.c  | 107 +++
 4 files changed, 120 insertions(+), 2 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d633f93..d28b62a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15580,7 +15580,7 @@ virDomainDefParseNode(xmlDocPtr xml,
 }


-static virDomainObjPtr
+virDomainObjPtr
 virDomainObjParseNode(xmlDocPtr xml,
   xmlNodePtr root,
   virCapsPtr caps,
@@ -21252,7 +21252,7 @@ virDomainDefFormat(virDomainDefPtr def, unsigned int 
flags)
 }


-static char *
+char *
 virDomainObjFormat(virDomainXMLOptionPtr xmlopt,
virDomainObjPtr obj,
unsigned int flags)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index bceb2d7..608f61f 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2565,6 +2565,12 @@ virDomainDefPtr virDomainDefParseNode(xmlDocPtr doc,
   virDomainXMLOptionPtr xmlopt,
   unsigned int expectedVirtTypes,
   unsigned int flags);
+virDomainObjPtr virDomainObjParseNode(xmlDocPtr xml,
+  xmlNodePtr root,
+  virCapsPtr caps,
+  virDomainXMLOptionPtr xmlopt,
+  unsigned int expectedVirtTypes,
+  unsigned int flags);
 virDomainObjPtr virDomainObjParseFile(const char *filename,
   virCapsPtr caps,
   virDomainXMLOptionPtr xmlopt,
@@ -2580,6 +2586,9 @@ unsigned int virDomainDefFormatConvertXMLFlags(unsigned 
int flags);

 char *virDomainDefFormat(virDomainDefPtr def,
  unsigned int flags);
+char *virDomainObjFormat(virDomainXMLOptionPtr xmlopt,
+ virDomainObjPtr obj,
+ unsigned int flags);
 int virDomainDefFormatInternal(virDomainDefPtr def,
unsigned int flags,
virBufferPtr buf);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 0beb44f..4ab8638 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -364,6 +364,7 @@ virDomainNostateReasonTypeFromString;
 virDomainNostateReasonTypeToString;
 virDomainObjAssignDef;
 virDomainObjCopyPersistentDef;
+virDomainObjFormat;
 virDomainObjGetMetadata;
 virDomainObjGetPersistentDef;
 virDomainObjGetState;
@@ -382,6 +383,7 @@ virDomainObjListNumOfDomains;
 virDomainObjListRemove;
 virDomainObjListRemoveLocked;
 virDomainObjNew;
+virDomainObjParseNode;
 virDomainObjSetDefTransient;
 virDomainObjSetMetadata;
 virDomainObjSetState;
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 627edca..b419231 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -106,6 +106,109 @@ testXML2XMLInactive(const void *opaque)
 }


+static const char testStatusXMLPrefix[] =
+"\n"
+"  \n"
+"  \n"
+"  \n"
+"\n"
+"  \n"
+"  \n"
+"\n"
+"\n"
+"\n"
+"\n"
+"\n"
+"\n"
+"\n"
+"\n"
+"\n"
+"  \n"
+"  \n"
+"\n"
+"\n"
+"\n"
+"\n"
+"\n"
+"  \n";
+
+static const char testStatusXMLSuffix[] =
+"\n";
+
+
+static int
+testCompareStatusXMLToXMLFiles(const void *opaque)
+{
+const struct testInfo *data = opaque;
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+xmlDocPtr xml = NULL;
+virDomainObjPtr obj = NULL;
+char *expect = NULL;
+char *actual = NULL;
+char *source = NULL;
+int ret = -1;
+int keepBlanksDefault = xmlKeepBlanksDefault(0);
+
+/* construct faked source status XML */
+virBufferAdd(&buf, testStatusXMLPrefix, -1);
+virBufferAdjustIndent(&buf, 2);
+virBufferAddStr(&buf, data->inFile);
+virBufferAdjustIndent(&buf, -2);
+virBufferAdd(&buf, testStatusXMLSuffix, -1);
+
+if (!(source = virBufferContentAndReset(&buf))) {
+fprintf(stderr, "Failed to create the source XML");
+goto cleanup;
+}
+
+/* construct the expect string */
+virBufferAdd(&buf, testStatusXMLPrefix, -1);
+virBufferAdjustIndent(&buf, 2);
+virBufferAddStr(&buf, data->outActiveFile);
+virBufferAdjustIndent(&buf, -2);
+virBufferAdd(&buf, testStatusXMLSuffix, -1);
+
+if (!(expect = virBufferContentAndReset(&buf))) {
+fprintf(stderr, "Failed to create the expect XM

[libvirt] [PATCH 2/4] util: buffer: Add support for adding text blocks with indentation

[Peter Krempa]

The current auto-indentation buffer code applies indentation only on
complete strings. To allow adding a string containing newlines and
having it properly indented this patch adds virBufferAddStr.
---
 src/libvirt_private.syms |  1 +
 src/util/virbuffer.c | 38 
 src/util/virbuffer.h |  1 +
 tests/virbuftest.c   | 50 
 4 files changed, 90 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 33222f0..0beb44f 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1099,6 +1099,7 @@ virBitmapToData;
 virBufferAdd;
 virBufferAddBuffer;
 virBufferAddChar;
+virBufferAddStr;
 virBufferAdjustIndent;
 virBufferAsprintf;
 virBufferCheckErrorInternal;
diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c
index 0089d1b..50d953e 100644
--- a/src/util/virbuffer.c
+++ b/src/util/virbuffer.c
@@ -756,3 +756,41 @@ virBufferTrim(virBufferPtr buf, const char *str, int len)
 buf->use -= len < 0 ? len2 : len;
 buf->content[buf->use] = '\0';
 }
+
+
+/**
+ * virBufferAddStr:
+ * @buf: the buffer to append to
+ * @str: string to append
+ *
+ * Appends @str to @buffer. Applies autoindentation on the separate lines of
+ * @str.
+ */
+void
+virBufferAddStr(virBufferPtr buf,
+const char *str)
+{
+size_t len = 0;
+const char *start = str;
+
+if (!buf || !str || buf->error)
+return;
+
+while (*str) {
+len++;
+
+if (*str == '\n') {
+virBufferAdd(buf, start, len);
+str++;
+len = 0;
+start = str;
+
+continue;
+}
+
+str++;
+}
+
+if (len > 0)
+virBufferAdd(buf, start, len);
+}
diff --git a/src/util/virbuffer.h b/src/util/virbuffer.h
index 24e81c7..144a1ba 100644
--- a/src/util/virbuffer.h
+++ b/src/util/virbuffer.h
@@ -96,5 +96,6 @@ void virBufferAdjustIndent(virBufferPtr buf, int indent);
 int virBufferGetIndent(const virBuffer *buf, bool dynamic);

 void virBufferTrim(virBufferPtr buf, const char *trim, int len);
+void virBufferAddStr(virBufferPtr buf, const char *str);

 #endif /* __VIR_BUFFER_H__ */
diff --git a/tests/virbuftest.c b/tests/virbuftest.c
index f964feb..067a77e 100644
--- a/tests/virbuftest.c
+++ b/tests/virbuftest.c
@@ -310,6 +310,44 @@ static int testBufAddBuffer(const void *data 
ATTRIBUTE_UNUSED)
 return ret;
 }

+struct testBufAddStrData {
+const char *data;
+const char *expect;
+};
+
+static int
+testBufAddStr(const void *opaque ATTRIBUTE_UNUSED)
+{
+const struct testBufAddStrData *data = opaque;
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+char *actual;
+int ret = -1;
+
+virBufferAddLit(&buf, "\n");
+virBufferAdjustIndent(&buf, 2);
+virBufferAddStr(&buf, data->data);
+virBufferAdjustIndent(&buf, -2);
+virBufferAddLit(&buf, "");
+
+if (!(actual = virBufferContentAndReset(&buf))) {
+TEST_ERROR("buf is empty");
+goto cleanup;
+}
+
+if (STRNEQ_NULLABLE(actual, data->expect)) {
+TEST_ERROR("testBufAddStr(): Strings don't match:\n"
+   "Expected:\n%s\nActual:\n%s\n",
+   data->expect, actual);
+goto cleanup;
+}
+
+ret = 0;
+
+ cleanup:
+VIR_FREE(actual);
+return ret;
+}
+

 static int
 mymain(void)
@@ -330,6 +368,18 @@ mymain(void)
 DO_TEST("Trim", testBufTrim, 0);
 DO_TEST("AddBuffer", testBufAddBuffer, 0);

+#define DO_TEST_ADD_STR(DATA, EXPECT)  \
+do {   \
+struct testBufAddStrData info = { DATA, EXPECT };  \
+if (virtTestRun("Buf: AddStr", testBufAddStr, &info) < 0)  \
+ret = -1;  \
+} while (0)
+
+DO_TEST_ADD_STR("", "\n");
+DO_TEST_ADD_STR("", "\n  ");
+DO_TEST_ADD_STR("\n", "\n  \n");
+DO_TEST_ADD_STR("\n  \n\n", "\n  \n\n  
\n");
+
 return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
 }

-- 
2.2.2

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

[libvirt] [PATCH 3/4] tests: xml2xml: Refactor the qemu xml 2 xml test

[Peter Krempa]

To allow adding more tests, refactor the XML-2-XML test so that the
files are not reloaded always and clarify the control flow.

Result of this changes is that the active and inactive portions of the
XML are tested in separate steps rather than one test step.
---
 tests/qemuxml2xmltest.c | 214 +++-
 1 file changed, 140 insertions(+), 74 deletions(-)

diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 0f16d5e..627edca 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -22,11 +22,30 @@

 static virQEMUDriver driver;

+enum {
+WHEN_INACTIVE = 1,
+WHEN_ACTIVE = 2,
+WHEN_EITHER = 3,
+};
+
+struct testInfo {
+char *inName;
+char *inFile;
+
+char *outActiveName;
+char *outActiveFile;
+
+char *outInactiveName;
+char *outInactiveFile;
+};
+
 static int
-testCompareXMLToXMLFiles(const char *inxml, const char *outxml, bool live)
+testXML2XMLHelper(const char *inxml,
+  const char *inXmlData,
+  const char *outxml,
+  const char *outXmlData,
+  bool live)
 {
-char *inXmlData = NULL;
-char *outXmlData = NULL;
 char *actual = NULL;
 int ret = -1;
 virDomainDefPtr def = NULL;
@@ -35,11 +54,6 @@ testCompareXMLToXMLFiles(const char *inxml, const char 
*outxml, bool live)
 if (!live)
 format_flags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE;

-if (virtTestLoadFile(inxml, &inXmlData) < 0)
-goto fail;
-if (virtTestLoadFile(outxml, &outXmlData) < 0)
-goto fail;
-
 if (!(def = virDomainDefParseString(inXmlData, driver.caps, driver.xmlopt,
 QEMU_EXPECTED_VIRT_TYPES, 
parse_flags)))
 goto fail;
@@ -58,82 +72,120 @@ testCompareXMLToXMLFiles(const char *inxml, const char 
*outxml, bool live)
 }

 ret = 0;
+
  fail:
-VIR_FREE(inXmlData);
-VIR_FREE(outXmlData);
 VIR_FREE(actual);
 virDomainDefFree(def);
 return ret;
 }

-enum {
-WHEN_INACTIVE = 1,
-WHEN_ACTIVE = 2,
-WHEN_EITHER = 3,
-};

-struct testInfo {
-const char *name;
-bool different;
-int when;
-};
+static int
+testXML2XMLActive(const void *opaque)
+{
+const struct testInfo *info = opaque;
+
+return testXML2XMLHelper(info->inName,
+ info->inFile,
+ info->outActiveName,
+ info->outActiveFile,
+ true);
+}
+

 static int
-testCompareXMLToXMLHelper(const void *data)
+testXML2XMLInactive(const void *opaque)
 {
-const struct testInfo *info = data;
-char *xml_in = NULL;
-char *xml_out = NULL;
-char *xml_out_active = NULL;
-char *xml_out_inactive = NULL;
-int ret = -1;
+const struct testInfo *info = opaque;
+
+return testXML2XMLHelper(info->inName,
+ info->inFile,
+ info->outInactiveName,
+ info->outInactiveFile,
+ false);
+}
+

-if (virAsprintf(&xml_in, "%s/qemuxml2argvdata/qemuxml2argv-%s.xml",
-abs_srcdir, info->name) < 0 ||
-virAsprintf(&xml_out, "%s/qemuxml2xmloutdata/qemuxml2xmlout-%s.xml",
-abs_srcdir, info->name) < 0 ||
-virAsprintf(&xml_out_active,
-"%s/qemuxml2xmloutdata/qemuxml2xmlout-%s-active.xml",
-abs_srcdir, info->name) < 0 ||
-virAsprintf(&xml_out_inactive,
-"%s/qemuxml2xmloutdata/qemuxml2xmlout-%s-inactive.xml",
-abs_srcdir, info->name) < 0)
-goto cleanup;
-
-if ((info->when & WHEN_INACTIVE)) {
-char *out;
-if (!info->different)
-out = xml_in;
-else if (virFileExists(xml_out_inactive))
-out = xml_out_inactive;
-else
-out = xml_out;
-
-if (testCompareXMLToXMLFiles(xml_in, out, false) < 0)
-goto cleanup;
+static void
+testInfoFree(struct testInfo *info)
+{
+VIR_FREE(info->inName);
+VIR_FREE(info->inFile);
+
+VIR_FREE(info->outActiveName);
+VIR_FREE(info->outActiveFile);
+
+VIR_FREE(info->outInactiveName);
+VIR_FREE(info->outInactiveFile);
+}
+
+
+static int
+testInfoSet(struct testInfo *info,
+const char *name,
+bool different,
+int when)
+{
+if (virAsprintf(&info->inName, "%s/qemuxml2argvdata/qemuxml2argv-%s.xml",
+abs_srcdir, name) < 0)
+goto error;
+
+if (virtTestLoadFile(info->inName, &info->inFile) < 0)
+goto error;
+
+if (when & WHEN_INACTIVE) {
+if (different) {
+if (virAsprintf(&info->outInactiveName,
+   
"%s/qemuxml2xmloutdata/qemuxml2xmlout-%s-inactive.xml",
+   abs_srcdir, name) < 0)
+goto error;
+
+if (!virFileExists(info->outIn

Re: [libvirt] [PATCHv2 3/5] Allocate virtio-serial addresses when starting a domain

[John Ferlan]

On 03/24/2015 07:41 AM, Ján Tomko wrote:
> On Mon, Mar 23, 2015 at 05:46:23PM -0400, John Ferlan wrote:
>>

...

>>
>> Mostly for my edification... These examples previously although
>> indicating they were "auto-assign" (of sorts) really weren't - they were
>> more force-assign for each example.
> 
> Force-assign after this patch? Otherwise I don't understand.
> 

I started writing this before figuring out the second part and just
didn't go back re-read my original thought... But I think I was trying
to point out that the "existing" code doesn't really auto assign and
after your patch these changes are doing that, hence why set to
port='0'... I agree with not force-assign, but was just making sure...

>> The way to auto-assign is by setting port='0', correct?
>>
> 
> Yes.
> 
>> However, I'm still missing something from the *auto.args output. It
>> seems the controller='#' is ignored and I guess I don't understand
>> that...
> 
> I must've overlooked that.
> It shouldn't be a problem to take it as a hint in 
> virDomainVirtioSerialAddrAssign.
> 

Right so I figured out after I sent this, but figured you'd know that
anyway.

>> Sure "port='0'" (meaning first available port on the
>> controller), but I would have expected to see :
>>
>> kvm.port.0 use "virtio.serial.0.0,nr=1..." (which it does)
>> kvm.port.foo use "virtio.serial.1.0,nr=1..." (on controller 0?, but XML
>> has controller='1')
>> kvm.port.bar use "virtio.serial.1.0,nr=3..." (which it does)
>> kvm.port.wizz use "virtio.serial.0.0,nr=2" (incorrect due to others)
>> kvm.port.ooh use "virtio.serial.1.0,nr=2", (on controller 0?, but XML
>> has controller='1'
>> kvm.port.lla use "virtio.serial.2.0,nr=1" (on controller 0?, but XML has
>> controller='2')
>>
>> Now if been if "lla" used controller='0', then I assume would nr=4 be
>> chosen since 1,2 were auto-assigned, 3 was specifically assigned, thus 4
>> would be the next one.
>>
>> Continuing with that same logic, the *-autoassign example could have
>> shown that if controller='0',port='2' and 'controller='1',port='1' were
>> preassigned, then the controllers/ports assigned would be 0.0,nr=1,
>> 0.0,nr=3, 0.0,nr=4, 1.0,nr=2  (since only 4 ports on controller='0' can
>> be used w/ 2 be static and controller='1' having port '1' already in use).
>>
> 
> nr=4 is out of bounds for a controller with 4 ports.
> The ports are numbered from 0, but port number 0 can only be used for
> virtconsoles, not channels.
> 

Oh yeah - right...doh! Too much context switching sometimes causes loses
of brain to finger functions...

John

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

Re: [libvirt] [libvirt-glib] build-sys: Fix libtoolize detection in autogen.sh

[Christophe Fergeau]

On Tue, Mar 24, 2015 at 02:22:13PM +0100, Michal Privoznik wrote:
> On 24.03.2015 13:51, Christophe Fergeau wrote:
> > autogen.sh is currently checking for the libtool binary, but
> > it's libtoolize which is needed by autoreconf, not libtool.
> > 
> > These binaries are packaged separately on Debian/Ubuntu so this can
> > cause actual issues on some systems.
> > 
> > Bug reported by Frederic Peters
> > ---
> >  autogen.sh | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/autogen.sh b/autogen.sh
> > index 8030ab1..4f7135f 100755
> > --- a/autogen.sh
> > +++ b/autogen.sh
> > @@ -10,7 +10,7 @@ cd $srcdir
> >  
> >  DIE=0
> >  
> > -for prog in intltoolize autoreconf automake autoconf libtool
> > +for prog in intltoolize autoreconf automake autoconf libtoolize
> >  do
> >  ($prog --version) < /dev/null > /dev/null 2>&1 || {
> >  echo
> > 
> 
> ACK

Thanks, pushed.

Christophe


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

Re: [libvirt] [libvirt-glib] build-sys: Fix libtoolize detection in autogen.sh

[Michal Privoznik]

On 24.03.2015 13:51, Christophe Fergeau wrote:
> autogen.sh is currently checking for the libtool binary, but
> it's libtoolize which is needed by autoreconf, not libtool.
> 
> These binaries are packaged separately on Debian/Ubuntu so this can
> cause actual issues on some systems.
> 
> Bug reported by Frederic Peters
> ---
>  autogen.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/autogen.sh b/autogen.sh
> index 8030ab1..4f7135f 100755
> --- a/autogen.sh
> +++ b/autogen.sh
> @@ -10,7 +10,7 @@ cd $srcdir
>  
>  DIE=0
>  
> -for prog in intltoolize autoreconf automake autoconf libtool
> +for prog in intltoolize autoreconf automake autoconf libtoolize
>  do
>  ($prog --version) < /dev/null > /dev/null 2>&1 || {
>  echo
> 

ACK

Michal

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

[libvirt] [PATCH] qemu: Give hint about -noTSX CPU model

[Jiri Denemark]

Because of the microcode update to Haswell/Broadwell CPUs, existing
domains using these CPUs may fail to start even though they used to run
just fine. To help users solve this issue we try to suggest switching to
-noTSX variant of the CPU model:

virsh # start cd
error: Failed to start domain cd
error: unsupported configuration: guest and host CPU are not
compatible: Host CPU does not provide required features: rtm, hle;
try using 'Haswell-noTSX' CPU model

Signed-off-by: Jiri Denemark 
---

This patch depends on the previous patch which introduces the new
*-noTSX models.

 src/qemu/qemu_command.c | 38 +-
 1 file changed, 33 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 7e6b95c..cabb8b2 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6807,6 +6807,7 @@ qemuBuildCpuModelArgStr(virQEMUDriverPtr driver,
 size_t ncpus = 0;
 char **cpus = NULL;
 virCPUDataPtr data = NULL;
+virCPUDataPtr hostData = NULL;
 char *compare_msg = NULL;
 virCPUCompareResult cmp;
 const char *preferred;
@@ -6838,16 +6839,42 @@ qemuBuildCpuModelArgStr(virQEMUDriverPtr driver,
 
 /* For non-KVM, CPU features are emulated, so host compat doesn't matter */
 if (compareAgainstHost) {
+bool noTSX = false;
+
 cmp = cpuGuestData(host, cpu, &data, &compare_msg);
 switch (cmp) {
 case VIR_CPU_COMPARE_INCOMPATIBLE:
+if (cpuEncode(host->arch, host, NULL, &hostData,
+  NULL, NULL, NULL, NULL) == 0 &&
+(!cpuHasFeature(hostData, "hle") ||
+ !cpuHasFeature(hostData, "rtm")) &&
+(STREQ_NULLABLE(cpu->model, "Haswell") ||
+ STREQ_NULLABLE(cpu->model, "Broadwell")))
+noTSX = true;
+
 if (compare_msg) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("guest and host CPU are not compatible: %s"),
-   compare_msg);
+if (noTSX) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("guest and host CPU are not compatible: "
+ "%s; try using '%s-noTSX' CPU model"),
+   compare_msg, cpu->model);
+} else {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("guest and host CPU are not compatible: "
+ "%s"),
+   compare_msg);
+}
 } else {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("guest CPU is not compatible with host CPU"));
+if (noTSX) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("guest CPU is not compatible with host "
+ "CPU; try using '%s-noTSX' CPU model"),
+   cpu->model);
+} else {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("guest CPU is not compatible with host "
+ "CPU"));
+}
 }
 /* fall through */
 case VIR_CPU_COMPARE_ERROR:
@@ -6941,6 +6968,7 @@ qemuBuildCpuModelArgStr(virQEMUDriverPtr driver,
 virObjectUnref(caps);
 VIR_FREE(compare_msg);
 cpuDataFree(data);
+cpuDataFree(hostData);
 virCPUDefFree(guest);
 virCPUDefFree(cpu);
 return ret;
-- 
2.3.3

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

Re: [libvirt] [PATCH] conf: fix running vm numa settings disappear after restart libvirtd

[Peter Krempa]

On Mon, Mar 23, 2015 at 20:11:19 +0800, Luyao Huang wrote:
> 
> On 03/23/2015 06:10 PM, Peter Krempa wrote:
> > On Thu, Mar 19, 2015 at 18:13:04 +0800, Luyao Huang wrote:
> >> 5bba61f introduce a issue : when start a vm with  settings and
> >> restart libvirtd, numa settings will disappear. Because when parse the
> >> vm states file, there is no node in "/domain/cpu/numa" this place.
> >>
> >> Change to use ./cpu/numa instead of /domain/cpu/numa.
> >>
> >> Signed-off-by: Luyao Huang 
> >> ---
> >>   src/conf/numa_conf.c | 4 ++--
> >>   1 file changed, 2 insertions(+), 2 deletions(-)
> > Indeed the status XML wraps the   element with another container
> > element.
> 
> Yes, i just didn't know how to describe it in a right way when i wrote 
> this :)
> 
> >> diff --git a/src/conf/numa_conf.c b/src/conf/numa_conf.c
> >> index b92cb44..c34ba5f 100644
> >> --- a/src/conf/numa_conf.c
> >> +++ b/src/conf/numa_conf.c
> >> @@ -663,10 +663,10 @@ virDomainNumaDefCPUParseXML(virDomainNumaPtr def,
> >>   int ret = -1;
> >>   
> >>   /* check if NUMA definition is present */
> >> -if (!virXPathNode("/domain/cpu/numa[1]", ctxt))
> >> +if (!virXPathNode("./cpu/numa[1]", ctxt))
> >>   return 0;
> >>   
> >> -if ((n = virXPathNodeSet("/domain/cpu/numa[1]/cell", ctxt, &nodes)) 
> >> <= 0) {
> >> +if ((n = virXPathNodeSet("./cpu/numa[1]/cell", ctxt, &nodes)) <= 0) {
> >>   virReportError(VIR_ERR_XML_ERROR, "%s",
> >>  _("NUMA topology defined without NUMA cells"));
> >>   goto cleanup;
> >> -- 
> > ACK, I'll push your patch after I figure out how to add tests for
> > problems like this.

I've pushed this patch as I've written tests that don't work without it.
I'll submit the tests later today.

Peter


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

Re: [libvirt] [PATCH] RFC: Add domain vmport attribute

[Marc-André Lureau]

Hi

On Mon, Mar 23, 2015 at 11:42 PM, Daniel P. Berrange
 wrote:
> I think we'd normally place this kind of attribute in the
>  block rather than here. eg see the
> toggle for turning on/off hyper-v emulation.

Are you suggesting this: ? and if it's not
there it would use the default behaviour.


-- 
Marc-André Lureau

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

[libvirt] [libvirt-glib] build-sys: Fix libtoolize detection in autogen.sh

[Christophe Fergeau]

autogen.sh is currently checking for the libtool binary, but
it's libtoolize which is needed by autoreconf, not libtool.

These binaries are packaged separately on Debian/Ubuntu so this can
cause actual issues on some systems.

Bug reported by Frederic Peters
---
 autogen.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/autogen.sh b/autogen.sh
index 8030ab1..4f7135f 100755
--- a/autogen.sh
+++ b/autogen.sh
@@ -10,7 +10,7 @@ cd $srcdir
 
 DIE=0
 
-for prog in intltoolize autoreconf automake autoconf libtool
+for prog in intltoolize autoreconf automake autoconf libtoolize
 do
 ($prog --version) < /dev/null > /dev/null 2>&1 || {
 echo
-- 
2.3.3

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

Re: [libvirt] [PATCH] hostdev: fix two bugs in virHostdevReAttachPCIDevices

[John Ferlan]

On 03/22/2015 09:59 AM, Huanle Han wrote:
> 
> Bug 1: The the next element in the pcidevs is skipped after we
> virPCIDeviceListDel the previous element.
> 
> Bug 2: virHostdevNetConfigRestore is called for store the hostdevs
> which may be used by other domain.
> 
> Signed-off-by: Huanle Han mailto:hanxue...@gmail.com>>
> ---
>  src/util/virhostdev.c | 25 +
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
Should this be two separate patches? IOW: Are you fixing two separate
issues or essentially the same issue? Makes it easier to cherry-pick and
choose what may need to be backported elsewhere...

If there's a bug associated that would have been "good" to list as well
as perhaps an example or two...  That is - what XML and command(s)
sequence(s) prompted you to write the patch(es)...

Also, my attempt to git am -3 your patch fails - perhaps it's your
mailer configuration.  Did you use git send-email?  or something else...
I'm seeing html format... So I'm only reviewing based on what I read.

> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
> index 9678e2b..ecbe5d8 100644
> --- a/src/util/virhostdev.c
> +++ b/src/util/virhostdev.c
> @@ -785,7 +785,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr
> hostdev_mgr,
>   * them and reset all the devices before re-attach.
>   * Attach mac and port profile parameters to devices
>   */

^^^
I think the comment here could be updated to make it clear what's being
done. To just say "Again 4 loops;..." is a bit misleading since the only
loops I found in the code were in virHostdevPreparePCIDevices where
there are (currently) 9 such loops!

> -for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) {
> +for (i = 0; i < virPCIDeviceListCount(pcidevs);) {
>  virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i);
>  virPCIDevicePtr activeDev = NULL;
>  
> @@ -806,6 +806,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr
> hostdev_mgr,
>  }
>  
>  virPCIDeviceListDel(hostdev_mgr->activePCIHostdevs, dev);
> +i++;
>  }


Not sure I'm seeing why your fix works... Are you claiming that the
remove from other domain check (e.g. where the code "continue;"'s)
shouldn't be updating the 'i' value?  All it seems you did was move the
i++ from the for line to after the second virPCIDeviceListDel in the
block... Which yes, does reduce the count of devices and I would think
should be the case that doesn't increment i...

If we have 4 devices [0, 1, 2, 3] and we remove [1], then there are 3
devices left and i would be 2, but the new array would be [0, 2, 3] and
thus we don't check [2].

Perhaps this would work/read better as a while loop.

>  
>  /* At this point, any device that had been used by the guest is in
> @@ -816,9 +817,25 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr
> hostdev_mgr,
>   * For SRIOV net host devices, unset mac and port profile before
>   * reset and reattach device
>   */
> -for (i = 0; i < nhostdevs; i++)
> -virHostdevNetConfigRestore(hostdevs[i], hostdev_mgr->stateDir,
> -   oldStateDir);
> +for (i = 0; i < nhostdevs; i++) {
> +virPCIDevicePtr dev;
> +virDomainHostdevDefPtr hostdev = hostdevs[i];
> +virDomainHostdevSubsysPCIPtr pcisrc =
> &hostdev->source.subsys.u.pci;
> +
> +if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS ||
> +hostdev->source.subsys.type !=
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI ||
> +hostdev->parent.type != VIR_DOMAIN_DEVICE_NET ||
> +!hostdev->parent.data.net )
 
This is what makes me believe you sent in html format

It's also directly taken from virHostdevNetConfigRestore... and could be
considered partially from virHostdevPreparePCIDevices

Suggestion - create a patch that has a new static bool function
("isHostdevPCINetDevice") that does the same check passing "hostdev"...
Then adjust the (currently) two places that make that check in order to
use the bool function to decide to continue or not. Then this patch
would use that function here instead.

> +continue;
> +
> +dev = virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus,
> +  pcisrc->addr.slot, pcisrc->addr.function);
> +if (virPCIDeviceListFind(pcidevs, dev)) {
> +virHostdevNetConfigRestore(hostdev, hostdev_mgr->stateDir,
> +   oldStateDir);
> +}
> +virPCIDeviceFree(dev);
> +   }

It's still not quite clear to me from your description why we have to
search the pcidevs for our device before calling the Restore function.
Does it matter if it's in or not in the activePCIHostdevs list (the
comment just before this section).  I think this is one of those cases
where the code perhaps isn't self documenting and that by adding a few

[libvirt] [PATCH] cpu: Add {Haswell,Broadwell}-noTSX CPU models

[Jiri Denemark]

QEMU 2.3 adds these new models to cover Haswell and Broadwell CPUs with
updated microcode. Luckily, they also reverted former the machine type
specific changes to existing models. And since these changes were never
released, we don't need to hack around them in libvirt.

Signed-off-by: Jiri Denemark 
---
 src/cpu/cpu_map.xml|  18 ++-
 tests/cputest.c|  15 ++-
 tests/cputestdata/x86-Haswell-noTSX-nofallback.xml |   4 +
 tests/cputestdata/x86-Haswell-noTSX.xml|   4 +
 tests/cputestdata/x86-Haswell.xml  |   6 +
 tests/cputestdata/x86-baseline-7-result.xml|   4 +
 tests/cputestdata/x86-baseline-7.xml   |  24 
 tests/cputestdata/x86-baseline-8-result.xml|   4 +
 tests/cputestdata/x86-baseline-8.xml   |  28 +
 ...aswell-noTSX+Haswell,haswell,Haswell-result.xml |   6 +
 ...ll-noTSX+Haswell-noTSX,Haswell-noTSX-result.xml |   4 +
 ...+Haswell-noTSX,haswell,Haswell-noTSX-result.xml |   6 +
 tests/cputestdata/x86-host-Haswell-noTSX.xml   |   6 +
 .../qemuxml2argv-cpu-Haswell-noTSX.args|   4 +
 .../qemuxml2argv-cpu-Haswell-noTSX.xml |  21 
 .../qemuxml2argvdata/qemuxml2argv-cpu-Haswell.args |   4 +
 .../qemuxml2argvdata/qemuxml2argv-cpu-Haswell.xml  |  21 
 .../qemuxml2argv-cpu-Haswell2.args |   4 +
 .../qemuxml2argvdata/qemuxml2argv-cpu-Haswell2.xml |  23 
 .../qemuxml2argv-cpu-Haswell3.args |   4 +
 .../qemuxml2argvdata/qemuxml2argv-cpu-Haswell3.xml |  23 
 tests/qemuxml2argvtest.c   |   8 ++
 tests/testutilsqemu.c  | 124 +++--
 tests/testutilsqemu.h  |   6 +
 24 files changed, 331 insertions(+), 40 deletions(-)
 create mode 100644 tests/cputestdata/x86-Haswell-noTSX-nofallback.xml
 create mode 100644 tests/cputestdata/x86-Haswell-noTSX.xml
 create mode 100644 tests/cputestdata/x86-Haswell.xml
 create mode 100644 tests/cputestdata/x86-baseline-7-result.xml
 create mode 100644 tests/cputestdata/x86-baseline-7.xml
 create mode 100644 tests/cputestdata/x86-baseline-8-result.xml
 create mode 100644 tests/cputestdata/x86-baseline-8.xml
 create mode 100644 
tests/cputestdata/x86-host-Haswell-noTSX+Haswell,haswell,Haswell-result.xml
 create mode 100644 
tests/cputestdata/x86-host-Haswell-noTSX+Haswell-noTSX,Haswell-noTSX-result.xml
 create mode 100644 
tests/cputestdata/x86-host-Haswell-noTSX+Haswell-noTSX,haswell,Haswell-noTSX-result.xml
 create mode 100644 tests/cputestdata/x86-host-Haswell-noTSX.xml
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cpu-Haswell-noTSX.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cpu-Haswell-noTSX.xml
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cpu-Haswell.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cpu-Haswell.xml
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cpu-Haswell2.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cpu-Haswell2.xml
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cpu-Haswell3.args
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-cpu-Haswell3.xml

diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml
index 413148f..2110c0b 100644
--- a/src/cpu/cpu_map.xml
+++ b/src/cpu/cpu_map.xml
@@ -500,30 +500,40 @@
   
 
 
-
+
   
   
   
   
   
   
-  
   
   
   
   
   
+
+
+
+  
+  
   
 
 
-
-  
+
+  
   
   
   
   
 
 
+
+  
+  
+  
+
+
 
 
   
diff --git a/tests/cputest.c b/tests/cputest.c
index 449b7d1..bf7a48f 100644
--- a/tests/cputest.c
+++ b/tests/cputest.c
@@ -122,8 +122,10 @@ cpuTestLoadMultiXML(const char *arch,
 goto cleanup;
 
 n = virXPathNodeSet("/cpuTest/cpu", ctxt, &nodes);
-if (n <= 0 || (VIR_ALLOC_N(cpus, n) < 0))
+if (n <= 0 || (VIR_ALLOC_N(cpus, n) < 0)) {
+fprintf(stderr, "\nNo /cpuTest/cpu elements found in %s\n", xml);
 goto cleanup;
+}
 
 for (i = 0; i < n; i++) {
 ctxt->node = nodes[i];
@@ -497,6 +499,7 @@ cpuTestRun(const char *name, const struct data *data)
 static const char *model486[]   = { "486" };
 static const char *nomodel[]= { "nomodel" };
 static const char *models[] = { "qemu64", "core2duo", "Nehalem" };
+static const char *haswell[]= { "SandyBridge", "Haswell" };
 static const char *ppc_models[] = { "POWER7", "POWER7_v2.1", 
"POWER8_v1.0"};
 
 static int
@@ -618,6 +621,8 @@ mymain(void)
 DO_TEST_BASELINE("x86", "5", VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES, 0);
 DO_TEST_BASELINE("x86", "6", 0, 0);
 DO_TEST_BASELINE("x86", "6", VIR_CONNECT_BASELINE_CPU_MIGRATABLE, 0);
+DO_TEST_BASELINE("x86", "7", 0, 0);
+DO_TEST_BASELINE("x86", "8", 0, 0);
 
 DO_TEST_BASELINE("ppc64", "incompatible-vendors", 0, -1

Re: [libvirt] [PATCHv2 3/5] Allocate virtio-serial addresses when starting a domain

[Ján Tomko]

On Mon, Mar 23, 2015 at 05:46:23PM -0400, John Ferlan wrote:
> 
> 
> On 03/17/2015 07:41 AM, Ján Tomko wrote:
> > Instead of always using controller 0 and incrementing port number,
> > respect the maximum port numbers of controllers and use all of them.
> > 
> > Ports for virtio consoles are quietly reserved, but not formatted
> > (neither in XML nor on QEMU command line).
> > 
> > Also rejects duplicate virtio-serial addresses.
> > https://bugzilla.redhat.com/show_bug.cgi?id=890606
> > https://bugzilla.redhat.com/show_bug.cgi?id=1076708
> > ---
> >  src/conf/domain_conf.c | 29 --
> >  src/qemu/qemu_command.c| 63 
> > ++
> >  src/qemu/qemu_domain.c |  1 +
> >  src/qemu/qemu_domain.h |  1 +
> >  src/qemu/qemu_process.c|  2 +
> >  .../qemuxml2argv-channel-virtio-auto.args  |  8 +--
> >  .../qemuxml2argv-channel-virtio-autoassign.args| 10 ++--
> >  .../qemuxml2xmlout-channel-virtio-auto.xml | 10 ++--
> >  8 files changed, 81 insertions(+), 43 deletions(-)
> > 

> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 02105c3..e7f86e1 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -1399,6 +1399,65 @@ qemuAssignSpaprVIOAddress(virDomainDefPtr def, 
> > virDomainDeviceInfoPtr info,
> >  return 0;
> >  }
> >  
> > +
> > +static int
> > +qemuDomainAssignVirtioSerialAddresses(virDomainDefPtr def,
> > +  virDomainObjPtr obj)
> > +{
> > +int ret = -1;
> > +size_t i;
> > +virDomainVirtioSerialAddrSetPtr addrs = NULL;
> > +qemuDomainObjPrivatePtr priv = NULL;
> > +
> > +if (!(addrs = virDomainVirtioSerialAddrSetCreate()))
> > +goto cleanup;
> 
> Coverity determines addrs != NULL, but
> 
> > +
> > +if (virDomainVirtioSerialAddrSetAddControllers(addrs, def) < 0)
> > +goto cleanup;
> > +
> > +if (virDomainDeviceInfoIterate(def, virDomainVirtioSerialAddrReserve,
> > +   addrs) < 0)
> > +goto cleanup;
> > +
> > +VIR_DEBUG("Finished reserving existing ports");
> > +
> > +for (i = 0; i < def->nconsoles; i++) {
> > +virDomainChrDefPtr chr = def->consoles[i];
> > +if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE &&
> > +chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_VIRTIO) {
> > +if (virDomainVirtioSerialAddrAssign(addrs, &chr->info, true) < 
> > 0)
> > +goto cleanup;
> > +}
> > +}
> > +
> > +for (i = 0; i < def->nchannels; i++) {
> > +virDomainChrDefPtr chr = def->channels[i];
> > +if (chr->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL 
> > &&
> > +chr->info.addr.vioserial.port == 0 &&
> > +virDomainVirtioSerialAddrAssign(addrs, &chr->info, false) < 0)
> > +goto cleanup;
> > +}
> > +
> > +if (obj && obj->privateData) {
> > +priv = obj->privateData;
> > +if (addrs) {
> 
> There's a check here where the "else" is DEADCODE
> 

Right. The address set should only be generated if there is a
virtio-serial controller present.

> > +/* if this is the live domain object, we persist the addresses 
> > */
> > +virDomainVirtioSerialAddrSetFree(priv->vioserialaddrs);
> > +priv->persistentAddrs = 1;
> > +priv->vioserialaddrs = addrs;
> > +addrs = NULL;
> > +} else {
> > +priv->persistentAddrs = 0;
> > +}
> > +}
> > +ret = 0;

> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > index ae315df..5589f22 100644
> > --- a/src/qemu/qemu_process.c
> > +++ b/src/qemu/qemu_process.c
> > @@ -5205,6 +5205,8 @@ void qemuProcessStop(virQEMUDriverPtr driver,
> >  virDomainDefClearCCWAddresses(vm->def);
> >  virDomainCCWAddressSetFree(priv->ccwaddrs);
> >  priv->ccwaddrs = NULL;
> > +virDomainVirtioSerialAddrSetFree(priv->vioserialaddrs);
> > +priv->vioserialaddrs = NULL;
> >  }
> >  
> >  qemuDomainReAttachHostDevices(driver, vm->def);
> 
> Mostly for my edification... These examples previously although
> indicating they were "auto-assign" (of sorts) really weren't - they were
> more force-assign for each example.

Force-assign after this patch? Otherwise I don't understand.

> The way to auto-assign is by setting port='0', correct?
> 

Yes.

> However, I'm still missing something from the *auto.args output. It
> seems the controller='#' is ignored and I guess I don't understand
> that...

I must've overlooked that.
It shouldn't be a problem to take it as a hint in 
virDomainVirtioSerialAddrAssign.

> Sure "port='0'" (meaning first available port on the
> controller), but I would have expected to see :
> 
> kvm.port.0 use "virtio.serial.0.0,nr=1..." (whic

Re: [libvirt] GSOC 2015: Libvirt Projects

[Michal Privoznik]

On 24.03.2015 10:27, Stefan Hajnoczi wrote:
> On Fri, Mar 13, 2015 at 12:03 AM, Richa Sehgal
>  wrote:
>> I am currently pursuing Master's in University of Illinois at Urbana
>> Champaign, USA and completed by B.Tech from Indian Institute of Technology -
>> Delhi (IIT- Delhi).
>>
>> I am interested in contributing to Libvirt community through GSOC 2015. I am
>> using VirtualBox to run Ubuntu and have used VMware Player in the past.
>> Actually I am very interested in understanding virtualization at a more
>> practical level and do not want to limit myself to theoretical knowledge. I
>> have played with QEMU during the OS class, but I am new to Libvirt, and thus
>> would like to start with beginner level projects. There are two that I
>> found:
>>
>> 1. Enhancing libvirt-designer
>> 2. Abstracting device address allocation
>>
>> Are these projects still open, or students have already applied to them? Are
>> there any other projects that I can start with? I am really excited about
>> contributing to the libvirt project. Please let me know of a suitable way of
>> starting with these. I look forward for a useful engagement with the
>> community this summer. My irc name is: richashi.
> 
> Hi,
> I have CCed the libvirt GSoC mentors in case you haven't contacted
> them directly yet.

She indeed has. We have an ongoing discussion.

Michal

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

[libvirt] [PATCH 3/7] conf: Add/modify storage formatting functions

[Erik Skultety]

This patch introduces virStoragePoolSaveStatus to properly format the
status XML. It also adds virStoragePoolDefFormatBuf function, which
alike in the network driver, formats the whole storage pool definition into
a buffer that might have been previously modified by
virStoragePoolSaveStatus to insert  element. The original
virStoragePoolDefFormat function had to be modified accordingly to use
virBuffer.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733
---
 src/conf/storage_conf.c  | 139 ++-
 src/conf/storage_conf.h  |   6 +-
 src/libvirt_private.syms |   1 +
 3 files changed, 107 insertions(+), 39 deletions(-)

diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index b070448..5d984f3 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -1150,76 +1150,90 @@ virStoragePoolSourceFormat(virBufferPtr buf,
 }
 
 
-char *
-virStoragePoolDefFormat(virStoragePoolDefPtr def)
+int
+virStoragePoolDefFormatBuf(virBufferPtr buf,
+   virStoragePoolDefPtr def)
 {
 virStoragePoolOptionsPtr options;
-virBuffer buf = VIR_BUFFER_INITIALIZER;
-const char *type;
 char uuid[VIR_UUID_STRING_BUFLEN];
+const char *type;
 
 options = virStoragePoolOptionsForPoolType(def->type);
 if (options == NULL)
-return NULL;
+goto error;
 
 type = virStoragePoolTypeToString(def->type);
 if (!type) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("unexpected pool type"));
-goto cleanup;
+goto error;
 }
-virBufferAsprintf(&buf, "\n", type);
-virBufferAdjustIndent(&buf, 2);
-virBufferEscapeString(&buf, "%s\n", def->name);
+virBufferAsprintf(buf, "\n", type);
+virBufferAdjustIndent(buf, 2);
+virBufferEscapeString(buf, "%s\n", def->name);
 
 virUUIDFormat(def->uuid, uuid);
-virBufferAsprintf(&buf, "%s\n", uuid);
+virBufferAsprintf(buf, "%s\n", uuid);
 
-virBufferAsprintf(&buf, "%llu\n",
+virBufferAsprintf(buf, "%llu\n",
   def->capacity);
-virBufferAsprintf(&buf, "%llu\n",
+virBufferAsprintf(buf, "%llu\n",
   def->allocation);
-virBufferAsprintf(&buf, "%llu\n",
+virBufferAsprintf(buf, "%llu\n",
   def->available);
 
-if (virStoragePoolSourceFormat(&buf, options, &def->source) < 0)
-goto cleanup;
+if (virStoragePoolSourceFormat(buf, options, &def->source) < 0)
+goto error;
 
 /* RBD, Sheepdog, and Gluster devices are not local block devs nor
  * files, so they don't have a target */
 if (def->type != VIR_STORAGE_POOL_RBD &&
 def->type != VIR_STORAGE_POOL_SHEEPDOG &&
 def->type != VIR_STORAGE_POOL_GLUSTER) {
-virBufferAddLit(&buf, "\n");
-virBufferAdjustIndent(&buf, 2);
+virBufferAddLit(buf, "\n");
+virBufferAdjustIndent(buf, 2);
 
-virBufferEscapeString(&buf, "%s\n", def->target.path);
+virBufferEscapeString(buf, "%s\n", def->target.path);
 
-virBufferAddLit(&buf, "\n");
-virBufferAdjustIndent(&buf, 2);
-virBufferAsprintf(&buf, "0%o\n",
+virBufferAddLit(buf, "\n");
+virBufferAdjustIndent(buf, 2);
+virBufferAsprintf(buf, "0%o\n",
   def->target.perms.mode);
-virBufferAsprintf(&buf, "%d\n",
+virBufferAsprintf(buf, "%d\n",
   (int) def->target.perms.uid);
-virBufferAsprintf(&buf, "%d\n",
+virBufferAsprintf(buf, "%d\n",
   (int) def->target.perms.gid);
-virBufferEscapeString(&buf, "%s\n",
+virBufferEscapeString(buf, "%s\n",
   def->target.perms.label);
 
-virBufferAdjustIndent(&buf, -2);
-virBufferAddLit(&buf, "\n");
-virBufferAdjustIndent(&buf, -2);
-virBufferAddLit(&buf, "\n");
+virBufferAdjustIndent(buf, -2);
+virBufferAddLit(buf, "\n");
+virBufferAdjustIndent(buf, -2);
+virBufferAddLit(buf, "\n");
 }
-virBufferAdjustIndent(&buf, -2);
-virBufferAddLit(&buf, "\n");
+virBufferAdjustIndent(buf, -2);
+virBufferAddLit(buf, "\n");
+
+return 0;
+
+ error:
+return -1;
+}
+
+char *
+virStoragePoolDefFormat(virStoragePoolDefPtr def)
+{
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+
+if (virStoragePoolDefFormatBuf(&buf, def) < 0)
+goto error;
 
 if (virBufferCheckError(&buf) < 0)
-goto cleanup;
+goto error;
 
 return virBufferContentAndReset(&buf);
 
- cleanup:
+ error:
 virBufferFreeAndReset(&buf);
 return NULL;
 }
@@ -1899,11 +1913,60 @@ virStoragePoolLoadAllConfigs(virStoragePoolObjListPtr 
pools,
 return ret;
 }
 
+
+static int virStoragePoolSaveXML(const char *configFile,
+ virStoragePoolDefPtr def,
+ const char *xml)
+{
+char uuidst

[libvirt] [PATCH 6/7] storage: Introduce storagePoolUpdateAllState function

[Erik Skultety]

The update was originally part of the storageDriverAutostart function,
but the pools have to be checked earlier during initialization phase, so
the 'checkPool' block has been moved to storagePoolUpdateAllState.
Prior to this update virStoragePoolLoadAllConfigs and
virStoragePoolLoadAllState functions gather all existing pool in the
system, so first it is necessary to filter out the ones that are
inactive (only config XML found), then determine storage backends for
the rest of the pools and run checkPool on each one of them to update
their 'active' property.

After update, pools have to be refreshed, otherwise the list of volumes
stays empty. Once again we need the connection, but all the
storage backends ignore this argument except for RBD, however RBD
doesn't support 'checkPool' callback, therefore it is safe to pass
connection as NULL pointer.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733
---
 src/storage/storage_driver.c | 73 
 1 file changed, 61 insertions(+), 12 deletions(-)

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index d09acce..2899521 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -75,6 +75,64 @@ static void storageDriverUnlock(void)
 }
 
 static void
+storagePoolUpdateAllState(void)
+{
+size_t i;
+bool active = false;
+
+for (i = 0; i < driver->pools.count; i++) {
+virStoragePoolObjPtr pool = driver->pools.objs[i];
+virStorageBackendPtr backend;
+
+virStoragePoolObjLock(pool);
+if (!virStoragePoolObjIsActive(pool)) {
+virStoragePoolObjUnlock(pool);
+continue;
+}
+
+if ((backend = virStorageBackendForType(pool->def->type)) == NULL) {
+VIR_ERROR(_("Missing backend %d"), pool->def->type);
+virStoragePoolObjUnlock(pool);
+continue;
+}
+
+/* Backends which do not support 'checkPool' are considered
+ * inactive by default.
+ */
+if (backend->checkPool &&
+backend->checkPool(pool, &active) < 0) {
+virErrorPtr err = virGetLastError();
+VIR_ERROR(_("Failed to initialize storage pool '%s': %s"),
+  pool->def->name, err ? err->message :
+  _("no error message found"));
+virStoragePoolObjUnlock(pool);
+continue;
+}
+
+/* We can pass NULL as connection, most backends do not use
+ * it anyway, but if they do and fail, we want to log error and
+ * continue with other pools.
+ */
+if (active) {
+virStoragePoolObjClearVols(pool);
+if (backend->refreshPool(NULL, pool) < 0) {
+virErrorPtr err = virGetLastError();
+if (backend->stopPool)
+backend->stopPool(NULL, pool);
+VIR_ERROR(_("Failed to restart storage pool '%s': %s"),
+  pool->def->name, err ? err->message :
+  _("no error message found"));
+virStoragePoolObjUnlock(pool);
+continue;
+}
+}
+
+pool->active = active;
+virStoragePoolObjUnlock(pool);
+}
+}
+
+static void
 storageDriverAutostart(void)
 {
 size_t i;
@@ -99,18 +157,7 @@ storageDriverAutostart(void)
 continue;
 }
 
-if (backend->checkPool &&
-backend->checkPool(pool, &started) < 0) {
-virErrorPtr err = virGetLastError();
-VIR_ERROR(_("Failed to initialize storage pool '%s': %s"),
-  pool->def->name, err ? err->message :
-  _("no error message found"));
-virStoragePoolObjUnlock(pool);
-continue;
-}
-
-if (!started &&
-pool->autostart &&
+if (pool->autostart &&
 !virStoragePoolObjIsActive(pool)) {
 if (backend->startPool &&
 backend->startPool(conn, pool) < 0) {
@@ -207,6 +254,8 @@ storageStateInitialize(bool privileged,
  driver->autostartDir) < 0)
 goto error;
 
+storagePoolUpdateAllState();
+
 storageDriverUnlock();
 
 ret = 0;
-- 
1.9.3

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

[libvirt] [PATCH 4/7] storage: Modify stateInitialize to support storage state XML

[Erik Skultety]

Storage state driver directories initialization needs to be modified
to become more generic.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733
---
 src/storage/storage_driver.c | 52 
 1 file changed, 29 insertions(+), 23 deletions(-)

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index e088ffa..9bd93d2 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -154,54 +154,60 @@ storageStateInitialize(bool privileged,
virStateInhibitCallback callback ATTRIBUTE_UNUSED,
void *opaque ATTRIBUTE_UNUSED)
 {
-char *base = NULL;
+int ret = -1;
+char *configdir = NULL;
+char *rundir = NULL;
 
 if (VIR_ALLOC(driver) < 0)
-return -1;
+return ret;
 
 if (virMutexInit(&driver->lock) < 0) {
 VIR_FREE(driver);
-return -1;
+return ret;
 }
 storageDriverLock();
 
 if (privileged) {
-if (VIR_STRDUP(base, SYSCONFDIR "/libvirt") < 0)
+if (VIR_STRDUP(driver->configDir,
+   SYSCONFDIR "/libvirt/storage") < 0 ||
+VIR_STRDUP(driver->autostartDir,
+   SYSCONFDIR "/libvirt/storage/autostart") < 0 ||
+VIR_STRDUP(driver->stateDir,
+   LOCALSTATEDIR "/run/libvirt/storage") < 0)
 goto error;
 } else {
-base = virGetUserConfigDirectory();
-if (!base)
+configdir = virGetUserConfigDirectory();
+rundir = virGetUserRuntimeDirectory();
+if (!(configdir && rundir))
+goto error;
+
+if ((virAsprintf(&driver->configDir,
+"%s/storage", configdir) < 0) ||
+(virAsprintf(&driver->autostartDir,
+"%s/storage", configdir) < 0) ||
+(virAsprintf(&driver->stateDir,
+ "%s/storage/run", rundir) < 0))
 goto error;
 }
 driver->privileged = privileged;
 
-/*
- * Configuration paths are either $USER_CONFIG_HOME/libvirt/storage/...
- * (session) or /etc/libvirt/storage/... (system).
- */
-if (virAsprintf(&driver->configDir,
-"%s/storage", base) == -1)
-goto error;
-
-if (virAsprintf(&driver->autostartDir,
-"%s/storage/autostart", base) == -1)
-goto error;
-
-VIR_FREE(base);
-
 if (virStoragePoolLoadAllConfigs(&driver->pools,
  driver->configDir,
  driver->autostartDir) < 0)
 goto error;
 
 storageDriverUnlock();
-return 0;
+
+ret = 0;
+ cleanup:
+VIR_FREE(configdir);
+VIR_FREE(rundir);
+return ret;
 
  error:
-VIR_FREE(base);
 storageDriverUnlock();
 storageStateCleanup();
-return -1;
+goto cleanup;
 }
 
 /**
-- 
1.9.3

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

[libvirt] [PATCH 5/7] conf: Introduce virStoragePoolLoadAllState && virStoragePoolLoadState

[Erik Skultety]

These functions operate exactly the same as
virStoragePoolLoadAllConfigs.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733
---
 src/conf/storage_conf.c  | 90 
 src/conf/storage_conf.h  |  7 
 src/libvirt_private.syms |  1 +
 src/storage/storage_driver.c | 11 ++
 4 files changed, 109 insertions(+)

diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 5d984f3..b158e30 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -1863,6 +1863,96 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools,
 }
 
 
+virStoragePoolObjPtr
+virStoragePoolLoadState(virStoragePoolObjListPtr pools,
+const char *stateDir,
+const char *name)
+{
+char *stateFile = NULL;
+virStoragePoolDefPtr def = NULL;
+virStoragePoolObjPtr pool = NULL;
+xmlDocPtr xml = NULL;
+xmlXPathContextPtr ctxt = NULL;
+xmlNodePtr node = NULL;
+
+if (!(stateFile = virFileBuildPath(stateDir, name, ".xml")))
+goto cleanup;
+
+if (!(xml = virXMLParseCtxt(stateFile, NULL, _("(pool status)"), &ctxt)))
+goto cleanup;
+
+if (!(node = virXPathNode("//pool", ctxt))) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Could not find any 'pool' element in status file"));
+goto cleanup;
+}
+
+ctxt->node = node;
+if (!(def = virStoragePoolDefParseXML(ctxt)))
+goto cleanup;
+
+if (!STREQ(name, def->name)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Storage pool status file '%s' does not match "
+ "pool name '%s'"),
+   stateFile, def->name);
+goto cleanup;
+}
+
+/* create the object */
+if (!(pool = virStoragePoolObjAssignDef(pools, def)))
+goto cleanup;
+
+/* XXX: future handling of some additional usefull status data,
+ * for now, if a status file for a pool exists, the pool will be marked
+ * as active
+ */
+
+pool->active = 1;
+
+ cleanup:
+VIR_FREE(stateFile);
+xmlFree(xml);
+xmlXPathFreeContext(ctxt);
+return pool;
+}
+
+
+int
+virStoragePoolLoadAllState(virStoragePoolObjListPtr pools,
+   const char *stateDir)
+{
+DIR *dir;
+struct dirent *entry;
+int ret = -1;
+
+if (!(dir = opendir(stateDir))) {
+if (errno == ENOENT)
+return 0;
+
+virReportSystemError(errno, _("Failed to open dir '%s'"), stateDir);
+return -1;
+}
+
+while ((ret = virDirRead(dir, &entry, stateDir)) > 0) {
+virStoragePoolObjPtr pool;
+
+if (entry->d_name[0] == '.')
+continue;
+
+if (!virFileStripSuffix(entry->d_name, ".xml"))
+continue;
+
+if (!(pool = virStoragePoolLoadState(pools, stateDir, entry->d_name)))
+continue;
+virStoragePoolObjUnlock(pool);
+}
+
+closedir(dir);
+return ret;
+}
+
+
 int
 virStoragePoolLoadAllConfigs(virStoragePoolObjListPtr pools,
  const char *configDir,
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h
index 99b2f4a..1f84504 100644
--- a/src/conf/storage_conf.h
+++ b/src/conf/storage_conf.h
@@ -318,6 +318,13 @@ int virStoragePoolLoadAllConfigs(virStoragePoolObjListPtr 
pools,
  const char *configDir,
  const char *autostartDir);
 
+int virStoragePoolLoadAllState(virStoragePoolObjListPtr pools,
+   const char *stateDir);
+
+virStoragePoolObjPtr
+virStoragePoolLoadState(virStoragePoolObjListPtr pools,
+const char *stateDir,
+const char *name);
 virStoragePoolObjPtr
 virStoragePoolObjFindByUUID(virStoragePoolObjListPtr pools,
 const unsigned char *uuid);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 689a08f..9bc8de8 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -798,6 +798,7 @@ virStoragePoolFormatFileSystemNetTypeToString;
 virStoragePoolFormatFileSystemTypeToString;
 virStoragePoolGetVhbaSCSIHostParent;
 virStoragePoolLoadAllConfigs;
+virStoragePoolLoadAllState;
 virStoragePoolObjAssignDef;
 virStoragePoolObjClearVols;
 virStoragePoolObjDeleteDef;
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 9bd93d2..d09acce 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -191,6 +191,17 @@ storageStateInitialize(bool privileged,
 }
 driver->privileged = privileged;
 
+if (virFileMakePath(driver->stateDir) < 0) {
+virReportError(errno,
+   _("cannot create directory %s"),
+   driver->stateDir);
+goto error;
+}
+
+if (virStoragePoolLoadAllState(&driver->pools,
+   driver->stateDir) < 0)
+ 

[libvirt] [PATCH 0/7] Introduce storage pool status XML

[Erik Skultety]

All of the commits resolve BZ 
https://bugzilla.redhat.com/show_bug.cgi?id=1177733

Erik Skultety (7):
  storage: Remove unused attribute conn from 'checkPool' callback
  conf: Add support for storage state directory
  conf: Add/modify storage formatting functions
  storage: Modify stateInitialize to support storage state XML
  conf: Introduce virStoragePoolLoadAllState && virStoragePoolLoadState
  storage: Introduce storagePoolUpdateAllState function
  storage: Create/Delete pool status XML

 src/conf/storage_conf.c   | 229 --
 src/conf/storage_conf.h   |  14 ++-
 src/libvirt_private.syms  |   2 +
 src/storage/storage_backend.h |   3 +-
 src/storage/storage_backend_fs.c  |   3 +-
 src/storage/storage_backend_iscsi.c   |   3 +-
 src/storage/storage_backend_logical.c |   3 +-
 src/storage/storage_backend_mpath.c   |   3 +-
 src/storage/storage_backend_scsi.c|   3 +-
 src/storage/storage_backend_zfs.c |   3 +-
 src/storage/storage_driver.c  | 166 +++-
 11 files changed, 348 insertions(+), 84 deletions(-)

-- 
1.9.3

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

[libvirt] [PATCH 7/7] storage: Create/Delete pool status XML

[Erik Skultety]

Once we introduced virStoragePoolSaveStatus function, create a status
XML every time a pool is created (virStoragePoolCreate,
  storageDriverAutostart)

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733
---
 src/storage/storage_driver.c | 37 +
 1 file changed, 37 insertions(+)

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 2899521..4ce3d34 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -136,6 +136,7 @@ static void
 storageDriverAutostart(void)
 {
 size_t i;
+char *stateFile = NULL;
 virConnectPtr conn = NULL;
 
 /* XXX Remove hardcoding of QEMU URI */
@@ -183,6 +184,12 @@ storageDriverAutostart(void)
 virStoragePoolObjUnlock(pool);
 continue;
 }
+
+if (!(stateFile = virFileBuildPath(driver->stateDir,
+   pool->def->name, ".xml")))
+continue;
+
+ignore_value(virStoragePoolSaveStatus(stateFile, pool->def));
 pool->active = 1;
 }
 virStoragePoolObjUnlock(pool);
@@ -812,6 +819,7 @@ storagePoolCreate(virStoragePoolPtr obj,
 virStoragePoolObjPtr pool;
 virStorageBackendPtr backend;
 int ret = -1;
+char *stateFile = NULL;
 
 virCheckFlags(0, -1);
 
@@ -840,11 +848,21 @@ storagePoolCreate(virStoragePoolPtr obj,
 goto cleanup;
 }
 
+/* save pool state */
+if (!(stateFile = virFileBuildPath(driver->stateDir,
+   pool->def->name, ".xml")))
+goto cleanup;
+
+if ((ret = virStoragePoolSaveStatus(stateFile,
+pool->def)) < 0)
+goto cleanup;
+
 VIR_INFO("Starting up storage pool '%s'", pool->def->name);
 pool->active = 1;
 ret = 0;
 
  cleanup:
+VIR_FREE(stateFile);
 virStoragePoolObjUnlock(pool);
 return ret;
 }
@@ -889,6 +907,7 @@ storagePoolDestroy(virStoragePoolPtr obj)
 {
 virStoragePoolObjPtr pool;
 virStorageBackendPtr backend;
+char *stateFile = NULL;
 int ret = -1;
 
 storageDriverLock();
@@ -937,6 +956,15 @@ storagePoolDestroy(virStoragePoolPtr obj)
 pool->def = pool->newDef;
 pool->newDef = NULL;
 }
+
+if (!(stateFile = virFileBuildPath(driver->stateDir,
+  pool->def->name,
+  ".xml")))
+goto cleanup;
+
+unlink(stateFile);
+VIR_FREE(stateFile);
+
 ret = 0;
 
  cleanup:
@@ -952,6 +980,7 @@ storagePoolDelete(virStoragePoolPtr obj,
 {
 virStoragePoolObjPtr pool;
 virStorageBackendPtr backend;
+char *stateFile = NULL;
 int ret = -1;
 
 if (!(pool = virStoragePoolObjFromStoragePool(obj)))
@@ -985,6 +1014,14 @@ storagePoolDelete(virStoragePoolPtr obj,
 if (backend->deletePool(obj->conn, pool, flags) < 0)
 goto cleanup;
 VIR_INFO("Deleting storage pool '%s'", pool->def->name);
+
+if (!(stateFile = virFileBuildPath(driver->stateDir,
+  pool->def->name,
+  ".xml")))
+goto cleanup;
+
+unlink(stateFile);
+VIR_FREE(stateFile);
 ret = 0;
 
  cleanup:
-- 
1.9.3

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

[libvirt] [PATCH] conf: refact virDomainHasDiskMirror and rename it to virDomainHasBlockjob

[Shanzhi Yu]

Create external snapshot or migrate a vm when there is a blockpull
job running should be forbidden by libvirt, otherwise qemu try to
create external snapshot and failed with error "unable to execute
QEMU command 'transaction': Device 'drive-virtio-disk0' is busy:
block device is in use by block job: stream", and migration will
succeed which will lead the blockpull job failed.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1203628
Signed-off-by: Shanzhi Yu 
---
 src/conf/domain_conf.c| 6 +++---
 src/conf/domain_conf.h| 2 +-
 src/libvirt_private.syms  | 2 +-
 src/qemu/qemu_driver.c| 6 +++---
 src/qemu/qemu_migration.c | 2 +-
 5 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d633f93..24445af 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -12264,13 +12264,13 @@ virDomainDiskRemoveByName(virDomainDefPtr def, const 
char *name)
 }
 
 /* Return true if VM has at least one disk involved in a current block
- * copy/commit job (that is, with a  element in the disk xml).  */
+ * copy/commit/pull job */
 bool
-virDomainHasDiskMirror(virDomainObjPtr vm)
+virDomainHasBlockjob(virDomainObjPtr vm)
 {
 size_t i;
 for (i = 0; i < vm->def->ndisks; i++)
-if (vm->def->disks[i]->mirror)
+if (vm->def->disks[i]->blockjob)
 return true;
 return false;
 }
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index bceb2d7..32674f3 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2645,7 +2645,7 @@ int virDomainDiskSourceParse(xmlNodePtr node,
  xmlXPathContextPtr ctxt,
  virStorageSourcePtr src);
 
-bool virDomainHasDiskMirror(virDomainObjPtr vm);
+bool virDomainHasBlockjob(virDomainObjPtr vm);
 
 int virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net);
 virDomainNetDefPtr virDomainNetFind(virDomainDefPtr def, const char *device);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 33222f0..9ebaf4a 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -297,7 +297,7 @@ virDomainGraphicsTypeFromString;
 virDomainGraphicsTypeToString;
 virDomainGraphicsVNCSharePolicyTypeFromString;
 virDomainGraphicsVNCSharePolicyTypeToString;
-virDomainHasDiskMirror;
+virDomainHasBlockjob;
 virDomainHasNet;
 virDomainHostdevCapsTypeToString;
 virDomainHostdevDefAlloc;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 80a3d77..51e302f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7398,7 +7398,7 @@ static virDomainPtr 
qemuDomainDefineXMLFlags(virConnectPtr conn, const char *xml
 
 virObjectRef(vm);
 def = NULL;
-if (virDomainHasDiskMirror(vm)) {
+if (virDomainHasBlockjob(vm)) {
 virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, "%s",
_("domain has active block job"));
 virDomainObjAssignDef(vm, NULL, false, NULL);
@@ -14583,7 +14583,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
"%s", _("domain is marked for auto destroy"));
 goto cleanup;
 }
-if (virDomainHasDiskMirror(vm)) {
+if (virDomainHasBlockjob(vm)) {
 virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, "%s",
_("domain has active block job"));
 goto cleanup;
@@ -15245,7 +15245,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr 
snapshot,
 if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
 goto cleanup;
 
-if (virDomainHasDiskMirror(vm)) {
+if (virDomainHasBlockjob(vm)) {
 virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, "%s",
_("domain has active block job"));
 goto cleanup;
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index d34bb02..27a76ec 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1977,7 +1977,7 @@ qemuMigrationIsAllowed(virQEMUDriverPtr driver, 
virDomainObjPtr vm,
 
 }
 
-if (virDomainHasDiskMirror(vm)) {
+if (virDomainHasBlockjob(vm)) {
 virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("domain has an active block job"));
 return false;
-- 
2.1.0

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

[libvirt] [PATCH 1/7] storage: Remove unused attribute conn from 'checkPool' callback

[Erik Skultety]

The checkPool callback should be used when updating states of all pools
during storageStateInitialize, not storageDriverAutostart, otherwise we
can't start a domain which mounts a volume after daemons restarted. This
is because qemuProcessReconnect is called earlier than
storageDriverAutostart which marks the pool as active, so that the
domain can mount a volume from this pool successfully.
In order to fix this, conn attribute has to be discarded from the
callback, because storageStateInitialize doesn't have a connection yet.
(it's not used anyway)

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733
---
 src/storage/storage_backend.h | 3 +--
 src/storage/storage_backend_fs.c  | 3 +--
 src/storage/storage_backend_iscsi.c   | 3 +--
 src/storage/storage_backend_logical.c | 3 +--
 src/storage/storage_backend_mpath.c   | 3 +--
 src/storage/storage_backend_scsi.c| 3 +--
 src/storage/storage_backend_zfs.c | 3 +--
 src/storage/storage_driver.c  | 2 +-
 8 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h
index 9f1db60..fd2451c 100644
--- a/src/storage/storage_backend.h
+++ b/src/storage/storage_backend.h
@@ -34,8 +34,7 @@
 typedef char * (*virStorageBackendFindPoolSources)(virConnectPtr conn,
const char *srcSpec,
unsigned int flags);
-typedef int (*virStorageBackendCheckPool)(virConnectPtr conn,
-  virStoragePoolObjPtr pool,
+typedef int (*virStorageBackendCheckPool)(virStoragePoolObjPtr pool,
   bool *active);
 typedef int (*virStorageBackendStartPool)(virConnectPtr conn,
   virStoragePoolObjPtr pool);
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index 35385db..d4d65bc 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -533,8 +533,7 @@ virStorageBackendFileSystemUnmount(virStoragePoolObjPtr 
pool)
 
 
 static int
-virStorageBackendFileSystemCheck(virConnectPtr conn ATTRIBUTE_UNUSED,
- virStoragePoolObjPtr pool,
+virStorageBackendFileSystemCheck(virStoragePoolObjPtr pool,
  bool *isActive)
 {
 if (pool->def->type == VIR_STORAGE_POOL_DIR) {
diff --git a/src/storage/storage_backend_iscsi.c 
b/src/storage/storage_backend_iscsi.c
index 079c767..497a71b 100644
--- a/src/storage/storage_backend_iscsi.c
+++ b/src/storage/storage_backend_iscsi.c
@@ -236,8 +236,7 @@ virStorageBackendISCSIFindPoolSources(virConnectPtr conn 
ATTRIBUTE_UNUSED,
 }
 
 static int
-virStorageBackendISCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED,
-virStoragePoolObjPtr pool,
+virStorageBackendISCSICheckPool(virStoragePoolObjPtr pool,
 bool *isActive)
 {
 char *session = NULL;
diff --git a/src/storage/storage_backend_logical.c 
b/src/storage/storage_backend_logical.c
index 7ba8ded..11c5683 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -479,8 +479,7 @@ virStorageBackendLogicalFindPoolSources(virConnectPtr conn 
ATTRIBUTE_UNUSED,
 
 
 static int
-virStorageBackendLogicalCheckPool(virConnectPtr conn ATTRIBUTE_UNUSED,
-  virStoragePoolObjPtr pool,
+virStorageBackendLogicalCheckPool(virStoragePoolObjPtr pool,
   bool *isActive)
 {
 *isActive = virFileExists(pool->def->target.path);
diff --git a/src/storage/storage_backend_mpath.c 
b/src/storage/storage_backend_mpath.c
index 44bcd60..971408a 100644
--- a/src/storage/storage_backend_mpath.c
+++ b/src/storage/storage_backend_mpath.c
@@ -245,8 +245,7 @@ virStorageBackendGetMaps(virStoragePoolObjPtr pool)
 }
 
 static int
-virStorageBackendMpathCheckPool(virConnectPtr conn ATTRIBUTE_UNUSED,
-virStoragePoolObjPtr pool ATTRIBUTE_UNUSED,
+virStorageBackendMpathCheckPool(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED,
 bool *isActive)
 {
 *isActive = virFileExists("/dev/mpath");
diff --git a/src/storage/storage_backend_scsi.c 
b/src/storage/storage_backend_scsi.c
index 58e7e6d..66e0846 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -844,8 +844,7 @@ deleteVport(virConnectPtr conn,
 
 
 static int
-virStorageBackendSCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED,
-   virStoragePoolObjPtr pool,
+virStorageBackendSCSICheckPool(virStoragePoolObjPtr pool,
bool *isActive)
 {
 char *path = NULL;
diff --git a/src/storage/storage_backend_zfs.c 
b/src/storage/storage_backend_zfs.c
index 9482706..cb2662a 100644
--- a/src/storage/storage_backend_zfs.c
+++ b/src/storage/storage_backend_zfs.c
@@ -41,

[libvirt] [PATCH 2/7] conf: Add support for storage state directory

[Erik Skultety]

Before introducing necessary changes to storage_driver.c, first prepare
our structures for storage state XML support.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733
---
 src/conf/storage_conf.h  | 1 +
 src/storage/storage_driver.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h
index 4584075..8ccc947 100644
--- a/src/conf/storage_conf.h
+++ b/src/conf/storage_conf.h
@@ -293,6 +293,7 @@ struct _virStorageDriverState {
 
 char *configDir;
 char *autostartDir;
+char *stateDir;
 bool privileged;
 };
 
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 64ea770..e088ffa 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -261,6 +261,7 @@ storageStateCleanup(void)
 
 VIR_FREE(driver->configDir);
 VIR_FREE(driver->autostartDir);
+VIR_FREE(driver->stateDir);
 storageDriverUnlock();
 virMutexDestroy(&driver->lock);
 VIR_FREE(driver);
-- 
1.9.3

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

[libvirt] [PATCH v3 2/2] network_conf: check if bridge exists on host for user created bridges

[Shivaprasad G Bhat]

virNetworkBridgeInUse() doesn't check if the bridge is manually created
outside of libvirt. Check if the bridge actually exist on host using the
virNetDevExists().

Signed-off-by: Shivaprasad G Bhat 
---
 src/conf/network_conf.c |   15 ---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index d7c5dec..c3ae2e4 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -3227,13 +3227,22 @@ virNetworkBridgeInUseHelper(const void *payload,
 int ret = 0;
 virNetworkObjPtr net = (virNetworkObjPtr) payload;
 const struct virNetworkBridgeInUseHelperData *data = opaque;
+bool defined_bridge = false;
 
 virObjectLock(net);
 if (net->def->bridge &&
-STREQ(net->def->bridge, data->bridge) &&
-!(data->skipname && STREQ(net->def->name, data->skipname)))
-ret = 1;
+STREQ(net->def->bridge, data->bridge)) {
+defined_bridge = true;
+if (!(data->skipname && STREQ(net->def->name, data->skipname)))
+ ret = 1;
+}
+
 virObjectUnlock(net);
+
+/* Bridge might have been created by a user manually outside libvirt */
+if (!defined_bridge && !ret)
+ret = virNetDevExists(data->bridge) ? 1 : 0;
+
 return ret;
 }
 

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

[libvirt] [PATCH v3 1/2] cleanup conf/device_conf.h inclusion from util/virnetdev.h

[Shivaprasad G Bhat]

Move the struct and enum definitions from device_conf.h to
virnetdev.h thus avoiding the file inclusion.

The wrong reference of conf files from util/ is allowed
in Makefile.am for now. The reason being,
The change in Makefile.am for libvirt_util_la_CFLAGS to remove
conf from the utils would require corresponding inclusion in util files
to specify the paths explicitly. This would result in syntax-check failures
(prohibit_cross_inclusion) which need to be worked around until
there are patches reworking the incorrect inclusion.

The syntax-check failure workaround seems dangerous as that might mask some
easily resolvable failures. So dont touch the Makefile.am for now. Resolve
the wrong inclusions in separate patches.

Signed-off-by: Shivaprasad G Bhat 
---
 src/conf/device_conf.h |   38 +-
 src/util/virnetdev.h   |   38 +-
 2 files changed, 38 insertions(+), 38 deletions(-)

diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
index 7ea90f6..a650189 100644
--- a/src/conf/device_conf.h
+++ b/src/conf/device_conf.h
@@ -31,19 +31,7 @@
 # include "virutil.h"
 # include "virthread.h"
 # include "virbuffer.h"
-
-typedef enum {
-VIR_INTERFACE_STATE_UNKNOWN = 1,
-VIR_INTERFACE_STATE_NOT_PRESENT,
-VIR_INTERFACE_STATE_DOWN,
-VIR_INTERFACE_STATE_LOWER_LAYER_DOWN,
-VIR_INTERFACE_STATE_TESTING,
-VIR_INTERFACE_STATE_DORMANT,
-VIR_INTERFACE_STATE_UP,
-VIR_INTERFACE_STATE_LAST
-} virInterfaceState;
-
-VIR_ENUM_DECL(virInterfaceState)
+# include "virnetdev.h"
 
 typedef struct _virDevicePCIAddress virDevicePCIAddress;
 typedef virDevicePCIAddress *virDevicePCIAddressPtr;
@@ -55,30 +43,6 @@ struct _virDevicePCIAddress {
 int  multi;  /* virTristateSwitch */
 };
 
-typedef struct _virInterfaceLink virInterfaceLink;
-typedef virInterfaceLink *virInterfaceLinkPtr;
-struct _virInterfaceLink {
-virInterfaceState state; /* link state */
-unsigned int speed;  /* link speed in Mbits per second */
-};
-
-typedef enum {
-VIR_NET_DEV_FEAT_GRXCSUM,
-VIR_NET_DEV_FEAT_GTXCSUM,
-VIR_NET_DEV_FEAT_GSG,
-VIR_NET_DEV_FEAT_GTSO,
-VIR_NET_DEV_FEAT_GGSO,
-VIR_NET_DEV_FEAT_GGRO,
-VIR_NET_DEV_FEAT_LRO,
-VIR_NET_DEV_FEAT_RXVLAN,
-VIR_NET_DEV_FEAT_TXVLAN,
-VIR_NET_DEV_FEAT_NTUPLE,
-VIR_NET_DEV_FEAT_RXHASH,
-VIR_NET_DEV_FEAT_LAST
-} virNetDevFeature;
-
-VIR_ENUM_DECL(virNetDevFeature)
-
 int virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr);
 
 int virDevicePCIAddressParseXML(xmlNodePtr node,
diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
index 856127b..daba0bb 100644
--- a/src/util/virnetdev.h
+++ b/src/util/virnetdev.h
@@ -30,7 +30,6 @@
 # include "virnetlink.h"
 # include "virmacaddr.h"
 # include "virpci.h"
-# include "device_conf.h"
 
 # ifdef HAVE_STRUCT_IFREQ
 typedef struct ifreq virIfreq;
@@ -47,6 +46,43 @@ typedef enum {
 } virNetDevRxFilterMode;
 VIR_ENUM_DECL(virNetDevRxFilterMode)
 
+typedef enum {
+VIR_INTERFACE_STATE_UNKNOWN = 1,
+VIR_INTERFACE_STATE_NOT_PRESENT,
+VIR_INTERFACE_STATE_DOWN,
+VIR_INTERFACE_STATE_LOWER_LAYER_DOWN,
+VIR_INTERFACE_STATE_TESTING,
+VIR_INTERFACE_STATE_DORMANT,
+VIR_INTERFACE_STATE_UP,
+VIR_INTERFACE_STATE_LAST
+} virInterfaceState;
+
+VIR_ENUM_DECL(virInterfaceState)
+
+typedef struct _virInterfaceLink virInterfaceLink;
+typedef virInterfaceLink *virInterfaceLinkPtr;
+struct _virInterfaceLink {
+virInterfaceState state; /* link state */
+unsigned int speed;  /* link speed in Mbits per second */
+};
+
+typedef enum {
+VIR_NET_DEV_FEAT_GRXCSUM,
+VIR_NET_DEV_FEAT_GTXCSUM,
+VIR_NET_DEV_FEAT_GSG,
+VIR_NET_DEV_FEAT_GTSO,
+VIR_NET_DEV_FEAT_GGSO,
+VIR_NET_DEV_FEAT_GGRO,
+VIR_NET_DEV_FEAT_LRO,
+VIR_NET_DEV_FEAT_RXVLAN,
+VIR_NET_DEV_FEAT_TXVLAN,
+VIR_NET_DEV_FEAT_NTUPLE,
+VIR_NET_DEV_FEAT_RXHASH,
+VIR_NET_DEV_FEAT_LAST
+} virNetDevFeature;
+
+VIR_ENUM_DECL(virNetDevFeature)
+
 typedef struct _virNetDevRxFilter virNetDevRxFilter;
 typedef virNetDevRxFilter *virNetDevRxFilterPtr;
 struct _virNetDevRxFilter {

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

[libvirt] [PATCH v3 0/2] network_conf: check if bridge exists on host for user created bridges

[Shivaprasad G Bhat]

The patch fixes the below problem.

==
If the bridge name is not mentioned in the  xml, the bridge name is
auto generated from virNetworkAllocateBridge(). If the default template named
bridge is created manually by a user, the bridge start will fail with
"File exists".

bash-4.3$ sudo brctl addbr virbr1

bash-4.3$ brctl show
bridge name bridge id STP enabled interfaces
br0 8000. no
virbr0 8000.525400a91d03 yes virbr0-nic
virbr1 8000. no

bash-4.3$ sudo virsh net-list --all
 Name State  Autostart Persistent
--
 default  active noyes

bash-4.3$ cat /tmp/isolated # Notice that the  intentionally not given.

  isolated
  
  

  

  


bash-4.3$ sudo virsh net-create /tmp/isolated
error: Failed to create network from isolated
error: Unable to create bridge virbr1: File exists
===

---

Shivaprasad G Bhat (2):
  cleanup conf/device_conf.h inclusion from util/virnetdev.h
  network_conf: check if bridge exists on host for user created bridges


 src/conf/device_conf.h  |   38 +-
 src/conf/network_conf.c |   15 ---
 src/util/virnetdev.h|   38 +-
 3 files changed, 50 insertions(+), 41 deletions(-)

--
Signature

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

Re: [libvirt] [PATCHv2 2/5] Add functions to track virtio-serial addresses

[Ján Tomko]

On Mon, Mar 23, 2015 at 05:46:19PM -0400, John Ferlan wrote:
> 
> 
> On 03/17/2015 07:41 AM, Ján Tomko wrote:
> > Create a sorted array of virtio-serial controllers.
> > Each of the elements contains the controller index
> > and a bitmap of available ports.
> > 
> > Buses are not tracked, because they aren't supported by QEMU.
> > ---
> >  src/conf/domain_addr.c   | 348 
> > +++
> >  src/conf/domain_addr.h   |  56 
> >  src/libvirt_private.syms |   9 ++
> >  3 files changed, 413 insertions(+)
> > 
> 
> I assumed the ACK to 1/5 sticks...

> > +
> > +static void
> > +virDomainVirtioSerialControllerFree(virDomainVirtioSerialControllerPtr 
> > cont)
> 
> Should the Free routine take a pointer so that when we VIR_FREE the
> pointer the caller doesn't have to "worry" about setting their copy to NULL?

None of the callers worry about that for this function. For the other
function, I like sticking to the existing convention:
*Free routines usually take a copy of the address, just like
virBitmapFree below.

I think
virFuncFree(foo->ptr);
looks more tidy than
virFuncFree(&(foo->ptr));

And in most of the cases, foo gets freed anyway.

> 
> > +{
> > +if (cont) {
> > +virBitmapFree(cont->ports);
> > +VIR_FREE(cont);
> > +}
> > +}
> > +
> > +static ssize_t
> > +virDomainVirtioSerialAddrPlaceController(virDomainVirtioSerialAddrSetPtr 
> > addrs,
> > + 
> > virDomainVirtioSerialControllerPtr cont)
> > +{
> > +size_t i;
> > +
> > +for (i = 0; i < addrs->ncontrollers; i++) {
> > +if (addrs->controllers[i]->idx >= cont->idx)
> > +return i;
> > +}
> 
> Would entries ""
> and " be rejected elsewhere?
> I would think "index" would be unique but this algorithm seems to be
> fine and happy with it.

For user-specified controllers, duplicate indexes are rejected by
virDomainDefRejectDuplicateControllers, so adding a controller
with a non-unique index would be a bug in the auto-allocation logic.

I'll squash this in:
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
index d9d01fc..cda9ad2 100644
--- a/src/conf/domain_addr.c
+++ b/src/conf/domain_addr.c
@@ -754,7 +754,14 @@ 
virDomainVirtioSerialAddrPlaceController(virDomainVirtioSerialAddrSetPtr addrs,
 size_t i;
 
 for (i = 0; i < addrs->ncontrollers; i++) {
-if (addrs->controllers[i]->idx >= cont->idx)
+if (addrs->controllers[i]->idx == cont->idx) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("virtio serial controller with index %u already 
exists"
+ " in the address set"),
+   cont->idx);
+return -2;
+}
+if (addrs->controllers[i]->idx > cont->idx)
 return i;
 }
 return -1;
@@ -804,7 +811,8 @@ 
virDomainVirtioSerialAddrSetAddController(virDomainVirtioSerialAddrSetPtr addrs,
 goto cleanup;
 cnt->idx = cont->idx;
 
-insertAt = virDomainVirtioSerialAddrPlaceController(addrs, cnt);
+if ((insertAt = virDomainVirtioSerialAddrPlaceController(addrs, cnt)) < -1)
+goto cleanup;
 if (VIR_INSERT_ELEMENT(addrs->controllers, insertAt,
addrs->ncontrollers, cnt) < 0)
 goto cleanup;


> > +/* virDomainVirtioSerialAddrSetRemoveController
> > + *
> > + * Removes a virtio serial controller from the address set.
> > + */
> > +int
> > +virDomainVirtioSerialAddrSetRemoveController(virDomainVirtioSerialAddrSetPtr
> >  addrs,
> > + virDomainControllerDefPtr 
> > cont)
> > +{
> > +int ret = -1;
> > +ssize_t pos;
> > +
> > +if (cont->type != VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL)
> > +return 0;
> > +
> > +VIR_DEBUG("Removing virtio serial controller index %u "
> > +  "from the address set", cont->idx);
> > +
> > +pos = virDomainVirtioSerialAddrFindController(addrs, cont->idx);
> > +
> > +if (pos >= 0 &&
> > +VIR_DELETE_ELEMENT(addrs->controllers, pos, addrs->ncontrollers) < 
> > 0)
> > +goto cleanup;
> > +
> 
> If 'pos' < 0, we return 0 (and perhaps leak something). OTOH, the
> controller was never added and the caller never checks status, maybe
> this should just be void (wonder why Coverity didn't whine)...
> 

There's nothing to be leaked. Coverity only whines if some callers check
the return value and some don't.

I'll change the return type to void.

> > +
> > +/* virDomainVirtioSerialAddrRelease
> > + *
> > + * Release the virtio serial address of the device
> > + */
> > +int
> > +virDomainVirtioSerialAddrRelease(virDomainVirtioSerialAddrSetPtr addrs,
> > + virDomainDeviceInfoPtr info)
> > +{
> > +virBitmapPtr map;
> > +char *str = NULL;
> > +int ret = -1;
> > +ssize_t i;
> > +
> > +if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL ||
> > +i

Re: [libvirt] [PATCH 4/6] qemu: check if domain is really active when do setvcpus with --live

[lhuang]

On 03/24/2015 05:31 PM, Pavel Hrdina wrote:

On Tue, Mar 24, 2015 at 02:27:42PM +0800, lhuang wrote:

On 03/20/2015 10:39 PM, Pavel Hrdina wrote:

From: Luyao Huang 

Commit e105dc9 fix setting vcpus for offline domain, but forget check
if vm is active when pass VIR_DOMAIN_AFFECT_LIVE flags.

   # virsh setvcpus test3 4 --live
   error: Failed to create controller cpu for group: No such file or directory

Add a check if we pass VIR_DOMAIN_AFFECT_LIVE flags.

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

Signed-off-by: Luyao Huang 
Signed-off-by: Pavel Hrdina 
---
   src/qemu/qemu_driver.c | 14 --
   1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4942712..c4d96bd 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4881,6 +4881,14 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int 
nvcpus,
   if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
   goto cleanup;
   
+if (flags & VIR_DOMAIN_AFFECT_LIVE) {

+if (!virDomainObjIsActive(vm)) {
+virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+   _("domain is not running"));
+goto endjob;
+}
+}
+
   if (flags & VIR_DOMAIN_AFFECT_LIVE && !(flags & VIR_DOMAIN_VCPU_GUEST)) {
   if (virCgroupNewEmulator(priv->cgroup, false, &cgroup_temp) < 0)
   goto endjob;
@@ -4947,12 +4955,6 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int 
nvcpus,
   if (ncpuinfo < 0)
   goto endjob;
   
-if (!virDomainObjIsActive(vm)) {

-virReportError(VIR_ERR_OPERATION_INVALID, "%s",
-   _("domain is not running"));
-goto endjob;
-}
-
   if (qemuAgentUpdateCPUInfo(nvcpus, cpuinfo, ncpuinfo) < 0)
   goto endjob;

Sorry, i forgot there is a function virDomainLiveConfigHelperMethod(),
move this function just after qemuDomainObjBeginJob() maybe a good way
to fix this issue. Also virDomainLiveConfigHelperMethod() may change
flags, so it should be done more early (as soon as possible after set a
lock to vm).

I've already sent a patch 7/6 to move that function.  I realized that right
after I've sent this series to mailing list.


Pavel


Okay, I think i missed patch 7/6 when i looked these patches :)

Thanks for your reply

   

Luyao


Luyao

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

Re: [libvirt] [PATCH 4/6] qemu: check if domain is really active when do setvcpus with --live

[Pavel Hrdina]

On Tue, Mar 24, 2015 at 02:27:42PM +0800, lhuang wrote:
> 
> On 03/20/2015 10:39 PM, Pavel Hrdina wrote:
> > From: Luyao Huang 
> >
> > Commit e105dc9 fix setting vcpus for offline domain, but forget check
> > if vm is active when pass VIR_DOMAIN_AFFECT_LIVE flags.
> >
> >   # virsh setvcpus test3 4 --live
> >   error: Failed to create controller cpu for group: No such file or 
> > directory
> >
> > Add a check if we pass VIR_DOMAIN_AFFECT_LIVE flags.
> >
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1204006
> >
> > Signed-off-by: Luyao Huang 
> > Signed-off-by: Pavel Hrdina 
> > ---
> >   src/qemu/qemu_driver.c | 14 --
> >   1 file changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 4942712..c4d96bd 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -4881,6 +4881,14 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned 
> > int nvcpus,
> >   if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
> >   goto cleanup;
> >   
> > +if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> > +if (!virDomainObjIsActive(vm)) {
> > +virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> > +   _("domain is not running"));
> > +goto endjob;
> > +}
> > +}
> > +
> >   if (flags & VIR_DOMAIN_AFFECT_LIVE && !(flags & 
> > VIR_DOMAIN_VCPU_GUEST)) {
> >   if (virCgroupNewEmulator(priv->cgroup, false, &cgroup_temp) < 0)
> >   goto endjob;
> > @@ -4947,12 +4955,6 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned 
> > int nvcpus,
> >   if (ncpuinfo < 0)
> >   goto endjob;
> >   
> > -if (!virDomainObjIsActive(vm)) {
> > -virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> > -   _("domain is not running"));
> > -goto endjob;
> > -}
> > -
> >   if (qemuAgentUpdateCPUInfo(nvcpus, cpuinfo, ncpuinfo) < 0)
> >   goto endjob;
> 
> Sorry, i forgot there is a function virDomainLiveConfigHelperMethod(), 
> move this function just after qemuDomainObjBeginJob() maybe a good way 
> to fix this issue. Also virDomainLiveConfigHelperMethod() may change 
> flags, so it should be done more early (as soon as possible after set a 
> lock to vm).

I've already sent a patch 7/6 to move that function.  I realized that right
after I've sent this series to mailing list.


Pavel

> 
> >   
> 
> Luyao

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

Re: [libvirt] GSOC 2015: Libvirt Projects

[Stefan Hajnoczi]

On Fri, Mar 13, 2015 at 12:03 AM, Richa Sehgal
 wrote:
> I am currently pursuing Master's in University of Illinois at Urbana
> Champaign, USA and completed by B.Tech from Indian Institute of Technology -
> Delhi (IIT- Delhi).
>
> I am interested in contributing to Libvirt community through GSOC 2015. I am
> using VirtualBox to run Ubuntu and have used VMware Player in the past.
> Actually I am very interested in understanding virtualization at a more
> practical level and do not want to limit myself to theoretical knowledge. I
> have played with QEMU during the OS class, but I am new to Libvirt, and thus
> would like to start with beginner level projects. There are two that I
> found:
>
> 1. Enhancing libvirt-designer
> 2. Abstracting device address allocation
>
> Are these projects still open, or students have already applied to them? Are
> there any other projects that I can start with? I am really excited about
> contributing to the libvirt project. Please let me know of a suitable way of
> starting with these. I look forward for a useful engagement with the
> community this summer. My irc name is: richashi.

Hi,
I have CCed the libvirt GSoC mentors in case you haven't contacted
them directly yet.

All project ideas are open until the student application deadline on
March 27th.  You need to have the right skills for the project whether
or not there are other candidates applying, so I suggest focussing on
the project ideas that most interest you.

Good luck!

Stefan

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

Re: [libvirt] [RFC PATCH v2 00/12] qemu: add support to hot-plug/unplug cpu device

[Zhu Guihua]

On 03/24/2015 04:30 PM, Zhi Yong Wu wrote:

HI,

Do you have plan to update this patchset based on the comments
recently or have the latest one to post?


The develop plan for cpu hotplug in qemu has been changed, it is to add 
socket-based

device_add.

So I think we could not continue this cpu hotplug work in libvirt until the
interface about this feature in qemu could be determined.

Thanks,
Zhu



I noticed that the patchset for memory hotplug had got merged, is
there any plan to merge this patchset?


On Fri, Feb 13, 2015 at 12:13 AM, Peter Krempa  wrote:

On Thu, Feb 12, 2015 at 19:53:13 +0800, Zhu Guihua wrote:

Hi all,

Any comments about this series?

I'm not sure whether this series' method to support cpu hotplug in
libvirt is reasonable, so could anyone give more suggestions about this
function? Thanks.

Well, as Dan pointed out in his review for this series and previous
version, we are not convinced that we need a way to specify the CPU
model and other parameters as having dissimilar CPUs doesn't make sense.

A solution is either to build the cpu plug on top of the existing API or
provide enough information to convince us that having the cpu model in
the XML actually makes sense.


Regards,
Zhu

Peter

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





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

Re: [libvirt] [RFC PATCH v2 00/12] qemu: add support to hot-plug/unplug cpu device

[Zhi Yong Wu]

HI,

Do you have plan to update this patchset based on the comments
recently or have the latest one to post?

I noticed that the patchset for memory hotplug had got merged, is
there any plan to merge this patchset?


On Fri, Feb 13, 2015 at 12:13 AM, Peter Krempa  wrote:
> On Thu, Feb 12, 2015 at 19:53:13 +0800, Zhu Guihua wrote:
>> Hi all,
>>
>> Any comments about this series?
>>
>> I'm not sure whether this series' method to support cpu hotplug in
>> libvirt is reasonable, so could anyone give more suggestions about this
>> function? Thanks.
>
> Well, as Dan pointed out in his review for this series and previous
> version, we are not convinced that we need a way to specify the CPU
> model and other parameters as having dissimilar CPUs doesn't make sense.
>
> A solution is either to build the cpu plug on top of the existing API or
> provide enough information to convince us that having the cpu model in
> the XML actually makes sense.
>
>>
>> Regards,
>> Zhu
>
> Peter
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list



-- 
Regards,

Zhi Yong Wu

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

Re: [libvirt] [PATCH 2/2] qemu: add a max_core setting to qemu.conf for core dump size

[Ján Tomko]

On Mon, Mar 23, 2015 at 08:43:31PM -0400, John Ferlan wrote:
> 
> 
> On 03/18/2015 08:36 AM, Daniel P. Berrange wrote:
> > Currently the QEMU processes inherit their core dump rlimit
> > from libvirtd, which is really suboptimal. This change allows
> > their limit to be directly controller from qemu.conf instead.
> > ---
> >  src/libvirt_private.syms   |  2 ++
> >  src/qemu/libvirtd_qemu.aug |  1 +
> >  src/qemu/qemu.conf | 12 
> >  src/qemu/qemu_conf.c   |  3 +++
> >  src/qemu/qemu_conf.h   |  2 ++
> >  src/qemu/qemu_process.c|  2 ++
> >  src/qemu/test_libvirtd_qemu.aug.in |  1 +
> >  src/util/vircommand.c  | 14 ++
> >  src/util/vircommand.h  |  1 +
> >  src/util/virprocess.c  | 35 +++
> >  src/util/virprocess.h  |  1 +
> >  11 files changed, 74 insertions(+)
> > 
> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > index ca3520d..7446357 100644
> > --- a/src/libvirt_private.syms
> > +++ b/src/libvirt_private.syms
> > @@ -1240,6 +1240,7 @@ virCommandSetErrorFD;
> >  virCommandSetGID;
> >  virCommandSetInputBuffer;
> >  virCommandSetInputFD;
> > +virCommandSetMaxCoreSize;
> >  virCommandSetMaxFiles;
> >  virCommandSetMaxMemLock;
> >  virCommandSetMaxProcesses;
> > @@ -1951,6 +1952,7 @@ virProcessRunInMountNamespace;
> >  virProcessSchedPolicyTypeFromString;
> >  virProcessSchedPolicyTypeToString;
> >  virProcessSetAffinity;
> > +virProcessSetMaxCoreSize;
> >  virProcessSetMaxFiles;
> >  virProcessSetMaxMemLock;
> >  virProcessSetMaxProcesses;
> > diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
> > index 62951da..029a55a 100644
> > --- a/src/qemu/libvirtd_qemu.aug
> > +++ b/src/qemu/libvirtd_qemu.aug
> > @@ -71,6 +71,7 @@ module Libvirtd_qemu =
> >   | bool_entry "set_process_name"
> >   | int_entry "max_processes"
> >   | int_entry "max_files"
> > + | int_entry "max_core"
> >  
> > let device_entry = bool_entry "mac_filter"
> >   | bool_entry "relaxed_acs_check"
> > diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> > index 1c589a2..12e4326 100644
> > --- a/src/qemu/qemu.conf
> > +++ b/src/qemu/qemu.conf
> > @@ -390,6 +390,18 @@
> >  #max_processes = 0
> >  #max_files = 0
> >  
> > +# If max_core is set to a positive integer, then QEMU will be
> > +# permitted to create core dumps when it crashes, provided its
> > +# RAM size is smaller than the limit set. Be warned that the
> > +# core dump will include a full copy of the guest RAM, so if
> > +# the largest guest is 32 GB in size, the max_core limit will
> > +# have to be at least 33/34 GB to allow enough overhead.
> > +#
> > +# By default it will inherit core limit from libvirtd, which
> > +# is usually set to 0 by systemd/init.
> > +#
> > +# Size is in bytes
> > +#max_core = 0
> 
> It's too bad it cannot be a "sized" value... Reading 20G in a file for
> me is so much easier than the comparable byte value say nothing if we
> get to 128G, 1T, etc. (some day).
> 

Having the option as a string would also allow non-integer values, like
"unlimited".

Jan


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