On Mon, 2015-11-02 at 10:06 +0530, Shivaprasad bhat wrote: The problem I have with this approach is that, as far as I understand, before your patch dev->stubDriver is a configuration item: you set it to whatever's appropriate, and the value is later inspected in virPCIDeviceBindToStub()[1] to decide what stub driver the device should be bound to. After your patch, depending on the context, it could be either a configuration item, as before, or state, eg. the current stub driver the device is bound to, with no clear way to tell one situation from the other. I think using a single attribute to store related but different information should be avoided, as it has the potential to make the code much harder to understand, especially later on for someone who's not been involved in the implementation. Moreover you only need that value for comparison purposes in a couple of places, and by moving the code to virPCIDeviceNew() you make it so the value is loaded (by performing filesystem operations) every single time a virDevice is created, whether the value is actually used or not. Cheers. [1] Rather virPCIDeviceDetach() in reality - I have a pending patch that moves it to virPCIDeviceBindToStub() where it IMHO belongs -- Andrea Bolognani Software Engineer - Virtualization Team