> > + if (l<3) return 1;
>
> Same as above - newlines make things more readable.
Even for such siple ifs? Usually those are better readable this way, but OK, I'll change it.
> > + size = fread(description, 1, 255, f);
> > + if (size==0) {
> > + free(description);
> > + return NULL;
> > + }
> > +
> > + description[size-1]=0; /* strip the trailing newline */
>
> 1) "if (size == 0) {" please.
> 2) As I said in my first review of this code:
>
> 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. (a
> read/realloc loop also isn't out of the question...)
Size variable holds a real size of the string. Because fread reads 255 blocks 1B big and returns number of successfully read blocks.
Also realloc loop is not really neccessary here as we need this description only for our text interface and we can't show more than 50 or so characters without breaking the dialog layout.
> > +int globErrFunc(const char *epath, int eerrno)
> > +{
> > + /* TODO check fatal errors */
> > +
> > + return 0;
> > +}
>
> This still seems like the wrong thing to do. From my original
> review:
>
> Seems like the wrong thing to do - you're effectively telling glob()
> to ignore
> all errors and try to continue processing, but you're not doing
> anything about
> the errors while doing so. I think "return immediately" (i.e. NULL or
> a
> globErrFunc() that returns nonzero) makes more sense if we're not
> doing any
> actual handling of the errors.
I want to ignore all errors and skip the files except when we run out of memory. So I need to fill this errno check in, but ignoring the rest is exatly what I had in mind when I wrote it.
> > +
> > +int dlabelUnpackRPMDir(char* rpmdir, char* destination)
> > +{
> > + char *kernelver;
> > + struct utsname unamedata;
> > + char *oldcwd;
> > + char *globpattern;
> > + int rc;
> > +
> > + /* get current working directory */
> > + oldcwd = getcwd(NULL, 0);
> > +
> > + /* set the cwd to destination */
> > + if (chdir(destination)) {
> > + logMessage(ERROR, _("We weren't able to CWD to %s"),
> destination);
> > + free(oldcwd);
> > + return 1;
> > + }
>
> This shouldn't be translated. Also, the conditional is wrong. It'd
> probably
> also be better as something like:
>
The conditional is OK, -1 translates to true in C world. And successfull chdir returns 0, so this check will work exactly as intended.
> oldcwd = getcwd(NULL, 0);
> if (!oldcwd) {
> logMessage(ERROR, "getcwd() failed: %m");
> return 1;
> }
This should be added though.
> > + for (fnPtr = driverDiskFiles; *fnPtr; fnPtr++) {
> > + snprintf(file, 200, "%s/%s/%s", mntpt, getProductArch(),
> *fnPtr);
> > + if (access(file, R_OK)) {
> > + logMessage(ERROR, "cannot find %s, bad driver disk",
> file);
> > + return LOADER_BACK;
> > + }
> > + }
>
> Again, from my original review of this code:
>
> I realize mntpt and getProductArch's return are unlikely to be very
> large, but
> these should still probably be snprintf() or checked_asprintf() (...
> or sprintfa()
> if it were to suddenly come into existence ;)
But it IS snprintf..
Martin
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
