3:48 p.m.
[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?
2.
Where is domain XML parsed and formatted? Is that what is referred to by
the schema formats in domaincommon.rng?
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?
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?
3:41 a.m.
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 :|
5:32 a.m.
On Fri, Nov 20, 2020 at 09:41:44 +0000, Daniel Berrange wrote:
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.
For new features though I strongly prefer if we no longer update the old
code. Implement the new protocol only for -blockdev.
Peter can probably offer better suggestions than me about what
specific
thing to probe for 'nfs'.
The simplest way to probe for nfs protocol support is to use the
following query string in virQEMUCapsQMPSchemaQueries[]:
"blockdev-add/arg-type/+nfs"
Looking at the @BlockdevOptionsNfs struct in
qemu.git/qapi/block-core.json it seems that all properties were
introduced at same time, so the check doesn't need to be more specific.
I can provide more insight on how virQEMUCapsQMPSchemaQueries[] works if
you are interested, but the above will work.
> 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
Adding the NFS protocol itself is rather trivial because we use enum
to string convertors which cause a compilation failure if you don't
populate the strings, so adding the protocol type will be enough to
figure out where.
If you want to implement other properties of the nfs protocol driver
such as @user or @group. I suggest you first send a RFC mail with your
proposed XML addition for review before diving into the rng schema and
XML/formatter parser.
Looking at the options in @BlockdevOptionsNfs @user and @group seem a
bit interesting. I'd not worry with the rest probably.
> 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.
I'm not going to outline why we've changed the old commit. You are
implementing new code. You'll need to add a handler to
qemuBlockStorageSourceGetBackendProps which converts the appropriate
fields of virStorageSource to a virJSONValue object which maps to the
qemu properties according to the @BlockdevOptionsNfs qemu struct.
To verify that it's correct you can add a TEST_DISK_TO_JSON case to
tests/qemublocktest.c (input files are in
libvirt/tests/qemublocktestdata/xml2json/ ) where you provide a disk XML
snippet and the output is what we'd use with qemu.
Note that the 'onlytarget' boolean formats a string which is written
into the qcow2 header file if you create an overlay/external snapshot,
so it must not include any transient or authentication data.
All of the above is QMP-schema validated so you'll be notified if it
doesn't conform to qemu's monitor schema.
You'll also need to implement a backing store string parser (that is
the string which qemu writes to the overlay as noted above). The parser
entry point is virStorageSourceParseBackingJSON and the backends for it
are registered in virStorageSourceJSONDriverParser struct.
The tests for the above are in tests/virstoragetest.c as
TEST_BACKING_PARSE.
Then depending on whether you actually want to add support for image
creation (e.g. to support creating snapshots backed by the NFS storage
directly) you then need to implement hanling in
qemuBlockStorageSourceCreateGetStorageProps.
You'll definitely see all the places that might need implementing once
you add the new protocol entry to enum virStorageNetProtocol as we in
many cases use switch statements with proper type where the compiler
reminds you that you need to add the handling for the new value in the
given patch.
Since implementation of the qemu bits should be in a separate commit
from the one adding the parser bits and thus the new enum, it's okay to
just add the enum value to the swithc case and implement it later.
12:07 p.m.
On Fri, Nov 20, 2020 at 5:33 AM Peter Krempa <pkrempa(a)redhat.com> wrote:
On Fri, Nov 20, 2020 at 09:41:44 +0000, Daniel Berrange wrote:
> 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.
For new features though I strongly prefer if we no longer update the old
code. Implement the new protocol only for -blockdev.
> Peter can probably offer better suggestions than me about what specific
> thing to probe for 'nfs'.
The simplest way to probe for nfs protocol support is to use the
following query string in virQEMUCapsQMPSchemaQueries[]:
"blockdev-add/arg-type/+nfs"
Looking at the @BlockdevOptionsNfs struct in
qemu.git/qapi/block-core.json it seems that all properties were
introduced at same time, so the check doesn't need to be more specific.
I can provide more insight on how virQEMUCapsQMPSchemaQueries[] works if
you are interested, but the above will work.
> > 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
>
Adding the NFS protocol itself is rather trivial because we use enum
to string convertors which cause a compilation failure if you don't
populate the strings, so adding the protocol type will be enough to
figure out where.
If you want to implement other properties of the nfs protocol driver
such as @user or @group. I suggest you first send a RFC mail with your
proposed XML addition for review before diving into the rng schema and
XML/formatter parser.
Looking at the options in @BlockdevOptionsNfs @user and @group seem a
bit interesting. I'd not worry with the rest probably.
> > 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.
I'm not going to outline why we've changed the old commit. You are
implementing new code. You'll need to add a handler to
qemuBlockStorageSourceGetBackendProps which converts the appropriate
fields of virStorageSource to a virJSONValue object which maps to the
qemu properties according to the @BlockdevOptionsNfs qemu struct.
To verify that it's correct you can add a TEST_DISK_TO_JSON case to
tests/qemublocktest.c (input files are in
libvirt/tests/qemublocktestdata/xml2json/ ) where you provide a disk XML
snippet and the output is what we'd use with qemu.
Note that the 'onlytarget' boolean formats a string which is written
into the qcow2 header file if you create an overlay/external snapshot,
so it must not include any transient or authentication data.
All of the above is QMP-schema validated so you'll be notified if it
doesn't conform to qemu's monitor schema.
You'll also need to implement a backing store string parser (that is
the string which qemu writes to the overlay as noted above). The parser
entry point is virStorageSourceParseBackingJSON and the backends for it
are registered in virStorageSourceJSONDriverParser struct.
The tests for the above are in tests/virstoragetest.c as
TEST_BACKING_PARSE.
Then depending on whether you actually want to add support for image
creation (e.g. to support creating snapshots backed by the NFS storage
directly) you then need to implement hanling in
qemuBlockStorageSourceCreateGetStorageProps.
You'll definitely see all the places that might need implementing once
you add the new protocol entry to enum virStorageNetProtocol as we in
many cases use switch statements with proper type where the compiler
reminds you that you need to add the handling for the new value in the
given patch.
Since implementation of the qemu bits should be in a separate commit
from the one adding the parser bits and thus the new enum, it's okay to
just add the enum value to the swithc case and implement it later.
We weren't exactly sure what you meant by submitting our proposed XML
additions if we are to avoid diving into the schemas. Our idea is to have
the NFS generate XML based on issue 90, where you have a network disk, a
source protocol, a host, and a new NFS tag which has a user attribute and a
group attribute (both required). In terms of the rng schema, we would make
it look similar to the VxHS schema (diskSourceNetworkProtocolVxHS) except
that below the diskSourceNetworkHost we would also interweave a
diskSourceNFS reference, which would require both user and group.
3:22 p.m.
On 11/20/20 1:07 PM, Dustan B Helm wrote:
On Fri, Nov 20, 2020 at 5:33 AM Peter Krempa <pkrempa(a)redhat.com
<mailto:pkrempa@redhat.com>> wrote:
On Fri, Nov 20, 2020 at 09:41:44 +0000, Daniel Berrange wrote:
> On Thu, Nov 19, 2020 at 03:48:48PM -0600, Dustan B Helm wrote:
> > [image: Dustan Helm] <https://gitlab.com/dustan.helm
<https://gitlab.com/dustan.helm>>
> > Dustan Helm <https://gitlab.com/dustan.helm
<https://gitlab.com/dustan.helm>> @dustan.helm
> > <https://gitlab.com/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#note_451306432>>
> > <https://gitlab.com/libvirt/libvirt/-/issues/90#
<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.
For new features though I strongly prefer if we no longer update
the old
code. Implement the new protocol only for -blockdev.
> Peter can probably offer better suggestions than me about what
specific
> thing to probe for 'nfs'.
The simplest way to probe for nfs protocol support is to use the
following query string in virQEMUCapsQMPSchemaQueries[]:
"blockdev-add/arg-type/+nfs"
Looking at the @BlockdevOptionsNfs struct in
qemu.git/qapi/block-core.json it seems that all properties were
introduced at same time, so the check doesn't need to be more
specific.
I can provide more insight on how virQEMUCapsQMPSchemaQueries[]
works if
you are interested, but the above will work.
> > 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
>
Adding the NFS protocol itself is rather trivial because we use enum
to string convertors which cause a compilation failure if you don't
populate the strings, so adding the protocol type will be enough to
figure out where.
If you want to implement other properties of the nfs protocol driver
such as @user or @group. I suggest you first send a RFC mail with your
proposed XML addition for review before diving into the rng schema and
XML/formatter parser.
Looking at the options in @BlockdevOptionsNfs @user and @group seem a
bit interesting. I'd not worry with the rest probably.
> > 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.
I'm not going to outline why we've changed the old commit. You are
implementing new code. You'll need to add a handler to
qemuBlockStorageSourceGetBackendProps which converts the appropriate
fields of virStorageSource to a virJSONValue object which maps to the
qemu properties according to the @BlockdevOptionsNfs qemu struct.
To verify that it's correct you can add a TEST_DISK_TO_JSON case to
tests/qemublocktest.c (input files are in
libvirt/tests/qemublocktestdata/xml2json/ ) where you provide a
disk XML
snippet and the output is what we'd use with qemu.
Note that the 'onlytarget' boolean formats a string which is written
into the qcow2 header file if you create an overlay/external snapshot,
so it must not include any transient or authentication data.
All of the above is QMP-schema validated so you'll be notified if it
doesn't conform to qemu's monitor schema.
You'll also need to implement a backing store string parser (that is
the string which qemu writes to the overlay as noted above). The
parser
entry point is virStorageSourceParseBackingJSON and the backends
for it
are registered in virStorageSourceJSONDriverParser struct.
The tests for the above are in tests/virstoragetest.c as
TEST_BACKING_PARSE.
Then depending on whether you actually want to add support for image
creation (e.g. to support creating snapshots backed by the NFS storage
directly) you then need to implement hanling in
qemuBlockStorageSourceCreateGetStorageProps.
You'll definitely see all the places that might need implementing once
you add the new protocol entry to enum virStorageNetProtocol as we in
many cases use switch statements with proper type where the compiler
reminds you that you need to add the handling for the new value in the
given patch.
Since implementation of the qemu bits should be in a separate commit
from the one adding the parser bits and thus the new enum, it's
okay to
just add the enum value to the swithc case and implement it later.
We weren't exactly sure what you meant by submitting our proposed XML
additions if we are to avoid diving into the schemas. Our idea is to
have the NFS generate XML based on issue 90, where you have a network
disk, a source protocol, a host, and a new NFS tag which has a user
attribute and a group attribute (both required). In terms of the rng
schema, we would make it look similar to the VxHS schema
(diskSourceNetworkProtocolVxHS) except that below the
diskSourceNetworkHost we would also interweave a diskSourceNFS
reference, which would require both user and group.
(since it's already quite late in the day on a Friday where Peter is
located, I'll make an attempt to answer for him :-)
I believe what he's asking for is just an email that says something like
(names and organizations completely fabricated on the spot for sake of
example):
"Our idea is to implement this new feature by adding a new value "blorg"
to the <bipple> element "blox", and an optional <bumble> subelement
of
<bipple) that contains blahblahbobloblaw details, like this:
<device something='xyzzy'>
<bipple blox='blorg'>
<blorg blahblah='bobloblaw'>lawblog</blorg>
</bipple>
...
So, what do you think?"
or whatever. i.e., not a vague description or a formal RNG
representation of the changes you want to make, but short and specific
description along with an example of what those changes will look like
in an actual XML config document - something like the descriptions and
example XML bits in https://www.libvirt.org/formatdomain.html. This is
*much* quicker to parse and discuss than an RNG grammar :-)
11:20 a.m.
On Fri, Nov 20, 2020 at 3:22 PM Laine Stump <laine(a)redhat.com> wrote:
On 11/20/20 1:07 PM, Dustan B Helm wrote:
On Fri, Nov 20, 2020 at 5:33 AM Peter Krempa <pkrempa(a)redhat.com> wrote:
> On Fri, Nov 20, 2020 at 09:41:44 +0000, Daniel Berrange wrote:
> > 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.
>
> For new features though I strongly prefer if we no longer update the old
> code. Implement the new protocol only for -blockdev.
>
> > Peter can probably offer better suggestions than me about what specific
> > thing to probe for 'nfs'.
>
> The simplest way to probe for nfs protocol support is to use the
> following query string in virQEMUCapsQMPSchemaQueries[]:
>
> "blockdev-add/arg-type/+nfs"
>
> Looking at the @BlockdevOptionsNfs struct in
> qemu.git/qapi/block-core.json it seems that all properties were
> introduced at same time, so the check doesn't need to be more specific.
>
> I can provide more insight on how virQEMUCapsQMPSchemaQueries[] works if
> you are interested, but the above will work.
>
> > > 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
> >
>
> Adding the NFS protocol itself is rather trivial because we use enum
> to string convertors which cause a compilation failure if you don't
> populate the strings, so adding the protocol type will be enough to
> figure out where.
>
> If you want to implement other properties of the nfs protocol driver
> such as @user or @group. I suggest you first send a RFC mail with your
> proposed XML addition for review before diving into the rng schema and
> XML/formatter parser.
>
> Looking at the options in @BlockdevOptionsNfs @user and @group seem a
> bit interesting. I'd not worry with the rest probably.
>
> > > 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.
>
> I'm not going to outline why we've changed the old commit. You are
> implementing new code. You'll need to add a handler to
> qemuBlockStorageSourceGetBackendProps which converts the appropriate
> fields of virStorageSource to a virJSONValue object which maps to the
> qemu properties according to the @BlockdevOptionsNfs qemu struct.
>
> To verify that it's correct you can add a TEST_DISK_TO_JSON case to
> tests/qemublocktest.c (input files are in
> libvirt/tests/qemublocktestdata/xml2json/ ) where you provide a disk XML
> snippet and the output is what we'd use with qemu.
>
> Note that the 'onlytarget' boolean formats a string which is written
> into the qcow2 header file if you create an overlay/external snapshot,
> so it must not include any transient or authentication data.
>
> All of the above is QMP-schema validated so you'll be notified if it
> doesn't conform to qemu's monitor schema.
>
> You'll also need to implement a backing store string parser (that is
> the string which qemu writes to the overlay as noted above). The parser
> entry point is virStorageSourceParseBackingJSON and the backends for it
> are registered in virStorageSourceJSONDriverParser struct.
>
> The tests for the above are in tests/virstoragetest.c as
> TEST_BACKING_PARSE.
>
> Then depending on whether you actually want to add support for image
> creation (e.g. to support creating snapshots backed by the NFS storage
> directly) you then need to implement hanling in
> qemuBlockStorageSourceCreateGetStorageProps.
>
> You'll definitely see all the places that might need implementing once
> you add the new protocol entry to enum virStorageNetProtocol as we in
> many cases use switch statements with proper type where the compiler
> reminds you that you need to add the handling for the new value in the
> given patch.
>
> Since implementation of the qemu bits should be in a separate commit
> from the one adding the parser bits and thus the new enum, it's okay to
> just add the enum value to the swithc case and implement it later.
>
> We weren't exactly sure what you meant by submitting our proposed XML
additions if we are to avoid diving into the schemas. Our idea is to have
the NFS generate XML based on issue 90, where you have a network disk, a
source protocol, a host, and a new NFS tag which has a user attribute and a
group attribute (both required). In terms of the rng schema, we would make
it look similar to the VxHS schema (diskSourceNetworkProtocolVxHS) except
that below the diskSourceNetworkHost we would also interweave a
diskSourceNFS reference, which would require both user and group.
(since it's already quite late in the day on a Friday where Peter is
located, I'll make an attempt to answer for him :-)
I believe what he's asking for is just an email that says something like
(names and organizations completely fabricated on the spot for sake of
example):
"Our idea is to implement this new feature by adding a new value "blorg"
to the <bipple> element "blox", and an optional <bumble> subelement
of
<bipple) that contains blahblahbobloblaw details, like this:
<device something='xyzzy'>
<bipple blox='blorg'>
<blorg blahblah='bobloblaw'>lawblog</blorg>
</bipple>
...
So, what do you think?"
or whatever. i.e., not a vague description or a formal RNG representation
of the changes you want to make, but short and specific description along
with an example of what those changes will look like in an actual XML
config document - something like the descriptions and example XML bits in
https://www.libvirt.org/formatdomain.html. This is *much* quicker to
parse and discuss than an RNG grammar :-)
We plan to support NFS protocol according to the example XML from Issue 90
<http://gitlab.com/libvirt/libvirt/-/issues/90>;. Since there is already
support for network disks of different protocol types and host information,
we think that the only new XML information we will add is an <nfs> element
which will be a subelement of <source>, with attributes “user” and “group”
(both strings). This element will only be generated if the source protocol
is “nfs” and we assume that both “user” and “group” will be required.
Here is the XML example given in the issue for reference:
<disk type='network' device='disk'>
<driver name='qemu' type='raw'/>
<source protocol='nfs' name='PATH'>
<host name='example.com' port='2049'/
<nfs user='USER' group='GROUP'/>
</source>
<target dev='vda' bus='virtio'/>
</disk>
What do you think of these proposed changes? Should either of the <nfs>
tag's string attributes be optional?
2:33 a.m.
On Sat, Nov 21, 2020 at 11:20:57 -0600, Dustan B Helm wrote:
We plan to support NFS protocol according to the example XML from
Issue 90
<http://gitlab.com/libvirt/libvirt/-/issues/90>;. Since there is already
support for network disks of different protocol types and host information,
we think that the only new XML information we will add is an <nfs> element
which will be a subelement of <source>, with attributes “user” and “group”
(both strings). This element will only be generated if the source protocol
is “nfs” and we assume that both “user” and “group” will be required.
Here is the XML example given in the issue for reference:
<disk type='network' device='disk'>
<driver name='qemu' type='raw'/>
<source protocol='nfs' name='PATH'>
<host name='example.com' port='2049'/
<nfs user='USER' group='GROUP'/>
</source>
<target dev='vda' bus='virtio'/>
</disk>
Sounds reasonable to me. We tend to name elements equivalent to <nfs>
you propose by their purpose (such as <auth> <initiator> <cookies> for
other protocols) but in this case I don't have a better suggestion so
going with <nfs> is reasonable.
Since you are proposing 'user' and 'group' to be strings while qemu
accepts only numeric UID/GID, please use the same conversion code we
have for the <inituser> and <initgroup> values in regards to forcing
numeric value to skip being interpreded:
https://www.libvirt.org/formatdomain.html#container-boot
What do you think of these proposed changes? Should either of the
<nfs>
tag's string attributes be optional?
In this case qemu doesn't mandate the use of the user/group field so you
can make the nfs element and both user and group optional especially
since it's only a workaround for the broken-by design NFS "security".
You can claim that a hypervisor-default uid/gid is used when the fields
are not present.
You also probably want to mention in the documentation that in most
cases qemu is running as non-root and thus doesn't have access to
privileged ports. Thus the export has to use the 'insecure' option to
allow non-privileged ports.
One further thing possibly worth mentioning is that the name=''
attribute starts with the NFS export name.
5:17 p.m.
On Mon, Nov 23, 2020 at 2:33 AM Peter Krempa <pkrempa(a)redhat.com> wrote:
On Sat, Nov 21, 2020 at 11:20:57 -0600, Dustan B Helm wrote:
> We plan to support NFS protocol according to the example XML from Issue
90
> <http://gitlab.com/libvirt/libvirt/-/issues/90>;. Since there is already
> support for network disks of different protocol types and host
information,
> we think that the only new XML information we will add is an <nfs>
element
> which will be a subelement of <source>, with attributes “user” and
“group”
> (both strings). This element will only be generated if the source
protocol
> is “nfs” and we assume that both “user” and “group” will be required.
>
> Here is the XML example given in the issue for reference:
>
> <disk type='network' device='disk'>
>
> <driver name='qemu' type='raw'/>
>
> <source protocol='nfs' name='PATH'>
>
> <host name='example.com' port='2049'/
>
> <nfs user='USER' group='GROUP'/>
>
> </source>
>
> <target dev='vda' bus='virtio'/>
>
> </disk>
Sounds reasonable to me. We tend to name elements equivalent to <nfs>
you propose by their purpose (such as <auth> <initiator> <cookies> for
other protocols) but in this case I don't have a better suggestion so
going with <nfs> is reasonable.
Since you are proposing 'user' and 'group' to be strings while qemu
accepts only numeric UID/GID, please use the same conversion code we
have for the <inituser> and <initgroup> values in regards to forcing
numeric value to skip being interpreded:
https://www.libvirt.org/formatdomain.html#container-boot
The linked documentation
<https://www.libvirt.org/formatdomain.html#container-boot> suggests that
providing a “+” prefix will force attribute data to be interpreted
numerically. Since QEMU requires that the group and user attributes be
numeric id values, would we want to simply insert a “+” prefix to these
attributes when they are provided explicitly (instead of using the
hypervisor default values, which we assume will just be numeric data)?
What do you think of these proposed changes? Should either of the
<nfs>
> tag's string attributes be optional?
In this case qemu doesn't mandate the use of the user/group field so you
can make the nfs element and both user and group optional especially
since it's only a workaround for the broken-by design NFS "security".
You can claim that a hypervisor-default uid/gid is used when the fields
are not present.
We have assumed that because “group” and “user” are optional, they are not
mutually
required- that is, one or the other may be provided explicitly, in
which case the other is assumed to be the hypervisor-default. Is this
correct?
Additionally, when both default values for the hypervisor are to be used
(that is, there is no explicitly given nfs-user or nfs-group attribute),
would we simply remove the entire <nfs> tag as it would be empty? Or is it
still necessary in order to bypass the “broken by design” NFS security?
You also probably want to mention in the documentation that in most
cases qemu is running as non-root and thus doesn't have access to
privileged ports. Thus the export has to use the 'insecure' option to
allow non-privileged ports.
One further thing possibly worth mentioning is that the name=''
attribute starts with the NFS export name.
Does the NFS export requiring the “insecure” option in most cases have
implications
on the code we add to support the protocol beyond
documentation of the usage semantics? Also, what specifically is referred
to by the “NFS export”- is this simply the directory containing all the
files/folders to be shared across the NFS (i.e. etc/exports/), or some
representation thereof in memory?
12:59 a.m.
On Mon, Nov 23, 2020 at 17:17:15 -0600, Dustan B Helm wrote:
On Mon, Nov 23, 2020 at 2:33 AM Peter Krempa
<pkrempa(a)redhat.com> wrote:
> On Sat, Nov 21, 2020 at 11:20:57 -0600, Dustan B Helm wrote:
> > We plan to support NFS protocol according to the example XML from Issue
> 90
> > <http://gitlab.com/libvirt/libvirt/-/issues/90>;. Since there is already
> > support for network disks of different protocol types and host
> information,
> > we think that the only new XML information we will add is an <nfs>
> element
> > which will be a subelement of <source>, with attributes “user” and
> “group”
> > (both strings). This element will only be generated if the source
> protocol
> > is “nfs” and we assume that both “user” and “group” will be required.
> >
> > Here is the XML example given in the issue for reference:
> >
> > <disk type='network' device='disk'>
> >
> > <driver name='qemu' type='raw'/>
> >
> > <source protocol='nfs' name='PATH'>
> >
> > <host name='example.com' port='2049'/
> >
> > <nfs user='USER' group='GROUP'/>
> >
> > </source>
> >
> > <target dev='vda' bus='virtio'/>
> >
> > </disk>
>
> Sounds reasonable to me. We tend to name elements equivalent to <nfs>
> you propose by their purpose (such as <auth> <initiator> <cookies>
for
> other protocols) but in this case I don't have a better suggestion so
> going with <nfs> is reasonable.
>
> Since you are proposing 'user' and 'group' to be strings while qemu
> accepts only numeric UID/GID, please use the same conversion code we
> have for the <inituser> and <initgroup> values in regards to forcing
> numeric value to skip being interpreded:
>
> https://www.libvirt.org/formatdomain.html#container-boot
>
> The linked documentation
<https://www.libvirt.org/formatdomain.html#container-boot> suggests that
providing a “+” prefix will force attribute data to be interpreted
numerically. Since QEMU requires that the group and user attributes be
numeric id values, would we want to simply insert a “+” prefix to these
attributes when they are provided explicitly (instead of using the
hypervisor default values, which we assume will just be numeric data)?
No, not at all. QEMU requires you to provide an integer according to the
schema. That means that for any '+'-prefixed value, you strip the + and
convert it to int. For a username you look it up in the system to get
the uid and use that. We have helpers for this, but I can't recall the
name. You'll have to do some digging.
> What do you think of these proposed changes? Should either of
the <nfs>
> > tag's string attributes be optional?
>
> In this case qemu doesn't mandate the use of the user/group field so you
> can make the nfs element and both user and group optional especially
> since it's only a workaround for the broken-by design NFS "security".
>
> You can claim that a hypervisor-default uid/gid is used when the fields
> are not present.
>
> We have assumed that because “group” and “user” are optional, they are not
mutually required- that is, one or the other may be provided explicitly, in
which case the other is assumed to be the hypervisor-default. Is this
correct?
Yes.
Additionally, when both default values for the hypervisor are to be
used
(that is, there is no explicitly given nfs-user or nfs-group attribute),
would we simply remove the entire <nfs> tag as it would be empty? Or is it
still necessary in order to bypass the “broken by design” NFS security?
Yes, you want to format the element only when any of the two attributes
are present. This is simple to achieve, our XML formatting helper
(virXMLFormatElement) does the correct thing if no attributes and no
subelements are passed for formatting.
> You also probably want to mention in the documentation that in
most
> cases qemu is running as non-root and thus doesn't have access to
> privileged ports. Thus the export has to use the 'insecure' option to
> allow non-privileged ports.
>
> One further thing possibly worth mentioning is that the name=''
> attribute starts with the NFS export name.
>
> Does the NFS export requiring the “insecure” option in most cases have
implications on the code we add to support the protocol beyond
documentation of the usage semantics? Also, what specifically is referred
No. I just wanted that you point out in the docs that the NFS client
used in qemu has no access to privileged ports in the general use case
and thus may have implications on what is used. We tend to use a vague
terminology such as. "Note that hypervisors may not be able to use
privileged ports to access the NFS export and thus may require to use
the ``insecure`` option for the export to allow access"
to by the “NFS export”- is this simply the directory containing all
the
files/folders to be shared across the NFS (i.e. etc/exports/), or some
representation thereof in memory?
NFS export is one exported directory, thus maps to one line in
/etc/exports. Since there are multiple exports possible on a host you
have to use it when addressing the specific file.
Example:
Assume we have the following exports (leftmost column of /etc/exports)
/some/export *:...
/foo/bar/baz *:...
And 'ls /foo/bar/baz' on the NFS server:
image.qcow2
image2.qcow2
So in your proposal, if you want to point to 'image2.qcow2' you'd use:
<disk type='network' device='disk'>
<driver name='qemu' type='qcow2'/>
<source protocol='nfs' name='/foo/bar/baz/image.qcow2'>
<host name='example.com' port='2049'/
<nfs user='+123' group='wheel'/>
</source>
<target dev='vda' bus='virtio'/>
</disk>
Here the export name is part of the "path" in the `name` attribute.
An alternative is to expose 'export' as a separate attribute:
<disk type='network' device='disk'>
<driver name='qemu' type='qcow2'/>
<source protocol='nfs' name='image.qcow2'>
<host name='example.com' port='2049'/
<nfs export='/foo/bar/baz' user='+123' group='wheel'/>
</source>
<target dev='vda' bus='virtio'/>
</disk>
Here export is stored explicitly. Note that qemu takes the export
concatenated with the filename as argument so you'd have to concatenate
them when formatting the JSON struct representing the backend of the
block device.
I don't have a preference for either of them, but I'd like you to
document that bit. Specifically we have very similar documentation for
other protocols such as iSCSI, etc. which also store specifics in the
name filed.
3:25 p.m.
On Tue, Nov 24, 2020 at 12:59 AM Peter Krempa <pkrempa(a)redhat.com> wrote:
On Mon, Nov 23, 2020 at 17:17:15 -0600, Dustan B Helm wrote:
> On Mon, Nov 23, 2020 at 2:33 AM Peter Krempa <pkrempa(a)redhat.com> wrote:
>
> > On Sat, Nov 21, 2020 at 11:20:57 -0600, Dustan B Helm wrote:
> > > We plan to support NFS protocol according to the example XML from
Issue
> > 90
> > > <http://gitlab.com/libvirt/libvirt/-/issues/90>;. Since there is
already
> > > support for network disks of different protocol types and host
> > information,
> > > we think that the only new XML information we will add is an <nfs>
> > element
> > > which will be a subelement of <source>, with attributes “user” and
> > “group”
> > > (both strings). This element will only be generated if the source
> > protocol
> > > is “nfs” and we assume that both “user” and “group” will be required.
> > >
> > > Here is the XML example given in the issue for reference:
> > >
> > > <disk type='network' device='disk'>
> > >
> > > <driver name='qemu' type='raw'/>
> > >
> > > <source protocol='nfs' name='PATH'>
> > >
> > > <host name='example.com' port='2049'/
> > >
> > > <nfs user='USER' group='GROUP'/>
> > >
> > > </source>
> > >
> > > <target dev='vda' bus='virtio'/>
> > >
> > > </disk>
> >
> > Sounds reasonable to me. We tend to name elements equivalent to <nfs>
> > you propose by their purpose (such as <auth> <initiator>
<cookies> for
> > other protocols) but in this case I don't have a better suggestion so
> > going with <nfs> is reasonable.
> >
> > Since you are proposing 'user' and 'group' to be strings while
qemu
> > accepts only numeric UID/GID, please use the same conversion code we
> > have for the <inituser> and <initgroup> values in regards to
forcing
> > numeric value to skip being interpreded:
> >
> > https://www.libvirt.org/formatdomain.html#container-boot
> >
> > The linked documentation
> <https://www.libvirt.org/formatdomain.html#container-boot> suggests that
> providing a “+” prefix will force attribute data to be interpreted
> numerically. Since QEMU requires that the group and user attributes be
> numeric id values, would we want to simply insert a “+” prefix to these
> attributes when they are provided explicitly (instead of using the
> hypervisor default values, which we assume will just be numeric data)?
No, not at all. QEMU requires you to provide an integer according to the
schema. That means that for any '+'-prefixed value, you strip the + and
convert it to int. For a username you look it up in the system to get
the uid and use that. We have helpers for this, but I can't recall the
name. You'll have to do some digging.
We found methods to convert user and group strings into integer IDs in
src/util/virutil.c (the getUserID and getGroupID methods). We plan on
checking if the parameter provided has a + at the front of the string, in
which case we will call these methods to convert the user or group name
into an ID. However, we aren’t sure where the _virStorageSource data is
actually being initialized. We found the
qemuBlockStorageSourceGetBackendProps method in src/qemu/qemu_block.c, for
which we would need to provide an NFS props method. At this point we need
the _virStorageSource to be initialized and contain the nfs_user and
nfs_group parameters, but we can’t find where to actually set these. You
had mentioned some kind of parser in domain_conf.c, but since domain_conf
is an extensive file, we had some trouble narrowing down our concerns to a
specific parsing/formatting mechanism. What method(s) would we change here?
Would that be satisfactory to set our _virStorageSource for use in the JSON
initialization?
12:44 a.m.
On Tue, Nov 24, 2020 at 15:25:01 -0600, Dustan B Helm wrote:
On Tue, Nov 24, 2020 at 12:59 AM Peter Krempa
<pkrempa(a)redhat.com> wrote:
> On Mon, Nov 23, 2020 at 17:17:15 -0600, Dustan B Helm wrote:
[...]
> We found methods to convert user and group strings into integer
IDs in
src/util/virutil.c (the getUserID and getGroupID methods). We plan on
checking if the parameter provided has a + at the front of the string, in
which case we will call these methods to convert the user or group name
into an ID.
virGetUserID/virGetGroupID have internal handling of the '+' prefix. You
can use them without adding any logic on top.
However, we aren’t sure where the _virStorageSource data is
actually being initialized. We found the
qemuBlockStorageSourceGetBackendProps method in src/qemu/qemu_block.c, for
which we would need to provide an NFS props method.
qemuBlockStorageSourceGetBackendProps is the place that convers
virStorageSource to a JSON object describing the source. Please note
that this function is not the place for any logic such as the
translation from username to the uid number. The function shall take a
virStorageSource and output a JSON struct, that's all.
At this point we need
the _virStorageSource to be initialized and contain the nfs_user and
nfs_group parameters, but we can’t find where to actually set these. You
had mentioned some kind of parser in domain_conf.c, but since domain_conf
is an extensive file, we had some trouble narrowing down our concerns to a
specific parsing/formatting mechanism. What method(s) would we change here?
Would that be satisfactory to set our _virStorageSource for use in the JSON
initialization?
There are two steps here:
1) you must parse/format the XML bits into a virStorage source
This is done in src/conf/domain_conf.c. Specifically you are dealing
with a storage source so the relevant functions are:
virDomainDiskSourceFormat and virDomainStorageSourceParse
2) The actual lookup from username/groupname to uid/gid
qemuDomainPrepareStorageSourceBlockdev
The two steps above means that you actually will have to add 4 fields to
virStorageSource. 2 strings for the values from the XML parser and then
2 integers for internal use for the virStorageSource -> JSON conversion.
Some notes:
- make sure that patches are split into logical parts and the code
compiles successfully after every patch as per contributor guidelines
https://www.libvirt.org/coding-style.html
https://www.libvirt.org/hacking.html
- when you add VIR_STORAGE_NET_PROTOCOL_NFS to enum virStorageNetProtocol
there will be a lot of places the compiler will notify that need to be
changed. Don't try to implement all of them at once. Add the entry to
the switch statement and come back to comply with splitting the patch
into logical pieces
- any XML addition requires tests to be added
In this case you'll be also needing to add a qemu commandline test
later so I suggest you add your XML test as a new file to:
tests/qemuxml2argvdata, and enable the test in
tests/qemuxml2xmltest.c (output file is stored in
tests/qemuxml2xmloutdata, you can also run the test with
VIR_TEST_REGENREATE_OUTPUT=1 env variable set which generates the
proper output file)
Always use one of the _CAPS_LATEST macros for invoking the test
regardless of prior art.
You can add multiple disks to the tested XML to test any combination
of the <nfs> parameters.
Please make sure to list at least one example of the NFS disk being
a <backingStore> of a <source>
(see tests/qemuxml2argvdata/disk-backing-chains.xml for inspiration)
Don't forget to also document the new elements in
docs/formatdomain.rst
- you'll also need to add proper backing store string parser
(virStorageSourceJSONDriverParser).
Tests for the parser are in tests/virstoragetest.c invoked via
TEST_BACKING_PARSE
- there's no need to add a specific capability for the NFS protocol as
it predates libvirt's use of -blockdev (QEMU_CAPS_BLOCKDEV). You have
to add a check for it to qemuDomainValidateStorageSource based on the
above capabapility.
Don't bother with a negative test case for this.
- Enabling support for external snapshots stored on NFS requires that
you also implement the support for blockdev-create
(qemuBlockStorageSourceCreateGetStorageProps, tests are in
tests/qemublocktest.c invoked via TEST_IMAGE_CREATE)
- once you add qemuBlockStorageSourceGetBackendProps, enable the test
case added for qemuxml2xmltest.c above also for qemuxml2argvtest.c
You can use the same trick with VIR_TEST_REGENERATE_OUTPUT env
variable to obtain the output file.
As noted, use _CAPS_LATEST macros to invoke the test.
qemuBlockStorageSourceGetBackendProps was historically tested by
TEST_DISK_TO_JSON cases in tests/qemublocktest.c for compliance with
the qemu monitor protocol schema, but that's no longer required.
qemuxml2argvtest does the same check for every <disk>
- add an entry to NEWS.rst outlining the addintion (separate commit)
4:03 p.m.
On Wed, Nov 25, 2020 at 12:44 AM Peter Krempa <pkrempa(a)redhat.com> wrote:
On Tue, Nov 24, 2020 at 15:25:01 -0600, Dustan B Helm wrote:
> On Tue, Nov 24, 2020 at 12:59 AM Peter Krempa <pkrempa(a)redhat.com>
wrote:
> > On Mon, Nov 23, 2020 at 17:17:15 -0600, Dustan B Helm wrote:
[...]
- any XML addition requires tests to be added
In this case you'll be also needing to add a qemu commandline test
later so I suggest you add your XML test as a new file to:
tests/qemuxml2argvdata, and enable the test in
tests/qemuxml2xmltest.c (output file is stored in
tests/qemuxml2xmloutdata, you can also run the test with
VIR_TEST_REGENREATE_OUTPUT=1 env variable set which generates the
proper output file)
Always use one of the _CAPS_LATEST macros for invoking the test
regardless of prior art.
You can add multiple disks to the tested XML to test any combination
of the <nfs> parameters.
Please make sure to list at least one example of the NFS disk being
a <backingStore> of a <source>
(see tests/qemuxml2argvdata/disk-backing-chains.xml for inspiration)
Don't forget to also document the new elements in
docs/formatdomain.rst
We don’t understand exactly how the testing frameworks are functioning.
With regards to our XML testing, the tests/qemuxml2argvdata/ directory
contains for each test both an xml file and an args file- is this .args
file the expected output qemu command we want to generate from the input
.xml file?
If so, how do we invoke the VIR_TEST_REGENERATE_OUTPUT flag to create this
.args file, and how concerned should we be about non-disk information
provided in the .xml file as input? Additionally, since our source name (a
file path) needs to include the NFS export at the beginning, how would we
set up the XML input? Can the file path and NFS export be chosen
arbitrarily?
8:13 p.m.
On Thu, Dec 03, 2020 at 16:03:55 -0600, Dustan B Helm wrote:
[...]
We don’t understand exactly how the testing frameworks are
functioning.
With regards to our XML testing, the tests/qemuxml2argvdata/ directory
contains for each test both an xml file and an args file- is this .args
file the expected output qemu command we want to generate from the input
.xml file?
Yes, qemuxml2argvtest exercises the XML parser and command line
formatter. In case of disk specification there are also provisions for
validating whether the generated -blockdev commandline actually conforms
to the schema of QEMU.
If so, how do we invoke the VIR_TEST_REGENERATE_OUTPUT flag to create
this
.args file,
It's an environment variable that the test program is looking for, thus:
VIR_TEST_REGENERATE_OUTPUT=1 ./tests/qemuxml2argvtest
in your build-directory.
(obviously you need to add invocation of the test first and compile the
program)
and how concerned should we be about non-disk information
provided in the .xml file as input?
Don't worry about them. At the same time there's no use in cramming too
much irrelevant non-disk config into the test.
Focus rather on adding multiple disks covering various combinations of
the configuration to avoid adding multiple test files.
Additionally, since our source name (a
file path) needs to include the NFS export at the beginning, how would we
set up the XML input? Can the file path and NFS export be chosen
arbitrarily?
Those are purely unit-tests, thus it doesn't actually try accessing the
storage in the first place. That means that it doesn't really matter
what path you choose.
1452
days inactive
1467
days old
12 comments
4 participants
participants (4)
-
Daniel P. Berrangé -
Dustan B Helm -
Laine Stump -
Peter Krempa