Re: [libvirt] [PATCH 1/4] virsh: make conn global

On 07/04/2011 02:41 AM, Michal Privoznik wrote: 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. 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: 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: 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(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org