[libvirt] [PATCH RFC 0/8] Sparse streams

[Michal Privoznik]

** NOT TO BE MERGED UPSTREAM **

This is merely an RFC.

What's the problem?
We have APIs for transferring disk images from/to host. Problem is, disk images
can be sparse files. Our code is, however, unaware of that fact so if for
instance the disk is one big hole of 8GB all those bytes have to: a) be read b)
come through our event loop. This is obviously very inefficient way.

How to deal with it?
The way I propose (and this is actually what I like you to comment on) is to
invent set of new API. We need to make read from and write to a stream
sparseness aware. The new APIs are as follows:

  int virStreamSendOffset(virStreamPtr stream,
  unsigned long long offset,
  const char *data,
  size_t nbytes);

  int virStreamRecvOffset(virStreamPtr stream,
  unsigned long long *offset,
  char *data,
  size_t nbytes);

The SendOffset() is fairly simple. It is given an offset to write @data from so
it basically lseek() to @offset and write() data.
The RecvOffset() has slightly complicated design - it has to be aware of the
fact that @offset it is required to read from fall into a hole. If that's the
case it sets @offset to new location where data starts.

Are there other ways possible?
Sure! If you have any specific in mind I am happy to discuss it. For instance
one idea I've heard (from Martin) was instead of SendOffset() and RecvOffset()
we may just invent our variant of lseek().

What's left to be done?
Basically, I still don't have RPC implementation. But before I dive into that I
thought of sharing my approach with you - because it may turn out that a
different approach is going to be needed and thus my work would render useless.

Is there any bug attached?
You can bet! https://bugzilla.redhat.com/show_bug.cgi?id=1282859

Michal Privoznik (8):
  fdstream: Realign
  Introduce virStream{Recv,Send}Offset
  Introduce VIR_STREAM_SPARSE flag
  fdstream: Implement virStreamSendOffset
  fdstream: Implement virStreamRecvOffset
  fdstreamtest: switch from boolean array to @flags
  fdstreamtest: Test virStreamRecvOffset
  fdstreamtest: test virStreamSendOffset

 include/libvirt/libvirt-stream.h |   9 +++
 src/driver-stream.h  |  13 
 src/fdstream.c   | 150 +++
 src/internal.h   |  20 ++
 src/libvirt-stream.c | 119 +++
 src/libvirt_public.syms  |   6 ++
 tests/fdstreamtest.c | 116 --
 7 files changed, 397 insertions(+), 36 deletions(-)

-- 
2.4.10

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

[libvirt] [PATCH RFC 2/8] Introduce virStream{Recv,Send}Offset

[Michal Privoznik]

When dealing with sparse files we need to be able to jump over
holes as there's no added value in reading/writing them. For
that, we need new set of send and receive APIs that will have
@offset argument. Sending data to a stream would be easy - just
say from which offset we are sending data. Reading is a bit
tricky - we need read function which can detect holes and thus
when requested to read from one it will set @offset to new value
that contains data.

Signed-off-by: Michal Privoznik 
---
 include/libvirt/libvirt-stream.h |   8 +++
 src/driver-stream.h  |  13 +
 src/libvirt-stream.c | 113 +++
 src/libvirt_public.syms  |   6 +++
 4 files changed, 140 insertions(+)

diff --git a/include/libvirt/libvirt-stream.h b/include/libvirt/libvirt-stream.h
index 831640d..5a2bde3 100644
--- a/include/libvirt/libvirt-stream.h
+++ b/include/libvirt/libvirt-stream.h
@@ -40,11 +40,19 @@ int virStreamRef(virStreamPtr st);
 int virStreamSend(virStreamPtr st,
   const char *data,
   size_t nbytes);
+int virStreamSendOffset(virStreamPtr stream,
+unsigned long long offset,
+const char *data,
+size_t nbytes);
 
 int virStreamRecv(virStreamPtr st,
   char *data,
   size_t nbytes);
 
+int virStreamRecvOffset(virStreamPtr stream,
+unsigned long long *offset,
+char *data,
+size_t nbytes);
 
 /**
  * virStreamSourceFunc:
diff --git a/src/driver-stream.h b/src/driver-stream.h
index 85b4e3b..5419b85 100644
--- a/src/driver-stream.h
+++ b/src/driver-stream.h
@@ -31,9 +31,20 @@ typedef int
 size_t nbytes);
 
 typedef int
+(*virDrvStreamSendOffset)(virStreamPtr st,
+  unsigned long long offset,
+  const char *data,
+  size_t nbytes);
+
+typedef int
 (*virDrvStreamRecv)(virStreamPtr st,
 char *data,
 size_t nbytes);
+typedef int
+(*virDrvStreamRecvOffset)(virStreamPtr st,
+  unsigned long long *offset,
+  char *data,
+  size_t nbytes);
 
 typedef int
 (*virDrvStreamEventAddCallback)(virStreamPtr stream,
@@ -60,7 +71,9 @@ typedef virStreamDriver *virStreamDriverPtr;
 
 struct _virStreamDriver {
 virDrvStreamSend streamSend;
+virDrvStreamSendOffset streamSendOffset;
 virDrvStreamRecv streamRecv;
+virDrvStreamRecvOffset streamRecvOffset;
 virDrvStreamEventAddCallback streamEventAddCallback;
 virDrvStreamEventUpdateCallback streamEventUpdateCallback;
 virDrvStreamEventRemoveCallback streamEventRemoveCallback;
diff --git a/src/libvirt-stream.c b/src/libvirt-stream.c
index c16f586..1df188c 100644
--- a/src/libvirt-stream.c
+++ b/src/libvirt-stream.c
@@ -192,6 +192,58 @@ virStreamSend(virStreamPtr stream,
 
 
 /**
+ * virStreamSendOffset:
+ * @stream: pointer to the stream object
+ * @offset: 
+ * @data: buffer to write to stream
+ * @nbytes: size of @data buffer
+ *
+ * Sends some data down the pipe.
+ *
+ * Returns the number of bytes written, which may be less
+ * than requested.
+ *
+ * Returns -1 upon error, at which time the stream will
+ * be marked as aborted, and the caller should now release
+ * the stream with virStreamFree.
+ *
+ * Returns -2 if the outgoing transmit buffers are full &
+ * the stream is marked as non-blocking.
+ */
+int
+virStreamSendOffset(virStreamPtr stream,
+unsigned long long offset,
+const char *data,
+size_t nbytes)
+{
+VIR_DEBUG("stream=%p, offset=%llu, data=%p, nbytes=%zu",
+  stream, offset, data, nbytes);
+
+virResetLastError();
+
+virCheckStreamReturn(stream, -1);
+virCheckNonNullArgGoto(data, error);
+
+if (stream->driver &&
+stream->driver->streamSendOffset) {
+int ret;
+ret = (stream->driver->streamSendOffset)(stream, offset, data, nbytes);
+if (ret == -2)
+return -2;
+if (ret < 0)
+goto error;
+return ret;
+}
+
+virReportUnsupportedError();
+
+ error:
+virDispatchError(stream->conn);
+return -1;
+}
+
+
+/**
  * virStreamRecv:
  * @stream: pointer to the stream object
  * @data: buffer to read into from stream
@@ -285,6 +337,67 @@ virStreamRecv(virStreamPtr stream,
 
 
 /**
+ * virStreamRecvOffset:
+ * @stream: pointer to the stream object
+ * @offset: 
+ * @data: buffer to write to stream
+ * @nbytes: size of @data buffer
+ *
+ * Recieve some data from stream. On return set offset to next location.
+ *
+ * Returns the number of bytes read, which may be less
+ * than requested.
+ *
+ * Returns 0 when either the end of the stream is reached, or
+ * there are no data to be sent at current @offset. In 

[libvirt] [PATCH RFC 3/8] Introduce VIR_STREAM_SPARSE flag

[Michal Privoznik]

This flag will create a virStream that will use sparse APIs
instead of those meant for regular, dense streams. Therefore we
must check for stream (non-)sparseness and deny API meant for one
or another type.

Signed-off-by: Michal Privoznik 
---
 include/libvirt/libvirt-stream.h |  1 +
 src/internal.h   | 20 
 src/libvirt-stream.c |  6 ++
 3 files changed, 27 insertions(+)

diff --git a/include/libvirt/libvirt-stream.h b/include/libvirt/libvirt-stream.h
index 5a2bde3..f05e703 100644
--- a/include/libvirt/libvirt-stream.h
+++ b/include/libvirt/libvirt-stream.h
@@ -31,6 +31,7 @@
 
 typedef enum {
 VIR_STREAM_NONBLOCK = (1 << 0),
+VIR_STREAM_SPARSE   = (1 << 1),
 } virStreamFlags;
 
 virStreamPtr virStreamNew(virConnectPtr conn,
diff --git a/src/internal.h b/src/internal.h
index db26fb0..b99d9fd 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -493,6 +493,26 @@
 goto label; \
 }   \
 } while (0)
+# define virCheckStreamSparseReturn(flags, retval)  \
+  do {  \
+  if (!((flags) & VIR_STREAM_SPARSE)) { \
+  virReportInvalidArg(flags,\
+  _("%s function is not supported " \
+"with sparse streams"), \
+  __FUNCTION__);\
+  return retval;\
+  } \
+  } while (0)
+# define virCheckStreamNonSparseReturn(flags, retval)   \
+  do {  \
+  if ((flags) & VIR_STREAM_SPARSE) {\
+  virReportInvalidArg(flags,\
+  _("%s function is not supported " \
+"on dense streams"),\
+  __FUNCTION__);\
+  return retval;\
+  } \
+  } while(0)
 
 
 
diff --git a/src/libvirt-stream.c b/src/libvirt-stream.c
index 1df188c..5c6b83f 100644
--- a/src/libvirt-stream.c
+++ b/src/libvirt-stream.c
@@ -171,6 +171,7 @@ virStreamSend(virStreamPtr stream,
 
 virCheckStreamReturn(stream, -1);
 virCheckNonNullArgGoto(data, error);
+virCheckStreamSparseReturn(stream->flags, -1);
 
 if (stream->driver &&
 stream->driver->streamSend) {
@@ -223,6 +224,7 @@ virStreamSendOffset(virStreamPtr stream,
 
 virCheckStreamReturn(stream, -1);
 virCheckNonNullArgGoto(data, error);
+virCheckStreamNonSparseReturn(stream->flags, -1);
 
 if (stream->driver &&
 stream->driver->streamSendOffset) {
@@ -316,6 +318,7 @@ virStreamRecv(virStreamPtr stream,
 
 virCheckStreamReturn(stream, -1);
 virCheckNonNullArgGoto(data, error);
+virCheckStreamSparseReturn(stream->flags, -1);
 
 if (stream->driver &&
 stream->driver->streamRecv) {
@@ -377,6 +380,7 @@ virStreamRecvOffset(virStreamPtr stream,
 virCheckStreamReturn(stream, -1);
 virCheckNonNullArgGoto(offset, error);
 virCheckNonNullArgGoto(data, error);
+virCheckStreamNonSparseReturn(stream->flags, -1);
 
 if (stream->driver &&
 stream->driver->streamRecvOffset) {
@@ -451,6 +455,7 @@ virStreamSendAll(virStreamPtr stream,
 
 virCheckStreamReturn(stream, -1);
 virCheckNonNullArgGoto(handler, cleanup);
+virCheckStreamSparseReturn(stream->flags, -1);
 
 if (stream->flags & VIR_STREAM_NONBLOCK) {
 virReportError(VIR_ERR_OPERATION_INVALID, "%s",
@@ -544,6 +549,7 @@ virStreamRecvAll(virStreamPtr stream,
 
 virCheckStreamReturn(stream, -1);
 virCheckNonNullArgGoto(handler, cleanup);
+virCheckStreamSparseReturn(stream->flags, -1);
 
 if (stream->flags & VIR_STREAM_NONBLOCK) {
 virReportError(VIR_ERR_OPERATION_INVALID, "%s",
-- 
2.4.10

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

[libvirt] [PATCH RFC 7/8] fdstreamtest: Test virStreamRecvOffset

[Michal Privoznik]

Signed-off-by: Michal Privoznik 
---
 tests/fdstreamtest.c | 62 ++--
 1 file changed, 51 insertions(+), 11 deletions(-)

diff --git a/tests/fdstreamtest.c b/tests/fdstreamtest.c
index f9caebf..5036cec 100644
--- a/tests/fdstreamtest.c
+++ b/tests/fdstreamtest.c
@@ -51,9 +51,11 @@ testFDStreamReadCommon(const char *scratchdir, const 
unsigned int flags)
 virStreamPtr st = NULL;
 size_t i;
 virConnectPtr conn = NULL;
+unsigned long long streamOffset;
 bool blocking = !(flags & VIR_STREAM_NONBLOCK);
+bool sparse = flags & VIR_STREAM_SPARSE;
 
-virCheckFlags(VIR_STREAM_NONBLOCK, -1);
+virCheckFlags(VIR_STREAM_NONBLOCK | VIR_STREAM_SPARSE, -1);
 
 if (!(conn = virConnectOpen("test:///default")))
 goto cleanup;
@@ -72,6 +74,10 @@ testFDStreamReadCommon(const char *scratchdir, const 
unsigned int flags)
 goto cleanup;
 
 for (i = 0; i < 10; i++) {
+if (i && sparse &&
+lseek(fd, 8192, SEEK_CUR) < 0)
+goto cleanup;
+
 if (safewrite(fd, pattern, PATTERN_LEN) != PATTERN_LEN)
 goto cleanup;
 }
@@ -82,17 +88,23 @@ testFDStreamReadCommon(const char *scratchdir, const 
unsigned int flags)
 if (!(st = virStreamNew(conn, flags)))
 goto cleanup;
 
-/* Start reading 1/2 way through first pattern
- * and end 1/2 way through last pattern
+/* Start reading 1/2 way through first pattern and end 1/2
+ * way through last pattern. In case of sparse file between
+ * each data blocks is one hole.
  */
+streamOffset = PATTERN_LEN / 2;
 if (virFDStreamOpenFile(st, file,
-PATTERN_LEN / 2, PATTERN_LEN * 9,
+streamOffset,
+sparse ? PATTERN_LEN * 18 : PATTERN_LEN * 9,
 O_RDONLY) < 0)
 goto cleanup;
 
 for (i = 0; i < 10; i++) {
 size_t offset = 0;
 size_t want;
+bool data = true;
+unsigned long long oldStreamOffset = streamOffset;
+
 if (i == 0)
 want = PATTERN_LEN / 2;
 else
@@ -101,7 +113,12 @@ testFDStreamReadCommon(const char *scratchdir, const 
unsigned int flags)
 while (want > 0) {
 int got;
 reread:
-got = st->driver->streamRecv(st, buf + offset, want);
+if (sparse)
+got = st->driver->streamRecvOffset(st, ,
+   buf + offset, want);
+else
+got = st->driver->streamRecv(st, buf + offset, want);
+
 if (got < 0) {
 if (got == -2 && !blocking) {
 usleep(20 * 1000);
@@ -112,16 +129,33 @@ testFDStreamReadCommon(const char *scratchdir, const 
unsigned int flags)
 goto cleanup;
 }
 if (got == 0) {
-/* Expect EOF 1/2 through last pattern */
-if (i == 9 && want == (PATTERN_LEN / 2))
-break;
-virFilePrintf(stderr, "Unexpected EOF block %zu want %zu\n",
-  i, want);
-goto cleanup;
+if (sparse &&
+oldStreamOffset != streamOffset) {
+/* oldStreamOffset points to data position. Re-read. */
+if (data) {
+virFilePrintf(stderr, "Unexpected hole at offset 
%llu\n",
+  streamOffset);
+}
+data = !data;
+streamOffset = oldStreamOffset;
+goto reread;
+} else {
+/* Expect EOF 1/2 through last pattern */
+if (i == 9 && want == (PATTERN_LEN / 2))
+break;
+virFilePrintf(stderr, "Unexpected EOF block %zu want 
%zu\n",
+  i, want);
+goto cleanup;
+}
 }
+if (i == 0)
+streamOffset = 8192 + PATTERN_LEN;
+else
+streamOffset += 8192 + PATTERN_LEN;
 offset += got;
 want -= got;
 }
+
 if (i == 0) {
 if (memcmp(buf, pattern + (PATTERN_LEN / 2), PATTERN_LEN / 2) != 
0) {
 virFilePrintf(stderr, "Mismatched pattern data iteration 
%zu\n", i);
@@ -170,6 +204,10 @@ static int testFDStreamReadNonblock(const void *data)
 {
 return testFDStreamReadCommon(data, VIR_STREAM_NONBLOCK);
 }
+static int testFDStreamSparseReadBlock(const void *data)
+{
+return testFDStreamReadCommon(data, VIR_STREAM_SPARSE);
+}
 
 
 static int testFDStreamWriteCommon(const char *scratchdir, const unsigned int 
flags)
@@ -334,6 +372,8 @@ mymain(void)
 ret = -1;
 if (virtTestRun("Stream write non-blocking ", testFDStreamWriteNonblock, 

[libvirt] [PATCH RFC 8/8] fdstreamtest: test virStreamSendOffset

[Michal Privoznik]

Signed-off-by: Michal Privoznik 
---
 tests/fdstreamtest.c | 39 +++
 1 file changed, 35 insertions(+), 4 deletions(-)

diff --git a/tests/fdstreamtest.c b/tests/fdstreamtest.c
index 5036cec..4fc88a7 100644
--- a/tests/fdstreamtest.c
+++ b/tests/fdstreamtest.c
@@ -220,9 +220,12 @@ static int testFDStreamWriteCommon(const char *scratchdir, 
const unsigned int fl
 virStreamPtr st = NULL;
 size_t i;
 virConnectPtr conn = NULL;
+unsigned long long streamOffset;
+unsigned long long streamLength;
 bool blocking = !(flags & VIR_STREAM_NONBLOCK);
+bool sparse = flags & VIR_STREAM_SPARSE;
 
-virCheckFlags(VIR_STREAM_NONBLOCK, -1);
+virCheckFlags(VIR_STREAM_NONBLOCK | VIR_STREAM_SPARSE, -1);
 
 if (!(conn = virConnectOpen("test:///default")))
 goto cleanup;
@@ -243,8 +246,11 @@ static int testFDStreamWriteCommon(const char *scratchdir, 
const unsigned int fl
 /* Start writing 1/2 way through first pattern
  * and end 1/2 way through last pattern
  */
+streamOffset = PATTERN_LEN / 2;
+streamLength = sparse ? PATTERN_LEN * 8192 : PATTERN_LEN * 9;
 if (virFDStreamCreateFile(st, file,
-  PATTERN_LEN / 2, PATTERN_LEN * 9,
+  streamOffset,
+  streamLength,
   O_WRONLY, 0600) < 0)
 goto cleanup;
 
@@ -258,8 +264,11 @@ static int testFDStreamWriteCommon(const char *scratchdir, 
const unsigned int fl
 
 while (want > 0) {
 int got;
-rewrite:
-got = st->driver->streamSend(st, pattern + offset, want);
+ rewrite:
+if (sparse)
+got = st->driver->streamSendOffset(st, streamOffset, pattern + 
offset, want);
+else
+got = st->driver->streamSend(st, pattern + offset, want);
 if (got < 0) {
 if (got == -2 && !blocking) {
 usleep(20 * 1000);
@@ -272,6 +281,10 @@ static int testFDStreamWriteCommon(const char *scratchdir, 
const unsigned int fl
   virGetLastErrorMessage());
 goto cleanup;
 }
+if (i == 0)
+streamOffset = 8192 + PATTERN_LEN;
+else
+streamOffset += 8192 + PATTERN_LEN;
 offset += got;
 want -= got;
 }
@@ -286,6 +299,7 @@ static int testFDStreamWriteCommon(const char *scratchdir, 
const unsigned int fl
 if ((fd = open(file, O_RDONLY)) < 0)
 goto cleanup;
 
+streamOffset = 0;
 for (i = 0; i < 10; i++) {
 size_t want;
 if (i == 9)
@@ -293,6 +307,12 @@ static int testFDStreamWriteCommon(const char *scratchdir, 
const unsigned int fl
 else
 want = PATTERN_LEN;
 
+if (sparse &&
+lseek(fd, streamOffset, SEEK_SET) < 0) {
+virFilePrintf(stderr, "unable to seek");
+goto cleanup;
+}
+
 if (saferead(fd, buf, want) != want) {
 virFilePrintf(stderr, "Short read from data\n");
 goto cleanup;
@@ -321,6 +341,11 @@ static int testFDStreamWriteCommon(const char *scratchdir, 
const unsigned int fl
 goto cleanup;
 }
 }
+
+if (i == 0)
+streamOffset = 8192 + PATTERN_LEN;
+else
+streamOffset += 8192 + PATTERN_LEN;
 }
 
 if (VIR_CLOSE(fd) < 0)
@@ -350,6 +375,10 @@ static int testFDStreamWriteNonblock(const void *data)
 {
 return testFDStreamWriteCommon(data, false);
 }
+static int testFDStreamSparseWriteBlock(const void *data)
+{
+return testFDStreamWriteCommon(data, VIR_STREAM_SPARSE);
+}
 
 #define SCRATCHDIRTEMPLATE abs_builddir "/fakesysfsdir-XX"
 
@@ -374,6 +403,8 @@ mymain(void)
 ret = -1;
 if (virtTestRun("Sparse stream read blocking ", 
testFDStreamSparseReadBlock, scratchdir) < 0)
 ret = -1;
+if (virtTestRun("Sparse stream write blocking ", 
testFDStreamSparseWriteBlock, scratchdir) < 0)
+ret = -1;
 
 if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL)
 virFileDeleteTree(scratchdir);
-- 
2.4.10

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

[libvirt] [PATCH RFC 1/8] fdstream: Realign

[Michal Privoznik]

Some lines in this file are misaligned which fires up my OCD.

Signed-off-by: Michal Privoznik 
---
 src/fdstream.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/fdstream.c b/src/fdstream.c
index b8ea86e..a85cf9d 100644
--- a/src/fdstream.c
+++ b/src/fdstream.c
@@ -215,10 +215,10 @@ virFDStreamAddCallback(virStreamPtr st,
 }
 
 if ((fdst->watch = virEventAddHandle(fdst->fd,
-   events,
-   virFDStreamEvent,
-   st,
-   virFDStreamCallbackFree)) < 0) {
+ events,
+ virFDStreamEvent,
+ st,
+ virFDStreamCallbackFree)) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
"%s", _("cannot register file watch on stream"));
 goto cleanup;
@@ -624,7 +624,7 @@ virFDStreamOpenFileInternal(virStreamPtr st,
  */
 if ((st->flags & VIR_STREAM_NONBLOCK) &&
 ((!S_ISCHR(sb.st_mode) &&
- !S_ISFIFO(sb.st_mode)) || forceIOHelper)) {
+  !S_ISFIFO(sb.st_mode)) || forceIOHelper)) {
 int fds[2] = { -1, -1 };
 
 if ((oflags & O_ACCMODE) == O_RDWR) {
-- 
2.4.10

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

[libvirt] [PATCH RFC 4/8] fdstream: Implement virStreamSendOffset

[Michal Privoznik]

Signed-off-by: Michal Privoznik 
---
 src/fdstream.c | 35 +++
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/src/fdstream.c b/src/fdstream.c
index a85cf9d..403ddf6 100644
--- a/src/fdstream.c
+++ b/src/fdstream.c
@@ -353,10 +353,14 @@ virFDStreamAbort(virStreamPtr st)
 return virFDStreamCloseInt(st, true);
 }
 
-static int virFDStreamWrite(virStreamPtr st, const char *bytes, size_t nbytes)
+static int
+virFDStreamWriteInternal(virStreamPtr st,
+ off_t offset,
+ const char *bytes,
+ size_t nbytes)
 {
 struct virFDStreamData *fdst = st->privateData;
-int ret;
+int ret = -1;
 
 if (nbytes > INT_MAX) {
 virReportSystemError(ERANGE, "%s",
@@ -376,14 +380,20 @@ static int virFDStreamWrite(virStreamPtr st, const char 
*bytes, size_t nbytes)
 if (fdst->length == fdst->offset) {
 virReportSystemError(ENOSPC, "%s",
  _("cannot write to stream"));
-virMutexUnlock(>lock);
-return -1;
+goto cleanup;
 }
 
 if ((fdst->length - fdst->offset) < nbytes)
 nbytes = fdst->length - fdst->offset;
 }
 
+if (offset != (off_t) -1 &&
+lseek(fdst->fd, offset, SEEK_SET) < 0) {
+virReportSystemError(errno, "%s",
+ _("unable to set stream position"));
+goto cleanup;
+}
+
  retry:
 ret = write(fdst->fd, bytes, nbytes);
 if (ret < 0) {
@@ -395,15 +405,31 @@ static int virFDStreamWrite(virStreamPtr st, const char 
*bytes, size_t nbytes)
 ret = -1;
 virReportSystemError(errno, "%s",
  _("cannot write to stream"));
+goto cleanup;
 }
 } else if (fdst->length) {
 fdst->offset += ret;
 }
 
+ cleanup:
 virMutexUnlock(>lock);
 return ret;
 }
 
+static int
+virFDStreamWrite(virStreamPtr st, const char *bytes, size_t nbytes)
+{
+return virFDStreamWriteInternal(st, (off_t) -1, bytes, nbytes);
+}
+
+static int
+virFDStreamWriteOffset(virStreamPtr st,
+   unsigned long long offset,
+   const char *bytes,
+   size_t nbytes)
+{
+return virFDStreamWriteInternal(st, offset, bytes, nbytes);
+}
 
 static int virFDStreamRead(virStreamPtr st, char *bytes, size_t nbytes)
 {
@@ -457,6 +483,7 @@ static int virFDStreamRead(virStreamPtr st, char *bytes, 
size_t nbytes)
 
 static virStreamDriver virFDStreamDrv = {
 .streamSend = virFDStreamWrite,
+.streamSendOffset = virFDStreamWriteOffset,
 .streamRecv = virFDStreamRead,
 .streamFinish = virFDStreamClose,
 .streamAbort = virFDStreamAbort,
-- 
2.4.10

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

[libvirt] [PATCH RFC 6/8] fdstreamtest: switch from boolean array to @flags

[Michal Privoznik]

This is no functional change, it just merely prepares
environment for next patch.

Signed-off-by: Michal Privoznik 
---
 tests/fdstreamtest.c | 19 +--
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/tests/fdstreamtest.c b/tests/fdstreamtest.c
index 56ba5d9..f9caebf 100644
--- a/tests/fdstreamtest.c
+++ b/tests/fdstreamtest.c
@@ -40,7 +40,8 @@ VIR_LOG_INIT("tests.fdstreamtest");
 
 #define PATTERN_LEN 256
 
-static int testFDStreamReadCommon(const char *scratchdir, bool blocking)
+static int
+testFDStreamReadCommon(const char *scratchdir, const unsigned int flags)
 {
 int fd = -1;
 char *file = NULL;
@@ -50,10 +51,9 @@ static int testFDStreamReadCommon(const char *scratchdir, 
bool blocking)
 virStreamPtr st = NULL;
 size_t i;
 virConnectPtr conn = NULL;
-int flags = 0;
+bool blocking = !(flags & VIR_STREAM_NONBLOCK);
 
-if (!blocking)
-flags |= VIR_STREAM_NONBLOCK;
+virCheckFlags(VIR_STREAM_NONBLOCK, -1);
 
 if (!(conn = virConnectOpen("test:///default")))
 goto cleanup;
@@ -164,15 +164,15 @@ static int testFDStreamReadCommon(const char *scratchdir, 
bool blocking)
 
 static int testFDStreamReadBlock(const void *data)
 {
-return testFDStreamReadCommon(data, true);
+return testFDStreamReadCommon(data, 0);
 }
 static int testFDStreamReadNonblock(const void *data)
 {
-return testFDStreamReadCommon(data, false);
+return testFDStreamReadCommon(data, VIR_STREAM_NONBLOCK);
 }
 
 
-static int testFDStreamWriteCommon(const char *scratchdir, bool blocking)
+static int testFDStreamWriteCommon(const char *scratchdir, const unsigned int 
flags)
 {
 int fd = -1;
 char *file = NULL;
@@ -182,10 +182,9 @@ static int testFDStreamWriteCommon(const char *scratchdir, 
bool blocking)
 virStreamPtr st = NULL;
 size_t i;
 virConnectPtr conn = NULL;
-int flags = 0;
+bool blocking = !(flags & VIR_STREAM_NONBLOCK);
 
-if (!blocking)
-flags |= VIR_STREAM_NONBLOCK;
+virCheckFlags(VIR_STREAM_NONBLOCK, -1);
 
 if (!(conn = virConnectOpen("test:///default")))
 goto cleanup;
-- 
2.4.10

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

Re: [libvirt] [PATCH 29/34] conf: Don't store vcpusched orthogonally to other vcpu info

[Peter Krempa]

On Mon, Jan 18, 2016 at 18:06:22 -0500, John Ferlan wrote:
> On 01/14/2016 11:27 AM, Peter Krempa wrote:

[...]

> > +virDomainDefGetVcpuSched(virDomainDefPtr def,
> > + unsigned int vcpu)
> > +{
> > +virDomainVcpuInfoPtr vcpuinfo;
> > +
> > +if (!(vcpuinfo = virDomainDefGetVcpu(def, vcpu)))
> > +return NULL;
> 
> Does there need to be a vcpuinfo->online check?  (aka OnlineVcpuMap)

No. If you look carefully you'll notice that the scheduler can be
specified also for offline vCPUs and is applied when cpus are onlined.

I'm planing to do the same for the pinning bitmaps in part 3.

> 
> Will the caller need to differentiate?  I know this is the parsing
> code... just thinking while typing especially for non-static function.
> Later thoughts made me think this should be static for parse...

Yes it's not used anywhere else. I don't remember why I didn't make it
static.

> 


> > @@ -14592,6 +14601,55 @@ virDomainSchedulerParse(xmlNodePtr node,
> > 
> > 
> >  static int
> > +virDomainThreadSchedParseHelper(xmlNodePtr node,
> > +const char *name,
> > +virDomainThreadSchedParamPtr 
> > (*func)(virDomainDefPtr, unsigned int),
> > +virDomainDefPtr def)
> > +{
> > +ssize_t next = -1;
> > +virBitmapPtr map = NULL;
> > +virDomainThreadSchedParamPtr sched;
> > +virProcessSchedPolicy policy;
> > +int priority;
> > +int ret = -1;
> > +
> > +if (!(map = virDomainSchedulerParse(node, name, , )))
> > +goto cleanup;
> > +
> 
> Replacing the virBitmapOverlaps...
> 
> > +while ((next = virBitmapNextSetBit(map, next)) > -1) {
> > +if (!(sched = func(def, next)))
> > +goto cleanup;
> 
> Could this be 'continue;' ?  That is, is the data required?  I'm
> thinking primarily of the vcpu->online == false case...

No, it's invalid to specify it for a non-existant object, but valid for
offline one.

> 
> > +
> > +if (sched->policy != VIR_PROC_POLICY_NONE) {
> > +virReportError(VIR_ERR_XML_DETAIL,
> > +   _("%ssched attributes 'vcpus' must not 
> > overlap"),
> 
> Since the code will be shared in patch 30, change message to:
> 
> "%sched attribute '%s' must not overlap",

Indeed, I forgot to fix this.

> 
> using 'name' for both %s params. Similar to virDomainFormatSchedDef has
> done things.
> 
> > +   name);
> > +goto cleanup;

[...]


> > @@ -21504,6 +21543,128 @@ 
> > virDomainDefHasCapabilitiesFeatures(virDomainDefPtr def)
> > 
> > 
> 
> Probably a good place to note the arguments, but specifically that
> "name" is used to generate the XML " 
> >  static int
> > +virDomainFormatSchedDef(virDomainDefPtr def,
> > +virBufferPtr buf,
> > +const char *name,
> > +virDomainThreadSchedParamPtr 
> > (*func)(virDomainDefPtr, unsigned int),
> > +virBitmapPtr resourceMap)
> > +

[...]

> > +virBufferAddLit(buf, "/>\n");
> > +
> > +/* subtract all vCPUs that were already found */
> > +virBitmapSubtract(schedMap, currentMap);
> > +}
> > +}
> 
> This is one heckuva loop - I hope others can look as well because my
> eyes and brain decided to run in the other direction ;-)

Well the loop is complex because the original design is orthogonal. If
it was designed properly, it would be quite a lot simpler.

> 
> > +
> > +ret = 0;
> > +
> > + cleanup:
> > +virBitmapFree(schedMap);
> > +virBitmapFree(prioMap);
> > +return ret;
> > +}
> > +
> > +
> > +static int
> > +virDomainFormatVcpuSchedDef(virDomainDefPtr def,
> > +virBufferPtr buf)
> > +{
> > +virBitmapPtr allcpumap;
> > +int ret;
> > +
> > +if (virDomainDefGetVcpusMax(def) == 0)
> > +return 0;
> 
> Hmm... a zero for maxvcpus...  In patch 2 we disallow machines with 0
> vcpus online... Just a strange check.

Just in qemu. Some other hypervisors think 0 is the right way to
describe maximum host vcpus which would make this crash.

> 
> > +
> > +if (!(allcpumap = virBitmapNew(virDomainDefGetVcpusMax(def
> 
> use of same function - although I assume the compiler will optimize that
> for you anyway...
> 
> > +return -1;
> > +
> > +virBitmapSetAll(allcpumap);
> > +
> > +ret = virDomainFormatSchedDef(def, buf, "vcpu", 
> > virDomainDefGetVcpuSched,
> > +  allcpumap);
> 
> This differs slightly from Parse which uses "vcpus" - I'm wondering if

The parser historically used "vcpus" or "iothreads" I kept it there that
way but I don't like it so I made this as it should be.

> it should be consistent.  At the very least documented...

I'll probably change the parser code to drop the extra s which can be
constructed internally.

Peter



signature.asc
Description: Digital signature

Re: [libvirt] [PATCH 4/4] vz: fix race condition when adding domain to domains list

[Maxim Nestratov]

28.01.2016 18:38, Mikhail Feoktistov пишет:

Race condition:
User calls defineXML to create new instance.
The main thread from vzDomainDefineXMLFlags() creates new instance by 
prlsdkCreateVm.
Then this thread calls prlsdkAddDomain to add new domain to domains list.
The second thread receives notification from hypervisor that new VM was created.
It calls prlsdkHandleVmAddedEvent() and also tries to add new domain to domains 
list.
These two threads call virDomainObjListFindByUUID() from prlsdkAddDomain() and 
don't find new domain.
So they add two domains with the same uuid to domains list.

This fix splits logic of prlsdkAddDomain() into two functions.
1. prlsdkNewDomain() creates new empty domain in domains list with the specific 
uuid.
2. prlsdkLoadDomain() add data from VM to domain object.

New algorithm for creating an instance:
In vzDomainDefineXMLFlags() we add new domain to domain list by calling 
prlsdkNewDomain()
and only after that we call CreateVm() to create VM.
It means that we "reserve" domain object with the specific uuid.
After creation of new VM we add info from this VM
to reserved domain object by calling prlsdkLoadDomain().

Before this patch prlsdkLoadDomain() worked in 2 different cases:
1. It creates and initializes new domain. Then updates it from sdk handle.
2. It updates existed domain from sdk handle.
In this patch we remove code which creates new domain from LoadDomain()
and move it to prlsdkNewDomain().
Now prlsdkLoadDomain() only updates domain from skd handle.

In notification handler prlsdkHandleVmAddedEvent() we check
the existence of a domain and if it doesn't exist we add new domain by calling
prlsdkNewDomain() and load info from sdk handle via prlsdkLoadDomain().
---
  src/vz/vz_driver.c |  13 ++-
  src/vz/vz_sdk.c| 253 +++--
  src/vz/vz_sdk.h|   9 +-
  3 files changed, 146 insertions(+), 129 deletions(-)

diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index 91a48b6..521efd4 100644
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -685,6 +685,7 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, 
unsigned int flags)
  virDomainPtr retdom = NULL;
  virDomainDefPtr def;
  virDomainObjPtr olddom = NULL;
+virDomainObjPtr newdom = NULL;
  unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
  
  virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL);

@@ -700,6 +701,9 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, 
unsigned int flags)
  olddom = virDomainObjListFindByUUID(privconn->domains, def->uuid);
  if (olddom == NULL) {
  virResetLastError();
+newdom = prlsdkNewDomain(privconn, def->name, def->uuid);
+if (!newdom)
+goto cleanup;
  if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
  if (prlsdkCreateVm(conn, def))
  goto cleanup;
@@ -713,8 +717,7 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, 
unsigned int flags)
  goto cleanup;
  }
  
-olddom = prlsdkAddDomain(privconn, def->uuid);

-if (!olddom)
+if (prlsdkLoadDomain(privconn, newdom))
  goto cleanup;
  } else {
  int state, reason;
@@ -755,6 +758,12 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char 
*xml, unsigned int flags)
   cleanup:
  if (olddom)
  virObjectUnlock(olddom);
+if (newdom) {
+if (!retdom)
+ virDomainObjListRemove(privconn->domains, newdom);
+else
+ virObjectUnlock(newdom);
+}
  virDomainDefFree(def);
  vzDriverUnlock(privconn);
  return retdom;
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 6fb2a97..9d2bdab 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -1236,58 +1236,35 @@ prlsdkConvertCpuMode(PRL_HANDLE sdkdom, virDomainDefPtr 
def)
  return -1;
  }
  
-/*

- * This function retrieves information about domain.
- * If the domains is already in the domains list
- * privconn->domains, then locked 'olddom' must be
- * provided. If the domains must be added to the list,
- * olddom must be NULL.
- *
- * The function return a pointer to a locked virDomainObj.
- */
-static virDomainObjPtr
-prlsdkLoadDomain(vzConnPtr privconn,
- PRL_HANDLE sdkdom,
- virDomainObjPtr olddom)
+int
+prlsdkLoadDomain(vzConnPtr privconn, virDomainObjPtr dom)
  {
-virDomainObjPtr dom = NULL;
  virDomainDefPtr def = NULL;
-vzDomObjPtr pdom = NULL;
  VIRTUAL_MACHINE_STATE domainState;
+char *home = NULL;
  
  PRL_UINT32 buflen = 0;

  PRL_RESULT pret;
  PRL_UINT32 ram;
  PRL_UINT32 envId;
  PRL_VM_AUTOSTART_OPTION autostart;
+PRL_HANDLE sdkdom = PRL_INVALID_HANDLE;
+vzDomObjPtr pdom = dom->privateData;
  
  virCheckNonNullArgGoto(privconn, error);

-virCheckNonNullArgGoto(sdkdom, error);
+virCheckNonNullArgGoto(dom, error);
+
+sdkdom = prlsdkSdkDomainLookupByUUID(privconn, 

Re: [libvirt] [PATCH RFC 0/8] Sparse streams

[Martin Kletzander]

On Fri, Jan 29, 2016 at 02:26:51PM +0100, Michal Privoznik wrote:

** NOT TO BE MERGED UPSTREAM **

This is merely an RFC.

What's the problem?
We have APIs for transferring disk images from/to host. Problem is, disk images
can be sparse files. Our code is, however, unaware of that fact so if for
instance the disk is one big hole of 8GB all those bytes have to: a) be read b)
come through our event loop. This is obviously very inefficient way.

How to deal with it?
The way I propose (and this is actually what I like you to comment on) is to
invent set of new API. We need to make read from and write to a stream
sparseness aware. The new APIs are as follows:

 int virStreamSendOffset(virStreamPtr stream,
 unsigned long long offset,
 const char *data,
 size_t nbytes);

 int virStreamRecvOffset(virStreamPtr stream,
 unsigned long long *offset,
 char *data,
 size_t nbytes);

The SendOffset() is fairly simple. It is given an offset to write @data from so
it basically lseek() to @offset and write() data.
The RecvOffset() has slightly complicated design - it has to be aware of the
fact that @offset it is required to read from fall into a hole. If that's the
case it sets @offset to new location where data starts.

Are there other ways possible?
Sure! If you have any specific in mind I am happy to discuss it. For instance
one idea I've heard (from Martin) was instead of SendOffset() and RecvOffset()
we may just invent our variant of lseek().



Also StreamRecv (and send) that would return the number of bytes read
(skipped) in a parameter and the return value itself would be an enum
saying whether that read ended up in a hole or end of file or it was
success.

One more idea was to have a a way of registering a callback that would
be called with offsets and buffers and the holes would be skipped that
way.


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

[libvirt] [PATCH RFC 5/8] fdstream: Implement virStreamRecvOffset

[Michal Privoznik]

Signed-off-by: Michal Privoznik 
---
 src/fdstream.c | 105 ++---
 1 file changed, 101 insertions(+), 4 deletions(-)

diff --git a/src/fdstream.c b/src/fdstream.c
index 403ddf6..20eda07 100644
--- a/src/fdstream.c
+++ b/src/fdstream.c
@@ -431,10 +431,15 @@ virFDStreamWriteOffset(virStreamPtr st,
 return virFDStreamWriteInternal(st, offset, bytes, nbytes);
 }
 
-static int virFDStreamRead(virStreamPtr st, char *bytes, size_t nbytes)
+static int
+virFDStreamReadInternal(virStreamPtr st,
+off_t *offset,
+char *bytes,
+size_t nbytes)
 {
 struct virFDStreamData *fdst = st->privateData;
-int ret;
+int ret = -1;
+off_t cur, data, hole;
 
 if (nbytes > INT_MAX) {
 virReportSystemError(ERANGE, "%s",
@@ -450,10 +455,84 @@ static int virFDStreamRead(virStreamPtr st, char *bytes, 
size_t nbytes)
 
 virMutexLock(>lock);
 
+if (offset) {
+/* Firstly, seek to desired position. */
+if ((cur = lseek(fdst->fd, *offset, SEEK_SET)) == (off_t) -1) {
+virReportSystemError(errno, "%s",
+ _("unable to set stream position"));
+goto cleanup;
+}
+
+/* Now try to detect if @cur is in hole or in data. There are four
+ * options:
+ * 1) data == cur; @cur is in data
+ * 2) data > cur; @cur is in a hole, next data at @data
+ * 3) data < 0, errno = ENXIO; either @cur is in trailing hole or @cur
+ *is beyond EOF.
+ * 4) data < 0, errno != ENXIO; we learned nothing
+ */
+
+if ((data = lseek(fdst->fd, cur, SEEK_DATA)) == (off_t) -1) {
+/* case 3 and 4 */
+if (errno != ENXIO) {
+virReportSystemError(errno, "%s",
+ _("unable to get data position"));
+} else {
+ret = 0;
+}
+goto cleanup;
+}
+
+if (data > cur) {
+/* case 2 */
+*offset = data;
+fdst->offset = *offset;
+ret = 0;
+goto cleanup;
+} else {
+/* case 1 */
+}
+
+/* Now, we must ensure we will not read out of data boundaries.
+ * Get position of the next hole. Cases are the same as previously. */
+
+if ((hole = lseek(fdst->fd, cur, SEEK_HOLE)) == (off_t) -1) {
+/* Interesting, so we are in data, but failed to seek to next hole.
+ * There's an implicit hole at EOF, if no is to be found earlier.
+ * This is obviously an error so we merge case 3 and 4. */
+virReportSystemError(errno, "%s",
+ _("unable to get hole position"));
+goto cleanup;
+}
+
+if (hole == cur) {
+/* Interesting, so the code above ensures @cur is in data, but now
+ * we found out it's in hole too. This shouldn't happen, but it's
+ * safer to error out. */
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("stream is in both data (%llu) and hole (%llu)"),
+   (unsigned long long) data,
+   (unsigned long long) hole);
+goto cleanup;
+}
+
+if (nbytes > (hole - cur))
+nbytes = hole - cur;
+
+/* We may possibly have moved to a hole. Restore original position. */
+if ((cur = lseek(fdst->fd, *offset, SEEK_SET)) == (off_t) -1) {
+virReportSystemError(errno, "%s",
+ _("unable to set stream position"));
+goto cleanup;
+}
+
+fdst->offset = *offset;
+}
+
 if (fdst->length) {
 if (fdst->length == fdst->offset) {
-virMutexUnlock(>lock);
-return 0;
+ret = 0;
+goto cleanup;
 }
 
 if ((fdst->length - fdst->offset) < nbytes)
@@ -471,20 +550,38 @@ static int virFDStreamRead(virStreamPtr st, char *bytes, 
size_t nbytes)
 ret = -1;
 virReportSystemError(errno, "%s",
  _("cannot read from stream"));
+goto cleanup;
 }
 } else if (fdst->length) {
 fdst->offset += ret;
 }
 
+ cleanup:
 virMutexUnlock(>lock);
 return ret;
 }
 
+static int
+virFDStreamRead(virStreamPtr st, char *bytes, size_t nbytes)
+{
+return virFDStreamReadInternal(st, NULL, bytes, nbytes);
+}
+
+static int
+virFDStreamReadOffset(virStreamPtr st,
+  unsigned long long *offset,
+  char *bytes,
+  size_t nbytes)
+{
+return virFDStreamReadInternal(st, (off_t *) offset, bytes, nbytes);
+}
+
 
 static virStreamDriver virFDStreamDrv = {
 .streamSend = virFDStreamWrite,
 .streamSendOffset = 

Re: [libvirt] [PATCH 4/4] vz: fix race condition when adding domain to domains list

[Mikhail Feoktistov]

29.01.2016 17:39, Maxim Nestratov пишет:


28.01.2016 18:38, Mikhail Feoktistov пишет:

Race condition:
User calls defineXML to create new instance.
The main thread from vzDomainDefineXMLFlags() creates new instance by 
prlsdkCreateVm.
Then this thread calls prlsdkAddDomain to add new domain to domains 
list.
The second thread receives notification from hypervisor that new VM 
was created.
It calls prlsdkHandleVmAddedEvent() and also tries to add new domain 
to domains list.
These two threads call virDomainObjListFindByUUID() from 
prlsdkAddDomain() and don't find new domain.

So they add two domains with the same uuid to domains list.

This fix splits logic of prlsdkAddDomain() into two functions.
1. prlsdkNewDomain() creates new empty domain in domains list with 
the specific uuid.

2. prlsdkLoadDomain() add data from VM to domain object.

New algorithm for creating an instance:
In vzDomainDefineXMLFlags() we add new domain to domain list by 
calling prlsdkNewDomain()

and only after that we call CreateVm() to create VM.
It means that we "reserve" domain object with the specific uuid.
After creation of new VM we add info from this VM
to reserved domain object by calling prlsdkLoadDomain().

Before this patch prlsdkLoadDomain() worked in 2 different cases:
1. It creates and initializes new domain. Then updates it from sdk 
handle.

2. It updates existed domain from sdk handle.
In this patch we remove code which creates new domain from LoadDomain()
and move it to prlsdkNewDomain().
Now prlsdkLoadDomain() only updates domain from skd handle.

In notification handler prlsdkHandleVmAddedEvent() we check
the existence of a domain and if it doesn't exist we add new domain 
by calling

prlsdkNewDomain() and load info from sdk handle via prlsdkLoadDomain().
---
  src/vz/vz_driver.c |  13 ++-
  src/vz/vz_sdk.c| 253 
+++--

  src/vz/vz_sdk.h|   9 +-
  3 files changed, 146 insertions(+), 129 deletions(-)

diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index 91a48b6..521efd4 100644
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -685,6 +685,7 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const 
char *xml, unsigned int flags)

  virDomainPtr retdom = NULL;
  virDomainDefPtr def;
  virDomainObjPtr olddom = NULL;
+virDomainObjPtr newdom = NULL;
  unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL);
@@ -700,6 +701,9 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const 
char *xml, unsigned int flags)

  olddom = virDomainObjListFindByUUID(privconn->domains, def->uuid);
  if (olddom == NULL) {
  virResetLastError();
+newdom = prlsdkNewDomain(privconn, def->name, def->uuid);
+if (!newdom)
+goto cleanup;
  if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
  if (prlsdkCreateVm(conn, def))
  goto cleanup;
@@ -713,8 +717,7 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const 
char *xml, unsigned int flags)

  goto cleanup;
  }
  -olddom = prlsdkAddDomain(privconn, def->uuid);
-if (!olddom)
+if (prlsdkLoadDomain(privconn, newdom))
  goto cleanup;
  } else {
  int state, reason;
@@ -755,6 +758,12 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const 
char *xml, unsigned int flags)

   cleanup:
  if (olddom)
  virObjectUnlock(olddom);
+if (newdom) {
+if (!retdom)
+ virDomainObjListRemove(privconn->domains, newdom);
+else
+ virObjectUnlock(newdom);
+}
  virDomainDefFree(def);
  vzDriverUnlock(privconn);
  return retdom;
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 6fb2a97..9d2bdab 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -1236,58 +1236,35 @@ prlsdkConvertCpuMode(PRL_HANDLE sdkdom, 
virDomainDefPtr def)

  return -1;
  }
  -/*
- * This function retrieves information about domain.
- * If the domains is already in the domains list
- * privconn->domains, then locked 'olddom' must be
- * provided. If the domains must be added to the list,
- * olddom must be NULL.
- *
- * The function return a pointer to a locked virDomainObj.
- */
-static virDomainObjPtr
-prlsdkLoadDomain(vzConnPtr privconn,
- PRL_HANDLE sdkdom,
- virDomainObjPtr olddom)
+int
+prlsdkLoadDomain(vzConnPtr privconn, virDomainObjPtr dom)
  {
-virDomainObjPtr dom = NULL;
  virDomainDefPtr def = NULL;
-vzDomObjPtr pdom = NULL;
  VIRTUAL_MACHINE_STATE domainState;
+char *home = NULL;
PRL_UINT32 buflen = 0;
  PRL_RESULT pret;
  PRL_UINT32 ram;
  PRL_UINT32 envId;
  PRL_VM_AUTOSTART_OPTION autostart;
+PRL_HANDLE sdkdom = PRL_INVALID_HANDLE;
+vzDomObjPtr pdom = dom->privateData;
virCheckNonNullArgGoto(privconn, error);
-virCheckNonNullArgGoto(sdkdom, error);
+virCheckNonNullArgGoto(dom, error);
+
+   

Re: [libvirt] [PATCH 29/34] conf: Don't store vcpusched orthogonally to other vcpu info

[Peter Krempa]

On Sat, Jan 16, 2016 at 10:23:13 -0500, John Ferlan wrote:
> 
> 
> On 01/14/2016 11:27 AM, Peter Krempa wrote:
> > Due to bad design the vcpu sched element is orthogonal to the way how
> > the data belongs to the corresponding objects. Now that vcpus are a
> > struct that allow to store other info too, let's convert the data to the
> > sane structure.
> > 
> > The helpers for the conversion are made universal so that they can be
> > reused for iothreads too.
> > 
> > This patch also resolves https://bugzilla.redhat.com/show_bug.cgi?id=1235180
> > since with the correct storage approach you can't have dangling data.
> > ---
> >  src/conf/domain_conf.c  | 231 
> > +++-
> >  src/conf/domain_conf.h  |   8 +-
> >  src/qemu/qemu_driver.c  |   6 +-
> >  src/qemu/qemu_process.c |   8 +-
> >  4 files changed, 202 insertions(+), 51 deletions(-)
> > 

[...]

> > +for (i = VIR_PROC_POLICY_NONE + 1; i < VIR_PROC_POLICY_LAST; i++) {
> > +virBitmapClearAll(schedMap);
> > +
> > +/* find vcpus using a particular scheduler */
> > +next = -1;
> > +while ((next = virBitmapNextSetBit(resourceMap, next)) > -1) {
> > +sched = func(def, next);
> > +
> > +if (sched->policy == i)
> > +ignore_value(virBitmapSetBit(schedMap, next));
> > +}
> > +
> > +/* it's necessary to discriminate priority levels for schedulers 
> > that
> > + * have them */
> > +while (!virBitmapIsAllClear(schedMap)) {

So start of this loop guarantees, that theres at least one element in
the bitmap ...

> > +virBitmapPtr currentMap = NULL;
> > +ssize_t nextprio;
> > +bool hasPriority = false;
> > +int priority;
> > +
> > +switch ((virProcessSchedPolicy) i) {
> > +case VIR_PROC_POLICY_NONE:
> > +case VIR_PROC_POLICY_BATCH:
> > +case VIR_PROC_POLICY_IDLE:
> > +case VIR_PROC_POLICY_LAST:
> > +currentMap = schedMap;
> > +break;
> > +
> > +case VIR_PROC_POLICY_FIFO:
> > +case VIR_PROC_POLICY_RR:
> > +virBitmapClearAll(prioMap);
> > +hasPriority = true;
> > +
> > +/* we need to find a subset of vCPUs with the given 
> > scheduler
> > + * that share the priority */
> > +nextprio = virBitmapNextSetBit(schedMap, -1);
> 
> Coverity notes that virBitmapNextSetBit can return -1; however, [1]

... thus this won't return -1 in any case here. Coverity is obviously
wrong as usual since it's terrible at introspecting the bitmap code.

> 
> > +sched = func(def, nextprio);
> > +priority = sched->priority;
> > +
> > +ignore_value(virBitmapSetBit(prioMap, nextprio));
> 
> [1] passing a -1 'nextprio' to virBitmapSetBit as 'size_t b' cannot happen.

So this doesn't make sense.

Peter


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

Re: [libvirt] [PATCH 1/4] vz: make output arguments in prlsdkGetDomainIds as optional

[Mikhail Feoktistov]

prlsdkGetDomainIds() returns name and uuid for specified instance.
Now output arguments can be NULL.
It allows to get only necessary info(name or uuid).
---
  src/vz/vz_sdk.c | 34 +++---
  1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 7973d6a..8181149 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -346,25 +346,29 @@ prlsdkGetDomainIds(PRL_HANDLE sdkdom,
  PRL_UINT32 len;
  PRL_RESULT pret;
  
-len = 0;

-/* get name length */
-pret = PrlVmCfg_GetName(sdkdom, NULL, );
-prlsdkCheckRetGoto(pret, error);
+if (name) {
+len = 0;
+/* get name length */
+pret = PrlVmCfg_GetName(sdkdom, NULL, );
+prlsdkCheckRetGoto(pret, error);
  
-if (VIR_ALLOC_N(*name, len) < 0)

-goto error;
+if (VIR_ALLOC_N(*name, len) < 0)
+goto error;
  
-PrlVmCfg_GetName(sdkdom, *name, );

-prlsdkCheckRetGoto(pret, error);
+PrlVmCfg_GetName(sdkdom, *name, );
+prlsdkCheckRetGoto(pret, error);
+}
  
-len = sizeof(uuidstr);

-PrlVmCfg_GetUuid(sdkdom, uuidstr, );
-prlsdkCheckRetGoto(pret, error);
+if (uuid) {
+len = sizeof(uuidstr);
+PrlVmCfg_GetUuid(sdkdom, uuidstr, );
+prlsdkCheckRetGoto(pret, error);
  
-if (prlsdkUUIDParse(uuidstr, uuid) < 0) {

-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Domain UUID is malformed or empty"));
-goto error;
+if (prlsdkUUIDParse(uuidstr, uuid) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Domain UUID is malformed or empty"));
+goto error;
+}
  }
  
  return 0;



ACK, but I would add *name = NULL as another patch or we depend on
caller setting it for us if we follow error path.

Done. Thanks!

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

Re: [libvirt] [PATCH 1/4] vz: make output arguments in prlsdkGetDomainIds as optional

[Mikhail Feoktistov]

29.01.2016 11:25, Maxim Nestratov пишет:

28.01.2016 18:38, Mikhail Feoktistov пишет:

prlsdkGetDomainIds() returns name and uuid for specified instance.
Now output arguments can be NULL.
It allows to get only necessary info(name or uuid).
---
  src/vz/vz_sdk.c | 34 +++---
  1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 7973d6a..8181149 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -346,25 +346,29 @@ prlsdkGetDomainIds(PRL_HANDLE sdkdom,
  PRL_UINT32 len;
  PRL_RESULT pret;
  -len = 0;
-/* get name length */
-pret = PrlVmCfg_GetName(sdkdom, NULL, );
-prlsdkCheckRetGoto(pret, error);
+if (name) {
+len = 0;
+/* get name length */
+pret = PrlVmCfg_GetName(sdkdom, NULL, );
+prlsdkCheckRetGoto(pret, error);
  -if (VIR_ALLOC_N(*name, len) < 0)
-goto error;
+if (VIR_ALLOC_N(*name, len) < 0)
+goto error;
  -PrlVmCfg_GetName(sdkdom, *name, );
-prlsdkCheckRetGoto(pret, error);
+PrlVmCfg_GetName(sdkdom, *name, );


I know it's not your mistake, but it would be better not to ignore 
what PrlVmCfg_GetName returns.

Otherwise next statement has no sence.

Yes, of cource. Thanks!

+prlsdkCheckRetGoto(pret, error);
+}
  -len = sizeof(uuidstr);
-PrlVmCfg_GetUuid(sdkdom, uuidstr, );
-prlsdkCheckRetGoto(pret, error);
+if (uuid) {
+len = sizeof(uuidstr);
+PrlVmCfg_GetUuid(sdkdom, uuidstr, );


The same is here.

+prlsdkCheckRetGoto(pret, error);
  -if (prlsdkUUIDParse(uuidstr, uuid) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Domain UUID is malformed or empty"));
-goto error;
+if (prlsdkUUIDParse(uuidstr, uuid) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Domain UUID is malformed or empty"));
+goto error;
+}
  }
return 0;




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

Re: [libvirt] [PATCH 4/4] vz: fix race condition when adding domain to domains list

[Nikolay Shirokovskiy]

On 28.01.2016 18:38, Mikhail Feoktistov wrote:
> Race condition:
> User calls defineXML to create new instance.
> The main thread from vzDomainDefineXMLFlags() creates new instance by 
> prlsdkCreateVm.
> Then this thread calls prlsdkAddDomain to add new domain to domains list.
> The second thread receives notification from hypervisor that new VM was 
> created.
> It calls prlsdkHandleVmAddedEvent() and also tries to add new domain to 
> domains list.
> These two threads call virDomainObjListFindByUUID() from prlsdkAddDomain() 
> and don't find new domain.
> So they add two domains with the same uuid to domains list.
> 
> This fix splits logic of prlsdkAddDomain() into two functions.
> 1. prlsdkNewDomain() creates new empty domain in domains list with the 
> specific uuid.
> 2. prlsdkLoadDomain() add data from VM to domain object.
> 
> New algorithm for creating an instance:
> In vzDomainDefineXMLFlags() we add new domain to domain list by calling 
> prlsdkNewDomain()
> and only after that we call CreateVm() to create VM.
> It means that we "reserve" domain object with the specific uuid.
> After creation of new VM we add info from this VM
> to reserved domain object by calling prlsdkLoadDomain().
> 
> Before this patch prlsdkLoadDomain() worked in 2 different cases:
> 1. It creates and initializes new domain. Then updates it from sdk handle.
> 2. It updates existed domain from sdk handle.
> In this patch we remove code which creates new domain from LoadDomain()
> and move it to prlsdkNewDomain().
> Now prlsdkLoadDomain() only updates domain from skd handle.
> 
> In notification handler prlsdkHandleVmAddedEvent() we check
> the existence of a domain and if it doesn't exist we add new domain by calling
> prlsdkNewDomain() and load info from sdk handle via prlsdkLoadDomain().
> ---
>  src/vz/vz_driver.c |  13 ++-
>  src/vz/vz_sdk.c| 253 
> +++--
>  src/vz/vz_sdk.h|   9 +-
>  3 files changed, 146 insertions(+), 129 deletions(-)
> 
> diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
> index 91a48b6..521efd4 100644
> --- a/src/vz/vz_driver.c
> +++ b/src/vz/vz_driver.c
> @@ -685,6 +685,7 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char 
> *xml, unsigned int flags)
>  virDomainPtr retdom = NULL;
>  virDomainDefPtr def;
>  virDomainObjPtr olddom = NULL;
> +virDomainObjPtr newdom = NULL;
>  unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
>  
>  virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL);
> @@ -700,6 +701,9 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char 
> *xml, unsigned int flags)
>  olddom = virDomainObjListFindByUUID(privconn->domains, def->uuid);
>  if (olddom == NULL) {
>  virResetLastError();
> +newdom = prlsdkNewDomain(privconn, def->name, def->uuid);
> +if (!newdom)
> +goto cleanup;
>  if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
>  if (prlsdkCreateVm(conn, def))
>  goto cleanup;
> @@ -713,8 +717,7 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char 
> *xml, unsigned int flags)
>  goto cleanup;
>  }
>  
> -olddom = prlsdkAddDomain(privconn, def->uuid);
> -if (!olddom)
> +if (prlsdkLoadDomain(privconn, newdom))
>  goto cleanup;
>  } else {
>  int state, reason;
> @@ -755,6 +758,12 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char 
> *xml, unsigned int flags)
>   cleanup:
>  if (olddom)
>  virObjectUnlock(olddom);
> +if (newdom) {
> +if (!retdom)
> + virDomainObjListRemove(privconn->domains, newdom);
> +else
> + virObjectUnlock(newdom);
> +}
>  virDomainDefFree(def);
>  vzDriverUnlock(privconn);
>  return retdom;
> diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
> index 6fb2a97..9d2bdab 100644
> --- a/src/vz/vz_sdk.c
> +++ b/src/vz/vz_sdk.c
> @@ -1236,58 +1236,35 @@ prlsdkConvertCpuMode(PRL_HANDLE sdkdom, 
> virDomainDefPtr def)
>  return -1;
>  }
>  
> -/*
> - * This function retrieves information about domain.
> - * If the domains is already in the domains list
> - * privconn->domains, then locked 'olddom' must be
> - * provided. If the domains must be added to the list,
> - * olddom must be NULL.
> - *
> - * The function return a pointer to a locked virDomainObj.
> - */
> -static virDomainObjPtr
> -prlsdkLoadDomain(vzConnPtr privconn,
> - PRL_HANDLE sdkdom,
> - virDomainObjPtr olddom)
> +int
> +prlsdkLoadDomain(vzConnPtr privconn, virDomainObjPtr dom)
>  {
> -virDomainObjPtr dom = NULL;
>  virDomainDefPtr def = NULL;
> -vzDomObjPtr pdom = NULL;
>  VIRTUAL_MACHINE_STATE domainState;
> +char *home = NULL;
>  
>  PRL_UINT32 buflen = 0;
>  PRL_RESULT pret;
>  PRL_UINT32 ram;
>  PRL_UINT32 envId;
>  PRL_VM_AUTOSTART_OPTION autostart;
> +PRL_HANDLE sdkdom = PRL_INVALID_HANDLE;
> +

Re: [libvirt] [PATCH 3/4] vz: fix notification subscription

[Maxim Nestratov]

28.01.2016 18:38, Mikhail Feoktistov пишет:

Bug cause:
Update the domain that is subscribed to hypervisor notification.
LoadDomain() rewrites notifications fields in vzDomObj structure and makes domain as 
"unsubscribed".
Fix:
Initialize notification fields in vzDomObj only if we create a new domain.
And do not reinitialize these fields if we update domain (by calling LoadDomain 
with olddom argument)
---
  src/vz/vz_sdk.c | 13 ++---
  1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 40ab2d9..6fb2a97 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -1270,6 +1270,12 @@ prlsdkLoadDomain(vzConnPtr privconn,
  if (!olddom) {
  if (VIR_ALLOC(pdom) < 0)
  goto error;
+pdom->cache.stats = PRL_INVALID_HANDLE;
+pdom->cache.count = -1;
+if (virCondInit(>cache.cond) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot initialize 
condition"));
+goto error;
+}
  } else {
  pdom = olddom->privateData;
  }
@@ -1281,13 +1287,6 @@ prlsdkLoadDomain(vzConnPtr privconn,
  
  def->id = -1;
  
-pdom->cache.stats = PRL_INVALID_HANDLE;

-pdom->cache.count = -1;
-if (virCondInit(>cache.cond) < 0) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot initialize 
condition"));
-goto error;
-}
-
  if (prlsdkGetDomainIds(sdkdom, >name, def->uuid) < 0)
  goto error;
  
Actually I'm not sure this patch is necessary at all as soon as you 
rewrite this function in the next patch.

The problem you are fixing could be just mentioned there.

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

Re: [libvirt] [PATCH 1/4] vz: make output arguments in prlsdkGetDomainIds as optional

[Maxim Nestratov]

28.01.2016 18:38, Mikhail Feoktistov пишет:

prlsdkGetDomainIds() returns name and uuid for specified instance.
Now output arguments can be NULL.
It allows to get only necessary info(name or uuid).
---
  src/vz/vz_sdk.c | 34 +++---
  1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 7973d6a..8181149 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -346,25 +346,29 @@ prlsdkGetDomainIds(PRL_HANDLE sdkdom,
  PRL_UINT32 len;
  PRL_RESULT pret;
  
-len = 0;

-/* get name length */
-pret = PrlVmCfg_GetName(sdkdom, NULL, );
-prlsdkCheckRetGoto(pret, error);
+if (name) {
+len = 0;
+/* get name length */
+pret = PrlVmCfg_GetName(sdkdom, NULL, );
+prlsdkCheckRetGoto(pret, error);
  
-if (VIR_ALLOC_N(*name, len) < 0)

-goto error;
+if (VIR_ALLOC_N(*name, len) < 0)
+goto error;
  
-PrlVmCfg_GetName(sdkdom, *name, );

-prlsdkCheckRetGoto(pret, error);
+PrlVmCfg_GetName(sdkdom, *name, );


I know it's not your mistake, but it would be better not to ignore what 
PrlVmCfg_GetName returns.

Otherwise next statement has no sence.

+prlsdkCheckRetGoto(pret, error);
+}
  
-len = sizeof(uuidstr);

-PrlVmCfg_GetUuid(sdkdom, uuidstr, );
-prlsdkCheckRetGoto(pret, error);
+if (uuid) {
+len = sizeof(uuidstr);
+PrlVmCfg_GetUuid(sdkdom, uuidstr, );


The same is here.

+prlsdkCheckRetGoto(pret, error);
  
-if (prlsdkUUIDParse(uuidstr, uuid) < 0) {

-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("Domain UUID is malformed or empty"));
-goto error;
+if (prlsdkUUIDParse(uuidstr, uuid) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("Domain UUID is malformed or empty"));
+goto error;
+}
  }
  
  return 0;


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

[libvirt] [PATCH v4 6/8] perf: reenable perf events when libvirtd restart

[Qiaowei Ren]

When libvirtd daemon restart, this patch will reenable those perf
events previously enabled.

Signed-off-by: Qiaowei Ren 
---
 src/qemu/qemu_process.c | 34 ++
 1 file changed, 34 insertions(+)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 4595489..8249c3e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3499,6 +3499,34 @@ qemuProcessUpdateDevices(virQEMUDriverPtr driver,
 return ret;
 }
 
+static int
+qemuDomainPerfRestart(virDomainObjPtr vm)
+{
+size_t i;
+virDomainDefPtr def = vm->def;
+qemuDomainObjPrivatePtr priv = vm->privateData;
+
+virPerfFree(priv->perf);
+
+priv->perf = virPerfNew();
+if (!priv->perf)
+return -1;
+
+for (i = 0; i < VIR_PERF_EVENT_LAST; i++) {
+if (def->perf->events[i] &&
+def->perf->events[i] == VIR_TRISTATE_BOOL_YES) {
+if (virPerfEventEnable(priv->perf, i, vm->pid))
+goto cleanup;
+}
+}
+
+return 0;
+
+ cleanup:
+virPerfFree(priv->perf);
+return -1;
+}
+
 struct qemuProcessReconnectData {
 virConnectPtr conn;
 virQEMUDriverPtr driver;
@@ -3584,6 +3612,9 @@ qemuProcessReconnect(void *opaque)
 if (qemuConnectCgroup(driver, obj) < 0)
 goto error;
 
+if (qemuDomainPerfRestart(obj) < 0)
+goto error;
+
 /* XXX: Need to change as long as lock is introduced for
  * qemu_driver->sharedDevices.
  */
@@ -5681,6 +5712,9 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
 if (virSecurityManagerGenLabel(driver->securityManager, vm->def) < 0)
 goto error;
 
+if (qemuDomainPerfRestart(vm) < 0)
+goto error;
+
 VIR_DEBUG("Creating domain log file");
 if (!(logCtxt = qemuDomainLogContextNew(driver, vm,
 
QEMU_DOMAIN_LOG_CONTEXT_MODE_ATTACH)))
-- 
1.9.1

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

[libvirt] [PATCH v4 4/8] qemu_driver: add support to perf event

[Qiaowei Ren]

This patch implement the internal driver API for perf event into
qemu driver.

Signed-off-by: Qiaowei Ren 
---
 include/libvirt/libvirt-domain.h |   1 +
 src/qemu/qemu_domain.h   |   3 +
 src/qemu/qemu_driver.c   | 133 +++
 src/qemu/qemu_process.c  |   6 ++
 4 files changed, 143 insertions(+)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index 5e39bb3..dff260a 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -1771,6 +1771,7 @@ typedef enum {
 VIR_DOMAIN_STATS_VCPU = (1 << 3), /* return domain virtual CPU info */
 VIR_DOMAIN_STATS_INTERFACE = (1 << 4), /* return domain interfaces info */
 VIR_DOMAIN_STATS_BLOCK = (1 << 5), /* return domain block info */
+VIR_DOMAIN_STATS_PERF = (1 << 6), /* return domain perf event info */
 } virDomainStatsTypes;
 
 typedef enum {
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 7fc4fff..2db4342 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -26,6 +26,7 @@
 
 # include "virthread.h"
 # include "vircgroup.h"
+# include "virperf.h"
 # include "domain_addr.h"
 # include "domain_conf.h"
 # include "snapshot_conf.h"
@@ -191,6 +192,8 @@ struct _qemuDomainObjPrivate {
 
 virCgroupPtr cgroup;
 
+virPerfPtr perf;
+
 virCond unplugFinished; /* signals that unpluggingDevice was unplugged */
 const char *unpluggingDevice; /* alias of the device that is being 
unplugged */
 char **qemuDevices; /* NULL-terminated list of devices aliases known to 
QEMU */
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index abcdbe6..58d6015 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -98,6 +98,7 @@
 #include "virhostdev.h"
 #include "domain_capabilities.h"
 #include "vircgroup.h"
+#include "virperf.h"
 #include "virnuma.h"
 #include "dirname.h"
 #include "network/bridge_driver.h"
@@ -10320,6 +10321,86 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
 }
 
 static int
+qemuDomainSetPerfEvents(virDomainPtr dom,
+virTypedParameterPtr params,
+int nparams)
+{
+size_t i;
+virDomainObjPtr vm = NULL;
+qemuDomainObjPrivatePtr priv;
+int ret = -1;
+virPerfEventType type;
+bool enabled;
+
+if (virTypedParamsValidate(params, nparams, VIR_PERF_PARAMETERS) < 0)
+return -1;
+
+if (!(vm = qemuDomObjFromDomain(dom)))
+return -1;
+
+priv = vm->privateData;
+
+if (virDomainSetPerfEventsEnsureACL(dom->conn, vm->def) < 0)
+goto cleanup;
+
+for (i = 0; i < nparams; i++) {
+virTypedParameterPtr param = [i];
+enabled = params->value.b;
+type = virPerfEventTypeFromString(param->field);
+
+if (!enabled && virPerfEventDisable(priv->perf, type))
+goto cleanup;
+if (enabled && virPerfEventEnable(priv->perf, type, vm->pid))
+goto cleanup;
+}
+
+ret = 0;
+
+ cleanup:
+virDomainObjEndAPI();
+return ret;
+}
+
+static int
+qemuDomainGetPerfEvents(virDomainPtr dom,
+virTypedParameterPtr *params,
+int *nparams)
+{
+size_t i;
+virDomainObjPtr vm = NULL;
+qemuDomainObjPrivatePtr priv;
+int ret = -1;
+virTypedParameterPtr par = NULL;
+int maxpar = 0;
+int npar = 0;
+
+if (!(vm = qemuDomObjFromDomain(dom)))
+goto cleanup;
+
+priv = vm->privateData;
+
+if (virDomainGetPerfEventsEnsureACL(dom->conn, vm->def) < 0)
+goto cleanup;
+
+for (i = 0; i < VIR_PERF_EVENT_LAST; i++) {
+if (virTypedParamsAddBoolean(, , ,
+ virPerfEventTypeToString(i),
+ virPerfEventIsEnabled(priv->perf, i)) < 
0) {
+virTypedParamsFree(par, npar);
+goto cleanup;
+}
+}
+
+*params = par;
+*nparams = npar;
+ret = 0;
+
+ cleanup:
+virDomainObjEndAPI();
+return ret;
+}
+
+static int
 qemuSetVcpusBWLive(virDomainObjPtr vm, virCgroupPtr cgroup,
unsigned long long period, long long quota)
 {
@@ -19497,6 +19578,55 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
 
 #undef QEMU_ADD_COUNT_PARAM
 
+static int
+qemuDomainGetStatsPerfCmt(virPerfPtr perf,
+  virDomainStatsRecordPtr record,
+  int *maxparams)
+{
+uint64_t cache = 0;
+
+if (virPerfReadEvent(perf, VIR_PERF_EVENT_CMT, ) < 0)
+return -1;
+
+if (virTypedParamsAddULLong(>params,
+>nparams,
+maxparams,
+"perf.cache",
+cache) < 0)
+return -1;
+
+return 0;
+}
+
+static int
+qemuDomainGetStatsPerf(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
+   virDomainObjPtr dom,
+   

[libvirt] [PATCH v4 3/8] perf: implement a set of util functions for perf event

[Qiaowei Ren]

This patch implement a set of interfaces for perf event. Based on
these interfaces, we can implement internal driver API for perf,
and get the results of perf conuter you care about.

Signed-off-by: Qiaowei Ren 
---
 include/libvirt/virterror.h |   2 +
 src/Makefile.am |   1 +
 src/libvirt_private.syms|  12 ++
 src/util/virerror.c |   2 +
 src/util/virperf.c  | 307 
 src/util/virperf.h  |  63 +
 6 files changed, 387 insertions(+)
 create mode 100644 src/util/virperf.c
 create mode 100644 src/util/virperf.h

diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
index c6d1a76..4acf368 100644
--- a/include/libvirt/virterror.h
+++ b/include/libvirt/virterror.h
@@ -130,6 +130,8 @@ typedef enum {
 VIR_FROM_LOGGING = 63,  /* Error from log manager */
 VIR_FROM_XENXL = 64,/* Error from Xen xl config code */
 
+VIR_FROM_PERF = 65, /* Error from perf */
+
 # ifdef VIR_ENUM_SENTINELS
 VIR_ERR_DOMAIN_LAST
 # endif
diff --git a/src/Makefile.am b/src/Makefile.am
index a4aef0f..a0ec8d1 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -98,6 +98,7 @@ UTIL_SOURCES =
\
util/virauthconfig.c util/virauthconfig.h   \
util/virbitmap.c util/virbitmap.h   \
util/virbuffer.c util/virbuffer.h   \
+   util/virperf.c util/virperf.h   \
util/vircgroup.c util/vircgroup.h util/vircgrouppriv.h  \
util/virclosecallbacks.c util/virclosecallbacks.h   
\
util/vircommand.c util/vircommand.h util/vircommandpriv.h \
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 3d0ec9d..3e4216d 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2013,6 +2013,18 @@ virPCIStubDriverTypeFromString;
 virPCIStubDriverTypeToString;
 
 
+# util/virperf.h
+virPerfEventDisable;
+virPerfEventEnable;
+virPerfEventIsEnabled;
+virPerfEventTypeFromString;
+virPerfEventTypeToString;
+virPerfFree;
+virPerfGetEventFd;
+virPerfNew;
+virPerfReadEvent;
+
+
 # util/virpidfile.h
 virPidFileAcquire;
 virPidFileAcquirePath;
diff --git a/src/util/virerror.c b/src/util/virerror.c
index 3a3ddef..25dbb62 100644
--- a/src/util/virerror.c
+++ b/src/util/virerror.c
@@ -136,6 +136,8 @@ VIR_ENUM_IMPL(virErrorDomain, VIR_ERR_DOMAIN_LAST,
   "Admin Interface",
   "Log Manager",
   "Xen XL Config",
+
+  "Perf",
 )
 
 
diff --git a/src/util/virperf.c b/src/util/virperf.c
new file mode 100644
index 000..42eee85
--- /dev/null
+++ b/src/util/virperf.c
@@ -0,0 +1,307 @@
+/*
+ * virperf.c: methods for managing perf events
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library.  If not, see
+ * .
+ *
+ * Authors:
+ *  Ren Qiaowei 
+ */
+#include 
+
+#include 
+#if defined HAVE_SYS_SYSCALL_H
+# include 
+#endif
+
+#include "virperf.h"
+#include "viralloc.h"
+#include "virerror.h"
+#include "virlog.h"
+#include "virfile.h"
+#include "virstring.h"
+#include "virtypedparam.h"
+
+VIR_LOG_INIT("util.perf");
+
+#define VIR_FROM_THIS VIR_FROM_PERF
+
+VIR_ENUM_IMPL(virPerfEvent, VIR_PERF_EVENT_LAST,
+  "cmt");
+
+struct virPerfEvent {
+int type;
+int fd;
+bool enabled;
+union {
+/* cmt */
+struct {
+int scale;
+} cmt;
+} efields;
+};
+typedef struct virPerfEvent *virPerfEventPtr;
+
+struct virPerf {
+struct virPerfEvent events[VIR_PERF_EVENT_LAST];
+};
+
+virPerfPtr
+virPerfNew(void)
+{
+size_t i;
+virPerfPtr perf;
+
+if (VIR_ALLOC(perf) < 0)
+return NULL;
+
+for (i = 0; i < VIR_PERF_EVENT_LAST; i++) {
+perf->events[i].type = i;
+perf->events[i].fd = -1;
+perf->events[i].enabled = false;
+}
+
+return perf;
+}
+
+void
+virPerfFree(virPerfPtr perf)
+{
+size_t i;
+
+if (perf == NULL)
+return;
+
+for (i = 0; i < VIR_PERF_EVENT_LAST; i++) {
+if (perf->events[i].enabled)
+virPerfEventDisable(perf, i);
+}
+
+VIR_FREE(perf);
+}
+
+#if defined(__linux__) && defined(HAVE_SYS_SYSCALL_H)
+
+# include 
+
+static virPerfEventPtr

[libvirt] [PATCH v4 2/8] perf: implement the remote protocol for perf event

[Qiaowei Ren]

Add remote support for perf event.

Signed-off-by: Qiaowei Ren 
---
 daemon/remote.c  | 47 
 src/remote/remote_driver.c   | 39 
 src/remote/remote_protocol.x | 30 +++-
 src/remote_protocol-structs  | 18 +
 4 files changed, 133 insertions(+), 1 deletion(-)

diff --git a/daemon/remote.c b/daemon/remote.c
index 370f442..46d2933 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -2842,6 +2842,53 @@ remoteDispatchNodeGetMemoryStats(virNetServerPtr server 
ATTRIBUTE_UNUSED,
 }
 
 static int
+remoteDispatchDomainGetPerfEvents(virNetServerPtr server ATTRIBUTE_UNUSED,
+  virNetServerClientPtr client 
ATTRIBUTE_UNUSED,
+  virNetMessagePtr msg ATTRIBUTE_UNUSED,
+  virNetMessageErrorPtr rerr,
+  remote_domain_get_perf_events_args *args,
+  remote_domain_get_perf_events_ret *ret)
+{
+virDomainPtr dom = NULL;
+virTypedParameterPtr params = NULL;
+int nparams = 0;
+int rv = -1;
+struct daemonClientPrivate *priv =
+virNetServerClientGetPrivateData(client);
+
+if (!priv->conn) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
+goto cleanup;
+}
+
+if (!(dom = get_nonnull_domain(priv->conn, args->dom)))
+goto cleanup;
+
+if (virDomainGetPerfEvents(dom, , ) < 0)
+goto cleanup;
+
+if (nparams > REMOTE_DOMAIN_MEMORY_PARAMETERS_MAX) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large"));
+goto cleanup;
+}
+
+if (remoteSerializeTypedParameters(params, nparams,
+   >params.params_val,
+   >params.params_len,
+   0) < 0)
+goto cleanup;
+
+rv = 0;
+
+ cleanup:
+if (rv < 0)
+virNetMessageSaveError(rerr);
+virTypedParamsFree(params, nparams);
+virObjectUnref(dom);
+return rv;
+}
+
+static int
 remoteDispatchDomainGetBlockJobInfo(virNetServerPtr server ATTRIBUTE_UNUSED,
 virNetServerClientPtr client 
ATTRIBUTE_UNUSED,
 virNetMessagePtr msg ATTRIBUTE_UNUSED,
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index d9d7ec8..7927ff5 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -2037,6 +2037,43 @@ remoteDomainGetNumaParameters(virDomainPtr domain,
 }
 
 static int
+remoteDomainGetPerfEvents(virDomainPtr domain,
+  virTypedParameterPtr *params,
+  int *nparams)
+{
+int rv = -1;
+remote_domain_get_perf_events_args args;
+remote_domain_get_perf_events_ret ret;
+struct private_data *priv = domain->conn->privateData;
+
+remoteDriverLock(priv);
+
+make_nonnull_domain(, domain);
+
+memset(, 0, sizeof(ret));
+if (call(domain->conn, priv, 0, REMOTE_PROC_DOMAIN_GET_PERF_EVENTS,
+ (xdrproc_t) xdr_remote_domain_get_perf_events_args, (char *) 
,
+ (xdrproc_t) xdr_remote_domain_get_perf_events_ret, (char *) ) 
== -1)
+goto done;
+
+if (remoteDeserializeTypedParameters(ret.params.params_val,
+ ret.params.params_len,
+ REMOTE_DOMAIN_PERF_EVENTS_MAX,
+ params,
+ nparams) < 0)
+goto cleanup;
+
+rv = 0;
+
+ cleanup:
+xdr_free((xdrproc_t) xdr_remote_domain_get_perf_events_ret,
+ (char *) );
+ done:
+remoteDriverUnlock(priv);
+return rv;
+}
+
+static int
 remoteDomainGetBlkioParameters(virDomainPtr domain,
virTypedParameterPtr params, int *nparams,
unsigned int flags)
@@ -8286,6 +8323,8 @@ static virHypervisorDriver hypervisor_driver = {
 .domainGetMemoryParameters = remoteDomainGetMemoryParameters, /* 0.8.5 */
 .domainSetBlkioParameters = remoteDomainSetBlkioParameters, /* 0.9.0 */
 .domainGetBlkioParameters = remoteDomainGetBlkioParameters, /* 0.9.0 */
+.domainGetPerfEvents = remoteDomainGetPerfEvents, /* 1.3.2 */
+.domainSetPerfEvents = remoteDomainSetPerfEvents, /* 1.3.2 */
 .domainGetInfo = remoteDomainGetInfo, /* 0.3.0 */
 .domainGetState = remoteDomainGetState, /* 0.9.2 */
 .domainGetControlInfo = remoteDomainGetControlInfo, /* 0.9.3 */
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
index bfdbce7..8b2218e 100644
--- a/src/remote/remote_protocol.x
+++ b/src/remote/remote_protocol.x
@@ -109,6 +109,9 @@ const REMOTE_DOMAIN_BLOCK_IO_TUNE_PARAMETERS_MAX = 16;
 /* Upper limit on list of numa parameters. */
 const 

[libvirt] [PATCH v4 8/8] virsh: extend domstats command

[Qiaowei Ren]

This patch extend domstats command to match extended
virDomainListGetStats API in previous patch.

Signed-off-by: Qiaowei Ren 
---
 tools/virsh-domain-monitor.c | 7 +++
 tools/virsh.pod  | 7 +--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index 7dcbc5c..5fee283 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -1960,6 +1960,10 @@ static const vshCmdOptDef opts_domstats[] = {
  .type = VSH_OT_BOOL,
  .help = N_("report domain block device statistics"),
 },
+{.name = "perf",
+ .type = VSH_OT_BOOL,
+ .help = N_("report domain perf event statistics"),
+},
 {.name = "list-active",
  .type = VSH_OT_BOOL,
  .help = N_("list only active domains"),
@@ -2071,6 +2075,9 @@ cmdDomstats(vshControl *ctl, const vshCmd *cmd)
 if (vshCommandOptBool(cmd, "block"))
 stats |= VIR_DOMAIN_STATS_BLOCK;
 
+if (vshCommandOptBool(cmd, "perf"))
+stats |= VIR_DOMAIN_STATS_PERF;
+
 if (vshCommandOptBool(cmd, "list-active"))
 flags |= VIR_CONNECT_GET_ALL_DOMAINS_STATS_ACTIVE;
 
diff --git a/tools/virsh.pod b/tools/virsh.pod
index eefe68a..b6835b3 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -846,7 +846,7 @@ or unique source names printed by this command.
 
 =item B [I<--raw>] [I<--enforce>] [I<--backing>] [I<--state>]
 [I<--cpu-total>] [I<--balloon>] [I<--vcpu>] [I<--interface>] [I<--block>]
-[[I<--list-active>] [I<--list-inactive>] [I<--list-persistent>]
+[I<--perf>] [[I<--list-active>] [I<--list-inactive>] [I<--list-persistent>]
 [I<--list-transient>] [I<--list-running>] [I<--list-paused>]
 [I<--list-shutoff>] [I<--list-other>]] | [I ...]
 
@@ -864,7 +864,7 @@ behavior use the I<--raw> flag.
 The individual statistics groups are selectable via specific flags. By
 default all supported statistics groups are returned. Supported
 statistics groups flags are: I<--state>, I<--cpu-total>, I<--balloon>,
-I<--vcpu>, I<--interface>, I<--block>.
+I<--vcpu>, I<--interface>, I<--block>, I<--perf>.
 
 When selecting the I<--state> group the following fields are returned:
 "state.state" - state of the VM, returned as number from virDomainState enum,
@@ -899,6 +899,9 @@ I<--interface> returns:
 "net..tx.errs" - number of transmission errors,
 "net..tx.drop" - number of transmit packets dropped
 
+I<--perf> returns the statistics of all enabled perf events:
+"perf.cache" - the cache usage in Byte currently used
+
 I<--block> returns information about disks associated with each
 domain.  Using the I<--backing> flag extends this information to
 cover all resources in the backing chain, rather than the default
-- 
1.9.1

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

[libvirt] [PATCH v4 1/8] perf: add new public APIs for perf event

[Qiaowei Ren]

API agreed on in
https://www.redhat.com/archives/libvir-list/2015-October/msg00872.html

* include/libvirt/libvirt-domain.h (virDomainGetPerfEvents,
virDomainSetPerfEvents): New declarations.
* src/libvirt_public.syms: Export new symbols.
* src/driver-hypervisor.h (virDrvDomainGetPerfEvents,
virDrvDomainSetPerfEvents): New typedefs.
* src/libvirt-domain.c: Implement virDomainGetPerfEvents and
virDomainSetPerfEvents.

Signed-off-by: Qiaowei Ren 
---
 include/libvirt/libvirt-domain.h | 18 
 src/driver-hypervisor.h  | 12 ++
 src/libvirt-domain.c | 93 
 src/libvirt_public.syms  |  6 +++
 4 files changed, 129 insertions(+)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index 65f1618..5e39bb3 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -1802,6 +1802,24 @@ int virDomainListGetStats(virDomainPtr *doms,
 void virDomainStatsRecordListFree(virDomainStatsRecordPtr *stats);
 
 /*
+ * Perf Event API
+ */
+
+/**
+ * VIR_PERF_PARAM_CMT:
+ *
+ * Macro for typed parameter name that represents CMT perf event.
+ */
+# define VIR_PERF_PARAM_CMT "cmt"
+
+int virDomainGetPerfEvents(virDomainPtr dom,
+   virTypedParameterPtr *params,
+   int *nparams);
+int virDomainSetPerfEvents(virDomainPtr dom,
+   virTypedParameterPtr params,
+   int nparams);
+
+/*
  * BlockJob API
  */
 
diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h
index ae2ec4d..aedbc83 100644
--- a/src/driver-hypervisor.h
+++ b/src/driver-hypervisor.h
@@ -958,6 +958,16 @@ typedef int
 unsigned int flags);
 
 typedef int
+(*virDrvDomainGetPerfEvents)(virDomainPtr dom,
+ virTypedParameterPtr *params,
+ int *nparams);
+
+typedef int
+(*virDrvDomainSetPerfEvents)(virDomainPtr dom,
+ virTypedParameterPtr params,
+ int nparams);
+
+typedef int
 (*virDrvDomainBlockJobAbort)(virDomainPtr dom,
  const char *path,
  unsigned int flags);
@@ -1413,6 +1423,8 @@ struct _virHypervisorDriver {
 virDrvConnectSetKeepAlive connectSetKeepAlive;
 virDrvConnectIsAlive connectIsAlive;
 virDrvNodeSuspendForDuration nodeSuspendForDuration;
+virDrvDomainGetPerfEvents domainGetPerfEvents;
+virDrvDomainSetPerfEvents domainSetPerfEvents;
 virDrvDomainSetBlockIoTune domainSetBlockIoTune;
 virDrvDomainGetBlockIoTune domainGetBlockIoTune;
 virDrvDomainGetCPUStats domainGetCPUStats;
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 9491845..bfa8432 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -9564,6 +9564,99 @@ virDomainOpenChannel(virDomainPtr dom,
 
 
 /**
+ * virDomainGetPerfEvents:
+ * @domain: a domain object
+ * @params: where to store perf events setting
+ * @nparams: number of items in @params
+ *
+ * Get all perf events setting. Possible fields returned in @params are
+ * defined by VIR_DOMAIN_PERF_* macros and new fields will likely be
+ * introduced in the future.
+ *
+ * Returns -1 in case of failure, 0 in case of success.
+ */
+int virDomainGetPerfEvents(virDomainPtr domain,
+   virTypedParameterPtr *params,
+   int *nparams)
+{
+virConnectPtr conn;
+
+VIR_DOMAIN_DEBUG(domain, "params=%p, nparams=%p",
+ params, nparams);
+
+virResetLastError();
+
+virCheckDomainReturn(domain, -1);
+virCheckNonNullArgGoto(params, error);
+virCheckNonNullArgGoto(nparams, error);
+
+conn = domain->conn;
+
+if (conn->driver->domainGetPerfEvents) {
+int ret;
+ret = conn->driver->domainGetPerfEvents(domain, params, nparams);
+if (ret < 0)
+goto error;
+return ret;
+}
+virReportUnsupportedError();
+
+ error:
+virDispatchError(domain->conn);
+return -1;
+}
+
+
+/**
+ * virDomainSetPerfEvents:
+ * @domain: a domain object
+ * @params: pointer to perf events parameter object
+ * @nparams: number of perf event parameters (this value can be the same
+ *   less than the number of parameters supported)
+ *
+ * Enable or disable the particular list of perf events you care about.
+ *
+ * Returns -1 in case of error, 0 in case of success.
+ */
+int virDomainSetPerfEvents(virDomainPtr domain,
+   virTypedParameterPtr params,
+   int nparams)
+{
+virConnectPtr conn;
+
+VIR_DOMAIN_DEBUG(domain, "params=%p, nparams=%d",
+ params, nparams);
+VIR_TYPED_PARAMS_DEBUG(params, nparams);
+
+virResetLastError();
+
+virCheckDomainReturn(domain, -1);
+conn = domain->conn;
+
+virCheckReadOnlyGoto(conn->flags, error);
+

[libvirt] [PATCH v4 7/8] virsh: implement new command to support perf

[Qiaowei Ren]

This patch add new perf command to enable/disable perf event
for a guest domain.

Signed-off-by: Qiaowei Ren 
---
 tools/virsh-domain.c | 128 +++
 tools/virsh.pod  |  20 
 2 files changed, 148 insertions(+)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index c2146d2..b1b5bfb 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -8483,6 +8483,128 @@ cmdMemtune(vshControl *ctl, const vshCmd *cmd)
 }
 
 /*
+ * "perf" command
+ */
+static const vshCmdInfo info_perf[] = {
+{.name = "help",
+.data = N_("Get or set perf event")
+},
+{.name = "desc",
+.data = N_("Get or set the current perf events for a guest"
+   " domain.\n"
+   "To get the perf events list use following command: 
\n\n"
+   "virsh # perf ")
+},
+{.name = NULL}
+};
+
+static const vshCmdOptDef opts_perf[] = {
+{.name = "domain",
+ .type = VSH_OT_DATA,
+ .flags = VSH_OFLAG_REQ,
+ .help = N_("domain name, id or uuid")
+},
+{.name = "enable",
+ .type = VSH_OT_STRING,
+ .help = N_("perf events which will be enabled")
+},
+{.name = "disable",
+ .type = VSH_OT_STRING,
+ .help = N_("perf events which will be disabled")
+},
+{.name = NULL}
+};
+
+static int
+virshParseEventStr(vshControl *ctl,
+   const char *event,
+   bool state,
+   virTypedParameterPtr *params,
+   int *nparams,
+   int *maxparams)
+{
+char **tok = NULL;
+size_t i, ntok;
+int ret = -1;
+
+if (!(tok = virStringSplitCount(event, "|", 0, )))
+return -1;
+
+if (ntok > VIR_PERF_EVENT_LAST) {
+vshError(ctl, _("event string '%s' has too many fields"), event);
+goto cleanup;
+}
+
+for(i = 0; i < ntok; i++) {
+if ((*tok[i] != '\0') &&
+virTypedParamsAddBoolean(params, nparams,
+ maxparams, tok[i], state) < 0)
+goto cleanup;
+}
+
+ret = 0;
+ cleanup:
+virStringFreeList(tok);
+return ret;
+}
+
+static bool
+cmdPerf(vshControl *ctl, const vshCmd *cmd)
+{
+virDomainPtr dom;
+int nparams = 0;
+int maxparams = 0;
+size_t i;
+virTypedParameterPtr params = NULL;
+bool ret = false;
+const char *enable = NULL, *disable = NULL;
+
+if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
+return false;
+
+if (vshCommandOptStringReq(ctl, cmd, "enable", ) < 0 ||
+vshCommandOptStringReq(ctl, cmd, "disable", ) < 0)
+return false;
+
+if (enable && virshParseEventStr(ctl, enable, true,
+ , , ) < 0)
+goto cleanup;
+
+if (disable && virshParseEventStr(ctl, disable, false,
+  , , ) < 0)
+   goto cleanup;
+
+if (nparams == 0) {
+if (virDomainGetPerfEvents(dom, , ) != 0) {
+vshError(ctl, "%s", _("Unable to get perf events"));
+goto cleanup;
+}
+for (i = 0; i < nparams; i++) {
+if (params[i].type == VIR_TYPED_PARAM_BOOLEAN &&
+params[i].value.b) {
+vshPrint(ctl, "%-15s: %s\n", params[i].field, _("enabled"));
+} else {
+vshPrint(ctl, "%-15s: %s\n", params[i].field, _("disabled"));
+}
+}
+} else {
+if (virDomainSetPerfEvents(dom, params, nparams) != 0)
+goto error;
+}
+
+ret = true;
+ cleanup:
+virTypedParamsFree(params, nparams);
+virDomainFree(dom);
+return ret;
+
+ error:
+vshError(ctl, "%s", _("Unable to enable/disable perf events"));
+goto cleanup;
+}
+
+
+/*
  * "numatune" command
  */
 static const vshCmdInfo info_numatune[] = {
@@ -12864,6 +12986,12 @@ const vshCmdDef domManagementCmds[] = {
  .info = info_memtune,
  .flags = 0
 },
+{.name = "perf",
+ .handler = cmdPerf,
+ .opts = opts_perf,
+ .info = info_perf,
+ .flags = 0
+},
 {.name = "metadata",
  .handler = cmdMetadata,
  .opts = opts_metadata,
diff --git a/tools/virsh.pod b/tools/virsh.pod
index e830c59..eefe68a 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -2116,6 +2116,26 @@ The guaranteed minimum memory allocation for the guest.
 
 Specifying -1 as a value for these limits is interpreted as unlimited.
 
+=item B I [I<--enable> B]
+[I<--disable> B]
+
+Get the current perf events setting or enable/disable specific perf
+event for a guest domain.
+
+Perf is a performance analyzing tool in Linux, and it can instrument
+CPU performance counters, tracepoints, kprobes, and uprobes (dynamic
+tracing). Perf supports a list of measurable events, and can measure
+events coming from different sources. For instance, some event are
+pure kernel counters, in this case they are called software events,
+including 

[libvirt] [PATCH v4 5/8] perf: add new xml element

[Qiaowei Ren]

This patch adds new xml element, and so we can have the option of
also having perf events enabled immediately at startup.

Signed-off-by: Qiaowei Ren 
---
 docs/schemas/domaincommon.rng |  27 +++
 src/conf/domain_conf.c| 111 ++
 src/conf/domain_conf.h|  10 +++
 src/qemu/qemu_driver.c|  26 ++
 src/qemu/qemu_process.c   |   8 +-
 tests/domainschemadata/domain-perf-simple.xml |  20 +
 6 files changed, 200 insertions(+), 2 deletions(-)
 create mode 100644 tests/domainschemadata/domain-perf-simple.xml

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 5deb17b..7f16620 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -56,6 +56,9 @@
   
 
 
+  
+
+
   
 
 
@@ -393,6 +396,30 @@
   
 
   
+  
+
+  
+
+  
+
+  cmt
+
+  
+  
+
+  
+
+  
+
+  
+
+  

[libvirt] [PATCH v4 0/8] Add perf and Intel CMT feature support

[Qiaowei Ren]

Add perf and Intel CMT feature support

The series mainly adds Intel CMT feature support into libvirt. CMT is
new introduced PQos (Platform Qos) feature to monitor the usage of
cache by applications running on the platform.

Currently CMT patches has been merged into Linux kernel mainline.
The CMT implementation in Linux kernel is based on perf mechanism and
there is no support for perf in libvirt, and so this series firstly
add perf support into libvirt, including two public API and a set of
util interfaces. And based on these APIs and interfaces, thie series
implements CMT perf event support.

Current state:

* 1/8 - perf: add new public APIs for perf event
- ACKed by Daniel

* 2/8 - perf: implement the remote protocol for perf event
- ACKed by Daniel

* 3/8 - perf: implement a set of util functions for perf event
- ACKed by Daniel

* 4/8 - qemu_driver: add support to perf event
- restart perf in qemuProcessReconnect and qemuProcessAttach
  in patch 6/8

* 5/8 - perf: add new xml element
- ACKed by Daniel

* 6/8 - perf: reenable perf events when libvirtd restart
- add qemuDomainPerfRestart() into qemuProcessReconnect() and
  qemuProcessAttach()

* 7/8 - virsh: implement new command to support perf
- ACKed by Daniel

* 8/8 - virsh: extend domstats command
- ACKed by Daniel

TODO:
1. add support for new APIs into libvirt-python library.

Changes since v1:
  * change perf APIs implementation to match new style of the API.
  * add new xml element
  * reenable all perf events previously enabled when libvirtd daemon
restart.
  * redesign perf util functions.

Changes since v2:
  * add an example XML file to the test suite.
  * add virPerfReadEvent().
  * change 'perf' xml element to new style.
  * change 'perf' command to new stype.

Changes since v3:
  * move qemuDomainPerfRestart() into qemuProcessReconnect() and
qemuProcessAttach().

Qiaowei Ren (8):
  perf: add new public APIs for perf event
  perf: implement the remote protocol for perf event
  perf: implement a set of util functions for perf event
  qemu_driver: add support to perf event
  perf: add new xml element
  perf: reenable perf events when libvirtd restart
  virsh: implement new command to support perf
  virsh: extend domstats command

 daemon/remote.c   |  47 
 docs/schemas/domaincommon.rng |  27 +++
 include/libvirt/libvirt-domain.h  |  19 ++
 include/libvirt/virterror.h   |   2 +
 src/Makefile.am   |   1 +
 src/conf/domain_conf.c| 111 ++
 src/conf/domain_conf.h|  10 +
 src/driver-hypervisor.h   |  12 +
 src/libvirt-domain.c  |  93 
 src/libvirt_private.syms  |  12 +
 src/libvirt_public.syms   |   6 +
 src/qemu/qemu_domain.h|   3 +
 src/qemu/qemu_driver.c| 159 +
 src/qemu/qemu_process.c   |  44 
 src/remote/remote_driver.c|  39 
 src/remote/remote_protocol.x  |  30 ++-
 src/remote_protocol-structs   |  18 ++
 src/util/virerror.c   |   2 +
 src/util/virperf.c| 307 ++
 src/util/virperf.h|  63 ++
 tests/domainschemadata/domain-perf-simple.xml |  20 ++
 tools/virsh-domain-monitor.c  |   7 +
 tools/virsh-domain.c  | 128 +++
 tools/virsh.pod   |  27 ++-
 24 files changed, 1184 insertions(+), 3 deletions(-)
 create mode 100644 src/util/virperf.c
 create mode 100644 src/util/virperf.h
 create mode 100644 tests/domainschemadata/domain-perf-simple.xml

-- 
1.9.1

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

[libvirt] [libvirt-glib] events: Mark 'eventlock' as static

[Christophe Fergeau]

It's not used outside of the libvirt-glib-event.c file, so there is no
good reason for not having it static. As it was not listed in
libvirt-glib.sym, this will make no change to the publicly exported
symbols (ie this is not an ABI change).
---
 libvirt-glib/libvirt-glib-event.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libvirt-glib/libvirt-glib-event.c 
b/libvirt-glib/libvirt-glib-event.c
index f8227d6..4548aa6 100644
--- a/libvirt-glib/libvirt-glib-event.c
+++ b/libvirt-glib/libvirt-glib-event.c
@@ -110,7 +110,7 @@ struct gvir_event_timeout
 virFreeCallback ff;
 };
 
-GMutex *eventlock = NULL;
+static GMutex *eventlock = NULL;
 
 static int nextwatch = 1;
 static GPtrArray *handles;
-- 
2.5.0

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

[libvirt] [PATCH v2] qemu: return -1 on error paths in qemuDomainSaveImageStartVM

[Nikolay Shirokovskiy]

Error paths after sending the event that domain is started written as if ret = 
-1
which is set at the beginning of the function. It's common idioma to keep 'ret'
equal to -1 until the end of function where it is set to 0. But here we use ret
to keep result of restore operation too and thus breaks the idioma and its 
users :)

Let's use different variable to hold restore result.

Signed-off-by: Nikolay Shirokovskiy 
---
 src/qemu/qemu_driver.c | 21 ++---
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 351e529..ced105b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6732,6 +6732,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
qemuDomainAsyncJob asyncJob)
 {
 int ret = -1;
+bool restored;
 virObjectEventPtr event;
 int intermediatefd = -1;
 virCommandPtr cmd = NULL;
@@ -6758,13 +6759,13 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
 }
 
 /* Set the migration source and start it up. */
-ret = qemuProcessStart(conn, driver, vm, asyncJob,
-   "stdio", *fd, path, NULL,
-   VIR_NETDEV_VPORT_PROFILE_OP_RESTORE,
-   VIR_QEMU_PROCESS_START_PAUSED);
+restored = qemuProcessStart(conn, driver, vm, asyncJob,
+"stdio", *fd, path, NULL,
+VIR_NETDEV_VPORT_PROFILE_OP_RESTORE,
+VIR_QEMU_PROCESS_START_PAUSED) >= 0;
 
 if (intermediatefd != -1) {
-if (ret < 0) {
+if (!restored) {
 /* if there was an error setting up qemu, the intermediate
  * process will wait forever to write to stdout, so we
  * must manually kill it.
@@ -6775,7 +6776,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
 
 if (virCommandWait(cmd, NULL) < 0) {
 qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0);
-ret = -1;
+restored = false;
 }
 VIR_DEBUG("Decompression binary stderr: %s", NULLSTR(errbuf));
 }
@@ -6783,18 +6784,16 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
 
 if (VIR_CLOSE(*fd) < 0) {
 virReportSystemError(errno, _("cannot close file: %s"), path);
-ret = -1;
+restored = false;
 }
 
-if (ret < 0) {
-virDomainAuditStart(vm, "restored", false);
+virDomainAuditStart(vm, "restored", restored);
+if (!restored)
 goto cleanup;
-}
 
 event = virDomainEventLifecycleNewFromObj(vm,
  VIR_DOMAIN_EVENT_STARTED,
  VIR_DOMAIN_EVENT_STARTED_RESTORED);
-virDomainAuditStart(vm, "restored", true);
 qemuDomainEventQueue(driver, event);
 
 
-- 
1.8.3.1

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

[libvirt] [PATCH v2 01/21] cgroup: Clean up virCgroupGetPercpuStats

[Peter Krempa]

Use 'ret' for return variable name, clarify use of 'param_idx' and avoid
unnecessary 'success' label. No functional changes. Also document the
function.
---

Notes:
V2:
- Fixed mistake in the refactor that would return incorrect error values 
(Found by Jan)
- Added comment explaining the terriblenes of the function (Requested by 
John)

 src/util/vircgroup.c | 64 +++-
 1 file changed, 43 insertions(+), 21 deletions(-)

diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index 9b56e27..da0df7a 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -3189,6 +3189,26 @@ virCgroupGetPercpuVcpuSum(virCgroupPtr group,
 }


+/**
+ * virCgroupGetPercpuStats:
+ * @cgroup: cgroup data structure
+ * @params: typed parameter array where data is returned
+ * @nparams: cardinality of @params
+ * @start_cpu: offset of physical CPU to get data for
+ * @ncpus: number of physical CPUs to get data for
+ * @nvcpupids: number of vCPU threads for a domain (actual number of vcpus)
+ *
+ * This function is the worker that retrieves data in the appropriate format
+ * for the terribly designed 'virDomainGetCPUStats' API. Sharing semantics with
+ * the API, this function has two modes of operation depending on magic 
settings
+ * of the input arguments. Please refer to docs of 'virDomainGetCPUStats' for
+ * the usage patterns of the similarly named arguments.
+ *
+ * @nvcpupids determines the count of active vcpu threads for the vm. If the
+ * threads could not be detected the percpu data is skipped.
+ *
+ * Please DON'T use this function anywhere else.
+ */
 int
 virCgroupGetPercpuStats(virCgroupPtr group,
 virTypedParameterPtr params,
@@ -3197,7 +3217,7 @@ virCgroupGetPercpuStats(virCgroupPtr group,
 unsigned int ncpus,
 unsigned int nvcpupids)
 {
-int rv = -1;
+int ret = -1;
 size_t i;
 int need_cpus, total_cpus;
 char *pos;
@@ -3218,12 +3238,13 @@ virCgroupGetPercpuStats(virCgroupPtr group,

 /* To parse account file, we need to know how many cpus are present.  */
 if (!(cpumap = nodeGetPresentCPUBitmap(NULL)))
-return rv;
+return -1;

 total_cpus = virBitmapSize(cpumap);

+/* return total number of cpus */
 if (ncpus == 0) {
-rv = total_cpus;
+ret = total_cpus;
 goto cleanup;
 }

@@ -3261,34 +3282,35 @@ virCgroupGetPercpuStats(virCgroupPtr group,
 goto cleanup;
 }

-if (nvcpupids == 0 || param_idx + 1 >= nparams)
-goto success;
 /* return percpu vcputime in index 1 */
-param_idx++;
+param_idx = 1;

-if (VIR_ALLOC_N(sum_cpu_time, need_cpus) < 0)
-goto cleanup;
-if (virCgroupGetPercpuVcpuSum(group, nvcpupids, sum_cpu_time, need_cpus,
-  cpumap) < 0)
-goto cleanup;
-
-for (i = start_cpu; i < need_cpus; i++) {
-if (virTypedParameterAssign([(i - start_cpu) * nparams +
-param_idx],
-VIR_DOMAIN_CPU_STATS_VCPUTIME,
-VIR_TYPED_PARAM_ULLONG,
-sum_cpu_time[i]) < 0)
+if (nvcpupids > 0 && param_idx < nparams) {
+if (VIR_ALLOC_N(sum_cpu_time, need_cpus) < 0)
 goto cleanup;
+if (virCgroupGetPercpuVcpuSum(group, nvcpupids, sum_cpu_time, 
need_cpus,
+  cpumap) < 0)
+goto cleanup;
+
+for (i = start_cpu; i < need_cpus; i++) {
+if (virTypedParameterAssign([(i - start_cpu) * nparams +
+param_idx],
+VIR_DOMAIN_CPU_STATS_VCPUTIME,
+VIR_TYPED_PARAM_ULLONG,
+sum_cpu_time[i]) < 0)
+goto cleanup;
+}
+
+param_idx++;
 }

- success:
-rv = param_idx + 1;
+ret = param_idx;

  cleanup:
 virBitmapFree(cpumap);
 VIR_FREE(sum_cpu_time);
 VIR_FREE(buf);
-return rv;
+return ret;
 }


-- 
2.6.2

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

[libvirt] [PATCH v2 10/21] conf: Store cpu pinning data in def->vcpus

[Peter Krempa]

Now with the new struct the data can be stored in a much saner place.
---

Notes:
v2:
- clear bitmap pointer after free to avoid use after free

 src/conf/domain_conf.c   | 136 ++--
 src/conf/domain_conf.h   |   3 +-
 src/libxl/libxl_domain.c |  17 +++---
 src/libxl/libxl_driver.c |  39 ++---
 src/qemu/qemu_cgroup.c   |  15 ++---
 src/qemu/qemu_driver.c   | 143 ++-
 src/qemu/qemu_process.c  |  36 ++--
 src/test/test_driver.c   |  43 ++
 8 files changed, 186 insertions(+), 246 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3dc5e00..c4b735e 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1289,6 +1289,9 @@ virDomainVcpuInfoClear(virDomainVcpuInfoPtr info)
 {
 if (!info)
 return;
+
+virBitmapFree(info->cpumask);
+info->cpumask = NULL;
 }


@@ -1422,7 +1425,14 @@ virDomainDefGetVcpu(virDomainDefPtr def,
 static bool
 virDomainDefHasVcpuPin(const virDomainDef *def)
 {
-return !!def->cputune.vcpupin;
+size_t i;
+
+for (i = 0; i < def->maxvcpus; i++) {
+if (def->vcpus[i].cpumask)
+return true;
+}
+
+return false;
 }


@@ -2593,8 +2603,6 @@ void virDomainDefFree(virDomainDefPtr def)

 virDomainIOThreadIDDefArrayFree(def->iothreadids, def->niothreadids);

-virDomainPinDefArrayFree(def->cputune.vcpupin, def->cputune.nvcpupin);
-
 virBitmapFree(def->cputune.emulatorpin);

 for (i = 0; i < def->cputune.nvcpusched; i++)
@@ -14137,83 +14145,68 @@ virDomainIOThreadIDDefParseXML(xmlNodePtr node,
 }


-/* Check if pin with same id already exists. */
-static bool
-virDomainPinIsDuplicate(virDomainPinDefPtr *def,
-int npin,
-int id)
-{
-size_t i;
-
-if (!def || !npin)
-return false;
-
-for (i = 0; i < npin; i++) {
-if (def[i]->id == id)
-return true;
-}
-
-return false;
-}
-
 /* Parse the XML definition for a vcpupin
  *
  * vcpupin has the form of
  *   
  */
-static virDomainPinDefPtr
-virDomainVcpuPinDefParseXML(xmlNodePtr node,
-xmlXPathContextPtr ctxt)
+static int
+virDomainVcpuPinDefParseXML(virDomainDefPtr def,
+xmlNodePtr node)
 {
-virDomainPinDefPtr def;
-xmlNodePtr oldnode = ctxt->node;
+virDomainVcpuInfoPtr vcpu;
 unsigned int vcpuid;
 char *tmp = NULL;
+int ret = -1;

-if (VIR_ALLOC(def) < 0)
-return NULL;
-
-ctxt->node = node;
-
-if (!(tmp = virXPathString("string(./@vcpu)", ctxt))) {
-virReportError(VIR_ERR_XML_ERROR, "%s",
-   _("missing vcpu id in vcpupin"));
-goto error;
+if (!(tmp = virXMLPropString(node, "vcpu"))) {
+virReportError(VIR_ERR_XML_ERROR, "%s", _("missing vcpu id in 
vcpupin"));
+goto cleanup;
 }

 if (virStrToLong_uip(tmp, NULL, 10, ) < 0) {
 virReportError(VIR_ERR_XML_ERROR,
_("invalid setting for vcpu '%s'"), tmp);
-goto error;
+goto cleanup;
 }
 VIR_FREE(tmp);

-def->id = vcpuid;
+if (!(vcpu = virDomainDefGetVcpu(def, vcpuid)) ||
+!vcpu->online) {
+/* To avoid the regression when daemon loading domain confs, we can't
+ * simply error out if  nodes greater than current vcpus.
+ * Ignore them instead. */
+VIR_WARN("Ignoring vcpupin for missing vcpus");
+ret = 0;
+goto cleanup;
+}

 if (!(tmp = virXMLPropString(node, "cpuset"))) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("missing cpuset for vcpupin"));
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("missing cpuset for vcpupin"));
+goto cleanup;
+}

-goto error;
+if (vcpu->cpumask) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("duplicate vcpupin for vcpu '%d'"), vcpuid);
+goto cleanup;
 }

-if (virBitmapParse(tmp, 0, >cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0)
-goto error;
+if (virBitmapParse(tmp, 0, >cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0)
+goto cleanup;

-if (virBitmapIsAllClear(def->cpumask)) {
+if (virBitmapIsAllClear(vcpu->cpumask)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Invalid value of 'cpuset': %s"), tmp);
-goto error;
+goto cleanup;
 }

+ret = 0;
+
  cleanup:
 VIR_FREE(tmp);
-ctxt->node = oldnode;
-return def;
-
- error:
-VIR_FREE(def);
-goto cleanup;
+return ret;
 }


@@ -15147,34 +15140,9 @@ virDomainDefParseXML(xmlDocPtr xml,
 if ((n = virXPathNodeSet("./cputune/vcpupin", ctxt, )) < 0)
 goto error;

-if (n && VIR_ALLOC_N(def->cputune.vcpupin, n) < 0)
-goto error;
-
 for (i = 0; i < n; i++) {
-virDomainPinDefPtr 

[libvirt] [PATCH v2 14/21] conf: Add helper to return a bitmap of active iothread ids

[Peter Krempa]

---
 src/conf/domain_conf.c   | 29 +
 src/conf/domain_conf.h   |  3 +++
 src/libvirt_private.syms |  1 +
 3 files changed, 33 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 0c28c03..556cd71 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -18292,6 +18292,35 @@ virDomainIOThreadIDAdd(virDomainDefPtr def,
 return NULL;
 }

+
+/*
+ * virDomainIOThreadIDMap:
+ * @def: domain definition
+ *
+ * Returns a map of active iothreads for @def.
+ */
+virBitmapPtr
+virDomainIOThreadIDMap(virDomainDefPtr def)
+{
+unsigned int max = 0;
+size_t i;
+virBitmapPtr ret = NULL;
+
+for (i = 0; i < def->niothreadids; i++) {
+if (def->iothreadids[i]->iothread_id > max)
+max = def->iothreadids[i]->iothread_id;
+}
+
+if (!(ret = virBitmapNew(max)))
+return NULL;
+
+for (i = 0; i < def->niothreadids; i++)
+ignore_value(virBitmapSetBit(ret, def->iothreadids[i]->iothread_id));
+
+return ret;
+}
+
+
 void
 virDomainIOThreadIDDel(virDomainDefPtr def,
unsigned int iothread_id)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 6a5f615..fe8334c 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2700,6 +2700,9 @@ virDomainIOThreadIDDefPtr 
virDomainIOThreadIDFind(virDomainDefPtr def,
   unsigned int iothread_id);
 virDomainIOThreadIDDefPtr virDomainIOThreadIDAdd(virDomainDefPtr def,
  unsigned int iothread_id);
+
+virBitmapPtr virDomainIOThreadIDMap(virDomainDefPtr def)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
 void virDomainIOThreadIDDel(virDomainDefPtr def, unsigned int iothread_id);
 void virDomainIOThreadSchedDelId(virDomainDefPtr def, unsigned int 
iothread_id);

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 7e9102a..af36a37 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -344,6 +344,7 @@ virDomainIOThreadIDAdd;
 virDomainIOThreadIDDefFree;
 virDomainIOThreadIDDel;
 virDomainIOThreadIDFind;
+virDomainIOThreadIDMap;
 virDomainIOThreadSchedDelId;
 virDomainKeyWrapCipherNameTypeFromString;
 virDomainKeyWrapCipherNameTypeToString;
-- 
2.6.2

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

[libvirt] [PATCH v2 06/21] qemu: domain: Prepare qemuDomainDetectVcpuPids for reuse

[Peter Krempa]

Free the old vcpupids array in case when this function is called again
during the run of the VM. It will be later reused in the vCPU hotplug
code. The function now returns the number of detected VCPUs.
---

Notes:
v2:
- fix comment spacing
- fix leak of cpupids on monitor exit failure
- change return value

v2:
- fix comment spacing
- fix leak of cpupids on monitor exit failure

 src/qemu/qemu_domain.c | 28 ++--
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 5ddf048..1895520 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4290,7 +4290,8 @@ qemuDomainGetVcpuPid(virDomainObjPtr vm,
  *
  * Updates vCPU thread ids in the private data of @vm.
  *
- * Returns 0 on success -1 on error and reports an appropriate error.
+ * Returns number of detected vCPU threads on success, -1 on error and reports
+ * an appropriate error.
  */
 int
 qemuDomainDetectVcpuPids(virQEMUDriverPtr driver,
@@ -4298,7 +4299,7 @@ qemuDomainDetectVcpuPids(virQEMUDriverPtr driver,
  int asyncJob)
 {
 pid_t *cpupids = NULL;
-int ncpupids;
+int ncpupids = 0;
 qemuDomainObjPrivatePtr priv = vm->privateData;

 /*
@@ -4329,26 +4330,23 @@ qemuDomainDetectVcpuPids(virQEMUDriverPtr driver,
  * Just disable CPU pinning with TCG until someone wants
  * to try to do this hard work.
  */
-if (vm->def->virtType == VIR_DOMAIN_VIRT_QEMU) {
-priv->nvcpupids = 0;
-priv->vcpupids = NULL;
-return 0;
-}
+if (vm->def->virtType == VIR_DOMAIN_VIRT_QEMU)
+goto done;

 if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
 return -1;
 ncpupids = qemuMonitorGetCPUInfo(priv->mon, );
-if (qemuDomainObjExitMonitor(driver, vm) < 0)
+if (qemuDomainObjExitMonitor(driver, vm) < 0) {
+VIR_FREE(cpupids);
 return -1;
-/* failure to get the VCPU<-> PID mapping or to execute the query
+}
+/* failure to get the VCPU <-> PID mapping or to execute the query
  * command will not be treated fatal as some versions of qemu don't
  * support this command */
 if (ncpupids <= 0) {
 virResetLastError();
-
-priv->nvcpupids = 0;
-priv->vcpupids = NULL;
-return 0;
+ncpupids = 0;
+goto done;
 }

 if (ncpupids != virDomainDefGetVcpus(vm->def)) {
@@ -4360,7 +4358,9 @@ qemuDomainDetectVcpuPids(virQEMUDriverPtr driver,
 return -1;
 }

+ done:
+VIR_FREE(priv->vcpupids);
 priv->nvcpupids = ncpupids;
 priv->vcpupids = cpupids;
-return 0;
+return ncpupids;
 }
-- 
2.6.2

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

[libvirt] [PATCH v2 08/21] conf: Don't copy def->cpumask into cpu pinning info

[Peter Krempa]

This step can be omitted, so that drivers can decide what to do when the
user requests to use default vcpu pinning.
---

Notes:
v2: use correct variable

 src/conf/domain_conf.c   | 32 
 src/libxl/libxl_domain.c | 15 ---
 src/libxl/libxl_driver.c |  2 ++
 src/qemu/qemu_driver.c   | 34 ++
 src/qemu/qemu_process.c  | 12 
 src/test/test_driver.c   |  2 ++
 src/vz/vz_sdk.c  |  4 ++--
 7 files changed, 28 insertions(+), 73 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index ead76a1..e06ec80 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15165,34 +15165,6 @@ virDomainDefParseXML(xmlDocPtr xml,
 }
 VIR_FREE(nodes);

-/* Initialize the pinning policy for vcpus which doesn't has
- * the policy specified explicitly as def->cpuset.
- */
-if (def->cpumask) {
-if (VIR_REALLOC_N(def->cputune.vcpupin, virDomainDefGetVcpus(def)) < 0)
-goto error;
-
-for (i = 0; i < virDomainDefGetVcpus(def); i++) {
-if (virDomainPinIsDuplicate(def->cputune.vcpupin,
-def->cputune.nvcpupin,
-i))
-continue;
-
-virDomainPinDefPtr vcpupin = NULL;
-
-if (VIR_ALLOC(vcpupin) < 0)
-goto error;
-
-if (!(vcpupin->cpumask = virBitmapNew(VIR_DOMAIN_CPUMASK_LEN))) {
-VIR_FREE(vcpupin);
-goto error;
-}
-virBitmapCopy(vcpupin->cpumask, def->cpumask);
-vcpupin->id = i;
-def->cputune.vcpupin[def->cputune.nvcpupin++] = vcpupin;
-}
-}
-
 if ((n = virXPathNodeSet("./cputune/emulatorpin", ctxt, )) < 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("cannot extract emulatorpin nodes"));
@@ -21832,10 +21804,6 @@ virDomainDefFormatInternal(virDomainDefPtr def,

 for (i = 0; i < def->cputune.nvcpupin; i++) {
 char *cpumask;
-/* Ignore the vcpupin which inherit from "cpuset of "." */
-if (virBitmapEqual(def->cpumask, def->cputune.vcpupin[i]->cpumask))
-continue;
-
 virBufferAsprintf(, "cputune.vcpupin[i]->id);

diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index c74c55c..b098d3d 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -829,9 +829,18 @@ libxlDomainSetVcpuAffinities(libxlDriverPrivatePtr driver, 
virDomainObjPtr vm)

 libxl_bitmap_init();

-for (i = 0; i < vm->def->cputune.nvcpupin; ++i) {
-pin = vm->def->cputune.vcpupin[i];
-cpumask = pin->cpumask;
+for (i = 0; i < virDomainDefGetVcpus(vm->def); ++i) {
+pin = virDomainPinFind(vm->def->cputune.vcpupin,
+   vm->def->cputune.nvcpupin,
+   i);
+
+if (pin && pin->cpumask)
+cpumask = pin->cpumask;
+else
+cpumask = vm->def->cpumask;
+
+if (!cpumask)
+continue;

 if (virBitmapToData(cpumask, , (int *)) < 0)
 goto cleanup;
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 4a9134e..6ab4f07 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -2472,6 +2472,8 @@ libxlDomainGetVcpuPinInfo(virDomainPtr dom, int ncpumaps,

 if (pininfo && pininfo->cpumask)
 bitmap = pininfo->cpumask;
+else if (targetDef->cpumask)
+bitmap = targetDef->cpumask;
 else
 bitmap = allcpumap;

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b8a60d8..c9d6d00 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4677,33 +4677,6 @@ qemuDomainAddCgroupForThread(virCgroupPtr cgroup,
 return NULL;
 }

-static int
-qemuDomainHotplugAddPin(virBitmapPtr cpumask,
-int idx,
-virDomainPinDefPtr **pindef_list,
-size_t *npin)
-{
-int ret = -1;
-virDomainPinDefPtr pindef = NULL;
-
-if (VIR_ALLOC(pindef) < 0)
-goto cleanup;
-
-if (!(pindef->cpumask = virBitmapNewCopy(cpumask))) {
-VIR_FREE(pindef);
-goto cleanup;
-}
-pindef->id = idx;
-if (VIR_APPEND_ELEMENT_COPY(*pindef_list, *npin, pindef) < 0) {
-virBitmapFree(pindef->cpumask);
-VIR_FREE(pindef);
-goto cleanup;
-}
-ret = 0;
-
- cleanup:
-return ret;
-}

 static int
 qemuDomainHotplugPinThread(virBitmapPtr cpumask,
@@ -4820,11 +4793,6 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,

 /* Inherit def->cpuset */
 if (vm->def->cpumask) {
-if (qemuDomainHotplugAddPin(vm->def->cpumask, vcpu,
->def->cputune.vcpupin,
->def->cputune.nvcpupin) < 0)
-goto cleanup;
-
 if 

[libvirt] [PATCH v2 13/21] util: bitmap: Introduce bitmap subtraction

[Peter Krempa]

Performs binary subtraction of two bitmaps. Stores result in the first
operand.
---
 src/libvirt_private.syms |  1 +
 src/util/virbitmap.c | 21 ++
 src/util/virbitmap.h |  3 +++
 tests/virbitmaptest.c| 55 
 4 files changed, 80 insertions(+)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 3806238..7e9102a 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1155,6 +1155,7 @@ virBitmapSetAll;
 virBitmapSetBit;
 virBitmapSize;
 virBitmapString;
+virBitmapSubtract;
 virBitmapToData;
 virBitmapToDataBuf;

diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c
index 57135a0..f116607 100644
--- a/src/util/virbitmap.c
+++ b/src/util/virbitmap.c
@@ -859,3 +859,24 @@ virBitmapOverlaps(virBitmapPtr b1,

 return false;
 }
+
+/**
+ * virBitmapSubtract:
+ * @a: minuend/result
+ * @b: subtrahend
+ *
+ * Performs bitwise subtraction: a = a - b
+ */
+void
+virBitmapSubtract(virBitmapPtr a,
+  virBitmapPtr b)
+{
+size_t i;
+size_t max = a->map_len;
+
+if (max > b->map_len)
+max = b->map_len;
+
+for (i = 0; i < max; i++)
+a->map[i] &= ~b->map[i];
+}
diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h
index 47488de..846aca3 100644
--- a/src/util/virbitmap.h
+++ b/src/util/virbitmap.h
@@ -129,4 +129,7 @@ bool virBitmapOverlaps(virBitmapPtr b1,
virBitmapPtr b2)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

+void virBitmapSubtract(virBitmapPtr a, virBitmapPtr b)
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+
 #endif
diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c
index 8e458d2..967a5c8 100644
--- a/tests/virbitmaptest.c
+++ b/tests/virbitmaptest.c
@@ -552,9 +552,55 @@ test10(const void *opaque ATTRIBUTE_UNUSED)
 return ret;
 }

+struct testBinaryOpData {
+const char *a;
+const char *b;
+const char *res;
+};
+
+static int
+test11(const void *opaque)
+{
+const struct testBinaryOpData *data = opaque;
+virBitmapPtr amap = NULL;
+virBitmapPtr bmap = NULL;
+virBitmapPtr resmap = NULL;
+int ret = -1;
+
+if (virBitmapParse(data->a, 0, , 256) < 0 ||
+virBitmapParse(data->b, 0, , 256) < 0 ||
+virBitmapParse(data->res, 0, , 256) < 0)
+goto cleanup;
+
+virBitmapSubtract(amap, bmap);
+
+if (!virBitmapEqual(amap, resmap)) {
+fprintf(stderr, "\n bitmap subtraction failed: '%s'-'%s'!='%s'\n",
+data->a, data->b, data->res);
+goto cleanup;
+}
+
+ret = 0;
+
+ cleanup:
+virBitmapFree(amap);
+virBitmapFree(bmap);
+virBitmapFree(resmap);
+
+return ret;
+}
+
+#define TESTBINARYOP(A, B, RES, FUNC) \
+testBinaryOpData.a = A;   \
+testBinaryOpData.b = B;   \
+testBinaryOpData.res = RES;   \
+if (virtTestRun(virtTestCounterNext(), FUNC, ) < 0)  \
+ret = -1;
+
 static int
 mymain(void)
 {
+struct testBinaryOpData testBinaryOpData;
 int ret = 0;

 if (virtTestRun("test1", test1, NULL) < 0)
@@ -578,6 +624,15 @@ mymain(void)
 if (virtTestRun("test10", test10, NULL) < 0)
 ret = -1;

+virtTestCounterReset("test11-");
+TESTBINARYOP("0", "0", "0,^0", test11);
+TESTBINARYOP("0-3", "0", "1-3", test11);
+TESTBINARYOP("0-3", "0,3", "1-2", test11);
+TESTBINARYOP("0,^0", "0", "0,^0", test11);
+TESTBINARYOP("0-3", "0-3", "0,^0", test11);
+TESTBINARYOP("0-3", "0,^0", "0-3", test11);
+TESTBINARYOP("0,2", "1,3", "0,2", test11);
+
 return ret;
 }

-- 
2.6.2

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

[libvirt] [PATCH v2 20/21] qemu: iothread: Aggregate code to set IOTrhead tuning

[Peter Krempa]

Rather than iterating 3 times for various settings this function
aggregates all the code into single place. One of the other advantages
is that it can then be reused for properly setting IOThread info on
hotplug.
---
 src/qemu/qemu_cgroup.c  |  93 ---
 src/qemu/qemu_cgroup.h  |   1 -
 src/qemu/qemu_process.c | 165 +---
 src/qemu/qemu_process.h |   2 +
 4 files changed, 116 insertions(+), 145 deletions(-)

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 0a73477..eb55d68 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -1067,99 +1067,6 @@ qemuSetupCgroupForEmulator(virDomainObjPtr vm)
 return -1;
 }

-int
-qemuSetupCgroupForIOThreads(virDomainObjPtr vm)
-{
-virCgroupPtr cgroup_iothread = NULL;
-qemuDomainObjPrivatePtr priv = vm->privateData;
-virDomainDefPtr def = vm->def;
-size_t i;
-unsigned long long period = vm->def->cputune.period;
-long long quota = vm->def->cputune.quota;
-char *mem_mask = NULL;
-virDomainNumatuneMemMode mem_mode;
-
-if (def->niothreadids == 0)
-return 0;
-
-if ((period || quota) &&
-!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("cgroup cpu is required for scheduler tuning"));
-return -1;
-}
-
-/*
- * If CPU cgroup controller is not initialized here, then we need
- * neither period nor quota settings.  And if CPUSET controller is
- * not initialized either, then there's nothing to do anyway.
- */
-if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU) &&
-!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET))
-return 0;
-
-if (virDomainNumatuneGetMode(vm->def->numa, -1, _mode) == 0 &&
-mem_mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
-virDomainNumatuneMaybeFormatNodeset(vm->def->numa,
-priv->autoNodeset,
-_mask, -1) < 0)
-goto cleanup;
-
-for (i = 0; i < def->niothreadids; i++) {
-/* IOThreads are numbered 1..n, although the array is 0..n-1,
- * so we will account for that here
- */
-if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_IOTHREAD,
-   def->iothreadids[i]->iothread_id,
-   true, _iothread) < 0)
-goto cleanup;
-
-if (period || quota) {
-if (qemuSetupCgroupVcpuBW(cgroup_iothread, period, quota) < 0)
-goto cleanup;
-}
-
-/* Set iothreadpin in cgroup if iothreadpin xml is provided */
-if (virCgroupHasController(priv->cgroup,
-   VIR_CGROUP_CONTROLLER_CPUSET)) {
-virBitmapPtr cpumask = NULL;
-
-if (mem_mask &&
-virCgroupSetCpusetMems(cgroup_iothread, mem_mask) < 0)
-goto cleanup;
-
-if (def->iothreadids[i]->cpumask)
-cpumask = def->iothreadids[i]->cpumask;
-else if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO)
-cpumask = priv->autoCpuset;
-else
-cpumask = def->cpumask;
-
-if (cpumask &&
-qemuSetupCgroupCpusetCpus(cgroup_iothread, cpumask) < 0)
-goto cleanup;
-}
-
-/* move the thread for iothread to sub dir */
-if (virCgroupAddTask(cgroup_iothread,
- def->iothreadids[i]->thread_id) < 0)
-goto cleanup;
-
-virCgroupFree(_iothread);
-}
-VIR_FREE(mem_mask);
-
-return 0;
-
- cleanup:
-if (cgroup_iothread) {
-virCgroupRemove(cgroup_iothread);
-virCgroupFree(_iothread);
-}
-VIR_FREE(mem_mask);
-
-return -1;
-}

 int
 qemuRemoveCgroup(virQEMUDriverPtr driver,
diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h
index fa3353a..a9718b5 100644
--- a/src/qemu/qemu_cgroup.h
+++ b/src/qemu/qemu_cgroup.h
@@ -53,7 +53,6 @@ int qemuSetupCgroupVcpuBW(virCgroupPtr cgroup,
   unsigned long long period,
   long long quota);
 int qemuSetupCgroupCpusetCpus(virCgroupPtr cgroup, virBitmapPtr cpumask);
-int qemuSetupCgroupForIOThreads(virDomainObjPtr vm);
 int qemuSetupCgroupForEmulator(virDomainObjPtr vm);
 int qemuRemoveCgroup(virQEMUDriverPtr driver, virDomainObjPtr vm);
 int qemuAddToCgroup(virDomainObjPtr vm);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 1682a76..f7ff572 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2208,47 +2208,6 @@ qemuProcessSetEmulatorAffinity(virDomainObjPtr vm)
 return ret;
 }

-/* Set CPU affinities for IOThreads threads. */
-static int
-qemuProcessSetIOThreadsAffinity(virDomainObjPtr vm)
-{
-

[libvirt] [PATCH v2 02/21] conf: Add helper to retrieve bitmap of active vcpus for a definition

[Peter Krempa]

In some cases it may be better to have a bitmap representing state of
individual vcpus rather than iterating the definition. The new helper
creates a bitmap representing the state from the domain definition.
---

Notes:
v2:
- renamed

 src/conf/domain_conf.c   | 24 
 src/conf/domain_conf.h   |  1 +
 src/libvirt_private.syms |  1 +
 3 files changed, 26 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1ea74a6..ead76a1 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1374,6 +1374,30 @@ virDomainDefGetVcpus(const virDomainDef *def)
 }


+/**
+ * virDomainDefGetOnlineVcpumap:
+ * @def: domain definition
+ *
+ * Returns a bitmap representing state of individual vcpus.
+ */
+virBitmapPtr
+virDomainDefGetOnlineVcpumap(const virDomainDef *def)
+{
+virBitmapPtr ret = NULL;
+size_t i;
+
+if (!(ret = virBitmapNew(def->maxvcpus)))
+return NULL;
+
+for (i = 0; i < def->maxvcpus; i++) {
+if (def->vcpus[i].online)
+ignore_value(virBitmapSetBit(ret, i));
+}
+
+return ret;
+}
+
+
 virDomainVcpuInfoPtr
 virDomainDefGetVcpu(virDomainDefPtr def,
 unsigned int vcpu)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 0141009..9fdfdf2 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2358,6 +2358,7 @@ bool virDomainDefHasVcpusOffline(const virDomainDef *def);
 unsigned int virDomainDefGetVcpusMax(const virDomainDef *def);
 int virDomainDefSetVcpus(virDomainDefPtr def, unsigned int vcpus);
 unsigned int virDomainDefGetVcpus(const virDomainDef *def);
+virBitmapPtr virDomainDefGetOnlineVcpumap(const virDomainDef *def);
 virDomainVcpuInfoPtr virDomainDefGetVcpu(virDomainDefPtr def, unsigned int 
vcpu)
 ATTRIBUTE_RETURN_CHECK;

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 3d0ec9d..1083782 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -217,6 +217,7 @@ virDomainDefFree;
 virDomainDefGetDefaultEmulator;
 virDomainDefGetMemoryActual;
 virDomainDefGetMemoryInitial;
+virDomainDefGetOnlineVcpumap;
 virDomainDefGetSecurityLabelDef;
 virDomainDefGetVcpu;
 virDomainDefGetVcpus;
-- 
2.6.2

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

Re: [libvirt] [PATCH v2 2/6] logical: Fix parsing of the 'devices' field lvs output

[Pavel Hrdina]

On Thu, Jan 28, 2016 at 05:44:05PM -0500, John Ferlan wrote:

[...]

> Signed-off-by: John Ferlan 
> ---
>  src/storage/storage_backend_logical.c | 149 
> --
>  tests/virstringtest.c |  11 +++
>  2 files changed, 62 insertions(+), 98 deletions(-)
> 
> diff --git a/src/storage/storage_backend_logical.c 
> b/src/storage/storage_backend_logical.c
> index 7c05b6a..3232c08 100644
> --- a/src/storage/storage_backend_logical.c
> +++ b/src/storage/storage_backend_logical.c
> @@ -64,8 +64,6 @@ virStorageBackendLogicalSetActive(virStoragePoolObjPtr pool,
>  }
>  
>  
> -#define VIR_STORAGE_VOL_LOGICAL_SEGTYPE_STRIPED "striped"
> -
>  struct virStorageBackendLogicalPoolVolData {
>  virStoragePoolObjPtr pool;
>  virStorageVolDefPtr vol;
> @@ -75,121 +73,76 @@ static int
>  virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol,
>  char **const groups)
>  {
> -int nextents, ret = -1;
> -const char *regex_unit = "(\\S+)\\((\\S+)\\)";
> -char *regex = NULL;
> -regex_t *reg = NULL;
> -regmatch_t *vars = NULL;
> -char *p = NULL;
> -size_t i;
> -int err, nvars;
> +int ret = -1;
> +char **extents = NULL;
> +char *extnum, *end;
> +size_t i, nextents = 0;
>  unsigned long long offset, size, length;
>  
> -nextents = 1;
> -if (STREQ(groups[4], VIR_STORAGE_VOL_LOGICAL_SEGTYPE_STRIPED)) {
> -if (virStrToLong_i(groups[5], NULL, 10, ) < 0) {
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -   _("malformed volume extent stripes value"));
> -goto cleanup;
> -}
> -}
> +/* The 'devices' (or extents) are split by a comma ",", so let's split
> + * each out into a parseable string. Since our regex to generate this
> + * data is "(\\S+)", we can safely assume at least one exists. */
> +if (!(extents = virStringSplitCount(groups[3], ",", 0, )))
> +goto cleanup;

At first I thought of the same solution but what if the device path contains
a comma?  I know, it's very unlikely but it's possible.

>  /* Allocate and fill in extents information */
>  if (VIR_REALLOC_N(vol->source.extents,
>vol->source.nextent + nextents) < 0)
>  goto cleanup;
> +vol->source.nextent += nextents;

I've also mentioned, that this could be done using VIR_APPEND_ELEMENT by
dropping this pre-allocation and ... [1]

>  
> -if (virStrToLong_ull(groups[6], NULL, 10, ) < 0) {
> +if (virStrToLong_ull(groups[5], NULL, 10, ) < 0) {
>  virReportError(VIR_ERR_INTERNAL_ERROR,
> "%s", _("malformed volume extent length value"));
>  goto cleanup;
>  }
>  
> -if (virStrToLong_ull(groups[7], NULL, 10, ) < 0) {
> +if (virStrToLong_ull(groups[6], NULL, 10, ) < 0) {
>  virReportError(VIR_ERR_INTERNAL_ERROR,
> "%s", _("malformed volume extent size value"));
>  goto cleanup;
>  }
>  
> -if (VIR_STRDUP(regex, regex_unit) < 0)
> -goto cleanup;
> -
> -for (i = 1; i < nextents; i++) {
> -if (VIR_REALLOC_N(regex, strlen(regex) + strlen(regex_unit) + 2) < 0)
> -goto cleanup;
> -/* "," is the separator of "devices" field */
> -strcat(regex, ",");
> -strncat(regex, regex_unit, strlen(regex_unit));
> -}

I would rather allocate the string with correct size outside of the cycle as we
know the length and just simply fill the string in a cycle.  The regex is more
robust than splitting the string by comma.

[...]

> -
> -vol->source.extents[vol->source.nextent].start = offset * size;
> -vol->source.extents[vol->source.nextent].end = (offset * size) + 
> length;
> -vol->source.nextent++;
> +vol->source.extents[i].start = offset * size;
> +vol->source.extents[i].end = (offset * size) + length;

[1] ... there you would call VIR_APPEND_ELEMENT().

>  }
> -
>  ret = 0;
>  
>   cleanup:
> -VIR_FREE(regex);
> -VIR_FREE(reg);
> -VIR_FREE(vars);
> +virStringFreeList(extents);
> +
>  return ret;

The cleanup is nice, but still I would rather use VIR_APPEND_ELEMENT as it's
more common in libvirt to create a new array of some elements.

One more question about the mirror device name, it's not actually a device name
but internal name used by lvm and it's mapped to real device.  Shouldn't we do
the job for user and display a real device?

Pavel

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

Re: [libvirt] [PATCH 26/34] util: bitmap: Introduce bitmap subtraction

[Peter Krempa]

On Mon, Jan 18, 2016 at 18:05:12 -0500, John Ferlan wrote:
> On 01/14/2016 11:27 AM, Peter Krempa wrote:

[...]

> > 
> > +virtTestCounterReset("test11-");
> > +TESTBINARYOP("0", "0", "0,^0", test11);
> > +TESTBINARYOP("0-3", "0", "1-3", test11);
> > +TESTBINARYOP("0-3", "0,3", "1-2", test11);
> > +TESTBINARYOP("0,^0", "0", "0,^0", test11);
> > +TESTBINARYOP("0-3", "0-3", "0,^0", test11);
> > +TESTBINARYOP("0-3", "0,^0", "0-3", test11);
> > +TESTBINARYOP("0,2", "1,3", "0,2", test11);
> > +
> 
> 0,^0 is "interesting" considering patches 22 & 23

Well, that's a simple way how to describe a empty bitmap without any
hacks that should be used with virBitmapParse since that function
doesn't parse empty string as an empty bitmap.

> 
> Concept seems fine though... although I guess I'd "half expect" there
> would be a need for "Add" as well as "Subtract".

Not really. The code using this function doesn't add bits. It collects a
bitmap and removes processed entries.

> 

Peter


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

[libvirt] [PATCH v2 11/21] conf: remove unused cpu pinning helpers and data structures

[Peter Krempa]

Now that the pinning info is stored elsewhere we can delete all the
obsolete code.
---
 src/conf/domain_conf.c   | 136 ---
 src/conf/domain_conf.h   |  27 --
 src/libvirt_private.syms |   6 ---
 3 files changed, 169 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c4b735e..e289b6f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2273,39 +2273,6 @@ virDomainClockDefClear(virDomainClockDefPtr def)
 VIR_FREE(def->timers);
 }

-virDomainPinDefPtr *
-virDomainPinDefCopy(virDomainPinDefPtr *src, int npin)
-{
-size_t i;
-virDomainPinDefPtr *ret = NULL;
-
-if (VIR_ALLOC_N(ret, npin) < 0)
-goto error;
-
-for (i = 0; i < npin; i++) {
-if (VIR_ALLOC(ret[i]) < 0)
-goto error;
-ret[i]->id = src[i]->id;
-if ((ret[i]->cpumask = virBitmapNewCopy(src[i]->cpumask)) == NULL)
-goto error;
-}
-
-return ret;
-
- error:
-if (ret) {
-for (i = 0; i < npin; i++) {
-if (ret[i]) {
-virBitmapFree(ret[i]->cpumask);
-VIR_FREE(ret[i]);
-}
-}
-VIR_FREE(ret);
-}
-
-return NULL;
-}
-

 static bool
 virDomainIOThreadIDArrayHasPin(virDomainDefPtr def)
@@ -2400,31 +2367,6 @@ virDomainIOThreadIDDefArrayInit(virDomainDefPtr def)


 void
-virDomainPinDefFree(virDomainPinDefPtr def)
-{
-if (def) {
-virBitmapFree(def->cpumask);
-VIR_FREE(def);
-}
-}
-
-void
-virDomainPinDefArrayFree(virDomainPinDefPtr *def,
- int npin)
-{
-size_t i;
-
-if (!def)
-return;
-
-for (i = 0; i < npin; i++)
-virDomainPinDefFree(def[i]);
-
-VIR_FREE(def);
-}
-
-
-void
 virDomainResourceDefFree(virDomainResourceDefPtr resource)
 {
 if (!resource)
@@ -18397,84 +18339,6 @@ virDomainIOThreadSchedDelId(virDomainDefPtr def,
 }
 }

-virDomainPinDefPtr
-virDomainPinFind(virDomainPinDefPtr *def,
- int npin,
- int id)
-{
-size_t i;
-
-if (!def || !npin)
-return NULL;
-
-for (i = 0; i < npin; i++) {
-if (def[i]->id == id)
-return def[i];
-}
-
-return NULL;
-}
-
-int
-virDomainPinAdd(virDomainPinDefPtr **pindef_list,
-size_t *npin,
-unsigned char *cpumap,
-int maplen,
-int id)
-{
-virDomainPinDefPtr pindef = NULL;
-
-if (!pindef_list)
-return -1;
-
-pindef = virDomainPinFind(*pindef_list, *npin, id);
-if (pindef) {
-pindef->id = id;
-virBitmapFree(pindef->cpumask);
-pindef->cpumask = virBitmapNewData(cpumap, maplen);
-if (!pindef->cpumask)
-return -1;
-
-return 0;
-}
-
-/* No existing pindef matches id, adding a new one */
-
-if (VIR_ALLOC(pindef) < 0)
-goto error;
-
-pindef->id = id;
-pindef->cpumask = virBitmapNewData(cpumap, maplen);
-if (!pindef->cpumask)
-goto error;
-
-if (VIR_APPEND_ELEMENT(*pindef_list, *npin, pindef) < 0)
-goto error;
-
-return 0;
-
- error:
-virDomainPinDefFree(pindef);
-return -1;
-}
-
-void
-virDomainPinDel(virDomainPinDefPtr **pindef_list,
-size_t *npin,
-int id)
-{
-int n;
-
-for (n = 0; n < *npin; n++) {
-if ((*pindef_list)[n]->id == id) {
-virBitmapFree((*pindef_list)[n]->cpumask);
-VIR_FREE((*pindef_list)[n]);
-VIR_DELETE_ELEMENT(*pindef_list, n, *npin);
-return;
-}
-}
-}
-

 static int
 virDomainEventActionDefFormat(virBufferPtr buf,
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index f33c575..6a5f615 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2100,33 +2100,6 @@ struct _virDomainIOThreadIDDef {
 void virDomainIOThreadIDDefFree(virDomainIOThreadIDDefPtr def);


-typedef struct _virDomainPinDef virDomainPinDef;
-typedef virDomainPinDef *virDomainPinDefPtr;
-struct _virDomainPinDef {
-int id;
-virBitmapPtr cpumask;
-};
-
-void virDomainPinDefFree(virDomainPinDefPtr def);
-void virDomainPinDefArrayFree(virDomainPinDefPtr *def, int npin);
-
-virDomainPinDefPtr *virDomainPinDefCopy(virDomainPinDefPtr *src,
-int npin);
-
-virDomainPinDefPtr virDomainPinFind(virDomainPinDefPtr *def,
-int npin,
-int id);
-
-int virDomainPinAdd(virDomainPinDefPtr **pindef_list,
-size_t *npin,
-unsigned char *cpumap,
-int maplen,
-int id);
-
-void virDomainPinDel(virDomainPinDefPtr **pindef_list,
- size_t *npin,
- int vcpu);
-
 typedef struct _virDomainCputune virDomainCputune;
 typedef virDomainCputune *virDomainCputunePtr;

diff --git 

[libvirt] [PATCH v2 16/21] conf: Don't store vcpusched orthogonally to other vcpu info

[Peter Krempa]

Due to bad design the vcpu sched element is orthogonal to the way how
the data belongs to the corresponding objects. Now that vcpus are a
struct that allow to store other info too, let's convert the data to the
sane structure.

The helpers for the conversion are made universal so that they can be
reused for iothreads too.

This patch also resolves https://bugzilla.redhat.com/show_bug.cgi?id=1235180
since with the correct storage approach you can't have dangling data.
---
 src/conf/domain_conf.c  | 246 +++-
 src/conf/domain_conf.h  |   5 +-
 src/qemu/qemu_driver.c  |   6 +-
 src/qemu/qemu_process.c |   8 +-
 4 files changed, 214 insertions(+), 51 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1ee9041..1d76beb 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1416,6 +1416,19 @@ virDomainDefGetVcpu(virDomainDefPtr def,
 }


+static virDomainThreadSchedParamPtr
+virDomainDefGetVcpuSched(virDomainDefPtr def,
+ unsigned int vcpu)
+{
+virDomainVcpuInfoPtr vcpuinfo;
+
+if (!(vcpuinfo = virDomainDefGetVcpu(def, vcpu)))
+return NULL;
+
+return >sched;
+}
+
+
 /**
  * virDomainDefHasVcpuPin:
  * @def: domain definition
@@ -2547,10 +2560,6 @@ void virDomainDefFree(virDomainDefPtr def)

 virBitmapFree(def->cputune.emulatorpin);

-for (i = 0; i < def->cputune.nvcpusched; i++)
-virBitmapFree(def->cputune.vcpusched[i].ids);
-VIR_FREE(def->cputune.vcpusched);
-
 for (i = 0; i < def->cputune.niothreadsched; i++)
 virBitmapFree(def->cputune.iothreadsched[i].ids);
 VIR_FREE(def->cputune.iothreadsched);
@@ -14593,6 +14602,55 @@ virDomainSchedulerParse(xmlNodePtr node,


 static int
+virDomainThreadSchedParseHelper(xmlNodePtr node,
+const char *name,
+virDomainThreadSchedParamPtr 
(*func)(virDomainDefPtr, unsigned int),
+virDomainDefPtr def)
+{
+ssize_t next = -1;
+virBitmapPtr map = NULL;
+virDomainThreadSchedParamPtr sched;
+virProcessSchedPolicy policy;
+int priority;
+int ret = -1;
+
+if (!(map = virDomainSchedulerParse(node, name, , )))
+goto cleanup;
+
+while ((next = virBitmapNextSetBit(map, next)) > -1) {
+if (!(sched = func(def, next)))
+goto cleanup;
+
+if (sched->policy != VIR_PROC_POLICY_NONE) {
+virReportError(VIR_ERR_XML_DETAIL,
+   _("%ssched attributes 'vcpus' must not overlap"),
+   name);
+goto cleanup;
+}
+
+sched->policy = policy;
+sched->priority = priority;
+}
+
+ret = 0;
+
+ cleanup:
+virBitmapFree(map);
+return ret;
+}
+
+
+static int
+virDomainVcpuThreadSchedParse(xmlNodePtr node,
+  virDomainDefPtr def)
+{
+return virDomainThreadSchedParseHelper(node, "vcpus",
+   virDomainDefGetVcpuSched,
+   def);
+}
+
+
+static int
 virDomainThreadSchedParse(xmlNodePtr node,
   unsigned int minid,
   unsigned int maxid,
@@ -15146,29 +15204,10 @@ virDomainDefParseXML(xmlDocPtr xml,
_("cannot extract vcpusched nodes"));
 goto error;
 }
-if (n) {
-if (VIR_ALLOC_N(def->cputune.vcpusched, n) < 0)
-goto error;
-def->cputune.nvcpusched = n;
-
-for (i = 0; i < def->cputune.nvcpusched; i++) {
-if (virDomainThreadSchedParse(nodes[i],
-  0,
-  virDomainDefGetVcpusMax(def) - 1,
-  "vcpus",
-  >cputune.vcpusched[i]) < 0)
-goto error;

-for (j = 0; j < i; j++) {
-if (virBitmapOverlaps(def->cputune.vcpusched[i].ids,
-  def->cputune.vcpusched[j].ids)) {
-virReportError(VIR_ERR_XML_DETAIL, "%s",
-   _("vcpusched attributes 'vcpus' "
- "must not overlap"));
-goto error;
-}
-}
-}
+for (i = 0; i < n; i++) {
+if (virDomainVcpuThreadSchedParse(nodes[i], def) < 0)
+goto error;
 }
 VIR_FREE(nodes);

@@ -21465,6 +21504,143 @@ virDomainDefHasCapabilitiesFeatures(virDomainDefPtr 
def)
 }


+/**
+ * virDomainFormatSchedDef:
+ * @def: domain definiton
+ * @buf: target XML buffer
+ * @name: name of the target XML element
+ * @func: function that returns the thread scheduler parameter struct for an 
object
+ * @resourceMap: bitmap of indexes of objects that shall be formated (used 
with @func
+ *
+ * Formats one of the two scheduler tuning elements 

[libvirt] [PATCH v2 07/21] qemu: Reuse qemuDomainDetectVcpuPids in cpu hot(un)plug

[Peter Krempa]

Now that qemuDomainDetectVcpuPids is able to refresh the vCPU pid
information it can be reused in the hotplug and hotunplug code paths
rather than open-coding a very similar algorithm.

A slight algoirithm change is necessary for unplug since the vCPU needs
to be marked offline prior to calling the thread detector function and
eventually rolled back if something fails.
---

Notes:
v2: fix bugs regarding error codes from redetection

 src/qemu/qemu_driver.c | 86 +-
 1 file changed, 29 insertions(+), 57 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9dce1b2..b8a60d8 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4765,11 +4765,10 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,
 int ret = -1;
 int rc;
 int oldvcpus = virDomainDefGetVcpus(vm->def);
-pid_t *cpupids = NULL;
-int ncpupids = 0;
 virCgroupPtr cgroup_vcpu = NULL;
 char *mem_mask = NULL;
 virDomainNumatuneMemMode mem_mode;
+pid_t vcpupid;

 if (!(vcpuinfo = virDomainDefGetVcpu(vm->def, vcpu)))
 return -1;
@@ -4784,9 +4783,6 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,

 rc = qemuMonitorSetCPU(priv->mon, vcpu, true);

-if (rc == 0)
-ncpupids = qemuMonitorGetCPUInfo(priv->mon, );
-
 if (qemuDomainObjExitMonitor(driver, vm) < 0)
 goto cleanup;

@@ -4797,23 +4793,15 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,

 vcpuinfo->online = true;

-if (ncpupids < 0)
-goto cleanup;
+if ((rc = qemuDomainDetectVcpuPids(driver, vm, QEMU_ASYNC_JOB_NONE)) <= 0) 
{
+/* vcpu pids were not detected, skip setting of affinity */
+if (rc == 0)
+ret = 0;

-/* failure to re-detect vCPU pids after hotplug due to lack of support was
- * historically deemed not fatal. We need to skip the rest of the steps 
though. */
-if (ncpupids == 0) {
-ret = 0;
 goto cleanup;
 }

-if (ncpupids != oldvcpus + 1) {
-virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("got wrong number of vCPU pids from QEMU monitor. "
- "got %d, wanted %d"),
-   ncpupids, oldvcpus + 1);
-goto cleanup;
-}
+vcpupid = qemuDomainGetVcpuPid(vm, vcpu);

 if (virDomainNumatuneGetMode(vm->def->numa, -1, _mode) == 0 &&
 mem_mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
@@ -4823,11 +4811,9 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,
 goto cleanup;

 if (priv->cgroup) {
-cgroup_vcpu =
-qemuDomainAddCgroupForThread(priv->cgroup,
- VIR_CGROUP_THREAD_VCPU,
- vcpu, mem_mask,
- cpupids[vcpu]);
+cgroup_vcpu = qemuDomainAddCgroupForThread(priv->cgroup,
+   VIR_CGROUP_THREAD_VCPU,
+   vcpu, mem_mask, vcpupid);
 if (!cgroup_vcpu)
 goto cleanup;
 }
@@ -4839,26 +4825,20 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,
 >def->cputune.nvcpupin) < 0)
 goto cleanup;

-if (qemuDomainHotplugPinThread(vm->def->cpumask, vcpu, cpupids[vcpu],
+if (qemuDomainHotplugPinThread(vm->def->cpumask, vcpu, vcpupid,
cgroup_vcpu) < 0) {
 goto cleanup;
 }
 }

-if (qemuProcessSetSchedParams(vcpu, cpupids[vcpu],
+if (qemuProcessSetSchedParams(vcpu, vcpupid,
   vm->def->cputune.nvcpusched,
   vm->def->cputune.vcpusched) < 0)
 goto cleanup;

-priv->nvcpupids = ncpupids;
-VIR_FREE(priv->vcpupids);
-priv->vcpupids = cpupids;
-cpupids = NULL;
-
 ret = 0;

  cleanup:
-VIR_FREE(cpupids);
 VIR_FREE(mem_mask);
 virCgroupFree(_vcpu);
 return ret;
@@ -4875,8 +4855,6 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver,
 int ret = -1;
 int rc;
 int oldvcpus = virDomainDefGetVcpus(vm->def);
-pid_t *cpupids = NULL;
-int ncpupids = 0;

 if (!(vcpuinfo = virDomainDefGetVcpu(vm->def, vcpu)))
 return -1;
@@ -4887,49 +4865,43 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver,
 return -1;
 }

+vcpuinfo->online = false;
+
 qemuDomainObjEnterMonitor(driver, vm);

 rc = qemuMonitorSetCPU(priv->mon, vcpu, false);

-if (rc == 0)
-ncpupids = qemuMonitorGetCPUInfo(priv->mon, );
-
 if (qemuDomainObjExitMonitor(driver, vm) < 0)
 goto cleanup;

-virDomainAuditVcpu(vm, oldvcpus, oldvcpus - 1, "update",
-   rc == 0 && ncpupids == oldvcpus -1);
+if (rc < 0)
+rc = qemuDomainDetectVcpuPids(driver, vm, QEMU_ASYNC_JOB_NONE);

-if (rc < 0 || ncpupids < 0)
-goto cleanup;
+ 

[libvirt] [PATCH v2 17/21] conf: Fix how iothread scheduler info is stored

[Peter Krempa]

Similarly to previous commit change the way how iothread scheduler info
is stored and clean up a lot of unnecessary code.
---
 src/conf/domain_conf.c | 141 +++--
 src/conf/domain_conf.h |   8 +-
 src/libvirt_private.syms   |   1 -
 src/qemu/qemu_driver.c |   3 -
 src/qemu/qemu_process.c|  39 +-
 .../qemuxml2xmlout-cputune-iothreadsched.xml   |   3 +-
 6 files changed, 53 insertions(+), 142 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1d76beb..8c372aa 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2560,10 +2560,6 @@ void virDomainDefFree(virDomainDefPtr def)

 virBitmapFree(def->cputune.emulatorpin);

-for (i = 0; i < def->cputune.niothreadsched; i++)
-virBitmapFree(def->cputune.iothreadsched[i].ids);
-VIR_FREE(def->cputune.iothreadsched);
-
 virDomainNumaFree(def->numa);

 virSysinfoDefFree(def->sysinfo);
@@ -14650,25 +14646,26 @@ virDomainVcpuThreadSchedParse(xmlNodePtr node,
 }


-static int
-virDomainThreadSchedParse(xmlNodePtr node,
-  unsigned int minid,
-  unsigned int maxid,
-  const char *name,
-  virDomainThreadSchedParamPtr sp)
-{
-if (!(sp->ids = virDomainSchedulerParse(node, name, >policy,
->priority)))
-return -1;
+static virDomainThreadSchedParamPtr
+virDomainDefGetIOThreadSched(virDomainDefPtr def,
+ unsigned int iothread)
+{
+virDomainIOThreadIDDefPtr iothrinfo;

-if (virBitmapNextSetBit(sp->ids, -1) < minid ||
-virBitmapLastSetBit(sp->ids) > maxid) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("%sched bitmap is out of range"), name);
-return -1;
-}
+if (!(iothrinfo = virDomainIOThreadIDFind(def, iothread)))
+return NULL;

-return 0;
+return >sched;
+}
+
+
+static int
+virDomainIOThreadSchedParse(xmlNodePtr node,
+virDomainDefPtr def)
+{
+return virDomainThreadSchedParseHelper(node, "iothreads",
+   virDomainDefGetIOThreadSched,
+   def);
 }


@@ -15216,46 +15213,10 @@ virDomainDefParseXML(xmlDocPtr xml,
_("cannot extract iothreadsched nodes"));
 goto error;
 }
-if (n) {
-if (n > def->niothreadids) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("too many iothreadsched nodes in cputune"));
-goto error;
-}

-if (VIR_ALLOC_N(def->cputune.iothreadsched, n) < 0)
+for (i = 0; i < n; i++) {
+if (virDomainIOThreadSchedParse(nodes[i], def) < 0)
 goto error;
-def->cputune.niothreadsched = n;
-
-for (i = 0; i < def->cputune.niothreadsched; i++) {
-ssize_t pos = -1;
-
-if (virDomainThreadSchedParse(nodes[i],
-  1, UINT_MAX,
-  "iothreads",
-  >cputune.iothreadsched[i]) < 0)
-goto error;
-
-while ((pos = 
virBitmapNextSetBit(def->cputune.iothreadsched[i].ids,
-  pos)) > -1) {
-if (!virDomainIOThreadIDFind(def, pos)) {
-virReportError(VIR_ERR_XML_DETAIL, "%s",
-   _("iothreadsched attribute 'iothreads' "
- "uses undefined iothread ids"));
-goto error;
-}
-}
-
-for (j = 0; j < i; j++) {
-if (virBitmapOverlaps(def->cputune.iothreadsched[i].ids,
-  def->cputune.iothreadsched[j].ids)) {
-virReportError(VIR_ERR_XML_DETAIL, "%s",
-   _("iothreadsched attributes 'iothreads' "
- "must not overlap"));
-goto error;
-}
-}
-}
 }
 VIR_FREE(nodes);

@@ -18405,29 +18366,6 @@ virDomainIOThreadIDDel(virDomainDefPtr def,
 }
 }

-void
-virDomainIOThreadSchedDelId(virDomainDefPtr def,
-unsigned int iothreadid)
-{
-size_t i;
-
-if (!def->cputune.iothreadsched || !def->cputune.niothreadsched)
-return;
-
-for (i = 0; i < def->cputune.niothreadsched; i++) {
-if (virBitmapIsBitSet(def->cputune.iothreadsched[i].ids, iothreadid)) {
-ignore_value(virBitmapClearBit(def->cputune.iothreadsched[i].ids,
-   iothreadid));
-if 

[libvirt] [PATCH v2 21/21] qemu: iothread: Reuse qemuProcessSetupIOThread in iothread hotplug

[Peter Krempa]

Since majority of the steps is shared, the function can be reused to
simplify code.

Similarly to previous path doing this same for vCPUs this also fixes the
a similar bug (which is not tracked).
---
 src/qemu/qemu_driver.c | 101 +
 1 file changed, 2 insertions(+), 99 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 51e2e86..60518f9 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4644,70 +4644,6 @@ static void qemuProcessEventHandler(void *data, void 
*opaque)
 VIR_FREE(processEvent);
 }

-static virCgroupPtr
-qemuDomainAddCgroupForThread(virCgroupPtr cgroup,
- virCgroupThreadName nameval,
- int idx,
- char *mem_mask,
- pid_t pid)
-{
-virCgroupPtr new_cgroup = NULL;
-int rv = -1;
-
-/* Create cgroup */
-if (virCgroupNewThread(cgroup, nameval, idx, true, _cgroup) < 0)
-return NULL;
-
-if (mem_mask &&
-virCgroupHasController(cgroup, VIR_CGROUP_CONTROLLER_CPUSET) &&
-virCgroupSetCpusetMems(new_cgroup, mem_mask) < 0)
-goto error;
-
-/* Add pid/thread to the cgroup */
-rv = virCgroupAddTask(new_cgroup, pid);
-if (rv < 0) {
-virCgroupRemove(new_cgroup);
-goto error;
-}
-
-return new_cgroup;
-
- error:
-virCgroupFree(_cgroup);
-return NULL;
-}
-
-
-static int
-qemuDomainHotplugPinThread(virBitmapPtr cpumask,
-   int idx,
-   pid_t pid,
-   virCgroupPtr cgroup)
-{
-int ret = -1;
-
-if (cgroup &&
-virCgroupHasController(cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) {
-if (qemuSetupCgroupCpusetCpus(cgroup, cpumask) < 0) {
-virReportError(VIR_ERR_OPERATION_INVALID,
-   _("failed to set cpuset.cpus in cgroup for id %d"),
-   idx);
-goto cleanup;
-}
-} else {
-if (virProcessSetAffinity(pid, cpumask) < 0) {
-virReportError(VIR_ERR_SYSTEM_ERROR,
-   _("failed to set cpu affinity for id %d"),
-   idx);
-goto cleanup;
-}
-}
-
-ret = 0;
-
- cleanup:
-return ret;
-}

 static int
 qemuDomainDelCgroupForThread(virCgroupPtr cgroup,
@@ -5928,11 +5864,7 @@ qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver,
 unsigned int exp_niothreads = vm->def->niothreadids;
 int new_niothreads = 0;
 qemuMonitorIOThreadInfoPtr *new_iothreads = NULL;
-virCgroupPtr cgroup_iothread = NULL;
-char *mem_mask = NULL;
-virDomainNumatuneMemMode mode;
 virDomainIOThreadIDDefPtr iothrid;
-virBitmapPtr cpumask;

 if (virDomainIOThreadIDFind(vm->def, iothread_id)) {
 virReportError(VIR_ERR_INVALID_ARG,
@@ -5972,14 +5904,6 @@ qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver,
 }
 vm->def->iothreads = exp_niothreads;

-if (virDomainNumatuneGetMode(vm->def->numa, -1, ) == 0 &&
-mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
-virDomainNumatuneMaybeFormatNodeset(vm->def->numa,
-priv->autoNodeset,
-_mask, -1) < 0)
-goto cleanup;
-
-
 /*
  * If we've successfully added an IOThread, find out where we added it
  * in the QEMU IOThread list, so we can add it to our iothreadids list
@@ -6001,27 +5925,8 @@ qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver,

 iothrid->thread_id = new_iothreads[idx]->thread_id;

-/* Add IOThread to cgroup if present */
-if (priv->cgroup) {
-cgroup_iothread =
-qemuDomainAddCgroupForThread(priv->cgroup,
- VIR_CGROUP_THREAD_IOTHREAD,
- iothread_id, mem_mask,
- iothrid->thread_id);
-if (!cgroup_iothread)
-goto cleanup;
-}
-
-if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO)
-cpumask = priv->autoCpuset;
-else
-cpumask = vm->def->cpumask;
-
-if (cpumask) {
-if (qemuDomainHotplugPinThread(cpumask, iothread_id,
-   iothrid->thread_id, cgroup_iothread) < 
0)
-goto cleanup;
-}
+if (qemuProcessSetupIOThread(vm, iothrid) < 0)
+goto cleanup;

 ret = 0;

@@ -6031,10 +5936,8 @@ qemuDomainHotplugAddIOThread(virQEMUDriverPtr driver,
 VIR_FREE(new_iothreads[idx]);
 VIR_FREE(new_iothreads);
 }
-VIR_FREE(mem_mask);
 virDomainAuditIOThread(vm, orig_niothreads, new_niothreads,
"update", rc == 0);
-virCgroupFree(_iothread);
 VIR_FREE(alias);
 return ret;

-- 
2.6.2

--
libvir-list mailing list
libvir-list@redhat.com

[libvirt] [PATCH v2 09/21] conf: Split out logic to determine whether cpupin was provided

[Peter Krempa]

Add a helper function to do the checking. The check is used when
determining whether the  element should be formatted.
---

Notes:
v2:
- renamed func
- used different variable to decide (not that it would matter)

 src/conf/domain_conf.c | 15 ++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e06ec80..3dc5e00 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1413,6 +1413,19 @@ virDomainDefGetVcpu(virDomainDefPtr def,
 }


+/**
+ * virDomainDefHasVcpuPin:
+ * @def: domain definition
+ *
+ * This helper returns true if any of the domain's vcpus has cpu pinning set
+ */
+static bool
+virDomainDefHasVcpuPin(const virDomainDef *def)
+{
+return !!def->cputune.vcpupin;
+}
+
+
 virDomainDiskDefPtr
 virDomainDiskDefNew(virDomainXMLOptionPtr xmlopt)
 {
@@ -15319,7 +15332,7 @@ virDomainDefParseXML(xmlDocPtr xml,
 goto error;

 if (virDomainNumatuneHasPlacementAuto(def->numa) &&
-!def->cpumask && !def->cputune.vcpupin &&
+!def->cpumask && !virDomainDefHasVcpuPin(def) &&
 !def->cputune.emulatorpin &&
 !virDomainIOThreadIDArrayHasPin(def))
 def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO;
-- 
2.6.2

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

[libvirt] [PATCH v2 18/21] qemu: vcpu: Aggregate code to set vCPU tuning

[Peter Krempa]

Rather than iterating 3 times for various settings this function
aggregates all the code into single place. One of the other advantages
is that it can then be reused for properly setting vCPU info on hotplug.
---
 src/qemu/qemu_cgroup.c  |  95 -
 src/qemu/qemu_cgroup.h  |   1 -
 src/qemu/qemu_process.c | 216 +---
 src/qemu/qemu_process.h |   4 +
 4 files changed, 152 insertions(+), 164 deletions(-)

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 8dff76c..0a73477 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -1001,101 +1001,6 @@ qemuSetupCgroupCpusetCpus(virCgroupPtr cgroup,
 return ret;
 }

-int
-qemuSetupCgroupForVcpu(virDomainObjPtr vm)
-{
-virCgroupPtr cgroup_vcpu = NULL;
-qemuDomainObjPrivatePtr priv = vm->privateData;
-virDomainDefPtr def = vm->def;
-size_t i;
-unsigned long long period = vm->def->cputune.period;
-long long quota = vm->def->cputune.quota;
-char *mem_mask = NULL;
-virDomainNumatuneMemMode mem_mode;
-
-if ((period || quota) &&
-!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("cgroup cpu is required for scheduler tuning"));
-return -1;
-}
-
-/*
- * If CPU cgroup controller is not initialized here, then we need
- * neither period nor quota settings.  And if CPUSET controller is
- * not initialized either, then there's nothing to do anyway. CPU pinning
- * will be set via virProcessSetAffinity.
- */
-if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU) &&
-!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET))
-return 0;
-
-/* If vCPU<->pid mapping is missing we can't do vCPU pinning */
-if (!qemuDomainHasVcpuPids(vm))
-return 0;
-
-if (virDomainNumatuneGetMode(vm->def->numa, -1, _mode) == 0 &&
-mem_mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
-virDomainNumatuneMaybeFormatNodeset(vm->def->numa,
-priv->autoNodeset,
-_mask, -1) < 0)
-goto cleanup;
-
-for (i = 0; i < virDomainDefGetVcpusMax(def); i++) {
-virDomainVcpuInfoPtr vcpu = virDomainDefGetVcpu(def, i);
-
-if (!vcpu->online)
-continue;
-
-virCgroupFree(_vcpu);
-if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_VCPU, i,
-   true, _vcpu) < 0)
-goto cleanup;
-
-if (period || quota) {
-if (qemuSetupCgroupVcpuBW(cgroup_vcpu, period, quota) < 0)
-goto cleanup;
-}
-
-/* Set vcpupin in cgroup if vcpupin xml is provided */
-if (virCgroupHasController(priv->cgroup, 
VIR_CGROUP_CONTROLLER_CPUSET)) {
-virBitmapPtr cpumap = NULL;
-
-if (mem_mask &&
-virCgroupSetCpusetMems(cgroup_vcpu, mem_mask) < 0)
-goto cleanup;
-
-if (vcpu->cpumask)
-cpumap = vcpu->cpumask;
-else if (vm->def->placement_mode == 
VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO)
-cpumap = priv->autoCpuset;
-else
-cpumap = vm->def->cpumask;
-
-if (cpumap && qemuSetupCgroupCpusetCpus(cgroup_vcpu, cpumap) < 0)
-goto cleanup;
-}
-
-/* move the thread for vcpu to sub dir */
-if (virCgroupAddTask(cgroup_vcpu,
- qemuDomainGetVcpuPid(vm, i)) < 0)
-goto cleanup;
-
-}
-virCgroupFree(_vcpu);
-VIR_FREE(mem_mask);
-
-return 0;
-
- cleanup:
-if (cgroup_vcpu) {
-virCgroupRemove(cgroup_vcpu);
-virCgroupFree(_vcpu);
-}
-VIR_FREE(mem_mask);
-
-return -1;
-}

 int
 qemuSetupCgroupForEmulator(virDomainObjPtr vm)
diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h
index 2bcf071..fa3353a 100644
--- a/src/qemu/qemu_cgroup.h
+++ b/src/qemu/qemu_cgroup.h
@@ -53,7 +53,6 @@ int qemuSetupCgroupVcpuBW(virCgroupPtr cgroup,
   unsigned long long period,
   long long quota);
 int qemuSetupCgroupCpusetCpus(virCgroupPtr cgroup, virBitmapPtr cpumask);
-int qemuSetupCgroupForVcpu(virDomainObjPtr vm);
 int qemuSetupCgroupForIOThreads(virDomainObjPtr vm);
 int qemuSetupCgroupForEmulator(virDomainObjPtr vm);
 int qemuRemoveCgroup(virQEMUDriverPtr driver, virDomainObjPtr vm);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index b1c2a43..1682a76 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2188,56 +2188,6 @@ qemuProcessSetLinkStates(virQEMUDriverPtr driver,
 return ret;
 }

-/* Set CPU affinities for vcpus if vcpupin xml provided. */
-static int
-qemuProcessSetVcpuAffinities(virDomainObjPtr vm)
-{
-virDomainDefPtr def = vm->def;
-

[libvirt] [PATCH v2 19/21] qemu: vcpu: Reuse qemuProcessSetupVcpu in vcpu hotplug

[Peter Krempa]

Since majority of the steps is shared, the function can be reused to
simplify code.

Additionally this resolves
https://bugzilla.redhat.com/show_bug.cgi?id=1244128 since the cpu
bandwidth limiting with cgroups would not be set on the hotplug path.
---
 src/qemu/qemu_driver.c | 35 +--
 1 file changed, 1 insertion(+), 34 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a247dc0..51e2e86 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4738,10 +4738,6 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,
 int ret = -1;
 int rc;
 int oldvcpus = virDomainDefGetVcpus(vm->def);
-virCgroupPtr cgroup_vcpu = NULL;
-char *mem_mask = NULL;
-virDomainNumatuneMemMode mem_mode;
-pid_t vcpupid;

 if (!(vcpuinfo = virDomainDefGetVcpu(vm->def, vcpu)))
 return -1;
@@ -4774,41 +4770,12 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,
 goto cleanup;
 }

-vcpupid = qemuDomainGetVcpuPid(vm, vcpu);
-
-if (virDomainNumatuneGetMode(vm->def->numa, -1, _mode) == 0 &&
-mem_mode == VIR_DOMAIN_NUMATUNE_MEM_STRICT &&
-virDomainNumatuneMaybeFormatNodeset(vm->def->numa,
-priv->autoNodeset,
-_mask, -1) < 0)
-goto cleanup;
-
-if (priv->cgroup) {
-cgroup_vcpu = qemuDomainAddCgroupForThread(priv->cgroup,
-   VIR_CGROUP_THREAD_VCPU,
-   vcpu, mem_mask, vcpupid);
-if (!cgroup_vcpu)
-goto cleanup;
-}
-
-/* Inherit def->cpuset */
-if (vm->def->cpumask) {
-if (qemuDomainHotplugPinThread(vm->def->cpumask, vcpu, vcpupid,
-   cgroup_vcpu) < 0) {
-goto cleanup;
-}
-}
-
-if (vcpuinfo->sched.policy != VIR_PROC_POLICY_NONE &&
-virProcessSetScheduler(vcpupid, vcpuinfo->sched.policy,
-   vcpuinfo->sched.priority) < 0)
+if (qemuProcessSetupVcpu(vm, vcpu) < 0)
 goto cleanup;

 ret = 0;

  cleanup:
-VIR_FREE(mem_mask);
-virCgroupFree(_vcpu);
 return ret;
 }

-- 
2.6.2

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

[libvirt] [PATCH v2 05/21] qemu: Move and rename qemuProcessDetectVcpuPIDs to qemuDomainDetectVcpuPids

[Peter Krempa]

Future patches will tweak and reuse the function in different places so
move it separately first.
---

Notes:
v2:
- fixed two typos

 src/qemu/qemu_domain.c  | 84 +
 src/qemu/qemu_domain.h  |  2 ++
 src/qemu/qemu_process.c | 76 ++--
 3 files changed, 88 insertions(+), 74 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 1df1b74..5ddf048 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4280,3 +4280,87 @@ qemuDomainGetVcpuPid(virDomainObjPtr vm,

 return priv->vcpupids[vcpu];
 }
+
+
+/**
+ * qemuDomainDetectVcpuPids:
+ * @driver: qemu driver data
+ * @vm: domain object
+ * @asyncJob: current asynchronous job type
+ *
+ * Updates vCPU thread ids in the private data of @vm.
+ *
+ * Returns 0 on success -1 on error and reports an appropriate error.
+ */
+int
+qemuDomainDetectVcpuPids(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ int asyncJob)
+{
+pid_t *cpupids = NULL;
+int ncpupids;
+qemuDomainObjPrivatePtr priv = vm->privateData;
+
+/*
+ * Current QEMU *can* report info about host threads mapped
+ * to vCPUs, but it is not in a manner we can correctly
+ * deal with. The TCG CPU emulation does have a separate vCPU
+ * thread, but it runs every vCPU in that same thread. So it
+ * is impossible to setup different affinity per thread.
+ *
+ * What's more the 'query-cpus' command returns bizarre
+ * data for the threads. It gives the TCG thread for the
+ * vCPU 0, but for vCPUs 1-> N, it actually replies with
+ * the main process thread ID.
+ *
+ * The result is that when we try to set affinity for
+ * vCPU 1, it will actually change the affinity of the
+ * emulator thread :-( When you try to set affinity for
+ * vCPUs 2, 3 it will fail if the affinity was
+ * different from vCPU 1.
+ *
+ * We *could* allow vcpu pinning with TCG, if we made the
+ * restriction that all vCPUs had the same mask. This would
+ * at least let us separate emulator from vCPUs threads, as
+ * we do for KVM. It would need some changes to our cgroups
+ * CPU layout though, and error reporting for the config
+ * restrictions.
+ *
+ * Just disable CPU pinning with TCG until someone wants
+ * to try to do this hard work.
+ */
+if (vm->def->virtType == VIR_DOMAIN_VIRT_QEMU) {
+priv->nvcpupids = 0;
+priv->vcpupids = NULL;
+return 0;
+}
+
+if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
+return -1;
+ncpupids = qemuMonitorGetCPUInfo(priv->mon, );
+if (qemuDomainObjExitMonitor(driver, vm) < 0)
+return -1;
+/* failure to get the VCPU<-> PID mapping or to execute the query
+ * command will not be treated fatal as some versions of qemu don't
+ * support this command */
+if (ncpupids <= 0) {
+virResetLastError();
+
+priv->nvcpupids = 0;
+priv->vcpupids = NULL;
+return 0;
+}
+
+if (ncpupids != virDomainDefGetVcpus(vm->def)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("got wrong number of vCPU pids from QEMU monitor. "
+ "got %d, wanted %d"),
+   ncpupids, virDomainDefGetVcpus(vm->def));
+VIR_FREE(cpupids);
+return -1;
+}
+
+priv->nvcpupids = ncpupids;
+priv->vcpupids = cpupids;
+return 0;
+}
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 7fc4fff..6a8cf70 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -508,5 +508,7 @@ int qemuDomainDefValidateMemoryHotplug(const virDomainDef 
*def,

 bool qemuDomainHasVcpuPids(virDomainObjPtr vm);
 pid_t qemuDomainGetVcpuPid(virDomainObjPtr vm, unsigned int vcpu);
+int qemuDomainDetectVcpuPids(virQEMUDriverPtr driver, virDomainObjPtr vm,
+ int asyncJob);

 #endif /* __QEMU_DOMAIN_H__ */
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index ee94d3f..7b09ba7 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1992,78 +1992,6 @@ qemuProcessWaitForMonitor(virQEMUDriverPtr driver,
 return ret;
 }

-static int
-qemuProcessDetectVcpuPIDs(virQEMUDriverPtr driver,
-  virDomainObjPtr vm, int asyncJob)
-{
-pid_t *cpupids = NULL;
-int ncpupids;
-qemuDomainObjPrivatePtr priv = vm->privateData;
-
-/*
- * Current QEMU *can* report info about host threads mapped
- * to vCPUs, but it is not in a manner we can correctly
- * deal with. The TCG CPU emulation does have a separate vCPU
- * thread, but it runs every vCPU in that same thread. So it
- * is impossible to setup different affinity per thread.
- *
- * What's more the 'query-cpus' command returns bizarre
- * data for the threads. It gives the TCG 

[libvirt] [PATCH v2 04/21] qemu: cpu hotplug: Set vcpu state directly in the new structure

[Peter Krempa]

Avoid using virDomainDefSetVcpus when we can set it directly in the
structure.
---
 src/qemu/qemu_driver.c | 24 ++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 0fa7d13..9dce1b2 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4761,6 +4761,7 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,
  unsigned int vcpu)
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
+virDomainVcpuInfoPtr vcpuinfo;
 int ret = -1;
 int rc;
 int oldvcpus = virDomainDefGetVcpus(vm->def);
@@ -4770,6 +4771,15 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,
 char *mem_mask = NULL;
 virDomainNumatuneMemMode mem_mode;

+if (!(vcpuinfo = virDomainDefGetVcpu(vm->def, vcpu)))
+return -1;
+
+if (vcpuinfo->online) {
+virReportError(VIR_ERR_INVALID_ARG,
+   _("vCPU '%u' is already online"), vcpu);
+return -1;
+}
+
 qemuDomainObjEnterMonitor(driver, vm);

 rc = qemuMonitorSetCPU(priv->mon, vcpu, true);
@@ -4785,7 +4795,7 @@ qemuDomainHotplugAddVcpu(virQEMUDriverPtr driver,
 if (rc < 0)
 goto cleanup;

-ignore_value(virDomainDefSetVcpus(vm->def, oldvcpus + 1));
+vcpuinfo->online = true;

 if (ncpupids < 0)
 goto cleanup;
@@ -4861,12 +4871,22 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver,
  unsigned int vcpu)
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
+virDomainVcpuInfoPtr vcpuinfo;
 int ret = -1;
 int rc;
 int oldvcpus = virDomainDefGetVcpus(vm->def);
 pid_t *cpupids = NULL;
 int ncpupids = 0;

+if (!(vcpuinfo = virDomainDefGetVcpu(vm->def, vcpu)))
+return -1;
+
+if (!vcpuinfo->online) {
+virReportError(VIR_ERR_INVALID_ARG,
+   _("vCPU '%u' is already offline"), vcpu);
+return -1;
+}
+
 qemuDomainObjEnterMonitor(driver, vm);

 rc = qemuMonitorSetCPU(priv->mon, vcpu, false);
@@ -4890,7 +4910,7 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver,
 goto cleanup;
 }

-ignore_value(virDomainDefSetVcpus(vm->def, oldvcpus - 1));
+vcpuinfo->online = false;

 if (qemuDomainDelCgroupForThread(priv->cgroup,
  VIR_CGROUP_THREAD_VCPU, vcpu) < 0)
-- 
2.6.2

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

[libvirt] [PATCH v2 00/21] vcpu info storage refactors - part 2

[Peter Krempa]

Some of patches were pushed. The rest fixes most of the feedback that made
sense that was against v1.

Peter Krempa (21):
  cgroup: Clean up virCgroupGetPercpuStats
  conf: Add helper to retrieve bitmap of active vcpus for a definition
  cgroup: Prepare for sparse vCPU topologies in virCgroupGetPercpuStats
  qemu: cpu hotplug: Set vcpu state directly in the new structure
  qemu: Move and rename qemuProcessDetectVcpuPIDs to
qemuDomainDetectVcpuPids
  qemu: domain: Prepare qemuDomainDetectVcpuPids for reuse
  qemu: Reuse qemuDomainDetectVcpuPids in cpu hot(un)plug
  conf: Don't copy def->cpumask into cpu pinning info
  conf: Split out logic to determine whether cpupin was provided
  conf: Store cpu pinning data in def->vcpus
  conf: remove unused cpu pinning helpers and data structures
  conf: Extract code that formats 
  util: bitmap: Introduce bitmap subtraction
  conf: Add helper to return a bitmap of active iothread ids
  conf: Extract code for parsing thread resource scheduler info
  conf: Don't store vcpusched orthogonally to other vcpu info
  conf: Fix how iothread scheduler info is stored
  qemu: vcpu: Aggregate code to set vCPU tuning
  qemu: vcpu: Reuse qemuProcessSetupVcpu in vcpu hotplug
  qemu: iothread: Aggregate code to set IOTrhead tuning
  qemu: iothread: Reuse qemuProcessSetupIOThread in iothread hotplug

 src/conf/domain_conf.c | 940 +++--
 src/conf/domain_conf.h |  45 +-
 src/libvirt_private.syms   |  10 +-
 src/libxl/libxl_domain.c   |  20 +-
 src/libxl/libxl_driver.c   |  41 +-
 src/lxc/lxc_driver.c   |   2 +-
 src/qemu/qemu_cgroup.c | 195 -
 src/qemu/qemu_cgroup.h |   2 -
 src/qemu/qemu_domain.c |  84 ++
 src/qemu/qemu_domain.h |   2 +
 src/qemu/qemu_driver.c | 405 +++--
 src/qemu/qemu_process.c| 478 ++-
 src/qemu/qemu_process.h|   6 +
 src/test/test_driver.c |  45 +-
 src/util/virbitmap.c   |  21 +
 src/util/virbitmap.h   |   3 +
 src/util/vircgroup.c   |  74 +-
 src/util/vircgroup.h   |   3 +-
 src/vz/vz_sdk.c|   4 +-
 .../qemuxml2xmlout-cputune-iothreadsched.xml   |   3 +-
 tests/virbitmaptest.c  |  55 ++
 tests/vircgrouptest.c  |   2 +-
 22 files changed, 1148 insertions(+), 1292 deletions(-)

-- 
2.6.2

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

[libvirt] [PATCH v2 12/21] conf: Extract code that formats

[Peter Krempa]

virDomainDefFormatInternal is growing rather large. Extract the cputune
formatter into a separate function.
---
 src/conf/domain_conf.c | 230 +++--
 1 file changed, 125 insertions(+), 105 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e289b6f..0c28c03 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -21414,6 +21414,129 @@ virDomainDefHasCapabilitiesFeatures(virDomainDefPtr 
def)
 return false;
 }

+
+static int
+virDomainCputuneDefFormat(virBufferPtr buf,
+  virDomainDefPtr def)
+{
+size_t i;
+virBuffer childrenBuf = VIR_BUFFER_INITIALIZER;
+int ret = -1;
+
+virBufferAdjustIndent(, virBufferGetIndent(buf, false) + 2);
+
+if (def->cputune.sharesSpecified)
+virBufferAsprintf(, "%lu\n",
+  def->cputune.shares);
+if (def->cputune.period)
+virBufferAsprintf(, "%llu\n",
+  def->cputune.period);
+if (def->cputune.quota)
+virBufferAsprintf(, "%lld\n",
+  def->cputune.quota);
+
+if (def->cputune.emulator_period)
+virBufferAsprintf(, "%llu"
+  "\n",
+  def->cputune.emulator_period);
+
+if (def->cputune.emulator_quota)
+virBufferAsprintf(, "%lld"
+  "\n",
+  def->cputune.emulator_quota);
+
+for (i = 0; i < def->maxvcpus; i++) {
+char *cpumask;
+virDomainVcpuInfoPtr vcpu = def->vcpus + i;
+
+if (!vcpu->cpumask)
+continue;
+
+if (!(cpumask = virBitmapFormat(vcpu->cpumask)))
+goto cleanup;
+
+virBufferAsprintf(,
+  "\n", i, cpumask);
+
+VIR_FREE(cpumask);
+}
+
+if (def->cputune.emulatorpin) {
+char *cpumask;
+virBufferAddLit(, "cputune.emulatorpin)))
+goto cleanup;
+
+virBufferAsprintf(, "cpuset='%s'/>\n", cpumask);
+VIR_FREE(cpumask);
+}
+
+for (i = 0; i < def->niothreadids; i++) {
+char *cpumask;
+
+/* Ignore iothreadids with no cpumask */
+if (!def->iothreadids[i]->cpumask)
+continue;
+
+virBufferAsprintf(, "iothreadids[i]->iothread_id);
+
+if (!(cpumask = virBitmapFormat(def->iothreadids[i]->cpumask)))
+goto cleanup;
+
+virBufferAsprintf(, "cpuset='%s'/>\n", cpumask);
+VIR_FREE(cpumask);
+}
+
+for (i = 0; i < def->cputune.nvcpusched; i++) {
+virDomainThreadSchedParamPtr sp = >cputune.vcpusched[i];
+char *ids = NULL;
+
+if (!(ids = virBitmapFormat(sp->ids)))
+goto cleanup;
+
+virBufferAsprintf(, "policy));
+VIR_FREE(ids);
+
+if (sp->policy == VIR_PROC_POLICY_FIFO ||
+sp->policy == VIR_PROC_POLICY_RR)
+virBufferAsprintf(, " priority='%d'", sp->priority);
+virBufferAddLit(, "/>\n");
+}
+
+for (i = 0; i < def->cputune.niothreadsched; i++) {
+virDomainThreadSchedParamPtr sp = >cputune.iothreadsched[i];
+char *ids = NULL;
+
+if (!(ids = virBitmapFormat(sp->ids)))
+goto cleanup;
+
+virBufferAsprintf(, "policy));
+VIR_FREE(ids);
+
+if (sp->policy == VIR_PROC_POLICY_FIFO ||
+sp->policy == VIR_PROC_POLICY_RR)
+virBufferAsprintf(, " priority='%d'", sp->priority);
+virBufferAddLit(, "/>\n");
+}
+
+if (virBufferUse()) {
+virBufferAddLit(buf, "\n");
+virBufferAddBuffer(buf, );
+virBufferAddLit(buf, "\n");
+}
+
+ret = 0;
+
+ cleanup:
+virBufferFreeAndReset();
+return ret;
+}
+
+
 /* This internal version appends to an existing buffer
  * (possibly with auto-indent), rather than flattening
  * to string.
@@ -21624,111 +21747,8 @@ virDomainDefFormatInternal(virDomainDefPtr def,
 }
 }

-/* start format cputune */
-indent = virBufferGetIndent(buf, false);
-virBufferAdjustIndent(, indent + 2);
-if (def->cputune.sharesSpecified)
-virBufferAsprintf(, "%lu\n",
-  def->cputune.shares);
-if (def->cputune.period)
-virBufferAsprintf(, "%llu\n",
-  def->cputune.period);
-if (def->cputune.quota)
-virBufferAsprintf(, "%lld\n",
-  def->cputune.quota);
-
-if (def->cputune.emulator_period)
-virBufferAsprintf(, "%llu"
-  "\n",
-  def->cputune.emulator_period);
-
-if (def->cputune.emulator_quota)
-virBufferAsprintf(, "%lld"
-  "\n",
-  def->cputune.emulator_quota);
-
-for (i = 0; i < def->maxvcpus; i++) {
-char *cpumask;
-virDomainVcpuInfoPtr vcpu = def->vcpus + i;
-
-if (!vcpu->cpumask)
-continue;
-
-if (!(cpumask 

[libvirt] [PATCH v2 03/21] cgroup: Prepare for sparse vCPU topologies in virCgroupGetPercpuStats

[Peter Krempa]

Pass a bitmap of enabled guest vCPUs to virCgroupGetPercpuStats so that
un-continuous vCPU topologies can be used.
---
 src/lxc/lxc_driver.c   |  2 +-
 src/qemu/qemu_driver.c |  7 ++-
 src/util/vircgroup.c   | 16 
 src/util/vircgroup.h   |  3 ++-
 tests/vircgrouptest.c  |  2 +-
 5 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 67088c8..f6fe207 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -5702,7 +5702,7 @@ lxcDomainGetCPUStats(virDomainPtr dom,
   params, nparams);
 else
 ret = virCgroupGetPercpuStats(priv->cgroup, params,
-  nparams, start_cpu, ncpus, 0);
+  nparams, start_cpu, ncpus, NULL);
  cleanup:
 virObjectUnlock(vm);
 return ret;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index abcdbe6..0fa7d13 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18173,6 +18173,7 @@ qemuDomainGetCPUStats(virDomainPtr domain,
 virDomainObjPtr vm = NULL;
 int ret = -1;
 qemuDomainObjPrivatePtr priv;
+virBitmapPtr guestvcpus = NULL;

 virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1);

@@ -18196,13 +18197,17 @@ qemuDomainGetCPUStats(virDomainPtr domain,
 goto cleanup;
 }

+if (!(guestvcpus = virDomainDefGetOnlineVcpumap(vm->def)))
+goto cleanup;
+
 if (start_cpu == -1)
 ret = virCgroupGetDomainTotalCpuStats(priv->cgroup,
   params, nparams);
 else
 ret = virCgroupGetPercpuStats(priv->cgroup, params, nparams,
-  start_cpu, ncpus, priv->nvcpupids);
+  start_cpu, ncpus, guestvcpus);
  cleanup:
+virBitmapFree(guestvcpus);
 virDomainObjEndAPI();
 return ret;
 }
diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
index da0df7a..37706c0 100644
--- a/src/util/vircgroup.c
+++ b/src/util/vircgroup.c
@@ -3143,17 +3143,17 @@ virCgroupDenyDevicePath(virCgroupPtr group, const char 
*path, int perms)
  */
 static int
 virCgroupGetPercpuVcpuSum(virCgroupPtr group,
-  unsigned int nvcpupids,
+  virBitmapPtr guestvcpus,
   unsigned long long *sum_cpu_time,
   size_t nsum,
   virBitmapPtr cpumap)
 {
 int ret = -1;
-size_t i;
+ssize_t i = -1;
 char *buf = NULL;
 virCgroupPtr group_vcpu = NULL;

-for (i = 0; i < nvcpupids; i++) {
+while ((i = virBitmapNextSetBit(guestvcpus, i)) >= 0) {
 char *pos;
 unsigned long long tmp;
 ssize_t j;
@@ -3215,7 +3215,7 @@ virCgroupGetPercpuStats(virCgroupPtr group,
 unsigned int nparams,
 int start_cpu,
 unsigned int ncpus,
-unsigned int nvcpupids)
+virBitmapPtr guestvcpus)
 {
 int ret = -1;
 size_t i;
@@ -3230,7 +3230,7 @@ virCgroupGetPercpuStats(virCgroupPtr group,

 /* return the number of supported params */
 if (nparams == 0 && ncpus != 0) {
-if (nvcpupids == 0)
+if (!guestvcpus)
 return CGROUP_NB_PER_CPU_STAT_PARAM;
 else
 return CGROUP_NB_PER_CPU_STAT_PARAM + 1;
@@ -3285,11 +3285,11 @@ virCgroupGetPercpuStats(virCgroupPtr group,
 /* return percpu vcputime in index 1 */
 param_idx = 1;

-if (nvcpupids > 0 && param_idx < nparams) {
+if (guestvcpus && param_idx < nparams) {
 if (VIR_ALLOC_N(sum_cpu_time, need_cpus) < 0)
 goto cleanup;
-if (virCgroupGetPercpuVcpuSum(group, nvcpupids, sum_cpu_time, 
need_cpus,
-  cpumap) < 0)
+if (virCgroupGetPercpuVcpuSum(group, guestvcpus, sum_cpu_time,
+  need_cpus, cpumap) < 0)
 goto cleanup;

 for (i = start_cpu; i < need_cpus; i++) {
diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h
index d754b1f..4a87b39 100644
--- a/src/util/vircgroup.h
+++ b/src/util/vircgroup.h
@@ -26,6 +26,7 @@
 # define __VIR_CGROUP_H__

 # include "virutil.h"
+# include "virbitmap.h"

 struct virCgroup;
 typedef struct virCgroup *virCgroupPtr;
@@ -246,7 +247,7 @@ virCgroupGetPercpuStats(virCgroupPtr group,
 unsigned int nparams,
 int start_cpu,
 unsigned int ncpus,
-unsigned int nvcpupids);
+virBitmapPtr guestvcpus);

 int
 virCgroupGetDomainTotalCpuStats(virCgroupPtr group,
diff --git a/tests/vircgrouptest.c b/tests/vircgrouptest.c
index 7ea6e13..322f6cb 100644
--- a/tests/vircgrouptest.c
+++ b/tests/vircgrouptest.c
@@ -656,7 +656,7 @@ static int testCgroupGetPercpuStats(const void *args 
ATTRIBUTE_UNUSED)

   

Re: [libvirt] [PATCH 4/4] vz: fix race condition when adding domain to domains list

[Mikhail Feoktistov]

29.01.2016 11:06, Nikolay Shirokovskiy пишет:


On 28.01.2016 18:38, Mikhail Feoktistov wrote:

Race condition:
User calls defineXML to create new instance.
The main thread from vzDomainDefineXMLFlags() creates new instance by 
prlsdkCreateVm.
Then this thread calls prlsdkAddDomain to add new domain to domains list.
The second thread receives notification from hypervisor that new VM was created.
It calls prlsdkHandleVmAddedEvent() and also tries to add new domain to domains 
list.
These two threads call virDomainObjListFindByUUID() from prlsdkAddDomain() and 
don't find new domain.
So they add two domains with the same uuid to domains list.

This fix splits logic of prlsdkAddDomain() into two functions.
1. prlsdkNewDomain() creates new empty domain in domains list with the specific 
uuid.
2. prlsdkLoadDomain() add data from VM to domain object.

New algorithm for creating an instance:
In vzDomainDefineXMLFlags() we add new domain to domain list by calling 
prlsdkNewDomain()
and only after that we call CreateVm() to create VM.
It means that we "reserve" domain object with the specific uuid.
After creation of new VM we add info from this VM
to reserved domain object by calling prlsdkLoadDomain().

Before this patch prlsdkLoadDomain() worked in 2 different cases:
1. It creates and initializes new domain. Then updates it from sdk handle.
2. It updates existed domain from sdk handle.
In this patch we remove code which creates new domain from LoadDomain()
and move it to prlsdkNewDomain().
Now prlsdkLoadDomain() only updates domain from skd handle.

In notification handler prlsdkHandleVmAddedEvent() we check
the existence of a domain and if it doesn't exist we add new domain by calling
prlsdkNewDomain() and load info from sdk handle via prlsdkLoadDomain().
---
  src/vz/vz_driver.c |  13 ++-
  src/vz/vz_sdk.c| 253 +++--
  src/vz/vz_sdk.h|   9 +-
  3 files changed, 146 insertions(+), 129 deletions(-)

diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index 91a48b6..521efd4 100644
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -685,6 +685,7 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, 
unsigned int flags)
  virDomainPtr retdom = NULL;
  virDomainDefPtr def;
  virDomainObjPtr olddom = NULL;
+virDomainObjPtr newdom = NULL;
  unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
  
  virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL);

@@ -700,6 +701,9 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, 
unsigned int flags)
  olddom = virDomainObjListFindByUUID(privconn->domains, def->uuid);
  if (olddom == NULL) {
  virResetLastError();
+newdom = prlsdkNewDomain(privconn, def->name, def->uuid);
+if (!newdom)
+goto cleanup;
  if (def->os.type == VIR_DOMAIN_OSTYPE_HVM) {
  if (prlsdkCreateVm(conn, def))
  goto cleanup;
@@ -713,8 +717,7 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char *xml, 
unsigned int flags)
  goto cleanup;
  }
  
-olddom = prlsdkAddDomain(privconn, def->uuid);

-if (!olddom)
+if (prlsdkLoadDomain(privconn, newdom))
  goto cleanup;
  } else {
  int state, reason;
@@ -755,6 +758,12 @@ vzDomainDefineXMLFlags(virConnectPtr conn, const char 
*xml, unsigned int flags)
   cleanup:
  if (olddom)
  virObjectUnlock(olddom);
+if (newdom) {
+if (!retdom)
+ virDomainObjListRemove(privconn->domains, newdom);
+else
+ virObjectUnlock(newdom);
+}
  virDomainDefFree(def);
  vzDriverUnlock(privconn);
  return retdom;
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 6fb2a97..9d2bdab 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -1236,58 +1236,35 @@ prlsdkConvertCpuMode(PRL_HANDLE sdkdom, virDomainDefPtr 
def)
  return -1;
  }
  
-/*

- * This function retrieves information about domain.
- * If the domains is already in the domains list
- * privconn->domains, then locked 'olddom' must be
- * provided. If the domains must be added to the list,
- * olddom must be NULL.
- *
- * The function return a pointer to a locked virDomainObj.
- */
-static virDomainObjPtr
-prlsdkLoadDomain(vzConnPtr privconn,
- PRL_HANDLE sdkdom,
- virDomainObjPtr olddom)
+int
+prlsdkLoadDomain(vzConnPtr privconn, virDomainObjPtr dom)
  {
-virDomainObjPtr dom = NULL;
  virDomainDefPtr def = NULL;
-vzDomObjPtr pdom = NULL;
  VIRTUAL_MACHINE_STATE domainState;
+char *home = NULL;
  
  PRL_UINT32 buflen = 0;

  PRL_RESULT pret;
  PRL_UINT32 ram;
  PRL_UINT32 envId;
  PRL_VM_AUTOSTART_OPTION autostart;
+PRL_HANDLE sdkdom = PRL_INVALID_HANDLE;
+vzDomObjPtr pdom = dom->privateData;
  
  virCheckNonNullArgGoto(privconn, error);

-virCheckNonNullArgGoto(sdkdom, error);
+virCheckNonNullArgGoto(dom, error);
+
+ 

Re: [libvirt] [PATCH] migration: add option to set target ndb server port

[Maxim Nestratov]

13.01.2016 14:01, Nikolay Shirokovskiy пишет:

Current libvirt + qemu pair lacks secure migrations in case of
VMs with non-shared disks. The only option to migrate securely
natively is to use tunneled mode and some kind of secure
destination URI. But tunelled mode does not support non-shared
disks.

The other way to make migration secure is to organize a tunnel
by external means. This is possible in case of shared disks
migration thru use of proper combination of destination URI,
migration URI and VIR_MIGRATE_PARAM_LISTEN_ADDRESS migration
param. But again this is not possible in case of non shared disks
migration as we have no option to control target nbd server port.
But fixing this much more simplier that supporting non-shared
disks in tunneled mode.

So this patch adds option to set target ndb port.

Finally all qemu migration connections will be secured AFAIK but
even in this case this patch could be convinient if one wants
all migration traffic be put in a single connection.
---
  include/libvirt/libvirt-domain.h |  10 +++
  src/qemu/qemu_driver.c   |  25 ---
  src/qemu/qemu_migration.c| 138 +++
  src/qemu/qemu_migration.h|   3 +
  tools/virsh-domain.c |  12 
  tools/virsh.pod  |   5 +-
  6 files changed, 141 insertions(+), 52 deletions(-)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index d26faa5..e577f26 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -757,6 +757,16 @@ typedef enum {
   */
  # define VIR_MIGRATE_PARAM_MIGRATE_DISKS"migrate_disks"
  
+/**

+ * VIR_MIGRATE_PARAM_NBD_PORT:
+ *
+ * virDomainMigrate* params field: port that destination nbd server should use
+ * for incoming disks migration. Type is VIR_TYPED_PARAM_INT. If set to 0 or
+ * omitted, libvirt will choose a suitable default. At the moment this is only
+ * supported by the QEMU driver.
+ */
+# define VIR_MIGRATE_PARAM_NBD_PORT"nbd_port"
+
  /* Domain migration. */
  virDomainPtr virDomainMigrate (virDomainPtr domain, virConnectPtr dconn,
 unsigned long flags, const char *dname,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8ccf68b..4d00ba4 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -12082,7 +12082,7 @@ qemuDomainMigratePrepare2(virConnectPtr dconn,
  ret = qemuMigrationPrepareDirect(driver, dconn,
   NULL, 0, NULL, NULL, /* No cookies */
   uri_in, uri_out,
- , origname, NULL, 0, NULL, flags);
+ , origname, NULL, 0, NULL, 0, flags);
  
   cleanup:

  VIR_FREE(origname);
@@ -12135,7 +12135,7 @@ qemuDomainMigratePerform(virDomainPtr dom,
   * Consume any cookie we were able to decode though
   */
  ret = qemuMigrationPerform(driver, dom->conn, vm,
-   NULL, dconnuri, uri, NULL, NULL, 0, NULL,
+   NULL, dconnuri, uri, NULL, NULL, 0, NULL, 0,
 cookie, cookielen,
 NULL, NULL, /* No output cookies in v2 */
 flags, dname, resource, false);
@@ -12308,7 +12308,7 @@ qemuDomainMigratePrepare3(virConnectPtr dconn,
   cookiein, cookieinlen,
   cookieout, cookieoutlen,
   uri_in, uri_out,
- , origname, NULL, 0, NULL, flags);
+ , origname, NULL, 0, NULL, 0, flags);
  
   cleanup:

  VIR_FREE(origname);
@@ -12334,6 +12334,7 @@ qemuDomainMigratePrepare3Params(virConnectPtr dconn,
  const char *dname = NULL;
  const char *uri_in = NULL;
  const char *listenAddress = cfg->migrationAddress;
+int nbdPort = 0;
  int nmigrate_disks;
  const char **migrate_disks = NULL;
  char *origname = NULL;
@@ -12354,7 +12355,10 @@ qemuDomainMigratePrepare3Params(virConnectPtr dconn,
  _in) < 0 ||
  virTypedParamsGetString(params, nparams,
  VIR_MIGRATE_PARAM_LISTEN_ADDRESS,
-) < 0)
+) < 0 ||
+virTypedParamsGetInt(params, nparams,
+VIR_MIGRATE_PARAM_NBD_PORT,
+) < 0)
  goto cleanup;
  
  nmigrate_disks = virTypedParamsGetStringList(params, nparams,

@@ -12385,7 +12389,8 @@ qemuDomainMigratePrepare3Params(virConnectPtr dconn,
   cookieout, cookieoutlen,
   uri_in, uri_out,
   , origname, listenAddress,
- nmigrate_disks, 

Re: [libvirt] [PATCH v2 1/6] logical: Create helper virStorageBackendLogicalParseVolExtents

[Pavel Hrdina]

On Thu, Jan 28, 2016 at 05:44:04PM -0500, John Ferlan wrote:
> Create a helper routine in order to parse any extents information
> including the extent size, length, and the device string contained
> within the generated 'lvs' output string.
> 
> A future patch would then be able to avoid the code more cleanly
> 
> Signed-off-by: John Ferlan 
> ---
>  src/storage/storage_backend_logical.c | 208 
> ++
>  1 file changed, 113 insertions(+), 95 deletions(-)

ACK

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

[libvirt] [PATCH v2 15/21] conf: Extract code for parsing thread resource scheduler info

[Peter Krempa]

As the scheduler info elements are represented orthogonally to how it
makes sense to actually store the information, the extracted code will
be later used when converting between XML and internal definitions.
---

Notes:
v2: tweaked spelling in error message

 src/conf/domain_conf.c | 69 --
 1 file changed, 45 insertions(+), 24 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 556cd71..1ee9041 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -14522,36 +14522,34 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
 return ret;
 }

-static int
-virDomainThreadSchedParse(xmlNodePtr node,
-  unsigned int minid,
-  unsigned int maxid,
-  const char *name,
-  virDomainThreadSchedParamPtr sp)
+
+static virBitmapPtr
+virDomainSchedulerParse(xmlNodePtr node,
+const char *name,
+virProcessSchedPolicy *policy,
+int *priority)
 {
+virBitmapPtr ret = NULL;
 char *tmp = NULL;
 int pol = 0;

-tmp = virXMLPropString(node, name);
-if (!tmp) {
+if (!(tmp = virXMLPropString(node, name))) {
 virReportError(VIR_ERR_XML_ERROR,
_("Missing attribute '%s' in element '%sched'"),
name, name);
 goto error;
 }

-if (virBitmapParse(tmp, 0, >ids, VIR_DOMAIN_CPUMASK_LEN) < 0)
+if (virBitmapParse(tmp, 0, , VIR_DOMAIN_CPUMASK_LEN) < 0)
 goto error;

-if (virBitmapIsAllClear(sp->ids) ||
-virBitmapNextSetBit(sp->ids, -1) < minid ||
-virBitmapLastSetBit(sp->ids) > maxid) {
-
+if (virBitmapIsAllClear(ret)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("Invalid value of '%s': %s"),
+   _("'%s' scheduler bitmap '%s' is empty"),
name, tmp);
 goto error;
 }
+
 VIR_FREE(tmp);

 if (!(tmp = virXMLPropString(node, "scheduler"))) {
@@ -14562,22 +14560,22 @@ virDomainThreadSchedParse(xmlNodePtr node,

 if ((pol = virProcessSchedPolicyTypeFromString(tmp)) <= 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("Invalid scheduler attribute: '%s'"),
-   tmp);
+   _("Invalid scheduler attribute: '%s'"), tmp);
 goto error;
 }
-sp->policy = pol;
+*policy = pol;

 VIR_FREE(tmp);
-if (sp->policy == VIR_PROC_POLICY_FIFO ||
-sp->policy == VIR_PROC_POLICY_RR) {
-tmp = virXMLPropString(node, "priority");
-if (!tmp) {
+
+if (pol == VIR_PROC_POLICY_FIFO ||
+pol == VIR_PROC_POLICY_RR) {
+if (!(tmp = virXMLPropString(node, "priority"))) {
 virReportError(VIR_ERR_XML_ERROR, "%s",
_("Missing scheduler priority"));
 goto error;
 }
-if (virStrToLong_i(tmp, NULL, 10, >priority) < 0) {
+
+if (virStrToLong_i(tmp, NULL, 10, priority) < 0) {
 virReportError(VIR_ERR_XML_ERROR, "%s",
_("Invalid value for element priority"));
 goto error;
@@ -14585,11 +14583,34 @@ virDomainThreadSchedParse(xmlNodePtr node,
 VIR_FREE(tmp);
 }

-return 0;
+return ret;

  error:
 VIR_FREE(tmp);
-return -1;
+virBitmapFree(ret);
+return NULL;
+}
+
+
+static int
+virDomainThreadSchedParse(xmlNodePtr node,
+  unsigned int minid,
+  unsigned int maxid,
+  const char *name,
+  virDomainThreadSchedParamPtr sp)
+{
+if (!(sp->ids = virDomainSchedulerParse(node, name, >policy,
+>priority)))
+return -1;
+
+if (virBitmapNextSetBit(sp->ids, -1) < minid ||
+virBitmapLastSetBit(sp->ids) > maxid) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("%sched bitmap is out of range"), name);
+return -1;
+}
+
+return 0;
 }


-- 
2.6.2

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

[libvirt] [PATCH] Simplify virDomainParseMemory

[Ján Tomko]

Do not store the return value of virDomainParseScaledValue,
it was overwritten anyway.

Delete the cleanup label, there is nothing to clean up.
---
 src/conf/domain_conf.c | 16 +---
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1ea74a6..f663969 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7547,28 +7547,22 @@ virDomainParseMemory(const char *xpath,
  bool required,
  bool capped)
 {
-int ret = -1;
 unsigned long long bytes, max;
 
 max = virMemoryMaxValue(capped);
 
-ret = virDomainParseScaledValue(xpath, units_xpath, ctxt,
-, 1024, max, required);
-if (ret < 0)
-goto cleanup;
+if (virDomainParseScaledValue(xpath, units_xpath, ctxt,
+  , 1024, max, required) < 0)
+return -1;
 
 /* Yes, we really do use kibibytes for our internal sizing.  */
 *mem = VIR_DIV_UP(bytes, 1024);
 
 if (*mem >= VIR_DIV_UP(max, 1024)) {
 virReportError(VIR_ERR_OVERFLOW, "%s", _("size value too large"));
-ret = -1;
-goto cleanup;
+return -1;
 }
-
-ret = 0;
- cleanup:
-return ret;
+return 0;
 }
 
 
-- 
2.4.10

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

Re: [libvirt] [PATCH] Simplify virDomainParseMemory

[Andrea Bolognani]

On Fri, 2016-01-29 at 18:12 +0100, Ján Tomko wrote:
> Do not store the return value of virDomainParseScaledValue,
> it was overwritten anyway.
> 
> Delete the cleanup label, there is nothing to clean up.
> ---
>  src/conf/domain_conf.c | 16 +---
>  1 file changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 1ea74a6..f663969 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -7547,28 +7547,22 @@ virDomainParseMemory(const char *xpath,
>   bool required,
>   bool capped)
>  {
> -int ret = -1;
>  unsigned long long bytes, max;
>  
>  max = virMemoryMaxValue(capped);
>  
> -ret = virDomainParseScaledValue(xpath, units_xpath, ctxt,
> -, 1024, max, required);
> -if (ret < 0)
> -goto cleanup;
> +if (virDomainParseScaledValue(xpath, units_xpath, ctxt,
> +  , 1024, max, required) < 0)
> +return -1;
>  
>  /* Yes, we really do use kibibytes for our internal sizing.  */
>  *mem = VIR_DIV_UP(bytes, 1024);
>  
>  if (*mem >= VIR_DIV_UP(max, 1024)) {
>  virReportError(VIR_ERR_OVERFLOW, "%s", _("size value too large"));
> -ret = -1;
> -goto cleanup;
> +return -1;
>  }
> -
> -ret = 0;
> - cleanup:
> -return ret;
> +return 0;
>  }

You might want to include the 'conf:' prefix in the summary.

ACK regardless :)

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

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

Re: [libvirt] [PATCH v2 2/6] logical: Fix parsing of the 'devices' field lvs output

[John Ferlan]

On 01/29/2016 11:07 AM, Pavel Hrdina wrote:
> On Thu, Jan 28, 2016 at 05:44:05PM -0500, John Ferlan wrote:
> 
> [...]
> 
>> Signed-off-by: John Ferlan 
>> ---
>>  src/storage/storage_backend_logical.c | 149 
>> --
>>  tests/virstringtest.c |  11 +++
>>  2 files changed, 62 insertions(+), 98 deletions(-)
>>
>> diff --git a/src/storage/storage_backend_logical.c 
>> b/src/storage/storage_backend_logical.c
>> index 7c05b6a..3232c08 100644
>> --- a/src/storage/storage_backend_logical.c
>> +++ b/src/storage/storage_backend_logical.c
>> @@ -64,8 +64,6 @@ virStorageBackendLogicalSetActive(virStoragePoolObjPtr 
>> pool,
>>  }
>>  
>>  
>> -#define VIR_STORAGE_VOL_LOGICAL_SEGTYPE_STRIPED "striped"
>> -
>>  struct virStorageBackendLogicalPoolVolData {
>>  virStoragePoolObjPtr pool;
>>  virStorageVolDefPtr vol;
>> @@ -75,121 +73,76 @@ static int
>>  virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol,
>>  char **const groups)
>>  {
>> -int nextents, ret = -1;
>> -const char *regex_unit = "(\\S+)\\((\\S+)\\)";
>> -char *regex = NULL;
>> -regex_t *reg = NULL;
>> -regmatch_t *vars = NULL;
>> -char *p = NULL;
>> -size_t i;
>> -int err, nvars;
>> +int ret = -1;
>> +char **extents = NULL;
>> +char *extnum, *end;
>> +size_t i, nextents = 0;
>>  unsigned long long offset, size, length;
>>  
>> -nextents = 1;
>> -if (STREQ(groups[4], VIR_STORAGE_VOL_LOGICAL_SEGTYPE_STRIPED)) {
>> -if (virStrToLong_i(groups[5], NULL, 10, ) < 0) {
>> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> -   _("malformed volume extent stripes value"));
>> -goto cleanup;
>> -}
>> -}
>> +/* The 'devices' (or extents) are split by a comma ",", so let's split
>> + * each out into a parseable string. Since our regex to generate this
>> + * data is "(\\S+)", we can safely assume at least one exists. */
>> +if (!(extents = virStringSplitCount(groups[3], ",", 0, )))
>> +goto cleanup;
> 
> At first I thought of the same solution but what if the device path contains
> a comma?  I know, it's very unlikely but it's possible.
> 

Well it would have failed with the current code... The existing
algorithm would concatenate 'nsegments' of 'regex_units' separated by a
comma [1].

I went back to the archives for the patches that Osier posted in Oct
2011 and Sep 2011. The reviews back then indicated concern over not
parsing the comma separated devices list... That is - the whole impetus
for making that change was that up to that point, this code used a comma
for a separator when generating the output. The initial change was to
just use "#", but then that change "grew" because it was pointed out
that the devices wouldn't be properly displayed in vol-dumpxml - or at
least separate devices would be listed.


>>  /* Allocate and fill in extents information */
>>  if (VIR_REALLOC_N(vol->source.extents,
>>vol->source.nextent + nextents) < 0)
>>  goto cleanup;
>> +vol->source.nextent += nextents;
> 
> I've also mentioned, that this could be done using VIR_APPEND_ELEMENT by
> dropping this pre-allocation and ... [1]
> 

According to cscope, usage is 50/50. "Many" of the uses have constructs
such as:

   size_t nnames;
   virStructNamePtr *names;

while the extents are defined as :

int nextent;
virStorageVolSourceExtentPtr extents;

>>  
>> -if (virStrToLong_ull(groups[6], NULL, 10, ) < 0) {
>> +if (virStrToLong_ull(groups[5], NULL, 10, ) < 0) {
>>  virReportError(VIR_ERR_INTERNAL_ERROR,
>> "%s", _("malformed volume extent length value"));
>>  goto cleanup;
>>  }
>>  
>> -if (virStrToLong_ull(groups[7], NULL, 10, ) < 0) {
>> +if (virStrToLong_ull(groups[6], NULL, 10, ) < 0) {
>>  virReportError(VIR_ERR_INTERNAL_ERROR,
>> "%s", _("malformed volume extent size value"));
>>  goto cleanup;
>>  }
>>  
>> -if (VIR_STRDUP(regex, regex_unit) < 0)
>> -goto cleanup;
>> -
>> -for (i = 1; i < nextents; i++) {
>> -if (VIR_REALLOC_N(regex, strlen(regex) + strlen(regex_unit) + 2) < 
>> 0)
>> -goto cleanup;
>> -/* "," is the separator of "devices" field */
>> -strcat(regex, ",");
>> -strncat(regex, regex_unit, strlen(regex_unit));

[1]

So for 2 'nextents' it's:

"(\\S+)\\((\\S+)\\),(\\S+)\\((\\S+)\\)"


>> -}
> 
> I would rather allocate the string with correct size outside of the cycle as 
> we
> know the length and just simply fill the string in a cycle.  The regex is more
> robust than splitting the string by comma.
> 

Not sure I follow the comment here, but I'll point out that part of the
problem with the existing algorithm is that it "assumes" extents=1 and
only allows adjustment if the 

Re: [libvirt] [PATCH 2/5] rbd: Add support for wiping RBD volumes

[John Ferlan]

On 01/27/2016 05:20 AM, Wido den Hollander wrote:
> When wiping the RBD image will be filled with zeros started
> at offset 0 and until the end of the volume.
> 
> This will result in the RBD volume growing to it's full allocation
> on the Ceph cluster. All data on the volume will be overwritten
> however, making it unavailable.
> 
> It does NOT take any RBD snapshots into account. The original data
> might still be in a snapshot of that RBD volume.
> 
> Signed-off-by: Wido den Hollander 
> ---
>  src/storage/storage_backend_rbd.c | 115 
> ++
>  1 file changed, 115 insertions(+)
> 
> diff --git a/src/storage/storage_backend_rbd.c 
> b/src/storage/storage_backend_rbd.c
> index 8c7a80d..7e669ff 100644
> --- a/src/storage/storage_backend_rbd.c
> +++ b/src/storage/storage_backend_rbd.c
> @@ -732,6 +732,120 @@ static int virStorageBackendRBDResizeVol(virConnectPtr 
> conn ATTRIBUTE_UNUSED,
>  return ret;
>  }
>  
> +static int
> +virStorageBackendRBDVolWipeZero(rbd_image_t image,
> +char *imgname,
> +rbd_image_info_t info,

^^
I changed this to a pointer (*info) in order to avoid Coverity whines
about pass_by_value... Had to obviously change the info. to be info->
and the call to pass by reference ().

> +uint64_t stripe_count)
> +{
> +int r = -1;
> +int ret = -1;
> +uint64_t offset = 0;
> +uint64_t length;
> +char *writebuf;
> +
> +if (VIR_ALLOC_N(writebuf, info.obj_size * stripe_count) < 0)
> +goto cleanup;
> +
> +while (offset < info.size) {
> +length = MIN((info.size - offset), (info.obj_size * stripe_count));
> +
> +if ((r = rbd_write(image, offset, length, writebuf)) < 0) {
> +virReportSystemError(-r, _("writing %zu bytes failed on "
> +   "RBD image %s at offset %zu"),
> +   length, imgname, offset);
> +goto cleanup;
> +}
> +
> +VIR_DEBUG("Wrote %zu bytes to RBD image %s at offset %zu",
> +  length, imgname, offset);
> +
> +offset += length;
> +}
> +
> +ret = 0;
> +
> + cleanup:
> +VIR_FREE(writebuf);
> +
> +return ret;
> +}
> +
> +static int
> +virStorageBackendRBDVolWipe(virConnectPtr conn,
> +virStoragePoolObjPtr pool,
> +virStorageVolDefPtr vol,
> +unsigned int algorithm,
> +unsigned int flags)
> +{
> +virStorageBackendRBDState ptr;
> +ptr.cluster = NULL;
> +ptr.ioctx = NULL;
> +rbd_image_t image = NULL;
> +rbd_image_info_t info;
> +uint64_t stripe_count;
> +int r = -1;
> +int ret = -1;
> +
> +virCheckFlags(VIR_STORAGE_VOL_WIPE_ALG_ZERO, -1);

Like 1/5... My bad...  I'll clean up...  It'll be (0, -1) since the
flags argument isn't defined. 

> +
> +VIR_DEBUG("Wiping RBD image %s/%s", pool->def->source.name, vol->name);
> +
> +if (virStorageBackendRBDOpenRADOSConn(, conn, >def->source) < 
> 0)
> +goto cleanup;
> +
> +if (virStorageBackendRBDOpenIoCTX(, pool) < 0)
> +goto cleanup;
> +
> +if ((r = rbd_open(ptr.ioctx, vol->name, , NULL)) < 0) {
> +virReportSystemError(-r, _("failed to open the RBD image %s"),
> + vol->name);
> +goto cleanup;
> +}
> +
> +if ((r = rbd_stat(image, , sizeof(info))) < 0) {
> +virReportSystemError(-r, _("failed to stat the RBD image %s"),
> + vol->name);
> +goto cleanup;
> +}
> +
> +if ((r = rbd_get_stripe_count(image, _count)) < 0) {
> +virReportSystemError(-r, _("failed to get stripe count of RBD image 
> %s"),
> + vol->name);
> +goto cleanup;
> +}
> +
> +VIR_DEBUG("Need to wipe %zu bytes from RBD image %s/%s",
> +  info.size, pool->def->source.name, vol->name);
> +
> +switch ((virStorageVolWipeAlgorithm) algorithm) {
> +case VIR_STORAGE_VOL_WIPE_ALG_ZERO:
> +r = virStorageBackendRBDVolWipeZero(image, vol->name,
> +info, stripe_count);
> +break;
> +default:

Similar to 1/5 this needs to be fully populated...  I'll clean up and push.

Tks -

John
> +virReportError(VIR_ERR_INVALID_ARG, _("unsupported algorithm 
> %d"),
> +   algorithm);
> +goto cleanup;
> +}
> +
> +if (r < 0) {
> +virReportSystemError(-r, _("failed to wipe RBD image %s"),
> + vol->name);
> +goto cleanup;
> +}
> +
> +ret = 0;
> +
> + cleanup:
> +if (image)
> +rbd_close(image);
> +
> +virStorageBackendRBDCloseRADOSConn();
> +
> +return ret;
> +}
> +
>  virStorageBackend virStorageBackendRBD = {
>  

Re: [libvirt] [PATCH 3/5] storage: Add TRIM algorithm to storage volume API

[John Ferlan]

On 01/27/2016 05:20 AM, Wido den Hollander wrote:
> This new algorithm adds support for wiping volumes using TRIM.
> 
> It does not overwrite all the data in a volume, but it tells the
> backing storage pool/driver that all bytes in a volume can be
> discarded.
> 
> It depends on the backing storage pool how this is handled.
> 
> A SCSI backend might send UNMAP commands to remove all data present
> on a LUN.
> 
> A Ceph backend might use rbd_discard() to instruct the Ceph cluster
> that all data on that RBD volume can be discarded.
> 
> Signed-off-by: Wido den Hollander 
> ---
>  include/libvirt/libvirt-storage.h | 3 +++
>  src/storage/storage_backend.c | 4 
>  tools/virsh-volume.c  | 2 +-
>  tools/virsh.pod   | 1 +
>  4 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/include/libvirt/libvirt-storage.h 
> b/include/libvirt/libvirt-storage.h
> index 2c55c93..232b4d3 100644
> --- a/include/libvirt/libvirt-storage.h
> +++ b/include/libvirt/libvirt-storage.h
> @@ -153,6 +153,9 @@ typedef enum {
>  
>  VIR_STORAGE_VOL_WIPE_ALG_RANDOM = 8, /* 1-pass random */
>  
> +VIR_STORAGE_VOL_WIPE_ALG_TRIM = 9, /* 1-pass, trim all data on the
> +  volume by using TRIM or DISCARD */
> +
>  # ifdef VIR_ENUM_SENTINELS
>  VIR_STORAGE_VOL_WIPE_ALG_LAST
>  /*
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index 15e9109..1bb44a7 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -2065,6 +2065,7 @@ virStorageBackendVolWipeLocal(virConnectPtr conn 
> ATTRIBUTE_UNUSED,
>VIR_STORAGE_VOL_WIPE_ALG_PFITZNER7 |
>VIR_STORAGE_VOL_WIPE_ALG_PFITZNER33 |
>VIR_STORAGE_VOL_WIPE_ALG_RANDOM |
> +  VIR_STORAGE_VOL_WIPE_ALG_TRIM |
>VIR_STORAGE_VOL_WIPE_ALG_LAST, -1);
>  
>  VIR_DEBUG("Wiping volume with path '%s' and algorithm %u",
> @@ -2112,6 +2113,9 @@ virStorageBackendVolWipeLocal(virConnectPtr conn 
> ATTRIBUTE_UNUSED,
>  case VIR_STORAGE_VOL_WIPE_ALG_RANDOM:
>  alg_char = "random";
>  break;
> +case VIR_STORAGE_VOL_WIPE_ALG_TRIM:
> +alg_char = "trim";
> +break;

This would be bad... Passing "trim" to the SCRUB image would cause a
failure...

I've replaced with:

case VIR_STORAGE_VOL_WIPE_ALG_TRIM:
virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
   _("'trim' algorithm not supported"));
goto cleanup;

Since at this point none of the drivers that utilize this function
supports the 'trim' option (in fact none do at this point).

>  default:
>  virReportError(VIR_ERR_INVALID_ARG,
> _("unsupported algorithm %d"),
> diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c
> index 661c876..35f0cbd 100644
> --- a/tools/virsh-volume.c
> +++ b/tools/virsh-volume.c
> @@ -906,7 +906,7 @@ static const vshCmdOptDef opts_vol_wipe[] = {
>  VIR_ENUM_DECL(virStorageVolWipeAlgorithm)
>  VIR_ENUM_IMPL(virStorageVolWipeAlgorithm, VIR_STORAGE_VOL_WIPE_ALG_LAST,
>"zero", "nnsa", "dod", "bsi", "gutmann", "schneier",
> -  "pfitzner7", "pfitzner33", "random");
> +  "pfitzner7", "pfitzner33", "random", "trim");
>  
>  static bool
>  cmdVolWipe(vshControl *ctl, const vshCmd *cmd)
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index e830c59..b259507 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -3546,6 +3546,7 @@ B
>pfitzner7  - Roy Pfitzner's 7-random-pass method: random x7.
>pfitzner33 - Roy Pfitzner's 33-random-pass method: random x33.
>random - 1-pass pattern: random.
> +  trim   - 1-pass trimming the volume using TRIM or DISCARD
>  
>  B: The availability of algorithms may be limited by the version
>  of the C binary installed on the host.
> 

I adjusted the  text to be:

B: The C binary will be used to handle the 'nnsa', 'dod',
'bsi', 'gutmann', 'schneier', 'pfitzner7' and 'pfitzner33' algorithms.
The availability of the algorithms may be limited by the version of
the C binary installed on the host. The 'zero' algorithm will
write zeroes to the entire volume. For some volumes, such as sparse
or rbd volumes, this may result in completely filling the volume with
zeroes making it appear to be completely full. As an alternative, the
'trim' algorithm does not overwrite all the data in a volume, rather
it expects the storage driver to be able to discard all bytes in a
volume. It is up to the storage driver to handle how the discarding
occurs. Not all storage drivers or volume types can support 'trim'.


Also, since I fixed the switch in 2/5 for storage_backend_rbd.c to not
use 'default:' - I added a temporary case for TRIM there (which gets
resolved in the next patch).

Tks - This has been pushed.

John

--
libvir-list mailing list
libvir-list@redhat.com

Re: [libvirt] [PATCH] rbd: Open in Read-Only mode when refreshing a volume

[John Ferlan]

On 01/28/2016 05:20 AM, Wido den Hollander wrote:
> By opening a RBD volume in Read-Only we do not register a
> watcher on the header object inside the Ceph cluster.
> 
> Refreshing a volume only calls rbd_stat() which is a operation
> which does not write to a RBD image.
> 
> This allows us to use a cephx user which has no write
> permissions if we would want to use the libvirt storage pool
> for informational purposes only.
> 
> It also saves us a write into the Ceph cluster which should
> speed up refreshing a RBD pool.
> 
> rbd_open_read_only() is available in all librbd versions which
> also support rbd_open().
> 
> Signed-off-by: Wido den Hollander 
> ---
>  src/storage/storage_backend_rbd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

ACK - and pushed

John

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

Re: [libvirt] [PATCH 1/5] storage: Properly check flags when wiping volumes

[John Ferlan]

On 01/27/2016 05:20 AM, Wido den Hollander wrote:
> Using virCheckFlags() we validate if the provided flags
> are support flags by this function.
> 
> The old code would also still run the command since it didn't
> go to cleanup when a invalid flag was supplied.
> 
> We now go to cleanup and exit if a invalid flag would be provided.
> 
> Signed-off-by: Wido den Hollander 
> ---
>  src/storage/storage_backend.c | 14 --
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index 03bc18c..15e9109 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -2056,7 +2056,16 @@ virStorageBackendVolWipeLocal(virConnectPtr conn 
> ATTRIBUTE_UNUSED,
>  struct stat st;
>  virCommandPtr cmd = NULL;
>  
> -virCheckFlags(0, -1);
> +virCheckFlags(VIR_STORAGE_VOL_WIPE_ALG_ZERO |
> +  VIR_STORAGE_VOL_WIPE_ALG_NNSA |
> +  VIR_STORAGE_VOL_WIPE_ALG_DOD |
> +  VIR_STORAGE_VOL_WIPE_ALG_BSI |
> +  VIR_STORAGE_VOL_WIPE_ALG_GUTMANN |
> +  VIR_STORAGE_VOL_WIPE_ALG_SCHNEIER |
> +  VIR_STORAGE_VOL_WIPE_ALG_PFITZNER7 |
> +  VIR_STORAGE_VOL_WIPE_ALG_PFITZNER33 |
> +  VIR_STORAGE_VOL_WIPE_ALG_RANDOM |
> +  VIR_STORAGE_VOL_WIPE_ALG_LAST, -1);

Looks like my brain had a misfire - these are bits for 'algorithm' and
not 'flags' .   I'll clean up, no need to resend.

>  
>  VIR_DEBUG("Wiping volume with path '%s' and algorithm %u",
>vol->target.path, algorithm);
> @@ -2078,7 +2087,7 @@ virStorageBackendVolWipeLocal(virConnectPtr conn 
> ATTRIBUTE_UNUSED,
>  
>  if (algorithm != VIR_STORAGE_VOL_WIPE_ALG_ZERO) {
>  const char *alg_char ATTRIBUTE_UNUSED = NULL;
> -switch (algorithm) {
> +switch ((virStorageVolWipeAlgorithm) algorithm) {

FYI: When you do this...  Then the "default:" changes to
VIR_STORAGE_VOL_WIPE_ALG_LAST and of course you'd need a
VIR_STORAGE_VOL_WIPE_ALG_ZERO case.

Because of that if construct I moved the switch outside the if, in order
to add case ZERO (otherwise it's a dead code path from coverity)

I cleaned it up already and pushed.

Tks -

John
>  case VIR_STORAGE_VOL_WIPE_ALG_NNSA:
>  alg_char = "nnsa";
>  break;
> @@ -2107,6 +2116,7 @@ virStorageBackendVolWipeLocal(virConnectPtr conn 
> ATTRIBUTE_UNUSED,
>  virReportError(VIR_ERR_INVALID_ARG,
> _("unsupported algorithm %d"),
> algorithm);
> +goto cleanup;
>  }
>  cmd = virCommandNew(SCRUB);
>  virCommandAddArgList(cmd, "-f", "-p", alg_char,
> 

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

Re: [libvirt] [PATCH 5/5] rbd: Implement buildVolFrom using RBD cloning

[John Ferlan]

On 01/27/2016 05:20 AM, Wido den Hollander wrote:
> RBD supports cloning by creating a snapshot, protecting it and create
> a child image based on that snapshot afterwards.
> 
> The RBD storage driver will try to find a snapshot with zero deltas between
> the current state of the original volume and the snapshot.
> 
> If such a snapshot is found a clone/child image will be created using
> the rbd_clone2() function from librbd.
> 
> rbd_clone2() is available in librbd since Ceph version Dumpling (0.67) which
> dates back to August 2013.
> 
> It will use the same features, strip size and stripe count as the parent 
> image.
> 
> This implementation will only create a single snapshot on the parent image if
> never changes. This reduces the amount of snapshots created for that RBD image
> which benefits the performance of the Ceph cluster.
> 
> During build the decision will be made to use either rbd_diff_iterate() or
> rbd_diff_iterate2().
> 
> The latter is faster, but only available on Ceph versions after 0.94 (Hammer).
> 
> Cloning is only supported if RBD format 2 is used. All images created by 
> libvirt
> are already format 2.
> 
> If a RBD format 1 image is used as the original volume the backend will report
> a VIR_ERR_OPERATION_UNSUPPORTED error.
> 
> Signed-off-by: Wido den Hollander 
> ---
>  src/storage/storage_backend_rbd.c | 341 
> ++
>  1 file changed, 341 insertions(+)
> 

ACK - pushed

John

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

Re: [libvirt] [PATCH 4/5] rbd: Add support for wiping RBD volumes using TRIM.

[John Ferlan]

On 01/27/2016 05:20 AM, Wido den Hollander wrote:
> Using VIR_STORAGE_VOL_WIPE_ALG_TRIM a RBD volume can be trimmed down
> to 0 bytes using rbd_dicard()

s/dicard/discard

> 
> Effectively all the data on the volume will be lost/gone, but the volume
> remains available for use afterwards.
> 
> Starting at offset 0 the storage pool will call rbd_discard() in stripe
> size * count increments which is usually 4MB. Stripe size being 4MB and
> count 1.
> 
> rbd_discard() is available since Ceph version Dumpling (0.67) which dates
> back to August 2013.
> 
> Signed-off-by: Wido den Hollander 
> ---
>  src/storage/storage_backend_rbd.c | 42 
> ++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/src/storage/storage_backend_rbd.c 
> b/src/storage/storage_backend_rbd.c
> index 7e669ff..8a95388 100644
> --- a/src/storage/storage_backend_rbd.c
> +++ b/src/storage/storage_backend_rbd.c
> @@ -772,6 +772,41 @@ virStorageBackendRBDVolWipeZero(rbd_image_t image,
>  }
>  
>  static int
> +virStorageBackendRBDVolWipeDiscard(rbd_image_t image,
> +   char *imgname,
> +   rbd_image_info_t info,

Like WipeZero, I adjusted to be *info to silence coverity. Other
adjustments for info-> and  were similarly made and pushed.

Tks -

John
> +   uint64_t stripe_count)
> +{
> +int r = -1;
> +int ret = -1;
> +uint64_t offset = 0;
> +uint64_t length;
> +
> +VIR_DEBUG("Wiping RBD %s volume using discard)", imgname);
> +
> +while (offset < info.size) {
> +length = MIN((info.size - offset), (info.obj_size * stripe_count));
> +
> +if ((r = rbd_discard(image, offset, length)) < 0) {
> +virReportSystemError(-r, _("discarding %zu bytes failed on "
> +   "RBD image %s at offset %zu"),
> + length, imgname, offset);
> +goto cleanup;
> +}
> +
> +VIR_DEBUG("Discarded %zu bytes of RBD image %s at offset %zu",
> +  length, imgname, offset);
> +
> +offset += length;
> +}
> +
> +ret = 0;
> +
> + cleanup:
> +return ret;
> +}
> +
> +static int
>  virStorageBackendRBDVolWipe(virConnectPtr conn,
>  virStoragePoolObjPtr pool,
>  virStorageVolDefPtr vol,
> @@ -787,7 +822,8 @@ virStorageBackendRBDVolWipe(virConnectPtr conn,
>  int r = -1;
>  int ret = -1;
>  
> -virCheckFlags(VIR_STORAGE_VOL_WIPE_ALG_ZERO, -1);
> +virCheckFlags(VIR_STORAGE_VOL_WIPE_ALG_ZERO |
> +  VIR_STORAGE_VOL_WIPE_ALG_TRIM, -1);
>  
>  VIR_DEBUG("Wiping RBD image %s/%s", pool->def->source.name, vol->name);
>  
> @@ -823,6 +859,10 @@ virStorageBackendRBDVolWipe(virConnectPtr conn,
>  r = virStorageBackendRBDVolWipeZero(image, vol->name,
>  info, stripe_count);
>  break;
> +case VIR_STORAGE_VOL_WIPE_ALG_TRIM:
> +r = virStorageBackendRBDVolWipeDiscard(image, vol->name,
> +   info, stripe_count);
> +break;
>  default:
>  virReportError(VIR_ERR_INVALID_ARG, _("unsupported algorithm 
> %d"),
> algorithm);
> 

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