+list Hello Guido,
I just finished with the refactoring. All things discussed in the thread are now implemented. Just one note: CallMonitorCommand still uses socat. But I have extracted the socket code away from QmpConnection in a new class MonitorSocket which QmpConnection now extends. I use MonitorSocket in PassTapFd as well. So the only thing now is to implement a MonitorConnection class to extend MonitorSocket and use it instead of socat. Can we proceed with hotplug review and postpone this big change after hotplug gets merged? Thanks a lot, dimara * Guido Trotter <[email protected]> [2013-07-31 14:00:18 +0200]: > On Tue, Jul 30, 2013 at 10:35 PM, Dimitris Aragiorgis <[email protected]> wrote: > > Hello Guido, > > > > Many thanks for the loooong and the detailed review! To sum up: > > > > - 80% of the comments are style issues in order the patch to comply to the > > ganeti code standards > > - docstrings: I got it! There should always be one that reflects to the > > actual implementation > > - In case hotplug is not possible (old devices, no fdsend, chroot, etc..) > > log warnings or raise exceptions > > - parse 'help info' monitor command to check if hotplug commands are > > supported > > - make HotplugDevice a base method and let other hypervisors raise a > > NotImplemented exception. > > - add REPLUG action for nic modifications > > Yes, as discussed let's call this "CHANGE" as we might get later to > implement some changes without a replug. > > > - move hypervisor agnostic code away from hv_kvm.py to backend.py > > - make GenernateKVMBlockDeviceOptions return only those options and > > not change/return the kvm_cmd > > > > Do I miss anything important? > > > > Yes, there's the "remove socat dependency and extract the socket that > you use for connecting to the kvm monitor in passtapfd to be the way > used by callmonitorcommand, qmp, and passtapfd. (there should be only > one version of the "socket" code, basically). > > > I'll try to apply all that in the next few days and when I have all > > issues covered I'll resend the whole patch rebased on current master. > > > > Thanks again for the feedback, > > dimara > > > > Thanks a lot, > > Guido
signature.asc
Description: Digital signature
