<div dir="ltr"><div><div>Hi, On Tue, Sep 22, 2015 at 6:53 PM, Daniel P. Berrange <<a href="mailto:berrange@redhat.com">berrange@redhat.com</a>> wrote: > > On Tue, Sep 22, 2015 at 07:50:41AM -0400, John Ferlan wrote: > > > > > > On 09/17/2015 04:46 AM, Shivangi Dhir wrote: > > > These patches solve a BiteSizedTask - Introduce new virtType enum item[0] > > > > > > [0] <a href="http://wiki.libvirt.org/page/BiteSizedTasks#Introduce_new_virtType_enum_item">http://wiki.libvirt.org/page/BiteSizedTasks#Introduce_new_virtType_enum_item</a> > > > > > > Shivangi Dhir (3): > > > conf: Add new VIR_DOMAIN_VIRT_NONE enum > > > qemu: Make virtType of type virDomainVirtType > > > conf: Change the description of virCapabilitiesDomainDataLookup() > > > > > > src/conf/capabilities.c | 6 +++--- > > > src/conf/domain_conf.c | 3 ++- > > > src/conf/domain_conf.h | 3 ++- > > > src/qemu/qemu_monitor.c | 2 +- > > > src/qemu/qemu_monitor.h | 2 +- > > > src/qemu/qemu_monitor_json.c | 2 +- > > > src/qemu/qemu_monitor_json.h | 2 +- > > > src/qemu/qemu_monitor_text.c | 2 +- > > > src/qemu/qemu_monitor_text.h | 2 +- > > > tests/qemumonitorjsontest.c | 2 +- > > > tests/vircapstest.c | 32 ++++++++++++++++---------------- > > > 11 files changed, 30 insertions(+), 28 deletions(-) > > > > > > > While yes this is a bite sized task, I do have a concern with changing > > the meaning of undefined from -1 to 0. Doing so will change (increase by > > 1) each of the VIR_DOMAIN_VIRT_* values, e.g. VIR_DOMAIN_VIRT_QEMU > > changes from 0 to 1. If you look through the history of changes to > > virDomainVirtType you'll note it's been appended to and never had > > something inserted in the middle. > > > > Inserting in the middle is not a problem if we could "guarantee" that > > nothing exists (saved) or is currently running that references an > > existing numerical value. Unfortunately, I'm not sure we can do that. > > > > As seen in patch 2, there is a 'virtType' value in the domain > > definition. A running domain VIR_DOMAIN_VIRT_QEMU would have been > > started with "0" in that field. When a new libvirtd with these changes > > is in place, the running domain would still have "0" stored away and > > that would mean something different. > > > > Furthermore (or likewise), a migration from an "older" host to a newer > > host would have a different virtType value and I would think cause > > virDomainDefCheckABIStability to complain. Similarly, a 'saved' domain > > would have a numerical anomaly - although for that case it's less clear > > whether we save the physical name or the numerical value. > > > > Of course I could be off-base/wrong, but let's see what others say... > > In general we should never be serializing the raw integer enum > values only the string representation. If we did have somewhere > that used an integer representation, we'd certainly have to avoid > this change as you mention. I think we're probably safe though > unless someone knows of a place we don't use the string rep. > > > > > FWIW: For a "first" patch set - it seems you've followed the guidelines > > quite well. About the only suggestions I have are in patch1 the > > comparison "if (domaintype)" in virCapabilitiesDomainDataLookupInternal > > could be "if (domaintype > VIR_DOMAIN_VIRT_NONE)" as well as patch3 > > should be merged with patch1 and the "int domaintype" in the args list > > for virCapabilitiesDomainDataLookup (and capabilities.h) should be > > "virDomainVirtType domaintype". > </div>Thanks alot for your feedback. </div>Should I modify and resend the patches after making the changes suggested above, if there are no other issues ? <div>-- <div>Regards, Shivangi Dhir</div></div></div>