Issue 90 Further Clarifications

[Daniel P. Berrangé]

On Thu, Nov 19, 2020 at 03:48:48PM -0600, Dustan B Helm wrote:
> [image: Dustan Helm] <https://gitlab.com/dustan.helm>
> Dustan Helm <https://gitlab.com/dustan.helm> @dustan.helm
> <https://gitlab.com/dustan.helm> · just now
> <https://gitlab.com/libvirt/libvirt/-/issues/90#note_451306432>
> <https://gitlab.com/libvirt/libvirt/-/issues/90#>
> 
> Before we start making changes and solidifying our XML parameter choices,
> we have a few clarifying questions about the issue we'd like to get out of
> the way.
> 
>    1.
> 
>    In src/qemu/qemu_capabilities.h, we found the string "/* -drive
>    file.driver=vxhs via query-qmp-schema */" after the QEMU_CAPS_VXHS
>    declaration. What is the purpose of these strings, and how do we modify
>    them to make sense for nfs? Would we simply mirror what is done for VXHS,
>    adding nfs as the protocol instead?

These comments are just a hint/reminder to readers about what QEMU command
line option(s), the capability check is tracking the availability of. That
particular example might be a bit misleading, since in the qemu_capabilities.c
file, we're actually looking for the blockdev-arg/arg-type/+vxhs feature,
not -drive.  QEMU has 2 ways of configuring disks, -drive is the historical
main way, and -blockdev is the modern way that libvirt introduced support
for relatively recently. We actually end up having to support both approachs
in libvirt currrently, as we try to make libvirt work with both old and new
QEMU versions.

Peter can probably offer better suggestions than me about what specific
thing to probe for 'nfs'. 

>    2.
> 
>    Where is domain XML parsed and formatted? Is that what is referred to by
>    the schema formats in domaincommon.rng?

The domaincommon.rng file provides the RelaxNG schema, which is used for
(optionally) validating XML files before parsing.

The actual parser lives in src/conf/domain_conf.{c,h} files.

There are also docs for users about the schema in docs/formatdomain.rst



>    3.
> 
>    In src/qemu/qemu_block.c, the json object arguments currently present in
>    qemuBlockStorageSourceGetVxHSProps(...) are not the same ones listed in the
>    example commit. What is the reason for this change, and how should we take
>    it into account when implementing a new protocol type?

I'll leave thi question to Peter too.



> Additionally, we were hoping we could get some clarification on the proper
> way to handle submitting patches with multiple commits. If we receive
> feedback for only part of a patch, for example, how would we be meant to
> update it? Would we simply amend the particular commit that needed updates,
> and if so, how do we amend a commit other than the most recent commit?
> Should we send in our patches for review one at a time, or try to get all
> of them made and then send them in all at once?

If you get comments on a particular patch in a series, then you need use
"git rebase -i master", to go back and edit the original patch that had
the comment on. We don't want "fixup" commits at the end of a series

When posting patches, you should always post the an entire self-contained
series. eg if you posted a series of 5 patches originally, and got comments
on 2 patches, update the two patches in question, and then re-post the
entire series of 5 patches.

If you've only received feedback on a couple of patches, it is hard to know
if that means the other others are fine, or if no one has had time to review
them yet. You don't have to wait for explicit comments on every patch before
re-posting a version 2 of a series. A rule of thumb might be to wait a day
to see if other patches get more comments, and then repost a v2 with updates.


The most important thing with a patch series, is that livirt be able to
succesfully compile and run tests at each individual patch in the series.

At tip to validate this locally before sending is to do something like

    git rebase -i master -x "ninja -C build test"

This will rebase your local branch against master, and stop at each
patch in the branch and run tests using ninja. (assumes you used
"meson build" initally).


In terms of how long to wait before sending patches there's no perfect
answer.

Generally you want a patch series to accomplish something "useful",
where "useful" involves a bit of personal judgement.

For example, don't send a patch that adds a function, but then doesn't
use it anywhere. Instead wait until you've got the corresponding usage
of that function in another patch.

If you know you're likely to end up with a set of 10 patches to complete
a particular feature, generally you would wait to get the complete set
ready, rather than drip-feeding the patches one at a time.

A counter example though. If when working on your feature, you see a
bunch of existing code that would benefit from some refactoring or
cleanup, then often people might send a series of patches just todo
the cleanup/refactoring, then send the impl of their desired feature
as a separate series later.

This might sound complex, but don't worry about it too much. This kind
of thing is a learning experiance for everyone, so we're not expecting
people to get it perfect each time. We'll try to give positive suggestions
if there improvements that could be made.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|