Hi John and David.
First of all: Sorry, I probably was or at least sounded way too grumpy;
I can only agree with the fact it was obviously broken *to me* because
that's a feature d-i uses and people noticed. That wasn't a reason to
be mad at people trying to fix bugs and to improve software. Sorry again
about that.
John Ogness <john.ogness@linutronix.de> (2014-03-09):
> I will look at how d-i is using ident and see what the problem is.
>
> > At least it reminds me that I have to find a way to make a testcase
> > which doesn't use --no-mount as this is of course hiding the issue…
> >
> > Sidenote: Why are you allowing apt-cdrom to do the mounting by itself
> > here if you have mounted it already and remount it after the run?
>
> While making the code changes, the handling of no-mount seemed odd to
> me, as if it was being used as some special case for some application
> and was not really a general-purpose feature. And it was different for
> "add" than it was for "ident". Now "ident" behaves like "add" which is
> probably the problem. I thought I was fixing buggy behavior for code
> that nobody used.
KiBi's tests:
-------------
So, I've learnt about/deployed debian-cd locally, and I'm now able to
generate tweaked ISO allowing for a patched apt to be tested within d-i.
There were two commits touching apt-cdrom:
[1] 62dcbf84c4aee8cb01e40c594d4c7f3a23b64836 apt-cdrom should succeed if any drive succeeds
[2] ce55512d4924b52c985276c62a0c69ac13e203cd remove duplication in pkgCdrom::Add and ::Ident
On top of current debian/sid, I verified that:
[1] applied + [2] applied = hang
[1] applied + [2] reverted = hang
[1] reverted + [2] reverted = success
so the hang is not a result of code deduplication (commit [2]).
I then concentrated on “instrumenting” the functions touched by [1], so
as to determine where the hang was taking place, namely DoAdd() and
DoIdent(); basically, DoAdd() works just fine, DoIdent() is then run and
that's the one hanging, apparently in the Ident() call. Adding some more
“cout << ... << endl; ” calls to Ident(), it finally appeared that we're
stuck in the ChangeCdrom() call at this point, which is presumably
waiting for user input. This is rather consistent given that strace
reports that the process is blocking on a read() on fd 0.
You'll find attached the patch I've used against the current debian/sid
branch, including patch [1], and excluding patch [2]; and d-i's syslog,
stripped down (limit to the apt-setup part).
FWIW: I'm happy to test any apt patch you can come up with, and to post
the results in a d-i environment.
Important note on interactivity:
--------------------------------
I've noticed the “apt-cdrom add” we have in apt-setup has a final
“< /dev/null” to explicitly disable interactivity. I've tried patching
all “apt-cdrom ident” calls to include that redirection as well, which
affects the following files:
generators/40cdrom
generators/41cdset
load-install-cd
I've generated an ISO with the updated udebs, and the default install
went through without any hangs. I've also verified half-way through
that packages available in the ISO were seen this way in /target by
apt, meaning only the missing packages were downloaded from the
configurer network mirror.
Initially, I wasn't too happy about patching apt-setup this late, but
the code deduplication commit points out that “apt-cdrom add” and
“apt-cdrom ident” are quite close, so maybe we should just release with
a patched apt-setup, disabling interactivity for all “apt-cdrom
{add,ident}” calls, and see how well it goes.
Do (apt/d-i) folks think this is crazy, or worth a try?
Mraw,
KiBi.
diff --git a/apt-pkg/cdrom.cc b/apt-pkg/cdrom.cc
index f577e35..f5b2d6b 100644
--- a/apt-pkg/cdrom.cc
+++ b/apt-pkg/cdrom.cc
@@ -565,54 +565,81 @@ bool pkgCdrom::Ident(string &ident, pkgCdromStatus *log) /*{{{*/
{
stringstream msg;
+ clog << "Ident: entering" << endl;
+
// Startup
string CDROM = _config->FindDir("Acquire::cdrom::mount");
if (CDROM[0] == '.')
CDROM= SafeGetCWD() + '/' + CDROM;
+ clog << "Ident: 1" << endl;
+
if (log != NULL)
{
msg.str("");
ioprintf(msg, _("Using CD-ROM mount point %s\nMounting CD-ROM\n"),
CDROM.c_str());
log->Update(msg.str());
+ clog << "Ident: 2" << endl;
}
+ clog << "Ident: 3" << endl;
+
// Unmount the CD and get the user to put in the one they want
if (_config->FindB("APT::CDROM::NoMount",false) == false)
{
+ clog << "Ident: 4" << endl;
if(log != NULL)
log->Update(_("Unmounting CD-ROM\n"), STEP_UNMOUNT);
+
+ clog << "Ident: 5" << endl;
+
UnmountCdrom(CDROM);
+ clog << "Ident: 6" << endl;
+
if(log != NULL)
{
+ clog << "Ident: 7" << endl;
log->Update(_("Waiting for disc...\n"), STEP_WAIT);
+ clog << "Ident: 8" << endl;
if(!log->ChangeCdrom()) {
+ clog << "Ident: 9" << endl;
// user aborted
return false;
}
}
// Mount the new CDROM
+ clog << "Ident: 10" << endl;
+
if(log != NULL)
log->Update(_("Mounting CD-ROM...\n"), STEP_MOUNT);
+ clog << "Ident: 11" << endl;
+
if (MountCdrom(CDROM) == false)
return _error->Error("Failed to mount the cdrom.");
}
+ clog << "Ident: 12" << endl;
+
// Hash the CD to get an ID
if (log != NULL)
log->Update(_("Identifying.. "));
+ clog << "Ident: 13" << endl;
+
if (IdentCdrom(CDROM,ident) == false)
{
+ clog << "Ident: 14" << endl;
ident = "";
return false;
}
+ clog << "Ident: 15" << endl;
+
if (log != NULL)
{
msg.str("");
@@ -620,31 +647,44 @@ bool pkgCdrom::Ident(string &ident, pkgCdromStatus *log) /*{{{*/
log->Update(msg.str());
}
+ clog << "Ident: 16" << endl;
+
// Read the database
Configuration Database;
string DFile = _config->FindFile("Dir::State::cdroms");
if (FileExists(DFile) == true)
{
+ clog << "Ident: 17" << endl;
if (ReadConfigFile(Database,DFile) == false)
return _error->Error("Unable to read the cdrom database %s",
DFile.c_str());
+ clog << "Ident: 18" << endl;
}
+ clog << "Ident: 19" << endl;
if (log != NULL)
{
+ clog << "Ident: 20" << endl;
msg.str("");
ioprintf(msg, _("Stored label: %s\n"),
Database.Find("CD::"+ident).c_str());
log->Update(msg.str());
+ clog << "Ident: 21" << endl;
}
+ clog << "Ident: 22" << endl;
+
// Unmount and finish
if (_config->FindB("APT::CDROM::NoMount",false) == false)
{
+ clog << "Ident: 23" << endl;
if (log != NULL)
log->Update(_("Unmounting CD-ROM...\n"), STEP_LAST);
+ clog << "Ident: 24" << endl;
UnmountCdrom(CDROM);
+ clog << "Ident: 25" << endl;
}
+ clog << "Ident: 26" << endl;
return true;
}
/*}}}*/
diff --git a/cmdline/apt-cdrom.cc b/cmdline/apt-cdrom.cc
index 20c6e88..858f698 100644
--- a/cmdline/apt-cdrom.cc
+++ b/cmdline/apt-cdrom.cc
@@ -166,7 +166,9 @@ bool DoAdd(CommandLine &)
unsigned int count = 0;
string AptMountPoint = _config->FindDir("Dir::Media::MountPath");
bool automounted = false;
- if (AutoDetect && UdevCdroms.Dlopen())
+ clog << "DoAdd: before if()." << endl;
+ if (AutoDetect && UdevCdroms.Dlopen()) {
+ clog << "DoAdd: Begin while loop" << endl;
while (AutoDetectCdrom(UdevCdroms, count, automounted)) {
if (count == 1) {
// begin loop with res false to detect any success using OR
@@ -186,15 +188,28 @@ bool DoAdd(CommandLine &)
if (_error->empty() == false)
_error->DumpErrors();
}
+ clog << "DoAdd: End while loop" << endl;
+ } else {
+ clog << "DoAdd: Else branch" << endl;
+ }
- if (count == 0)
+ if (count == 0) {
+ clog << "DoAdd: count == 0, so calling cdrom.Add()" << endl;
res = cdrom.Add(&log);
+ clog << "DoAdd: back from calling cdrom.Add()" << endl;
+ }
- if (res == false)
+ if (res == false) {
+ clog << "DoAdd: res == false, so calling error handling" << endl;
_error->Error("%s", _(W_NO_CDROM_FOUND));
- else
+ clog << "DoAdd: back from error handling" << endl;
+ }
+ else {
+ clog << "DoAdd: Should be good now." << endl;
cout << _("Repeat this process for the rest of the CDs in your set.") << endl;
+ }
+ clog << "DoAdd: return res " << res << endl;
return res;
}
/*}}}*/
@@ -214,7 +229,9 @@ bool DoIdent(CommandLine &)
unsigned int count = 0;
string AptMountPoint = _config->FindDir("Dir::Media::MountPath");
bool automounted = false;
- if (AutoDetect && UdevCdroms.Dlopen())
+ clog << "DoIdent: before if()" << endl;
+ if (AutoDetect && UdevCdroms.Dlopen()) {
+ clog << "DoIdent: before while()" << endl;
while (AutoDetectCdrom(UdevCdroms, count, automounted)) {
if (count == 1) {
// begin loop with res false to detect any success using OR
@@ -234,12 +251,23 @@ bool DoIdent(CommandLine &)
if (_error->empty() == false)
_error->DumpErrors();
}
+ } else {
+ clog << "DoIdent: else branch" << endl;
+ }
- if (count == 0)
+ if (count == 0) {
+ clog << "DoIdent: count == 0, so calling Ident" << endl;
res = cdrom.Ident(ident, &log);
+ clog << "DoIdent: back from ident" << endl;
+ }
- if (res == false)
+ if (res == false) {
+ clog << "DoIdent: res == false, calling error handling" << endl;
_error->Error("%s", _(W_NO_CDROM_FOUND));
+ clog << "DoIdent: back from error handling" << endl;
+ }
+
+ clog << "DoIdent: returning res " << res << endl;
return res;
}
Mar 13 03:05:15 anna[1885]: DEBUG: retrieving apt-setup-udeb 1:0.85 Mar 13 03:06:49 main-menu[256]: INFO: Menu item 'apt-setup-udeb' selected umount: can't umount /target/media/cdrom0: Invalid argument DoAdd: before if(). DoAdd: Else branch DoAdd: count == 0, so calling cdrom.Add() Using CD-ROM mount point /media/cdrom/ Unmounting CD-ROM Waiting for disc... Please insert a Disc in the drive and press enter Mounting CD-ROM... Identifying.. [93c8981bc0f34c2b6000e8c0d3bc6852-2] Scanning disc for index files.. Found 2 package indexes, 0 source indexes, 2 translation indexes and 0 signatures This disc is called: 'Debian GNU/Linux 8.0 alpha 1 _Jessie_ - Unofficial amd64 NETINST Binary-1 20140313-03:04' Copying package lists... ^MReading Package Indexes... 0%^M ^MReading Package Indexes... 1%^M ^MReading Package Indexes... Done^M ^MReading Translation Indexes... 0%^M ^MReading Translation Indexes... Done^M Writing new source list Source list entries for this disc are: deb cdrom:[Debian GNU/Linux 8.0 alpha 1 _Jessie_ - Unofficial amd64 NETINST Binary-1 20140313-03:04]/ jessie local main Unmounting CD-ROM... Repeat this process for the rest of the CDs in your set. DoAdd: back from calling cdrom.Add() DoAdd: Should be good now. DoAdd: return res 1 DoIdent: before if() DoIdent: else branch DoIdent: count == 0, so calling Ident Ident: entering Ident: 1 Ident: 2 Ident: 3 Ident: 4 Ident: 5 Ident: 6 Ident: 7 Ident: 8
Attachment:
signature.asc
Description: Digital signature