search

mercanca / spart

spart: a user-oriented partition info command for slurm
GNU General Public License v2.0
19 stars 4 forks source link

Long partition info fields are truncated #10

Closed kcgthb closed 4 years ago

kcgthb commented 4 years ago

We have a number of partitions with quite long info fields. One example is the AllowGroups field, which in our most extreme case, has 173 comma-separated values, for a total length of 1276 characters.

The result is that for that partition, only the SPART_INFO_STRING_SIZE first characters are taken into account, and the result is an incomplete partition list for some of our users (as their groups are not found in the partition's AllowGroups field). The truncation happens here: https://github.com/mercanca/spart/blob/acfd56256cbe05a988f1989a816bab94d911ba9e/spart.c#L1415

A quick fix would be to increase the value of SPART_INFO_STRING_SIZE in: https://github.com/mercanca/spart/blob/acfd56256cbe05a988f1989a816bab94d911ba9e/spart.c#L69

I tested with a value of 2,048 without any noticeable problem. Happy to send a PR if that helps!

mercanca commented 4 years ago

For the security resaons, I wrote every string operations with a size limit. The SPART_INFO_STRING_SIZE macro defines general string size. Yes, for your situation, you can increase this value without worry. But, I think 173 values for Groups is not a general case. What do you think? May be, we can increase SPART_INFO_STRING_SIZE, little bit more. But in expense of the unnecessary memory usage for the some of the clusters.

Thank you for your issue report. The most difficult thing for me, is understanding (guessing) the setting of the other clusters. Your issue report helps me to understand little more.

kcgthb commented 4 years ago

Yes, I understand that each situation may be different, but it's probably difficult to come up with an arbitrary string length limit that doesn't truncate information is some cases.

I don't know if Slurm has a limit for the partition structure members, but if there's one, maybe the same one could be used for SPART_INFO_STRING_SIZE, to make sure no information is truncated?

mercanca commented 4 years ago

The slurm uses the dynamic memory allocation. But it is slow, complex, bug-prone. I have increased the string limit, and uploaded a new version of the spart (v1.2.2). Also, at the future, it will be good to add a check for the string size overflow, but I did not yet.

kcgthb commented 4 years ago

Great, thank you!