Re: [libvirt] [PATCH 02/27] Add API for 'info cpus' monitor command
On Mon, Sep 28, 2009 at 02:22:46PM +0100, Mark McLoughlin wrote:
On Thu, 2009-09-24 at 16:00 +0100, Daniel P. Berrange wrote:
> * src/qemu/qemu_monitor.h, src/qemu/qemu_monitor.c: Add a new
> qemuMonitorGetCPUInfo() command
> * src/qemu/qemu_driver.c: Refactor qemudDetectVcpuPIDs to
> use qemuMonitorGetCPUInfo()
> ---
> src/qemu/qemu_driver.c | 114 ++++++++++--------------------------------
> src/qemu/qemu_monitor_text.c | 85 +++++++++++++++++++++++++++++++
> src/qemu/qemu_monitor_text.h | 4 +-
> 3 files changed, 115 insertions(+), 88 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 9f17aae..30d1468 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
...
> diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
> index 76842a5..d93e475 100644
> --- a/src/qemu/qemu_monitor_text.c
> +++ b/src/qemu/qemu_monitor_text.c
> @@ -31,6 +31,7 @@
>
> #include "qemu_monitor_text.h"
> #include "qemu_conf.h"
> +#include "c-ctype.h"
> #include "memory.h"
> #include "logging.h"
> #include "driver.h"
> @@ -435,3 +436,87 @@ qemudMonitorSendCont(virConnectPtr conn,
> VIR_FREE(reply);
> return 0;
> }
> +
> +
> +int qemuMonitorGetCPUInfo(const virDomainObjPtr vm,
> + int **pids)
> +{
> + char *qemucpus = NULL;
> + char *line;
> + int lastVcpu = -1;
> + pid_t *cpupids = NULL;
> + size_t ncpupids = 0;
> +
> + if (qemudMonitorCommand(vm, "info cpus", &qemucpus) < 0) {
> + qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
> + "%s", _("cannot run monitor command to
fetch CPU thread info"));
> + return -1;
> + }
Not passing conn to ReportError in most monitor functions now; is that a
problem?
Historically we needed to pass virConnectPtr around everywhere so that
errors would be reported against the connection. When we went multi-
threaded, all errors started being reported in a global thread local.
The public API entrypoints in src/libvirt.c copy the error across to
the per-virConnectPtr error variable for back-compatability right at
the end. Thus it is pointless passing virConnectPtr around to all
functions internally, except for places which use it for something
unrelated to error reporting[1].
In fact passing virConnectPtr around everywhere has actually caused
several crashes in the past, because there are some codepaths where
we do not have any virConnectPtr available, and thus just pass NULL.
Later codepaths mistakenly assumeed that the virConnectPtr was non-NULL
with predictably crashtastic results.
I'd like to set a goal of removing the virConnectPtr parameters from
*all* internal methods, except for the top level driver API method
implementations whose API signature obviously matches the public API.
The only place we still need to pass virConnectPtr is in places like the
QEMU driver which access the conn->networkDriver or conn->secretDriver
fields. Addressing that will be more difficult - it would need us to
change the way we activate secondary drivers, so the primary driver held
a direct reference to the desired secondary driver. I'm not going to
attempt that any time soon, but we can still trivially eliminate the
virConnectPtr param from 95% of all internal methods.
Regards,
Daniel
[1] Not entirely accurate, because we there is possibility of registering
a callback function against a virConnectPtr for receiving errors. This is
currently very very unsafe because the callback may be invoked deep inside
libvirt internal code where countless locks are held. The invocation of
the callbacks needs to be moved right up to the top in src/libvirt.c at
the same place as all the virSetConnError() calls.
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|