On 07/04/2011 02:41 AM, Michal Privoznik wrote:
> Since virsh is not multi-threaded,
Technically, virsh _is_ multi-threaded, in that it uses multiple threads
in order to process Ctrl-C to abort a migration; however, in this
instance, the main thread waits for the worker thread to complete, and
there is no input processing that changes 'conn' during that time.
> it is safe to have it as global
> variable.
At any rate, I still think this part of the claim is true; and if it
ever changes, we can make conn a thread-local variable instead.
However, if we do that, we will want to use an accessor method to get at
the thread-local conn via portable code, rather than relying on gcc
__thread extensions. So I would recommend a v2 of this patch:
> This is going to be needed in some special cases where we
> can't change function prototype but want to have connection object
> accessible.
> ---
> tools/virsh.c | 555 +++++++++++++++++++++++++++++----------------------------
> 1 files changed, 278 insertions(+), 277 deletions(-)
> @@ -242,6 +241,8 @@ typedef struct vshCmdGrp {
> const vshCmdDef *commands;
> } vshCmdGrp;
>
> +virConnectPtr conn; /* connection to hypervisor (MAY BE NULL) */
Make this static. Just because it is global to the file doesn't mean
that it has to be exported beyond the file. Then add:
static virConnectPtr vshConn(void) { return conn; }
and instead of doing s/ctl->conn/conn/, you instead do
s/ctl->conn/vshConn()/. That way, if we ever need to make conn
thread-local, only vshConn needs to change, rather than changing 200+
lines of callers to yet another convention.
The bulk of this patch is mechanical, but I did spot a problem given my
above preference for future-proofing our maintenance efforts:
When updating the 'doMigrate' function, I would recommend modifying the
struct __vshCtrlData to add a virConnectPtr member, and pass vshConn()
through that struct rather than having doMigrate rely on the global conn
(seeing as how if we ever change conn to be thread-safe, the doMigrate
execution would be in a different thread than the primary thread that
initialized conn). And that means that this change:
> -static bool vshConnectionUsability(vshControl *ctl, virConnectPtr conn);
> +static bool vshConnectionUsability(vshControl *ctl);
is ill-advised. vshConnectionUsability is one of the functions called
inside doMigrate, so it needs to operate on the virConnectPtr passed in
as an argument rather than assuming that the current global 'conn' is
correct.
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list