| Message ID | 20210105183407.25126-1-wesley.lindauer@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | raw handler: Allow symlink traversal in blkprotect | expand |
Hi Wesley, On 05.01.21 19:34, wesley.lindauer@gmail.com wrote: > From: Wes Lindauer <wesley.lindauer@gmail.com> > > If the device given in the sw-description is a symlink, this will > traverse the symlink to find if the underlying block device has a > force_ro flag. > > This is useful when using device symlinks with udev rules. i.e. > /dev/disk/by-label/boot -> /dev/mmcblk0boot0. These udev symlinks can > be helpful when using an A/B partitioning layout. > > Signed-off-by: Wes Lindauer <wesley.lindauer@gmail.com> > --- > handlers/raw_handler.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/handlers/raw_handler.c b/handlers/raw_handler.c > index 489919d..b14642e 100644 > --- a/handlers/raw_handler.c > +++ b/handlers/raw_handler.c > @@ -38,6 +38,7 @@ void raw_copyimage_handler(void); > */ > static int blkprotect(struct img_type *img, bool on) > { > + char abs_path[PATH_MAX]; > const char c_sys_path[] = "/sys/class/block/%s/force_ro"; > const char c_unprot_char = '0'; > const char c_prot_char = '1'; > @@ -53,7 +54,7 @@ static int blkprotect(struct img_type *img, bool on) > return ret; > } > > - if (lstat(img->device, &sb) == -1) { > + if (stat(img->device, &sb) == -1) { > TRACE("stat for device %s failed: %s", img->device, strerror(errno)); > return ret; > } > @@ -61,7 +62,13 @@ static int blkprotect(struct img_type *img, bool on) > return ret; > } > > - ret_int = asprintf(&sysfs_path, c_sys_path, img->device + 5); // remove "/dev/" from device path > + // If given, traverse symlink and convert to absolute path > + if (realpath(img->device, abs_path) == NULL) { > + ret = -errno; > + goto blkprotect_out; > + } > + > + ret_int = asprintf(&sysfs_path, c_sys_path, abs_path + 5); // remove "/dev/" from device path > if(ret_int < 0) { > ret = -ENOMEM; > goto blkprotect_out; > Patch is ok, but code is C, please remove // comments and use C-like comments surrounded by /*..*/. Best regards, Stefano Babic
I used that style for consistency. This same function already has three other lines with // style comments. Changing my comment line will make it inconsistent with the rest of the comments in this function. Just confirming that is what you really want me to do? Or do you want me to change all comment lines in the function to be /* */ style? On Tue, Jan 5, 2021 at 2:37 PM Stefano Babic <sbabic@denx.de> wrote: > Hi Wesley, > > On 05.01.21 19:34, wesley.lindauer@gmail.com wrote: > > From: Wes Lindauer <wesley.lindauer@gmail.com> > > > > If the device given in the sw-description is a symlink, this will > > traverse the symlink to find if the underlying block device has a > > force_ro flag. > > > > This is useful when using device symlinks with udev rules. i.e. > > /dev/disk/by-label/boot -> /dev/mmcblk0boot0. These udev symlinks can > > be helpful when using an A/B partitioning layout. > > > > Signed-off-by: Wes Lindauer <wesley.lindauer@gmail.com> > > --- > > handlers/raw_handler.c | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/handlers/raw_handler.c b/handlers/raw_handler.c > > index 489919d..b14642e 100644 > > --- a/handlers/raw_handler.c > > +++ b/handlers/raw_handler.c > > @@ -38,6 +38,7 @@ void raw_copyimage_handler(void); > > */ > > static int blkprotect(struct img_type *img, bool on) > > { > > + char abs_path[PATH_MAX]; > > const char c_sys_path[] = "/sys/class/block/%s/force_ro"; > > const char c_unprot_char = '0'; > > const char c_prot_char = '1'; > > @@ -53,7 +54,7 @@ static int blkprotect(struct img_type *img, bool on) > > return ret; > > } > > > > - if (lstat(img->device, &sb) == -1) { > > + if (stat(img->device, &sb) == -1) { > > TRACE("stat for device %s failed: %s", img->device, > strerror(errno)); > > return ret; > > } > > @@ -61,7 +62,13 @@ static int blkprotect(struct img_type *img, bool on) > > return ret; > > } > > > > - ret_int = asprintf(&sysfs_path, c_sys_path, img->device + 5); // > remove "/dev/" from device path > > + // If given, traverse symlink and convert to absolute path > > + if (realpath(img->device, abs_path) == NULL) { > > + ret = -errno; > > + goto blkprotect_out; > > + } > > + > > + ret_int = asprintf(&sysfs_path, c_sys_path, abs_path + 5); // > remove "/dev/" from device path > > if(ret_int < 0) { > > ret = -ENOMEM; > > goto blkprotect_out; > > > > Patch is ok, but code is C, please remove // comments and use C-like > comments surrounded by /*..*/. > > Best regards, > Stefano Babic > > -- > ===================================================================== > 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: sbabic@denx.de > ===================================================================== >
Hi Wes, On 05.01.21 21:55, Wes Lindauer wrote: > I used that style for consistency. This same function already has three > other lines with // style comments. I was not careful enough when I merge that patch. > Changing my comment line will make > it inconsistent with the rest of the comments in this function. Just > confirming that is what you really want me to do? Or do you want me to > change all comment lines in the function to be /* */ style? Yes, please - you can do it as follow-up patch, of course. Best regards, Stefano Babic > > On Tue, Jan 5, 2021 at 2:37 PM Stefano Babic <sbabic@denx.de > <mailto:sbabic@denx.de>> wrote: > > Hi Wesley, > > On 05.01.21 19:34, wesley.lindauer@gmail.com > <mailto:wesley.lindauer@gmail.com> wrote: > > From: Wes Lindauer <wesley.lindauer@gmail.com > <mailto:wesley.lindauer@gmail.com>> > > > > If the device given in the sw-description is a symlink, this will > > traverse the symlink to find if the underlying block device has a > > force_ro flag. > > > > This is useful when using device symlinks with udev rules. i.e. > > /dev/disk/by-label/boot -> /dev/mmcblk0boot0. These udev symlinks can > > be helpful when using an A/B partitioning layout. > > > > Signed-off-by: Wes Lindauer <wesley.lindauer@gmail.com > <mailto:wesley.lindauer@gmail.com>> > > --- > > handlers/raw_handler.c | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/handlers/raw_handler.c b/handlers/raw_handler.c > > index 489919d..b14642e 100644 > > --- a/handlers/raw_handler.c > > +++ b/handlers/raw_handler.c > > @@ -38,6 +38,7 @@ void raw_copyimage_handler(void); > > */ > > static int blkprotect(struct img_type *img, bool on) > > { > > + char abs_path[PATH_MAX]; > > const char c_sys_path[] = "/sys/class/block/%s/force_ro"; > > const char c_unprot_char = '0'; > > const char c_prot_char = '1'; > > @@ -53,7 +54,7 @@ static int blkprotect(struct img_type *img, > bool on) > > return ret; > > } > > > > - if (lstat(img->device, &sb) == -1) { > > + if (stat(img->device, &sb) == -1) { > > TRACE("stat for device %s failed: %s", img->device, > strerror(errno)); > > return ret; > > } > > @@ -61,7 +62,13 @@ static int blkprotect(struct img_type *img, > bool on) > > return ret; > > } > > > > - ret_int = asprintf(&sysfs_path, c_sys_path, img->device + > 5); // remove "/dev/" from device path > > + // If given, traverse symlink and convert to absolute path > > + if (realpath(img->device, abs_path) == NULL) { > > + ret = -errno; > > + goto blkprotect_out; > > + } > > + > > + ret_int = asprintf(&sysfs_path, c_sys_path, abs_path + 5); > // remove "/dev/" from device path > > if(ret_int < 0) { > > ret = -ENOMEM; > > goto blkprotect_out; > > > > Patch is ok, but code is C, please remove // comments and use C-like > comments surrounded by /*..*/. > > Best regards, > Stefano Babic > > -- > ===================================================================== > 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: > sbabic@denx.de <mailto:sbabic@denx.de> > ===================================================================== > > -- > You received this message because you are subscribed to the Google > Groups "swupdate" group. > To unsubscribe from this group and stop receiving emails from it, send > an email to swupdate+unsubscribe@googlegroups.com > <mailto:swupdate+unsubscribe@googlegroups.com>. > To view this discussion on the web visit > https://groups.google.com/d/msgid/swupdate/CALBQxSu830FQwBr_dLoUHx0DSyX4SHhkOiTjeMe3amqnV0GFhA%40mail.gmail.com > <https://groups.google.com/d/msgid/swupdate/CALBQxSu830FQwBr_dLoUHx0DSyX4SHhkOiTjeMe3amqnV0GFhA%40mail.gmail.com?utm_medium=email&utm_source=footer>.
diff --git a/handlers/raw_handler.c b/handlers/raw_handler.c index 489919d..b14642e 100644 --- a/handlers/raw_handler.c +++ b/handlers/raw_handler.c @@ -38,6 +38,7 @@ void raw_copyimage_handler(void); */ static int blkprotect(struct img_type *img, bool on) { + char abs_path[PATH_MAX]; const char c_sys_path[] = "/sys/class/block/%s/force_ro"; const char c_unprot_char = '0'; const char c_prot_char = '1'; @@ -53,7 +54,7 @@ static int blkprotect(struct img_type *img, bool on) return ret; } - if (lstat(img->device, &sb) == -1) { + if (stat(img->device, &sb) == -1) { TRACE("stat for device %s failed: %s", img->device, strerror(errno)); return ret; } @@ -61,7 +62,13 @@ static int blkprotect(struct img_type *img, bool on) return ret; } - ret_int = asprintf(&sysfs_path, c_sys_path, img->device + 5); // remove "/dev/" from device path + // If given, traverse symlink and convert to absolute path + if (realpath(img->device, abs_path) == NULL) { + ret = -errno; + goto blkprotect_out; + } + + ret_int = asprintf(&sysfs_path, c_sys_path, abs_path + 5); // remove "/dev/" from device path if(ret_int < 0) { ret = -ENOMEM; goto blkprotect_out;