© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
7020100
(Get the code!)
Review for Package: src: v4l2-relayd
[Summary]
We have a very new application (and package) here that can be used to capture
video of MIPI cameras and transform/relay it into a standard v4l2 format, so it
can be used by the usual desktop applications.
Thank you for preparing the packaging and getting it into universe already!
I think there is a bit of work to do in order to get this package in shape to
fulfill the quality criteria required in main with a focus on 3 primary topics:
+ Dependencies to be MIRed
+ Security hardening
+ Testing
MIR team ACK under the constraint to resolve the below listed
required TODOs and as much as possible having a look at the
recommended TODOs.
This does not need a security review
List of specific binary packages to be promoted to main: 4vl2-relayd
Specific binary packages built, but NOT to be promoted to main: <None>
Notes:
- Please improve the rationale a bit, is there any other way you tried enabling
those MIPI cameras with tools that are already available?
- This is looking good security wise (except the daemon being run as root
without any systemd service hardening). I expect we do not need security
review, if we improve the systemd service security a bit.
Required TODOs:
- The v4l2loopback-dkms dependency needs to be MIRed, too – NO linux-image-
- The daemon is run as root. Its systemd service should use hardening features.
See below and http://
security concerns.
- In addition to basic automatic testing, the manual test-plan/script should be
improved to show individual steps (for setup & running of the test) and
expected outcome (see below)
- Consider dropping the Ubuntu delta, or explain why it is needed (see below)
- Fix certain lintian hints (see below):
+ v4l2-relayd: systemd-
+ v4l2-relayd source: quilt-patch-
+ v4l2-relayd: no-manual-page
Recommended TODOs:
- The package should get a team bug subscriber before being promoted
- the testing situation should be improved either during build or at autopkgtest
stage (loading of the v4l2loopback-dkms kernel module shouldn't be a blocker
inside autopkgtest's qemu environments).
- Provide basic upstream documentation (README.md)
- The current maintainer/uploader could consider applying for PPU to avoid
sponsored maintenance
[Duplication]
There is no other package in main providing the same functionality.
I found uv4l/mjpegstream, which seems to provide similar functionality,
but that is not currently packaged in Ubuntu.
https:/
[Dependencies]
OK:
- no -dev/-debug/-doc packages that need exclusion
- No dependencies in main that are only superficially tested requiring
more tests now.
Problems:
- depends on v4l2loopback-dkms (in universe)
[Embedded sources and static linking]
OK:
- no embedded source present
- no static linking
- does not have odd Built-Using entries
- not a go package, no extra constraints to consider in that regard
Problems: None
[Security]
OK:
- history of CVEs does not look concerning
- does not use webkit1,2
- does not use lib*v8 directly
- does not parse data formats
- does not open a port/socket
- does not process arbitrary web content
- does not use centralized online accounts
- does not integrate arbitrary javascript into the desktop
- does not deal with system authentication (eg, pam), etc)
- does not deal with security attestation (secure boot, tpm, signatures)
Problems:
- does run a daemon as root, should be locked down (maybe using User=/Group=
stanzas and similar to how it was done here:
https:/
[Common blockers]
OK:
- does not FTBFS currently
- no new python2 dependency
Problems:
- does NOT have a test suite that runs at build time
- does NOT have a non-trivial test suite that runs as autopkgtest
- if special HW does prevent build/autopkgtest is there a test plan, code,
log provided? => I'm wondering why there can't be some kind of autopkgtest,
loading a kernel module in qemu shouldn't be a problem. We cannot test the
real HW camera, but maybe we could simulate an input, capture an output frame
and compare that to the input to see if relaying works?
- if a non-trivial test on this level does not make sense (the lib alone
is only doing rather simple things), is the overall solution (app+libs)
extensively covered i.e. via end to end autopkgtest? => => The manual test plan
on LP: #1958108 and "check if camera is working via webrtc" seems a bit
light to me. Could we please formalize this somewhere in the wiki or so,
describing individual steps (also for the preparation of the test
environment) and the expected results?
[Packaging red flags]
OK:
- symbols tracking not applicable for this kind of code.
- d/watch is present and looks ok (if needed, e.g. non-native)
- Upstream update history is good => OK, but very new package
- Debian/Ubuntu update history is good => OK, but very new package
- the current release is packaged
- d/rules is rather clean
- It is not on the lto-disabled list
Problems:
- why carry an Ubuntu delta and distro patches in debian/patches/ if we are the
upstream?
- The current maintainer/uploader (while not a MOTU) does not have upload rights
to main and needs sponsoring, maybe they should apply for PPU?
- Some lintian hints to improve upon:
X: v4l2-relayd: systemd-
N:
N: The specified systemd .service file does not appear to enable any
N: hardening options.
N:
N: systemd has support for many security-oriented features such as isolating
N: services from the network, private /tmp directories, as well as control
N: over making directories appear read-only or even inaccessible, etc.
N:
N: Please consider supporting some options, collaborating upstream where
N: necessary about any potential changes.
N:
N: Please refer to the systemd.service(5) manual page and
N: http://
I: v4l2-relayd source: quilt-patch-
N:
N: quilt patch files should start with a description of patch. All lines
N: before the start of the patch itself are considered part of the
N: description. You can edit the description with quilt header -e when the
N: patch is at the top of the stack.
N:
N: As well as a description of the purpose and function of the patch, the
N: description should ideally contain author information, a URL for the bug
N: report (if any), Debian or upstream bugs fixed by it, upstream status, the
N: Debian version and date the patch was first included, and any other
N: information that would be useful if someone were investigating the patch
N: and underlying problem. Please consider using the DEP 3 format for this
N: information.
N:
N: Please refer to https:/
W: v4l2-relayd: no-manual-page usr/bin/v4l2-relayd
N:
N: Each binary in /usr/bin, /usr/sbin, /bin, /sbin or /usr/games should have
N: a manual page
N:
N: Note that though the man program has the capability to check for several
N: program names in the NAMES section, each of these programs should have its
N: own manual page (a symbolic link to the appropriate manual page is
N: sufficient) because other manual page viewers such as xman or tkman don't
N: support this.
N:
N: If the name of the manual page differs from the binary by case, man may be
N: able to find it anyway; however, it is still best practice to match the
N: exact capitalization of the executable in the manual page.
N:
N: If the manual pages are provided by another package on which this package
N: depends, Lintian may not be able to determine that manual pages are
N: available. In this case, after confirming that all binaries do have manual
N: pages after this package and its dependencies are installed, please add a
N: Lintian override.
N:
N: Please refer to Debian Policy Manual section 12.1 for details.
[Upstream red flags]
OK:
- no Errors/warnings during the build
- no incautious use of malloc/sprintf (as far as we can check it)
- no use of sudo, gksu, pkexec, or LD_LIBRARY_PATH (usage is OK inside tests)
- no use of user nobody
- no use of setuid
- no important open bugs (crashers, etc) in Debian or Ubuntu
- no dependency on webkit, qtwebkit, seed or libgoa-*
- not part of the UI for extra checks
- no translation present, but none needed for this case (user visible)?
Problems:
- Documentation is very light, there should be some manpage and an improved
README.md file