Re: [PATCH v2] libmtd: add helper funcs for getting regioninfo and locked info

[Mike Frysinger <vapier@gentoo.org>]

On Wed, Jun 8, 2011 at 08:27, Artem Bityutskiy wrote:
> On Wed, 2011-06-08 at 14:52 +0300, Artem Bityutskiy wrote:
>> On Tue, 2011-06-07 at 11:28 -0400, Mike Frysinger wrote:
>> > This extends the libmtd with the helper functions:
>> >     mtd_regioninfo: interface to MEMGETREGIONINFO
>> >     mtd_islocked: interface to MEMISLOCKED
>> >
>> > Users of these functions will follow shortly ...
>> >
>> > Signed-off-by: Mike Frysinger <vapier@gentoo.org>
>>
>> Pushed with a small tweak, thanks!
>>
>> > +int mtd_islocked(const struct mtd_dev_info *mtd, int fd, int eb)
>> > +{
>> > +   int ret;
>> > +   erase_info_t ei;
>> > +
>> > +   ei.start = eb * mtd->eb_size;
>> > +   ei.length = mtd->eb_size;
>> > +
>> > +   ret = ioctl(fd, MEMISLOCKED, &ei);
>> > +   if (ret < 0)
>> > +           return mtd_ioctl_error(mtd, eb, "MEMISLOCKED");
>> > +
>>
>> I've removed this error message - if we fail, better return an error
>> code silently. At least your next patch is built a way that it will keep
>> iterating and executing this function.
>>
>> We might as well print an error message if (!ENOTTY && !ENOTSUPP), but I
>> did not do this. I expect you to send a correction patch if you do not
>> like this :-)
>
> I've just pushed a correction patch on top of this:
>
> From 92a06994b7c3f146cbdb770b30780e32f021118f Mon Sep 17 00:00:00 2001
> From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> Date: Wed, 8 Jun 2011 15:30:14 +0300
> Subject: [PATCH] libmtd: improve mtd_islocked interface
>
> This patch first of all, re-names 'mtd_islocked()' into 'mtd_is_locked()' since
> this seems to be the name Mike wanted, and it looks a bit nicer.
>
> This patch also makes 'mtd_is_locked()' print an error message if it fails. I'm
> not sure if it is good idea for a library to do so, but all functions do this,
> so it certainly _not_ a good idea to be inconsistent.
>
> However, for the special case, when the the "is locked" ioctl is not supported
> or is not valid for this device, we do not print an error message and return
> ENOTSUPP error code.
>
> Thus, the user can distinguish between real errors and non-fatal "not
> supported" cases.
>
> Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> ---
>  include/libmtd.h |    8 +++++---
>  lib/libmtd.c     |   13 +++++++++++--
>  2 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/include/libmtd.h b/include/libmtd.h
> index 7275246..9efccbc 100644
> --- a/include/libmtd.h
> +++ b/include/libmtd.h
> @@ -190,16 +190,18 @@ int mtd_erase(libmtd_t desc, const struct mtd_dev_info *mtd, int fd, int eb);
>  int mtd_regioninfo(int fd, int regidx, struct region_info_user *reginfo);
>
>  /**
> - * mtd_islocked - see if the specified eraseblock is locked.
> + * mtd_is_locked - see if the specified eraseblock is locked.
>  * @mtd: MTD device description object
>  * @fd: MTD device node file descriptor
>  * @eb: eraseblock to check
>  *
>  * This function checks to see if eraseblock @eb of MTD device described
>  * by @fd is locked. Returns %0 if it is unlocked, %1 if it is locked, and
> - * %-1 in case of failure.
> + * %-1 in case of failure. If the ioctl is not supported (support was added in
> + * Linux kernel 2.6.36) or this particular device does not support it, errno is
> + * set to @ENOTSUPP.
>  */
> -int mtd_islocked(const struct mtd_dev_info *mtd, int fd, int eb);
> +int mtd_is_locked(const struct mtd_dev_info *mtd, int fd, int eb);
>
>  /**
>  * mtd_torture - torture an eraseblock.
> diff --git a/lib/libmtd.c b/lib/libmtd.c
> index 2573cc7..c34874e 100644
> --- a/lib/libmtd.c
> +++ b/lib/libmtd.c
> @@ -900,14 +900,23 @@ int mtd_regioninfo(int fd, int regidx, struct region_info_user *reginfo)
>        return 0;
>  }
>
> -int mtd_islocked(const struct mtd_dev_info *mtd, int fd, int eb)
> +int mtd_is_locked(const struct mtd_dev_info *mtd, int fd, int eb)
>  {
> +       int ret;
>        erase_info_t ei;
>
>        ei.start = eb * mtd->eb_size;
>        ei.length = mtd->eb_size;
>
> -       return ioctl(fd, MEMISLOCKED, &ei);
> +       ret = ioctl(fd, MEMISLOCKED, &ei);
> +       if (ret < 0) {
> +               if (errno != ENOTTY && errno != EOPNOTSUPP)
> +                       return mtd_ioctl_error(mtd, eb, "MEMISLOCKED");
> +               else
> +                       errno = EOPNOTSUPP;
> +       }
> +
> +       return ret;
>  }

if you remove the new "islocked_supported" flag from my v3, this is
almost what i had already.  so i'll drop any further work here (and
focus on `mtdinfo`) since we've come to feature parity.
-mike