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

2015-03-22 Thread lhuang


On 03/20/2015 11:01 PM, Pavel Hrdina wrote:

On Fri, Mar 20, 2015 at 04:22:24PM +0800, Luyao Huang wrote:

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

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.

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

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 1d8225c..6ab7b05 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -6742,9 +6742,8 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
  flags |= VIR_DOMAIN_AFFECT_LIVE;
  if (guest)
  flags |= VIR_DOMAIN_VCPU_GUEST;
-/* none of the options were specified */
-if (!current  flags == 0)
-flags = -1;
+if (maximum)
+flags |= VIR_DOMAIN_VCPU_MAXIMUM;
  
  if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))

  return false;
@@ -6754,27 +6753,19 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
  goto cleanup;
  }
  
-if (flags == -1) {

+/* none of the options were specified */
+if (!current  flags == 0) {
  if (virDomainSetVcpus(dom, count) != 0)
  goto cleanup;
  } else {
-/* If the --maximum flag was given, we need to ensure only the
-   --config flag is in effect as well */
-if (maximum) {
-vshDebug(ctl, VSH_ERR_DEBUG, --maximum flag was given\n);
-
-flags |= VIR_DOMAIN_VCPU_MAXIMUM;
-
-/* If neither the --config nor --live flags were given, OR
-   if just the --live flag was given, we need to error out
-   warning the user that the --maximum flag can only be used
-   with the --config flag */
-if (live || !config) {
-
-/* Warn the user about the invalid flag combination */
-vshError(ctl, _(--maximum must be used with --config only));
-goto cleanup;
-}
+/* If neither the --config nor --live flags were given, OR
+   if just the --live flag was given, we need to error out
+   warning the user that the --maximum flag can only be used
+   with the --config flag */
+if (maximum  (live || !config)) {
+/* Warn the user about the invalid flag combination */
+vshError(ctl, _(--maximum must be used with --config only));
+goto cleanup;

Instead of this ugly code you could've used VSH_EXCLUSIVE_OPTIONS_VAR to set
that maximum and live are mutually exclusive.  I've updated your patch and send
it together with some other cleanups.

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


Okay, i have saw your patches, it is a good idea and will make codes 
clean, thanks for your review.



Pavel




Luyao

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


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

2015-03-22 Thread lhuang


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

On Fri, Mar 20, 2015 at 03:07:09PM +0800, Luyao Huang wrote:

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

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.

Signed-off-by: Luyao Huang lhu...@redhat.com
---
  src/qemu/qemu_driver.c | 6 ++
  1 file changed, 6 insertions(+)


I've updated the patch to error out for every case we are trying to update live
domain either by using VIR_DOMAIN_AFFECT_LIVE or VIR_DOMAIN_AFFECT_CURRENT.

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


Good, thanks for your help and review.


Pavel


Luyao

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


Re: [libvirt] [PATCH v2] util: don't fail if no PortData is found while getting migrateData

2015-03-22 Thread zhang bo
Thanks for your reply, this patch has already been commited by John on Mar.14, 
the commit ID is: 25df57db73adc3e610193ee1fcdd202c47ba471d
Thank you all the same:)

On 2015/3/21 0:10, Michal Privoznik wrote:

 On 04.03.2015 03:59, zhang bo wrote:

 
 I've tried to review this patch. However it is corrupt.
 
 ---
 V1 here:
  https://www.redhat.com/archives/libvir-list/2015-February/msg00388.html
 V2:
Add --if-exists option to ovs-vsctl cmd,  making ovs-vsctl not raise 
 error if there's no portData
available. Suggested by Martin.
We Tested the patch, it works.
 ---
 
 I guess this ^^ is the problem.  We put notes ...
 

 Introduced by f6a2f97e

 Problem description:
 After multiple times of migrating a domain, which has an ovs interface with 
 no portData set,
 with non-shared disk, nbd ports got overflowed.

 The steps to reproduce the problem:
 1 define and start a domain with its network configured as:
 interface type='bridge'
   source bridge='br0'/
   virtualport type='openvswitch'
   /virtualport
   model type='virtio'/
   driver name='vhost' queues='4'/
 /interface
 2 do not set the network's portData.
 3 migrate(ToURI2) it with flag 91(1011011), which means:
   VIR_MIGRATE_LIVE
   VIR_MIGRATE_PEER2PEER
   VIR_MIGRATE_PERSIST_DEST
   VIR_MIGRATE_UNDEFINE_SOURCE
   VIR_MIGRATE_NON_SHARED_DISK
 4 migrate success, but we got an error log in libvirtd.log:
   error : virCommandWait:2423 : internal error: Child process (ovs-vsctl 
 --timeout=5 get Interface
   vnet1 external_ids:PortData) unexpected exit status 1: ovs-vsctl: no key 
 PortData in Interface
   record vnet1 column external_ids
 5 migrate it back, migrate it , migrate it back, ...
 6 nbd port got overflowed.

 The reasons for the problem is :
 1 virNetDevOpenvswitchGetMigrateData() takes it as wrong if no portData is 
 available for  the ovs
  interface of a domain. (We think it's not appropriate, as portData is just 
 OPTIONAL)
 2 in func qemuMigrationBakeCookie(), it fails in 
 qemuMigrationCookieAddNetwork(), and returns with -1.
  qemuMigrationCookieAddNBD() is not called thereafter, and mig-nbd is still 
 NULL.
 3 However, qemuMigrationRun() just *WARN* if qemuMigrationBakeCookie() 
 fails, migration still successes.
  cookie is NULL, it's not baked on the src side.
 4 On the destination side, it would alloc a port first and then free the nbd 
 port in COOKIE.
  But the cookie is NULL due to qemuMigrationCookieAddNetwork() failure at 
 src side. thus the nbd port
  is not freed.

 In this patch, we add --if-exists option to make ovs-vsctl not raise error 
 if there's no portData available.
 Further more, because portData may be NULL in the cookie at the dest side, 
 check it before setting portData.


 Signed-off-by: Zhou Yimin zhouyi...@huawei.com
 Signed-off-by: Zhang Bo oscar.zhan...@huawei.com
 ---
 
 ... here. Afte this line. However, removing the block did not help. But
 editing a patch directly has very low success rate anyway. Can you
 please resend?
 
  src/util/virnetdevopenvswitch.c | 11 ++-
  1 file changed, 10 insertions(+), 1 deletion(-)

 
 Michal
 
 .
 



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


[libvirt] [PATCH 1/2] Revert LXC: create a bind mount for sysfs when enable userns but disable netns

2015-03-22 Thread Chen Hanxiao
This reverts commit a86b6215a74b1feb2667204e214fbfd2f7decc5c.

Discussed at:
http://www.redhat.com/archives/libvir-list/2015-March/msg01023.html

Signed-off-by: Chen Hanxiao chenhanx...@cn.fujitsu.com

Conflicts:
src/lxc/lxc_container.c
---
 src/lxc/lxc_container.c | 43 ++-
 1 file changed, 10 insertions(+), 33 deletions(-)

diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index cc20b6d..e34968a 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -934,8 +934,6 @@ static int lxcContainerMountBasicFS(bool userns_enabled,
 {
 size_t i;
 int rc = -1;
-char* mnt_src = NULL;
-int mnt_mflags;
 
 VIR_DEBUG(Mounting basic filesystems);
 
@@ -943,23 +941,8 @@ static int lxcContainerMountBasicFS(bool userns_enabled,
 bool bindOverReadonly;
 virLXCBasicMountInfo const *mnt = lxcBasicMounts[i];
 
-/* When enable userns but disable netns, kernel will
- * forbid us doing a new fresh mount for sysfs.
- * So we had to do a bind mount for sysfs instead.
- */
-if (userns_enabled  netns_disabled 
-STREQ(mnt-src, sysfs)) {
-if (VIR_STRDUP(mnt_src, /sys)  0)
-goto cleanup;
-mnt_mflags = MS_NOSUID|MS_NOEXEC|MS_NODEV|MS_RDONLY|MS_BIND;
-} else {
-if (VIR_STRDUP(mnt_src, mnt-src)  0)
-goto cleanup;
-mnt_mflags = mnt-mflags;
-}
-
 VIR_DEBUG(Processing %s - %s,
-  mnt_src, mnt-dst);
+  mnt-src, mnt-dst);
 
 if (mnt-skipUnmounted) {
 char *hostdir;
@@ -976,28 +959,24 @@ static int lxcContainerMountBasicFS(bool userns_enabled,
 if (ret == 0) {
 VIR_DEBUG(Skipping '%s' which isn't mounted in host,
   mnt-dst);
-VIR_FREE(mnt_src);
 continue;
 }
 }
 
 if (mnt-skipUserNS  userns_enabled) {
 VIR_DEBUG(Skipping due to user ns enablement);
-VIR_FREE(mnt_src);
 continue;
 }
 
 /* Skip mounts with missing source without shouting: it may be a
  * missing folder in /proc due to the absence of a kernel feature */
-if (STRPREFIX(mnt_src, /)  !virFileExists(mnt_src)) {
-VIR_DEBUG(Skipping due to missing source: %s, mnt_src);
-VIR_FREE(mnt_src);
+if (STRPREFIX(mnt-src, /)  !virFileExists(mnt-src)) {
+VIR_DEBUG(Skipping due to missing source: %s, mnt-src);
 continue;
 }
 
 if (mnt-skipNoNetns  netns_disabled) {
 VIR_DEBUG(Skipping due to absence of network namespace);
-VIR_FREE(mnt_src);
 continue;
 }
 
@@ -1015,35 +994,33 @@ static int lxcContainerMountBasicFS(bool userns_enabled,
  * we mount the filesystem in read-write mode initially, and then do a
  * separate read-only bind mount on top of that.
  */
-bindOverReadonly = !!(mnt_mflags  MS_RDONLY);
+bindOverReadonly = !!(mnt-mflags  MS_RDONLY);
 
 VIR_DEBUG(Mount %s on %s type=%s flags=%x,
-  mnt_src, mnt-dst, mnt-type, mnt_mflags  ~MS_RDONLY);
-if (mount(mnt_src, mnt-dst, mnt-type, mnt_mflags  ~MS_RDONLY, NULL) 
 0) {
+  mnt-src, mnt-dst, mnt-type, mnt-mflags  ~MS_RDONLY);
+if (mount(mnt-src, mnt-dst, mnt-type, mnt-mflags  ~MS_RDONLY, 
NULL)  0) {
 virReportSystemError(errno,
  _(Failed to mount %s on %s type %s 
flags=%x),
- mnt_src, mnt-dst, NULLSTR(mnt-type),
- mnt_mflags  ~MS_RDONLY);
+ mnt-src, mnt-dst, NULLSTR(mnt-type),
+ mnt-mflags  ~MS_RDONLY);
 goto cleanup;
 }
 
 if (bindOverReadonly 
-mount(mnt_src, mnt-dst, NULL,
+mount(mnt-src, mnt-dst, NULL,
   MS_BIND|MS_REMOUNT|MS_RDONLY, NULL)  0) {
 virReportSystemError(errno,
  _(Failed to re-mount %s on %s flags=%x),
- mnt_src, mnt-dst,
+ mnt-src, mnt-dst,
  MS_BIND|MS_REMOUNT|MS_RDONLY);
 goto cleanup;
 }
 
-VIR_FREE(mnt_src);
 }
 
 rc = 0;
 
  cleanup:
-VIR_FREE(mnt_src);
 VIR_DEBUG(rc=%d, rc);
 return rc;
 }
-- 
2.1.0

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


[libvirt] [PATCH 2/2] LXC: make sure netns been enabled when trying to enable userns

2015-03-22 Thread Chen Hanxiao
Discussed at:
http://www.redhat.com/archives/libvir-list/2015-March/msg01023.html

Signed-off-by: Chen Hanxiao chenhanx...@cn.fujitsu.com
---
 src/lxc/lxc_container.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index e34968a..69a8f2f 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -941,6 +941,16 @@ static int lxcContainerMountBasicFS(bool userns_enabled,
 bool bindOverReadonly;
 virLXCBasicMountInfo const *mnt = lxcBasicMounts[i];
 
+/* When enable userns but disable netns, kernel will
+ * forbid us doing a new fresh mount for sysfs for security reason.
+ * So we should not allow this.
+ */
+if (userns_enabled  netns_disabled) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
+   _(Userns could not be enabled without netns));
+goto cleanup;
+}
+
 VIR_DEBUG(Processing %s - %s,
   mnt-src, mnt-dst);
 
-- 
2.1.0

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


Re: [libvirt] [PATCH] LXC: make sure netns been enabled when trying to enable userns

2015-03-22 Thread Chen, Hanxiao


 -Original Message-
 From: Martin Kletzander [mailto:mklet...@redhat.com]
 Sent: Friday, March 20, 2015 6:31 PM
 To: Chen, Hanxiao/陈 晗霄
 Cc: libvir-list@redhat.com
 Subject: Re: [libvirt] [PATCH] LXC: make sure netns been enabled when trying 
 to
 enable userns
 
 On Fri, Mar 20, 2015 at 05:58:22AM -0400, Chen Hanxiao wrote:
 This patch revert commit:
 7dc5dbc879bd0779924b5132a48b731a0bc04a1e
 
 
 I haven't found this commit in the log, also this is not how reverts
 should be done, please use git revert commit_id, it basically
 cherry-picks inverted version of that patch, so resolutions may be
 done for conflicts, and it will let you amend the commit messsage.
 

I put a wrong commit ID, sorry for that.
I'll resend this by using 'git revert'
Thanks for your help.

Regards,
- Chen


 Discussed at:
 http://www.redhat.com/archives/libvir-list/2015-March/msg01023.html
 
 Signed-off-by: Chen Hanxiao chenhanx...@cn.fujitsu.com
 ---
  src/lxc/lxc_container.c | 45 -
  1 file changed, 16 insertions(+), 29 deletions(-)
 
 diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
 index cc20b6d..69a8f2f 100644
 --- a/src/lxc/lxc_container.c
 +++ b/src/lxc/lxc_container.c
 @@ -934,8 +934,6 @@ static int lxcContainerMountBasicFS(bool userns_enabled,
  {
  size_t i;
  int rc = -1;
 -char* mnt_src = NULL;
 -int mnt_mflags;
 
  VIR_DEBUG(Mounting basic filesystems);
 
 @@ -944,22 +942,17 @@ static int lxcContainerMountBasicFS(bool 
 userns_enabled,
  virLXCBasicMountInfo const *mnt = lxcBasicMounts[i];
 
  /* When enable userns but disable netns, kernel will
 - * forbid us doing a new fresh mount for sysfs.
 - * So we had to do a bind mount for sysfs instead.
 + * forbid us doing a new fresh mount for sysfs for security reason.
 + * So we should not allow this.
   */
 -if (userns_enabled  netns_disabled 
 -STREQ(mnt-src, sysfs)) {
 -if (VIR_STRDUP(mnt_src, /sys)  0)
 -goto cleanup;
 -mnt_mflags = MS_NOSUID|MS_NOEXEC|MS_NODEV|MS_RDONLY|MS_BIND;
 -} else {
 -if (VIR_STRDUP(mnt_src, mnt-src)  0)
 -goto cleanup;
 -mnt_mflags = mnt-mflags;
 +if (userns_enabled  netns_disabled) {
 +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, %s,
 +   _(Userns could not be enabled without netns));
 +goto cleanup;
  }
 
  VIR_DEBUG(Processing %s - %s,
 -  mnt_src, mnt-dst);
 +  mnt-src, mnt-dst);
 
  if (mnt-skipUnmounted) {
  char *hostdir;
 @@ -976,28 +969,24 @@ static int lxcContainerMountBasicFS(bool 
 userns_enabled,
  if (ret == 0) {
  VIR_DEBUG(Skipping '%s' which isn't mounted in host,
mnt-dst);
 -VIR_FREE(mnt_src);
  continue;
  }
  }
 
  if (mnt-skipUserNS  userns_enabled) {
  VIR_DEBUG(Skipping due to user ns enablement);
 -VIR_FREE(mnt_src);
  continue;
  }
 
  /* Skip mounts with missing source without shouting: it may be a
   * missing folder in /proc due to the absence of a kernel feature */
 -if (STRPREFIX(mnt_src, /)  !virFileExists(mnt_src)) {
 -VIR_DEBUG(Skipping due to missing source: %s, mnt_src);
 -VIR_FREE(mnt_src);
 +if (STRPREFIX(mnt-src, /)  !virFileExists(mnt-src)) {
 +VIR_DEBUG(Skipping due to missing source: %s, mnt-src);
  continue;
  }
 
  if (mnt-skipNoNetns  netns_disabled) {
  VIR_DEBUG(Skipping due to absence of network namespace);
 -VIR_FREE(mnt_src);
  continue;
  }
 
 @@ -1015,35 +1004,33 @@ static int lxcContainerMountBasicFS(bool 
 userns_enabled,
   * we mount the filesystem in read-write mode initially, and then 
  do a
   * separate read-only bind mount on top of that.
   */
 -bindOverReadonly = !!(mnt_mflags  MS_RDONLY);
 +bindOverReadonly = !!(mnt-mflags  MS_RDONLY);
 
  VIR_DEBUG(Mount %s on %s type=%s flags=%x,
 -  mnt_src, mnt-dst, mnt-type, mnt_mflags  ~MS_RDONLY);
 -if (mount(mnt_src, mnt-dst, mnt-type, mnt_mflags  ~MS_RDONLY, 
 NULL)
  0) {
 +  mnt-src, mnt-dst, mnt-type, mnt-mflags  ~MS_RDONLY);
 +if (mount(mnt-src, mnt-dst, mnt-type, mnt-mflags  ~MS_RDONLY, 
 NULL)
  0) {
  virReportSystemError(errno,
   _(Failed to mount %s on %s type %s 
  flags=%x),
 - mnt_src, mnt-dst, NULLSTR(mnt-type),
 - mnt_mflags  ~MS_RDONLY);
 + mnt-src, mnt-dst, NULLSTR(mnt-type),
 + mnt-mflags  

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

2015-03-22 Thread Yanbing Du



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

From: Luyao Huang lhu...@redhat.com

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 lhu...@redhat.com
Signed-off-by: Pavel Hrdina phrd...@redhat.com
---
  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);


'maximum' should be used with 'config', and 'live' and 'maximum' are 
mutually exclusive




  if (config)
  flags |= VIR_DOMAIN_AFFECT_CONFIG;
@@ -6742,9 +6744,8 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
  flags |= VIR_DOMAIN_AFFECT_LIVE;
  if (guest)
  flags |= VIR_DOMAIN_VCPU_GUEST;
-/* none of the options were specified */
-if (!current  flags == 0)
-flags = -1;
+if (maximum)
+flags |= VIR_DOMAIN_VCPU_MAXIMUM;

  if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
  return false;
@@ -6754,30 +6755,11 @@ cmdSetvcpus(vshControl *ctl, const vshCmd *cmd)
  goto cleanup;
  }

-if (flags == -1) {
+/* none of the options were specified */
+if (!current  flags == 0) {
  if (virDomainSetVcpus(dom, count) != 0)
  goto cleanup;
  } else {
-/* If the --maximum flag was given, we need to ensure only the
-   --config flag is in effect as well */
-if (maximum) {
-vshDebug(ctl, VSH_ERR_DEBUG, --maximum flag was given\n);
-
-flags |= VIR_DOMAIN_VCPU_MAXIMUM;
-
-/* If neither the --config nor --live flags were given, OR
-   if just the --live flag was given, we need to error out
-   warning the user that the --maximum flag can only be used
-   with the --config flag */
-if (live || !config) {
-
-/* Warn the user about the invalid flag combination */
-vshError(ctl, _(--maximum must be used with --config only));
-goto cleanup;
-}
-}
-
-/* Apply the virtual cpu changes */
  if (virDomainSetVcpusFlags(dom, count, flags)  0)
  goto cleanup;
  }



--
Regards,
Yanbing Du

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


[libvirt] [PATCH 0/2] fail out if enable userns but disable netns

2015-03-22 Thread Chen Hanxiao

Chen Hanxiao (2):
  Revert LXC: create a bind mount for sysfs when enable userns but
disable netns
  LXC: make sure netns been enabled when trying to enable userns

 src/lxc/lxc_container.c | 45 -
 1 file changed, 16 insertions(+), 29 deletions(-)

-- 
2.1.0

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