[libvirt] Domblklist

2013-08-20 Thread Yaniv Hadad 
I am looking for the equivalent of virsh domblklist  in libvirt API 
reference. I am using the Java binding.
What I want to get is the device list of the domain as in the following 
example:

virsh # domblklist centos64test
Target Source

hda/instances/centos64test.img
hdc-


Yaniv Hadad
Servers & Network Group, IBM R&D Labs in Israel
Software Devloper
yan...@il.ibm.com, +972-4-829-6594
Fax: +972-4-829-6111, Cell: +972-50-4078908




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

Re: [libvirt] [PATCH] util:report diskchain as broken if without enough permission

2013-08-20 Thread Guannan Ren 

On 08/21/2013 03:57 AM, Eric Blake wrote:

On 08/20/2013 01:54 AM, Guannan Ren wrote:

When backing files of a disk file are stored in NFS shared folder,
qemu process requires at least the read permission.
But in some rare situation, these backing files can not even be read
If so, we should treat the diskchains as broken.
This patch add a checking for this, report negative in such case.

We have been historically bitten by changes to this code.  I think we
need to patch tests/virstoragetest.c (and/or add a new test) before
accepting this change, to make sure that we are testing the behavior we
want.


Okay, will do it soon.
Thanks

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

Re: [libvirt] [PATCH 2/1] selinux: enhance test to cover nfs label failure

2013-08-20 Thread Eric Blake 
On 08/20/2013 04:58 PM, Jim Fehlig wrote:
> Eric Blake wrote:
>> Daniel Berrange (correctly) pointed out that we should do a better
>> job of testing selinux labeling fallbacks on NFS disks that lack
>> labeling support.
>>
> 
> FYI, this is causing a build failure on a host without libattr-devel
> 
> make[2]: Entering directory `/home/jfehlig/virt/upstream/libvirt/tests'
>   CCLD libshunload.la
>   CC   securityselinuxhelper.lo
> securityselinuxhelper.c:34:24: fatal error: attr/xattr.h: No such file
> or directory
> compilation terminated.
> make[2]: *** [securityselinuxhelper.lo] Error 1
> 

>> +/* This file is only compiled on Linux, and only if xattr support was
>> + * detected. */
>> +
>> +#include 
>> +#include 
>> +#include 
>>  #include 
>> +#include 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>> -#include 
>> -#if WITH_ATTR
>> -# include 
>> -#endif
>> +#include 

Bummer - that means the makefile is not correctly avoiding the build of
this file when attr/xattr.h does not exist.  I don't know where the
disconnect is between WITH_ATTR in the config.h setup, vs. WITH_ATTR as
the automake variable used in tests/Makefile.am.  I'm also out of time
today, but if you don't beat me to it, I'll have a fix tomorrow.

-- 
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

Re: [libvirt] [PATCH] migration: do not restore labels on failed migration

2013-08-20 Thread Eric Blake 
On 08/20/2013 04:46 PM, Eric Blake wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=822052
> 
> When doing a live migration, if the destination fails for any
> reason after the point in which files should be labeled, then
> the cleanup of the destination would restore the labels to their
> defaults, even though the source is still trying to continue
> running with the image open.  Bug 822052 mentioned one source
> of live migration failure - a mismatch in SELinux virt_use_nfs
> settings (on for source, off for destination); but I found other
> situations that would also trigger it (for example, having a
> graphics device tied to port 5999 on the source, and a different
> domain on the destination already using that port, so that the
> destination cannot reuse the port).
> 
> In short, just as cleanup of the source on a successful migration
> must not relabel files (because the destination would be crippled
> by the relabel), cleanup of the destination on a failed migraion
> must not relabel files (because the source would be crippled).

Note that while debugging this, I ran across a bigger design problem.
This patch works as long as source and destination are configured
equally, or where the difference (such as having selinux virt_use_nfs
settings distinct) can be detected immediately upon the attempt to start
qemu on the destination.  A much more insidious case occurs if you have
a shared disk image on NFS, but where you have a configuration mismatch
between source and destination.  In my case, the mismatch was caused by
running Fedora's pre-packaged libvirtd (which is built using qemu:qemu
as the default user/group for qemu processes) on the source, but a
self-built libvirtd (built with ./autogen.sh --system) on the
destination, and with no edits to /etc/libvirt/qemu.conf on either side.
 Note that the default user is a configure option, and that if
/etc/libvirt/qemu.conf does NOT call out a user/group setting, then
libvirtd uses the configure defaults; and since './autogen.sh --system'
currently does not change the configure defaults, it ends up using
root:root to run qemu guests.  This is probably a shortcoming of
'./autogen.sh --system' for not matching what the rpm does, but fixing
that is a separate patch.

The problem at hand is that when migrating between different
configurations, the destination first attempts to label files before
even starting qemu - and does a chown() to root:root.  The failure case
described above for matched configurations occurred when the destination
did a chown() to root:root as part of the cleanup after failure to
migrate, but with my mismatched configuration, the chown() happened much
earlier.  One of the (numerous) annoyances of NFS is that a chown() can
kill access of existing open fds (on a POSIX file system, once you have
an fd via open() or other means, you cannot lose rights to the inode
that the fd is visiting, no matter whether chown(), unlink(), or any
other number of things occur on the file that you originally opened in
the meantime; not so for NFS).  But since the forced loss of access
isn't instantaneous, you now have a race - if live migration is fast
enough, the source can still get the guest migrated to the destination;
but if there is lots of I/O and memory churn in the guest, so that the
live migration takes long enough for the forced loss of access to kick
in, then the source will start seeing I/O errors, possibly causing
severe corruption to the guest.

We should really consider whether we need more framework for diagnosing
incompatible configurations between source and destination, and outright
rejecting migration where two machines do not share the same user/group
setup.

-- 
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

Re: [libvirt] [PATCH 2/1] selinux: enhance test to cover nfs label failure

2013-08-20 Thread Jim Fehlig 
Eric Blake wrote:
> Daniel Berrange (correctly) pointed out that we should do a better
> job of testing selinux labeling fallbacks on NFS disks that lack
> labeling support.
>
> * tests/securityselinuxhelper.c (includes): Makefile already
> guaranteed xattr support.  Add additional headers.
> (init_syms): New function, borrowing from vircgroupmock.c.
> (setfilecon_raw, getfilecon_raw): Fake NFS failure.
> (statfs): Fake an NFS mount point.
> (security_getenforce, security_get_boolean_active): Don't let host
> environment affect test.
> * tests/securityselinuxlabeldata/nfs.data: New file.
> * tests/securityselinuxlabeldata/nfs.xml: New file.
> * tests/securityselinuxlabeltest.c (testSELinuxCreateDisks)
> (testSELinuxDeleteDisks): Setup and cleanup for fake NFS mount.
> (testSELinuxCheckLabels): Test handling of SELinux NFS denial.
> Fix memory leak.
> (testSELinuxLabeling): Avoid infinite loop on dirty tree.
> (mymain): Add new test.
> ---
>  tests/securityselinuxhelper.c  | 84 
> ++
>  tests/securityselinuxlabeldata/nfs.txt |  1 +
>  tests/securityselinuxlabeldata/nfs.xml | 24 ++
>  tests/securityselinuxlabeltest.c   | 17 +--
>  4 files changed, 115 insertions(+), 11 deletions(-)
>  create mode 100644 tests/securityselinuxlabeldata/nfs.txt
>  create mode 100644 tests/securityselinuxlabeldata/nfs.xml
>   

FYI, this is causing a build failure on a host without libattr-devel

make[2]: Entering directory `/home/jfehlig/virt/upstream/libvirt/tests'
  CCLD libshunload.la
  CC   securityselinuxhelper.lo
securityselinuxhelper.c:34:24: fatal error: attr/xattr.h: No such file
or directory
compilation terminated.
make[2]: *** [securityselinuxhelper.lo] Error 1

I need to go now but should have time to take a look later.

Regards,
Jim

> diff --git a/tests/securityselinuxhelper.c b/tests/securityselinuxhelper.c
> index a82ca6d..d7aae26 100644
> --- a/tests/securityselinuxhelper.c
> +++ b/tests/securityselinuxhelper.c
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (C) 2011-2012 Red Hat, Inc.
> + * Copyright (C) 2011-2013 Red Hat, Inc.
>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Lesser General Public
> @@ -19,22 +19,51 @@
>
>  #include 
>
> +/* This file is only compiled on Linux, and only if xattr support was
> + * detected. */
> +
> +#include 
> +#include 
> +#include 
>  #include 
> +#include 
>  #include 
>  #include 
> +#include 
>  #include 
> -#include 
> -#if WITH_ATTR
> -# include 
> -#endif
> +#include 
>
>  #include "virstring.h"
>
> +static int (*realstatfs)(const char *path, struct statfs *buf);
> +static int (*realsecurity_get_boolean_active)(const char *name);
> +
> +static void init_syms(void)
> +{
> +if (realstatfs)
> +return;
> +
> +# define LOAD_SYM(name) \
> +do {\
> +if (!(real ## name = dlsym(RTLD_NEXT, #name))) {\
> +fprintf(stderr, "Cannot find real '%s' symbol\n", #name);   \
> +abort();\
> +}   \
> +} while (0)
> +
> +LOAD_SYM(statfs);
> +LOAD_SYM(security_get_boolean_active);
> +}
> +
> +
>  /*
>   * The kernel policy will not allow us to arbitrarily change
>   * test process context. This helper is used as an LD_PRELOAD
>   * so that the libvirt code /thinks/ it is changing/reading
> - * the process context, where as in fact we're faking it all
> + * the process context, where as in fact we're faking it all.
> + * Furthermore, we fake out that we are using an nfs subdirectory,
> + * where we control whether selinux is enforcing and whether
> + * the virt_use_nfs bool is set.
>   */
>
>  int getcon_raw(security_context_t *context)
> @@ -83,10 +112,13 @@ int setcon(security_context_t context)
>  }
>
>
> -#if WITH_ATTR
>  int setfilecon_raw(const char *path, security_context_t con)
>  {
>  const char *constr = con;
> +if (STRPREFIX(path, abs_builddir "/securityselinuxlabeldata/nfs/")) {
> +errno = EOPNOTSUPP;
> +return -1;
> +}
>  return setxattr(path, "user.libvirt.selinux",
>  constr, strlen(constr), 0);
>  }
> @@ -101,6 +133,10 @@ int getfilecon_raw(const char *path, security_context_t 
> *con)
>  char *constr = NULL;
>  ssize_t len = getxattr(path, "user.libvirt.selinux",
> NULL, 0);
> +if (STRPREFIX(path, abs_builddir "/securityselinuxlabeldata/nfs/")) {
> +errno = EOPNOTSUPP;
> +return -1;
> +}
>  if (len < 0)
>  return -1;
>  if (!(constr = malloc(len+1)))
> @@ -114,8 +150,40 @@ int getfilecon_raw(const char *path, security_context_t 
> *con)
>  constr[len] = '\0';
>  return 0;
>  }
> +
> +
>  int getfilecon(const char *path

[libvirt] [PATCH] migration: do not restore labels on failed migration

2013-08-20 Thread Eric Blake 
https://bugzilla.redhat.com/show_bug.cgi?id=822052

When doing a live migration, if the destination fails for any
reason after the point in which files should be labeled, then
the cleanup of the destination would restore the labels to their
defaults, even though the source is still trying to continue
running with the image open.  Bug 822052 mentioned one source
of live migration failure - a mismatch in SELinux virt_use_nfs
settings (on for source, off for destination); but I found other
situations that would also trigger it (for example, having a
graphics device tied to port 5999 on the source, and a different
domain on the destination already using that port, so that the
destination cannot reuse the port).

In short, just as cleanup of the source on a successful migration
must not relabel files (because the destination would be crippled
by the relabel), cleanup of the destination on a failed migraion
must not relabel files (because the source would be crippled).

* src/qemu/qemu_process.c (qemuProcessStart): Set flag to avoid
label restoration when cleaning up on failed migration.

Signed-off-by: Eric Blake 
---
 src/qemu/qemu_process.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 31de759..d727fc4 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3466,6 +3466,10 @@ int qemuProcessStart(virConnectPtr conn,
  * restore any security label as we would overwrite labels
  * we did not set. */
 stop_flags = VIR_QEMU_PROCESS_STOP_NO_RELABEL;
+/* If we fail while doing incoming migration, then we must not
+ * relabel, as the source is still using the files.  */
+if (migrateFrom)
+stop_flags |= VIR_QEMU_PROCESS_STOP_MIGRATED;

 hookData.conn = conn;
 hookData.vm = vm;
-- 
1.8.3.1

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

Re: [libvirt] [PATCH v2 4/9] virsh: Add vshDomainCompleter

2013-08-20 Thread Eric Blake 
On 08/20/2013 02:30 PM, Eric Blake wrote:
> I'm not a fan of this.  We worked hard to get some commands (like 'virsh
> echo') to NOT need a connection, and this would be undoing that work.  I
> think we should NOT autoconnect UNTIL we reach the point that completion
> is being attempted on something where a connection is needed to get the
> completer list, rather than this code which autoconnects regardless of
> whether we need completion.  Having 'virsh echo' avoid an autoconnect is
> useful - it can be used to prove whether virsh itself is working or has
> a problem such as a missing .so, without complicating the diagnosis with
> a question of whether it is a bad default connection to blame.

More along that line of thought:

If I type:

$ virsh
virsh> e

I want a completion to list all commands that start with 'e'; one of
those commands is 'echo', which does not need a connection.

Therefore, if I use the shell:

$ virsh e

and the shell uses, under the hood,

$ virsh complete e

that particular completion attempt should NOT attempt an autoconnect,
because it is not completing on any information that requires a
connection.  autoconnect to qemu:///session can be noticeably
time-consuming on the first time it is attempted.  Although the session
server tends to stick around for a few seconds after the last
connection, so that future connection attempts in a short timeframe are
serviced faster, you still don't want to pay the penalty for a delay for
the first autoconnect until it is mandatory for the completion at hand.
 Whereas an interactive virsh keeps one connection open across multiple
uses, the bash completion routines will be invoking a different instance
of 'virsh' for every  hit on the shell command line, where we want
each instance to be as fast as possible.

-- 
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

Re: [libvirt] [PATCH v2 1/9] virsh: C99 style for info_domfstrim and opts_lxc_enter_namespace

2013-08-20 Thread Eric Blake 
On 08/20/2013 02:02 PM, Tomas Meszaros wrote:
> Change info_domfstrim and opts_lxc_enter_namespace initialization style
> to C99.
> ---
>  tools/virsh-domain.c | 21 -
>  1 file changed, 16 insertions(+), 5 deletions(-)

ACK.  I wonder if it is worth adding a 'make syntax-check' test that
enforces this style within virsh files, but don't have a good suggestion
for a foolproof regex that would detect C89 initialization.

-- 
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

Re: [libvirt] [PATCH v2 4/9] virsh: Add vshDomainCompleter

2013-08-20 Thread Eric Blake 
On 08/20/2013 02:02 PM, Tomas Meszaros wrote:

Your commit message should give a brief summary of your change and why
it is important ("Add vshDomainCompleter" is the what, but you didn't
spell out the why - something along the lines of "future patches will
use this to improve readline completion").

> * global variable __my_conn renamed to vshConn
> * @name is now const char *
> * label cleanup renamed to error

These three lines are a changelog from your v1 patch, and not the "why"
you are making the change.  In fact, since this patch does not mention
"__my_conn", no one will understand why you mentioned it at all in the
commit message.  It is information that belongs better...

> ---

...here, after the --- separator, which is where 'git am' interprets the
break between the "why" of the commit message that belongs in git
history, vs. the "help" that is useful to list reviewers now, but has no
bearing on a future person reading git history.


> +
> +names = vshMalloc(NULL, sizeof(char *) * (ndomains + 1));
> +
> +if (!names)
> +return NULL;

Dead code.  names is non-null, thanks to vshMalloc.

> +error:
> +for (i = 0; names[i]; i++)
> +VIR_FREE(names[i]);
> +VIR_FREE(names);

Simpler to use virStringFreeList(names) here, instead of inlining the
iteration.

> @@ -3510,6 +3557,7 @@ main(int argc, char **argv)
>  ctl->debug = VSH_DEBUG_DEFAULT;
>  ctl->escapeChar = "^]"; /* Same default as telnet */
>  
> +vshConn = &ctl->conn;
>  
>  if (!setlocale(LC_ALL, "")) {
>  perror("setlocale");
> @@ -3571,6 +3619,11 @@ main(int argc, char **argv)
>  exit(EXIT_FAILURE);
>  }
>  
> +/* Need to connect immediately after start in order to provide
> + * autocompletion for the very first command.
> + */
> +vshReconnect(ctl);
> +

I'm not a fan of this.  We worked hard to get some commands (like 'virsh
echo') to NOT need a connection, and this would be undoing that work.  I
think we should NOT autoconnect UNTIL we reach the point that completion
is being attempted on something where a connection is needed to get the
completer list, rather than this code which autoconnects regardless of
whether we need completion.  Having 'virsh echo' avoid an autoconnect is
useful - it can be used to prove whether virsh itself is working or has
a problem such as a missing .so, without complicating the diagnosis with
a question of whether it is a bad default connection to blame.

Also, at some point in your series, you will want to introduce a new
virsh command whose job is to perform the completion of its arguments.
That way, we can have auto-completion both within the virsh interactive
shell, and from bash, while only having to maintain it in one place.
That is, if I type:

$ virsh
virsh> start a

I'm using the interactive virsh completion to list domains starting with
a; meanwhile, if I type:

$ virsh start a

then the bash completion handler would call:
$ virsh complete start a

under the hood, so that virsh outputs the list of names that can be fed
back to the bash completion hooks.  The bash completion routine is then
dead simple - forward all completions to virsh itself.  I don't want to
have to teach bash completion routines which commands expect a domain,
nor how to generate its own domain listing, when virsh already has that
code.

-- 
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

[libvirt] [PATCH v2 9/9] virsh: Add completer functions to domMonitoringCmds

2013-08-20 Thread Tomas Meszaros 
Add .completer and .completer_flags to domMonitoringCmds.

Provides domain completion for domMonitoringCmds commands.
---
 tools/virsh-domain-monitor.c | 32 +---
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index b29b82a..0f30902 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -1876,7 +1876,9 @@ const vshCmdDef domMonitoringCmds[] = {
  .handler = cmdDomBlkError,
  .opts = opts_domblkerror,
  .info = info_domblkerror,
- .flags = 0
+ .flags = 0,
+ .completer = vshDomainCompleter,
+ .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE
 },
 {.name = "domblkinfo",
  .handler = cmdDomblkinfo,
@@ -1888,7 +1890,10 @@ const vshCmdDef domMonitoringCmds[] = {
  .handler = cmdDomblklist,
  .opts = opts_domblklist,
  .info = info_domblklist,
- .flags = 0
+ .flags = 0,
+ .completer = vshDomainCompleter,
+ .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE |
+VIR_CONNECT_LIST_DOMAINS_INACTIVE
 },
 {.name = "domblkstat",
  .handler = cmdDomblkstat,
@@ -1900,7 +1905,9 @@ const vshCmdDef domMonitoringCmds[] = {
  .handler = cmdDomControl,
  .opts = opts_domcontrol,
  .info = info_domcontrol,
- .flags = 0
+ .flags = 0,
+ .completer = vshDomainCompleter,
+ .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE
 },
 {.name = "domif-getlink",
  .handler = cmdDomIfGetLink,
@@ -1912,7 +1919,10 @@ const vshCmdDef domMonitoringCmds[] = {
  .handler = cmdDomiflist,
  .opts = opts_domiflist,
  .info = info_domiflist,
- .flags = 0
+ .flags = 0,
+ .completer = vshDomainCompleter,
+ .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE |
+VIR_CONNECT_LIST_DOMAINS_INACTIVE
 },
 {.name = "domifstat",
  .handler = cmdDomIfstat,
@@ -1924,19 +1934,27 @@ const vshCmdDef domMonitoringCmds[] = {
  .handler = cmdDominfo,
  .opts = opts_dominfo,
  .info = info_dominfo,
- .flags = 0
+ .flags = 0,
+ .completer = vshDomainCompleter,
+ .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE |
+VIR_CONNECT_LIST_DOMAINS_INACTIVE
 },
 {.name = "dommemstat",
  .handler = cmdDomMemStat,
  .opts = opts_dommemstat,
  .info = info_dommemstat,
- .flags = 0
+ .flags = 0,
+ .completer = vshDomainCompleter,
+ .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE
 },
 {.name = "domstate",
  .handler = cmdDomstate,
  .opts = opts_domstate,
  .info = info_domstate,
- .flags = 0
+ .flags = 0,
+ .completer = vshDomainCompleter,
+ .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE |
+VIR_CONNECT_LIST_DOMAINS_INACTIVE
 },
 {.name = "list",
  .handler = cmdList,
-- 
1.8.3.1

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

[libvirt] [PATCH v2 2/9] virsh: Add vshCmdCompleter and vshOptCompleter

2013-08-20 Thread Tomas Meszaros 
completer and completer_flags added to the _vshCmdDef and _vshCmdOptDef
structures so it will be possible for completion generators to
conveniently call completer functions with desired flags.
---
 tools/virsh.h | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/tools/virsh.h b/tools/virsh.h
index 466ca2d..064acde 100644
--- a/tools/virsh.h
+++ b/tools/virsh.h
@@ -147,6 +147,9 @@ typedef struct _vshCmdOptDef vshCmdOptDef;
 typedef struct _vshControl vshControl;
 typedef struct _vshCtrlData vshCtrlData;
 
+typedef char **(*vshCmdCompleter)(unsigned int flags);
+typedef char **(*vshOptCompleter)(unsigned int flags);
+
 /*
  * vshCmdInfo -- name/value pair for information about command
  *
@@ -168,6 +171,8 @@ struct _vshCmdOptDef {
 unsigned int flags; /* flags */
 const char *help;   /* non-NULL help string; or for VSH_OT_ALIAS
  * the name of a later public option */
+vshOptCompleter completer;  /* option completer */
+unsigned int completer_flags;   /* option completer flags */
 };
 
 /*
@@ -199,6 +204,8 @@ struct _vshCmdDef {
 const vshCmdOptDef *opts;   /* definition of command options */
 const vshCmdInfo *info; /* details about command */
 unsigned int flags; /* bitwise OR of VSH_CMD_FLAG */
+vshCmdCompleter completer;  /* command completer */
+unsigned int completer_flags;   /* command completer flags */
 };
 
 /*
-- 
1.8.3.1

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

[libvirt] [PATCH v2 7/9] virsh: Add completer functions to domManaggementCmds

2013-08-20 Thread Tomas Meszaros 
* opts_dom_pm_suspend completer initialization for "target" removed
  (moved to another patch)
---
 tools/virsh-domain.c | 230 +++
 1 file changed, 179 insertions(+), 51 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index a2002c5..ff7a6e3 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -2756,8 +2756,7 @@ static const vshCmdOptDef opts_dom_pm_suspend[] = {
  .flags = VSH_OFLAG_REQ,
  .help = N_("mem(Suspend-to-RAM), "
 "disk(Suspend-to-Disk), "
-"hybrid(Hybrid-Suspend)"),
- .completer = vshSuspendTargetCompleter
+"hybrid(Hybrid-Suspend)")
 },
 {.name = NULL}
 };
@@ -4714,7 +4713,8 @@ static const vshCmdOptDef opts_shutdown[] = {
 },
 {.name = "mode",
  .type = VSH_OT_STRING,
- .help = N_("shutdown mode: acpi|agent|initctl|signal")
+ .help = N_("shutdown mode: acpi|agent|initctl|signal"),
+ .completer = vshRebootShutdownModeCompleter
 },
 {.name = NULL}
 };
@@ -4800,7 +4800,8 @@ static const vshCmdOptDef opts_reboot[] = {
 },
 {.name = "mode",
  .type = VSH_OT_STRING,
- .help = N_("shutdown mode: acpi|agent|initctl|signal")
+ .help = N_("shutdown mode: acpi|agent|initctl|signal"),
+ .completer = vshRebootShutdownModeCompleter
 },
 {.name = NULL}
 };
@@ -10359,7 +10360,10 @@ const vshCmdDef domManagementCmds[] = {
  .handler = cmdAutostart,
  .opts = opts_autostart,
  .info = info_autostart,
- .flags = 0
+ .flags = 0,
+ .completer = vshDomainCompleter,
+ .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE |
+VIR_CONNECT_LIST_DOMAINS_INACTIVE
 },
 {.name = "blkdeviotune",
  .handler = cmdBlkdeviotune,
@@ -10371,7 +10375,10 @@ const vshCmdDef domManagementCmds[] = {
  .handler = cmdBlkiotune,
  .opts = opts_blkiotune,
  .info = info_blkiotune,
- .flags = 0
+ .flags = 0,
+ .completer = vshDomainCompleter,
+ .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE |
+VIR_CONNECT_LIST_DOMAINS_INACTIVE
 },
 {.name = "blockcommit",
  .handler = cmdBlockCommit,
@@ -10414,7 +10421,10 @@ const vshCmdDef domManagementCmds[] = {
  .handler = cmdConsole,
  .opts = opts_console,
  .info = info_console,
- .flags = 0
+ .flags = 0,
+ .completer = vshDomainCompleter,
+ .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE |
+VIR_CONNECT_LIST_DOMAINS_INACTIVE
 },
 #endif
 {.name = "cpu-baseline",
@@ -10451,13 +10461,18 @@ const vshCmdDef domManagementCmds[] = {
  .handler = cmdDesc,
  .opts = opts_desc,
  .info = info_desc,
- .flags = 0
+ .flags = 0,
+ .completer = vshDomainCompleter,
+ .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE |
+VIR_CONNECT_LIST_DOMAINS_INACTIVE
 },
 {.name = "destroy",
  .handler = cmdDestroy,
  .opts = opts_destroy,
  .info = info_destroy,
- .flags = 0
+ .flags = 0,
+ .completer = vshDomainCompleter,
+ .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE
 },
 {.name = "detach-device",
  .handler = cmdDetachDevice,
@@ -10481,25 +10496,37 @@ const vshCmdDef domManagementCmds[] = {
  .handler = cmdDomDisplay,
  .opts = opts_domdisplay,
  .info = info_domdisplay,
- .flags = 0
+ .flags = 0,
+ .completer = vshDomainCompleter,
+ .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE |
+VIR_CONNECT_LIST_DOMAINS_INACTIVE
 },
 {.name = "domfstrim",
  .handler = cmdDomFSTrim,
  .opts = opts_domfstrim,
  .info = info_domfstrim,
- .flags = 0
+ .flags = 0,
+ .completer = vshDomainCompleter,
+ .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE |
+VIR_CONNECT_LIST_DOMAINS_INACTIVE
 },
 {.name = "domhostname",
  .handler = cmdDomHostname,
  .opts = opts_domhostname,
  .info = info_domhostname,
- .flags = 0
+ .flags = 0,
+ .completer = vshDomainCompleter,
+ .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE |
+VIR_CONNECT_LIST_DOMAINS_INACTIVE
 },
 {.name = "domid",
  .handler = cmdDomid,
  .opts = opts_domid,
  .info = info_domid,
- .flags = 0
+ .flags = 0,
+ .completer = vshDomainCompleter,
+ .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE |
+VIR_CONNECT_LIST_DOMAINS_INACTIVE
 },
 {.name = "domif-setlink",
  .handler = cmdDomIfSetLink,
@@ -10517,13 +10544,17 @@ const vshCmdDef domManagementCmds[] = {
  .handler = cmdDomjobabort,
  .opts = opts_domjobabort,
  .info = info_domjobabort,
- .flags = 0
+ .flags = 0,
+ .completer = vshDomainCompleter,
+ .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE
 },
 {.name = "domjobinfo",
  .handler = cmdDomjobinf

[libvirt] [PATCH v2 6/9] virsh: Add vshRebootShutdownModeCompleter

2013-08-20 Thread Tomas Meszaros 
vshRebootShutdownModeCompleter returns available modes
for reboot/shutdown commands.
---
 tools/virsh.c | 28 
 tools/virsh.h |  1 +
 2 files changed, 29 insertions(+)

diff --git a/tools/virsh.c b/tools/virsh.c
index 85d74ad..965b141 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -2578,6 +2578,34 @@ error:
 return NULL;
 }
 
+char **
+vshRebootShutdownModeCompleter(unsigned int unused_flags ATTRIBUTE_UNUSED)
+{
+const char *modes[] = {"acpi", "agent", "initctl", "signal"};
+const unsigned int modes_size = ARRAY_CARDINALITY(modes);
+char **names = NULL;
+size_t i;
+
+names = vshMalloc(NULL, sizeof(char *) * (modes_size + 1));
+
+if (!names)
+return NULL;
+
+for (i = 0; i < modes_size; i++) {
+if (VIR_STRDUP(names[i], modes[i]) < 0)
+goto cleanup;
+}
+
+names[i] = NULL;
+return names;
+
+cleanup:
+for (i = 0; names[i]; i++)
+VIR_FREE(names[i]);
+VIR_FREE(names);
+return NULL;
+}
+
 /* -
  * Readline stuff
  * -
diff --git a/tools/virsh.h b/tools/virsh.h
index 6767e65..61510b0 100644
--- a/tools/virsh.h
+++ b/tools/virsh.h
@@ -257,6 +257,7 @@ struct _vshCmdGrp {
 
 char **vshDomainCompleter(unsigned int flags);
 char **vshSuspendTargetCompleter(unsigned int unused_flags);
+char **vshRebootShutdownModeCompleter(unsigned int unused_flags);
 
 void vshError(vshControl *ctl, const char *format, ...)
 ATTRIBUTE_FMT_PRINTF(2, 3);
-- 
1.8.3.1

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

[libvirt] [PATCH v2 8/9] virsh: Add completer functions to snapshotCmds

2013-08-20 Thread Tomas Meszaros 
Add .completer and .completer_flags to snapshotCmds.

Provides domain completion for most of the snapshotCmds commands.
---
 tools/virsh-snapshot.c | 45 -
 1 file changed, 36 insertions(+), 9 deletions(-)

diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
index e37a5b3..77f9273 100644
--- a/tools/virsh-snapshot.c
+++ b/tools/virsh-snapshot.c
@@ -2006,25 +2006,37 @@ const vshCmdDef snapshotCmds[] = {
  .handler = cmdSnapshotCreate,
  .opts = opts_snapshot_create,
  .info = info_snapshot_create,
- .flags = 0
+ .flags = 0,
+ .completer = vshDomainCompleter,
+ .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE |
+VIR_CONNECT_LIST_DOMAINS_INACTIVE
 },
 {.name = "snapshot-create-as",
  .handler = cmdSnapshotCreateAs,
  .opts = opts_snapshot_create_as,
  .info = info_snapshot_create_as,
- .flags = 0
+ .flags = 0,
+ .completer = vshDomainCompleter,
+ .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE |
+VIR_CONNECT_LIST_DOMAINS_INACTIVE
 },
 {.name = "snapshot-current",
  .handler = cmdSnapshotCurrent,
  .opts = opts_snapshot_current,
  .info = info_snapshot_current,
- .flags = 0
+ .flags = 0,
+ .completer = vshDomainCompleter,
+ .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE |
+VIR_CONNECT_LIST_DOMAINS_INACTIVE
 },
 {.name = "snapshot-delete",
  .handler = cmdSnapshotDelete,
  .opts = opts_snapshot_delete,
  .info = info_snapshot_delete,
- .flags = 0
+ .flags = 0,
+ .completer = vshDomainCompleter,
+ .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE |
+VIR_CONNECT_LIST_DOMAINS_INACTIVE
 },
 {.name = "snapshot-dumpxml",
  .handler = cmdSnapshotDumpXML,
@@ -2036,31 +2048,46 @@ const vshCmdDef snapshotCmds[] = {
  .handler = cmdSnapshotEdit,
  .opts = opts_snapshot_edit,
  .info = info_snapshot_edit,
- .flags = 0
+ .flags = 0,
+ .completer = vshDomainCompleter,
+ .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE |
+VIR_CONNECT_LIST_DOMAINS_INACTIVE
 },
 {.name = "snapshot-info",
  .handler = cmdSnapshotInfo,
  .opts = opts_snapshot_info,
  .info = info_snapshot_info,
- .flags = 0
+ .flags = 0,
+ .completer = vshDomainCompleter,
+ .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE |
+VIR_CONNECT_LIST_DOMAINS_INACTIVE
 },
 {.name = "snapshot-list",
  .handler = cmdSnapshotList,
  .opts = opts_snapshot_list,
  .info = info_snapshot_list,
- .flags = 0
+ .flags = 0,
+ .completer = vshDomainCompleter,
+ .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE |
+VIR_CONNECT_LIST_DOMAINS_INACTIVE
 },
 {.name = "snapshot-parent",
  .handler = cmdSnapshotParent,
  .opts = opts_snapshot_parent,
  .info = info_snapshot_parent,
- .flags = 0
+ .flags = 0,
+ .completer = vshDomainCompleter,
+ .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE |
+VIR_CONNECT_LIST_DOMAINS_INACTIVE
 },
 {.name = "snapshot-revert",
  .handler = cmdDomainSnapshotRevert,
  .opts = opts_snapshot_revert,
  .info = info_snapshot_revert,
- .flags = 0
+ .flags = 0,
+ .completer = vshDomainCompleter,
+ .completer_flags = VIR_CONNECT_LIST_DOMAINS_ACTIVE |
+VIR_CONNECT_LIST_DOMAINS_INACTIVE
 },
 {.name = NULL}
 };
-- 
1.8.3.1

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

[libvirt] [PATCH v2 4/9] virsh: Add vshDomainCompleter

2013-08-20 Thread Tomas Meszaros 
* global variable __my_conn renamed to vshConn
* @name is now const char *
* label cleanup renamed to error
---
 tools/virsh.c | 53 +
 tools/virsh.h |  2 ++
 2 files changed, 55 insertions(+)

diff --git a/tools/virsh.c b/tools/virsh.c
index af6e939..13c27df 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -88,6 +88,8 @@ static char *progname;
 
 static const vshCmdGrp cmdGroups[];
 
+virConnectPtr *vshConn;
+
 /* Bypass header poison */
 #undef strdup
 
@@ -2503,6 +2505,51 @@ vshCloseLogFile(vshControl *ctl)
 
 #ifdef USE_READLINE
 
+/* -
+ * Completers
+ * -
+ */
+
+char **
+vshDomainCompleter(unsigned int flags)
+{
+virDomainPtr *domains;
+size_t i;
+char **names = NULL;
+int ndomains;
+
+if (!*vshConn)
+return NULL;
+
+ndomains = virConnectListAllDomains(*vshConn, &domains, flags);
+
+if (ndomains < 0)
+return NULL;
+
+names = vshMalloc(NULL, sizeof(char *) * (ndomains + 1));
+
+if (!names)
+return NULL;
+
+for (i = 0; i < ndomains; i++) {
+const char *name = virDomainGetName(domains[i]);
+if (VIR_STRDUP(names[i], name) < 0) {
+virDomainFree(domains[i]);
+goto error;
+}
+virDomainFree(domains[i]);
+}
+names[i] = NULL;
+VIR_FREE(domains);
+return names;
+
+error:
+for (i = 0; names[i]; i++)
+VIR_FREE(names[i]);
+VIR_FREE(names);
+return NULL;
+}
+
 /* -
  * Readline stuff
  * -
@@ -3510,6 +3557,7 @@ main(int argc, char **argv)
 ctl->debug = VSH_DEBUG_DEFAULT;
 ctl->escapeChar = "^]"; /* Same default as telnet */
 
+vshConn = &ctl->conn;
 
 if (!setlocale(LC_ALL, "")) {
 perror("setlocale");
@@ -3571,6 +3619,11 @@ main(int argc, char **argv)
 exit(EXIT_FAILURE);
 }
 
+/* Need to connect immediately after start in order to provide
+ * autocompletion for the very first command.
+ */
+vshReconnect(ctl);
+
 do {
 const char *prompt = ctl->readonly ? VSH_PROMPT_RO : VSH_PROMPT_RW;
 ctl->cmdstr =
diff --git a/tools/virsh.h b/tools/virsh.h
index 064acde..68414e4 100644
--- a/tools/virsh.h
+++ b/tools/virsh.h
@@ -255,6 +255,8 @@ struct _vshCmdGrp {
 const vshCmdDef *commands;
 };
 
+char **vshDomainCompleter(unsigned int flags);
+
 void vshError(vshControl *ctl, const char *format, ...)
 ATTRIBUTE_FMT_PRINTF(2, 3);
 void vshOpenLogFile(vshControl *ctl);
-- 
1.8.3.1

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

[libvirt] [PATCH v2 5/9] virsh: Add vshSuspendTargetCompleter

2013-08-20 Thread Tomas Meszaros 
* label cleanup renamed to error
* vshSuspendTargetCompleter added to opts_dom_pm_suspend
---
 tools/virsh-domain.c |  3 ++-
 tools/virsh.c| 28 
 tools/virsh.h|  1 +
 3 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 5d4913d..a2002c5 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -2756,7 +2756,8 @@ static const vshCmdOptDef opts_dom_pm_suspend[] = {
  .flags = VSH_OFLAG_REQ,
  .help = N_("mem(Suspend-to-RAM), "
 "disk(Suspend-to-Disk), "
-"hybrid(Hybrid-Suspend)")
+"hybrid(Hybrid-Suspend)"),
+ .completer = vshSuspendTargetCompleter
 },
 {.name = NULL}
 };
diff --git a/tools/virsh.c b/tools/virsh.c
index 13c27df..85d74ad 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -2550,6 +2550,34 @@ error:
 return NULL;
 }
 
+char **
+vshSuspendTargetCompleter(unsigned int unused_flags ATTRIBUTE_UNUSED)
+{
+const char *targets[] = {"mem", "disk", "hybrid"};
+const unsigned int targets_size = ARRAY_CARDINALITY(targets);
+char **names = NULL;
+size_t i;
+
+names = vshMalloc(NULL, sizeof(char *) * (targets_size + 1));
+
+if (!names)
+return NULL;
+
+for (i = 0; i < targets_size; i++) {
+if (VIR_STRDUP(names[i], targets[i]) < 0)
+goto error;
+}
+
+names[i] = NULL;
+return names;
+
+error:
+for (i = 0; names[i]; i++)
+VIR_FREE(names[i]);
+VIR_FREE(names);
+return NULL;
+}
+
 /* -
  * Readline stuff
  * -
diff --git a/tools/virsh.h b/tools/virsh.h
index 68414e4..6767e65 100644
--- a/tools/virsh.h
+++ b/tools/virsh.h
@@ -256,6 +256,7 @@ struct _vshCmdGrp {
 };
 
 char **vshDomainCompleter(unsigned int flags);
+char **vshSuspendTargetCompleter(unsigned int unused_flags);
 
 void vshError(vshControl *ctl, const char *format, ...)
 ATTRIBUTE_FMT_PRINTF(2, 3);
-- 
1.8.3.1

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

[libvirt] [PATCH v2 3/9] virsh: Improve readline generators and readline completion

2013-08-20 Thread Tomas Meszaros 
* vshMalloc is now used in vshDetermineCommandName
* vshStrdup instead of vshMalloc + snprintf
* joined if's in vshReadlineOptionsCompletionGenerator
---
 tools/virsh.c | 395 ++
 1 file changed, 373 insertions(+), 22 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c
index 15f529e..af6e939 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -2508,6 +2508,25 @@ vshCloseLogFile(vshControl *ctl)
  * -
  */
 
+static const vshCmdDef *
+vshDetermineCommandName(void)
+{
+const vshCmdDef *cmd = NULL;
+char *p;
+char *cmdname;
+
+if (!(p = strchr(rl_line_buffer, ' ')))
+return NULL;
+
+cmdname = vshMalloc(NULL, (p - rl_line_buffer) + 1);
+memcpy(cmdname, rl_line_buffer, p - rl_line_buffer);
+
+cmd = vshCmddefSearch(cmdname);
+VIR_FREE(cmdname);
+
+return cmd;
+}
+
 /*
  * Generator function for command completion.  STATE lets us
  * know whether to start from scratch; without any state
@@ -2555,25 +2574,14 @@ vshReadlineCommandGenerator(const char *text, int state)
 static char *
 vshReadlineOptionsGenerator(const char *text, int state)
 {
-static int list_index, len;
 static const vshCmdDef *cmd = NULL;
+static int list_index, len;
 const char *name;
 
 if (!state) {
-/* determine command name */
-char *p;
-char *cmdname;
-
-if (!(p = strchr(rl_line_buffer, ' ')))
-return NULL;
-
-cmdname = vshCalloc(NULL, (p - rl_line_buffer) + 1, 1);
-memcpy(cmdname, rl_line_buffer, p - rl_line_buffer);
-
-cmd = vshCmddefSearch(cmdname);
+cmd = vshDetermineCommandName();
 list_index = 0;
 len = strlen(text);
-VIR_FREE(cmdname);
 }
 
 if (!cmd)
@@ -2605,22 +2613,365 @@ vshReadlineOptionsGenerator(const char *text, int 
state)
 return NULL;
 }
 
+/*
+ * Generator function for command completion, but unlike
+ * the vshRaadlineCommandGenerator which completes command name, this function
+ * provides more advanced completion for commands by calling specific command
+ * completers (e.g. vshDomainCompleter).
+ */
+static char *
+vshReadlineCommandCompletionGenerator(const char *text, int state)
+{
+static const vshCmdDef *cmd = NULL;
+static int list_index, len;
+char **completed_names = NULL;
+char *name;
+
+if (!state) {
+cmd = vshDetermineCommandName();
+list_index = 0;
+len = strlen(text);
+}
+
+if (!cmd)
+return NULL;
+
+if (!cmd->completer)
+return NULL;
+
+completed_names = cmd->completer(cmd->completer_flags);
+
+if (!completed_names)
+return NULL;
+
+while ((name = completed_names[list_index])) {
+char *res;
+list_index++;
+
+if (STRNEQLEN(name, text, len))
+/* Skip irrelevant names */
+continue;
+
+res = vshStrdup(NULL, name);
+VIR_FREE(name);
+return res;
+}
+VIR_FREE(completed_names);
+
+return NULL;
+}
+
+/*
+ * Generator function for command option completion. Provides advances
+ * completion for command options.
+ */
+static char *
+vshReadlineOptionsCompletionGenerator(const char *text ATTRIBUTE_UNUSED,
+  int state ATTRIBUTE_UNUSED)
+{
+static const vshCmdDef *cmd = NULL;
+static const vshCmdOptDef *opt = NULL;
+static int list_index, len;
+unsigned long int opt_index = 0;
+size_t i;
+char **completed_names = NULL;
+char *name;
+char *ptr = NULL;
+
+if (!state) {
+cmd = vshDetermineCommandName();
+list_index = 0;
+len = strlen(text);
+}
+
+if (!cmd)
+return NULL;
+
+if (!cmd->opts)
+return NULL;
+
+for (i = 0; cmd->opts[i].name; i++) {
+if ((ptr = strstr(rl_line_buffer, cmd->opts[i].name)) &&
+(opt_index < (ptr - rl_line_buffer))) {
+opt_index = ptr - rl_line_buffer;
+opt = &cmd->opts[i];
+}
+}
+
+if (!opt)
+return NULL;
+
+if (!opt->completer)
+return NULL;
+
+completed_names = opt->completer(opt->completer_flags);
+
+if (!completed_names)
+return NULL;
+
+while ((name = completed_names[list_index])) {
+char *res;
+list_index++;
+
+if (STRNEQLEN(name, text, len))
+/* Skip irrelevant names */
+continue;
+
+res = vshStrdup(NULL, name);
+VIR_FREE(name);
+return res;
+}
+VIR_FREE(completed_names);
+
+return NULL;
+}
+
+/*
+ * Returns current valid command name present in the rl_line_buffer.
+ */
+static char *
+vshCurrentCmd(void)
+{
+const char *name;
+const vshCmdGrp *grp;
+const vshCmdDef *cmds;
+size_t grp_list_index, cmd_list_index;
+char *found_cmd = NULL;
+char *rl_copy = NULL;
+char *pch;
+
+grp_list_index = 0;
+cmd_list_index = 0;
+grp = cmdGroups

[libvirt] [PATCH v2 1/9] virsh: C99 style for info_domfstrim and opts_lxc_enter_namespace

2013-08-20 Thread Tomas Meszaros 
Change info_domfstrim and opts_lxc_enter_namespace initialization style
to C99.
---
 tools/virsh-domain.c | 21 -
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index b29f934..5d4913d 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -7903,10 +7903,21 @@ static const vshCmdInfo info_lxc_enter_namespace[] = {
 };
 
 static const vshCmdOptDef opts_lxc_enter_namespace[] = {
-{"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")},
-{"noseclabel", VSH_OT_BOOL, 0, N_("Do not change process security label")},
-{"cmd", VSH_OT_ARGV, VSH_OFLAG_REQ, N_("namespace")},
-{NULL, 0, 0, NULL}
+{.name = "domain",
+ .type = VSH_OT_DATA,
+ .flags = VSH_OFLAG_REQ,
+ .help = N_("domain name, id or uuid")
+},
+{.name = "noseclabel",
+ .type = VSH_OT_BOOL,
+ .help = N_("Do not change process security label")
+},
+{.name = "cmd",
+ .type = VSH_OT_ARGV,
+ .flags = VSH_OFLAG_REQ,
+ .help = N_("namespace")
+},
+{.name = NULL}
 };
 
 static bool
@@ -10290,7 +10301,7 @@ static const vshCmdOptDef opts_domfstrim[] = {
  .type = VSH_OT_DATA,
  .help = N_("which mount point to trim")
 },
-{NULL, 0, 0, NULL}
+{.name = NULL}
 };
 static bool
 cmdDomFSTrim(vshControl *ctl, const vshCmd *cmd)
-- 
1.8.3.1

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

[libvirt] [PATCH v2 0/9] virsh: More intelligent auto-completion

2013-08-20 Thread Tomas Meszaros 
Original series + some fixes/improvements.

Tomas Meszaros (9):
  virsh: C99 style for info_domfstrim and opts_lxc_enter_namespace
  virsh: Add vshCmdCompleter and vshOptCompleter
  virsh: Improve readline generators and readline completion
  virsh: Add vshDomainCompleter
  virsh: Add vshSuspendTargetCompleter
  virsh: Add vshRebootShutdownModeCompleter
  virsh: Add completer functions to domManaggementCmds
  virsh: Add completer functions to snapshotCmds
  virsh: Add completer functions to domMonitoringCmds

 tools/virsh-domain-monitor.c |  32 ++-
 tools/virsh-domain.c | 248 -
 tools/virsh-snapshot.c   |  45 +++-
 tools/virsh.c| 504 +--
 tools/virsh.h|  11 +
 5 files changed, 748 insertions(+), 92 deletions(-)

-- 
1.8.3.1

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

Re: [libvirt] [PATCH] util:report diskchain as broken if without enough permission

2013-08-20 Thread Eric Blake 
On 08/20/2013 01:54 AM, Guannan Ren wrote:
> When backing files of a disk file are stored in NFS shared folder,
> qemu process requires at least the read permission.
> But in some rare situation, these backing files can not even be read
> If so, we should treat the diskchains as broken.
> This patch add a checking for this, report negative in such case.

We have been historically bitten by changes to this code.  I think we
need to patch tests/virstoragetest.c (and/or add a new test) before
accepting this change, to make sure that we are testing the behavior we
want.

-- 
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

Re: [libvirt] [PATCH 2/2 v3] Check for --no-copy-dt-needed linker flag

2013-08-20 Thread Eric Blake 
On 08/20/2013 08:40 AM, Guido Günther wrote:
> and use it when available
> ---

> +AC_DEFUN([LIBVIRT_LINKER_NO_INDIRECT],[
> +AC_MSG_CHECKING([for how to avoid indirect lib deps])
> +
> +NO_INDIRECT_LDFLAGS=
> +`$LD --help 2>&1 | grep -- "--no-copy-dt-needed-entries" >/dev/null` && \
> +NO_INDIRECT_LDFLAGS="-Wl,--no-copy-dt-needed-entries"
> +AC_SUBST([NO_INDIRECT_LDFLAGS])

You fixed the relro detection to use case instead of ``&&..., but forgot
to fix this one.

> +
> +AC_MSG_RESULT([$NO_INDIRECT_LDFLAGS])
> +])
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 7c3d8a1..faa2cd6 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -35,6 +35,7 @@ AM_CFLAGS = $(LIBXML_CFLAGS)
> \
>  AM_LDFLAGS = $(DRIVER_MODULE_LDFLAGS)\
>   $(COVERAGE_LDFLAGS) \
>   $(RELRO_LDFLAGS)\
> + $(NO_INDIRECT_LDFLAGS)  \

Of course, depending on the verdict on 1/2 will determine whether this
patch applies or must be split into a per-binary usage.

-- 
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

[libvirt] [PATCH] docs: Reformat attribute description in formatdomain

2013-08-20 Thread John Ferlan 
Reformat the description to more cleanly delineate the attributes
for a  element.
---

Similar to the recently changed  attribute.

 docs/formatdomain.html.in | 121 +++---
 1 file changed, 71 insertions(+), 50 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 0d1a74f..9d12293 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1562,56 +1562,77 @@
 
   disk
   The disk element is the main container for describing
-disks. The type attribute is either "file",
-"block", "dir", "network", or "volume"
-and refers to the underlying source for the disk. The optional
-device attribute indicates how the disk is to be exposed
-to the guest OS. Possible values for this attribute are
-"floppy", "disk", "cdrom", and "lun", defaulting to
-"disk". "lun" (since 0.9.10) is only
-valid when type is "block" and the target element's "bus"
-attribute is "virtio", and behaves identically to "disk",
-except that generic SCSI commands from the guest are accepted
-and passed through to the physical device
-- also note that device='lun' will only be recognized for
-actual raw devices, never for individual partitions or LVM
-partitions (in those cases, the kernel will reject the generic
-SCSI commands, making it identical to device='disk').
-The optional rawio attribute
-(since 0.9.10) indicates whether
-the disk is needs rawio capability; valid settings are "yes"
-or "no" (default is "no"). If any one disk in a domain has
-rawio='yes', rawio capability will be enabled for all disks in
-the domain (because, in the case of QEMU, this capability can
-only be set on a per-process basis). This attribute is only
-valid when device is "lun". NB, rawio intends to
-confine the capability per-device, however, current QEMU
-implementation gives the domain process broader capability
-than that (per-process basis, affects all the domain disks).
-To confine the capability as much as possible for QEMU driver
-as this stage, sgio is recommended, it's more
-secure than rawio.
-The optional sgio (since 1.0.2)
-attribute indicates whether the kernel will filter unprivileged
-SG_IO commands for the disk, valid settings are "filtered" or
-"unfiltered". Defaults to "filtered". Similar to rawio,
-sgio is only valid for device 'lun'.
-The optional snapshot attribute indicates the default
-behavior of the disk during disk snapshots: "internal"
-requires a file format such as qcow2 that can store both the
-snapshot and the data changes since the snapshot;
-"external" will separate the snapshot from the live data; and
-"no" means the disk will not participate in snapshots.
-Read-only disks default to "no", while the default for other
-disks depends on the hypervisor's capabilities.  Some
-hypervisors allow a per-snapshot choice as well,
-during domain snapshot
-creation.  Not all snapshot modes are supported;
-for example, snapshot='yes' with a transient disk
-generally does not make sense.  Since 0.0.3;
-"device" attribute since 0.1.4;
-"network" attribute since 0.8.7; "snapshot" since
-0.9.5
+  disks (since 0.0.3).
+
+  type attribute
+  since 0.0.3
+
+Valid values are "file", "block",
+"dir" (since 0.7.5),
+"network" (since 0.8.7), or
+"volume" (since 1.0.5)
+and refer to the underlying source for the disk.
+
+  device attribute
+  since 0.1.4
+
+Indicates how the disk is to be exposed to the guest OS. Possible
+values for this attribute are "floppy", "disk", "cdrom", and "lun",
+defaulting to "disk".
+
+Using "lun" (since 0.9.10) is only
+valid when type is "block" and the target element's "bus"
+attribute is "virtio", and behaves identically to "disk",
+except that generic SCSI commands from the guest are accepted
+and passed through to the physical device. Also note that
+device='lun' will only be recognized for actual raw devices,
+but never for individual partitions or LVM partitions (in those
+cases, the kernel will reject the generic SCSI commands, making
+it identical to device='disk').
+
+
+  rawio attribute
+  since 0.9.10
+
+Indicates whether the disk is needs rawio capability; valid
+settings are "yes" or "no" (default is "no"). If any one disk
+in a domain has rawi

Re: [libvirt] [PATCH v2] Check for --no-copy-dt-needed linker flag

2013-08-20 Thread Guido Günther 
On Mon, Aug 19, 2013 at 01:36:36PM -0600, Eric Blake wrote:
> On 08/19/2013 12:07 PM, Guido Günther wrote:
> 
> >>> +NO_INDIRECT_LDFLAGS=
> >>> +`$LD --help 2>&1 | grep -- "--no-copy-dt-needed-entries" >/dev/null` 
> >>> && \
> >>
> >> Doesn't do what you think (it tries to execute the output of grep -
> >> which is thankfully empty on both success and failure).  Also wastes a
> >> fork on grep, compared to the simpler:
> > 
> > I was looking following virt-linker-relro.m4 and I'm getting the correct
> > result since the output is always empty but for the non matching case
> > grep exits with 1. However
> > 
> >>
> >> case `$LD --help 2>&1` in
> >>   *--no-copy-dt-needed-entries*) NO_INDIRECT_LDFLAGS=... ;;
> >> esac
> > 
> > this looks nicer.
> 
> Oh sure - blame copy-and-paste of bad habits :)

Not actually blaming more following current practice. We're not using
AM_LDFLAGS everywhere. This should be a sperate cleanup in case this is
considered useful. Which I think would be since we're misusing LDADDS in
e.g. tests/Makefile.am for cflags, linkerflags and actually adding
additional libraries.
Cheers,
 -- Guido

> 
> >> Why aren't you building this directly into $(AM_LDFLAGS) once, rather
> >> than having to copy it into each and every library recipe?
> >>
> >> Why only src/ and tests/? What about tools/ and daemon/?
> > 
> > Again following the above example. I'll come up with a v3.
> 
> For a v3, it might be nice to have two patches - one that cleans up the
> bad habits of RELRO handling, and the second that extends it with the
> cleaner idioms.  Thanks for putting up with me :)
> 
> -- 
> 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

[libvirt] [PATCH 2/2 v3] Check for --no-copy-dt-needed linker flag

2013-08-20 Thread Guido Günther 
and use it when available
---
 configure.ac  |  1 +
 daemon/Makefile.am|  1 +
 m4/virt-linker-no-indirect.m4 | 30 ++
 src/Makefile.am   |  1 +
 tests/Makefile.am |  1 +
 tools/Makefile.am |  1 +
 6 files changed, 35 insertions(+)
 create mode 100644 m4/virt-linker-no-indirect.m4

diff --git a/configure.ac b/configure.ac
index ac8cfa1..25d91ce 100644
--- a/configure.ac
+++ b/configure.ac
@@ -160,6 +160,7 @@ AC_MSG_RESULT([$VERSION_SCRIPT_FLAGS])
 LIBVIRT_COMPILE_WARNINGS
 LIBVIRT_COMPILE_PIE
 LIBVIRT_LINKER_RELRO
+LIBVIRT_LINKER_NO_INDIRECT
 
 LIBVIRT_CHECK_APPARMOR
 LIBVIRT_CHECK_ATTR
diff --git a/daemon/Makefile.am b/daemon/Makefile.am
index 5cd95aa..e34868b 100644
--- a/daemon/Makefile.am
+++ b/daemon/Makefile.am
@@ -129,6 +129,7 @@ libvirtd_LDFLAGS =  \
$(PIE_LDFLAGS)  \
$(RELRO_LDFLAGS)\
$(COVERAGE_LDFLAGS) \
+   $(NO_INDIRECT_LDFLAGS)  \
$(NULL)
 
 libvirtd_LDADD =   \
diff --git a/m4/virt-linker-no-indirect.m4 b/m4/virt-linker-no-indirect.m4
new file mode 100644
index 000..2ccd960
--- /dev/null
+++ b/m4/virt-linker-no-indirect.m4
@@ -0,0 +1,30 @@
+dnl
+dnl Check for --no-copy-dt-needed-entries
+dnl
+dnl Copyright (C) 2013 Guido Günther 
+dnl
+dnl This library is free software; you can redistribute it and/or
+dnl modify it under the terms of the GNU Lesser General Public
+dnl License as published by the Free Software Foundation; either
+dnl version 2.1 of the License, or (at your option) any later version.
+dnl
+dnl This library is distributed in the hope that it will be useful,
+dnl but WITHOUT ANY WARRANTY; without even the implied warranty of
+dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+dnl Lesser General Public License for more details.
+dnl
+dnl You should have received a copy of the GNU Lesser General Public
+dnl License along with this library.  If not, see
+dnl .
+dnl
+
+AC_DEFUN([LIBVIRT_LINKER_NO_INDIRECT],[
+AC_MSG_CHECKING([for how to avoid indirect lib deps])
+
+NO_INDIRECT_LDFLAGS=
+`$LD --help 2>&1 | grep -- "--no-copy-dt-needed-entries" >/dev/null` && \
+NO_INDIRECT_LDFLAGS="-Wl,--no-copy-dt-needed-entries"
+AC_SUBST([NO_INDIRECT_LDFLAGS])
+
+AC_MSG_RESULT([$NO_INDIRECT_LDFLAGS])
+])
diff --git a/src/Makefile.am b/src/Makefile.am
index 7c3d8a1..faa2cd6 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -35,6 +35,7 @@ AM_CFLAGS =   $(LIBXML_CFLAGS)
\
 AM_LDFLAGS =   $(DRIVER_MODULE_LDFLAGS)\
$(COVERAGE_LDFLAGS) \
$(RELRO_LDFLAGS)\
+   $(NO_INDIRECT_LDFLAGS)  \
$(NULL)
 
 EXTRA_DIST = $(conf_DATA) util/keymaps.csv
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 9098dec..86c3e11 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -50,6 +50,7 @@ endif
 
 LDADDS = \
 $(WARN_CFLAGS) \
+   $(NO_INDIRECT_LDFLAGS) \
$(PROBES_O) \
../src/libvirt.la \
../gnulib/lib/libgnu.la
diff --git a/tools/Makefile.am b/tools/Makefile.am
index be7ed23..c5bd5bb 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -24,6 +24,7 @@ INCLUDES = \
 
 AM_LDFLAGS = \
$(RELRO_LDFLAGS)\
+   $(NO_INDIRECT_LDFLAGS)  \
$(NULL)
 
 POD2MAN = pod2man -c "Virtualization Support" -r "$(PACKAGE)-$(VERSION)"
-- 
1.8.4.rc3

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

Re: [libvirt] [PATCH 1/2] Simplify RELRO_LDFLAGS

2013-08-20 Thread Eric Blake 
On 08/20/2013 08:39 AM, Guido Günther wrote:
> by adding it to AM_LDFLAGS instead of every linking rule and
> by avoiding a forked grep.
> ---
> Daniel kind of nacked the AM_LDFLAGS part already but I think it's a
> reasonable cleanup. We should rather use AM_LDFLAGS everywhere which
> (we currently don't and which would be another cleanup). Or are there
> any reasons to not have a read only GOT?

Personally, I like the idea of using AM_LDFLAGS everywhere, but as
Daniel and I have opposing opinions, we'll need a third person to speak up.

> +++ b/m4/virt-linker-relro.m4
> @@ -22,10 +22,10 @@ AC_DEFUN([LIBVIRT_LINKER_RELRO],[
>  AC_MSG_CHECKING([for how to force completely read-only GOT table])
>  
>  RELRO_LDFLAGS=
> -`$LD --help 2>&1 | grep -- "-z relro" >/dev/null` && \
> -RELRO_LDFLAGS="-Wl,-z -Wl,relro"
> -`$LD --help 2>&1 | grep -- "-z now" >/dev/null` && \
> -RELRO_LDFLAGS="$RELRO_LDFLAGS -Wl,-z -Wl,now"
> +case `$LD --help 2>&1` in
> +*"-z relro"*) RELRO_LDFLAGS="-Wl,-z -Wl,relro" ;&

';&' is a bashism; it is not portable to configure scripts

> +*"-z now"*)   RELRO_LDFLAGS="$RELRO_LDFLAGS -Wl,-z -Wl,now" ;;
> +esac

Instead, this needs to be:

ld_help=`$LD --help 2>&1`
case $ld_help in
  *"-z relro"*) RELRO_LDFLAGS="-Wl,-z -Wl,relro" ;;
esac
case $ld_help in
  *"-z now"*) RELRO_LDFLAGS="$RELRO_LDFLAGS -Wl,-z -Wl,now" ;;
esac


> @@ -1812,7 +1814,6 @@ libvirt_la_LDFLAGS = \
>   -version-info $(LIBVIRT_VERSION_INFO) \
>   $(LIBVIRT_NODELETE) \
>   $(AM_LDFLAGS) \
> - $(RELRO_LDFLAGS) \
>   $(CYGWIN_EXTRA_LDFLAGS) \
>   $(MINGW_EXTRA_LDFLAGS) \
>   $(NULL)

Hmm - maybe I see why Daniel expressed his opinion for not using
$(AM_LDFLAGS) - notice that libraries to NOT use $(PIE_LDFLAGS)...

> @@ -1964,7 +1963,6 @@ virtlockd_CFLAGS = \
>  virtlockd_LDFLAGS = \
>   $(AM_LDFLAGS) \
>   $(PIE_LDFLAGS) \
> - $(RELRO_LDFLAGS) \
>   $(CYGWIN_EXTRA_LDFLAGS) \
>   $(MINGW_EXTRA_LDFLAGS) \
>   $(NULL)

...but executables do.  But even then, maybe we want:

LIBRARY_LDFLAGS = $(AM_LDFLAGS)
EXEC_LDFLAGS = $(AM_LDFLAGS) $(PIE_LDFLAGS)

then use the appropriate $({LIBRARY,EXEC}_LDFLAGS) in each place, rather
than having lots of duplicate $(PIE_LDFLAGS)?

-- 
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

[libvirt] [PATCH 1/2] Simplify RELRO_LDFLAGS

2013-08-20 Thread Guido Günther 
by adding it to AM_LDFLAGS instead of every linking rule and
by avoiding a forked grep.
---
Daniel kind of nacked the AM_LDFLAGS part already but I think it's a
reasonable cleanup. We should rather use AM_LDFLAGS everywhere which
(we currently don't and which would be another cleanup). Or are there
any reasons to not have a read only GOT?

 daemon/Makefile.am  |  4 +++-
 m4/virt-linker-relro.m4 |  8 
 src/Makefile.am | 13 +++--
 tools/Makefile.am   | 18 +-
 4 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/daemon/Makefile.am b/daemon/Makefile.am
index ad7544c..5cd95aa 100644
--- a/daemon/Makefile.am
+++ b/daemon/Makefile.am
@@ -125,9 +125,11 @@ libvirtd_CFLAGS = \
-DQEMUD_PID_FILE="\"$(QEMUD_PID_FILE)\""
 
 libvirtd_LDFLAGS = \
+   $(RELRO_LDFLAGS)\
$(PIE_LDFLAGS)  \
$(RELRO_LDFLAGS)\
-   $(COVERAGE_LDFLAGS)
+   $(COVERAGE_LDFLAGS) \
+   $(NULL)
 
 libvirtd_LDADD =   \
$(LIBXML_LIBS)  \
diff --git a/m4/virt-linker-relro.m4 b/m4/virt-linker-relro.m4
index 9bca90e..d287cbc 100644
--- a/m4/virt-linker-relro.m4
+++ b/m4/virt-linker-relro.m4
@@ -22,10 +22,10 @@ AC_DEFUN([LIBVIRT_LINKER_RELRO],[
 AC_MSG_CHECKING([for how to force completely read-only GOT table])
 
 RELRO_LDFLAGS=
-`$LD --help 2>&1 | grep -- "-z relro" >/dev/null` && \
-RELRO_LDFLAGS="-Wl,-z -Wl,relro"
-`$LD --help 2>&1 | grep -- "-z now" >/dev/null` && \
-RELRO_LDFLAGS="$RELRO_LDFLAGS -Wl,-z -Wl,now"
+case `$LD --help 2>&1` in
+*"-z relro"*) RELRO_LDFLAGS="-Wl,-z -Wl,relro" ;&
+*"-z now"*)   RELRO_LDFLAGS="$RELRO_LDFLAGS -Wl,-z -Wl,now" ;;
+esac
 AC_SUBST([RELRO_LDFLAGS])
 
 AC_MSG_RESULT([$RELRO_LDFLAGS])
diff --git a/src/Makefile.am b/src/Makefile.am
index 4702cde..7c3d8a1 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -33,7 +33,9 @@ AM_CFLAGS =   $(LIBXML_CFLAGS)
\
$(WIN32_EXTRA_CFLAGS)   \
$(COVERAGE_CFLAGS)
 AM_LDFLAGS =   $(DRIVER_MODULE_LDFLAGS)\
-   $(COVERAGE_LDFLAGS)
+   $(COVERAGE_LDFLAGS) \
+   $(RELRO_LDFLAGS)\
+   $(NULL)
 
 EXTRA_DIST = $(conf_DATA) util/keymaps.csv
 
@@ -1812,7 +1814,6 @@ libvirt_la_LDFLAGS = \
-version-info $(LIBVIRT_VERSION_INFO) \
$(LIBVIRT_NODELETE) \
$(AM_LDFLAGS) \
-   $(RELRO_LDFLAGS) \
$(CYGWIN_EXTRA_LDFLAGS) \
$(MINGW_EXTRA_LDFLAGS) \
$(NULL)
@@ -1896,7 +1897,6 @@ libvirt_qemu_la_LDFLAGS = \
$(VERSION_SCRIPT_FLAGS)$(LIBVIRT_QEMU_SYMBOL_FILE) \
-version-info $(LIBVIRT_VERSION_INFO) \
$(AM_LDFLAGS) \
-   $(RELRO_LDFLAGS) \
$(CYGWIN_EXTRA_LDFLAGS) \
$(MINGW_EXTRA_LDFLAGS) \
$(NULL)
@@ -1908,7 +1908,6 @@ libvirt_lxc_la_LDFLAGS = \
$(VERSION_SCRIPT_FLAGS)$(LIBVIRT_LXC_SYMBOL_FILE) \
-version-info $(LIBVIRT_VERSION_INFO) \
$(AM_LDFLAGS) \
-   $(RELRO_LDFLAGS) \
$(CYGWIN_EXTRA_LDFLAGS) \
$(MINGW_EXTRA_LDFLAGS) \
$(NULL)
@@ -1964,7 +1963,6 @@ virtlockd_CFLAGS = \
 virtlockd_LDFLAGS = \
$(AM_LDFLAGS) \
$(PIE_LDFLAGS) \
-   $(RELRO_LDFLAGS) \
$(CYGWIN_EXTRA_LDFLAGS) \
$(MINGW_EXTRA_LDFLAGS) \
$(NULL)
@@ -2243,7 +2241,6 @@ libvirt_iohelper_SOURCES = $(UTIL_IO_HELPER_SOURCES)
 libvirt_iohelper_LDFLAGS = \
$(AM_LDFLAGS) \
$(PIE_LDFLAGS) \
-   $(RELRO_LDFLAGS) \
$(NULL)
 libvirt_iohelper_LDADD =   \
libvirt_util.la \
@@ -2266,7 +2263,6 @@ libvirt_parthelper_SOURCES = 
$(STORAGE_HELPER_DISK_SOURCES)
 libvirt_parthelper_LDFLAGS = \
$(AM_LDFLAGS) \
$(PIE_LDFLAGS) \
-   $(RELRO_LDFLAGS) \
$(NULL)
 libvirt_parthelper_LDADD = \
$(LIBPARTED_LIBS)   \
@@ -2298,7 +2294,6 @@ libvirt_sanlock_helper_CFLAGS = \
 libvirt_sanlock_helper_LDFLAGS = \
$(AM_LDFLAGS) \
$(PIE_LDFLAGS) \
-   $(RELRO_LDFLAGS) \
$(NULL)
 libvirt_sanlock_helper_LDADD = libvirt.la
 endif
@@ -2314,7 +2309,6 @@ libvirt_lxc_SOURCES = 
\
 libvirt_lxc_LDFLAGS = \
$(AM_LDFLAGS) \
$(PIE_LDFLAGS) \
-   $(RELRO_LD

Re: [libvirt] [PATCH] python: return dictionay without value in case of no blockjob

2013-08-20 Thread Eric Blake 
On 07/15/2013 04:13 AM, Guannan Ren wrote:
> On 07/13/2013 01:36 AM, Michal Privoznik wrote:
>> On 17.05.2013 08:30, Guannan Ren wrote:
>> s/dictionay/dictionary/ in $SUBJ
>>
>>> Currently, when there is no blockjob, dom.blockJobInfo('vda')
>>> still reports error because it didn't distinguish return value 0 from
>>> -1.
>> s/didn't/doesn't/
>>
>>> libvirt.libvirtError: virDomainGetBlockJobInfo() failed
>>>
>>> virDomainGetBlockJobInfo() API return value:
>>>   -1 in case of failure, 0 when nothing found, 1 found.
>>>
>>> And use PyDict_SetItemString instead of PyDict_SetItem when key is
>>> string type. PyDict_SetItemString increments key/value reference
>> s/string type/of string type/
>>
>>> count, so calling Py_DECREF() for value. For key, we don't need to
>> s/calling/call/
>>
>>> do this, because PyDict_SetItemString will handle it internally.
>>> ---
>>>   python/libvirt-override.c | 54
>>> ++-
>>>   1 file changed, 39 insertions(+), 15 deletions(-)
>> ACK with the commit message fixed.
>>
>> Michal
> 
>Thanks for this review.
>I pushed with these fixed

As I hit the same issue in Fedora 19
(https://bugzilla.redhat.com/show_bug.cgi?id=999077), I have now
backported this to v1.0.5-maint.

-- 
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

Re: [libvirt] [PATCH 0/6] Additional iSCSI/chap changes

2013-08-20 Thread John Ferlan 
On 08/07/2013 08:43 PM, John Ferlan wrote:
> These patches address a couple of issues I ran into while doing extra
> testing on the iSCSI chap authentication patches:
> 
> https://www.redhat.com/archives/libvir-list/2013-July/msg01378.html
> 
> The first 2 or 3 patches should be candidiates to be put into rhel7.0
> as updates/fixes to issues. The last 3 patches update the documentation
> to show how to use secrets, pools, and iSCIS devices in the domain.
> 
> John Ferlan (6):
>   virsh: Print cephx and iscsi usage
>   qemu_conf: Fix broken logic for adding passthrough iscsi lun
>   Report secret usage error message similarly
>   docs: Update the formatdomain disk examples
>   docs: Update formatsecrets to include more examples of each type
>   docs: Update iSCSI storage pool example
> 
>  docs/formatdomain.html.in   |  57 +++--
>  docs/formatsecret.html.in   | 156 
> +---
>  docs/formatstorage.html.in  |   5 +-
>  src/qemu/qemu_command.c |  28 +--
>  src/qemu/qemu_conf.c|   2 +-
>  src/storage/storage_backend_iscsi.c |  28 +--
>  src/storage/storage_backend_rbd.c   |  35 ++--
>  tools/virsh-secret.c|  17 ++--
>  8 files changed, 278 insertions(+), 50 deletions(-)
> 

I have pushed the series now.

Thanks for the reviews,

John

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

[libvirt] [PATCH] libxl: Resolve possible NULL dereference

2013-08-20 Thread John Ferlan 
If we reached cleanup: prior to allocating cpus, it was possible that
'nr_nodes' had a value, but cpus was NULL leading to a possible NULL
deref. Add a 'cpus' as an end condition to for loop
---

Will push based upon pre-approved ACK:

https://www.redhat.com/archives/libvir-list/2013-August/msg00981.html

 src/libxl/libxl_conf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 01626b9..8129c7f 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -196,7 +196,7 @@ libxlCapsInitNuma(libxl_ctx *ctx, virCapsPtr caps)
 
  cleanup:
 if (ret != 0) {
-for (i = 0; i < nr_nodes; i++)
+for (i = 0; cpus && i < nr_nodes; i++)
 VIR_FREE(cpus[i]);
 virCapabilitiesFreeNUMAInfo(caps);
 }
-- 
1.8.3.1

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

Re: [libvirt] [PATCH] libxl: implement NUMA capabilities reporting

2013-08-20 Thread Eric Blake 
On 08/20/2013 11:01 AM, John Ferlan wrote:
> 
> The following resolves Coverity's complaint and keeps things safer:
> 
> -for (i = 0; i < nr_nodes; i++)
> +for (i = 0; cpus && i < nr_nodes; i++)
> 

Pre-approved ACK if you want to work that into a formal patch.

-- 
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

Re: [libvirt] [PATCH] libxl: implement NUMA capabilities reporting

2013-08-20 Thread John Ferlan 
On 08/16/2013 05:46 PM, Jim Fehlig wrote:
> From: Dario Faggioli 

...snip...

> +
> + cleanup:
> +if (ret != 0) {
> +for (i = 0; i < nr_nodes; i++)
> +VIR_FREE(cpus[i]);
> +virCapabilitiesFreeNUMAInfo(caps);
> +}
> +

Coverity got grumpy with respect to the above loop.  While I can only
assume logically that 'nr_nodes' is not changed if libxl_get_numainfo()
returns NULL, Coverity doesn't assume that.

Also, even if libxl_get_numainfo() did return data and nr_nodes had a
value, if the "else" condition fails, eg "if (cpu_topo == NULL ||
nr_cpus == 0) {", then 'nr_nodes > 0', but 'cpus' is still NULL, which
will cause sudden death syndrome :-).

The following resolves Coverity's complaint and keeps things safer:

-for (i = 0; i < nr_nodes; i++)
+for (i = 0; cpus && i < nr_nodes; i++)

John

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

Re: [libvirt] [PATCH] selinux: distinguish failure to label from request to avoid label

2013-08-20 Thread Eric Blake 
On 08/20/2013 07:04 AM, Daniel P. Berrange wrote:
> On Mon, Aug 12, 2013 at 10:19:47PM -0600, Eric Blake wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=924153
>>

>> The solution is to separate the semantics of a chain that must
>> not be labeled (which the user can set even on persistent domains)
>> vs. the optimization of not attempting a relabel on cleanup (a
>> live-only annotation), and using only the user's explicit notation
>> rather than the optimization as the decision on whether to skip
>> a label attempt in the first place.  When upgrading an older
>> libvirtd to a newer, an NFS volume will still attempt the relabel;
>> but as the avoidance of a relabel was only an optimization, this
>> shouldn't cause any problems.
>>

> 
> ACK

I've pushed this and the followup testsuite securityselinuxlabeltest
enhancement.

-- 
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

Re: [libvirt] [PATCHv2 1/3] virsh: modify vshStringToArray to duplicate the elements too

2013-08-20 Thread Peter Krempa 
On 08/20/13 17:56, Eric Blake wrote:
> On 08/20/2013 09:45 AM, Peter Krempa wrote:
> 
 +++ b/tools/virsh.h
 @@ -37,6 +37,7 @@
  # include "virerror.h"
  # include "virthread.h"
  # include "virnetdevbandwidth.h"
 +# include "virstring.h"
>>>
>>> Is this change necessary?
>>>
>>
>> It's to import virStringFreeList to virsh as it's used to free the
>> string list from vshStringToArray in most places. Adding it to the
>> corresponding files calling it might save half of the includes though. I
>> can change it to separate includes if you wish so.
> 
> I can live with it either way; it doesn't hurt too much to make virsh.h
> a convenience header that pulls in lots of extras to make life for the
> individual virsh-*.c files easier.
> 

Thanks, I've fixed the indentation and pushed the series.

Peter



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

Re: [libvirt] [PATCHv2 1/3] virsh: modify vshStringToArray to duplicate the elements too

2013-08-20 Thread Eric Blake 
On 08/20/2013 09:45 AM, Peter Krempa wrote:

>>> +++ b/tools/virsh.h
>>> @@ -37,6 +37,7 @@
>>>  # include "virerror.h"
>>>  # include "virthread.h"
>>>  # include "virnetdevbandwidth.h"
>>> +# include "virstring.h"
>>
>> Is this change necessary?
>>
> 
> It's to import virStringFreeList to virsh as it's used to free the
> string list from vshStringToArray in most places. Adding it to the
> corresponding files calling it might save half of the includes though. I
> can change it to separate includes if you wish so.

I can live with it either way; it doesn't hurt too much to make virsh.h
a convenience header that pulls in lots of extras to make life for the
individual virsh-*.c files easier.

-- 
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

Re: [libvirt] [PATCHv2 1/3] virsh: modify vshStringToArray to duplicate the elements too

2013-08-20 Thread Peter Krempa 
On 08/20/13 17:42, Eric Blake wrote:
> On 08/20/2013 08:15 AM, Peter Krempa wrote:
>> At a slightly larger memory expense allow stealing of items from the
>> string array returned from vshStringToArray and turn the result into a
>> string list compatible with virStringSplit. This will allow to use the
>> common dealloc function.
>>
>> This patch also fixes a few forgotten checks of return from
>> vshStringToArray and one memory leak.
>> ---
>>
> 
>> @@ -409,7 +406,8 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd 
>> ATTRIBUTE_UNUSED)
>>  vshError(ctl, "%s", _("Options --tree and --cap are 
>> incompatible"));
>>  return false;
>>  }
>> -ncaps = vshStringToArray(cap_str, &caps);
>> +if ((ncaps = vshStringToArray(cap_str, &caps)) < 0)
>> +  return false;
> 
> Indentation looks off.

Hmm, yes.

> 
>> +++ b/tools/virsh.h
>> @@ -37,6 +37,7 @@
>>  # include "virerror.h"
>>  # include "virthread.h"
>>  # include "virnetdevbandwidth.h"
>> +# include "virstring.h"
> 
> Is this change necessary?
> 

It's to import virStringFreeList to virsh as it's used to free the
string list from vshStringToArray in most places. Adding it to the
corresponding files calling it might save half of the includes though. I
can change it to separate includes if you wish so.

Peter




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

Re: [libvirt] [PATCHv2 1/3] virsh: modify vshStringToArray to duplicate the elements too

2013-08-20 Thread Osier Yang 

On 20/08/13 23:42, Eric Blake wrote:

On 08/20/2013 08:15 AM, Peter Krempa wrote:

At a slightly larger memory expense allow stealing of items from the
string array returned from vshStringToArray and turn the result into a
string list compatible with virStringSplit. This will allow to use the
common dealloc function.

This patch also fixes a few forgotten checks of return from
vshStringToArray and one memory leak.
---

@@ -409,7 +406,8 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
  vshError(ctl, "%s", _("Options --tree and --cap are 
incompatible"));
  return false;
  }
-ncaps = vshStringToArray(cap_str, &caps);
+if ((ncaps = vshStringToArray(cap_str, &caps)) < 0)
+  return false;

Indentation looks off.


+++ b/tools/virsh.h
@@ -37,6 +37,7 @@
  # include "virerror.h"
  # include "virthread.h"
  # include "virnetdevbandwidth.h"
+# include "virstring.h"

Is this change necessary?


I think the purpose is to avoid including "virstring.h" in all of the .c
files which need to use virStringSplit.

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

Re: [libvirt] [PATCHv2 3/3] virsh: Don't leak list of volumes when undefining domain with storage

2013-08-20 Thread Osier Yang 

On 20/08/13 22:15, Peter Krempa wrote:

Use the new semantics of vshStringToArray to avoid leaking the array of
volumes to be deleted. The array would be leaked in case the first
volume was found in the domain definition. Also refactor the code a bit
to sanitize naming of variables hoding arrays and dimensions of the
arrays.

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

Notes:
 Version 2:
 * renamed a few variables:
  - vol_list -> vol_string
  - volumes -> vol_list
 and the corresponding count variables.
 * reordered the declaration order ov variables

  tools/virsh-domain.c | 134 +--
  1 file changed, 65 insertions(+), 69 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 13e3045..b29f934 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -2932,24 +2932,23 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
  int rc = -1;
  int running;
  /* list of volumes to remove along with this domain */
-vshUndefineVolume *vlist = NULL;
-int nvols = 0;
-const char *volumes = NULL;
-char **volume_tokens = NULL;
-char *volume_tok = NULL;
-int nvolume_tokens = 0;
-char *def = NULL;
+const char *vol_string = NULL;  /* string containing volumes to delete */
+char **vol_list = NULL; /* tokenized vol_string */
+int nvol_list = 0;
+vshUndefineVolume *vols = NULL; /* info about the volumes to delete*/
+size_t nvols = 0;
+char *def = NULL;   /* domain def */
+xmlDocPtr doc = NULL;
+xmlXPathContextPtr ctxt = NULL;
+xmlNodePtr *vol_nodes = NULL;   /* XML nodes of volumes of the guest */
+int nvol_nodes;
  char *source = NULL;
  char *target = NULL;
  size_t i;
  size_t j;
-xmlDocPtr doc = NULL;
-xmlXPathContextPtr ctxt = NULL;
-xmlNodePtr *vol_nodes = NULL;
-int nvolumes = 0;
-bool vol_not_found = false;

-ignore_value(vshCommandOptString(cmd, "storage", &volumes));
+
+ignore_value(vshCommandOptString(cmd, "storage", &vol_string));

  if (managed_save) {
  flags |= VIR_DOMAIN_UNDEFINE_MANAGED_SAVE;
@@ -3017,14 +3016,17 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
  }

  /* Stash domain description for later use */
-if (volumes || remove_all_storage) {
+if (vol_string || remove_all_storage) {
  if (running) {
-vshError(ctl, _("Storage volume deletion is supported only on stopped 
domains"));
+vshError(ctl,
+ _("Storage volume deletion is supported only on "
+   "stopped domains"));
  goto cleanup;
  }

-if (volumes && remove_all_storage) {
-vshError(ctl, _("Specified both --storage and 
--remove-all-storage"));
+if (vol_string && remove_all_storage) {
+vshError(ctl,
+ _("Specified both --storage and --remove-all-storage"));
  goto cleanup;
  }

@@ -3038,20 +3040,17 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
  goto error;

  /* tokenize the string from user and save it's parts into an array */
-if (volumes) {
-if ((nvolume_tokens = vshStringToArray(volumes, &volume_tokens)) < 
0)
-goto cleanup;
-}
-
-if ((nvolumes = virXPathNodeSet("./devices/disk", ctxt,
-&vol_nodes)) < 0)
+if (vol_string &&
+(nvol_list = vshStringToArray(vol_string, &vol_list)) < 0)
  goto error;

-if (nvolumes > 0)
-vlist = vshCalloc(ctl, nvolumes, sizeof(*vlist));
+if ((nvol_nodes = virXPathNodeSet("./devices/disk", ctxt,
+  &vol_nodes)) < 0)
+goto error;

-for (i = 0; i < nvolumes; i++) {
+for (i = 0; i < nvol_nodes; i++) {
  ctxt->node = vol_nodes[i];
+vshUndefineVolume vol;
  VIR_FREE(source);
  VIR_FREE(target);

@@ -3063,56 +3062,53 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
"./source/@file|"
"./source/@dir|"
"./source/@name|"
-  "./source/@dev)", ctxt))) {
-if (last_error && last_error->code != VIR_ERR_OK)
-goto error;
-else
-continue;
-}
+  "./source/@dev)", ctxt)))
+continue;

  /* lookup if volume was selected by user */
-if (volumes) {
-volume_tok = NULL;
-for (j = 0; j < nvolume_tokens; j++) {
-if (volume_tokens[j] &&
-(STREQ(volume_tokens[j], target) ||
-

Re: [libvirt] [PATCHv2 1/3] virsh: modify vshStringToArray to duplicate the elements too

2013-08-20 Thread Eric Blake 
On 08/20/2013 08:15 AM, Peter Krempa wrote:
> At a slightly larger memory expense allow stealing of items from the
> string array returned from vshStringToArray and turn the result into a
> string list compatible with virStringSplit. This will allow to use the
> common dealloc function.
> 
> This patch also fixes a few forgotten checks of return from
> vshStringToArray and one memory leak.
> ---
> 

> @@ -409,7 +406,8 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd 
> ATTRIBUTE_UNUSED)
>  vshError(ctl, "%s", _("Options --tree and --cap are 
> incompatible"));
>  return false;
>  }
> -ncaps = vshStringToArray(cap_str, &caps);
> +if ((ncaps = vshStringToArray(cap_str, &caps)) < 0)
> +  return false;

Indentation looks off.

> +++ b/tools/virsh.h
> @@ -37,6 +37,7 @@
>  # include "virerror.h"
>  # include "virthread.h"
>  # include "virnetdevbandwidth.h"
> +# include "virstring.h"

Is this change necessary?

-- 
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

Re: [libvirt] [PATCHv2 1/3] virsh: modify vshStringToArray to duplicate the elements too

2013-08-20 Thread Osier Yang 

On 20/08/13 22:15, Peter Krempa wrote:

At a slightly larger memory expense allow stealing of items from the
string array returned from vshStringToArray and turn the result into a
string list compatible with virStringSplit. This will allow to use the
common dealloc function.

This patch also fixes a few forgotten checks of return from
vshStringToArray and one memory leak.
---

Notes:
 Version 2:
 * tweaked comment of vshStringToArray to mention deallocation of the 
returned array


ACK

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

Re: [libvirt] [PATCH v2] storage: Fix coverity warning

2013-08-20 Thread Eric Blake 
On 08/20/2013 09:28 AM, Osier Yang wrote:
> Introduced by commit e0139e30444:
> 
> 1777  /* Updating pool metadata */
> 
> (40) Event var_deref_op: Dereferencing null pointer "newvol".
>  Also see events: [assign_zero]
> 
> 1778  pool->def->allocation += newvol->allocation;
> 1779  pool->def->available -= newvol->allocation;
> ---
>  src/storage/storage_driver.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)

ACK - this turned out cleaner.

-- 
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

[libvirt] [PATCH v2] storage: Fix coverity warning

2013-08-20 Thread Osier Yang 
Introduced by commit e0139e30444:

1777/* Updating pool metadata */

(40) Event var_deref_op: Dereferencing null pointer "newvol".
 Also see events: [assign_zero]

1778pool->def->allocation += newvol->allocation;
1779pool->def->available -= newvol->allocation;
---
 src/storage/storage_driver.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 7908ba6..58d0fc0 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1635,6 +1635,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
 virStorageBackendPtr backend;
 virStorageVolDefPtr origvol = NULL, newvol = NULL;
 virStorageVolPtr ret = NULL, volobj = NULL;
+unsigned long long allocation;
 int buildret;
 
 virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL);
@@ -1758,6 +1759,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
 
 origvol->building = 0;
 newvol->building = 0;
+allocation = newvol->allocation;
 newvol = NULL;
 pool->asyncjobs--;
 
@@ -1775,8 +1777,8 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
 }
 
 /* Updating pool metadata */
-pool->def->allocation += newvol->allocation;
-pool->def->available -= newvol->allocation;
+pool->def->allocation += allocation;
+pool->def->available -= allocation;
 
 VIR_INFO("Creating volume '%s' in storage pool '%s'",
  volobj->name, pool->def->name);
-- 
1.8.1.4

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

Re: [libvirt] [PATCH 2/2] storage: Fix the use-after-free memory bug

2013-08-20 Thread John Ferlan 
On 08/20/2013 05:08 AM, Osier Yang wrote:
> Introduced by commit e0139e30444. virStorageVolDefFree free'ed the
> pointers that are still used by the added volume object, this changes
> it back to VIR_FREE.
> ---
>  src/storage/storage_driver.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index 63a954b..883e4e9 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -1618,7 +1618,7 @@ storageVolCreateXML(virStoragePoolPtr obj,
>  cleanup:
>  virObjectUnref(volobj);
>  virStorageVolDefFree(voldef);
> -virStorageVolDefFree(buildvoldef);
> +VIR_FREE(buildvoldef);
>  if (pool)
>  virStoragePoolObjUnlock(pool);
>  return ret;
> 

Perhaps a comment "/* Free just the shallow copy of buildvoldef */".
Just for the clarity of why you wouldn't want to use virStorageVolDefFree().

Of course the same possible other solution applies here as well as it
did in your 1/2 patch where you define a local allocation variable to
handle the pool->def->{allocation|available} math.  That way buildvoldef
moves back inside the "if (backend->buildVol)"...

ACK either way though.

John

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

Re: [libvirt] [PATCH 1/2] storage: Fix coverity warning

2013-08-20 Thread John Ferlan 
On 08/20/2013 05:08 AM, Osier Yang wrote:
> Introduced by commit e0139e30444:
> 
> 1777  /* Updating pool metadata */
> 
> (40) Event var_deref_op: Dereferencing null pointer "newvol".
>  Also see events: [assign_zero]
> 
> 1778  pool->def->allocation += newvol->allocation;
> 1779  pool->def->available -= newvol->allocation;
> ---
>  src/storage/storage_driver.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index 7908ba6..63a954b 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -1758,7 +1758,6 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
>  
>  origvol->building = 0;
>  newvol->building = 0;
> -newvol = NULL;
>  pool->asyncjobs--;
>  
>  if (origpool) {

...
The next condition is:

if (buildret < 0) {
virStoragePoolObjUnlock(pool);
storageVolDelete(volobj, 0);
pool = NULL;
goto cleanup;
}

Since previously we'd have 'newvol = NULL;' already, there would need to
be one added here too..  Since, prior to this there's code:

pool->volumes.objs[pool->volumes.count++] = newvol;

which saves the pointer...

Perhaps it'd work better to do the following:

unsigned long long allocation = 0x0ULL;

...


allocation = newvol->allocation;
newvol = NULL;

...

pool->def->allocation += allocation;
pool->def->available -= allocation;



> @@ -1781,6 +1780,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
>  VIR_INFO("Creating volume '%s' in storage pool '%s'",
>   volobj->name, pool->def->name);
>  ret = volobj;
> +newvol = NULL;

and this would become unnecessary

>  volobj = NULL;
>  
>  cleanup:
> 

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

Re: [libvirt] [RFC] Expose cpu_map.xml via API

2013-08-20 Thread Cole Robinson 
On 08/20/2013 05:50 AM, Daniel P. Berrange wrote:
> On Mon, Aug 19, 2013 at 08:19:56PM +0200, Giuseppe Scrivano wrote:
>> ---
>> I have started working on:
>>
>>   https://bugzilla.redhat.com/show_bug.cgi?id=916786
>>
>> +/**
>> + * virConnectGetCPUMapDesc:
>> + *
>> + * @conn: virConnect connection
>> + *
>> + * Get the content of the cpu_map.xml file used by the connection.
>> + *
>> + * Returns XML description of the cpu_map.xml file.
>> + */
>> +char *virConnectGetCPUMapDesc(virConnectPtr conn);
> 
> This is really not at all neccessary, or desirable.
> 
> We now have the ability to query the full CPU flag set from the
> virConnectBaselineCPU API.
> 
> All we're missing is thus a way to get a list of supported CPU
> model names. For that we can just have
> 
>   int virConnectGetCPUModelNames(virConnectPtr conn, char ***models);
> 

That API would at least need receive an 'arch' value to limit the returned
models. And there's other data in cpu_map than just the model name, though API
users likely don't require it (yet).

I think the suggestion elsewhere in the thread for a hypervisor capabilities
or per-emulator capabilities API would work, and in the future we could extend
it with things like available device lists. But we need to think hard about
the XML format to make it as future proof as possible, since there's all sorts
of crazy interdependencies between arch, machine type, domain type,
hvm/pv/exe, etc.

- Cole

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

[libvirt] Schedule for next release

2013-08-20 Thread Daniel Veillard 
 To try to stay on the end of month release schedule, I am suggesting
to enter the freeze for 1.1.2 next Tuesday, i.e. the 27th Aug, and
then shoot for a release on the morning of Monday 2nd of Sep.

  So unless I hear disagreements, that's the plan :-)

Daniel

-- 
Daniel Veillard  | Open Source and Standards, Red Hat
veill...@redhat.com  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | virtualization library  http://libvirt.org/

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

[libvirt] [PATCHv2 0/3] Clean up usage of vshStringToArray

2013-08-20 Thread Peter Krempa 
Fixed after review from Osier, please see individual patches.

Peter Krempa (3):
  virsh: modify vshStringToArray to duplicate the elements too
  virsh-pool: Improve error message in cmdPoolList
  virsh: Don't leak list of volumes when undefining domain with storage

 tools/virsh-domain.c   | 134 -
 tools/virsh-nodedev.c  |  18 ++-
 tools/virsh-pool.c |  12 ++---
 tools/virsh-snapshot.c |  10 +---
 tools/virsh.c  |  15 +++---
 tools/virsh.h  |   1 +
 6 files changed, 86 insertions(+), 104 deletions(-)

-- 
1.8.3.2

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

Re: [libvirt] [PATCH 5/6] docs: Update formatsecrets to include more examples of each type

2013-08-20 Thread John Ferlan 
On 08/20/2013 02:09 AM, Osier Yang wrote:
> On 08/08/13 08:43, John Ferlan wrote:
>> Update formatsecret docs to describe the various options and provide
>> examples
>> in order to set up secrets for each type of secret.
>> ---
>>   docs/formatsecret.html.in | 156
>> ++
>>   1 file changed, 145 insertions(+), 11 deletions(-)
>>
>> diff --git a/docs/formatsecret.html.in b/docs/formatsecret.html.in
>> index 3e306b5..7dd0927 100644
>> --- a/docs/formatsecret.html.in
>> +++ b/docs/formatsecret.html.in
>> @@ -46,18 +46,51 @@
>> 
>>   
>>   -Usage type "volume"
>> +Usage type "volume"
>> 
>> This secret is associated with a volume, and it is safe to
>> delete the
>> secret after the volume is deleted.  The > type='volume'> element must contain a
>> single volume element that specifies the key of
>> the volume
>> -  this secret is associated with.
>> +  this secret is associated with. For example, create a
>> demo-secret.xml
> 
> Given the way you names the xml file for other secret types in this
> context,
> this should be volume-secret.xml
> 

Done

>> +  file as follows:
>>   
>>   -Usage type "ceph"
>> +
>> +  
>> + LUKS passphrase for the main hard drive
>> of our mail server
>> + 0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f
>> + 
>> +   
>> /var/lib/libvirt/images/mail.img
>> + 
>> +  
>> +
>> +
>> +
>> +  Define the secret and set the pass phrase as follows:
>> +
>> +
>> +  # virsh secret-define demo-secret.xml
>> +  Secret 0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f created
>> +  #
>> +  # MYSECRET=`printf %s "open seseme" | base64`
>> +  # virsh secret-set-value 0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f
>> $MYSECRET
>> +  Secret value set
>> +  #
>> +
>> +
>> +
>> +  The volume can then be used in the XML for a disk volume
> 
> s/volume can/volume secret/, or s/volume can/volume type secret/,
> I prefer the latter one, since both of "volume" and "secret" are general
> terms in libvirt.
> 
> And s/disk volume/storage volume/,
> 

I went with 'volume type secret' and 'storage volume'

>> +  encryption as follows:
>> +
>> +
>> +  
>> +> uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f'/>
>> +  
>> +
>>   +Usage type "ceph"
>>   
>> This secret is associated with a Ceph RBD (rados block device).
>> The  element must contain
>> @@ -66,10 +99,57 @@
>> this usage name via the  element of
>> a disk device or
>> a storage pool (rbd).
>> -  Since 0.9.7.
>> +  Since 0.9.7. The following is an
>> example
>> +  of the steps to be taken.  First create a ceph-secret.xml file:
>> +
>> +
>> +
>> +  
>> + CEPH passphrase example
>> + 
>> + 
>> +ceph_example
>> + 
>> +  
>> +
>> +
>> +
>> +  Next, use virsh secret-define ceph-secret.xml to
>> define
>> +  the secret and virsh secret-set-value using the
>> generated
>> +  UUID value and a base64 generated secret value in order to
>> define the
>> +  chosen secret pass phrase.
>>   
>> +
>> +  # virsh secret-define ceph-secret.xml
>> +  Secret 1b40a534-8301-45d5-b1aa-11894ebb1735 created
>> +  #
>> +  # virsh secret-list
>> +  UUID Usage
>> +  ---
>> +  1b40a534-8301-45d5-b1aa-11894ebb1735 cephx ceph_example
>> +  #
>> +  # CEPHPHRASE=`printf %s "pass phrase" | base64`
>> +  # virsh secret-set-value 1b40a534-8301-45d5-b1aa-11894ebb1735
>> $CEPHPHRASE
>> +  Secret value set
>>   -Usage type "iscsi"
>> +  #
>> +
>> +
>> +
>> +  The ceph secret can then be used by UUID or by the
>> +  usage name via the  element in a
>> +  domain's  element as follows:
>> +
>> +
>> +  
>> +
>> +  
>> +  
>> +
>> +  
>> +
>> +
> 
> Given that you created example for storage pool chap auth. Here
> we should have example for storage pool ceph auth too.
> 

OK, done. The following was added (see your comment [1] below 
to understand my grammar usage)


  As well as the  element in a
  storage pool (rbd)
   element as follows:

[libvirt] [PATCHv2 1/3] virsh: modify vshStringToArray to duplicate the elements too

2013-08-20 Thread Peter Krempa 
At a slightly larger memory expense allow stealing of items from the
string array returned from vshStringToArray and turn the result into a
string list compatible with virStringSplit. This will allow to use the
common dealloc function.

This patch also fixes a few forgotten checks of return from
vshStringToArray and one memory leak.
---

Notes:
Version 2:
* tweaked comment of vshStringToArray to mention deallocation of the 
returned array

 tools/virsh-nodedev.c  | 18 +-
 tools/virsh-pool.c | 10 --
 tools/virsh-snapshot.c | 10 ++
 tools/virsh.c  | 15 ---
 tools/virsh.h  |  1 +
 5 files changed, 20 insertions(+), 34 deletions(-)

diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c
index 3255079..cfbf3bc 100644
--- a/tools/virsh-nodedev.c
+++ b/tools/virsh-nodedev.c
@@ -161,10 +161,7 @@ cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd)

 ret = true;
 cleanup:
-if (arr) {
-VIR_FREE(*arr);
-VIR_FREE(arr);
-}
+virStringFreeList(arr);
 virNodeDeviceFree(dev);
 return ret;
 }
@@ -409,7 +406,8 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 vshError(ctl, "%s", _("Options --tree and --cap are 
incompatible"));
 return false;
 }
-ncaps = vshStringToArray(cap_str, &caps);
+if ((ncaps = vshStringToArray(cap_str, &caps)) < 0)
+  return false;
 }

 for (i = 0; i < ncaps; i++) {
@@ -503,10 +501,7 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 }

 cleanup:
-if (caps) {
-VIR_FREE(*caps);
-VIR_FREE(caps);
-}
+virStringFreeList(caps);
 vshNodeDeviceListFree(list);
 return ret;
 }
@@ -574,10 +569,7 @@ cmdNodeDeviceDumpXML(vshControl *ctl, const vshCmd *cmd)

 ret = true;
 cleanup:
-if (arr) {
-VIR_FREE(*arr);
-VIR_FREE(arr);
-}
+virStringFreeList(arr);
 VIR_FREE(xml);
 virNodeDeviceFree(device);
 return ret;
diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c
index 1622be2..b8fc8d7 100644
--- a/tools/virsh-pool.c
+++ b/tools/virsh-pool.c
@@ -995,12 +995,13 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 char **poolTypes = NULL;
 int npoolTypes = 0;

-npoolTypes = vshStringToArray(type, &poolTypes);
+if ((npoolTypes = vshStringToArray(type, &poolTypes)) < 0)
+return false;

 for (i = 0; i < npoolTypes; i++) {
 if ((poolType = virStoragePoolTypeFromString(poolTypes[i])) < 0) {
 vshError(ctl, "%s", _("Invalid pool type"));
-VIR_FREE(poolTypes);
+virStringFreeList(poolTypes);
 return false;
 }

@@ -1036,10 +1037,7 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
 break;
 }
 }
-if (poolTypes) {
-VIR_FREE(*poolTypes);
-VIR_FREE(poolTypes);
-}
+virStringFreeList(poolTypes);
 }

 if (!(list = vshStoragePoolListCollect(ctl, flags)))
diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
index db9715b..e37a5b3 100644
--- a/tools/virsh-snapshot.c
+++ b/tools/virsh-snapshot.c
@@ -261,10 +261,7 @@ vshParseSnapshotMemspec(vshControl *ctl, virBufferPtr buf, 
const char *str)
 cleanup:
 if (ret < 0)
 vshError(ctl, _("unable to parse memspec: %s"), str);
-if (array) {
-VIR_FREE(*array);
-VIR_FREE(array);
-}
+virStringFreeList(array);
 return ret;
 }

@@ -313,10 +310,7 @@ vshParseSnapshotDiskspec(vshControl *ctl, virBufferPtr 
buf, const char *str)
 cleanup:
 if (ret < 0)
 vshError(ctl, _("unable to parse diskspec: %s"), str);
-if (array) {
-VIR_FREE(*array);
-VIR_FREE(array);
-}
+virStringFreeList(array);
 return ret;
 }

diff --git a/tools/virsh.c b/tools/virsh.c
index f65dc79..15f529e 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -163,10 +163,9 @@ vshPrettyCapacity(unsigned long long val, const char 
**unit)
 }

 /*
- * Convert the strings separated by ',' into array. The caller
- * must free the first array element and the returned array after
- * use (all other array elements belong to the memory allocated
- * for the first array element).
+ * Convert the strings separated by ',' into array. The returned
+ * array is a NULL terminated string list. The caller has to free
+ * the array using virStringFreeList or a similar method.
  *
  * Returns the length of the filled array on success, or -1
  * on error.
@@ -196,7 +195,8 @@ vshStringToArray(const char *str,
 str_tok++;
 }

-if (VIR_ALLOC_N(arr, nstr_tokens) < 0) {
+/* reserve the NULL element at the end */
+if (VIR_ALLOC_N(arr, nstr_tokens + 1) < 0) {
 VIR_FREE(str_copied);
 return -1;
 }
@@ -212,12 +212,13 @@ vshStringToArray(const char *str,

[libvirt] [PATCHv2 3/3] virsh: Don't leak list of volumes when undefining domain with storage

2013-08-20 Thread Peter Krempa 
Use the new semantics of vshStringToArray to avoid leaking the array of
volumes to be deleted. The array would be leaked in case the first
volume was found in the domain definition. Also refactor the code a bit
to sanitize naming of variables hoding arrays and dimensions of the
arrays.

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

Notes:
Version 2:
* renamed a few variables:
 - vol_list -> vol_string
 - volumes -> vol_list
and the corresponding count variables.
* reordered the declaration order ov variables

 tools/virsh-domain.c | 134 +--
 1 file changed, 65 insertions(+), 69 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 13e3045..b29f934 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -2932,24 +2932,23 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
 int rc = -1;
 int running;
 /* list of volumes to remove along with this domain */
-vshUndefineVolume *vlist = NULL;
-int nvols = 0;
-const char *volumes = NULL;
-char **volume_tokens = NULL;
-char *volume_tok = NULL;
-int nvolume_tokens = 0;
-char *def = NULL;
+const char *vol_string = NULL;  /* string containing volumes to delete */
+char **vol_list = NULL; /* tokenized vol_string */
+int nvol_list = 0;
+vshUndefineVolume *vols = NULL; /* info about the volumes to delete*/
+size_t nvols = 0;
+char *def = NULL;   /* domain def */
+xmlDocPtr doc = NULL;
+xmlXPathContextPtr ctxt = NULL;
+xmlNodePtr *vol_nodes = NULL;   /* XML nodes of volumes of the guest */
+int nvol_nodes;
 char *source = NULL;
 char *target = NULL;
 size_t i;
 size_t j;
-xmlDocPtr doc = NULL;
-xmlXPathContextPtr ctxt = NULL;
-xmlNodePtr *vol_nodes = NULL;
-int nvolumes = 0;
-bool vol_not_found = false;

-ignore_value(vshCommandOptString(cmd, "storage", &volumes));
+
+ignore_value(vshCommandOptString(cmd, "storage", &vol_string));

 if (managed_save) {
 flags |= VIR_DOMAIN_UNDEFINE_MANAGED_SAVE;
@@ -3017,14 +3016,17 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
 }

 /* Stash domain description for later use */
-if (volumes || remove_all_storage) {
+if (vol_string || remove_all_storage) {
 if (running) {
-vshError(ctl, _("Storage volume deletion is supported only on 
stopped domains"));
+vshError(ctl,
+ _("Storage volume deletion is supported only on "
+   "stopped domains"));
 goto cleanup;
 }

-if (volumes && remove_all_storage) {
-vshError(ctl, _("Specified both --storage and 
--remove-all-storage"));
+if (vol_string && remove_all_storage) {
+vshError(ctl,
+ _("Specified both --storage and --remove-all-storage"));
 goto cleanup;
 }

@@ -3038,20 +3040,17 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
 goto error;

 /* tokenize the string from user and save it's parts into an array */
-if (volumes) {
-if ((nvolume_tokens = vshStringToArray(volumes, &volume_tokens)) < 
0)
-goto cleanup;
-}
-
-if ((nvolumes = virXPathNodeSet("./devices/disk", ctxt,
-&vol_nodes)) < 0)
+if (vol_string &&
+(nvol_list = vshStringToArray(vol_string, &vol_list)) < 0)
 goto error;

-if (nvolumes > 0)
-vlist = vshCalloc(ctl, nvolumes, sizeof(*vlist));
+if ((nvol_nodes = virXPathNodeSet("./devices/disk", ctxt,
+  &vol_nodes)) < 0)
+goto error;

-for (i = 0; i < nvolumes; i++) {
+for (i = 0; i < nvol_nodes; i++) {
 ctxt->node = vol_nodes[i];
+vshUndefineVolume vol;
 VIR_FREE(source);
 VIR_FREE(target);

@@ -3063,56 +3062,53 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
   "./source/@file|"
   "./source/@dir|"
   "./source/@name|"
-  "./source/@dev)", ctxt))) {
-if (last_error && last_error->code != VIR_ERR_OK)
-goto error;
-else
-continue;
-}
+  "./source/@dev)", ctxt)))
+continue;

 /* lookup if volume was selected by user */
-if (volumes) {
-volume_tok = NULL;
-for (j = 0; j < nvolume_tokens; j++) {
-if (volume_tokens[j] &&
-(STREQ(volume_tokens[j], target) ||
- STREQ(volume_tokens[j], source))) {
-volume

[libvirt] [PATCHv2 2/3] virsh-pool: Improve error message in cmdPoolList

2013-08-20 Thread Peter Krempa 
Explicitly let the user know about the unknown pool type.
---

Notes:
Version 2:
* fixed commit message
* ACK'd in v1 (would cause conflict if reordering ...)

 tools/virsh-pool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c
index b8fc8d7..592b81f 100644
--- a/tools/virsh-pool.c
+++ b/tools/virsh-pool.c
@@ -1000,7 +1000,7 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)

 for (i = 0; i < npoolTypes; i++) {
 if ((poolType = virStoragePoolTypeFromString(poolTypes[i])) < 0) {
-vshError(ctl, "%s", _("Invalid pool type"));
+vshError(ctl, _("Invalid pool type '%s'"), poolTypes[i]);
 virStringFreeList(poolTypes);
 return false;
 }
-- 
1.8.3.2

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

Re: [libvirt] [PATCH 16/17] qemuhotplugtest: Add tests for USB disk hotplug

2013-08-20 Thread Daniel P. Berrange 
On Thu, Aug 01, 2013 at 09:28:25PM +0200, Jiri Denemark wrote:
> ---
>  tests/qemuhotplugtest.c| 17 
>  tests/qemuhotplugtestdata/qemuhotplug-disk-usb.xml |  7 
>  .../qemuhotplug-hotplug-base+disk-usb.xml  | 45 
> ++
>  3 files changed, 69 insertions(+)
>  create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-disk-usb.xml
>  create mode 100644 
> tests/qemuhotplugtestdata/qemuhotplug-hotplug-base+disk-usb.xml

ACK


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

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

Re: [libvirt] [PATCH 17/17] qemuhotplugtest: Add tests for virtio SCSI disk hotplug

2013-08-20 Thread Daniel P. Berrange 
On Thu, Aug 01, 2013 at 09:28:26PM +0200, Jiri Denemark wrote:
> ---
>  tests/qemuhotplugtest.c| 18 +
>  .../qemuhotplugtestdata/qemuhotplug-disk-scsi.xml  |  7 
>  .../qemuhotplug-hotplug-base+disk-scsi.xml | 46 
> ++
>  3 files changed, 71 insertions(+)
>  create mode 100644 tests/qemuhotplugtestdata/qemuhotplug-disk-scsi.xml
>  create mode 100644 
> tests/qemuhotplugtestdata/qemuhotplug-hotplug-base+disk-scsi.xml

ACK

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

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

Re: [libvirt] [PATCH 15/17] qemuhotplugtest: Add tests for async virtio disk detach

2013-08-20 Thread Daniel P. Berrange 
On Thu, Aug 01, 2013 at 09:28:24PM +0200, Jiri Denemark wrote:
> ---
>  tests/qemuhotplugtest.c | 23 +++
>  1 file changed, 23 insertions(+)

ACK


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

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

Re: [libvirt] [PATCH] qemuBuildCommandLine: Fall back to mem balloon if there's no hard_limit

2013-08-20 Thread Ján Tomko 
On 08/20/2013 03:06 PM, Michal Privoznik wrote:
> If there's no hard_limit set and domain uses VFIO we still must lock the
> guest memory (prerequisite from qemu). Hence, we should compute the

> amount to be locked from max_baloon.

You forgot about the bookkeepers, didn't you?

> ---
>  src/qemu/qemu_command.c | 15 +--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index c8f7df2..71c220f 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -9219,8 +9219,19 @@ qemuBuildCommandLine(virConnectPtr conn,
>  goto error;
>  }
>  
> -if (mlock)
> -virCommandSetMaxMemLock(cmd, def->mem.hard_limit * 1024);
> +if (mlock) {
> +unsigned long long memKB;
> +
> +/* VFIO requires all of the guest's memory to be
> + * locked resident, plus some amount for IO
> + * space. Alex Williamson suggested adding 1GiB for IO
> + * space just to be safe (some finer tuning might be
> + * nice, though).
> + */
> +memKB = def->mem.hard_limit ?
> +def->mem.hard_limit : def->mem.max_balloon + 1024 * 1024;
> +virCommandSetMaxMemLock(cmd, memKB * 1024);
> +}
>  

16bcb3b "qemu: Drop qemuDomainMemoryLimit" also changed the MemLock limit in
qemuDomainAttachHostPciDevice, I think we need yet another patch :(

Jan

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

Re: [libvirt] [PATCH 13/17] qemu: Let tests override waiting time for device unplug

2013-08-20 Thread Daniel P. Berrange 
On Thu, Aug 01, 2013 at 09:28:22PM +0200, Jiri Denemark wrote:
> We don't want tests to wait 5 seconds for an event which we know will
> never come.
> ---
>  src/Makefile.am |  1 +
>  src/qemu/qemu_hotplug.c | 10 ++
>  src/qemu/qemu_hotplugpriv.h | 31 +++
>  3 files changed, 38 insertions(+), 4 deletions(-)
>  create mode 100644 src/qemu/qemu_hotplugpriv.h
> 
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 88dc5fe..3f410da 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -633,6 +633,7 @@ QEMU_DRIVER_SOURCES = 
> \
>   qemu/qemu_cgroup.c qemu/qemu_cgroup.h   \
>   qemu/qemu_hostdev.c qemu/qemu_hostdev.h \
>   qemu/qemu_hotplug.c qemu/qemu_hotplug.h \
> + qemu/qemu_hotplugpriv.h \
>   qemu/qemu_conf.c qemu/qemu_conf.h   \
>   qemu/qemu_process.c qemu/qemu_process.h \
>   qemu/qemu_processpriv.h \
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index f8bcc9a..f9d75dc 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -25,6 +25,7 @@
>  #include 
>  
>  #include "qemu_hotplug.h"
> +#include "qemu_hotplugpriv.h"
>  #include "qemu_capabilities.h"
>  #include "qemu_domain.h"
>  #include "qemu_command.h"
> @@ -53,6 +54,10 @@
>  #define VIR_FROM_THIS VIR_FROM_QEMU
>  #define CHANGE_MEDIA_RETRIES 10
>  
> +/* Wait up to 5 seconds for device removal to finish. */
> +unsigned long long qemuDomainRemoveDeviceWaitTime = 1000ull * 5;
> +
> +
>  int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> virDomainDiskDefPtr disk,
> @@ -2679,9 +2684,6 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver,
>  }
>  
>  
> -/* Wait up to 5 seconds for device removal to finish. */
> -#define QEMU_REMOVAL_WAIT_TIME (1000ull * 5)
> -
>  static void
>  qemuDomainMarkDeviceForRemoval(virDomainObjPtr vm,
> virDomainDeviceInfoPtr info)
> @@ -2718,7 +2720,7 @@ qemuDomainWaitForDeviceRemoval(virDomainObjPtr vm)
>  
>  if (virTimeMillisNow(&until) < 0)
>  return -1;
> -until += QEMU_REMOVAL_WAIT_TIME;
> +until += qemuDomainRemoveDeviceWaitTime;
>  
>  while (priv->unpluggingDevice) {
>  if (virCondWaitUntil(&priv->unplugFinished,
> diff --git a/src/qemu/qemu_hotplugpriv.h b/src/qemu/qemu_hotplugpriv.h
> new file mode 100644
> index 000..e4a1164
> --- /dev/null
> +++ b/src/qemu/qemu_hotplugpriv.h
> @@ -0,0 +1,31 @@
> +/*
> + * qemu_hotplugpriv.h: private declarations for QEMU device hotplug 
> management
> + *
> + * Copyright (C) 2013 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library.  If not, see
> + * .
> + *
> + */
> +
> +#ifndef __QEMU_HOTPLUGPRIV_H__
> +# define __QEMU_HOTPLUGPRIV_H__
> +
> +/*
> + * This header file should never be used outside unit tests.
> + */
> +
> +unsigned long long qemuDomainRemoveDeviceWaitTime;

Don't you need 'extern' here otherwise every .c file that includes
this header will allocate storage a new copy of this variable.


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

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

Re: [libvirt] [PATCH 14/17] qemuhotplugtest: Add support for DEVICE_DELETED event

2013-08-20 Thread Daniel P. Berrange 
On Thu, Aug 01, 2013 at 09:28:23PM +0200, Jiri Denemark wrote:
> ---
>  tests/qemuhotplugtest.c  | 29 +++--
>  tests/qemumonitortestutils.c |  2 ++
>  2 files changed, 25 insertions(+), 6 deletions(-)

ACK


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

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

Re: [libvirt] [PATCH 11/17] tests: Add support for passing driver to qemu monitor

2013-08-20 Thread Daniel P. Berrange 
On Thu, Aug 01, 2013 at 09:28:20PM +0200, Jiri Denemark wrote:
> The driver is then passed to monitor event handlers.
> ---
>  tests/qemuhotplugtest.c  | 2 +-
>  tests/qemumonitortestutils.c | 5 +++--
>  tests/qemumonitortestutils.h | 6 --
>  3 files changed, 8 insertions(+), 5 deletions(-)

ACK

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

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

Re: [libvirt] [PATCH 10/17] tests: Add support for passing vm to qemu monitor

2013-08-20 Thread Daniel P. Berrange 
On Thu, Aug 01, 2013 at 09:28:19PM +0200, Jiri Denemark wrote:
> Some tests need the monitor to operate on an already existing VM object
> rather than on a new mock-up the monitor test normally creates.
> ---
>  tests/qemuhotplugtest.c  |  2 +-
>  tests/qemumonitorjsontest.c  | 26 +-
>  tests/qemumonitortestutils.c | 19 ++-
>  tests/qemumonitortestutils.h |  6 +-
>  4 files changed, 33 insertions(+), 20 deletions(-)

ACK

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

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

Re: [libvirt] [PATCH 09/17] qemuhotplugtest: Add tests for virtio disk hotplug

2013-08-20 Thread Daniel P. Berrange 
On Thu, Aug 01, 2013 at 09:28:18PM +0200, Jiri Denemark wrote:
> diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
> index 4712334..bb047fe 100644
> --- a/tests/qemuhotplugtest.c
> +++ b/tests/qemuhotplugtest.c
> @@ -313,12 +327,22 @@ mymain(void)
>  driver.config = virQEMUDriverConfigNew(false);
>  VIR_FREE(driver.config->spiceListen);
>  VIR_FREE(driver.config->vncListen);
> +/* some dummy values from 'config file' */
> +if (VIR_STRDUP_QUIET(driver.config->spicePassword, "123456") < 0)
> +return EXIT_FAILURE;
>  
>  if (!(driver.domainEventState = virDomainEventStateNew()))
>  return EXIT_FAILURE;
>  
> -/* some dummy values from 'config file' */
> -if (VIR_STRDUP_QUIET(driver.config->spicePassword, "123456") < 0)
> +driver.lockManager = virLockManagerPluginNew("nop", "qemu",
> + 
> driver.config->configBaseDir,
> + 0);
> +if (!driver.lockManager)
> +return EXIT_FAILURE;
> +
> +if (!(mgr = virSecurityManagerNew(NULL, "qemu", false, false, false)))
> +return EXIT_FAILURE;

Isn't passing "NULL" here going to cause it to auto-detect the sec
driver, potentially running the real SELinux / AppArmour drivers?

I would have thought  we'd pass 'none' here to explicitly get the
no-op security driver.


Though, in the future we likely want to have a special "test" security
driver, so we can validate that the appropriate driver methods are
being invoked for each hotplug operation. Likewise for the lock manager.

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

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

Re: [libvirt] [PATCH 12/17] qemu: Export qemuProcessHandleDeviceDeleted for tests

2013-08-20 Thread Daniel P. Berrange 
On Thu, Aug 01, 2013 at 09:28:21PM +0200, Jiri Denemark wrote:
> ---
>  src/Makefile.am |  1 +
>  src/qemu/qemu_process.c |  5 +++--
>  src/qemu/qemu_process.h |  2 +-
>  src/qemu/qemu_processpriv.h | 37 +
>  4 files changed, 42 insertions(+), 3 deletions(-)
>  create mode 100644 src/qemu/qemu_processpriv.h

ACK


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

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

Re: [libvirt] [PATCH 4/6] docs: Update the formatdomain disk examples

2013-08-20 Thread Osier Yang 

On 20/08/13 21:10, John Ferlan wrote:

On 08/20/2013 03:43 AM, Osier Yang wrote:

On 20/08/13 05:21, John Ferlan wrote:

>From e31f3596893302ee1f96d2eb0cf4e006294c528c Mon Sep 17 00:00:00 2001
From: John Ferlan 
Date: Wed, 7 Aug 2013 09:05:43 -0400
Subject: [PATCH 4/7] docs: Update the formatdomain disk examples

Add more iSCSI examples including having a secret attached. There are 4 new
examples one for each way to have an iSCSI - a network disk using virtio,
a passthrough network lun using scsi, a volume disk using "mode='host'",
and a volume disk using "mode='direct'"
---
  docs/formatdomain.html.in | 164 ++
  1 file changed, 121 insertions(+), 43 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 4a927cc..5450be0 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1518,6 +1518,42 @@


  
+
+  
+  
+
+  
+  
+
+  
+  
+
+
+  
+  
+
+  
+  
+
+  
+  
+
+
+  
+  
+  
+
+  
+  
+
+
+  
+  
+  
+
+  
+  
+

...
  
@@ -1525,7 +1561,7 @@

disk
The disk element is the main container for describing
  disks. The type attribute is either "file",
-"block", "dir", or "network"
+"block", "dir", "network", or "volume"
  and refers to the underlying source for the disk. The optional
  device attribute indicates how the disk is to be exposed
  to the guest OS. Possible values for this attribute are
@@ -1575,57 +1611,96 @@
  "network" attribute since 0.8.7; "snapshot" since
  0.9.5
source
-  If the disk type is "file", then
-the file attribute specifies the fully-qualified
-path to the file holding the disk. If the disk
-type is "block", then the dev
-attribute specifies the path to the host device to serve as
-the disk. With "file", "block", and "volume", one or more optional
+  Representation of the disk source depends on the
+  disk type attribute value as follows:

disk type


Changed


+  
+  type='file'

if you are 2 spaces as the indention..


+  Since 0.0.3
+  
+  The file attribute specifies the fully-qualified

Then you will have enough spaces to indent lines like this with 2 spaces
too.


Oh right... Rather than indent more I kept the 2 spaces as the indent,
but moved back the  elements since everything under  was at 4
spaces.


+  path to the file holding the disk.
+  
+  type='block'
+  Since 0.0.3
+  
+  The dev attribute specifies the path to the
+  host device to serve as the disk.
+  
+  type='dir'
+  Since 0.7.5
+  
+  The dir attribute specifies the fully-qualified path
+  to the directory to use as the disk.
+  
+  type='network'
+  Since 0.8.7
+  
+  The protocol attribute specifies the protocol to
+  access to the requested image. Possible values are "nbd",
+  "iscsi", "rbd", "sheepdog" or "gluster".  If the
+  protocol attribute is "rbd", "sheepdog" or
+  "gluster", an additional attribute name is
+  mandatory to specify which volume/image will be used. For "nbd",
+  the name attribute is optional. For "iscsi"
+  (since 1.0.4), the name
+  attribute may include a logical unit number, separated from the
+  target's name by a slash (e.g.,
+  iqn.2013-07

Re: [libvirt] [PATCH 08/17] qemuxml2argvtest: Add XML for testing device hotplug

2013-08-20 Thread Daniel P. Berrange 
On Thu, Aug 01, 2013 at 09:28:17PM +0200, Jiri Denemark wrote:
> This is a generic XML usable for hotplugging various types of devices.
> ---
>  .../qemuxml2argv-hotplug-base.args |  7 
>  .../qemuxml2argvdata/qemuxml2argv-hotplug-base.xml | 38 
> ++
>  tests/qemuxml2argvtest.c   |  4 +++
>  3 files changed, 49 insertions(+)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hotplug-base.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hotplug-base.xml

ACK


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

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

Re: [libvirt] [PATCH 07/17] qemuhotplugtest: Define QMP_OK for the most common reply

2013-08-20 Thread Daniel P. Berrange 
On Thu, Aug 01, 2013 at 09:28:16PM +0200, Jiri Denemark wrote:
> ---
>  tests/qemuhotplugtest.c | 15 +--
>  1 file changed, 9 insertions(+), 6 deletions(-)

ACK

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

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

Re: [libvirt] [PATCH 06/17] qemuhotplugtest: Compare domain XML after device hotplug

2013-08-20 Thread Daniel P. Berrange 
On Thu, Aug 01, 2013 at 09:28:15PM +0200, Jiri Denemark wrote:
> We need to make sure a device is properly added/removed (or not) to a
> domain definition to check that a hotplug API did not lie to us.
> ---
>  tests/qemuhotplugtest.c|  67 +--
>  ...qemuhotplug-console-compat-2+console-virtio.xml | 127 
> +
>  2 files changed, 183 insertions(+), 11 deletions(-)
>  create mode 100644 
> tests/qemuhotplugtestdata/qemuhotplug-console-compat-2+console-virtio.xml

ACK


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

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

Re: [libvirt] [PATCH 04/17] qemu: Move qemuDomainDetachDeviceDiskLive to qemu_hotplug.c

2013-08-20 Thread Daniel P. Berrange 
On Thu, Aug 01, 2013 at 09:28:13PM +0200, Jiri Denemark wrote:
> ---
>  src/qemu/qemu_driver.c  | 52 --
>  src/qemu/qemu_hotplug.c | 67 
> -
>  src/qemu/qemu_hotplug.h |  9 +++
>  3 files changed, 64 insertions(+), 64 deletions(-)

ACK

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

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

Re: [libvirt] [PATCH 05/17] qemuhotplugtest: Generate better output

2013-08-20 Thread Daniel P. Berrange 
On Thu, Aug 01, 2013 at 09:28:14PM +0200, Jiri Denemark wrote:
> Each test case label now contains more data useful to identify the test.
> ---
>  tests/qemuhotplugtest.c | 29 -
>  1 file changed, 12 insertions(+), 17 deletions(-)

ACK


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

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

Re: [libvirt] [PATCH 02/17] qemu: Avoid using global qemu_driver in event handlers

2013-08-20 Thread Daniel P. Berrange 
On Thu, Aug 01, 2013 at 09:28:11PM +0200, Jiri Denemark wrote:
> We will have to pass a mock-up of the driver when testing monitor
> events.
> ---
>  src/qemu/qemu_capabilities.c |  5 ++-
>  src/qemu/qemu_monitor.c  | 24 +++
>  src/qemu/qemu_monitor.h  | 69 --
>  src/qemu/qemu_process.c  | 99 
> +++-
>  tests/qemumonitortestutils.c |  9 ++--
>  5 files changed, 130 insertions(+), 76 deletions(-)

ACK


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

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

Re: [libvirt] [PATCH 03/17] qemu: Move qemuDomainAttachDeviceDiskLive to qemu_hotplug.c

2013-08-20 Thread Daniel P. Berrange 
On Thu, Aug 01, 2013 at 09:28:12PM +0200, Jiri Denemark wrote:
> ---
>  src/qemu/qemu_driver.c  | 114 --
>  src/qemu/qemu_hotplug.c | 142 
> 
>  src/qemu/qemu_hotplug.h |  16 ++
>  3 files changed, 134 insertions(+), 138 deletions(-)

ACK


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

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

Re: [libvirt] [PATCH 01/17] qemu: Typedef monitor callbacks

2013-08-20 Thread Daniel P. Berrange 
On Thu, Aug 01, 2013 at 09:28:10PM +0200, Jiri Denemark wrote:
> Otherwise defining variables that hold callbacks pointers is ugly and
> several places have to be changed when new parameters are added.
> ---
>  src/qemu/qemu_monitor.c |   6 +-
>  src/qemu/qemu_monitor.h | 171 
> +++-
>  2 files changed, 98 insertions(+), 79 deletions(-)

ACK

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

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

Re: [libvirt] [PATCH] qemuBuildCommandLine: Fall back to mem balloon if there's no hard_limit

2013-08-20 Thread Eric Blake 
On 08/20/2013 07:06 AM, Michal Privoznik wrote:
> If there's no hard_limit set and domain uses VFIO we still must lock the
> guest memory (prerequisite from qemu). Hence, we should compute the
> amount to be locked from max_baloon.

s/baloon/balloon/

> ---
>  src/qemu/qemu_command.c | 15 +--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index c8f7df2..71c220f 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -9219,8 +9219,19 @@ qemuBuildCommandLine(virConnectPtr conn,
>  goto error;
>  }
>  
> -if (mlock)
> -virCommandSetMaxMemLock(cmd, def->mem.hard_limit * 1024);
> +if (mlock) {
> +unsigned long long memKB;
> +
> +/* VFIO requires all of the guest's memory to be
> + * locked resident, plus some amount for IO
> + * space. Alex Williamson suggested adding 1GiB for IO
> + * space just to be safe (some finer tuning might be
> + * nice, though).
> + */
> +memKB = def->mem.hard_limit ?
> +def->mem.hard_limit : def->mem.max_balloon + 1024 * 1024;
> +virCommandSetMaxMemLock(cmd, memKB * 1024);

If I'm understanding correctly, this is how much memory can be locked,
but the domain can use more (unlocked) memory as needed.  Locked memory
corresponds to what the guest sees, whereas the OOM-killer on hard-limit
was kicking in when qemu itself allocated memory not visible to the
guest.  Therefore, this is not presenting the same risk for OOM-killer
as the other hard_limit hueristic was.  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

Re: [libvirt] [PATCH] qemuSetupMemoryCgroup: Handle hard_limit properly

2013-08-20 Thread Ján Tomko 
On 08/20/2013 03:09 PM, Michal Privoznik wrote:
> On 20.08.2013 14:11, Ján Tomko wrote:
>> On 08/20/2013 11:10 AM, Michal Privoznik wrote:
>>> Since 16bcb3 we have a regression. The hard_limit is set
>>> unconditionally. By default, the limit is zero. Hence, if user hasn't
>>> configured any, we set the zero in cgroup subsystem making the kernel
>>> kill the corresponding qemu process immediately. The proper fix is to
>>> set hard_limit iff user has configure any.
>>> ---
...
>>>
>>
>> This still calls SetMaxMemLock with 0 if mem.hard_limit is 0. According to
>> commit 9395894 [1], it seems this won't be enough for VFIO passthrough
>> (although I haven't tested it). (Commit 6d8ebc7 [2] changed this to use
>> qemuDomainMemoryLimit)
>>
>> Jan
>>
>> [1] http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=9395894
>> [2] http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=6d8ebc7
> 
> I've posted a separate patch for that. So if you agree, can I push this one?
> 

Sure, go ahead.

Jan

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

Re: [libvirt] [PATCH 0/3] Fix bitmap parsing code and add tests

2013-08-20 Thread Peter Krempa 
On 08/16/13 14:41, Peter Krempa wrote:
> On 08/16/13 12:39, Daniel P. Berrange wrote:
>> On Fri, Aug 16, 2013 at 12:32:06PM +0200, Peter Krempa wrote:
>>> The bitmap parsing code might cause a crash of the application using it.
>>> Fix it and add tests so that it doesn't happen again.
>>>
>>> Peter Krempa (3):
>>>   virbitmap: Refactor virBitmapParse to avoid access beyond bounds of
>>> array
>>>   virbitmaptest: Fix function header formatting
>>>   virbitmaptest: Add test for out of bounds condition
>>>
>>>  src/util/virbitmap.c  | 38 +---
>>>  tests/virbitmaptest.c | 60 
>>> ---
>>>  2 files changed, 67 insertions(+), 31 deletions(-)
>>
>> ACK to all 3. I was just going to suggest adding tests to Alex,
>> when I saw your followup.
> 
> Thanks, I've fixed the commit message of patch 3/3 according to Eric's
> suggestion and added info about the commit introducing the problem to
> 1/3 and pushed.
> 

I've also backported the fix (1/3) to the maint branches:
v1.1.1-maint (thanks to Jan Tomko for doing that for me)
v1.1.0-maint
v1.0.6-maint
v1.0.5-maint
v1.0.4-maint and
v0.10.2-maint

I couldn't successfully build v1.0.3-maint and thus didn't bother with
the other maint branches.

Peter




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

Re: [libvirt] [PATCH 4/6] docs: Update the formatdomain disk examples

2013-08-20 Thread John Ferlan 
On 08/20/2013 03:43 AM, Osier Yang wrote:
> On 20/08/13 05:21, John Ferlan wrote:
>>
>> >From e31f3596893302ee1f96d2eb0cf4e006294c528c Mon Sep 17 00:00:00 2001
>> From: John Ferlan 
>> Date: Wed, 7 Aug 2013 09:05:43 -0400
>> Subject: [PATCH 4/7] docs: Update the formatdomain disk examples
>>
>> Add more iSCSI examples including having a secret attached. There are 4 new
>> examples one for each way to have an iSCSI - a network disk using virtio,
>> a passthrough network lun using scsi, a volume disk using "mode='host'",
>> and a volume disk using "mode='direct'"
>> ---
>>  docs/formatdomain.html.in | 164 
>> ++
>>  1 file changed, 121 insertions(+), 43 deletions(-)
>>
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index 4a927cc..5450be0 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -1518,6 +1518,42 @@
>>
>>
>>  
>> +
>> +  
>> +  
>> +
>> +  
>> +  
>> +
>> +  
>> +  
>> +
>> +
>> +  
>> +  
>> +
>> +  
>> +  
>> +
>> +  
>> +  
>> +
>> +
>> +  
>> +  
>> +  
>> +
>> +  
>> +  
>> +
>> +
>> +  
>> +  
>> +  
>> +
>> +  
>> +  
>> +
>>
>>...
>>  
>> @@ -1525,7 +1561,7 @@
>>disk
>>The disk element is the main container for describing
>>  disks. The type attribute is either "file",
>> -"block", "dir", or "network"
>> +"block", "dir", "network", or "volume"
>>  and refers to the underlying source for the disk. The optional
>>  device attribute indicates how the disk is to be 
>> exposed
>>  to the guest OS. Possible values for this attribute are
>> @@ -1575,57 +1611,96 @@
>>  "network" attribute since 0.8.7; "snapshot" since
>>  0.9.5
>>source
>> -  If the disk type is "file", then
>> -the file attribute specifies the fully-qualified
>> -path to the file holding the disk. If the disk
>> -type is "block", then the dev
>> -attribute specifies the path to the host device to serve as
>> -the disk. With "file", "block", and "volume", one or more optional
>> +  Representation of the disk source depends on the
>> +  disk type attribute value as follows:
> 
> disk type
> 

Changed

>> +  
>> +  type='file'
> 
> if you are 2 spaces as the indention..
> 
>> +  Since 0.0.3
>> +  
>> +  The file attribute specifies the fully-qualified
> 
> Then you will have enough spaces to indent lines like this with 2 spaces
> too.
> 

Oh right... Rather than indent more I kept the 2 spaces as the indent,
but moved back the  elements since everything under  was at 4
spaces.

>> +  path to the file holding the disk.
>> +  
>> +  type='block'
>> +  Since 0.0.3
>> +  
>> +  The dev attribute specifies the path to the
>> +  host device to serve as the disk.
>> +  
>> +  type='dir'
>> +  Since 0.7.5
>> +  
>> +  The dir attribute specifies the fully-qualified 
>> path
>> +  to the directory to use as the disk.
>> +  
>> +  type='network'
>> +  Since 0.8.7
>> +  
>> +  The protocol attribute specifies the protocol to
>> +  access to the requested image. Possible values are "nbd",
>> +  "iscsi", "rbd", "sheepdog" or "gluster".  If the
>> +  protocol attribute is "rbd", "sheepdog" or
>> +  "gluster", an additional attribute name is
>> +  mandatory to spe

Re: [libvirt] [PATCH] qemuSetupMemoryCgroup: Handle hard_limit properly

2013-08-20 Thread Michal Privoznik 
On 20.08.2013 14:11, Ján Tomko wrote:
> On 08/20/2013 11:10 AM, Michal Privoznik wrote:
>> Since 16bcb3 we have a regression. The hard_limit is set
>> unconditionally. By default, the limit is zero. Hence, if user hasn't
>> configured any, we set the zero in cgroup subsystem making the kernel
>> kill the corresponding qemu process immediately. The proper fix is to
>> set hard_limit iff user has configure any.
>> ---
>>  src/qemu/qemu_cgroup.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
>> index 9673e8e..e27945e 100644
>> --- a/src/qemu/qemu_cgroup.c
>> +++ b/src/qemu/qemu_cgroup.c
>> @@ -428,7 +428,8 @@ qemuSetupMemoryCgroup(virDomainObjPtr vm)
>>  }
>>  }
>>  
>> -if (virCgroupSetMemoryHardLimit(priv->cgroup, vm->def->mem.hard_limit) 
>> < 0)
>> +if (vm->def->mem.hard_limit != 0 &&
>> +virCgroupSetMemoryHardLimit(priv->cgroup, vm->def->mem.hard_limit) 
>> < 0)
>>  return -1;
>>  
>>  if (vm->def->mem.soft_limit != 0 &&
>>
> 
> This still calls SetMaxMemLock with 0 if mem.hard_limit is 0. According to
> commit 9395894 [1], it seems this won't be enough for VFIO passthrough
> (although I haven't tested it). (Commit 6d8ebc7 [2] changed this to use
> qemuDomainMemoryLimit)
> 
> Jan
> 
> [1] http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=9395894
> [2] http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=6d8ebc7

I've posted a separate patch for that. So if you agree, can I push this one?

Michal

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

[libvirt] [PATCH] qemuBuildCommandLine: Fall back to mem balloon if there's no hard_limit

2013-08-20 Thread Michal Privoznik 
If there's no hard_limit set and domain uses VFIO we still must lock the
guest memory (prerequisite from qemu). Hence, we should compute the
amount to be locked from max_baloon.
---
 src/qemu/qemu_command.c | 15 +--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c8f7df2..71c220f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9219,8 +9219,19 @@ qemuBuildCommandLine(virConnectPtr conn,
 goto error;
 }
 
-if (mlock)
-virCommandSetMaxMemLock(cmd, def->mem.hard_limit * 1024);
+if (mlock) {
+unsigned long long memKB;
+
+/* VFIO requires all of the guest's memory to be
+ * locked resident, plus some amount for IO
+ * space. Alex Williamson suggested adding 1GiB for IO
+ * space just to be safe (some finer tuning might be
+ * nice, though).
+ */
+memKB = def->mem.hard_limit ?
+def->mem.hard_limit : def->mem.max_balloon + 1024 * 1024;
+virCommandSetMaxMemLock(cmd, memKB * 1024);
+}
 
 virObjectUnref(cfg);
 return cmd;
-- 
1.8.1.5

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

Re: [libvirt] [PATCH] selinux: distinguish failure to label from request to avoid label

2013-08-20 Thread Daniel P. Berrange 
On Mon, Aug 12, 2013 at 10:19:47PM -0600, Eric Blake wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=924153
> 
> Commit 904e05a2 (v0.9.9) added a per- seclabel element with
> an attribute relabel='no' in order to try and minimize the
> impact of shutdown delays when an NFS server disappears.  The idea
> was that if a disk is on NFS and can't be labeled in the first
> place, there is no need to attempt the (no-op) relabel on domain
> shutdown.  Unfortunately, the way this was implemented was by
> modifying the domain XML so that the optimization would survive
> libvirtd restart, but in a way that is indistinguishable from an
> explicit user setting.  Furthermore, once the setting is turned
> on, libvirt avoids attempts at labeling, even for operations like
> snapshot or blockcopy where the chain is being extended or pivoted
> onto non-NFS, where SELinux labeling is once again possible.  As
> a result, it was impossible to do a blockcopy to pivot from an
> NFS image file onto a local file.
> 
> The solution is to separate the semantics of a chain that must
> not be labeled (which the user can set even on persistent domains)
> vs. the optimization of not attempting a relabel on cleanup (a
> live-only annotation), and using only the user's explicit notation
> rather than the optimization as the decision on whether to skip
> a label attempt in the first place.  When upgrading an older
> libvirtd to a newer, an NFS volume will still attempt the relabel;
> but as the avoidance of a relabel was only an optimization, this
> shouldn't cause any problems.
> 
> In the ideal future, libvirt will eventually have XML describing
> EVERY file in the backing chain, with each file having a separate
>  element.  At that point, libvirt will be able to track
> more closely which files need a relabel attempt at shutdown.  But
> until we reach that point, the single  for the entire
>  chain is treated as a hint - when a chain has only one
> file, then we know it is accurate; but if the chain has more than
> one file, we have to attempt relabel in spite of the attribute,
> in case part of the chain is local and SELinux mattered for that
> portion of the chain.
> 
> * src/conf/domain_conf.h (_virSecurityDeviceLabelDef): Add new
> member.
> * src/conf/domain_conf.c (virSecurityDeviceLabelDefParseXML):
> Parse it, for live images only.
> (virSecurityDeviceLabelDefFormat): Output it.
> (virDomainDiskDefParseXML, virDomainChrSourceDefParseXML)
> (virDomainDiskSourceDefFormat, virDomainChrDefFormat)
> (virDomainDiskDefFormat): Pass flags on through.
> * src/security/security_selinux.c
> (virSecuritySELinuxRestoreSecurityImageLabelInt): Honor labelskip
> when possible.
> (virSecuritySELinuxSetSecurityFileLabel): Set labelskip, not
> norelabel, if labeling fails.
> * docs/formatdomain.html.in (seclabel): Document new xml.
> * docs/schemas/domaincommon.rng (devSeclabel): Allow it in RNG.
> * tests/qemuxml2argvdata/qemuxml2argv-seclabel-*-skiplabel.xml:
> * tests/qemuxml2argvdata/qemuxml2argv-seclabel-*-skiplabel.args:
> * tests/qemuxml2xmloutdata/qemuxml2xmlout-seclabel-*-skiplabel.xml:
> New test files.
> * tests/qemuxml2argvtest.c (mymain): Run the new tests.
> * tests/qemuxml2xmltest.c (mymain): Likewise.
> 
> Signed-off-by: Eric Blake 
> ---
> 
>  docs/formatdomain.html.in  |  6 ++-
>  docs/schemas/domaincommon.rng  | 27 +++--
>  src/conf/domain_conf.c | 47 
> --
>  src/conf/domain_conf.h |  3 +-
>  src/security/security_selinux.c| 10 -
>  .../qemuxml2argv-seclabel-dynamic-skiplabel.args   |  5 +++
>  .../qemuxml2argv-seclabel-dynamic-skiplabel.xml| 32 +++
>  .../qemuxml2argv-seclabel-static-skiplabel.args|  5 +++
>  .../qemuxml2argv-seclabel-static-skiplabel.xml | 33 +++
>  tests/qemuxml2argvtest.c   |  2 +
>  .../qemuxml2xmlout-seclabel-dynamic-skiplabel.xml  | 31 ++
>  tests/qemuxml2xmltest.c|  8 ++--
>  12 files changed, 178 insertions(+), 31 deletions(-)
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-skiplabel.args
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-seclabel-dynamic-skiplabel.xml
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-seclabel-static-skiplabel.args
>  create mode 100644 
> tests/qemuxml2argvdata/qemuxml2argv-seclabel-static-skiplabel.xml
>  create mode 100644 
> tests/qemuxml2xmloutdata/qemuxml2xmlout-seclabel-dynamic-skiplabel.xml

ACK


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@redha

Re: [libvirt] [PATCH 3/6] Report secret usage error message similarly

2013-08-20 Thread Ján Tomko 
On 08/19/2013 11:09 PM, John Ferlan wrote:
> On 08/16/2013 12:34 PM, Ján Tomko wrote:
>> On 08/08/2013 02:43 AM, John Ferlan wrote:
>>> 
>>> VIR_SECRET_GET_VALUE_INTERNAL_CALL);
>>>  if (!secret_value) {
>>> -virReportError(VIR_ERR_INTERNAL_ERROR,
>>> -   _("could not get the value of the secret "
>>> - "for username %s"), chap.username);
>>> +if (chap.secret.uuidUsable) {
>>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>>> +   _("could not get the value of the secret "
>>> + "for username %s using uuid '%s'"),
>>> + chap.username, chap.secret.uuid);
>>> +} else {
>>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>>> +   _("could not get the value of the secret "
>>> + "for username %s using usage value '%s'"),
>>> + chap.username, chap.secret.usage);
>>
>> This looks to me like the username plays a part in getting the secret value,
>> but I'm not sure how to rephrase it while still keeping the username there.
>>
> 
> This error is a failure to find the secret value (e.g. call to
> 'secretGetValue' to the secret driver) given the "secret key"
> (virSecretPtr) which was obtained by using either the UUID or Usage value.
> 
> The username is mainly for convenience in the error message since it's
> not used by the secret driver. The username would be passed to whatever
> the code is authenticating to along with the secret value pulled from
> the secret driver.
> 
> I suppose username could be removed from the message, but it just seemed
> easier to reference the username as well since it's part of the xml that
> would use the secret.
> 

Usually we don't print the surrounding elements. Maybe the username was a part
of the error message to avoid separate errors for uuid and usage? But I'm fine
either way, the only condition for my ACK was that you remove the unrelated
whitespace change.

>>> +}
>>>  goto cleanup;
>>>  }
>>>  } else {
>>> -virReportError(VIR_ERR_INTERNAL_ERROR,
>>> -   _("username '%s' specified but secret not found"),
>>> -   chap.username);
>>> +if (chap.secret.uuidUsable) {
>>> +virReportError(VIR_ERR_NO_SECRET,
>>> +   _("username '%s' specified but the secret for "
>>> + "uuid '%s' not found"),
>>> +   chap.username, chap.secret.uuid);
>>> +} else {
>>> +virReportError(VIR_ERR_NO_SECRET,
>>> +   _("username '%s' specified but the secret for "
>>> + "usage value '%s' not found"),
>>> +   chap.username, chap.secret.usage);
>>> +}
>>>  goto cleanup;
>>
>> With VIR_ERR_NO_SECRET, this says "secret not found" twice:
>> error: Secret not found: username 'nahasapeemapetilon' specified but the
>> secret for usage value 'pwagmattasquarmsettport' not found
>>
>> But that's minor. Maybe "... but no secret matches usage '%s'"?
>>
> 
> This error message is the inability to find the "secret key"
> (virSecretPtr) given either a UUID or Usage value (the output of that
> virsh secret-list).
> 
> Is the following more appropriate?
> 
> 
> change
> 
> _("%s username '%s' specified but secret for "
>  "uuid '%s' not found")
> 
> to be:
> 
> _("no secret matches uuid '%s'")
> 
> and
> 
> _("%s username '%s' specified but secret for "
>   "usage value '%s' not found"),
> 
> to be:
> 
> _("no secret matches usage value '%s'")
> 
> 

Personally, I like the shorter versions more. But all three versions are fine.

> 
>>>  }
>>>  
>>> diff --git a/src/storage/storage_backend_rbd.c 
>>> b/src/storage/storage_backend_rbd.c
>>> index e3340f6..9c6d179 100644
>>> --- a/src/storage/storage_backend_rbd.c
>>> +++ b/src/storage/storage_backend_rbd.c
>> ...
>>>  
>>> -secret_value = conn->secretDriver->secretGetValue(secret, 
>>> &secret_value_size, 0,
>>> +secret_value = conn->secretDriver->secretGetValue(secret,
>>> +  
>>> &secret_value_size, 0,
>>>
>>> VIR_SECRET_GET_VALUE_INTERNAL_CALL);
>>>  
> 
> I removed this since it's unrelated - that's just my desire to have 80
> column displays.

Feel free to push it separately under the trivial rule, but it doesn't really
remove any lines longer than 80 columns :)


Jan

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

Re: [libvirt] [PATCH] qemuSetupMemoryCgroup: Handle hard_limit properly

2013-08-20 Thread Ján Tomko 
On 08/20/2013 11:10 AM, Michal Privoznik wrote:
> Since 16bcb3 we have a regression. The hard_limit is set
> unconditionally. By default, the limit is zero. Hence, if user hasn't
> configured any, we set the zero in cgroup subsystem making the kernel
> kill the corresponding qemu process immediately. The proper fix is to
> set hard_limit iff user has configure any.
> ---
>  src/qemu/qemu_cgroup.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> index 9673e8e..e27945e 100644
> --- a/src/qemu/qemu_cgroup.c
> +++ b/src/qemu/qemu_cgroup.c
> @@ -428,7 +428,8 @@ qemuSetupMemoryCgroup(virDomainObjPtr vm)
>  }
>  }
>  
> -if (virCgroupSetMemoryHardLimit(priv->cgroup, vm->def->mem.hard_limit) < 
> 0)
> +if (vm->def->mem.hard_limit != 0 &&
> +virCgroupSetMemoryHardLimit(priv->cgroup, vm->def->mem.hard_limit) < 
> 0)
>  return -1;
>  
>  if (vm->def->mem.soft_limit != 0 &&
> 

This still calls SetMaxMemLock with 0 if mem.hard_limit is 0. According to
commit 9395894 [1], it seems this won't be enough for VFIO passthrough
(although I haven't tested it). (Commit 6d8ebc7 [2] changed this to use
qemuDomainMemoryLimit)

Jan

[1] http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=9395894
[2] http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=6d8ebc7

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

Re: [libvirt] [PATCH 1/3] virsh: modify vshStringToArray to duplicate the elements too

2013-08-20 Thread Peter Krempa 
On 08/20/13 12:01, Osier Yang wrote:
> On 20/08/13 17:20, Peter Krempa wrote:
>> On 08/20/13 11:16, Osier Yang wrote:
>>> On 20/08/13 17:05, Peter Krempa wrote:
 At a slightly larger memory expense allow stealing of items from the
 string array returned from vshStringToArray and turn the result into a
 string list compatible with virStringSplit. This will allow to use the
 common dealloc function.

 This patch also fixes a few forgotten checks of return from
 vshStringToArray and one memory leak.
 ---
tools/virsh-nodedev.c  | 18 +-
tools/virsh-pool.c | 10 --
tools/virsh-snapshot.c | 10 ++
tools/virsh.c  |  8 +---
tools/virsh.h  |  1 +
5 files changed, 17 insertions(+), 30 deletions(-)

>> ...
>>
 diff --git a/tools/virsh.c b/tools/virsh.c
 index f65dc79..d6e3cb3 100644
 --- a/tools/virsh.c
 +++ b/tools/virsh.c
 @@ -196,7 +196,8 @@ vshStringToArray(const char *str,
>>> Why not to destroy the use of vshStringToArray, and convert to
>> vshStringToArray is converting double commas (",,") as escape sequence
>> for a comma. virStringSplit doesn't do this. We would regress in virsh
>> option parsing if we'd convert it to virStringSplit.
> 
> Integrating the escaping into virStringSplit with a flag will work,
> but I'm not against if you keep using it, since the mainly purpose
> of these patches is to fix the bug.

It won't be that easy for virStringSplit. virStringSplit is splitting
the string on a delimiter _string_ thus it won't be easy to teach it
escape sequences. The double comma escape is a virsh thing, thus I
didn't bother changing that globally (and risk potential regressions in
code that is using virStringSplit).

Peter





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

Re: [libvirt] [PATCH] qemuSetupMemoryCgroup: Handle hard_limit properly

2013-08-20 Thread Peter Krempa 
On 08/20/13 12:08, Osier Yang wrote:
> On 20/08/13 17:10, Michal Privoznik wrote:
>> Since 16bcb3 we have a regression. The hard_limit is set
>> unconditionally. By default, the limit is zero. Hence, if user hasn't

s/default,/default/

>> configured any, we set the zero in cgroup subsystem making the kernel
>> kill the corresponding qemu process immediately. The proper fix is to
>> set hard_limit iff user has configure any.
> 
> s/iff/if/,

iff means 'if and only if':
http://en.wikipedia.org/wiki/If_and_only_if

A typo is in s/configure/configured/ .

Peter




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

Re: [libvirt] [PATCH 3/3] virsh: Don't leak list of volumes when undefining domain with storage

2013-08-20 Thread Peter Krempa 
On 08/20/13 11:58, Osier Yang wrote:
> On 20/08/13 17:05, Peter Krempa wrote:
>> Use the new semantics of vshStringToArray to avoid leaking the array of
>> volumes to be deleted. The array would be leaked in case the first
>> volume was found in the domain definition. Also refactor the code a bit
>> to sanitize naming of variables hoding arrays and dimensions of the
>> arrays.
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=996050
>> ---
>>   tools/virsh-domain.c | 121
>> ---
>>   1 file changed, 58 insertions(+), 63 deletions(-)
>>
>> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
>> index 13e3045..a4b81a7 100644
>> --- a/tools/virsh-domain.c
>> +++ b/tools/virsh-domain.c
>> @@ -2932,12 +2932,11 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
>>   int rc = -1;
>>   int running;
>>   /* list of volumes to remove along with this domain */
>> -vshUndefineVolume *vlist = NULL;
>> -int nvols = 0;
>> -const char *volumes = NULL;
>> -char **volume_tokens = NULL;
>> -char *volume_tok = NULL;
>> -int nvolume_tokens = 0;
>> +vshUndefineVolume *vols = NULL;
>> +size_t nvols = 0;
>> +const char *vol_list = NULL;
>> +char **volumes = NULL;
>> +int nvolumes = 0;
> 
> Better, but still confused, e.g. "vols" and "volumes". How about:
> 
> vshUndefineVolume *vol_list = NULL;
> const char *vol_string = NULL;
> char **vol_array = NULL;
> 
> hope it's not even worse. :-)

Yeah, we need 3 sets of variables and it's getting confusing ...

> 
>>   char *def = NULL;
>>   char *source = NULL;
>>   char *target = NULL;
>> @@ -2946,10 +2945,9 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
>>   xmlDocPtr doc = NULL;
>>   xmlXPathContextPtr ctxt = NULL;
>>   xmlNodePtr *vol_nodes = NULL;
>> -int nvolumes = 0;
>> -bool vol_not_found = false;
>> +int nvol_nodes;
> 
> Hm, "vol_*" is better more now.

Yeah, seems so.

> 
>>
>> -ignore_value(vshCommandOptString(cmd, "storage", &volumes));
>> +ignore_value(vshCommandOptString(cmd, "storage", &vol_list));
>>
>>   if (managed_save) {
>>   flags |= VIR_DOMAIN_UNDEFINE_MANAGED_SAVE;
>> @@ -3017,14 +3015,17 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
>>   }
>>
>>   /* Stash domain description for later use */
>> -if (volumes || remove_all_storage) {
>> +if (vol_list || remove_all_storage) {
>>   if (running) {
>> -vshError(ctl, _("Storage volume deletion is supported
>> only on stopped domains"));
>> +vshError(ctl,
>> + _("Storage volume deletion is supported only on "
>> +   "stopped domains"));
> 
> I think we will want to change it into "inactive domains", but it's
> another story.

Okay, may be worth doing when respinning.

> 
>>   goto cleanup;
>>   }
>>
>> -if (volumes && remove_all_storage) {
>> -vshError(ctl, _("Specified both --storage and
>> --remove-all-storage"));
>> +if (vol_list && remove_all_storage) {
>> +vshError(ctl,
>> + _("Specified both --storage and
>> --remove-all-storage"));
>>   goto cleanup;
>>   }
>>

...

>> @@ -3063,56 +3061,53 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
>> "./source/@file|"
>> "./source/@dir|"
>> "./source/@name|"
>> -  "./source/@dev)", ctxt))) {
>> -if (last_error && last_error->code != VIR_ERR_OK)
>> -goto error;
> 
> Is is fine to remove this checking?

Yes it is. The check was bogous as it was actually expecting that the
error from virXPathString would raise the error and store it in
last_error which will not happen as it's not an RPC call and would
require calling vshSaveLibvirtError.

The code later on can't undefine volumes without a source anyways. The
only thing this was meant to catch is an XML parsing problem, but this
was so unlikely that we can chose to ignore it.

> 
>> -else
>> -continue;
>> -}
>> +  "./source/@dev)", ctxt)))
>> +continue;
>>
>>   /* lookup if volume was selected by user */
>>   if (volumes) {
>> -volume_tok = NULL;
>> -for (j = 0; j < nvolume_tokens; j++) {
>> -if (volume_tokens[j] &&
>> -(STREQ(volume_tokens[j], target) ||
>> - STREQ(volume_tokens[j], source))) {
>> -volume_tok = volume_tokens[j];
>> -volume_tokens[j] = NULL;
>> +bool found = false;
>> +for (j = 0; j < nvolumes; j++) {
>> +if (STREQ_NULLA

Re: [libvirt] [PATCH] qemuSetupMemoryCgroup: Handle hard_limit properly

2013-08-20 Thread Osier Yang 

On 20/08/13 17:10, Michal Privoznik wrote:

Since 16bcb3 we have a regression. The hard_limit is set
unconditionally. By default, the limit is zero. Hence, if user hasn't
configured any, we set the zero in cgroup subsystem making the kernel
kill the corresponding qemu process immediately. The proper fix is to
set hard_limit iff user has configure any.


s/iff/if/,


---
  src/qemu/qemu_cgroup.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 9673e8e..e27945e 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -428,7 +428,8 @@ qemuSetupMemoryCgroup(virDomainObjPtr vm)
  }
  }
  
-if (virCgroupSetMemoryHardLimit(priv->cgroup, vm->def->mem.hard_limit) < 0)

+if (vm->def->mem.hard_limit != 0 &&
+virCgroupSetMemoryHardLimit(priv->cgroup, vm->def->mem.hard_limit) < 0)
  return -1;
  
  if (vm->def->mem.soft_limit != 0 &&


ACK

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

Re: [libvirt] [PATCH 1/3] virsh: modify vshStringToArray to duplicate the elements too

2013-08-20 Thread Osier Yang 

On 20/08/13 17:20, Peter Krempa wrote:

On 08/20/13 11:16, Osier Yang wrote:

On 20/08/13 17:05, Peter Krempa wrote:

At a slightly larger memory expense allow stealing of items from the
string array returned from vshStringToArray and turn the result into a
string list compatible with virStringSplit. This will allow to use the
common dealloc function.

This patch also fixes a few forgotten checks of return from
vshStringToArray and one memory leak.
---
   tools/virsh-nodedev.c  | 18 +-
   tools/virsh-pool.c | 10 --
   tools/virsh-snapshot.c | 10 ++
   tools/virsh.c  |  8 +---
   tools/virsh.h  |  1 +
   5 files changed, 17 insertions(+), 30 deletions(-)


...


diff --git a/tools/virsh.c b/tools/virsh.c
index f65dc79..d6e3cb3 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -196,7 +196,8 @@ vshStringToArray(const char *str,

Why not to destroy the use of vshStringToArray, and convert to

vshStringToArray is converting double commas (",,") as escape sequence
for a comma. virStringSplit doesn't do this. We would regress in virsh
option parsing if we'd convert it to virStringSplit.


Integrating the escaping into virStringSplit with a flag will work,
but I'm not against if you keep using it, since the mainly purpose
of these patches is to fix the bug.




virStringSplit? and the comment of vshStringToArray should be updated,
since the method for free'ing the memory is different now, if we keep
using it.

Okay, I'll touch up the comment. I didn't notice it contained info how
to free it.

Peter





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

Re: [libvirt] [RFC] QCOW2 version defaults in qemu-img and libvirt

2013-08-20 Thread Daniel P. Berrange 
On Tue, Aug 20, 2013 at 11:33:36AM +0200, Ján Tomko wrote:
> Hello!
> 
> 
> QEMU is switching the default QCOW2 version from v2 (compat=0.10) to v3
> (compat=1.1) [1]
> 
> Currently, libvirt only specifies the compat=0.10 option if it was explicitly
> requested (to avoid parsing qemu-img help output [2]) and assumes the format
> to be v2 when it calls qemu-img without the compat option.
> 
> With this change in qemu-img a volume with no  or  elements
> will be created as qcow2v3 with the new qemu-img (but the compat level won't
> be reflected in volume XML until refresh).
> 
> 
> According to the IRC conversation with Eric Blake and Kevin Wolf (bug I filed:
> [3]), it seems we should:
> 
> * always specify the compat option if it's supported by qemu-img (which would
> solve the problem mentioned above)

This is definitely desired.

> 
> * provide an option in qemu.conf to set the default compatibility level,
> defaulting to 1.1 to make it easier to use the new format
> This would probably require a new storage.conf file, since the storage driver
> doesn't have access to the qemu driver config, but: does this seem reasonable?
> Should we add a default feature list (for the only feature) as well?

I'm not really convinced by this. If we allowing change the default to
be v3, this may well break applications / harm portability of the
qcow files they create.

IMHO we should only ever use v3 if the app requested v3.

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

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

Re: [libvirt] [PATCH 3/3] virsh: Don't leak list of volumes when undefining domain with storage

2013-08-20 Thread Osier Yang 

On 20/08/13 17:05, Peter Krempa wrote:

Use the new semantics of vshStringToArray to avoid leaking the array of
volumes to be deleted. The array would be leaked in case the first
volume was found in the domain definition. Also refactor the code a bit
to sanitize naming of variables hoding arrays and dimensions of the
arrays.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=996050
---
  tools/virsh-domain.c | 121 ---
  1 file changed, 58 insertions(+), 63 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 13e3045..a4b81a7 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -2932,12 +2932,11 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
  int rc = -1;
  int running;
  /* list of volumes to remove along with this domain */
-vshUndefineVolume *vlist = NULL;
-int nvols = 0;
-const char *volumes = NULL;
-char **volume_tokens = NULL;
-char *volume_tok = NULL;
-int nvolume_tokens = 0;
+vshUndefineVolume *vols = NULL;
+size_t nvols = 0;
+const char *vol_list = NULL;
+char **volumes = NULL;
+int nvolumes = 0;


Better, but still confused, e.g. "vols" and "volumes". How about:

vshUndefineVolume *vol_list = NULL;
const char *vol_string = NULL;
char **vol_array = NULL;

hope it's not even worse. :-)


  char *def = NULL;
  char *source = NULL;
  char *target = NULL;
@@ -2946,10 +2945,9 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
  xmlDocPtr doc = NULL;
  xmlXPathContextPtr ctxt = NULL;
  xmlNodePtr *vol_nodes = NULL;
-int nvolumes = 0;
-bool vol_not_found = false;
+int nvol_nodes;


Hm, "vol_*" is better more now.



-ignore_value(vshCommandOptString(cmd, "storage", &volumes));
+ignore_value(vshCommandOptString(cmd, "storage", &vol_list));

  if (managed_save) {
  flags |= VIR_DOMAIN_UNDEFINE_MANAGED_SAVE;
@@ -3017,14 +3015,17 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
  }

  /* Stash domain description for later use */
-if (volumes || remove_all_storage) {
+if (vol_list || remove_all_storage) {
  if (running) {
-vshError(ctl, _("Storage volume deletion is supported only on stopped 
domains"));
+vshError(ctl,
+ _("Storage volume deletion is supported only on "
+   "stopped domains"));


I think we will want to change it into "inactive domains", but it's 
another story.



  goto cleanup;
  }

-if (volumes && remove_all_storage) {
-vshError(ctl, _("Specified both --storage and 
--remove-all-storage"));
+if (vol_list && remove_all_storage) {
+vshError(ctl,
+ _("Specified both --storage and --remove-all-storage"));
  goto cleanup;
  }

@@ -3038,20 +3039,17 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
  goto error;

  /* tokenize the string from user and save it's parts into an array */
-if (volumes) {
-if ((nvolume_tokens = vshStringToArray(volumes, &volume_tokens)) < 
0)
-goto cleanup;
-}
-
-if ((nvolumes = virXPathNodeSet("./devices/disk", ctxt,
-&vol_nodes)) < 0)
+if (vol_list &&
+(nvolumes = vshStringToArray(vol_list, &volumes)) < 0)
  goto error;

-if (nvolumes > 0)
-vlist = vshCalloc(ctl, nvolumes, sizeof(*vlist));
+if ((nvol_nodes = virXPathNodeSet("./devices/disk", ctxt,
+  &vol_nodes)) < 0)
+goto error;

-for (i = 0; i < nvolumes; i++) {
+for (i = 0; i < nvol_nodes; i++) {
  ctxt->node = vol_nodes[i];
+vshUndefineVolume vol;
  VIR_FREE(source);
  VIR_FREE(target);

@@ -3063,56 +3061,53 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd)
"./source/@file|"
"./source/@dir|"
"./source/@name|"
-  "./source/@dev)", ctxt))) {
-if (last_error && last_error->code != VIR_ERR_OK)
-goto error;


Is is fine to remove this checking?


-else
-continue;
-}
+  "./source/@dev)", ctxt)))
+continue;

  /* lookup if volume was selected by user */
  if (volumes) {
-volume_tok = NULL;
-for (j = 0; j < nvolume_tokens; j++) {
-if (volume_tokens[j] &&
-(STREQ(volume_tokens[j], target) ||
- STREQ(volume_tokens[j], source))) {
-volume_tok = volume_tokens[j];
-volume_tokens

Re: [libvirt] [PATCH v2] Check for --no-copy-dt-needed linker flag

2013-08-20 Thread Daniel P. Berrange 
On Mon, Aug 19, 2013 at 08:07:42PM +0200, Guido Günther wrote:
> On Mon, Aug 19, 2013 at 11:37:24AM -0600, Eric Blake wrote:
> > On 08/19/2013 11:28 AM, Guido Günther wrote:
> > > and use it when available
> > > ---
> > >  configure.ac  |  1 +
> > >  m4/virt-linker-no-indirect.m4 | 30 ++
> > >  src/Makefile.am   |  9 +
> > >  tests/Makefile.am |  1 +
> > >  4 files changed, 41 insertions(+)
> > >  create mode 100644 m4/virt-linker-no-indirect.m4
> > > 
> > 
> > > +AC_DEFUN([LIBVIRT_LINKER_NO_INDIRECT],[
> > > +AC_MSG_CHECKING([for how to avoid indirect lib deps])
> > > +
> > > +NO_INDIRECT_LDFLAGS=
> > > +`$LD --help 2>&1 | grep -- "--no-copy-dt-needed-entries" >/dev/null` 
> > > && \
> > 
> > Doesn't do what you think (it tries to execute the output of grep -
> > which is thankfully empty on both success and failure).  Also wastes a
> > fork on grep, compared to the simpler:
> 
> I was looking following virt-linker-relro.m4 and I'm getting the correct
> result since the output is always empty but for the non matching case
> grep exits with 1. However
> 
> > 
> > case `$LD --help 2>&1` in
> >   *--no-copy-dt-needed-entries*) NO_INDIRECT_LDFLAGS=... ;;
> > esac
> 
> this looks nicer.
> 
> > 
> > > +++ b/src/Makefile.am
> > > @@ -1813,6 +1813,7 @@ libvirt_la_LDFLAGS = \
> > >   $(LIBVIRT_NODELETE) \
> > >   $(AM_LDFLAGS) \
> > >   $(RELRO_LDFLAGS) \
> > > + $(NO_INDIRECT_LDFLAGS) \
> > 
> > Why aren't you building this directly into $(AM_LDFLAGS) once, rather
> > than having to copy it into each and every library recipe?
> > 
> > Why only src/ and tests/? What about tools/ and daemon/?
> 
> Again following the above example. I'll come up with a v3.
> Cheers,

Personally I prefer to see things explicitly listed rather than hide
them behind AM_LDFLAGS.

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

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

Re: [libvirt] [RFC] Expose cpu_map.xml via API

2013-08-20 Thread Daniel P. Berrange 
On Mon, Aug 19, 2013 at 08:19:56PM +0200, Giuseppe Scrivano wrote:
> ---
> I have started working on:
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=916786
> 
> +/**
> + * virConnectGetCPUMapDesc:
> + *
> + * @conn: virConnect connection
> + *
> + * Get the content of the cpu_map.xml file used by the connection.
> + *
> + * Returns XML description of the cpu_map.xml file.
> + */
> +char *virConnectGetCPUMapDesc(virConnectPtr conn);

This is really not at all neccessary, or desirable.

We now have the ability to query the full CPU flag set from the
virConnectBaselineCPU API.

All we're missing is thus a way to get a list of supported CPU
model names. For that we can just have

  int virConnectGetCPUModelNames(virConnectPtr conn, char ***models);


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

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

Re: [libvirt] [PATCH v2]LXC: Helper function for checking ownership of dir when userns enabled

2013-08-20 Thread Chen HanXiao 
Hi
Any comments?

Thanks

> -Original Message-
> From: libvir-list-boun...@redhat.com
[mailto:libvir-list-boun...@redhat.com]
> On Behalf Of Chen HanXiao
> Sent: Wednesday, August 14, 2013 9:30 AM
> To: 'Daniel P. Berrange'
> Cc: libvir-list@redhat.com
> Subject: Re: [libvirt] [PATCH v2]LXC: Helper function for checking
ownership of
> dir when userns enabled
> 
> 
> 
> > -Original Message-
> > From: Daniel P. Berrange [mailto:berra...@redhat.com]
> > Sent: Saturday, August 10, 2013 12:54 AM
> > To: Chen Hanxiao
> > Cc: libvir-list@redhat.com
> > Subject: Re: [libvirt] [PATCH v2]LXC: Helper function for checking
ownership of
> > dir when userns enabled
> >
> > On Fri, Aug 09, 2013 at 04:05:58PM +0800, Chen Hanxiao wrote:
> > > From: Chen Hanxiao 
> > >
> > > If we enable userns, the ownership of dir we provided for containers
> > > should match the uid/gid in idmap.
> > > Currently, the debug log is very implicit or misleading sometimes.
> > > This patch will help clarify this for us when using
> > > debug log or virsh.
> >
> > I do recall hitting some permission issue once, but can't remember
> > just what it was. Can you describe exactly how to reproduce the
> > problem ?
> >
> 
> 1)  Enable user namespace in kernel
> 2)  Add idmap for container
> 3)  Don't change the ownership of devices/ filesystem/ source dir  ( leave
> them to 'root' for instance)
> 4)  Start the container
> 
> Usually I got an input/output error by virsh, which is not a good hint.
> 
> 
> > > Signed-off-by: Chen Hanxiao 
> > > ---
> > >  src/lxc/lxc_container.c |   46
> > ++
> > >  1 files changed, 46 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> > > index b910b10..2ccdc61 100644
> > > --- a/src/lxc/lxc_container.c
> > > +++ b/src/lxc/lxc_container.c
> > > @@ -1815,6 +1815,49 @@ lxcNeedNetworkNamespace(virDomainDefPtr
> > def)
> > >  return false;
> > >  }
> > >
> > > +/*
> > > + * Helper function for helping check
> > > + * whether we have enough privilege
> > > + * to operate the source dir when userns enabled
> > > + * @vmDef: pointer to vm definition structure
> > > + * Returns 0 on success or -1 in case of error
> > > + */
> > > +static int
> > > +lxcContainerUsernsSrcOwnershipCheck(virDomainDefPtr vmDef)
> > > +{
> > > +struct stat buf;
> > > +size_t i;
> > > +uid_t uid;
> > > +gid_t gid;
> > > +
> > > +VIR_DEBUG("vmDef->nfss %d", (int)vmDef->nfss);
> > > +for (i = 0; i < vmDef->nfss; i++) {
> > > +VIR_DEBUG("dst is %s, src is %s",
> > > +  vmDef->fss[i]->dst,
> > > +  vmDef->fss[i]->src);
> > > +
> > > +uid = vmDef->idmap.uidmap[0].target;
> > > +gid = vmDef->idmap.gidmap[0].target;
> > > +
> > > +if (lstat(vmDef->fss[i]->src, &buf) < 0) {
> > > +virReportSystemError(errno, _("Cannot access '%s'"),
> > > + vmDef->fss[i]->src);
> > > +return -1;
> > > +} else if (uid != buf.st_uid || gid != buf.st_gid) {
> > > +VIR_DEBUG("In userns uid is %d, gid is %d\n",
> > > +  uid, gid);
> > > +errno = EINVAL;
> > > +
> > > +virReportSystemError(errno,
> > > +  _("[userns] Src dir '%s' does not
> > belong to uid/gid: %d/%d"),
> > > + vmDef->fss[i]->src, uid, gid);
> > > +return -1;
> > > +}
> > > +}
> > > +
> > > +return 0;
> > > +}
> > > +
> > >  /**
> > >   * lxcContainerStart:
> > >   * @def: pointer to virtual machine structure
> > > @@ -1866,6 +1909,9 @@ int lxcContainerStart(virDomainDefPtr def,
> > >  if (userns_supported()) {
> > >  VIR_DEBUG("Enable user namespace");
> > >  cflags |= CLONE_NEWUSER;
> > > +if (lxcContainerUsernsSrcOwnershipCheck(def) < 0) {
> > > +return -1;
> > > +}
> > >  } else {
> > >  virReportSystemError(VIR_ERR_CONFIG_UNSUPPORTED,
> > "%s",
> > >   _("Kernel doesn't support user
> > namespace"));
> >
> >
> > 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


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

[libvirt] [RFC] QCOW2 version defaults in qemu-img and libvirt

2013-08-20 Thread Ján Tomko 
Hello!


QEMU is switching the default QCOW2 version from v2 (compat=0.10) to v3
(compat=1.1) [1]

Currently, libvirt only specifies the compat=0.10 option if it was explicitly
requested (to avoid parsing qemu-img help output [2]) and assumes the format
to be v2 when it calls qemu-img without the compat option.

With this change in qemu-img a volume with no  or  elements
will be created as qcow2v3 with the new qemu-img (but the compat level won't
be reflected in volume XML until refresh).


According to the IRC conversation with Eric Blake and Kevin Wolf (bug I filed:
[3]), it seems we should:

* always specify the compat option if it's supported by qemu-img (which would
solve the problem mentioned above)

* provide an option in qemu.conf to set the default compatibility level,
defaulting to 1.1 to make it easier to use the new format
This would probably require a new storage.conf file, since the storage driver
doesn't have access to the qemu driver config, but: does this seem reasonable?
Should we add a default feature list (for the only feature) as well?


Jan


[1] http://lists.nongnu.org/archive/html/qemu-devel/2013-08/msg02549.html
[2] https://www.redhat.com/archives/libvir-list/2013-February/msg00301.html
[3] https://bugzilla.redhat.com/show_bug.cgi?id=997977

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

[libvirt] SIGSEGV using virConnect.newStream with Python

2013-08-20 Thread Claudio Bley 
Hi.

I tried this on Fedora 19, using libvirt 1.0.5 and also tested with
git v1.1.1-maint as well as git master on Ubuntu 12.04.

How to reproduce:

 python -
import libvirt as l
c = l.virConnect("test:///default")
v = c.virStream()
-

Here's my GDB session with git master:

$ gdb python
GNU gdb (Ubuntu/Linaro 7.4-2012.04-0ubuntu2.1) 7.4-2012.04
Copyright (C) 2012 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later 
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-linux-gnu".
For bug reporting instructions, please see:
...
Reading symbols from /usr/bin/python...(no debugging symbols found)...done.
(gdb) run
Starting program: /usr/bin/python
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Python 2.7.3 (default, Apr 10 2013, 06:20:15)
[GCC 4.6.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import libvirt as l
2013-08-20 09:21:39.437+: 12678: info : libvirt version: 1.1.1
2013-08-20 09:21:39.437+: 12678: debug : virGlobalInit:438 : register 
drivers
2013-08-20 09:21:39.437+: 12678: debug : virRegisterDriver:764 : 
driver=0x75dc6760 name=Test
2013-08-20 09:21:39.437+: 12678: debug : virRegisterDriver:776 : 
registering Test as driver 0
2013-08-20 09:21:39.437+: 12678: debug : virRegisterNetworkDriver:611 : 
registering Test as network driver 0
2013-08-20 09:21:39.437+: 12678: debug : virRegisterInterfaceDriver:638 : 
registering Test as interface driver 0
2013-08-20 09:21:39.437+: 12678: debug : virRegisterStorageDriver:665 : 
registering Test as storage driver 0
2013-08-20 09:21:39.437+: 12678: debug : virRegisterNodeDeviceDriver:692 : 
registering Test as device driver 0
2013-08-20 09:21:39.437+: 12678: debug : virRegisterSecretDriver:719 : 
registering Test as secret driver 0
2013-08-20 09:21:39.437+: 12678: debug : virRegisterNWFilterDriver:746 : 
registering Test as network filter driver 0
2013-08-20 09:21:39.437+: 12678: debug : virRegisterSecretDriver:719 : 
registering Test as secret driver 0 
 
[19/1965]
2013-08-20 09:21:39.437+: 12678: debug : virRegisterNWFilterDriver:746 : 
registering Test as network filter driver 0
2013-08-20 09:21:39.437+: 12678: debug : virRegisterDriver:764 : 
driver=0x75dc7ec0 name=OPENVZ
2013-08-20 09:21:39.437+: 12678: debug : virRegisterDriver:776 : 
registering OPENVZ as driver 1
2013-08-20 09:21:39.437+: 12678: debug : virRegisterDriver:764 : 
driver=0x75dc84e0 name=VMWARE
2013-08-20 09:21:39.437+: 12678: debug : virRegisterDriver:776 : 
registering VMWARE as driver 2
2013-08-20 09:21:39.437+: 12678: debug : parallelsRegister:2423 : Can't 
find prlctl command in the PATH env
2013-08-20 09:21:39.437+: 12678: debug : virRegisterDriver:764 : 
driver=0x75dc71a0 name=remote
2013-08-20 09:21:39.437+: 12678: debug : virRegisterDriver:776 : 
registering remote as driver 3
2013-08-20 09:21:39.437+: 12678: debug : virRegisterNetworkDriver:611 : 
registering remote as network driver 1
2013-08-20 09:21:39.437+: 12678: debug : virRegisterInterfaceDriver:638 : 
registering remote as interface driver 1
2013-08-20 09:21:39.437+: 12678: debug : virRegisterStorageDriver:665 : 
registering remote as storage driver 1
2013-08-20 09:21:39.437+: 12678: debug : virRegisterNodeDeviceDriver:692 : 
registering remote as device driver 1
2013-08-20 09:21:39.437+: 12678: debug : virRegisterSecretDriver:719 : 
registering remote as secret driver 1
2013-08-20 09:21:39.437+: 12678: debug : virRegisterNWFilterDriver:746 : 
registering remote as network filter driver 1
>>> c = l.virConnect("test:///default")
>>> v = c.newStream()
2013-08-20 09:22:08.120+: 12678: debug : virStreamNew:16922 : conn=0xf, 
flags=0

Program received signal SIGSEGV, Segmentation fault.
0x759be0e9 in virObjectIsClass (anyobj=, klass=0x0) at 
util/virobject.c:362
362 return virClassIsDerivedFrom(obj->klass, klass);

(gdb) bt full
#0  0x759be0e9 in virObjectIsClass (anyobj=, klass=0x0) 
at util/virobject.c:362
obj = 0xf
#1  0x75a7fca8 in virStreamNew (conn=0xf, flags=0) at libvirt.c:16926
st = 
__func__ = "virStreamNew"
__FUNCTION__ = "virStreamNew"
#2  0x75df7f72 in libvirt_virStreamNew (self=, 
args=) at libvirt.c:2617
_save = 0x0
c_retval = 
conn = 0xf
pyobj_conn = 0x77edd5a8
flags = 0
#3  0x00466254 in PyEval_EvalFrameEx ()
No symbol table info available.
#4  0x0057bd02 in PyEval_

[libvirt] [PATCHv2 2/2] virBitmapParse: Don't shadow errors

2013-08-20 Thread Peter Krempa 
A few of the callers of virBitmapParse shadow the returned error.
---

Notes:
I'm kind of worried that we are making some error messages
worse compared to what they were before. If you don't like
the way this will turn out, I'll prepare a inverse fix
where all callers will be fixed to report proper errors
and virBitmapParse will remain silent.

 src/conf/domain_conf.c  | 5 +
 src/conf/network_conf.c | 3 ---
 src/nodeinfo.c  | 5 +
 src/qemu/qemu_driver.c  | 2 --
 src/xenxs/xen_sxpr.c| 5 +
 tests/cpuset| 2 +-
 6 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 12b68ea..3fa4f00 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -10971,11 +10971,8 @@ virDomainDefParseXML(xmlDocPtr xml,
 tmp = virXPathString("string(./vcpu[1]/@cpuset)", ctxt);
 if (tmp) {
 if (virBitmapParse(tmp, 0, &def->cpumask,
-   VIR_DOMAIN_CPUMASK_LEN) < 0) {
-virReportError(VIR_ERR_XML_ERROR,
-   "%s", _("topology cpuset syntax error"));
+   VIR_DOMAIN_CPUMASK_LEN) < 0)
 goto error;
-}
 VIR_FREE(tmp);
 }
 }
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index bbc980b..8aef609 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -2897,9 +2897,6 @@ virNetworkLoadState(virNetworkObjListPtr nets,
 if ((class_id = virXPathString("string(./class_id[1]/@bitmap)", 
ctxt))) {
 if (virBitmapParse(class_id, 0, &class_id_map,
CLASS_ID_BITMAP_SIZE) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("Malformed 'class_id' attribute: %s"),
-   class_id);
 VIR_FREE(class_id);
 goto error;
 }
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 4df4851..33a79b7 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -1547,11 +1547,8 @@ virNodeGetSiblingsList(const char *dir, int cpu_id)
 if (virFileReadAll(path, SYSFS_THREAD_SIBLINGS_LIST_LENGTH_MAX, &buf) < 0)
 goto cleanup;

-if (virBitmapParse(buf, 0, &ret, NUMA_MAX_N_CPUS) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Failed to parse thread siblings"));
+if (virBitmapParse(buf, 0, &ret, NUMA_MAX_N_CPUS) < 0)
 goto cleanup;
-}

 cleanup:
 VIR_FREE(buf);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2ad236e..5124f27 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8357,8 +8357,6 @@ qemuDomainSetNumaParameters(virDomainPtr dom,
 if (virBitmapParse(params[i].value.s,
0, &nodeset,
VIR_DOMAIN_CPUMASK_LEN) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Failed to parse nodeset"));
 ret = -1;
 continue;
 }
diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c
index fbbbaa9..6209c68 100644
--- a/src/xenxs/xen_sxpr.c
+++ b/src/xenxs/xen_sxpr.c
@@ -1160,11 +1160,8 @@ xenParseSxpr(const struct sexpr *root,

 if (cpus != NULL) {
 if (virBitmapParse(cpus, 0, &def->cpumask,
-   VIR_DOMAIN_CPUMASK_LEN) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("invalid CPU mask %s"), cpus);
+   VIR_DOMAIN_CPUMASK_LEN) < 0)
 goto error;
-}
 }

 def->maxvcpus = sexpr_int(root, "domain/vcpus");
diff --git a/tests/cpuset b/tests/cpuset
index b617d6f..8bcaae9 100755
--- a/tests/cpuset
+++ b/tests/cpuset
@@ -42,7 +42,7 @@ sed "s/vcpu placement='static'>/vcpu cpuset='aaa'>/" xml > 
xml-invalid || fail=1
 $abs_top_builddir/tools/virsh --connect test:///default define xml-invalid > 
out 2>&1 && fail=1
 cat <<\EOF > exp || fail=1
 error: Failed to define domain from xml-invalid
-error: XML error: topology cpuset syntax error
+error: internal error: Failed to parse bitmap 'aaa'

 EOF
 compare exp out || fail=1
-- 
1.8.3.2

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

[libvirt] [PATCHv2 1/2] virBitmapParse: Fix behavior in case of error

2013-08-20 Thread Peter Krempa 
Re-arrange the code so that the returned bitmap is always initialized to
NULL even on early failures and return an error message as some callers
are already expecting it.
---

Notes:
Version 2:
Was already ACKed in v1, but:
* fixed bracing of arguments of the _() macro
* added src/uti/virbitmap.c to po/POTFILES.in
(noticed by doing a full build ... )

 po/POTFILES.in   |  1 +
 src/util/virbitmap.c | 18 +-
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/po/POTFILES.in b/po/POTFILES.in
index 36d027a..9a83069 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -147,6 +147,7 @@ src/util/viralloc.c
 src/util/viraudit.c
 src/util/virauth.c
 src/util/virauthconfig.c
+src/util/virbitmap.c
 src/util/vircgroup.c
 src/util/virclosecallbacks.c
 src/util/vircommand.c
diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c
index 47c678e..289a7b9 100644
--- a/src/util/virbitmap.c
+++ b/src/util/virbitmap.c
@@ -298,23 +298,21 @@ virBitmapParse(const char *str,
size_t bitmapSize)
 {
 bool neg = false;
-const char *cur;
+const char *cur = str;
 char *tmp;
 size_t i;
 int start, last;

-if (!str)
+if (!(*bitmap = virBitmapNew(bitmapSize)))
 return -1;

-cur = str;
-virSkipSpaces(&cur);
+if (!str)
+goto error;

-if (*cur == 0)
-return -1;
+virSkipSpaces(&cur);

-*bitmap = virBitmapNew(bitmapSize);
-if (!*bitmap)
-return -1;
+if (*cur == '\0')
+goto error;

 while (*cur != 0 && *cur != terminator) {
 /*
@@ -384,6 +382,8 @@ virBitmapParse(const char *str,
 return virBitmapCountBits(*bitmap);

 error:
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Failed to parse bitmap '%s'"), str);
 virBitmapFree(*bitmap);
 *bitmap = NULL;
 return -1;
-- 
1.8.3.2

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

[libvirt] [PATCHv2 0/2] Fix error reporting of virBitmapParse

2013-08-20 Thread Peter Krempa 
Please see notes in individual patches.

Peter Krempa (2):
  virBitmapParse: Fix behavior in case of error
  virBitmapParse: Don't shadow errors

 po/POTFILES.in  |  1 +
 src/conf/domain_conf.c  |  5 +
 src/conf/network_conf.c |  3 ---
 src/nodeinfo.c  |  5 +
 src/qemu/qemu_driver.c  |  2 --
 src/util/virbitmap.c| 18 +-
 src/xenxs/xen_sxpr.c|  5 +
 tests/cpuset|  2 +-
 8 files changed, 14 insertions(+), 27 deletions(-)

-- 
1.8.3.2

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

Re: [libvirt] [PATCH 1/3] virsh: modify vshStringToArray to duplicate the elements too

2013-08-20 Thread Peter Krempa 
On 08/20/13 11:16, Osier Yang wrote:
> On 20/08/13 17:05, Peter Krempa wrote:
>> At a slightly larger memory expense allow stealing of items from the
>> string array returned from vshStringToArray and turn the result into a
>> string list compatible with virStringSplit. This will allow to use the
>> common dealloc function.
>>
>> This patch also fixes a few forgotten checks of return from
>> vshStringToArray and one memory leak.
>> ---
>>   tools/virsh-nodedev.c  | 18 +-
>>   tools/virsh-pool.c | 10 --
>>   tools/virsh-snapshot.c | 10 ++
>>   tools/virsh.c  |  8 +---
>>   tools/virsh.h  |  1 +
>>   5 files changed, 17 insertions(+), 30 deletions(-)
>>

...

>>
>> diff --git a/tools/virsh.c b/tools/virsh.c
>> index f65dc79..d6e3cb3 100644
>> --- a/tools/virsh.c
>> +++ b/tools/virsh.c
>> @@ -196,7 +196,8 @@ vshStringToArray(const char *str,
> 
> Why not to destroy the use of vshStringToArray, and convert to

vshStringToArray is converting double commas (",,") as escape sequence
for a comma. virStringSplit doesn't do this. We would regress in virsh
option parsing if we'd convert it to virStringSplit.

> virStringSplit? and the comment of vshStringToArray should be updated,
> since the method for free'ing the memory is different now, if we keep
> using it.

Okay, I'll touch up the comment. I didn't notice it contained info how
to free it.

Peter





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

Re: [libvirt] [PATCH 2/3] virsh-pool: Improve error message in cmdPoolList

2013-08-20 Thread Osier Yang 

On 20/08/13 17:05, Peter Krempa wrote:

Explicitly let the user know about the unknown volume name.


s/volume name/pool type/,

ACK with above.

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

Re: [libvirt] [PATCH 1/3] virsh: modify vshStringToArray to duplicate the elements too

2013-08-20 Thread Osier Yang 

On 20/08/13 17:05, Peter Krempa wrote:

At a slightly larger memory expense allow stealing of items from the
string array returned from vshStringToArray and turn the result into a
string list compatible with virStringSplit. This will allow to use the
common dealloc function.

This patch also fixes a few forgotten checks of return from
vshStringToArray and one memory leak.
---
  tools/virsh-nodedev.c  | 18 +-
  tools/virsh-pool.c | 10 --
  tools/virsh-snapshot.c | 10 ++
  tools/virsh.c  |  8 +---
  tools/virsh.h  |  1 +
  5 files changed, 17 insertions(+), 30 deletions(-)

diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c
index 3255079..cfbf3bc 100644
--- a/tools/virsh-nodedev.c
+++ b/tools/virsh-nodedev.c
@@ -161,10 +161,7 @@ cmdNodeDeviceDestroy(vshControl *ctl, const vshCmd *cmd)

  ret = true;
  cleanup:
-if (arr) {
-VIR_FREE(*arr);
-VIR_FREE(arr);
-}
+virStringFreeList(arr);
  virNodeDeviceFree(dev);
  return ret;
  }
@@ -409,7 +406,8 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
  vshError(ctl, "%s", _("Options --tree and --cap are 
incompatible"));
  return false;
  }
-ncaps = vshStringToArray(cap_str, &caps);
+if ((ncaps = vshStringToArray(cap_str, &caps)) < 0)
+  return false;
  }

  for (i = 0; i < ncaps; i++) {
@@ -503,10 +501,7 @@ cmdNodeListDevices(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
  }

  cleanup:
-if (caps) {
-VIR_FREE(*caps);
-VIR_FREE(caps);
-}
+virStringFreeList(caps);
  vshNodeDeviceListFree(list);
  return ret;
  }
@@ -574,10 +569,7 @@ cmdNodeDeviceDumpXML(vshControl *ctl, const vshCmd *cmd)

  ret = true;
  cleanup:
-if (arr) {
-VIR_FREE(*arr);
-VIR_FREE(arr);
-}
+virStringFreeList(arr);
  VIR_FREE(xml);
  virNodeDeviceFree(device);
  return ret;
diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c
index 1622be2..b8fc8d7 100644
--- a/tools/virsh-pool.c
+++ b/tools/virsh-pool.c
@@ -995,12 +995,13 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
  char **poolTypes = NULL;
  int npoolTypes = 0;

-npoolTypes = vshStringToArray(type, &poolTypes);
+if ((npoolTypes = vshStringToArray(type, &poolTypes)) < 0)
+return false;

  for (i = 0; i < npoolTypes; i++) {
  if ((poolType = virStoragePoolTypeFromString(poolTypes[i])) < 0) {
  vshError(ctl, "%s", _("Invalid pool type"));
-VIR_FREE(poolTypes);
+virStringFreeList(poolTypes);
  return false;
  }

@@ -1036,10 +1037,7 @@ cmdPoolList(vshControl *ctl, const vshCmd *cmd 
ATTRIBUTE_UNUSED)
  break;
  }
  }
-if (poolTypes) {
-VIR_FREE(*poolTypes);
-VIR_FREE(poolTypes);
-}
+virStringFreeList(poolTypes);
  }

  if (!(list = vshStoragePoolListCollect(ctl, flags)))
diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
index db9715b..e37a5b3 100644
--- a/tools/virsh-snapshot.c
+++ b/tools/virsh-snapshot.c
@@ -261,10 +261,7 @@ vshParseSnapshotMemspec(vshControl *ctl, virBufferPtr buf, 
const char *str)
  cleanup:
  if (ret < 0)
  vshError(ctl, _("unable to parse memspec: %s"), str);
-if (array) {
-VIR_FREE(*array);
-VIR_FREE(array);
-}
+virStringFreeList(array);
  return ret;
  }

@@ -313,10 +310,7 @@ vshParseSnapshotDiskspec(vshControl *ctl, virBufferPtr 
buf, const char *str)
  cleanup:
  if (ret < 0)
  vshError(ctl, _("unable to parse diskspec: %s"), str);
-if (array) {
-VIR_FREE(*array);
-VIR_FREE(array);
-}
+virStringFreeList(array);
  return ret;
  }

diff --git a/tools/virsh.c b/tools/virsh.c
index f65dc79..d6e3cb3 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -196,7 +196,8 @@ vshStringToArray(const char *str,


Why not to destroy the use of vshStringToArray, and convert to
virStringSplit? and the comment of vshStringToArray should be updated,
since the method for free'ing the memory is different now, if we keep
using it.

Osier

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

[libvirt] [PATCH] qemuSetupMemoryCgroup: Handle hard_limit properly

2013-08-20 Thread Michal Privoznik 
Since 16bcb3 we have a regression. The hard_limit is set
unconditionally. By default, the limit is zero. Hence, if user hasn't
configured any, we set the zero in cgroup subsystem making the kernel
kill the corresponding qemu process immediately. The proper fix is to
set hard_limit iff user has configure any.
---
 src/qemu/qemu_cgroup.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 9673e8e..e27945e 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -428,7 +428,8 @@ qemuSetupMemoryCgroup(virDomainObjPtr vm)
 }
 }
 
-if (virCgroupSetMemoryHardLimit(priv->cgroup, vm->def->mem.hard_limit) < 0)
+if (vm->def->mem.hard_limit != 0 &&
+virCgroupSetMemoryHardLimit(priv->cgroup, vm->def->mem.hard_limit) < 0)
 return -1;
 
 if (vm->def->mem.soft_limit != 0 &&
-- 
1.8.1.5

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

[libvirt] [PATCH 0/3] Clean up usage of vshStringToArray

2013-08-20 Thread Peter Krempa 
Usage of vshStringToArray in virsh was problematic in a few places.
Clean it up to avoid a few memleaks. Also improve an error message.


Peter Krempa (3):
  virsh: modify vshStringToArray to duplicate the elements too
  virsh-pool: Improve error message in cmdPoolList
  virsh: Don't leak list of volumes when undefining domain with storage

 tools/virsh-domain.c   | 121 -
 tools/virsh-nodedev.c  |  18 ++--
 tools/virsh-pool.c |  12 ++---
 tools/virsh-snapshot.c |  10 +---
 tools/virsh.c  |   8 ++--
 tools/virsh.h  |   1 +
 6 files changed, 76 insertions(+), 94 deletions(-)

-- 
1.8.3.2

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

[libvirt] [PATCH 0/2] Follow up patches for the storage fix.

2013-08-20 Thread Osier Yang 
Osier Yang (2):
  storage: Fix coverity warning
  storage: Fix the use-after-free memory bug

 src/storage/storage_driver.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
1.8.1.4

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

[libvirt] [PATCH 2/2] storage: Fix the use-after-free memory bug

2013-08-20 Thread Osier Yang 
Introduced by commit e0139e30444. virStorageVolDefFree free'ed the
pointers that are still used by the added volume object, this changes
it back to VIR_FREE.
---
 src/storage/storage_driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 63a954b..883e4e9 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1618,7 +1618,7 @@ storageVolCreateXML(virStoragePoolPtr obj,
 cleanup:
 virObjectUnref(volobj);
 virStorageVolDefFree(voldef);
-virStorageVolDefFree(buildvoldef);
+VIR_FREE(buildvoldef);
 if (pool)
 virStoragePoolObjUnlock(pool);
 return ret;
-- 
1.8.1.4

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

  1   2   >