[pacman-dev] [PATCH 1/2] Make sync DB reading a bit more flexible
We can reorganize things a bit to not require reading a directory-only entry
first (or at all). This was noticed while working on some pactest
improvements, but should be a good step forward anyway.
Also make _alpm_splitname() a bit more generic in where it stores the data it
parses.
Signed-off-by: Dan McGee
Discovered this when doing some pactest rewrite work to generate
archives in memory only. If a sync database file or PKGINFO file is
missing a newline on the final line, the text from that line gets tossed
aside and never read into the package struct. This is pretty critical
when that last line is a depend or something.
Signed-off-by: Dan McGee
On 24/06/11 15:18, Dan McGee wrote:
We can reorganize things a bit to not require reading a directory-only entry first (or at all). This was noticed while working on some pactest improvements, but should be a good step forward anyway.
Also make _alpm_splitname() a bit more generic in where it stores the data it parses.
Signed-off-by: Dan McGee
Signed-off-by: Allan with minor query below.
--- lib/libalpm/be_local.c | 3 +- lib/libalpm/be_sync.c | 130 ++++++++++++++++++++++++----------------------- lib/libalpm/util.c | 46 ++++++++++------- lib/libalpm/util.h | 3 +- 4 files changed, 97 insertions(+), 85 deletions(-)
<snip> All the be_sync.c stuff looks fine to me.
diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index 4976703..e56c9f2 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -825,46 +825,54 @@ cleanup: } }
-int _alpm_splitname(const char *target, pmpkg_t *pkg) +int _alpm_splitname(const char *target, char **name, char **version, + unsigned long *name_hash) { /* the format of a db entry is as follows: * package-version-rel/ + * package-version-rel/desc (we ignore the filename portion) * package name can contain hyphens, so parse from the back- go back * two hyphens and we have split the version from the name. */ - const char *version, *end; + const char *pkgver, *end;
- if(target == NULL || pkg == NULL) { + if(target == NULL) { return -1; }
Should we check name/version/name_hash here for being NULL and abort similar to what was done for pkg previously? Or could it be of use for this function to be called with those as NULL?
On 24/06/11 15:18, Dan McGee wrote:
Discovered this when doing some pactest rewrite work to generate archives in memory only. If a sync database file or PKGINFO file is missing a newline on the final line, the text from that line gets tossed aside and never read into the package struct. This is pretty critical when that last line is a depend or something.
Signed-off-by: Dan McGee
Signed-off-by: Allan
On Mon, Jun 27, 2011 at 4:36 AM, Allan McRaewrote: > On 24/06/11 15:18, Dan McGee wrote: >> >> We can reorganize things a bit to not require reading a directory-only >> entry >> first (or at all). This was noticed while working on some pactest >> improvements, but should be a good step forward anyway. >> >> Also make _alpm_splitname() a bit more generic in where it stores the data >> it >> parses. >> >> Signed-off-by: Dan McGee > > Signed-off-by: Allan with minor query below. > >> --- >> lib/libalpm/be_local.c | 3 +- >> lib/libalpm/be_sync.c | 130 >> ++++++++++++++++++++++++----------------------- >> lib/libalpm/util.c | 46 ++++++++++------- >> lib/libalpm/util.h | 3 +- >> 4 files changed, 97 insertions(+), 85 deletions(-) >> > > > All the be_sync.c stuff looks fine to me. > >> >> diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c >> index 4976703..e56c9f2 100644 >> --- a/lib/libalpm/util.c >> +++ b/lib/libalpm/util.c >> @@ -825,46 +825,54 @@ cleanup: >> } >> } >> >> -int _alpm_splitname(const char *target, pmpkg_t *pkg) >> +int _alpm_splitname(const char *target, char **name, char **version, >> + unsigned long *name_hash) >> { >> /* the format of a db entry is as follows: >> * package-version-rel/ >> + * package-version-rel/desc (we ignore the filename portion) >> * package name can contain hyphens, so parse from the back- go >> back >> * two hyphens and we have split the version from the name. >> */ >> - const char *version, *end; >> + const char *pkgver, *end; >> >> - if(target == NULL || pkg == NULL) { >> + if(target == NULL) { >> return -1; >> } > > Should we check name/version/name_hash here for being NULL and abort similar > to what was done for pkg previously? Or could it be of use for this > function to be called with those as NULL? I didn't null check here because 1) Look later in the function; we only set version and name if they are non-NULL 2) in reality, the current two callers guarantee non-NULL for those arguments, so even that might be overkill. -Dan
On 27/06/11 22:04, Dan McGee wrote: > On Mon, Jun 27, 2011 at 4:36 AM, Allan McRaewrote: >> On 24/06/11 15:18, Dan McGee wrote: >>> >>> We can reorganize things a bit to not require reading a directory-only >>> entry >>> first (or at all). This was noticed while working on some pactest >>> improvements, but should be a good step forward anyway. >>> >>> Also make _alpm_splitname() a bit more generic in where it stores the data >>> it >>> parses. >>> >>> Signed-off-by: Dan McGee >> >> Signed-off-by: Allan with minor query below. >> >>> --- >>> lib/libalpm/be_local.c | 3 +- >>> lib/libalpm/be_sync.c | 130 >>> ++++++++++++++++++++++++----------------------- >>> lib/libalpm/util.c | 46 ++++++++++------- >>> lib/libalpm/util.h | 3 +- >>> 4 files changed, 97 insertions(+), 85 deletions(-) >>> >> >> >> All the be_sync.c stuff looks fine to me. >> >>> >>> diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c >>> index 4976703..e56c9f2 100644 >>> --- a/lib/libalpm/util.c >>> +++ b/lib/libalpm/util.c >>> @@ -825,46 +825,54 @@ cleanup: >>> } >>> } >>> >>> -int _alpm_splitname(const char *target, pmpkg_t *pkg) >>> +int _alpm_splitname(const char *target, char **name, char **version, >>> + unsigned long *name_hash) >>> { >>> /* the format of a db entry is as follows: >>> * package-version-rel/ >>> + * package-version-rel/desc (we ignore the filename portion) >>> * package name can contain hyphens, so parse from the back- go >>> back >>> * two hyphens and we have split the version from the name. >>> */ >>> - const char *version, *end; >>> + const char *pkgver, *end; >>> >>> - if(target == NULL || pkg == NULL) { >>> + if(target == NULL) { >>> return -1; >>> } >> >> Should we check name/version/name_hash here for being NULL and abort similar >> to what was done for pkg previously? Or could it be of use for this >> function to be called with those as NULL? > I didn't null check here because > 1) Look later in the function; we only set version and name if they are non-NULL > 2) in reality, the current two callers guarantee non-NULL for those > arguments, so even that might be overkill. Sure. If they were checks for NULL there, then checks later in the function could obviously be removed. But I suppose that one day we might like to use this just to get the name so version and name_hash could be passed as NULL. So ack from me. BTW, the patch needed a minor rebase to apply to the current head. That is done on my working branch. Allan
participants (3)
-
Allan McRae
-
Dan McGee
-
Dan McGee