Re: [PATCH 6/7] util: Added a backing store NFS parser
On Mon, Jan 4, 2021 at 8:41 AM Peter Krempa <pkrempa(a)redhat.com> wrote:
On Tue, Dec 29, 2020 at 15:21:28 -0600, Ryan Gahagan wrote:
> + src->nfs_uid = (uid_t) uidStore;
> + src->nfs_gid = (gid_t) gidStore;
This function must not fill in runtime data, just configuration. I
presume you did this to silence tests but you'll need to add a hack into
the test code rather than abusing this to fill runtime data.
Ideally in the future the runtime data will be split off into an opaque
sub-object so it will not be accessible in this code. Don't touch
nfs_uid/nfs_gid in this function at all.
We removed this code (specifically these assignments to nfs_uid and
nfs_gid) and did all the other refactors requested for this method.
Interestingly, the virstoragetest which tests this code still passes.
However, we're unaware of any "hack" in our test code which actually fixes
the missing uid and gid values. We're worried that this might indicate a
problem with our test. Where should this storage when parsing backing store
data actually be done?
For reference, here is the test
<https://gitlab.com/bschoney/libvirt/-/blob/issue-90-rt2/tests/virstoraget...
(on our branch after the changes) which is passing. Here
<https://gitlab.com/bschoney/libvirt/-/blob/issue-90-rt2/src/util/virstora...
is
the parse backing store method after the changes. Maybe we're just
overreacting but we figured asking to make sure would be better than
submitting a new patch only to find a problem.
Attachments:
- attachment.html (text/html — 1.9 KB)