Re: [libvirt] [PATCH] Add qcow2 cache configuration

On Wed, Aug 30, 2017 at 04:07:16PM +0800, Liu Qing wrote: >Random write IOPS will drop dramatically if qcow2 l2 cache could not >cover the whole disk. This patch give libvirt user a chance to adjust >the qcow2 cache configuration. > Thanks for the patch, but it has no documentation, no capability checking, no RNG schema adjustments and no tests. Ideally the patch should be a series of patches, each introducing part of the functionality. For example: [PATCH 1/3] conf, docs: Add support for bla bla bla Add stuff to docs/schemas/*.rng, docs/*.html.in, src/conf/*.c, add parsing tests to tests/qemuxml2xmltest.c (and possibly qemuxml2argvtest.c if there is some negative test that should fail parsing). Code needs to compile cleanly and tests need to pass after this patch. Documentation should cleanly state the reasoning and rules for the possible values so that users know *if* and *why* they need to set this up and to *what* values. It is also good to think about why QEMU doesn't use such values as default and whether or not (or why/not) libvirt should default to such values without making the user do so. [PATCH 2/3] qemu: Add capability checking for bla bla bla Here you would check that we properly probe qemu for the possibility of setting such tunables in src/qemu/qemu_capabilities.[hc]. Code needs to compile cleanly and tests need to pass after this patch. [PATCH 3/3] qemu: Add support for bla bla bla Here you would check if the emulator has the required capabilities, format them on the command line and add positive tests to tests/qemuxml2argvtest.c. Code needs to compile cleanly and tests need to pass after this patch. In rare cases where the functionality and required tests are minimal, patches [2/3] and [3/3] could be merged together, but they can always be separate, IMHO. All of this ^^ is only about the way the patch is supposed to be sent. Whether or not it makes sense to expose such tunables is left as an exercise to all readers (and possibly a discussion on v2 of this patch).

Have a nice day, Martin