On 05/21/2013 04:31 PM, Guannan Ren wrote:
>
> v3->v4
> add missing .args, .xml files
> (I raw it in git Untracked files this morning and
>thought about it for a while, then git-cleaned them...)
> rebase work.
>
> v2->v3
> rebase work.
>
> v1->v2:
> changed attribute name from 'policy' to 'sharePolicy'
> renamed caps flag name: QEMU_CAPS_VNC_SHARE_POLICY
> fixed issues pointed out in v1 review
> have to keep hard-coded version probe after checking qemu -help and qmp
> command.
>
> As there were some vnc patches recently, I chose do rebase work a little
> later.
>
> These patches try to add a new attribute 'sharePolicy' to element graphics of
> vnc type. This attribute has three values:
> allow-exclusive,force-shared and ignore.
>
ACK series.
Jan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 21/05/13 23:40, Eric Blake wrote:
On 05/21/2013 04:01 AM, Osier Yang wrote:
Only a few cases are allowed:
1) The expression is empty for "for" loop, E.g.
for (i = 0; ; i++)
2) An empty statement
while (write(statuswrite, &status, 1) == -1 &&
errno == EINTR)
; /* empty */
3) ";" is inside double-quote, I.e, as part of const string. E.g.
vshPrint(ctl, "a ; b ; cd;\n");
The "for" loop in src/rpc/virnettlscontext.c is the special case,
1) applies for it, so change it together in this patch.
---
build-aux/bracket-spacing.pl | 18 ++
src/rpc/virnettlscontext.c | 2 +-
2 files changed, 19 insertions(+), 1 deletion(-)
ACK.
Pushed.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 21/05/13 23:41, Ján Tomko wrote:
s/nwfitler/nwfilter/ in the subject
Thanks, pushed with the fix.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Since neither getpwuid_r() nor initgroups() are safe to call in
between fork and exec (they obtain a mutex, but if some other
thread in the parent also held the mutex at the time of the fork,
the child will deadlock), we have to split out the functionality
that is unsafe. This patch adds a nice wrapper around
getgrouplist, which does the lookup by uid instead of name and
which mallocs the result; at least glibc's initgroups() uses
getgrouplist under the hood.
* configure.ac (AC_CHECK_FUNCS_ONCE): Check for getgrouplist.
* src/util/virutil.h (virGetGroupList): New declaration.
* src/util/virutil.c (virGetGroupList): New function.
* src/libvirt_private.syms (virutil.h): Export it.
Signed-off-by: Eric Blake
---
configure.ac | 4 +--
src/libvirt_private.syms | 1 +
src/util/virutil.c | 93
src/util/virutil.h | 2 ++
4 files changed, 98 insertions(+), 2 deletions(-)
diff --git a/configure.ac b/configure.ac
index 2055637..d20b90e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -205,8 +205,8 @@ AC_CHECK_SIZEOF([long])
dnl Availability of various common functions (non-fatal if missing),
dnl and various less common threadsafe functions
-AC_CHECK_FUNCS_ONCE([cfmakeraw geteuid getgid getgrnam_r getmntent_r \
- getpwuid_r getuid initgroups kill mmap newlocale posix_fallocate \
+AC_CHECK_FUNCS_ONCE([cfmakeraw geteuid getgid getgrnam_r getgrouplist \
+ getmntent_r getpwuid_r getuid initgroups kill mmap newlocale posix_fallocate
\
posix_memalign prlimit regexec sched_getaffinity setns setrlimit symlink])
dnl Availability of pthread functions (if missing, win32 threading is
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a81a865..3d9d89f 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1933,6 +1933,7 @@ virGetDeviceID;
virGetDeviceUnprivSGIO;
virGetFCHostNameByWWN;
virGetGroupID;
+virGetGroupList;
virGetGroupName;
virGetHostname;
virGetUnprivSGIOSysfsPath;
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 239fba8..a3cb64f 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -993,6 +993,99 @@ virGetGroupID(const char *group, gid_t *gid)
return 0;
}
+
+/* Compute the list of supplementary groups associated with @uid, and
+ * including @gid in the list (unless it is -1), storing a malloc'd
+ * result into @list. Return the size of the list on success, or -1
+ * on failure with error reported and errno set. May not be called
+ * between fork and exec. */
+int
+virGetGroupList(uid_t uid, gid_t gid, gid_t **list)
+{
+char *buf = NULL;
+gid_t *groups = NULL;
+int ngroups = 0;
+int ret = -1;
+
+*list = NULL;
+if (uid == (uid_t)-1)
+return 0;
+
+# ifdef HAVE_GETGROUPLIST
+{
+struct passwd pwd, *pwd_result;
+size_t bufsize;
+int rc;
+
+bufsize = sysconf(_SC_GETPW_R_SIZE_MAX);
+if (bufsize == -1)
+bufsize = 16384;
+
+if (VIR_ALLOC_N(buf, bufsize) < 0) {
+virReportOOMError();
+goto error;
+}
+while ((rc = getpwuid_r(uid, &pwd, buf, bufsize,
+&pwd_result)) == ERANGE) {
+if (VIR_RESIZE_N(buf, bufsize, bufsize, bufsize) < 0) {
+virReportOOMError();
+goto error;
+}
+}
+
+if (rc) {
+virReportSystemError(rc, _("cannot getpwuid_r(%u)"),
+ (unsigned int) uid);
+goto error;
+}
+
+if (!pwd_result) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("getpwuid_r failed to retrieve data "
+ "for uid '%u'"),
+ (unsigned int) uid);
+goto error;
+}
+
+if (gid == (gid_t)-1)
+gid = pwd.pw_gid;
+
+/* Figure out what size list to expect */
+getgrouplist(pwd.pw_name, gid, groups, &ngroups);
+
+if (VIR_ALLOC_N(groups, ngroups) < 0) {
+virReportOOMError();
+goto error;
+}
+
+if (getgrouplist(pwd.pw_name, gid, groups, &ngroups) < 0) {
+virReportSystemError(errno,
+ _("cannot getgrouplist(\"%s\", %d)"),
+ pwd.pw_name, (unsigned int) gid);
+goto error;
+}
+}
+# endif
+
+if (!ngroups && gid != (gid_t)-1) {
+if (VIR_ALLOC_N(groups, 1) < 0) {
+virReportOOMError();
+goto error;
+}
+groups[ngroups++] = gid;
+}
+
+*list = groups;
+groups = NULL;
+ret = ngroups;
+
+error:
+VIR_FREE(buf);
+VIR_FREE(groups);
+return ret;
+}
+
+
/* Set the real and effective uid and gid to the given values, and call
* initgroups so that the process has all the assumed group membership of
* that uid. return 0 on success, -1 on failure (the origin
https://bugzilla.redhat.com/show_bug.cgi?id=964358
POSIX states that multi-threaded apps should not use functions
that are not async-signal-safe between fork and exec, yet we
were using getpwuid_r and initgroups. Although rare, it is
possible to hit deadlock in the child, when it tries to grab
a mutex that was already held by another thread in the parent.
I actually hit this deadlock when testing multiple domains
being started in parallel with a command hook, with the following
backtrace in the child:
Thread 1 (Thread 0x7fd56bbf2700 (LWP 3212)):
#0 __lll_lock_wait ()
at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:136
#1 0x7fd5761e7388 in _L_lock_854 () from /lib64/libpthread.so.0
#2 0x7fd5761e7257 in __pthread_mutex_lock (mutex=0x7fd56be00360)
at pthread_mutex_lock.c:61
#3 0x7fd56bbf9fc5 in _nss_files_getpwuid_r (uid=0, result=0x7fd56bbf0c70,
buffer=0x7fd55c2a65f0 "", buflen=1024, errnop=0x7fd56bbf25b8)
at nss_files/files-pwd.c:40
#4 0x7fd575aeff1d in __getpwuid_r (uid=0, resbuf=0x7fd56bbf0c70,
buffer=0x7fd55c2a65f0 "", buflen=1024, result=0x7fd56bbf0cb0)
at ../nss/getXXbyYY_r.c:253
#5 0x7fd578aebafc in virSetUIDGID (uid=0, gid=0) at util/virutil.c:1031
#6 0x7fd578aebf43 in virSetUIDGIDWithCaps (uid=0, gid=0, capBits=0,
clearExistingCaps=true) at util/virutil.c:1388
#7 0x7fd578a9a20b in virExec (cmd=0x7fd55c231f10) at util/vircommand.c:654
#8 0x7fd578a9dfa2 in virCommandRunAsync (cmd=0x7fd55c231f10, pid=0x0)
at util/vircommand.c:2247
#9 0x7fd578a9d74e in virCommandRun (cmd=0x7fd55c231f10, exitstatus=0x0)
at util/vircommand.c:2100
#10 0x7fd56326fde5 in qemuProcessStart (conn=0x7fd53c000df0,
driver=0x7fd55c0dc4f0, vm=0x7fd54800b100, migrateFrom=0x0, stdin_fd=-1,
stdin_path=0x0, snapshot=0x0, vmop=VIR_NETDEV_VPORT_PROFILE_OP_CREATE,
flags=1) at qemu/qemu_process.c:3694
...
The solution is to split the work of getpwuid_r/initgroups into the
unsafe portions (getgrouplist, called pre-fork) and safe portions
(setgroups, called post-fork).
* src/util/virutil.h (virSetUIDGID, virSetUIDGIDWithCaps): Adjust
signature.
* src/util/virutil.c (virSetUIDGID): Add parameters.
(virSetUIDGIDWithCaps): Adjust clients.
* src/util/vircommand.c (virExec): Likewise.
* src/util/virfile.c (virFileAccessibleAs, virFileOpenForked)
(virDirCreate): Likewise.
* src/security/security_dac.c (virSecurityDACSetProcessLabel):
Likewise.
* configure.ac (AC_CHECK_FUNCS_ONCE): Check for setgroups, not
initgroups.
Signed-off-by: Eric Blake
---
configure.ac| 5 +-
src/security/security_dac.c | 16 +--
src/util/vircommand.c | 10 +++-
src/util/virfile.c | 30 ++--
src/util/virutil.c | 108
src/util/virutil.h | 5 +-
6 files changed, 84 insertions(+), 90 deletions(-)
diff --git a/configure.ac b/configure.ac
index d20b90e..26deac7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -206,8 +206,9 @@ AC_CHECK_SIZEOF([long])
dnl Availability of various common functions (non-fatal if missing),
dnl and various less common threadsafe functions
AC_CHECK_FUNCS_ONCE([cfmakeraw geteuid getgid getgrnam_r getgrouplist \
- getmntent_r getpwuid_r getuid initgroups kill mmap newlocale posix_fallocate
\
- posix_memalign prlimit regexec sched_getaffinity setns setrlimit symlink])
+ getmntent_r getpwuid_r getuid kill mmap newlocale posix_fallocate \
+ posix_memalign prlimit regexec sched_getaffinity setgroups setns \
+ setrlimit symlink])
dnl Availability of pthread functions (if missing, win32 threading is
dnl assumed). Because of $LIB_PTHREAD, we cannot use AC_CHECK_FUNCS_ONCE.
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index d922ad2..907dbd5 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1000,17 +1000,25 @@ virSecurityDACSetProcessLabel(virSecurityManagerPtr mgr,
uid_t user;
gid_t group;
virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
+gid_t *groups;
+int ngroups;
+int ret = -1;
if (virSecurityDACGetIds(def, priv, &user, &group))
return -1;
+ngroups = virGetGroupList(user, group, &groups);
+if (ngroups < 0)
+return -1;
VIR_DEBUG("Dropping privileges of DEF to %u:%u",
(unsigned int) user, (unsigned int) group);
-if (virSetUIDGID(user, group) < 0)
-return -1;
-
-return 0;
+if (virSetUIDGID(user, group, groups, ngroups) < 0)
+goto cleanup;
+ret = 0;
+cleanup:
+VIR_FREE(groups);
+return ret;
}
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index 665f549..ba03802 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -402,6 +402,8 @@ virExec(virCommandPtr cmd)
const char *binary = NULL;
int forkRet, ret;
struct sigaction waxon, waxoff;
+gid_t *groups = NULL;
+int ngr
https://bugzilla.redhat.com/show_bug.cgi?id=964358
Posting now to get reviews started. I'd especially like to get
feedback that it doesn't break LXC, and that it works with root-squash
NFS when using qemu:qemu instead of root:root in /etc/libvirt/qemu.conf.
I hope to do more testing myself, and also see if I can try writing an
LD_PRELOAD shim as part of 'make check' to make it easier to test that
the right system calls are made during the sequence.
Eric Blake (2):
util: add virGetGroupList
util: make virSetUIDGID async-signal-safe
configure.ac| 7 +--
src/libvirt_private.syms| 1 +
src/security/security_dac.c | 16 --
src/util/vircommand.c | 10 +++-
src/util/virfile.c | 30 +--
src/util/virutil.c | 123 ++--
src/util/virutil.h | 7 ++-
7 files changed, 142 insertions(+), 52 deletions(-)
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 05/21/2013 10:51 PM, Eric Blake wrote:
On 05/21/2013 07:29 AM, Guannan Ren wrote:
virConnectListAllInterfaces should support to list all of
interfaces when the value of flags is 0. The behaviour is
consistent with other virConnectListAll* APIs
---
src/conf/interface_conf.h | 4
src/interface/interface_backend_netcf.c | 29 +++--
src/interface/interface_backend_udev.c | 25 -
3 files changed, 31 insertions(+), 27 deletions(-)
ACK.
Thanks, pushed.
Guannan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
"Daniel P. Berrange" writes:
> On Tue, May 21, 2013 at 11:12:26AM -0600, Eric Blake wrote:
>> I have also argued in the past that it would be useful for libvirt to
>> support the idea of a template, where you can specify a domain XML that
>> inherits defaults from the template. We've already done things like
>> this for networking, nwfilter, and even secret management (in domain
>> XML, you declare that you are using a named network object, and that
>> network object serves as the template instead of you having to hard-code
>> all the elements into your domain XML), so we have a design to base it
>> on. But until someone adds such a feature for libvirt, then OpenStack
>> should be passing explicit XML to libvirt, and tracking defaults at the
>> OpenStack layer.
>
> I don't think the idea of a template belongs in libvirt.
This is fine. But the conversation started with a statement that it's
QEMU's job to define reasonable defaults and libvirt just exposes
those.
But in QEMU, we punt this problem by letting a user globally override
this default. libvirt hides this ability from the end user.
So either it's libvirt's problem to solve, or you should expose the
ability to set the global setting within QEMU. We can't just point our
fingers at each other and hope the problem goes away :-)
Regards,
Anthony Liguori
> Creating basic
> XML structure with relevant defaults pre-filled for a particular usecase
> is something that the libvirt-designer library is aiming to take care of
> for applications.
>
> 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
On Tue, May 21, 2013 at 11:12:26AM -0600, Eric Blake wrote:
> On 05/21/2013 10:42 AM, Anthony Liguori wrote:
> > Perhaps the right thing to do for OpenStack is to allow for a user
> > specified configuration file to select things like the default hardware
> > models/machine types? Then this could become node configuration instead
> > of dynamic configuration.
> >
> > I think it could be useful for general users too. Every domain requires
> > a lot of the same boiler plate bits. I think a lot of configurations
> > would benefit from being able to set global domain options.
>
> I have also argued in the past that it would be useful for libvirt to
> support the idea of a template, where you can specify a domain XML that
> inherits defaults from the template. We've already done things like
> this for networking, nwfilter, and even secret management (in domain
> XML, you declare that you are using a named network object, and that
> network object serves as the template instead of you having to hard-code
> all the elements into your domain XML), so we have a design to base it
> on. But until someone adds such a feature for libvirt, then OpenStack
> should be passing explicit XML to libvirt, and tracking defaults at the
> OpenStack layer.
I don't think the idea of a template belongs in libvirt. Creating basic
XML structure with relevant defaults pre-filled for a particular usecase
is something that the libvirt-designer library is aiming to take care of
for applications.
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
On 05/21/2013 03:14 AM, Michal Privoznik wrote:
>>> @@ -392,17 +392,14 @@ virCapabilitiesAddGuest(virCapsPtr caps,
>>> if (VIR_ALLOC(guest) < 0)
>>> goto no_memory;
>>>
>>> -if ((guest->ostype = strdup(ostype)) == NULL)
>>> +if (VIR_STRDUP(guest->ostype, ostype) < 0)
>>> goto no_memory;
>>
>> Local double-oom. You might want to clean this one up now.
>
> You mean s/no_memory/error/ ? Because even if the label is called
> no_memory not every label does call virReportOOMError(), like in this case.
Ah, now that I look at the label, you're right!
Yes, s/no_memory/error/ would avoid my confusion (generally, I've been
assuming that a no_memory label implied an OOM report).
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 05/21/2013 03:43 AM, Daniel P. Berrange wrote:
> On Mon, May 20, 2013 at 09:07:17PM -0600, Eric Blake wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=965169 documents a
>> problem starting domains when cgroups are enabled; I was able
>> to reliably reproduce the race about 5% of the time when I added
>> hooks to domain startup by 3 seconds (as that seemed to be about
>> the length of time that qemu created and then closed a temporary
>> thread, probably related to aio handling of initially opening
>> a disk image).
>>
>> * src/util/vircgroup.c (virCgroupAddTaskStrController): Ignore
>> ESRCH, because a thread ended between read and write attempts.
>> (virCgroupMoveTask): Loop until all threads have moved.
>>
>
> ACK, this looks like the best we can do here.
Thanks; pushed. I'm also in the process of backporting it to various
maint branches (at least v0.10.2-maint, since the mentioned BZ was
tagged against Fedora 18) - problem was introduced in 0.10.0, in commit
9102829.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 05/21/2013 10:42 AM, Anthony Liguori wrote:
> Perhaps the right thing to do for OpenStack is to allow for a user
> specified configuration file to select things like the default hardware
> models/machine types? Then this could become node configuration instead
> of dynamic configuration.
>
> I think it could be useful for general users too. Every domain requires
> a lot of the same boiler plate bits. I think a lot of configurations
> would benefit from being able to set global domain options.
I have also argued in the past that it would be useful for libvirt to
support the idea of a template, where you can specify a domain XML that
inherits defaults from the template. We've already done things like
this for networking, nwfilter, and even secret management (in domain
XML, you declare that you are using a named network object, and that
network object serves as the template instead of you having to hard-code
all the elements into your domain XML), so we have a design to base it
on. But until someone adds such a feature for libvirt, then OpenStack
should be passing explicit XML to libvirt, and tracking defaults at the
OpenStack layer.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
"Daniel P. Berrange" writes:
> On Tue, May 21, 2013 at 07:55:27PM +1000, Paul Mackerras wrote:
>> On Tue, May 21, 2013 at 09:39:53AM +0100, Daniel P. Berrange wrote:
>> I think libvirt needs some more sensible way to ask qemu what its
>> capabilities are. Currently it has no way to ask qemu "what machines
>> can you emulate with kvm acceleration?" If the user has asked for a
>> KVM domain then the default machine should be one that can be provided
>> by KVM. At present it isn't, on PowerPC.
>
> If QEMU can provide more intelligent info in this respect, then
> libvirt can use it. We're doing the best we can with picking
> defaults given the info QEMU currently provides us.
Thinking about this a little more.
OpenStack pushes a lot of configuration to the nodes themselves instead
of making things dynamic and exposing APIs (think host network
configuration).
QEMU actually does allow a user to change the default machine type via
the global config file so in theory you could do this with OpenStack.
However, since libvirt uses -nouserconfig, this doesn't work in
practice.
Perhaps the right thing to do for OpenStack is to allow for a user
specified configuration file to select things like the default hardware
models/machine types? Then this could become node configuration instead
of dynamic configuration.
I think it could be useful for general users too. Every domain requires
a lot of the same boiler plate bits. I think a lot of configurations
would benefit from being able to set global domain options.
Regards,
Anthony Liguori
>
>
> 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
On Tue, May 21, 2013 at 09:12:49AM -0400, dwa...@redhat.com wrote:
> From: Dan Walsh
>
> mcstransd is a translation tool that can translate MCS Labels into human
> understandable code. I have patched it to watch for translation files in the
> /run/setrans directory. This allows us to run commands like ps -eZ and see
> system_u:system_r:svirt_t:Fedora18 rather then
> system_u:system_r:svirt_t:s0:c1,c2.
> When used with containers it would make an easy way to list all processes
> within
> a container using ps -eZ | grep Fedora18
>
> Pass in privileged field into Security Manager so this is only attempted on
> privileged
> machines
Did you actually test this patch, because it doesn't work at all ?
An LXC guest fails to start:
2013-05-21 16:26:30.894+: 1: error : virSecuritySELinuxAddMCSFile:107 :
unable to create MCS file /var/run/setrans/busy: No such file or directory
If I create that directory inside the container, it at least starts,
but doesn't have any effect because you're trying to write to /var/run
directory inside the container, rather than in the host.
With a QEMU guest this does nothing at all, because the QEMU driver
uses virSecurityManagerSetChildProcessLabel instead of
virSecurityManagerSetProcessLabel so this new code simply never
runs.
Trying todo this from the virSecurityManagerSetProcessLabel method
is just wrong. As I said last time, virSecurityManagerGenProcessLabel
is a better place IMHO.
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index 5d108b9..c41 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -83,6 +83,57 @@
> virSecuritySELinuxRestoreSecurityTPMFileLabelInt(virSecurityManagerPtr mgr,
> virDomainTPMDefPtr tpm);
>
>
> +static int
> +virSecuritySELinuxAddMCSFile(const char *name,
> + const char *label)
> +{
> +int ret = -1;
> +char *tmp = NULL;
> +context_t con = NULL;
> +
> +if (virAsprintf(&tmp, "%s/%s", SELINUX_TRANS_DIR, name) < 0) {
SELINUX_TRANS_DIR doesn't appear to exist in any libselinux package
prior to Fedora 19, so this breaks the build on all RHEL distros
and Fedora < 18. This code needs to be made conditional on this
constant existing in the headers.
> +virReportOOMError();
> +return -1;
> +}
> +if (!(con = context_new(label))) {
> +virReportSystemError(errno, "%s",
> + _("unable to allocate security context"));
> +goto cleanup;
> +}
> +if (virFileWriteStr(tmp, context_range_get(con), S_IRUSR|S_IWUSR) < 0) {
> +virReportSystemError(errno,
> + _("unable to create MCS file %s"), tmp);
> +goto cleanup;
> +}
> +ret = 0;
> +
> +cleanup:
> +VIR_FREE(tmp);
> +context_free(con);
> +return ret;
> +}
> +
> +static int
> +virSecuritySELinuxRemoveMCSFile(const char *name)
> +{
> +char *tmp = NULL;
> +int ret = -1;
> +if (virAsprintf(&tmp, "%s/%s", SELINUX_TRANS_DIR, name) < 0) {
> +virReportOOMError();
> +return -1;
> +}
> +if (unlink(tmp) < 0 && errno != ENOENT) {
> +virReportSystemError(errno,
> + _("Unable to remove MCS file %s"), tmp);
> +goto cleanup;
> +}
> +ret = 0;
> +
> +cleanup:
> +VIR_FREE(tmp);
> +return ret;
> +}
> +
> /*
> * Returns 0 on success, 1 if already reserved, or -1 on fatal error
> */
> @@ -1953,7 +2004,7 @@
> virSecuritySELinuxReleaseSecurityLabel(virSecurityManagerPtr mgr,
> }
> VIR_FREE(secdef->imagelabel);
>
> -return 0;
> +return virSecuritySELinuxRemoveMCSFile(def->name);
> }
>
>
> @@ -2047,10 +2098,14 @@
> virSecuritySELinuxSetSecurityProcessLabel(virSecurityManagerPtr mgr
> ATTRIBUTE_UN
> return -1;
> }
>
> +if (virSecurityManagerGetPrivileged(mgr) &&
> (virSecuritySELinuxAddMCSFile(def->name, secdef->label) < 0))
> +return -1;
As I said last time, failure to create the MCS file should not be treated
as a fatal error IMHO.
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
Impressive. :)
- Original Message -
From: "Eric Blake"
To: "Guannan Ren"
Cc: libvir-list@redhat.com
Sent: Tuesday, May 21, 2013 10:49:57 PM
Subject: Re: [libvirt] [PATCH] interface: list all interfaces with flags == 0
On 05/21/2013 06:49 AM, Guannan Ren wrote:
Guannan's logic says when to drop:
>>> +if (MATCH(VIR_CONNECT_LIST_INTERFACES_FILTERS_ACTIVE) &&
>>> +!((MATCH(VIR_CONNECT_LIST_INTERFACES_ACTIVE) &&
>>> + (status & NETCF_IFACE_ACTIVE)) ||
>>> + (MATCH(VIR_CONNECT_LIST_INTERFACES_INACTIVE) &&
>>> + (status & NETCF_IFACE_INACTIVE
>>> +continue;
Jirka's logic says when to keep; so it would need a ! around the overall
expression if we want to turn it into the condition that represents when
to drop.
>> if (!MATCH(VIR_CONNECT_LIST_INTERFACES_FILTERS_ACTIVE) ||
>> (MATCH(VIR_CONNECT_LIST_INTERFACES_ACTIVE) &&
>> (status & NETCF_IFACE_ACTIVE)) ||
>> (MATCH(VIR_CONNECT_LIST_INTERFACES_INACTIVE) &&
>> (status & NETCF_IFACE_INACTIVE)))
> yes, currently, there are only one group of flags with two
> values(active/inactive),
> the way of yours is better to read.
> if in future, there are more than one group of flags which are
> going to support,
> the way of my version will be better.
Let's compare what happens for all four combinations of the two flags
[active is flag 1, inactive is flag 2]:
flags status Guannan's [0 means keep] Jirka's [1 means keep]
0 active 0&&!(0||0)=keep !0||0||0=keep
0 inactive 0&&!(0||0)=keep !0||0||0=keep
1 active 1&&!(1||0)=keep !1||1||0=keep
1 inactive 1&&!(0||0)=drop !1||0||0=drop
2 active 1&&!(0||0)=drop !1||0||0=drop
2 inactive 1&&!(0||1)=keep !1||0||1=keep
3 active 1&&!(1||0)=keep !1||1||0=keep
3 inactive 1&&!(0||1)=keep !1||0||1=keep
Ultimately, the two expressions are equivalent (you can use deMorgan's
law to prove the equivalence), but I find Jirka's positive logic a bit
easier to reason with than Guannan's negative logic, even if Guannan's
style copied from how we did it on other API. I do agree that for
purposes of adding future filter groups, as well as minimizing nested
conditionals, that the logic looks better in terms of drop checks:
foreach item:
if (filter group 1 drops)
continue;
if (filter group 2 drops)
continue;
item was kept by all filters, add it to return
rather than logic in terms of keep checks:
foreach item:
if (filter group 1 keeps)
if (filter group 2 keeps)
add it to return
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 05/21/2013 11:34 AM, Daniel P. Berrange wrote:
> On Tue, May 21, 2013 at 11:29:26AM -0400, Laine Stump wrote:
>> On 05/21/2013 07:44 AM, Ján Tomko wrote:
>>> On 05/21/2013 01:37 PM, Laine Stump wrote:
On 05/21/2013 04:03 AM, Ján Tomko wrote:
> On 05/21/2013 09:32 AM, Dominik Mostowiec wrote:
>> hi,
>> I try to add 2 VF by "hostdev".
>> Networks (vnet0, vnet1) with:
>>
>>
>> .
>>
>> Domain:
>>
>>
>>
>>
>>
>>
>>
>> virsh create error:
>> "error: internal error process exited while connecting to monitor: kvm:
>> -device
>> pci-assign,configfd=25,host=01:10.1,id=hostdev0,bus=pci.0,addr=0x4:
>> Duplicate ID 'hostdev0' for device"
>>
> Hi,
>
> it seems we have been assigning the same id to all network hostdevs until
> this
> recent commit (not yet released):
>
> http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=6597cc25
If that patch has such an effect, it's purely accidental. Have you
tested this? (I plan to test a before and after as soon as I've had
breakfast)
>>> I haven't tested it and looking at the code again, I might've been wrong :(
>>>
>>> Sorry about that.
>> Nothing to be sorry about even if you weren't right. And as it turns out
>> you *were* right!
>>
>> I was curious why, so I tested with a 1.0.4 build running under gdb. The
>> reason for the failure was that the person who wrote the code in
>> qemuBuildCommandLine that called qemuAssignDeviceHostdevAlias() (as well
>> as the person who reviewed that code, which would be me :-/) missed a
>> detail of qemuAssignDeviceHostdevAlias() - it only searches for a free
>> alias index if you send -1 as the index, otherwise it just uses the
>> index you send. This is what was being called:
>>
>> qemuAssignDeviceHostdevAlias(def, hostdev, (def->nhostdevs-1))
>>
>> So the first time def->nhostdevs-1 == -1, and a search would be made in
>> the (empty) hostdevs array, resulting in an index of 0 being used. The
>> second time it's called, def->nhostdevs-1 == 0, so it would just use 0
>> without even checking for a conflict.
>>
>> My patch for VFIO (the one Jan pointed out as being a fix) completely
>> removed this code from qemuBuildCommandLine(), relying instead on the
>> alias being assigned with all other aliases in
>> qemuAssignDeviceAliases(). (I didn't do this on purpose to fix this bug
>> though; I did it because the old code had become redundant).
>>
>> Anyway, thanks for being so observant. I otherwise would have wasted
>> investigation time wondering why it wasn't broken for me.
> So the fact that we missed this suggests we have a gap in our XML->ARGV
> test coverage for host device assignment, right ?
It seems you're on a mission today :-)
Yes. The problem with network commandlines has been that the setup of
the tap devices has always been depply intertwined with the commandline
generation, making it impossible to test from make check. It's been an
annoyance to me for a long time, but finally got to my conscious brain
last month when I put in the vfio patches. (this was the side topic I
opened when Osier sent his patches that add a callback object to the
qemuBuildCommandLine() args).
This is also a problem with generic legacy hostdev devices (generic vfio
host devices don't seem to have a problem), but their only need is a bit
of sysfs.
I would actually prefer the network device host-side setup to be handled
completely outside of qemuBuildCommandLine() in
qemuNetworkPrepareDevices() (as is done for usb and pci passthrough
devices) rather than in a callback, but that would require sending an
array of file descriptors into qemuBuildCommandLine(), and there's no
convenient place to do that. (Note that the arg "vmop" is also only
needed for network device host-side setup, and so moving that setup
outside of qemuBuildCommandline() would eliminate the need for that arg).
You've said that you're against adding hidden driver specific state
information to the virDomainDef (which makes sense. although I will
point out that there is already both driver-specific (everything in
) and state data (the subelement which isn't
driver-specific, but describes what type of network device was actually
assigned to the guest) in the virDomainDef), so we're left with either:
1) moving the network device "preparation" into qemuNetworkPrepareDevices,
then replacing "vmop" with a "networkData" arg (or maybe "deviceData",
so that it can easily have similar stuff for other devices added to it)
that contains all the fd's for the tap and vhost-net devices.
or
2) moving the network device preparation into a callback that's called
from qemuBuildCommandLine(), and keeping vmop simply to pass it into
this callback.
(unless someone else has a better idea). My opinion is that option (1)
is better, because it puts all the netdev setup in a si
On 21/05/13 23:37, Eric Blake wrote:
On 05/21/2013 04:00 AM, Osier Yang wrote:
---
src/phyp/phyp_driver.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
ACK for being mechanical. However...
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
index 4594cbf..70d3adb 100644
--- a/src/phyp/phyp_driver.c
+++ b/src/phyp/phyp_driver.c
@@ -1008,7 +1008,7 @@ connected:
libssh2_session_set_blocking(session, 0);
while ((rc = libssh2_session_startup(session, sock)) ==
- LIBSSH2_ERROR_EAGAIN) ;
+ LIBSSH2_ERROR_EAGAIN);
My review of patch 30/31 made me revisit this patch. It has an empty
while loop. Should we enforce a particular style where all empty-bodied
loops should use a more obvious style to prove that we knew what we were
doing? For example:
while ((rc = libssh2_session_startup(session, sock)) ==
LIBSSH2_ERROR_EAGAIN)
; /* empty */
or
while ((rc = libssh2_session_startup(session, sock)) ==
LIBSSH2_ERROR_EAGAIN) {
/* empty */
}
Either of above 2 is better than the original one. It's more clear to know
the loop has an empty body. But not sure if it's overladen forcing.
Osier
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 05/21/2013 04:01 AM, Osier Yang wrote:
> Only a few cases are allowed:
>
> 1) The expression is empty for "for" loop, E.g.
>
> for (i = 0; ; i++)
>
> 2) An empty statement
>
> while (write(statuswrite, &status, 1) == -1 &&
> errno == EINTR)
> ; /* empty */
>
> 3) ";" is inside double-quote, I.e, as part of const string. E.g.
>
> vshPrint(ctl, "a ; b ; cd;\n");
>
> The "for" loop in src/rpc/virnettlscontext.c is the special case,
> 1) applies for it, so change it together in this patch.
> ---
> build-aux/bracket-spacing.pl | 18 ++
> src/rpc/virnettlscontext.c | 2 +-
> 2 files changed, 19 insertions(+), 1 deletion(-)
ACK.
>
> diff --git a/build-aux/bracket-spacing.pl b/build-aux/bracket-spacing.pl
> index d3a916f..0d5d2ed 100755
> --- a/build-aux/bracket-spacing.pl
> +++ b/build-aux/bracket-spacing.pl
> @@ -109,6 +109,24 @@ foreach my $file (@ARGV) {
> $ret = 1;
> last;
> }
> +
> +# Forbid whitespace before ";". Things like below are allowed:
> +#
> +# for (i = 0; ; i++)
> +#
> +# while (write(statuswrite, &status, 1) == -1 &&
> +#errno == EINTR)
> +# ;
> +#
> +# printf("%s", "a ; b\n");
> +while ($data =~ /[^;\s]\s+;/) {
> +# Inside the double-quote
> +if ($data !~ /"[^"]*\s;/) {
False negative on:
for (str = "foo" ; *str; str++)
but I don't think we are likely to iterate over bytes of a string
literal, so I'm not too worried about it.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 05/21/2013 07:44 AM, Ján Tomko wrote:
> On 05/21/2013 01:37 PM, Laine Stump wrote:
>> On 05/21/2013 04:03 AM, Ján Tomko wrote:
>>> On 05/21/2013 09:32 AM, Dominik Mostowiec wrote:
hi,
I try to add 2 VF by "hostdev".
Networks (vnet0, vnet1) with:
.
Domain:
virsh create error:
"error: internal error process exited while connecting to monitor: kvm:
-device pci-assign,configfd=25,host=01:10.1,id=hostdev0,bus=pci.0,addr=0x4:
Duplicate ID 'hostdev0' for device"
>>> Hi,
>>>
>>> it seems we have been assigning the same id to all network hostdevs until
>>> this
>>> recent commit (not yet released):
>>>
>>> http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=6597cc25
>>
>> If that patch has such an effect, it's purely accidental. Have you
>> tested this? (I plan to test a before and after as soon as I've had
>> breakfast)
>>
> I haven't tested it and looking at the code again, I might've been wrong :(
>
> Sorry about that.
Nothing to be sorry about even if you weren't right. And as it turns out
you *were* right!
I was curious why, so I tested with a 1.0.4 build running under gdb. The
reason for the failure was that the person who wrote the code in
qemuBuildCommandLine that called qemuAssignDeviceHostdevAlias() (as well
as the person who reviewed that code, which would be me :-/) missed a
detail of qemuAssignDeviceHostdevAlias() - it only searches for a free
alias index if you send -1 as the index, otherwise it just uses the
index you send. This is what was being called:
qemuAssignDeviceHostdevAlias(def, hostdev, (def->nhostdevs-1))
So the first time def->nhostdevs-1 == -1, and a search would be made in
the (empty) hostdevs array, resulting in an index of 0 being used. The
second time it's called, def->nhostdevs-1 == 0, so it would just use 0
without even checking for a conflict.
My patch for VFIO (the one Jan pointed out as being a fix) completely
removed this code from qemuBuildCommandLine(), relying instead on the
alias being assigned with all other aliases in
qemuAssignDeviceAliases(). (I didn't do this on purpose to fix this bug
though; I did it because the old code had become redundant).
Anyway, thanks for being so observant. I otherwise would have wasted
investigation time wondering why it wasn't broken for me.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 05/21/2013 10:18 AM, Michal Privoznik wrote:
> This attribute is going to represent number of queues for
> multique vhost network interface. This commit implements XML
> extension part of the feature and add one test as well. For now,
> we can only do xml2xml test as qemu command line generation code
> is not adapted yet.
> ---
> docs/formatdomain.html.in | 12 -
> docs/schemas/domaincommon.rng | 5 +++
> src/conf/domain_conf.c | 15 +++
> src/conf/domain_conf.h | 1 +
> .../qemuxml2argvdata/qemuxml2argv-vhost_queues.xml | 51
> ++
> tests/qemuxml2xmltest.c| 1 +
> 6 files changed, 84 insertions(+), 1 deletion(-)
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-vhost_queues.xml
ACK.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 05/21/2013 04:01 AM, Osier Yang wrote:
> The more common habit is to add the comment after the statements.
> ---
> src/nwfilter/nwfilter_dhcpsnoop.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c
> b/src/nwfilter/nwfilter_dhcpsnoop.c
> index b9921e5..451c783 100644
> --- a/src/nwfilter/nwfilter_dhcpsnoop.c
> +++ b/src/nwfilter/nwfilter_dhcpsnoop.c
> @@ -369,7 +369,7 @@ virNWFilterSnoopListAdd(virNWFilterSnoopIPLeasePtr plnew,
>
> for (pl = *end; pl && plnew->timeout < pl->timeout;
> pl = pl->prev)
> -/* empty */ ;
> +; /* empty */
How did you determine which style was more common? Do you have 'git
grep | wc' numbers to back your claim? I guess putting the comment
after makes it easier to avoid false positives in patch 31?
I did notice a couple of empty while loops in earlier patches (let me
search for them again...)
found it: patch 8/31. Reply coming up there, soon.
ACK to all patches up to 30 where I didn't explicitly reply.
Weak ack to this one (I don't care if it goes in, but I also don't mind
if it gets omitted).
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 05/21/2013 04:00 AM, Osier Yang wrote:
> ---
> src/phyp/phyp_driver.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
ACK for being mechanical. However...
>
> diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
> index 4594cbf..70d3adb 100644
> --- a/src/phyp/phyp_driver.c
> +++ b/src/phyp/phyp_driver.c
> @@ -1008,7 +1008,7 @@ connected:
> libssh2_session_set_blocking(session, 0);
>
> while ((rc = libssh2_session_startup(session, sock)) ==
> - LIBSSH2_ERROR_EAGAIN) ;
> + LIBSSH2_ERROR_EAGAIN);
My review of patch 30/31 made me revisit this patch. It has an empty
while loop. Should we enforce a particular style where all empty-bodied
loops should use a more obvious style to prove that we knew what we were
doing? For example:
while ((rc = libssh2_session_startup(session, sock)) ==
LIBSSH2_ERROR_EAGAIN)
; /* empty */
or
while ((rc = libssh2_session_startup(session, sock)) ==
LIBSSH2_ERROR_EAGAIN) {
/* empty */
}
If so, how would we enforce such a style?
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 21/05/13 23:35, Eric Blake wrote:
On 05/21/2013 04:01 AM, Osier Yang wrote:
The more common habit is to add the comment after the statements.
---
src/nwfilter/nwfilter_dhcpsnoop.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c
b/src/nwfilter/nwfilter_dhcpsnoop.c
index b9921e5..451c783 100644
--- a/src/nwfilter/nwfilter_dhcpsnoop.c
+++ b/src/nwfilter/nwfilter_dhcpsnoop.c
@@ -369,7 +369,7 @@ virNWFilterSnoopListAdd(virNWFilterSnoopIPLeasePtr plnew,
for (pl = *end; pl && plnew->timeout < pl->timeout;
pl = pl->prev)
-/* empty */ ;
+; /* empty */
How did you determine which style was more common? Do you have 'git
grep | wc' numbers to back your claim? I guess putting the comment
after makes it easier to avoid false positives in patch 31?
Yes, you are right. :-)
As far I saw when writing the script to exclude the empty statements,
all other comments for an empty statements are after ";"
I did notice a couple of empty while loops in earlier patches (let me
search for them again...)
found it: patch 8/31. Reply coming up there, soon.
ACK to all patches up to 30 where I didn't explicitly reply.
Pushed 1 to 30. Thanks.
Weak ack to this one (I don't care if it goes in, but I also don't mind
if it gets omitted).
I leave this one for more opinion, if there is no more opinion till
tomorrow, will push it.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, May 21, 2013 at 11:29:26AM -0400, Laine Stump wrote:
> On 05/21/2013 07:44 AM, Ján Tomko wrote:
> > On 05/21/2013 01:37 PM, Laine Stump wrote:
> >> On 05/21/2013 04:03 AM, Ján Tomko wrote:
> >>> On 05/21/2013 09:32 AM, Dominik Mostowiec wrote:
> hi,
> I try to add 2 VF by "hostdev".
> Networks (vnet0, vnet1) with:
>
>
> .
>
> Domain:
>
>
>
>
>
>
>
> virsh create error:
> "error: internal error process exited while connecting to monitor: kvm:
> -device
> pci-assign,configfd=25,host=01:10.1,id=hostdev0,bus=pci.0,addr=0x4:
> Duplicate ID 'hostdev0' for device"
>
> >>> Hi,
> >>>
> >>> it seems we have been assigning the same id to all network hostdevs until
> >>> this
> >>> recent commit (not yet released):
> >>>
> >>> http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=6597cc25
> >>
> >> If that patch has such an effect, it's purely accidental. Have you
> >> tested this? (I plan to test a before and after as soon as I've had
> >> breakfast)
> >>
> > I haven't tested it and looking at the code again, I might've been wrong :(
> >
> > Sorry about that.
>
> Nothing to be sorry about even if you weren't right. And as it turns out
> you *were* right!
>
> I was curious why, so I tested with a 1.0.4 build running under gdb. The
> reason for the failure was that the person who wrote the code in
> qemuBuildCommandLine that called qemuAssignDeviceHostdevAlias() (as well
> as the person who reviewed that code, which would be me :-/) missed a
> detail of qemuAssignDeviceHostdevAlias() - it only searches for a free
> alias index if you send -1 as the index, otherwise it just uses the
> index you send. This is what was being called:
>
> qemuAssignDeviceHostdevAlias(def, hostdev, (def->nhostdevs-1))
>
> So the first time def->nhostdevs-1 == -1, and a search would be made in
> the (empty) hostdevs array, resulting in an index of 0 being used. The
> second time it's called, def->nhostdevs-1 == 0, so it would just use 0
> without even checking for a conflict.
>
> My patch for VFIO (the one Jan pointed out as being a fix) completely
> removed this code from qemuBuildCommandLine(), relying instead on the
> alias being assigned with all other aliases in
> qemuAssignDeviceAliases(). (I didn't do this on purpose to fix this bug
> though; I did it because the old code had become redundant).
>
> Anyway, thanks for being so observant. I otherwise would have wasted
> investigation time wondering why it wasn't broken for me.
So the fact that we missed this suggests we have a gap in our XML->ARGV
test coverage for host device assignment, right ?
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
On 21/05/13 23:07, Eric Blake wrote:
On 05/21/2013 09:04 AM, Osier Yang wrote:
Commit f60a50c7957 intended to remove the warning only, but not with
the "return 0" together.
---
src/qemu/qemu_cgroup.c | 2 ++
1 file changed, 2 insertions(+)
ACK.
Pushed, thanks.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 2013年05月21日 17:55, Paul Mackerras wrote:
On Tue, May 21, 2013 at 09:39:53AM +0100, Daniel P. Berrange wrote:
On Tue, May 21, 2013 at 09:31:26AM +0100, Peter Maydell wrote:
On 21 May 2013 09:19, Li Zhang wrote:
We encounter this problem in openstack which always use
default machine type. Currently, QEMU sets mac99 as default
setting for ppc64 but it doesn't work on our platform at all.
I tried to fix this in libvirt which it is not acceptable because
libvirt only considers to get default setting from QEMU.
This will need to be fixed for ARM -- the whole idea of there
being a sensible "default machine type" and it being the one
QEMU starts by default is pretty x86-centric. libvirt needs
to have support for specifying which machine to use.
Libvirt has always had support for specifying what machine type to use.
This discussion is simply about what machine type to default to, if the
user hasn't explicitly asked for one.
So, the situation is that the XML in question has hvm,
in the section. Does that say anything about what type of
machine is being requested? Why is the machine type in the
section rather than the section?
QEMU has the notion of a default machine for each target, and that is
what libvirt uses if the user hasn't specified a machine. It is not
libvirt's job to override QEMU's notion of the default machine here,
so if the 'mac99' machine type isn't suitable as the default either
QEMU needs to change that for the ppc target, or the user needs to
explicitly specify their desired machine type.
We are getting the default changed to 'pseries', at least for cases
where pseries support is compiled in, which isn't necessarily
always. That will of course not satisfy the Freescale guys.
You are right. This default setting can't satisfy every platform.
So it will needs management tool to add one machine type option,
if it wants to support different machine types for ppc64.
Thanks. :)
I think libvirt needs some more sensible way to ask qemu what its
capabilities are. Currently it has no way to ask qemu "what machines
can you emulate with kvm acceleration?" If the user has asked for a
KVM domain then the default machine should be one that can be provided
by KVM. At present it isn't, on PowerPC.
Paul.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 2013年05月21日 17:25, Daniel P. Berrange wrote:
On Tue, May 21, 2013 at 05:02:51PM +0800, Li Zhang wrote:
On 2013年05月21日 16:45, Peter Maydell wrote:
On 21 May 2013 09:39, Daniel P. Berrange wrote:
Libvirt has always had support for specifying what machine type to use.
This discussion is simply about what machine type to default to, if the
user hasn't explicitly asked for one.
QEMU has the notion of a default machine for each target, and that is
what libvirt uses if the user hasn't specified a machine. It is not
libvirt's job to override QEMU's notion of the default machine here,
Agreed; thanks for the clarification.
so if the 'mac99' machine type isn't suitable as the default either
QEMU needs to change that for the ppc target, or the user needs to
explicitly specify their desired machine type.
OK, that makes sense. So is the problem here just configuration
or is it the next layer above libvirt not being configurable?
Currently, the next layer above libvirt is not configurable.
It is dependent on this default setting. Users also expect
to start one VM successfully by default.
What is the application above libvirt you are using ? It clearly
needs to be fixed if it is to use non-x86 archs successfully.
Sorry for replying late. The applications are openstack and ovirt.
Thanks.
-- Li
Daniel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, May 21, 2013 at 04:56:37PM +0200, Viktor Mihajlovski wrote:
> On 05/20/2013 01:35 PM, Osier Yang wrote:
> >>>(!virCgroupHasController(priv->cgroup,VIR_CGROUP_CONTROLLER_MEMORY)) {
> >>>+if (vm->def->mem.hard_limit != 0 ||
> >>>+vm->def->mem.soft_limit != 0 ||
> >>>+vm->def->mem.swap_hard_limit != 0) {
> >>>+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> >>>+ _("Memory cgroup is not available on this
> >>>host"));
> >>>+return -1;
> >>>+} else {
> >>>+VIR_WARN("Could not autoset a RSS limit for domain %s",
> >>>vm->def->name);
> >>>+return 0;
> >>>+}
> >>Not sure why we need this VIR_WARN at all. If no limits are set in the
> >>XML,
> >>then we should not warn about a missing feature that we don't actually
> >>need.
> >
> >Agreed. Having a warning for no XML config is confused. I removed it.
>
> We may not need the warning, but the return 0 must stay. I can't start
> guests on my system with no memory controller after this commit.
Yes, absolutely. I only suggested killing the warning, the 'return 0'
must remain for sure.
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
On 21/05/13 22:56, Viktor Mihajlovski wrote:
On 05/20/2013 01:35 PM, Osier Yang wrote:
(!virCgroupHasController(priv->cgroup,VIR_CGROUP_CONTROLLER_MEMORY)) {
+if (vm->def->mem.hard_limit != 0 ||
+vm->def->mem.soft_limit != 0 ||
+vm->def->mem.swap_hard_limit != 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Memory cgroup is not available on this
host"));
+return -1;
+} else {
+VIR_WARN("Could not autoset a RSS limit for domain %s",
vm->def->name);
+return 0;
+}
Not sure why we need this VIR_WARN at all. If no limits are set in the
XML,
then we should not warn about a missing feature that we don't actually
need.
Agreed. Having a warning for no XML config is confused. I removed it.
We may not need the warning, but the return 0 must stay. I can't start
guests on my system with no memory controller after this commit.
oops, I blindly removed it. Will fix soon.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 21/05/13 14:04, Anthony Liguori wrote:
> "Daniel P. Berrange" writes:
>
>> On Tue, May 21, 2013 at 07:55:27PM +1000, Paul Mackerras wrote:
>>> On Tue, May 21, 2013 at 09:39:53AM +0100, Daniel P. Berrange wrote:
QEMU has the notion of a default machine for each target, and that is
what libvirt uses if the user hasn't specified a machine. It is not
libvirt's job to override QEMU's notion of the default machine here,
so if the 'mac99' machine type isn't suitable as the default either
QEMU needs to change that for the ppc target, or the user needs to
explicitly specify their desired machine type.
>>>
>>> We are getting the default changed to 'pseries', at least for cases
>>> where pseries support is compiled in, which isn't necessarily
>>> always. That will of course not satisfy the Freescale guys.
>>>
>>> I think libvirt needs some more sensible way to ask qemu what its
>>> capabilities are. Currently it has no way to ask qemu "what machines
>>> can you emulate with kvm acceleration?" If the user has asked for a
>>> KVM domain then the default machine should be one that can be provided
>>> by KVM. At present it isn't, on PowerPC.
>>
>> If QEMU can provide more intelligent info in this respect, then
>> libvirt can use it. We're doing the best we can with picking
>> defaults given the info QEMU currently provides us.
>
> We've talked in the past about having an accelerator specific machine
> default. I think this is a perfectly reasonable thing to do and would
> solve the problem for ARM and for PPC.
If we get such thing, then virtio-ccw might also be the right default for kvm
on s390.
Christian
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 05/20/2013 01:35 PM, Osier Yang wrote:
(!virCgroupHasController(priv->cgroup,VIR_CGROUP_CONTROLLER_MEMORY)) {
+if (vm->def->mem.hard_limit != 0 ||
+vm->def->mem.soft_limit != 0 ||
+vm->def->mem.swap_hard_limit != 0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Memory cgroup is not available on this
host"));
+return -1;
+} else {
+VIR_WARN("Could not autoset a RSS limit for domain %s",
vm->def->name);
+return 0;
+}
Not sure why we need this VIR_WARN at all. If no limits are set in the
XML,
then we should not warn about a missing feature that we don't actually
need.
Agreed. Having a warning for no XML config is confused. I removed it.
We may not need the warning, but the return 0 must stay. I can't start
guests on my system with no memory controller after this commit.
--
Mit freundlichen Grüßen/Kind Regards
Viktor Mihajlovski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 05/21/2013 07:29 AM, Guannan Ren wrote:
> virConnectListAllInterfaces should support to list all of
> interfaces when the value of flags is 0. The behaviour is
> consistent with other virConnectListAll* APIs
> ---
> src/conf/interface_conf.h | 4
> src/interface/interface_backend_netcf.c | 29 +++--
> src/interface/interface_backend_udev.c | 25 -
> 3 files changed, 31 insertions(+), 27 deletions(-)
ACK.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 05/21/2013 06:49 AM, Guannan Ren wrote:
Guannan's logic says when to drop:
>>> +if (MATCH(VIR_CONNECT_LIST_INTERFACES_FILTERS_ACTIVE) &&
>>> +!((MATCH(VIR_CONNECT_LIST_INTERFACES_ACTIVE) &&
>>> + (status & NETCF_IFACE_ACTIVE)) ||
>>> + (MATCH(VIR_CONNECT_LIST_INTERFACES_INACTIVE) &&
>>> + (status & NETCF_IFACE_INACTIVE
>>> +continue;
Jirka's logic says when to keep; so it would need a ! around the overall
expression if we want to turn it into the condition that represents when
to drop.
>> if (!MATCH(VIR_CONNECT_LIST_INTERFACES_FILTERS_ACTIVE) ||
>> (MATCH(VIR_CONNECT_LIST_INTERFACES_ACTIVE) &&
>> (status & NETCF_IFACE_ACTIVE)) ||
>> (MATCH(VIR_CONNECT_LIST_INTERFACES_INACTIVE) &&
>> (status & NETCF_IFACE_INACTIVE)))
> yes, currently, there are only one group of flags with two
> values(active/inactive),
> the way of yours is better to read.
> if in future, there are more than one group of flags which are
> going to support,
> the way of my version will be better.
Let's compare what happens for all four combinations of the two flags
[active is flag 1, inactive is flag 2]:
flags status Guannan's [0 means keep] Jirka's [1 means keep]
0 active 0&&!(0||0)=keep !0||0||0=keep
0 inactive 0&&!(0||0)=keep !0||0||0=keep
1 active 1&&!(1||0)=keep !1||1||0=keep
1 inactive 1&&!(0||0)=drop !1||0||0=drop
2 active 1&&!(0||0)=drop !1||0||0=drop
2 inactive 1&&!(0||1)=keep !1||0||1=keep
3 active 1&&!(1||0)=keep !1||1||0=keep
3 inactive 1&&!(0||1)=keep !1||0||1=keep
Ultimately, the two expressions are equivalent (you can use deMorgan's
law to prove the equivalence), but I find Jirka's positive logic a bit
easier to reason with than Guannan's negative logic, even if Guannan's
style copied from how we did it on other API. I do agree that for
purposes of adding future filter groups, as well as minimizing nested
conditionals, that the logic looks better in terms of drop checks:
foreach item:
if (filter group 1 drops)
continue;
if (filter group 2 drops)
continue;
item was kept by all filters, add it to return
rather than logic in terms of keep checks:
foreach item:
if (filter group 1 keeps)
if (filter group 2 keeps)
add it to return
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
v3->v4
add missing .args, .xml files
(I raw it in git Untracked files this morning and
thought about it for a while, then git-cleaned them...)
rebase work.
v2->v3
rebase work.
v1->v2:
changed attribute name from 'policy' to 'sharePolicy'
renamed caps flag name: QEMU_CAPS_VNC_SHARE_POLICY
fixed issues pointed out in v1 review
have to keep hard-coded version probe after checking qemu -help and qmp
command.
As there were some vnc patches recently, I chose do rebase work a little later.
These patches try to add a new attribute 'sharePolicy' to element graphics of
vnc type. This attribute has three values:
allow-exclusive,force-shared and ignore.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 05/21/2013 10:09 PM, Laine Stump wrote:
On 05/21/2013 09:28 AM, Guannan Ren wrote:
When using udev backend, virsh iface-list outputs bridge devices
like:
Name State MAC Address
em1 active e8:39:35:58:d5:94
lo active 00:00:00:00:00:00
virbr0 active fe:54:00:a5:f6:42
The patch filters out bridge devices.
---
src/interface/interface_backend_udev.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/interface/interface_backend_udev.c
b/src/interface/interface_backend_udev.c
index 1fd7d46..92648e8 100644
--- a/src/interface/interface_backend_udev.c
+++ b/src/interface/interface_backend_udev.c
@@ -78,6 +78,9 @@ udevGetDevices(struct udev *udev, virUdevStatus status)
/* Ignore devices that are part of a bridge */
udev_enumerate_add_nomatch_sysattr(enumerate, "brport/state", NULL);
+/* Ignore bridge devices */
+udev_enumerate_add_nomatch_sysattr(enumerate, "bridge/bridge_id", NULL);
+
NACK. It should list bridge devices. What shouldn't be listed is
*transient* interfaces (such as the bridge devices created by libvirt).
This is a problem inherent in the design of the udev driver. It's
examining the current state of the network system, while the original
intent of libvirt's interface driver was really to examine/modify the
persistent system config.
Get it, if so, netcf does a right work.
Thanks for this explanation.
I pointed this out to Doug at the time he
added the udev driver, and he acknowledged the problem but couldn't
think of a simple way to eliminate it, and it didn't seem serious enough
to delay adding in the driver.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 05/21/2013 09:28 AM, Guannan Ren wrote:
> When using udev backend, virsh iface-list outputs bridge devices
> like:
> Name State MAC Address
>
> em1 active e8:39:35:58:d5:94
> lo active 00:00:00:00:00:00
> virbr0 active fe:54:00:a5:f6:42
>
> The patch filters out bridge devices.
>
> ---
> src/interface/interface_backend_udev.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/src/interface/interface_backend_udev.c
> b/src/interface/interface_backend_udev.c
> index 1fd7d46..92648e8 100644
> --- a/src/interface/interface_backend_udev.c
> +++ b/src/interface/interface_backend_udev.c
> @@ -78,6 +78,9 @@ udevGetDevices(struct udev *udev, virUdevStatus status)
> /* Ignore devices that are part of a bridge */
> udev_enumerate_add_nomatch_sysattr(enumerate, "brport/state", NULL);
>
> +/* Ignore bridge devices */
> +udev_enumerate_add_nomatch_sysattr(enumerate, "bridge/bridge_id", NULL);
> +
NACK. It should list bridge devices. What shouldn't be listed is
*transient* interfaces (such as the bridge devices created by libvirt).
This is a problem inherent in the design of the udev driver. It's
examining the current state of the network system, while the original
intent of libvirt's interface driver was really to examine/modify the
persistent system config. I pointed this out to Doug at the time he
added the udev driver, and he acknowledged the problem but couldn't
think of a simple way to eliminate it, and it didn't seem serious enough
to delay adding in the driver.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Fixed version with Laine's comments worked in [1]. Moreover, most of the
patches joined together.
Patches 2, 3 and 5 has been ACKed already. BTW, patch 4 is really a different
to patch 3. The only thing they share is a commit message body.
Kernel and subsequently QEMU learned multiple transmit queues a while ago. The
feature has a nice advantage, it allows a single guest to transmit multiple
flows of network data using multiple CPUs simultaneously which increase traffic
bandwidth. A lot.
The documentation how to use this is available at [2] or [3].
1: https://www.redhat.com/archives/libvir-list/2013-May/msg01196.html
2:
https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/tree/Documentation/networking/tuntap.txt
3: http://git.qemu.org/?p=qemu.git;a=blob;f=qemu-options.hx;hb=HEAD#l1363
Michal Privoznik (4):
Introduce /domain/devices/interface/driver/@queues attribute
qemu: Move interface cmd line construction into a separate function
qemu: Adapt qemuBuildInterfaceCommandLine to to multiqueue net
qemu: Enable multiqueue network
docs/formatdomain.html.in | 12 +-
docs/schemas/domaincommon.rng | 5 +
src/conf/domain_conf.c | 15 +
src/conf/domain_conf.h | 1 +
src/network/bridge_driver.c| 2 +-
src/qemu/qemu_command.c| 462 +
src/qemu/qemu_command.h| 13 +-
src/qemu/qemu_hotplug.c| 113 +++--
src/qemu/qemu_monitor.c| 78 ++--
src/qemu/qemu_monitor.h| 8 +-
src/uml/uml_conf.c | 5 +-
src/util/virnetdevtap.c| 113 ++---
src/util/virnetdevtap.h| 2 +
.../qemuxml2argvdata/qemuxml2argv-vhost_queues.xml | 51 +++
tests/qemuxml2xmltest.c| 1 +
15 files changed, 593 insertions(+), 288 deletions(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-vhost_queues.xml
--
1.8.2.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
It also adds the ability to pass in privileged field into Security Manager so
that writing to /run/setrans only attempted on privileged machines
[PATCH] libvirt writes an mcs translation file to /run/setrans
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
When using udev backend, virsh iface-list outputs bridge devices
like:
Name State MAC Address
em1 active e8:39:35:58:d5:94
lo active 00:00:00:00:00:00
virbr0 active fe:54:00:a5:f6:42
The patch filters out bridge devices.
---
src/interface/interface_backend_udev.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/interface/interface_backend_udev.c
b/src/interface/interface_backend_udev.c
index 1fd7d46..92648e8 100644
--- a/src/interface/interface_backend_udev.c
+++ b/src/interface/interface_backend_udev.c
@@ -78,6 +78,9 @@ udevGetDevices(struct udev *udev, virUdevStatus status)
/* Ignore devices that are part of a bridge */
udev_enumerate_add_nomatch_sysattr(enumerate, "brport/state", NULL);
+/* Ignore bridge devices */
+udev_enumerate_add_nomatch_sysattr(enumerate, "bridge/bridge_id", NULL);
+
/* State of the device */
switch (status) {
case VIR_UDEV_IFACE_ACTIVE:
--
1.8.1.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Hmm,
It seems to be working (after only simple tests).
--
Dominik
2013/5/21 Ján Tomko
> On 05/21/2013 01:37 PM, Laine Stump wrote:
> > On 05/21/2013 04:03 AM, Ján Tomko wrote:
> >> On 05/21/2013 09:32 AM, Dominik Mostowiec wrote:
> >>> hi,
> >>> I try to add 2 VF by "hostdev".
> >>> Networks (vnet0, vnet1) with:
> >>>
> >>>
> >>> .
> >>>
> >>> Domain:
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>
> >>>
> >>> virsh create error:
> >>> "error: internal error process exited while connecting to monitor: kvm:
> >>> -device
> pci-assign,configfd=25,host=01:10.1,id=hostdev0,bus=pci.0,addr=0x4:
> >>> Duplicate ID 'hostdev0' for device"
> >>>
> >> Hi,
> >>
> >> it seems we have been assigning the same id to all network hostdevs
> until this
> >> recent commit (not yet released):
> >>
> >> http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=6597cc25
> >
> >
> > If that patch has such an effect, it's purely accidental. Have you
> > tested this? (I plan to test a before and after as soon as I've had
> > breakfast)
> >
>
> I haven't tested it and looking at the code again, I might've been wrong :(
>
> Sorry about that.
>
> Jan
>
>
>
>
--
Pozdrawiam
Dominik
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, May 20, 2013 at 11:40:34AM -0600, Jim Fehlig wrote:
> I finally have some time to continue reviewing this series...
>
> Daniel P. Berrange wrote:
> > From: "Daniel P. Berrange"
> >
> > Introduce use of a virDomainDefPtr in the domain migrate &
> > start APIs to simplify introduction of ACL security checks.
> >
>
> Not really the 'start' API, but GetXMLDesc, Define, and Undefine. Should
> those be part of the lifecycle changes made in patch 2?
Opps bad description. I tried to make each patch as small as
possible. So where I grouped several methods together, I only
did it because they were mutually dependent wrt the refactoring.
Will fix the description of this commit.
> > @@ -44,7 +44,8 @@ int xenXMDomainGetState(virConnectPtr conn,
> > virDomainDefPtr def,
> > int *state,
> > int *reason);
> > -char *xenXMDomainGetXMLDesc(virDomainPtr domain, unsigned int flags);
> > +virDomainDefPtr xenXMDomainGetXMLDesc(virConnectPtr conn,
> > + virDomainDefPtr def);
> >
>
> This still squeezes into one line too.
As you've probably seen by now, I've preferred to split the lines
when there are > 1 parameter in the method, even if it would
technically fit on one line.
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
On 05/21/2013 08:15 PM, Jiri Denemark wrote:
On Tue, May 21, 2013 at 17:05:09 +0800, Guan Nan Ren wrote:
virConnectListAllInterfaces should support to list all of
interfaces when the value of flags is 0. The behaviour is
consistent with other virConnectListAll* APIs
---
diff --git a/src/interface/interface_backend_netcf.c
b/src/interface/interface_backend_netcf.c
index cbba4fd..c6e069a 100644
--- a/src/interface/interface_backend_netcf.c
+++ b/src/interface/interface_backend_netcf.c
...
@@ -361,16 +359,17 @@ netcfConnectListAllInterfaces(virConnectPtr conn,
/* XXX: Filter the result, need to be splitted once new filter flags
* except active|inactive are supported.
*/
-if (((status & NETCF_IFACE_ACTIVE) &&
- (flags & VIR_CONNECT_LIST_INTERFACES_ACTIVE)) ||
-((status & NETCF_IFACE_INACTIVE) &&
- (flags & VIR_CONNECT_LIST_INTERFACES_INACTIVE))) {
-if (ifaces) {
-iface_obj = virGetInterface(conn, ncf_if_name(iface),
-ncf_if_mac_string(iface));
-tmp_iface_objs[niface_objs] = iface_obj;
-}
-niface_objs++;
+if (MATCH(VIR_CONNECT_LIST_INTERFACES_FILTERS_ACTIVE) &&
+!((MATCH(VIR_CONNECT_LIST_INTERFACES_ACTIVE) &&
+ (status & NETCF_IFACE_ACTIVE)) ||
+ (MATCH(VIR_CONNECT_LIST_INTERFACES_INACTIVE) &&
+ (status & NETCF_IFACE_INACTIVE
+continue;
IMHO this is much harder to read. I'd just rewrite the original
condition as (i.e., !MATCH(VIR_CONNECT_LIST_INTERFACES_FILTERS_ACTIVE)
is the part that was missing):
if (!MATCH(VIR_CONNECT_LIST_INTERFACES_FILTERS_ACTIVE) ||
(MATCH(VIR_CONNECT_LIST_INTERFACES_ACTIVE) &&
(status & NETCF_IFACE_ACTIVE)) ||
(MATCH(VIR_CONNECT_LIST_INTERFACES_INACTIVE) &&
(status & NETCF_IFACE_INACTIVE)))
Moreover, your version is wrong as it skips ncf_if_free(iface) in case
iface should be skipped.
yes, currently, there are only one group of flags with two
values(active/inactive),
the way of yours is better to read.
if in future, there are more than one group of flags which are
going to support,
the way of my version will be better. virConnectListAllDomains
use this way to
filter list of domains.
Guannan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 21 May 2013 13:04, Anthony Liguori wrote:
> We've talked in the past about having an accelerator specific machine
> default. I think this is a perfectly reasonable thing to do and would
> solve the problem for ARM and for PPC.
For ARM I would prefer not to have a default at all, and make
the next level up specify it. It's reasonable to have a default
machine choice for when you create a VM configuration; it's
not so sensible to have a default that's picked every time
you run the VM. The thing I think is most likely to be the
useful choice for KVM/ARM isn't even in the tree yet...
There's no user level difference between "pick whether you
wanted a model of a versatile-PB or a zaurus" and "pick
whether you wanted a model of an x86-PC or an ARM zaurus".
At the moment we require the user to specify the latter choice
(by having them in separate executables) but try to guess
the former.
> That said, why is mac99 the default?
Presumably because it was a sensible choice when it was originally
put in, and once you have a default you can't change it without
breaking peoples' command lines.
thanks
-- PMM
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
"Daniel P. Berrange" writes:
> On Tue, May 21, 2013 at 07:55:27PM +1000, Paul Mackerras wrote:
>> On Tue, May 21, 2013 at 09:39:53AM +0100, Daniel P. Berrange wrote:
>> > QEMU has the notion of a default machine for each target, and that is
>> > what libvirt uses if the user hasn't specified a machine. It is not
>> > libvirt's job to override QEMU's notion of the default machine here,
>> > so if the 'mac99' machine type isn't suitable as the default either
>> > QEMU needs to change that for the ppc target, or the user needs to
>> > explicitly specify their desired machine type.
>>
>> We are getting the default changed to 'pseries', at least for cases
>> where pseries support is compiled in, which isn't necessarily
>> always. That will of course not satisfy the Freescale guys.
>>
>> I think libvirt needs some more sensible way to ask qemu what its
>> capabilities are. Currently it has no way to ask qemu "what machines
>> can you emulate with kvm acceleration?" If the user has asked for a
>> KVM domain then the default machine should be one that can be provided
>> by KVM. At present it isn't, on PowerPC.
>
> If QEMU can provide more intelligent info in this respect, then
> libvirt can use it. We're doing the best we can with picking
> defaults given the info QEMU currently provides us.
We've talked in the past about having an accelerator specific machine
default. I think this is a perfectly reasonable thing to do and would
solve the problem for ARM and for PPC.
That said, why is mac99 the default? It doesn't seem to work at all for
me. Even with TCG, I've had more luck with -M pseries.
While adding an accelerator specific default, if mac99 is the wrong
default for TCG, then we should change it.
Regards,
Anthony Liguori
>
>
> 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
On 05/21/2013 01:37 PM, Laine Stump wrote:
> On 05/21/2013 04:03 AM, Ján Tomko wrote:
>> On 05/21/2013 09:32 AM, Dominik Mostowiec wrote:
>>> hi,
>>> I try to add 2 VF by "hostdev".
>>> Networks (vnet0, vnet1) with:
>>>
>>>
>>> .
>>>
>>> Domain:
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>> virsh create error:
>>> "error: internal error process exited while connecting to monitor: kvm:
>>> -device pci-assign,configfd=25,host=01:10.1,id=hostdev0,bus=pci.0,addr=0x4:
>>> Duplicate ID 'hostdev0' for device"
>>>
>> Hi,
>>
>> it seems we have been assigning the same id to all network hostdevs until
>> this
>> recent commit (not yet released):
>>
>> http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=6597cc25
>
>
> If that patch has such an effect, it's purely accidental. Have you
> tested this? (I plan to test a before and after as soon as I've had
> breakfast)
>
I haven't tested it and looking at the code again, I might've been wrong :(
Sorry about that.
Jan
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 05/21/2013 04:03 AM, Ján Tomko wrote:
> On 05/21/2013 09:32 AM, Dominik Mostowiec wrote:
>> hi,
>> I try to add 2 VF by "hostdev".
>> Networks (vnet0, vnet1) with:
>>
>>
>> .
>>
>> Domain:
>>
>>
>>
>>
>>
>>
>>
>> virsh create error:
>> "error: internal error process exited while connecting to monitor: kvm:
>> -device pci-assign,configfd=25,host=01:10.1,id=hostdev0,bus=pci.0,addr=0x4:
>> Duplicate ID 'hostdev0' for device"
>>
> Hi,
>
> it seems we have been assigning the same id to all network hostdevs until this
> recent commit (not yet released):
>
> http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=6597cc25
If that patch has such an effect, it's purely accidental. Have you
tested this? (I plan to test a before and after as soon as I've had
breakfast)
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 21 May 2013 11:01, Daniel P. Berrange wrote:
> On Tue, May 21, 2013 at 07:55:27PM +1000, Paul Mackerras wrote:
>> I think libvirt needs some more sensible way to ask qemu what its
>> capabilities are. Currently it has no way to ask qemu "what machines
>> can you emulate with kvm acceleration?" If the user has asked for a
>> KVM domain then the default machine should be one that can be provided
>> by KVM. At present it isn't, on PowerPC.
>
> If QEMU can provide more intelligent info in this respect, then
> libvirt can use it.
I think this would make sense. Currently for ARM to get KVM
acceleration you have to use a specific guest CPU (A15), and
so you have to use a machine which supports that CPU (currently
just vexpress-a15). But we don't expose any way for libvirt to
tell that it needs an A15 or which machine models support which
CPUs. In fact we don't even give a sensible error message if
you try to use a wrong CPU, we'll just blithely push ahead
and behave very weirdly.
thanks
-- PMM
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, May 21, 2013 at 07:55:27PM +1000, Paul Mackerras wrote:
> On Tue, May 21, 2013 at 09:39:53AM +0100, Daniel P. Berrange wrote:
> > On Tue, May 21, 2013 at 09:31:26AM +0100, Peter Maydell wrote:
> > > On 21 May 2013 09:19, Li Zhang wrote:
> > > > We encounter this problem in openstack which always use
> > > > default machine type. Currently, QEMU sets mac99 as default
> > > > setting for ppc64 but it doesn't work on our platform at all.
> > > >
> > > > I tried to fix this in libvirt which it is not acceptable because
> > > > libvirt only considers to get default setting from QEMU.
> > >
> > > This will need to be fixed for ARM -- the whole idea of there
> > > being a sensible "default machine type" and it being the one
> > > QEMU starts by default is pretty x86-centric. libvirt needs
> > > to have support for specifying which machine to use.
> >
> > Libvirt has always had support for specifying what machine type to use.
> > This discussion is simply about what machine type to default to, if the
> > user hasn't explicitly asked for one.
>
> So, the situation is that the XML in question has hvm,
> in the section. Does that say anything about what type of
> machine is being requested?
Obviously not, since you've not specified any machine type and
thus you'll get the default. If you don't like that, change your
app to request a specific machine type.
> Why is the machine type in the
> section rather than the section?
That's just the way it was chosen to be represented when first
implemented. In hindsight that may be sub-optimal, but libvirt
will not change existing XML schema design since that breaks
back compatibilty which is worse.
> > QEMU has the notion of a default machine for each target, and that is
> > what libvirt uses if the user hasn't specified a machine. It is not
> > libvirt's job to override QEMU's notion of the default machine here,
> > so if the 'mac99' machine type isn't suitable as the default either
> > QEMU needs to change that for the ppc target, or the user needs to
> > explicitly specify their desired machine type.
>
> We are getting the default changed to 'pseries', at least for cases
> where pseries support is compiled in, which isn't necessarily
> always. That will of course not satisfy the Freescale guys.
>
> I think libvirt needs some more sensible way to ask qemu what its
> capabilities are. Currently it has no way to ask qemu "what machines
> can you emulate with kvm acceleration?" If the user has asked for a
> KVM domain then the default machine should be one that can be provided
> by KVM. At present it isn't, on PowerPC.
If QEMU can provide more intelligent info in this respect, then
libvirt can use it. We're doing the best we can with picking
defaults given the info QEMU currently provides us.
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
