On Wed, Apr 23, 2014 at 09:57:38PM -0600, EBo wrote: > OK. The changes were a LOT more invasive than I expected, but > things mostly look ok and I would like to have them checked.
I apologize for not giving feedback on these patches a few months ago when your work was fresh. In general these look like good changes that are necessary for LP64 platforms like linux on x86_64/amd64 CPUs. It is necessary to use fixed-width types when the good old fashioned types can't be trusted to have expected widths. I would like to see this updated and submitted as a git patch or a github pull request. When you do this, please remember to include the "signed-off-by" line that grants us permission to incorporate your change into LinuxCNC (this is a recent change to our patch submission procedure). I am looking into it now because I am reviewing and testing some other long-languishing changes, the proposed fix in http://sourceforge.net/p/emc/bugs/328/ which seems to fix http://sourceforge.net/p/emc/bugs/395/ -- however, at least in the form offered in fix-emccommand-queue-fix1.patch I am almost certain it breaks NML-over-TCP worse than it was already broken. (and, it is somewhat fixed by this patch, according to the followups) I've rebased that patch onto our master development branch, you can find it on git.linuxcnc.org in the branch jepler/master/sf328. However, there's a third patch in http://sourceforge.net/p/emc/bugs/328/ which may help with the NML-over-TCP and so it would also be great if you can evaluate and test that patch. ... and ARGH while finalizing this message I just found in my mail archives that there's a third version of this patch that you offered a few days after this one. I'm going to send this review in its current form even though you may have addressed some of the issues in your newer patch. One other note. Seb Kuzminsky (back in April but in mail that was accidentally sent only privately to EBo, so I am happy to forward a copy to anyone interested in this old message) said the following: | If you're going to fix up these patches, I would suggest that you also | simplify the uint32_t accesses to those variables. Replace code like | this: | > status = (CMS_STATUS) ntohl(*((uint32_t *) temp_buffer + 1)); | | With code like this: | > status = (CMS_STATUS) ntohl(temp_buffer[1]); I disagree with this specific review suggestion, because it does two unrelated things in the same commit: * makes the functional change of long -> uint32_t * makes a non-functional change of *(x+y) -> x[y] I *like* both changes, but review is easier when they are separated into two steps. I talked to him (he's in person today at TXRX labs) about this and told him the same; I think he came around to my way of thinking. Any, on to my own review comments on your patch: > rcs_print > - ("NML_INTERP_LIST::append(nml_msg_ptr{size=%ld,type=%s}) : > list_size=%d, line_number=%d\n", > + ("NML_INTERP_LIST::append(nml_msg_ptr{size=%d,type=%s}) : > list_size=%d, line_number=%d\n", > nml_msg_ptr->size, emc_symbol_lookup(nml_msg_ptr->type), > linked_list_ptr->list_size, temp_node.line_number); I think that you changed 'size' from 'long' to 'int32_t'. If so, neither %d nor %ld is actually correct in C to print this nml_msg_ptr->size in this context. Instead, the preprocessor string constants from <inttypes.h> must be used. For instance, instead of long size; printf("size=%ld\n", size); you need int32_t i; printf("size=%"PRId32"\n", size); Online documentation on inttypes.h, which is part of the C99 standard, includes http://en.wikibooks.org/wiki/C_Programming/C_Reference/inttypes.h http://pubs.opengroup.org/onlinepubs/009695399/basedefs/inttypes.h.html > diff -ruN libnml.orig/buffer/physmem.cc libnml/buffer/physmem.cc > --- libnml.orig/buffer/physmem.cc 2014-04-23 21:48:19.267021754 -0500 > +++ libnml/buffer/physmem.cc 2014-04-23 22:17:42.362954657 -0500 > @@ -41,8 +41,8 @@ > > } > > -PHYSMEM_HANDLE::PHYSMEM_HANDLE(unsigned long _physical_address, > - long _address_code, long _size) > +PHYSMEM_HANDLE::PHYSMEM_HANDLE(uint32_t _physical_address, > + int32_t _address_code, int32_t _size) I'm not sure that the changes to physmem are correct. In PHYSMEM_HANDLE::PHYSMEM_HANDLE, _physical_address is copied to physical_address and later to local_address. local_address is cast back to a pointer type (char*) and then dereferenced in PHYSMEM_HANDLE::read On a 64-bit machine, addresses can be >2GB, so I don't think this change to int32_t is correct. The correct type may be: intptr_t, a type which is of the proper width to hold any integer value for the current platform. That assumes these values don't go "over the wire". If they do go "over the wire" (which seems like a weird thing to do with a 'physical memory address') then I guess the best choice would be int64_t, which is wide enough for pointers on both 32-bit and 64-bit platforms. (incidentally, these have inttypes.h formatting macros too, PRIdPTR and PRId64 if I am not mistaken) > - long offset; /* Operations read and write work use offset */ > - long size; > - int read(void *_to, long _read_size); /* Read _read_size bytes and > + int32_t offset; /* Operations read and write work use offset */ > + int32_t size; > + int read(void *_to, int32_t _read_size); /* Read _read_size bytes and > store */ Offsets and sizes probably should be int32_t, since all the buffers used in nml would need to be sized to fit on 32-bit platforms. i.e., this chunk seems fine to me. > - timeout_tv.tv_sec = (long) _timeout; > - timeout_tv.tv_usec = (long) (_timeout * 1000000.0); > + timeout_tv.tv_sec = (time_t) _timeout; > + timeout_tv.tv_usec = (time_t) (_timeout * 1000000.0); My manpage (I read 'man gettimeofday') says that the type of the "tv_usec" field is actually 'suseconds_t' http://pubs.opengroup.org/onlinepubs/7908799/xsh/systime.h.html > - ("CMS: couldn't create RCS_SHAREDMEM(%d(0x%X), %ld(0x%lX), > RCS_SHAREDMEM_NOCREATE).\n", > + ("CMS: couldn't create RCS_SHAREDMEM(%d(0x%X), %d(0x%X), > RCS_SHAREDMEM_NOCREATE).\n", > key, key, size, size); Use PRIx32 if it's an int32_t to be printed as hex > - putbe32(diag_info_buf, (uint32_t)serial_number); > + putbe32((char *)diag_info_buf, (uint32_t)serial_number); I do not understand why these casts of diag_info_buf are necessary, since the declaration for diag_info_buf I have is src/libnml/cms/cms_srv.hh: char set_diag_info_buf[0x400]; oh much later in the patch I saw that you changed the type of set_diag_info_buf, so this cast is necessary in light of that change. Overall is it an improvement to change the buffer type *and* require a cast (nearly?) everywhere it's used? > - tm.tv_sec = (long) timeout; > - tm.tv_sec = (long) (fmod(timeout, 1.0) * 1e6); > + tm.tv_sec = (int32_t) timeout; > + tm.tv_sec = (int32_t) (fmod(timeout, 1.0) * 1e6); Hmm here we have a good old fashioned bug (before your change)! note that the tv_sec field is being set twice and the tv_usec field not at all. Please fix. If your git expertise is up to it, please make the tv_sec -> tv_usec change a separate commit from the word-size fixes. If you want to get a little more in depth, it seems like we really need a function which takes a 'double timeout' in seconds and returns (or initializes via a pointer) a struct timeval. My eyes have slid across a lot of timeout initializations while reviewing this patch. > - char diag_info_buf[0x400]; > +// char diag_info_buf[0x400]; > + uint32_t diag_info_buf[0x100]; Prefer to delete old code, rather than commenting it out. I know this is a super handy way to operate while actively working on code, but by the time it's commited to git it's better to just erase the old code. > - long number = strtol(end_current_string, (char **) NULL, 10); > + int32_t number = strtol(end_current_string, (char **) NULL, 10); the mismatch "int32_t" vs "strto*L*" is unfortunate, but as there's no 'strtouint32' this is the best you can write. long is guaranteed to be at least 32 bits so there's no danger of failing to parse a value that fits inside int32_t. So this is OK, I just wanted to comment on it after having this thought process for myself. > -CMS_STATUS CMS_ASCII_UPDATER::update(long int &x) > +/******* > +CMS_STATUS CMS_ASCII_UPDATER::update(int32_t &x) > { ... > } > +*********/ Again, a final version should delete rather than commenting out these functions which are removed / no longer used. > -CMS_SERVER *CMS_SERVER_REMOTE_PORT::find_server(long _pid, long _tid /* =0 > +CMS_SERVER *CMS_SERVER_REMOTE_PORT::find_server(int32_t _pid, int32_t _tid > /* =0 ... > - rcs_print(" \t(%ld (0x%lX), %ld (0x%lX))\n", > + rcs_print(" \t(%d (0x%X), %d (0x%X))\n", pid_t is the integral type to use for process and thread ID, but there's no corresponding printf specifier for it. Your change is probably fine unless there are platforms where pid_t is bigger than int32_t. Or you could use pid32_t for the parameter, the printf format macro PRIiMAX, and a cast to (intmax_t) in the printf argument list. ugh. So all in all, this change is probably fine and you shouldn't do any of what I said in this paragraph. (but %d needs to be %"PRId32" like elsewhere) > diff -ruN libnml.orig/posemath/gotypes.h libnml/posemath/gotypes.h > --- libnml.orig/posemath/gotypes.h 2014-04-23 21:48:19.287021367 -0500 > +++ libnml/posemath/gotypes.h 2014-04-23 22:09:48.492110939 -0500 > @@ -122,7 +122,7 @@ > #endif > > #elif defined(GO_INTEGER_LONG) > -typedef long int go_integer; > +typedef int32_t go_integer; > #define GO_INTEGER go_integer_long > extern int go_integer_long; > #if defined(LONG_MAX) > @@ -130,7 +130,7 @@ > #endif > > #elif defined(GO_INTEGER_LONG_LONG) > -typedef long long int go_integer; > +typedef int64_t go_integer; > #define GO_INTEGER go_integer_long_long > extern int go_integer_long_long; > #if defined(LONG_MAX) Fixing integer size stuff in posemath is probably best as a separate commit. also, the 'before' declarations don't make much sense to me so I can't really comment on the fixes. > --- emc/task/emctaskmain.cc 2014-04-23 22:33:09.385042441 -0500 > +++ emc/task/emctaskmain.cc 2014-04-23 22:40:01.837072914 -0500 > @@ -343,7 +343,11 @@ > // convert string to argc/argv > argvize(s, buffer, argv, EMC_SYSTEM_CMD_LEN); > // drop any setuid privileges > - setuid(getuid()); > + if (!setuid(getuid())) { > + rcs_print("emcSystemCmd: setuid failed...\n" > + "emcSystemCmd: continuing anyway\n"); > + } > + > execvp(argv[0], argv); > // if we get here, we didn't exec > if (emc_debug & EMC_DEBUG_TASK_ISSUE) { > diff -ruN hal/simdrivers/uparport.h hal/simdrivers/uparport.h > --- hal/simdrivers/uparport.h 2014-04-23 22:33:09.485040512 -0500 > +++ hal/simdrivers/uparport.h 2014-04-23 22:37:43.329749197 -0500 > @@ -205,7 +205,11 @@ > rtapi_print_msg(RTAPI_MSG_ERR, > "PARPORT: dropping root permissions to uid %d\n", > getuid()); > - setuid(getuid()); > + if (!setuid(getuid())) { > + rtapi_print_msg(RTAPI_MSG_WARN, > + "PARPORT: setuid failed...\n" > + "PARPORT: continuing anyway.\n"); > + } > #endif > > #ifndef SIM Please put these unrelated changes in separate commits / pull requests. uparport.h is deleted in the latest master, and emctaskmain shuold simply have the setuid call deleted--we have never made task setuid in any emc2.x version, and this is such a weird misconfiguration that I don't know why we bother trying to deal with it. and both before and after the change, we don't actually warn about the weird thing (being setuid), but instead about a thing that is unlikely to fail (silently dropping setuid permissions). Jeff ------------------------------------------------------------------------------ Comprehensive Server Monitoring with Site24x7. Monitor 10 servers for $9/Month. Get alerted through email, SMS, voice calls or mobile push notifications. Take corrective actions from your mobile device. http://p.sf.net/sfu/Zoho _______________________________________________ Emc-developers mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/emc-developers
