[libcamera-devel] [PATCH v2 1/2] libcamera: ipa_module: add IPA shared library loader
Paul Elder
paul.elder at ideasonboard.com
Thu May 16 19:51:16 CEST 2019
Hi,
On Thu, May 16, 2019 at 12:19:42AM +0300, Laurent Pinchart wrote:
> Hello,
>
> On Wed, May 15, 2019 at 04:26:48PM +0100, Kieran Bingham wrote:
> > On 15/05/2019 16:02, Paul Elder wrote:
> > > On Wed, May 15, 2019 at 10:28:46AM +0100, Kieran Bingham wrote:
> > >> Hi Paul,
> > >>
> > >> Thank you for your patch, This is looking good and it's going in the
> > >> right direction but I have a few design comments ... feel free to
> > >> disagree though ...
> > >
> > > Thank you for the review.
> > >
> > >> On 14/05/2019 23:38, Paul Elder wrote:
> > >>> Implement a class to load a struct IPAModuleInfo of a given symbol name
> > >>> from a .so shared object library.
> > >>>
> > >>> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > >>> ---
> > >>> Changes in v2:
> > >>> - renamed LibLoader class to IPAModule
> > >>> - added documentation
> > >>> - added logging
> > >>> - check that bitness of the shared object is the same as libcamera
> > >>> - moved symbol loading ("init") to the constructor, and added isValid()
> > >>> - added elfPointer() to prevent segfaults when reading data from mmap
> > >>> - moved struct IPAModuleInfo out of IPAModule
> > >>> - rename getIPAModuleInfo() to IPAModuleInfo(), and make it return a
> > >>> const reference
> > >>> - added munmap after the mmap
> > >>>
> > >>> src/libcamera/include/ipa_module.h | 43 +++++
> > >>> src/libcamera/ipa_module.cpp | 277 +++++++++++++++++++++++++++++
> > >>> src/libcamera/meson.build | 2 +
> > >>> 3 files changed, 322 insertions(+)
> > >>> create mode 100644 src/libcamera/include/ipa_module.h
> > >>> create mode 100644 src/libcamera/ipa_module.cpp
> > >>>
> > >>> diff --git a/src/libcamera/include/ipa_module.h b/src/libcamera/include/ipa_module.h
> > >>> new file mode 100644
> > >>> index 0000000..9eb0094
> > >>> --- /dev/null
> > >>> +++ b/src/libcamera/include/ipa_module.h
> > >>> @@ -0,0 +1,43 @@
> > >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > >>> +/*
> > >>> + * Copyright (C) 2019, Google Inc.
> > >>> + *
> > >>> + * ipa_module.h - load IPA module information from shared library at runtime
> > >>
> > >> Hrm ... we have two sides here.
> > >>
> > >> We need a public header which defines the interface between an IPA
> > >> module and libcamera. That would include a struct IPAModuleInfo and any
> > >> registration details, but should not include any internal libcamera
> > >> private details regarding how the module is loaded.
> > >
> > > Yes, I agree.
> > >
> > >>> + */
> > >>> +#ifndef __LIBCAMERA_IPA_MODULE_H__
> > >>> +#define __LIBCAMERA_IPA_MODULE_H__
> > >>> +
> > >>> +#include <string>
> > >>> +
> > >>> +namespace libcamera {
> > >>> +
> > >>> +struct IPAModuleInfo {
> > >>> + char name[256];
> > >>> + unsigned int version;
> > >>> +};
> > >>> +
> > >>
> > >> So IPModuleInfo (and then later, the class definition for how a
> > >> developer would construct an IPA Module) should live in the public
> > >> headers at:
> > >>
> > >> /include/libcamera/ipa_module.h
> > >
> > > Yeah.
> > >
> > > Then, where should class IPAModule go?
> >
> > Stays here in this file I think :-)
>
> That's what I had envisioned, so it sounds good to me.
>
> > >
> > >>> +class IPAModule
> >
> > I'm wondering if we have two sides if this class should be calls
> > IPAModule though.
> >
> > Surely an IPAModule would implement a class called IPAModule, and
> > perhaps 'this' class is an IPAModuleParser? IPAModuleLoader?
> >
> > Probably a Loader :D
>
> I was wondering about this. One option would indeed to provide an
> IPAModule class with pure virtual functions that the module would need
> to implement. Another option would be to use a structure with C-style
> function pointers. I was thinking about the latter, but the former is
> not a bad idea. However, we need to take care about backward
> compatibility. Can a module compiled with an old version of g++ (or
> clang++) be incompatible with libcamera built with a newer version if
> using a C++ ABI ?
>
> Ie we go the C++ way, we will indeed need two different names for the
> classes. I was thinking about IPAModule for this class and
> IPAModuleInterface for the new one, in which case no change would be
> needed here. Note that IPAModule would inherit from IPAModuleInterface
> as it would provide the IPA API to pipeline handlers. I don't like the
> name IPAModuleLoader much, as from a pipeline handler point of view
> we're using modules, not module loaders. Other options may be possible.
I don't know about C++ ABI compatability. I think to just avoid the risk
itself we should go with C-style function pointers.
In which case Laurent's shim idea would work well for process isolation
later on. Since the only layers are Pipeline -> IPAModule there isn't
really anywhere else we can break off into another process without
making the Pipeline -> IPAModule interface ugly.
> > >>> +{
> > >>> +public:
> > >>> + explicit IPAModule(const std::string &libPath, const std::string &symbol);
>
> No need to pass the symbol name here, you can hardcode it in the
> implementation. All modules will use the same symbol name.
>
> > >>> +
> > >>> + bool isValid() const;
> > >>> +
> > >>> + struct IPAModuleInfo const &IPAModuleInfo() const;
>
> We usually write this const struct IPAModuleInfo &IPAModuleInfo() const.
> And I would name the function info(), as IPAModule::info() makes it
> clear that we're retrieving the information of an IPA module.
>
> > >>> +
> > >>> +private:
> > >>> + struct IPAModuleInfo info_;
> > >>> +
> > >>> + bool loaded_;
>
> Let's name this valid_ to match isValid(). We will later have a load()
> function to dlopen() the module, and likely a loaded_ flag to match
> that.
>
> > >>> + int bitClass_;
> > >>> +
> > >>> + int loadELFIdent(int fd);
> > >>> + template<class ElfHeader, class SecHeader, class SymHeader>
> > >>> + int loadSymbol(void *data, size_t size, char *map, size_t soSize,
> > >>> + const char *symbol);
> > >>> + int loadIPAModuleInfo(const char *libPath, const char *symbol);
> > >>> +};
> > >>> +
> > >>> +} /* namespace libcamera */
> > >>> +
> > >>> +#endif /* __LIBCAMERA_IPA_MODULE_H__ */
> > >>> diff --git a/src/libcamera/ipa_module.cpp b/src/libcamera/ipa_module.cpp
> > >>> new file mode 100644
> > >>> index 0000000..5ca16e8
> > >>> --- /dev/null
> > >>> +++ b/src/libcamera/ipa_module.cpp
> > >>> @@ -0,0 +1,277 @@
> > >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > >>> +/*
> > >>> + * Copyright (C) 2019, Google Inc.
> > >>> + *
> > >>> + * ipa_module.cpp - load IPA module information from shared library at runtime
>
> I think we can already update the description to "Image Processing
> Algorithm module" or similar
>
> > >>> + */
> > >>> +
> > >>> +#include "ipa_module.h"
> > >>> +
> > >>> +#include <elf.h>
> > >>> +#include <errno.h>
> > >>> +#include <fcntl.h>
> > >>> +#include <string.h>
> > >>> +#include <sys/mman.h>
> > >>> +#include <sys/stat.h>
> > >>> +#include <sys/types.h>
> > >>> +#include <unistd.h>
> > >>> +
> > >>> +#include <iostream>
> > >>> +
> > >>> +#include "log.h"
> > >>> +
> > >>> +/**
> > >>> + * \file ipa_module.h
> > >>> + * \brief IPA module information loading
>
> Same here.
>
> > >>> + */
> > >>> +
> > >>> +namespace libcamera {
> > >>> +
> > >>> +/**
> > >>> + * \struct IPAModuleInfo
> > >>> + * \brief Information of an IPA plugin
> > >>> + *
> > >>> + * This structure contains the information of an IPA plugin. It is loaded,
> > >>> + * read, and validated before anything else is loaded from the plugin.
> > >>> + *
> > >>> + * \var IPAModuleInfo::name
> > >>> + * \brief The name of the IPA plugin
> > >>> + *
> > >>> + * \var IPAModuleInfo::version
> > >>> + * \brief The version of the IPA plugin
> > >>
> > >> I don't think we need to store the 'version' of the plugin, so much as
> > >> the version of the API it was compiled against to ensure that it is
> > >> compatible with this 'running' instance of libcamera.
> > >>
> > >> I.e. We don't care that it's the 3rd iteration of the rk3399 module -
> > >> but we do care that it is compatible with the API defined in Libcamera 1.0.
> > >
> > > Yeah, good point; I agree.
> > >
> > > How would we get the API version of libcamera?
> >
> > You get to define it :-)
> >
> > To start with it's probably just:
> >
> > #define LIBCAMERA_IPA_API_VERSION 1
> >
> > This would be a 'local' IPAModule API version number or such I think, as
> > our IPA modules must go through a defined interface...
> >
> > Any time an ABI break occurs in the IPAModule 'API' we would have to
> > bump up the apiversion.
>
> I agree, but I think we can defer this. The IPAModuleInfo structure is
> currently a mock up to start implementing the IPAModule class, I expect
> it to change a lot.
>
> > >> Maybe we should add compatible strings to match against pipeline
> > >> handlers as some point too :-) <I'm not even sure if I'm joking now>
>
> I think we will need that :-)
>
> Paul, could you record there open items ?
I'm not sure what you mean.
> > >>> + */
> > >>> +
> > >>> +LOG_DEFINE_CATEGORY(IPAModule)
> > >>> +
> > >>> +template<typename T>
> > >>> +typename std::remove_extent<T>::type *elfPointer(void *map, off_t offset,
> > >>> + size_t fileSize)
> > >>> +{
> > >>> + size_t size = offset + sizeof(T);
> > >>> + if (size > fileSize || size < sizeof(T))
> > >>> + return nullptr;
> > >>> +
> > >>> + return reinterpret_cast<typename std::remove_extent<T>::type *>
> > >>> + (static_cast<char *>(map) + offset);
> > >>> +}
>
> This function should be static. The C way of doing so is using the
> static keyword, the C++ way is to put it in an anonymous namespace.
>
> namespace {
>
> template<typename T>
> typename std::remove_extent<T>::type *elfPointer(void *map, off_t offset,
> size_t fileSize)
> {
> ...
> }
>
> };
>
> > >>> +
> > >>> +/**
> > >>> + * \class IPAModule
> > >>> + * \brief Load IPA module information from an IPA plugin
>
> To match the suggestion above, "IPA module wrapper" ? I think we can do
> better than that, maybe "Wrapper around an IPA module shared object" ?
> Suggestions are welcome.
>
> > >>> + */
> > >>> +
> > >>> +/**
> > >>> + * \brief Construct an IPAModule instance
> > >>> + * \param[in] libPath path to IPA plugin
> > >>> + * \param[in] symbol name of IPAModuleInfo to load from libPath
> > >>> + *
> > >>> + * Loads the IPAModuleInfo of the given symbol from the IPA plugin at libPath.
> > >>> + * The IPA plugin shared object file must be of the same endianness and
> > >>> + * bitness as libcamera.
> > >>> + *
> > >>> + * Note that isValid() should be called to check if this loading was successful
> > >>> + * or not.
> > >>
> > >> Hrm ... this is a slightly different design pattern than the rest of our
> > >> objects which are always constructable, and then actions later can fail.
> > >>
> > >> I don't mind - but I wonder if we should add some documentation
> > >> regarding our design patterns somewhere.
> > >>
> > >> (not an action on this patch)
>
> I've discussed this with Paul, and I think that in this case this
> implementation makes sense. There's no need to delay loading, and as the
> IPAModule class is a wrapper around a .so, constructing an instance on
> which on can call isValid() to check if the IPA is valid makes sense to
> me.
>
> Maybe we need to reconsider our other classes indeed. Any
> suggestion/preference ?
Do you mean to make the other classes follow the same model? As long as
we document the exception (well the usage is already documented...) then
shouldn't it be fine?
> > >>> + */
> > >>> +
> > >>> +IPAModule::IPAModule(const std::string &libPath, const std::string &symbol)
> > >>> + : loaded_(false)
> > >>
> > >> So actually, from my text below - what I'm really saying is that I don't
> > >> think you should provide a &symbol to this IPAModule constructor :)
>
> So we agree :-)
>
> > >>> +{
> > >>> + int ret = loadIPAModuleInfo(libPath.c_str(), symbol.c_str());
> > >>
> > >> I see that you have moved the load into this constructor based on
> > >> comments from Laurent in v1. I think that's fine, but I think merging
> > >> the loading of the object, and the parsing of the symbol might be
> > >> combining too much. But it depends (as ever).
>
> Note that this only loads the module info, not the module itself (I
> foresee a load() function that will dlopen() the module). I'm not sure
> what you mean by merging the loading of the object and the parsing of
> the symbol.
>
> > >> For efficiency, the constructor can load the file once and when created
> > >> and .isValid() is true, then we know we can successfully parse symbols.
>
> But you can't know it's a valid IPA unless you can load the
> IPAModuleInfo successfully (and later maybe even cryptographically
> verifying the module).
>
> > >> But I think loadIPAModuleInfo() should be 'hardcoded' to know exactly
> > >> which symbol name it will expect for an IPAModule. Think of it like a
> > >> 'main()' entry point for our IPAModule. You would always expect a C
> > >> binary to have a main() symbol...
> > >
> > > Yeah, I think that's a good idea.
> > >
> > >> So I would expect a libcamera IPA module to have a registration
> > >> something like:
> > >>
> > >>
> > >> /* Register our module with Libcamera */
> > >> const struct IPAModuleInfo ipaModuleInfo = {
> > >> .name = "Image Processing for the RK3399",
> > >> .version = 1, /* Perhaps this should be apiversion to prevent
> > >> * people thinking it describes the version of the
> > >> * module ? ...
> > >> */
> > >> };
> > >
> > > Should the info entry point be called ipaModuleInfo?
> >
> > I think variables are in camelCase with a lowerCase first letter in the
> > project right? or was this not quite your question?
>
> This sounds good to me.
My question was about the entry point name, not the casing :p
> > >> This could even then be turned into a macro:
> > >>
> > >> #define LIBCAMERA_IPA_MODULE(__NAME) \
> > >> const struct IPAModuleInfo ipaModuleInfo = { \
> > >> .name = __NAME, \
> > >> .apiversion = 1, \
> > >> }
>
> We'll have to throw an extern "C" here, otherwise your symbol name will
> be _ZL13ipaModuleInfo with g++.
>
> > >>
> > >> so that a module rk3399_ipa.cpp could define:
> > >>
> > >> LIBCAMERA_IPA_MODULE("Image Processing Algorithms for the RK3399");
> > >>
> > >> (apiversion would be hardcoded by the macro because it defines what
> > >> version of the API/libraries it's compiled against..., and we check it's
> > >> compatible at runtime)
>
> Something like that. If the number of fields grows, and if some of them
> become complex, we may want to fill the structure manually, be we should
> still use a macro to instantiate the right structure with the right name
> and the extern "C" keyword.
Yeah I think I'll do that.
> > >>> + if (ret < 0) {
> > >>> + loaded_ = false;
>
> This isn't needed, you initialise loaded_ to false above.
>
> > >>> + return;
> > >>> + }
> > >>> +
> > >>> + loaded_ = true;
> > >>> +}
> > >>> +
> > >>> +int IPAModule::loadELFIdent(int fd)
>
> This function doesn't actually load much, how about renaming it to
> verifyELFIdent() ?
>
> > >>> +{
> > >>> + int ret = lseek(fd, 0, SEEK_SET);
> > >>> + if (ret < 0)
> > >>> + return -errno;
> > >>> +
>
> As we're mmap()ing the whole .so, you could mmap() before calling
> loadELFIdent(), and pass the void *map and size to the function, instead
> of using read.
I didn't want to have to do elfPointer for every single byte that I read
it, but I forgot that I could just use elfPointer on an array.
> > >>> + unsigned char e_ident[EI_NIDENT];
> > >>> + ret = read(fd, e_ident, EI_NIDENT);
> > >>> + if (ret < 0)
> > >>> + return -errno;
> > >>> +
> > >>> + if (e_ident[EI_MAG0] != ELFMAG0 ||
> > >>> + e_ident[EI_MAG1] != ELFMAG1 ||
> > >>> + e_ident[EI_MAG2] != ELFMAG2 ||
> > >>> + e_ident[EI_MAG3] != ELFMAG3 ||
> > >>> + e_ident[EI_VERSION] != EV_CURRENT)
> > >>> + return -ENOEXEC;
> > >>> +
> > >>> + if ((e_ident[EI_CLASS] == ELFCLASS32 && sizeof(unsigned long) == 4) ||
> > >>> + (e_ident[EI_CLASS] == ELFCLASS64 && sizeof(unsigned long) == 8))
> > >>> + bitClass_ = e_ident[EI_CLASS];
> > >>> + else
> > >>> + return -ENOEXEC;
>
> How about
>
> bitClass_ = sizeof(unsigned long) == 4 ? ELFCLASS32 : ELFCLASS64;
> if (e_ident[EI_CLASS] != bitClass_)
> return -ENOEXEC;
Yeah. I guess we're going to demote bitClass_ to a local variable.
> > >>> +
> > >>> + int a = 1;
> > >>> + unsigned char endianness = *reinterpret_cast<char *>(&a) == 1
> > >>> + ? ELFDATA2LSB : ELFDATA2MSB;
>
> So you didn't like my union-based solution ? :-)
Not really :p
imo this was more readable.
> > >>> + if (e_ident[EI_DATA] != endianness)
> > >>> + return -ENOEXEC;
> > >>> +
> > >>> + return 0;
> > >>> +}
> > >>> +
> > >>> +template<class ElfHeader, class SecHeader, class SymHeader>
> > >>> +int IPAModule::loadSymbol(void *dst, size_t size, char *map, size_t soSize,
> > >>> + const char *symbol)
> > >>> +{
> > >>> + ElfHeader *eHdr = elfPointer<ElfHeader>(map, 0, soSize);
> > >>> + if (!eHdr)
> > >>> + return -ENOEXEC;
> > >>> +
> > >>> + off_t offset = eHdr->e_shoff + eHdr->e_shentsize * eHdr->e_shstrndx;
> > >>> + SecHeader *sHdr = elfPointer<SecHeader>(map, offset, soSize);
> > >>> + if (!sHdr)
> > >>> + return -ENOEXEC;
> > >>> + off_t shnameoff = sHdr->sh_offset;
> > >>> +
> > >>> + /* Locate .dynsym section header. */
> > >>> + bool found = false;
> > >>> + SecHeader *dynsym;
> > >>> + for (unsigned int i = 0; i < eHdr->e_shnum; i++) {
> > >>> + offset = eHdr->e_shoff + eHdr->e_shentsize * i;
> > >>> + sHdr = elfPointer<SecHeader>(map, offset, soSize);
> > >>> + if (!sHdr)
> > >>> + return -ENOEXEC;
> > >>> +
> > >>> + offset = shnameoff + sHdr->sh_name;
> > >>> + char *name = elfPointer<char>(map, offset, soSize);
>
> This should be elfPointer<char[8]> as you want to ensure that at least 8
> bytes are available (sizeof(".dynsym"), including the terminating 0).
Ah right, I forgot that sizeof(T) was there.
> > >>> + if (!name)
> > >>> + return -ENOEXEC;
> > >>> +
> > >>> + if (sHdr->sh_type == SHT_DYNSYM && !strcmp(name, ".dynsym")) {
> > >>> + found = true;
> > >>> + dynsym = sHdr;
>
> You could initialise dynsym to nullptr and remove found.
>
> > >>> + break;
> > >>> + }
> > >>> + }
> > >>> +
> > >>> + if (!found) {
> > >>> + LOG(IPAModule, Error) << "ELF has no .dynsym section";
> > >>> + return -ENOEXEC;
> > >>> + }
> > >>> +
> > >>> + offset = eHdr->e_shoff + eHdr->e_shentsize * dynsym->sh_link;
> > >>> + sHdr = elfPointer<SecHeader>(map, offset, soSize);
> > >>> + if (!sHdr)
> > >>> + return -ENOEXEC;
> > >>> + off_t dynsym_nameoff = sHdr->sh_offset;
> > >>> +
> > >>> + /* Locate symbol in the .dynsym section. */
> > >>> + found = false;
> > >>> + SymHeader *target_symbol;
>
> targetSymbol ?
>
> > >>> + unsigned int dynsym_num = dynsym->sh_size / dynsym->sh_entsize;
> > >>> + for (unsigned int i = 0; i < dynsym_num; i++) {
> > >>> + offset = dynsym->sh_offset + dynsym->sh_entsize * i;
> > >>> + SymHeader *sym = elfPointer<SymHeader>(map, offset, soSize);
> > >>> + if (!sym)
> > >>> + return -ENOEXEC;
> > >>> +
> > >>> + offset = dynsym_nameoff + sym->st_name;
> > >>> + char *name = elfPointer<char>(map, offset, soSize);
>
> Same here, you want elfPointer<char[strlen(symbol) + 1]>. I hope this
> will compile :-)
>
> > >>> + if (!name)
> > >>> + return -ENOEXEC;
> > >>> +
> > >>> + if (!strcmp(name, symbol) &&
> > >>> + sym->st_info & STB_GLOBAL &&
> > >>> + sym->st_size == sizeof(struct IPAModuleInfo)) {
> > >>
> > >> I think this should be a check on the size_t size passed in?
> > >
> > > Ah yeah, I totally missed that.
> >
> > No worries.
> >
> > >>> + found = true;
> > >>> + target_symbol = sym;
>
> You could also remove found here.
>
> > >>> + break;
> > >>> + }
> > >>> + }
> > >>> +
> > >>> + if (!found) {
> > >>> + LOG(IPAModule, Error) << "IPAModuleInfo symbol not found";
> > >>
> > >> This function is called loadSymbol, it doesn't 'know' that it's loading
> > >> an IPAModuleInfo symbol.
> > >>
> > >> I'd change this error to "Symbol " << symbol << " not found";
> > >
> > > And this.
> > >
> > >>> + return -ENOEXEC;
> > >>> + }
> > >>> +
> > >>> + /* Locate and return data of symbol. */
>
> As a sanity check you may want to verify that target_symbol->st_shndx <
> eHdr->e_shnum. Otherwise none of the elfPointer<> checks below may
> trigger, and you will load an invalid symbol. This can happen for
> instance if the symbol is absolute (SHN_ABS).
>
> > >>> + offset = eHdr->e_shoff + target_symbol->st_shndx * eHdr->e_shentsize;
> > >>> + sHdr = elfPointer<SecHeader>(map, offset, soSize);
> > >>> + if (!sHdr)
> > >>> + return -ENOEXEC;
> > >>> + offset = sHdr->sh_offset + (target_symbol->st_value - sHdr->sh_addr);
> > >>> + char *data = elfPointer<char>(map, offset, soSize);
>
> elfPointer<char[size]>
>
> > >>> + if (!data)
> > >>> + return -ENOEXEC;
> > >>> + memcpy(dst, data, size);
> > >>
> > >> Oh - interesting, you're copying the symbol out. Once it's mmapped ...
> > >> why not parse it directly?
> > >
> > > I'm not sure what you mean by "parse"?
> >
> > I mean once you find the symbol and obtain the pointer to it ('data'
> > here) then you have a pointer to memory which contains the structure.
> > You could return that pointer, and cast it to the type. Then you can
> > read the structure members directly.
> >
> > But that requires knowing that the symbol doesn't require any further
> > relocations ... which is fine for our simple info structure.
> >
> > It just seems logical in my head to have a function called findSymbol()
> > which returns a pointer to the symbol, and then (if you're going to copy
> > it) a function called loadStructure(), which finds the symbol, checks
> > the size then copies it or such. Maybe it's generalising the functions
> > more than we care if we will only use them for one thing
>
> I have no particular opinion on this. We could indeed rename this
> function to findSymbol() and move the copy outside. It may actually be a
> good idea, but then this function should return both the address (or
> offset) and size (one or both through output arguments).
I don't think we really need to separate it into find and load. I
suppose if findSymbol turns out to be useful somewhere down the road it
could be changed.
> > >> Can we do all of the 'work' we need to do on the file, and then close it
> > >> at the destructor?
> > >
> > > I don't want to keep the file open/mapped over multiple method calls.
> > > That's why in my first version there was a constructor that did nothing
> > > and an init() that did everything (which got moved to the constructor in
> > > this version).
>
> I agree, we shouldn't keep it mapped.
>
> > The only things we'll do on this structure is read it and decide if this
> > library is OK for loading, perhaps even printing the name string in debug.
> >
> > Once that has occurred, can't the IPAModuleInfo structure be discarded?
> > The next thing that we'll do is spawn a container/namespace/safe-thingy
> > and use the normal dl_open methods to re-open the library there...
>
> No, we need it at least to match the module with pipeline handlers, so
> it needs to be copied. I'm sure we'll have other fields that we will
> also need to access later.
>
> > >> I guess actually this might then keep the whole file mapped in memory
> > >> for longer which might not be desirable...
> > >>
> > >>> +
> > >>> + return 0;
> > >>> +}
> > >>> +
> > >>> +int IPAModule::loadIPAModuleInfo(const char *libPath, const char *symbol)
>
> I would store libPath in a member variable as we will need it for
> dlopen(), and use that member variable in this function.
>
> > >>> +{
> > >>> + int fd = open(libPath, O_RDONLY);
> > >>> + if (fd < 0) {
> > >>> + LOG(IPAModule, Error) << "Failed to open IPA library: "
> > >>> + << strerror(errno);
>
> You need
>
> ret = -errno;
> LOG(IPAModule, Error) << "Failed to open IPA library: "
> << strerror(-ret);
> return ret;
>
> ass errno may be changed by LOG(IPAModule, Error).
>
> > >>> + return fd;
> > >>> + }
> > >>> +
> > >>> + int ret = loadELFIdent(fd);
> > >>> + if (ret)
> > >>> + goto close;
> > >>> +
> > >>> + size_t soSize;
> > >>> + char *map;
> > >>> + struct stat st;
> > >>> + ret = fstat(fd, &st);
> > >>> + if (ret < 0)
> > >>> + goto close;
> > >>> + soSize = st.st_size;
> > >>> + map = static_cast<char *>(mmap(NULL, soSize, PROT_READ,
> > >>> + MAP_PRIVATE, fd, 0));
>
> struct stat st;
> ret = fstat(fd, &st);
> if (ret < 0)
> goto close;
>
> size_t soSize = st.st_size;
> char *map = static_cast<char *>(mmap(NULL, soSize, PROT_READ,
> MAP_PRIVATE, fd, 0));
Last time I tried that the compiler was complaining about the goto
jumping over declarations, or something along those lines. I'll try it
again.
> I think I would make it a void *map, pass it as a void * to all the
> functions, and let elfPointer<> do the case.
>
> > >> *iff* the constructor did the open, and the symbol parsing was
> > >> separated, then I think soSize and map would go to class privates. Then
> > >> they could be accessed directly by loadSymbol too. But that's an only if...
> > >>
> > >> Essentially, it would be
> > >>
> > >> IPAModule("SO-path")
> > >> - Constructor opens the file, performs stat, and mmap.
> > >> (or those become an open() call)
> > >> - loadIPAModuleInfo() calls loadSymbol("ipaModuleInfo"); and deals only
> > >> with the ipaModule info structure and checking or such.
> > >
> > > If loadIPAModuleInfo() is to be called from within the constructor
> > > (after open, stat, and mmap), then I agree. I'm not sure map and soSize
> > > need to be privates though. It could go either way.
>
> I thought about it, but it the address and size are really temporary
> variables, as we unmap the file after the parsing, so I think it's best
> to pass them explicitly to functions.
>
> > Well, I leave that all up to you now :-)
> >
> > >>> + if (map == MAP_FAILED) {
> > >>> + ret = -errno;
> > >>> + goto close;
> > >>> + }
> > >>> +
> > >>> + if (bitClass_ == ELFCLASS32)
> > >>> + ret = loadSymbol<Elf32_Ehdr, Elf32_Shdr, Elf32_Sym>
> > >>> + (&info_, sizeof(info_), map, soSize, symbol);
> > >>> + else if (bitClass_ == ELFCLASS64)
> > >>> + ret = loadSymbol<Elf64_Ehdr, Elf64_Shdr, Elf64_Sym>
> > >>> + (&info_, sizeof(info_), map, soSize, symbol);
>
> As we don't need to support a module with a different bit class than
> libcamera, would we avoid compiling in both version of the function ?
> Maybe something like
>
> #if sizeof(long) == 4
> ret = loadSymbol<Elf32_Ehdr, Elf32_Shdr, Elf32_Sym>
> (&info_, sizeof(info_), map, soSize, symbol);
> #else
> ret = loadSymbol<Elf64_Ehdr, Elf64_Shdr, Elf64_Sym>
> (&info_, sizeof(info_), map, soSize, symbol);
> #endif
>
> and given that we only need one version of the function, I even wonder
> if we need to keep it defined as a template function, maybe we could
> have a
>
> #if sizeof(long) == 4
> #define Elf_Ehdr Elf32_Ehdr
> #define Elf_Shdr Elf32_Shdr
> #define Elf_Sym Elf32_Sym
> #else
> #define Elf_Ehdr Elf64_Ehdr
> #define Elf_Shdr Elf64_Shdr
> #define Elf_Sym Elf64_Sym
> #endif
>
> just before the function definition. This would however require moving
> the function away from the class to a global function, private to this
> file, to avoid exposing it in the ipa_module.h header, otherwise we
> would have to move the above #if's there, which isn't neat. If we do
> this we should move the loadElfIdent function too. Neither function use
> any of the class members (except for bitClass_, which isn't needed
> anymore), so they're essentially static, and separating them from the
> IPAModule class would open the door to sharing them with other classes
> if needed later. elfPointer<> is already separate. If you prefer to keep
> them as class members, they should be declared as static in the class
> definition.
I was actually thinking about separating them from IPAModule before
anyway, so I think I'll go this route.
> > >>> + if (ret)
> > >>> + goto unmap;
> > >>> +
> > >>> +unmap:
> > >>> + munmap(map, soSize);
> > >>> +close:
> > >>> + close(fd);
> > >>> + return ret;
> > >>> +}
> > >>> +
> > >>> +/**
> > >>> + * \brief Check if construction of the IPAModule instance succeeded
>
> I would say "Check if the IPAModule instance is valid" and then add a
> paragraph to detail what a valid (or invalid) IPAModule is.
>
> > >>> + *
> > >>> + * \return true if the constructor succeeded with no errors, false otherwise
>
> true if the IPAModule is valid, false otherwise
>
> > >>> + */
> > >>> +
>
> No need for this blank line.
Huh, I thought I saw some other file do this. Maybe I missaw :/
> > >>> +bool IPAModule::isValid() const
> > >>> +{
> > >>> + return loaded_;
> > >>> +}
> > >>> +
> > >>> +/**
> > >>> + * \brief Get the loaded IPAModuleInfo
>
> "Get the IPA module information"
>
> > >>> + *
> > >>> + * Check isValid() before calling this.
>
> "The content of the IPA module information is loaded from the module,
> and is valid only if the module is valid (as returned by isValid()).
> Calling this function on an invalid module is an error."
>
> > >>> + *
> > >>> + * \return IPAModuleInfo
> > >>> + */
> > >>> +
>
> No need for a blank line here either
>
> > >>> +struct IPAModuleInfo const &IPAModule::IPAModuleInfo() const
> > >>> +{
> > >>> + return info_;
> > >>> +}
> > >>> +
> > >>> +} /* namespace libcamera */
> > >>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > >>> index 8796f49..e5b48f2 100644
> > >>> --- a/src/libcamera/meson.build
> > >>> +++ b/src/libcamera/meson.build
> > >>> @@ -10,6 +10,7 @@ libcamera_sources = files([
> > >>> 'event_notifier.cpp',
> > >>> 'formats.cpp',
> > >>> 'geometry.cpp',
> > >>> + 'ipa_module.cpp',
> > >>> 'log.cpp',
> > >>> 'media_device.cpp',
> > >>> 'media_object.cpp',
> > >>> @@ -31,6 +32,7 @@ libcamera_headers = files([
> > >>> 'include/device_enumerator_udev.h',
> > >>> 'include/event_dispatcher_poll.h',
> > >>> 'include/formats.h',
> > >>> + 'include/ipa_module.h',
> > >>> 'include/log.h',
> > >>> 'include/media_device.h',
> > >>> 'include/media_object.h',
Everything else is either an ack or an affirmative.
Thanks,
Paul
More information about the libcamera-devel
mailing list