Re: [flashrom] Libflashrom draft patch - initial review

[Антон Кочков]

Uh, forgot to add, that loading layout will be quite useful too, in the API and
adding romentry_t* argument for all fl_image_* functions.
Best regards,
Anton Kochkov.

On Fri, Jun 19, 2015 at 2:43 PM, Антон Кочков  wrote:
> Hi!
> I've done also some rebasing of the libflashrom patches before (on top
> of the layout branch from the stefanct)
> https://github.com/XVilka/flashrom/commits/libflashrom
>
> May be you'll find something useful here.
> Best regards,
> Anton Kochkov.
>
> On Thu, Jun 18, 2015 at 7:45 AM, Urja Rannikko  wrote:
> > Hi,
> >
> > On Fri, Jun 12, 2015 at 12:26 AM, Łukasz Dmitrowski
> >  wrote:
> > > Hello,
> > >
> > > My name is Łukasz Dmitrowski and I am one of coreboot/flashrom GSoC students
> > > this year. You can read about my project here:
> > > http://blogs.coreboot.org/blog/author/dmitro/.
> > >
> > > One of my first steps is updating and extending Nico Hubers libflashrom
> > > patch. Work is still in progress, but some changes are already made, so I
> > > thought that it will be good idea to send a patch just for review to know if
> > > I am going in a good direction. I want to improve my skills and provide best
> > > code quality as I can, so I am open for criticism - I am here to learn :)
> >
> > I'm here essentially trying out reviewing stuff, so expect more from
> > somebody a bit more experienced, but I heard my comments would be
> > useful, so moving on :)
> > > Patch summary:
> >
> > First thing i have to say about the patch is that it has been
> > malformed, most likely line wrapped by your email client. If you cant
> > properly include the patch inline, attaching a .patch would be okay.
> > (Also turn off that HTML output if you can... although it wasnt used
> > for anything, but still.). So I didnt do any compile testing on this
> > stuff because of this.
> > > Makefile: I added cli_common.o and print.o to LIB_OBJS - it solves undefined
> > > references in current libflashrom version, but spoils standard flashrom
> > > build with 'make', I did not investigated it yet as my focus is to extend
> > > library with new functions and update already existing, need to discuss it
> > > with my mentor
> > >
> > > cli_classic.c, cli_output.c, flash.h: Only Nico Hubers changes here, didn't
> > > touch it
> >
> > I might comment on any part of the patch because I havent reviewed the
> > original and it's only about the code.
> > > libflashrom.c, libflashrom.h: I added 8 functions and made a few changes to
> > > existing ones, as some of used functions are static, or does not exist.
> > >
> > > New functions:
> > >
> > > int fl_supported_programmers(const char **supported_programmers);
> > > int fl_supported_flash_chips(fl_flashchip_info_t *fchips);
> > > int fl_supported_boards(fl_board_info_t *boards);
> > > int fl_supported_chipsets(fl_chipset_info_t *chipsets);
> > > int fl_supported_programmers_number();
> > > int fl_supported_flash_chips_number();
> > > int fl_supported_boards_number();
> > > int fl_supported_chipsets_number();
> > >
> > > New types:
> > > fl_flashchip_info_t - smaller version of struct flashchip
> > > fl_board_info_t - smaller version of struct board_info
> > > fl_chipset_info_t - smaller version of struct penable
> > > fl_test_state - copy of enum test_state
> > >
> > > So we have 4 functions that return a list of supported programmers, chips,
> > > boards and chipsets. Next 4 functions return number of supported hardware of
> > > certain type. For example, to obtain a list of supported boards, you can
> > > create an array of structures fl_board_info of suitable size (the size is
> > > known - fl_supported_boards_number()), then pass it to
> > > fl_supported_boards(fl_board_info_t *boards) and you will have it filled
> > > with data.
> >
> > My opinion is that the _number functions make the API unnecessarily
> > complex to use.
> > I'm suggesting like this:
> > fl_flashchip_info_t * fl_supported_flash_chips(void);
> > Have the call allocate the table using a not specified allocator
> > (really just malloc for now, but we're not telling), and freed with a
> > function like
> > int fl_support_info_free(void*p);
> > This would allow implementation using const tables or allocation,
> > whatever we like.
> > (if (p == const_table) return 0; in our _free() if mixed.)
> > > Regarding changes in Nico Hubers functions: I made temporary comments for
> > > every change and marked it with LD_CHANGE_NOTE. Every comment contains
> > > previous code and reason why it has been commented out.
> > >
> > > Thanks in advance for your insights!
> > >
> > > Regards,
> > > Lukasz Dmitrowski
> > >
> > > Signed-off-by: Łukasz Dmitrowski
> > > Index: Makefile
> > >
> > > --- Makefile    (revision 1891)
> > > +++ Makefile    (working copy)
> > > @@ -374,7 +374,7 @@
> > >
> > > ###############################################################################
> > >  # Library code.
> > >
> > > -LIB_OBJS = layout.o flashrom.o udelay.o programmer.o helpers.o
> > > +LIB_OBJS = libflashrom.o layout.o flashrom.o udelay.o programmer.o
> > > helpers.o cli_common.o print.o
> > >
> > > ###############################################################################
> > >  # Frontend related stuff.
> > > Index: cli_classic.c
> > > ===================================================================
> > > --- cli_classic.c    (revision 1891)
> > > +++ cli_classic.c    (working copy)
> > > @@ -30,6 +30,7 @@
> > >  #include "flash.h"
> > >  #include "flashchips.h"
> > >  #include "programmer.h"
> > > +#include "libflashrom.h"
> > >
> > > static void cli_classic_usage(const char *name)
> > >  {
> > > @@ -135,6 +136,8 @@
> > >      char *tempstr = NULL;
> > >      char *pparam = NULL;
> > > fl_set_log_callback((fl_log_callback_t *)&fl_print_cb);
> > > print_version();
> > >    print_banner();
> > > Index: cli_output.c
> > >
> > > --- cli_output.c    (revision 1891)
> > > +++ cli_output.c    (working copy)
> > > @@ -71,9 +71,8 @@
> > >  #endif /* !STANDALONE */
> > >
> > > /* Please note that level is the verbosity, not the importance of the
> > > message. */
> > > -int print(enum msglevel level, const char *fmt, ...)
> > > +int fl_print_cb(enum msglevel level, const char *fmt, va_list ap)
> > >  {
> > > va_list ap;
> > > int ret = 0;
> > > FILE *output_type = stdout;
> > >
> > > @@ -81,9 +80,7 @@
> > >          output_type = stderr;
> > >  if (level <= verbose_screen) {
> > >    va_start(ap, fmt);
> > >    ret = vfprintf(output_type, fmt, ap);
> > >    va_end(ap);
> > >    /* msg_*spew often happens inside chip accessors in possibly
> > >     * time-critical operations. Don't slow them down by flushing. */
> > >    if (level != MSG_SPEW)
> > >
> > > @@ -91,9 +88,7 @@
> > >      }
> > >  #ifndef STANDALONE
> > >      if ((level <= verbose_logfile) && logfile) {
> > >    va_start(ap, fmt);
> > >    ret = vfprintf(logfile, fmt, ap);
> > >    va_end(ap);
> > >    if (level != MSG_SPEW)
> > >        fflush(logfile);
> > > }
> > > Index: flash.h
> > >
> > > --- flash.h    (revision 1891)
> > > +++ flash.h    (working copy)
> > > @@ -31,6 +31,7 @@
> > >  #include <stdint.h>
> > >  #include <stddef.h>
> > >  #include <stdbool.h>
> > > +#include <stdarg.h>
> > >  #if IS_WINDOWS
> > >  #include <windows.h>
> > >  #undef min
> > > @@ -317,6 +318,7 @@
> > >      MSG_DEBUG2    = 4,
> > >      MSG_SPEW    = 5,
> > >  };
> > > +int fl_print_cb(enum msglevel level, const char *fmt, va_list ap);
> > >  /* Let gcc and clang check for correct printf-style format strings. */
> > >  int print(enum msglevel level, const char *fmt, ...)
> > >  #ifdef __MINGW32__
> > > Index: libflashrom.c
> > > ===================================================================
> > > --- libflashrom.c    (revision 0)
> > > +++ libflashrom.c    (working copy)
> > > @@ -0,0 +1,603 @@
> > > +/*
> > > This file is part of the flashrom project.
> > > Copyright (C) 2012 secunet Security Networks AG
> > > (Written by Nico Huber  for secunet)
> > > This program is free software; you can redistribute it and/or modify
> > > it under the terms of the GNU General Public License as published by
> > > the Free Software Foundation; either version 2 of the License, or
> > > (at your option) any later version.
> > > This program is distributed in the hope that it will be useful,
> > > but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > GNU General Public License for more details.
> > > You should have received a copy of the GNU General Public License
> > > along with this program; if not, write to the Free Software
> > > Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301
> > >
> > > USA
> > > */
> > >
> > > +/**
> > > @mainpage
> > > Have a look at the Modules section for a function reference.
> > > */
> > >
> > > +#include <stdlib.h>
> > > +#include <string.h>
> > > +#include <stdarg.h>
> > >
> > > +#include "flash.h"
> > > +#include "programmer.h"
> > > +#include "libflashrom.h"
> > >
> > > +/**
> > > @defgroup fl-general General
> > > @{
> > > */
> > >
> > > +/** Pointer to log callback function. */
> > > +static fl_log_callback_t *fl_log_callback = NULL;
> > >
> > > +/**
> > > @brief Initialize libflashrom.
> > > @param perform_selfcheck If not zero, perform a self check.
> > > @return 0 on success
> > > */
> > >
> > > +int fl_init(const int perform_selfcheck)
> > > +{
> > >    if (perform_selfcheck && selfcheck())
> > >            return 1;
> > >    myusec_calibrate_delay();
> > >    return 0;
> > >
> > > +}
> > >
> > > +/**
> > > @brief Shut down libflashrom.
> > > @return 0 on success
> > > */
> > >
> > > +int fl_shutdown(void)
> > > +{
> > >    return 0; /* TODO: nothing to do? */
> >
> > I'm pretty sure there is, but need to investigate more.
> > > +}
> > >
> > > +/* TODO: fl_set_loglevel()? do we need it?
> > >     For now, let the user decide in his callback. */
> > >
> > > +/**
> > > @brief Set the log callback function.
> > > Set a callback function which will be invoked whenever libflashrom wants
> > > to output messages. This allows frontends to do whatever they see fit
> > >
> > > with
> > > such messages, e.g. write them to syslog, or to file, or print them in a
> > > GUI window, etc.
> > > @param log_callback Pointer to the new log callback function.
> > > */
> > >
> > > +void fl_set_log_callback(fl_log_callback_t *const log_callback)
> > > +{
> > >    fl_log_callback = log_callback;
> > >
> > > +}
> > > +/** @private */
> > > +int print(const enum msglevel level, const char *const fmt, ...)
> > > +{
> > >    if (fl_log_callback) {
> > >            int ret;
> > >            va_list args;
> > >            va_start(args, fmt);
> > >            ret = fl_log_callback(level, fmt, args);
> > >            va_end(args);
> > >            return ret;
> > >    }
> > >    return 0;
> > >
> > > +}
> > >
> > > +/** @} */ /* end fl-general */
> > >
> > > +/**
> > > @defgroup fl-query Querying
> > > @{
> > > */
> > >
> > > +int fl_supported_programmers(const char **supported_programmers)
> > > +{
> > >    int ret = 0;
> > >    enum programmer p = 0;
> > >    if (supported_programmers != NULL) {
> > >            for (; p < PROGRAMMER_INVALID; ++p)
> > >                    supported_programmers[p] =
> > >
> > > programmer_table[p].name;
> > >    } else {
> > >            ret = 1;
> > >    }
> > >    return ret;
> > >
> > > +}
> > >
> > > +int fl_supported_programmers_number()
> > > +{
> > >    return PROGRAMMER_INVALID;
> > >
> > > +}
> > >
> > > +int fl_supported_flash_chips(fl_flashchip_info_t *fchips)
> > > +{
> > >    int ret = 0;
> > >    int i = 0;
> > >    if (fchips != NULL) {
> > >            for (; i < flashchips_size; ++i) {
> > >                    fchips[i].vendor = flashchips[i].vendor;
> > >                    fchips[i].name = flashchips[i].name;
> > >                    fchips[i].tested.erase =
> > >
> > > flashchips[i].tested.erase;
> > >                    fchips[i].tested.probe =
> > >
> > > flashchips[i].tested.probe;
> > >                    fchips[i].tested.read = flashchips[i].tested.read;
> > >                    fchips[i].tested.write =
> > >
> > > flashchips[i].tested.write;
> > >                    fchips[i].total_size = flashchips[i].total_size;
> > >            }
> > >    } else {
> > >            ret = 1;
> > >    }
> > >    return ret;
> > >
> > > +}
> > >
> > > +int fl_supported_flash_chips_number()
> > > +{
> > >    return flashchips_size;
> > >
> > > +}
> > >
> > > +int fl_supported_boards(fl_board_info_t *boards)
> > > +{
> > >    int ret = 0;
> > >    const struct board_info *binfo = boards_known;
> > >    if (boards != NULL) {
> > >            while (binfo->vendor != NULL) {
> > >                    boards->vendor = binfo->vendor;
> > >                    boards->name = binfo->name;
> > >                    boards->working = binfo->working;
> > >                    ++binfo;
> > >                    ++boards;
> > >            }
> > >    } else {
> > >            ret = 1;
> > >    }
> > >    return ret;
> > >
> > > +}
> > >
> > > +int fl_supported_boards_number()
> > > +{
> > >    int boards_number = 0;
> > >    const struct board_info *binfo = boards_known;
> > >    while (binfo->vendor != NULL) {
> > >            ++boards_number;
> > >            ++binfo;
> > >    }
> > >    return boards_number;
> > >
> > > +}
> > >
> > > +int fl_supported_chipsets(fl_chipset_info_t *chipsets)
> > > +{
> > >    int ret = 0;
> > >    const struct penable *chipset = chipset_enables;
> > >    if (chipsets != NULL) {
> > >            while (chipset->vendor_name != NULL) {
> > >                    chipsets->vendor = chipset->vendor_name;
> > >                    chipsets->chipset = chipset->device_name;
> > >                    chipsets->status = chipset->status;
> > >                    ++chipset;
> > >                    ++chipsets;
> >
> > These names are confusing (just FYI), i first thought we're counting
> > chipsets here..
> > >            }
> > >    } else {
> > >            return ret;
> > >    }
> > >    return ret;
> > >
> > > +}
> > >
> > > +int fl_supported_chipsets_number()
> > > +{
> > >    int chipsets_number = 0;
> > >    const struct penable *chipset = chipset_enables;
> > >    while (chipset->vendor_name != NULL) {
> > >            ++chipsets_number;
> > >            ++chipset;
> > >    }
> > >    return chipsets_number;
> > >
> > > +}
> > >
> > > +/** @} */ /* end fl-query */
> > >
> > > +/**
> > > @defgroup fl-prog Programmers
> > > @{
> > > */
> > >
> > > +/**
> > > @brief Initialize the specified programmer.
> > > @param prog_name Name of the programmer to initialize.
> > > @param prog_param Pointer to programmer specific parameters.
> > > @return 0 on success
> > > */
> > >
> > > +int fl_programmer_init(const char *const prog_name, const char *const
> > > prog_param)
> > > +{
> > >    unsigned prog;
> > >    for (prog = 0; prog < PROGRAMMER_INVALID; prog++) {
> > >            if (strcmp(prog_name, programmer_table[prog].name) == 0)
> > >                    break;
> > >    }
> > >    if (prog >= PROGRAMMER_INVALID) {
> > >            msg_ginfo("Error: Unknown programmer \"%s\". Valid choices
> > >
> > > are:\n", prog_name);
> > >            list_programmers_linebreak(0, 80, 0);
> > >            return 1;
> > >    }
> > >    return programmer_init(prog, prog_param);
> > >
> > > +}
> > >
> > > +/**
> > > @brief Shut down the initialized programmer.
> > > @return 0 on success
> > > */
> > >
> > > +int fl_programmer_shutdown(void)
> > > +{
> > >    return programmer_shutdown();
> > >
> > > +}
> > >
> > > +/* TODO: fl_programmer_capabilities()? */
> > >
> > > +/** @} */ /* end fl-prog */
> > >
> > > +/**
> > > @defgroup fl-flash Flash chips
> > > @{
> > > */
> > >
> > > +/**
> > > @brief Probe for a flash chip.
> > > Probes for a flash chip and returns a flash context, that can be used
> > > later with flash chip and @ref fl-ops "image operations", if exactly one
> > > matching chip is found.
> > > @param[out] flashctx Points to a pointer of type fl_flashctx_t that will
> > >                  be set if exactly one chip is found. *flashctx has
> > >                  to be freed by the caller with @ref
> > >
> > > fl_flash_release.
> > > @param[in] chip_name Name of a chip to probe for, or NULL to probe for
> > >                  all known chips.
> > > @return 0 on success,
> > >     3 if multiple chips were found,
> > >     2 if no chip was found,
> > >     or 1 on any other error.
> > > */
> >
> > Need a way to output multiple flash chips from here, it shouldnt be an
> > error, just a
> > "pick from these" dialog and continue with "OK", or something like that.
> > Maybe add int *chip_count as a parameter and the set pointer will
> > point to an array of flashctx with the int set to the count of found
> > chips.
> > (Hopefully I'm not saying totally obvious stuff here...)
> > > +int fl_flash_probe(fl_flashctx_t **const flashctx, const char *const
> > > chip_name)
> > > +{
> > >    int i, ret = 2;
> > >    fl_flashctx_t second_flashctx = { 0, };
> > >    chip_to_probe = chip_name; /* chip_to_probe is global in flashrom.c
> > >
> > > */
> > >    *flashctx = malloc(sizeof(**flashctx));
> > >    if (!*flashctx)
> > >            return 1;
> > >    memset(*flashctx, 0, sizeof(**flashctx));
> > >    /* LD_CHANGE_NOTE for (i = 0; i < registered_programmer_count; ++i)
> > >
> > > {
> > >     *
> > >     * Reason of change: registered_programmer_count does not exist, I
> > >
> > > assume that a proper one is
> > >     * now registered_master_count - I will check and confirm
> > >     */
> > >    for (i = 0; i < registered_master_count; ++i) {
> > >            int flash_idx = -1;
> > >            /* LD_CHANGE_NOTE if (!ret || (flash_idx =
> > >
> > > probe_flash(&registered_programmers[i], 0, *flashctx, 0)) != -1) {
> > >             *
> > >             * Reason of change: registered_programmers does not exist,
> > >
> > > I assume that a proper one is
> > >             * now registered_masters - I will check and confirm
> > >             */
> > >            if (!ret || (flash_idx =
> > >
> > > probe_flash(&registered_masters[i], 0, *flashctx, 0)) != -1) {
> > >                    ret = 0;
> > >                    /* We found one chip, now check that there is no
> > >
> > > second match. */
> > >                    /* LD_CHANGE_NOTE if
> > >
> > > (probe_flash(&registered_programmers[i], flash_idx + 1, &second_flashctx, 0)
> > > != -1) {
> > >                     *
> > >                     * Reason of change: registered_programmers does
> > >
> > > not exist, I assume that a proper one is
> > >                     * now registered_masters - I will check and
> > >
> > > confirm
> > >                     *
> > >                     */
> > >                    if (probe_flash(&registered_masters[i], flash_idx +
> > >
> > > 1, &second_flashctx, 0) != -1) {
> > >                            ret = 3;
> > >                            break;
> > >                    }
> > >            }
> > >    }
> > >    if (ret) {
> > >            free(*flashctx);
> > >            *flashctx = NULL;
> > >    }
> > >    return ret;
> > >
> > > +}
> > >
> > > +/**
> > > @brief Returns the size of the specified flash chip in bytes.
> > > @param flashctx The queried flash context.
> > > @return Size of flash chip in bytes.
> > > */
> > >
> > > +size_t fl_flash_getsize(const fl_flashctx_t *const flashctx)
> > > +{
> > >    return flashctx->chip->total_size << 10;
> > >
> > > +}
> > >
> > > +/** @private */
> > > +int erase_and_write_flash(struct flashctx *flash, uint8_t *oldcontents,
> > > uint8_t *newcontents);
> > > +/** @private */
> > > +/* LD_CHANGE_NOTE void emergency_help_message(void);
> > > Reason of change: This has been commented out as emergency_help_message
> > >
> > > is static
> >
> > Maybe have some other method to indicate that the UI should display an
> > emergency help message, maybe ret = 2. Same comment for the _write
> > version, but that already has ret values for these.
> > > */
> > >
> > > +/**
> > > @brief Erase the specified ROM chip.
> > > @param flashctx The context of the flash chip to erase.
> > > @return 0 on success.
> > > */
> > >
> > > +int fl_flash_erase(fl_flashctx_t *const flashctx)
> > > +{
> > >    const size_t flash_size = flashctx->chip->total_size * 1024;
> > >    int ret = 0;
> > >    uint8_t *const newcontents = malloc(flash_size);
> > >    if (!newcontents) {
> > >            msg_gerr("Out of memory!\n");
> > >            return 1;
> > >    }
> > >    uint8_t *const oldcontents = malloc(flash_size);
> > >    if (!oldcontents) {
> > >            msg_gerr("Out of memory!\n");
> > >            free(newcontents);
> > >            return 1;
> > >    }
> > >    if (flashctx->chip->unlock)
> > >            flashctx->chip->unlock(flashctx);
> > >    /* Assume worst case for old contents: All bits are 0. */
> > >    memset(oldcontents, 0x00, flash_size);
> > >    /* Assume best case for new contents: All bits should be 1. */
> > >    memset(newcontents, 0xff, flash_size);
> > >    /* Side effect of the assumptions above: Default write action is
> > >
> > > erase
> > >     * because newcontents looks like a completely erased chip, and
> > >     * oldcontents being completely 0x00 means we have to erase
> > >
> > > everything
> > >     * before we can write.
> > >     */
> > >    if (erase_and_write_flash(flashctx, oldcontents, newcontents)) {
> > >            /* FIXME: Do we really want the scary warning if erase
> > >
> > > failed?
> > >             * After all, after erase the chip is either blank or
> > >
> > > partially
> > >             * blank or it has the old contents. A blank chip won't
> > >
> > > boot,
> > >             * so if the user wanted erase and reboots afterwards, the
> > >
> > > user
> > >             * knows very well that booting won't work.
> > >             */
> > >            /* LD_CHANGE_NOTE emergency_help_message();
> > >             *
> > >             * Reason of change: This function is static
> > >             */
> > >            ret = 1;
> > >    }
> > >    free(oldcontents);
> > >    free(newcontents);
> > >    return ret;
> > >
> > > +}
> > >
> > > +/**
> > > @brief Free a flash context.
> > > @param flashctx Flash context to free.
> > > */
> > >
> > > +void fl_flash_release(fl_flashctx_t *const flashctx)
> > > +{
> > >    free(flashctx);
> > >
> > > +}
> > >
> > > +/** @} */ /* end fl-flash */
> > >
> > > +/**
> > > @defgroup fl-ops Operations
> > > @{
> > > */
> > >
> > > +/**
> > > @brief Read the current image from the specified ROM chip.
> > > @param flashctx The context of the flash chip.
> > > @param buffer Target buffer to write image to.
> > > @param buffer_len Size of target buffer in bytes.
> > > @return 0 on success,
> > >     2 if buffer_len is to short for the flash chip's contents,
> > >     or 1 on any other failure.
> > > */
> > >
> > > +int fl_image_read(fl_flashctx_t *const flashctx, void *const buffer, const
> > > size_t buffer_len)
> > > +{
> > >    const size_t flash_size = flashctx->chip->total_size * 1024;
> > >    int ret = 0;
> > >    if (flashctx->chip->unlock)
> > >            flashctx->chip->unlock(flashctx);
> > >    msg_cinfo("Reading flash... ");
> > >    if (flash_size > buffer_len) {
> > >            msg_cerr("Buffer to short for this flash chip (%u <
> > >
> > > %u).\n",
> > >                     (unsigned int)buffer_len, (unsigned
> > >
> > > int)flash_size);
> > >            ret = 2;
> > >            goto _out;
> > >    }
> > >    if (!flashctx->chip->read) {
> > >            msg_cerr("No read function available for this flash
> > >
> > > chip.\n");
> > >            ret = 1;
> > >            goto _out;
> > >    }
> > >    if (flashctx->chip->read(flashctx, buffer, 0, flash_size)) {
> > >            msg_cerr("Read operation failed!\n");
> > >            ret = 1;
> > >            goto _out;
> > >    }
> > >
> > > +_out:
> > >    msg_cinfo("%s.\n", ret ? "FAILED" : "done");
> > >    return ret;
> > >
> > > +}
> > >
> > > +/** @private */
> > > +/* LD_CHANGE_NOTE void nonfatal_help_message(void);
> > > Reason of change: This function is static
> > > */
> > >
> > > +/**
> > > @brief Write the specified image to the ROM chip.
> > > @param flashctx The context of the flash chip.
> > > @param buffer Source buffer to read image from.
> > > @param buffer_len Size of source buffer in bytes.
> > > @return 0 on success,
> > >     4 if buffer_len doesn't match the size of the flash chip,
> > >     3 if write was tried, but nothing has changed,
> > >     2 if write was tried, but flash contents changed,
> > >     or 1 on any other failure.
> > > */
> > >
> > > +int fl_image_write(fl_flashctx_t *const flashctx, void *const buffer, const
> > > size_t buffer_len)
> > > +{
> > >    const size_t flash_size = flashctx->chip->total_size * 1024;
> > >    int ret = 0;
> > >    if (buffer_len != flash_size) {
> > >            msg_cerr("Buffer size doesn't match size of flash chip (%u
> > >
> > > != %u)\n.",
> > >                     (unsigned int)buffer_len, (unsigned
> > >
> > > int)flash_size);
> > >            return 4;
> > >    }
> > >    uint8_t *const newcontents = buffer;
> > >    uint8_t *const oldcontents = malloc(flash_size);
> > >    if (!oldcontents) {
> > >            msg_gerr("Out of memory!\n");
> > >            return 1;
> > >    }
> > >    if (fl_image_read(flashctx, oldcontents, flash_size)) {
> > >            ret = 1;
> > >            goto _free_out;
> > >    }
> > >    /* LD_CHANGE_NOTE handle_romentries(flashctx, oldcontents,
> > >
> > > newcontents);
> > >     *
> > >     * Reason of change: This function does not exist. There is a need
> > >
> > > to investigate what was
> > >     * its purpose and how it can be replaced.
> > >     */
> > >    if (erase_and_write_flash(flashctx, oldcontents, newcontents)) {
> > >            msg_cerr("Uh oh. Erase/write failed. Checking if anything
> > >
> > > changed.\n");
> > >            if (!flashctx->chip->read(flashctx, newcontents, 0,
> > >
> > > flash_size)) {
> > >                    if (!memcmp(oldcontents, newcontents, flash_size))
> > >
> > > {
> > >                            msg_cinfo("Good. It seems nothing was
> > >
> > > changed.\n");
> > >                            /* LD_CHANGE_NOTE nonfatal_help_message();
> > >                             *
> > >                             * Reason of change: This function is
> > >
> > > static
> > >                             */
> > >                            ret = 3;
> > >                            goto _free_out;
> > >                    }
> > >            }
> > >            /* LD_CHANGE_NOTE emergency_help_message();
> > >             *
> > >             * Reason of change: This function is static
> > >             */
> > >            ret = 2;
> > >            goto _free_out;
> > >    }
> > >
> > > +_free_out:
> > >    free(oldcontents);
> > >    return ret;
> > >
> > > +}
> > >
> > > +/** @private */
> > > +/* LD_CHANGE_NOTE int compare_range(const uint8_t *wantbuf, const uint8_t
> > > *havebuf, unsigned int start, unsigned int len);
> > > Reason of change: This function is static
> >
> > For any of these "this is static"... then make it not static or figure
> > out a cleaner way of doing it, but yes i get this is an early patch.
> > > */
> > >
> > > +/**
> > > @brief Verify the ROM chip's contents with the specified image.
> > > @param flashctx The context of the flash chip.
> > > @param buffer Source buffer to verify with.
> > > @param buffer_len Size of source buffer in bytes.
> > > @return 0 on success,
> > >     2 if buffer_len doesn't match the size of the flash chip,
> > >     or 1 on any other failure.
> > > */
> > >
> > > +int fl_image_verify(fl_flashctx_t *const flashctx, void *const buffer,
> > > const size_t buffer_len)
> > > +{
> > >    const size_t flash_size = flashctx->chip->total_size * 1024;
> > >    int ret = 0;
> > >    if (buffer_len != flash_size) {
> > >            msg_cerr("Buffer size doesn't match size of flash chip (%u
> > >
> > > != %u)\n.",
> > >                     (unsigned int)buffer_len, (unsigned
> > >
> > > int)flash_size);
> > >            return 2;
> > >    }
> > >    /* LD_CHANGE_NOTE uint8_t *const newcontents = buffer; - used only
> > >
> > > in handle_romentries() function
> > >     *
> > >     * Reason of change: This pointer is used only in handle_romentries
> > >
> > > function, which has been
> > >     * commented out
> > >     */
> > >    uint8_t *const oldcontents = malloc(flash_size);
> > >    if (!oldcontents) {
> > >            msg_gerr("Out of memory!\n");
> > >            return 1;
> > >    }
> > >    if (fl_image_read(flashctx, oldcontents, flash_size)) {
> > >            ret = 1;
> > >            goto _free_out;
> > >    }
> > >    /* LD_CHANGE_NOTE handle_romentries(flashctx, oldcontents,
> > >
> > > newcontents);
> > >     *
> > >     * Reason of change: This function does not exist
> > >     */
> > >    msg_cinfo("Verifying flash... ");
> > >    /* LD_CHANGE_NOTE ret = compare_range(newcontents, oldcontents, 0,
> > >
> > > flash_size);
> > >     *
> > >     * Reason of change: This function is static. For now replaced with
> > >
> > > ret = 1 as ret must be used
> > >     */
> > >    ret = 1;
> > >    if (!ret)
> > >            msg_cinfo("VERIFIED.\n");
> > >
> > > +_free_out:
> > >    free(oldcontents);
> > >    return ret;
> > >
> > > +}
> > >
> > > +/** @} */ /* end fl-ops */
> > > Index: libflashrom.h
> > > ===================================================================
> > > --- libflashrom.h    (revision 0)
> > > +++ libflashrom.h    (working copy)
> > > @@ -0,0 +1,97 @@
> > > +/*
> > > This file is part of the flashrom project.
> > > Copyright (C) 2012 secunet Security Networks AG
> > > (Written by Nico Huber  for secunet)
> > > This program is free software; you can redistribute it and/or modify
> > > it under the terms of the GNU General Public License as published by
> > > the Free Software Foundation; either version 2 of the License, or
> > > (at your option) any later version.
> > > This program is distributed in the hope that it will be useful,
> > > but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > GNU General Public License for more details.
> > > You should have received a copy of the GNU General Public License
> > > along with this program; if not, write to the Free Software
> > > Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301
> > >
> > > USA
> > > */
> > >
> > > +#ifndef __LIBFLASHROM_H__
> > > +#define __LIBFLASHROM_H__ 1
> > >
> > > +#include <stdarg.h>
> > >
> > > +int fl_init(int perform_selfcheck);
> > > +int fl_shutdown(void);
> > > +/** @ingroup fl-general */
> > > +typedef enum { /* This has to match enum msglevel. */
> >
> > Maybe you could just bring enum msglevel here from flash.h...
> > or have some other header for this printing stuff thats
> > used by both libflashrom.h and other parts of flashrom.
> > >    FL_MSG_ERROR    = 0,
> > >    FL_MSG_INFO    = 1,
> > >    FL_MSG_DEBUG    = 2,
> > >    FL_MSG_DEBUG2    = 3,
> > >    FL_MSG_SPEW    = 4,
> > >
> > > +} fl_log_level_t;
> > > +/** @ingroup fl-general */
> > > +typedef int(fl_log_callback_t)(fl_log_level_t, const char *format,
> > > va_list);
> > > +void fl_set_log_callback(fl_log_callback_t *);
> > >
> > > +typedef enum {
> > >    FL_TESTED_OK  = 0,
> > >    FL_TESTED_NT  = 1,
> > >    FL_TESTED_BAD = 2,
> > >    FL_TESTED_DEP = 3,
> > >    FL_TESTED_NA  = 4,
> > >
> > > +} fl_test_state;
> > >
> > > +typedef struct {
> > >    const char *vendor;
> > >    const char *name;
> > >    unsigned int total_size;
> > >    struct fl_tested {
> > >            fl_test_state probe;
> > >            fl_test_state read;
> > >            fl_test_state erase;
> > >            fl_test_state write;
> > >    } tested;
> > >
> > > +} fl_flashchip_info_t;
> > >
> > > +typedef struct {
> > >    const char *vendor;
> > >    const char *name;
> > >    fl_test_state working;
> > >
> > > +} fl_board_info_t;
> > >
> > > +typedef struct {
> > >    const char *vendor;
> > >    const char *chipset;
> > >    fl_test_state status;
> > >
> > > +} fl_chipset_info_t;
> > >
> > > +int fl_supported_programmers(const char **supported_programmers);
> > > +int fl_supported_flash_chips(fl_flashchip_info_t *fchips);
> > > +int fl_supported_boards(fl_board_info_t *boards);
> > > +int fl_supported_chipsets(fl_chipset_info_t *chipsets);
> > > +int fl_supported_programmers_number();
> > > +int fl_supported_flash_chips_number();
> > > +int fl_supported_boards_number();
> > > +int fl_supported_chipsets_number();
> > >
> > > +int fl_programmer_init(const char *prog_name, const char *prog_params);
> > > +int fl_programmer_shutdown(void);
> > >
> > > +struct flashctx;
> > > +typedef struct flashctx fl_flashctx_t;
> > > +int fl_flash_probe(fl_flashctx_t **, const char *chip_name);
> > > +size_t fl_flash_getsize(const fl_flashctx_t *);
> > > +int fl_flash_erase(fl_flashctx_t *);
> > > +void fl_flash_release(fl_flashctx_t *);
> > >
> > > +int fl_image_read(fl_flashctx_t *, void *buffer, size_t buffer_len);
> > > +int fl_image_write(fl_flashctx_t *, void *buffer, size_t buffer_len);
> > > +int fl_image_verify(fl_flashctx_t *, void *buffer, size_t buffer_len);
> > >
> > > +#endif                /* !__LIBFLASHROM_H__ */
> > >
> > > flashrom mailing list
> > > [e-mail]
> > > http://www.flashrom.org/mailman/listinfo/flashrom
> >
> > flashrom mailing list
> > [e-mail]
> > http://www.flashrom.org/mailman/listinfo/flashrom