Re: [libvirt] [PATCH] conf: Resolve Coverity FORWARD_NULL

[Michal Privoznik]

On 04.08.2015 17:54, John Ferlan wrote:
 
 
 On 08/04/2015 11:42 AM, Michal Privoznik wrote:
 On 04.08.2015 13:11, John Ferlan wrote:
 The recent changes to perform SCSI device address checks during the
 post parse callbacks ran afoul of the Coverity checker since the changes
 assumed that the 'xmlopt' parameter to virDomainDeviceDefPostParse
 would be non NULL (commit id 'ca2cf74e87'); however, what was missed
 is there was an if (xmlopt  check being made, so Coverity believed
 that it could be possible for a NULL 'xmlopt'.

 Checking the various calling paths seemingly disproves that. If called
 from virDomainDeviceDefParse, there were two other possible calls that
 would end up dereffing, so that path could not be NULL. If called via
 virDomainDefPostParseDeviceIterator via virDomainDefPostParse there
 are two callers (virDomainDefParseXML and qemuParseCommandLine)
 which deref xmlopt either directly or through another call.

 So I'm removing the check for non-NULL xmlopt.

 Signed-off-by: John Ferlan jfer...@redhat.com
 ---
  src/conf/domain_conf.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
 index 77a50c3..dd5ebd7 100644
 --- a/src/conf/domain_conf.c
 +++ b/src/conf/domain_conf.c
 @@ -4145,7 +4145,7 @@ virDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
  {
  int ret;
  
 -if (xmlopt  xmlopt-config.devicesPostParseCallback) {
 +if (xmlopt-config.devicesPostParseCallback) {
  ret = xmlopt-config.devicesPostParseCallback(dev, def, caps,
xmlopt-config.priv);
  if (ret  0)


 ACK

 Although with the virDomainDefPostParse() it should be the same story.
 @xmlopt can't be NULL there too. But that can be saved for a follow up
 patch if you want.

 Michal

 
 Myopia strikes again...  I need a larger workspace!
 
 John
 
 Perhaps it'd be better to squash in the following rather than have a
 separate patch since it's same issue even though Coverity only complained
 about the initial one. Although if it's preferred to have a separate
 patch that's fine by me too:
 
 diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
 index 7d3a24d..5eaeb21 100644
 --- a/src/conf/domain_conf.c
 +++ b/src/conf/domain_conf.c
 @@ -4190,7 +4190,7 @@ virDomainDefPostParse(virDomainDefPtr def,
  };
  
  /* call the domain config callback */
 -if (xmlopt  xmlopt-config.domainPostParseCallback) {
 +if (xmlopt-config.domainPostParseCallback) {
  ret = xmlopt-config.domainPostParseCallback(def, caps,
   xmlopt-config.priv);
  if (ret  0)
 
 

I'd prefer to have it in a single patch. ACK with this squashed in then.

Michal

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

Re: [libvirt] [PATCH] virsh: fix domfsinfo wrong output in quiet mode

[Ján Tomko]

On Wed, Aug 05, 2015 at 11:17:22AM +0800, Luyao Huang wrote:
 https://bugzilla.redhat.com/show_bug.cgi?id=1250287
 
 When run domfsinfo in quiet mode, we cannot get any
 useful information (just get \n), this is because
 we didn't use vshPrint to print useful information.
 
 Signed-off-by: Luyao Huang lhu...@redhat.com
 ---
  tools/virsh-domain.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)
 

ACK and 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] [PATCH] virsh: fix domfsinfo wrong output in quiet mode

[lhuang]

On 08/05/2015 04:05 PM, Ján Tomko wrote:

On Wed, Aug 05, 2015 at 11:17:22AM +0800, Luyao Huang wrote:

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

When run domfsinfo in quiet mode, we cannot get any
useful information (just get \n), this is because
we didn't use vshPrint to print useful information.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  tools/virsh-domain.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)


ACK and pushed.


Thanks a lot for your quick review.



Jan


Luyao

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

[libvirt] [PATCHv2 0/2] Added waiting for DAD to finish for bridge address.

[Maxim Perevedentsev]

This is a fix for commit db488c79173b240459c7754f38c3c6af9b432970
dnsmasq main process which is relied on when waiting for DAD to complete
exits without actually waiting for DAD. This is dnsmasq daemon's task.

It seems to be a race that DAD finished before dnsmasq main process exited.
The above commit needs the execution to block until DAD finishes
for bridge IPv6 address because then it closes dummy tap device.
Thus we need to ensure this ourselves.

So we periodically poll the kernel using netlink and
check whether there are any IPv6 addresses assigned to bridge
which have 'tentative' state. After DAD is finished, execution continues.
I guess that is what dnsmasq was assumed to do.

We use netlink to dump information about existing IPv6 addresses. Netlink's
response is a multi-part message. Unfortunately, the current implementation
of virNetlink treats such messages as faulty and throws an error. So the patch 
2/2
adds multi-part nelink response support.

Update v2: fixed syntax.

Maxim Perevedentsev (2):
  network: added waiting for DAD to finish for bridge address.
  Add support for multi-part netlink messages.

 src/network/bridge_driver.c | 113 +++-
 src/util/virnetlink.c   |   4 +-
 2 files changed, 115 insertions(+), 2 deletions(-)

--
Sincerely,
Maxim Perevedentsev

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

[libvirt] [PATCHv2 1/2] network: added waiting for DAD to finish for bridge address.

[Maxim Perevedentsev]

This is a fix for commit db488c79173b240459c7754f38c3c6af9b432970
dnsmasq main process exits without waiting for DAD, this is dnsmasq
daemon's task. So we periodically poll the kernel using netlink and
check whether there are any IPv6 addresses assigned to bridge
which have 'tentative' state. After DAD is finished, execution continues.
I guess that is what dnsmasq was assumed to do.
---
Difference to v1: fixed syntax (comments and alignment).

 src/network/bridge_driver.c | 113 +++-
 1 file changed, 112 insertions(+), 1 deletion(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 3d6721b..fb69ae5 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -2025,6 +2025,111 @@ networkAddRouteToBridge(virNetworkObjPtr network,
 return 0;
 }

+/* return whether there is a known address with 'tentative' flag set */
+static int
+networkParseDadStatus(struct nlmsghdr *nlh, int len,
+  virNetworkObjPtr network)
+{
+struct ifaddrmsg *ifaddrmsg_ptr;
+unsigned int ifaddrmsg_len;
+struct rtattr *rtattr_ptr;
+size_t i;
+virNetworkIpDefPtr ipdef;
+unsigned char prefix;
+struct in6_addr *addr;
+for (; NLMSG_OK(nlh, len); nlh = NLMSG_NEXT(nlh, len)) {
+if (NLMSG_PAYLOAD(nlh, 0)  sizeof(struct ifaddrmsg)) {
+/* Message without payload is the last one. */
+break;
+}
+
+ifaddrmsg_ptr = (struct ifaddrmsg *)NLMSG_DATA(nlh);
+if (!(ifaddrmsg_ptr-ifa_flags  IFA_F_TENTATIVE)) {
+/* Not tentative: we are not interested in this entry. */
+continue;
+}
+
+ifaddrmsg_len = IFA_PAYLOAD(nlh);
+rtattr_ptr = (struct rtattr *) IFA_RTA(ifaddrmsg_ptr);
+for (; RTA_OK(rtattr_ptr, ifaddrmsg_len);
+rtattr_ptr = RTA_NEXT(rtattr_ptr, ifaddrmsg_len)) {
+if (RTA_PAYLOAD(rtattr_ptr) != sizeof(struct in6_addr)) {
+/* No address: ignore. */
+continue;
+}
+
+/* We check only known addresses. */
+for (i = 0;
+ (ipdef = virNetworkDefGetIpByIndex(network-def, AF_INET6, 
i));
+ i++) {
+
+prefix = virNetworkIpDefPrefix(ipdef);
+addr = ipdef-address.data.inet6.sin6_addr;
+
+if (prefix == ifaddrmsg_ptr-ifa_prefixlen 
+!memcmp(addr, RTA_DATA(rtattr_ptr),
+sizeof(struct in6_addr))) {
+/* We found matching tentative address. */
+return 1;
+}
+}
+}
+}
+return 0;
+}
+
+/* return after DAD finishes for all known IPv6 addresses or an error */
+static int
+networkWaitDadFinish(virNetworkObjPtr network)
+{
+struct nl_msg *nlmsg = NULL;
+struct ifaddrmsg ifa;
+struct nlmsghdr *resp = NULL;
+unsigned int recvbuflen;
+int ret = -1, dad = 1;
+
+if (!(nlmsg = nlmsg_alloc_simple(RTM_GETADDR,
+ NLM_F_REQUEST | NLM_F_DUMP))) {
+virReportOOMError();
+return -1;
+}
+
+memset(ifa, 0, sizeof(ifa));
+/* DAD is for IPv6 adresses only. */
+ifa.ifa_family = AF_INET6;
+if (nlmsg_append(nlmsg, ifa, sizeof(ifa), NLMSG_ALIGNTO)  0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, %s,
+   _(allocated netlink buffer is too small));
+goto cleanup;
+}
+
+/* Periodically query netlink until DAD finishes on all known addresses. */
+while (dad) {
+if (virNetlinkCommand(nlmsg, resp, recvbuflen, 0, 0,
+  NETLINK_ROUTE, 0)  0)
+goto cleanup;
+
+if (virNetlinkGetErrorCode(resp, recvbuflen)  0) {
+virReportError(VIR_ERR_SYSTEM_ERROR, %s,
+   _(error reading DAD state information));
+goto cleanup;
+}
+
+/* Parse response. */
+dad = networkParseDadStatus(resp, recvbuflen, network);
+if (dad)
+usleep(1000 * 10);
+
+VIR_FREE(resp);
+}
+ret = 0;
+
+ cleanup:
+VIR_FREE(resp);
+nlmsg_free(nlmsg);
+return ret;
+}
+
 static int
 networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
virNetworkObjPtr network)
@@ -2159,7 +2264,13 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr 
driver,
 if (v6present  networkStartRadvd(driver, network)  0)
 goto err4;

-/* DAD has happened (dnsmasq waits for it), dnsmasq is now bound to the
+/* dnsmasq main process does not wait for DAD to complete,
+ * so we need to wait for it ourselves.
+ */
+if (v6present  networkWaitDadFinish(network)  0)
+goto err4;
+
+/* DAD has happened, dnsmasq is now bound to the
  * bridge's IPv6 address, so we can now set the dummy tun down.
  */
 if (tapfd = 0) {
--
Sincerely,
Maxim Perevedentsev

[libvirt] [PATCHv2 2/2] Add support for multi-part netlink messages.

[Maxim Perevedentsev]

Such messages do not have NLMSG_ERROR or NLMSG_DONE type
but they are valid responses. We test 'multi-partness'
by looking for NLM_F_MULTI flag.
---
Difference to v1: fixed comment style.

 src/util/virnetlink.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
index 0052ef9..f02bb59 100644
--- a/src/util/virnetlink.c
+++ b/src/util/virnetlink.c
@@ -386,7 +386,9 @@ virNetlinkGetErrorCode(struct nlmsghdr *resp, unsigned int 
recvbuflen)
 break;

 default:
-goto malformed_resp;
+/* We allow multipart messages. */
+if (!(resp-nlmsg_flags  NLM_F_MULTI))
+goto malformed_resp;
 }

 return result;
--
Sincerely,
Maxim Perevedentsev

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

[libvirt] [PATCH 0/5] domainRename API implementation

[Tomas Meszaros]

This is an effort to implement domain rename API. Presented patch series
consists of the following: virDomainRename API implementation for qemu,
implementation of the virsh command domrename and the additional support
code.

The idea behind this endeavor is to provide convenient and safe way to rename
a domain.

Instead of the:

virsh dumpxml domain  domain.xml
(change domain name in domain.xml)
virsh undefine domain
virsh define domain.xml

user can simply type:

virsh domrename foo bar

or call virDomainRename() API and domain foo will be renamed to bar.

We currently support only renaming inactive domains without snapshots.
Renaming procedure takes care of domain log, config, guest agent path and should
be able to recover in case of failure.

I've been working on this functionality in collaboration with Michal Privoznik
who is my mentor during the GSoC 2015. If you have any questions, ideas
or criticism feel free to join the discussion.


Tomas Meszaros (5):
  Introduce virDomainRename API
  virsh: Implement domrename command
  domain_conf: Introducde virDomainObjListRenameAddNew() 
virDomainObjListRenameRemove()
  Introduce new VIR_DOMAIN_EVENT_DEFINED_RENAMED event
  qemu: Implement virDomainRename

 examples/object-events/event-test.c |   2 +
 include/libvirt/libvirt-domain.h|   3 +
 src/access/viraccessperm.c  |   3 +-
 src/access/viraccessperm.h  |   6 ++
 src/conf/domain_conf.c  |  35 
 src/conf/domain_conf.h  |   5 ++
 src/driver-hypervisor.h |   5 ++
 src/libvirt-domain.c|  31 +++
 src/libvirt_private.syms|   2 +
 src/libvirt_public.syms |   5 ++
 src/qemu/qemu_driver.c  | 172 
 src/remote/remote_driver.c  |   1 +
 src/remote/remote_protocol.x|  18 +++-
 src/remote_protocol-structs |   8 ++
 tools/virsh-domain.c|  60 -
 tools/virsh.pod |   7 ++
 16 files changed, 360 insertions(+), 3 deletions(-)

--
2.1.0

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

[libvirt] [PATCH 5/5] qemu: Implement virDomainRename

[Tomas Meszaros]

Currently supports only renaming inactive domains without snapshots.

Signed-off-by: Tomas Meszaros e...@tty.sk
---
 src/qemu/qemu_driver.c | 172 +
 1 file changed, 172 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b9278f8..3ff04fe 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -19835,6 +19835,177 @@ qemuDomainSetUserPassword(virDomainPtr dom,
 }
 
 
+static int qemuDomainRename(virDomainPtr dom,
+const char *new_name)
+{
+virQEMUDriverPtr driver = dom-conn-privateData;
+virQEMUDriverConfigPtr cfg = NULL;
+virDomainObjPtr vm;
+virObjectEventPtr event = NULL;
+int ret = -1;
+int logfile = -1;
+size_t i;
+char ebuf[1024];
+char *timestamp;
+char *rename_log_msg = NULL;
+char *new_dom_name = NULL;
+char *old_dom_name = NULL;
+char *old_dom_cfg_file = NULL;
+char *new_guest_path = NULL;
+char *old_guest_path = NULL;
+virDomainChrDefPtr agent = NULL;
+
+if (VIR_STRDUP(new_dom_name, new_name)  0)
+goto cleanup;
+
+if (!(vm = qemuDomObjFromDomain(dom)))
+goto cleanup;
+
+if (virDomainRenameEnsureACL(dom-conn, vm-def)  0)
+goto cleanup;
+
+cfg = virQEMUDriverGetConfig(driver);
+
+if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY)  0)
+goto cleanup;
+
+if (virDomainObjIsActive(vm)) {
+virReportError(VIR_ERR_OPERATION_INVALID, %s,
+   _(cannot rename active domain));
+goto endjob;
+}
+
+if (!vm-persistent) {
+virReportError(VIR_ERR_OPERATION_INVALID, %s,
+   _(cannot rename a transient domain));
+goto endjob;
+}
+
+if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_SHUTOFF) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+   %s, _(domain has to be shutoff before renaming));
+goto endjob;
+}
+
+if (virDomainSnapshotObjListNum(vm-snapshots, NULL, 0)  0) {
+virReportError(VIR_ERR_OPERATION_INVALID,
+   %s, _(cannot rename domain with snapshots));
+goto endjob;
+}
+
+if (virAsprintf(rename_log_msg, : domain %s has been renamed to %s\n,
+vm-def-name, new_name)  0) {
+goto endjob;
+}
+
+if (!(old_dom_cfg_file = virDomainConfigFile(cfg-configDir,
+ vm-def-name))) {
+goto endjob;
+}
+
+if (virDomainObjListRenameAddNew(driver-domains, vm, new_name)  0)
+goto endjob;
+
+if ((logfile = qemuDomainCreateLog(driver, vm, true))  0)
+goto rollback;
+
+/* Switch name in domain definition. */
+old_dom_name = vm-def-name;
+vm-def-name = new_dom_name;
+new_dom_name = NULL;
+
+/* Change guest agent path. */
+for (i = 0; i  vm-def-nchannels; i++) {
+virDomainChrDefPtr channel = vm-def-channels[i];
+if (channel-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL 
+channel-targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO 
+channel-source.data.file.path 
+channel-target.name) {
+
+if (virAsprintf(new_guest_path, %s/%s.%s,
+cfg-channelTargetDir,
+new_name, channel-target.name)  0)
+goto rollback;
+
+old_guest_path = channel-source.data.file.path;
+channel-source.data.file.path = new_guest_path;
+new_guest_path = NULL;
+agent = channel;
+break;
+}
+}
+
+
+if (virDomainSaveConfig(cfg-configDir, vm-def)  0)
+goto rollback;
+
+if (virFileExists(old_dom_cfg_file) 
+unlink(old_dom_cfg_file)  0) {
+virReportSystemError(errno,
+ _(cannot remove old domain config file %s),
+ old_dom_cfg_file);
+goto rollback;
+}
+
+/* Remove old domain name from table. */
+virDomainObjListRenameRemove(driver-domains, old_dom_name);
+
+event = virDomainEventLifecycleNewFromObj(vm,
+  VIR_DOMAIN_EVENT_DEFINED,
+  
VIR_DOMAIN_EVENT_DEFINED_RENAMED);
+
+/* Write message to the log. */
+if ((timestamp = virTimeStringNow()) != NULL) {
+if (safewrite(logfile, timestamp, strlen(timestamp))  0 ||
+safewrite(logfile, rename_log_msg,
+  strlen(rename_log_msg))  0) {
+VIR_WARN(Unable to write timestamp to logfile: %s,
+ virStrerror(errno, ebuf, sizeof(ebuf)));
+}
+VIR_FREE(timestamp);
+}
+
+/* Success, domain has been renamed. */
+ret = 0;
+
+ endjob:
+qemuDomainObjEndJob(driver, vm);
+
+ cleanup:
+if (VIR_CLOSE(logfile)  0) {
+VIR_WARN(Unable to close logfile: %s,
+ 

[libvirt] [PATCH 1/5] Introduce virDomainRename API

[Tomas Meszaros]

Also, among with this new API new ACL that restricts rename capability
is invented too.

Signed-off-by: Tomas Meszaros e...@tty.sk
---
 include/libvirt/libvirt-domain.h |  2 ++
 src/access/viraccessperm.c   |  3 ++-
 src/access/viraccessperm.h   |  6 ++
 src/driver-hypervisor.h  |  5 +
 src/libvirt-domain.c | 31 +++
 src/libvirt_public.syms  |  5 +
 src/remote/remote_driver.c   |  1 +
 src/remote/remote_protocol.x | 18 +-
 src/remote_protocol-structs  |  8 
 9 files changed, 77 insertions(+), 2 deletions(-)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index e8202cf..2ddc47d 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -3837,4 +3837,6 @@ int virDomainSetUserPassword(virDomainPtr dom,
  const char *password,
  unsigned int flags);
 
+int virDomainRename(virDomainPtr dom, const char *new_name);
+
 #endif /* __VIR_LIBVIRT_DOMAIN_H__ */
diff --git a/src/access/viraccessperm.c b/src/access/viraccessperm.c
index 0f58290..bdc7f60 100644
--- a/src/access/viraccessperm.c
+++ b/src/access/viraccessperm.c
@@ -43,7 +43,8 @@ VIR_ENUM_IMPL(virAccessPermDomain,
   fs_trim, fs_freeze,
   block_read, block_write, mem_read,
   open_graphics, open_device, screenshot,
-  open_namespace, set_time, set_password);
+  open_namespace, set_time, set_password,
+  rename);
 
 VIR_ENUM_IMPL(virAccessPermInterface,
   VIR_ACCESS_PERM_INTERFACE_LAST,
diff --git a/src/access/viraccessperm.h b/src/access/viraccessperm.h
index 1817da7..6ae4ee7 100644
--- a/src/access/viraccessperm.h
+++ b/src/access/viraccessperm.h
@@ -306,6 +306,12 @@ typedef enum {
  */
 VIR_ACCESS_PERM_DOMAIN_SET_PASSWORD,
 
+/**
+ * @desc: Rename domain
+ * @message: Renaming the domain requires authorization
+ */
+VIR_ACCESS_PERM_DOMAIN_RENAME,
+
 VIR_ACCESS_PERM_DOMAIN_LAST,
 } virAccessPermDomain;
 
diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h
index 3275343..e8c8c2a 100644
--- a/src/driver-hypervisor.h
+++ b/src/driver-hypervisor.h
@@ -650,6 +650,10 @@ typedef int
 (*virDrvDomainIsActive)(virDomainPtr dom);
 
 typedef int
+(*virDrvDomainRename)(virDomainPtr dom,
+  const char *new_name);
+
+typedef int
 (*virDrvDomainIsPersistent)(virDomainPtr dom);
 
 typedef int
@@ -1347,6 +1351,7 @@ struct _virHypervisorDriver {
 virDrvConnectIsEncrypted connectIsEncrypted;
 virDrvConnectIsSecure connectIsSecure;
 virDrvDomainIsActive domainIsActive;
+virDrvDomainRename domainRename;
 virDrvDomainIsPersistent domainIsPersistent;
 virDrvDomainIsUpdated domainIsUpdated;
 virDrvConnectCompareCPU connectCompareCPU;
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 837933f..c200965 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -8774,6 +8774,37 @@ virDomainIsPersistent(virDomainPtr dom)
 return -1;
 }
 
+/**
+ * virDomainRename:
+ * @dom: pointer to the domain object
+ * @new_name: new domain name
+ *
+ * Rename an inactive domain. New domain name is specified in the second
+ * argument.
+ *
+ * Returns 0 if renamed, -1 on error
+ */
+int
+virDomainRename(virDomainPtr dom, const char *new_name)
+{
+VIR_DEBUG(dom=%p, new_name=%s, dom, NULLSTR(new_name));
+
+virResetLastError();
+virCheckDomainReturn(dom, -1);
+virCheckNonNullArgGoto(new_name, error);
+
+if (dom-conn-driver-domainRename) {
+int ret = dom-conn-driver-domainRename(dom, new_name);
+if (ret  0)
+goto error;
+return ret;
+}
+
+virReportUnsupportedError();
+ error:
+virDispatchError(dom-conn);
+return -1;
+}
 
 /**
  * virDomainIsUpdated:
diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
index 2c653f2..dd94191 100644
--- a/src/libvirt_public.syms
+++ b/src/libvirt_public.syms
@@ -720,4 +720,9 @@ LIBVIRT_1.2.17 {
 virTypedParamsAddStringList;
 } LIBVIRT_1.2.16;
 
+LIBVIRT_1.2.19 {
+global:
+virDomainRename;
+} LIBVIRT_1.2.17;
+
 #  define new API here using predicted next version number 
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 5c4cf7c..ec26ebe 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -8391,6 +8391,7 @@ static virHypervisorDriver hypervisor_driver = {
 .domainGetFSInfo = remoteDomainGetFSInfo, /* 1.2.11 */
 .domainInterfaceAddresses = remoteDomainInterfaceAddresses, /* 1.2.14 */
 .domainSetUserPassword = remoteDomainSetUserPassword, /* 1.2.16 */
+.domainRename = remoteDomainRename, /* 1.2.19 */
 };
 
 static virNetworkDriver network_driver = {
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
index 9f1be6b..0f26793 100644
--- 

[libvirt] [PATCH 3/5] domain_conf: Introducde virDomainObjListRenameAddNew() virDomainObjListRenameRemove()

[Tomas Meszaros]

We just need to update the entry in the second hash table. Since commit 8728a56
we have two hash tables for the domain list so that we can do O(1) lookup
regardless of looking up by UUID or name. Since with renaming a domain UUID does
not change, we only need to update the second hash table, where domains are
referenced by their name.

We will call both functions from the qemuDomainRename().

Signed-off-by: Tomas Meszaros e...@tty.sk
---
 src/conf/domain_conf.c   | 35 +++
 src/conf/domain_conf.h   |  5 +
 src/libvirt_private.syms |  2 ++
 3 files changed, 42 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 6b557d1..c47ccf9 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2848,6 +2848,41 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr 
doms,
 return ret;
 }
 
+
+int
+virDomainObjListRenameAddNew(virDomainObjListPtr doms,
+ virDomainObjPtr vm,
+ const char *name)
+{
+int ret = -1;
+virObjectLock(doms);
+
+/* Add new name into the hash table of domain names. */
+if (virHashAddEntry(doms-objsName, name, vm)  0)
+goto cleanup;
+
+/* Okay, this is crazy. virHashAddEntry() does not increment
+ * the refcounter of @vm, but virHashRemoveEntry() does
+ * decrement it. We need to work around it. */
+virObjectRef(vm);
+
+ret = 0;
+ cleanup:
+virObjectUnlock(doms);
+return ret;
+}
+
+
+int
+virDomainObjListRenameRemove(virDomainObjListPtr doms, const char *name)
+{
+virObjectLock(doms);
+virHashRemoveEntry(doms-objsName, name);
+virObjectUnlock(doms);
+return 0;
+}
+
+
 /*
  * Mark the running VM config as transient. Ensures transient hotplug
  * operations do not persist past shutdown.
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 0fe6b1a..719018f 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2545,6 +2545,11 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr 
doms,
 virDomainXMLOptionPtr xmlopt,
 unsigned int flags,
 virDomainDefPtr *oldDef);
+int virDomainObjListRenameAddNew(virDomainObjListPtr doms,
+ virDomainObjPtr vm,
+ const char *name);
+int virDomainObjListRenameRemove(virDomainObjListPtr doms,
+ const char *name);
 void virDomainObjAssignDef(virDomainObjPtr domain,
virDomainDefPtr def,
bool live,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index ff322d6..70ef880 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -408,6 +408,8 @@ virDomainObjListNew;
 virDomainObjListNumOfDomains;
 virDomainObjListRemove;
 virDomainObjListRemoveLocked;
+virDomainObjListRenameAddNew;
+virDomainObjListRenameRemove;
 virDomainObjNew;
 virDomainObjParseNode;
 virDomainObjSetDefTransient;
-- 
2.1.0

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

[libvirt] [PATCH 4/5] Introduce new VIR_DOMAIN_EVENT_DEFINED_RENAMED event

[Tomas Meszaros]

This should be emitted whenever a domain is renamed.

Signed-off-by: Tomas Meszaros e...@tty.sk
---
 examples/object-events/event-test.c | 2 ++
 include/libvirt/libvirt-domain.h| 1 +
 tools/virsh-domain.c| 3 ++-
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/examples/object-events/event-test.c 
b/examples/object-events/event-test.c
index 4f17273..c62bd56 100644
--- a/examples/object-events/event-test.c
+++ b/examples/object-events/event-test.c
@@ -108,6 +108,8 @@ static const char *eventDetailToString(int event, int 
detail) {
 ret = Added;
 else if (detail == VIR_DOMAIN_EVENT_DEFINED_UPDATED)
 ret = Updated;
+else if (detail == VIR_DOMAIN_EVENT_DEFINED_RENAMED)
+ret = Renamed;
 break;
 case VIR_DOMAIN_EVENT_UNDEFINED:
 if (detail == VIR_DOMAIN_EVENT_UNDEFINED_REMOVED)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index 2ddc47d..d43ec27 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -2322,6 +2322,7 @@ typedef enum {
 typedef enum {
 VIR_DOMAIN_EVENT_DEFINED_ADDED = 0, /* Newly created config file */
 VIR_DOMAIN_EVENT_DEFINED_UPDATED = 1,   /* Changed config file */
+VIR_DOMAIN_EVENT_DEFINED_RENAMED = 2,   /* Domain was renamed */
 
 # ifdef VIR_ENUM_SENTINELS
 VIR_DOMAIN_EVENT_DEFINED_LAST
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index c7e218f..7291980 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -11763,7 +11763,8 @@ VIR_ENUM_DECL(vshDomainEventDefined)
 VIR_ENUM_IMPL(vshDomainEventDefined,
   VIR_DOMAIN_EVENT_DEFINED_LAST,
   N_(Added),
-  N_(Updated))
+  N_(Updated),
+  N_(Renamed))
 
 VIR_ENUM_DECL(vshDomainEventUndefined)
 VIR_ENUM_IMPL(vshDomainEventUndefined,
-- 
2.1.0

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

[libvirt] [PATCH 2/5] virsh: Implement domrename command

[Tomas Meszaros]

This patch implements new virsh command, domrename.

Using domrename, it will be possible to rename domain from the virsh shell by
calling virRenameDomain API.

It takes two arguments, current domain name and new domain name.

Example:

virsh # list --all
 IdName   State
 
  - barshut off

virsh # domrename bar foo
Domain successfully renamed

virsh # list --all
 IdName   State
 
  - fooshut off

virsh #

Signed-off-by: Tomas Meszaros e...@tty.sk
---
 tools/virsh-domain.c | 57 
 tools/virsh.pod  |  7 +++
 2 files changed, 64 insertions(+)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index a61656f..c7e218f 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -9724,6 +9724,57 @@ cmdDomname(vshControl *ctl, const vshCmd *cmd)
 }
 
 /*
+ * domrename command
+ */
+static const vshCmdInfo info_domrename[] = {
+{.name = help,
+ .data = N_(rename a domain)
+},
+{.name = desc,
+ .data = Rename an inactive domain.
+},
+{.name = NULL}
+};
+
+static const vshCmdOptDef opts_domrename[] = {
+{.name = domain,
+ .type = VSH_OT_DATA,
+ .flags = VSH_OFLAG_REQ,
+ .help = N_(domain name, id or uuid)
+},
+{.name = new-name,
+ .type = VSH_OT_DATA,
+ .flags = VSH_OFLAG_REQ,
+ .help = N_(new domain name)
+},
+{.name = NULL}
+};
+
+static bool
+cmdDomrename(vshControl *ctl, const vshCmd *cmd)
+{
+virDomainPtr dom;
+const char *new_name = NULL;
+bool ret = false;
+
+if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
+return ret;
+
+if (vshCommandOptStringReq(ctl, cmd, new-name, new_name)  0)
+goto cleanup;
+
+if (virDomainRename(dom, new_name)  0)
+goto cleanup;
+
+vshPrint(ctl, Domain successfully renamed\n);
+ret = true;
+
+ cleanup:
+virDomainFree(dom);
+return false;
+}
+
+/*
  * domid command
  */
 static const vshCmdInfo info_domid[] = {
@@ -13080,6 +13131,12 @@ const vshCmdDef domManagementCmds[] = {
  .info = info_domname,
  .flags = 0
 },
+{.name = domrename,
+ .handler = cmdDomrename,
+ .opts = opts_domrename,
+ .info = info_domrename,
+ .flags = 0
+},
 {.name = dompmsuspend,
  .handler = cmdDomPMSuspend,
  .opts = opts_dom_pm_suspend,
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 5ee9a96..c049f8f 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -1258,6 +1258,13 @@ on both of them).
 
 Convert a domain Id (or UUID) to domain name
 
+=item Bdomrename Idomain Inew-name
+
+Rename a domain. This command changes current domain name to the new name
+specified in the second argument.
+
+BNote: Domain must be inactive and without snapshots.
+
 =item Bdomstate Idomain [I--reason]
 
 Returns state about a domain.  I--reason tells virsh to also print
-- 
2.1.0

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

Re: [libvirt] [PATCH 1/5] Introduce virDomainRename API

[Daniel P. Berrange]

On Wed, Aug 05, 2015 at 01:59:07PM +0200, Tomas Meszaros wrote:
 Also, among with this new API new ACL that restricts rename capability
 is invented too.
 
 Signed-off-by: Tomas Meszaros e...@tty.sk
 ---
  include/libvirt/libvirt-domain.h |  2 ++
  src/access/viraccessperm.c   |  3 ++-
  src/access/viraccessperm.h   |  6 ++
  src/driver-hypervisor.h  |  5 +
  src/libvirt-domain.c | 31 +++
  src/libvirt_public.syms  |  5 +
  src/remote/remote_driver.c   |  1 +
  src/remote/remote_protocol.x | 18 +-
  src/remote_protocol-structs  |  8 
  9 files changed, 77 insertions(+), 2 deletions(-)

 diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
 index 9f1be6b..0f26793 100644
 --- a/src/remote/remote_protocol.x
 +++ b/src/remote/remote_protocol.x
 @@ -3230,6 +3230,14 @@ struct remote_domain_set_user_password_args {
  unsigned int flags;
  };
  
 +struct remote_domain_rename_args {
 +remote_nonnull_domain dom;
 +remote_string new_name;
 +};
 +
 +struct remote_domain_rename_ret {
 +int rename;
 +};
  
  /*- Protocol. -*/
  
 @@ -5696,5 +5704,13 @@ enum remote_procedure {
   * @generate:both
   * @acl: domain:set_password
   */
 -REMOTE_PROC_DOMAIN_SET_USER_PASSWORD = 357
 +REMOTE_PROC_DOMAIN_SET_USER_PASSWORD = 357,
 +
 +/**
 + * @generate: both
 + * @acl: domain:rename
 + * @acl: domain:write
 + * @acl: domain:save

When you require write + save you have already given away the
keys to the kingdom. Adding a rename permission doesn't
really have any benefit at that point.  So I'd just get rid
of the new rename permission.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCH 01/18] cpu: Mark driver functions in ppc64 driver

[Jiri Denemark]

On Tue, Aug 04, 2015 at 11:37:52 +0200, Andrea Bolognani wrote:
 Use the ppc64Driver prefix for all functions that are used to
 fill in the cpuDriverPPC64 structure, ie. those that are going
 to be called by the generic CPU code.
 
 This makes it clear which functions are exported and which are
 implementation details; it also gets rid of the ambiguity that
 affected the ppc64DataFree() function which, despite what the
 name suggested, was not related to ppc64DataCopy() and could
 not be used to release the memory allocated for a
 virCPUppc64Data* instance.
 
 No functional changes.

ACK

Jirka

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

Re: [libvirt] [PATCH 02/18] cpu: Simplify NULL handling in ppc64 driver

[Jiri Denemark]

On Tue, Aug 04, 2015 at 11:37:53 +0200, Andrea Bolognani wrote:
 Use briefer checks, eg. (!model) instead of (model == NULL), and
 avoid initializing to NULL a pointer that would be assigned in
 the first line of the function anyway.
 
 Also remove a pointless NULL assignment.
 
 No functional changes.
 ---
  src/cpu/cpu_ppc64.c | 33 -
  1 file changed, 16 insertions(+), 17 deletions(-)

ACK

Jirka

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

Re: [libvirt] [PATCH 5/5] qemu: Implement virDomainRename

[Daniel P. Berrange]

On Wed, Aug 05, 2015 at 01:59:11PM +0200, Tomas Meszaros wrote:
 Currently supports only renaming inactive domains without snapshots.
 
 Signed-off-by: Tomas Meszaros e...@tty.sk
 ---
  src/qemu/qemu_driver.c | 172 
 +
  1 file changed, 172 insertions(+)
 
 diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
 index b9278f8..3ff04fe 100644
 --- a/src/qemu/qemu_driver.c
 +++ b/src/qemu/qemu_driver.c
 @@ -19835,6 +19835,177 @@ qemuDomainSetUserPassword(virDomainPtr dom,
  }
  
  
 +static int qemuDomainRename(virDomainPtr dom,
 +const char *new_name)

 +/* Change guest agent path. */
 +for (i = 0; i  vm-def-nchannels; i++) {
 +virDomainChrDefPtr channel = vm-def-channels[i];
 +if (channel-deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL 
 +channel-targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO 
 
 +channel-source.data.file.path 
 +channel-target.name) {
 +
 +if (virAsprintf(new_guest_path, %s/%s.%s,
 +cfg-channelTargetDir,
 +new_name, channel-target.name)  0)
 +goto rollback;
 +
 +old_guest_path = channel-source.data.file.path;
 +channel-source.data.file.path = new_guest_path;
 +new_guest_path = NULL;
 +agent = channel;
 +break;
 +}
 +}

IMHO, we should not touch this at all. It is making a (potentially
invalid) assumption about the way the socket paths are named. If
we go down this route of changing various aspects of device config,
then people will question why channels are special cased and not
renaming other things. It just will just end up as a mess of hard
coded policy which works for some cases and is wrong for others.
So I think we should just stick to changing the name of the VM
only and not touch any other aspect of XML config.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCH 4/5] Introduce new VIR_DOMAIN_EVENT_DEFINED_RENAMED event

[Daniel P. Berrange]

On Wed, Aug 05, 2015 at 01:59:10PM +0200, Tomas Meszaros wrote:
 This should be emitted whenever a domain is renamed.
 
 Signed-off-by: Tomas Meszaros e...@tty.sk
 ---
  examples/object-events/event-test.c | 2 ++
  include/libvirt/libvirt-domain.h| 1 +
  tools/virsh-domain.c| 3 ++-
  3 files changed, 5 insertions(+), 1 deletion(-)
 
 diff --git a/examples/object-events/event-test.c 
 b/examples/object-events/event-test.c
 index 4f17273..c62bd56 100644
 --- a/examples/object-events/event-test.c
 +++ b/examples/object-events/event-test.c
 @@ -108,6 +108,8 @@ static const char *eventDetailToString(int event, int 
 detail) {
  ret = Added;
  else if (detail == VIR_DOMAIN_EVENT_DEFINED_UPDATED)
  ret = Updated;
 +else if (detail == VIR_DOMAIN_EVENT_DEFINED_RENAMED)
 +ret = Renamed;
  break;
  case VIR_DOMAIN_EVENT_UNDEFINED:
  if (detail == VIR_DOMAIN_EVENT_UNDEFINED_REMOVED)
 diff --git a/include/libvirt/libvirt-domain.h 
 b/include/libvirt/libvirt-domain.h
 index 2ddc47d..d43ec27 100644
 --- a/include/libvirt/libvirt-domain.h
 +++ b/include/libvirt/libvirt-domain.h
 @@ -2322,6 +2322,7 @@ typedef enum {
  typedef enum {
  VIR_DOMAIN_EVENT_DEFINED_ADDED = 0, /* Newly created config file */
  VIR_DOMAIN_EVENT_DEFINED_UPDATED = 1,   /* Changed config file */
 +VIR_DOMAIN_EVENT_DEFINED_RENAMED = 2,   /* Domain was renamed */
  
  # ifdef VIR_ENUM_SENTINELS
  VIR_DOMAIN_EVENT_DEFINED_LAST
 diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
 index c7e218f..7291980 100644
 --- a/tools/virsh-domain.c
 +++ b/tools/virsh-domain.c
 @@ -11763,7 +11763,8 @@ VIR_ENUM_DECL(vshDomainEventDefined)
  VIR_ENUM_IMPL(vshDomainEventDefined,
VIR_DOMAIN_EVENT_DEFINED_LAST,
N_(Added),
 -  N_(Updated))
 +  N_(Updated),
 +  N_(Renamed))

If we're going to emit a defined + renamed event for the new name,
we should probably also emit an undefined + renamed event for the
old one. That should ensure existing apps continue to work right


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

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

Re: [libvirt] [PATCH 03/18] cpu: Add NULL check in ppc64ModelCopy()

[Jiri Denemark]

On Tue, Aug 04, 2015 at 11:37:54 +0200, Andrea Bolognani wrote:
 ---
  src/cpu/cpu_ppc64.c | 3 +++
  1 file changed, 3 insertions(+)
 
 diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c
 index 05ff8f2..dd02a3f 100644
 --- a/src/cpu/cpu_ppc64.c
 +++ b/src/cpu/cpu_ppc64.c
 @@ -115,6 +115,9 @@ ppc64ModelCopy(const struct ppc64_model *model)
  {
  struct ppc64_model *copy;
  
 +if (!model)
 +return NULL;
 +
  if (VIR_ALLOC(copy)  0 ||
  VIR_STRDUP(copy-name, model-name)  0) {
  ppc64ModelFree(copy);

This doesn't seem to be really necessary since the function is not
called with model == NULL and I don't think that should change. If any
caller wants to pass NULL for a model, it should rather report an error
and return instead of trying to copy this NULL.

However, the only called of ppc64ModelCopy is pretty confusing:

static struct ppc64_model *
ppc64ModelFromCPU(const virCPUDef *cpu,
  const struct ppc64_map *map)
{
struct ppc64_model *model;

if (!(model = ppc64ModelFind(map, cpu-model))) {
virReportError(VIR_ERR_INTERNAL_ERROR,
   _(Unknown CPU model %s), cpu-model);
goto error;
}

if (!(model = ppc64ModelCopy(model)))
goto error;

return model;

 error:
ppc64ModelFree(model);
return NULL;
}

It uses model for pointing to a model which must not be freed and also
to its copy which has to be freed. There's no bug here but I think
changing is a good idea :-)

Jirka

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

Re: [libvirt] [PATCH 04/18] cpu: Use a different name for the copy in ppc64ModelFromCPU()

[Jiri Denemark]

On Tue, Aug 04, 2015 at 11:37:55 +0200, Andrea Bolognani wrote:
 While the previous code was correct, it looked wrong at first
 sight because the same variable used to store the result of a
 map lookup is later used to store a copy of said result. The
 copy is deallocated on error, but due to the fact that a single
 variable is used, it looks like the result of the lookup is
 deallocated instead, which would be a bug.
 
 Using a separate variable for the copy means the code is just
 as correct but much less likely to result confusing.
 
 No functional changes.
 ---
  src/cpu/cpu_ppc64.c | 7 ---
  1 file changed, 4 insertions(+), 3 deletions(-)
 
 diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c
 index dd02a3f..c769221 100644
 --- a/src/cpu/cpu_ppc64.c
 +++ b/src/cpu/cpu_ppc64.c
 @@ -162,6 +162,7 @@ ppc64ModelFromCPU(const virCPUDef *cpu,
const struct ppc64_map *map)
  {
  struct ppc64_model *model;
 +struct ppc64_model *copy;
  
  if (!(model = ppc64ModelFind(map, cpu-model))) {
  virReportError(VIR_ERR_INTERNAL_ERROR,
 @@ -169,13 +170,13 @@ ppc64ModelFromCPU(const virCPUDef *cpu,
  goto error;
  }
  
 -if (!(model = ppc64ModelCopy(model)))
 +if (!(copy = ppc64ModelCopy(model)))
  goto error;
  
 -return model;
 +return copy;
  
   error:
 -ppc64ModelFree(model);
 +ppc64ModelFree(copy);
  return NULL;

You forgot to initialize copy to NULL, but why not just

return ppc64ModelCopy(model);

and removing copy end the error label completely since it will never
do anything anyway?

Jirka

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

Re: [libvirt] [PATCH] qemu: Forbid image pre-creation for non-shared storage migration

[Ján Tomko]

On Tue, Aug 04, 2015 at 10:21:32AM +0200, Peter Krempa wrote:
 Libvirt doesn't reliably know the location of the backing chain when
 pre-creating images for non-shared migration. This isn't a problem for
 full copy, but incremental copy requires the information.
 
 Forbid pre-creating the image in cases where incremental migration is
 required. This limitation can perhaps be lifted once libvirt will fully
 support loading of backing chain information from the XML.
 
 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1249587
 ---
  src/qemu/qemu_migration.c | 13 +++--
  1 file changed, 11 insertions(+), 2 deletions(-)
 

ACK

Jan


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: Forbid image pre-creation for non-shared storage migration

[Peter Krempa]

On Wed, Aug 05, 2015 at 17:04:28 +0200, Ján Tomko wrote:
 On Tue, Aug 04, 2015 at 10:21:32AM +0200, Peter Krempa wrote:
  Libvirt doesn't reliably know the location of the backing chain when
  pre-creating images for non-shared migration. This isn't a problem for
  full copy, but incremental copy requires the information.
  
  Forbid pre-creating the image in cases where incremental migration is
  required. This limitation can perhaps be lifted once libvirt will fully
  support loading of backing chain information from the XML.
  
  Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1249587
  ---
   src/qemu/qemu_migration.c | 13 +++--
   1 file changed, 11 insertions(+), 2 deletions(-)
  
 
 ACK

Pushed; Thanks.

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 04/18] cpu: Use a different name for the copy in ppc64ModelFromCPU()

[Andrea Bolognani]

On Wed, 2015-08-05 at 14:28 +0200, Jiri Denemark wrote:
 
 You forgot to initialize copy to NULL, but why not just
 
 return ppc64ModelCopy(model);
 
 and removing copy end the error label completely since it will 
 never
 do anything anyway?

You're right! Done :)

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

[libvirt] 'stack smashing detected' in 1.2.18 (caused by virNetDevGFeatureAvailable)

[Brian Rak]

I recently compiled 1.2.18 to start testing with it, and was getting 
this error on startup:


*** stack smashing detected ***: libvirtd terminated
=== Backtrace: =
/lib64/libc.so.6(__fortify_fail+0x37)[0x7fe1ac631527]
/lib64/libc.so.6(__fortify_fail+0x0)[0x7fe1ac6314f0]
//lib/libvirt.so.0(+0xa7927)[0x7fe1aeda2927]
//lib/libvirt/connection-driver/libvirt_driver_nodedev.so(+0x947d)[0x7fe1958a047d]
//lib/libvirt/connection-driver/libvirt_driver_nodedev.so(+0xa6c2)[0x7fe1958a16c2]
//lib/libvirt/connection-driver/libvirt_driver_nodedev.so(+0xaf4e)[0x7fe1958a1f4e]
//lib/libvirt.so.0(virStateInitialize+0xb8)[0x7fe1aee6d0a8]
libvirtd(+0x15120)[0x7fe1afae6120]
//lib/libvirt.so.0(+0xd4975)[0x7fe1aedcf975]
/lib64/libpthread.so.0(+0x30316079d1)[0x7fe1ada8c9d1]
/lib64/libc.so.6(clone+0x6d)[0x7fe1ac6178fd]

(gdb) bt
#0  0x74a8f625 in raise () from /lib64/libc.so.6
#1  0x74a90e05 in abort () from /lib64/libc.so.6
#2  0x74acd537 in __libc_message () from /lib64/libc.so.6
#3  0x74b5f527 in __fortify_fail () from /lib64/libc.so.6
#4  0x74b5f4f0 in __stack_chk_fail () from /lib64/libc.so.6
#5  0x772d0927 in virNetDevGetFeatures (ifname=value optimized 
out, out=value optimized out) at util/virnetdev.c:3200
#6  0x7fffdddce47d in udevProcessNetworkInterface 
(device=0x7fffd4071f70, def=0x6) at node_device/node_device_udev.c:694
#7  udevGetDeviceDetails (device=0x7fffd4071f70, def=0x6) at 
node_device/node_device_udev.c:1272
#8  0x7fffdddcf6c2 in udevAddOneDevice (device=0x7fffd4071f70) at 
node_device/node_device_udev.c:1394
#9  0x7fffdddcff4e in udevProcessDeviceListEntry (privileged=value 
optimized out, callback=value optimized out, opaque=value optimized 
out)

at node_device/node_device_udev.c:1433
#10 udevEnumerateDevices (privileged=value optimized out, 
callback=value optimized out, opaque=value optimized out) at 
node_device/node_device_udev.c:1463
#11 nodeStateInitialize (privileged=value optimized out, 
callback=value optimized out, opaque=value optimized out) at 
node_device/node_device_udev.c:1773
#12 0x7739b0a8 in virStateInitialize (privileged=true, 
callback=0x55569070 daemonInhibitCallback, opaque=0x557f1db0) 
at libvirt.c:777
#13 0x55569120 in daemonRunStateInit (opaque=value optimized 
out) at libvirtd.c:947
#14 0x772fd975 in virThreadHelper (data=value optimized out) 
at util/virthread.c:206

#15 0x75fba9d1 in start_thread () from /lib64/libpthread.so.0
#16 0x74b458fd in clone () from /lib64/libc.so.6

In IRC, we tracked this down to this bit of code:

g_cmd.cmd = ETHTOOL_GFEATURES;
g_cmd.size = GFEATURES_SIZE;
if (virNetDevGFeatureAvailable(ifname, g_cmd))
ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_TXUDPTNL));

GFEATURES_SIZE is currently defined as 2, but this value needs to be 
higher in order to support newer kernels.  It looks like this code was 
added in ac3ed2085fcbeecaf5aa347c0b1bffaf94fff293


ethtool calculates this value based on the number of supported features: 
http://lxr.free-electrons.com/source/net/core/ethtool.c#L55


I don't know enough about this to properly fix this, but raising 
GFEATURES_SIZE to 3 has fixed this issue for me (though, this will 
obviously need to go higher as more features get added)


This crash was occurring on a CentOS 6 system, running a the ELRepo 
kernel-ml kernel.  The stock CentOS 6 kernel (2.6.32) does not appear to 
have sufficient features available to trigger this.


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

[libvirt] 'stack smashing detected' in 1.2.18 (caused by virNetDevGFeatureAvailable)

[Brian Rak]

I recently compiled 1.2.18 to start testing with it, and was getting 
this error on startup:


*** stack smashing detected ***: libvirtd terminated
=== Backtrace: =
/lib64/libc.so.6(__fortify_fail+0x37)[0x7fe1ac631527]
/lib64/libc.so.6(__fortify_fail+0x0)[0x7fe1ac6314f0]
//lib/libvirt.so.0(+0xa7927)[0x7fe1aeda2927]
//lib/libvirt/connection-driver/libvirt_driver_nodedev.so(+0x947d)[0x7fe1958a047d]
//lib/libvirt/connection-driver/libvirt_driver_nodedev.so(+0xa6c2)[0x7fe1958a16c2]
//lib/libvirt/connection-driver/libvirt_driver_nodedev.so(+0xaf4e)[0x7fe1958a1f4e]
//lib/libvirt.so.0(virStateInitialize+0xb8)[0x7fe1aee6d0a8]
libvirtd(+0x15120)[0x7fe1afae6120]
//lib/libvirt.so.0(+0xd4975)[0x7fe1aedcf975]
/lib64/libpthread.so.0(+0x30316079d1)[0x7fe1ada8c9d1]
/lib64/libc.so.6(clone+0x6d)[0x7fe1ac6178fd]

(gdb) bt
#0  0x74a8f625 in raise () from /lib64/libc.so.6
#1  0x74a90e05 in abort () from /lib64/libc.so.6
#2  0x74acd537 in __libc_message () from /lib64/libc.so.6
#3  0x74b5f527 in __fortify_fail () from /lib64/libc.so.6
#4  0x74b5f4f0 in __stack_chk_fail () from /lib64/libc.so.6
#5  0x772d0927 in virNetDevGetFeatures (ifname=value optimized 
out, out=value optimized out) at util/virnetdev.c:3200
#6  0x7fffdddce47d in udevProcessNetworkInterface 
(device=0x7fffd4071f70, def=0x6) at node_device/node_device_udev.c:694
#7  udevGetDeviceDetails (device=0x7fffd4071f70, def=0x6) at 
node_device/node_device_udev.c:1272
#8  0x7fffdddcf6c2 in udevAddOneDevice (device=0x7fffd4071f70) at 
node_device/node_device_udev.c:1394
#9  0x7fffdddcff4e in udevProcessDeviceListEntry (privileged=value 
optimized out, callback=value optimized out, opaque=value optimized 
out)

at node_device/node_device_udev.c:1433
#10 udevEnumerateDevices (privileged=value optimized out, 
callback=value optimized out, opaque=value optimized out) at 
node_device/node_device_udev.c:1463
#11 nodeStateInitialize (privileged=value optimized out, 
callback=value optimized out, opaque=value optimized out) at 
node_device/node_device_udev.c:1773
#12 0x7739b0a8 in virStateInitialize (privileged=true, 
callback=0x55569070 daemonInhibitCallback, opaque=0x557f1db0) 
at libvirt.c:777
#13 0x55569120 in daemonRunStateInit (opaque=value optimized 
out) at libvirtd.c:947
#14 0x772fd975 in virThreadHelper (data=value optimized out) 
at util/virthread.c:206

#15 0x75fba9d1 in start_thread () from /lib64/libpthread.so.0
#16 0x74b458fd in clone () from /lib64/libc.so.6

In IRC, we tracked this down to this bit of code:

g_cmd.cmd = ETHTOOL_GFEATURES;
g_cmd.size = GFEATURES_SIZE;
if (virNetDevGFeatureAvailable(ifname, g_cmd))
ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_TXUDPTNL));

GFEATURES_SIZE is currently defined as 2, but this value needs to be 
higher in order to support newer kernels.  It looks like this code was 
added in ac3ed2085fcbeecaf5aa347c0b1bffaf94fff293


ethtool calculates this value based on the number of supported features: 
http://lxr.free-electrons.com/source/net/core/ethtool.c#L55


I don't know enough about this to properly fix this, but raising 
GFEATURES_SIZE to 3 has fixed this issue for me (though, this will 
obviously need to go higher as more features get added)


This crash was occurring on a CentOS 6 system, running a the ELRepo 
kernel-ml kernel.  The stock CentOS 6 kernel (2.6.32) does not appear to 
have sufficient features available to trigger this.


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

Re: [libvirt] [PATCH 03/18] cpu: Add NULL check in ppc64ModelCopy()

[Andrea Bolognani]

On Wed, 2015-08-05 at 14:19 +0200, Jiri Denemark wrote:
 On Tue, Aug 04, 2015 at 11:37:54 +0200, Andrea Bolognani wrote:
  ---
   src/cpu/cpu_ppc64.c | 3 +++
   1 file changed, 3 insertions(+)
  
  diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c
  index 05ff8f2..dd02a3f 100644
  --- a/src/cpu/cpu_ppc64.c
  +++ b/src/cpu/cpu_ppc64.c
  @@ -115,6 +115,9 @@ ppc64ModelCopy(const struct ppc64_model *model)
   {
   struct ppc64_model *copy;
   
  +if (!model)
  +return NULL;
  +
   if (VIR_ALLOC(copy)  0 ||
   VIR_STRDUP(copy-name, model-name)  0) {
   ppc64ModelFree(copy);
 
 This doesn't seem to be really necessary since the function is not
 called with model == NULL and I don't think that should change. If 
 any
 caller wants to pass NULL for a model, it should rather report an 
 error
 and return instead of trying to copy this NULL.

I've dropped this commit and removed a similar check from
ppc64DataCopy(), which will be introduced later in the series.

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

Re: [libvirt] 'stack smashing detected' in 1.2.18 (caused by virNetDevGFeatureAvailable)

[Laine Stump]

On 08/05/2015 12:09 PM, Brian Rak wrote:
 I recently compiled 1.2.18 to start testing with it, and was getting
 this error on startup:

 *** stack smashing detected ***: libvirtd terminated
 === Backtrace: =
 /lib64/libc.so.6(__fortify_fail+0x37)[0x7fe1ac631527]
 /lib64/libc.so.6(__fortify_fail+0x0)[0x7fe1ac6314f0]
 //lib/libvirt.so.0(+0xa7927)[0x7fe1aeda2927]
 //lib/libvirt/connection-driver/libvirt_driver_nodedev.so(+0x947d)[0x7fe1958a047d]

 //lib/libvirt/connection-driver/libvirt_driver_nodedev.so(+0xa6c2)[0x7fe1958a16c2]

 //lib/libvirt/connection-driver/libvirt_driver_nodedev.so(+0xaf4e)[0x7fe1958a1f4e]

 //lib/libvirt.so.0(virStateInitialize+0xb8)[0x7fe1aee6d0a8]
 libvirtd(+0x15120)[0x7fe1afae6120]
 //lib/libvirt.so.0(+0xd4975)[0x7fe1aedcf975]
 /lib64/libpthread.so.0(+0x30316079d1)[0x7fe1ada8c9d1]
 /lib64/libc.so.6(clone+0x6d)[0x7fe1ac6178fd]

 (gdb) bt
 #0  0x74a8f625 in raise () from /lib64/libc.so.6
 #1  0x74a90e05 in abort () from /lib64/libc.so.6
 #2  0x74acd537 in __libc_message () from /lib64/libc.so.6
 #3  0x74b5f527 in __fortify_fail () from /lib64/libc.so.6
 #4  0x74b5f4f0 in __stack_chk_fail () from /lib64/libc.so.6
 #5  0x772d0927 in virNetDevGetFeatures (ifname=value
 optimized out, out=value optimized out) at util/virnetdev.c:3200
 #6  0x7fffdddce47d in udevProcessNetworkInterface
 (device=0x7fffd4071f70, def=0x6) at node_device/node_device_udev.c:694
 #7  udevGetDeviceDetails (device=0x7fffd4071f70, def=0x6) at
 node_device/node_device_udev.c:1272
 #8  0x7fffdddcf6c2 in udevAddOneDevice (device=0x7fffd4071f70) at
 node_device/node_device_udev.c:1394
 #9  0x7fffdddcff4e in udevProcessDeviceListEntry
 (privileged=value optimized out, callback=value optimized out,
 opaque=value optimized out)
 at node_device/node_device_udev.c:1433
 #10 udevEnumerateDevices (privileged=value optimized out,
 callback=value optimized out, opaque=value optimized out) at
 node_device/node_device_udev.c:1463
 #11 nodeStateInitialize (privileged=value optimized out,
 callback=value optimized out, opaque=value optimized out) at
 node_device/node_device_udev.c:1773
 #12 0x7739b0a8 in virStateInitialize (privileged=true,
 callback=0x55569070 daemonInhibitCallback,
 opaque=0x557f1db0) at libvirt.c:777
 #13 0x55569120 in daemonRunStateInit (opaque=value optimized
 out) at libvirtd.c:947
 #14 0x772fd975 in virThreadHelper (data=value optimized out)
 at util/virthread.c:206
 #15 0x75fba9d1 in start_thread () from /lib64/libpthread.so.0
 #16 0x74b458fd in clone () from /lib64/libc.so.6

 In IRC, we tracked this down to this bit of code:

 g_cmd.cmd = ETHTOOL_GFEATURES;
 g_cmd.size = GFEATURES_SIZE;
 if (virNetDevGFeatureAvailable(ifname, g_cmd))
 ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_TXUDPTNL));

 GFEATURES_SIZE is currently defined as 2, but this value needs to be
 higher in order to support newer kernels.  It looks like this code was
 added in ac3ed2085fcbeecaf5aa347c0b1bffaf94fff293

 ethtool calculates this value based on the number of supported
 features: http://lxr.free-electrons.com/source/net/core/ethtool.c#L55

 I don't know enough about this to properly fix this, but raising
 GFEATURES_SIZE to 3 has fixed this issue for me (though, this will
 obviously need to go higher as more features get added)

(in later IRC conversation, Brian noted that raising GFEATURES_SIZE
*didn't* always eliminate the issue...)


The problem goes beyond that:

1) as far as I can see, g_cmd.size needs to be set to the number of
items in the array g_cmd.feature, and we're setting it to 2
(GFEATURES_SIZE), but we have allocated space for exactly *0* items in
that array. If we're going to tell the kernel we have 2 items in the
array, we need to actually have that space available, or the kernel will
overwrite something else.

2) the feature we're looking for is called TX_UDP_TNL in libvirt, and
is manually #defined to be bit 25. From the title of the commit log for
the patch that added this code to libvirt, you can see what we want to
check for is the feature called tx-udp_tnl-segmentation, and if you
look at the ethtool.c source from the kernel that Brian has linked to
above, you'll see that the
netdev_features_strings[NETIF_F_GSO_UDP_TUNNEL_BIT] is initialized to
tx-upd_tnl-segmentation. When you look up NETIF_F_GSO_UDP_TUNNEL_BIT,
it seems to be *26* in the enum where it is defined:
http://osxr.org/linux/source/include/linux/netdev_features.h#0047

So are we checking for the wrong feature?

It would be really nice if we could avoid #defining magic values like
TX_UDP_TNL and GFEATURES_SIZE in our source. In my quick investigation I
couldn't see a way around that though (since NETIF_GSO_UDP_TUNNEL_BIT)
isn't available outside kernel source).


 This crash was occurring on a CentOS 6 system, running a the ELRepo
 kernel-ml kernel.  The stock CentOS 6 kernel (2.6.32) does not