Change of behavior of device tag

[Hamish Guthrie]

I have noticed a change in behavior of the device tag since commit aef3186196.

If I use an existing sw-description file from a working previous version of swupdate (using syntax as shown by examples in the documentation), I get failures. The problem is that the realpath function used in get_mtd_from_device function expects an absolute path. If I use device = "mtd4" in a partitions or images section, swupdate fails. If I replace those instances with an absolute path it works.

I can see the reason for why this patch was submitted, however, it breaks the documented functionality of swupdate and now the documentation does not represent the new behavior of the software.

Regards
Hamish

[Stefano Babic]

Hi Hamish,

ok - do you send a patch for documentation ?

Best regards,
Stefano Babic


--
Meet DENX at the Embedded World Trade Show
14 Mar - 16 Mar 2017, Nuremberg Trade Fair Centre, Hall 4, Booth 581
--
=====================================================================
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sba...@denx.de
=====================================================================

[Hamish Guthrie]

Hi Stefano,

> ok - do you send a patch for documentation ?

Are you sure you want all of your existing users of swupdate to change their existing sw-description files because a single patch broke the previously described behavior?

I think it would be better if the code was modified to add back the currently documented functionality - I can add this, but the author who wrote the previous patch should not have broken documented functionality!

Would you like me to fix that code and submit a patch for it?

Hamish

[Stefano Babic]

On 09/03/2017 11:35, Hamish Guthrie wrote:
> Hi Stefano,
>
>> ok - do you send a patch for documentation ?
>
> Are you sure you want all of your existing users of swupdate to change their existing sw-description files because a single patch broke the previously described behavior?

Currently, documentation is not compliant with the code. I am sure that
both code and documentation must be in sync.

Anyway, I could not ensure that the syntax in sw-description will not be
never changed, I just agree that is better to let it compatible.

>
> I think it would be better if the code was modified to add back the currently documented functionality - I can add this, but the author who wrote the previous patch should not have broken documented functionality!
>
> Would you like me to fix that code and submit a patch for it?

Please send any patch to the ML - any change should be discussed here
and evaluated before merging. I have added in CC Alexander as he is the
author of the patch, so that we all have an overview of the issue.

Thanks,

[Alexander Kuzmich]

From: Alexander Kuzmich <al...@hms.se>

Partition naming like "mtd4" broken by aef3186196 should
work again
---
corelib/mtd-interface.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/corelib/mtd-interface.c b/corelib/mtd-interface.c
index 2fa7955..af50ec4 100644
--- a/corelib/mtd-interface.c
+++ b/corelib/mtd-interface.c
@@ -26,6 +26,7 @@
#include <sys/mount.h>
#include <unistd.h>
#include <errno.h>
+#include <limits.h>
#include "bsdqueue.h"
#include "util.h"
#include "flash.h"
@@ -57,8 +58,17 @@ int get_mtd_from_device(char *s) {
return -1;

real_s = realpath(s, NULL);
- if (real_s == NULL)
- return -1;
+ if (real_s == NULL) {
+ char tmp_s[PATH_MAX] = {0};
+
+ if (! strncmp(s, "/dev/", 5))
+ return -1;
+
+ snprintf(tmp_s, sizeof(tmp_s), "/dev/%s", s);
+ real_s = realpath(tmp_s, NULL);
+ if (real_s == NULL)
+ return -1;
+ }

TRACE("mtd name [%s] resolved to [%s]\n", s, real_s);
ret = sscanf(real_s, "mtd%d", &mtdnum);
--
2.7.4

[Hamish Guthrie]

Hi Alexander,

> Partition naming like "mtd4" broken by aef3186196 should work again

I sent a patch just a few minutes before receiving this patch, but I sent it separately from this thread because my work email is Outlook so I use git send-mail to send patches.

You may want to look at my implementation, which does the same as yours but in a simpler way perhaps?

Hamish

[Stefano Babic]

On 09/03/2017 15:20, Hamish Guthrie wrote:
> Hi Alexander,
>
>> Your implementation allows to use device ="/dev/mtd-dtb2", "mtd5"
>> and "/dev/mtd5"
>>
>> My in addition supports device ="mtd-dtb2"
>>
>
> You are correct, I had not thought about your use-case with just the
> devicenode part of the symlink - I had assumed that in your
> implementation you would use an absolute path, but your
> implementation indeed should do the trick for all existing users as
> well as your use-case, so Stefano, I would be quite happy if you
> accept Alexander's patch for this.
>

Ok, thanks both. Anyway, there is still one point missing that was the
origin of all: documentation must be fixe and it must be explained that
not just a "mtd" number is possible, but also something else that is
automatically resolved to the device number by SWUpdate. So a
"/dev/update" link that points to the correct device number /dev/mtdX.

Alexander, can you extend your patch to fix the documentation, too ?

Thanks,
Stefano

[Alexander Kuzmich]

From: Alexander Kuzmich <al...@hms.se>

Partition naming like "mtd4" broken by aef3186196 should
work again

---
corelib/mtd-interface.c | 14 ++++++++++++--
doc/source/sw-description.rst | 10 ++++++++--
2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/doc/source/sw-description.rst b/doc/source/sw-description.rst
index cdb5bd4..6af71de 100644
--- a/doc/source/sw-description.rst
+++ b/doc/source/sw-description.rst
@@ -569,8 +569,14 @@ There are 4 main sections inside sw-description:
| ubipartition| string | images | Just if type = "ubivol". Volume to be |
| | | | created or adjusted with a new size |
+-------------+----------+------------+---------------------------------------+
- | device | string | images | devicenode as found in /dev. Usage |
- | | | files | depends on handler. |
+ | device | string | images | devicenode as found in /dev or a |
+ | | | files | symlink to it. Can be specified as |
+ | | | | absolute path or a name in /dev folder|
+ | | | | For example if /dev/mtd-dtb is a link |
+ | | | | to /dev/mtd3 "mtd3", "mtd-dtb", |
+ | | | | "/dev/mtd3" and "/dev/mtd-dtb" are |
+ | | | | valid names. |
+ | | | | Usage depends on handler. |
| | | | For files, it indicates on which |
| | | | device the "filesystem" must be |
| | | | mounted. If not specified, the current|
--
2.7.4

[Stefano Babic]

It looks fine, thanks !

Acked-by: Stefano Babic < sba...@denx.de>

Best regards,
Stefano Babic