| Message ID | 1348007874-20466-4-git-send-email-swarren@wwwdotorg.org |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
On 09/18/2012 05:37 PM, Stephen Warren wrote: > From: Stephen Warren <swarren@nvidia.com> > > This patch introduces function get_device(). This looks up a > block_dev_desc_t from an interface name (e.g. mmc) and device number > (e.g. 0). This function is essentially the non-partition-specific > prefix of get_device_and_partition(). Then shouldn't get_device_and_partition just call get_device. Perhaps create get_partition() and then get_device_and_partition is just a wrapper. Rob > > Signed-off-by: Stephen Warren <swarren@nvidia.com> > --- > v3: New patch. > --- > disk/part.c | 22 ++++++++++++++++++++++ > include/part.h | 5 +++++ > 2 files changed, 27 insertions(+), 0 deletions(-) > > diff --git a/disk/part.c b/disk/part.c > index 277a243..9920d48 100644 > --- a/disk/part.c > +++ b/disk/part.c > @@ -435,6 +435,28 @@ void print_part (block_dev_desc_t * dev_desc) > > #endif > > +int get_device(const char *ifname, const char *dev_str, > + block_dev_desc_t **dev_desc) > +{ > + char *ep; > + int dev; > + > + dev = simple_strtoul(dev_str, &ep, 16); > + if (*ep) { > + printf("** Bad device specification %s %s **\n", > + ifname, dev_str); > + return -1; > + } > + > + *dev_desc = get_dev(ifname, dev); > + if (!(*dev_desc) || ((*dev_desc)->type == DEV_TYPE_UNKNOWN)) { > + printf("** Bad device %s %s **\n", ifname, dev_str); > + return -1; > + } > + > + return dev; > +} > + > #define MAX_SEARCH_PARTITIONS 16 > int get_device_and_partition(const char *ifname, const char *dev_str, > block_dev_desc_t **dev_desc, > diff --git a/include/part.h b/include/part.h > index a6d06f3..144df4e 100644 > --- a/include/part.h > +++ b/include/part.h > @@ -112,6 +112,8 @@ int get_partition_info (block_dev_desc_t * dev_desc, int part, disk_partition_t > void print_part (block_dev_desc_t *dev_desc); > void init_part (block_dev_desc_t *dev_desc); > void dev_print(block_dev_desc_t *dev_desc); > +int get_device(const char *ifname, const char *dev_str, > + block_dev_desc_t **dev_desc); > int get_device_and_partition(const char *ifname, const char *dev_str, > block_dev_desc_t **dev_desc, > disk_partition_t *info); > @@ -131,6 +133,9 @@ static inline int get_partition_info (block_dev_desc_t * dev_desc, int part, > static inline void print_part (block_dev_desc_t *dev_desc) {} > static inline void init_part (block_dev_desc_t *dev_desc) {} > static inline void dev_print(block_dev_desc_t *dev_desc) {} > +static inline int get_device(const char *ifname, const char *dev_str, > + block_dev_desc_t **dev_desc) > +{ return -1; } > static inline int get_device_and_partition(const char *ifname, > const char *dev_str, > block_dev_desc_t **dev_desc, >
On 09/18/2012 08:21 PM, Rob Herring wrote: > On 09/18/2012 05:37 PM, Stephen Warren wrote: >> From: Stephen Warren <swarren@nvidia.com> >> >> This patch introduces function get_device(). This looks up a >> block_dev_desc_t from an interface name (e.g. mmc) and device number >> (e.g. 0). This function is essentially the non-partition-specific >> prefix of get_device_and_partition(). > > Then shouldn't get_device_and_partition just call get_device. Perhaps > create get_partition() and then get_device_and_partition is just a wrapper. > I should read all the way through the series before replying... Anyway, I would squash it all unless you want to have restructuring with current functionality and then enhancements. Rob > Rob > >> >> Signed-off-by: Stephen Warren <swarren@nvidia.com> >> --- >> v3: New patch. >> --- >> disk/part.c | 22 ++++++++++++++++++++++ >> include/part.h | 5 +++++ >> 2 files changed, 27 insertions(+), 0 deletions(-) >> >> diff --git a/disk/part.c b/disk/part.c >> index 277a243..9920d48 100644 >> --- a/disk/part.c >> +++ b/disk/part.c >> @@ -435,6 +435,28 @@ void print_part (block_dev_desc_t * dev_desc) >> >> #endif >> >> +int get_device(const char *ifname, const char *dev_str, >> + block_dev_desc_t **dev_desc) >> +{ >> + char *ep; >> + int dev; >> + >> + dev = simple_strtoul(dev_str, &ep, 16); >> + if (*ep) { >> + printf("** Bad device specification %s %s **\n", >> + ifname, dev_str); >> + return -1; >> + } >> + >> + *dev_desc = get_dev(ifname, dev); >> + if (!(*dev_desc) || ((*dev_desc)->type == DEV_TYPE_UNKNOWN)) { >> + printf("** Bad device %s %s **\n", ifname, dev_str); >> + return -1; >> + } >> + >> + return dev; >> +} >> + >> #define MAX_SEARCH_PARTITIONS 16 >> int get_device_and_partition(const char *ifname, const char *dev_str, >> block_dev_desc_t **dev_desc, >> diff --git a/include/part.h b/include/part.h >> index a6d06f3..144df4e 100644 >> --- a/include/part.h >> +++ b/include/part.h >> @@ -112,6 +112,8 @@ int get_partition_info (block_dev_desc_t * dev_desc, int part, disk_partition_t >> void print_part (block_dev_desc_t *dev_desc); >> void init_part (block_dev_desc_t *dev_desc); >> void dev_print(block_dev_desc_t *dev_desc); >> +int get_device(const char *ifname, const char *dev_str, >> + block_dev_desc_t **dev_desc); >> int get_device_and_partition(const char *ifname, const char *dev_str, >> block_dev_desc_t **dev_desc, >> disk_partition_t *info); >> @@ -131,6 +133,9 @@ static inline int get_partition_info (block_dev_desc_t * dev_desc, int part, >> static inline void print_part (block_dev_desc_t *dev_desc) {} >> static inline void init_part (block_dev_desc_t *dev_desc) {} >> static inline void dev_print(block_dev_desc_t *dev_desc) {} >> +static inline int get_device(const char *ifname, const char *dev_str, >> + block_dev_desc_t **dev_desc) >> +{ return -1; } >> static inline int get_device_and_partition(const char *ifname, >> const char *dev_str, >> block_dev_desc_t **dev_desc, >> >
On 09/18/2012 07:25 PM, Rob Herring wrote: > On 09/18/2012 08:21 PM, Rob Herring wrote: >> On 09/18/2012 05:37 PM, Stephen Warren wrote: >>> From: Stephen Warren <swarren@nvidia.com> >>> >>> This patch introduces function get_device(). This looks up a >>> block_dev_desc_t from an interface name (e.g. mmc) and device number >>> (e.g. 0). This function is essentially the non-partition-specific >>> prefix of get_device_and_partition(). >> >> Then shouldn't get_device_and_partition just call get_device. Perhaps >> create get_partition() and then get_device_and_partition is just a wrapper. >> > > I should read all the way through the series before replying... > > Anyway, I would squash it all unless you want to have restructuring with > current functionality and then enhancements. OK, I'm happy to squash everything together, and repost a series with both your and my patches; I separated it out to hopefully make working out what changes I made a little easier. I did wonder about creating separate get_device, get_partition, and get_device_or_partition functions, the latter probably using a common implementation. I guess if I squash my changes into your original patches, then creating those 3 separate functions would end up being less churn. So, unless there are objections, I'll do that.
On Tue, Sep 18, 2012 at 08:25:31PM -0500, Rob Herring wrote: > On 09/18/2012 08:21 PM, Rob Herring wrote: > > On 09/18/2012 05:37 PM, Stephen Warren wrote: > >> From: Stephen Warren <swarren@nvidia.com> > >> > >> This patch introduces function get_device(). This looks up a > >> block_dev_desc_t from an interface name (e.g. mmc) and device number > >> (e.g. 0). This function is essentially the non-partition-specific > >> prefix of get_device_and_partition(). > > > > Then shouldn't get_device_and_partition just call get_device. Perhaps > > create get_partition() and then get_device_and_partition is just a wrapper. > > > > I should read all the way through the series before replying... > > Anyway, I would squash it all unless you want to have restructuring with > current functionality and then enhancements. IMHO, restucture and then enhancements makes the most sense since it means we can bisect a latent bug easier. So no need for a v4 to squash patches down.
On 09/18/2012 05:37 PM, Stephen Warren wrote: > From: Stephen Warren <swarren@nvidia.com> > > This patch introduces function get_device(). This looks up a > block_dev_desc_t from an interface name (e.g. mmc) and device number > (e.g. 0). This function is essentially the non-partition-specific > prefix of get_device_and_partition(). > > Signed-off-by: Stephen Warren <swarren@nvidia.com> > --- > v3: New patch. > --- > disk/part.c | 22 ++++++++++++++++++++++ > include/part.h | 5 +++++ > 2 files changed, 27 insertions(+), 0 deletions(-) > > diff --git a/disk/part.c b/disk/part.c > index 277a243..9920d48 100644 > --- a/disk/part.c > +++ b/disk/part.c > @@ -435,6 +435,28 @@ void print_part (block_dev_desc_t * dev_desc) > > #endif > > +int get_device(const char *ifname, const char *dev_str, > + block_dev_desc_t **dev_desc) > +{ > + char *ep; > + int dev; > + Why don't you look up bootdevice here? That would be more consistent behavior. Rob > + dev = simple_strtoul(dev_str, &ep, 16); > + if (*ep) { > + printf("** Bad device specification %s %s **\n", > + ifname, dev_str); > + return -1; > + } > + > + *dev_desc = get_dev(ifname, dev); > + if (!(*dev_desc) || ((*dev_desc)->type == DEV_TYPE_UNKNOWN)) { > + printf("** Bad device %s %s **\n", ifname, dev_str); > + return -1; > + } > + > + return dev; > +} > + > #define MAX_SEARCH_PARTITIONS 16 > int get_device_and_partition(const char *ifname, const char *dev_str, > block_dev_desc_t **dev_desc, > diff --git a/include/part.h b/include/part.h > index a6d06f3..144df4e 100644 > --- a/include/part.h > +++ b/include/part.h > @@ -112,6 +112,8 @@ int get_partition_info (block_dev_desc_t * dev_desc, int part, disk_partition_t > void print_part (block_dev_desc_t *dev_desc); > void init_part (block_dev_desc_t *dev_desc); > void dev_print(block_dev_desc_t *dev_desc); > +int get_device(const char *ifname, const char *dev_str, > + block_dev_desc_t **dev_desc); > int get_device_and_partition(const char *ifname, const char *dev_str, > block_dev_desc_t **dev_desc, > disk_partition_t *info); > @@ -131,6 +133,9 @@ static inline int get_partition_info (block_dev_desc_t * dev_desc, int part, > static inline void print_part (block_dev_desc_t *dev_desc) {} > static inline void init_part (block_dev_desc_t *dev_desc) {} > static inline void dev_print(block_dev_desc_t *dev_desc) {} > +static inline int get_device(const char *ifname, const char *dev_str, > + block_dev_desc_t **dev_desc) > +{ return -1; } > static inline int get_device_and_partition(const char *ifname, > const char *dev_str, > block_dev_desc_t **dev_desc, >
On 09/21/2012 06:53 AM, Rob Herring wrote: > On 09/18/2012 05:37 PM, Stephen Warren wrote: >> From: Stephen Warren <swarren@nvidia.com> >> >> This patch introduces function get_device(). This looks up a >> block_dev_desc_t from an interface name (e.g. mmc) and device number >> (e.g. 0). This function is essentially the non-partition-specific >> prefix of get_device_and_partition(). >> +int get_device(const char *ifname, const char *dev_str, >> + block_dev_desc_t **dev_desc) >> +{ >> + char *ep; >> + int dev; >> + > > Why don't you look up bootdevice here? That would be more consistent > behavior. bootdevice names a partition (or can name a partition), whereas this function is about retrieving a device handle and never a partition handle. I'm not sure it makes semantic sense to always fall back to bootdevice for commands that call get_device() directly. I'd far prefer people to always just pass the device they want to a command rather than relying implicitly on environment variables. If we did read bootdevice here, we'd end up having to read/parse it in both get_device() and get_device_and_partition(), here to extract just the device portion and in get_device_and_partition() to extract just the partition portion. And we'd have to make sure the code here only allowed the user to specify a partition /if/ this function was called from get_device_and_partition() and not if a command called it directly. That all seems a bit complex.
diff --git a/disk/part.c b/disk/part.c index 277a243..9920d48 100644 --- a/disk/part.c +++ b/disk/part.c @@ -435,6 +435,28 @@ void print_part (block_dev_desc_t * dev_desc) #endif +int get_device(const char *ifname, const char *dev_str, + block_dev_desc_t **dev_desc) +{ + char *ep; + int dev; + + dev = simple_strtoul(dev_str, &ep, 16); + if (*ep) { + printf("** Bad device specification %s %s **\n", + ifname, dev_str); + return -1; + } + + *dev_desc = get_dev(ifname, dev); + if (!(*dev_desc) || ((*dev_desc)->type == DEV_TYPE_UNKNOWN)) { + printf("** Bad device %s %s **\n", ifname, dev_str); + return -1; + } + + return dev; +} + #define MAX_SEARCH_PARTITIONS 16 int get_device_and_partition(const char *ifname, const char *dev_str, block_dev_desc_t **dev_desc, diff --git a/include/part.h b/include/part.h index a6d06f3..144df4e 100644 --- a/include/part.h +++ b/include/part.h @@ -112,6 +112,8 @@ int get_partition_info (block_dev_desc_t * dev_desc, int part, disk_partition_t void print_part (block_dev_desc_t *dev_desc); void init_part (block_dev_desc_t *dev_desc); void dev_print(block_dev_desc_t *dev_desc); +int get_device(const char *ifname, const char *dev_str, + block_dev_desc_t **dev_desc); int get_device_and_partition(const char *ifname, const char *dev_str, block_dev_desc_t **dev_desc, disk_partition_t *info); @@ -131,6 +133,9 @@ static inline int get_partition_info (block_dev_desc_t * dev_desc, int part, static inline void print_part (block_dev_desc_t *dev_desc) {} static inline void init_part (block_dev_desc_t *dev_desc) {} static inline void dev_print(block_dev_desc_t *dev_desc) {} +static inline int get_device(const char *ifname, const char *dev_str, + block_dev_desc_t **dev_desc) +{ return -1; } static inline int get_device_and_partition(const char *ifname, const char *dev_str, block_dev_desc_t **dev_desc,