Re: [libvirt] [PATCH v4 1/8] docs: schema: Add basic documentation for the virtual

On 07/07/2017 04:07 AM, Longpeng(Mike) wrote: Add an example <address... > too. Is this the "legal name" that would be used for a commit? Generally we prefer to see a more legal name rather than someone's email name. There's plenty of examples in git history. For some reason I'm only seeing this patch from the series come through. Whether that's something specific to the RH email or in general, I'm not sure. Similarly for your v3 series, just the first patch came through. Since they were close together - I have to wonder if the RH email system was having one it's clogged or senior moments and the patches are still stuck in some queue somewhere. It's happened before, but usually everything gets backed up, not just one series from one submittor. I see from the archive you pinged on 7/25 looking for a review on the series, but even that didn't come through. It's very strange. Still I think you need to repost and adjust anyway. Here's some thoughts looking just at the archives though... Patches 1 & 3 have a "relationship" insomuch as as you're documenting in patch 1 before the domain_conf code exists. I think it's best to combine them. * For both, will the default of MODEL_VIRTIO and BACKEND_BUILTIN live for perpetuity? Or is it possible that at some point a "default" or "unknown" would be required? I ask only since both would be equal to zero for the enum and VIR_ALLOC means default to zero. So sometimes adding a "default" or "none" type entry ensures that something does get set and it's not some default as a result of the allocation algorithm that takes over. * When you add the XML parsing code, you should add the xml2xml tests. That means grabbing qemuxml2xmltest.c and xml from patches 7 & 8 and moving them into here. * For new functions, make sure there's 2 blank lines before and after the function... virDomainCryptoDefFree only has 1 before. * For the queues parse, use virStrToLong_uip to ensure no negative is supplied (per the rng below using positiveInteger) Patch 2 should be the last patch as news is always last. Patch 4 is going to need some merge conflict resolution. There is also now some tests/qemucapabilitiesdata/*ppc* replies/xml that exist - whether that relates here or not I'm not sure, but something that I think may have been added since you last posted... Patch 5... * There's an error message that has "faile" instead of "failed". * There's a switch for dev->data.crypto->model that uses VIR_DOMAIN_RNG_MODEL_LAST for a case. * Should the alias include the "virtio" in some way. Would it ever be reasonable for a domain to use two different types for different devices? Maybe virtio is supplied today and becomes legacy and who knows what is the new sleek thing next year, but both are allowed so you have to change the alias then. * You may way to create an accessor that prints the "obj%s" alias since it's formatted twice. It'll be useful if you support hotplug as well. * What about hotplug? You either should support or explicitly deny. I'm kind of surprised you didn't get build warnings because VIR_DOMAIN_DEVICE_CRYPTO wasn't added to qemu_driver.c and qemu_hotplug.c since the switch ((virDomainDeviceType) def->type) is there. * This is when the qemuxml2argvtest should be adjusted. Patch 6... Put the comma on the AddLit rather than the next virBufferAsprintf.... although since PPC and CCW are supported from the start, I'd say add them both at the same time. Although I do understand and appreciate why they're separate. Still it's not "new" functionality for CCW support, so just do it all at once. Patch 7... Tests are usually added at the time the command is adjusted. This looks merge-able with patches 3 and 5 Patch 8... Looks merge-able with patches 3 and 6 Couple more comments below... s/card(provides/card (provides) It'd be 3.7.0 at the earliest either "is provided" or "to provide" s/of the crypto/for the crypto/ Again for the crypto device reads better to me, but it's a bit redundant with the first sentence. This makes me wonder what happens if someone uses 100 or 1000 or ... queues. Is there some maximum (I didn't check the qemu code). Beyond that what use does increasing the number of queues have? There's also an <address> that is required to be "pci" or "ccw" - that should be mentioned here. You can use a link from here to the device address section IIRC. Hopefully the next time posted, the series will show up for me too! John