Re: [libvirt] [PATCH v2 13/13] qemu: add support for sending QEMU stdout/stderr to virtlogd

[John Ferlan]

On 11/12/2015 12:19 PM, Daniel P. Berrange wrote:
> Currently the QEMU stdout/stderr streams are written directly to
> a regular file (eg /var/log/libvirt/qemu/$GUEST.log). While those
> can be rotated by logrotate (using copytruncate option) this is
> not very efficient. It also leaves open a window of opportunity
> for a compromised/broken QEMU to DOS the host filesystem by
> writing lots of text to stdout/stderr.
> 
> This makes it possible to connect the stdout/stderr file handles
> to a pipe that is provided by virtlogd. The virtlogd daemon will
> read from this pipe and write data to the log file, performing
> file rotation whenever a pre-determined size limit is reached.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  cfg.mk |   2 +-
>  src/qemu/libvirtd_qemu.aug |   1 +
>  src/qemu/qemu.conf |  15 
>  src/qemu/qemu_conf.c   |  18 +
>  src/qemu/qemu_conf.h   |   1 +
>  src/qemu/qemu_domain.c | 153 
> ++---
>  src/qemu/test_libvirtd_qemu.aug.in |   1 +
>  7 files changed, 145 insertions(+), 46 deletions(-)
> 

ACK

John

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

Re: [libvirt] [PATCH v2 12/13] qemu: convert monitor to use qemuDomainLogContextPtr indirectly

[John Ferlan]

On 11/12/2015 12:19 PM, Daniel P. Berrange wrote:
> Currently the QEMU monitor is given an FD to the logfile. This
> won't work in the future with virtlogd, so it needs to use the
> qemuDomainLogContextPtr instead, but it shouldn't directly
> access that object either. So define a callback that the
> monitor can use for reporting errors from the log file.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  src/qemu/qemu_domain.c| 12 --
>  src/qemu/qemu_domain.h|  2 -
>  src/qemu/qemu_migration.c |  2 +-
>  src/qemu/qemu_monitor.c   | 51 ++
>  src/qemu/qemu_monitor.h   |  8 +++-
>  src/qemu/qemu_process.c   | 93 
> ---
>  src/qemu/qemu_process.h   |  4 --
>  7 files changed, 84 insertions(+), 88 deletions(-)
> 

[...]

> index d3f0c09..bd5d9ca 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -94,9 +94,10 @@ struct _qemuMonitor {
>  char *balloonpath;
>  bool ballooninit;
>  
> -/* Log file fd of the qemu process to dig for usable info */
> -int logfd;
> -off_t logpos;
> +/* Log file context of the qemu process to dig for usable info */
> +qemuMonitorReportDomainLogError logFunc;
> +void *logOpaque;
> +virFreeCallback logDestroy;
>  };
>  
>  /**
> @@ -315,7 +316,6 @@ qemuMonitorDispose(void *obj)
>  VIR_FREE(mon->buffer);
>  virJSONValueFree(mon->options);
>  VIR_FREE(mon->balloonpath);
> -VIR_FORCE_CLOSE(mon->logfd);
>  }
>  
>  
> @@ -706,18 +706,17 @@ qemuMonitorIO(int watch, int fd, int events, void 
> *opaque)
>  }
>  
>  if (error || eof) {
> -if (hangup && mon->logfd != -1) {
> +if (hangup && mon->logFunc != NULL) {
>  /* Check if an error message from qemu is available and if so, 
> use
>   * it to overwrite the actual message. It's done only in early
>   * startup phases or during incoming migration when the message
>   * from qemu is certainly more interesting than a
>   * "connection reset by peer" message.
>   */
> -qemuProcessReportLogError(mon->logfd,
> -  mon->logpos,
> -  _("early end of file from monitor, "
> -"possible problem"));
> -VIR_FORCE_CLOSE(mon->logfd);
> +mon->logFunc(mon,
> + _("early end of file from monitor, "
> +   "possible problem"),
> + mon->logOpaque);

Mostly a consistency thing and not that it should happen since
qemuConnectMonitor checks for logCtxt before calling
qemuMonitorSetDomainLog; however, qemuMonitorClose and
qemuMonitorSetDomainLog check for mon->logOpaque before calling
logDestroy. Likewise, if logFunc is called with NULL (logOpaque) life
won't be good.

I don't think there's anything wrong with the current code - just caused
me to double check things.

>  virCopyLastError(&mon->lastError);
>  virResetLastError();
>  }
> @@ -802,7 +801,6 @@ qemuMonitorOpenInternal(virDomainObjPtr vm,
>  if (!(mon = virObjectLockableNew(qemuMonitorClass)))
>  return NULL;
>  
> -mon->logfd = -1;
>  if (virCondInit(&mon->notify) < 0) {
>  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("cannot initialize monitor condition"));
> @@ -932,6 +930,13 @@ qemuMonitorClose(qemuMonitorPtr mon)
>  VIR_FORCE_CLOSE(mon->fd);
>  }
>  
> +if (mon->logDestroy && mon->logOpaque) {
> +mon->logDestroy(mon->logOpaque);
> +mon->logOpaque = NULL;
> +mon->logDestroy = NULL;
> +mon->logFunc = NULL;
> +}
> +
>  /* In case another thread is waiting for its monitor command to be
>   * processed, we need to wake it up with appropriate error set.
>   */
> @@ -3646,21 +3651,21 @@ qemuMonitorGetDeviceAliases(qemuMonitorPtr mon,
>   * early startup errors of qemu.
>   *
>   * @mon: Monitor object to set the log file reading on
> - * @logfd: File descriptor of the already open log file
> - * @pos: position to read errors from
> + * @func: the callback to report errors
> + * @opaque: data to pass to @func

Missing @destroy

>   */
> -int
> -qemuMonitorSetDomainLog(qemuMonitorPtr mon, int logfd, off_t pos)
> +void
> +qemuMonitorSetDomainLog(qemuMonitorPtr mon,
> +qemuMonitorReportDomainLogError func,
> +void *opaque,
> +virFreeCallback destroy)

[...]

> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 132b3eb..4a2e2c1 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -1544,9 +1544,22 @@ static qemuMonitorCallbacks monitorCallbacks = {
>  .domainMigrationStatus = qemuProcessHandleMigrationStatus,
>  };
>  
> +static void
> +qemuProcessMonitorReportLogError(qemuMonitorPtr mon,
>

[libvirt] [PATCH] libvirt-domain: Fix typo in debug message

[Cole Robinson]

---
Pushed as trivial

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

diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index de7eb04..7cfbe58 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -2015,7 +2015,7 @@ virDomainSetMemoryStatsPeriod(virDomainPtr domain, int 
period,
 {
 virConnectPtr conn;
 
-VIR_DOMAIN_DEBUG(domain, "peroid=%d, flags=%x", period, flags);
+VIR_DOMAIN_DEBUG(domain, "period=%d, flags=%x", period, flags);
 
 virResetLastError();
 
-- 
2.5.0

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

Re: [libvirt] [PATCH v2 11/13] qemu: convert process stop/attach to use qemuDomainLogContextPtr

[John Ferlan]

On 11/12/2015 12:19 PM, Daniel P. Berrange wrote:
> When the qemuProcessAttach/Stop methods write a marker into
> the log file, they can use qemuDomainLogContextWrite to
> write a formatted message.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  src/qemu/qemu_process.c | 22 ++
>  1 file changed, 2 insertions(+), 20 deletions(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index f729065..132b3eb 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -77,9 +77,6 @@
>  
>  VIR_LOG_INIT("qemu.qemu_process");
>  
> -#define ATTACH_POSTFIX ": attaching\n"
> -#define SHUTDOWN_POSTFIX ": shutting down\n"
> -
>  /**
>   * qemuProcessRemoveDomainStatus
>   *
> @@ -4913,7 +4910,6 @@ void qemuProcessStop(virQEMUDriverPtr driver,
>  size_t i;
>  char *timestamp;
>  char *tmppath = NULL;
> -char ebuf[1024];
>  virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>  qemuDomainLogContextPtr logCtxt = NULL;
>  
> @@ -4946,15 +4942,8 @@ void qemuProcessStop(virQEMUDriverPtr driver,
>  
>  if ((logCtxt = qemuDomainLogContextNew(driver, vm,
> 
> QEMU_DOMAIN_LOG_CONTEXT_MODE_STOP))) {
> -int logfile = qemuDomainLogContextGetWriteFD(logCtxt);
>  if ((timestamp = virTimeStringNow()) != NULL) {
> -if (safewrite(logfile, timestamp, strlen(timestamp)) < 0 ||
> -safewrite(logfile, SHUTDOWN_POSTFIX,
> -  strlen(SHUTDOWN_POSTFIX)) < 0) {
> -VIR_WARN("Unable to write timestamp to logfile: %s",
> - virStrerror(errno, ebuf, sizeof(ebuf)));
> -}
> -
> +qemuDomainLogContextWrite(logCtxt, "%s: stopping\n", timestamp);

was #define SHUTDOWN_POSTFIX ": shutting down\n", but this changes to
"stopping" which is 'different', although perhaps more technically
correct w/r/t what's going on here.

I have to believe there's scripts out in the wild looking for 'shutting
down' that fail with 'stopping'... For some reason I seem to recall
tests scripts that would essentially 'tail -f' the log file for that to
ensure that whatever shutdown/stop command was sent actually worked.

ACK - either way, but I tend to favor shutdown down only because it has
precedence.

John

>  VIR_FREE(timestamp);
>  }
>  qemuDomainLogContextFree(logCtxt);
> @@ -5216,8 +5205,6 @@ int qemuProcessAttach(virConnectPtr conn 
> ATTRIBUTE_UNUSED,
>bool monJSON)
>  {
>  size_t i;
> -char ebuf[1024];
> -int logfile = -1;
>  qemuDomainLogContextPtr logCtxt = NULL;
>  char *timestamp;
>  qemuDomainObjPrivatePtr priv = vm->privateData;
> @@ -5316,7 +5303,6 @@ int qemuProcessAttach(virConnectPtr conn 
> ATTRIBUTE_UNUSED,
>  if (!(logCtxt = qemuDomainLogContextNew(driver, vm,
>  
> QEMU_DOMAIN_LOG_CONTEXT_MODE_ATTACH)))
>  goto error;
> -logfile = qemuDomainLogContextGetWriteFD(logCtxt);
>  
>  VIR_DEBUG("Determining emulator version");
>  virObjectUnref(priv->qemuCaps);
> @@ -5348,11 +5334,7 @@ int qemuProcessAttach(virConnectPtr conn 
> ATTRIBUTE_UNUSED,
>  if ((timestamp = virTimeStringNow()) == NULL)
>  goto error;
>  
> -if (safewrite(logfile, timestamp, strlen(timestamp)) < 0 ||
> -safewrite(logfile, ATTACH_POSTFIX, strlen(ATTACH_POSTFIX)) < 0) {
> -VIR_WARN("Unable to write timestamp to logfile: %s",
> - virStrerror(errno, ebuf, sizeof(ebuf)));
> -}
> +qemuDomainLogContextWrite(logCtxt, "%s: attaching\n", timestamp);
>  VIR_FREE(timestamp);
>  
>  qemuDomainObjTaint(driver, vm, VIR_DOMAIN_TAINT_EXTERNAL_LAUNCH, 
> logCtxt);
> 

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

Re: [libvirt] [PATCH v2 10/13] qemu: convert qemuLogOperation to take a qemuDomainLogContextPtr

[John Ferlan]

On 11/12/2015 12:19 PM, Daniel P. Berrange wrote:
> Instead of writing directly to a log file descriptor, change
> qemuLogOperation to use qemuDomainLogContextWrite().
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  src/qemu/qemu_process.c | 39 ---
>  1 file changed, 16 insertions(+), 23 deletions(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 85ad3a5..f729065 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -4064,40 +4064,33 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
>  static void
>  qemuLogOperation(virDomainObjPtr vm,
>   const char *msg,
> - int logfd,
> - virCommandPtr cmd)
> + virCommandPtr cmd,
> + qemuDomainLogContextPtr logCtxt)
>  {
>  char *timestamp;
> -char *logline;
>  qemuDomainObjPrivatePtr priv = vm->privateData;
>  int qemuVersion = virQEMUCapsGetVersion(priv->qemuCaps);
>  const char *package = virQEMUCapsGetPackage(priv->qemuCaps);
> -char ebuf[1024];
>  
>  if ((timestamp = virTimeStringNow()) == NULL)
> -goto error;
> -
> -if (virAsprintf(&logline, "%s: %s %s, qemu version: %d.%d.%d%s\n",
> -timestamp, msg, VIR_LOG_VERSION_STRING,
> -(qemuVersion / 100) % 1000, (qemuVersion / 1000) % 
> 1000, qemuVersion % 1000,
> -package ? package : "") < 0)
> -goto error;
> +goto cleanup;
>  
> -if (safewrite(logfd, logline, strlen(logline)) < 0)
> -goto error;
> +if (qemuDomainLogContextWrite(logCtxt, "%s: %s %s, qemu version: 
> %d.%d.%d%s\n",
> +  timestamp, msg, VIR_LOG_VERSION_STRING,
> +  (qemuVersion / 100) % 1000,
> +  (qemuVersion / 1000) % 1000,
> +  qemuVersion % 1000,
> +  package ? package : "") < 0)
> +goto cleanup;
>  
> -if (cmd)
> -virCommandWriteArgLog(cmd, logfd);
> +if (cmd) {
> +char *args = virCommandToString(cmd);
> +qemuDomainLogContextWrite(logCtxt, "%s", args);

Should this be "%s\n"?  Seems virCommandWriteArgLog will print one at
the end and I see callers to virCommandToString will add the '\n'
character...


ACK with the "expected result"...

John

> +VIR_FREE(args);
> +}
>  
>   cleanup:
>  VIR_FREE(timestamp);
> -VIR_FREE(logline);
> -return;
> -
> - error:
> -VIR_WARN("Unable to write banner to logfile: %s",
> - virStrerror(errno, ebuf, sizeof(ebuf)));
> -goto cleanup;
>  }
>  
>  int qemuProcessStart(virConnectPtr conn,
> @@ -4518,7 +4511,7 @@ int qemuProcessStart(virConnectPtr conn,
>  goto error;
>  }
>  
> -qemuLogOperation(vm, "starting up", logfile, cmd);
> +qemuLogOperation(vm, "starting up", cmd, logCtxt);
>  
>  qemuDomainObjCheckTaint(driver, vm, logCtxt);
>  
> 

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

Re: [libvirt] [PATCH v2 09/13] qemu: change qemuDomainTaint APIs to accept qemuDomainLogContextPtr

[John Ferlan]

On 11/12/2015 12:19 PM, Daniel P. Berrange wrote:
> The qemuDomainTaint APIs currently expect to be passed a log file
> descriptor. Change them to instead use a qemuDomainLogContextPtr
> to hide the implementation details.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  src/qemu/qemu_domain.c  | 96 
> ++---
>  src/qemu/qemu_domain.h  | 15 +++-
>  src/qemu/qemu_driver.c  | 10 +++---
>  src/qemu/qemu_process.c |  4 +--
>  4 files changed, 47 insertions(+), 78 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index f3bb8d4..75f78fe 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2107,9 +2107,10 @@ qemuDomainDefFormatLive(virQEMUDriverPtr driver,
>  void qemuDomainObjTaint(virQEMUDriverPtr driver,
>  virDomainObjPtr obj,
>  virDomainTaintFlags taint,
> -int logFD)
> +qemuDomainLogContextPtr logCtxt)
>  {
>  virErrorPtr orig_err = NULL;
> +bool closeLog = false;
>  
>  if (virDomainObjTaint(obj, taint)) {
>  char uuidstr[VIR_UUID_STRING_BUFLEN];
> @@ -2125,11 +2126,23 @@ void qemuDomainObjTaint(virQEMUDriverPtr driver,
>   * preserve original error, and clear any error that
>   * is raised */
>  orig_err = virSaveLastError();
> -if (qemuDomainAppendLog(driver, obj, logFD,
> -"Domain id=%d is tainted: %s\n",
> -obj->def->id,
> -virDomainTaintTypeToString(taint)) < 0)
> +if (logCtxt == NULL) {
> +logCtxt = qemuDomainLogContextNew(driver, obj,
> +  
> QEMU_DOMAIN_LOG_CONTEXT_MODE_ATTACH);
> +if (!logCtxt) {
> +VIR_WARN("Unable to open domainlog");

Coverity found - leaking orig_err


> +return;
> +}
> +closeLog = true;
> +}
> +
> +if (qemuDomainLogContextWrite(logCtxt,
> +  "Domain id=%d is tainted: %s\n",
> +  obj->def->id,
> +  virDomainTaintTypeToString(taint)) < 0)
>  virResetLastError();
> +if (closeLog)
> +qemuDomainLogContextFree(logCtxt);
>  if (orig_err) {
>  virSetError(orig_err);
>  virFreeError(orig_err);
> @@ -2140,7 +2153,7 @@ void qemuDomainObjTaint(virQEMUDriverPtr driver,
>  

[...]

ACK with the adjustment

John

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

Re: [libvirt] [PATCH v2 08/13] qemu: convert log file creation to use qemuDomainLogContextPtr

[John Ferlan]

On 11/12/2015 12:19 PM, Daniel P. Berrange wrote:
> Convert the places which create/open log files to use the new
> qemuDomainLogContextPtr object instead.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  src/qemu/qemu_domain.c  | 100 
> +---
>  src/qemu/qemu_domain.h  |   2 -
>  src/qemu/qemu_process.c |  78 +
>  3 files changed, 46 insertions(+), 134 deletions(-)
> 

[...]

> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c

[...]

> @@ -4854,7 +4850,7 @@ int qemuProcessStart(virConnectPtr conn,
>  
>   cleanup:
>  virCommandFree(cmd);
> -VIR_FORCE_CLOSE(logfile);
> +qemuDomainLogContextFree(logCtxt);
>  virObjectUnref(cfg);
>  virObjectUnref(caps);
>  VIR_FREE(nicindexes);
> @@ -4868,6 +4864,9 @@ int qemuProcessStart(virConnectPtr conn,
>   * pretend we never started it */
>  if (priv->mon)
>  qemuMonitorSetDomainLog(priv->mon, -1, -1);
> +/* Must close log now to allow ProcessSto to re-open it */

ProcessStop  (almost not worth mentioning ;-))

> +qemuDomainLogContextFree(logCtxt);
> +logCtxt = NULL;
>  qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, stop_flags);
>  goto cleanup;
>  

[...]

ACK

John

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

Re: [libvirt] [PATCH v2 07/13] qemu: introduce a qemuDomainLogContext object

[John Ferlan]

On 11/12/2015 12:19 PM, Daniel P. Berrange wrote:
> Introduce a qemuDomainLogContext object to encapsulate
> handling of I/O to/from the domain log file. This will
> hide details of the log file implementation from the
> rest of the driver, making it easier to introduce
> support for virtlogd later.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  src/qemu/qemu_domain.c | 178 
> +
>  src/qemu/qemu_domain.h |  22 ++
>  2 files changed, 200 insertions(+)
> 

[...]

>  
> +qemuDomainLogContextPtr qemuDomainLogContextNew(virQEMUDriverPtr driver,
> +virDomainObjPtr vm,
> +qemuDomainLogContextMode 
> mode)
> +{
> +virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> +qemuDomainLogContextPtr ctxt = NULL;
> +char *logfile = NULL;
> +
> +if (VIR_ALLOC(ctxt) < 0)
> +goto error;
> +
> +ctxt->writefd = -1;
> +ctxt->readfd = -1;
> +virAtomicIntSet(&ctxt->refs, 1);
> +
> +if (virAsprintf(&logfile, "%s/%s.log", cfg->logDir, vm->def->name) < 0)
> +goto error;
> +
> +if ((ctxt->writefd = open(logfile, O_WRONLY | O_CREAT | O_APPEND, 
> S_IRUSR | S_IWUSR)) < 0) {
> +virReportSystemError(errno, _("failed to create logfile %s"),
> + logfile);
> +goto error;
> +}
> +if (virSetCloseExec(ctxt->writefd) < 0) {
> +virReportSystemError(errno, _("failed to set close-on-exec flag on 
> %s"),
> + logfile);
> +goto error;
> +}
> +
> +/* For unprivileged startup we must truncate the file since
> + * we can't rely on logrotate. We don't use O_TRUNC since
> + * it is better for SELinux policy if we truncate afterwards */
> +if (mode == QEMU_DOMAIN_LOG_CONTEXT_MODE_START &&
> +!virQEMUDriverIsPrivileged(driver) &&
> +ftruncate(ctxt->writefd, 0) < 0) {
> +virReportSystemError(errno, _("failed to truncate %s"),
> + logfile);
> +goto error;
> +}
> +
> +if (mode == QEMU_DOMAIN_LOG_CONTEXT_MODE_START) {
> +if ((ctxt->readfd = open(logfile, O_RDONLY, S_IRUSR | S_IWUSR)) < 0) 
> {
> +virReportSystemError(errno, _("failed to open logfile %s"),
> + logfile);
> +goto error;
> +}
> +if (virSetCloseExec(ctxt->readfd) < 0) {
> +virReportSystemError(errno, _("failed to set close-on-exec flag 
> on %s"),
> + logfile);
> +goto error;
> +}
> +}

should 'pos' be initialized here?  Otherwise the first ContextRead would
use ctxt->pos == 0


> +
> +virObjectUnref(cfg);
> +return ctxt;
> +
> + error:
> +virObjectUnref(cfg);
> +qemuDomainLogContextFree(ctxt);
> +return NULL;
> +}
> +
> +

[...]

> +
> +ssize_t qemuDomainLogContextRead(qemuDomainLogContextPtr ctxt,
> + char **msg)
> +{
> +char *buf;
> +size_t buflen = 1024 * 128;
> +ssize_t got;
> +
> +/* Best effort jump to start of messages */
> +ignore_value(lseek(ctxt->readfd, ctxt->pos, SEEK_SET));
> +
> +if (VIR_ALLOC_N(buf, buflen) < 0)
> +return -1;
> +
> +got = saferead(ctxt->readfd, buf, buflen - 1);
> +if (got < 0) {
> +virReportSystemError(errno, "%s",
> + _("Unable to read from log file"));

Coverity points out buf is leaked.

> +return -1;
> +}
> +
> +buf[got] = '\0';
> +
> +ignore_value(VIR_REALLOC_N_QUIET(buf, got + 1));
> +*msg = buf;
> +
> +return got;
> +}
> +
> +

ACK with the obvious adjustment - not quite sure how to handle 'pos'
(yet) - although perhaps future patches will allow the light to shine.

John

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

Re: [libvirt] [PATCH v2 06/13] qemu: unify code for reporting errors from QEMU log files

[John Ferlan]

On 11/12/2015 12:19 PM, Daniel P. Berrange wrote:
> There are two pretty similar functions qemuProcessReadLog and
> qemuProcessReadChildErrors. Both read from the QEMU log file
> and try to strip out libvirt messages. The latter than reports

s/than/then

> an error, while the former lets the callers report an error.
> 
> Re-write qemuProcessReadLog so that it uses a single read
> into a dynamically allocated buffer. Then introduce a new
> qemuProcessReportLogError that calls qemuProcessReadLog
> and reports an error.
> 
> Convert all callers to use qemuProcessReportLogError.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  src/qemu/qemu_domain.c|  24 +-
>  src/qemu/qemu_domain.h|   2 +-
>  src/qemu/qemu_migration.c |   2 +-
>  src/qemu/qemu_monitor.c   |  58 +++---
>  src/qemu/qemu_monitor.h   |   2 +-
>  src/qemu/qemu_process.c   | 200 
> ++
>  src/qemu/qemu_process.h   |   4 +-
>  7 files changed, 95 insertions(+), 197 deletions(-)
> 
[...]

> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 69a0f97..c09e9dc 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -1549,7 +1549,7 @@ static qemuMonitorCallbacks monitorCallbacks = {
>  
>  static int
>  qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm, int asyncJob,
> -   int logfd)
> +   int logfd, off_t pos)
>  {
>  qemuDomainObjPrivatePtr priv = vm->privateData;
>  int ret = -1;
> @@ -1575,8 +1575,8 @@ qemuConnectMonitor(virQEMUDriverPtr driver, 
> virDomainObjPtr vm, int asyncJob,
>&monitorCallbacks,
>driver);
>  
> -if (mon)
> -ignore_value(qemuMonitorSetDomainLog(mon, logfd));
> +if (mon && logfd != -1 && pos != -1)
> +ignore_value(qemuMonitorSetDomainLog(mon, logfd, pos));
>  
>  virObjectLock(vm);
>  virObjectUnref(vm);
> @@ -1630,111 +1630,76 @@ qemuConnectMonitor(virQEMUDriverPtr driver, 
> virDomainObjPtr vm, int asyncJob,
>  /**
>   * qemuProcessReadLog: Read log file of a qemu VM
>   * @fd: File descriptor of the log file
> - * @buf: buffer to store the read messages
> - * @buflen: allocated space available in @buf
> + * @msg: pointer to buffer to store the read messages in

msg after off? (not that it matters that much)

>   * @off: Offset to start reading from
> - * @skipchar: Skip messages about created character devices
>   *
>   * Reads log of a qemu VM. Skips messages not produced by qemu or irrelevant
> - * messages. Returns length of the message stored in @buf, or -1 on error.
> + * messages. Returns returns 0 on success or -1 on error
>   */
> -int
> -qemuProcessReadLog(int fd, char *buf, int buflen, int off, bool skipchar)
> +static int
> +qemuProcessReadLog(int fd, off_t offset, char **msg)
>  {
> -char *filter_next = buf;
> -ssize_t bytes;
> +char *buf;
> +size_t buflen = 1024 * 128;
> +ssize_t got;
>  char *eol;
> +char *filter_next;
>  
> -while (off < buflen - 1) {
> -bytes = saferead(fd, buf + off, buflen - off - 1);
> -if (bytes < 0)
> -return -1;
> -
> -off += bytes;
> -buf[off] = '\0';
> +/* Best effort jump to start of messages */
> +ignore_value(lseek(fd, offset, SEEK_SET));
>  
> -if (bytes == 0)
> -break;
> +if (VIR_ALLOC_N(buf, buflen) < 0)
> +return -1;
>  
> -/* Filter out debug messages from intermediate libvirt process */
> -while ((eol = strchr(filter_next, '\n'))) {
> -*eol = '\0';
> -if (virLogProbablyLogMessage(filter_next) ||
> -(skipchar &&
> - STRPREFIX(filter_next, "char device redirected to"))) {
> -memmove(filter_next, eol + 1, off - (eol - buf));
> -off -= eol + 1 - filter_next;
> -} else {
> -filter_next = eol + 1;
> -*eol = '\n';
> -}
> -}
> +got = saferead(fd, buf, buflen - 1);
> +if (got < 0) {
> +virReportSystemError(errno, "%s",
> + _("Unable to read from log file"));

I know (because I ran Coverity on all the patches) that a future patch
changes this, but buf is leaked here - at least for a few patches.

> +return -1;
>  }

Additionally, theoretically 'got' could be 0 here
>  
> -return off;
> -}
> -
> -/*
> - * Read domain log and probably overwrite error if there's one in
> - * the domain log file. This function exists to cover the small
> - * window between fork() and exec() during which child may fail
> - * by libvirt's hand, e.g. placing onto a NUMA node failed.
> - */
> -static int
> -qemuProcessReadChildErrors(virQEMUDriverPtr driver,
> -   virDomainObjPtr vm,
> -   off_t originalOff)
> -{
> -int ret = -1;
> -int logfd;
> -off_t off = 0;
> -ssize

[libvirt] [PATCH 1/2] libxl: unref libxlDriverConfig object

[Jim Fehlig]

Commits b6e19cf4 and 6472e54a missed unref'ing the
libxlDriverConfig object. Add missing calls to virObjectUnref.

Signed-off-by: Jim Fehlig 
---
 src/libxl/libxl_driver.c | 22 --
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index fd4bae1..4609c00 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -4648,18 +4648,21 @@ libxlDomainGetTotalCPUStats(libxlDriverPrivatePtr 
driver,
 virTypedParameterPtr params,
 unsigned int nparams)
 {
-libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
+libxlDriverConfigPtr cfg;
 libxl_dominfo d_info;
 int ret = -1;
 
 if (nparams == 0)
 return LIBXL_NB_TOTAL_CPU_STAT_PARAM;
 
+libxl_dominfo_init(&d_info);
+cfg = libxlDriverConfigGet(driver);
+
 if (libxl_domain_info(cfg->ctx, &d_info, vm->def->id) != 0) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("libxl_domain_info failed for domain '%d'"),
vm->def->id);
-return ret;
+goto cleanup;
 }
 
 if (virTypedParameterAssign(¶ms[0], VIR_DOMAIN_CPU_STATS_CPUTIME,
@@ -4670,6 +4673,7 @@ libxlDomainGetTotalCPUStats(libxlDriverPrivatePtr driver,
 
  cleanup:
 libxl_dominfo_dispose(&d_info);
+virObjectUnref(cfg);
 return ret;
 }
 
@@ -4684,7 +4688,7 @@ libxlDomainGetPerCPUStats(libxlDriverPrivatePtr driver,
 libxl_vcpuinfo *vcpuinfo;
 int maxcpu, hostcpus;
 size_t i;
-libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
+libxlDriverConfigPtr cfg;
 int ret = -1;
 
 if (nparams == 0 && ncpus != 0)
@@ -4692,12 +4696,13 @@ libxlDomainGetPerCPUStats(libxlDriverPrivatePtr driver,
 else if (nparams == 0)
 return vm->def->maxvcpus;
 
+cfg = libxlDriverConfigGet(driver);
 if ((vcpuinfo = libxl_list_vcpu(cfg->ctx, vm->def->id, &maxcpu,
 &hostcpus)) == NULL) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to list vcpus for domain '%d' with 
libxenlight"),
vm->def->id);
-return ret;
+goto cleanup;
 }
 
 for (i = start_cpu; i < maxcpu && i < ncpus; ++i) {
@@ -4710,7 +4715,9 @@ libxlDomainGetPerCPUStats(libxlDriverPrivatePtr driver,
 ret = nparams;
 
  cleanup:
-libxl_vcpuinfo_list_free(vcpuinfo, maxcpu);
+if (vcpuinfo)
+libxl_vcpuinfo_list_free(vcpuinfo, maxcpu);
+virObjectUnref(cfg);
 return ret;
 }
 
@@ -4766,7 +4773,7 @@ libxlDomainMemoryStats(virDomainPtr dom,
unsigned int flags)
 {
 libxlDriverPrivatePtr driver = dom->conn->privateData;
-libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
+libxlDriverConfigPtr cfg;
 virDomainObjPtr vm;
 libxl_dominfo d_info;
 unsigned mem, maxmem;
@@ -4775,6 +4782,8 @@ libxlDomainMemoryStats(virDomainPtr dom,
 
 virCheckFlags(0, -1);
 
+cfg = libxlDriverConfigGet(driver);
+
 if (!(vm = libxlDomObjFromDomain(dom)))
 goto cleanup;
 
@@ -4815,6 +4824,7 @@ libxlDomainMemoryStats(virDomainPtr dom,
  cleanup:
 if (vm)
 virObjectUnlock(vm);
+virObjectUnref(cfg);
 return ret;
 }
 
-- 
2.1.4

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

[libvirt] [PATCH 2/2] libxl: don't unlock virDomainObj if refcnt is 0

[Jim Fehlig]

Commit 6472e54a unlocks the virDomainObj even if libxlDomainObjEndJob
returns false, indicating that its refcnt has dropped to 0.

Signed-off-by: Jim Fehlig 
---
 src/libxl/libxl_driver.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 4609c00..d77a0e4 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -4816,10 +4816,8 @@ libxlDomainMemoryStats(virDomainPtr dom,
 libxl_dominfo_dispose(&d_info);
 
  endjob:
-if (!libxlDomainObjEndJob(driver, vm)) {
-virObjectUnlock(vm);
+if (!libxlDomainObjEndJob(driver, vm))
 vm = NULL;
-}
 
  cleanup:
 if (vm)
-- 
2.1.4

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

[libvirt] [PATCH 0/2] libxl: a few minor fixes

[Jim Fehlig]

A few minor fixes for patches 1 and 2 of Joao's series adding stats
support to the libxl driver

https://www.redhat.com/archives/libvir-list/2015-November/msg00489.html

I only noticed the minor flaws while reviewing patches later in the series,
after already committing patches 1 and 2.

Jim Fehlig (2):
  libxl: unref libxlDriverConfig object
  libxl: don't unlock virDomainObj if refcnt is 0

 src/libxl/libxl_driver.c | 26 +-
 1 file changed, 17 insertions(+), 9 deletions(-)

-- 
2.1.4

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

Re: [libvirt] [Xen-devel] [PATCH v3 2/8] libxl: implement virDomainMemorystats

[Konrad Rzeszutek Wilk]

On November 17, 2015 6:15:38 PM EST, Jim Fehlig  wrote:
>Joao Martins wrote:
>> Introduce support for domainMemoryStats API call, which
>> consequently enables the use of `virsh dommemstat` command to
>> query for memory statistics of a domain. We support
>> the following statistics: balloon info, available and currently
>> in use. swap-in, swap-out, major-faults, minor-faults require
>> cooperation of the guest and thus currently not supported.
>> 
>> We build on the data returned from libxl_domain_info and deliver
>> it in the virDomainMemoryStat format.
>> 
>> Signed-off-by: Joao Martins 
>> ---
>> Changes since v1:
>>  - Cleanup properly after error fetching domain stats
>>  - Dispose libxl_dominfo after succesfull call to
>>  libxl_domain_info()
>>  - Bump version to 1.2.22
>> ---
>>  src/libxl/libxl_driver.c | 70
>
>>  1 file changed, 70 insertions(+)
>> 
>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>> index 50f6e34..f4fc7bc 100644
>> --- a/src/libxl/libxl_driver.c
>> +++ b/src/libxl/libxl_driver.c
>> @@ -4749,6 +4749,75 @@ libxlDomainGetCPUStats(virDomainPtr dom,
>>  return ret;
>>  }
>>  
>> +#define LIBXL_SET_MEMSTAT(TAG, VAL) \
>> +if (i < nr_stats) { \
>> +stats[i].tag = TAG; \
>> +stats[i].val = VAL; \
>> +i++; \
>> +}
>> +
>> +static int
>> +libxlDomainMemoryStats(virDomainPtr dom,
>> +   virDomainMemoryStatPtr stats,
>> +   unsigned int nr_stats,
>> +   unsigned int flags)
>> +{
>> +libxlDriverPrivatePtr driver = dom->conn->privateData;
>> +libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
>> +virDomainObjPtr vm;
>> +libxl_dominfo d_info;
>> +unsigned mem, maxmem;
>> +size_t i = 0;
>> +int ret = -1;
>> +
>> +virCheckFlags(0, -1);
>> +
>> +if (!(vm = libxlDomObjFromDomain(dom)))
>> +goto cleanup;
>> +
>> +if (virDomainMemoryStatsEnsureACL(dom->conn, vm->def) < 0)
>> +goto cleanup;
>> +
>> +if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_QUERY) < 0)
>> +goto cleanup;
>> +
>> +if (!virDomainObjIsActive(vm)) {
>> +virReportError(VIR_ERR_OPERATION_INVALID,
>> +   "%s", _("domain is not running"));
>> +goto endjob;
>> +}
>> +
>> +if (libxl_domain_info(cfg->ctx, &d_info, vm->def->id) != 0) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>> +   _("libxl_domain_info failed for domain
>'%d'"),
>> +   vm->def->id);
>> +goto endjob;
>> +}
>> +mem = d_info.current_memkb;
>> +maxmem = d_info.max_memkb;
>> +
>> +LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, maxmem
>- mem);
>
>Should this just be 'mem'? On a domain with
>
>  1048576
>  1048576
>
>I see
>
># virsh dommemstat test
>actual 1024
>available 1049600
>rss 1048576
>
>The documentation for VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON only says
>"Current
>balloon value (in KB)". I (perhaps incorrectly) interpret that as the
>amount of
>memory currently assigned to the domain.
>
>Also, I wonder if we should provide rss for Xen domains? I'm not sure
>it makes
>much sense since Xen domains are more than just a process running on
>the host.
>AFAIK, rss would always be set to d_info.current_memkb even if a domain
>was not
>actually using all the memory.
>

You could go negative with that - say dom0 has 8GB and your guest has 16GB. 
From a Linux dom0 POV we are -8GB, and since tools would most likely use 
unsigned int the sum of RSS would minus total memory would be negative,but that 
is 0x... So the tools might show the guest consuming gobs of memory?


Perhaps just how much QEMU is consuming (if running)?

>Regards,
>Jim
>
>> +LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_AVAILABLE, maxmem);
>> +LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_RSS, mem);
>> +
>> +ret = i;
>> +
>> +libxl_dominfo_dispose(&d_info);
>> +
>> + endjob:
>> +if (!libxlDomainObjEndJob(driver, vm)) {
>> +virObjectUnlock(vm);
>> +vm = NULL;
>> +}
>> +
>> + cleanup:
>> +if (vm)
>> +virObjectUnlock(vm);
>> +return ret;
>> +}
>> +
>> +#undef LIBXL_SET_MEMSTAT
>> +
>>  static int
>>  libxlConnectDomainEventRegisterAny(virConnectPtr conn, virDomainPtr
>dom, int eventID,
>>
>virConnectDomainEventGenericCallback callback,
>> @@ -5342,6 +5411,7 @@ static virHypervisorDriver
>libxlHypervisorDriver = {
>>  #endif
>>  .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
>>  .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1
>*/
>> +.domainMemoryStats = libxlDomainMemoryStats, /* 1.2.22 */
>>  .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.2.22 */
>>  .connectDomainEventRegister = libxlConnectDomainEventRegister,
>/* 0.9.0 */
>>  .connectDomainEventDeregister =
>libxlConnectDomainEventDeregist

Re: [libvirt] [PATCH v3 6/8] libxl: implement virConnectGetAllDomainStats

[Jim Fehlig]

On 11/13/2015 06:14 AM, Joao Martins wrote:
> Introduce support for connectGetAllDomainStats call that
> allow us to _all_ domain(s) statistics including network, block,

allows us to get

> cpus and memory. Changes are rather mechanical and mostly
> take care of the format to export the data.
>
> Signed-off-by: Joao Martins 
> ---
> Changes since v1:
>  - Rework flags checking on libxlDomainGetStats
>  for VIR_DOMAIN_STATS_{VCPU,INTERFACE,BLOCK}
>  - Removed path since we are reusing .ifname
>  - Init dominfo and dispose it on cleanup.
>  - Fixed VIR_FREE issue that was reported with make syntax-check"
>  - Bump version to 1.2.22
> ---
>  src/libxl/libxl_driver.c | 266 
> +++
>  1 file changed, 266 insertions(+)
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index ba1d67b..8db6536 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -5238,6 +5238,271 @@ libxlDomainMemoryStats(virDomainPtr dom,
>  
>  #undef LIBXL_SET_MEMSTAT
>  
> +#define LIBXL_RECORD_UINT(error, key, value, ...) \
> +do { \
> +char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
> +snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
> + key, ##__VA_ARGS__); \
> +if (virTypedParamsAddUInt(&tmp->params, \
> +  &tmp->nparams, \
> +  &maxparams, \
> +  param_name, \
> +  value) < 0) \
> +goto error;   \
> +} while (0)
> +
> +#define LIBXL_RECORD_LL(error, key, value, ...) \
> +do { \
> +char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
> +snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
> + key, ##__VA_ARGS__); \
> +if (virTypedParamsAddLLong(&tmp->params, \
> +   &tmp->nparams, \
> +   &maxparams, \
> +   param_name, \
> +   value) < 0) \
> +goto error; \
> +} while (0)
> +
> +#define LIBXL_RECORD_ULL(error, key, value, ...) \
> +do { \
> +char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
> +snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
> + key, ##__VA_ARGS__); \
> +if (virTypedParamsAddULLong(&tmp->params, \
> +&tmp->nparams, \
> +&maxparams, \
> +param_name, \
> +value) < 0) \
> +goto error; \
> +} while (0)
> +
> +#define LIBXL_RECORD_STR(error, key, value, ...) \
> +do { \
> +char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
> +snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
> + key, ##__VA_ARGS__); \
> +if (virTypedParamsAddString(&tmp->params, \
> +&tmp->nparams, \
> +&maxparams, \
> +param_name, \
> +value) < 0) \
> +goto error; \
> +} while (0)
> +
> +static int
> +libxlDomainGetStats(virConnectPtr conn,
> +virDomainObjPtr dom,
> +unsigned int stats,
> +virDomainStatsRecordPtr *record)
> +{
> +libxlDriverPrivatePtr driver = conn->privateData;
> +libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);

cfg needs to be unref'ed when this function terminates. Sadly I noticed this
only after pushing some of the other patches with a similar flaw.

> +virDomainStatsRecordPtr tmp;
> +libxl_dominfo d_info;
> +libxl_vcpuinfo *vcpuinfo = NULL;
> +int maxcpu, hostcpus;
> +unsigned long long mem, maxmem;
> +int maxparams = 0;
> +int ret = -1;
> +size_t i, state;
> +unsigned int domflags = stats & (VIR_DOMAIN_STATS_BALLOON |
> + VIR_DOMAIN_STATS_CPU_TOTAL);
> +
> +if (VIR_ALLOC(tmp) < 0)
> +return ret;
> +
> +libxl_dominfo_init(&d_info);
> +
> +mem = virDomainDefGetMemoryInitial(dom->def);
> +maxmem = virDomainDefGetMemoryActual(dom->def);
> +d_info.cpu_time = 0;
> +
> +if (domflags && virDomainObjIsActive(dom) &&
> +!libxl_domain_info(cfg->ctx, &d_info, dom->def->id)) {
> +mem = d_info.current_memkb;
> +maxmem = d_info.max_memkb;
> +}
> +
> +if (stats & VIR_DOMAIN_STATS_STATE) {
> +LIBXL_RECORD_UINT(cleanup, "state.reason", dom->state.reason);
> +LIBXL_RECORD_UINT(cleanup, "state.state", dom->state.state);
> +}
> +
> +if (stats & VIR_DOMAIN_STATS_BALLOON) {
> +LIBXL_RECORD_ULL(cleanup, "balloon.current", mem);
> +LIBXL_RECORD_ULL(cleanup, "balloon.maximum", maxmem);
> +}
> +
> +if (stats & VIR_DOMAIN_STATS_CPU_TOTAL)
> +LIBXL_RECORD_ULL(cleanup, "cpu.time", d_info.cpu_time);
> +
> +if (stats & VI

Re: [libvirt] [PATCH v2 05/13] qemu: remove writing to QEMU log file for rename operation

[John Ferlan]

On 11/12/2015 12:19 PM, Daniel P. Berrange wrote:
> The rename operation only works on inactive virtual machines,
> but it none the less writes to the log file used by the QEMU
> processes. This log file is not intended to provide a general
> purpose audit trail of operations performed on VMs. The audit
> subsystem has recording of important operations. If we want
> to extend that to cover all significant public APIs that is
> a valid thing to consider, but we shouldn't arbitrarily log
> specific APIs into the QEMU log file in the meantime.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  src/qemu/qemu_driver.c | 28 
>  1 file changed, 28 deletions(-)
> 

Seems reasonable.  Of course one could have "renamed" the log file to
use the new name in order to keep all the history of the same machine
(the uuid seems to remain constant).

ACK -

John

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

Re: [libvirt] [PATCH v2 04/13] logging: add client for virtlogd daemon

[John Ferlan]

On 11/12/2015 12:19 PM, Daniel P. Berrange wrote:
> Add the virLogManager API which allows for communication with
> the virtlogd daemon to RPC program. This provides the client
> side API to open log files for guest domains.
> 
> The virtlogd daemon is setup to auto-spawn on first use when
> running unprivileged. For privileged usage, systemd socket
> activation is used instead.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  po/POTFILES.in|   2 +
>  src/Makefile.am   |   4 +-
>  src/libvirt_private.syms  |   8 ++
>  src/logging/log_daemon_dispatch.c |   7 +
>  src/logging/log_handler.c |   7 +-
>  src/logging/log_manager.c | 283 
> ++
>  src/logging/log_manager.h |  61 
>  src/logging/log_protocol.x|   1 +
>  8 files changed, 370 insertions(+), 3 deletions(-)
>  create mode 100644 src/logging/log_manager.c
>  create mode 100644 src/logging/log_manager.h
> 

[...]

> diff --git a/src/logging/log_daemon_dispatch.c 
> b/src/logging/log_daemon_dispatch.c
> index f612be1..203fdc4 100644
> --- a/src/logging/log_daemon_dispatch.c
> +++ b/src/logging/log_daemon_dispatch.c
> @@ -116,6 +116,13 @@ 
> virLogManagerProtocolDispatchDomainReadLogFile(virNetServerPtr server 
> ATTRIBUTE_
>  int rv = -1;
>  char *data;
>  
> +if (args->maxlen > VIR_LOG_MANAGER_PROTOCOL_STRING_MAX) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Requested data len %zu is larger than maximum %d"),
> +   args->maxlen, VIR_LOG_MANAGER_PROTOCOL_STRING_MAX);
> +goto cleanup;
> +}
> +

This feels like it should have been in prior patch...

>  if ((data = 
> virLogHandlerDomainReadLogFile(virLogDaemonGetHandler(logDaemon),
> args->driver,
> (unsigned char 
> *)args->dom.uuid,
> diff --git a/src/logging/log_handler.c b/src/logging/log_handler.c
> index 8853fd0..1465c5a 100644
> --- a/src/logging/log_handler.c
> +++ b/src/logging/log_handler.c
> @@ -442,6 +442,7 @@ char *virLogHandlerDomainReadLogFile(virLogHandlerPtr 
> handler,
>  char *path;
>  virRotatingFileReaderPtr file = NULL;
>  char *data = NULL;
> +ssize_t got;
>  
>  if (!(path = virLogHandlerGetLogFilePathForDomain(handler,
>driver,
> @@ -455,11 +456,13 @@ char *virLogHandlerDomainReadLogFile(virLogHandlerPtr 
> handler,
>  if (virRotatingFileReaderSeek(file, inode, offset) < 0)
>  goto error;
>  
> -if (VIR_ALLOC_N(data, maxlen) < 0)
> +if (VIR_ALLOC_N(data, maxlen + 1) < 0)
>  goto error;
>  
> -if (virRotatingFileReaderConsume(file, data, maxlen) < 0)
> +got = virRotatingFileReaderConsume(file, data, maxlen);
> +if (got < 0)
>  goto error;
> +data[got] = '\0';

Perhaps another prior patch..

>  
>  virRotatingFileReaderFree(file);
>  return data;

[...]  Nothing jumped out at me in log_manager.{ch}

> diff --git a/src/logging/log_protocol.x b/src/logging/log_protocol.x
> index 2702beb..de57c69 100644
> --- a/src/logging/log_protocol.x
> +++ b/src/logging/log_protocol.x
> @@ -57,6 +57,7 @@ struct virLogManagerProtocolDomainReadLogFileArgs {
>  virLogManagerProtocolDomain dom;
>  virLogManagerProtocolLogFilePosition pos;
>  unsigned hyper maxlen;
> +unsigned int flags;

should this perhaps have been in the prior patch?

>  };
>  
>  struct virLogManagerProtocolDomainReadLogFileRet {
> 

ACK - nothing major here - your call on whether things should move.

John

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

Re: [libvirt] [PATCH v3 4/8] util: add virDiskNameParse to handle disk and partition idx

[Jim Fehlig]

On 11/13/2015 06:14 AM, Joao Martins wrote:
> Introduce a new helper function "virDiskNameParse" which extends
> virDiskNameToIndex but handling both disk index and partition index.
> Also rework virDiskNameToIndex to be based on virDiskNameParse.
> A test is also added for this function testing both valid and
> invalid disk names.
>
> Signed-off-by: Joao Martins 
> ---
> Changes since v2:
>  - Sort the newly added symbol in the list
> ---
>  src/libvirt_private.syms |  1 +
>  src/util/virutil.c   | 41 +++
>  src/util/virutil.h   |  1 +
>  tests/utiltest.c | 56 
> 
>  4 files changed, 95 insertions(+), 4 deletions(-)

ACK; pushed. Thanks!

Regards,
Jim

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

Re: [libvirt] [PATCH v3 2/8] libxl: implement virDomainMemorystats

[Jim Fehlig]

On 11/18/2015 11:05 AM, Joao Martins wrote:
>
> On 11/17/2015 11:15 PM, Jim Fehlig wrote:
>> Joao Martins wrote:
>>> Introduce support for domainMemoryStats API call, which
>>> consequently enables the use of `virsh dommemstat` command to
>>> query for memory statistics of a domain. We support
>>> the following statistics: balloon info, available and currently
>>> in use. swap-in, swap-out, major-faults, minor-faults require
>>> cooperation of the guest and thus currently not supported.
>>>
>>> We build on the data returned from libxl_domain_info and deliver
>>> it in the virDomainMemoryStat format.
>>>
>>> Signed-off-by: Joao Martins 
>>> ---
>>> Changes since v1:
>>>  - Cleanup properly after error fetching domain stats
>>>  - Dispose libxl_dominfo after succesfull call to
>>>  libxl_domain_info()
>>>  - Bump version to 1.2.22
>>> ---
>>>  src/libxl/libxl_driver.c | 70 
>>> 
>>>  1 file changed, 70 insertions(+)
>>>
>>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>>> index 50f6e34..f4fc7bc 100644
>>> --- a/src/libxl/libxl_driver.c
>>> +++ b/src/libxl/libxl_driver.c
>>> @@ -4749,6 +4749,75 @@ libxlDomainGetCPUStats(virDomainPtr dom,
>>>  return ret;
>>>  }
>>>  
>>> +#define LIBXL_SET_MEMSTAT(TAG, VAL) \
>>> +if (i < nr_stats) { \
>>> +stats[i].tag = TAG; \
>>> +stats[i].val = VAL; \
>>> +i++; \
>>> +}
>>> +
>>> +static int
>>> +libxlDomainMemoryStats(virDomainPtr dom,
>>> +   virDomainMemoryStatPtr stats,
>>> +   unsigned int nr_stats,
>>> +   unsigned int flags)
>>> +{
>>> +libxlDriverPrivatePtr driver = dom->conn->privateData;
>>> +libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
>>> +virDomainObjPtr vm;
>>> +libxl_dominfo d_info;
>>> +unsigned mem, maxmem;
>>> +size_t i = 0;
>>> +int ret = -1;
>>> +
>>> +virCheckFlags(0, -1);
>>> +
>>> +if (!(vm = libxlDomObjFromDomain(dom)))
>>> +goto cleanup;
>>> +
>>> +if (virDomainMemoryStatsEnsureACL(dom->conn, vm->def) < 0)
>>> +goto cleanup;
>>> +
>>> +if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_QUERY) < 0)
>>> +goto cleanup;
>>> +
>>> +if (!virDomainObjIsActive(vm)) {
>>> +virReportError(VIR_ERR_OPERATION_INVALID,
>>> +   "%s", _("domain is not running"));
>>> +goto endjob;
>>> +}
>>> +
>>> +if (libxl_domain_info(cfg->ctx, &d_info, vm->def->id) != 0) {
>>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>>> +   _("libxl_domain_info failed for domain '%d'"),
>>> +   vm->def->id);
>>> +goto endjob;
>>> +}
>>> +mem = d_info.current_memkb;
>>> +maxmem = d_info.max_memkb;
>>> +
>>> +LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, maxmem - mem);
>> Should this just be 'mem'? On a domain with
>>
>>   1048576
>>   1048576
>>
>> I see
>>
>> # virsh dommemstat test
>> actual 1024
>> available 1049600
>> rss 1048576
>>
>> The documentation for VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON only says 
>> "Current
>> balloon value (in KB)". I (perhaps incorrectly) interpret that as the amount 
>> of
>> memory currently assigned to the domain.
> I interpreted that it based on the size of the balloon instead of based on the
> memory. But trying out with plain qemu (to see what the monitor socket 
> returns,
> since qemu driver takes those values directly) and also libvirt+qemu and it 
> gave
> me what you mentioned. VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON is indeed the
> amount of memory currently assigned to the domain. Apologies for the 
> confusion.
>
>> Also, I wonder if we should provide rss for Xen domains? I'm not sure it 
>> makes
>> much sense since Xen domains are more than just a process running on the 
>> host.
>> AFAIK, rss would always be set to d_info.current_memkb even if a domain was 
>> not
>> actually using all the memory.
> I agree, but I mainly included RSS stat for the lack of a "current memory in
> use". But since this is cleared and ACTUAL_BALLOON is not the size of the
> balloon but to the actual memory, I guess this renders RSS stat a bit useless,
> and perhaps misleading like you suggest. This is the hunk to be applied on top
> of this patch, having tested already, and make {syntax-check,test}-ed.
>
> Regards,
> Joao
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index db13fd2..c1563c2 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -4799,9 +4799,8 @@ libxlDomainMemoryStats(virDomainPtr dom,
>  mem = d_info.current_memkb;
>  maxmem = d_info.max_memkb;
>
> -LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, maxmem - mem);
> +LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, mem);
>  LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_AVAILABLE, maxmem);
> -LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_RSS, 

Re: [libvirt] [PATCH v2 03/13] logging: introduce log handling protocol

[John Ferlan]

On 11/12/2015 12:19 PM, Daniel P. Berrange wrote:
> Define a new RPC protocol for the virtlogd daemon that provides
> for handling of logs. The initial RPC method defined allows a
> client to obtain a file handle to use for writing to a log
> file for a guest domain. The file handle passed back will not
> actually refer to the log file, but rather an anonymous pipe.
> The virtlogd daemon will forward I/O between them, ensuring
> file rotation happens when required.
> 
> Initially the log setup is hardcoded to cap log files at
> 128 KB, and keep 2 backups when rolling over.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  po/POTFILES.in|   1 +
>  src/Makefile.am   |   4 +
>  src/logging/log_daemon.c  |  30 +++
>  src/logging/log_daemon.h  |   3 +
>  src/logging/log_daemon_dispatch.c | 101 +++-
>  src/logging/log_handler.c | 521 
> ++
>  src/logging/log_handler.h |  63 +
>  src/logging/log_protocol.x|  94 ++-
>  8 files changed, 815 insertions(+), 2 deletions(-)
>  create mode 100644 src/logging/log_handler.c
>  create mode 100644 src/logging/log_handler.h
> 

As I was thinking about the "view of a client" - I began to wonder if
the logic in virRotatingFileReaderSeek which doesn't find the
inode/offset should perhaps return a failure instead of the offset of
the oldest file (especially if maxbackup == 0)?  Is it possible to have
a case where there are two backups and enough time has passed such that
we've rotated more than once (or is that an Amsterdam moment ;-))?

Shouldn't perhaps the client be able to handle whether the file it was
looking at has rotated out of existence and then decide what to do
rather than assuming that the client would want the offset of the most
recent rotated file (if of course it even exists)?


Another thought I had - within this new model might it be reasonable to
allow a client to set the logging level desired for a connection rather
than using the same logging level for all virtlogd connections? I know
I've found use in the past of raising and lowering logging levels for
debugging libvirtd, but I have to turn off all domains if I want more;
otherwise, the log file is overwhelmed.  But I could certainly see need
if I were debugging to 'enable' or 'disable' specific logging "on the
fly"...  Different problem!

Anyway...

[...]

>
> diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c
> index 184076c..bc13257 100644
> --- a/src/logging/log_daemon.c
> +++ b/src/logging/log_daemon.c

[...]

> @@ -157,6 +162,12 @@ virLogDaemonNew(virLogDaemonConfigPtr config, bool 
> privileged)
>  }
>  
>  
> +virLogHandlerPtr virLogDaemonGetHandler(virLogDaemonPtr daemon)

NIT: Two lines
virLogHandlerPtr
virLogDaemon...

> +{
> +return daemon->handler;
> +}
> +
> +

[...]

> diff --git a/src/logging/log_daemon_dispatch.c 
> b/src/logging/log_daemon_dispatch.c
> index 98df178..f612be1 100644
> --- a/src/logging/log_daemon_dispatch.c
> +++ b/src/logging/log_daemon_dispatch.c
> @@ -1,7 +1,7 @@
>  /*
>   * log_daemon_dispatch.c: log management daemon dispatch
>   *
> - * Copyright (C) 2006-2015 Red Hat, Inc.
> + * Copyright (C) 2015 Red Hat, Inc.

Probably should have been something in patch 2...  I didn't mention it
there mainly because I read your comment to Peter...

>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Lesser General Public

[...]

> diff --git a/src/logging/log_handler.c b/src/logging/log_handler.c
> new file mode 100644
> index 000..8853fd0

[...]

> +
> +static void
> +virLogHandlerDomainLogFileEvent(int watch,
> +int fd,
> +int events,
> +void *opaque)
> +{
> +virLogHandlerPtr handler = opaque;
> +virLogHandlerLogFilePtr logfile;
> +char buf[1024];
> +ssize_t len;
> +
> +virObjectLock(handler);
> +logfile = virLogHandlerGetLogFileFromWatch(handler, watch);
> +if (!logfile || logfile->pipefd != fd) {
> +virEventRemoveHandle(watch);

handler is still locked

> +return;
> +}
> +
> + reread:
> +len = read(fd, buf, sizeof(buf));
> +if (len < 0) {
> +if (errno == EINTR)
> +goto reread;
> +
> +virReportSystemError(errno, "%s",
> + _("Unable to read from log pipe"));
> +goto error;
> +}
> +
> +if (virRotatingFileWriterAppend(logfile->file, buf, len) != len)
> +goto error;
> +
> +if (events & VIR_EVENT_HANDLE_HANGUP)
> +goto error;
> +
> +virObjectUnlock(handler);
> +return;
> +
> + error:
> +virLogHandlerLogFileClose(handler, logfile);
> +virObjectUnlock(handler);
> +}
> +
> +

[...]

> +
> +
> +static virLogHandlerLogFilePtr 
> virLogHandlerLogFilePostExecRestart(virJSONValuePtr object)
> +{
> +virLogHandlerLogFilePtr f

[libvirt] qemuParseCommandLine and virDomainDefPostParse (and virDomaniDefAddImplicitControllers)

[Laine Stump]

While playing with the idea of forcing explicit USB controller models, I 
ended up with the qemuargv2xml test failing; it ended up that this was 
because changes I had made to qemuDomainDefPostParse() were affecting 
the XML produced by qemuParseCommandline(). The reason - after 
constructing a virDomainDef object by parsing a qemu commandline, 
qemuParseCommandline() calls two functions that are supposed to be 
called after parsing domain XML - virDomainDefPostParse() (which calls 
qemuDomainDefPostParse()).


In my opinion, qemuParseCommandLine() shouldn't be calling 
virDomainDefPostParse() (or virDomainefAddImplicitControllers(), which 
it calls in the wrong order relative to virDomainDefPostParse() BTW). 
The reasons are:


1) this is causing the argv-to-xml conversion to include things in the 
XML that were not on the original commandline, in particular "default" 
devices like PCI and USB controllers (added in qemuDomainDefPostParse() 
based on machinetype) as well as disk, smartcard, virtio-serial, and 
hostdev-scsi controllers in virDomainDefAddImplicitControllers() (why 
the duality there, anyway?)


2) If the output of argv-to-xml is used for a virDomainDefine, those 
post-parse functions will be called then, and the implicit/auto devices 
will be added at that time anyway, so in practice nothing is gained by 
adding them during argv-to-xml.


Does anyone else have an opinion about this?

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

Re: [libvirt] [PATCH v3 5/8] libxl: implement virDomainBlockStats

[Jim Fehlig]

On 11/13/2015 06:14 AM, Joao Martins wrote:
> Introduce initial support for domainBlockStats API call that
> allow us to query block device statistics. openstack nova
> uses this API call to query block statistics, alongside
> virDomainMemoryStats and virDomainInterfaceStats.  Note that
> this patch only introduces it for VBD for starters. QDisk
> will come in a separate patch series.
>
> A new statistics data structure is introduced to fit common
> statistics among others specific to the underlying block
> backends. For the VBD statistics on linux these are exported
> via sysfs on the path:
>
> "/sys/bus/xen-backend/devices/vbd--/statistics"
>
> To calculate the block devno libxlDiskPathToID is introduced.
> Each backend implements its own function to extract statistics
> and let there be handled the different platforms. An alternative
> would be to reuse libvirt xen driver function.
>
> VBD stats are exposed in reqs and number of sectors from
> blkback, and it's up to us to convert it to sector sizes.
> The sector size is gathered through xenstore in the device
> backend entry "physical-sector-size". This adds up an extra
> dependency namely of xenstore for doing the xs_read.
>
> BlockStatsFlags variant is also implemented which has the
> added benefit of getting the number of flush requests.
>
> Signed-off-by: Joao Martins 
> ---
> Changes since v1:
>  - Fix identation issues
>  - Set ret to LIBXL_VBD_SECTOR_SIZE
>  - Reuse VIR_STRDUP error instead of doing virReportError
>  when we fail to set stats->backend
>  - Change virAsprintf(...) error checking
>  - Change error to VIR_ERR_OPERATION_FAILED when xenstore path
>  does not exist and when failing to read stat.
>  - Resolve issues with 'make syntax-check' with cppi installed.
>  - Remove libxlDiskPathMatches in favor of using virutil
>  virDiskNameParse to fetch disk and partition index.
>  - Rename libxlDiskPathParse to libxlDiskPathToID and rework
>  function to just convert disk and partition index to devno.
>  - Bump version to 1.2.22
> ---
>  configure.ac |   2 +-
>  src/libxl/libxl_driver.c | 375 
> +++
>  2 files changed, 376 insertions(+), 1 deletion(-)
>
> diff --git a/configure.ac b/configure.ac
> index f481c50..10c56e5 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -896,7 +896,7 @@ if test "$with_libxl" != "no" ; then
>  LIBS="$LIBS $LIBXL_LIBS"
>  AC_CHECK_LIB([xenlight], [libxl_ctx_alloc], [
>  with_libxl=yes
> -LIBXL_LIBS="$LIBXL_LIBS -lxenlight -lxenctrl"
> +LIBXL_LIBS="$LIBXL_LIBS -lxenlight -lxenctrl -lxenstore"
>  ],[
>  if test "$with_libxl" = "yes"; then
>  fail=1
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 9a5a74c..ba1d67b 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -59,6 +59,7 @@
>  #include "network/bridge_driver.h"
>  #include "locking/domain_lock.h"
>  #include "virstats.h"
> +#include 
>  
>  #define VIR_FROM_THIS VIR_FROM_LIBXL
>  
> @@ -75,6 +76,7 @@ VIR_LOG_INIT("libxl.libxl_driver");
>  #define LIBXL_CONFIG_FORMAT_SEXPR "xen-sxpr"
>  
>  #define LIBXL_NB_TOTAL_CPU_STAT_PARAM 1
> +#define LIBXL_NB_TOTAL_BLK_STAT_PARAM 6
>  
>  #define HYPERVISOR_CAPABILITIES "/proc/xen/capabilities"
>  #define HYPERVISOR_XENSTORED "/dev/xen/xenstored"
> @@ -103,6 +105,25 @@ struct _libxlOSEventHookInfo {
>  int id;
>  };
>  
> +/* Object used to store disk statistics across multiple xen backends */
> +typedef struct _libxlBlockStats libxlBlockStats;
> +typedef libxlBlockStats *libxlBlockStatsPtr;
> +struct _libxlBlockStats {
> +long long rd_req;
> +long long rd_bytes;
> +long long wr_req;
> +long long wr_bytes;
> +long long f_req;
> +
> +char *backend;
> +union {
> +struct {
> +long long ds_req;
> +long long oo_req;
> +} vbd;
> +} u;
> +};
> +
>  /* Function declarations */
>  static int
>  libxlDomainManagedSaveLoad(virDomainObjPtr vm,
> @@ -4644,6 +4665,358 @@ libxlDomainIsUpdated(virDomainPtr dom)
>  }
>  
>  static int
> +libxlDiskPathToID(const char *virtpath)
> +{
> +static char const* drive_prefix[] = {"xvd", "hd", "sd"};
> +int disk, partition, chrused;
> +int fmt, id;
> +char *r;
> +size_t i;
> +
> +fmt = id = -1;
> +
> +/* Find any disk prefixes we know about */
> +for (i = 0; i < ARRAY_CARDINALITY(drive_prefix); i++) {
> +if (STRPREFIX(virtpath, drive_prefix[i]) &&
> +!virDiskNameParse(virtpath, &disk, &partition)) {
> +fmt = i;
> +break;
> +}
> +}
> +
> +/* Handle it same way as xvd */
> +if (fmt < 0 &&
> +(sscanf(virtpath, "d%ip%i%n", &disk, &partition, &chrused) >= 2
> + && chrused == strlen(virtpath)))
> +fmt = 0;
> +
> +/* Test indexes ranges and calculate the device id */
> +switch (fmt) {
> +case 0: /* xv

[libvirt] [PATCH] set UID and GID according to dynamicOwnership

[Matteo Cafasso]

When called with dynamicOwnership set, the qemuOpenFileAs function ignores it 
resulting in files not created with proper permissions.

The issue is reported in:

https://www.redhat.com/archives/libvirt-users/2015-November/msg00039.html

and also mentioned in:

https://www.redhat.com/archives/libvir-list/2015-November/msg00453.html


Signed-off-by: Matteo Cafasso 
---
 src/qemu/qemu_driver.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 92a9961..b3b59b6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2932,6 +2932,11 @@ qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid,
 if (path_shared <= 0 || dynamicOwnership)
 vfoflags |= VIR_FILE_OPEN_FORCE_OWNER;
 
+if (dynamicOwnership) {
+uid = fallback_uid;
+gid = fallback_gid;
+}
+
 if (stat(path, &sb) == 0) {
 /* It already exists, we don't want to delete it on error */
 need_unlink = false;
-- 
2.6.2

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

Re: [libvirt] [PATCH v3 1/8] libxl: implement virDomainGetCPUStats

[Joao Martins]

On 11/18/2015 05:33 PM, Jim Fehlig wrote:
> On 11/16/2015 07:59 PM, Jim Fehlig wrote:
>> On 11/13/2015 06:14 AM, Joao Martins wrote:
>> @@ -5233,6 +5342,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
>>  #endif
>>  .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
>>  .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */
>> +.domainGetCPUStats = libxlDomainGetCPUStats, /* 1.2.22 */
>> ACK with the above nit fixed. I'll push this one too after verifying whether
>> next release is 1.2.22 or 1.3.0.
> 
> Opps, forgot to mention that I pushed this yesterday. Thanks!
> 
Great, so 1.2.22 then!

Joao

> Regards,
> Jim
> 

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

[libvirt] [PATCH 16/21] qemu: migration: dest: nbd-server to UNIX sock

[Pavel Boldin]

Modify qemuMigrationStartNBDServer so it can instruct QEMU to start
NBD server binded to a local UNIX socket.

Signed-off-by: Pavel Boldin 
---
 src/qemu/qemu_migration.c | 18 +++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 43f71e9..303cd47 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1694,6 +1694,7 @@ qemuMigrationPrecreateStorage(virConnectPtr conn,
 static int
 qemuMigrationStartNBDServer(virQEMUDriverPtr driver,
 virDomainObjPtr vm,
+bool tunnel,
 const char *listenAddr,
 size_t nmigrate_disks,
 const char **migrate_disks)
@@ -1701,8 +1702,9 @@ qemuMigrationStartNBDServer(virQEMUDriverPtr driver,
 int ret = -1;
 qemuDomainObjPrivatePtr priv = vm->privateData;
 unsigned short port = 0;
-char *diskAlias = NULL;
+char *diskAlias = NULL, *tunnelName = NULL;
 size_t i;
+virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
 
 for (i = 0; i < vm->def->ndisks; i++) {
 virDomainDiskDefPtr disk = vm->def->disks[i];
@@ -1720,12 +1722,20 @@ qemuMigrationStartNBDServer(virQEMUDriverPtr driver,
QEMU_ASYNC_JOB_MIGRATION_IN) < 0)
 goto cleanup;
 
-if (!port &&
+if (!tunnel && !port &&
 ((virPortAllocatorAcquire(driver->migrationPorts, &port) < 0) ||
  (qemuMonitorNBDServerStart(priv->mon, listenAddr, port) < 0))) {
 goto exit_monitor;
 }
 
+if (tunnel && !tunnelName &&
+((virAsprintf(&tunnelName,
+  "%s/domain-%s/qemu.nbdtunnelmigrate.src",
+  cfg->libDir, vm->def->name) < 0) ||
+ (qemuMonitorNBDServerStartUnix(priv->mon, tunnelName) < 0))) {
+goto exit_monitor;
+}
+
 if (qemuMonitorNBDServerAdd(priv->mon, diskAlias, true) < 0)
 goto exit_monitor;
 if (qemuDomainObjExitMonitor(driver, vm) < 0)
@@ -1736,7 +1746,9 @@ qemuMigrationStartNBDServer(virQEMUDriverPtr driver,
 ret = 0;
 
  cleanup:
+virObjectUnref(cfg);
 VIR_FREE(diskAlias);
+VIR_FREE(tunnelName);
 if (ret < 0)
 virPortAllocatorRelease(driver->migrationPorts, port);
 return ret;
@@ -3488,7 +3500,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
 if (mig->nbd &&
 flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) &&
 virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NBD_SERVER)) {
-if (qemuMigrationStartNBDServer(driver, vm, listenAddress,
+if (qemuMigrationStartNBDServer(driver, vm, tunnel, listenAddress,
 nmigrate_disks, migrate_disks) < 0) {
 /* error already reported */
 goto endjob;
-- 
1.9.1

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

[libvirt] [PATCH 02/21] driver: add virDrvDomainMigrateOpenTunnel

[Pavel Boldin]

Add virDrvDomainMigrateOpenTunnel call that is to be implemented by
the drivers in order to provide a way to open tunnel during migration.

Signed-off-by: Pavel Boldin 
---
 src/driver-hypervisor.h | 8 
 1 file changed, 8 insertions(+)

diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h
index ae2ec4d..30a7446 100644
--- a/src/driver-hypervisor.h
+++ b/src/driver-hypervisor.h
@@ -1212,6 +1212,13 @@ typedef int
const char *password,
unsigned int flags);
 
+typedef int
+(*virDrvDomainMigrateOpenTunnel)(virConnectPtr dconn,
+ virStreamPtr st,
+ unsigned char uuid[VIR_UUID_BUFLEN],
+ unsigned int flags);
+
+
 typedef struct _virHypervisorDriver virHypervisorDriver;
 typedef virHypervisorDriver *virHypervisorDriverPtr;
 
@@ -1443,6 +1450,7 @@ struct _virHypervisorDriver {
 virDrvDomainGetFSInfo domainGetFSInfo;
 virDrvDomainInterfaceAddresses domainInterfaceAddresses;
 virDrvDomainSetUserPassword domainSetUserPassword;
+virDrvDomainMigrateOpenTunnel domainMigrateOpenTunnel;
 };
 
 
-- 
1.9.1

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

[libvirt] [PATCH 00/21] Support NBD for tunnelled migration

[Pavel Boldin]

The provided patchset implements NBD disk migration over a tunnelled
connection provided by libvirt.

The migration source instructs QEMU to NBD mirror drives into the provided
UNIX socket. These connections and all the data are then tunnelled to the
destination using newly introduced RPC call. The migration destination
implements a driver method that connects the tunnelled stream to the QEMU's
NBD destination.

The detailed scheme is the following:

PREPARE
 1. Migration destination starts QEMU's NBD server listening on a UNIX socket
using the `nbd-server-add` monitor command and tells NBD to accept listed
disks via code added to qemuMigrationStartNBDServer that calls introduced
qemuMonitorNBDServerStartUnix monitor function.

PERFORM
 2. Migration source creates a UNIX socket that is later used as NBDs
destination in `drive-mirror` monitor command.

This is implemented as a call to virNetSocketNewListenUnix from
doTunnelMigrate.

 3. Source starts IOThread that polls on the UNIX socket, accepting every
incoming QEMU connection.

This is done by adding a new pollfd in the poll(2) call in
qemuMigrationIOFunc that calls introduced qemuNBDTunnelAcceptAndPipe
function.

 4. The qemuNBDTunnelAcceptAndPipe function accepts the connection and creates
two virStream's. One is `local` that is later associated with just accepted
connection using virFDStreamOpen. Second is `remote` that is later
tunnelled to the remote destination stream.

The `local` stream is converted to a virFDStreamDrv stream using the
virFDStreamOpen call on the fd returned by accept(2).

The `remote` stream is associated with a stream on the destination in
the way similar to used by PrepareTunnel3* function. That is, the
virDomainMigrateOpenTunnel function called on the destination
connection object. The virDomainMigrateOpenTunnel calls remote driver's
handler remoteDomainMigrateOpenTunnel that makes DOMAIN_MIGRATE_OPEN_TUNNEL
call to the destination host. The code in remoteDomainMigrateOpenTunnel
ties passed virStream object to a virStream on the destination host via
remoteStreamDrv driver. The remote driver handles stream's IO by tunnelling
data through the RPC connection.

The qemuNBDTunnelAcceptAndPipe at last assigns both streams the same event
callback qemuMigrationPipeEvent. Its job is to track statuses of the
streams doing IO whenever it is necessary.

 5. Source starts the drive mirroring using the qemuMigrationDriveMirror func.
The function instructs QEMU to mirror drives to the UNIX socket that thread
listens on.

Since it is necessary for the mirror driving to get into the 'synchronized'
state, where writes go to both destinations simultaneously, before
continuing VM migration, the thread serving the connections must be
started earlier.

 6. When the connection to a UNIX socket on the migration source is made
the DOMAIN_MIGRATE_OPEN_TUNNEL proc is called on the migration destination.

The handler of this code calls virDomainMigrateOpenTunnel which calls
qemuMigrationOpenNBDTunnel by the means of qemuDomainMigrateOpenTunnel. 

The qemuMigrationOpenNBDTunnel connects the stream linked to a source's
stream to the NBD's UNIX socket on the migration destination side.
 
 7. The rest of the disk migration occurs semimagically: virStream* APIs tunnel
data in both directions. This is done by qemuMigrationPipeEvent event
callback set for both streams.


The order of the patches is roughly the following:

 *  First, the RPC machinery and remote driver's virDrvDomainMigrateOpenTunnel
implementation are added.

 *  Then, the source-side of the protocol is implemented: code listening
on a UNIX socket is added, DriveMirror is enhanced to instruct QEMU to
`drive-mirror` here and starting IOThread driving the tunneling sooner.

 *  After that, the destination-side of the protocol is implemented:
the qemuMonitorNBDServerStartUnix added and qemuMigrationStartNBDServer
enhanced to call it. The qemuDomainMigrateOpenTunnel is implemented
along with qemuMigrationOpenNBDTunnel that does the real job.

 *  Finally, the code blocking NBD migration for tunnelled migration is
removed.

Pavel Boldin (21):
  rpc: add DOMAIN_MIGRATE_OPEN_TUNNEL proc
  driver: add virDrvDomainMigrateOpenTunnel
  remote_driver: introduce virRemoteClientNew
  remote_driver: add remoteDomainMigrateOpenTunnel
  domain: add virDomainMigrateOpenTunnel
  domain: add virDomainMigrateTunnelFlags
  remote: impl remoteDispatchDomainMigrateOpenTunnel
  qemu: migration: src: add nbd tunnel socket data
  qemu: migration: src: nbdtunnel unix socket
  qemu: migration: src: qemu `drive-mirror` to UNIX
  qemu: migration: src: qemuSock for running thread
  qemu: migration: src: add NBD unixSock to iothread
  qemu: migration: src: qemuNBDTunnelAcceptAndPipe
  qemu: migration: src: stream piping
  qemu: monitor:

[libvirt] [PATCH 20/21] qemu: migration: allow NBD tunneling migration

[Pavel Boldin]

Now that all the pieces are in their places finally allow NBD in
tunnelled migration.

Signed-off-by: Pavel Boldin 
---
 src/qemu/qemu_migration.c | 16 ++--
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 27c1acb..9520e34 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3041,13 +3041,6 @@ qemuMigrationBeginPhase(virQEMUDriverPtr driver,
 goto cleanup;
 }
 }
-
-if (flags & VIR_MIGRATE_TUNNELLED) {
-virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
-   _("Selecting disks to migrate is not "
- "implemented for tunnelled migration"));
-goto cleanup;
-}
 } else {
 virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
_("qemu does not support drive-mirror 
command"));
@@ -3056,13 +3049,8 @@ qemuMigrationBeginPhase(virQEMUDriverPtr driver,
 }
 
 if (has_drive_mirror) {
-/* TODO support NBD for TUNNELLED migration */
-if (flags & VIR_MIGRATE_TUNNELLED) {
-VIR_WARN("NBD in tunnelled migration is currently not 
supported");
-} else {
-cookieFlags |= QEMU_MIGRATION_COOKIE_NBD;
-priv->nbdPort = 0;
-}
+cookieFlags |= QEMU_MIGRATION_COOKIE_NBD;
+priv->nbdPort = 0;
 }
 }
 
-- 
1.9.1

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

[libvirt] [PATCH 11/21] qemu: migration: src: qemuSock for running thread

[Pavel Boldin]

Tunnelled drive mirroring requires an active thread to accept incoming
connections from the QEMU and pumping them to the remote host through
the tunnel.

For this, we need to split thread's QEMU socket initialization from
the start of the thread and introduce qemuMigrationSetQEMUSocket
to specify it later.

Signed-off-by: Pavel Boldin 
---
 src/qemu/qemu_migration.c | 93 ++-
 1 file changed, 59 insertions(+), 34 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index d95cd66..61e78c5 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3991,14 +3991,15 @@ typedef struct _qemuMigrationIOThread 
qemuMigrationIOThread;
 typedef qemuMigrationIOThread *qemuMigrationIOThreadPtr;
 struct _qemuMigrationIOThread {
 virThread thread;
-virStreamPtr st;
-int sock;
+virStreamPtr qemuStream;
+int qemuSock;
 virError err;
 int wakeupRecvFD;
 int wakeupSendFD;
 };
 
-static void qemuMigrationIOFunc(void *arg)
+static void
+qemuMigrationIOFunc(void *arg)
 {
 qemuMigrationIOThreadPtr data = arg;
 char *buffer = NULL;
@@ -4006,21 +4007,18 @@ static void qemuMigrationIOFunc(void *arg)
 int timeout = -1;
 virErrorPtr err = NULL;
 
-VIR_DEBUG("Running migration tunnel; stream=%p, sock=%d",
-  data->st, data->sock);
+VIR_DEBUG("Running migration tunnel; qemuStream=%p", data->qemuStream);
 
 if (VIR_ALLOC_N(buffer, TUNNEL_SEND_BUF_SIZE) < 0)
 goto abrt;
 
-fds[0].fd = data->sock;
-fds[1].fd = data->wakeupRecvFD;
+fds[0].fd = data->wakeupRecvFD;
+fds[1].fd = -1;
+fds[0].events = fds[1].events = POLLIN;
 
 for (;;) {
 int ret;
 
-fds[0].events = fds[1].events = POLLIN;
-fds[0].revents = fds[1].revents = 0;
-
 ret = poll(fds, ARRAY_CARDINALITY(fds), timeout);
 
 if (ret < 0) {
@@ -4040,30 +4038,36 @@ static void qemuMigrationIOFunc(void *arg)
 break;
 }
 
-if (fds[1].revents & (POLLIN | POLLERR | POLLHUP)) {
-char stop = 0;
+if (fds[0].revents & (POLLIN | POLLERR | POLLHUP)) {
+char action = 0;
 
-if (saferead(data->wakeupRecvFD, &stop, 1) != 1) {
+if (saferead(data->wakeupRecvFD, &action, 1) != 1) {
 virReportSystemError(errno, "%s",
  _("failed to read from wakeup fd"));
 goto abrt;
 }
 
-VIR_DEBUG("Migration tunnel was asked to %s",
-  stop ? "abort" : "finish");
-if (stop) {
-goto abrt;
-} else {
-timeout = 0;
+VIR_DEBUG("Migration tunnel was asked to %c", action);
+switch (action) {
+case 's':
+goto abrt;
+break;
+case 'f':
+timeout = 0;
+break;
+case 'u':
+fds[1].fd = data->qemuSock;
+VIR_DEBUG("qemuSock set %d", data->qemuSock);
+break;
 }
 }
 
-if (fds[0].revents & (POLLIN | POLLERR | POLLHUP)) {
+if (fds[1].revents & (POLLIN | POLLERR | POLLHUP)) {
 int nbytes;
 
-nbytes = saferead(data->sock, buffer, TUNNEL_SEND_BUF_SIZE);
+nbytes = saferead(data->qemuSock, buffer, TUNNEL_SEND_BUF_SIZE);
 if (nbytes > 0) {
-if (virStreamSend(data->st, buffer, nbytes) < 0)
+if (virStreamSend(data->qemuStream, buffer, nbytes) < 0)
 goto error;
 } else if (nbytes < 0) {
 virReportSystemError(errno, "%s",
@@ -4076,10 +4080,9 @@ static void qemuMigrationIOFunc(void *arg)
 }
 }
 
-if (virStreamFinish(data->st) < 0)
-goto error;
+virStreamFinish(data->qemuStream);
 
-VIR_FORCE_CLOSE(data->sock);
+VIR_FORCE_CLOSE(data->qemuSock);
 VIR_FREE(buffer);
 
 return;
@@ -4090,7 +4093,7 @@ static void qemuMigrationIOFunc(void *arg)
 virFreeError(err);
 err = NULL;
 }
-virStreamAbort(data->st);
+virStreamAbort(data->qemuStream);
 if (err) {
 virSetError(err);
 virFreeError(err);
@@ -4099,7 +4102,7 @@ static void qemuMigrationIOFunc(void *arg)
  error:
 /* Let the source qemu know that the transfer cant continue anymore.
  * Don't copy the error for EPIPE as destination has the actual error. */
-VIR_FORCE_CLOSE(data->sock);
+VIR_FORCE_CLOSE(data->qemuSock);
 if (!virLastErrorIsSystemErrno(EPIPE))
 virCopyLastError(&data->err);
 virResetLastError();
@@ -4108,8 +4111,7 @@ static void qemuMigrationIOFunc(void *arg)
 
 
 static qemuMigrationIOThreadPtr
-qemuMigrationStartTunnel(virStreamPtr st,
- int sock)
+qemuMigrationStartTunnel(virStreamPtr qemuStream)
 {
 

[libvirt] [PATCH 09/21] qemu: migration: src: nbdtunnel unix socket

[Pavel Boldin]

Create a UNIX socket that will be a target for outgoing NBD connection
from the QEMU side.

Signed-off-by: Pavel Boldin 
---
 src/qemu/qemu_migration.c | 24 +++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index fb2a216..d587c56 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -4597,7 +4597,7 @@ static int doTunnelMigrate(virQEMUDriverPtr driver,
size_t nmigrate_disks,
const char **migrate_disks)
 {
-virNetSocketPtr sock = NULL;
+virNetSocketPtr sock = NULL, nbdSock = NULL;
 int ret = -1;
 qemuMigrationSpec spec;
 virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
@@ -4613,6 +4613,23 @@ static int doTunnelMigrate(virQEMUDriverPtr driver,
 spec.fwdType = MIGRATION_FWD_STREAM;
 spec.fwd.stream = st;
 
+if (nmigrate_disks) {
+spec.nbd_tunnel_unix_socket.sock = -1;
+spec.nbd_tunnel_unix_socket.file = NULL;
+
+if (virAsprintf(&spec.nbd_tunnel_unix_socket.file,
+"%s/domain-%s/qemu.nbdtunnelmigrate.src",
+cfg->libDir, vm->def->name) < 0)
+goto cleanup;
+
+if (virNetSocketNewListenUNIX(spec.nbd_tunnel_unix_socket.file, 0700,
+  cfg->user, cfg->group,
+  &nbdSock) < 0 ||
+virNetSocketListen(nbdSock, 1) < 0)
+goto cleanup;
+
+spec.nbd_tunnel_unix_socket.sock = virNetSocketGetFD(nbdSock);
+}
 
 spec.destType = MIGRATION_DEST_FD;
 spec.dest.fd.qemu = -1;
@@ -4643,6 +4660,11 @@ static int doTunnelMigrate(virQEMUDriverPtr driver,
 VIR_FREE(spec.dest.unix_socket.file);
 }
 
+if (nmigrate_disks) {
+virObjectUnref(nbdSock);
+VIR_FREE(spec.nbd_tunnel_unix_socket.file);
+}
+
 virObjectUnref(cfg);
 return ret;
 }
-- 
1.9.1

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

[libvirt] [PATCH 15/21] qemu: monitor: add qemuMonitorNBDServerStartUnix

[Pavel Boldin]

Add qemuMonitorNBDServerStartUnix used to instruct QEMU to connect
to a UNIX socket as a NBD drive mirror destination.

Signed-off-by: Pavel Boldin 
---
 src/qemu/qemu_monitor.c  | 12 
 src/qemu/qemu_monitor.h  |  2 ++
 src/qemu/qemu_monitor_json.c | 35 +++
 src/qemu/qemu_monitor_json.h |  2 ++
 4 files changed, 51 insertions(+)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 49d4aa2..9c8e0fe 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3594,6 +3594,18 @@ qemuMonitorNBDServerStart(qemuMonitorPtr mon,
 
 
 int
+qemuMonitorNBDServerStartUnix(qemuMonitorPtr mon,
+  const char *file)
+{
+VIR_DEBUG("file=%s", file);
+
+QEMU_CHECK_MONITOR_JSON(mon);
+
+return qemuMonitorJSONNBDServerStartUnix(mon, file);
+}
+
+
+int
 qemuMonitorNBDServerAdd(qemuMonitorPtr mon,
 const char *deviceID,
 bool writable)
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 2ce3958..e94ac93 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -873,6 +873,8 @@ char *qemuMonitorGetTargetArch(qemuMonitorPtr mon);
 int qemuMonitorNBDServerStart(qemuMonitorPtr mon,
   const char *host,
   unsigned int port);
+int qemuMonitorNBDServerStartUnix(qemuMonitorPtr mon,
+  const char *path);
 int qemuMonitorNBDServerAdd(qemuMonitorPtr mon,
 const char *deviceID,
 bool writable);
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index b39b29b..ef162a4 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -5749,6 +5749,41 @@ qemuMonitorJSONNBDServerStart(qemuMonitorPtr mon,
 }
 
 int
+qemuMonitorJSONNBDServerStartUnix(qemuMonitorPtr mon,
+  const char *file)
+{
+int ret = -1;
+virJSONValuePtr cmd = NULL;
+virJSONValuePtr reply = NULL;
+virJSONValuePtr addr = NULL;
+
+if (!(addr = qemuMonitorJSONBuildUnixSocketAddress(file)))
+return ret;
+
+if (!(cmd = qemuMonitorJSONMakeCommand("nbd-server-start",
+   "a:addr", addr,
+   NULL)))
+goto cleanup;
+
+/* From now on, @addr is part of @cmd */
+addr = NULL;
+
+if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
+goto cleanup;
+
+if (qemuMonitorJSONCheckError(cmd, reply) < 0)
+goto cleanup;
+
+ret = 0;
+
+ cleanup:
+virJSONValueFree(reply);
+virJSONValueFree(cmd);
+virJSONValueFree(addr);
+return ret;
+}
+
+int
 qemuMonitorJSONNBDServerAdd(qemuMonitorPtr mon,
 const char *deviceID,
 bool writable)
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 120bd93..28e17c0 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -441,6 +441,8 @@ char *qemuMonitorJSONGetTargetArch(qemuMonitorPtr mon);
 int qemuMonitorJSONNBDServerStart(qemuMonitorPtr mon,
   const char *host,
   unsigned int port);
+int qemuMonitorJSONNBDServerStartUnix(qemuMonitorPtr mon,
+  const char *path);
 int qemuMonitorJSONNBDServerAdd(qemuMonitorPtr mon,
 const char *deviceID,
 bool writable);
-- 
1.9.1

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

[libvirt] [PATCH 01/21] rpc: add DOMAIN_MIGRATE_OPEN_TUNNEL proc

[Pavel Boldin]

Add REMOTE_PROC_DOMAIN_MIGRATE_OPEN_TUNNEL remote call, args and rets.

Signed-off-by: Pavel Boldin 
---
 daemon/remote.c  | 12 
 src/remote/remote_protocol.x | 19 ++-
 src/remote_protocol-structs  |  8 
 3 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/daemon/remote.c b/daemon/remote.c
index 3a3eb09..237124d 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -6661,6 +6661,18 @@ remoteDispatchDomainInterfaceAddresses(virNetServerPtr 
server ATTRIBUTE_UNUSED,
 }
 
 
+static int
+remoteDispatchDomainMigrateOpenTunnel(virNetServerPtr server ATTRIBUTE_UNUSED,
+  virNetServerClientPtr client 
ATTRIBUTE_UNUSED,
+  virNetMessagePtr msg ATTRIBUTE_UNUSED,
+  virNetMessageErrorPtr rerr 
ATTRIBUTE_UNUSED,
+  remote_domain_migrate_open_tunnel_args 
*args ATTRIBUTE_UNUSED,
+  remote_domain_migrate_open_tunnel_ret 
*ret ATTRIBUTE_UNUSED)
+{
+return -1;
+}
+
+
 /*- Helpers. -*/
 
 /* get_nonnull_domain and get_nonnull_network turn an on-wire
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
index 80f4a8b..3d8702f 100644
--- a/src/remote/remote_protocol.x
+++ b/src/remote/remote_protocol.x
@@ -3240,6 +3240,15 @@ struct remote_domain_rename_ret {
 int retcode;
 };
 
+struct remote_domain_migrate_open_tunnel_args {
+remote_uuid uuid;
+unsigned int flags;
+};
+
+struct remote_domain_migrate_open_tunnel_ret {
+int retcode;
+};
+
 /*- Protocol. -*/
 
 /* Define the program number, protocol version and procedure numbers here. */
@@ -5712,5 +5721,13 @@ enum remote_procedure {
  * @acl: domain:write
  * @acl: domain:save
  */
-REMOTE_PROC_DOMAIN_RENAME = 358
+REMOTE_PROC_DOMAIN_RENAME = 358,
+
+/**
+ * @generate: none
+ * @acl: domain:migrate
+ * @acl: domain:start
+ * @acl: domain:write
+ */
+REMOTE_PROC_DOMAIN_MIGRATE_OPEN_TUNNEL = 359
 };
diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
index ff99c00..b065576 100644
--- a/src/remote_protocol-structs
+++ b/src/remote_protocol-structs
@@ -2692,6 +2692,13 @@ struct remote_domain_rename_args {
 struct remote_domain_rename_ret {
 intretcode;
 };
+struct remote_domain_migrate_open_tunnel_args {
+remote_uuiduuid;
+u_int  flags;
+};
+struct remote_domain_migrate_open_tunnel_ret {
+intretcode;
+};
 enum remote_procedure {
 REMOTE_PROC_CONNECT_OPEN = 1,
 REMOTE_PROC_CONNECT_CLOSE = 2,
@@ -3051,4 +3058,5 @@ enum remote_procedure {
 REMOTE_PROC_DOMAIN_DEL_IOTHREAD = 356,
 REMOTE_PROC_DOMAIN_SET_USER_PASSWORD = 357,
 REMOTE_PROC_DOMAIN_RENAME = 358,
+REMOTE_PROC_DOMAIN_MIGRATE_OPEN_TUNNEL = 359,
 };
-- 
1.9.1

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

[libvirt] [PATCH 07/21] remote: impl remoteDispatchDomainMigrateOpenTunnel

[Pavel Boldin]

Implement remoteDispatchDomainMigrateOpenTunnel.

Signed-off-by: Pavel Boldin 
---
 daemon/remote.c | 50 --
 1 file changed, 44 insertions(+), 6 deletions(-)

diff --git a/daemon/remote.c b/daemon/remote.c
index 237124d..3c6803e 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -6663,13 +6663,51 @@ remoteDispatchDomainInterfaceAddresses(virNetServerPtr 
server ATTRIBUTE_UNUSED,
 
 static int
 remoteDispatchDomainMigrateOpenTunnel(virNetServerPtr server ATTRIBUTE_UNUSED,
-  virNetServerClientPtr client 
ATTRIBUTE_UNUSED,
-  virNetMessagePtr msg ATTRIBUTE_UNUSED,
-  virNetMessageErrorPtr rerr 
ATTRIBUTE_UNUSED,
-  remote_domain_migrate_open_tunnel_args 
*args ATTRIBUTE_UNUSED,
-  remote_domain_migrate_open_tunnel_ret 
*ret ATTRIBUTE_UNUSED)
+  virNetServerClientPtr client,
+  virNetMessagePtr msg,
+  virNetMessageErrorPtr rerr,
+  remote_domain_migrate_open_tunnel_args 
*args,
+  remote_domain_migrate_open_tunnel_ret 
*ret)
 {
-return -1;
+int rv = -1;
+struct daemonClientPrivate *priv =
+virNetServerClientGetPrivateData(client);
+virStreamPtr st = NULL;
+daemonClientStreamPtr stream = NULL;
+
+if (!priv->conn) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));
+goto cleanup;
+}
+
+if (!(st = virStreamNew(priv->conn, VIR_STREAM_NONBLOCK)) ||
+!(stream = daemonCreateClientStream(client, st, remoteProgram,
+&msg->header)))
+goto cleanup;
+
+ret->retcode = virDomainMigrateOpenTunnel(priv->conn, st,
+  (unsigned char *)args->uuid,
+  args->flags);
+
+if (ret->retcode < 0)
+goto cleanup;
+
+if (daemonAddClientStream(client, stream, true) < 0)
+goto cleanup;
+
+rv = 0;
+
+ cleanup:
+if (rv < 0) {
+virNetMessageSaveError(rerr);
+if (stream) {
+virStreamAbort(st);
+daemonFreeClientStream(client, stream);
+} else {
+virObjectUnref(st);
+}
+}
+return rv;
 }
 
 
-- 
1.9.1

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

[libvirt] [PATCH 14/21] qemu: migration: src: stream piping

[Pavel Boldin]

Add and use qemuMigrationPipeEvent piped streams' event handler. It
sets the appropriate event flags for each of the stream and pumps the
pipe using qemuMigrationPipeIO whenever there is a data at any end.

Signed-off-by: Pavel Boldin 
---
 src/qemu/qemu_migration.c | 85 ++-
 1 file changed, 84 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 0f35c13..43f71e9 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -4010,8 +4010,28 @@ struct _qemuMigrationPipe {
 qemuMigrationIOThreadPtr data;
 virStreamPtr local;
 virStreamPtr remote;
+
+int local_flags : 4;
+int remote_flags : 4;
+char buffer[TUNNEL_SEND_BUF_SIZE];
 };
 
+static int
+qemuMigrationPipeIO(virStreamPtr from, virStreamPtr to, char *buffer)
+{
+int done, got, offset = 0;
+got = virStreamRecv(from, buffer, TUNNEL_SEND_BUF_SIZE);
+
+while (offset < got) {
+done = virStreamSend(to, buffer + offset, got - offset);
+if (done < 0)
+break;
+offset += done;
+}
+
+return got;
+}
+
 static void
 qemuMigrationPipeClose(qemuMigrationPipePtr pipe, bool abort)
 {
@@ -4030,6 +4050,55 @@ qemuMigrationPipeClose(qemuMigrationPipePtr pipe, bool 
abort)
 virObjectUnref(pipe->remote);
 }
 
+static void
+qemuMigrationPipeEvent(virStreamPtr stream, int events, void *opaque)
+{
+qemuMigrationPipePtr pipe = opaque;
+
+if (stream == pipe->remote)
+pipe->remote_flags |= events;
+if (stream == pipe->local)
+pipe->local_flags |= events;
+
+VIR_DEBUG("remote = %p, remote_flags = %x, local = %p, local_flags = %x",
+  pipe->remote, pipe->remote_flags,
+  pipe->local, pipe->local_flags);
+
+if (events & (VIR_STREAM_EVENT_ERROR | VIR_STREAM_EVENT_HANGUP)) {
+char dummy;
+virStreamRecv(stream, &dummy, 1);
+ abrt:
+virCopyLastError(&pipe->data->err);
+qemuMigrationPipeClose(pipe, true);
+if (safewrite(pipe->data->wakeupSendFD, "c", 1) != 1) {
+virReportSystemError(errno, "%s",
+ _("failed to stop migration tunnel"));
+}
+return;
+}
+
+if ((pipe->remote_flags & VIR_STREAM_EVENT_READABLE) &&
+(pipe->local_flags & VIR_STREAM_EVENT_WRITABLE)) {
+
+if (qemuMigrationPipeIO(pipe->remote, pipe->local, pipe->buffer) == -1)
+goto abrt;
+
+pipe->remote_flags &= ~VIR_STREAM_EVENT_READABLE;
+pipe->local_flags &= ~VIR_STREAM_EVENT_WRITABLE;
+}
+
+if ((pipe->local_flags & VIR_STREAM_EVENT_READABLE) &&
+(pipe->remote_flags & VIR_STREAM_EVENT_WRITABLE)) {
+
+if (qemuMigrationPipeIO(pipe->local, pipe->remote, pipe->buffer) == -1)
+goto abrt;
+
+pipe->local_flags &= ~VIR_STREAM_EVENT_READABLE;
+pipe->remote_flags &= ~VIR_STREAM_EVENT_WRITABLE;
+}
+}
+
+
 static qemuMigrationPipePtr
 qemuMigrationPipeCreate(virStreamPtr local, virStreamPtr remote)
 {
@@ -4041,6 +4110,20 @@ qemuMigrationPipeCreate(virStreamPtr local, virStreamPtr 
remote)
 pipe->local = local;
 pipe->remote = remote;
 
+if (virStreamEventAddCallback(local,
+  VIR_STREAM_EVENT_READABLE |
+  VIR_STREAM_EVENT_WRITABLE,
+  qemuMigrationPipeEvent,
+  pipe, NULL) < 0)
+goto error;
+
+if (virStreamEventAddCallback(remote,
+  VIR_STREAM_EVENT_READABLE |
+  VIR_STREAM_EVENT_WRITABLE,
+  qemuMigrationPipeEvent,
+  pipe, NULL) < 0)
+goto error;
+
 return pipe;
 
  error:
@@ -4230,7 +4313,7 @@ qemuMigrationIOFunc(void *arg)
 /* Let the source qemu know that the transfer cant continue anymore.
  * Don't copy the error for EPIPE as destination has the actual error. */
 VIR_FORCE_CLOSE(data->qemuSock);
-if (!virLastErrorIsSystemErrno(EPIPE))
+if (data->err.code == VIR_ERR_OK && !virLastErrorIsSystemErrno(EPIPE))
 virCopyLastError(&data->err);
 virResetLastError();
 VIR_FREE(buffer);
-- 
1.9.1

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

[libvirt] [PATCH 04/21] remote_driver: add remoteDomainMigrateOpenTunnel

[Pavel Boldin]

Add remoteDomainMigrateOpenTunnel that ties passed stream to the
network stream and then makes the appropriate remote call.

Signed-off-by: Pavel Boldin 
---
 src/remote/remote_driver.c | 43 +++
 1 file changed, 43 insertions(+)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index b72cb86..f6571a9 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -8087,6 +8087,48 @@ remoteDomainRename(virDomainPtr dom, const char 
*new_name, unsigned int flags)
 }
 
 
+static int
+remoteDomainMigrateOpenTunnel(virConnectPtr dconn,
+  virStreamPtr st,
+  unsigned char uuid[VIR_UUID_BUFLEN],
+  unsigned int flags)
+{
+struct private_data *priv = dconn->privateData;
+int rv = -1;
+remote_domain_migrate_open_tunnel_args args;
+remote_domain_migrate_open_tunnel_ret ret;
+virNetClientStreamPtr netst;
+
+remoteDriverLock(priv);
+
+memset(&args, 0, sizeof(args));
+memset(&ret, 0, sizeof(ret));
+
+netst = virRemoteClientOpen(st, priv,
+REMOTE_PROC_DOMAIN_MIGRATE_OPEN_TUNNEL);
+
+if (netst == NULL)
+goto done;
+
+memcpy(args.uuid, uuid, VIR_UUID_BUFLEN);
+args.flags = flags;
+
+if (call(dconn, priv, 0, REMOTE_PROC_DOMAIN_MIGRATE_OPEN_TUNNEL,
+ (xdrproc_t) xdr_remote_domain_migrate_open_tunnel_args, (char *) 
&args,
+ (xdrproc_t) xdr_remote_domain_migrate_open_tunnel_ret, (char *) 
&ret) == -1) {
+virNetClientRemoveStream(priv->client, netst);
+virObjectUnref(netst);
+goto done;
+}
+
+rv = ret.retcode;
+
+ done:
+remoteDriverUnlock(priv);
+return rv;
+}
+
+
 /* get_nonnull_domain and get_nonnull_network turn an on-wire
  * (name, uuid) pair into virDomainPtr or virNetworkPtr object.
  * These can return NULL if underlying memory allocations fail,
@@ -8437,6 +8479,7 @@ static virHypervisorDriver hypervisor_driver = {
 .domainInterfaceAddresses = remoteDomainInterfaceAddresses, /* 1.2.14 */
 .domainSetUserPassword = remoteDomainSetUserPassword, /* 1.2.16 */
 .domainRename = remoteDomainRename, /* 1.2.19 */
+.domainMigrateOpenTunnel = remoteDomainMigrateOpenTunnel, /* 1.2.XX */
 };
 
 static virNetworkDriver network_driver = {
-- 
1.9.1

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

[libvirt] [PATCH 18/21] qemu: driver: add qemuDomainMigrateOpenTunnel

[Pavel Boldin]

Add domainMigrateOpenTunnel handler for QEMU driver.

Signed-off-by: Pavel Boldin 
---
 src/qemu/qemu_driver.c | 24 
 1 file changed, 24 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 92a9961..ad9a6a0 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -20064,6 +20064,29 @@ static int qemuDomainRename(virDomainPtr dom,
 goto endjob;
 }
 
+static int qemuDomainMigrateOpenTunnel(virConnectPtr dconn,
+   virStreamPtr st,
+   unsigned char uuid[VIR_UUID_BUFLEN],
+   unsigned int flags)
+{
+virQEMUDriverPtr driver = dconn->privateData;
+int ret = -1;
+virDomainObjPtr vm;
+
+virCheckFlags(VIR_MIGRATE_TUNNEL_NBD, -1);
+
+vm = virDomainObjListFindByUUIDRef(driver->domains, uuid);
+
+if (virDomainMigrateOpenTunnelEnsureACL(dconn, vm->def) < 0)
+goto cleanup;
+
+ret = qemuMigrationOpenTunnel(driver, dconn, st, vm->def, flags);
+
+ cleanup:
+virDomainObjEndAPI(&vm);
+return ret;
+}
+
 static virHypervisorDriver qemuHypervisorDriver = {
 .name = QEMU_DRIVER_NAME,
 .connectOpen = qemuConnectOpen, /* 0.2.0 */
@@ -20272,6 +20295,7 @@ static virHypervisorDriver qemuHypervisorDriver = {
 .domainInterfaceAddresses = qemuDomainInterfaceAddresses, /* 1.2.14 */
 .domainSetUserPassword = qemuDomainSetUserPassword, /* 1.2.16 */
 .domainRename = qemuDomainRename, /* 1.2.19 */
+.domainMigrateOpenTunnel = qemuDomainMigrateOpenTunnel, /* 1.2.XX */
 };
 
 
-- 
1.9.1

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

[libvirt] [PATCH 19/21] qemu: migration: dest: qemuMigrationOpenNBDTunnel

[Pavel Boldin]

Add qemuMigrationOpenNBDTunnel that connects a remote stream to the
local NBD UNIX socket.

Signed-off-by: Pavel Boldin 
---
 src/qemu/qemu_migration.c | 29 +
 1 file changed, 29 insertions(+)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 4708387..27c1acb 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3605,6 +3605,32 @@ qemuMigrationPrepareTunnel(virQEMUDriverPtr driver,
 }
 
 
+static int
+qemuMigrationOpenNBDTunnel(virQEMUDriverPtr driver,
+   virStreamPtr st,
+   const char *name)
+{
+char *tunnelName = NULL;
+int ret = -1;
+virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+
+if (virAsprintf(&tunnelName,
+"%s/domain-%s/qemu.nbdtunnelmigrate.src",
+cfg->libDir, name) < 0)
+goto cleanup;
+
+if (virFDStreamConnectUNIX(st, tunnelName, false) < 0)
+goto cleanup;
+
+ret = 0;
+
+ cleanup:
+VIR_FREE(tunnelName);
+virObjectUnref(cfg);
+return ret;
+}
+
+
 int
 qemuMigrationOpenTunnel(virQEMUDriverPtr driver,
 virConnectPtr dconn,
@@ -3621,6 +3647,9 @@ qemuMigrationOpenTunnel(virQEMUDriverPtr driver,
 return -1;
 }
 
+if (flags & VIR_MIGRATE_TUNNEL_NBD)
+return qemuMigrationOpenNBDTunnel(driver, st, def->name);
+
 return 0;
 }
 
-- 
1.9.1

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

[libvirt] [PATCH 21/21] apparmor: fix tunnelmigrate permissions

[Pavel Boldin]

Signed-off-by: Pavel Boldin 
---
 src/security/virt-aa-helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 5de56e5..87af98f 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -1367,9 +1367,9 @@ main(int argc, char **argv)
   LOCALSTATEDIR, ctl->def->name);
 virBufferAsprintf(&buf, "  \"/run/libvirt/**/%s.pid\" rwk,\n",
   ctl->def->name);
-virBufferAsprintf(&buf, "  
\"%s/run/libvirt/**/*.tunnelmigrate.dest.%s\" rw,\n",
+virBufferAsprintf(&buf, "  
\"%s/lib/libvirt/qemu/domain-%s/*tunnelmigrate.src\" rw,\n",
   LOCALSTATEDIR, ctl->def->name);
-virBufferAsprintf(&buf, "  
\"/run/libvirt/**/*.tunnelmigrate.dest.%s\" rw,\n",
+virBufferAsprintf(&buf, "  
\"/lib/libvirt/qemu/domain-%s/*tunnelmigrate.src\" rw,\n",
   ctl->def->name);
 }
 if (ctl->files)
-- 
1.9.1

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

[libvirt] [PATCH 17/21] qemu: migration: dest: qemuMigrationOpenTunnel

[Pavel Boldin]

Introduce an auxiliary handler domainMigrateOpenTunnel for QEMU.

Signed-off-by: Pavel Boldin 
---
 src/qemu/qemu_migration.c | 19 +++
 src/qemu/qemu_migration.h |  6 ++
 2 files changed, 25 insertions(+)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 303cd47..4708387 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3605,6 +3605,25 @@ qemuMigrationPrepareTunnel(virQEMUDriverPtr driver,
 }
 
 
+int
+qemuMigrationOpenTunnel(virQEMUDriverPtr driver,
+virConnectPtr dconn,
+virStreamPtr st,
+virDomainDefPtr def,
+unsigned long flags)
+{
+VIR_DEBUG("driver=%p, dconn=%p, st=%p, def=%p, flags=%lx",
+  driver, dconn, st, def, flags);
+
+if (st == NULL) {
+virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+   _("opening a tunnel requested but NULL stream passed"));
+return -1;
+}
+
+return 0;
+}
+
 static virURIPtr
 qemuMigrationParseURI(const char *uri, bool *wellFormed)
 {
diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h
index 8175f4b..f91791e 100644
--- a/src/qemu/qemu_migration.h
+++ b/src/qemu/qemu_migration.h
@@ -121,6 +121,12 @@ int qemuMigrationPrepareTunnel(virQEMUDriverPtr driver,
const char *origname,
unsigned long flags);
 
+int qemuMigrationOpenTunnel(virQEMUDriverPtr driver,
+virConnectPtr dconn,
+virStreamPtr st,
+virDomainDefPtr def,
+unsigned long flags);
+
 int qemuMigrationPrepareDirect(virQEMUDriverPtr driver,
virConnectPtr dconn,
const char *cookiein,
-- 
1.9.1

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

[libvirt] [PATCH 13/21] qemu: migration: src: qemuNBDTunnelAcceptAndPipe

[Pavel Boldin]

Add qemuNBDTunnelAcceptAndPipe function that is called to handle POLLIN
on the UNIX socket connection from the QEMU's NBD server.

The function creates a pipe of a remote stream connected to the QEMU
NBD Unix socket on destination and a local stream connected to
the incoming connection from the source QEMU's NBD.

Signed-off-by: Pavel Boldin 
---
 src/qemu/qemu_migration.c | 134 +-
 1 file changed, 132 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 0682fd8..0f35c13 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3987,6 +3987,9 @@ struct _qemuMigrationSpec {
 
 #define TUNNEL_SEND_BUF_SIZE 65536
 
+typedef struct _qemuMigrationPipe qemuMigrationPipe;
+typedef qemuMigrationPipe *qemuMigrationPipePtr;
+
 typedef struct _qemuMigrationIOThread qemuMigrationIOThread;
 typedef qemuMigrationIOThread *qemuMigrationIOThreadPtr;
 struct _qemuMigrationIOThread {
@@ -3997,9 +4000,124 @@ struct _qemuMigrationIOThread {
 virError err;
 int wakeupRecvFD;
 int wakeupSendFD;
+qemuMigrationPipePtr pipes;
+virConnectPtr dconn;
+unsigned char uuid[VIR_UUID_BUFLEN];
+};
+
+struct _qemuMigrationPipe {
+qemuMigrationPipePtr next;
+qemuMigrationIOThreadPtr data;
+virStreamPtr local;
+virStreamPtr remote;
 };
 
 static void
+qemuMigrationPipeClose(qemuMigrationPipePtr pipe, bool abort)
+{
+virStreamEventUpdateCallback(pipe->local, 0);
+virStreamEventUpdateCallback(pipe->remote, 0);
+
+if (abort) {
+virStreamAbort(pipe->local);
+virStreamAbort(pipe->remote);
+} else {
+virStreamFinish(pipe->local);
+virStreamFinish(pipe->remote);
+}
+
+virObjectUnref(pipe->local);
+virObjectUnref(pipe->remote);
+}
+
+static qemuMigrationPipePtr
+qemuMigrationPipeCreate(virStreamPtr local, virStreamPtr remote)
+{
+qemuMigrationPipePtr pipe = NULL;
+
+if (VIR_ALLOC(pipe) < 0)
+goto error;
+
+pipe->local = local;
+pipe->remote = remote;
+
+return pipe;
+
+ error:
+virStreamEventRemoveCallback(local);
+virStreamEventRemoveCallback(remote);
+VIR_FREE(pipe);
+return NULL;
+}
+
+
+static int
+qemuNBDTunnelAcceptAndPipe(qemuMigrationIOThreadPtr data)
+{
+int fd, ret;
+virStreamPtr local = NULL, remote = NULL;
+qemuMigrationPipePtr pipe = NULL;
+
+while ((fd = accept(data->unixSock, NULL, NULL)) < 0) {
+if (errno == EAGAIN || errno == EINTR)
+continue;
+virReportSystemError(
+errno, "%s", _("failed to accept connection from qemu"));
+goto abrt;
+}
+
+if (!(local = virStreamNew(data->dconn, VIR_STREAM_NONBLOCK)))
+goto abrt;
+
+if (!(remote = virStreamNew(data->dconn, VIR_STREAM_NONBLOCK)))
+goto abrt;
+
+ret = virDomainMigrateOpenTunnel(data->dconn,
+ remote,
+ data->uuid,
+ VIR_MIGRATE_TUNNEL_NBD);
+
+if (ret < 0)
+goto abrt;
+
+if (virFDStreamOpen(local, fd) < 0)
+goto abrt;
+
+if (!(pipe = qemuMigrationPipeCreate(local, remote)))
+goto abrt;
+
+pipe->data = data;
+pipe->next = data->pipes;
+data->pipes = pipe;
+
+return 0;
+
+ abrt:
+VIR_FORCE_CLOSE(fd);
+virStreamAbort(local);
+virStreamAbort(remote);
+
+virObjectUnref(local);
+virObjectUnref(remote);
+return -1;
+}
+
+static void
+qemuMigrationPipesStop(qemuMigrationPipePtr pipe, bool abort)
+{
+qemuMigrationPipePtr tmp;
+
+while (pipe) {
+tmp = pipe->next;
+
+qemuMigrationPipeClose(pipe, abort);
+VIR_FREE(pipe);
+
+pipe = tmp;
+}
+}
+
+static void
 qemuMigrationIOFunc(void *arg)
 {
 qemuMigrationIOThreadPtr data = arg;
@@ -4081,9 +4199,14 @@ qemuMigrationIOFunc(void *arg)
 break;
 }
 }
+
+if (fds[2].revents & (POLLIN | POLLERR | POLLHUP) &&
+qemuNBDTunnelAcceptAndPipe(data) < 0)
+goto abrt;
 }
 
 virStreamFinish(data->qemuStream);
+qemuMigrationPipesStop(data->pipes, false);
 
 VIR_FORCE_CLOSE(data->qemuSock);
 VIR_FREE(buffer);
@@ -4097,6 +4220,7 @@ qemuMigrationIOFunc(void *arg)
 err = NULL;
 }
 virStreamAbort(data->qemuStream);
+qemuMigrationPipesStop(data->pipes, true);
 if (err) {
 virSetError(err);
 virFreeError(err);
@@ -4114,7 +4238,9 @@ qemuMigrationIOFunc(void *arg)
 
 
 static qemuMigrationIOThreadPtr
-qemuMigrationStartTunnel(virStreamPtr qemuStream)
+qemuMigrationStartTunnel(virStreamPtr qemuStream,
+ virConnectPtr dconn,
+ unsigned char uuid[VIR_UUID_BUFLEN])
 {
 qemuMigrationIOThreadPtr io = NULL;
 int wakeupFD[2] = { -1, -1 };
@@ -4132,6 +4258,8 @@ qemuMigrationStartTunnel(virStreamPtr qemuStream)
 io->qemuSock = 

Re: [libvirt] [PATCH v3 3/8] libxl: implement virDomainInterfaceStats

[Joao Martins]

On 11/17/2015 11:38 PM, Jim Fehlig wrote:
> Joao Martins wrote:
>>
>> On 11/17/2015 02:48 AM, Jim Fehlig wrote:
>>> On 11/13/2015 06:14 AM, Joao Martins wrote:
 Introduce support for domainInterfaceStats API call for querying
 network interface statistics. Consequently it also enables the
 use of `virsh domifstat  ` command.

 After succesful guest creation we fill the network
 interfaces names based on domain, device id and append suffix
 if it's emulated in the following form: vif.[-emu]. Because
 we need the devid from domain config (which is filled by libxl on domain
 create)  we cannot do generate the names in console callback.
>>> Bummer, but see below.
>>>
  On domain
 cleanup we also clear ifname, in case it was set by libvirt (i.e.
 being prefixed with "vif"). We also skip these two steps in case the name
 of the interface was manually inserted by the adminstrator.

 For getting the interface statistics we resort to virNetInterfaceStats
 and let libvirt handle the platform specific nits. Note that the latter
 is not yet supported in FreeBSD.

 Signed-off-by: Joao Martins 
 ---
 Changes since v2:
  - Clear ifname if it's autogenerated, since otherwise will persist
  on successive domain starts. Change commit message reflecting this
  change.

 Changes since v1:
  - Fill .ifname after domain start with generated
  name from libxl  based on domain id and devid returned by libxl.
  After that path validation don interfaceStats is enterily based
  on ifname pretty much like the other drivers.
  - Modify commit message reflecting the changes mentioned in
  the previous item.
  - Bump version to 1.2.22
 ---
  src/libxl/libxl_domain.c | 26 ++
  src/libxl/libxl_driver.c | 48 
 
  2 files changed, 74 insertions(+)

 diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
 index 40dcea1..adb4563 100644
 --- a/src/libxl/libxl_domain.c
 +++ b/src/libxl/libxl_domain.c
 @@ -728,6 +728,17 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
  }
  }
  
 +if ((vm->def->nnets)) {
 +ssize_t i;
 +
 +for (i = 0; i < vm->def->nnets; i++) {
 +virDomainNetDefPtr net = vm->def->nets[i];
 +
 +if (STRPREFIX(net->ifname, "vif"))
 +VIR_FREE(net->ifname);
>>> Would not be nice if user-specified ifname started with "vif" :-).
>>>
>> I think QEMU they do in a similar way, in case, specified interfaces started
>> with a prefix that is solely for autogenerated interface naming.
> 
> Ah, right. Same with bhyve and UML drivers.
> 
>> Would you
>> prefer removing it? The problem with removing this is that ifname would 
>> persist
>> on the XML, and future domainStart would use the old value.
> 
> Yep, understood. Fine to leave it.
> 
OK.

>>
 +}
 +}
 +
  if (virAsprintf(&file, "%s/%s.xml", cfg->stateDir, vm->def->name) > 
 0) {
  if (unlink(file) < 0 && errno != ENOENT && errno != ENOTDIR)
  VIR_DEBUG("Failed to remove domain XML for %s", 
 vm->def->name);
 @@ -901,6 +912,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, 
 virDomainObjPtr vm,
  virDomainDefPtr def = NULL;
  virObjectEventPtr event = NULL;
  libxlSavefileHeader hdr;
 +ssize_t i;
  int ret = -1;
  uint32_t domid = 0;
  char *dom_xml = NULL;
 @@ -1023,6 +1035,20 @@ libxlDomainStart(libxlDriverPrivatePtr driver, 
 virDomainObjPtr vm,
   */
  vm->def->id = domid;
  
 +for (i = 0; i < vm->def->nnets; i++) {
 +virDomainNetDefPtr net = vm->def->nets[i];
 +libxl_device_nic *x_nic = &d_config.nics[i];
 +const char *suffix =
 +x_nic->nictype != LIBXL_NIC_TYPE_VIF ? "-emu" : "";
 +
 +if (net->ifname)
 +continue;
 +
 +if (virAsprintf(&net->ifname, "vif%d.%d%s",
 +domid, x_nic->devid, suffix) < 0)
 +continue;
 +}
 +
>>> This could be done in the callback, if we added the libxl_domain_config 
>>> object
>>> to the libxlDomainObjPrivate struct. Currently we create, use, and dispose 
>>> the
>>> object in libxlDomainStart, but it would probably be useful keep the object
>>> around while the domain is active.
>> That's a good idea. This chunk would definitely be better placed in 
>> ConsoleCallback.
> 
> Agreed, with ConsoleCallback being renamed to something a bit more generic 
> like
> DomainStartCallback or similar.
> 
OK.

>>
>>> Actually, you could directly use the object
>>> in libxlDomainInterfaceStats if it was included in the libxlDomainObjPrivate
>>> struct. I realize it is a bit more work

[libvirt] [PATCH 05/21] domain: add virDomainMigrateOpenTunnel

[Pavel Boldin]

Add auxiliary private function that calls the apropriate driver's
domainMigrateOpenTunnel function.

Signed-off-by: Pavel Boldin 
---
 docs/apibuild.py |  1 +
 docs/hvsupport.pl|  1 +
 src/libvirt-domain.c | 43 +++
 src/libvirt_internal.h   |  6 ++
 src/libvirt_private.syms |  1 +
 5 files changed, 52 insertions(+)

diff --git a/docs/apibuild.py b/docs/apibuild.py
index f934fb2..6e60093 100755
--- a/docs/apibuild.py
+++ b/docs/apibuild.py
@@ -102,6 +102,7 @@ ignored_functions = {
   "virDomainMigratePrepare3Params": "private function for migration",
   "virDomainMigrateConfirm3Params": "private function for migration",
   "virDomainMigratePrepareTunnel3Params": "private function for tunnelled 
migration",
+  "virDomainMigrateOpenTunnel": "private function for tunnelled migration",
   "virErrorCopyNew": "private",
 }
 
diff --git a/docs/hvsupport.pl b/docs/hvsupport.pl
index 44a30ce..3b6ee65 100755
--- a/docs/hvsupport.pl
+++ b/docs/hvsupport.pl
@@ -200,6 +200,7 @@ $apis{virDomainMigratePerform3Params}->{vers} = "1.1.0";
 $apis{virDomainMigrateFinish3Params}->{vers} = "1.1.0";
 $apis{virDomainMigrateConfirm3Params}->{vers} = "1.1.0";
 
+$apis{virDomainMigrateOpenTunnel}->{vers} = "1.2.XX";
 
 
 # Now we want to get the mapping between public APIs
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index de7eb04..3037c01 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -11595,3 +11595,46 @@ virDomainInterfaceFree(virDomainInterfacePtr iface)
 
 VIR_FREE(iface);
 }
+
+
+/*
+ * Not for public use.  This function is part of the internal
+ * implementation of migration in the remote case.
+ */
+int
+virDomainMigrateOpenTunnel(virConnectPtr conn,
+   virStreamPtr st,
+   unsigned char uuid[VIR_UUID_BUFLEN],
+   unsigned int flags)
+{
+char uuidstr[VIR_UUID_STRING_BUFLEN];
+
+virUUIDFormat(uuid, uuidstr);
+VIR_DEBUG("conn=%p, stream=%p, uuid=%s, flags=%x",
+  conn, st, uuidstr, flags);
+
+virResetLastError();
+
+virCheckConnectReturn(conn, -1);
+virCheckReadOnlyGoto(conn->flags, error);
+
+if (conn != st->conn) {
+virReportInvalidArg(conn, "%s",
+_("conn must match stream connection"));
+goto error;
+}
+
+if (conn->driver->domainMigrateOpenTunnel) {
+int rv;
+rv = conn->driver->domainMigrateOpenTunnel(conn, st, uuid, flags);
+if (rv < 0)
+goto error;
+return rv;
+}
+
+virReportUnsupportedError();
+
+ error:
+virDispatchError(conn);
+return -1;
+}
diff --git a/src/libvirt_internal.h b/src/libvirt_internal.h
index 1313b58..bbfba0b 100644
--- a/src/libvirt_internal.h
+++ b/src/libvirt_internal.h
@@ -289,4 +289,10 @@ virTypedParameterValidateSet(virConnectPtr conn,
  virTypedParameterPtr params,
  int nparams);
 
+int
+virDomainMigrateOpenTunnel(virConnectPtr conn,
+   virStreamPtr st,
+   unsigned char uuid[VIR_UUID_BUFLEN],
+   unsigned int flags);
+
 #endif
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a835f18..cf5725c 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -952,6 +952,7 @@ virDomainMigrateFinish;
 virDomainMigrateFinish2;
 virDomainMigrateFinish3;
 virDomainMigrateFinish3Params;
+virDomainMigrateOpenTunnel;
 virDomainMigratePerform;
 virDomainMigratePerform3;
 virDomainMigratePerform3Params;
-- 
1.9.1

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

[libvirt] [PATCH 12/21] qemu: migration: src: add NBD unixSock to iothread

[Pavel Boldin]

Pass UNIX socket used as a local NBD server destination to the
migration iothread.

Signed-off-by: Pavel Boldin 
---
 src/qemu/qemu_migration.c | 46 ++
 1 file changed, 38 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 61e78c5..0682fd8 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3993,6 +3993,7 @@ struct _qemuMigrationIOThread {
 virThread thread;
 virStreamPtr qemuStream;
 int qemuSock;
+int unixSock;
 virError err;
 int wakeupRecvFD;
 int wakeupSendFD;
@@ -4003,7 +4004,7 @@ qemuMigrationIOFunc(void *arg)
 {
 qemuMigrationIOThreadPtr data = arg;
 char *buffer = NULL;
-struct pollfd fds[2];
+struct pollfd fds[3];
 int timeout = -1;
 virErrorPtr err = NULL;
 
@@ -4013,8 +4014,8 @@ qemuMigrationIOFunc(void *arg)
 goto abrt;
 
 fds[0].fd = data->wakeupRecvFD;
-fds[1].fd = -1;
-fds[0].events = fds[1].events = POLLIN;
+fds[1].fd = fds[2].fd = -1;
+fds[0].events = fds[1].events = fds[2].events = POLLIN;
 
 for (;;) {
 int ret;
@@ -4057,7 +4058,9 @@ qemuMigrationIOFunc(void *arg)
 break;
 case 'u':
 fds[1].fd = data->qemuSock;
-VIR_DEBUG("qemuSock set %d", data->qemuSock);
+fds[2].fd = data->unixSock;
+VIR_DEBUG("qemuSock set %d, unixSock set %d",
+  data->qemuSock, data->unixSock);
 break;
 }
 }
@@ -4126,7 +4129,7 @@ qemuMigrationStartTunnel(virStreamPtr qemuStream)
 goto error;
 
 io->qemuStream = qemuStream;
-io->qemuSock = -1;
+io->qemuSock = io->unixSock = -1;
 io->wakeupRecvFD = wakeupFD[0];
 io->wakeupSendFD = wakeupFD[1];
 
@@ -4202,6 +4205,26 @@ qemuMigrationSetQEMUSocket(qemuMigrationIOThreadPtr io, 
int sock)
 }
 
 static int
+qemuMigrationSetUnixSocket(qemuMigrationIOThreadPtr io, int sock)
+{
+int rv = -1;
+char action = 'u';
+
+io->unixSock = sock;
+
+if (safewrite(io->wakeupSendFD, &action, 1) != 1) {
+virReportSystemError(errno, "%s",
+ _("failed to update migration tunnel"));
+goto error;
+}
+
+rv = 0;
+
+ error:
+return rv;
+}
+
+static int
 qemuMigrationConnect(virQEMUDriverPtr driver,
  virDomainObjPtr vm,
  qemuMigrationSpecPtr spec)
@@ -4313,6 +4336,16 @@ qemuMigrationRun(virQEMUDriverPtr driver,
 if (qemuDomainMigrateGraphicsRelocate(driver, vm, mig, graphicsuri) < 0)
 VIR_WARN("unable to provide data for graphics client relocation");
 
+if (spec->fwdType != MIGRATION_FWD_DIRECT) {
+if (!(iothread = qemuMigrationStartTunnel(spec->fwd.stream)))
+goto cancel;
+
+if (nmigrate_disks &&
+qemuMigrationSetUnixSocket(iothread,
+   spec->nbd_tunnel_unix_socket.sock) < 0)
+goto cancel;
+}
+
 if (migrate_flags & (QEMU_MONITOR_MIGRATE_NON_SHARED_DISK |
  QEMU_MONITOR_MIGRATE_NON_SHARED_INC)) {
 bool dest_host = spec->destType == MIGRATION_DEST_HOST;
@@ -,9 +4477,6 @@ qemuMigrationRun(virQEMUDriverPtr driver,
 }
 
 if (spec->fwdType != MIGRATION_FWD_DIRECT) {
-if (!(iothread = qemuMigrationStartTunnel(spec->fwd.stream)))
-goto cancel;
-
 if (qemuMigrationSetQEMUSocket(iothread, fd) < 0)
 goto cancel;
 /* If we've created a tunnel, then the 'fd' will be closed in the
-- 
1.9.1

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

[libvirt] [PATCH 06/21] domain: add virDomainMigrateTunnelFlags

[Pavel Boldin]

Add virDomainMigrateTunnelFlags enum.

Signed-off-by: Pavel Boldin 
---
 include/libvirt/libvirt-domain.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index a1ea6a5..444deee 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -661,6 +661,9 @@ typedef enum {
 VIR_MIGRATE_RDMA_PIN_ALL  = (1 << 14), /* RDMA memory pinning */
 } virDomainMigrateFlags;
 
+typedef enum {
+VIR_MIGRATE_TUNNEL_NBD= (1 << 0), /* open tunnel for NBD */
+} virDomainMigrateTunnelFlags;
 
 /**
  * VIR_MIGRATE_PARAM_URI:
-- 
1.9.1

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

[libvirt] [PATCH 10/21] qemu: migration: src: qemu `drive-mirror` to UNIX

[Pavel Boldin]

Make qemuMigrationDriveMirror able to instruct QEMU to connect to
a local UNIX socket used for tunnelling.

Signed-off-by: Pavel Boldin 
---
 src/qemu/qemu_migration.c | 45 ++---
 1 file changed, 30 insertions(+), 15 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index d587c56..d95cd66 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2048,7 +2048,8 @@ static int
 qemuMigrationDriveMirror(virQEMUDriverPtr driver,
  virDomainObjPtr vm,
  qemuMigrationCookiePtr mig,
- const char *host,
+ bool dest_host,
+ const char *dest,
  unsigned long speed,
  unsigned int *migrate_flags,
  size_t nmigrate_disks,
@@ -2057,7 +2058,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
 {
 qemuDomainObjPrivatePtr priv = vm->privateData;
 int ret = -1;
-int port;
+int port = -1;
 size_t i;
 char *diskAlias = NULL;
 char *nbd_dest = NULL;
@@ -2068,16 +2069,20 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
 
 VIR_DEBUG("Starting drive mirrors for domain %s", vm->def->name);
 
-/* steal NBD port and thus prevent its propagation back to destination */
-port = mig->nbd->port;
-mig->nbd->port = 0;
+virCheckNonNullArgGoto(dest, cleanup);
+
+if (dest_host) {
+/* steal NBD port and thus prevent its propagation back to destination 
*/
+port = mig->nbd->port;
+mig->nbd->port = 0;
 
-/* escape literal IPv6 address */
-if (strchr(host, ':')) {
-if (virAsprintf(&hoststr, "[%s]", host) < 0)
+/* escape literal IPv6 address */
+if (strchr(dest, ':')) {
+if (virAsprintf(&hoststr, "[%s]", dest) < 0)
+goto cleanup;
+} else if (VIR_STRDUP(hoststr, dest) < 0) {
 goto cleanup;
-} else if (VIR_STRDUP(hoststr, host) < 0) {
-goto cleanup;
+}
 }
 
 if (*migrate_flags & QEMU_MONITOR_MIGRATE_NON_SHARED_INC)
@@ -2092,11 +2097,18 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
 if (!qemuMigrateDisk(disk, nmigrate_disks, migrate_disks))
 continue;
 
-if ((virAsprintf(&diskAlias, "%s%s",
- QEMU_DRIVE_HOST_PREFIX, disk->info.alias) < 0) ||
-(virAsprintf(&nbd_dest, "nbd:%s:%d:exportname=%s",
- hoststr, port, diskAlias) < 0))
+if (virAsprintf(&diskAlias, "%s%s",
+QEMU_DRIVE_HOST_PREFIX, disk->info.alias) < 0)
 goto cleanup;
+if (dest_host) {
+if (virAsprintf(&nbd_dest, "nbd:%s:%d:exportname=%s",
+hoststr, port, diskAlias) < 0)
+goto cleanup;
+} else {
+if (virAsprintf(&nbd_dest, "nbd:unix:%s:exportname=%s",
+dest, diskAlias) < 0)
+goto cleanup;
+}
 
 if (qemuDomainObjEnterMonitorAsync(driver, vm,
QEMU_ASYNC_JOB_MIGRATION_OUT) < 0)
@@ -4281,10 +4293,13 @@ qemuMigrationRun(virQEMUDriverPtr driver,
 
 if (migrate_flags & (QEMU_MONITOR_MIGRATE_NON_SHARED_DISK |
  QEMU_MONITOR_MIGRATE_NON_SHARED_INC)) {
+bool dest_host = spec->destType == MIGRATION_DEST_HOST;
+const char *dest = dest_host ? spec->dest.host.name :
+   spec->nbd_tunnel_unix_socket.file;
 if (mig->nbd) {
 /* This will update migrate_flags on success */
 if (qemuMigrationDriveMirror(driver, vm, mig,
- spec->dest.host.name,
+ dest_host, dest,
  migrate_speed,
  &migrate_flags,
  nmigrate_disks,
-- 
1.9.1

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

[libvirt] [PATCH 08/21] qemu: migration: src: add nbd tunnel socket data

[Pavel Boldin]

Add local NBD tunnel socket info to the qemuMigrationSpec structure.

Signed-off-by: Pavel Boldin 
---
 src/qemu/qemu_migration.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 3eee3a5..fb2a216 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3966,6 +3966,11 @@ struct _qemuMigrationSpec {
 union {
 virStreamPtr stream;
 } fwd;
+
+struct {
+char *file;
+int sock;
+} nbd_tunnel_unix_socket;
 };
 
 #define TUNNEL_SEND_BUF_SIZE 65536
-- 
1.9.1

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

[libvirt] [PATCH 03/21] remote_driver: introduce virRemoteClientNew

[Pavel Boldin]

Move common code to a function.

Signed-off-by: Pavel Boldin 
---
 src/remote/remote_driver.c | 48 +++---
 1 file changed, 28 insertions(+), 20 deletions(-)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index a1dd640..b72cb86 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -6119,6 +6119,28 @@ remoteDomainMigratePrepare3(virConnectPtr dconn,
 goto done;
 }
 
+static virNetClientStreamPtr
+virRemoteClientOpen(virStreamPtr st,
+struct private_data *priv,
+enum remote_procedure proc)
+{
+virNetClientStreamPtr netst;
+
+if (!(netst = virNetClientStreamNew(priv->remoteProgram,
+proc,
+priv->counter)))
+return NULL;
+
+if (virNetClientAddStream(priv->client, netst) < 0) {
+virObjectUnref(netst);
+return NULL;
+}
+
+st->driver = &remoteStreamDrv;
+st->privateData = netst;
+
+return netst;
+}
 
 static int
 remoteDomainMigratePrepareTunnel3(virConnectPtr dconn,
@@ -6143,18 +6165,11 @@ remoteDomainMigratePrepareTunnel3(virConnectPtr dconn,
 memset(&args, 0, sizeof(args));
 memset(&ret, 0, sizeof(ret));
 
-if (!(netst = virNetClientStreamNew(priv->remoteProgram,
-
REMOTE_PROC_DOMAIN_MIGRATE_PREPARE_TUNNEL3,
-priv->counter)))
-goto done;
+netst = virRemoteClientOpen(st, priv,
+REMOTE_PROC_DOMAIN_MIGRATE_PREPARE_TUNNEL3);
 
-if (virNetClientAddStream(priv->client, netst) < 0) {
-virObjectUnref(netst);
+if (netst == NULL)
 goto done;
-}
-
-st->driver = &remoteStreamDrv;
-st->privateData = netst;
 
 args.cookie_in.cookie_in_val = (char *)cookiein;
 args.cookie_in.cookie_in_len = cookieinlen;
@@ -7193,18 +7208,11 @@ remoteDomainMigratePrepareTunnel3Params(virConnectPtr 
dconn,
 goto cleanup;
 }
 
-if (!(netst = virNetClientStreamNew(priv->remoteProgram,
-
REMOTE_PROC_DOMAIN_MIGRATE_PREPARE_TUNNEL3_PARAMS,
-priv->counter)))
-goto cleanup;
+netst = virRemoteClientOpen(st, priv,
+
REMOTE_PROC_DOMAIN_MIGRATE_PREPARE_TUNNEL3_PARAMS);
 
-if (virNetClientAddStream(priv->client, netst) < 0) {
-virObjectUnref(netst);
+if (netst == NULL)
 goto cleanup;
-}
-
-st->driver = &remoteStreamDrv;
-st->privateData = netst;
 
 if (call(dconn, priv, 0, REMOTE_PROC_DOMAIN_MIGRATE_PREPARE_TUNNEL3_PARAMS,
  (xdrproc_t) xdr_remote_domain_migrate_prepare_tunnel3_params_args,
-- 
1.9.1

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

Re: [libvirt] [PATCH v3 2/8] libxl: implement virDomainMemorystats

[Joao Martins]

On 11/17/2015 11:15 PM, Jim Fehlig wrote:
> Joao Martins wrote:
>> Introduce support for domainMemoryStats API call, which
>> consequently enables the use of `virsh dommemstat` command to
>> query for memory statistics of a domain. We support
>> the following statistics: balloon info, available and currently
>> in use. swap-in, swap-out, major-faults, minor-faults require
>> cooperation of the guest and thus currently not supported.
>>
>> We build on the data returned from libxl_domain_info and deliver
>> it in the virDomainMemoryStat format.
>>
>> Signed-off-by: Joao Martins 
>> ---
>> Changes since v1:
>>  - Cleanup properly after error fetching domain stats
>>  - Dispose libxl_dominfo after succesfull call to
>>  libxl_domain_info()
>>  - Bump version to 1.2.22
>> ---
>>  src/libxl/libxl_driver.c | 70 
>> 
>>  1 file changed, 70 insertions(+)
>>
>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>> index 50f6e34..f4fc7bc 100644
>> --- a/src/libxl/libxl_driver.c
>> +++ b/src/libxl/libxl_driver.c
>> @@ -4749,6 +4749,75 @@ libxlDomainGetCPUStats(virDomainPtr dom,
>>  return ret;
>>  }
>>  
>> +#define LIBXL_SET_MEMSTAT(TAG, VAL) \
>> +if (i < nr_stats) { \
>> +stats[i].tag = TAG; \
>> +stats[i].val = VAL; \
>> +i++; \
>> +}
>> +
>> +static int
>> +libxlDomainMemoryStats(virDomainPtr dom,
>> +   virDomainMemoryStatPtr stats,
>> +   unsigned int nr_stats,
>> +   unsigned int flags)
>> +{
>> +libxlDriverPrivatePtr driver = dom->conn->privateData;
>> +libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
>> +virDomainObjPtr vm;
>> +libxl_dominfo d_info;
>> +unsigned mem, maxmem;
>> +size_t i = 0;
>> +int ret = -1;
>> +
>> +virCheckFlags(0, -1);
>> +
>> +if (!(vm = libxlDomObjFromDomain(dom)))
>> +goto cleanup;
>> +
>> +if (virDomainMemoryStatsEnsureACL(dom->conn, vm->def) < 0)
>> +goto cleanup;
>> +
>> +if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_QUERY) < 0)
>> +goto cleanup;
>> +
>> +if (!virDomainObjIsActive(vm)) {
>> +virReportError(VIR_ERR_OPERATION_INVALID,
>> +   "%s", _("domain is not running"));
>> +goto endjob;
>> +}
>> +
>> +if (libxl_domain_info(cfg->ctx, &d_info, vm->def->id) != 0) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>> +   _("libxl_domain_info failed for domain '%d'"),
>> +   vm->def->id);
>> +goto endjob;
>> +}
>> +mem = d_info.current_memkb;
>> +maxmem = d_info.max_memkb;
>> +
>> +LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, maxmem - mem);
> 
> Should this just be 'mem'? On a domain with
> 
>   1048576
>   1048576
> 
> I see
> 
> # virsh dommemstat test
> actual 1024
> available 1049600
> rss 1048576
> 
> The documentation for VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON only says "Current
> balloon value (in KB)". I (perhaps incorrectly) interpret that as the amount 
> of
> memory currently assigned to the domain.
I interpreted that it based on the size of the balloon instead of based on the
memory. But trying out with plain qemu (to see what the monitor socket returns,
since qemu driver takes those values directly) and also libvirt+qemu and it gave
me what you mentioned. VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON is indeed the
amount of memory currently assigned to the domain. Apologies for the confusion.

> 
> Also, I wonder if we should provide rss for Xen domains? I'm not sure it makes
> much sense since Xen domains are more than just a process running on the host.
> AFAIK, rss would always be set to d_info.current_memkb even if a domain was 
> not
> actually using all the memory.
I agree, but I mainly included RSS stat for the lack of a "current memory in
use". But since this is cleared and ACTUAL_BALLOON is not the size of the
balloon but to the actual memory, I guess this renders RSS stat a bit useless,
and perhaps misleading like you suggest. This is the hunk to be applied on top
of this patch, having tested already, and make {syntax-check,test}-ed.

Regards,
Joao

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index db13fd2..c1563c2 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -4799,9 +4799,8 @@ libxlDomainMemoryStats(virDomainPtr dom,
 mem = d_info.current_memkb;
 maxmem = d_info.max_memkb;

-LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, maxmem - mem);
+LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, mem);
 LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_AVAILABLE, maxmem);
-LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_RSS, mem);

 ret = i;
> 
> Regards,
> Jim
> 
>> +LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_AVAILABLE, maxmem);
>> +LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_RSS, mem);
>> +
>> +ret = i;
>> +
>> +   

Re: [libvirt] [PATCH v3 1/8] libxl: implement virDomainGetCPUStats

[Jim Fehlig]

On 11/16/2015 07:59 PM, Jim Fehlig wrote:
> On 11/13/2015 06:14 AM, Joao Martins wrote:
> @@ -5233,6 +5342,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
>  #endif
>  .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
>  .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */
> +.domainGetCPUStats = libxlDomainGetCPUStats, /* 1.2.22 */
> ACK with the above nit fixed. I'll push this one too after verifying whether
> next release is 1.2.22 or 1.3.0.

Opps, forgot to mention that I pushed this yesterday. Thanks!

Regards,
Jim

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

Re: [libvirt] [PATCH 4/6] qemu: Reduce memlock limit after detaching hostdev

[Peter Krempa]

On Wed, Nov 18, 2015 at 17:04:55 +0100, Andrea Bolognani wrote:
> On Wed, 2015-11-18 at 16:17 +0100, Peter Krempa wrote:
> > > +/* QEMU might no longer need to lock as much memory, eg. we just 
> > > detached
> > > + * a VFIO device, so adjust the limit here */
> > > +if (qemuDomainRequiresMlock(vm->def))
> > > +if (virProcessSetMaxMemLock(vm->pid,
> > > +
> > > qemuDomainGetMlockLimitBytes(vm->def)) < 0)
> > > +VIR_WARN("Failed to adjust locked memory limit");
> > > +
> > 
> > Hmmm, looks like we should reset it to default (64KiB afaik) if it was
> > required before and is not required any more. Otherwise we would not
> > decrease the limit after unplugging the last VFIO device (on x86).
> 
> I agree, and I planned to do something about that in a
> follow-up patch as this change alone is already a small
> improvement over the status quo.
> 
> Would you prefer it if I pulled this patch from the series
> for now and posted it again once it supports restoring the
> limit back to the default once the last VFIO device has been
> removed from the guest? I'd be okay with that.

Hmm, yeah, I'd prefer to do it the first time the right way.

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 6/6] qemu: Add ppc64-specific math to qemuDomainGetMlockLimitBytes()

[Peter Krempa]

On Wed, Nov 18, 2015 at 15:13:20 +0100, Andrea Bolognani wrote:
> The amount of memory a ppc64 domain might need to lock is different
> than that of a equally-sized x86 domain, so we need to check the
> domain's architecture and act accordingly.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1273480
> ---
>  src/qemu/qemu_domain.c | 80 
> +-
>  1 file changed, 79 insertions(+), 1 deletion(-)
> 

ACK, although I'd like to hear David's opinion (cc'd).

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 v2 02/13] Import stripped down virtlockd code as basis of virtlogd

[John Ferlan]

On 11/12/2015 12:18 PM, Daniel P. Berrange wrote:
> Copy the virtlockd codebase across to form the initial virlogd
> code. Simple search & replace of s/lock/log/ and gut the remote
> protocol & dispatcher. This gives us a daemon that starts up
> and listens for connections, but does nothing with them.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  .gitignore|7 +
>  cfg.mk|4 +-
>  include/libvirt/virterror.h   |1 +
>  libvirt.spec.in   |   24 +-
>  po/POTFILES.in|2 +
>  src/Makefile.am   |  169 +-
>  src/logging/log_daemon.c  | 1177 
> +
>  src/logging/log_daemon.h  |   42 ++
>  src/logging/log_daemon_config.c   |  203 +++
>  src/logging/log_daemon_config.h   |   50 ++
>  src/logging/log_daemon_dispatch.c |   37 ++
>  src/logging/log_daemon_dispatch.h |   31 +
>  src/logging/log_protocol.x|   22 +
>  src/logging/test_virtlogd.aug.in  |   12 +
>  src/logging/virtlogd.aug  |   45 ++
>  src/logging/virtlogd.conf |   67 +++
>  src/logging/virtlogd.init.in  |   94 +++
>  src/logging/virtlogd.pod.in   |  162 +
>  src/logging/virtlogd.service.in   |   17 +
>  src/logging/virtlogd.socket.in|8 +
>  src/logging/virtlogd.sysconf  |3 +
>  src/util/virerror.c   |1 +
>  22 files changed, 2156 insertions(+), 22 deletions(-)
>  create mode 100644 src/logging/log_daemon.c
>  create mode 100644 src/logging/log_daemon.h
>  create mode 100644 src/logging/log_daemon_config.c
>  create mode 100644 src/logging/log_daemon_config.h
>  create mode 100644 src/logging/log_daemon_dispatch.c
>  create mode 100644 src/logging/log_daemon_dispatch.h
>  create mode 100644 src/logging/log_protocol.x
>  create mode 100644 src/logging/test_virtlogd.aug.in
>  create mode 100644 src/logging/virtlogd.aug
>  create mode 100644 src/logging/virtlogd.conf
>  create mode 100644 src/logging/virtlogd.init.in
>  create mode 100644 src/logging/virtlogd.pod.in
>  create mode 100644 src/logging/virtlogd.service.in
>  create mode 100644 src/logging/virtlogd.socket.in
>  create mode 100644 src/logging/virtlogd.sysconf
> 

Full disclosure - the aspects of Makefiles, cfg files, spec files, etc.
- not my area of expertise... Looks like things were copied correctly
though and it does build... Whether it builds on all platforms for all
strange variants of make - I'll leave to existing build processes...

[...]

Hopefully some assumptions can be made regarding how much of this is
copied from lockd ;-)

> diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c
> new file mode 100644
> index 000..184076c
> --- /dev/null
> +++ b/src/logging/log_daemon.c
> @@ -0,0 +1,1177 @@
> +/*
> + * log_daemon.c: log management daemon
> + *
> + * Copyright (C) 2006-2015 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library;  If not, see
> + * .
> + *
> + * Author: Daniel P. Berrange 
> + */

[...]

> +static void
> +virLogDaemonFree(virLogDaemonPtr logd)
> +{
> +if (!logd)
> +return;

Should there be a virMutexDestroy(logd->lock); ?  If so, it's also
missing from lockd

> +
> +virObjectUnref(logd->srv);
> +virObjectUnref(logd->dmn);
> +
> +VIR_FREE(logd);
> +}
> +
> +

[...]

> +
> +static int
> +virLogDaemonUnixSocketPaths(bool privileged,
> +char **sockfile)
> +{
> +if (privileged) {
> +if (VIR_STRDUP(*sockfile, LOCALSTATEDIR 
> "/run/libvirt/virtlogd-sock") < 0)
> +goto error;
> +} else {
> +char *rundir = NULL;
> +mode_t old_umask;
> +
> +if (!(rundir = virGetUserRuntimeDirectory()))
> +goto error;
> +
> +old_umask = umask(077);
> +if (virFileMakePath(rundir) < 0) {
> +umask(old_umask);

VIR_FREE(rundir);  - I see this is true in lockd too...

> +goto error;
> +}
> +umask(old_umask);
> +
> +if (virAsprintf(sockfile, "%s/virtlogd-sock", rundir) < 0) {
> +VIR_FREE(rundir);
> +goto error;
> +}
> +
> +VIR_FREE(rundir);
> +}
> +return 0;
> +
> + error:
> +return -1;
> +}
> +
> +
> +static void
> +virLogDaemonErrorHandler(void *opaque ATTRIBUTE_UNUSE

Re: [libvirt] [PATCH 4/6] qemu: Reduce memlock limit after detaching hostdev

[Andrea Bolognani]

On Wed, 2015-11-18 at 16:17 +0100, Peter Krempa wrote:
> > +/* QEMU might no longer need to lock as much memory, eg. we just 
> > detached
> > + * a VFIO device, so adjust the limit here */
> > +if (qemuDomainRequiresMlock(vm->def))
> > +if (virProcessSetMaxMemLock(vm->pid,
> > +qemuDomainGetMlockLimitBytes(vm->def)) 
> > < 0)
> > +VIR_WARN("Failed to adjust locked memory limit");
> > +
> 
> Hmmm, looks like we should reset it to default (64KiB afaik) if it was
> required before and is not required any more. Otherwise we would not
> decrease the limit after unplugging the last VFIO device (on x86).

I agree, and I planned to do something about that in a
follow-up patch as this change alone is already a small
improvement over the status quo.

Would you prefer it if I pulled this patch from the series
for now and posted it again once it supports restoring the
limit back to the default once the last VFIO device has been
removed from the guest? I'd be okay with that.

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 5/6] qemu: Always set locked memory limit for ppc64 domains

[Andrea Bolognani]

On Wed, 2015-11-18 at 16:11 +0100, Peter Krempa wrote:
> On Wed, Nov 18, 2015 at 15:13:19 +0100, Andrea Bolognani wrote:
> > Unlike other architectures, ppc64 domains need to lock memory
> > even when VFIO is not used.
> > 
> > Change qemuDomainRequiresMlock() to reflect this fact.
> > ---
> >  src/qemu/qemu_domain.c | 9 +++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> ACK, but I think this should go in after 6/6 that adds the magic
> calculation for PPC.

Sure, I'll change the order before pushing.

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 6/6] qemu: Add ppc64-specific math to qemuDomainGetMlockLimitBytes()

[Andrea Bolognani]

On Wed, 2015-11-18 at 16:39 +0100, Peter Krempa wrote:
> Is there any public documentation where you were taking the info from?
> Should we link it to the code?

The information was provided by David Gibson, and AFAIU
what he did is go through the kernel / qemu code and take
note of all the bits of memory that need to be locked.

So there's no comprehensive documentation that I know of,
except for the code itself :)

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 6/6] qemu: Add ppc64-specific math to qemuDomainGetMlockLimitBytes()

[Peter Krempa]

On Wed, Nov 18, 2015 at 15:13:20 +0100, Andrea Bolognani wrote:
> The amount of memory a ppc64 domain might need to lock is different
> than that of a equally-sized x86 domain, so we need to check the
> domain's architecture and act accordingly.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1273480
> ---
>  src/qemu/qemu_domain.c | 80 
> +-
>  1 file changed, 79 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 2c0f5af..1e92b9d 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c

[...]

> +/* baseLimit := maxMemory / 128  (a)
> + *  + 4 MiB * #PHBs + 8 MiB  (b)
> + *
> + * (a) is the hash table
> + *
> + * (b) is accounting for the 32-bit DMA window - it could be either 
> the
> + * KVM accelerated TCE tables for emulated devices, or the VFIO
> + * userspace view. The 4 MiB per-PHB (including the default one) 
> covers
> + * a 2GiB DMA window: default is 1GiB, but it's possible it'll be
> + * increased to help performance. The 8 MiB extra should be plenty 
> for
> + * the TCE table index for any reasonable number of PHBs and several
> + * spapr-vlan or spapr-vscsi devices (512kB + a tiny bit each) */
> +baseLimit = maxMemory / 128 +
> +4096 * nPCIHostBridges +
> +8192;
> +
> +/* passthroughLimit := max( 2 GiB * #PHBs,   (c)
> + *  memory   (d)
> + *  + memory * 1/512 * #PHBs + 8 MiB )   (e)
> + *
> + * (c) is the pre-DDW VFIO DMA window accounting. We're allowing 2 
> GiB
> + * rather than 1 GiB
> + *
> + * (d) is the with-DDW (and memory pre-registration and related
> + * features) DMA window accounting - assuming that we only account 
> RAM
> + * once, even if mapped to multiple PHBs
> + *
> + * (e) is the with-DDW userspace view and overhead for the 64-bit DMA
> + * window. This is based a bit on expected guest behaviour, but there
> + * really isn't a way to completely avoid that. We assume the guest
> + * requests a 64-bit DMA window (per PHB) just big enough to map all
> + * its RAM. 4 kiB page size gives the 1/512; it will be less with 64
> + * kiB pages, less still if the guest is mapped with hugepages 
> (unlike
> + * the default 32-bit DMA window, DDW windows can use large IOMMU
> + * pages). 8 MiB is for second and further level overheads, like (b) 
> */
> +passthroughLimit = MAX(2 * 1024 * 1024 * nPCIHostBridges,
> +   memory +
> +   memory / 512 * nPCIHostBridges + 8192);
> +
> +if (usesVFIO)
> +memKB = baseLimit + passthroughLimit;
> +else
> +memKB = baseLimit;

Is there any public documentation where you were taking the info from?
Should we link it to the code?

Peter


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

[libvirt] [PATCH] vz: implementation of domainReboot callback

[Mikhail Feoktistov]

---
 src/vz/vz_driver.c | 8 
 src/vz/vz_sdk.c| 8 
 src/vz/vz_sdk.h| 1 +
 3 files changed, 17 insertions(+)

diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c
index 0a968b9..5f56eaf 100644
--- a/src/vz/vz_driver.c
+++ b/src/vz/vz_driver.c
@@ -923,6 +923,13 @@ static int vzDomainShutdown(virDomainPtr domain)
 return prlsdkDomainChangeState(domain, prlsdkStop);
 }
 
+static int vzDomainReboot(virDomainPtr domain,
+  unsigned int flags)
+{
+flags = flags;
+return prlsdkDomainChangeState(domain, prlsdkRestart);
+}
+
 static int vzDomainIsActive(virDomainPtr domain)
 {
 virDomainObjPtr dom = NULL;
@@ -1486,6 +1493,7 @@ static virHypervisorDriver vzDriver = {
 .domainShutdown = vzDomainShutdown, /* 0.10.0 */
 .domainCreate = vzDomainCreate,/* 0.10.0 */
 .domainCreateWithFlags = vzDomainCreateWithFlags, /* 1.2.10 */
+.domainReboot = vzDomainReboot, /* 1.2.22 */
 .domainDefineXML = vzDomainDefineXML,  /* 0.10.0 */
 .domainDefineXMLFlags = vzDomainDefineXMLFlags, /* 1.2.12 */
 .domainUndefine = vzDomainUndefine, /* 1.2.10 */
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 89c9e89..bf71e96 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -1831,6 +1831,14 @@ PRL_RESULT prlsdkSuspend(PRL_HANDLE sdkdom)
 return waitJob(job);
 }
 
+PRL_RESULT prlsdkRestart(PRL_HANDLE sdkdom)
+{
+PRL_HANDLE job = PRL_INVALID_HANDLE;
+
+job = PrlVm_Restart(sdkdom);
+return waitJob(job);
+}
+
 int
 prlsdkDomainChangeStateLocked(vzConnPtr privconn,
   virDomainObjPtr dom,
diff --git a/src/vz/vz_sdk.h b/src/vz/vz_sdk.h
index ebe4591..88ee7d9 100644
--- a/src/vz/vz_sdk.h
+++ b/src/vz/vz_sdk.h
@@ -41,6 +41,7 @@ PRL_RESULT prlsdkStop(PRL_HANDLE sdkdom);
 PRL_RESULT prlsdkPause(PRL_HANDLE sdkdom);
 PRL_RESULT prlsdkResume(PRL_HANDLE sdkdom);
 PRL_RESULT prlsdkSuspend(PRL_HANDLE sdkdom);
+PRL_RESULT prlsdkRestart(PRL_HANDLE sdkdom);
 
 typedef PRL_RESULT (*prlsdkChangeStateFunc)(PRL_HANDLE sdkdom);
 int
-- 
1.8.3.1

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

Re: [libvirt] [PATCH 3/6] qemu: Use qemuDomainRequiresMlock() when attaching PCI hostdev

[Peter Krempa]

On Wed, Nov 18, 2015 at 15:13:17 +0100, Andrea Bolognani wrote:
> The function is used everywhere else to check whether the locked
> memory limit should be set / updated, and it should be used here
> as well.
> 
> Moreover, qemuDomainGetMlockLimitBytes() expects the hostdev to
> have already been added to the domain definition, but we only do
> that at the end of qemuDomainAttachHostPCIDevice(). Work around
> the issue by adding the hostdev before adjusting the locked memory
> limit and removing it immediately afterwards.
> ---
>  src/qemu/qemu_hotplug.c | 21 +++--
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 89e5c0d..0bd88ce 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1269,18 +1269,27 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver,
>   "supported by this version of qemu"));
>  goto error;
>  }
> -
> -/* setup memory locking limits, that are necessary for VFIO */
> -if (virProcessSetMaxMemLock(vm->pid,
> -qemuDomainGetMlockLimitBytes(vm->def)) < 
> 0)
> -goto error;
> -
>  break;
>  
>  default:
>  break;
>  }
>  
> +/* Temporarily add the hostdev to the domain definition. This is needed
> + * because qemuDomainRequiresMlock() and qemuDomainGetMlockLimitBytes()
> + * require the hostdev to be already part of the domain definition, but
> + * other functions like qemuAssignDeviceHostdevAlias() used below expect
> + * it *not* to be there. A better way to handle this would be nice */
> +vm->def->hostdevs[vm->def->nhostdevs++] = hostdev;
> +if (qemuDomainRequiresMlock(vm->def)) {
> +if (virProcessSetMaxMemLock(vm->pid,
> +qemuDomainGetMlockLimitBytes(vm->def)) < 
> 0) {
> +vm->def->hostdevs[--(vm->def->nhostdevs)] = NULL;
> +goto error;
> +}
> +}
> +vm->def->hostdevs[--(vm->def->nhostdevs)] = NULL;
> +
>  if (qemuSetupHostdevCGroup(vm, hostdev) < 0)
>  goto error;
>  teardowncgroup = true;

Hmm, ugly, byt required for the PPC64 calculation in the following
patches. I guess reworking the hotplug code would be rather ugly so

relucant ACK.

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/6] process: Log when limiting the amount of locked memory

[Peter Krempa]

On Wed, Nov 18, 2015 at 15:13:15 +0100, Andrea Bolognani wrote:
> This can be useful for debugging.
> ---
>  src/util/virprocess.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> index 103c114..4b18903 100644
> --- a/src/util/virprocess.c
> +++ b/src/util/virprocess.c
> @@ -766,6 +766,10 @@ virProcessSetMaxMemLock(pid_t pid, unsigned long long 
> bytes)
>  return -1;
>  }
>  }
> +
> +VIR_DEBUG("Locked memory for process %lld limited to %llu bytes",
> +  (long long int) pid, bytes);
> +

ACK

Peter


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

[libvirt] [PATCH] vz: allow only en-us keymap for VNC

[Mikhail Feoktistov]

In virtuozzo we support only en-us keymap for VMs and containers.
If keymap is specified than check that it's en-us, otherwise
show error message.
---
 src/vz/vz_sdk.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c
index 89c9e89..526c0f2 100644
--- a/src/vz/vz_sdk.c
+++ b/src/vz/vz_sdk.c
@@ -2220,10 +2220,11 @@ static int 
prlsdkCheckGraphicsUnsupportedParams(virDomainDefPtr def)
 return -1;
 }
 
-if (gr->data.vnc.keymap != 0) {
+if (gr->data.vnc.keymap != 0 &&
+STRNEQ(gr->data.vnc.keymap, "en-us")) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("vz driver doesn't support "
- "keymap setting for VNC graphics."));
+   _("vz driver supports only "
+ "\"en-us\" keymap for VNC graphics."));
 return -1;
 }
 
-- 
1.8.3.1

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

Re: [libvirt] [PATCH 4/6] qemu: Reduce memlock limit after detaching hostdev

[Peter Krempa]

On Wed, Nov 18, 2015 at 15:13:18 +0100, Andrea Bolognani wrote:
> We increase the limit before plugging in a PCI hostdev or a memory
> module because some memory might need to be locked due to eg. VFIO.
> 
> Of course we should do the opposite after unplugging a device: this
> was already the case for memory modules, but not for hostdevs.
> ---
>  src/qemu/qemu_hotplug.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 0bd88ce..2a16f89 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -3084,6 +3084,14 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
>  networkReleaseActualDevice(vm->def, net);
>  virDomainNetDefFree(net);
>  }
> +
> +/* QEMU might no longer need to lock as much memory, eg. we just detached
> + * a VFIO device, so adjust the limit here */
> +if (qemuDomainRequiresMlock(vm->def))
> +if (virProcessSetMaxMemLock(vm->pid,
> +qemuDomainGetMlockLimitBytes(vm->def)) < 
> 0)
> +VIR_WARN("Failed to adjust locked memory limit");
> +

Hmmm, looks like we should reset it to default (64KiB afaik) if it was
required before and is not required any more. Otherwise we would not
decrease the limit after unplugging the last VFIO device (on x86).

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 5/6] qemu: Always set locked memory limit for ppc64 domains

[Peter Krempa]

On Wed, Nov 18, 2015 at 15:13:19 +0100, Andrea Bolognani wrote:
> Unlike other architectures, ppc64 domains need to lock memory
> even when VFIO is not used.
> 
> Change qemuDomainRequiresMlock() to reflect this fact.
> ---
>  src/qemu/qemu_domain.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

ACK, but I think this should go in after 6/6 that adds the magic
calculation for PPC.

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 2/6] qemu: Use qemuDomainRequiresMlock() in qemuBuildCommandLine()

[Peter Krempa]

On Wed, Nov 18, 2015 at 15:13:16 +0100, Andrea Bolognani wrote:
> This removes a duplication of the logic used to decide whether
> the memory locking limit should be set.
> ---
>  src/qemu/qemu_command.c | 9 +++--
>  1 file changed, 3 insertions(+), 6 deletions(-)

ACK,

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 v2 02/13] Import stripped down virtlockd code as basis of virtlogd

[Daniel P. Berrange]

On Wed, Nov 18, 2015 at 03:54:37PM +0100, Peter Krempa wrote:
> On Thu, Nov 12, 2015 at 17:18:59 +, Daniel Berrange wrote:
> > Copy the virtlockd codebase across to form the initial virlogd
> > code. Simple search & replace of s/lock/log/ and gut the remote
> > protocol & dispatcher. This gives us a daemon that starts up
> > and listens for connections, but does nothing with them.
> > 
> > Signed-off-by: Daniel P. Berrange 
> > ---
> 
> [...]
> 
> > diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c
> > new file mode 100644
> > index 000..184076c
> > --- /dev/null
> > +++ b/src/logging/log_daemon.c
> > @@ -0,0 +1,1177 @@
> > +/*
> > + * log_daemon.c: log management daemon
> > + *
> > + * Copyright (C) 2006-2015 Red Hat, Inc.
> 
> Um 2006? Here and in every other header.

All this code is cut+paste from the virtlockd code, which is in
turn cut+paste from libvirtd. So preserving the original
copyright lines as-is is appropriate.

> > +VIR_ENUM_DECL(virDaemonErr)
> > +VIR_ENUM_IMPL(virDaemonErr, VIR_LOG_DAEMON_ERR_LAST,
> > +  "Initialization successful",
> > +  "Unable to obtain pidfile",
> > +  "Unable to create rundir",
> > +  "Unable to initialize libvirt",
> 
> Will this need to call libvirt? Or should this be 'virtlogd'?

I think I can probably delete this error entirely - I think it
is unused.


> > +/*
> > + * Set up the logging environment
> > + * By default if daemonized all errors go to the logfile libvirtd.log,
> > + * but if verbose or error debugging is asked for then also output
> > + * informational and debug messages. Default size if 64 kB.
> 
> The logging ring buffer isn't present any more.

Hah, yeah, should be killed.



> > diff --git a/src/logging/log_protocol.x b/src/logging/log_protocol.x
> > new file mode 100644
> > index 000..9b8fa41
> > --- /dev/null
> > +++ b/src/logging/log_protocol.x
> > @@ -0,0 +1,22 @@
> > +/* -*- c -*-
> > + */
> > +
> > +%#include "internal.h"
> > +
> > +typedef opaque virLogManagerProtocolUUID[VIR_UUID_BUFLEN];
> > +
> > +/* Length of long, but not unbounded, strings.
> > + * This is an arbitrary limit designed to stop the decoder from trying
> > + * to allocate unbounded amounts of memory when fed with a bad message.
> > + */
> > +const VIR_LOG_MANAGER_PROTOCOL_STRING_MAX = 65536;
> 
> This is going to be modified in the next patch. Shouldn't you use the
> right value directly here?

Yep, good point.



> > +
> > +# The maximum number of concurrent client connections to allow
> > +# over all sockets combined.
> > +# Each running virtual machine will require one open connection
> > +# to virtlogd. So 'max_clients' will affect how many VMs can
> > +# be run on a host
> > +#max_clients = 1024
> 
> Should we mention this also in the libvirtd config file?

No, this doesn't have an impact on the max_clients requirement for
libvirtd.


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

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

Re: [libvirt] [PATCH v2 01/13] util: add APIs for reading/writing from/to rotating files

[Daniel P. Berrange]

On Wed, Nov 18, 2015 at 08:46:43AM -0500, John Ferlan wrote:
> 
> 
> On 11/12/2015 12:18 PM, Daniel P. Berrange wrote:
> > Add virRotatingFileReader and virRotatingFileWriter objects
> > which allow reading & writing from/to files with automation
> > rotation to N backup files when a size limit is reached. This
> > is useful for guest logging when a guaranteed finite size
> > limit is required. Use of external tools like logrotate is
> > inadequate since it leaves the possibility for guest to DOS
> > the host in between invokations of logrotate.
> > 
> > Signed-off-by: Daniel P. Berrange 
> > ---
> >  po/POTFILES.in  |   1 +
> >  src/Makefile.am |   1 +
> >  src/libvirt_private.syms|  13 +
> >  src/util/virrotatingfile.c  | 608 ++
> >  src/util/virrotatingfile.h  |  62 
> >  tests/Makefile.am   |   6 +
> >  tests/virrotatingfiletest.c | 698 
> > 
> >  7 files changed, 1389 insertions(+)
> >  create mode 100644 src/util/virrotatingfile.c
> >  create mode 100644 src/util/virrotatingfile.h
> >  create mode 100644 tests/virrotatingfiletest.c
> > 
> 
> [...]
> 
> > --- /dev/null
> > +++ b/src/util/virrotatingfile.c
> > @@ -0,0 +1,608 @@
> 
> In the grand scheme of things a nit - lately there seems to be an effort
> to use two lines for functions, such as:
> 
> [static] int
> virFunctionName(param1Type param1,
> param2Type param2)
> 
> This module has some that are and some that aren't.  I don't have heart
> break one way or another, but someone may ;-)
> 
> > +/*
> > + * virrotatingfile.c: file I/O with size rotation
> > + *
> > + * Copyright (C) 2015 Red Hat, Inc.
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library.  If not, see
> > + * .
> > + *
> > + */
> > +
> 
> [...]
> 
> > +
> > +
> > +static virRotatingFileReaderEntryPtr
> > +virRotatingFileReaderEntryNew(const char *path)
> > +{
> > +virRotatingFileReaderEntryPtr entry;
> > +struct stat sb;
> > +
> > +VIR_DEBUG("Opening %s", path);
> > +
> > +if (VIR_ALLOC(entry) < 0)
> > +return NULL;
> > +
> > +if (VIR_STRDUP(entry->path, path) < 0)
> > +goto error;
> 
> Should the open occur first; otherwise, entry->fd == 0 and the *Free
> routine will VIR_FORCE_CLOSE(0);

Yep, correct.


> 
> Although I don't see it being documented/used later in/for some config
> file (and the max value I see used in code is 2)... Should there be some
> sort of "sanity check" that maxbackup doesn't exceed some number - only
> so many files can be open per process, right?  Then of course there's
> the silly test of someone passing -1...

Well it is defined a size_t so you "cant" pass -1, but yeah we should
refuse greater than say 20 backup files.

> Also, as a config value it seems if someone changes the maxbackup value
> (higher to lower), then some algorithms may miss files... If then going
> from lower to higher, then files that may not have been deleted might be
> found.

IMHO if changing from higher to lower it is the admins responsibility
to kill off obsolete files.


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

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

Re: [libvirt] [PATCH v2 02/13] Import stripped down virtlockd code as basis of virtlogd

[Peter Krempa]

On Thu, Nov 12, 2015 at 17:18:59 +, Daniel Berrange wrote:
> Copy the virtlockd codebase across to form the initial virlogd
> code. Simple search & replace of s/lock/log/ and gut the remote
> protocol & dispatcher. This gives us a daemon that starts up
> and listens for connections, but does nothing with them.
> 
> Signed-off-by: Daniel P. Berrange 
> ---

[...]

> diff --git a/src/logging/log_daemon.c b/src/logging/log_daemon.c
> new file mode 100644
> index 000..184076c
> --- /dev/null
> +++ b/src/logging/log_daemon.c
> @@ -0,0 +1,1177 @@
> +/*
> + * log_daemon.c: log management daemon
> + *
> + * Copyright (C) 2006-2015 Red Hat, Inc.

Um 2006? Here and in every other header.

[...]

> +
> +struct _virLogDaemon {
> +virMutex lock;
> +virNetDaemonPtr dmn;
> +virNetServerPtr srv;
> +};
> +
> +virLogDaemonPtr logDaemon = NULL;
> +
> +static bool execRestart;
> +
> +enum {
> +VIR_LOG_DAEMON_ERR_NONE = 0,
> +VIR_LOG_DAEMON_ERR_PIDFILE,
> +VIR_LOG_DAEMON_ERR_RUNDIR,
> +VIR_LOG_DAEMON_ERR_INIT,
> +VIR_LOG_DAEMON_ERR_SIGNAL,
> +VIR_LOG_DAEMON_ERR_PRIVS,
> +VIR_LOG_DAEMON_ERR_NETWORK,
> +VIR_LOG_DAEMON_ERR_CONFIG,
> +VIR_LOG_DAEMON_ERR_HOOKS,
> +VIR_LOG_DAEMON_ERR_REEXEC,
> +
> +VIR_LOG_DAEMON_ERR_LAST
> +};
> +
> +VIR_ENUM_DECL(virDaemonErr)
> +VIR_ENUM_IMPL(virDaemonErr, VIR_LOG_DAEMON_ERR_LAST,
> +  "Initialization successful",
> +  "Unable to obtain pidfile",
> +  "Unable to create rundir",
> +  "Unable to initialize libvirt",

Will this need to call libvirt? Or should this be 'virtlogd'?

> +  "Unable to setup signal handlers",
> +  "Unable to drop privileges",
> +  "Unable to initialize network sockets",
> +  "Unable to load configuration file",
> +  "Unable to look for hook scripts",
> +  "Unable to re-execute daemon");
> +

[...]

> +
> +
> +static void
> +virLogDaemonErrorHandler(void *opaque ATTRIBUTE_UNUSED,
> + virErrorPtr err ATTRIBUTE_UNUSED)
> +{
> +/* Don't do anything, since logging infrastructure already
> + * took care of reporting the error */
> +}
> +
> +
> +/*
> + * Set up the logging environment
> + * By default if daemonized all errors go to the logfile libvirtd.log,
> + * but if verbose or error debugging is asked for then also output
> + * informational and debug messages. Default size if 64 kB.

The logging ring buffer isn't present any more.

> + */
> +static int
> +virLogDaemonSetupLogging(virLogDaemonConfigPtr config,
> + bool privileged,
> + bool verbose,
> + bool godaemon)
> +{
> +virLogReset();
> +
> +/*
> + * Libvirtd's order of precedence is:

libvirtd?

[...]

> +
> +static void
> +virLogDaemonUsage(const char *argv0, bool privileged)
> +{
> +fprintf(stderr,
> +_("\n"
> +  "Usage:\n"
> +  "  %s [options]\n"
> +  "\n"
> +  "Options:\n"
> +  "  -h | --helpDisplay program help:\n"
> +  "  -v | --verbose Verbose messages.\n"
> +  "  -d | --daemon  Run as a daemon & write PID file.\n"
> +  "  -t | --timeout   Exit after timeout period.\n"
> +  "  -f | --configConfiguration file.\n"
> +  "  -V | --version Display version information.\n"
> +  "  -p | --pid-file  Change name of PID file.\n"
> +  "\n"
> +  "libvirt lock management daemon:\n"), argv0);

Log management.

[...]

> +#define MAX_LISTEN 5

This macro isn't used in this patch.

> +int main(int argc, char **argv) {


[...]


> diff --git a/src/logging/log_daemon_config.c b/src/logging/log_daemon_config.c
> new file mode 100644
> index 000..98d4c89
> --- /dev/null
> +++ b/src/logging/log_daemon_config.c
> @@ -0,0 +1,203 @@

[...]

> +/* Read the config file if it exists.
> + * Only used in the remote case, hence the name.

name?

> + */
> +int
> +virLogDaemonConfigLoadFile(virLogDaemonConfigPtr data,
> +   const char *filename,
> +   bool allow_missing)
> +{
> +virConfPtr conf;
> +int ret;
> +
> +if (allow_missing &&
> +access(filename, R_OK) == -1 &&
> +errno == ENOENT)
> +return 0;
> +
> +conf = virConfReadFile(filename, 0);
> +if (!conf)
> +return -1;
> +
> +ret = virLogDaemonConfigLoadOptions(data, filename, conf);
> +virConfFree(conf);
> +return ret;
> +}

[...]

> diff --git a/src/logging/log_protocol.x b/src/logging/log_protocol.x
> new file mode 100644
> index 000..9b8fa41
> --- /dev/null
> +++ b/src/logging/log_protocol.x
> @@ -0,0 +1,22 @@
> +/* -*- c -*-
> + */
> +
> +%#include "internal.h"
> +
> +typedef opaque virLogManagerProtocolUUID[VIR_UUID_BUFLEN];
> +
> +/* Length of long, but not unbounded

[libvirt] Found mem leak in livirt, need help to debug

[Piotr Rybicki]

Hi.

There is a mem leak in libvirt, when doing external snapshot (for backup 
purposes). My KVM domain uses raw storage images via libgfapi. I'm using 
latest 1.2.21 libvirt (although previous versions act the same).


My bash script for snapshot backup uses series of shell commands (virsh 
connect to a remote libvirt host):


* virsh domblklist KVM
* qemu-img create -f qcow2 -o backing_file=gluster(...) - precreate 
backing file
* virsh snapshot-create KVM SNAP.xml (...) - create snapshot from 
precreated XML snapshot file

* cp main img file
* virsh blockcommit KVM disk (...)

Backup script works fine, however libvirtd process gets bigger and 
bigger each time I run this script.


Some proof of memleak:

32017 - libvirtd pid

When libvirt started:
# ps p 32017 o vsz,rss
   VSZ   RSS
585736 15220

When I start KVM via virsh start KVM
# ps p 32017 o vsz,rss
   VSZ   RSS
1327968 125956

When i start backup script, after snapshot is created (lots of mem 
allocated)

# ps p 32017 o vsz,rss
   VSZ   RSS
3264544 537632

After backup script finished
# ps p 32017 o vsz,rss
   VSZ   RSS
3715920 644940

When i start backup script for a second time, after snapshot is created
# ps p 32017 o vsz,rss
   VSZ   RSS
5521424 1056352

And so on, until libvirt spills 'Out of memory' when connecting, ane 
being really huge process.


Now, I would like to diagnose it further, to provide detailed 
information about memleak. I tried to use valgrind, but unfortunatelly 
I'm on Opteron 6380 platform, and valgrind doesn't support XOP quitting 
witch SIGILL.


If someone could provide me with detailed information on how to get some 
usefull debug info about this memleak, i'll be more than happy to do it, 
and share results here.


Thanks in advance and best regards
Piotr Rybicki

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

[libvirt] [PATCH 0/6] Fixes for memory locking limit

[Andrea Bolognani]

The code dealing with RLIMIT_MEMLOCK contained a few
assumptions that hold for x86 but don't necessarily work
as well for other platforms, eg. that qemu will need to
lock memory only when VFIO passthrough is involved.

This series removes such assumptions by removing ad-hoc
code and making sure that the function containing the
correct logic is called instead; it also implements the
platform-specific calculations needed to calculate the
correct RLIMIT_MEMLOCK value for ppc64 guests.

Patches 1-4 are architecture-agnostic, patches 5-6 are
specific to ppc64.

Cheers.


Andrea Bolognani (6):
  process: Log when limiting the amount of locked memory
  qemu: Use qemuDomainRequiresMlock() in qemuBuildCommandLine()
  qemu: Use qemuDomainRequiresMlock() when attaching PCI hostdev
  qemu: Reduce memlock limit after detaching hostdev
  qemu: Always set locked memory limit for ppc64 domains
  qemu: Add ppc64-specific math to qemuDomainGetMlockLimitBytes()

 src/qemu/qemu_command.c |  9 ++---
 src/qemu/qemu_domain.c  | 89 +++--
 src/qemu/qemu_hotplug.c | 29 
 src/util/virprocess.c   |  4 +++
 4 files changed, 116 insertions(+), 15 deletions(-)

-- 
2.5.0

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

[libvirt] [PATCH 6/6] qemu: Add ppc64-specific math to qemuDomainGetMlockLimitBytes()

[Andrea Bolognani]

The amount of memory a ppc64 domain might need to lock is different
than that of a equally-sized x86 domain, so we need to check the
domain's architecture and act accordingly.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1273480
---
 src/qemu/qemu_domain.c | 80 +-
 1 file changed, 79 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 2c0f5af..1e92b9d 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3793,7 +3793,7 @@ qemuDomainUpdateCurrentMemorySize(virQEMUDriverPtr driver,
  * @def: domain definition
  *
  * Returns the size of the memory in bytes that needs to be set as
- * RLIMIT_MEMLOCK for purpose of VFIO device passthrough.
+ * RLIMIT_MEMLOCK for the QEMU process.
  * If a mem.hard_limit is set, then that value is preferred; otherwise, the
  * value returned may depend upon the architecture or devices present.
  */
@@ -3808,6 +3808,84 @@ qemuDomainGetMlockLimitBytes(virDomainDefPtr def)
 goto done;
 }
 
+if (ARCH_IS_PPC64(def->os.arch)) {
+unsigned long long maxMemory;
+unsigned long long memory;
+unsigned long long baseLimit;
+unsigned long long passthroughLimit;
+size_t nPCIHostBridges;
+size_t i;
+bool usesVFIO = false;
+
+/* TODO: Detect at runtime once we start using more than just
+ *   the default PCI Host Bridge */
+nPCIHostBridges = 1;
+
+for (i = 0; i < def->nhostdevs; i++) {
+virDomainHostdevDefPtr dev = def->hostdevs[i];
+
+if (dev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
+dev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI 
&&
+dev->source.subsys.u.pci.backend == 
VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
+usesVFIO = true;
+break;
+}
+}
+
+memory = virDomainDefGetMemoryActual(def);
+
+if (def->mem.max_memory)
+maxMemory = def->mem.max_memory;
+else
+maxMemory = memory;
+
+/* baseLimit := maxMemory / 128  (a)
+ *  + 4 MiB * #PHBs + 8 MiB  (b)
+ *
+ * (a) is the hash table
+ *
+ * (b) is accounting for the 32-bit DMA window - it could be either the
+ * KVM accelerated TCE tables for emulated devices, or the VFIO
+ * userspace view. The 4 MiB per-PHB (including the default one) covers
+ * a 2GiB DMA window: default is 1GiB, but it's possible it'll be
+ * increased to help performance. The 8 MiB extra should be plenty for
+ * the TCE table index for any reasonable number of PHBs and several
+ * spapr-vlan or spapr-vscsi devices (512kB + a tiny bit each) */
+baseLimit = maxMemory / 128 +
+4096 * nPCIHostBridges +
+8192;
+
+/* passthroughLimit := max( 2 GiB * #PHBs,   (c)
+ *  memory   (d)
+ *  + memory * 1/512 * #PHBs + 8 MiB )   (e)
+ *
+ * (c) is the pre-DDW VFIO DMA window accounting. We're allowing 2 GiB
+ * rather than 1 GiB
+ *
+ * (d) is the with-DDW (and memory pre-registration and related
+ * features) DMA window accounting - assuming that we only account RAM
+ * once, even if mapped to multiple PHBs
+ *
+ * (e) is the with-DDW userspace view and overhead for the 64-bit DMA
+ * window. This is based a bit on expected guest behaviour, but there
+ * really isn't a way to completely avoid that. We assume the guest
+ * requests a 64-bit DMA window (per PHB) just big enough to map all
+ * its RAM. 4 kiB page size gives the 1/512; it will be less with 64
+ * kiB pages, less still if the guest is mapped with hugepages (unlike
+ * the default 32-bit DMA window, DDW windows can use large IOMMU
+ * pages). 8 MiB is for second and further level overheads, like (b) */
+passthroughLimit = MAX(2 * 1024 * 1024 * nPCIHostBridges,
+   memory +
+   memory / 512 * nPCIHostBridges + 8192);
+
+if (usesVFIO)
+memKB = baseLimit + passthroughLimit;
+else
+memKB = baseLimit;
+
+goto done;
+}
+
 /* For device passthrough using VFIO the guest memory and MMIO memory
  * regions need to be locked persistent in order to allow DMA.
  *
-- 
2.5.0

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

[libvirt] [PATCH 3/6] qemu: Use qemuDomainRequiresMlock() when attaching PCI hostdev

[Andrea Bolognani]

The function is used everywhere else to check whether the locked
memory limit should be set / updated, and it should be used here
as well.

Moreover, qemuDomainGetMlockLimitBytes() expects the hostdev to
have already been added to the domain definition, but we only do
that at the end of qemuDomainAttachHostPCIDevice(). Work around
the issue by adding the hostdev before adjusting the locked memory
limit and removing it immediately afterwards.
---
 src/qemu/qemu_hotplug.c | 21 +++--
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 89e5c0d..0bd88ce 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1269,18 +1269,27 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver,
  "supported by this version of qemu"));
 goto error;
 }
-
-/* setup memory locking limits, that are necessary for VFIO */
-if (virProcessSetMaxMemLock(vm->pid,
-qemuDomainGetMlockLimitBytes(vm->def)) < 0)
-goto error;
-
 break;
 
 default:
 break;
 }
 
+/* Temporarily add the hostdev to the domain definition. This is needed
+ * because qemuDomainRequiresMlock() and qemuDomainGetMlockLimitBytes()
+ * require the hostdev to be already part of the domain definition, but
+ * other functions like qemuAssignDeviceHostdevAlias() used below expect
+ * it *not* to be there. A better way to handle this would be nice */
+vm->def->hostdevs[vm->def->nhostdevs++] = hostdev;
+if (qemuDomainRequiresMlock(vm->def)) {
+if (virProcessSetMaxMemLock(vm->pid,
+qemuDomainGetMlockLimitBytes(vm->def)) < 
0) {
+vm->def->hostdevs[--(vm->def->nhostdevs)] = NULL;
+goto error;
+}
+}
+vm->def->hostdevs[--(vm->def->nhostdevs)] = NULL;
+
 if (qemuSetupHostdevCGroup(vm, hostdev) < 0)
 goto error;
 teardowncgroup = true;
-- 
2.5.0

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

[libvirt] [PATCH 1/6] process: Log when limiting the amount of locked memory

[Andrea Bolognani]

This can be useful for debugging.
---
 src/util/virprocess.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index 103c114..4b18903 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -766,6 +766,10 @@ virProcessSetMaxMemLock(pid_t pid, unsigned long long 
bytes)
 return -1;
 }
 }
+
+VIR_DEBUG("Locked memory for process %lld limited to %llu bytes",
+  (long long int) pid, bytes);
+
 return 0;
 }
 #else /* ! (HAVE_SETRLIMIT && defined(RLIMIT_MEMLOCK)) */
-- 
2.5.0

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

[libvirt] [PATCH 2/6] qemu: Use qemuDomainRequiresMlock() in qemuBuildCommandLine()

[Andrea Bolognani]

This removes a duplication of the logic used to decide whether
the memory locking limit should be set.
---
 src/qemu/qemu_command.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 8fdf90c..3e9eb8c 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9135,7 +9135,6 @@ qemuBuildCommandLine(virConnectPtr conn,
 int usbcontroller = 0;
 int actualSerials = 0;
 bool usblegacy = false;
-bool mlock = false;
 int contOrder[] = {
 /*
  * List of controller types that we add commandline args for,
@@ -9304,7 +9303,6 @@ qemuBuildCommandLine(virConnectPtr conn,
 virCommandAddArgFormat(cmd, "mlock=%s",
def->mem.locked ? "on" : "off");
 }
-mlock = def->mem.locked;
 
 virCommandAddArg(cmd, "-smp");
 if (!(smp = qemuBuildSmpArgStr(def, qemuCaps)))
@@ -10869,9 +10867,6 @@ qemuBuildCommandLine(virConnectPtr conn,
  "supported by this version of qemu"));
 goto error;
 }
-/* VFIO requires all of the guest's memory to be locked
- * resident */
-mlock = true;
 }
 
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
@@ -11124,7 +9,9 @@ qemuBuildCommandLine(virConnectPtr conn,
 goto error;
 }
 
-if (mlock)
+/* In some situations, eg. VFIO passthrough, QEMU might need to lock a
+ * significant amount of memory, so we need to set the limit accordingly */
+if (qemuDomainRequiresMlock(def))
 virCommandSetMaxMemLock(cmd, qemuDomainGetMlockLimitBytes(def));
 
 if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_MSG_TIMESTAMP) &&
-- 
2.5.0

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

[libvirt] [PATCH 5/6] qemu: Always set locked memory limit for ppc64 domains

[Andrea Bolognani]

Unlike other architectures, ppc64 domains need to lock memory
even when VFIO is not used.

Change qemuDomainRequiresMlock() to reflect this fact.
---
 src/qemu/qemu_domain.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 1f73709..2c0f5af 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3836,8 +3836,9 @@ qemuDomainGetMlockLimitBytes(virDomainDefPtr def)
 /**
  * @def: domain definition
  *
- * Returns ture if the locked memory limit needs to be set or updated due to
- * configuration or passthrough devices.
+ * Returns true if the locked memory limit needs to be set or updated because
+ * of domain configuration, VFIO passthrough devices or architecture-specific
+ * requirements.
  * */
 bool
 qemuDomainRequiresMlock(virDomainDefPtr def)
@@ -3847,6 +3848,10 @@ qemuDomainRequiresMlock(virDomainDefPtr def)
 if (def->mem.locked)
 return true;
 
+/* ppc64 domains need to lock some memory even when VFIO is not used */
+if (ARCH_IS_PPC64(def->os.arch))
+return true;
+
 for (i = 0; i < def->nhostdevs; i++) {
 virDomainHostdevDefPtr dev = def->hostdevs[i];
 
-- 
2.5.0

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

[libvirt] [PATCH 4/6] qemu: Reduce memlock limit after detaching hostdev

[Andrea Bolognani]

We increase the limit before plugging in a PCI hostdev or a memory
module because some memory might need to be locked due to eg. VFIO.

Of course we should do the opposite after unplugging a device: this
was already the case for memory modules, but not for hostdevs.
---
 src/qemu/qemu_hotplug.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 0bd88ce..2a16f89 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3084,6 +3084,14 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
 networkReleaseActualDevice(vm->def, net);
 virDomainNetDefFree(net);
 }
+
+/* QEMU might no longer need to lock as much memory, eg. we just detached
+ * a VFIO device, so adjust the limit here */
+if (qemuDomainRequiresMlock(vm->def))
+if (virProcessSetMaxMemLock(vm->pid,
+qemuDomainGetMlockLimitBytes(vm->def)) < 0)
+VIR_WARN("Failed to adjust locked memory limit");
+
 ret = 0;
 
  cleanup:
-- 
2.5.0

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

Re: [libvirt] [PATCH v2 01/13] util: add APIs for reading/writing from/to rotating files

[John Ferlan]

On 11/12/2015 12:18 PM, Daniel P. Berrange wrote:
> Add virRotatingFileReader and virRotatingFileWriter objects
> which allow reading & writing from/to files with automation
> rotation to N backup files when a size limit is reached. This
> is useful for guest logging when a guaranteed finite size
> limit is required. Use of external tools like logrotate is
> inadequate since it leaves the possibility for guest to DOS
> the host in between invokations of logrotate.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  po/POTFILES.in  |   1 +
>  src/Makefile.am |   1 +
>  src/libvirt_private.syms|  13 +
>  src/util/virrotatingfile.c  | 608 ++
>  src/util/virrotatingfile.h  |  62 
>  tests/Makefile.am   |   6 +
>  tests/virrotatingfiletest.c | 698 
> 
>  7 files changed, 1389 insertions(+)
>  create mode 100644 src/util/virrotatingfile.c
>  create mode 100644 src/util/virrotatingfile.h
>  create mode 100644 tests/virrotatingfiletest.c
> 

[...]

> --- /dev/null
> +++ b/src/util/virrotatingfile.c
> @@ -0,0 +1,608 @@

In the grand scheme of things a nit - lately there seems to be an effort
to use two lines for functions, such as:

[static] int
virFunctionName(param1Type param1,
param2Type param2)

This module has some that are and some that aren't.  I don't have heart
break one way or another, but someone may ;-)

> +/*
> + * virrotatingfile.c: file I/O with size rotation
> + *
> + * Copyright (C) 2015 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library.  If not, see
> + * .
> + *
> + */
> +

[...]

> +
> +
> +static virRotatingFileReaderEntryPtr
> +virRotatingFileReaderEntryNew(const char *path)
> +{
> +virRotatingFileReaderEntryPtr entry;
> +struct stat sb;
> +
> +VIR_DEBUG("Opening %s", path);
> +
> +if (VIR_ALLOC(entry) < 0)
> +return NULL;
> +
> +if (VIR_STRDUP(entry->path, path) < 0)
> +goto error;

Should the open occur first; otherwise, entry->fd == 0 and the *Free
routine will VIR_FORCE_CLOSE(0);

> +
> +if ((entry->fd = open(path, O_RDONLY)) < 0) {
> +if (errno != ENOENT) {
> +virReportSystemError(errno,
> + _("Unable to open file: %s"), path);
> +goto error;
> +}
> +}
> +
> +if (entry->fd != -1) {
> +if (fstat(entry->fd, &sb) < 0) {
> +virReportSystemError(errno,
> + _("Unable to determine current file inode: 
> %s"),
> + path);
> +goto error;
> +}
> +
> +entry->inode = sb.st_ino;
> +}
> +
> +return entry;
> +
> + error:
> +virRotatingFileReaderEntryFree(entry);
> +return NULL;
> +}
> +

[...]

> +/**
> + * virRotatingFileWriterNew
> + * @path: the base path for files
> + * @maxlen: the maximum number of bytes to write before rollover
> + * @maxbackup: number of backup files to keep when rolloing over

rolling over

Although I don't see it being documented/used later in/for some config
file (and the max value I see used in code is 2)... Should there be some
sort of "sanity check" that maxbackup doesn't exceed some number - only
so many files can be open per process, right?  Then of course there's
the silly test of someone passing -1...

Also, as a config value it seems if someone changes the maxbackup value
(higher to lower), then some algorithms may miss files... If then going
from lower to higher, then files that may not have been deleted might be
found.

ACK -

John
> + * @truncate: whether to truncate the current files when opening
> + * @mode: the file mode to use for creating new files
> + *
> + * Create a new object for writing data to a file with
> + * automatic rollover. If @maxbackup is zero, no backup
> + * files will be created. The primary file will just get
> + * truncated and reopened.
> + *
> + * The files will never exceed @maxlen bytes in size,
> + * but may be rolled over before they reach this size
> + * in order to avoid splitting lines
> + */
> +virRotatingFileWriterPtr virRotatingFileWriterNew(const char *path,
> +  off_t maxlen,
> +  size_t maxbackup,
> +  

[libvirt] [PATCH v2] storage sheepdog: allow to specify redundancy level

[Vasiliy Tolstov]

This is fixed version that compiles and passes make check

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

[libvirt] [PATCH] storage sheepdog: allow to specify redundancy level

[Vasiliy Tolstov]

Some storage backends allows to specify per volume redundancy options.
Sheepdog use x format for specify copies, and x:y format to specify
data and parity block count.

Signed-off-by: Alexey Tyabin 
Signed-off-by: Vasiliy Tolstov 
---
 docs/schemas/storagevol.rng |   3 +
 src/conf/storage_conf.c |   2 +
 src/storage/storage_backend_sheepdog.c  | 141 +---
 src/util/virstoragefile.c   |   4 +-
 src/util/virstoragefile.h   |   2 +
 tests/storagebackendsheepdogtest.c  |  79 +---
 tests/storagevolxml2xmlin/vol-sheepdog.xml  |   1 +
 tests/storagevolxml2xmlout/vol-sheepdog.xml |   1 +
 8 files changed, 141 insertions(+), 92 deletions(-)

diff --git a/docs/schemas/storagevol.rng b/docs/schemas/storagevol.rng
index 7450547..068993f 100644
--- a/docs/schemas/storagevol.rng
+++ b/docs/schemas/storagevol.rng
@@ -55,6 +55,9 @@
 
   
 
+
+  
+
   
 
   
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 9b8abea..d37c93a 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -1345,6 +1345,8 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
 ret->target.allocation = ret->target.capacity;
 }
 
+ret->target.redundancy = virXPathString("string(./redundancy)", ctxt);
+
 ret->target.path = virXPathString("string(./target/path)", ctxt);
 if (options->formatFromString) {
 char *format = virXPathString("string(./target/format/@type)", ctxt);
diff --git a/src/storage/storage_backend_sheepdog.c 
b/src/storage/storage_backend_sheepdog.c
index 1200813..b84a497 100644
--- a/src/storage/storage_backend_sheepdog.c
+++ b/src/storage/storage_backend_sheepdog.c
@@ -51,43 +51,51 @@ virStorageBackendSheepdogParseNodeInfo(virStoragePoolDefPtr 
pool,
  * node id/total, size, used, use%, [total vdi size]
  *
  * example output:
- * 0 15245667872 117571104 0%
- * Total 15245667872 117571104 0% 20972341
+ * 0 425814278144 4871131136 420943147008 1%
+ * Total 2671562256384 32160083968 2639402172416 1% 75161927680
  */
-
-const char *p, *next;
+char **lines = NULL;
+char **cells = NULL;
+size_t i;
+int ret = -1;
 
 pool->allocation = pool->capacity = pool->available = 0;
 
-p = output;
-do {
-char *end;
+lines = virStringSplit(output, "\n", 0);
+if (lines == NULL)
+goto cleanup;
 
-if ((next = strchr(p, '\n')))
-++next;
-else
-break;
+for (i = 0; lines[i]; i++) {
+char *line = lines[i];
+if (line == NULL)
+goto cleanup;
 
-if (!STRPREFIX(p, "Total "))
+if (!STRPREFIX(line, "Total "))
 continue;
 
-p = p + 6;
+virStringStripControlChars(line);
+if ((cells = virStringSplit(line, " ", 0)) == NULL)
+continue;
 
-if (virStrToLong_ull(p, &end, 10, &pool->capacity) < 0)
-break;
+if (virStringListLength(cells) < 3) {
+goto cleanup;
+}
 
-if ((p = end + 1) > next)
-break;
+if (virStrToLong_ull(cells[1], NULL, 10, &pool->capacity) < 0)
+goto cleanup;
 
-if (virStrToLong_ull(p, &end, 10, &pool->allocation) < 0)
-break;
+if (virStrToLong_ull(cells[2], NULL, 10, &pool->allocation) < 0)
+goto cleanup;
 
 pool->available = pool->capacity - pool->allocation;
-return 0;
-
-} while ((p = next));
+break;
+}
+ret = 0;
 
-return -1;
+ cleanup:
+virStringFreeList(lines);
+virStringFreeList(cells);
+return ret;
 }
 
 void
@@ -275,6 +283,10 @@ virStorageBackendSheepdogBuildVol(virConnectPtr conn 
ATTRIBUTE_UNUSED,
 
 cmd = virCommandNewArgList(SHEEPDOGCLI, "vdi", "create", vol->name, NULL);
 virCommandAddArgFormat(cmd, "%llu", vol->target.capacity);
+
+if(NULL != vol->target.redundancy)
+virCommandAddArgFormat(cmd, "-c %s", vol->target.redundancy);
+
 virStorageBackendSheepdogAddHostArg(cmd, pool);
 if (virCommandRun(cmd, NULL) < 0)
 goto cleanup;
@@ -291,60 +303,77 @@ virStorageBackendSheepdogParseVdiList(virStorageVolDefPtr 
vol,
   char *output)
 {
 /* fields:
- * current/clone/snapshot, name, id, size, used, shared, creation time, 
vdi id, [tag]
+ * current/clone/snapshot, name, id, size, used, shared, creation time, 
vdi id, redundancy, [tag], size shift
  *
  * example output:
- * s test 1 10 0 0 1336556634 7c2b25
- * s test 2 10 0 0 1336557203 7c2b26
- * = test 3 10 0 0 1336557216 7c2b27
+ * s test 1 10 0 0 1336556634 7c2b25 1 tt 22
+ * s test 2 10 0 0 1336557203 7c2b26 2 zz 22
+ * = 39865 0 21474836480 247463936 1337982976 1447516646 47d187 2  22
+ * = test 3 10 0 0 1336557216 7c2b27 3 xx 22
  */
-
-int 

Re: [libvirt] [PATCH v2 01/13] util: add APIs for reading/writing from/to rotating files

[Peter Krempa]

On Thu, Nov 12, 2015 at 17:18:58 +, Daniel Berrange wrote:
> Add virRotatingFileReader and virRotatingFileWriter objects
> which allow reading & writing from/to files with automation
> rotation to N backup files when a size limit is reached. This
> is useful for guest logging when a guaranteed finite size
> limit is required. Use of external tools like logrotate is
> inadequate since it leaves the possibility for guest to DOS
> the host in between invokations of logrotate.
> 
> Signed-off-by: Daniel P. Berrange 
> ---

[...]

> diff --git a/src/util/virrotatingfile.c b/src/util/virrotatingfile.c
> new file mode 100644
> index 000..840b55f
> --- /dev/null
> +++ b/src/util/virrotatingfile.c
> @@ -0,0 +1,608 @@

[...]

> +
> +
> +/**
> + * virRotatingFileWriterGetPath:
> + * @file: the file context
> + *
> + * Return the primary file path
> + */
> +const char *virRotatingFileWriterGetPath(virRotatingFileWriterPtr file)
> +{
> +return file->basepath;
> +}
> +
> +
> +/**
> + * virRotatingFileWriterGetINode:
> + * @file: the file context
> + *
> + * Return the inode of the file currently being written to
> + */
> +ino_t virRotatingFileWriterGetINode(virRotatingFileWriterPtr file)
> +{
> +return file->entry->inode;
> +}
> +
> +
> +/**
> + * virRotatingFileWriterGetOffset:
> + * @file: the file context
> + *
> + * Return the offset at which data is currently being written
> + */
> +off_t virRotatingFileWriterGetOffset(virRotatingFileWriterPtr file)
> +{
> +return file->entry->pos;
> +}

I see how you are going to use this. I think the usage pattern is a bit
complicated, but for the purpose it will serve it's probably all right.

ACK

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] [RFC] virSetUIDGID: Don't leak supplementary groups

[Daniel P. Berrange]

On Tue, Jun 23, 2015 at 01:48:42PM +0200, Richard Weinberger wrote:
> The LXC driver uses virSetUIDGID() to become UID/GID 0.
> It passes an empty groups list to virSetUIDGID()
> to get rid of all supplementary groups from the host side.
> But virSetUIDGID() calls setgroups() only if the supplied list
> is larger than 0.
> This leads to a container root with unrelated supplementary groups.
> In most cases this issue is unoticed as libvirtd runs as UID/GID 0
> without any supplementary groups.
> 
> Signed-off-by: Richard Weinberger 
> ---
> I've marked that patch as RFC as I'm not sure if all users of virSetUIDGID()
> expect this behavior too.
> 
> Thanks,
> //richard
> ---
>  src/util/virutil.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index cddc78a..ea697a3 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -1103,7 +1103,7 @@ virSetUIDGID(uid_t uid, gid_t gid, gid_t *groups 
> ATTRIBUTE_UNUSED,
>  }
>  
>  # if HAVE_SETGROUPS
> -if (ngroups && setgroups(ngroups, groups) < 0) {
> +if (setgroups(ngroups, groups) < 0) {

After running unit tests I see this causes a failure in virCommand.
We were using 'ngroups != NULL' as a crude check to skip setgroups()
when unprivileged.

The better way to check this is by doing  'gid != (gid_t_-1' as we
use on the line above which calls setgid(). So I'll push this instead:

-if (ngroups && setgroups(ngroups, groups) < 0) {
+if (gid != (gid_t)-1 && setgroups(ngroups, groups) < 0) {
 

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

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

Re: [libvirt] [PATCH] [RFC] virSetUIDGID: Don't leak supplementary groups

[Daniel P. Berrange]

On Wed, Nov 18, 2015 at 07:35:39AM +0100, Martin Kletzander wrote:
> On Tue, Nov 17, 2015 at 10:02:36PM +0100, Richard Weinberger wrote:
> >On Wed, Jun 24, 2015 at 11:19 AM, Martin Kletzander  
> >wrote:
> >>On Tue, Jun 23, 2015 at 01:48:42PM +0200, Richard Weinberger wrote:
> >>>
> >>>The LXC driver uses virSetUIDGID() to become UID/GID 0.
> >>>It passes an empty groups list to virSetUIDGID()
> >>>to get rid of all supplementary groups from the host side.
> >>>But virSetUIDGID() calls setgroups() only if the supplied list
> >>>is larger than 0.
> >>>This leads to a container root with unrelated supplementary groups.
> >>>In most cases this issue is unoticed as libvirtd runs as UID/GID 0
> >>>without any supplementary groups.
> >>>
> >>>Signed-off-by: Richard Weinberger 
> >>>---
> >>>I've marked that patch as RFC as I'm not sure if all users of
> >>>virSetUIDGID()
> >>>expect this behavior too.
> >>>
> >>
> >>I went through the callers and I see no reason why setgroups should
> >>not be called.  ACK.  I also can't think of a use case in which we'd
> >>like to keep the supplemental groups.
> >
> >Ping?
> >
> 
> Oh, sorry, I didn't realize you don't have push access.  Would you
> happen to have these patches around somewhere?  The originals got
> archived automatically.  If you send them to me, I'll push them, it
> would be easier than me sucking it out of the ML archive (the same
> applies for the other patch: "bind mount container TTYs").

Don't worry, I've pushed them all.

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

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

Re: [libvirt] [PATCH] [RFC] virSetUIDGID: Don't leak supplementary groups

[Daniel P. Berrange]

On Tue, Jun 23, 2015 at 01:48:42PM +0200, Richard Weinberger wrote:
> The LXC driver uses virSetUIDGID() to become UID/GID 0.
> It passes an empty groups list to virSetUIDGID()
> to get rid of all supplementary groups from the host side.
> But virSetUIDGID() calls setgroups() only if the supplied list
> is larger than 0.
> This leads to a container root with unrelated supplementary groups.
> In most cases this issue is unoticed as libvirtd runs as UID/GID 0
> without any supplementary groups.
> 
> Signed-off-by: Richard Weinberger 
> ---
> I've marked that patch as RFC as I'm not sure if all users of virSetUIDGID()
> expect this behavior too.

ACK & pushed - I concur with Martin that this is good practice.


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

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

Re: [libvirt] [PATCH] lxc: Bind mount container TTYs

[Daniel P. Berrange]

On Tue, Jun 23, 2015 at 04:38:57PM +0200, Richard Weinberger wrote:
> Instead of creating symlinks, bind mount the devices to
> /dev/pts/XY.
> Using bind mounts it is no longer needed to add pts devices
> to files like /dev/securetty.
> 
> Signed-off-by: Richard Weinberger 
> ---
>  src/lxc/lxc_container.c | 38 +-
>  1 file changed, 21 insertions(+), 17 deletions(-)

ACK, no idea why we didn't think of this approach sooner :-)

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

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

Re: [libvirt] [PATCH] lxc: Don't make container's TTY a controlling TTY

[Daniel P. Berrange]

On Tue, Jun 23, 2015 at 03:18:53PM +0200, Richard Weinberger wrote:
> Userspace does not expect that the initial console
> is a controlling TTY. systemd can deal with that, others not.
> On sysv init distros getty will fail to spawn a controlling on
> /dev/console or /dev/tty1. Which will cause to whole container
> to reboot upon ctrl-c.
> 
> This patch changes the behavior of libvirt to match the kernel
> behavior where the initial TTY is also not controlling.
> 
> The only user visible change should be that a container with
> bash as PID 1 would complain. But this matches exactly the kernel
> be behavior with intit=/bin/bash.
> To get a controlling TTY for bash just run "setsid /bin/bash".
> 
> Signed-off-by: Richard Weinberger 
> ---
>  src/lxc/lxc_container.c | 14 +-
>  1 file changed, 1 insertion(+), 13 deletions(-)

I checked with a systemd init and with bash as init, and it works as
described.

ACK & pushed to master

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

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

[libvirt] How to disable kvm_steal_time feature

[Piotr Rybicki]

Hi.

I would like to workaround a bug, when after live-migration of KVM 
guest, there is a 100% steal time shown in guest.


I've read, that disabling 'kvm_steal_time' feature should workarund this 
bug, but i can't find a way to disable it in libvirt's domain xml file.


Tried in  section:

but that doesn't work.

Also, couldn't find any related information in libvirt documentation. 
Google helps neither.


How can I disable this feature?

Thanks in advance.
Piotr Rybicki

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

Re: [libvirt] [PATCH v2] locking: Add io_timeout to sanlock

[Michal Privoznik]

On 27.10.2015 17:53, Jiri Denemark wrote:
> On Tue, Oct 27, 2015 at 16:29:51 +0100, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1251190
>>
>> So, if domain loses access to storage, sanlock tries to kill it
>> after some timeout. So far, the default is 80 seconds. But for
>> some scenarios this might not be enough. We should allow users to
>> adjust the timeout according to their needs.
>>
>> Signed-off-by: Michal Privoznik 
>> ---
>>
>> diff to v2:
>> - Check if the new sanlock API is accessible. If not, forbid setting timeout 
>> in
>>   the config file.
>>
>>  m4/virt-sanlock.m4  |  7 +++
>>  src/locking/libvirt_sanlock.aug |  1 +
>>  src/locking/lock_driver_sanlock.c   | 15 +++
>>  src/locking/sanlock.conf|  7 +++
>>  src/locking/test_libvirt_sanlock.aug.in |  1 +
>>  5 files changed, 31 insertions(+)
>>


> Ouch, please, don't mix #if and if blocks. The following would be much
> better:
> 
> #ifdef HAVE_SANLOCK_ADD_LOCKSPACE_TIMEOUT
> rv = sanlock_add_lockspace_timeout(&ls, 0, driver->io_timeout);
> #else
> if (driver->io_timeout) {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>_("unable to use io_timeout with this version of 
> sanlock"));
> goto error;
> }
> rv = sanlock_add_lockspace(&ls, 0);
> #endif
> 
> if (rv < 0) {
> 
> Jirka
> 

Fixed and pushed. Thanks.

Michal

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

Re: [libvirt] [PATCH v2 0/7] Allow memory hotplug without NUMA on ppc64

[Peter Krempa]

On Thu, Nov 12, 2015 at 14:28:51 -0500, John Ferlan wrote:
> > 
> ACK series

Pushed; Thanks.

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 0/8] Add perf and Intel CMT feature support

[Ren, Qiaowei]

> -Original Message-
> From: Daniel P. Berrange [mailto:berra...@redhat.com]
> Sent: Tuesday, November 17, 2015 6:47 PM
> To: Ren, Qiaowei
> Cc: libvir-list@redhat.com; Jiri Denemark
> Subject: Re: [PATCH 0/8] Add perf and Intel CMT feature support
> 
> On Tue, Nov 17, 2015 at 04:00:40PM +0800, Qiaowei Ren wrote:
> > 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.
> >
> > TODO:
> > 1. This series relys on keeping an open file descriptor for the guest.
> > We should add one patch to call sys_perf_event_open again iff libvirtd
> > is restarted.
> 
> I've not done a detailed patch review, but overall I think this design is 
> pretty
> good now, so thanks for taking time to re-write this.
> 

Daniel, thanks for your feedback. ^-^

Thanks,
Qiaowei

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