Re: Extending libdebian-installer's API to include certainty on subarch detection
On Wed, Sep 08, 2010 at 01:51:20AM -0400, Michael Casadevall wrote:
> Perfectly. I've reworked my patches to implement these suggestions, here's my
> current result:
>
> === modified file 'include/debian-installer/system/subarch.h'
> --- include/debian-installer/system/subarch.h 2005-08-03 09:42:05 +0000
> +++ include/debian-installer/system/subarch.h 2010-09-07 08:09:28 +0000
> @@ -24,6 +24,10 @@
> #ifndef DEBIAN_INSTALLER__SYSTEM__SUBARCH_H
> #define DEBIAN_INSTALLER__SYSTEM__SUBARCH_H
>
> +#ifdef __ARMEL__
> +#define SUBARCH_CAN_GUESS 1
> +#endif
This is namespace pollution. libd-i's namespace isn't all *that*
regular, but I think DI_SYSTEM_SUBARCH_CAN_GUESS would be a bit more
conventional.
> +/**
> + * Return a string with a best-guess of the current subarchitecture
> + *
> + * Only present on armel currently, and is a stub on all other subarchs
"all other architectures"
> + */
> +
> +const char *di_system_subarch_analyze_guess(void);
Local convention is space before parenthesis (libd-i is inconsistent,
but we should at least be consistent within each file).
> === modified file 'src/libdebian-installer.ver'
> --- src/libdebian-installer.ver 2006-09-30 15:06:52 +0000
> +++ src/libdebian-installer.ver 2010-09-07 08:03:43 +0000
> @@ -146,6 +146,7 @@
>
> LIBDI_4.6 {
> global:
> + di_system_subarch_analyze_guess;
> di_tree_destroy;
> di_tree_foreach;
> di_tree_insert;
To add a new interface, I think we need to bump to 4.7
(LIBRARY_VERSION_REVISION=7 in configure.ac) and add a new block in
src/libdebian-installer.ver. This shouldn't require any
reverse-dependencies to be rebuilt.
(Otavio, I don't suppose you'd like to comment on whether this would be
possible for squeeze?)
> +const char *di_system_subarch_analyze_guess (void)
> +{
> + struct utsname sysinfo;
> + int uname_release_len, i;
> +
> + /* Attempt to determine subarch based on kernel release version */
> + uname(&sysinfo);
> + uname_release_len = strlen(sysinfo.release);
> +
> + for (i = 0; supported_generic_subarches[i] != NULL; i++)
> + {
> + int subarch_len = strlen (supported_generic_subarches[i]);
> + if (!strncmp(sysinfo.release+uname_release_len-subarch_len,
> + supported_generic_subarches[i],
> + subarch_len))
strlen returns size_t and strncmp takes size_t as its third argument, so
uname_release_len and subarch_len should be size_t rather than int.
> +#ifndef SUBARCH_CAN_GUESS
> +
> +/**
> + * HACK: If there's a better way to do this, we should probably use that
> + * instead of thus stub function for non armel archs
> + */
> +
> +const char *di_system_subarch_analyze_guess (void)
> +{
> + return di_system_subarch_analyze();
> +}
> +
> +#endif
As I think you suggested on IRC, a weak symbol would probably be neater,
but this will do fine for now. (But s/thus/this/ in the comment, and
that probably doesn't need to be a Doxygen comment with the leading
"/**".)
> -int main(int argc ATTRIBUTE_UNUSED, char *argv[] ATTRIBUTE_UNUSED)
> +int main(int argc, char *argv[])
> {
> const char *subarch;
>
> + while (1)
> + {
> + int option_index = 0;
> + int c;
> +
> + c = getopt_long (argc, argv, "g", long_options,
> + &option_index);
> + if (c == -1) break;
> +
> + switch (c)
> + {
> + case 'g':
> + if (!(subarch = di_system_subarch_analyze_guess()))
> + return EXIT_FAILURE;
> +
> + goto print_subarch;
> + break;
I think it would be neater to set a flag and do the actual processing
outside the getopt loop.
hw-detect will also need a build-dependency on a new enough
libdebian-installer4-dev.
Thanks,
--
Colin Watson [cjwatson@debian.org]
Reply to: