Hi, thanks for comments.
> "git config --global sendemail.chainreplyto no" might be a good thing
> to do ;)
Right, thanks :)
> On 12/02/2009 11:27 AM, Martin Sivak wrote:
>
> > +/* modprobe DD mode */
> > +int modprobeDDmode()
> > +{
> > + FILE *f = fopen("/etc/depmod.d/ddmode.conf", "w");
> > + if(f){
> > + struct utsname unamedata;
> > + if(uname(&unamedata)){
> > + fprintf(f, " pblacklist /lib/modules\n");
> > + }
> > + else{
> > + fprintf(f, " pblacklist /lib/modules/%s\n",
> unamedata.release);
> > + }
> > +
> > + fclose(f);
> > + }
>
> What's with the funky coding style? It doesn't match the rest of the
> file.
Well the code used to have more lines in the if/else clauses, but I can't see anything else...
> > +
> > + return f==NULL;
> > +}
> > +
> > +int modprobeNormalmode()
> > +{
> > + unlink("/etc/depmod.d/ddmode.conf");
> > +
> > + /* run depmod to refresh modules db */
> > + if(system("depmod -a")){
> > + /* FIXME: depmod didn't run */
> > + }
>
> Seems like there's nothing you can really do here except log it.
>
and return error code in case we want to check it in the loader.c
> > +
> > +/* RPM extraction dependency checks */
> > +int dlabelDeps(const char* depends, void *userptr)
> > +{
> > + logMessage(DEBUGLVL, "Depends on: %s\n", depends);
> > + return 0;
> > +}
>
> What's the point of this at all?
I wanted to have a complete set for dependency checks as I hope to sell this to RPM guys some day, so we do not have to maintain it ourselves. Right now, it is just an empty callback, which I can remove.
>
> > +
> > +int dlabelProvides(const char* dep, void *userptr)
> > +{
> > + char *kernelver = (char*)userptr;
> > +
> > + logMessage(DEBUGLVL, "Provides: %s\n", dep);
> > +
> > + return strcmp(dep, kernelver);
> > +}
>
> A better function name and some comments would be nice here. Upon
> reading it,
> I'm not at all sure what it's supposed to do.
I'm adding a comment. This checks if the RPM provides symbol name passed through userptr
> > +
> > +char* moduleDescription(const char* modulePath)
> > +{
> > + char *command = rstrscat(NULL, "modinfo --description '",
> modulePath, "'", NULL);
> > + FILE *f = popen(command, "r");
> > + if(f==NULL) return NULL;
> > +
> > + char *description = malloc(sizeof(char)*256);
> > + int size = fread(description, 1, 255, f);
>
> Couple of things here:
> 1) All the code/decleration mixing here isn't very easy to read.
Ok
> 2) allocations need to be checked
Ok
> 3) There's a pretty good case for using checked_asprintf() instead of
> using
> *anything* provided by rpmlib. Or implement a stack-allocating
> sprintfa(),
> which would be even better since it'll cut down on free()s in the
> return
> paths. You can borrow this code from nash in mkinitrd (git is at
> ssh://git.fedoraproject.org/git/hosted/mkinitrd) if you like. I
> think I
> called it asprintfa() there, but whatever.
I'll take a look
> > + if(size==0){
> > + free(description);
> > + return NULL;
> > + }
> > + description[size-1]=0; /* strip the trailing newline */
>
> I'm pretty sure this should be using strrchr() to find the newline
> rather than
> expecting all description strings to be exactly 254 bytes plus a
> newline.
size contains number of actually read blocks, block size is set to 1, number of blocks to max 255. So [size-1] is the last character and it doesn't have to be the 255th. And honestly, I do not care about shortening the description to max 255 chars, since we can't show that in the interface anyways..
> > + pclose(f);
> > +
> > + free(command);
>
> Why not free this immediately after the popen?
>
You found a possible memory leak, thanks.
> > + return description;
> > +}
> > +
> > +int globErrFunc(const char *epath, int eerrno)
> > +{
> > + return 0;
> > +}
>
> Seems like the wrong thing to do - you're effectively telling glob()
> to ignore
> all errors and try to continue processing
Actually, that is what I want, I want to copy/see everything possible. If one file fails, the rest is still usable.
I might add a check for the fatal errors though...
> rc = uname(&unamedata);
> checked_asprintf(&kernelver, "kernel-modules-%s", rc ? "unknown" :
> unamedata.release);
>
That looks much better, thanks
> I'm really not a big fan of hiding the format specifier behind a
> macro, but
> I do see the case for re-use here. Given that, it probably ought to
> be
> defined somewhere very near to this code, not in a separate header
> file.
>
> Also maybe name it something like DD_RPMDIR_TEMPLATE ?
TEMPLATE in the name seems like good idea.
>
> Seems like using glib's linked lists is better than adding our own.
>
Yep, it was backport from rhel5 code. I'll change it (I was thinking about this too, but I have forgotten to do it)
> > + if(!strncmp(getProductName(), "Red Hat", 7)){
> > + flags |= LOADER_FLAGS_AUTOMODDISK;
> > + }
> > +
>
> You probably ought to split this checking out to its own function
> somewhere.
>
What I would like to do is to have some "flags" in the treeinfo file, which would control this. But that is a topic for a longer separate discussion.
Martin
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
