| Message ID | 20220308125150.231149-1-gary.bisson@boundarydevices.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | uboot_env: fix fileprotect for Android | expand |
Hi Gary, On 08.03.22 13:51, Gary Bisson wrote: > - Android ueventd daemon puts the mmcblk devices under /dev/block/ [1] > - Update fileprotect function to take the "/dev/block/" name offset into > account > > [1] > https://android.googlesource.com/platform/system/core/+/refs/heads/android11-gsi/init/README.ueventd.md#dev > > Signed-off-by: Gary Bisson <gary.bisson@boundarydevices.com> > --- > src/uboot_env.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/src/uboot_env.c b/src/uboot_env.c > index 6fb6c69..76e2619 100644 > --- a/src/uboot_env.c > +++ b/src/uboot_env.c > @@ -597,7 +597,9 @@ static int fileprotect(struct uboot_flash_env *dev, bool on) > int fd_force_ro; > > // Devices without ro flag at /sys/class/block/mmcblk?boot?/force_ro are ignored > - if (strncmp("/dev/", dev->devname, 5) == 0) { > + if (strncmp("/dev/block/", dev->devname, 11) == 0) { > + devfile = dev->devname + 11; > + } else if (strncmp("/dev/", dev->devname, 5) == 0) { I know there shouln't be issues, but can we revert the priority, that is first is tested for embedded linux and then for Android ? It avoids to have bad thoughts for possible regression issues... > - if (strncmp("/dev/", dev->devname, 5) == 0) { > + if (strncmp("/dev/", dev->devname, 5) == 0) { > + devfile = dev->devname + 11; > + } else if (strncmp("/dev/block/", dev->devname, 11) == 0) { Best regards, Stefano
Hi Stefano, On Thu, Mar 10, 2022 at 11:16:31AM +0100, Stefano Babic wrote: > Hi Gary, > > On 08.03.22 13:51, Gary Bisson wrote: > > - Android ueventd daemon puts the mmcblk devices under /dev/block/ [1] > > - Update fileprotect function to take the "/dev/block/" name offset into > > account > > > > [1] > > https://android.googlesource.com/platform/system/core/+/refs/heads/android11-gsi/init/README.ueventd.md#dev > > > > Signed-off-by: Gary Bisson <gary.bisson@boundarydevices.com> > > --- > > src/uboot_env.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/src/uboot_env.c b/src/uboot_env.c > > index 6fb6c69..76e2619 100644 > > --- a/src/uboot_env.c > > +++ b/src/uboot_env.c > > @@ -597,7 +597,9 @@ static int fileprotect(struct uboot_flash_env *dev, bool on) > > int fd_force_ro; > > // Devices without ro flag at /sys/class/block/mmcblk?boot?/force_ro are ignored > > - if (strncmp("/dev/", dev->devname, 5) == 0) { > > + if (strncmp("/dev/block/", dev->devname, 11) == 0) { > > + devfile = dev->devname + 11; > > + } else if (strncmp("/dev/", dev->devname, 5) == 0) { > > I know there shouln't be issues, but can we revert the priority, that is > first is tested for embedded linux and then for Android ? It avoids to have > bad thoughts for possible regression issues... > > > - if (strncmp("/dev/", dev->devname, 5) == 0) { > > + if (strncmp("/dev/", dev->devname, 5) == 0) { > > + devfile = dev->devname + 11; > > + } else if (strncmp("/dev/block/", dev->devname, 11) == 0) { Well no, that's the issue, because if you compare against "/dev/" it will match at the first test and therefore devfile will be "block/mmcblkX" which is what happens currently. So we have to compare against the longest string first. Another option is to test within the test but that looked more bloated: if (strncmp("/dev/", dev->devname, 5) == 0) { if (strncmp("/dev/block/", dev->devname, 11) == 0) devfile = dev->devname + 11; else devfile = dev->devname + 5; } Or even this to change the less code possible: if (strncmp("/dev/", dev->devname, 5) == 0) { devfile = dev->devname + 5; if (strncmp("/dev/block/", dev->devname, 11) == 0) devfile = dev->devname + 11; } Regards, Gary
Hi Gary, On 10.03.22 14:00, Gary Bisson wrote: > Hi Stefano, > > On Thu, Mar 10, 2022 at 11:16:31AM +0100, Stefano Babic wrote: >> Hi Gary, >> >> On 08.03.22 13:51, Gary Bisson wrote: >>> - Android ueventd daemon puts the mmcblk devices under /dev/block/ [1] >>> - Update fileprotect function to take the "/dev/block/" name offset into >>> account >>> >>> [1] >>> https://android.googlesource.com/platform/system/core/+/refs/heads/android11-gsi/init/README.ueventd.md#dev >>> >>> Signed-off-by: Gary Bisson <gary.bisson@boundarydevices.com> >>> --- >>> src/uboot_env.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/src/uboot_env.c b/src/uboot_env.c >>> index 6fb6c69..76e2619 100644 >>> --- a/src/uboot_env.c >>> +++ b/src/uboot_env.c >>> @@ -597,7 +597,9 @@ static int fileprotect(struct uboot_flash_env *dev, bool on) >>> int fd_force_ro; >>> // Devices without ro flag at /sys/class/block/mmcblk?boot?/force_ro are ignored >>> - if (strncmp("/dev/", dev->devname, 5) == 0) { >>> + if (strncmp("/dev/block/", dev->devname, 11) == 0) { >>> + devfile = dev->devname + 11; >>> + } else if (strncmp("/dev/", dev->devname, 5) == 0) { >> >> I know there shouln't be issues, but can we revert the priority, that is >> first is tested for embedded linux and then for Android ? It avoids to have >> bad thoughts for possible regression issues... >> >>> - if (strncmp("/dev/", dev->devname, 5) == 0) { >>> + if (strncmp("/dev/", dev->devname, 5) == 0) { >>> + devfile = dev->devname + 11; >>> + } else if (strncmp("/dev/block/", dev->devname, 11) == 0) { > > Well no, that's the issue, because if you compare against "/dev/" it > will match at the first test and therefore devfile will be > "block/mmcblkX" which is what happens currently. Oh yeah, I guess I was annoyed that Android always wants to find the own way, that I have not careful read the patc. In principle, there should not be any problem, so I will merge as it is. > > So we have to compare against the longest string first. > Another option is to test within the test but that looked more bloated: Right, we won't have it ;-) > if (strncmp("/dev/", dev->devname, 5) == 0) { > if (strncmp("/dev/block/", dev->devname, 11) == 0) > devfile = dev->devname + 11; > else > devfile = dev->devname + 5; > } > Or even this to change the less code possible: > if (strncmp("/dev/", dev->devname, 5) == 0) { > devfile = dev->devname + 5; > if (strncmp("/dev/block/", dev->devname, 11) == 0) > devfile = dev->devname + 11; > } > Best regards, Stefano > Regards, > Gary >
diff --git a/src/uboot_env.c b/src/uboot_env.c index 6fb6c69..76e2619 100644 --- a/src/uboot_env.c +++ b/src/uboot_env.c @@ -597,7 +597,9 @@ static int fileprotect(struct uboot_flash_env *dev, bool on) int fd_force_ro; // Devices without ro flag at /sys/class/block/mmcblk?boot?/force_ro are ignored - if (strncmp("/dev/", dev->devname, 5) == 0) { + if (strncmp("/dev/block/", dev->devname, 11) == 0) { + devfile = dev->devname + 11; + } else if (strncmp("/dev/", dev->devname, 5) == 0) { devfile = dev->devname + 5; } else { return ret;
- Android ueventd daemon puts the mmcblk devices under /dev/block/ [1] - Update fileprotect function to take the "/dev/block/" name offset into account [1] https://android.googlesource.com/platform/system/core/+/refs/heads/android11-gsi/init/README.ueventd.md#dev Signed-off-by: Gary Bisson <gary.bisson@boundarydevices.com> --- src/uboot_env.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)