Re: [libvirt] [PATCHv1 4/9] conf: Add helper for listing domains on drivers supporting virDomainObj

On 06/05/2012 07:19 AM, Peter Krempa wrote: You know, the only flag group you don't have here is DOMAINS_[NO_]MANAGEDSAVE, but it wouldn't be that hard to update virDomainObjPtr to have a new bool field that tracks this information (update the information when a managedsave image is created, deleted, and when reloading domain state at libvirtd startup). Besides, we already have precedence for that - it is how we manage domain snapshots (that is, qemu_driver.c updates vm->snapshots at key points in snapshot life cycle). I'd rather see a pre-requisite patch that adds a new 'has_managedsave' bool flag to the struct, at which point [0]... While what you have works, I like to add a blank line between any macro definition that uses \-newline to take up more than one source line, so that it is a bit more obvious that the last line should not have \ (or if someone does accidentally put \ on the last line, at least the next macro name is still an independent macro name rather than an unintended continuation of the first macro). That said... ...I would just inline the listing of the two new files to DOMAIN_CONF_SOURCES rather than creating a new category DOMAIN_LIST_SOURCES. Are we really borrowing any contents written in 2011, or can this be shortened to just 2012? s/VIR_FROM_NONE/VIR_FROM_DOMAIN/ I would consider adding a second size_t variable, so that you can track usage independently from allocations. In other words, the number of domains can be large enough that reallocating the array for every member can be noticeably slower than reallocating the array geometrically [1]. Or, don't even bother with reallocations [2]. Rather than listing all the other states here (and missing something when a new state is added), I would write this clause as: (MATCH(VIR_CONNECT_LIST_DOMAINS_OTHER) && st != VIR_DOMAIN_RUNNING && st != VIR_DOMAIN_PAUSED && st != VIR_DOMAIN_SHUTOFF) Careful. ESX and VBox support snapshots, but do not track them in &vm->snapshots. We can keep this common filtering only if those two hypervisors mask out both HAS_SNAPSHOT and NO_SNAPSHOT bits before calling into this function. Then again, those two hypervisors don't use virDomainObj in the first place, so maybe I'm getting ahead of myself. [1] Here, I'd use the four-argument VIR_RESIZE_N, for geometric growth of the array, if we even keep the realloc as part of this callback. Why the double space before NULL? [2] Why not just allocate the end result according to the size of the hash table? We know that the result cannot exceed virHashSize(domobjs) (it might be smaller, but if we overallocate now, then we don't have to mess with realloc in the middle of each callback). [0]... we might not even need this function, if you can get the common virDomainObjPtr code to track managedsave images in place (at least, that was the only reason that 5/9 used it; I didn't check if 6/9 or 7/9 needed to use it for other reasons). VIR_SHRINK_N might be nicer here. Same question on copyright year. Worth another round of review on this patch. -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org