Re: [libvirt] [PATCHv9 1/4] libvirt/qemu - persistent modification of devices.

On 04/12/2011 08:49 PM, KAMEZAWA Hiroyuki wrote: It's taken quite a few iterations, but I've finally scheduled time to start reviewing it. You missed one. You changed attach and detach, but not update. This parameter can usefully forward to all three types of updates. In fact, I think I'd like to go one further, and refactor things further before we start adding persistent devices. It seems like there are two steps in all three categories of functions - obtain the locks, then do the work. I like how you've factored things into one function that obtains the locks then forwards on to other functions that do the work. Furthermore, I don't see any reason why we can't support LIVE|CONFIG at once, provided we know for which devices we can make a successful CONFIG change. You are right that it's not implemented yet, but no worse than what we already have, so it's not a regression. But I think it is doable, by rewriting this into one giant function: 1. obtain lock 2. if CURRENT, convert to LIVE or CONFIG 3. if LIVE but not active, error 4. if CONFIG but not persistent, error 5. if CONFIG call, make temporary vmdef and modify that, or quit if that device is not supported yet 6. if LIVE, make live modification; if that errors out, quit 7. if CONFIG, make temporary vmdef permanent (hopefully it succeeds, because we don't roll back the LIVE portion of LIVE|CONFIG) 8. clean up locks I'll propose a followup patch that tries to capture this idiom in code. This causes a change in behavior. Previously, we silently ignored VIR_DOMAIN_DEVICE_MODIFY_FORCE, now we error out. But overall, that's a good change (FORCE only made sense for ModifyDevice, not Attach), and it goes to show that we don't use virCheckFlags on nearly enough APIs. For example, here your logic is wrong. You covered the CONFIG|LIVE case with qemuDomainModifyDevicePersistent (by erroring out), but you _don't_ cover the CURRENT case (which should be either CONFIG or LIVE). But to learn if the vm is active, you have to obtain the lock, and the way you've written this, you've already forwarded into one of the two routines. Rather, all the public entries should forward into the one giant helper routine which grabs the locks, checks the flags, then calls into the right callbacks. Another change in noisily rejecting FORCE, but I think it's good. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org