Re: Issue 90 Further Clarifications

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx> 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)




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux