New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Move the database of supported devices out into runtime loaded f… #293

Merged
merged 6 commits into from Nov 2, 2017

Conversation

3 participants
@hughsie
Owner

hughsie commented Oct 28, 2017

…iles

When fwupd is installed in long-term support distros it's very hard to backport
new versions as new hardware is released.

There are several reasons why we can't just include the mapping and quirk
information in the AppStream metadata:

  • The extra data is hugely specific to the installed fwupd plugin versions
  • The device-id is per-device, and the mapping is usually per-plugin
  • Often the information is needed before the FuDevice is created
  • There are security implications in allowing plugins to handle new devices

The idea with FuHwdb is that the end user can drop an additional (or replace
an existing) file in a .d director with a simple format and the hardware will
magically start working. This assumes no new quirks are required, as this would
obviously need code changes, but allows us to get most existing devices working
in an easy way without the user compiling anything.

This allows us to fix issues like #265

FIXMEs:

  • I'm not happy with FuHwdb as a name, it's not a DB and too similar to FuHwids
  • Is .hwdb a good extension? Do we need Yet Another File Format? Really?
  • Could we use a fwupd-installed AppStream file with the quirks in?
    org.freedesktop.fwupd.plugin.dfu.quirks.metainfo and then use <custom> and <value> --
    XML isn't super human-write-friendly and kinda verbose...
  • The DFU parts need to be split off as a different commit
  • Other plugins need to be ported, e.g. ebitdo, unifying, purism, and avr32
  • This needs to be tested on real hardware

@hughsie hughsie requested a review from superm1 Oct 28, 2017

@gnolsen

This comment has been minimized.

Show comment
Hide comment
@gnolsen

gnolsen Oct 30, 2017

Collaborator

This looks good.

For the Jabra DFU quirk handling code to be completely freed of PID switch statements, we would need to be able to also encode the 'rep' and 'adr' bytes of the FWU mode switch command in the quirks file. Then new DFU devices that do not require new quirks can be added without code changes.

Collaborator

gnolsen commented Oct 30, 2017

This looks good.

For the Jabra DFU quirk handling code to be completely freed of PID switch statements, we would need to be able to also encode the 'rep' and 'adr' bytes of the FWU mode switch command in the quirks file. Then new DFU devices that do not require new quirks can be added without code changes.

@superm1

I like the approach. Something else I think that could be interesting is to let the quirks come down with LVFS metadata distributed with updates.
If you allow quirks to be put directly into CAB files, you can in effect not have to even do "OS" updates of quirks for supporting new devices in some cases.

Show outdated Hide outdated plugins/dfu/dfu-common.c Outdated
Show outdated Hide outdated plugins/dfu/dfu-common.c Outdated
Show outdated Hide outdated plugins/dfu/dfu-common.c Outdated
Show outdated Hide outdated plugins/dfu/dfu-device.c Outdated
Show outdated Hide outdated src/fu-engine.c Outdated
@@ -208,6 +208,8 @@ mkdir -p --mode=0700 $RPM_BUILD_ROOT%{_localstatedir}/lib/fwupd/gnupg
%{_unitdir}/fwupd.service
%{_unitdir}/system-update.target.wants/
%dir %{_localstatedir}/lib/fwupd
%dir %{_datadir}/fwupd/quirks.d

This comment has been minimized.

@superm1

superm1 Oct 30, 2017

Collaborator

Just a thought, can rpm packaging let you just wildcards to pick everything like %dir %{_datadir}/fwupd/*? It should let this have to change less when you shuffle stuff around if you can.

@superm1

superm1 Oct 30, 2017

Collaborator

Just a thought, can rpm packaging let you just wildcards to pick everything like %dir %{_datadir}/fwupd/*? It should let this have to change less when you shuffle stuff around if you can.

This comment has been minimized.

@hughsie

hughsie Oct 30, 2017

Owner

Yes, although IIUC it's best practice to list them individually, which means you notice if they go away or get renamed. You could argue the file-list could just be * :)

@hughsie

hughsie Oct 30, 2017

Owner

Yes, although IIUC it's best practice to list them individually, which means you notice if they go away or get renamed. You could argue the file-list could just be * :)

This comment has been minimized.

@superm1

superm1 Oct 30, 2017

Collaborator

But considering you're the upstream and you know when files come and go as a result, is that still a relevant best practice? Just seems like paperwork at that point....

@superm1

superm1 Oct 30, 2017

Collaborator

But considering you're the upstream and you know when files come and go as a result, is that still a relevant best practice? Just seems like paperwork at that point....

This comment has been minimized.

@hughsie

hughsie Oct 30, 2017

Owner

I think using logic with the Fedora Packaging Policy is a dangerous game :) The other thing %dir lets you do is choose the permissions or group ownership, although in this case the quirks are not security sensitive and can use the default.

@hughsie

hughsie Oct 30, 2017

Owner

I think using logic with the Fedora Packaging Policy is a dangerous game :) The other thing %dir lets you do is choose the permissions or group ownership, although in this case the quirks are not security sensitive and can use the default.

This comment has been minimized.

@superm1

superm1 Oct 30, 2017

Collaborator

Ah well I guess this is a difference from debian approach when it comes to policy. dh_fixperms will run at set policy on everything unless you override it.

       dh_fixperms is a debhelper program that is responsible for setting the permissions of files and directories in package build directories to a sane state -- a state that complies with Debian
       policy.

I guess you know fedora policy and such better than me on what would fly with this kind of stuff. I just think TPS reports are overrated and try to avoid them when I can.

@superm1

superm1 Oct 30, 2017

Collaborator

Ah well I guess this is a difference from debian approach when it comes to policy. dh_fixperms will run at set policy on everything unless you override it.

       dh_fixperms is a debhelper program that is responsible for setting the permissions of files and directories in package build directories to a sane state -- a state that complies with Debian
       policy.

I guess you know fedora policy and such better than me on what would fly with this kind of stuff. I just think TPS reports are overrated and try to avoid them when I can.

@hughsie hughsie referenced this pull request Oct 30, 2017

Merged

Wip/hughsie/atmel flip #292

@hughsie

This comment has been minimized.

Show comment
Hide comment
@hughsie

hughsie Oct 30, 2017

Owner

Okay, lots of fixes. @superm1 if you can test the new Dell stuff that would be handy. I'm also not sure about the prefix names, e.g. do we want:

  • fwupd-uefi-version-format
  • FwupdUefiVersionFormat
  • org.freedesktop.fwupd.plugin.uefi:version-format
  • org.freedesktop.fwupd.plugin.uefi:VersionFormat
  • fwupd:plugin:uefi/version_format
  • something else entirely.

I'm also not sure how to document the quirks; in the code they'd be scattered around and in the .quirk files there might be multiple files adding keys with the same prefix. In a random .txt file it might be tricky to describe what the quirks are doing without the code inline. Ideas welcome.

Owner

hughsie commented Oct 30, 2017

Okay, lots of fixes. @superm1 if you can test the new Dell stuff that would be handy. I'm also not sure about the prefix names, e.g. do we want:

  • fwupd-uefi-version-format
  • FwupdUefiVersionFormat
  • org.freedesktop.fwupd.plugin.uefi:version-format
  • org.freedesktop.fwupd.plugin.uefi:VersionFormat
  • fwupd:plugin:uefi/version_format
  • something else entirely.

I'm also not sure how to document the quirks; in the code they'd be scattered around and in the .quirk files there might be multiple files adding keys with the same prefix. In a random .txt file it might be tricky to describe what the quirks are doing without the code inline. Ideas welcome.

@superm1

This comment has been minimized.

Show comment
Hide comment
@superm1

superm1 Oct 31, 2017

Collaborator

I think it would be a good practice to put all the quirks used in fu-quirks.h (or a similar header file name), and document them there. That header file can then be installed with fwupd so that you can easily look at it from a text editor to know what quirks were supported by the installed version of fwupd.

To avoid having to do code inline how about every quirk in the header file gets a comment like this:

Standard behavior: Version format is recognized as AA.BB.CCDD
Quirked behavior: Version format is recognized as AA.BB.CC.DD

I think adding pointers (or copying )the code is unnecessary effort. Once you've got a string, it's easy enough to just search from github or grep the codebase to find where it's used.

Other idea is just that ini style file like you've got implemented but same strings above. Everything that is quirkable should have an empty section then by default in the ini file.

Regarding prefix name, nothing else is going to use this other than fwupd right? I think dbus style naming is confusing outside a dbus context. I'm personally a fan of the first option you listed (fwupd-<consumer>-<quirk type>)

Collaborator

superm1 commented Oct 31, 2017

I think it would be a good practice to put all the quirks used in fu-quirks.h (or a similar header file name), and document them there. That header file can then be installed with fwupd so that you can easily look at it from a text editor to know what quirks were supported by the installed version of fwupd.

To avoid having to do code inline how about every quirk in the header file gets a comment like this:

Standard behavior: Version format is recognized as AA.BB.CCDD
Quirked behavior: Version format is recognized as AA.BB.CC.DD

I think adding pointers (or copying )the code is unnecessary effort. Once you've got a string, it's easy enough to just search from github or grep the codebase to find where it's used.

Other idea is just that ini style file like you've got implemented but same strings above. Everything that is quirkable should have an empty section then by default in the ini file.

Regarding prefix name, nothing else is going to use this other than fwupd right? I think dbus style naming is confusing outside a dbus context. I'm personally a fan of the first option you listed (fwupd-<consumer>-<quirk type>)

Move the database of supported devices out into runtime loaded files
When fwupd is installed in long-term support distros it's very hard to backport
new versions as new hardware is released.

There are several reasons why we can't just include the mapping and quirk
information in the AppStream metadata:

 * The extra data is hugely specific to the installed fwupd plugin versions
 * The device-id is per-device, and the mapping is usually per-plugin
 * Often the information is needed before the FuDevice is created
 * There are security implications in allowing plugins to handle new devices

The idea with quirks is that the end user can drop an additional (or replace
an existing) file in a .d director with a simple format and the hardware will
magically start working. This assumes no new quirks are required, as this would
obviously need code changes, but allows us to get most existing devices working
in an easy way without the user compiling anything.

This allows us to fix issues like #265
@hughsie

This comment has been minimized.

Show comment
Hide comment
@hughsie

hughsie Oct 31, 2017

Owner

I've spent a bunch of time documenting all the quirks and giving them better names. I did put all the documentation in fu-quirks.h like you suggested but rather than disting the file added them to the gtk-doc so they show up in the product docs.

Owner

hughsie commented Oct 31, 2017

I've spent a bunch of time documenting all the quirks and giving them better names. I did put all the documentation in fu-quirks.h like you suggested but rather than disting the file added them to the gtk-doc so they show up in the product docs.

@superm1

This comment has been minimized.

Show comment
Hide comment
@superm1

superm1 Oct 31, 2017

Collaborator

That looks really good, and gtk-doc output is definitely a much better consumer than a header file.

The DFU ones are still missing, is that intentional? I was actually thinking that those are the more interesting ones because there are so many and they can be used together.

Collaborator

superm1 commented Oct 31, 2017

That looks really good, and gtk-doc output is definitely a much better consumer than a header file.

The DFU ones are still missing, is that intentional? I was actually thinking that those are the more interesting ones because there are so many and they can be used together.

Show outdated Hide outdated plugins/dell/dell.quirk Outdated
Dell Inc.=none
Alienware=none
[fwupd-daemon-version-format]

This comment has been minimized.

@superm1

superm1 Oct 31, 2017

Collaborator

Same question, FU_QUIRKS_DAEMON_VERSION_FORMAT ?

@superm1

superm1 Oct 31, 2017

Collaborator

Same question, FU_QUIRKS_DAEMON_VERSION_FORMAT ?

install_data(['dell.quirk'],
install_dir: join_paths(get_option('datadir'), 'fwupd', 'quirks.d')
)

This comment has been minimized.

@superm1

superm1 Oct 31, 2017

Collaborator

If those other strings can't work, would pre-processing (.quirks.in ->.quirks) make sense rather than hardcoding in source in two places?

@superm1

superm1 Oct 31, 2017

Collaborator

If those other strings can't work, would pre-processing (.quirks.in ->.quirks) make sense rather than hardcoding in source in two places?

Show outdated Hide outdated src/fu-quirks.h Outdated
*
* Since: 1.0.1
*/
#define FU_QUIRKS_DAEMON_VERSION_FORMAT "fwupd-daemon-version-format"

This comment has been minimized.

@superm1

superm1 Oct 31, 2017

Collaborator

I think put the UEFI plugin as the last plugin, and the daemon at the very end. So all plugin quirks come in order (alphabetically) and then daemon quirks come afterwards. Makes them easier to find rather UEFI in between all the DFU ones.

@superm1

superm1 Oct 31, 2017

Collaborator

I think put the UEFI plugin as the last plugin, and the daemon at the very end. So all plugin quirks come in order (alphabetically) and then daemon quirks come afterwards. Makes them easier to find rather UEFI in between all the DFU ones.

This comment has been minimized.

@hughsie

hughsie Oct 31, 2017

Owner

so fwupd-dfu-alternate-vidpid-plugin, fwupd-dfu-jabra-detach-plugin, fwupd-uefi-version-format-plugin and fwupd-version-format-daemon? Doing it this way moves the "where" to right at the end. Maybe we drop the -plugin suffix entirely?

@hughsie

hughsie Oct 31, 2017

Owner

so fwupd-dfu-alternate-vidpid-plugin, fwupd-dfu-jabra-detach-plugin, fwupd-uefi-version-format-plugin and fwupd-version-format-daemon? Doing it this way moves the "where" to right at the end. Maybe we drop the -plugin suffix entirely?

This comment has been minimized.

@superm1

superm1 Nov 1, 2017

Collaborator

I think drop the suffix entirely yes. Order sounds right.

@superm1

superm1 Nov 1, 2017

Collaborator

I think drop the suffix entirely yes. Order sounds right.

@hughsie

This comment has been minimized.

Show comment
Hide comment
@hughsie

hughsie Oct 31, 2017

Owner

The dfu ones should be here, I force pushed so you might need to reload this page to avoid the ajax being funky.

Owner

hughsie commented Oct 31, 2017

The dfu ones should be here, I force pushed so you might need to reload this page to avoid the ajax being funky.

hughsie added some commits Oct 30, 2017

dfu: Use the FuQuirk infrastructure to move the quirks out of the code
This is slightly more verbose than desired as we also have to include the quirk
information when running the dfu-tool, which does not have an already set-up
FuQuirks object as it has no plugin.
dfu: Use FuQuirk to encode the Jabra magic packet contents
This allows us to remove the Jabra-specific quirk entry in the device bitfield,
and more importantly allows us to support some more Jabra devices in the future
without code changes.
@hughsie

This comment has been minimized.

Show comment
Hide comment
@hughsie

hughsie Nov 2, 2017

Owner

Okay, all fixes done, thanks for all the help so far.

Owner

hughsie commented Nov 2, 2017

Okay, all fixes done, thanks for all the help so far.

@superm1

superm1 approved these changes Nov 2, 2017

LGTM now

@hughsie hughsie merged commit 674ed34 into master Nov 2, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@hughsie hughsie deleted the wip/hughsie/hwdb branch Nov 2, 2017

@hughsie

This comment has been minimized.

Show comment
Hide comment
@hughsie

hughsie Nov 2, 2017

Owner

@gnolsen when you're ready, could you open a new PR for the new Jabra models please? It should just be a case of adding a few lines to dfu.quirk

Owner

hughsie commented Nov 2, 2017

@gnolsen when you're ready, could you open a new PR for the new Jabra models please? It should just be a case of adding a few lines to dfu.quirk

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment