| Message ID | 20210311114104.71593-1-sbabic@denx.de |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] util: add function to retrieve root device | expand |
Hi Stefano, > get_find_root() is a utility function that returns the device mounted as > rootfs. > > Signed-off-by: Stefano Babic <sbabic@denx.de> > --- > core/util.c | 35 +++++++++++++++++++++++++++++++++++ > include/util.h | 1 + > 2 files changed, 36 insertions(+) > > diff --git a/core/util.c b/core/util.c > index 2025276..7c82c2a 100644 > --- a/core/util.c > +++ b/core/util.c > @@ -842,3 +842,38 @@ size_t snescape(char *dst, size_t n, const char *src) > > return len; > } > + > +/* > + * This returns the device name where rootfs is mounted > + */ > +char *get_root_device(void) > +{ > + struct stat info; > + FILE *fp; > + char *devname = NULL; > + unsigned long major, minor, nblocks; > + char buf[256]; > + int ret; > + > + if (stat("/", &info) < 0) > + return NULL; > + > + fp = fopen("/proc/partitions", "r"); > + if (!fp) > + return NULL; > + > + while (fgets(buf, sizeof(buf), fp)) { > + ret = sscanf(buf, "%ld %ld %ld %ms", > + &major, &minor, &nblocks, &devname); > + if (ret != 4) > + continue; > + if ((major == info.st_dev / 256) && (minor == info.st_dev % 256)) { > + fclose(fp); > + return devname; > + } > + free(devname); > + } > + > + fclose(fp); > + return NULL; > +} > diff --git a/include/util.h b/include/util.h > index 2987203..8447ab6 100644 > --- a/include/util.h > +++ b/include/util.h > @@ -229,6 +229,7 @@ int set_aes_ivt(const char *ivt); > int get_install_info(sourcetype *source, char *buf, size_t len); > void get_install_swset(char *buf, size_t len); > void get_install_running_mode(char *buf, size_t len); > +char *get_root_device(void); > > /* Setting global information */ > void set_version_range(const char *minversion, Hm, this doesn't work, e.g,. on this /proc/partitions: major minor #blocks name 259 0 500107608 nvme0n1 259 1 266240 nvme0n1p1 259 2 499840327 nvme0n1p2 254 0 499838279 dm-0 As the second patch makes this available to the Lua realm and hence hints to that function to be used in a Lua Handler, wouldn't it be more flexible / better to just expose stat(2) to the Lua realm (currently not the case) and do the logics in Lua? Maybe in an example handler implementing get_root_device() in Lua? That said, stat(2) could be of use besides this use case in the Lua realm (e.g,. for handling symlinks there).... Kind regards, Christian
Hi Christian, On 12.03.21 17:04, Christian Storm wrote: > Hi Stefano, > >> get_find_root() is a utility function that returns the device mounted as >> rootfs. >> >> Signed-off-by: Stefano Babic <sbabic@denx.de> >> --- >> core/util.c | 35 +++++++++++++++++++++++++++++++++++ >> include/util.h | 1 + >> 2 files changed, 36 insertions(+) >> >> diff --git a/core/util.c b/core/util.c >> index 2025276..7c82c2a 100644 >> --- a/core/util.c >> +++ b/core/util.c >> @@ -842,3 +842,38 @@ size_t snescape(char *dst, size_t n, const char *src) >> >> return len; >> } >> + >> +/* >> + * This returns the device name where rootfs is mounted >> + */ >> +char *get_root_device(void) >> +{ >> + struct stat info; >> + FILE *fp; >> + char *devname = NULL; >> + unsigned long major, minor, nblocks; >> + char buf[256]; >> + int ret; >> + >> + if (stat("/", &info) < 0) >> + return NULL; >> + >> + fp = fopen("/proc/partitions", "r"); >> + if (!fp) >> + return NULL; >> + >> + while (fgets(buf, sizeof(buf), fp)) { >> + ret = sscanf(buf, "%ld %ld %ld %ms", >> + &major, &minor, &nblocks, &devname); >> + if (ret != 4) >> + continue; >> + if ((major == info.st_dev / 256) && (minor == info.st_dev % 256)) { >> + fclose(fp); >> + return devname; >> + } >> + free(devname); >> + } >> + >> + fclose(fp); >> + return NULL; >> +} >> diff --git a/include/util.h b/include/util.h >> index 2987203..8447ab6 100644 >> --- a/include/util.h >> +++ b/include/util.h >> @@ -229,6 +229,7 @@ int set_aes_ivt(const char *ivt); >> int get_install_info(sourcetype *source, char *buf, size_t len); >> void get_install_swset(char *buf, size_t len); >> void get_install_running_mode(char *buf, size_t len); >> +char *get_root_device(void); >> >> /* Setting global information */ >> void set_version_range(const char *minversion, > > > Hm, this doesn't work, e.g,. on this /proc/partitions: > major minor #blocks name > > 259 0 500107608 nvme0n1 > 259 1 266240 nvme0n1p1 > 259 2 499840327 nvme0n1p2 > 254 0 499838279 dm-0 > Can you say what is not working ? I have tested this case. I added a simple : printf("ROOT=%s\n", get_root_device()); to SWUpdate code. I have in /proc/partitions: 259 0 976762584 nvme0n1 259 1 524288 nvme0n1p1 259 2 976236544 nvme0n1p2 259 3 976762584 nvme1n1 259 4 976759808 nvme1n1p1 so twice NVME peripherals. And result is ROOT=nvme0n1p2 as expected. What is coming from your system ? > As the second patch makes this available to the Lua realm > and hence hints to that function to be used in a Lua Handler, > wouldn't it be more flexible / better to just expose stat(2) Sure, this can be done. Nevertheless, stat has a lot of output. Do we need all fields reported by a struct stat ? > to the Lua realm (currently not the case) and do the logics > in Lua? Yes, this can be done. I wanted to simplify the usage in Lua, so that it can be called simply from a hook. > Maybe in an example handler implementing get_root_device() > in Lua? > > That said, stat(2) could be of use besides this use case in > the Lua realm (e.g,. for handling symlinks there).... Ok - I can do it for a V2, but I am uncertain if all fields should be exposed in a table. Best regards, Stefano
Hi Stefano, > > > get_find_root() is a utility function that returns the device mounted as > > > rootfs. > > > > > > Signed-off-by: Stefano Babic <sbabic@denx.de> > > > --- > > > core/util.c | 35 +++++++++++++++++++++++++++++++++++ > > > include/util.h | 1 + > > > 2 files changed, 36 insertions(+) > > > > > > diff --git a/core/util.c b/core/util.c > > > index 2025276..7c82c2a 100644 > > > --- a/core/util.c > > > +++ b/core/util.c > > > @@ -842,3 +842,38 @@ size_t snescape(char *dst, size_t n, const char *src) > > > return len; > > > } > > > + > > > +/* > > > + * This returns the device name where rootfs is mounted > > > + */ > > > +char *get_root_device(void) > > > +{ > > > + struct stat info; > > > + FILE *fp; > > > + char *devname = NULL; > > > + unsigned long major, minor, nblocks; > > > + char buf[256]; > > > + int ret; > > > + > > > + if (stat("/", &info) < 0) > > > + return NULL; > > > + > > > + fp = fopen("/proc/partitions", "r"); > > > + if (!fp) > > > + return NULL; > > > + > > > + while (fgets(buf, sizeof(buf), fp)) { > > > + ret = sscanf(buf, "%ld %ld %ld %ms", > > > + &major, &minor, &nblocks, &devname); > > > + if (ret != 4) > > > + continue; > > > + if ((major == info.st_dev / 256) && (minor == info.st_dev % 256)) { > > > + fclose(fp); > > > + return devname; > > > + } > > > + free(devname); > > > + } > > > + > > > + fclose(fp); > > > + return NULL; > > > +} > > > diff --git a/include/util.h b/include/util.h > > > index 2987203..8447ab6 100644 > > > --- a/include/util.h > > > +++ b/include/util.h > > > @@ -229,6 +229,7 @@ int set_aes_ivt(const char *ivt); > > > int get_install_info(sourcetype *source, char *buf, size_t len); > > > void get_install_swset(char *buf, size_t len); > > > void get_install_running_mode(char *buf, size_t len); > > > +char *get_root_device(void); > > > /* Setting global information */ > > > void set_version_range(const char *minversion, > > > > > > Hm, this doesn't work, e.g,. on this /proc/partitions: > > major minor #blocks name > > > > 259 0 500107608 nvme0n1 > > 259 1 266240 nvme0n1p1 > > 259 2 499840327 nvme0n1p2 > > 254 0 499838279 dm-0 > > > > Can you say what is not working ? I have tested this case. I added a simple : > > printf("ROOT=%s\n", get_root_device()); > > to SWUpdate code. I have in /proc/partitions: > > 259 0 976762584 nvme0n1 > 259 1 524288 nvme0n1p1 > 259 2 976236544 nvme0n1p2 > 259 3 976762584 nvme1n1 > 259 4 976759808 nvme1n1p1 > > so twice NVME peripherals. > > And result is > > ROOT=nvme0n1p2 > > > as expected. What is coming from your system ? Nothing, actually. This is because the root device is luks-crypted and hence it's (on my test machine) /dev/mapper/cryptroot which doesn't give an output with this logics... > > As the second patch makes this available to the Lua realm > > and hence hints to that function to be used in a Lua Handler, > > wouldn't it be more flexible / better to just expose stat(2) > > Sure, this can be done. Nevertheless, stat has a lot of output. Do we need all > fields reported by a struct stat ? Hm, not sure if we need the times and sizes but I think we need at least st_dev, st_mode, and maybe st_nlink in Lua. st_mode is needed in Lua to resolve /dev/root symlinks sometimes found on some systems... > > to the Lua realm (currently not the case) and do the logics > > in Lua? > > Yes, this can be done. I wanted to simplify the usage in Lua, so that it can > be called simply from a hook. Yes, good point. But then this logics has to go great lengths to cover all the different ways of finding the root ― mine here is just one example ― in order to be reliable and to work in all conditions. Maybe it's easier to give the means to the Lua realm (in terms of stat(2)) and make an example there? > > Maybe in an example handler implementing get_root_device() > > in Lua? > > > > That said, stat(2) could be of use besides this use case in > > the Lua realm (e.g,. for handling symlinks there).... > > Ok - I can do it for a V2, but I am uncertain if all fields should be exposed > in a table. See above, a sensible selection would do it I think since this is not meant as a full wrapper for stat(2) but instead for specific information. Maybe we can make this clear by not naming it stat(2) but instead something like get_file_status() ― which is what stat(2) is but named differently? Just an idea... Kind regards, Christian
Hi Christian, On 15.03.21 10:15, Christian Storm wrote: > Hi Stefano, > >>>> get_find_root() is a utility function that returns the device mounted as >>>> rootfs. >>>> >>>> Signed-off-by: Stefano Babic <sbabic@denx.de> >>>> --- >>>> core/util.c | 35 +++++++++++++++++++++++++++++++++++ >>>> include/util.h | 1 + >>>> 2 files changed, 36 insertions(+) >>>> >>>> diff --git a/core/util.c b/core/util.c >>>> index 2025276..7c82c2a 100644 >>>> --- a/core/util.c >>>> +++ b/core/util.c >>>> @@ -842,3 +842,38 @@ size_t snescape(char *dst, size_t n, const char *src) >>>> return len; >>>> } >>>> + >>>> +/* >>>> + * This returns the device name where rootfs is mounted >>>> + */ >>>> +char *get_root_device(void) >>>> +{ >>>> + struct stat info; >>>> + FILE *fp; >>>> + char *devname = NULL; >>>> + unsigned long major, minor, nblocks; >>>> + char buf[256]; >>>> + int ret; >>>> + >>>> + if (stat("/", &info) < 0) >>>> + return NULL; >>>> + >>>> + fp = fopen("/proc/partitions", "r"); >>>> + if (!fp) >>>> + return NULL; >>>> + >>>> + while (fgets(buf, sizeof(buf), fp)) { >>>> + ret = sscanf(buf, "%ld %ld %ld %ms", >>>> + &major, &minor, &nblocks, &devname); >>>> + if (ret != 4) >>>> + continue; >>>> + if ((major == info.st_dev / 256) && (minor == info.st_dev % 256)) { >>>> + fclose(fp); >>>> + return devname; >>>> + } >>>> + free(devname); >>>> + } >>>> + >>>> + fclose(fp); >>>> + return NULL; >>>> +} >>>> diff --git a/include/util.h b/include/util.h >>>> index 2987203..8447ab6 100644 >>>> --- a/include/util.h >>>> +++ b/include/util.h >>>> @@ -229,6 +229,7 @@ int set_aes_ivt(const char *ivt); >>>> int get_install_info(sourcetype *source, char *buf, size_t len); >>>> void get_install_swset(char *buf, size_t len); >>>> void get_install_running_mode(char *buf, size_t len); >>>> +char *get_root_device(void); >>>> /* Setting global information */ >>>> void set_version_range(const char *minversion, >>> >>> >>> Hm, this doesn't work, e.g,. on this /proc/partitions: >>> major minor #blocks name >>> >>> 259 0 500107608 nvme0n1 >>> 259 1 266240 nvme0n1p1 >>> 259 2 499840327 nvme0n1p2 >>> 254 0 499838279 dm-0 >>> >> >> Can you say what is not working ? I have tested this case. I added a simple : >> >> printf("ROOT=%s\n", get_root_device()); >> >> to SWUpdate code. I have in /proc/partitions: >> >> 259 0 976762584 nvme0n1 >> 259 1 524288 nvme0n1p1 >> 259 2 976236544 nvme0n1p2 >> 259 3 976762584 nvme1n1 >> 259 4 976759808 nvme1n1p1 >> >> so twice NVME peripherals. >> >> And result is >> >> ROOT=nvme0n1p2 >> >> >> as expected. What is coming from your system ? > > Nothing, actually. This is because the root device is luks-crypted and > hence it's (on my test machine) /dev/mapper/cryptroot which doesn't give > an output with this logics... Ok - well, this does not forbid the usage of the function for simple mounted block device (it works with dev/mapper, too, but not for cryptsetup of course). I mean, the funcion is then available and the integrator can decide if it is suitable for its project. And for most projects (it works with UBIFS, too) it works. Maybe should we rename it, something like get_root_block_device() ? (but it is becomeing too long..) > > >>> As the second patch makes this available to the Lua realm >>> and hence hints to that function to be used in a Lua Handler, >>> wouldn't it be more flexible / better to just expose stat(2) >> >> Sure, this can be done. Nevertheless, stat has a lot of output. Do we need all >> fields reported by a struct stat ? > > Hm, not sure if we need the times and sizes but I think we need at least > st_dev, st_mode, and maybe st_nlink in Lua. I tend to say size can be useful, too. > st_mode is needed in Lua to resolve /dev/root symlinks sometimes found > on some systems... Ok Nevertheless, adding stat to lua interface is quite another topic, this should be addressed with a follow up patch. > > >>> to the Lua realm (currently not the case) and do the logics >>> in Lua? >> >> Yes, this can be done. I wanted to simplify the usage in Lua, so that it can >> be called simply from a hook. > > Yes, good point. But then this logics has to go great lengths to cover > all the different ways of finding the root ― mine here is just one > example ― in order to be reliable and to work in all conditions. > Maybe it's easier to give the means to the Lua realm (in terms of > stat(2)) and make an example there? It is useful, yes. But IMHO it is not bad that there is a SWUpdate's function that returns which is the root device, and if there are some other use cases not covered by the implementation of this patch (like the one with cryptsetup), they should be added. So if the current method does not find the root device, it should proceed with a further algorithm, until it tried all known ways to retrieve it. > > >>> Maybe in an example handler implementing get_root_device() >>> in Lua? >>> >>> That said, stat(2) could be of use besides this use case in >>> the Lua realm (e.g,. for handling symlinks there).... >> >> Ok - I can do it for a V2, but I am uncertain if all fields should be exposed >> in a table. > > See above, a sensible selection would do it I think since this > is not meant as a full wrapper for stat(2) but instead for specific > information. Maybe we can make this clear by not naming it stat(2) but > instead something like get_file_status() Yes, agree - it should have another name. > ― which is what stat(2) is but > named differently? Just an idea... > Best regards, Stefano
Hi Stefano, > > > > > get_find_root() is a utility function that returns the device mounted as > > > > > rootfs. > > > > > > > > > > Signed-off-by: Stefano Babic <sbabic@denx.de> > > > > > --- > > > > > core/util.c | 35 +++++++++++++++++++++++++++++++++++ > > > > > include/util.h | 1 + > > > > > 2 files changed, 36 insertions(+) > > > > > > > > > > diff --git a/core/util.c b/core/util.c > > > > > index 2025276..7c82c2a 100644 > > > > > --- a/core/util.c > > > > > +++ b/core/util.c > > > > > @@ -842,3 +842,38 @@ size_t snescape(char *dst, size_t n, const char *src) > > > > > return len; > > > > > } > > > > > + > > > > > +/* > > > > > + * This returns the device name where rootfs is mounted > > > > > + */ > > > > > +char *get_root_device(void) > > > > > +{ > > > > > + struct stat info; > > > > > + FILE *fp; > > > > > + char *devname = NULL; > > > > > + unsigned long major, minor, nblocks; > > > > > + char buf[256]; > > > > > + int ret; > > > > > + > > > > > + if (stat("/", &info) < 0) > > > > > + return NULL; > > > > > + > > > > > + fp = fopen("/proc/partitions", "r"); > > > > > + if (!fp) > > > > > + return NULL; > > > > > + > > > > > + while (fgets(buf, sizeof(buf), fp)) { > > > > > + ret = sscanf(buf, "%ld %ld %ld %ms", > > > > > + &major, &minor, &nblocks, &devname); > > > > > + if (ret != 4) > > > > > + continue; > > > > > + if ((major == info.st_dev / 256) && (minor == info.st_dev % 256)) { > > > > > + fclose(fp); > > > > > + return devname; > > > > > + } > > > > > + free(devname); > > > > > + } > > > > > + > > > > > + fclose(fp); > > > > > + return NULL; > > > > > +} > > > > > diff --git a/include/util.h b/include/util.h > > > > > index 2987203..8447ab6 100644 > > > > > --- a/include/util.h > > > > > +++ b/include/util.h > > > > > @@ -229,6 +229,7 @@ int set_aes_ivt(const char *ivt); > > > > > int get_install_info(sourcetype *source, char *buf, size_t len); > > > > > void get_install_swset(char *buf, size_t len); > > > > > void get_install_running_mode(char *buf, size_t len); > > > > > +char *get_root_device(void); > > > > > /* Setting global information */ > > > > > void set_version_range(const char *minversion, > > > > > > > > > > > > Hm, this doesn't work, e.g,. on this /proc/partitions: > > > > major minor #blocks name > > > > > > > > 259 0 500107608 nvme0n1 > > > > 259 1 266240 nvme0n1p1 > > > > 259 2 499840327 nvme0n1p2 > > > > 254 0 499838279 dm-0 > > > > > > > > > > Can you say what is not working ? I have tested this case. I added a simple : > > > > > > printf("ROOT=%s\n", get_root_device()); > > > > > > to SWUpdate code. I have in /proc/partitions: > > > > > > 259 0 976762584 nvme0n1 > > > 259 1 524288 nvme0n1p1 > > > 259 2 976236544 nvme0n1p2 > > > 259 3 976762584 nvme1n1 > > > 259 4 976759808 nvme1n1p1 > > > > > > so twice NVME peripherals. > > > > > > And result is > > > > > > ROOT=nvme0n1p2 > > > > > > > > > as expected. What is coming from your system ? > > > > Nothing, actually. This is because the root device is luks-crypted and > > hence it's (on my test machine) /dev/mapper/cryptroot which doesn't give > > an output with this logics... > > Ok - well, this does not forbid the usage of the function for simple mounted > block device (it works with dev/mapper, too, but not for cryptsetup of > course). I mean, the funcion is then available and the integrator can decide > if it is suitable for its project. And for most projects (it works with UBIFS, > too) it works. Maybe should we rename it, something like > get_root_block_device() ? (but it is becomeing too long..) Yes, it doesn't inhibit using this for *most* purposes, yes, and if that one suits your use case it does work well. In the other cases, you have to come up with your own solution. I think either we make this work for all the various configurations or we mark this as example, and in this case, I think it's better suited as a Lua example. Otherwise, one might expect this to work while it can't as it's not designed for this purpose.... (see below). > > > > As the second patch makes this available to the Lua realm > > > > and hence hints to that function to be used in a Lua Handler, > > > > wouldn't it be more flexible / better to just expose stat(2) > > > > > > Sure, this can be done. Nevertheless, stat has a lot of output. Do we need all > > > fields reported by a struct stat ? > > > > Hm, not sure if we need the times and sizes but I think we need at least > > st_dev, st_mode, and maybe st_nlink in Lua. > > I tend to say size can be useful, too. Yes, while not for this purpose at hand, but I agree, it's useful. > > st_mode is needed in Lua to resolve /dev/root symlinks sometimes found > > on some systems... > > Ok > > Nevertheless, adding stat to lua interface is quite another topic, this should > be addressed with a follow up patch. Yes, that's fine. > > > > to the Lua realm (currently not the case) and do the logics > > > > in Lua? > > > > > > Yes, this can be done. I wanted to simplify the usage in Lua, so that it can > > > be called simply from a hook. > > > > Yes, good point. But then this logics has to go great lengths to cover > > all the different ways of finding the root ― mine here is just one > > example ― in order to be reliable and to work in all conditions. > > Maybe it's easier to give the means to the Lua realm (in terms of > > stat(2)) and make an example there? > > It is useful, yes. But IMHO it is not bad that there is a SWUpdate's function > that returns which is the root device, and if there are some other use cases > not covered by the implementation of this patch (like the one with > cryptsetup), they should be added. So if the current method does not find the > root device, it should proceed with a further algorithm, until it tried all > known ways to retrieve it. OK, so it's set to become a function that really tries hard to find the root device. In this case, I think it's a valuable addition to SWUpdate's C core part. Then, I think some parsing of /proc/cmdline as well as resolving a /dev/root symlink might be added? Kind regards, Christian
Hi Christian, On 15.03.21 15:39, Christian Storm wrote: > > Hi Stefano, > >>>>>> get_find_root() is a utility function that returns the device mounted as >>>>>> rootfs. >>>>>> >>>>>> Signed-off-by: Stefano Babic <sbabic@denx.de> >>>>>> --- >>>>>> core/util.c | 35 +++++++++++++++++++++++++++++++++++ >>>>>> include/util.h | 1 + >>>>>> 2 files changed, 36 insertions(+) >>>>>> >>>>>> diff --git a/core/util.c b/core/util.c >>>>>> index 2025276..7c82c2a 100644 >>>>>> --- a/core/util.c >>>>>> +++ b/core/util.c >>>>>> @@ -842,3 +842,38 @@ size_t snescape(char *dst, size_t n, const char *src) >>>>>> return len; >>>>>> } >>>>>> + >>>>>> +/* >>>>>> + * This returns the device name where rootfs is mounted >>>>>> + */ >>>>>> +char *get_root_device(void) >>>>>> +{ >>>>>> + struct stat info; >>>>>> + FILE *fp; >>>>>> + char *devname = NULL; >>>>>> + unsigned long major, minor, nblocks; >>>>>> + char buf[256]; >>>>>> + int ret; >>>>>> + >>>>>> + if (stat("/", &info) < 0) >>>>>> + return NULL; >>>>>> + >>>>>> + fp = fopen("/proc/partitions", "r"); >>>>>> + if (!fp) >>>>>> + return NULL; >>>>>> + >>>>>> + while (fgets(buf, sizeof(buf), fp)) { >>>>>> + ret = sscanf(buf, "%ld %ld %ld %ms", >>>>>> + &major, &minor, &nblocks, &devname); >>>>>> + if (ret != 4) >>>>>> + continue; >>>>>> + if ((major == info.st_dev / 256) && (minor == info.st_dev % 256)) { >>>>>> + fclose(fp); >>>>>> + return devname; >>>>>> + } >>>>>> + free(devname); >>>>>> + } >>>>>> + >>>>>> + fclose(fp); >>>>>> + return NULL; >>>>>> +} >>>>>> diff --git a/include/util.h b/include/util.h >>>>>> index 2987203..8447ab6 100644 >>>>>> --- a/include/util.h >>>>>> +++ b/include/util.h >>>>>> @@ -229,6 +229,7 @@ int set_aes_ivt(const char *ivt); >>>>>> int get_install_info(sourcetype *source, char *buf, size_t len); >>>>>> void get_install_swset(char *buf, size_t len); >>>>>> void get_install_running_mode(char *buf, size_t len); >>>>>> +char *get_root_device(void); >>>>>> /* Setting global information */ >>>>>> void set_version_range(const char *minversion, >>>>> >>>>> >>>>> Hm, this doesn't work, e.g,. on this /proc/partitions: >>>>> major minor #blocks name >>>>> >>>>> 259 0 500107608 nvme0n1 >>>>> 259 1 266240 nvme0n1p1 >>>>> 259 2 499840327 nvme0n1p2 >>>>> 254 0 499838279 dm-0 >>>>> >>>> >>>> Can you say what is not working ? I have tested this case. I added a simple : >>>> >>>> printf("ROOT=%s\n", get_root_device()); >>>> >>>> to SWUpdate code. I have in /proc/partitions: >>>> >>>> 259 0 976762584 nvme0n1 >>>> 259 1 524288 nvme0n1p1 >>>> 259 2 976236544 nvme0n1p2 >>>> 259 3 976762584 nvme1n1 >>>> 259 4 976759808 nvme1n1p1 >>>> >>>> so twice NVME peripherals. >>>> >>>> And result is >>>> >>>> ROOT=nvme0n1p2 >>>> >>>> >>>> as expected. What is coming from your system ? >>> >>> Nothing, actually. This is because the root device is luks-crypted and >>> hence it's (on my test machine) /dev/mapper/cryptroot which doesn't give >>> an output with this logics... >> >> Ok - well, this does not forbid the usage of the function for simple mounted >> block device (it works with dev/mapper, too, but not for cryptsetup of >> course). I mean, the funcion is then available and the integrator can decide >> if it is suitable for its project. And for most projects (it works with UBIFS, >> too) it works. Maybe should we rename it, something like >> get_root_block_device() ? (but it is becomeing too long..) > > Yes, it doesn't inhibit using this for *most* purposes, yes, and if that > one suits your use case it does work well. In the other cases, you have > to come up with your own solution. I think either we make this work for > all the various configurations or we mark this as example, and in this > case, I think it's better suited as a Lua example. Otherwise, one > might expect this to work while it can't as it's not designed for this > purpose.... (see below). > > >>>>> As the second patch makes this available to the Lua realm >>>>> and hence hints to that function to be used in a Lua Handler, >>>>> wouldn't it be more flexible / better to just expose stat(2) >>>> >>>> Sure, this can be done. Nevertheless, stat has a lot of output. Do we need all >>>> fields reported by a struct stat ? >>> >>> Hm, not sure if we need the times and sizes but I think we need at least >>> st_dev, st_mode, and maybe st_nlink in Lua. >> >> I tend to say size can be useful, too. > > Yes, while not for this purpose at hand, but I agree, it's useful. > > >>> st_mode is needed in Lua to resolve /dev/root symlinks sometimes found >>> on some systems... >> >> Ok >> >> Nevertheless, adding stat to lua interface is quite another topic, this should >> be addressed with a follow up patch. > > Yes, that's fine. > > >>>>> to the Lua realm (currently not the case) and do the logics >>>>> in Lua? >>>> >>>> Yes, this can be done. I wanted to simplify the usage in Lua, so that it can >>>> be called simply from a hook. >>> >>> Yes, good point. But then this logics has to go great lengths to cover >>> all the different ways of finding the root ― mine here is just one >>> example ― in order to be reliable and to work in all conditions. >>> Maybe it's easier to give the means to the Lua realm (in terms of >>> stat(2)) and make an example there? >> >> It is useful, yes. But IMHO it is not bad that there is a SWUpdate's function >> that returns which is the root device, and if there are some other use cases >> not covered by the implementation of this patch (like the one with >> cryptsetup), they should be added. So if the current method does not find the >> root device, it should proceed with a further algorithm, until it tried all >> known ways to retrieve it. > > OK, so it's set to become a function that really tries hard to find the > root device. In this case, I think it's a valuable addition to > SWUpdate's C core part. Then, I think some parsing of /proc/cmdline as > well as resolving a /dev/root symlink might be added? Sure - so the function can fallback to parse /proc/cmdline if from the first method it cannot find a suitable device. So it should implement: 1. search for partition where "/" is mounted (this post) 2. parse cmdline if 1 reports NULL 3. check /dev/root if 2 reports NULL But I am missing what we should have with 3. /dev/root should point to a device where "/" is mounted, so which is the use case where 1. fails and 3. not ? (for 1 and 2 is clear, you have already shown an example). Best regards, Stefano
Hi Stefano, > > > > > > > get_find_root() is a utility function that returns the device mounted as > > > > > > > rootfs. > > > > > > > > > > > > > > Signed-off-by: Stefano Babic <sbabic@denx.de> > > > > > > > --- > > > > > > > core/util.c | 35 +++++++++++++++++++++++++++++++++++ > > > > > > > include/util.h | 1 + > > > > > > > 2 files changed, 36 insertions(+) > > > > > > > > > > > > > > diff --git a/core/util.c b/core/util.c > > > > > > > index 2025276..7c82c2a 100644 > > > > > > > --- a/core/util.c > > > > > > > +++ b/core/util.c > > > > > > > @@ -842,3 +842,38 @@ size_t snescape(char *dst, size_t n, const char *src) > > > > > > > return len; > > > > > > > } > > > > > > > + > > > > > > > +/* > > > > > > > + * This returns the device name where rootfs is mounted > > > > > > > + */ > > > > > > > +char *get_root_device(void) > > > > > > > +{ > > > > > > > + struct stat info; > > > > > > > + FILE *fp; > > > > > > > + char *devname = NULL; > > > > > > > + unsigned long major, minor, nblocks; > > > > > > > + char buf[256]; > > > > > > > + int ret; > > > > > > > + > > > > > > > + if (stat("/", &info) < 0) > > > > > > > + return NULL; > > > > > > > + > > > > > > > + fp = fopen("/proc/partitions", "r"); > > > > > > > + if (!fp) > > > > > > > + return NULL; > > > > > > > + > > > > > > > + while (fgets(buf, sizeof(buf), fp)) { > > > > > > > + ret = sscanf(buf, "%ld %ld %ld %ms", > > > > > > > + &major, &minor, &nblocks, &devname); > > > > > > > + if (ret != 4) > > > > > > > + continue; > > > > > > > + if ((major == info.st_dev / 256) && (minor == info.st_dev % 256)) { > > > > > > > + fclose(fp); > > > > > > > + return devname; > > > > > > > + } > > > > > > > + free(devname); > > > > > > > + } > > > > > > > + > > > > > > > + fclose(fp); > > > > > > > + return NULL; > > > > > > > +} > > > > > > > diff --git a/include/util.h b/include/util.h > > > > > > > index 2987203..8447ab6 100644 > > > > > > > --- a/include/util.h > > > > > > > +++ b/include/util.h > > > > > > > @@ -229,6 +229,7 @@ int set_aes_ivt(const char *ivt); > > > > > > > int get_install_info(sourcetype *source, char *buf, size_t len); > > > > > > > void get_install_swset(char *buf, size_t len); > > > > > > > void get_install_running_mode(char *buf, size_t len); > > > > > > > +char *get_root_device(void); > > > > > > > /* Setting global information */ > > > > > > > void set_version_range(const char *minversion, > > > > > > > > > > > > > > > > > > Hm, this doesn't work, e.g,. on this /proc/partitions: > > > > > > major minor #blocks name > > > > > > > > > > > > 259 0 500107608 nvme0n1 > > > > > > 259 1 266240 nvme0n1p1 > > > > > > 259 2 499840327 nvme0n1p2 > > > > > > 254 0 499838279 dm-0 > > > > > > > > > > > > > > > > Can you say what is not working ? I have tested this case. I added a simple : > > > > > > > > > > printf("ROOT=%s\n", get_root_device()); > > > > > > > > > > to SWUpdate code. I have in /proc/partitions: > > > > > > > > > > 259 0 976762584 nvme0n1 > > > > > 259 1 524288 nvme0n1p1 > > > > > 259 2 976236544 nvme0n1p2 > > > > > 259 3 976762584 nvme1n1 > > > > > 259 4 976759808 nvme1n1p1 > > > > > > > > > > so twice NVME peripherals. > > > > > > > > > > And result is > > > > > > > > > > ROOT=nvme0n1p2 > > > > > > > > > > > > > > > as expected. What is coming from your system ? > > > > > > > > Nothing, actually. This is because the root device is luks-crypted and > > > > hence it's (on my test machine) /dev/mapper/cryptroot which doesn't give > > > > an output with this logics... > > > > > > Ok - well, this does not forbid the usage of the function for simple mounted > > > block device (it works with dev/mapper, too, but not for cryptsetup of > > > course). I mean, the funcion is then available and the integrator can decide > > > if it is suitable for its project. And for most projects (it works with UBIFS, > > > too) it works. Maybe should we rename it, something like > > > get_root_block_device() ? (but it is becomeing too long..) > > > > Yes, it doesn't inhibit using this for *most* purposes, yes, and if that > > one suits your use case it does work well. In the other cases, you have > > to come up with your own solution. I think either we make this work for > > all the various configurations or we mark this as example, and in this > > case, I think it's better suited as a Lua example. Otherwise, one > > might expect this to work while it can't as it's not designed for this > > purpose.... (see below). > > > > > > > > > > As the second patch makes this available to the Lua realm > > > > > > and hence hints to that function to be used in a Lua Handler, > > > > > > wouldn't it be more flexible / better to just expose stat(2) > > > > > > > > > > Sure, this can be done. Nevertheless, stat has a lot of output. Do we need all > > > > > fields reported by a struct stat ? > > > > > > > > Hm, not sure if we need the times and sizes but I think we need at least > > > > st_dev, st_mode, and maybe st_nlink in Lua. > > > > > > I tend to say size can be useful, too. > > > > Yes, while not for this purpose at hand, but I agree, it's useful. > > > > > > > > st_mode is needed in Lua to resolve /dev/root symlinks sometimes found > > > > on some systems... > > > > > > Ok > > > > > > Nevertheless, adding stat to lua interface is quite another topic, this should > > > be addressed with a follow up patch. > > > > Yes, that's fine. > > > > > > > > > > to the Lua realm (currently not the case) and do the logics > > > > > > in Lua? > > > > > > > > > > Yes, this can be done. I wanted to simplify the usage in Lua, so that it can > > > > > be called simply from a hook. > > > > > > > > Yes, good point. But then this logics has to go great lengths to cover > > > > all the different ways of finding the root ― mine here is just one > > > > example ― in order to be reliable and to work in all conditions. > > > > Maybe it's easier to give the means to the Lua realm (in terms of > > > > stat(2)) and make an example there? > > > > > > It is useful, yes. But IMHO it is not bad that there is a SWUpdate's function > > > that returns which is the root device, and if there are some other use cases > > > not covered by the implementation of this patch (like the one with > > > cryptsetup), they should be added. So if the current method does not find the > > > root device, it should proceed with a further algorithm, until it tried all > > > known ways to retrieve it. > > > > OK, so it's set to become a function that really tries hard to find the > > root device. In this case, I think it's a valuable addition to > > SWUpdate's C core part. Then, I think some parsing of /proc/cmdline as > > well as resolving a /dev/root symlink might be added? > > Sure - so the function can fallback to parse /proc/cmdline if from the first > method it cannot find a suitable device. So it should implement: > > 1. search for partition where "/" is mounted (this post) > 2. parse cmdline if 1 reports NULL > 3. check /dev/root if 2 reports NULL > > But I am missing what we should have with 3. /dev/root should point to a > device where "/" is mounted, so which is the use case where 1. fails and 3. > not ? (for 1 and 2 is clear, you have already shown an example). Hm, yes, you're of course right. I also think 3. is not necessary as 1. should have found it. So, I guess 1. and 2. are sufficient ― at least for the use cases I can think of right now, that is. Thanks! Kind regards, Christian
diff --git a/core/util.c b/core/util.c index 2025276..7c82c2a 100644 --- a/core/util.c +++ b/core/util.c @@ -842,3 +842,38 @@ size_t snescape(char *dst, size_t n, const char *src) return len; } + +/* + * This returns the device name where rootfs is mounted + */ +char *get_root_device(void) +{ + struct stat info; + FILE *fp; + char *devname = NULL; + unsigned long major, minor, nblocks; + char buf[256]; + int ret; + + if (stat("/", &info) < 0) + return NULL; + + fp = fopen("/proc/partitions", "r"); + if (!fp) + return NULL; + + while (fgets(buf, sizeof(buf), fp)) { + ret = sscanf(buf, "%ld %ld %ld %ms", + &major, &minor, &nblocks, &devname); + if (ret != 4) + continue; + if ((major == info.st_dev / 256) && (minor == info.st_dev % 256)) { + fclose(fp); + return devname; + } + free(devname); + } + + fclose(fp); + return NULL; +} diff --git a/include/util.h b/include/util.h index 2987203..8447ab6 100644 --- a/include/util.h +++ b/include/util.h @@ -229,6 +229,7 @@ int set_aes_ivt(const char *ivt); int get_install_info(sourcetype *source, char *buf, size_t len); void get_install_swset(char *buf, size_t len); void get_install_running_mode(char *buf, size_t len); +char *get_root_device(void); /* Setting global information */ void set_version_range(const char *minversion,
get_find_root() is a utility function that returns the device mounted as rootfs. Signed-off-by: Stefano Babic <sbabic@denx.de> --- core/util.c | 35 +++++++++++++++++++++++++++++++++++ include/util.h | 1 + 2 files changed, 36 insertions(+)