Hello @hsw_bitmark.com, I'm not doing FreeBSD stuff anymore, all my bits are taking into safekeeping.
The update seems fine but would be good for someone else to take a look at it.

In D26035#578290, @jhb wrote:

Definitely that is what will happen, somebody else will write a wrapper for this configuration file!!! The problem I see here is the standard(syntax and etc), it does not look like any configuration file I have ever see, and you know after you commit it, it will live there for forever, because we need to respect POLA.

I think you are conflating a few things. First, the syntax to me at least seems rather straight forward and even rather common (e.g. Windows INI files, the OpenSSL config file, even rc.conf) all use 'name=value' lines. It doesn't have [group] lines. I suppose if you really wanted that you could do that for nesting instead of a MIB name, but that seems equally unreadable.

Also, to be clear, the main thrust here is not really having a config file (that is more thrown in as a PoC of how one can combine both files and command line options to build a config), but it is really the nvlist tree and separating config parsing from acting on the config. That removes lots of dubious string parsing code from device models and also makes it much easier (IMO) to add new config variables in the future with minimal code changes. One of the things this model permits is adding config options to USB devices with previously were not permitted by the limited syntax of '-s' options used for XHCI controllers. The "flat" config file is basically the simplest type of config file format one could have (direct key=value without additional implicit knowledge). Right now we already require people using '-s' to know about which PCI slot devices live on, so the "flat" config file isn't worse, just equally non-ideal in that regard.

I think your actual concern is not the syntax, but the fact that the file maps 1:1 onto the set of config values and the stability of how the config tree is managed over time. Devices in particular I think are one of the hardest to reason about. I had originally toyed with trying to do a device namespace that looked a bit more like this:

device.0.type="ahci"
device.0.location="pci0:1:0"
device.0.path="disk.img"

etc. However, while more abstract, it is harder to deal with this sanely in bhyve itself as there may be parent-child dependencies this doesn't capture. Even the current 'pci.<bus>.<slot>.<function>' scheme isn't really ideal if we ever have support for PCI-PCI bridges (though it is doable, you do pci bus 0 first and do a depth-first traversal using the bridge bus numbers to look for devices instead of the current iteration over all possible values).

The other scheme I considered would be to have the device nodes be an actual tree reflecting their layout much like ACPI or FDT. USB devices are already following this somewhat by being hung under the xhci node, whereas lpc devices are not (they are not children of the lpc device). For PCI devices this would be more invasive if PCI-PCI bridges were added since that would then become more of a tree rather than the current flat layout it is now. However, this would also be more complex than the current layout which more naturally fit the existing device model initialization.

In D26035#578006, @jhb wrote:

In D26035#577636, @araujo wrote:

The configuration file is not intuitive at all, how I will know what pci slot my virtio-blk shall live?
Why not simplify it instead of complicate more? The configuration file supposed to be something to simplify the command line, and instead is getting more complex than bhyve cli itself. Perhaps we will need another io<something> wrapper to simplify the creation of bhyve configuration files.

This is much more complex than bhyve cli itself.
pci.0.0.0.device=hostbridge
pci.0.2.0.device=virtio-net
pci.0.2.0.backend=%(tap)
pci.0.3.0.device=virtio-blk
pci.0.3.0.path=/dev/zvol/zroot/bhyve/%(name)
pci.0.4.0.device=virtio-rnd
pci.0.31.0.device=lpc
lpc.com1.device=stdio
lpc.com2.device=/dev/nmdm%(name)2B
lpc.bootrom=/usr/local/share/uefi-firmware/BHYVE_UEFI.fd

Could we use a similar approach of the configuration file syntax like everybody else is using? I know make things simple is hard.

To be clear, one can build a UCL parser (I think I mentioned this on the list) that uses rules to write a backing config. I explicitly mentioned there that the UCL syntax could auto-compute the PCI slots to use while having an optional keyword to force a specific PCI slot if desired. Reading the config file is just a means of translating the config the UCL file likes into a set of configuration variable settings. That is, you could very much do something like:

In D26035#577644, @wanpengqian_gmail.com wrote:

In D26035#577636, @araujo wrote:

The configuration file is not intuitive at all, how I will know what pci slot my virtio-blk shall live?
Why not simplify it instead of complicate more? The configuration file supposed to be something to simplify the command line, and instead is getting more complex than bhyve cli itself. Perhaps we will need another io<something> wrapper to simplify the creation of bhyve configuration files.

Could we use a similar approach of the configuration file syntax like everybody else is using? I know make things simple is hard.

The config file here is for bhyve internal, it is accurate but complex.
Maybe other hypervisor manager, such as vm-bhyve will read their simple config file and output this config style for bhyve, then launch the bhyve process.

The configuration file is not intuitive at all, how I will know what pci slot my virtio-blk shall live?
Why not simplify it instead of complicate more? The configuration file supposed to be something to simplify the command line, and instead is getting more complex than bhyve cli itself. Perhaps we will need another io<something> wrapper to simplify the creation of bhyve configuration files.

LGTM!

Nice to see you back Hope all is good with you

LGTM, add the Approved by as usual.

Approve by: araujo

OK, as soon as the poudriere get the results, share it here.

Did you test it using poudriere?

How are you testing these changes that you didn't catch this problem before?

Ops my bad :)

If you will remove the port, why botter to update it and trigger hooks around to rebuild the package?

Wait...

@bcran You can commit it, you have my approval. Just add:

Have you ever take a look at: https://github.com/xcllnt/libvdsk
It seemed to me a better idea than what are you trying to achieve here, and there, there is an experimental implementation of qcow2.

It was committed already by @jhb at: https://svnweb.freebsd.org/changeset/base/355912

@jhb Does that make more sense?

• move the implementation to libvmmapi.

In D22615#494924, @daniel.engberg.lists_pyret.net wrote:

Thanks!

Committed at: https://svnweb.freebsd.org/changeset/ports/518840

Taking it.

LGTM, gimme couple hours and I will commit it.

@samm Can you move forward with this patch? The maintainer has timeout already.

So please, proceed to merge these two reviews and incorporate the submitter suggestions.

In D22045#483993, @samm wrote:

In D22045#483373, @araujo wrote:

@samm Could you double check with @daniel.engberg.lists_pyret.net the changes that he is proposing and perhaps after that we can close the D21969 review?

A single place makes more sense, however @daniel.engberg.lists_pyret.net opened the review first, so technically he will receive the credits for the patch submission. I just want a single review for this port update.

Best Regards,

Hi, i am perfectly fine to give him credits and merge the patches. Main problem is that there is no feedback from the maintainer. What should we do to get this done?

In D21969#483377, @daniel.engberg.lists_pyret.net wrote:

Sure, but I think it's a discouraging way to handle submissions in terms of chronological order.

@samm Could you double check with @daniel.engberg.lists_pyret.net the changes that he is proposing and perhaps after that we can close the D21969 review?

In D21969#483371, @daniel.engberg.lists_pyret.net wrote:

At least fix the issues reported in that case?

Can we abandon this review in favor of the another one?

LGTM, I will commit soon!!!

Lgtm!

You should bump PORTREVISION to force package rebuild.

In D22051#481744, @dmgk wrote:

The PHB seems to say otherwise:

PORTREVISION must be increased each time a change is made to the port that changes the generated package in any way.

Changing MAINTAINER does change package metadata (and the generated package as a consequence) and according to the above, needs a PORTREVISION bump. But looking at the svn logs, nobody deems this change "important enough" to warrant a rebuild :)

For record only, accepting it from Fukushima, Japan!

Why bump PORTREVISION?

You don't need the gratuitous PORTREVISION bump here!

Forgot to commit? Is there anything left to be reviewed?

In D22022#481030, @samm wrote:

Thank you for feedback!

In D22022#481027, @araujo wrote:

Do you use poudriere?
https://www.freebsd.org/doc/handbook/ports-poudriere.html

Yes, i am using it already

You must always test the changes using it, also when you open a review, you should fullfil the test area saying at least something like:

poudriere (11.2-, 12.0-RELEASE, 13.0-CURRENT@r348350 amd64 + i386) -> OK
portlint and portclippy -> OK
Runtime tests -> OK

Ok, was not aware of this, will do from the next submission. Only problem is that host for the poudriere is now running on 11.3, so 12+ tests will fail.

And will think how to automate all the process...

Probably will setup bhyve vm on 12+ to do the tests or will upgrade this host.

Do you use poudriere?
https://www.freebsd.org/doc/handbook/ports-poudriere.html

You have two "right" options, choose wisely :)

Two separate commits makes it easier to write the history log and in case you need one day to make a revert, it will be more easier to identify the reason. But it is not mandatory, but it is what I have been doing since 2007.

Sorry, I should have accepted it!!! But please wait for krion to approve it.

Thanks for that! You did right already!

I came here from D21838 to say: looks good too!!! :)

Lgtm, thank you!

Lgtm