[PATCH Try#3 3/3] Major improvements to RPM
Denis Vlasenko
vda.linux at googlemail.com
Tue Jun 12 23:50:50 UTC 2007
On Sunday 10 June 2007 18:55, andy at warmcat.com wrote:
> Adds many new features to RPM including a 'database' of installed
> packages down /var/lib/rpm. See the comments in rpm.c and the
> usage help for more information.
>
> Try #3
>
> - moved rpm.h back into rpm.c (DV)
> - removed repeated includes already in libbb.h (DV)
> - use shifts to define rpm_functions_e (DV)
> - szaRelationship pointers themselves to be const (DV)
> - restrict rpm.c local functions to static (DV)
> - migrate to xlseek() (DV)
> - assignments in if() evil (DV)
> - move to bb_perror_msg_and_die (DV)
> - open -> xopen (DV)
> - write -> xwrite everywhere (DV)
> - read -> xread where return unchecked
> - bb_*_and_die() to have lower case descriptive text at start (DV)
> - xrealloc_getcwd_or_warn (DV)
> - broke out some very indented code into fuctions (DV)
>
>
> Signed-off-by: Andy Green <andy at warmcat.com>
Thanks. Review follows. You can submit patch as single gzipped
diff, it's not mandatory to split it into three files
and also gzipped file will fit under mailing list size limit.
- bb_perror_msg_and_die("cannot remove old file");
+#ifndef CONFIG_CPIO_NONFATAL_UNPACK_ERRORS
+ bb_perror_msg_and_die("Couldnt remove old file %s",
+ file_header->name);
+#else
+ bb_perror_msg("Couldnt remove old file %s",
+ file_header->name);
+ data_skip(archive_handle);
+ return;
+#endif
Use #if [!]ENABLE_CPIO_NONFATAL_UNPACK_ERRORS, it is typo-resistant
(gcc will warn).
Why "Couldnt"? It's longer. It is wrongly capitalized.
data_extract_all() isn't a cpio internal thing. it is used by tar
and ar too. Why *CPIO*_NONFATAL_UNPACK_ERRORS? Did you review what
will happen in tar/ar if data_extract_all() will return with error?
They cannot even *know* there was an error, as it returns void...
Nasty situation! :(
+typedef enum { HASH_SHA1, HASH_MD5 } hash_algo_t;
+extern uint8_t *hash_file(const char *filename, hash_algo_t hash_algo);
Should be in libbb.h
+#define RPMTAG_PACKAGER 1015
+#define RPMTAG_GROUP 1016
+#define RPMTAG_URL 1020
+// pre/post IN are pre- and post-install scripts
+#define RPMTAG_PREIN 1023
+#define RPMTAG_POSTIN 1024
+// pre/post UN are pre- and post-uninstall scripts
+#define RPMTAG_PREUN 1025
+#define RPMTAG_POSTUN 1026
+#define RPMTAG_FILEMD5S 1035 /* s[] */
+#define RPMTAG_FILEFLAGS 1037
+#define RPMTAG_FILEUSERNAME 1039
Here you sometomes use tabs, somethimes spaces.
Use spaces uniformly. Generally tabs are to be used for
initial whitespace (indent) at the start of the line,
not in the middle.
+ RPMSENSE_ANY = 0,
+ /* @-enummemuse@ */
+ RPMSENSE_SERIAL = (1 << 0), /* !< @todo Legacy. */
+ /* @=enummemuse@ */
+ RPMSENSE_LESS = (1 << 1),
+ RPMSENSE_GREATER = (1 << 2),
+ RPMSENSE_EQUAL = (1 << 3),
+ RPMSENSE_PROVIDES = (1 << 4), /* only used internally by builds */
+ RPMSENSE_CONFLICTS = (1 << 5), /* only used internally by builds */
+ /* bit 6 used to be RPMSENSE_PREREQ */
+#define RPMSENSE_PREREQ RPMSENSE_ANY
Well, you can make RPMSENSE_PREREQ enum as well.
+ RPMSENSE_OBSOLETES = (1 << 7), /* only used internally by builds */
+ RPMSENSE_INTERP =
+ (1 << 8), /* !< Interpreter used by scriptlet. */
Maybe this way?
/* Interpreter used by scriptlet. */
RPMSENSE_INTERP = (1 << 8),
+void
+rpm_construct_scan_state(rpm_scan_state * prss) {
+ prss->m_fd = -1;
+ prss->m_tags = NULL;
+}
+void rpm_destruct_scan_state(rpm_scan_state * prss) {
Shouldn't they be static? Why different style of placement
of return type? Pick one (we have no policy which, just use the same thru
the entire source file.)
Function's opening '{' should be on next line, like this one:
+static int
+rpm_compare_versions(const char * sz1, const char * sz2)
{
+ if ((*psza[0] != '\0') && (*psza[1] == '\0'))
+ /* second ran out but first has more */
+ return RPMSENSE_GREATER;
Not a problem, but don't you think psza[n][0] is easier to
understand ("aha, first char of the n'th string") than *psza[n]?
+ if (!prss->m_tags) {
+ bb_perror_msg_and_die("error reading rpm header\n");
+ exit(-1);
+ }
die() will die. Trust me.
+ char * szFilepathTemp = tempnam("/tmp", "rpm_");
+ int fd = xopen3(szFilepathTemp, O_CREAT|O_TRUNC|O_WRONLY, 0700);
+ int nret;
+
+ xwrite(fd, sz, strlen(sz));
+ close(fd);
+
+ szRunCommand = xmalloc(strlen(szFilepathTemp) + 9);
+ strcpy(szRunCommand, "/bin/sh ");
+ strcpy(szRunCommand+8, szFilepathTemp);
+
+/*
+ * printf("(Running %s script %s)\n", ScriptTagToName(nTagType),
+ * szFilepathTemp);
+ */
+
+ nret = system(szRunCommand);
+
+ free(szRunCommand);
+ unlink(szFilepathTemp);
+
Why? Maybe just nret = wait4pid(spawn(argv)) with suitable argv?
+ while ((read(fdTemp, &dep, sizeof(dep)) != 0) &&
+ (!fSeen)) {
+
+ if (strcmp(dep.m_szPackage, sz) != 0)
+ continue;
What happens on read errors here?
+#ifdef RPM_DEBUG
+ printf("RPM_DEBUG: OLD Post-uninstall script completed\n");
+#endif
You use RPM_DEBUG only for prints. You can conditionally #define DEBUG_PRINTF()
and use that. Will reduce #ifdef forest.
+ pszFilename = xmalloc(strlen(pszDatabaseDirpath) +
+ strlen(pdirentry->d_name) + 2);
+ strcpy(pszFilename, pszDatabaseDirpath); /* first the eff root dir */
+ strcat(pszFilename, pdirentry->d_name); /* then the directory name */
pszFilename = xasprintf("%s%s", pszDatabaseDirpath, pdirentry->d_name);
(you have many such places)
#define rpm_full_usage \
- "Manipulate RPM packages" \
- "\n\nOptions:" \
- "\n -i Install package" \
- "\n -q Query package" \
- "\n -p Query uninstalled package" \
- "\n -i Show information" \
- "\n -l List contents" \
- "\n -d List documents" \
- "\n -c List config files"
+ "Manipulates RPM packages" \
+ "\n\nOptions:" \
+ "\n\t-r <path> Set effective root to <path>" \
+ "\n\t-i pkgname.rpm Install package" \
+ "\n\t-e pkgname Erase (uninstall) package" \
+ "\n\t-U pkgname.rpm Update (or install) package" \
+ "\n\t-u -o pkgname.rpm Upgrade of newer package by older package" \
+ "\n\t-F pkgname.rpm Freshen package" \
+ "\n\t-V pkgname Verify installation against package files" \
+ "\n\t-q -a Query all installed packages" \
+ "\n\t-qi pkgname Show information" \
+ "\n\t-qip pkgname.rpm Show information" \
+ "\n\t-ql pkgname List contents" \
+ "\n\t-qlp pkgname.rpm List contents" \
+ "\n\t-q -C vers pkgname Compare versions"
Please use real tabs, not \t's, and use tabs instead of spaces.
These strings go into executable, have to keep them small:
"\n__TAB___-qi pkgname_TAB_Show information" \
--
vda
More information about the busybox
mailing list