[torquedev] new node attribute: "note"

Marcus R. Epperson mrepper at sandia.gov
Tue Feb 6 11:58:34 MST 2007


Thanks for the quick feedback, Garrick.  I finally had time to work on this again.  Responses below.

Garrick Staples wrote:
> Do we need to worry about ugly characters in the node_note file? Like
> newlines?  I think that will break things.

Yes, good point.  Is there anything other than newline that we need to worry about?  I can't think of anything else.

>> +/* Macros used to convert a #define'd value to a string literal */
>> +#define STR(x)   #x
>> +#define TEXT(x)  "" STR(x) ""
>> +
> 
> Will that be portable?  TORQUE is built on a very wide variety of OSes
> and compilers, we have to keep things pretty simple.
> 
> This might be perfectly safe, but I'm not good enough of a programmer to
> know :)

I guess I can't guarantee that it's portable so I've redone it the simple way.

>> +    sprintf(log_buffer,"Warning: Client attempted to set note with len (%d) > MAX_NOTE (%d)",
>> +      nsize,
>> +      MAX_NOTE);
> 
> Can we return this to pbsnodes?  I don't know off the top of my head if
> we can.

I can return this to pbsnodes by returning PBSE_BADNDATVAL internally:

  # pbsnodes -n $BIGNOTE node1
  Error setting note attribute for node node1 - Illegal value for

(that's the exact output)

I'm actually leaning more towards this approach for all questionable note cases, instead of trying to truncate, sanitize, etc.  I think it will be safer to just return an error and drop the bad note.  One issue with trying to sanitize is that the unsanitized note still ends up in the server logs.  This is capped at 4096 characters, but things like newlines make it through.  I don't think that's good.  When I reject a note, the contents of the note do not end up in the log.

>> +  /* initialize note attributes */
>> +  nin = fopen(path_nodenote,"r");
>> +
>> +  if (nin != NULL)
>> +    {
>> +
>> +    while (fscanf(nin,"%s %" TEXT(MAX_NOTE) "[^\n]",
>> +        line,
>> +        note) == 2)
>> +      {
>> +      for (i = 0;i < svr_totnodes;i++)
>> +        {
>> +        np = pbsndmast[i];
>> +
>> +        if (strcmp(np->nd_name,line) == 0)
> 
> Do we need to skip deleted nodes here?  I think nd_name could be null.

Fixed.

>> +void write_node_note()
[...]
> I know that node_status follows this same behaviour, but can it write to
> a tmp file and then atomically link it to node_note?  This will help
> save us from bad things like full disks.

Fixed.

-Marcus



More information about the torquedev mailing list