Bug 1481630 - Review Request: virtualbox-guest-additions - VirtualBox Guest Additions
Summary: Review Request: virtualbox-guest-additions - VirtualBox Guest Additions
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review   
(Show other bugs)
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Neal Gompa
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Keywords: Reopened
Depends On:
Blocks: 1534595
TreeView+ depends on / blocked
 
Reported: 2017-08-15 09:14 UTC by Hans de Goede
Modified: 2019-01-29 08:29 UTC (History)
13 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2018-05-10 08:10:33 UTC
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
ngompa13: fedora-review+


Attachments (Terms of Use)

Description Hans de Goede 2017-08-15 09:14:51 UTC
Spec URL: https://fedorapeople.org/~jwrdegoede/VirtualBox-guest-additions.spec
SRPM URL: https://fedorapeople.org/~jwrdegoede/VirtualBox-guest-additions-5.1.26-2.fc27.src.rpm
Description:
VirtualBox is a powerful x86 and AMD64/Intel64 virtualization product for
enterprise as well as home use. This package contains the VirtualBox
Guest Additions which support better integration of VirtualBox guests
with the Host, including file sharing, clipboard sharing, OpenGL
passthru and Seamless mode.

To use OpenGL passthru mode run apps using "VBoxOGLRun foo -opt1 -opt2".

Fedora Account System Username: jwrdegoede

Comment 1 c72578 2017-08-16 11:47:05 UTC
There seems to be a problem with the https certificate at download.virtualbox.org:
fedora-review -b 1481630 -m fedora-rawhide-x86_64
...
08-16 12:46 root         INFO     Downloading (Source0): https://download.virtualbox.org/virtualbox/5.1.26/VirtualBox-5.1.26.tar.bz2
08-16 12:46 root         DEBUG    Exception down the road...
CertificateError: hostname 'download.virtualbox.org' doesn't match either of '*.akamaized.net', '*.akamaihd-staging.net', '*.akamaized-staging.net', '*.akamaihd.net', 'a248.e.akamai.net'
...

An alternative could be to use the http link instead:
http://download.virtualbox.org/virtualbox/5.1.26/VirtualBox-5.1.26.tar.bz2

Comment 2 Neal Gompa 2017-08-16 11:54:56 UTC
Taking this review.

Comment 3 Hans de Goede 2017-08-16 12:32:34 UTC
Hi Neal,

Please note that the guest kernel modules are not yet in place in the Fedora kernel (I'm working on this) still it would be good to get this review done, so that this can land as soon as the kernel side is ready.

If you want to test things, here are the git repos where I'm cleaning up the 2 missing kernel drivers:

https://github.com/jwrdegoede/vboxguest
https://github.com/jwrdegoede/vboxsf

You need to build both modules, then copy them to /lib/modules/$(uname -r)/misc and then do a depmod -a before rebooting.

Also I see that the rawhide kernels do not yet have the vboxvideo driver enabled (which has landed upstream in 4.13). I've just fixed this, a scratchbuild with the fix is building here now:
https://koji.fedoraproject.org/koji/taskinfo?taskID=21262911

Regards,

Hans

Comment 4 Sergio Monteiro Basto 2017-08-16 12:58:01 UTC
Hello , I'm thinking what should write , of course I will do the review ASAP ( today evening or so, next weekend at most ) 
but I got some questions ... 
1 - and VirtualBox server package ? 
2 - And how about do this for VBox 5.2 beta ? , 5.2 beta we should have  

+    VBOX_USE_SYSTEM_GL_HEADERS=1
+    VBOX_NO_LEGACY_XORG_X11=1 \

instead use our patches ...

Comment 5 Hans de Goede 2017-08-16 13:23:54 UTC
(In reply to Sergio Monteiro Basto from comment #4)
> Hello , I'm thinking what should write , of course I will do the review ASAP
> ( today evening or so, next weekend at most ) 
> but I got some questions ... 
> 1 - and VirtualBox server package ? 

There are no plans to upstream the vbox host kernel modules, so the VirtualBox-server package will need to stay in rpmfusion. Nothing changes for the rpmfusion pkgs really, other then dropping the -guest-additions sub-package when this one is ready.

> 2 - And how about do this for VBox 5.2 beta ? , 5.2 beta we should have  

README.GuestAdditionsPackaging clearly state that upstream advises to use the latest *stable* release for packaging guest-additions, so that is something to worry about once it becomes stable, and then it will just be as any other package rebase to a new upstream, switch to the new upstream tarbal and fix what ever needs to be fixed.

Comment 6 Sergio Monteiro Basto 2017-08-17 11:38:52 UTC
Neal Gompa, please review it , thanks

Comment 7 Sergio Monteiro Basto 2017-08-20 01:13:26 UTC
Hi, 
After tweak fedora-review [1] I needed 2 patches [2] , I see one issue , the provides of libGL.so.1()(64bit) [3] can we remove this rpm provides ? 
Regards.

[1] 
https://copr.fedorainfracloud.org/coprs/sergiomb/builds_for_Stable_Releases/build/591889/

[2] 
0001-don-t-verify-certificates-of-ssl-connections.patch
0001-use-mock-old-chroot-to-not-break-some-scripts.patch


[3]

Provides
--------
VirtualBox-guest-additions:
    VirtualBox-guest-additions
    VirtualBox-guest-additions(x86-64)
    libGL.so.1()(64bit)

Comment 8 Neal Gompa 2017-08-21 18:32:45 UTC
I'm working on setting up a test environment for this, per Hans' comment in comment 3.

Comment 9 Nicolas Chauvet (kwizart) 2017-08-24 18:15:02 UTC
IIRC, you need something like this: (taken from libglvnd).

%global __provides_exclude_from %{_libdir}/%{name}
%global __requires_exclude_from %{_libdir}/%{name}

Comment 10 Hans de Goede 2017-08-28 09:37:25 UTC
Hi,

(In reply to Nicolas Chauvet (kwizart) from comment #9)
> IIRC, you need something like this: (taken from libglvnd).
> 
> %global __provides_exclude_from %{_libdir}/%{name}
> %global __requires_exclude_from %{_libdir}/%{name}

Thank you for that, that is much better then the old-fashioned provides filtering I had in mind.

Regards,

Hans

Comment 11 Hans de Goede 2017-08-28 13:05:55 UTC
(In reply to Sergio Monteiro Basto from comment #7)
> Hi, 
> After tweak fedora-review [1] I needed 2 patches [2] , I see one issue , the
> provides of libGL.so.1()(64bit) [3] can we remove this rpm provides ? 
> Regards.
> 
> [1] 
> https://copr.fedorainfracloud.org/coprs/sergiomb/builds_for_Stable_Releases/
> build/591889/

This link does not work.

> [2] 
> 0001-don-t-verify-certificates-of-ssl-connections.patch
> 0001-use-mock-old-chroot-to-not-break-some-scripts.patch

The pkg builds fine in mock for me, also I don't see these patches anywhere.

> 
> 
> [3]
> 
> Provides
> --------
> VirtualBox-guest-additions:
>     VirtualBox-guest-additions
>     VirtualBox-guest-additions(x86-64)
>     libGL.so.1()(64bit)

I've fixed this for the next release.

Comment 12 Hans de Goede 2017-08-28 13:06:41 UTC
Here is a new version:

Spec URL: https://fedorapeople.org/~jwrdegoede/VirtualBox-guest-additions.spec
SRPM URL: https://fedorapeople.org/~jwrdegoede/VirtualBox-guest-additions-5.1.26-3.fc27.src.rpm

Changelog:

* Mon Aug 28 2017 Hans de Goede <hdegoede@redhat.com> - 5.1.26-3
- Put the libGL.so.1 replacement libs and VBoxOGLRun scripts in an -ogl
  subpackage, so that people can install both the i686 and x86_64 versions.
- Filter out libGL.so.1 provides

Comment 13 Neal Gompa 2017-08-28 13:34:33 UTC
> BuildRequires:  kBuild >= 0.1.9998
> BuildRequires:  desktop-file-utils makeself yasm
> BuildRequires:  boost-devel
> BuildRequires:  libXcomposite-devel libXmu-devel libXrandr-devel libXt-devel
> BuildRequires:  mesa-libEGL-devel mesa-libGL-devel mesa-libGLU-devel
> BuildRequires:  pam-devel zlib-devel

Could you please break out all the BuildRequires onto newlines? It makes it easier to review...

Comment 14 Hans de Goede 2017-08-28 14:17:40 UTC
(In reply to Neal Gompa from comment #13)
> > BuildRequires:  kBuild >= 0.1.9998
> > BuildRequires:  desktop-file-utils makeself yasm
> > BuildRequires:  boost-devel
> > BuildRequires:  libXcomposite-devel libXmu-devel libXrandr-devel libXt-devel
> > BuildRequires:  mesa-libEGL-devel mesa-libGL-devel mesa-libGLU-devel
> > BuildRequires:  pam-devel zlib-devel
> 
> Could you please break out all the BuildRequires onto newlines? It makes it
> easier to review...

Ok, I've updated the .spec and the -3 src.rpm with this change, I've not bumped the release given the very minor nature of this change.

Comment 15 Sergio Monteiro Basto 2017-08-28 14:45:30 UTC
(In reply to Hans de Goede from comment #11)
> (In reply to Sergio Monteiro Basto from comment #7)
> > Hi, 
> > After tweak fedora-review [1] I needed 2 patches [2] , I see one issue , the
> > provides of libGL.so.1()(64bit) [3] can we remove this rpm provides ? 
> > Regards.
> > 
> > [1] 
> > https://copr.fedorainfracloud.org/coprs/sergiomb/builds_for_Stable_Releases/
> > build/591889/
> 
> This link does not work.

Link is here : https://copr.fedorainfracloud.org/coprs/sergiomb/builds_for_Stable_Releases/package/fedora-review/ 

> 
> > [2] 
> > 0001-don-t-verify-certificates-of-ssl-connections.patch
> > 0001-use-mock-old-chroot-to-not-break-some-scripts.patch
> 
> The pkg builds fine in mock for me, also I don't see these patches anywhere.

Patches are here : http://copr-dist-git.fedorainfracloud.org/cgit/sergiomb/builds_for_Stable_Releases/fedora-review.git/tree/

But, patch 0001 we don't need anymore, because now you change https to http in source0 .
The patch 002 if a problem in my system and kde4 in general that /etc/localtime wasn't a symlink . 

Cheers.

Comment 16 Neal Gompa 2017-09-13 09:26:35 UTC
@Hans:

The package looks good to me, but I don't want to approve it until you've pinned down the kernel that will support these drivers.

Can you please get this figured out?

Comment 17 Hans de Goede 2017-09-13 10:03:39 UTC
Hi Neal,

Thank you for the review and I understand. 2 weeks ago Virtual Box upstream let me know that they want to do some cleanups on the vboxguest character device ioctl API before they are willing to declare it stable and promise they will not break the API in the future. Virtual Box upstream is currently working on these cleanups.

Once these cleanups are done, I will need to adjust my kernel patches to match (*) and then submit a new version of the vbox patches to the upstream Linux kernel. So for now I'm waiting on Virtual Box upstream. I will let you know (here in this bug) when there is some progress.

Regards,

Hans


*) and do a new version of the VirtualBox-guest-additions package implementing the new API)

Comment 18 Neal Gompa 2017-09-13 10:09:11 UTC
Thanks!

Comment 19 Neal Gompa 2017-09-24 20:59:43 UTC
Hans,

What progress has been made on the drivers?

Comment 20 Hans de Goede 2017-09-25 11:39:19 UTC
Hi,

I got a headsup from upstream last Wednesday that the new stable ioctl interface is in place. They did more changes to the ioctl interface then I expected. So now I'm adjusting my cleaned up version of the driver to match the new ioctl interface, I hope to have that done and submit a non RFC v1 of the patch for inclusion into the mainline kernel in 2-3 days.

Regards,

Hans

Comment 21 Johnny Robeson 2017-09-27 21:29:01 UTC
d

Comment 22 Neal Gompa 2017-09-28 00:59:06 UTC
(In reply to Hans de Goede from comment #20)
> Hi,
> 
> I got a headsup from upstream last Wednesday that the new stable ioctl
> interface is in place. They did more changes to the ioctl interface then I
> expected. So now I'm adjusting my cleaned up version of the driver to match
> the new ioctl interface, I hope to have that done and submit a non RFC v1 of
> the patch for inclusion into the mainline kernel in 2-3 days.
> 

Excellent!

Comment 23 Johnny Robeson 2017-09-28 02:44:51 UTC
ah sorry for that d there. I just meant to CC.

Can there be a subpackage just for the non gui related bits like the shared mounting ?

Comment 24 Hans de Goede 2017-10-04 07:10:18 UTC
(In reply to Hans de Goede from comment #20)
> Hi,
> 
> I got a headsup from upstream last Wednesday that the new stable ioctl
> interface is in place. They did more changes to the ioctl interface then I
> expected. So now I'm adjusting my cleaned up version of the driver to match
> the new ioctl interface, I hope to have that done and submit a non RFC v1 of
> the patch for inclusion into the mainline kernel in 2-3 days.

Ok, so I've just send v1 (first non RFC version) of the patch to add the vboxguest driver to the mainline kernel upstream.

I also have prepared a new version of the package based on the 5.2 pre-releases as a 5.2 version of the Guest Additions is necessary since that implements the new ioctl API as it is (hopefully) going upstream:

Spec URL: https://fedorapeople.org/~jwrdegoede/VirtualBox-guest-additions.spec
SRPM URL: https://fedorapeople.org/~jwrdegoede/VirtualBox-guest-additions-5.2.0-0.1.svn68769.fc27.src.rpm

This is based on a svn snapshot as vbox upstream is not releasing source tarbals for the pre-releases.

Comment 25 Neal Gompa 2017-10-19 03:47:33 UTC
So VirtualBox 5.2 released... Is there an update to this based on the final release? Also, how has upstreaming the kernel part gone?

Comment 26 Hans de Goede 2017-10-19 08:13:58 UTC
(In reply to Neal Gompa from comment #25)
> So VirtualBox 5.2 released... Is there an update to this based on the final
> release?

Yes, which is good news as that implements the same vboxguest ioctl API as which will hopefully go upstream. I've not yet created updated packages, I have put that on my todo list. May take a week or so before I get around to this.

> Also, how has upstreaming the kernel part gone?

v1 result in upstream requesting some more rework, I finished that last week and submitted v2. So far things have been quiet around v2, so it is mostly waiting for some people to find time to review it atm.

Comment 27 Sergio Monteiro Basto 2017-10-29 23:52:45 UTC
@Hans, I'm reviewing VirtualBox packages , to adjust guest 3D passthru, and I have some questions .
1 - you do not add /usr/lib64/VBoxEGL.so to package ? 

2- Reading VBoxOGLRun.sh  we got [1] , shouldn't we enable that as system wide , by default in all system ?  

My plan is update Vbox packages with 5.1.30 in all branches and after that begin to pack 5.2 in rawhide and F27, we / I may start thinking in drop guest-additions in RPMFusion packages or something like that and plan to coordinate the existence of the 2 src.rpms in repos. 

3- Fedora kernels for f27 or rawhide already have vboxguest kernel modules ? or do we need still enable it ? (this part still not studied, but for testing we need one kernel with the modules ... )  

[1]
if VBoxClient --check3d; then
    export LD_LIBRARY_PATH=/usr/lib64/VBoxGuestAdditions:/usr/lib/VBoxGuestAdditions

Comment 28 Neal Gompa 2017-10-30 00:50:02 UTC
(In reply to Sergio Monteiro Basto from comment #27)
> @Hans, I'm reviewing VirtualBox packages , to adjust guest 3D passthru, and
> I have some questions .
> 1 - you do not add /usr/lib64/VBoxEGL.so to package ? 
> 
> 2- Reading VBoxOGLRun.sh  we got [1] , shouldn't we enable that as system
> wide , by default in all system ?  
> 
> My plan is update Vbox packages with 5.1.30 in all branches and after that
> begin to pack 5.2 in rawhide and F27, we / I may start thinking in drop
> guest-additions in RPMFusion packages or something like that and plan to
> coordinate the existence of the 2 src.rpms in repos. 
> 
> 3- Fedora kernels for f27 or rawhide already have vboxguest kernel modules ?
> or do we need still enable it ? (this part still not studied, but for
> testing we need one kernel with the modules ... )  
> 
> [1]
> if VBoxClient --check3d; then
>     export
> LD_LIBRARY_PATH=/usr/lib64/VBoxGuestAdditions:/usr/lib/VBoxGuestAdditions

vboxvideo seems to have been activated in the Fedora 27 kernel, but vboxguest is still missing...

Comment 29 Neal Gompa 2017-10-30 00:51:01 UTC
Hans,

Any update on the state of VirtualBox guest addition kmods for the Fedora 27 kernel?

Comment 30 Hans de Goede 2017-10-30 11:20:24 UTC
Hi,

(In reply to Sergio Monteiro Basto from comment #27)
> @Hans, I'm reviewing VirtualBox packages , to adjust guest 3D passthru, and
> I have some questions .
> 1 - you do not add /usr/lib64/VBoxEGL.so to package ? 

Yes, because it is broken the upstream guest-additions install script also no longer installs it. With this in LD_LIBRARY_PATH gnome will no longer work in Wayland mode.

> 2- Reading VBoxOGLRun.sh  we got [1] , shouldn't we enable that as system
> wide , by default in all system ?  

The opengl passthru code is not all that great:

1) It provides a very old version of opengl, 2.1, many apps including e.g. webGL in firefox will not work with it
2) It makes any GL rendered surfaces being always on top
3) It does work with gnome-shell on top of X11 but the always on top thing then breaks running any OpenGL apps on top of gnome-shell, when you do that everything becomes horribly slow and the window gets no window decorations

TL;DR: the OpenGL passthru support is not in a state where enabling it system-wide is a good idea. This is also why it is disabled by default if you create a new vm.

> My plan is update Vbox packages with 5.1.30 in all branches and after that
> begin to pack 5.2 in rawhide and F27, we / I may start thinking in drop
> guest-additions in RPMFusion packages or something like that and plan to
> coordinate the existence of the 2 src.rpms in repos. 
> 
> 3- Fedora kernels for f27 or rawhide already have vboxguest kernel modules ?
> or do we need still enable it ? (this part still not studied, but for
> testing we need one kernel with the modules ... )  

As Neal already replied vboxvideo is in place I'm still waiting for upstream review of my v2 patch to add vboxguest support upstream. I will go and ping some people about this.

Regards,

Hans

Comment 31 Neal Gompa 2017-11-04 14:52:01 UTC
Hans,

Any progress on vboxsf kernel module? IIRC, that's the third one provided by the guest additions...

Comment 32 Hans de Goede 2017-11-06 14:24:39 UTC
(In reply to Neal Gompa from comment #31)
> Hans,
> 
> Any progress on vboxsf kernel module? IIRC, that's the third one provided by
> the guest additions...

Correct that is the third module. vboxsf depends on IPC provided by vboxguest, I've it ready for upstream submission but first I need to get vboxguest upstream.

I've pinged one of the upstream maintainers asking for a review of vboxguest.

Comment 33 Sergio Monteiro Basto 2017-11-13 21:30:22 UTC
(In reply to Hans de Goede from comment #9)
> Hi,
> 
> So I've just finished packaging the vbox guest-additions for Fedora proper:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1481630
> 
> I've solved the OpenGL issue there by putting all libs under
> %{_libdir}/VBoxGuestAdditions and
> offering a script to run apps through OpenGL passthru like this:
> 
> VBoxOGLRun glxgears
> 
> I've opted to go this route rather then auto-enable at boot since OpenGL
> passthru has some issues with some apps (and does not work when using
> gnome-shell + other opengl apps on top).

I had modified RPMFusion vbox package with these changes ( just in F27 and rawhide)   
I haven't check it yet 


> With this fix, the ldconfig calls can be moved from the %post scripts, 

hum , I have to check this , I could run tuxcart in a vm without OGL .

> I've
> also removed VirtualBox-guest.modules and the restart of
> modules-load.service since all guest modules will properly autoload so this
> is not necessary.

Fedora kernel already have all modules for guest machines ?

> Feel free to pick up these changes for the rpmfusion pkgs :)

kmk need this [1] all least in my case ... 

[1] 
+    VBOX_PATH_APP_PRIVATE=%{_libdir}/virtualbox \
+    VBOX_PATH_APP_DOCS=%{_docdir}/VirtualBox    \

Comment 34 Sergio Monteiro Basto 2017-11-14 10:07:39 UTC
I was comment https://bugzilla.rpmfusion.org/show_bug.cgi?id=4617#c9

Comment 35 Sergio Monteiro Basto 2017-11-22 23:57:39 UTC
(In reply to Sergio Monteiro Basto from comment #33)

> > With this fix, the ldconfig calls can be moved from the %post scripts, 
> 
> hum , I have to check this , I could run tuxcart in a vm without OGL .

Tuxkart and extreme tux racer still runs well, as they have some king of 3D enabled , so I went forward and applied updates of vbox also in stable branches. 

Now we need prepare update for Vbox 5.2 

Cheers.

Comment 36 Neal Gompa 2017-11-23 15:48:23 UTC
Hans,

Any news on vboxguest and vboxsf?

Comment 37 Hans de Goede 2017-11-23 15:55:02 UTC
(In reply to Neal Gompa from comment #36)
> Hans,
> 
> Any news on vboxguest and vboxsf?

Actually I send a mail this morning asking another kernel developer if he would be willing to help me out by reviewing the vboxguest driver, if he says yes then I'm going to ask the misc. subsys maintainers if that works for them. TL;DR: still in upstream review process limbo I'm afraid.

Comment 38 Hans de Goede 2017-12-14 15:28:29 UTC
Here is a new version of the pkg, updated to 5.2.2:

SPEC: https://fedorapeople.org/~jwrdegoede/VirtualBox-guest-additions.spec
SRPM: https://fedorapeople.org/~jwrdegoede/VirtualBox-guest-additions-5.2.2-1.fc27.src.rpm

Unfortunately the kernel patches are still awaiting review by the misc subsys maintainers upstream. I did manage to get a review done by another kernel contributor (and addressed his comments), so hopefully something will happen wrt this soon.

Comment 39 Sergio Monteiro Basto 2017-12-14 16:20:20 UTC
Hi Hans, 

Did you see this thread [1] I replied please "run app with VBoxOGLRun.sh or
export LD_LIBRARY_PATH=/usr/lib64/VBoxGuestAdditions:/usr/lib/VBoxGuestAdditions" 
which Vic replied with : "I already set LD_LIBRARY_PATH as a  temporary workaround.

Thanks" 

Conclusion, maybe we need enable LD_LIBRARY_PATH all the time , or may we move some files from /usr/lib/VBoxGuestAdditions to /usr/lib/ ? 

Thanks,


[1] 
https://lists.rpmfusion.org/archives/list/rpmfusion-users@lists.rpmfusion.org/thread/L365ROTMFLGVRBJCGT35FRCBLLNGV2PX/

Comment 40 Hans de Goede 2017-12-14 18:20:08 UTC
(In reply to Sergio Monteiro Basto from comment #39)
> Did you see this thread [1]

Weird, Vic writes: "Some apps like atril and emacs are failing because VBoxOGLcrutil.so can't be found.", but the only thing requiring VBoxOGLcrutil.so should be the replacement libGL.so.1 which only gets used if LD_LIBRARY_PATH is set in the first place.

I've the feeling that Vic somehow has a ld.so.cache entry pointed to the vbox 
libGL.so.1. I'm not on rpmfusion-users, can you ask him to file a bugzilla so we can discuss this there? Also ask him to check for ld.so.conf.d files which may explain this.

Comment 41 Nicolas Chauvet (kwizart) 2017-12-14 18:38:53 UTC
(In reply to Hans de Goede from comment #38)
> Here is a new version of the pkg, updated to 5.2.2:
> 
> SPEC: https://fedorapeople.org/~jwrdegoede/VirtualBox-guest-additions.spec
> SRPM:
> https://fedorapeople.org/~jwrdegoede/VirtualBox-guest-additions-5.2.2-1.fc27.
> src.rpm
> 
> Unfortunately the kernel patches are still awaiting review by the misc
> subsys maintainers upstream. I did manage to get a review done by another
> kernel contributor (and addressed his comments), so hopefully something will
> happen wrt this soon.

Quick review with few minors things:

* Keep from using requires on kernel from userspace. Most of the time this is invalid (you could be within a chroot , container or systemd-nspawn, etc). Also you need to select the appropriate variant (on x86_64 kernel-debug exists or even kernel-rt).
# FIXME once kernel modules have landed
#Requires:       kernel >= 4.FIXME

* Please use pkgconf --libs libpng and etc not to hardcode one particular library path. (say if there is a  need to use one particular scl at some point).

* Is there a way "not to hardcode the vendor", such as:
VBOX_BUILD_PUBLISHER=_%{?vendor}%{?!vendor:Unknown}

* The replacement of the RPM Fusion package should works for the "userspace" but not much for the kmod-VirtualBox. Best is probably to keep a (virtual) Provides: VirtualBox-kmod-common = 5.2.2 (the last version of the rpmfusion counterpart before it is disabled). So that kmod-Virtualbox is kept while it will not try to seek the main VirtualBox(host) capability.
Then, it will only be a matter for the kernel

Obsoletes/Provides for the akmod isn't an option because it will affect host kmod-VirtualBox and VirtualBox users.

Another option is probably to only have the Obsoletes: akmod-VirtualBox < 5.22-100
(needs testing).

Comment 42 Hans de Goede 2017-12-19 11:10:19 UTC
Good news, the vboxguest kernel module patches have been merged into char-misc-next. If all goes well and they do not get reverted in a couple of days I will add them to the rawhide kernels as downstream patches for now and then we can finally move forward with this review. Note we will then still miss the vboxsf driver. I will submit a much cleaned up version of that upstream soon and then we will see from there.

Nicolas, thank you for your comments, I will try to address those in a new version which I will prepare once the kernel side is sorted out.

Comment 43 Hans de Goede 2017-12-19 21:21:27 UTC
(In reply to Nicolas Chauvet (kwizart) from comment #41)
> * Keep from using requires on kernel from userspace. Most of the time this
> is invalid (you could be within a chroot , container or systemd-nspawn,
> etc).

Where one would pretty much never install VirtualBox-guest-additions...

> Also you need to select the appropriate variant (on x86_64
> kernel-debug exists or even kernel-rt).
> # FIXME once kernel modules have landed
> #Requires:       kernel >= 4.FIXME

Ok, so I'm thinking to make the upgrade path issues easier to only
add the VirtualBox-guest-additions to Fedora's repos for F28+ in which case we can simply assume the kernel-bits will be there and not have this. How does this sound from an rpmfusion pov, this should make things easy for rpmfusion too, simply disable the VirtualBox-guest-additions subpackage for F28+.

> * The replacement of the RPM Fusion package should works for the "userspace"
> but not much for the kmod-VirtualBox. Best is probably to keep a (virtual)
> Provides: VirtualBox-kmod-common = 5.2.2 (the last version of the rpmfusion
> counterpart before it is disabled). So that kmod-Virtualbox is kept while it
> will not try to seek the main VirtualBox(host) capability.
> Then, it will only be a matter for the kernel

So what you're suggesting is that people who upgrade a virtual-machine with the rpmfusion akmod installed, keep the akmod, but now it will only built (unused) vbox host modules, instead of building both host _ guest modules, correct?

That sounds reasonable, this should also be easy (change the akmod to only build host modules) if we make the switch for F28+ and keep older releases as is.

Note I will try to fix your other remarks my next version.

Comment 44 Zbigniew Jędrzejewski-Szmek 2018-01-04 13:59:50 UTC
Package name should be virtualbox-guest-additions. https://fedoraproject.org/wiki/Packaging:Naming#General_Naming says "Package names should be in lower case" and that's the common practice nowadays. Provides/Obsoletes for the capitalized name are enough to help people upgrade.

http://www.virtualbox.org/wiki/VirtualBox → https://

Likewise, https:// should be used for the download URL. Even if fedora-review had some issue with the certificate, not downloading over http is more important than what fedora-review thinks.

> # FIXME once kernel modules have landed
> #Requires:       kernel >= 4.FIXME
Like kwizart already said, this can never work. You can just drop this part.

BuildRequires: systemd is necessary for %{?systemd_requires}.

96-vbox.preset must be dropped, and the preset must be added following the normal procedue in https://fedoraproject.org/wiki/Starting_services_by_default.

> ConditionVirtualization=|oracle
Nice! This should make the package a total noop on any other system.

Comment 45 Neal Gompa 2018-01-04 16:32:10 UTC
> Package name should be virtualbox-guest-additions. https://fedoraproject.org/wiki/Packaging:Naming#General_Naming says "Package names should be in lower case" and that's the common practice nowadays. Provides/Obsoletes for the capitalized name are enough to help people upgrade.

I actually would vote against this, as it's been packaged as "VirtualBox-guest-additions" for long enough and the lowercase thing isn't mandatory (cues from other packagers packaging it are expressly suggested for figuring out names). If you want to add "virtualbox-guest-additions" Provides, that's fine.

That said, if you want to make the change, I won't block it, but I will say you will *have* to have "VirtualBox-guest-additions" work as a way to install it no matter what.

Comment 46 Zbigniew Jędrzejewski-Szmek 2018-01-04 16:35:12 UTC
I agree that VirtualBox-guest-additions must be provided either way. But I think the lowercase name is much more in agreement with current style and the guidelines. It also matches the service name. The fact that there was an external package matter that much.

Comment 47 Nicolas Chauvet (kwizart) 2018-01-08 22:27:05 UTC
Yep I would appreciate a lower case package name also. I find hard to remember one or another package name often, so not to take the case into account. Using lower case all along make things easier to remember.
(with Obsoletes/Provides in the current package case for compat anyway).

About obsoleting the kmod, I think the only good way would be to only use:
Obsoletes: akmods-VirtualBox < 5.1.31 #whatever version is introduced in fedora
(so without any provides on purpose, not to mess with VirtualBox HYP users).

That been said, I think there is few guest users (people with the virtualbox guest package installed) compared with host one.
(at least I don't install guest drivers on my vbox guests).

Comment 48 Sergio Monteiro Basto 2018-01-09 16:35:27 UTC
(In reply to Hans de Goede from comment #40)
> (In reply to Sergio Monteiro Basto from comment #39)
> > Did you see this thread [1]
> 
> Weird, Vic writes: "Some apps like atril and emacs are failing because
> VBoxOGLcrutil.so can't be found.", but the only thing requiring
> VBoxOGLcrutil.so should be the replacement libGL.so.1 which only gets used
> if LD_LIBRARY_PATH is set in the first place.
> 
> I've the feeling that Vic somehow has a ld.so.cache entry pointed to the
> vbox 
> libGL.so.1. I'm not on rpmfusion-users, can you ask him to file a bugzilla
> so we can discuss this there? Also ask him to check for ld.so.conf.d files
> which may explain this.

Hello , Vic replied was an problem with an guest installation with oracle tools , maybe is not a bad idea check and or clean old installations of guest (since with oracle tool, installation may not be cleaned) here is the transcription of what I think that was relevant: 

"I took a look, but there was nothing of interest in /etc/ld.so.conf.d/, but that did lead me to find an old copy of the Guest Additions in /opt.  I uninstalled that, and re-installed the RPM.  All seems to be well now. (without LD_LIBRARY_PATH)
It seems my problem was self-inflicted.  Apologies." 


Best regards,

Comment 49 Sergio Monteiro Basto 2018-01-15 14:17:05 UTC
(In reply to Hans de Goede from comment #38)
> Here is a new version of the pkg, updated to 5.2.2:
> 
> SPEC: https://fedorapeople.org/~jwrdegoede/VirtualBox-guest-additions.spec
> SRPM:
> https://fedorapeople.org/~jwrdegoede/VirtualBox-guest-additions-5.2.2-1.fc27.
> src.rpm
> 
> Unfortunately the kernel patches are still awaiting review by the misc
> subsys maintainers upstream. I did manage to get a review done by another
> kernel contributor (and addressed his comments), so hopefully something will
> happen wrt this soon.

Hello
why keep VirtualBox-5.0.18-xserver_guest.patch [1] ?, I think the new kmk option VBOX_USE_SYSTEM_GL_HEADERS=1 do exactly the same or at least is for that propose .

Thanks

[1]
Patch1:     VirtualBox-5.0.18-xserver_guest.patch

Comment 50 Hans de Goede 2018-01-29 09:13:24 UTC
(In reply to Sergio Monteiro Basto from comment #33)
> kmk need this [1] all least in my case ... 
> 
> [1] 
> +    VBOX_PATH_APP_PRIVATE=%{_libdir}/virtualbox \
> +    VBOX_PATH_APP_DOCS=%{_docdir}/VirtualBox    \

That is not needed when building just the additions, the additions do not install any files into these paths.

(In reply to Nicolas Chauvet (kwizart) from comment #41)
> (In reply to Hans de Goede from comment #38)
> Quick review with few minors things:
> 
> * Keep from using requires on kernel from userspace. Most of the time this
> is invalid (you could be within a chroot , container or systemd-nspawn,
> etc). Also you need to select the appropriate variant (on x86_64
> kernel-debug exists or even kernel-rt).
> # FIXME once kernel modules have landed
> #Requires:       kernel >= 4.FIXME

Fixed in the next version.

> * Please use pkgconf --libs libpng and etc not to hardcode one particular
> library path. (say if there is a  need to use one particular scl at some
> point).

Fixed in the next version.

> * Is there a way "not to hardcode the vendor", such as:
> VBOX_BUILD_PUBLISHER=_%{?vendor}%{?!vendor:Unknown}

No that is not going to work, that would set the vendor to "_Fedora Project"
and it cannot contain spaces and I think that is too long too. vbox is very picky about the version string into which this gets embedded. So it is best to just hardcode this.

> * The replacement of the RPM Fusion package should works for the "userspace"
> but not much for the kmod-VirtualBox. Best is probably to keep a (virtual)
> Provides: VirtualBox-kmod-common = 5.2.2 (the last version of the rpmfusion
> counterpart before it is disabled). So that kmod-Virtualbox is kept while it
> will not try to seek the main VirtualBox(host) capability.
> Then, it will only be a matter for the kernel
> 
> Obsoletes/Provides for the akmod isn't an option because it will affect host
> kmod-VirtualBox and VirtualBox users.

Ok, I ve added a:

Provides:       VirtualBox-kmod-common

To the .spec for the next version.

(In reply to Zbigniew Jędrzejewski-Szmek from comment #44)
> Package name should be virtualbox-guest-additions.

Fixed in the next version.

(In reply to Sergio Monteiro Basto from comment #49)
> why keep VirtualBox-5.0.18-xserver_guest.patch [1] ?

Even with VBOX_USE_SYSTEM_GL_HEADERS=1 there still is one small issue breaking compilation, in my pkg this is a modified version of your original patch keeping only the small chunk still necessary.

Comment 51 Hans de Goede 2018-01-29 10:48:07 UTC
With the upcoming 4.16-rc1 coming to rawhide soon, which will include the vboxguest driver, I believe we are ready to complete this review and get virtualbox-guest-additions into Rawhide / Fedora 28 (and only there).

Here is a new version, addressing all review comments made:

Spec URL: https://fedorapeople.org/~jwrdegoede/VirtualBox-guest-additions.spec
SRPM URL: https://fedorapeople.org/~jwrdegoede/virtualbox-guest-additions-5.2.6-3.fc27.src.rpm

Changelog:

* Mon Jan 29 2018 Hans de Goede <hdegoede@redhat.com> - 5.2.6-3
- Update to 5.2.6
- Drop VirtualBox-4.3.0-no-bundles.patch, set make variables instead
- Adjust automount vboxservice for mainline vboxsf filesystem driver
- Drop mount.vboxsf, the mainline vboxsf filesystem driver works with the
  regular mount binary
- Drop commented out Requires: kernel, this is bad idea (rhbz#1534595)
- Use pkgconfig to get include/libs instead of hardcoding (rhbz#1534595)
- Rename to lowercaps virtualbox-guest-additions, add Obsoletes / Provides
  for upgradepath from rpmfusion (rhbz#1534595)
- Add Provides: VirtualBox-kmod-common for rpmfusion upgradepath (rhbz#1534595)
- Latest rpmfusion Release is 2, set our Release field to 3

If someone can do an official review and approve this (or let me know what needs to be fixed) then that would be great.

Comment 52 Neal Gompa 2018-01-29 13:02:32 UTC
@Hans,

The spec file doesn't include this new change. Can you please upload it?

Comment 53 Hans de Goede 2018-01-29 13:05:43 UTC
(In reply to Neal Gompa from comment #52)
> @Hans,
> 
> The spec file doesn't include this new change. Can you please upload it?

It is already there, but I messed up the link (Caps vs caps) because of the name change, correct link:

https://fedorapeople.org/~jwrdegoede/virtualbox-guest-additions.spec

Sorry & Thank you,

Hans

Comment 54 Nicolas Chauvet (kwizart) 2018-01-29 13:26:40 UTC
There is nothing holding review maybe exept that the 4.16-rc1 kernel lands (or maybe even before rc1). Does it have everything needed ?
I plan to do some runtime tests once everything has landed in the coming weeks.

Once this guest addition package will be created, I think it will be updated along the rpmfusion VirtualBox-guest-addition package. As I expect, this last will be disabled for f28+, but continue to exist for older fedora,rhel.
Based on this I think it's indeed correct to have the obsoletes/provide to use a version macro. So everything is fine (maybe only to have a comment to schedule the removal of obsoletes/provides for f28+2 or whatever rhel version is relevant).

One item I wonder if it can be improved is the "vboxsf" user/group creation:
(which I think is the correct way to have this user/group created). But:

Is this really needed ? Can this be avoided in the long run so vbox graphical users don't need to add themselves to this group manually ?

Comment 55 Hans de Goede 2018-01-29 13:38:31 UTC
(In reply to Nicolas Chauvet (kwizart) from comment #54)
> There is nothing holding review maybe except that the 4.16-rc1 kernel lands
> (or maybe even before rc1). Does it have everything needed ?

4.16-rc1 has the vboxguest driver, but it is lacking the vboxsf driver which is necessary for shared folder support. I've submitted this upstream and it has been reviewed and I've posted a new version addressing all review comments. So hopefully vboxsf will get accepted into next soonish and then I can add it as a downstream patch to Fedora. That is plan A, plan B is to ship without shared folder support. If we end up with plan B we should probably have the akmod-VirtualBox package build vboxsf instead of it not building any guest modules at all. 

> I plan to do some runtime tests once everything has landed in the coming
> weeks.
> 
> Once this guest addition package will be created, I think it will be updated
> along the rpmfusion VirtualBox-guest-addition package. As I expect, this
> last will be disabled for f28+, but continue to exist for older fedora,rhel.
> Based on this I think it's indeed correct to have the obsoletes/provide to
> use a version macro. So everything is fine (maybe only to have a comment to
> schedule the removal of obsoletes/provides for f28+2 or whatever rhel
> version is relevant).

Ok, we also need to modify akmod-VirtualBox to not build the guest drivers on F-28+, as discussed before we will keep it around on existing guests which upgrade to F28 to avoid playing obsolete tricks which will cause problems on vbox-host installations.

> One item I wonder if it can be improved is the "vboxsf" user/group creation:
> (which I think is the correct way to have this user/group created). But:
> 
> Is this really needed ? Can this be avoided in the long run so vbox
> graphical users don't need to add themselves to this group manually ?

The vboxsf user/group is used by shared folders marked as automount, these get mounted under /media and are only readable/writable by users in the vboxsf group.

Comment 56 Neal Gompa 2018-01-29 17:37:01 UTC
At this point, I see no remaining issues with the VirtualBox Guest Additions packaging.

PACKAGE APPROVED.

Comment 57 Sergio Monteiro Basto 2018-01-30 01:13:11 UTC
Hi , 
comment diff between specs 5.2.2 and 5.2.6 

1 -
+# Small compile fix 
 Patch1:     VirtualBox-5.0.18-xserver_guest.patch

yeah something happened and upstream doesn't do the VBOX_NO_LEGACY_XORG_X11=1 that is not working as expect so we still need part of  VirtualBox-5.0.18-xserver_guest.patch

2 -
+# Mainline vboxsf uses an option string rather then a custom binary data struct
+Patch2:     0001-VBoxServiceAutoMount-Change-Linux-mount-code-to-use-.patch

-obj/bin/additions/mount.vboxsf

-%{_sbindir}/mount.vboxsf

Share folders still work ?

3- 
+Provides:       VirtualBox-kmod-common = %{version}-%{release}

haven't yet express my opinion , but I tested VirtualBox-kmod does not affect the other kernel modules i.e if we have 2 voxvideo.ko the kernel one is loaded first and other is not load so is pacific have both installed , if mount and all kmod works with this package I think you just need 
delete Requires:   %{name}-kmod = %{version}
But I have think better and test it  ...

Comment 58 Hans de Goede 2018-01-30 09:50:32 UTC
(In reply to Sergio Monteiro Basto from comment #57)
> +# Mainline vboxsf uses an option string rather then a custom binary data
> struct
> +Patch2:     0001-VBoxServiceAutoMount-Change-Linux-mount-code-to-use-.patch
> 
> -obj/bin/additions/mount.vboxsf
> 
> -%{_sbindir}/mount.vboxsf
> 
> Share folders still work ?

With the vboxsf kernel driver which I'm working to get upstream, yes. It now works with the regular mount binary as the options are now simply passed as a string. E.g. if you call "mount -t vboxfs something /mnt -o foo=bar,bar=foo"
then "foo=bar,bar=foo" is simply passed as a string to the kernel and the mainline version will parse this rather then using a custom vboxsf option-struct. Upstream vbox will also merge my changes for this and move over to doing things this way (not sure when).

Comment 59 Gwyn Ciesla 2018-01-30 13:22:21 UTC
(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/virtualbox-guest-additions

Comment 60 Sergio Monteiro Basto 2018-02-03 07:27:42 UTC
(In reply to Hans de Goede from comment #51)
> - Latest rpmfusion Release is 2, set our Release field to 3

Maybe could be better bump Epoch, for example VirtualBox had a rebuild today ...

Comment 61 Sergio Monteiro Basto 2018-02-03 08:08:13 UTC
(In reply to Hans de Goede from comment #51)

> - Use pkgconfig to get include/libs instead of hardcoding (rhbz#1534595)

we can't use pkgconf, for example: 

pkgconf --cflags libxml-2.0

-I/usr/include/libxml2 

when we have to use [1] and result with pkgconf is [2] , which doesn't work .

[1]
SDK_VBOX_LIBXML2_INCS=/usr/include/libxml2

[2]
SDK_VBOX_LIBXML2_INCS=-I/usr/include/libxml2

Comment 62 Hans de Goede 2018-02-07 11:04:44 UTC
(In reply to Sergio Monteiro Basto from comment #60)
> (In reply to Hans de Goede from comment #51)
> > - Latest rpmfusion Release is 2, set our Release field to 3
> 
> Maybe could be better bump Epoch, for example VirtualBox had a rebuild today
> ...

No Epoch's are evil. We can simply bump the release a bit extra if needed and rpmfusion should stop building the guest-additions sub-package for F28/rawhide soon now. I plan to import and build the Fedora guest-additions package for rawhide as soon as rawhide gets a 4.16-rc0 or rc1 with the vboxguest driver enabled.

(In reply to Sergio Monteiro Basto from comment #61)
> (In reply to Hans de Goede from comment #51)
> 
> > - Use pkgconfig to get include/libs instead of hardcoding (rhbz#1534595)
> 
> we can't use pkgconf, for example: 
> 
> pkgconf --cflags libxml-2.0
> 
> -I/usr/include/libxml2 
> 
> when we have to use [1] and result with pkgconf is [2] , which doesn't work .
> 
> [1]
> SDK_VBOX_LIBXML2_INCS=/usr/include/libxml2
> 
> [2]
> SDK_VBOX_LIBXML2_INCS=-I/usr/include/libxml2

Right, so this still worked because the additions don't care about any of the includes I was setting this way, so for the next release  I'm simply setting them all to "".

Comment 63 Sergio Monteiro Basto 2018-02-07 15:53:36 UTC
(In reply to Hans de Goede from comment #62)
> (In reply to Sergio Monteiro Basto from comment #60)
> > (In reply to Hans de Goede from comment #51)
> > > - Latest rpmfusion Release is 2, set our Release field to 3
> > 
> > Maybe could be better bump Epoch, for example VirtualBox had a rebuild today
> > ...
> 
> No Epoch's are evil. We can simply bump the release a bit extra if needed
> and rpmfusion should stop building the guest-additions sub-package for
> F28/rawhide soon now. I plan to import and build the Fedora guest-additions
> package for rawhide as soon as rawhide gets a 4.16-rc0 or rc1 with the
> vboxguest driver enabled.

OK , no problem for me . 

But , shared folder will work ? 

> With the vboxsf kernel driver which I'm working to get upstream, yes.

but this will be ready for 4.16-rc1 ? i.e. or we will still need vboxsf.ko from 
akmod-VirtualBox ? 

you can use /usr/lib/modules-load.d/VirtualBox-guest.conf only with vboxsf and still Requires:   %{name}-kmod = %{version}

Comment 64 Hans de Goede 2018-02-08 10:36:34 UTC
(In reply to Sergio Monteiro Basto from comment #63)
> But , shared folder will work ? 

See comment 58.

> but this will be ready for 4.16-rc1 ? i.e. or we will still need vboxsf.ko
> from akmod-VirtualBox ? 

4.16-rc1 will not contain a vboxsf, I hope to get another review to my upstream submission of vboxsf soon and then hopefully get it into -next, after which I can add it as a downstream patch to the Fedora kernels.

> you can use /usr/lib/modules-load.d/VirtualBox-guest.conf only with vboxsf
> and still Requires:   %{name}-kmod = %{version}

1) I cannot add a Requires to a Fedora package on something which is in rpmfusion
2) The vbox guest-additions bundled version will not work with the mainline vboxguest driver

I suggest we simply wait a bit to see how the upstreaming process of vboxsf goes, I don't expect many users to be running rawhide inside vbox. Plan A. is still for vboxsf.ko to be part of the Fedora kernels for rawhide/F28.

If that falls through the floor plan B is that I will do a standalone / out-of-tree version of my version of vboxsf (which works with the mainline vboxguest driver) and you can put that in the akmod for F28+ and users who want the shared-folder functionality can install the akmod (you can even use enhances to make this automatic).

Comment 65 Hans de Goede 2018-02-14 07:43:42 UTC
Ok, I've just imported virtualbox-guest-additions-5.2.6-4 (with one small fix over the last version posted here) and started a build for it for rawhide.

Please disable the guest-additions sub-package in the rpmfusion package.

If you want to test this with vboxsf, here is a standalone version of vboxsf which will build against the vboxguest included in the 4.16-rc1+ kernels in rawhide:

https://github.com/jwrdegoede/vboxsf/

Comment 66 Sergio Monteiro Basto 2018-02-19 02:43:55 UTC
(In reply to Hans de Goede from comment #64)
> (In reply to Sergio Monteiro Basto from comment #63)
> > But , shared folder will work ? 
> 
> See comment 58.

I do not understood the comment 58 , do we need build vboxsf.ko or not to work right now ? 

(In reply to Hans de Goede from comment #65)
> Ok, I've just imported virtualbox-guest-additions-5.2.6-4 (with one small
> fix over the last version posted here) and started a build for it for
> rawhide.
> 
> Please disable the guest-additions sub-package in the rpmfusion package.

In next iteration I will , is not a priority ... , everything still work without any problem ...

> If you want to test this with vboxsf, here is a standalone version of vboxsf
> which will build against the vboxguest included in the 4.16-rc1+ kernels in
> rawhide:
> 
> https://github.com/jwrdegoede/vboxsf/

So this code , can be add to VirtualBox-kmodsrc-5.2.x ? to build vboxsf.ko ? 

But I remember back here because I have others question. We (RPMFusion) also support epel7 and I remember to ask if you think have guest-additions on epel7 ? and BTW if F27 also will have this package ?

ah (I almost forgot) and I'd like to know , please, if modules still in staging or not , I'd like talk with Larry Finger from opensuse and a friend which is the package maintainer of Debian and I don't know where the modules are, to tell them ... 

IMHO this ticket should still open , to not lost the track ... 

Awesome work, best regards.

Comment 67 Hans de Goede 2018-02-19 09:56:11 UTC
Hi,

(In reply to Sergio Monteiro Basto from comment #66)
> (In reply to Hans de Goede from comment #64)
> > (In reply to Sergio Monteiro Basto from comment #63)
> > > But , shared folder will work ? 
> > 
> > See comment 58.
> 
> I do not understood the comment 58 , do we need build vboxsf.ko or not to
> work right now ? 

Right now for rawhide you still need to build vboxsf.ko.

> (In reply to Hans de Goede from comment #65)
> > Ok, I've just imported virtualbox-guest-additions-5.2.6-4 (with one small
> > fix over the last version posted here) and started a build for it for
> > rawhide.
> > 
> > Please disable the guest-additions sub-package in the rpmfusion package.
> 
> In next iteration I will , is not a priority ... , everything still work
> without any problem ...
> 
> > If you want to test this with vboxsf, here is a standalone version of vboxsf
> > which will build against the vboxguest included in the 4.16-rc1+ kernels in
> > rawhide:
> > 
> > https://github.com/jwrdegoede/vboxsf/
> 
> So this code , can be add to VirtualBox-kmodsrc-5.2.x ? to build vboxsf.ko ? 

Yes, with this code you can use the vboxguest which ships with 4.16-rc1+ in
rawhide and build a vboxsf.ko against it.

Note you only need to build vboxsf for rawhide, vboxguest is already there and should not be replaced.

> But I remember back here because I have others question. We (RPMFusion) also
> support epel7 and I remember to ask if you think have guest-additions on
> epel7 ? and BTW if F27 also will have this package ?

I do not plan to add this package to EPEL7.
I also will not add this package to F27.

> ah (I almost forgot) and I'd like to know , please, if modules still in
> staging or not , I'd like talk with Larry Finger from opensuse and a friend
> which is the package maintainer of Debian and I don't know where the modules
> are, to tell them ... 

The vboxguest driver lives under drivers/virt. vboxvideo is still in drivers/staging.

Regards,

Hans

Comment 68 Sergio Monteiro Basto 2018-03-15 03:44:49 UTC
Hello , 

(In reply to Hans de Goede from comment #55)
> (In reply to Nicolas Chauvet (kwizart) from comment #54)
(...)
> If we end up with plan B we should
> probably have the akmod-VirtualBox package build vboxsf instead of it not
> building any guest modules at all. 

OK I'm trying do this , how vboxguest.ko is loaded in boot time ? 
Before we loaded with /usr/lib/modules-load.d/VirtualBox.conf file but now we 
don't add any file to add  /usr/lib/modules-load.d/ ... 


(...)
> > Once this guest addition package will be created, I think it will be updated
> > along the rpmfusion VirtualBox-guest-addition package. As I expect, this
> > last will be disabled for f28+, but continue to exist for older fedora,rhel.

OK, this raise me one question when kernel 4.16 go to F27 what I should do ? seems to me this is more if kernel than if fedora version 

> > Based on this I think it's indeed correct to have the obsoletes/provide to
> > use a version macro. So everything is fine (maybe only to have a comment to
> > schedule the removal of obsoletes/provides for f28+2 or whatever rhel
> > version is relevant).

OK
 
> Ok, we also need to modify akmod-VirtualBox to not build the guest drivers
> on F-28+, as discussed before we will keep it around on existing guests
> which upgrade to F28 to avoid playing obsolete tricks which will cause
> problems on vbox-host installations.


(In reply to Hans de Goede from comment #65)
> Ok, I've just imported virtualbox-guest-additions-5.2.6-4 (with one small
> fix over the last version posted here) and started a build for it for
> rawhide.
> 
> Please disable the guest-additions sub-package in the rpmfusion package.

I'm trying :) 
 
> If you want to test this with vboxsf, here is a standalone version of vboxsf
> which will build against the vboxguest included in the 4.16-rc1+ kernels in
> rawhide:
> 
> https://github.com/jwrdegoede/vboxsf/

again depends on kernel version , I can't do this on a kernel of F26 or of epel 7, isn't it ? 
So have 2 codes for vboxsf is more complicated 

Thanks

Comment 69 Hans de Goede 2018-03-15 07:54:15 UTC
Hi,

(In reply to Sergio Monteiro Basto from comment #68)
> Hello , 
> 
> (In reply to Hans de Goede from comment #55)
> > (In reply to Nicolas Chauvet (kwizart) from comment #54)
> (...)
> > If we end up with plan B we should
> > probably have the akmod-VirtualBox package build vboxsf instead of it not
> > building any guest modules at all. 
> 
> OK I'm trying do this , how vboxguest.ko is loaded in boot time ? 
> Before we loaded with /usr/lib/modules-load.d/VirtualBox.conf file but now
> we 
> don't add any file to add  /usr/lib/modules-load.d/ ... 

The guest interface is a virtual PCI device, so the module gets autoloaded as it exports a pci-id table matching the virtual PCI device IDs.

Likewise the new vboxsf driver exports a fs-type, so it too will be autoloaded when you try to mount a mount with a "-t vboxsf".

> (...)
> > > Once this guest addition package will be created, I think it will be updated
> > > along the rpmfusion VirtualBox-guest-addition package. As I expect, this
> > > last will be disabled for f28+, but continue to exist for older fedora,rhel.
> 
> OK, this raise me one question when kernel 4.16 go to F27 what I should do ?
> seems to me this is more if kernel than if fedora version 

I will make sure that the vboxguest driver gets disabled for F27 kernel builds.

> > > Based on this I think it's indeed correct to have the obsoletes/provide to
> > > use a version macro. So everything is fine (maybe only to have a comment to
> > > schedule the removal of obsoletes/provides for f28+2 or whatever rhel
> > > version is relevant).
> 
> OK
>  
> > Ok, we also need to modify akmod-VirtualBox to not build the guest drivers
> > on F-28+, as discussed before we will keep it around on existing guests
> > which upgrade to F28 to avoid playing obsolete tricks which will cause
> > problems on vbox-host installations.
> 
> 
> (In reply to Hans de Goede from comment #65)
> > Ok, I've just imported virtualbox-guest-additions-5.2.6-4 (with one small
> > fix over the last version posted here) and started a build for it for
> > rawhide.
> > 
> > Please disable the guest-additions sub-package in the rpmfusion package.
> 
> I'm trying :) 
>  
> > If you want to test this with vboxsf, here is a standalone version of vboxsf
> > which will build against the vboxguest included in the 4.16-rc1+ kernels in
> > rawhide:
> > 
> > https://github.com/jwrdegoede/vboxsf/
> 
> again depends on kernel version , I can't do this on a kernel of F26 or of
> epel 7, isn't it ? 
> So have 2 codes for vboxsf is more complicated 

See above.

Comment 70 Sergio Monteiro Basto 2018-03-15 16:56:49 UTC
While  I read this new information , 
Hans please can you merge my PR [1] and virtualbox-guest-additions for F28 , thanks 


[1]
https://src.fedoraproject.org/rpms/virtualbox-guest-additions/pull-request/1

Comment 71 Sergio Monteiro Basto 2018-03-18 05:40:14 UTC
Hello ,

Would be an honor if you give commit access to this package, I could do the builds and updates of this package.

I miss one word in my previous comment (*) please _build_ virtualbox-guest-additions for F28, new version 5.2.8 is not built yet in F28.

I started to test virtualbox-guest-additions package from Fedora stack (I built it on copr) and here is the result of `modprobe vboxsf` [1] 

2 points: one, we also may build VirtualBox-kmodsrc sub-package in Fedora package, I haven't finish my pull request for it , but could be a solution VirtualBox-kmodsrc of Fedora have the vboxsf for vboxguest in kernel and VirtualBox-kmodsrc of RPMFusion have vboxguest for Fedora < 28 and epel-7 

Second point, how we load vboxsf module, is need add the file /usr/lib/modules-load.d/VirtualBox.conf with "vboxsf" to load automatically vboxsf.ko ? 

I'm ready to remove guest-additions from RPMFusion on F28+ , I'm just waiting for the change become smooth .

Thanks.


[1]
[  200.408370] vboxsf: loading out-of-tree module taints kernel.
[  200.408468] vboxsf: module verification failed: signature and/or required key missing - tainting kernel
[  200.408532] vboxsf: Unknown symbol VBoxGuest_RTMemTmpFree (err 0)
[  200.408545] vboxsf: Unknown symbol VBoxGuestIDC (err 0)
[  200.408562] vboxsf: Unknown symbol VBoxGuest_RTSemFastMutexRequest (err 0)
[  200.408579] vboxsf: Unknown symbol VBoxGuest_RTSemFastMutexRelease (err 0)
[  200.408592] vboxsf: Unknown symbol VBoxGuest_RTLogRelGetDefaultInstanceEx (err 0)
[  200.408604] vboxsf: Unknown symbol VBoxGuest_RTStrCopy (err 0)
[  200.408618] vboxsf: Unknown symbol VBoxGuest_RTErrConvertToErrno (err 0)
[  200.408638] vboxsf: Unknown symbol VBoxGuest_RTSemFastMutexCreate (err 0)
[  200.408650] vboxsf: Unknown symbol VBoxGuest_RTSemFastMutexDestroy (err 0)
[  200.408673] vboxsf: Unknown symbol VBoxGuest_RTAssertShouldPanic (err 0)
[  200.408688] vboxsf: Unknown symbol VBoxGuest_RTLogLoggerEx (err 0)
[  200.408702] vboxsf: Unknown symbol VBoxGuest_RTMemTmpAllocTag (err 0)
[  200.408718] vboxsf: Unknown symbol VBoxGuest_RTAssertMsg1Weak (err 0)
[  200.408734] vboxsf: Unknown symbol VBoxGuest_RTAssertMsg2Weak (err 0)

Comment 72 Hans de Goede 2018-03-18 21:12:56 UTC
Hi,

(In reply to Sergio Monteiro Basto from comment #71)
> Hello ,
> 
> Would be an honor if you give commit access to this package, I could do the
> builds and updates of this package.

I would be happy to have you as a co-maintainer, I've given commit rights to you now.

> I miss one word in my previous comment (*) please _build_
> virtualbox-guest-additions for F28, new version 5.2.8 is not built yet in
> F28.

Good point, fixed now.

> I started to test virtualbox-guest-additions package from Fedora stack (I
> built it on copr) and here is the result of `modprobe vboxsf` [1] 

That looks like your building from the virtualbox-upstream sources, as mentioned in comment 65, you need a modified version of vboxsf to work with the upstream-kernel version of the vboxguest module. This modified version is available here:
https://github.com/jwrdegoede/vboxsf/

This is the version of the vboxsf sources which the rpmfusion akmod package should use for F28+.

> 2 points: one, we also may build VirtualBox-kmodsrc sub-package in Fedora
> package

No that is not possible, Fedora does not allow out-of-tree kernel modules / kmod packages as part of Fedora.

> Second point, how we load vboxsf module, is need add the file
> /usr/lib/modules-load.d/VirtualBox.conf with "vboxsf" to load automatically
> vboxsf.ko ? 

You don't need to do anything as soon as someone does "mount -t vboxsf" or the virtualbox.service tries to do the equivalent for a folder selected for automount, then the module will autoload. My vboxsf sources/module contains:
"MODULE_ALIAS_FS("vboxsf");" which will make the module autoload whenever necessary.

Comment 73 Nicolas Chauvet (kwizart) 2018-04-24 09:52:41 UTC
(In reply to Hans de Goede from comment #72)
> Hi,
From fedora virtualbox-guest-additions
>Provides:       VirtualBox-kmod-common = %{version}-%{release}
I think we said that the fedora counterpart should not deal with the kmod part.
Obsoleting/providing the rpmfusion counterpart should be enough.

I don't get the point with vboxsf. Doesn't that module shouldn't be backported to later kernel ? why do we need any oot module to deal with that ?

Comment 74 Nicolas Chauvet (kwizart) 2018-04-24 10:24:31 UTC
Also I don't get why the guest-additions is kept in the rpmfusion counterpart.
we shouldn't replace any fedora package, so if the fedora package replace the rpmfusion one , It cannot go back and forth.

@sergio
Please disable the guest-additions in rpmfusion.

Comment 75 Nicolas Chauvet (kwizart) 2018-04-24 14:47:42 UTC
FYI https://bugzilla.rpmfusion.org/show_bug.cgi?id=4880
I think there is a miss-understanding that any work is needed on the rpmfusion side. It shouldn't have been.

Comment 76 Evangelos Foutras 2018-05-02 01:52:07 UTC
@Hans Do you know if the upstream fix for VBoxServiceAutoMount and vboxsf.mount will support both pre-4.16 and current kernels? (Or if it's going to support mainline vboxsf only.)

Supporting both in VBoxServiceAutoMount seems somewhat straightforward (current Arch Linux workaround at [1]), but mount.vboxsf is more tricky. In my hacky patch linked below, I just print a message to use `mount -cit` instead. A problem with this approach, however, is that there's no way (that I can see) to disable the helper from within fstab entries.

If the upstream solution is going to be in line with Fedora's (only supporting option strings for mounting), then we might as well roll with that in Arch Linux. Seems like it was an easy decision for Fedora 28 since only Linux 4.16 has to be supported, but we also ship the latest LTS kernel (and Manjaro even ships almost all kernels found on kernel.org :O).

[1] https://git.archlinux.org/svntogit/community.git/tree/trunk/010-linux-4.16-mount-fixes.patch?h=packages/virtualbox

Comment 77 Hans de Goede 2018-05-07 15:15:03 UTC
(In reply to Evangelos Foutras from comment #76)
> @Hans Do you know if the upstream fix for VBoxServiceAutoMount and
> vboxsf.mount will support both pre-4.16 and current kernels? (Or if it's
> going to support mainline vboxsf only.)

virtualbox upstream always keeps their out-of-tree kernel modules and their userspace bits in virtualbox-guest-additions in sync. AFAIK they plan to adopt the new mount args scheme on both the out-of-tree kernel-module and userspace side.

> Supporting both in VBoxServiceAutoMount seems somewhat straightforward
> (current Arch Linux workaround at [1]), but mount.vboxsf is more tricky. In
> my hacky patch linked below, I just print a message to use `mount -cit`
> instead. A problem with this approach, however, is that there's no way (that
> I can see) to disable the helper from within fstab entries.
> 
> If the upstream solution is going to be in line with Fedora's (only
> supporting option strings for mounting), then we might as well roll with
> that in Arch Linux.

AFAIK the plan is to only support option strings, in which release they are going to do that I do not know.

Comment 78 Sergio Monteiro Basto 2018-05-10 01:19:12 UTC
(In reply to Hans de Goede from comment #69)
> I will make sure that the vboxguest driver gets disabled for F27 kernel
> builds.


Hello 
I did the quick fix in VirtualBox-kmod [1] and just for Fedora >= 28, now akmods will build vboxsf.ko from "your" source [2] 
We don't need complicate this build , since we hope this is temporary stage ... (also I'm very busy) 

[1]
https://pkgs.rpmfusion.org/cgit/free/VirtualBox-kmod.git/commit/?id=23c3dbcf4ddfea55fccc8b08ef9c64d1440da6c7

[2]
https://github.com/jwrdegoede/vboxsf/


I think we may close this package review , because it is all done .
Thanks.

Comment 79 Sergio Monteiro Basto 2018-05-10 01:22:51 UTC
(In reply to Evangelos Foutras from comment #76)
> @Hans Do you know if the upstream fix for VBoxServiceAutoMount and
> vboxsf.mount will support both pre-4.16 and current kernels? (Or if it's
> going to support mainline vboxsf only.)
> 
> Supporting both in VBoxServiceAutoMount seems somewhat straightforward
> (current Arch Linux workaround at [1]), but mount.vboxsf is more tricky. In
> my hacky patch linked below, I just print a message to use `mount -cit`
> instead. A problem with this approach, however, is that there's no way (that
> I can see) to disable the helper from within fstab entries.
> 
> If the upstream solution is going to be in line with Fedora's (only
> supporting option strings for mounting), then we might as well roll with
> that in Arch Linux. Seems like it was an easy decision for Fedora 28 since
> only Linux 4.16 has to be supported, but we also ship the latest LTS kernel
> (and Manjaro even ships almost all kernels found on kernel.org :O).
> 
> [1]
> https://git.archlinux.org/svntogit/community.git/tree/trunk/010-linux-4.16-
> mount-fixes.patch?h=packages/virtualbox

I may add the patch mention if it fixes something ... 

Note that we remove binary mount.vboxsf (on Fedora 28+). 
If you want share folders, you need build vboxsf.ko with akmods and after `mount -t vboxsf bla bla` will work out of the box.

I checked the archlinux package (all patches and build parameters), RPMFusion package is based on Debian , opensSUSE etc but I wasn't aware of archlinux packaging and I may include some little improvements based on archlinux package, but I have to test it first. 

Best regards,

Comment 80 Evangelos Foutras 2018-06-06 22:22:39 UTC
(In reply to Hans de Goede from comment #65)
> If you want to test this with vboxsf, here is a standalone version of vboxsf
> which will build against the vboxguest included in the 4.16-rc1+ kernels in
> rawhide:
> 
> https://github.com/jwrdegoede/vboxsf/

Is there a version of this newer than v7? I've seen a report about pwritev(2) hanging while writing about 500 KiB:

  https://www.virtualbox.org/ticket/17716#comment:16

It also hangs on Fedora 28 w/ akmod-VirtualBox (exact same strace as in Arch).

Comment 81 Hans de Goede 2018-06-07 13:47:20 UTC
Hi,

(In reply to Evangelos Foutras from comment #80)
> (In reply to Hans de Goede from comment #65)
> > If you want to test this with vboxsf, here is a standalone version of vboxsf
> > which will build against the vboxguest included in the 4.16-rc1+ kernels in
> > rawhide:
> > 
> > https://github.com/jwrdegoede/vboxsf/
> 
> Is there a version of this newer than v7? 

No.

> I've seen a report about
> pwritev(2) hanging while writing about 500 KiB:
> 
>   https://www.virtualbox.org/ticket/17716#comment:16
> 
> It also hangs on Fedora 28 w/ akmod-VirtualBox (exact same strace as in
> Arch).

Thank you for bringing this to my attention I will take a look into this. It might be a couple of weeks before I get around to this though.

Regards,

Hans

Comment 82 Evangelos Foutras 2018-07-06 13:36:37 UTC
Hi Hans,

Did you get a chance to look at the pwritev issue?

Comment 83 Hans de Goede 2018-07-07 06:07:10 UTC
Hi,

(In reply to Evangelos Foutras from comment #82)
> Did you get a chance to look at the pwritev issue?

Sorry, not yet. But I've been making good progress on the rest of my TODO list, so I hope to get around to this soon.

Regards,

Hans

Comment 84 xnoreq 2019-01-13 16:18:30 UTC
>Sorry, not yet. But I've been making good progress on the rest of my TODO list, so I hope to get around to this soon.

Over half a year later, has this been fixed?

Comment 85 Hans de Goede 2019-01-14 14:33:28 UTC
(In reply to xnoreq from comment #84)
> >Sorry, not yet. But I've been making good progress on the rest of my TODO list, so I hope to get around to this soon.
> 
> Over half a year later, has this been fixed?

I've been unable to make time for this, sorry. I hope to get back to doing some virtualbox related work again soonish.

Comment 87 xnoreq 2019-01-19 13:03:43 UTC
The pwritev hang/loop/corruption issue with shared folders still exists in linux 4.20.3 with virtualbox 6.0.2 (virtualbox-guest-modules-arch, virtualbox-guest-utils-nox).

Since this bug has been closed, could you either re-open it or create a new one?

Comment 88 Hans de Goede 2019-01-21 21:19:05 UTC
Hi,

Thank you for your work on maintaining the vbox guest-additions package in Fedora.

(In reply to Sergio Monteiro Basto from comment #86)
> BTW please review

> https://src.fedoraproject.org/rpms/virtualbox-guest-additions/blob/master/f/
> 010-linux-4.16-mount-fixes.patch

This one is not necessary and can be dropped.

> https://src.fedoraproject.org/rpms/virtualbox-guest-additions/blob/master/f/
> 0001-VBoxServiceAutoMount-Change-Linux-mount-code-to-use-.patch

This patch looks good, thank you for rebasing it.

Regards,

Hans

Comment 89 Sergio Monteiro Basto 2019-01-27 16:49:54 UTC
(In reply to Hans de Goede from comment #88)
> Hi,
> 
> Thank you for your work on maintaining the vbox guest-additions package in
> Fedora.
> 
> (In reply to Sergio Monteiro Basto from comment #86)
> > BTW please review
> 
> > https://src.fedoraproject.org/rpms/virtualbox-guest-additions/blob/master/f/
> > 010-linux-4.16-mount-fixes.patch
> 
> This one is not necessary and can be dropped.
> 
> > https://src.fedoraproject.org/rpms/virtualbox-guest-additions/blob/master/f/
> > 0001-VBoxServiceAutoMount-Change-Linux-mount-code-to-use-.patch
> 
> This patch looks good, thank you for rebasing it.
> 
> Regards,
> 
> Hans 

Hi , thanks for the reply , another question, can we use new vboxsf source [1] in el7 ?

i.e. instead use new vboxsf source [1] code only in F28+ [2], can I use it for all cases (el7 and old F27 for example )  ? 

BTW in vboxsf-master/super.c line 153 if (flags & MS_REMOUNT) FTBFS with kernel 5.0.0-rc3 , I'm testing add this [3] to source code , based on [4]


Thanks.

[1]
https://github.com/jwrdegoede/vboxsf

[2]
%if 0%{?fedora} > 27
%bcond_without  newvboxsf
%else
%bcond_with     newvboxsf
%endif


[3]
#if LINUX_VERSION_CODE >= KERNEL_VERSION(5, 0, 0)
# include <uapi/linux/mount.h>
#endif

[4]
https://github.com/torvalds/linux/commits/master/security/tomoyo/mount.c
https://github.com/torvalds/linux/commit/e262e32d6bde0f77fb0c95d977482fc872c51996#diff-445cd3f3706b497a6e327c47d03ee661

Comment 90 Hans de Goede 2019-01-29 08:29:08 UTC
(In reply to Sergio Monteiro Basto from comment #89)
> Hi , thanks for the reply , another question, can we use new vboxsf source
> [1] in el7 ?
> 
> i.e. instead use new vboxsf source [1] code only in F28+ [2], can I use it
> for all cases (el7 and old F27 for example )  ? 

No, the new vboxsf sources rely on the upstreamed vboxguest driver which
is not in the RHEL7 / F27 kernels.

> BTW in vboxsf-master/super.c line 153 if (flags & MS_REMOUNT) FTBFS with
> kernel 5.0.0-rc3 , I'm testing add this [3] to source code , based on [4]

I've fixed this myself by simply dropping the 2 lines, the VFS core already
does this check, so it is not necessary. I've just pushed this change to:
https://github.com/jwrdegoede/vboxsf


Note You need to log in before you can comment on or make changes to this bug.