date:20120322

Re: [libvirt] [test-API PATCH 7/7] domain/[start|destroy]: Add a optional noping flag to skip the ping test

2012-03-22 Thread Martin Kletzander

On 03/21/2012 06:13 PM, Guannan Ren wrote:
 On 03/21/2012 08:46 PM, Peter Krempa wrote:
 For some tests it's not needed to ping the guest in the startup process.
 This patch adds a flag to the start and destroy test to skip such
 attempts (that consume a lot of time)
 ---
   repos/domain/destroy.py |   54
 ++
   repos/domain/start.py   |   50
 --
   2 files changed, 54 insertions(+), 50 deletions(-)

 diff --git a/repos/domain/destroy.py b/repos/domain/destroy.py
 index f98b602..12399d6 100644
 --- a/repos/domain/destroy.py
 +++ b/repos/domain/destroy.py
 @@ -50,7 +50,10 @@ def destroy(params):
  {'guestname': guestname}

  logger -- an object of utils/Python/log.py
 -   guestname -- same as the domain name
 +   guestname -- the domain name
 +   flags -- optional arguments:
 +  noping: Don't do the ping test
 +

  Return 0 on SUCCESS or 1 on FAILURE
   
 @@ -62,6 +65,7 @@ def destroy(params):
   if params_check_result:
   return 1
   guestname = params['guestname']
 +flags = params['flags']
 
   The 'flags' is optional, then we have to check if the
 dictionary of params has key or not
   if params.has_key('flags'):
   ...
   otherwise, it will report KeyError: 
 
   If 'flags' is mandatory,  it'd better to to check it in
 check_params function.
 

I'd rather do it using the get() method for dictionaries with some
default, i.e. params.get('flags', None).

Just my $0.02
Martin

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

Re: [libvirt] [PATCH 06/14] Store parsed query parameters directly in the virURIPtr struct

2012-03-22 Thread Osier Yang


On 2012年03月21日 01:33, Daniel P. Berrange wrote:

From: Daniel P. Berrangeberra...@redhat.com

Avoid the need for each driver to parse query parameters itself
by storing them directly in the virURIPtr struct. The parsing
code is a copy of that from src/util/qparams.c  The latter will
be removed in a later patch

* src/util/viruri.h: Add query params to virURIPtr
* src/util/viruri.c: Parse query parameters when creating virURIPtr
* tests/viruritest.c: Expand test to cover params
---
  src/libvirt_private.syms |1 +
  src/util/viruri.c|  139 ++
  src/util/viruri.h|   15 +
  tests/viruritest.c   |   46 +---
  4 files changed, 193 insertions(+), 8 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 7cd6a96..49fb2ee 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1471,6 +1471,7 @@ virTypedParameterAssign;
  # viruri.h
  virURIFree;
  virURIFormat;
+virURIFormatQuery;
  virURIParse;


diff --git a/src/util/viruri.c b/src/util/viruri.c
index d8618d1..f5adca5 100644
--- a/src/util/viruri.c
+++ b/src/util/viruri.c
@@ -13,6 +13,7 @@
  #include memory.h
  #include util.h
  #include virterror_internal.h
+#include buf.h

  #define VIR_FROM_THIS VIR_FROM_URI

@@ -21,6 +22,117 @@
   __FUNCTION__, __LINE__, __VA_ARGS__)


+static int
+virURIParamAppend(virURIPtr uri,
+  const char *name,
+  const char *value)
+{
+char *pname = NULL;
+char *pvalue = NULL;
+
+if (!(pname = strdup(name)))
+goto no_memory;
+if (!(pvalue = strdup (value)))
+goto no_memory;
+
+if (VIR_RESIZE_N(uri-params, uri-paramsAlloc, uri-paramsCount, 1)  0)
+goto no_memory;
+
+uri-params[uri-paramsCount].name = pname;
+uri-params[uri-paramsCount].value = pvalue;
+uri-params[uri-paramsCount].ignore = 0;
+uri-paramsCount++;
+
+return 0;
+
+no_memory:
+VIR_FREE(pname);
+VIR_FREE(pvalue);
+virReportOOMError();
+return -1;
+}
+
+
+static int
+virURIParseParams(virURIPtr uri)
+{
+const char *end, *eq;
+const char *query = uri-query;
+
+if (!query || query[0] == '\0')
+return 0;
+
+while (*query) {
+char *name = NULL, *value = NULL;
+
+/* Find the next separator, or end of the string. */
+end = strchr (query, '');
+if (!end)
+end = strchr (query, ';');
+if (!end)
+end = query + strlen (query);
+
+/* Find the first '=' character between here and end. */
+eq = strchr (query, '=');
+if (eq  eq= end) eq = NULL;
+
+/* Empty section (eg. ). */
+if (end == query)
+goto next;
+
+/* If there is no '=' character, then we have just name
+ * and consistent with CGI.pm we assume value is .
+ */
+else if (!eq) {
+name = xmlURIUnescapeString (query, end - query, NULL);
+if (!name) goto no_memory;
+}
+/* Or if we have name= here (works around annoying
+ * problem when calling xmlURIUnescapeString with len = 0).
+ */
+else if (eq+1 == end) {
+name = xmlURIUnescapeString (query, eq - query, NULL);
+if (!name) goto no_memory;
+}
+/* If the '=' character is at the beginning then we have
+ * =value and consistent with CGI.pm we _ignore_ this.
+ */
+else if (query == eq)
+goto next;
+
+/* Otherwise it's name=value. */
+else {
+name = xmlURIUnescapeString (query, eq - query, NULL);
+if (!name)
+goto no_memory;
+value = xmlURIUnescapeString (eq+1, end - (eq+1), NULL);
+if (!value) {
+VIR_FREE(name);
+goto no_memory;
+}
+}
+
+/* Append to the parameter set. */
+if (virURIParamAppend(uri, name, value ? value : )  0) {
+VIR_FREE(name);
+VIR_FREE(value);
+goto no_memory;
+}
+VIR_FREE(name);
+VIR_FREE(value);
+
+next:
+query = end;
+if (*query) query ++; /* skip '' separator */
+}
+
+return 0;
+
+ no_memory:
+virReportOOMError();
+return -1;
+}
+
  /**
   * virURIParse:
   * @uri: URI to parse
@@ -92,12 +204,16 @@ virURIParse(const char *uri)
   * the uri with xmlFreeURI() */
  }

+if (virURIParseParams(ret)  0)
+goto error;
+
  xmlFreeURI(xmluri);

  return ret;

  no_memory:
  virReportOOMError();
+error:
  xmlFreeURI(xmluri);
  virURIFree(ret);
  return NULL;
@@ -153,6 +269,29 @@ cleanup:
  }


+char *virURIFormatQuery(virURIPtr uri)


Should we name the function as virURIFormatParams instead? Per
params is used in the context everywhere, and virURIParseParams.


+{
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+int i,

Re: [libvirt] [PATCH 06/14] Store parsed query parameters directly in the virURIPtr struct

2012-03-22 Thread Osier Yang


On 2012年03月21日 01:33, Daniel P. Berrange wrote:

From: Daniel P. Berrangeberra...@redhat.com

Avoid the need for each driver to parse query parameters itself
by storing them directly in the virURIPtr struct. The parsing
code is a copy of that from src/util/qparams.c  The latter will
be removed in a later patch

* src/util/viruri.h: Add query params to virURIPtr
* src/util/viruri.c: Parse query parameters when creating virURIPtr
* tests/viruritest.c: Expand test to cover params
---
  src/libvirt_private.syms |1 +
  src/util/viruri.c|  139 ++
  src/util/viruri.h|   15 +
  tests/viruritest.c   |   46 +---
  4 files changed, 193 insertions(+), 8 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 7cd6a96..49fb2ee 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1471,6 +1471,7 @@ virTypedParameterAssign;
  # viruri.h
  virURIFree;
  virURIFormat;
+virURIFormatQuery;
  virURIParse;


diff --git a/src/util/viruri.c b/src/util/viruri.c
index d8618d1..f5adca5 100644
--- a/src/util/viruri.c
+++ b/src/util/viruri.c
@@ -13,6 +13,7 @@
  #include memory.h
  #include util.h
  #include virterror_internal.h
+#include buf.h

  #define VIR_FROM_THIS VIR_FROM_URI

@@ -21,6 +22,117 @@
   __FUNCTION__, __LINE__, __VA_ARGS__)


+static int
+virURIParamAppend(virURIPtr uri,
+  const char *name,
+  const char *value)
+{
+char *pname = NULL;
+char *pvalue = NULL;
+
+if (!(pname = strdup(name)))
+goto no_memory;
+if (!(pvalue = strdup (value)))
+goto no_memory;
+
+if (VIR_RESIZE_N(uri-params, uri-paramsAlloc, uri-paramsCount, 1)  0)
+goto no_memory;
+
+uri-params[uri-paramsCount].name = pname;
+uri-params[uri-paramsCount].value = pvalue;
+uri-params[uri-paramsCount].ignore = 0;
+uri-paramsCount++;
+
+return 0;
+
+no_memory:
+VIR_FREE(pname);
+VIR_FREE(pvalue);
+virReportOOMError();
+return -1;
+}
+
+
+static int
+virURIParseParams(virURIPtr uri)
+{
+const char *end, *eq;
+const char *query = uri-query;
+
+if (!query || query[0] == '\0')
+return 0;
+
+while (*query) {
+char *name = NULL, *value = NULL;
+
+/* Find the next separator, or end of the string. */
+end = strchr (query, '');
+if (!end)
+end = strchr (query, ';');
+if (!end)
+end = query + strlen (query);
+
+/* Find the first '=' character between here and end. */
+eq = strchr (query, '=');
+if (eq  eq= end) eq = NULL;
+
+/* Empty section (eg. ). */
+if (end == query)
+goto next;
+
+/* If there is no '=' character, then we have just name
+ * and consistent with CGI.pm we assume value is .
+ */
+else if (!eq) {
+name = xmlURIUnescapeString (query, end - query, NULL);
+if (!name) goto no_memory;
+}
+/* Or if we have name= here (works around annoying
+ * problem when calling xmlURIUnescapeString with len = 0).
+ */
+else if (eq+1 == end) {
+name = xmlURIUnescapeString (query, eq - query, NULL);
+if (!name) goto no_memory;
+}
+/* If the '=' character is at the beginning then we have
+ * =value and consistent with CGI.pm we _ignore_ this.
+ */
+else if (query == eq)
+goto next;
+
+/* Otherwise it's name=value. */
+else {
+name = xmlURIUnescapeString (query, eq - query, NULL);
+if (!name)
+goto no_memory;
+value = xmlURIUnescapeString (eq+1, end - (eq+1), NULL);
+if (!value) {
+VIR_FREE(name);
+goto no_memory;
+}
+}
+
+/* Append to the parameter set. */
+if (virURIParamAppend(uri, name, value ? value : )  0) {
+VIR_FREE(name);
+VIR_FREE(value);
+goto no_memory;
+}
+VIR_FREE(name);
+VIR_FREE(value);
+
+next:
+query = end;
+if (*query) query ++; /* skip '' separator */
+}
+
+return 0;
+
+ no_memory:
+virReportOOMError();
+return -1;
+}
+
  /**
   * virURIParse:
   * @uri: URI to parse
@@ -92,12 +204,16 @@ virURIParse(const char *uri)
   * the uri with xmlFreeURI() */
  }

+if (virURIParseParams(ret)  0)
+goto error;
+
  xmlFreeURI(xmluri);

  return ret;

  no_memory:
  virReportOOMError();
+error:
  xmlFreeURI(xmluri);
  virURIFree(ret);
  return NULL;
@@ -153,6 +269,29 @@ cleanup:
  }


+char *virURIFormatQuery(virURIPtr uri)
+{
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+int i, amp = 0;
+
+for (i = 0; i  uri-paramsCount; ++i) {
+if (!uri-params[i].ignore) {
+if (amp) virBufferAddChar

Re: [libvirt] [PATCH 07/14] Convert drivers over to use virURIPtr for query params

2012-03-22 Thread Osier Yang


On 2012年03月21日 01:33, Daniel P. Berrange wrote:

From: Daniel P. Berrangeberra...@redhat.com

Convert drivers currently using the qparams APIs, to instead
use the virURIPtr query parameters directly.

* src/esx/esx_util.c, src/hyperv/hyperv_util.c,
   src/remote/remote_driver.c, src/xenapi/xenapi_utils.c: Remove
   use of qparams
* src/util/qparams.h, src/util/qparams.c: Delete
* src/Makefile.am, src/libvirt_private.syms: Remove qparams
---
  src/Makefile.am|1 -
  src/esx/esx_util.c |   17 +---
  src/hyperv/hyperv_util.c   |   17 +---
  src/libvirt_private.syms   |6 -
  src/remote/remote_driver.c |   17 +---
  src/util/qparams.c |  265 
  src/util/qparams.h |   58 --
  src/xenapi/xenapi_utils.c  |   20 +---
  8 files changed, 9 insertions(+), 392 deletions(-)
  delete mode 100644 src/util/qparams.c
  delete mode 100644 src/util/qparams.h

diff --git a/src/Makefile.am b/src/Makefile.am
index e57eca2..39076cc 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -70,7 +70,6 @@ UTIL_SOURCES =
\
util/pci.c util/pci.h   \
util/processinfo.c util/processinfo.h   \
util/hostusb.c util/hostusb.h   \
-   util/qparams.c util/qparams.h   \
util/sexpr.c util/sexpr.h   \
util/stats_linux.c util/stats_linux.h   \
util/storage_file.c util/storage_file.h \
diff --git a/src/esx/esx_util.c b/src/esx/esx_util.c
index 67b07b7..a08ca19 100644
--- a/src/esx/esx_util.c
+++ b/src/esx/esx_util.c
@@ -28,7 +28,6 @@

  #include internal.h
  #include datatypes.h
-#include qparams.h
  #include util.h
  #include memory.h
  #include logging.h
@@ -45,8 +44,6 @@ int
  esxUtil_ParseUri(esxUtil_ParsedUri **parsedUri, virURIPtr uri)
  {
  int result = -1;
-struct qparam_set *queryParamSet = NULL;
-struct qparam *queryParam = NULL;
  int i;
  int noVerify;
  int autoAnswer;
@@ -62,14 +59,8 @@ esxUtil_ParseUri(esxUtil_ParsedUri **parsedUri, virURIPtr 
uri)
  return -1;
  }

-queryParamSet = qparam_query_parse(uri-query);
-
-if (queryParamSet == NULL) {
-goto cleanup;
-}
-
-for (i = 0; i  queryParamSet-n; i++) {
-queryParam =queryParamSet-p[i];
+for (i = 0; i  uri-paramsCount; i++) {
+virURIParamPtr queryParam =uri-params[i];

  if (STRCASEEQ(queryParam-name, transport)) {
  VIR_FREE((*parsedUri)-transport);
@@ -204,10 +195,6 @@ esxUtil_ParseUri(esxUtil_ParsedUri **parsedUri, virURIPtr 
uri)
  esxUtil_FreeParsedUri(parsedUri);
  }

-if (queryParamSet != NULL) {
-free_qparam_set(queryParamSet);
-}
-
  return result;
  }

diff --git a/src/hyperv/hyperv_util.c b/src/hyperv/hyperv_util.c
index 63c761b..81c087e 100644
--- a/src/hyperv/hyperv_util.c
+++ b/src/hyperv/hyperv_util.c
@@ -24,7 +24,6 @@

  #include internal.h
  #include datatypes.h
-#include qparams.h
  #include util.h
  #include memory.h
  #include logging.h
@@ -40,8 +39,6 @@ int
  hypervParseUri(hypervParsedUri **parsedUri, virURIPtr uri)
  {
  int result = -1;
-struct qparam_set *queryParamSet = NULL;
-struct qparam *queryParam = NULL;
  int i;

  if (parsedUri == NULL || *parsedUri != NULL) {
@@ -54,14 +51,8 @@ hypervParseUri(hypervParsedUri **parsedUri, virURIPtr uri)
  return -1;
  }

-queryParamSet = qparam_query_parse(uri-query);
-
-if (queryParamSet == NULL) {
-goto cleanup;
-}
-
-for (i = 0; i  queryParamSet-n; i++) {
-queryParam =queryParamSet-p[i];
+for (i = 0; i  uri-paramsCount; i++) {
+virURIParamPtr queryParam =uri-params[i];

  if (STRCASEEQ(queryParam-name, transport)) {
  VIR_FREE((*parsedUri)-transport);
@@ -103,10 +94,6 @@ hypervParseUri(hypervParsedUri **parsedUri, virURIPtr uri)
  hypervFreeParsedUri(parsedUri);
  }

-if (queryParamSet != NULL) {
-free_qparam_set(queryParamSet);
-}
-
  return result;
  }

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 49fb2ee..8a14838 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -917,12 +917,6 @@ virProcessInfoGetAffinity;
  virProcessInfoSetAffinity;


-# qparams.h
-free_qparam_set;
-qparam_get_query;
-qparam_query_parse;
-
-
  # secret_conf.h
  virSecretDefFormat;
  virSecretDefFree;
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 9de966f..bc6fea2 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -35,7 +35,6 @@
  #include domain_event.h
  #include driver.h
  #include buf.h
-#include qparams.h
  #include remote_driver.h
  #include remote_protocol.h
  #include qemu_protocol.h
@@ -311,7 +310,6 @@ doRemoteOpen (virConnectPtr

Re: [libvirt] [test-API PATCH 0/7] Multiple fixes and improvements series

2012-03-22 Thread Guannan Ren


On 03/21/2012 08:46 PM, Peter Krempa wrote:

This is a set of more or less independent fixes and improvements to the
test API. I ran across these while trying to write a basic test case as
a Hello world! to the test-API.

Improvements are in fields of cross-distro compatibility, broken API's and
typos and usability.

Peter Krempa (7):
   utils: Make ipget.sh more portable
   lib: fix streamAPI class
   domainAPI: Add wrapper method to work with domain's console
 connections
   repos/domain/create.py: Fix typo in path to python executable
   utils: Allow suffixes in path to the main checkout folder
   parser: Be more specific on mistakes in case files
   domain/[start|destroy]: Add a optional noping flag to skip the ping
 test

  exception.py|   10 +
  lib/domainAPI.py|9 
  lib/streamAPI.py|   52 +++---
  proxy.py|4 +++
  repos/domain/create.py  |2 +-
  repos/domain/destroy.py |   50 +---
  repos/domain/start.py   |   50 +---
  utils/Python/utils.py   |2 +-
  utils/ipget.sh  |4 +-
  9 files changed, 105 insertions(+), 78 deletions(-)



Hi Peter

   I have pushed the set of patches after some small modifications 
based on my comments.

   If you have any comment, please be free to tell me.
   Thanks your patches.

Guannan Ren

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

Re: [libvirt] [test-API PATCH 7/7] domain/[start|destroy]: Add a optional noping flag to skip the ping test

2012-03-22 Thread Guannan Ren


On 03/22/2012 03:02 PM, Martin Kletzander wrote:


I'd rather do it using the get() method for dictionaries with some
default, i.e. params.get('flags', None).

Just my $0.02
Martin


Thanks,  This belongs to enhancement work, let us do it a little later.
Probably, we need a cleanup patch for these.

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

Re: [libvirt] [PATCH 09/14] Rename src/util/authhelper.[ch] to src/util/virauth.[ch]

2012-03-22 Thread Osier Yang


On 2012年03月21日 01:33, Daniel P. Berrange wrote:

From: Daniel P. Berrangeberra...@redhat.com

To follow latest naming conventions, rename src/util/authhelper.[ch]
to src/util/virauth.[ch].

* src/util/authhelper.[ch]: Rename to src/util/virauth.[ch]
* src/esx/esx_driver.c, src/hyperv/hyperv_driver.c,
   src/phyp/phyp_driver.c, src/xenapi/xenapi_driver.c: Update
   for renamed include files
---
  po/POTFILES.in   |2 +-
  src/Makefile.am  |2 +-
  src/esx/esx_driver.c |2 +-
  src/hyperv/hyperv_driver.c   |2 +-
  src/phyp/phyp_driver.c   |2 +-
  src/util/{authhelper.c =  virauth.c} |5 ++---
  src/util/{authhelper.h =  virauth.h} |9 -
  src/xenapi/xenapi_driver.c   |2 +-
  8 files changed, 12 insertions(+), 14 deletions(-)
  rename src/util/{authhelper.c =  virauth.c} (97%)
  rename src/util/{authhelper.h =  virauth.h} (87%)

diff --git a/po/POTFILES.in b/po/POTFILES.in
index 8354c09..8eaa8ad 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -106,7 +106,6 @@ src/storage/storage_driver.c
  src/test/test_driver.c
  src/uml/uml_conf.c
  src/uml/uml_driver.c
-src/util/authhelper.c
  src/util/cgroup.c
  src/util/command.c
  src/util/conf.c
@@ -124,6 +123,7 @@ src/util/stats_linux.c
  src/util/storage_file.c
  src/util/sysinfo.c
  src/util/util.c
+src/util/virauth.c
  src/util/virauditc


Better to put it after viraudit.c, but it doesn't hurt. ACK

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

Re: [libvirt] [PATCH 10/14] Rename virRequest{Username, Password} to virAuthGet{Username, Password}

2012-03-22 Thread Osier Yang


On 2012年03月21日 01:33, Daniel P. Berrange wrote:

From: Daniel P. Berrangeberra...@redhat.com

Ensure that the functions in virauth.h have names matching the file
prefix, by renaming  virRequest{Username,Password} to
virAuthGet{Username,Password}
---
  src/esx/esx_driver.c   |8 
  src/hyperv/hyperv_driver.c |4 ++--
  src/libvirt_private.syms   |5 +
  src/phyp/phyp_driver.c |4 ++--
  src/util/virauth.c |4 ++--
  src/util/virauth.h |4 ++--
  src/xenapi/xenapi_driver.c |4 ++--
  7 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
index 51bd5b2..7689306 100644
--- a/src/esx/esx_driver.c
+++ b/src/esx/esx_driver.c
@@ -700,7 +700,7 @@ esxConnectToHost(esxPrivate *priv, virConnectAuthPtr auth,
  goto cleanup;
  }
  } else {
-username = virRequestUsername(auth, root, hostname);
+username = virAuthGetUsername(auth, root, hostname);

  if (username == NULL) {
  ESX_ERROR(VIR_ERR_AUTH_FAILED, %s, _(Username request 
failed));
@@ -708,7 +708,7 @@ esxConnectToHost(esxPrivate *priv, virConnectAuthPtr auth,
  }
  }

-unescapedPassword = virRequestPassword(auth, username, hostname);
+unescapedPassword = virAuthGetPassword(auth, username, hostname);

  if (unescapedPassword == NULL) {
  ESX_ERROR(VIR_ERR_AUTH_FAILED, %s, _(Password request failed));
@@ -830,7 +830,7 @@ esxConnectToVCenter(esxPrivate *priv, virConnectAuthPtr 
auth,
  goto cleanup;
  }
  } else {
-username = virRequestUsername(auth, administrator, hostname);
+username = virAuthGetUsername(auth, administrator, hostname);

  if (username == NULL) {
  ESX_ERROR(VIR_ERR_AUTH_FAILED, %s, _(Username request 
failed));
@@ -838,7 +838,7 @@ esxConnectToVCenter(esxPrivate *priv, virConnectAuthPtr 
auth,
  }
  }

-unescapedPassword = virRequestPassword(auth, username, hostname);
+unescapedPassword = virAuthGetPassword(auth, username, hostname);

  if (unescapedPassword == NULL) {
  ESX_ERROR(VIR_ERR_AUTH_FAILED, %s, _(Password request failed));
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index 5ca20cf..0469e2e 100644
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -147,7 +147,7 @@ hypervOpen(virConnectPtr conn, virConnectAuthPtr auth, 
unsigned int flags)
  goto cleanup;
  }
  } else {
-username = virRequestUsername(auth, administrator, 
conn-uri-server);
+username = virAuthGetUsername(auth, administrator, 
conn-uri-server);

  if (username == NULL) {
  HYPERV_ERROR(VIR_ERR_AUTH_FAILED, %s, _(Username request 
failed));
@@ -155,7 +155,7 @@ hypervOpen(virConnectPtr conn, virConnectAuthPtr auth, 
unsigned int flags)
  }
  }

-password = virRequestPassword(auth, username, conn-uri-server);
+password = virAuthGetPassword(auth, username, conn-uri-server);

  if (password == NULL) {
  HYPERV_ERROR(VIR_ERR_AUTH_FAILED, %s, _(Password request failed));
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 3f69ec1..07302cd 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1165,6 +1165,11 @@ virUUIDGenerate;
  virUUIDParse;


+# virauth.h
+virAuthGetUsername;
+virAuthGetPassword;



virRequest{Username,Password} are forgoten to remove.

# authhelper.h
virRequestPassword;
virRequestUsername;

ACK with they are removed.

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

Re: [libvirt] [PATCH 11/14] Add helper API for finding auth file path

2012-03-22 Thread Osier Yang


On 2012年03月21日 01:33, Daniel P. Berrange wrote:

From: Daniel P. Berrangeberra...@redhat.com

* src/util/virauth.c, src/util/virauth.h: Add virAuthGetConfigFilePath
* include/libvirt/virterror.h, src/util/virterror.c: Add
   VIR_FROM_AUTH error domain
---
  include/libvirt/virterror.h |1 +
  src/libvirt_private.syms|1 +
  src/util/virauth.c  |   74 +++
  src/util/virauth.h  |3 ++
  src/util/virterror.c|3 ++
  5 files changed, 82 insertions(+), 0 deletions(-)

diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
index c8ec8d4..e04d29e 100644
--- a/include/libvirt/virterror.h
+++ b/include/libvirt/virterror.h
@@ -86,6 +86,7 @@ typedef enum {
  VIR_FROM_HYPERV = 43, /* Error from Hyper-V driver */
  VIR_FROM_CAPABILITIES = 44, /* Error from capabilities */
  VIR_FROM_URI = 45,  /* Error from URI handling */
+VIR_FROM_AUTH = 46, /* Error from auth handling */
  } virErrorDomain;


diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 07302cd..6f53aa8 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1168,6 +1168,7 @@ virUUIDParse;
  # virauth.h
  virAuthGetUsername;
  virAuthGetPassword;
+virAuthGetConfigFilePath;


virAuthGetConfigFilePath;
virAuthGetPassword;
virAuthGetUsername;




  # viraudit.h
diff --git a/src/util/virauth.c b/src/util/virauth.c
index d7375e9..150b8e7 100644
--- a/src/util/virauth.c
+++ b/src/util/virauth.c
@@ -21,9 +21,83 @@

  #includeconfig.h

+#includestdlib.h
+
  #include virauth.h
  #include util.h
  #include memory.h
+#include logging.h
+#include datatypes.h
+#include virterror_internal.h
+#include configmake.h
+
+#define VIR_FROM_THIS VIR_FROM_AUTH
+
+
+int virAuthGetConfigFilePath(virConnectPtr conn,
+ char **path)
+{
+int ret = -1;
+size_t i;
+const char *authenv = getenv(LIBVIRT_AUTH_FILE);
+char *userdir = NULL;
+
+*path = NULL;
+
+VIR_DEBUG(Determining auth config file path);
+
+if (authenv) {
+VIR_DEBUG(Using path from env '%s', authenv);
+if (!(*path = strdup(authenv)))
+goto no_memory;
+return 0;
+}
+
+for (i = 0 ; i  conn-uri-paramsCount ; i++) {
+if (STREQ_NULLABLE(conn-uri-params[i].name, authfile)
+conn-uri-params[i].value) {
+VIR_DEBUG(Using path from URI '%s',
+  conn-uri-params[i].value);
+if (!(*path = strdup(conn-uri-params[i].value)))
+goto no_memory;
+return 0;
+}
+}
+
+if (!(userdir = virGetUserDirectory(geteuid(
+goto cleanup;
+
+if (virAsprintf(path, %s/.libvirt/auth.conf, userdir)  0)
+goto no_memory;
+
+VIR_DEBUG(Checking for readability of '%s', *path);
+if (access(*path, R_OK) == 0)
+goto done;
+
+VIR_FREE(*path);
+
+if (!(*path = strdup(SYSCONFDIR /libvirt/auth.conf)))
+goto no_memory;
+
+VIR_DEBUG(Checking for readability of '%s', *path);
+if (access(*path, R_OK) == 0)
+goto done;
+
+VIR_FREE(*path);


*path will be NULL if the last choice of authfile (e.g.
/etc/libvirt/auth.conf) is not readable, while the function
will return 0.

ACK with this fixed.

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

Re: [libvirt] [PATCH 12/14] Add APIs for handling lookup of auth credentials from config file

2012-03-22 Thread Osier Yang


On 2012年03月21日 01:33, Daniel P. Berrange wrote:

From: Daniel P. Berrangeberra...@redhat.com

This defines the format for the auth credential config file and
provides APIs to access the data. The config file contains
one or more named 'credential' sets

   [credentials-$NAME]
   credname1=value1
   credname2=value2

eg

   [credentials-test]
   authname=fred
   password=123456

   [credentials-prod]
   authname=bar
   password=letmein

There are then one or more 'auth' sets which match services/hosts
and map to credential sets.

   [auth-$SERVICE-$HOSTNAME]
   credentials=$CREDENTIALS

eg

   [auth-libvirt-test1.example.com]
   credentials=test

   [auth-libvirt-test2.example.com]
   credentials=test

   [auth-libvirt-demo3.example.com]
   credentials=test

   [auth-libvirt-prod1.example.com]
   credentials=prod

* docs/auth.html.in: Document use of client auth config files
* src/Makefile.am, src/libvirt_private.syms,
   src/util/virauthconfig.c, src/util/virauthconfig.h: Add
   APIs for processing auth.conf file
---
  docs/auth.html.in |  118 ++-
  po/POTFILES.in|1 +
  src/Makefile.am   |1 +
  src/libvirt_private.syms  |7 ++
  src/util/virauthconfig.c  |  175 +
  src/util/virauthconfig.h  |   45 
  tests/Makefile.am |9 ++-
  tests/virauthconfigtest.c |  140 
  8 files changed, 494 insertions(+), 2 deletions(-)
  create mode 100644 src/util/virauthconfig.c
  create mode 100644 src/util/virauthconfig.h
  create mode 100644 tests/virauthconfigtest.c

diff --git a/docs/auth.html.in b/docs/auth.html.in
index 2163959..ecff0fc 100644
--- a/docs/auth.html.in
+++ b/docs/auth.html.in
@@ -1,7 +1,7 @@
  ?xml version=1.0?
  html
body
-h1Access control/h1
+h1Authenticationamp; access control/h1
  p
When connecting to libvirt, some connections may require client
authentication before allowing use of the APIs. The set of possible
@@ -11,6 +11,122 @@

  ul id=toc/ul

+h2a name=Auth_client_configClient configuration/a/h2
+
+p
+  When connecting to a remote hypervisor which requires authentication,
+most libvirt applications will prompt the user for the credentials. It is
+also possible to provide a client configuration file containing all the
+authentication credentials, avoiding any interaction. Libvirt will look
+for the authentication file using the following sequence:
+/p
+ol
+liThe file path specified by the $LIBVIRT_AUTH_FILE environment
+variable./li
+liThe file path specified by the authfile=/some/file URI
+query parameter/li
+liThe file $HOME/.libvirt/auth.conf/li
+liThe file /etc/libvirt/auth.conf/li
+/ol
+
+p
+  The auth configuration file uses the traditionalcode.ini/code
+  style syntax. There are two types of groups that can be present in
+  the config. First there are one or morestrongcredential/strong
+  sets, which provide the actual authentication credentials. The keys
+  within the group may be:
+/p
+
+ul
+licodeusername/code: the user login name to act as. This
+is relevant for ESX, Xen, HyperV and SSH, but probably not
+the one you want to libvirtd with SASL./li
+licodeauthname/code: the name to authorize as. This is
+what is commonly required for libvirtd with SASL./li
+licodepassword/code: the secret password/li
+licoderealm/code: the domain realm for SASL, mostly
+unused/li
+/ul
+
+p
+  Each set of credentials has a name, which is part of the group
+  entry name. Overall the syntax is
+/p
+
+pre
+[credentials-$NAME]
+credname1=value1
+credname2=value2/pre
+
+p
+  For example, to define two sets of credentials used for production
+  and test machines, using libvirtd, and a further ESX server for dev:
+/p
+pre
+[credentials-test]
+authname=fred
+password=123456
+
+[credentials-prod]
+authname=bar
+password=letmein
+
+[credentials-dev]
+username=joe
+password=hello/pre
+
+p
+  The second set of groups provide mappings of credentials to
+  specific machine services. The config file group names compromise
+  the service type and host:
+/p
+
+pre
+[auth-$SERVICE-$HOSTNAME]
+credentials=$CREDENTIALS/pre
+
+p
+  For example, following the previous example, here is how to
+  list some machines
+/p
+
+pre
+[auth-libvirt-test1.example.com]
+credentials=test
+
+[auth-libvirt-test2.example.com]
+credentials=test
+
+[auth-libvirt-demo3.example.com]
+credentials=test
+
+[auth-libvirt-prod1.example.com]
+credentials=prod
+
+[auth-esx-dev1.example.com]
+credentials=dev/pre
+
+p
+  The following service types are known to libvirt
+/p
+
+ol
+licodelibvirt/code  - used for connections to a libvirtd
+server, which is configured with SASL auth/li
+licodessh/code  - used for connections to a Phyp server
+over SSH/li
+licodeesx/code  - used for connections to an ESX or
+VirtualCenter

Re: [libvirt] [test-API PATCH 2/7] lib: fix streamAPI class

2012-03-22 Thread Peter Krempa


On 03/21/2012 04:45 PM, Guannan Ren wrote:

On 03/21/2012 08:46 PM, Peter Krempa wrote:

The streamAPI class that encapsulates work with libvirt's streams was
fundamentaly broken:
- each call to one of the methods created a new stream and
performed the call
- some methods called virStream methods with different numbers
of arguments
- there was no way to extract the actual libvirt stream object

thanks for the patch.


---
lib/streamAPI.py | 52
++--
1 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/lib/streamAPI.py b/lib/streamAPI.py
index bc7d217..69e183e 100644
--- a/lib/streamAPI.py
+++ b/lib/streamAPI.py
@@ -38,76 +38,76 @@ append_path(result.group(0))
import exception


- def eventAddCallback(self, flag = 0, cb, opaque):
+ def eventAddCallback(self, cb, opaque):


The API need an argument event to pass in which is virEventHandleType
VIR_EVENT_HANDLE_READABLE = 1
VIR_EVENT_HANDLE_WRITABLE = 2
VIR_EVENT_HANDLE_ERROR = 4
VIR_EVENT_HANDLE_HANGUP = 8

We could just overdefine it in streamAPI.py which make use of them
easier for
writing testcase.
It's not a good idea to import libvirt.py directly in testcases.


Oh, in this case I'll need to redefine the virConsoleFlags enum in the 
domainAPI too as I'm importing libvirt.py in my (not yet published ) 
test that deals with consoles.


Peter

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

Re: [libvirt] [PATCH 13/14] Refactor code prompting for SASL credentials

2012-03-22 Thread Osier Yang


On 2012年03月21日 01:33, Daniel P. Berrange wrote:

From: Daniel P. Berrangeberra...@redhat.com

SASL may prompt for credentials after either a 'start' or 'step'
invocation. In both cases the code to handle this is the same.
Refactor this code into a separate method to reduce the duplication,
since the complexity is about to grow

* src/remote/remote_driver.c: Refactor interaction with SASL
---
  src/remote/remote_driver.c |  135 +--
  1 files changed, 78 insertions(+), 57 deletions(-)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index bc6fea2..1faaf9e 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -2863,7 +2863,8 @@ static sasl_callback_t *remoteAuthMakeCallbacks(int 
*credtype, int ncredtype)
   * are basically a 1-to-1 copy of each other.
   */
  static int remoteAuthMakeCredentials(sasl_interact_t *interact,
- virConnectCredentialPtr *cred)
+ virConnectCredentialPtr *cred,
+ size_t *ncred)
  {
  int ninteract;
  if (!cred)
@@ -2889,16 +2890,8 @@ static int remoteAuthMakeCredentials(sasl_interact_t 
*interact,
  (*cred)[ninteract].result = NULL;
  }

-return ninteract;
-}
-
-static void remoteAuthFreeCredentials(virConnectCredentialPtr cred,
-  int ncred)
-{
-int i;
-for (i = 0 ; i  ncred ; i++)
-VIR_FREE(cred[i].result);
-VIR_FREE(cred);
+*ncred = ninteract;
+return 0;


Better to change the comments for remoteAuthFreeCredentials.

ACK.

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

Re: [libvirt] [test-API PATCH 3/7] domainAPI: Add wrapper method to work with domain's console connections

2012-03-22 Thread Peter Krempa


On 03/21/2012 05:02 PM, Guannan Ren wrote:

On 03/21/2012 08:46 PM, Peter Krempa wrote:

This patch adds a wrapper that enables work with consoles in the
test-API.
---
lib/domainAPI.py | 9 +
1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/lib/domainAPI.py b/lib/domainAPI.py
index 91f2ba3..bc0069b 100644
--- a/lib/domainAPI.py
+++ b/lib/domainAPI.py
@@ -878,6 +878,15 @@ class DomainAPI(object):
code = e.get_error_code()
raise exception.LibvirtAPI(message, code)

+ def openConsole(self, domname, device, stream, flags = 0):
+ try:
+ dom_obj = self.get_domain_by_name(domname)
+ st_obj = stream.getStream()



we could get the st_obj in testcase then pass it into openConsole
If so, st_obj could be reused in the following statement in testcase
like:

st_obj = stream.getStream()
dom.openConsole(domname, None, st_obj)
receivedData = st_obj.recv(1024)
(the last statement probably resides in a registered stream callback
function via eventAddCallback API)


Wouldn't this actualy defeat the encapsulation that is provided in the 
StreamAPI class? With this change you can reuse the StreamAPI object for 
other actions later on too, without loosing the abstraction.


With the chagne you pushed:
diff --git a/lib/domainAPI.py b/lib/domainAPI.py
index 91f2ba3..ddcdc91 100644
--- a/lib/domainAPI.py
+++ b/lib/domainAPI.py
@@ -878,6 +878,14 @@ class DomainAPI(object):
 code = e.get_error_code()
 raise exception.LibvirtAPI(message, code)

+def openConsole(self, domname, device, st_obj, flags = 0):
+try:
+dom_obj = self.get_domain_by_name(domname)
+return dom_obj.openConsole(device, st_obj, flags)
+except libvirt.libvirtError, e:
+message = e.get_error_message()
+code = e.get_error_code()
+raise exception.LibvirtAPI(message, code)

You have to create a new StreamAPI object with DomainAPI.newStream() and 
then you have to extract the virStream object from that to pass it 
manualy to DomainAPI.openConsole(). IMO this will encourage to use the 
virStream object directly without the encapsulation that is provided by 
the StreamAPI class.


Peter





Thanks for the patch.

Guannan Ren

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

Re: [libvirt] [PATCH 14/14] Lookup auth credentials in config file before prompting

2012-03-22 Thread Osier Yang


On 2012年03月21日 01:33, Daniel P. Berrange wrote:

From: Daniel P. Berrangeberra...@redhat.com

When SASL requests auth credentials, try to look them up in the
config file first. If any are found, remove them from the list
that the user is prompted for
---
  src/esx/esx_driver.c   |   58 +-
  src/hyperv/hyperv_driver.c |4 +-
  src/phyp/phyp_driver.c |4 +-
  src/remote/remote_driver.c |  138 
  src/util/virauth.c |   69 +-
  src/util/virauth.h |   10 +++-
  src/xenapi/xenapi_driver.c |4 +-
  7 files changed, 224 insertions(+), 63 deletions(-)

diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
index 7689306..6962832 100644
--- a/src/esx/esx_driver.c
+++ b/src/esx/esx_driver.c
@@ -667,10 +667,8 @@ esxCapsInit(esxPrivate *priv)


  static int
-esxConnectToHost(esxPrivate *priv, virConnectAuthPtr auth,
- const char *hostname, int port,
- const char *predefinedUsername,
- esxVI_ProductVersion expectedProductVersion,
+esxConnectToHost(virConnectPtr conn,
+ virConnectAuthPtr auth,
   char **vCenterIpAddress)
  {
  int result = -1;
@@ -682,25 +680,29 @@ esxConnectToHost(esxPrivate *priv, virConnectAuthPtr auth,
  esxVI_String *propertyNameList = NULL;
  esxVI_ObjectContent *hostSystem = NULL;
  esxVI_Boolean inMaintenanceMode = esxVI_Boolean_Undefined;
+esxPrivate *priv = conn-privateData;
+esxVI_ProductVersion expectedProductVersion = STRCASEEQ(conn-uri-scheme, 
esx)
+? esxVI_ProductVersion_ESX
+: esxVI_ProductVersion_GSX;

  if (vCenterIpAddress == NULL || *vCenterIpAddress != NULL) {
  ESX_VI_ERROR(VIR_ERR_INTERNAL_ERROR, %s, _(Invalid argument));
  return -1;
  }

-if (esxUtil_ResolveHostname(hostname, ipAddress, NI_MAXHOST)  0) {
+if (esxUtil_ResolveHostname(conn-uri-server, ipAddress, NI_MAXHOST)  0) 
{
  return -1;
  }

-if (predefinedUsername != NULL) {
-username = strdup(predefinedUsername);
+if (conn-uri-user != NULL) {
+username = strdup(conn-uri-user);

  if (username == NULL) {
  virReportOOMError();
  goto cleanup;
  }
  } else {
-username = virAuthGetUsername(auth, root, hostname);
+username = virAuthGetUsername(conn, auth, esx, root, 
conn-uri-server);

  if (username == NULL) {
  ESX_ERROR(VIR_ERR_AUTH_FAILED, %s, _(Username request 
failed));
@@ -708,7 +710,7 @@ esxConnectToHost(esxPrivate *priv, virConnectAuthPtr auth,
  }
  }

-unescapedPassword = virAuthGetPassword(auth, username, hostname);
+unescapedPassword = virAuthGetPassword(conn, auth, esx, username, 
conn-uri-server);

  if (unescapedPassword == NULL) {
  ESX_ERROR(VIR_ERR_AUTH_FAILED, %s, _(Password request failed));
@@ -722,7 +724,7 @@ esxConnectToHost(esxPrivate *priv, virConnectAuthPtr auth,
  }

  if (virAsprintf(url, %s://%s:%d/sdk, priv-parsedUri-transport,
-hostname, port)  0) {
+conn-uri-server, conn-uri-port)  0) {
  virReportOOMError();
  goto cleanup;
  }
@@ -743,13 +745,13 @@ esxConnectToHost(esxPrivate *priv, virConnectAuthPtr auth,
  priv-host-productVersion != esxVI_ProductVersion_ESX5x) {
  ESX_ERROR(VIR_ERR_INTERNAL_ERROR,
_(%s is neither an ESX 3.5, 4.x nor 5.x host),
-  hostname);
+  conn-uri-server);
  goto cleanup;
  }
  } else { /* GSX */
  if (priv-host-productVersion != esxVI_ProductVersion_GSX20) {
  ESX_ERROR(VIR_ERR_INTERNAL_ERROR,
-  _(%s isn't a GSX 2.0 host), hostname);
+  _(%s isn't a GSX 2.0 host), conn-uri-server);
  goto cleanup;
  }
  }
@@ -799,9 +801,9 @@ esxConnectToHost(esxPrivate *priv, virConnectAuthPtr auth,


  static int
-esxConnectToVCenter(esxPrivate *priv, virConnectAuthPtr auth,
-const char *hostname, int port,
-const char *predefinedUsername,
+esxConnectToVCenter(virConnectPtr conn,
+virConnectAuthPtr auth,
+const char *hostname,
  const char *hostSystemIpAddress)
  {
  int result = -1;
@@ -810,6 +812,7 @@ esxConnectToVCenter(esxPrivate *priv, virConnectAuthPtr 
auth,
  char *unescapedPassword = NULL;
  char *password = NULL;
  char *url = NULL;
+esxPrivate *priv = conn-privateData;

  if (hostSystemIpAddress == NULL
  (priv-parsedUri-path == NULL || STREQ(priv-parsedUri-path, /))) 
{
@@ -822,15 +825,15 @@ esxConnectToVCenter(esxPrivate *priv, virConnectAuthPtr 
auth,
  return -1;
  }

-if (predefinedUsername != NULL) {
-username = strdup(predefinedUsername);
+

Re: [libvirt] [test-API PATCH 3/7] domainAPI: Add wrapper method to work with domain's console connections

2012-03-22 Thread Guannan Ren





Wouldn't this actualy defeat the encapsulation that is provided in the 
StreamAPI class? With this change you can reuse the StreamAPI object 
for other actions later on too, without loosing the abstraction.


With the chagne you pushed:
diff --git a/lib/domainAPI.py b/lib/domainAPI.py
index 91f2ba3..ddcdc91 100644
--- a/lib/domainAPI.py
+++ b/lib/domainAPI.py
@@ -878,6 +878,14 @@ class DomainAPI(object):
 code = e.get_error_code()
 raise exception.LibvirtAPI(message, code)

+def openConsole(self, domname, device, st_obj, flags = 0):
+try:
+dom_obj = self.get_domain_by_name(domname)
+return dom_obj.openConsole(device, st_obj, flags)
+except libvirt.libvirtError, e:
+message = e.get_error_message()
+code = e.get_error_code()
+raise exception.LibvirtAPI(message, code)

You have to create a new StreamAPI object with DomainAPI.newStream() 
and then you have to extract the virStream object from that to pass it 
manualy to DomainAPI.openConsole(). IMO this will encourage to use the 
virStream object directly without the encapsulation that is provided 
by the StreamAPI class.


Peter



   Yeath, you are right,  we can not cross the encapsulation line.
   We have to operate above the abstraction layer.
   Your patch is correct definitely.

   Guannan Ren

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

Re: [libvirt] [Qemu-devel] Modern CPU models cannot be used with libvirt

2012-03-22 Thread Gleb Natapov

On Tue, Mar 13, 2012 at 11:53:19AM -0300, Eduardo Habkost wrote:
 So, trying to summarize what was discussed in the call:
 
 On Mon, Mar 12, 2012 at 10:08:10AM -0300, Eduardo Habkost wrote:
   Let's say we moved CPU definitions to /usr/share/qemu/cpu-models.xml.
   
   Obviously, we'd want a command line option to be able to change that
   location so we'd introduce -cpu-models PATH.
   
   But we want all of our command line options to be settable by the
   global configuration file so we would have a cpu-model=PATH to the
   configuration file.
   
   But why hard code a path when we can just set the default path in the
   configuration file so let's avoid hard coding and just put
   cpu-models=/usr/share/qemu/cpu-models.xml in the default
   configuration file.
  
  We wouldn't do the above.
  
  -nodefconfig should disable the loading of files on /etc, but it
  shouldn't disable loading internal non-configurable data that we just
  happened to choose to store outside the qemu binary because it makes
  development easier.
 
 The statement above is the one not fulfilled by the compromise solution:
 -nodefconfig would really disable the loading of files on /usr/share.
 
What does this mean? Will -nodefconfig disable loading of bios.bin,
option roms, keymaps?

  
  Really, the requirement of a default configuration file is a problem
  by itself. Qemu should not require a default configuration file to work,
  and it shouldn't require users to copy the default configuration file to
  change options from the default.
 
 The statement above is only partly true. The default configuration file
 would be still needed, but if defaults are stored on /usr/share, I will
 be happy with it.
 
 My main problem was with the need to _copy_ or edit a non-trivial
 default config file. If the not-often-edited defaults/templates are
 easily found on /usr/share to be used with -readconfig, I will be happy
 with this solution, even if -nodefconfig disable the files on
 /usr/share.
 
  
  Doing this would make it impossible to deploy fixes to users if we evern
  find out that the default configuration file had a serious bug. What if
  a bug in our default configuration file has a serious security
  implication?
 
 The answer to this is: if the broken templates/defaults are on
 /usr/share, it would be easy to deploy the fix.
 
 So, the compromise solution is:
 
 - We can move some configuration data (especially defaults/templates)
   to /usr/share (machine-types and CPU models could go there). This
   way we can easily deploy fixes to the defaults, if necessary.
 - To reuse Qemu models, or machine-types, and not define everything from
   scratch, libvirt will have to use something like:
   -nodefconfig -readconfig /usr/share/qemu/cpu-models-x86.conf
 
cpu-models-x86.conf is not a configuration file. It is hardware
description file. QEMU should not lose capability just because you run
it with -nodefconfig. -nodefconfig means that QEMU does not create
machine for you, but all parts needed to create a machine that would have
been created without -nodefconfig are still present. Not been able to
create Nehalem CPU after specifying -nodefconfig is the same as not been
able to create virtio-net i.e the bug.

 
 (the item below is not something discussed on the call, just something I
 want to add)
 
 To make this work better, we can allow users (humans or machines) to
 extend CPU models on the config file, instead of having to define
 everything from scratch. So, on /etc (or on a libvirt-generated config)
 we could have something like:
 
 =
 [cpu]
 base_cpudef = Nehalem
 add_features = vmx
 =
 
 Then, as long as /usr/share/cpu-models-x86.conf is loaded, the user will
 be able to reuse the Nehalem CPU model provided by Qemu.
 
And if it will not be loaded?

  
   
   But now when libvirt uses -nodefconfig, those models go away.
   -nodefconfig means start QEMU in the most minimal state possible.
   You get what you pay for if you use it.
   
   We'll have the same problem with machine configuration files.  At
   some point in time, -nodefconfig will make machine models disappear.
  
  It shouldn't. Machine-types are defaults to be used as base, they are
  not user-provided configuration. And the fact that we decided to store
  some data outside of the Qemu binary is orthogonal the design decisions
  in the Qemu command-line and configuration interface.
 
 So, this problem is solved if the defaults are easily found on
 /usr/share.
 
What problem is solved and why are we mixing machine configuration files
and cpu configuration files? They are different and should be treated
differently. -nodefconfig exists only because there is not machine
configuration files currently. With machine configuration files
libvirt does not need -nodefconfig because it can create its own machine
file and make QEMU use it. So specifying machine file on QEMU's command
line implies -nodefconfig. The option itself loses its meaning and can

Re: [libvirt] [PATCH 08/14] Add a virKeyfilePtr object for parsing '.ini' files

2012-03-22 Thread Osier Yang


On 2012年03月21日 01:33, Daniel P. Berrange wrote:

From: Daniel P. Berrangeberra...@redhat.com

The '.ini' file format is a useful alternative to the existing
config file style, when you need to have config files which
are hashes of hashes. The 'virKeyFilePtr' object provides a
way to parse these file types.

* src/Makefile.am, src/util/virkeyfile.c,
   src/util/virkeyfile.h: Add .ini file parser
* tests/Makefile.am, tests/virkeyfiletest.c: Test
   basic parsing capabilities
---
  po/POTFILES.in   |1 +
  src/Makefile.am  |1 +
  src/libvirt_private.syms |   10 ++
  src/util/virkeyfile.c|  367 ++
  src/util/virkeyfile.h|   64 
  tests/Makefile.am|8 +-
  tests/virkeyfiletest.c   |  123 
  7 files changed, 573 insertions(+), 1 deletions(-)
  create mode 100644 src/util/virkeyfile.c
  create mode 100644 src/util/virkeyfile.h
  create mode 100644 tests/virkeyfiletest.c

diff --git a/po/POTFILES.in b/po/POTFILES.in
index 16a3f9e..8354c09 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -127,6 +127,7 @@ src/util/util.c
  src/util/viraudit.c
  src/util/virfile.c
  src/util/virhash.c
+src/util/virkeyfile.c
  src/util/virnetdev.c
  src/util/virnetdevbridge.c
  src/util/virnetdevmacvlan.c
diff --git a/src/Makefile.am b/src/Makefile.am
index 39076cc..07d7faa 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -90,6 +90,7 @@ UTIL_SOURCES =
\
util/virhash.c util/virhash.h   \
util/virhashcode.c util/virhashcode.h   \
util/virkeycode.c util/virkeycode.h \
+   util/virkeyfile.c util/virkeyfile.h \
util/virkeymaps.h   \
util/virmacaddr.h util/virmacaddr.c \
util/virnetdev.h util/virnetdev.c   \
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 8a14838..3f69ec1 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1198,6 +1198,16 @@ virKeycodeValueFromString;
  virKeycodeValueTranslate;


+# virkeyfile.h
+virKeyFileNew;
+virKeyFileLoadFile;
+virKeyFileLoadData;
+virKeyFileFree;
+virKeyFileHasValue;
+virKeyFileHasGroup;
+virKeyFileGetValueString;


Again, the sorting, per all the strings are sorted now. :-)


+
+
  # virmacaddr.h
  virMacAddrCompare;
  virMacAddrFormat;
diff --git a/src/util/virkeyfile.c b/src/util/virkeyfile.c
new file mode 100644
index 000..3dd4960
--- /dev/null
+++ b/src/util/virkeyfile.c
@@ -0,0 +1,367 @@
+/*
+ * virkeyfile.c: ini-style configuration file handling
+ *
+ * Copyright (C) 2012 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, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307  USA
+ *
+ * Authors:
+ * Daniel P. Berrangeberra...@redhat.com
+ */
+
+#includeconfig.h
+
+#includestdio.h
+
+#include c-ctype.h
+#include logging.h
+#include memory.h
+#include util.h
+#include virhash.h
+#include virkeyfile.h
+#include virterror_internal.h
+
+#define VIR_FROM_THIS VIR_FROM_CONF
+
+typedef struct _virKeyFileGroup virKeyFileGroup;
+typedef virKeyFileGroup *virKeyFileGroupPtr;
+
+typedef struct _virKeyFileParserCtxt virKeyFileParserCtxt;
+typedef virKeyFileParserCtxt *virKeyFileParserCtxtPtr;
+
+struct _virKeyFile {
+virHashTablePtr groups;
+};
+
+struct _virKeyFileParserCtxt {
+virKeyFilePtr conf;
+
+const char *filename;
+
+const char *base;
+const char *cur;
+const char *end;
+size_t line;
+
+char *groupname;
+virHashTablePtr group;
+};
+
+/*
+ * The grammar for the keyfile
+ *
+ * KEYFILE = (GROUP | COMMENT | BLANK )*
+ *
+ * COMMENT = ('#' | ';') [^\n]* '\n'
+ * BLANK = (' ' | '\t' )* '\n'
+ *
+ * GROUP = '[' GROUPNAME ']' '\n' (ENTRY ) *
+ * GROUPNAME = [^[]\n]+
+ *
+ * ENTRY = KEYNAME '=' VALUE
+ * VALUE = [^\n]* '\n'
+ * KEYNAME = [-a-zA-Z0-9]+
+ */
+
+#define IS_EOF (ctxt-cur= ctxt-end)
+#define IS_EOL(c) (((c) == '\n') || ((c) == '\r'))
+#define CUR (*ctxt-cur)
+#define NEXT if (!IS_EOF) ctxt-cur++;
+
+
+#define virKeyFileError(ctxt, error, info) \
+virKeyFileErrorHelper(__FILE__, __FUNCTION__, __LINE__, ctxt, error, info)
+static void
+virKeyFileErrorHelper(const char *file, const char

Re: [libvirt] Does libvirt check MCS labels during hot-add disk image ?

2012-03-22 Thread Daniel P. Berrange

On Thu, Mar 22, 2012 at 09:36:30AM +0530, Onkar N Mahajan wrote:
 Libvirt doesn't care about security during hot add disk images. It even
 accepts addition of disk images of other guest running on the host. 
 
 Steps followed to create this scenario : 


 Now, try to add vm1's disk image into vm2 - this must not be allowed -
 since for virtualized guest images. Only svirt_t processes with the 
 same MCS fields can read/write these images. i.e.,  for vm2 to access
 vm1's disk image it's MCS label must be 's0:c660,c689'. 
 
 Hot addition of vm1's image i.e., /var/lib/libvirt/images/vm1.img is
 successful ( which must not be allowed )

sVirt does not try to stop any host administrator actions. Its goal
is isolate guests from each other. There is nothing wrong with the
scenario you descibe from sVirt's POV. Only one guest is able to
access the disk at a time - the first VM looses access when you
give the disk to the second VM, so there is no security flaw here.

Responsibility for stopping administrator actions like this lies
with the disk locking framework. If you enable the sanlock driver
in libvirt, you would have been prevented from adding the disk
to the second guest, while the host is running

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] [test-API PATCH 0/7] Multiple fixes and improvements series

2012-03-22 Thread Peter Krempa


On 03/22/2012 08:54 AM, Guannan Ren wrote:

On 03/21/2012 08:46 PM, Peter Krempa wrote:

This is a set of more or less independent fixes and improvements to the
test API. I ran across these while trying to write a basic test case as
a Hello world! to the test-API.

Improvements are in fields of cross-distro compatibility, broken API's
and
typos and usability.

Peter Krempa (7):
utils: Make ipget.sh more portable
lib: fix streamAPI class
domainAPI: Add wrapper method to work with domain's console
connections
repos/domain/create.py: Fix typo in path to python executable
utils: Allow suffixes in path to the main checkout folder
parser: Be more specific on mistakes in case files
domain/[start|destroy]: Add a optional noping flag to skip the ping
test

exception.py | 10 +
lib/domainAPI.py | 9 
lib/streamAPI.py | 52 +++---
proxy.py | 4 +++
repos/domain/create.py | 2 +-
repos/domain/destroy.py | 50 +---
repos/domain/start.py | 50 +---
utils/Python/utils.py | 2 +-
utils/ipget.sh | 4 +-
9 files changed, 105 insertions(+), 78 deletions(-)



Hi Peter

I have pushed the set of patches after some small modifications based on
my comments.
If you have any comment, please be free to tell me.
Thanks your patches.


Thanks for the review,pointing out some mistakes (I'm a python 
beginner), and pushing the series.


Peter



Guannan Ren

--
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] [PATCH 0/2] Cleanup for return with parentheses

2012-03-22 Thread Martin Kletzander

There was one big inconsistency in our source files when return was
used. Sometimes it was used with parentheses and sometimes
without. The old-style return(value) was removed in the first commit,
the second commit introduces new syntax-check to ensure the style will
remain the same in the future.

Martin Kletzander (2):
  Cleanup for a return statement in source files
  Added syntax-check rule for return with parentheses

 cfg.mk|6 +
 examples/dominfo/info1.c  |2 +-
 examples/domsuspend/suspend.c |6 +-
 python/generator.py   |8 +-
 python/libvirt-override.c |  216 +-
 python/libvirt-qemu-override.c|4 +-
 python/typewrappers.c |   76 +-
 src/conf/domain_conf.c|   22 ++--
 src/conf/interface_conf.c |   70 +-
 src/conf/nwfilter_params.c|2 +-
 src/cpu/cpu_x86.c |2 +-
 src/datatypes.c   |   80 +-
 src/interface/netcf_driver.c  |2 +-
 src/lxc/lxc_driver.c  |4 +-
 src/nwfilter/nwfilter_ebiptables_driver.c |4 +-
 src/qemu/qemu_command.c   |6 +-
 src/qemu/qemu_driver.c|4 +-
 src/qemu/qemu_process.c   |2 +-
 src/remote/remote_driver.c|2 +-
 src/security/security_selinux.c   |2 +-
 src/test/test_driver.c|8 +-
 src/util/conf.c   |  106 +++---
 src/util/hooks.c  |   20 ++--
 src/util/sexpr.c  |   14 +-
 src/util/util.c   |8 +-
 src/util/uuid.c   |6 +-
 src/util/virhash.c|   30 ++--
 src/util/virmacaddr.c |2 +-
 src/util/virnetlink.c |2 +-
 src/util/virsocketaddr.c  |   48 +++---
 src/util/virterror.c  |   14 +-
 src/util/xml.c|   50 +++---
 src/xen/xen_driver.c  |   32 ++--
 src/xen/xen_hypervisor.c  |  228 ++--
 src/xen/xend_internal.c   |  234 ++--
 src/xen/xm_internal.c |   98 ++--
 src/xen/xs_internal.c |  120 
 src/xenapi/xenapi_driver.c|2 +-
 src/xenxs/xen_sxpr.c  |2 +-
 src/xenxs/xen_xm.c|2 +-
 tests/commandtest.c   |4 +-
 tests/cputest.c   |2 +-
 tests/domainsnapshotxml2xmltest.c |4 +-
 tests/interfacexml2xmltest.c  |2 +-
 tests/networkxml2argvtest.c   |2 +-
 tests/networkxml2xmltest.c|2 +-
 tests/nodedevxml2xmltest.c|2 +-
 tests/nodeinfotest.c  |2 +-
 tests/nwfilterxml2xmltest.c   |2 +-
 tests/qemuargv2xmltest.c  |2 +-
 tests/qemuxml2argvtest.c  |2 +-
 tests/qemuxml2xmltest.c   |4 +-
 tests/qemuxmlnstest.c |2 +-
 tests/qparamtest.c|2 +-
 tests/sexpr2xmltest.c |4 +-
 tests/sockettest.c|2 +-
 tests/statstest.c |2 +-
 tests/storagepoolxml2xmltest.c|2 +-
 tests/storagevolxml2xmltest.c |2 +-
 tests/virbuftest.c|2 +-
 tests/virnetmessagetest.c |2 +-
 tests/virnetsockettest.c  |4 +-
 tests/virnettlscontexttest.c  |2 +-
 tests/virshtest.c |2 +-
 tests/virtimetest.c   |2 +-
 tests/xencapstest.c   |2 +-
 tests/xmconfigtest.c  |4 +-
 tests/xml2sexprtest.c |4 +-
 tools/virsh.c |8 +-
 69 files changed, 815 insertions(+), 809 deletions(-)

--
1.7.3.4

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

[libvirt] [PATCH 2/2] Added syntax-check rule for return with parentheses

2012-03-22 Thread Martin Kletzander

After cleanup introduced with previous commit, there is a need for
syntax-check rule taking care of return(). Regexp used in 'prohibit'
parameter is taken from the cleanup commit and modified so it fits
'grep -E' format. Semicolon at the end is needed, otherwise the regexp
could match return with cast.

No exception is needed thanks to files excused by default. The
exception is not even needed for python files because 1) when
returning a tuple, you can still omit parentheses around (unless there
is only one value inside, but that's very unusual case), 2) return in
python doesn't need semicolon at the end of statement when there is no
statement following (and statement following 'return' doesn't make
sense).
---
 cfg.mk |6 ++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/cfg.mk b/cfg.mk
index 24e6a69..59b07ef 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -469,6 +469,12 @@ sc_prohibit_xmlURI:
halt='use virURI(Parse|Format), not xml(ParseURI|SaveUri)'  \
  $(_sc_search_regexp)

+# we don't want old old-style return with parentheses around argument
+sc_prohibit_return_as_function:
+   @prohibit='\return *\(([^()]*(\([^()]*\)[^()]*)*)\) *;'\
+   halt='avoid extra () with return statements'\
+ $(_sc_search_regexp)
+
 # ATTRIBUTE_UNUSED should only be applied in implementations, not
 # header declarations
 sc_avoid_attribute_unused_in_header:
-- 
1.7.3.4

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

[libvirt] [PATCH] Use virDomainFindbyID and pass id instead of a pointer to lxcMontitorEvent.

2012-03-22 Thread Thomas Hunger

This fixes a race condition when VIR_EVENT_HANDLE_HANGUP is triggered
during lxcDomainDestroyFlags: lxcMonitorEvent tries to acquire the
driver lock held by lxcDomainDestroyFlags and blocks. Meanwhile
lxcDomainDestroyFlags will free the vm structure and release the driver
lock. lxcMonitorEvent unblocks and operates on an invalid vm pointer.

By using virDomainFindbyID lxcMonitorEvent avoids using an invalid vm
pointer.
---
 src/lxc/lxc_driver.c |   13 +
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 3af8084..c55e164 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -1523,12 +1523,17 @@ static void lxcMonitorEvent(int watch,
 void *data)
 {
 lxc_driver_t *driver = lxc_driver;
-virDomainObjPtr vm = data;
+int id = (int)data;
+virDomainObjPtr vm = NULL;
 virDomainEventPtr event = NULL;
 lxcDomainObjPrivatePtr priv;
 
 lxcDriverLock(driver);
-virDomainObjLock(vm);
+vm = virDomainFindByID(driver-domains, id);
+if (vm == NULL) {
+lxcDriverUnlock(driver);
+goto cleanup;
+}
 lxcDriverUnlock(driver);
 
 priv = vm-privateData;
@@ -1950,7 +1955,7 @@ static int lxcVmStart(virConnectPtr conn,
  priv-monitor,
  VIR_EVENT_HANDLE_ERROR | VIR_EVENT_HANDLE_HANGUP,
  lxcMonitorEvent,
- vm, NULL))  0) {
+ (void*)vm-def-id, NULL))  0) {
 goto error;
 }
 
@@ -2513,7 +2518,7 @@ lxcReconnectVM(void *payload, const void *name 
ATTRIBUTE_UNUSED, void *opaque)
  priv-monitor,
  VIR_EVENT_HANDLE_ERROR | VIR_EVENT_HANDLE_HANGUP,
  lxcMonitorEvent,
- vm, NULL))  0)
+ (void*)vm-def-id, NULL))  0)
 goto error;
 
 if (virSecurityManagerReserveLabel(driver-securityManager,
-- 
1.7.2.5

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

[libvirt] [test-API PATCH] streamAPI: Create stream objects that are already encapsulated

2012-03-22 Thread Peter Krempa

With this patch, the newStream() method of the ConnectAPI class returns
an already encapsulated StreamAPI object .
 *lib/connectAPI.py: - modify newStream method to return StreamAPI
   object
 *lib/streamAPI.py: - modify constructor to take virStream objects
---
 lib/connectAPI.py |4 +++-
 lib/streamAPI.py  |9 ++---
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/lib/connectAPI.py b/lib/connectAPI.py
index f52467f..2723fc8 100644
--- a/lib/connectAPI.py
+++ b/lib/connectAPI.py
@@ -24,6 +24,7 @@ import os
 import re

 import libvirt
+from lib import streamAPI

 def append_path(path):
 Append root path of package
@@ -226,7 +227,8 @@ class ConnectAPI(object):

 def newStream(self, flag = 0):
 try:
-return self.conn.newStream(flag)
+st_obj = self.conn.newStream(flag)
+return streamAPI.StreamAPI(st_obj)
 except libvirt.libvirtError, e:
 message = e.get_error_message()
 code = e.get_error_code()
diff --git a/lib/streamAPI.py b/lib/streamAPI.py
index 18ce71e..f551992 100644
--- a/lib/streamAPI.py
+++ b/lib/streamAPI.py
@@ -38,13 +38,8 @@ append_path(result.group(0))
 import exception

 class StreamAPI(object):
-def __init__(self, conn, flags = 0):
-try:
-self.stream = conn.newStream(flags)
-except libvirt.libvirtError, e:
-message = e.get_error_message()
-code = e.get_error_code()
-raise exception.LibvirtAPI(message, code)
+def __init__(self, stream):
+self.stream = stream;

 def getStream(self):
 return self.stream
-- 
1.7.3.4

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

[libvirt] [test-API PATCH] libs: Add flags for streams and the console

2012-03-22 Thread Peter Krempa

Add the local copy of the flags.
---
 lib/connectAPI.py |2 ++
 lib/domainAPI.py  |3 +++
 2 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/lib/connectAPI.py b/lib/connectAPI.py
index dab5e7d..2723fc8 100644
--- a/lib/connectAPI.py
+++ b/lib/connectAPI.py
@@ -350,3 +350,5 @@ class ConnectAPI(object):
 VIR_CRED_AUTHNAME = libvirt.VIR_CRED_AUTHNAME
 VIR_CRED_PASSPHRASE = libvirt.VIR_CRED_PASSPHRASE

+# virStreamFlags
+VIR_STREAM_NONBLOCK = 1
diff --git a/lib/domainAPI.py b/lib/domainAPI.py
index bc0069b..43565c2 100644
--- a/lib/domainAPI.py
+++ b/lib/domainAPI.py
@@ -912,3 +912,6 @@ VIR_DOMAIN_AFFECT_CURRENT = 0
 VIR_DOMAIN_AFFECT_LIVE = 1
 VIR_DOMAIN_AFFECT_CONFIG = 2

+# virDomainConsoleFlags
+VIR_DOMAIN_CONSOLE_FORCE = 1
+VIR_DOMAIN_CONSOLE_SAFE  = 2
-- 
1.7.3.4

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

Re: [libvirt] [PATCH 12/14] Add APIs for handling lookup of auth credentials from config file

2012-03-22 Thread Eric Blake

On 03/20/2012 11:33 AM, Daniel P. Berrange wrote:
 From: Daniel P. Berrange berra...@redhat.com
 
 This defines the format for the auth credential config file and
 provides APIs to access the data. The config file contains
 one or more named 'credential' sets
 
   [credentials-$NAME]
   credname1=value1
   credname2=value2
 
 eg
 
   [credentials-test]
   authname=fred
   password=123456

I'm not always a fan of plain-text passwords; do you have plans to
further enhance this to hook into our virSecret design, where the config
file can list the name of a secret to reference, which in turn will
trigger appropriate calls to the virSecret API to grab credentials on
first use, securely caching them for later uses that need the same
credentials but without the drawbacks of plain-text config files?  But
that's future enhancement, and doesn't stop this patch from going in
once you address Osier's review comments.

-- 
Eric Blake   ebl...@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] Problem with Open vSwitch and dnsmasq

2012-03-22 Thread Daniele Milani


Dear all,
I have the following situation:
-I replaced the standard bridge driver with the Open VSwitch one;
-I started a NAT-network on Libvirt (bridge name virbr1);
-I started a Virtual Machine (VM1) on Libvirt, and I tagged his interface 
(vnet0) with tag=2;
-if I run # ovs-vsctl show I obtain:

Bridge virbr1
Port vnet0
tag: 2
Interface vnet0
Port virbr1-nic
Interface virb1-nic
Port virbr1
Interface virbr1
type: internal

-the problem is that it is impossible to assign to VM1 an IP, because the 
dnsmasq daemon does not accept the tagged DHCP Discover frame.

Does someone know if there is a way for dnsmasq to accept tagged frames through 
virbr1, and send a tagged DHCP Offer packet back to VM1?

Greetings,
Daniele Milani

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

Re: [libvirt] [Qemu-devel] Modern CPU models cannot be used with libvirt

2012-03-22 Thread Eduardo Habkost

On Thu, Mar 22, 2012 at 11:32:44AM +0200, Gleb Natapov wrote:
 On Tue, Mar 13, 2012 at 11:53:19AM -0300, Eduardo Habkost wrote:
  So, trying to summarize what was discussed in the call:
  
  On Mon, Mar 12, 2012 at 10:08:10AM -0300, Eduardo Habkost wrote:
Let's say we moved CPU definitions to /usr/share/qemu/cpu-models.xml.

Obviously, we'd want a command line option to be able to change that
location so we'd introduce -cpu-models PATH.

But we want all of our command line options to be settable by the
global configuration file so we would have a cpu-model=PATH to the
configuration file.

But why hard code a path when we can just set the default path in the
configuration file so let's avoid hard coding and just put
cpu-models=/usr/share/qemu/cpu-models.xml in the default
configuration file.
   
   We wouldn't do the above.
   
   -nodefconfig should disable the loading of files on /etc, but it
   shouldn't disable loading internal non-configurable data that we just
   happened to choose to store outside the qemu binary because it makes
   development easier.
  
  The statement above is the one not fulfilled by the compromise solution:
  -nodefconfig would really disable the loading of files on /usr/share.
  
 What does this mean? Will -nodefconfig disable loading of bios.bin,
 option roms, keymaps?

Correcting myself: loading of _config_ files on /usr/share. ROM images
are opaque data to be presented to the guest somehow, just like a disk
image or kernel binary. But maybe keymaps will become configuration
someday, I really don't know.


   
   Doing this would make it impossible to deploy fixes to users if we evern
   find out that the default configuration file had a serious bug. What if
   a bug in our default configuration file has a serious security
   implication?
  
  The answer to this is: if the broken templates/defaults are on
  /usr/share, it would be easy to deploy the fix.
  
  So, the compromise solution is:
  
  - We can move some configuration data (especially defaults/templates)
to /usr/share (machine-types and CPU models could go there). This
way we can easily deploy fixes to the defaults, if necessary.
  - To reuse Qemu models, or machine-types, and not define everything from
scratch, libvirt will have to use something like:
-nodefconfig -readconfig /usr/share/qemu/cpu-models-x86.conf
  
 cpu-models-x86.conf is not a configuration file. It is hardware
 description file. QEMU should not lose capability just because you run
 it with -nodefconfig. -nodefconfig means that QEMU does not create
 machine for you, but all parts needed to create a machine that would have
 been created without -nodefconfig are still present. Not been able to
 create Nehalem CPU after specifying -nodefconfig is the same as not been
 able to create virtio-net i.e the bug.


The current design direction Qemu seems to be following is different
from that: hardware description is also considered configuration just
like actual machine configuration. Anthony, please correct me if I am
wrong.


What you propose is to have two levels of configuration (or
descriptions, or whatever we call it):

1) Hardware descriptions (or templates, or models, whatever we call it),
   that are not editable by the user (and not disabled by -nodefconfig).
   This may include CPU models, hardware emulation implemented in
   another language, machine-types, and other stuff that is part of
   what Qemu always provides.
2) Actual machine configuration file, that is configurable and editable
   by the user, and normally loaded from /etc on from the command-line.

The only problem is: the Qemu design simply doesn't have this
distinction today (well, it _has_, the only difference is that today
item (1) is almost completely coded inside tables in .c files). So if we
want to go in that direction we have to agree this will be part of the
Qemu design.

I am not strongly inclined either way. Both approaches look good to me,
we just have to decide where we are going, because we're are in this
weird position todady because we never decided it explicitly, libvirt
expected one thing, and we implemented something else.

On the one hand I think the two-layer design gives us more freedom to
move stuff outside .c files and change implementation details, and fits
how we have been doing until today with machine types and built-in CPU
models, keymaps, etc.

On the other hand, I think not having this distinction between machine
configuration and hardware description may be a good thing.

For example: today there are two different ways of enabling a feature on
a CPU: defining a new model, and adding a flag to -cpu. And I think
this asymmetry shouldn't be there: you just need a good system to define
a CPU, a good set of defaults/templates, and a good system to base your
configuration on those defaults/teampltes, no need to have two separate
CPU definition languages. Also, we wouldn't have to code

Re: [libvirt] [test-API PATCH] streamAPI: Create stream objects that are already encapsulated

2012-03-22 Thread Guannan Ren


On 03/22/2012 08:54 PM, Peter Krempa wrote:

With this patch, the newStream() method of the ConnectAPI class returns
an already encapsulated StreamAPI object .
  *lib/connectAPI.py: - modify newStream method to return StreamAPI
object
  *lib/streamAPI.py: - modify constructor to take virStream objects
---
  lib/connectAPI.py |4 +++-
  lib/streamAPI.py  |9 ++---
  2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/lib/connectAPI.py b/lib/connectAPI.py
index f52467f..2723fc8 100644
--- a/lib/connectAPI.py
+++ b/lib/connectAPI.py
@@ -24,6 +24,7 @@ import os
  import re

  import libvirt
+from lib import streamAPI


  The initial idea is to have a separate file for each of 
classes(domain, storage, etc)
 in libvirt.py at least in the form of files. The direction you are 
heading here is to

 combine them together again.
  Testcase is the right place where these objects of 
encapsulated class should meet

 each other.
  I prefer the way of what you did in openConsole API.  We just 
pass the instance of
 ConnectAPI to initiate SteamAPI class for getting the object of 
SteamAPI for use

 we should delete the newStream method in connectAPI.py.
  Right now, the virConnect object from libvirt.py is exposed 
directly in existing testcases,
 that's not good way compared to what you did in openConsole. I 
will clean them next.

  What do you think?

Guannan Ren

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

Re: [libvirt] [test-API PATCH] libs: Add flags for streams and the console

2012-03-22 Thread Guannan Ren


On 03/22/2012 08:56 PM, Peter Krempa wrote:

Add the local copy of the flags.
---
  lib/connectAPI.py |2 ++
  lib/domainAPI.py  |3 +++
  2 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/lib/connectAPI.py b/lib/connectAPI.py
index dab5e7d..2723fc8 100644
--- a/lib/connectAPI.py
+++ b/lib/connectAPI.py
@@ -350,3 +350,5 @@ class ConnectAPI(object):
  VIR_CRED_AUTHNAME = libvirt.VIR_CRED_AUTHNAME
  VIR_CRED_PASSPHRASE = libvirt.VIR_CRED_PASSPHRASE

+# virStreamFlags
+VIR_STREAM_NONBLOCK = 1


  The stream flags or all of stream related methods should belong 
to streamAPI.py
  The connectAPI.py just includes hypervisor or host APIs, others 
excluded.



diff --git a/lib/domainAPI.py b/lib/domainAPI.py
index bc0069b..43565c2 100644
--- a/lib/domainAPI.py
+++ b/lib/domainAPI.py
@@ -912,3 +912,6 @@ VIR_DOMAIN_AFFECT_CURRENT = 0
  VIR_DOMAIN_AFFECT_LIVE = 1
  VIR_DOMAIN_AFFECT_CONFIG = 2

+# virDomainConsoleFlags
+VIR_DOMAIN_CONSOLE_FORCE = 1
+VIR_DOMAIN_CONSOLE_SAFE  = 2


Sorry I didn't find virDomainConsoleFlags in libvirt.py

Guannan Ren

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

[libvirt] [PATCH] Leave all child processes running when stopping systemd service

2012-03-22 Thread Daniel P. Berrange

From: Daniel P. Berrange berra...@redhat.com

Currently the libvirt.service unit file for systemd does not
specify any kill mode. So systemd kills off every process
inside its cgroup. ie all dnsmasq processes, all virtual
machines. This obviously not what we want. Set KillMode=process
so that it only kills the top level process of libvirtd

* daemon/libvirtd.service.in: Add KillMode=process

Reported-By: Mark McLoughlin mar...@redhat.com
Signed-off-by: Daniel P. Berrange berra...@redhat.com
---
 daemon/libvirtd.service.in |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/daemon/libvirtd.service.in b/daemon/libvirtd.service.in
index 8f2458a..d6e46ab 100644
--- a/daemon/libvirtd.service.in
+++ b/daemon/libvirtd.service.in
@@ -15,6 +15,7 @@ Before=libvirt-guests.service
 EnvironmentFile=-/etc/sysconfig/libvirtd
 ExecStart=@sbindir@/libvirtd $LIBVIRTD_ARGS
 ExecReload=/bin/kill -HUP $MAINPID
+KillMode=process
 # Override the maximum number of opened files
 #LimitNOFILE=2048
 
-- 
1.7.7.6

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

Re: [libvirt] [PATCH] Leave all child processes running when stopping systemd service

2012-03-22 Thread Mark McLoughlin

On Thu, 2012-03-22 at 14:03 +, Daniel P. Berrange wrote:
 From: Daniel P. Berrange berra...@redhat.com
 
 Currently the libvirt.service unit file for systemd does not
 specify any kill mode. So systemd kills off every process
 inside its cgroup. ie all dnsmasq processes, all virtual
 machines. This obviously not what we want. Set KillMode=process
 so that it only kills the top level process of libvirtd
 
 * daemon/libvirtd.service.in: Add KillMode=process

I was still looking through this. Try doing:

  $ systemctl restart libvirtd.service

It's killing the existing processes in the cgroup when starting the
service. From poking at an strace, this looks like the code that's doing
it:

  
http://cgit.freedesktop.org/systemd/systemd/tree/src/service.c?id=75c8e3cf#n2093

Cheers,
Mark.

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

Re: [libvirt] [PATCH 1/2] snapshot: add support for qemu transaction command

2012-03-22 Thread Peter Krempa


On 03/17/2012 10:27 PM, Eric Blake wrote:

QEmu 1.1 is adding a 'transaction' command to the JSON monitor.
Each element of a transaction corresponds to a top-level command,
with the additional guarantee that the transaction flushes all
pending I/O, then guarantees that all actions will be successful
as a group or that failure will roll back the state to what it
was before the monitor command.  The difference between a
top-level command:

{ execute: blockdev-snapshot-sync, arguments:
   { device: virtio0, ... } }

and a transaction:

{ execute: transaction, arguments:
   { actions: [
 { type blockdev-snapshot-sync, data:


Shouldn't there be a colon between type and block?


   { device: virtio0, ... } } ] } }

is just a couple of changed key names and nesting the shorter
command inside a JSON array to the longer command.  This patch
just adds the framework; the next patch will actually use a
transaction.



diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 4dd6924..ba07e84 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -383,7 +384,8 @@ qemuMonitorJSONMakeCommand(const char *cmdname,
  if (!(obj = virJSONValueNewObject()))
  goto no_memory;

-if (virJSONValueObjectAppendString(obj, execute, cmdname)  0)
+if (virJSONValueObjectAppendString(obj, wrap ? type : execute,
+   cmdname)  0)
  goto no_memory;


Hopefuly QEmu won't add another wrapping type.



  while ((key = va_arg(args, char *)) != NULL) {


ACK,

Peter

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

Re: [libvirt] [PATCH] Leave all child processes running when stopping systemd service

2012-03-22 Thread Mark McLoughlin

On Thu, 2012-03-22 at 14:10 +, Mark McLoughlin wrote:
 On Thu, 2012-03-22 at 14:03 +, Daniel P. Berrange wrote:
  From: Daniel P. Berrange berra...@redhat.com
  
  Currently the libvirt.service unit file for systemd does not
  specify any kill mode. So systemd kills off every process
  inside its cgroup. ie all dnsmasq processes, all virtual
  machines. This obviously not what we want. Set KillMode=process
  so that it only kills the top level process of libvirtd
  
  * daemon/libvirtd.service.in: Add KillMode=process
 
 I was still looking through this. Try doing:
 
   $ systemctl restart libvirtd.service
 
 It's killing the existing processes in the cgroup when starting the
 service. From poking at an strace, this looks like the code that's doing
 it:
 
   
 http://cgit.freedesktop.org/systemd/systemd/tree/src/service.c?id=75c8e3cf#n2093
 

Filed a bug:

  https://bugzilla.redhat.com/805942

Cheers,
Mark.

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

Re: [libvirt] [PATCH] Leave all child processes running when stopping systemd service

2012-03-22 Thread Daniel P. Berrange

On Thu, Mar 22, 2012 at 02:10:30PM +, Mark McLoughlin wrote:
 On Thu, 2012-03-22 at 14:03 +, Daniel P. Berrange wrote:
  From: Daniel P. Berrange berra...@redhat.com
  
  Currently the libvirt.service unit file for systemd does not
  specify any kill mode. So systemd kills off every process
  inside its cgroup. ie all dnsmasq processes, all virtual
  machines. This obviously not what we want. Set KillMode=process
  so that it only kills the top level process of libvirtd
  
  * daemon/libvirtd.service.in: Add KillMode=process
 
 I was still looking through this. Try doing:
 
   $ systemctl restart libvirtd.service
 
 It's killing the existing processes in the cgroup when starting the
 service. From poking at an strace, this looks like the code that's doing
 it:
 
   
 http://cgit.freedesktop.org/systemd/systemd/tree/src/service.c?id=75c8e3cf#n2093

It didn't kill guests/dnsmasq in my testing.  Are you sure you did
a 'systemctl daemon-reload' after adding the KillMode flag to the
unit file ?


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] Leave all child processes running when stopping systemd service

2012-03-22 Thread Mark McLoughlin

On Thu, 2012-03-22 at 14:22 +, Daniel P. Berrange wrote:
 On Thu, Mar 22, 2012 at 02:10:30PM +, Mark McLoughlin wrote:
  On Thu, 2012-03-22 at 14:03 +, Daniel P. Berrange wrote:
   From: Daniel P. Berrange berra...@redhat.com
   
   Currently the libvirt.service unit file for systemd does not
   specify any kill mode. So systemd kills off every process
   inside its cgroup. ie all dnsmasq processes, all virtual
   machines. This obviously not what we want. Set KillMode=process
   so that it only kills the top level process of libvirtd
   
   * daemon/libvirtd.service.in: Add KillMode=process
  
  I was still looking through this. Try doing:
  
$ systemctl restart libvirtd.service
  
  It's killing the existing processes in the cgroup when starting the
  service. From poking at an strace, this looks like the code that's doing
  it:
  

  http://cgit.freedesktop.org/systemd/systemd/tree/src/service.c?id=75c8e3cf#n2093
 
 It didn't kill guests/dnsmasq in my testing.  Are you sure you did
 a 'systemctl daemon-reload' after adding the KillMode flag to the
 unit file ?

Yep:

[markmc@zig ~]$ sudo systemctl show libvirtd.service | grep KillMode
KillMode=process
[markmc@zig ~]$ systemd-cgls /system/libvirtd.service
/system/libvirtd.service:
├ 12660 /usr/sbin/libvirtd
├ 20598 /usr/bin/qemu-kvm -S -M pc-0.15 -enable-kvm -m 2048 -smp 
1,sockets=1,cores=1,threads=1 -name instance-0021 -uuid 39e9ec0e-2...
└ 20665 /sbin/dnsmasq --strict-order --bind-interfaces 
--pid-file=/var/run/libvirt/network/default.pid --conf-file= --except-interface 
...
[markmc@zig ~]$ sudo systemctl restart libvirtd.service
[markmc@zig ~]$ systemd-cgls /system/libvirtd.service
/system/libvirtd.service:
└ 20742 /usr/sbin/libvirtd

Cheers,
Mark.

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

Re: [libvirt] [Qemu-devel] Modern CPU models cannot be used with libvirt

2012-03-22 Thread Gleb Natapov

On Thu, Mar 22, 2012 at 10:31:21AM -0300, Eduardo Habkost wrote:
 On Thu, Mar 22, 2012 at 11:32:44AM +0200, Gleb Natapov wrote:
  On Tue, Mar 13, 2012 at 11:53:19AM -0300, Eduardo Habkost wrote:
   So, trying to summarize what was discussed in the call:
   
   On Mon, Mar 12, 2012 at 10:08:10AM -0300, Eduardo Habkost wrote:
 Let's say we moved CPU definitions to /usr/share/qemu/cpu-models.xml.
 
 Obviously, we'd want a command line option to be able to change that
 location so we'd introduce -cpu-models PATH.
 
 But we want all of our command line options to be settable by the
 global configuration file so we would have a cpu-model=PATH to the
 configuration file.
 
 But why hard code a path when we can just set the default path in the
 configuration file so let's avoid hard coding and just put
 cpu-models=/usr/share/qemu/cpu-models.xml in the default
 configuration file.

We wouldn't do the above.

-nodefconfig should disable the loading of files on /etc, but it
shouldn't disable loading internal non-configurable data that we just
happened to choose to store outside the qemu binary because it makes
development easier.
   
   The statement above is the one not fulfilled by the compromise solution:
   -nodefconfig would really disable the loading of files on /usr/share.
   
  What does this mean? Will -nodefconfig disable loading of bios.bin,
  option roms, keymaps?
 
 Correcting myself: loading of _config_ files on /usr/share. ROM images
 are opaque data to be presented to the guest somehow, just like a disk
 image or kernel binary. But maybe keymaps will become configuration
 someday, I really don't know.
 
Where do you draw the line between opaque data and configuration. CPU
models are also something that is present to a guest somehow. Are you
consider ROMs to be opaque data because they are binary and CPU models
to be config just because it is ascii file? What if we pre-process CPU
models into binary for QEMU to read will it magically stop being
configuration?

 

Doing this would make it impossible to deploy fixes to users if we evern
find out that the default configuration file had a serious bug. What if
a bug in our default configuration file has a serious security
implication?
   
   The answer to this is: if the broken templates/defaults are on
   /usr/share, it would be easy to deploy the fix.
   
   So, the compromise solution is:
   
   - We can move some configuration data (especially defaults/templates)
 to /usr/share (machine-types and CPU models could go there). This
 way we can easily deploy fixes to the defaults, if necessary.
   - To reuse Qemu models, or machine-types, and not define everything from
 scratch, libvirt will have to use something like:
 -nodefconfig -readconfig /usr/share/qemu/cpu-models-x86.conf
   
  cpu-models-x86.conf is not a configuration file. It is hardware
  description file. QEMU should not lose capability just because you run
  it with -nodefconfig. -nodefconfig means that QEMU does not create
  machine for you, but all parts needed to create a machine that would have
  been created without -nodefconfig are still present. Not been able to
  create Nehalem CPU after specifying -nodefconfig is the same as not been
  able to create virtio-net i.e the bug.
 
 
 The current design direction Qemu seems to be following is different
 from that: hardware description is also considered configuration just
 like actual machine configuration. Anthony, please correct me if I am
 wrong.
That's a bug. Why trying to rationalize it now instead of fixing it. It
was fixed in RHEL by the same person who introduced it in upstream in
the first place. He just forgot to send the fix upstream. Does bug that
is present for a long time is promoted to a feature?

 
 
 What you propose is to have two levels of configuration (or
 descriptions, or whatever we call it):
 
 1) Hardware descriptions (or templates, or models, whatever we call it),
that are not editable by the user (and not disabled by -nodefconfig).
This may include CPU models, hardware emulation implemented in
another language, machine-types, and other stuff that is part of
what Qemu always provides.
 2) Actual machine configuration file, that is configurable and editable
by the user, and normally loaded from /etc on from the command-line.
 
 The only problem is: the Qemu design simply doesn't have this
 distinction today (well, it _has_, the only difference is that today
 item (1) is almost completely coded inside tables in .c files). So if we
 want to go in that direction we have to agree this will be part of the
 Qemu design.
 
 I am not strongly inclined either way. Both approaches look good to me,
 we just have to decide where we are going, because we're are in this
 weird position todady because we never decided it explicitly, libvirt
 expected one thing, and we implemented something else.

Re: [libvirt] [PATCH] Leave all child processes running when stopping systemd service

2012-03-22 Thread Daniel P. Berrange

On Thu, Mar 22, 2012 at 02:27:59PM +, Mark McLoughlin wrote:
 On Thu, 2012-03-22 at 14:22 +, Daniel P. Berrange wrote:
  On Thu, Mar 22, 2012 at 02:10:30PM +, Mark McLoughlin wrote:
   On Thu, 2012-03-22 at 14:03 +, Daniel P. Berrange wrote:
From: Daniel P. Berrange berra...@redhat.com

Currently the libvirt.service unit file for systemd does not
specify any kill mode. So systemd kills off every process
inside its cgroup. ie all dnsmasq processes, all virtual
machines. This obviously not what we want. Set KillMode=process
so that it only kills the top level process of libvirtd

* daemon/libvirtd.service.in: Add KillMode=process
   
   I was still looking through this. Try doing:
   
 $ systemctl restart libvirtd.service
   
   It's killing the existing processes in the cgroup when starting the
   service. From poking at an strace, this looks like the code that's doing
   it:
   
 
   http://cgit.freedesktop.org/systemd/systemd/tree/src/service.c?id=75c8e3cf#n2093
  
  It didn't kill guests/dnsmasq in my testing.  Are you sure you did
  a 'systemctl daemon-reload' after adding the KillMode flag to the
  unit file ?
 
 Yep:
 
 [markmc@zig ~]$ sudo systemctl show libvirtd.service | grep KillMode
 KillMode=process
 [markmc@zig ~]$ systemd-cgls /system/libvirtd.service
 /system/libvirtd.service:
 ├ 12660 /usr/sbin/libvirtd
 ├ 20598 /usr/bin/qemu-kvm -S -M pc-0.15 -enable-kvm -m 2048 -smp 
 1,sockets=1,cores=1,threads=1 -name instance-0021 -uuid 39e9ec0e-2...
 └ 20665 /sbin/dnsmasq --strict-order --bind-interfaces 
 --pid-file=/var/run/libvirt/network/default.pid --conf-file= 
 --except-interface ...
 [markmc@zig ~]$ sudo systemctl restart libvirtd.service
 [markmc@zig ~]$ systemd-cgls /system/libvirtd.service
 /system/libvirtd.service:
 └ 20742 /usr/sbin/libvirtd

Hmm, that's not the behaviour I see, once I ran 'daemon-reload'.

# systemd-cgls /system/libvirtd.service
/system/libvirtd.service:
├ 19607 /usr/sbin/dnsmasq --strict-order --bind-interfaces 
--pid-file=/var/run/libvirt/network/default.pid --conf-file= --ex...
├ 19809 /usr/libexec/qemu-kvm -S -M pc-0.13 -enable-kvm -m 215 -smp 
4,sockets=4,cores=1,threads=1 -name vm1 -uuid c7a3edbd-e...
└ 19849 /usr/sbin/libvirtd

[root@t500wlan system]# systemctl stop libvirtd.service
[root@t500wlan system]# systemd-cgls /system/libvirtd.service
/system/libvirtd.service:
├ 19607 /usr/sbin/dnsmasq --strict-order --bind-interfaces 
--pid-file=/var/run/libvirt/network/default.pid --conf-file= --ex...
└ 19809 /usr/libexec/qemu-kvm -S -M pc-0.13 -enable-kvm -m 215 -smp 
4,sockets=4,cores=1,threads=1 -name vm1 -uuid c7a3edbd-e...

[root@t500wlan system]# systemctl start libvirtd.service
[root@t500wlan system]# systemd-cgls /system/libvirtd.service
/system/libvirtd.service:
├ 19607 /usr/sbin/dnsmasq --strict-order --bind-interfaces 
--pid-file=/var/run/libvirt/network/default.pid --conf-file= --ex...
├ 19809 /usr/libexec/qemu-kvm -S -M pc-0.13 -enable-kvm -m 215 -smp 
4,sockets=4,cores=1,threads=1 -name vm1 -uuid c7a3edbd-e...
└ 26391 /usr/sbin/libvirtd


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 2/2] snapshot: wire up qemu transaction command

2012-03-22 Thread Peter Krempa


On 03/17/2012 10:27 PM, Eric Blake wrote:

The hardest part about adding transactions is not using the new
monitor command, but undoing the partial changes we made prior
to a failed transaction.

* src/qemu/qemu_driver.c (qemuDomainSnapshotCreateDiskActive): Use
transaction when available.
(qemuDomainSnapshotUndoSingleDiskActive): New function.
(qemuDomainSnapshotCreateSingleDiskActive): Pass through actions.
(qemuDomainSnapshotCreateXML): Adjust caller.
---
  src/qemu/qemu_driver.c |  112 +--
  1 files changed, 107 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c3bbc3f..2bc8d5d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9822,7 +9822,8 @@ qemuDomainSnapshotCreateSingleDiskActive(struct 
qemud_driver *driver,
   virDomainObjPtr vm,
   virDomainSnapshotDiskDefPtr snap,
   virDomainDiskDefPtr disk,
- virDomainDiskDefPtr persistDisk)
+ virDomainDiskDefPtr persistDisk,
+ virJSONValuePtr actions)
  {
  qemuDomainObjPrivatePtr priv = vm-privateData;
  char *device = NULL;
@@ -9882,7 +9883,7 @@ qemuDomainSnapshotCreateSingleDiskActive(struct 
qemud_driver *driver,
  origdriver = NULL;

  /* create the actual snapshot */
-ret = qemuMonitorDiskSnapshot(priv-mon, NULL, device, source);
+ret = qemuMonitorDiskSnapshot(priv-mon, actions, device, source);
  virDomainAuditDisk(vm, disk-src, source, snapshot, ret= 0);
  if (ret  0)
  goto cleanup;
@@ -9923,6 +9924,69 @@ cleanup:
  return ret;
  }

+/* The domain is expected to hold monitor lock.  This is the
+ * counterpart to qemuDomainSnapshotCreateSingleDiskActive, called
+ * only on a failed transaction. */
+static void
+qemuDomainSnapshotUndoSingleDiskActive(struct qemud_driver *driver,
+   virDomainObjPtr vm,
+   virDomainDiskDefPtr origdisk,
+   virDomainDiskDefPtr disk,
+   virDomainDiskDefPtr persistDisk,
+   bool need_unlink)
+{
+char *source = NULL;
+char *driverType = NULL;
+char *persistSource = NULL;
+char *persistDriverType = NULL;
+struct stat st;
+
+if (!(source = strdup(origdisk-src)) ||
+(origdisk-driverType
+ !(driverType = strdup(origdisk-driverType))) ||
+(persistDisk
+ (!(persistSource = strdup(source)) ||
+  (driverType  !(persistDriverType = strdup(driverType)) {
+virReportOOMError();
+goto cleanup;
+}
+
+if (virSecurityManagerRestoreImageLabel(driver-securityManager,
+vm-def, disk)  0)
+VIR_WARN(Unable to restore security label on %s, disk-src);
+if (virDomainLockDiskDetach(driver-lockManager, vm, disk)  0)
+VIR_WARN(Unable to release lock on %s, source);
+if (need_unlink  stat(disk-src,st) == 0
+  st.st_size == 0  S_ISREG(st.st_mode)  unlink(disk-src)  0)
+VIR_WARN(Unable to release lock on %s, disk-src);


Is this second warning correct? It looks to me like if you are unlinking 
the disk source file, but the error message states that lock release failed.



+
+/* Update vm in place to match changes.  */
+VIR_FREE(disk-src);
+disk-src = source;
+source = NULL;
+VIR_FREE(disk-driverType);
+if (driverType) {
+disk-driverType = driverType;
+driverType = NULL;
+}
+if (persistDisk) {
+VIR_FREE(persistDisk-src);
+persistDisk-src = persistSource;
+persistSource = NULL;
+VIR_FREE(persistDisk-driverType);
+if (persistDriverType) {
+persistDisk-driverType = persistDriverType;
+persistDriverType = NULL;
+}
+}
+
+cleanup:
+VIR_FREE(source);
+VIR_FREE(driverType);
+VIR_FREE(persistSource);
+VIR_FREE(persistDriverType);
+}
+


I don't have many experience working with the locking and security code, 
but this funcion looks as it's doing what it should. I'd feel more 
confident if somebody other would look at this part.


ACK to the rest, and a not-so-confident ACK of the marked part if my 
question gets cleared.


Peter

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

Re: [libvirt] [PATCH] snapshot: improve qemu handling of reused snapshot targets

2012-03-22 Thread Eric Blake

On 03/20/2012 03:29 PM, Eric Blake wrote:
 The oVirt developers have stated that the real reasons they want
 to have qemu reuse existing volumes when creating a snapshot are:
 1. the management framework is set up so that creation has to be
 done from a central node for proper resource tracking, and having
 libvirt and/or qemu create things violates the framework, and
 2. qemu defaults to creating snapshots with an absolute path to
 the backing file, but oVirt wants to manage a backing chain that
 uses just relative names, to allow for easier migration of a chain
 across storage locations.
 

 
  /* create the stub file and set selinux labels; manipulate disk in
   * place, in a way that can be reverted on failure. */
 -fd = qemuOpenFile(driver, source, O_WRONLY | O_TRUNC | O_CREAT,
 +fd = qemuOpenFile(driver, source,
 +  O_WRONLY | (reuse ? 0 : O_TRUNC | O_CREAT),
need_unlink, NULL);
  if (fd  0)
  goto cleanup;

I just realized this is a pointless open in the reuse case, since the
very next statement closes the fd (in other words, we are using open for
the side-effect of creation, but reuse says no creation is necessary).
I plan on squashing this:

diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index ca81e75..4286b2a 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -9884,12 +9884,13 @@ qemuDomainSnapshotCreateSingleDiskActive(struct
qemud_driver *driver,

 /* create the stub file and set selinux labels; manipulate disk in
  * place, in a way that can be reverted on failure. */
-fd = qemuOpenFile(driver, source,
-  O_WRONLY | (reuse ? 0 : O_TRUNC | O_CREAT),
-  need_unlink, NULL);
-if (fd  0)
-goto cleanup;
-VIR_FORCE_CLOSE(fd);
+if (!reuse) {
+fd = qemuOpenFile(driver, source, O_WRONLY | O_TRUNC | O_CREAT,
+  need_unlink, NULL);
+if (fd  0)
+goto cleanup;
+VIR_FORCE_CLOSE(fd);
+}

 origsrc = disk-src;
 disk-src = source;

-- 
Eric Blake   ebl...@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 1/1] Clarify what documentation is being referenced

2012-03-22 Thread Laine Stump

On 03/21/2012 10:07 PM, Dave Allan wrote:
 virsh.pod had several instances in which it referred to the
 documentation which was a little puzzling to me since it is
 documentation.  Reading the document from end to end makes it clear
 that it means a specific URI which was noted previously in the text,
 but I had never noticed those URIs in several years of referring to
 the man page.  This patch adds those URIs to several additional places
 in the text.
 ---
  tools/virsh.pod |   48 +++-
  1 files changed, 27 insertions(+), 21 deletions(-)

ACK.

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

Re: [libvirt] [Qemu-devel] Modern CPU models cannot be used with libvirt

2012-03-22 Thread Eduardo Habkost

On Thu, Mar 22, 2012 at 11:37:39AM -0500, Anthony Liguori wrote:
 On 03/22/2012 04:32 AM, Gleb Natapov wrote:
 On Tue, Mar 13, 2012 at 11:53:19AM -0300, Eduardo Habkost wrote:
 So, this problem is solved if the defaults are easily found on
 /usr/share.
 
 What problem is solved and why are we mixing machine configuration files
 and cpu configuration files? They are different and should be treated
 differently. -nodefconfig exists only because there is not machine
 configuration files currently. With machine configuration files
 libvirt does not need -nodefconfig because it can create its own machine
 file and make QEMU use it. So specifying machine file on QEMU's command
 line implies -nodefconfig. The option itself loses its meaning and can be
 dropped.
 
 No, -nodefconfig means no default config.
 
 As with many projects, we can have *some* configuration required.
 
 The default configure should have a:
 
 [system]
 readconfig=@SYSCONFDIR@/cpu-models-x86_64.cfg

Not @SYSCONFDIR@, but @DATADIR@. CPU models belong to /usr/share because
they aren't meant to be changed by the user (I think I already explained
why: because we have to be able to deploy fixes to them).

 
 Stanza by default.  If libvirt wants to reuse this, they can use
 -readconfig if they use -nodefconfig.

You are just repeating how you believe it should work based on the
premise that cpudefs are configuration. We're discussing/questioning
this exact premise, here, and I would really appreciate to hear why the
model Gleb is proposing is not valid.

More precisely, this part:

 cpu-models-x86.conf is not a configuration file. It is hardware
 description file. QEMU should not lose capability just because you run
 it with -nodefconfig. -nodefconfig means that QEMU does not create
 machine for you, but all parts needed to create a machine that would have
 been created without -nodefconfig are still present. Not been able to
 create Nehalem CPU after specifying -nodefconfig is the same as not been
 able to create virtio-net i.e the bug.

And the related points Gleb mentioned further in this thread.

-- 
Eduardo

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

[libvirt] [libvirt PATCHv6 1/1] add DHCP snooping

2012-03-22 Thread David L Stevens

This patch adds DHCP snooping support to libvirt. The learning method for
IP addresses is specified by setting the ip_learning variable to one of
any [default] (existing IP learning code), none (static only addresses)
or dhcp (DHCP snooping).

Active leases are saved in a lease file and reloaded on restart or HUP.

Changes since v5:
- use VMUUID+MAC to identify interfaces for leases
- use direct pthread_cancel to kill snooper threads to avoid races with
  re-used host interfaces

Signed-off-by: David L Stevens dlstev...@us.ibm.com
---
 docs/formatnwfilter.html.in|   17 +
 src/Makefile.am|2 +
 src/nwfilter/nwfilter_dhcpsnoop.c  | 1078 
 src/nwfilter/nwfilter_dhcpsnoop.h  |   38 ++
 src/nwfilter/nwfilter_driver.c |6 +
 src/nwfilter/nwfilter_gentech_driver.c |   59 ++-
 6 files changed, 1187 insertions(+), 13 deletions(-)
 create mode 100644 src/nwfilter/nwfilter_dhcpsnoop.c
 create mode 100644 src/nwfilter/nwfilter_dhcpsnoop.h

diff --git a/docs/formatnwfilter.html.in b/docs/formatnwfilter.html.in
index 9cb7644..ad10765 100644
--- a/docs/formatnwfilter.html.in
+++ b/docs/formatnwfilter.html.in
@@ -2189,6 +2189,23 @@
br/br/
In case a VM is resumed after suspension or migrated, IP address
detection will be restarted.
+   br/br/
+   Variable iip_learning/i may be used to specify
+   the IP address learning method. Valid values are iany/i, 
idhcp/i,
+   or inone/i. The default value is iany/i, meaning that libvirt
+   may use any packet to determine the address in use by a VM. A value of
+   idhcp/i specifies that libvirt should only honor DHCP 
server-assigned
+   addresses with valid leases. If iip_learning/i is set to 
inone/i,
+   libvirt does not do address learning and referencing iIP/i without
+   assigning it an explicit value is an error.
+   br/br/
+   Use of iip_learning=dhcp/i (DHCP snooping) provides additional
+   anti-spoofing security, especially when combined with a filter allowing
+   only trusted DHCP servers to assign addresses. If the DHCP lease 
expires,
+   the VM will no longer be able to use the IP address until it acquires a
+   new, valid lease from a DHCP server. If the VM is migrated, it must get
+   a new valid DHCP lease to use an IP address (e.g., by
+   bringing the VM interface down and up again).
  /p
 
 h3a name=nwflimitsmigrVM Migration/a/h3
diff --git a/src/Makefile.am b/src/Makefile.am
index e57eca2..4dc1609 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -508,6 +508,8 @@ NWFILTER_DRIVER_SOURCES =   
\
nwfilter/nwfilter_driver.h nwfilter/nwfilter_driver.c   \
nwfilter/nwfilter_gentech_driver.c  \
nwfilter/nwfilter_gentech_driver.h  \
+   nwfilter/nwfilter_dhcpsnoop.c   \
+   nwfilter/nwfilter_dhcpsnoop.h   \
nwfilter/nwfilter_ebiptables_driver.c   \
nwfilter/nwfilter_ebiptables_driver.h   \
nwfilter/nwfilter_learnipaddr.c \
diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c 
b/src/nwfilter/nwfilter_dhcpsnoop.c
new file mode 100644
index 000..682a4ab
--- /dev/null
+++ b/src/nwfilter/nwfilter_dhcpsnoop.c
@@ -0,0 +1,1078 @@
+/*
+ * nwfilter_dhcpsnoop.c: support for DHCP snooping used by a VM
+ * on an interface
+ *
+ * Copyright (C) 2011 IBM Corp.
+ * Copyright (C) 2011 David L Stevens
+ *
+ * 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, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307  USA
+ *
+ * Author: David L Stevens dlstev...@us.ibm.com
+ * Based in part on work by Stefan Berger stef...@us.ibm.com
+ */
+
+#include config.h
+
+#ifdef HAVE_LIBPCAP
+#include pcap.h
+#endif
+
+#include fcntl.h
+#include sys/ioctl.h
+#include signal.h
+
+#include arpa/inet.h
+#include net/ethernet.h
+#include netinet/ip.h
+#include netinet/udp.h
+#include net/if.h
+#include net/if_arp.h
+#include intprops.h
+
+#include internal.h
+
+#include buf.h
+#include memory.h
+#include logging.h
+#include datatypes.h
+#include virterror_internal.h
+#include

Re: [libvirt] Problem with Open vSwitch and dnsmasq

2012-03-22 Thread Ansis Atteka

On Thu, Mar 22, 2012 at 6:10 AM, Daniele Milani dano1...@hotmail.it wrote:

  Dear all,
 I have the following situation:
 -I replaced the standard bridge driver with the Open VSwitch one;
 -I started a NAT-network on Libvirt (bridge name virbr1);
 -I started a Virtual Machine (VM1) on Libvirt, and I tagged his interface
 (vnet0) with tag=2;
 -if I run # ovs-vsctl show I obtain:

 Bridge virbr1
 Port vnet0
 tag: 2
 Interface vnet0
 Port virbr1-nic
 Interface virb1-nic
 Port virbr1
 Interface virbr1
 type: internal

 -the problem is that it is impossible to assign to VM1 an IP, because the
 dnsmasq daemon does not accept the tagged DHCP Discover frame.

 Does someone know if there is a way for dnsmasq to accept tagged frames
 through virbr1, and send a tagged DHCP Offer packet back to VM1?

 I believe you would need to run dedicated dnsmasq process instance per
each VLAN that you have. By
default I guess dnsmasq runs on virbr1, hence it does not see the tagged
traffic that comes from vnet0.

You could try to:

   1. add another port to that bridge with the same VLAN as VM has. And run
   a separate instance of dnsmasq there; or
   2. change the tag of virb1 port, but this might lead to other issues
   (e.g. then non-tagged VMs will not get DHCP leases).

Perhaps someone else can suggest something easier...

Greetings,
 Daniele Milani


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

Re: [libvirt] Problem with Open vSwitch and dnsmasq

2012-03-22 Thread Daniele Milani


I think I could try the first solution. Can you explain me how do I create the 
port used by dnsmasq?

For example, is it correct to execute 
# ovs-vsctl add-port virbr1 port2 tag=2
to create a port for the vLan whose tag is 2 named port2?

Daniele Milani


Date: Thu, 22 Mar 2012 10:54:21 -0700
Subject: Re: [libvirt] Problem with Open vSwitch and dnsmasq
From: aatt...@nicira.com
To: dano1...@hotmail.it
CC: libvir-list@redhat.com; roberto.sa...@polito.it



On Thu, Mar 22, 2012 at 6:10 AM, Daniele Milani dano1...@hotmail.it wrote:





Dear all,
I have the following situation:
-I replaced the standard bridge driver with the Open VSwitch one;
-I started a NAT-network on Libvirt (bridge name virbr1);
-I started a Virtual Machine (VM1) on Libvirt, and I tagged his interface 
(vnet0) with tag=2;

-if I run # ovs-vsctl show I obtain:

Bridge virbr1
Port vnet0
tag: 2
Interface vnet0
Port virbr1-nic

Interface virb1-nic
Port virbr1
Interface virbr1
type: internal

-the problem is that it is impossible to assign to VM1 an IP, because the 
dnsmasq daemon does not accept the tagged DHCP Discover frame.


Does someone know if there is a way for dnsmasq to accept tagged frames through 
virbr1, and send a tagged DHCP Offer packet back to VM1?

I believe you would need to run dedicated dnsmasq process instance per each 
VLAN that you have. By

default I guess dnsmasq runs on virbr1, hence it does not see the tagged 
traffic that comes from vnet0.

You could try to:
add another port to that bridge with the same VLAN as VM has. And run a 
separate instance of dnsmasq there; or


change the tag of virb1 port, but this might lead to other issues (e.g. then 
non-tagged VMs will not get DHCP leases).
Perhaps someone else can suggest something easier...


Greetings,
Daniele Milani

  

--

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

Re: [libvirt] Problem with Open vSwitch and dnsmasq

2012-03-22 Thread Ansis Atteka

On Thu, Mar 22, 2012 at 11:11 AM, Daniele Milani dano1...@hotmail.itwrote:

  I think I could try the first solution. Can you explain me how do I
 create the port used by dnsmasq?

 For example, is it correct to execute
 # ovs-vsctl add-port virbr1 port2 tag=2
 to create a port for the vLan whose tag is 2 named port2?


Try something like this:

 ovs-vsctl add-port virbr1 port2 tag=2
 ovs-vsctl set Interface port2 type=internal
 ifconfig port2 10.0.0.1
 ifconfig port2 up
 /usr/sbin/dnsmasq --strict-order --bind-interfaces --except-interface lo
 --listen-address 10.0.0.1 --dhcp-range 10.0.0.10,10.0.0.20
 --dhcp-leasefile=/var/lib/libvirt/dnsmasq/vlan2.leases --dhcp-lease-max=253
 --dhcp-no-override

Though I have not tested it...


 Daniele Milani


 Date: Thu, 22 Mar 2012 10:54:21 -0700
 Subject: Re: [libvirt] Problem with Open vSwitch and dnsmasq
 From: aatt...@nicira.com
 To: dano1...@hotmail.it
 CC: libvir-list@redhat.com; roberto.sa...@polito.it




 On Thu, Mar 22, 2012 at 6:10 AM, Daniele Milani dano1...@hotmail.itwrote:

  Dear all,
 I have the following situation:
 -I replaced the standard bridge driver with the Open VSwitch one;
 -I started a NAT-network on Libvirt (bridge name virbr1);
 -I started a Virtual Machine (VM1) on Libvirt, and I tagged his interface
 (vnet0) with tag=2;
 -if I run # ovs-vsctl show I obtain:

 Bridge virbr1
 Port vnet0
 tag: 2
 Interface vnet0
 Port virbr1-nic
 Interface virb1-nic
 Port virbr1
 Interface virbr1
 type: internal

 -the problem is that it is impossible to assign to VM1 an IP, because the
 dnsmasq daemon does not accept the tagged DHCP Discover frame.

 Does someone know if there is a way for dnsmasq to accept tagged frames
 through virbr1, and send a tagged DHCP Offer packet back to VM1?

 I believe you would need to run dedicated dnsmasq process instance per
 each VLAN that you have. By
 default I guess dnsmasq runs on virbr1, hence it does not see the tagged
 traffic that comes from vnet0.

 You could try to:

1. add another port to that bridge with the same VLAN as VM has. And
run a separate instance of dnsmasq there; or




 change the tag of virb1 port, but this might lead to other issues (e.g.
 then non-tagged VMs will not get DHCP leases).
 Perhaps someone else can suggest something easier...

 Greetings,
 Daniele Milani


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

Re: [libvirt] Problem with Open vSwitch and dnsmasq

2012-03-22 Thread Laine Stump

On 03/22/2012 09:10 AM, Daniele Milani wrote:
 Dear all,
 I have the following situation:
 -I replaced the standard bridge driver with the Open VSwitch one;

I'm curious what you mean by this.

libvirt's virtual networks currently only support the standard linux
bridge device. There is no code in libvirt to use openvswitch devices
for libvirt's virtual networks. The openvswitch support in libvirt only
applies to guest devices that use interface type='bridge'.

From your message, it sounds like there is a driver that that replaces
the linux host bridge driver with something that emulates that driver,
but is actually an openvswitch bridge on the backend. I hadn't
previously heard of this.


 -I started a NAT-network on Libvirt (bridge name virbr1);
 -I started a Virtual Machine (VM1) on Libvirt, and I tagged his
 interface (vnet0) with tag=2;
 -if I run # ovs-vsctl show I obtain:

 Bridge virbr1
 Port vnet0
 tag: 2
 Interface vnet0
 Port virbr1-nic
 Interface virb1-nic
 Port virbr1
 Interface virbr1
 type: internal

 -the problem is that it is impossible to assign to VM1 an IP, because
 the dnsmasq daemon does not accept the tagged DHCP Discover frame.

 Does someone know if there is a way for dnsmasq to accept tagged
 frames through virbr1, and send a tagged DHCP Offer packet back to VM1?


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

Re: [libvirt] Problem with Open vSwitch and dnsmasq

2012-03-22 Thread Laine Stump

(Cc'ing to libvirt-us...@redhat.com)

On 03/22/2012 02:43 PM, Ansis Atteka wrote:


 On Thu, Mar 22, 2012 at 11:11 AM, Daniele Milani dano1...@hotmail.it
 mailto:dano1...@hotmail.it wrote:

 I think I could try the first solution. Can you explain me how do
 I create the port used by dnsmasq?

 For example, is it correct to execute
 # ovs-vsctl add-port virbr1 port2 tag=2
 to create a port for the vLan whose tag is 2 named port2?


 Try something like this:

 ovs-vsctl add-port virbr1 port2 tag=2
 ovs-vsctl set Interface port2 type=internal
 ifconfig port2 10.0.0.1
 ifconfig port2 up
 /usr/sbin/dnsmasq --strict-order --bind-interfaces
 --except-interface lo --listen-address 10.0.0.1 --dhcp-range
 10.0.0.10,10.0.0.20
 --dhcp-leasefile=/var/lib/libvirt/dnsmasq/vlan2.leases
 --dhcp-lease-max=253 --dhcp-no-override


By the time you are doing this much stuff outside of libvirt, I think it
would really be better to not use a libvirt virtual network at all,
since libvirt is likely to do things to that network that would end up
breaking the config you've done outside of libvirt.

Instead, just use the host OS configuration files to create the bridge,
add the tagged port, run dnsmasq, etc., then use interface
type='bridge' in the guest rather than interface type='network'


 Though I have not tested it...


 Daniele Milani


 Date: Thu, 22 Mar 2012 10:54:21 -0700
 Subject: Re: [libvirt] Problem with Open vSwitch and dnsmasq
 From: aatt...@nicira.com mailto:aatt...@nicira.com
 To: dano1...@hotmail.it mailto:dano1...@hotmail.it
 CC: libvir-list@redhat.com mailto:libvir-list@redhat.com;
 roberto.sa...@polito.it mailto:roberto.sa...@polito.it




 On Thu, Mar 22, 2012 at 6:10 AM, Daniele Milani
 dano1...@hotmail.it mailto:dano1...@hotmail.it wrote:

 Dear all,
 I have the following situation:
 -I replaced the standard bridge driver with the Open VSwitch one;
 -I started a NAT-network on Libvirt (bridge name virbr1);
 -I started a Virtual Machine (VM1) on Libvirt, and I tagged
 his interface (vnet0) with tag=2;
 -if I run # ovs-vsctl show I obtain:

 Bridge virbr1
 Port vnet0
 tag: 2
 Interface vnet0
 Port virbr1-nic
 Interface virb1-nic
 Port virbr1
 Interface virbr1
 type: internal

 -the problem is that it is impossible to assign to VM1 an IP,
 because the dnsmasq daemon does not accept the tagged DHCP
 Discover frame.

 Does someone know if there is a way for dnsmasq to accept
 tagged frames through virbr1, and send a tagged DHCP Offer
 packet back to VM1?

 I believe you would need to run dedicated dnsmasq process instance
 per each VLAN that you have. By
 default I guess dnsmasq runs on virbr1, hence it does not see the
 tagged traffic that comes from vnet0.

 You could try to:

  1. add another port to that bridge with the same VLAN as VM has.
 And run a separate instance of dnsmasq there; or




 change the tag of virb1 port, but this might lead to other issues
 (e.g. then non-tagged VMs will not get DHCP leases).
 Perhaps someone else can suggest something easier...

 Greetings,
 Daniele Milani


 --
 libvir-list mailing list
 libvir-list@redhat.com mailto: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

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

Re: [libvirt] [libvirt PATCHv6 1/1] add DHCP snooping

2012-03-22 Thread Stefan Berger

David Stevens/Beaverton/IBM@IBMUS wrote on 03/22/2012 01:27:10 PM:


 
 This patch adds DHCP snooping support to libvirt. The learning method 
for
 IP addresses is specified by setting the ip_learning variable to one 
of
 any [default] (existing IP learning code), none (static only 
addresses)
 or dhcp (DHCP snooping).
 
 Active leases are saved in a lease file and reloaded on restart or HUP.
 
 Changes since v5:
 - use VMUUID+MAC to identify interfaces for leases
 - use direct pthread_cancel to kill snooper threads to avoid races with
   re-used host interfaces

I tried it. It doesn't apply more than one IP address. The code also 
doesn't apply cleanly to the tip.

   Stefan

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

[libvirt] clarify freecell documentation

2012-03-22 Thread Dave Allan


There were a couple of minor inaccuracies in the freecell manpage and
virsh help.  The first patch fixes the manpage and the second, to
virsh.c, attempts to fix the help output.  The virsh.c patch appears
to produce the correct output, but is pure cargo cult programming, so
it should be very carefully reviewed.

Dave

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

Re: [libvirt] [Qemu-devel] Modern CPU models cannot be used with libvirt

2012-03-22 Thread Anthony Liguori


On 03/22/2012 12:14 PM, Eduardo Habkost wrote:

On Thu, Mar 22, 2012 at 11:37:39AM -0500, Anthony Liguori wrote:

On 03/22/2012 04:32 AM, Gleb Natapov wrote:

On Tue, Mar 13, 2012 at 11:53:19AM -0300, Eduardo Habkost wrote:

So, this problem is solved if the defaults are easily found on
/usr/share.


What problem is solved and why are we mixing machine configuration files
and cpu configuration files? They are different and should be treated
differently. -nodefconfig exists only because there is not machine
configuration files currently. With machine configuration files
libvirt does not need -nodefconfig because it can create its own machine
file and make QEMU use it. So specifying machine file on QEMU's command
line implies -nodefconfig. The option itself loses its meaning and can be
dropped.


No, -nodefconfig means no default config.

As with many projects, we can have *some* configuration required.

The default configure should have a:

[system]
readconfig=@SYSCONFDIR@/cpu-models-x86_64.cfg


Not @SYSCONFDIR@, but @DATADIR@. CPU models belong to /usr/share because
they aren't meant to be changed by the user (I think I already explained
why: because we have to be able to deploy fixes to them).



Stanza by default.  If libvirt wants to reuse this, they can use
-readconfig if they use -nodefconfig.


You are just repeating how you believe it should work based on the
premise that cpudefs are configuration. We're discussing/questioning
this exact premise, here, and I would really appreciate to hear why the
model Gleb is proposing is not valid.

More precisely, this part:


cpu-models-x86.conf is not a configuration file. It is hardware
description file. QEMU should not lose capability just because you run
it with -nodefconfig. -nodefconfig means that QEMU does not create
machine for you, but all parts needed to create a machine that would have
been created without -nodefconfig are still present. Not been able to
create Nehalem CPU after specifying -nodefconfig is the same as not been
able to create virtio-net i.e the bug.


And the related points Gleb mentioned further in this thread.


Because the next patch series that would follow would be a -cpu-defs-path that 
would be a one-off hack with a global variable and a -no-cpu-defs.


So let's avoid that and start by having a positive configuration mechanism that 
the user can use to change the path and exclude it.  My suggestion eliminate the 
need for two future command line options.


Regards,

Anthony Liguori





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

[libvirt] [PATCH 2/2] Indicate freecell --cellno is optional

2012-03-22 Thread Dave Allan

---
 tools/virsh.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c
index 9e5c9b2..d9cff0c 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -4742,7 +4742,7 @@ static const vshCmdInfo info_freecell[] = {
 };

 static const vshCmdOptDef opts_freecell[] = {
-{cellno, VSH_OT_INT, 0, N_(NUMA cell number)},
+{cellno, VSH_OT_INT, VSH_OFLAG_REQ, N_(NUMA cell number)},
 {all, VSH_OT_BOOL, 0, N_(show free memory for all NUMA cells)},
 {NULL, 0, 0, NULL}
 };
-- 
1.7.7.6

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

Re: [libvirt] [PATCH 2/2] Indicate freecell --cellno is optional

2012-03-22 Thread Eric Blake

On 03/22/2012 01:59 PM, Dave Allan wrote:
 ---
  tools/virsh.c |2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)
 
 diff --git a/tools/virsh.c b/tools/virsh.c
 index 9e5c9b2..d9cff0c 100644
 --- a/tools/virsh.c
 +++ b/tools/virsh.c
 @@ -4742,7 +4742,7 @@ static const vshCmdInfo info_freecell[] = {
  };
 
  static const vshCmdOptDef opts_freecell[] = {
 -{cellno, VSH_OT_INT, 0, N_(NUMA cell number)},
 +{cellno, VSH_OT_INT, VSH_OFLAG_REQ, N_(NUMA cell number)},

NACK.  VSH_OFLAG_REQ means required, absence of that flag (ie. using 0
for the flag) means optional.  This patch would break the command by
requiring a --cellno argument, even with --all.

This is the current 'virsh help freecell' output, without your patch:

$ virsh help freecell
  NAME
freecell - NUMA free memory

  SYNOPSIS
freecell [--cellno number] [--all]

  DESCRIPTION
display available free memory for the NUMA cell.

  OPTIONS
--cellno number  NUMA cell number
--allshow free memory for all NUMA cells

It shows that both --cellno and --all are optional; however, what it
does not show (and cannot show, without a lot more work throughout
virsh), is the notion of mutual exclusion (that is, there is no trivial
way to make virsh help output the {} operators to show the alternation
that the virsh.pod has by hand).

-- 
Eric Blake   ebl...@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 1/1] Clarify what documentation is being referenced

2012-03-22 Thread Laine Stump

On 03/22/2012 12:56 PM, Laine Stump wrote:
 On 03/21/2012 10:07 PM, Dave Allan wrote:
 virsh.pod had several instances in which it referred to the
 documentation which was a little puzzling to me since it is
 documentation.  Reading the document from end to end makes it clear
 that it means a specific URI which was noted previously in the text,
 but I had never noticed those URIs in several years of referring to
 the man page.  This patch adds those URIs to several additional places
 in the text.
 ---
  tools/virsh.pod |   48 +++-
  1 files changed, 27 insertions(+), 21 deletions(-)
 ACK.


Also pushed.

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

Re: [libvirt] [PATCH 1/2] Clarify virsh freecell manpage entry

2012-03-22 Thread Eric Blake

On 03/22/2012 01:59 PM, Dave Allan wrote:
 ---
  tools/virsh.pod |   23 ++-
  1 files changed, 18 insertions(+), 5 deletions(-)
 
 diff --git a/tools/virsh.pod b/tools/virsh.pod
 index f0df4fd..b4deae8 100644
 --- a/tools/virsh.pod
 +++ b/tools/virsh.pod
 @@ -401,11 +401,24 @@ Bvirsh list --title
0 Domain-0 runningMailserver 1
2 fedora   paused
 
 -=item Bfreecell [Bcellno | I--all]
 -
 -Prints the available amount of memory on the machine or within a
 -NUMA cell if Icellno is provided.  If I--all is provided instead
 -of I--cellno, then show the information on all NUMA cells.
 +=item Bfreecell [{ [I--cellno] Bcellno | I--all }]
 +
 +Prints the available amount of memory on the machine or within a NUMA
 +cell.  The freecell command can provide one of three different
 +displays of available memory on the machine depending on the options
 +specified.  With no options, it displays the total free memory on the
 +machine.  With the --all option, it displays the free memory on each
 +node and the total free memory on the machine.  Finally, with a
 +numeric argument or with --cellno it will display the free memory for
 +that node only.

I like this part.

 +
 + For example:
 +
 +  freecell= total free memory on the machine
 +  freecell --all  = free memory on each node, and the total
 +  freecell --cellno 0 = free memory on node 0 only
 +  freecell 0  = free memory on node 0 only
 +  freecell --cellno=0 = free memory on node 0 only

This may be a bit verbose; we aren't giving this many examples elsewhere
in the man page.  I'm not entirely opposed to verbosity, if it helps
silence confusion, but if others agree to keep this many examples, can
we at least add some variety, as in:

freecell 1  = free memory on node 1 only

-- 
Eric Blake   ebl...@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] [libvirt PATCHv6 1/1] add DHCP snooping

2012-03-22 Thread David Stevens

Stefan Berger/Watson/IBM wrote on 03/22/2012 12:22:20 PM:

 
 I tried it. It doesn't apply more than one IP address. The code also
 doesn't apply cleanly to the tip.
 
Stefan

Stefan,
I did a git pull yesterday to which this patch is
applied; here is the last entry before the patch:

commit 25fb4c65a54e3c34c8084b2d49b888d11685a973
Author: Eric Blake ebl...@redhat.com
Date:   Tue Mar 20 17:04:38 2012 -0600

build: drop a painfully long gnulib test

Am I using the wrong git tree, or can you be
more specific about the errors you're seeing?
The last I looked at this there was no multiple
address support and this code is not attempting to add it.
If you're saying that ip_learning, which this optionally substitutes
for, now supports multiple addresses, I can look at adding
it. The goal of this patch is simply to use only valid
DHCP leases as the basis for anti-spoofing rules as a more
secure method than the existing learning code.

+-DLS

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

Re: [libvirt] [PATCH 1/2] Clarify virsh freecell manpage entry

2012-03-22 Thread Dave Allan

On Thu, Mar 22, 2012 at 02:13:07PM -0600, Eric Blake wrote:
 On 03/22/2012 01:59 PM, Dave Allan wrote:
  ---
   tools/virsh.pod |   23 ++-
   1 files changed, 18 insertions(+), 5 deletions(-)
  
  diff --git a/tools/virsh.pod b/tools/virsh.pod
  index f0df4fd..b4deae8 100644
  --- a/tools/virsh.pod
  +++ b/tools/virsh.pod
  @@ -401,11 +401,24 @@ Bvirsh list --title
 0 Domain-0 runningMailserver 1
 2 fedora   paused
  
  -=item Bfreecell [Bcellno | I--all]
  -
  -Prints the available amount of memory on the machine or within a
  -NUMA cell if Icellno is provided.  If I--all is provided instead
  -of I--cellno, then show the information on all NUMA cells.
  +=item Bfreecell [{ [I--cellno] Bcellno | I--all }]
  +
  +Prints the available amount of memory on the machine or within a NUMA
  +cell.  The freecell command can provide one of three different
  +displays of available memory on the machine depending on the options
  +specified.  With no options, it displays the total free memory on the
  +machine.  With the --all option, it displays the free memory on each
  +node and the total free memory on the machine.  Finally, with a
  +numeric argument or with --cellno it will display the free memory for
  +that node only.
 
 I like this part.
 
  +
  + For example:
  +
  +  freecell= total free memory on the machine
  +  freecell --all  = free memory on each node, and the total
  +  freecell --cellno 0 = free memory on node 0 only
  +  freecell 0  = free memory on node 0 only
  +  freecell --cellno=0 = free memory on node 0 only
 
 This may be a bit verbose; we aren't giving this many examples elsewhere
 in the man page.  I'm not entirely opposed to verbosity, if it helps
 silence confusion, but if others agree to keep this many examples, can
 we at least add some variety, as in:

That's a good point, and I think it's clear enough without it; I'll
drop that bit and resubmit.

Dave

 freecell 1  = free memory on node 1 only
 
 -- 
 Eric Blake   ebl...@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

Re: [libvirt] [PATCH 2/2] Indicate freecell --cellno is optional

2012-03-22 Thread Dave Allan

On Thu, Mar 22, 2012 at 02:10:30PM -0600, Eric Blake wrote:
 On 03/22/2012 01:59 PM, Dave Allan wrote:
  ---
   tools/virsh.c |2 +-
   1 files changed, 1 insertions(+), 1 deletions(-)
  
  diff --git a/tools/virsh.c b/tools/virsh.c
  index 9e5c9b2..d9cff0c 100644
  --- a/tools/virsh.c
  +++ b/tools/virsh.c
  @@ -4742,7 +4742,7 @@ static const vshCmdInfo info_freecell[] = {
   };
  
   static const vshCmdOptDef opts_freecell[] = {
  -{cellno, VSH_OT_INT, 0, N_(NUMA cell number)},
  +{cellno, VSH_OT_INT, VSH_OFLAG_REQ, N_(NUMA cell number)},
 
 NACK.  VSH_OFLAG_REQ means required, absence of that flag (ie. using 0
 for the flag) means optional.  This patch would break the command by
 requiring a --cellno argument, even with --all.
 
 This is the current 'virsh help freecell' output, without your patch:
 
 $ virsh help freecell
   NAME
 freecell - NUMA free memory
 
   SYNOPSIS
 freecell [--cellno number] [--all]
 
   DESCRIPTION
 display available free memory for the NUMA cell.
 
   OPTIONS
 --cellno number  NUMA cell number
 --allshow free memory for all NUMA cells
 
 It shows that both --cellno and --all are optional; however, what it
 does not show (and cannot show, without a lot more work throughout
 virsh), is the notion of mutual exclusion (that is, there is no trivial
 way to make virsh help output the {} operators to show the alternation
 that the virsh.pod has by hand).

Ok, that's what I was afraid someone was going to say.  We can kill
that patch as far as I'm concerned.

Dave


 -- 
 Eric Blake   ebl...@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] clarify freecell documentation v2

2012-03-22 Thread Dave Allan


Modified per Eric's feedback: removed incorrect virsh help change,
removed unnecessary examples in the manpage.  I also fixed two
instances in which I referred to nodes instead of cells.

Dave

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

[libvirt] [PATCH 1/1] Clarify virsh freecell manpage entry

2012-03-22 Thread Dave Allan

---
 tools/virsh.pod |   15 ++-
 1 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/tools/virsh.pod b/tools/virsh.pod
index f0df4fd..a8bd739 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -401,11 +401,16 @@ Bvirsh list --title
   0 Domain-0 runningMailserver 1
   2 fedora   paused

-=item Bfreecell [Bcellno | I--all]
-
-Prints the available amount of memory on the machine or within a
-NUMA cell if Icellno is provided.  If I--all is provided instead
-of I--cellno, then show the information on all NUMA cells.
+=item Bfreecell [{ [I--cellno] Bcellno | I--all }]
+
+Prints the available amount of memory on the machine or within a NUMA
+cell.  The freecell command can provide one of three different
+displays of available memory on the machine depending on the options
+specified.  With no options, it displays the total free memory on the
+machine.  With the --all option, it displays the free memory in each
+cell and the total free memory on the machine.  Finally, with a
+numeric argument or with --cellno plus a cell number it will display
+the free memory for the specified cell only.

 =item Bcpu-baseline IFILE

-- 
1.7.7.6

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

Re: [libvirt] [libvirt PATCHv6 1/1] add DHCP snooping

2012-03-22 Thread Stefan Berger

David Stevens/Beaverton/IBM wrote on 03/22/2012 04:22:55 PM:

 From: David Stevens/Beaverton/IBM
 To: Stefan Berger/Watson/IBM
 Cc: Daniel P. Berrange berra...@redhat.com, libvir-list@redhat.com
 Date: 03/22/2012 04:23 PM
 Subject: Re: [libvirt PATCHv6 1/1] add DHCP snooping

 Stefan Berger/Watson/IBM wrote on 03/22/2012 12:22:20 PM:

  I tried it. It doesn't apply more than one IP address. The code also
  doesn't apply cleanly to the tip.

 Stefan

 Stefan,
  I did a git pull yesterday to which this patch is
 applied; here is the last entry before the patch:

 commit 25fb4c65a54e3c34c8084b2d49b888d11685a973
 Author: Eric Blake ebl...@redhat.com
 Date:   Tue Mar 20 17:04:38 2012 -0600

 build: drop a painfully long gnulib test

Maybe it was my fault while copying and pasting from email app into a VNC 
session and tabs got distorted ...

I have some concerns about the cancelation of the thread. It can hold the 
snoop lock and get cancelled while holding it. Next time that lock is 
grabbed we will get a deadlock...

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

Re: [libvirt] [libvirt PATCHv6 1/1] add DHCP snooping

2012-03-22 Thread David Stevens

Stefan Berger/Watson/IBM wrote on 03/22/2012 03:04:53 PM:

 
 I have some concerns about the cancelation of the thread. It can 
 hold the snoop lock and get cancelled while holding it. Next time 
 that lock is grabbed we will get a deadlock...


The snoop lock is acquired in virNWFilterDHCPSnoopEnd(), which
is where the pthread_cancel() directly (for valid leases) or the
freeReq()/pthread_cancel() is done. So, the canceler has the lock
and the canceling thread can't have it, then.

The only other case I see is when the config goes invalid and
we exit the snooper loop; he frees the snoop_lock() before removing
his own hash entry,  which will cancel the same thread doing
the remove. Again, it can't have the snoop lock when canceled.

Is there some other case you think I'm missing?

+-DLS

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

Re: [libvirt] [libvirt PATCHv6 1/1] add DHCP snooping

2012-03-22 Thread Eric Blake

On 03/22/2012 04:49 PM, David Stevens wrote:
 Stefan Berger/Watson/IBM wrote on 03/22/2012 03:04:53 PM:
 

 I have some concerns about the cancelation of the thread. It can 
 hold the snoop lock and get cancelled while holding it. Next time 
 that lock is grabbed we will get a deadlock...

 
 The snoop lock is acquired in virNWFilterDHCPSnoopEnd(), which
 is where the pthread_cancel() directly (for valid leases) or the
 freeReq()/pthread_cancel() is done. So, the canceler has the lock
 and the canceling thread can't have it, then.
 
 The only other case I see is when the config goes invalid and
 we exit the snooper loop; he frees the snoop_lock() before removing
 his own hash entry,  which will cancel the same thread doing
 the remove. Again, it can't have the snoop lock when canceled.
 
 Is there some other case you think I'm missing?

pthread_cancel() tends to imply that you are properly managing signal
blocking across threads; we haven't used it anywhere else in libvirt,
and I'm extremely wary of pulling it in now, as there's probably a lot
of subtle bugs that it would expose.  Are you sure you can't do this in
some other manner without dragging in pthread_cancel()?

-- 
Eric Blake   ebl...@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] [libvirt PATCHv6 1/1] add DHCP snooping

2012-03-22 Thread David Stevens

Eric Blake ebl...@redhat.com wrote on 03/22/2012 03:54:31 PM:

 
 pthread_cancel() tends to imply that you are properly managing signal
 blocking across threads; we haven't used it anywhere else in libvirt,
 and I'm extremely wary of pulling it in now, as there's probably a lot
 of subtle bugs that it would expose.  Are you sure you can't do this in
 some other manner without dragging in pthread_cancel()?

Well, I was trying to avoid it in the earlier versions, but we ran
into races where a new snooper thread could start up on the same interface
before the old one woke up and noticed it was supposed to die; the old
thread would then interfere with the new thread in unpleasant ways.

I certainly had no problems with it during my testing. In this case,
an asynchronous signal to kill an otherwise blocked-on-a-read thread
is exactly what we need.

That said, I think it's possible to avoid it, but using it greatly
simplified the restart and reload cases. If you think it's too much
risk, I can take a shot at reworking that to avoid it again.

+-DLS

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

Re: [libvirt] [libvirt PATCHv6 1/1] add DHCP snooping

2012-03-22 Thread Stefan Berger

David Stevens/Beaverton/IBM wrote on 03/22/2012 07:26:06 PM:

 From: David Stevens/Beaverton/IBM
 To: Eric Blake ebl...@redhat.com
 Cc: libvir-list@redhat.com, Stefan Berger/Watson/IBM@IBMUS
 Date: 03/22/2012 07:26 PM
 Subject: Re: [libvirt] [libvirt PATCHv6 1/1] add DHCP snooping

 Eric Blake ebl...@redhat.com wrote on 03/22/2012 03:54:31 PM:

  pthread_cancel() tends to imply that you are properly managing signal
  blocking across threads; we haven't used it anywhere else in libvirt,
  and I'm extremely wary of pulling it in now, as there's probably a lot
  of subtle bugs that it would expose.  Are you sure you can't do this 
in
  some other manner without dragging in pthread_cancel()?

 Well, I was trying to avoid it in the earlier versions, but we ran
 into races where a new snooper thread could start up on the same 
interface
 before the old one woke up and noticed it was supposed to die; the old
 thread would then interfere with the new thread in unpleasant ways.

Right. I had created several patches on top of your previous code. From 
what I remember pretty much all scenarios were working: SIGHUP, multiple 
IP addresses, suspend/resume... Some of the code that became necessary due 
to the interaction of the threads really wasn't 'nice' (tricky)...

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

Re: [libvirt] [libvirt PATCHv6 1/1] add DHCP snooping

2012-03-22 Thread David Stevens

Stefan Berger/Watson/IBM wrote on 03/22/2012 05:00:45 PM:

 Maybe we should go with the previous code from a while ago which was
 setting a flag for the thread to die. It caused other work-arounds 
 to become necessary but at least we don't have to deal with possibly
 async. deaths of threads holding locks.

Yes, I have in mind a way to do this now that should keep the
simplicity and still not use signals. I'll try this out and
repost.

+-DLS


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

Re: [libvirt] [libvirt PATCHv6 1/1] add DHCP snooping

2012-03-22 Thread Stefan Berger

Eric Blake ebl...@redhat.com wrote on 03/22/2012 06:54:31 PM:


 
 On 03/22/2012 04:49 PM, David Stevens wrote:
  Stefan Berger/Watson/IBM wrote on 03/22/2012 03:04:53 PM:
  
 
  I have some concerns about the cancelation of the thread. It can 
  hold the snoop lock and get cancelled while holding it. Next time 
  that lock is grabbed we will get a deadlock...
 
  
  The snoop lock is acquired in virNWFilterDHCPSnoopEnd(), which
  is where the pthread_cancel() directly (for valid leases) or the
  freeReq()/pthread_cancel() is done. So, the canceler has the lock
  and the canceling thread can't have it, then.
  
  The only other case I see is when the config goes invalid and
  we exit the snooper loop; he frees the snoop_lock() before removing
  his own hash entry,  which will cancel the same thread doing
  the remove. Again, it can't have the snoop lock when canceled.
  
  Is there some other case you think I'm missing?
 
 pthread_cancel() tends to imply that you are properly managing signal
 blocking across threads; we haven't used it anywhere else in libvirt,
 and I'm extremely wary of pulling it in now, as there's probably a lot
 of subtle bugs that it would expose.  Are you sure you can't do this in
 some other manner without dragging in pthread_cancel()?

From the patch:

+if (pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, tmp)  0)
+virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, 
pthread_setcanceltype 
+   failed; ifname \%s\, req-ifname ?
+   req-ifname : NULL);

from the man page of above function:

  PTHREAD_CANCEL_ASYNCHRONOUS
  The  thread can be canceled at any time.  (Typically, it 
will be
  canceled immediately upon receiving a cancellation request, 
but
  the system doesn't guarantee this.)

It seems implementation dependent on when the thread actually die...
So I can construct a scenario where the canceller holds the lock and the 
thread doesn't, the thread gets cancelled but not immediately and can 
still grab the lock and then dies.  This is what I concluded to have seen 
from below stack trace when the snooping thread was running and in rapid 
succession I was shooting two SIGHUPs at libvirt:

Thread 11 (Thread 0x7f3c29ce8700 (LWP 12800)):
#0  0x0038ec60b3b4 in pthread_cond_wait@@GLIBC_2.3.2 ()
   from /lib64/libpthread.so.0
#1  0x7f3c2fe2229a in virCondWait (c=value optimized out, 
m=value optimized out) at util/threads-pthread.c:117
#2  0x7f3c2fe2295b in virThreadPoolWorker (opaque=value optimized 
out)
at util/threadpool.c:103
#3  0x7f3c2fe21f36 in virThreadHelper (data=value optimized out)
at util/threads-pthread.c:161
#4  0x0038ec606ccb in start_thread () from /lib64/libpthread.so.0
#5  0x0038ec2e0c2d in clone () from /lib64/libc.so.6

Thread 10 (Thread 0x7f3c294e7700 (LWP 12801)):
#0  0x0038ec60b3b4 in pthread_cond_wait@@GLIBC_2.3.2 ()
   from /lib64/libpthread.so.0
#1  0x7f3c2fe2229a in virCondWait (c=value optimized out, 
m=value optimized out) at util/threads-pthread.c:117
#2  0x7f3c2fe2295b in virThreadPoolWorker (opaque=value optimized 
out)
at util/threadpool.c:103
#3  0x7f3c2fe21f36 in virThreadHelper (data=value optimized out)
at util/threads-pthread.c:161
#4  0x0038ec606ccb in start_thread () from /lib64/libpthread.so.0
---Type return to continue, or q return to quit---
#5  0x0038ec2e0c2d in clone () from /lib64/libc.so.6

Thread 9 (Thread 0x7f3c28ce6700 (LWP 12802)):
#0  0x0038ec60b3b4 in pthread_cond_wait@@GLIBC_2.3.2 ()
   from /lib64/libpthread.so.0
#1  0x7f3c2fe2229a in virCondWait (c=value optimized out, 
m=value optimized out) at util/threads-pthread.c:117
#2  0x7f3c2fe2295b in virThreadPoolWorker (opaque=value optimized 
out)
at util/threadpool.c:103
#3  0x7f3c2fe21f36 in virThreadHelper (data=value optimized out)
at util/threads-pthread.c:161
#4  0x0038ec606ccb in start_thread () from /lib64/libpthread.so.0
#5  0x0038ec2e0c2d in clone () from /lib64/libc.so.6

Thread 8 (Thread 0x7f3c284e5700 (LWP 12803)):
#0  0x0038ec60b3b4 in pthread_cond_wait@@GLIBC_2.3.2 ()
   from /lib64/libpthread.so.0
#1  0x7f3c2fe2229a in virCondWait (c=value optimized out, 
m=value optimized out) at util/threads-pthread.c:117
#2  0x7f3c2fe2295b in virThreadPoolWorker (opaque=value optimized 
out)
at util/threadpool.c:103
#3  0x7f3c2fe21f36 in virThreadHelper (data=value optimized out)
at util/threads-pthread.c:161
---Type return to continue, or q return to quit---
#4  0x0038ec606ccb in start_thread () from /lib64/libpthread.so.0
#5  0x0038ec2e0c2d in clone () from /lib64/libc.so.6

Thread 7 (Thread 0x7f3c27ce4700 (LWP 12804)):
#0  0x0038ec60b3b4 in pthread_cond_wait@@GLIBC_2.3.2 ()
   from /lib64/libpthread.so.0
#1  0x7f3c2fe2229a in virCondWait (c=value optimized out, 
m=value optimized out) at util/threads-pthread.c:117
#2

Re: [libvirt] [libvirt PATCHv6 1/1] add DHCP snooping

2012-03-22 Thread Stefan Berger

David Stevens/Beaverton/IBM wrote on 03/22/2012 08:10:44 PM:

 From: David Stevens/Beaverton/IBM
 To: Stefan Berger/Watson/IBM
 Cc: Eric Blake ebl...@redhat.com, libvir-list@redhat.com
 Date: 03/22/2012 08:10 PM
 Subject: Re: [libvirt] [libvirt PATCHv6 1/1] add DHCP snooping

 Stefan Berger/Watson/IBM wrote on 03/22/2012 05:00:45 PM:

  Maybe we should go with the previous code from a while ago which was
  setting a flag for the thread to die. It caused other work-arounds 
  to become necessary but at least we don't have to deal with possibly
  async. deaths of threads holding locks.

 Yes, I have in mind a way to do this now that should keep the
 simplicity and still not use signals. I'll try this out and
 repost.

Ok.
An idea may be that the threat has to 'find' its snoop request in a global 
list every time it processes a packet. Once it cannot find it anymore, it 
dies. Removing the request from the global list would be the way to 
terminate the threat. Also, it would have to hold a look to the snoop 
request while it does anything else than waiting for packets in the pcap 
library.

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

Re: [libvirt] [libvirt PATCHv6 1/1] add DHCP snooping

2012-03-22 Thread David Stevens

Stefan Berger/Watson/IBM wrote on 03/22/2012 05:33:41 PM:

 
 Ok.
 An idea may be that the threat has to 'find' its snoop request in a 
 global list every time it processes a packet. Once it cannot find it
 anymore, it dies. Removing the request from the global list would be
 the way to terminate the threat. Also, it would have to hold a look 
 to the snoop request while it does anything else than waiting for 
 packets in the pcap library.

Actually, that's exactly what I was going to do -- a hash list
of valid threads and exit if it isn't in the list; then still remove
the req's and free them as the current code does, which means they
won't interfere with each other, but the cancel code can be separated,
in the same place, but synchronous with no signal; Thread management
independent of req management.

+_DLS

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

[libvirt] [PATCH] libvirt:docs: fix typo

2012-03-22 Thread Zhou Peng

---
 docs/formatdomain.html.in |4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 4edada3..3a504a1 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2903,8 +2903,8 @@ qemu-kvm -net nic,model=? /dev/null
   0.9.3/span.
 /p
 p
-  Mouse mode is set by the codemousecode/ element,
-  setting it's codemodecode/ attribute to one of
+  Mouse mode is set by the codemouse/code element,
+  setting it's codemode/code attribute to one of
   codeserver/code or codeclient/code ,
   span class=sincesince 0.9.11/span. If no mode is
   specified, the qemu default will be used (client mode).
-- 
1.7.7.6

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

Re: [libvirt] [PATCH] libvirt:docs: fix typo

2012-03-22 Thread Eric Blake

On 03/22/2012 08:40 PM, Zhou Peng wrote:

Commit message could have mentioned the typo; it took me three reads to
see what you changed.

 ---
  docs/formatdomain.html.in |4 ++--
  1 files changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
 index 4edada3..3a504a1 100644
 --- a/docs/formatdomain.html.in
 +++ b/docs/formatdomain.html.in
 @@ -2903,8 +2903,8 @@ qemu-kvm -net nic,model=? /dev/null
0.9.3/span.
  /p
  p
 -  Mouse mode is set by the codemousecode/ element,
 -  setting it's codemodecode/ attribute to one of
 +  Mouse mode is set by the codemouse/code element,
 +  setting it's codemode/code attribute to one of

And while fixing the code/ typo, I'm also s/it's/its/ for grammar.

ACK and pushed.

-- 
Eric Blake   ebl...@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 1/1] Clarify virsh freecell manpage entry

2012-03-22 Thread Eric Blake

On 03/22/2012 02:49 PM, Dave Allan wrote:
 ---
  tools/virsh.pod |   15 ++-
  1 files changed, 10 insertions(+), 5 deletions(-)
 
 diff --git a/tools/virsh.pod b/tools/virsh.pod
 index f0df4fd..a8bd739 100644
 --- a/tools/virsh.pod
 +++ b/tools/virsh.pod
 @@ -401,11 +401,16 @@ Bvirsh list --title
0 Domain-0 runningMailserver 1
2 fedora   paused
 
 -=item Bfreecell [Bcellno | I--all]
 -
 -Prints the available amount of memory on the machine or within a
 -NUMA cell if Icellno is provided.  If I--all is provided instead
 -of I--cellno, then show the information on all NUMA cells.
 +=item Bfreecell [{ [I--cellno] Bcellno | I--all }]
 +
 +Prints the available amount of memory on the machine or within a NUMA
 +cell.  The freecell command can provide one of three different
 +displays of available memory on the machine depending on the options
 +specified.  With no options, it displays the total free memory on the
 +machine.  With the --all option, it displays the free memory in each
 +cell and the total free memory on the machine.  Finally, with a
 +numeric argument or with --cellno plus a cell number it will display
 +the free memory for the specified cell only.

ACK and pushed.

-- 
Eric Blake   ebl...@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 1/2] Cleanup for a return statement in source files

2012-03-22 Thread Osier Yang


On 2012年03月22日 19:33, Martin Kletzander wrote:

Return statements with parameter enclosed in parentheses were modified
and parentheses were removed. The whole change was scripted, here is how:

List of files was obtained using this command:
git grep -l -e '\return\s*([^()]*\(([^()]*)[^()]*\)*)\s*;' | \
grep -e '\.[ch]$' -e '\.py$'

Found files were modified with this command:
sed -i -e \
's_^\(.*\return\)\s*(\(\([^()]*([^()]*)[^()]*\)*\))\s*\(;.*$\)_\1 \2\4_' \
-e 's_^\(.*\return\)\s*(\([^()]*\))\s*\(;.*$\)_\1 \2\3_'

Then checked for nonsenses.

The whole command looks like this:
git grep -l -e '\return\s*([^()]*\(([^()]*)[^()]*\)*)\s*;' | \
grep -e '\.[ch]$' -e '\.py$' | xargs sed -i -e\
's_^\(.*\return\)\s*(\(\([^()]*([^()]*)[^()]*\)*\))\s*\(;.*$\)_\1 \2\4_' \
-e 's_^\(.*\return\)\s*(\([^()]*\))\s*\(;.*$\)_\1 \2\3_'
---
  examples/dominfo/info1.c  |2 +-
  examples/domsuspend/suspend.c |6 +-
  python/generator.py   |8 +-
  python/libvirt-override.c |  216 +-
  python/libvirt-qemu-override.c|4 +-
  python/typewrappers.c |   76 +-
  src/conf/domain_conf.c|   22 ++--
  src/conf/interface_conf.c |   70 +-
  src/conf/nwfilter_params.c|2 +-
  src/cpu/cpu_x86.c |2 +-
  src/datatypes.c   |   80 +-
  src/interface/netcf_driver.c  |2 +-
  src/lxc/lxc_driver.c  |4 +-
  src/nwfilter/nwfilter_ebiptables_driver.c |4 +-
  src/qemu/qemu_command.c   |6 +-
  src/qemu/qemu_driver.c|4 +-
  src/qemu/qemu_process.c   |2 +-
  src/remote/remote_driver.c|2 +-
  src/security/security_selinux.c   |2 +-
  src/test/test_driver.c|8 +-
  src/util/conf.c   |  106 +++---
  src/util/hooks.c  |   20 ++--
  src/util/sexpr.c  |   14 +-
  src/util/util.c   |8 +-
  src/util/uuid.c   |6 +-
  src/util/virhash.c|   30 ++--
  src/util/virmacaddr.c |2 +-
  src/util/virnetlink.c |2 +-
  src/util/virsocketaddr.c  |   48 +++---
  src/util/virterror.c  |   14 +-
  src/util/xml.c|   50 +++---
  src/xen/xen_driver.c  |   32 ++--
  src/xen/xen_hypervisor.c  |  228 ++--
  src/xen/xend_internal.c   |  234 ++--
  src/xen/xm_internal.c |   98 ++--
  src/xen/xs_internal.c |  120 
  src/xenapi/xenapi_driver.c|2 +-
  src/xenxs/xen_sxpr.c  |2 +-
  src/xenxs/xen_xm.c|2 +-
  tests/commandtest.c   |4 +-
  tests/cputest.c   |2 +-
  tests/domainsnapshotxml2xmltest.c |4 +-
  tests/interfacexml2xmltest.c  |2 +-
  tests/networkxml2argvtest.c   |2 +-
  tests/networkxml2xmltest.c|2 +-
  tests/nodedevxml2xmltest.c|2 +-
  tests/nodeinfotest.c  |2 +-
  tests/nwfilterxml2xmltest.c   |2 +-
  tests/qemuargv2xmltest.c  |2 +-
  tests/qemuxml2argvtest.c  |2 +-
  tests/qemuxml2xmltest.c   |4 +-
  tests/qemuxmlnstest.c |2 +-
  tests/qparamtest.c|2 +-
  tests/sexpr2xmltest.c |4 +-
  tests/sockettest.c|2 +-
  tests/statstest.c |2 +-
  tests/storagepoolxml2xmltest.c|2 +-
  tests/storagevolxml2xmltest.c |2 +-
  tests/virbuftest.c|2 +-
  tests/virnetmessagetest.c |2 +-
  tests/virnetsockettest.c  |4 +-
  tests/virnettlscontexttest.c  |2 +-
  tests/virshtest.c |2 +-
  tests/virtimetest.c   |2 +-
  tests/xencapstest.c   |2 +-
  tests/xmconfigtest.c  |4 +-
  tests/xml2sexprtest.c |4 +-
  tools/virsh.c |8 +-
  68 files changed, 809 insertions(+), 809 deletions(-)


Quite long, but looks good, ACK.

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

[libvirt] [PATCH 4/6] snapshot: expose qemu commands for mirrored storage migration

2012-03-22 Thread Eric Blake

A snapshot-based mirrored storage migration sequence requires both the
'drive-mirror' action in 'transaction' (present if the 'drive-mirror'
standalone monitor command also exists) and the 'drive-reopen' monitor
command (it would be nice if that were also part of a 'transaction',
but the initial qemu implementation has it standalone only).

As of this[1] qemu email, both commands have been proposed but not yet
incorporated into the tree, so there is a risk that qemu 1.1 will
not have these commands, or will have something subtly different.
[1]https://lists.gnu.org/archive/html/qemu-devel/2012-03/msg01524.html

* src/qemu/qemu_capabilities.h (QEMU_CAPS_DRIVE_MIRROR)
(QEMU_CAPS_DRIVE_REOPEN): New bits.
* src/qemu/qemu_capabilities.c (qemuCaps): Name them.
* src/qemu/qemu_monitor_json.c (qemuMonitorJSONCheckCommands): Set
them.
(qemuMonitorJSONDriveMirror, qemuMonitorDriveReopen): New functions.
* src/qemu/qemu_monitor_json.h (qemuMonitorJSONDriveMirror)
(qemuMonitorDriveReopen): Declare them.
* src/qemu/qemu_monitor.c (qemuMonitorDriveMirror)
(qemuMonitorDriveReopen): New passthroughs.
* src/qemu/qemu_monitor.h (qemuMonitorDriveMirror)
(qemuMonitorDriveReopen): Declare them.
---
 src/qemu/qemu_capabilities.c |3 ++
 src/qemu/qemu_capabilities.h |2 +
 src/qemu/qemu_monitor.c  |   50 ++
 src/qemu/qemu_monitor.h  |   14 
 src/qemu/qemu_monitor_json.c |   70 +++--
 src/qemu/qemu_monitor_json.h |   21 +++-
 6 files changed, 154 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 0e09d6d..1938ae4 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -156,6 +156,9 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST,
   scsi-disk.channel,
   scsi-block,
   transaction,
+
+  drive-mirror, /* 90 */
+  drive-reopen,
 );

 struct qemu_feature_flags {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 78cdbe0..405bf2a 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -124,6 +124,8 @@ enum qemuCapsFlags {
 QEMU_CAPS_SCSI_DISK_CHANNEL  = 87, /* Is scsi-disk.channel available? */
 QEMU_CAPS_SCSI_BLOCK = 88, /* -device scsi-block */
 QEMU_CAPS_TRANSACTION= 89, /* transaction monitor command */
+QEMU_CAPS_DRIVE_MIRROR   = 90, /* drive-mirror monitor command */
+QEMU_CAPS_DRIVE_REOPEN   = 91, /* drive-reopen monitor command */

 QEMU_CAPS_LAST,   /* this must always be the last item */
 };
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index bbe0f98..06d24c8 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2652,6 +2652,32 @@ qemuMonitorDiskSnapshot(qemuMonitorPtr mon, 
virJSONValuePtr actions,
 return ret;
 }

+/* Add the drive-mirror action to a transaction.  */
+int
+qemuMonitorDriveMirror(qemuMonitorPtr mon, virJSONValuePtr actions,
+   const char *device, const char *file,
+   const char *format, bool reuse)
+{
+int ret;
+
+VIR_DEBUG(mon=%p, actions=%p, device=%s, file=%s, format=%s, reuse=%d,
+  mon, actions, device, file, format, reuse);
+
+if (!mon) {
+qemuReportError(VIR_ERR_INVALID_ARG, %s,
+_(monitor must not be NULL));
+return -1;
+}
+
+if (mon-json)
+ret = qemuMonitorJSONDriveMirror(mon, actions, device, file, format,
+ reuse);
+else
+qemuReportError(VIR_ERR_INVALID_ARG, %s,
+_(drive-mirror requires JSON monitor));
+return ret;
+}
+
 /* Use the transaction QMP command to run atomic snapshot commands.  */
 int
 qemuMonitorTransaction(qemuMonitorPtr mon, virJSONValuePtr actions)
@@ -2668,6 +2694,30 @@ qemuMonitorTransaction(qemuMonitorPtr mon, 
virJSONValuePtr actions)
 return ret;
 }

+/* Use the drive-reopen monitor command.  */
+int
+qemuMonitorDriveReopen(qemuMonitorPtr mon, const char *device,
+   const char *file, const char *format)
+{
+int ret;
+
+VIR_DEBUG(mon=%p, device=%s, file=%s, format=%s,
+  mon, device, file, format);
+
+if (!mon) {
+qemuReportError(VIR_ERR_INVALID_ARG, %s,
+_(monitor must not be NULL));
+return -1;
+}
+
+if (mon-json)
+ret = qemuMonitorJSONDriveReopen(mon, device, file, format);
+else
+qemuReportError(VIR_ERR_INVALID_ARG, %s,
+_(drive-reopen requires JSON monitor));
+return ret;
+}
+
 int qemuMonitorArbitraryCommand(qemuMonitorPtr mon,
 const char *cmd,
 char **reply,
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 909e9ac..d213818 100644
--- a/src/qemu/qemu_monitor.h
+++

[libvirt] [PATCH 5/6] snapshot: implement new snapshot delete flags in qemu

2012-03-22 Thread Eric Blake

Delete a mirrored snapshot by picking which of the two files in the
mirror to reopen.  This is not atomic, so we update the snapshot
in place as we iterate through each successful disk.  Since we limited
mirrored snapshots to transient domains, there is no persistent
configuration to update.  This mode implies deleting the snapshot
metadata but preserving the new file.

* src/qemu/qemu_driver.c (qemuDomainSnapshotDelete): Honor new
flags.
(qemuDomainSnapshotDeleteMirror): New helper function.
---
 src/qemu/qemu_driver.c |   75 ++-
 1 files changed, 73 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 89aa56c..428ba0f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11222,6 +11222,61 @@ qemuDomainSnapshotReparentChildren(void *payload,
rep-driver-snapshotDir);
 }

+/* Must be called while holding a job.  */
+static int
+qemuDomainSnapshotDeleteMirror(struct qemud_driver *driver,
+   virDomainObjPtr vm,
+   virDomainSnapshotObjPtr snap,
+   bool pivot)
+{
+int i;
+int ret = 0;
+qemuDomainObjPrivatePtr priv = vm-privateData;
+char *device = NULL;
+char *file;
+
+if (!virDomainObjIsActive(vm)) {
+qemuReportError(VIR_ERR_OPERATION_INVALID,
+%s, _(domain is not running));
+return -1;
+}
+
+qemuDomainObjEnterMonitorWithDriver(driver, vm);
+
+for (i = 0; i  snap-def-ndisks; i++) {
+if (!snap-def-disks[i].mirror)
+continue;
+VIR_FREE(device);
+if (virAsprintf(device, drive-%s,
+vm-def-disks[i]-info.alias)  0) {
+virReportOOMError();
+break;
+}
+file = pivot ? snap-def-disks[i].mirror : snap-def-disks[i].file;
+if (qemuMonitorDriveReopen(priv-mon, device, file,
+   snap-def-disks[i].driverType)  0)
+break;
+/* TODO: Release lock and reset label.  */
+
+VIR_FREE(vm-def-disks[i]-src);
+vm-def-disks[i]-src = file;
+if (pivot)
+snap-def-disks[i].file = snap-def-disks[i].mirror;
+snap-def-disks[i].mirror = NULL;
+}
+VIR_FREE(device);
+if (i  snap-def-ndisks)
+ret = -1;
+
+qemuDomainObjExitMonitorWithDriver(driver, vm);
+
+if (ret  0 
+qemuDomainSnapshotWriteMetadata(vm, snap, driver-snapshotDir)  0)
+VIR_WARN(failed writing snapshot '%s' after partial mirror deletion,
+ snap-def-name);
+return ret;
+}
+
 static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
 unsigned int flags)
 {
@@ -11234,10 +11289,14 @@ static int 
qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
 struct snap_reparent rep;
 bool metadata_only = !!(flags  VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY);
 int external = 0;
+bool mirror_abort = (flags  VIR_DOMAIN_SNAPSHOT_DELETE_MIRROR_ABORT) != 0;
+bool mirror_pivot = (flags  VIR_DOMAIN_SNAPSHOT_DELETE_MIRROR_PIVOT) != 0;

 virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN |
   VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY |
-  VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY, -1);
+  VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY |
+  VIR_DOMAIN_SNAPSHOT_DELETE_MIRROR_ABORT |
+  VIR_DOMAIN_SNAPSHOT_DELETE_MIRROR_PIVOT, -1);

 qemuDriverLock(driver);
 virUUIDFormat(snapshot-domain-uuid, uuidstr);
@@ -11262,8 +11321,16 @@ static int 
qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
 _(domain has active disk mirrors));
 goto cleanup;
 }
+if (!(mirror_abort || mirror_pivot)) {
+qemuReportError(VIR_ERR_OPERATION_INVALID, %s,
+_(must specify whether to abort or pivot 
+  mirrors before deleting snapshot));
+goto cleanup;
+}
+flags |= VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY;
+} else if (mirror_abort || mirror_pivot) {
 qemuReportError(VIR_ERR_OPERATION_INVALID, %s,
-_(deletion of active disk mirrors unimplemented));
+_(snapshot has no disk mirrors));
 goto cleanup;
 }

@@ -11286,6 +11353,10 @@ static int 
qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
 if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY)  0)
 goto cleanup;

+if ((mirror_abort || mirror_pivot) 
+qemuDomainSnapshotDeleteMirror(driver, vm, snap, mirror_pivot)  0)
+goto endjob;
+
 if (flags  (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN |
  VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) {
 rem.driver = driver;
-- 
1.7.7.6

--

[libvirt] [PATCH 0/6] atomic snapshots, rounds 4 and 5

2012-03-22 Thread Eric Blake

I was originally going to send this as two rounds, one for
the XML additions of adding mirrors, and one for the flag
addition to virDomainSnapshotDelete for mapping to drive-reopen;
but it turned out that testing is easier if I finish the series.

Note that this is minimally tested at the moment; I'm posting it
to get review started, but I plan on hammering it tomorrow, and
possibly coming up with slight alterations for a v2.

This assumes that rounds 1-3 (ending here) have been applied:
https://www.redhat.com/archives/libvir-list/2012-March/msg00846.html

This should finish out my RFC here:
https://www.redhat.com/archives/libvir-list/2012-March/msg00578.html
but with one major caveat:
https://lists.gnu.org/archive/html/qemu-devel/2012-03/msg01524.html
Upstream qemu has not yet committed the 'drive-mirror' or 'drive-reopen'
monitor commands, which this series (rounds 4-5) depends on.  There are
still bugs being worked out under the hood on the qemu side, and there
is a slight possibility that fixing those bugs may change the public
interface.  Therefore, part of the review of this series should be to
determine whether we take the risk of applying the patch now, or
waiting for qemu to stabilize a bit further.

I will probably push rounds 1-3 tomorrow, since they have been ACK'd
(well, round 3 is still awaiting review), and since qemu has committed
to the 'transaction' monitor command for qemu 1.1.

For reference, it is likely that RHEL 6.3 will not wait for qemu 1.1,
but will instead provide RHEL-specific monitor commands named
__com.redhat_drive-mirror and __com.redhat_drive-reopen; I have
hopefully structured things well enough that it will be a couple
lines of patching to qemu_monitor_json.c to recognize that alternate
command name.

Eric Blake (6):
  snapshot: allow for creation of mirrored snapshots
  snapshot: add new snapshot delete flags
  snapshot: make it possible to check for mirrored snapshot
  snapshot: expose qemu commands for mirrored storage migration
  snapshot: implement new snapshot delete flags in qemu
  snapshot: enable mirrored snapshots on transient vm

 docs/formatsnapshot.html.in|   31 +++
 docs/schemas/domainsnapshot.rng|8 +
 include/libvirt/libvirt.h.in   |6 +
 src/conf/domain_conf.c |   58 +-
 src/conf/domain_conf.h |4 +
 src/libvirt.c  |   37 +++-
 src/libvirt_private.syms   |2 +
 src/qemu/qemu_capabilities.c   |3 +
 src/qemu/qemu_capabilities.h   |2 +
 src/qemu/qemu_driver.c |  242 +++-
 src/qemu/qemu_monitor.c|   50 
 src/qemu/qemu_monitor.h|   14 ++
 src/qemu/qemu_monitor_json.c   |   70 ++-
 src/qemu/qemu_monitor_json.h   |   21 ++-
 .../disk_snapshot_mirror.xml   |   13 +
 .../disk_snapshot_mirror.xml   |   49 
 tests/domainsnapshotxml2xmltest.c  |4 +-
 tools/virsh.c  |   13 +
 tools/virsh.pod|   21 ++-
 19 files changed, 622 insertions(+), 26 deletions(-)
 create mode 100644 tests/domainsnapshotxml2xmlin/disk_snapshot_mirror.xml
 create mode 100644 tests/domainsnapshotxml2xmlout/disk_snapshot_mirror.xml

-- 
1.7.7.6

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

[libvirt] [PATCH 1/6] snapshot: allow for creation of mirrored snapshots

2012-03-22 Thread Eric Blake

This extends domainsnapshot XML to add a new element under each
disk of a disk snapshot:

  disk name='vda'
source file='/path/to/live'/
mirror file='/path/to/mirror'/
  /disk

For now, if a mirror is requested, the snapshot must be external,
and assumes the same driver format (qcow2 or qed) as the source.
qemu allows more flexibility in mirror creation, which could
possibly be added by further XML enhancements, but more likely
would be better served by adding a new API to specifically target
mirroring, as well as XML changes to represent the entire backing
chain in domain rather than commandeering domainsnapshot.
Meanwhile, the changes in this patch are sufficient for the oVirt
use case of using a mirror for live storage migration.

The new XML is parsed but rejected until later patches update the
hypervisor drivers to act on mirror requests.  The testsuite
additions show that we can round-trip the XML.

* docs/schemas/domainsnapshot.rng (disksnapshot): Add optional
mirror.
* docs/formatsnapshot.html.in: Document it.
* src/conf/domain_conf.h (VIR_DOMAIN_SNAPSHOT_PARSE_MIRROR): New
flag.
(_virDomainSnapshotDiskDef): Add new member.
* src/conf/domain_conf.c (virDomainSnapshotDiskDefParseXML): Add
parameter, to parse or reject mirrors.
(virDomainSnapshotDefParseString): Honor new flag.
(virDomainSnapshotDefFormat): Output any mirrors.
(virDomainSnapshotDiskDefClear): Clean up.
* tests/domainsnapshotxml2xmltest.c (mymain): Add a test.
(testCompareXMLToXMLFiles): Allow for mirrors.
* tests/domainsnapshotxml2xmlout/disk_snapshot_mirror.xml: New file.
* tests/domainsnapshotxml2xmlin/disk_snapshot_mirror.xml: Likewise.
---
 docs/formatsnapshot.html.in|   31 
 docs/schemas/domainsnapshot.rng|8 +++
 src/conf/domain_conf.c |   38 ++-
 src/conf/domain_conf.h |2 +
 .../disk_snapshot_mirror.xml   |   13 +
 .../disk_snapshot_mirror.xml   |   49 
 tests/domainsnapshotxml2xmltest.c  |4 +-
 7 files changed, 141 insertions(+), 4 deletions(-)
 create mode 100644 tests/domainsnapshotxml2xmlin/disk_snapshot_mirror.xml
 create mode 100644 tests/domainsnapshotxml2xmlout/disk_snapshot_mirror.xml

diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in
index ec5ebf3..cd39f1b 100644
--- a/docs/formatsnapshot.html.in
+++ b/docs/formatsnapshot.html.in
@@ -90,6 +90,30 @@
   another snapshot.
 /p
 p
+  External disk snapshots have another use of allowing live
+  storage migration across storage domains, while still
+  guaranteeing that at all points in time along the storage
+  migration, there exists a single storage domain with a
+  consistent view of the guest's data.  This is done by adding the
+  concept of snapshot mirroring, where a snapshot request
+  specifies not only the new file name on the original storage
+  domain, but a secondary mirror file name on the target storage
+  domain.  While the snapshot is active, reads are still tied to
+  the first storage domain, but all writes of changes from the
+  backing file are recorded in both storage domains; and the
+  management application can synchronize the backing files across
+  storage domains in parallel with the guest execution.  Once the
+  second storage domain has all data in place, the management
+  application then deletes the snapshot, which stops the mirroring
+  and lets the hypervisor swap over to the second storage domain,
+  so that the first storage domain is no longer in active use.
+  Depending on the hypervisor, the use of a mirrored snapshots may
+  be restricted to transient domains, and the existence of a
+  mirrored snapshot can prevent migration, domain saves, disk hot
+  plug actions, and further snapshot manipulation until the
+  mirrored snapshot is deleted in order to stop the mirroring.
+/p
+p
   The top-level codedomainsnapshot/code element may contain
   the following elements:
 /p
@@ -156,6 +180,13 @@
 snapshots, the original file name becomes the read-only
 snapshot, and the new file name contains the read-write
 delta of all disk changes since the snapshot.
+span class=sinceSince 0.9.11/span, an external
+snapshot may also have an optional
+element codemirror/code, which names a second file
+alongside codesource/code that will mirror all changes
+since the snapshot and using the same file format.  A
+mirror must have the codefile/code attribute listing
+the file name to use.
   /dd
 /dl
   /dd
diff --git a/docs/schemas/domainsnapshot.rng b/docs/schemas/domainsnapshot.rng
index 0ef0631..86193a7 100644
--- a/docs/schemas/domainsnapshot.rng
+++

[libvirt] [PATCH 6/6] snapshot: enable mirrored snapshots on transient vm

2012-03-22 Thread Eric Blake

The hardest part of this patch is figuring out how to provide proper
security labeling and lock manager setup for the mirror, as well as
rolling it all back on error.

* src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML): Decide
when mirrors are allowed.
(qemuDomainSnapshotDiskPrepare): Prepare for mirrors.
(qemuDomainSnapshotCreateSingleDiskActive): Turn on mirrors.
(qemuDomainSnapshotUndoSingleDiskActive): Undo mirrors on failure.
(qemuDomainSnapshotCreateDiskActive): Update caller.
---
 src/qemu/qemu_driver.c |  116 +--
 1 files changed, 111 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 428ba0f..d03064f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9815,6 +9815,25 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, 
virDomainSnapshotDefPtr def,
 disk-name, disk-file);
 goto cleanup;
 }
+if (disk-mirror) {
+if (stat(disk-mirror, st)  0) {
+if (errno != ENOENT) {
+virReportSystemError(errno,
+ _(unable to stat mirror for 
+   disk %s: %s),
+ disk-name, disk-mirror);
+goto cleanup;
+}
+} else if (!(S_ISBLK(st.st_mode) || !st.st_size ||
+ allow_reuse)) {
+qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+_(external mirror file for disk %s 
+  already exists and is not a block 
+  device: %s),
+disk-name, disk-mirror);
+goto cleanup;
+}
+}
 found = true;
 external++;
 break;
@@ -9875,6 +9894,7 @@ qemuDomainSnapshotCreateSingleDiskActive(struct 
qemud_driver *driver,
 char *origsrc = NULL;
 char *origdriver = NULL;
 bool need_unlink = false;
+bool need_unlink_mirror = false;

 if (snap-snapshot != VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL) {
 qemuReportError(VIR_ERR_INTERNAL_ERROR, %s,
@@ -9902,13 +9922,22 @@ qemuDomainSnapshotCreateSingleDiskActive(struct 
qemud_driver *driver,
 if (fd  0)
 goto cleanup;
 VIR_FORCE_CLOSE(fd);
+
+if (snap-mirror) {
+fd = qemuOpenFile(driver, snap-mirror,
+  O_WRONLY | O_TRUNC | O_CREAT,
+  need_unlink_mirror, NULL);
+if (fd  0)
+goto cleanup;
+VIR_FORCE_CLOSE(fd);
+}
 }

 origsrc = disk-src;
-disk-src = source;
 origdriver = disk-driverType;
 disk-driverType = (char *) raw; /* Don't want to probe backing files */

+disk-src = source;
 if (virDomainLockDiskAttach(driver-lockManager, vm, disk)  0)
 goto cleanup;
 if (virSecurityManagerSetImageLabel(driver-securityManager, vm-def,
@@ -9918,9 +9947,21 @@ qemuDomainSnapshotCreateSingleDiskActive(struct 
qemud_driver *driver,
 goto cleanup;
 }

+if (snap-mirror) {
+disk-src = snap-mirror;
+if (virDomainLockDiskAttach(driver-lockManager, vm, disk)  0)
+goto cleanup2;
+if (virSecurityManagerSetImageLabel(driver-securityManager, vm-def,
+disk)  0) {
+if (virDomainLockDiskDetach(driver-lockManager, vm, disk)  0)
+VIR_WARN(Unable to release lock on %s, source);
+goto cleanup2;
+}
+}
+
 disk-src = origsrc;
-origsrc = NULL;
 disk-driverType = origdriver;
+origsrc = NULL;
 origdriver = NULL;

 /* create the actual snapshot */
@@ -9929,9 +9970,19 @@ qemuDomainSnapshotCreateSingleDiskActive(struct 
qemud_driver *driver,
 virDomainAuditDisk(vm, disk-src, source, snapshot, ret = 0);
 if (ret  0)
 goto cleanup;
+if (snap-mirror) {
+ret = qemuMonitorDriveMirror(priv-mon, actions, device, snap-mirror,
+ driverType, reuse);
+virDomainAuditDisk(vm, NULL, snap-mirror, mirror, ret = 0);
+if (ret  0) {
+ret = -2;
+goto cleanup;
+}
+}

 /* Update vm in place to match changes.  */
 need_unlink = false;
+need_unlink_mirror = false;
 VIR_FREE(disk-src);
 disk-src = source;
 source = NULL;
@@ -9958,12 +10009,25 @@ cleanup:
 }
 if (need_unlink  unlink(source))
 VIR_WARN(unable to unlink just-created %s, source);
+if (need_unlink_mirror  unlink(snap-mirror))
+VIR_WARN(unable to unlink just-created %s, snap-mirror);
 VIR_FREE(device);
 VIR_FREE(source);
 VIR_FREE(driverType);

[libvirt] [PATCH 2/6] snapshot: add new snapshot delete flags

2012-03-22 Thread Eric Blake

This completes the public API for using mirrored snapshots as a
means of performing live storage migration.  Of course, it will
take additional patches to actually provide the implementation.

The idea here is that oVirt can start with a domain with 'vda'
open on /path1/to/old.qcow2 with a base file of /path1/to/common,
then do the following steps for a live migration to /path2 (here
using virsh commands, although the underlying API would be
available to oVirt through other language bindings as well).

First, pre-create two files; it is important that the mirror file
have a relative backing file name (if we let qemu create the entire
snapshot, the backing file name would be absolute to /path1, and
pivoting to the mirror would still be using the original source):

$ qemu-img create -f qcow2 -o backing_file=old.qcow2 \
   -o backing_fmt=qcow2 /path1/to/old.migrate
$ qemu-img create -f qcow2 -o backing_file=old.qcow2 \
   -o backing_fmt=qcow2 /path2/to/new.qcow2

Next, create a mirrored snapshot, while telling qemu to respect
the pre-existing qcow2 header in both files:

$ virsh snapshot-create-as $dom migrate --reuse-external \
   --diskspec vda,file=/path1/to/old.migrate,mirror=/path2/to/new.qcow2

which means 'vda' is now open read-write on /path1/to/old.migrate
and mirrored (write only) on /path2/to/new.qcow2, where qemu is
using /path1/to/old.qcow2 as the logial backing for both files,
but where the relative name is still intact in /path2/to/new.qcow2.
Next, the backing files can be copied between locations:

$ cp /path1/to/common /path1/to/old.qcow2 /path2/to

When that is complete, the mirrored snapshot is no longer necessary,
and requesting a pivot while deleting things will force qemu to
reread the header of /path2/to/new.qcow2, notice a relative backing
file name, and open /path2/to/old.qcow2 as the logical backing file:

$ virsh snapshot-delete $dom migrate --mirror-pivot

Now /path1 is no longer in use, but the backing chain on /path2
is longer than original.  This can be cleaned up with:

$ virsh blockpull $dom vda --base /path2/to/common

to go back to having /path2/to/new.qcow2 directly backed by
/path2/to/common.

That smells like a hack, right?  Well, there is a proposal for a
better, more powerful API named virDomainBlockCopy:
https://www.redhat.com/archives/libvir-list/2012-March/msg00585.html
which would more appropriately expose the various knobs of the new
qemu commands; but being a new API, it lacks the ability to be
backported without causing a .so bump.  So this is the compromise.

* include/libvirt/libvirt.h.in
(VIR_DOMAIN_SNAPSHOT_DELETE_MIRROR_ABORT)
(VIR_DOMAIN_SNAPSHOT_DELETE_MIRROR_PIVOT): New flags.
* src/libvirt.c (virDomainSnapshotDelete): Document them.
(virDomainSnapshotCreateXML, virDomainRevertToSnapshot)
(virDomainSaveFlags): Mention effects of mirrored snapshots.
* tools/virsh.c (vshParseSnapshotDiskspec): Add mirror support.
(cmdSnapshotDelete): Add --mirror-abort, --mirror-pivot flags.
* tools/virsh.pod (snapshot-delete, snapshot-create-as): Document
new options.
---
 include/libvirt/libvirt.h.in |6 ++
 src/libvirt.c|   37 +++--
 tools/virsh.c|   13 +
 tools/virsh.pod  |   21 ++---
 4 files changed, 68 insertions(+), 9 deletions(-)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 4566580..dbb6358 100644
--- a/include/libvirt/libvirt.h.in
+++ b/include/libvirt/libvirt.h.in
@@ -3369,6 +3369,12 @@ typedef enum {
 VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN  = (1  0), /* Also delete 
children */
 VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY = (1  1), /* Delete just 
metadata */
 VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY = (1  2), /* Delete just 
children */
+VIR_DOMAIN_SNAPSHOT_DELETE_MIRROR_ABORT  = (1  3), /* Stop a mirrored
+snapshot, reopening
+to the source.  */
+VIR_DOMAIN_SNAPSHOT_DELETE_MIRROR_PIVOT  = (1  4), /* Stop a mirrored
+snapshot, reopening
+to the mirror.  */
 } virDomainSnapshotDeleteFlags;

 int virDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
diff --git a/src/libvirt.c b/src/libvirt.c
index 7df3667..800d1ee 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -2702,6 +2702,9 @@ error:
  * @flags will override what state gets saved into the file.  These
  * two flags are mutually exclusive.
  *
+ * Some hypervisors may prevent this call if there is a current
+ * mirrored snapshot.
+ *
  * A save file can be inspected or modified slightly with
  * virDomainSaveImageGetXMLDesc() and virDomainSaveImageDefineXML().
  *
@@ -17087,12 +17090,14 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr 
snapshot)
  * not exist, the hypervisor may validate that reverting to

[libvirt] [PATCH 3/6] snapshot: make it possible to check for mirrored snapshot

2012-03-22 Thread Eric Blake

For now, disk migration via mirroring is not implemented.  But when
we do implement it, we have to deal with the fact that qemu does
not provide an easy way to re-start a qemu process with mirroring
still intact (it _might_ be possible by using qemu -S then an
initial 'drive-mirror' with disk reuse before starting the domain,
but that gets hairy).  Even something like 'virDomainSave' becomes
hairy, if you realize the implications that 'virDomainRestore' would
be stuck with recreating the same mirror layout.

But if we step back and look at the bigger picture, we realize that
the initial client of live storage migration via disk mirroring is
oVirt, which always uses transient domains, and that if a transient
domain is destroyed while a mirror exists, oVirt can easily restart
the storage migration by creating a new domain that visits just the
source storage, with no loss in data.

We can make life a lot easier by being cowards, and forbidding
certain operations on a domain.  This patch guarantees that there
will be at most one snapshot with disk mirroring, that it will
always be the current snapshot, and that the user cannot redefine
that snapshot nor hot-plug or hot-unplug any domain disks -
for now, the only way to delete such a snapshot is by destroying
the transient domain it is attached to.  A future patch will add
the ability to also delete such snapshots under a condition of a
flag which says how to end the mirroring with the 'drive-reopen'
monitor command; and once that is in place, then we can finally
implement creation of the mirroring via 'drive-mirror'.  With
just this patch applied, the only way you can ever get
virDomainHasDiskMirror to return true is by manually playing with
the libvirt internal directory and then restarting libvirtd.

* src/conf/domain_conf.h (virDomainSnapshotHasDiskMirror)
(virDomainHasDiskMirror): New prototypes.
* src/conf/domain_conf.c (virDomainSnapshotHasDiskMirror)
(virDomainHasDiskMirror): Implement them.
* src/libvirt_private.syms (domain_conf.h): Export them.
* src/qemu/qemu_driver.c (qemuDomainSaveInternal)
(qemuDomainSnapshotCreateXML, qemuDomainRevertToSnapshot)
(qemuDomainSnapshotDelete, qemuDomainAttachDeviceDiskLive)
(qemuDomainDetachDeviceDiskLive): Use it.
(qemuDomainSnapshotLoad): Allow libvirtd restarts with active disk
mirroring, but sanity check for only a current image with mirrors.
---
 src/conf/domain_conf.c   |   20 +
 src/conf/domain_conf.h   |2 +
 src/libvirt_private.syms |2 +
 src/qemu/qemu_driver.c   |   53 +-
 4 files changed, 76 insertions(+), 1 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f84304c..85abf32 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -13652,6 +13652,26 @@ cleanup:
 return ret;
 }

+/* Determine if the given snapshot contains a disk mirror.  */
+bool
+virDomainSnapshotHasDiskMirror(virDomainSnapshotDefPtr snapshot)
+{
+int i;
+for (i = 0; i  snapshot-ndisks; i++)
+if (snapshot-disks[i].mirror)
+return true;
+return false;
+}
+
+/* Determine if the given domain has a current snapshot that contains
+ * a disk mirror.  */
+bool virDomainHasDiskMirror(virDomainObjPtr vm)
+{
+if (!vm-current_snapshot)
+return false;
+return virDomainSnapshotHasDiskMirror(vm-current_snapshot-def);
+}
+
 char *virDomainSnapshotDefFormat(const char *domain_uuid,
  virDomainSnapshotDefPtr def,
  unsigned int flags,
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 01a504b..1ea7332 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1706,6 +1706,7 @@ char *virDomainSnapshotDefFormat(const char *domain_uuid,
 int virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapshot,
 int default_snapshot,
 bool require_match);
+bool virDomainSnapshotHasDiskMirror(virDomainSnapshotDefPtr snapshot);
 virDomainSnapshotObjPtr virDomainSnapshotAssignDef(virDomainSnapshotObjListPtr 
snapshots,
const 
virDomainSnapshotDefPtr def);

@@ -1931,6 +1932,7 @@ virDomainDiskDefPtr
 virDomainDiskRemove(virDomainDefPtr def, size_t i);
 virDomainDiskDefPtr
 virDomainDiskRemoveByName(virDomainDefPtr def, const char *name);
+bool virDomainHasDiskMirror(virDomainObjPtr vm);

 int virDomainNetIndexByMac(virDomainDefPtr def, const unsigned char *mac);
 int virDomainNetInsert(virDomainDefPtr def, virDomainNetDefPtr net);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 9a718b4..6739733 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -358,6 +358,7 @@ virDomainGraphicsSpiceZlibCompressionTypeFromString;
 virDomainGraphicsSpiceZlibCompressionTypeToString;
 virDomainGraphicsTypeFromString;
 virDomainGraphicsTypeToString;
+virDomainHasDiskMirror;

[libvirt] [PATCH] Add qemu support for ppc64 on FC16 or above for rpm packaging

2012-03-22 Thread Li Zhang

On Fedora16 or above, qemu is supported now. So it is added
in rpm packaging.

Signed-off-by: Li Zhang zhlci...@linux.vnet.ibm.com
---
 libvirt.spec.in |4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 072fd8e..568a84f 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -139,8 +139,8 @@
 %define with_xen 0
 %endif
 
-# Fedora doesn't have any QEMU on ppc64 - only ppc
-%if 0%{?fedora}
+# Fedora doesn't have any QEMU on ppc64 until FC16 - only ppc
+%if 0%{?fedora}  16
 %ifarch ppc64
 %define with_qemu 0
 %endif
-- 
1.7.5.4

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

Re: [libvirt] [PATCH 2/2] Added syntax-check rule for return with parentheses

2012-03-22 Thread Osier Yang


On 03/22/2012 07:33 PM, Martin Kletzander wrote:

After cleanup introduced with previous commit, there is a need for
syntax-check rule taking care of return(). Regexp used in 'prohibit'
parameter is taken from the cleanup commit and modified so it fits
'grep -E' format. Semicolon at the end is needed, otherwise the regexp
could match return with cast.

No exception is needed thanks to files excused by default. The
exception is not even needed for python files because 1) when
returning a tuple, you can still omit parentheses around (unless there
is only one value inside, but that's very unusual case),  2) return in
python doesn't need semicolon at the end of statement when there is no
statement following (and statement following 'return' doesn't make
sense).


This forces the Python coding style actually, prohibiting one
writes Python codes like:

def test():
return (1);

def test1():
return (1,2,3);

regardless of whether coding like this is good or bad, usual or
unsual, they are legal in Python, and we don't say they are not
allowed in HACKING or somewhere else.

Not sure if we should allow this, even we will allow that, we
need to document it somewhere, or more clear and specific for .py
files in halt message.


---
  cfg.mk |6 ++
  1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/cfg.mk b/cfg.mk
index 24e6a69..59b07ef 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -469,6 +469,12 @@ sc_prohibit_xmlURI:
halt='use virURI(Parse|Format), not xml(ParseURI|SaveUri)'  \
  $(_sc_search_regexp)

+# we don't want old old-style return with parentheses around argument
+sc_prohibit_return_as_function:
+   @prohibit='\return *\(([^()]*(\([^()]*\)[^()]*)*)\) *;'\
+   halt='avoid extra () with return statements'\
+ $(_sc_search_regexp)
+
  # ATTRIBUTE_UNUSED should only be applied in implementations, not
  # header declarations
  sc_avoid_attribute_unused_in_header:


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

81 matches

Mail list logo