Messages in this thread | | | Date | Wed, 30 Sep 2015 09:33:38 +0200 | From | Greg KH <> | Subject | Re: [PATCH] ver_linux: uniform output across various linux distros |
| |
On Tue, Sep 29, 2015 at 09:08:20PM +0300, Alexander Kapshuk wrote: > On Tue, Sep 29, 2015 at 7:43 PM, Greg KH <gregkh@linuxfoundation.org> wrote: > > On Tue, Sep 29, 2015 at 06:36:12PM +0300, Alexander Kapshuk wrote: > >> I've reread 'Documentation/SubmittingPatches', and opted for method 1 > >> to create the patch. > >> > >> When you mentioned about the patch changing the formatting, were you > >> referring to the formatting of the actual script, or the output the > >> script generated? If it's the former, I'm not sure I'm clear on how to > >> keep separate the changes made to the code base from the changes in > >> the formatting of the new code base introduced by these changes. If > >> it's the latter, the overall output generated by the script is mostly > >> identical to that generated by the original implementation. > >> > >> If the patch below is still off the mark, I apologise for that, and > >> would appreciate being pointed out what else needs changing, when you > >> have the time. I understand you having more pressing matters to attend > >> to. > >> > >> Signed-off-by: Alexander Kapshuk <alexander.kapshuk@gmail.com> > > > > This is the "oddest" changelog comments I've ever seen for a patch, it > > doesn't describe what it is doing at all :) > > > > Please fix up, and resend the whole thing correctly, so we can review > > that. > > > > thanks, > > > > greg k-h > > > O, boy. Will I ever get this right :-( > Sorry for being such a nuisance. > Here we go...
Heh, in the future, all of this above here needs to be removed, it wouldn't look good in a changelog, right?
But anyway, comments below on your patch:
> Having run 'scripts/ver_linux' on my Gentoo system, as well as having > looked through some recent bug reports on the kernel bugzilla website > showing the output of the script in question, I have observed that > the output is not accurate across various distros. While the current > implementation of the script expects version numbers to be found > in particular fields, some of the utilities invoked by the script, > output their version information in varying formats, which results in > the script displaying information other than the version number. > > The proposed implementation relies mostly on sed to detect the version > numbers more accurately in a more general way. > > Running the patched version of the script on the distros below resulted > in accurate and uniform output. > > Gentoo Linux > Debian 6.0.10 > Oracle Linux Server release 7.1 > Arch Linux > openSuSE 13.2 > > The items left unchanged are those I did not have access to. I would > be willing to work on those too, if supplied the output of the affected > commands whose format differs based on the distro. > > ------------------------------------- > A few comments are in order. > > /usr/src/linux/scripts/ver_linux.orig:22,26 > The current implementation outputs the version of 'util-linux' by > calling 'fdformat', which had not been found installed on most of the > test systems available to me. As 'mount' is a part of the 'util-linux' > package, the proposed implementation invokes 'mount' only, and unifies > the lines above into a single block. > > /usr/src/linux/scripts/ver_linux.orig:77,83 > 'loadkeys -h' does not output the versionning number across all the > test systems available to me, while 'loadkeys -V' does. The proposed > implementation calls 'loadkeys -V' only once, and outputs the versioning > info acquired for both 'Kdb' and 'console-tools'. > > /usr/src/linux/scripts/ver_linux.orig:90 > None of the test systems I have access to seem to have 'udevinfo' > installed. Perhaps, 'udevinfo' is the former name for something that > could now be substituted by 'udevadm --version'. > ------------------------------------- > Sample output of the current implementation on Gentoo Linux: > > If some fields are empty or look unusual you may have an old version. > Compare to the current minimal requirements in Documentation/Changes. > > Linux box0 4.2.1-vanilla #1 SMP Thu Sep 24 18:05:42 EEST 2015 i686 > Intel(R) Pentium(R) Dual CPU T3400 @ 2.16GHz GenuineIntel GNU/Linux > > Gnu C 4.8.5 > Gnu make 4.1 > binutils 2.25.1 > 1.1 > 2.25.1 > util-linux /usr/src/linux/scripts/ver_linux.orig: line 23: > fdformat: command not found > mount debug > module-init-tools found > Linux C Library Dynamic linker (ldd) 2.20 > Procps 3.3.9 > Net-tools 1.60_p20130513023548 > Kbd 1.15.5 > Sh-utils 8.23 > Modules Loaded xt_mark xt_LOG vgem v4l2_dv_timings ulpi udf > sil164 nf_nat_sip nf_nat_irc uvcvideo videobuf2_vmalloc > videobuf2_memops nf_nat_ipv4 videobuf2_core v4l2_common nf_nat_ftp > nf_nat nf_log_ipv6 nf_log_ipv4 nf_log_common lcd i2c_mux hp_wireless > gspca_main videodev cuse fuse crc_itu_t ch7006 ath5k coretemp > input_leds led_class ath > ------------------------------------- > Sample output of the patched implementation on Gentoo Linux: > > If some fields are empty or look unusual you may have an old version. > Compare to the current minimal requirements in Documentation/Changes. > > Linux box1 4.0.5-gentoo #5 SMP Fri Sep 4 18:11:11 EEST 2015 i686 > Intel(R) Core(TM)2 Duo CPU E4600 @ 2.40GHz GenuineIntel GNU/Linux > > GNU C 4.8.5 > GNU Make 4.1 > binutils 2.25.1 > util-linux 2.26.2 > mount 2.26.2 > module-init-tools 20 > e2fsprogs 1.42.13 > Linux C Library 2.20 > Dynamic linker (ldd) 2.20 > Linux C++ Library 6.0.19 > Procps 3.3.9 > net-tools 1.60 > Kbd 1.15.5 > Console-tools 1.15.5 > Sh-utils 8.23 > Modules Loaded ch7006 coretemp crc_itu_t fuse gspca_main > hwmon_vid i2c_mux it87 mii nf_log_common nf_log_ipv4 nf_log_ipv6 > nf_nat nf_nat_ftp nf_nat_ipv4 nf_nat_irc nf_nat_sip r8169 sil164 > snd_aloop udf uvcvideo v4l2_common v4l2_dv_timings videobuf2_core > videobuf2_memops videobuf2_vmalloc videodev xt_LOG xt_mark > ------------------------------------- > The actual patch. > -------------------------------------
Why is this here?
> Signed-off-by: Alexander Kapshuk <alexander.kapshuk@gmail.com> > --- > --- linux/scripts/ver_linux.orig 2015-04-13 01:12:50.000000000 +0300 > +++ linux/scripts/ver_linux 2015-09-29 20:47:48.664509177 +0300 > @@ -4,54 +4,112 @@ > # /bin /sbin /usr/bin /usr/sbin /usr/local/bin, but it may > # differ on your system. > # > + > echo 'If some fields are empty or look unusual you may have an old version.' > echo 'Compare to the current minimal requirements in Documentation/Changes.' > -echo ' ' > +echo > > uname -a > -echo ' '
You do a lot of different things in this patch, making it almost impossible to review. Please break them all up into individual changes, each patch only doing one thing (like changing "echo ' '", cleaning up argument lists, etc.) and resend this as a patch series that we can review and hopefully apply to the tree.
thanks,
greg k-h
| |