Re: [libvirt] [PATCH] tools: fix the wrong check when use virsh setvcpus --maximum
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
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
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
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
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
-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
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
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