Re: [libvirt] [PATCH v2 1/3] conf: Properly truncate wide character names in virDomainObjGetShortName

On 08/25/2017 07:21 AM, Martin Kletzander wrote: Yeah - I tried - it got ugly fast.... Although I suppose in a test environment would could just "pick" a charset like en_US.UTF-8 and just ensure that things work as expected with that. Perhaps even make the shortened name tests go at the bottom with a FAILURE to compare argv results before setting the locale and a success after a call on the same test by name. Not something for this series because I'm not even sure it would work properly. Maybe something for the byte sized tasks (rather literally too ;-)!) If the domain was started before these patches, then the domain is still hidden since the shortened path names won't match... Such is life... consistently speaking return NULL; You could go with something like : virReportSystemError(errno, _("Cannot convert domain name to wide character string, len=%zd"), len); That way you'd know if you failed because len == -1 or the second mbstowcs call failed and you'd know why. Not sure ->name at this point could be considered an invalid argument, but getting the system error would certainly point you in the right direction. FWIW: There may be an off by 1 error. With your this, 1234567890123456789č would be shortened to 19 characters (it's at 21), thus if you have two domains: 1234567890123456789č and 1234567890123456789 They'd return the same answer Of course so would 123456789012345678901 and 1234567890123456789012, but at least for those two the length is > 20. Not sure if it matters or not as 19 or 20 characters for uniqueness is a lot. Still the bz had 7 multibyte characters that took up 21 char's - so it would seem things would be more limited with that charset. If that last character was the differentiator between all domains, then the short name is too short using only 18 chars that appear as 6 multibyte chars for the short name. Likewise a name using fully wide chars, e.g. ÄèÎØÛÄèÎØÛÄèÎØÛÄèÎØÛÄèÎØÛ would be shortened to ÄèÎØÛÄèÎØÛ. At this point I'm ambivalent as to whether that's considered a problem. Would returning NULL here elicit a useful error message? Given a name fully using a wide character set if this were altered to: if (len > VIR_DOMAIN_SHORT_NAME_MAX*2) len = VIR_DOMAIN_SHORT_NAME_MAX*2; Then ÄèÎØÛÄèÎØÛÄèÎØÛÄèÎØÛÄèÎØÛ shortens to ÄèÎØÛÄèÎØÛÄèÎØÛÄèÎØÛ, but 1234567890123456789č! doesn't get shortened to 1234567890123456789č. The multibyte charsets would need what * 3? It's somewhat ludicrous - but why I suggested originally to compare strlen() and mbstowcs() - if they're the same, less then 20, or having an error on the mbstowcs(), then just stick with the old logic. Only fall into the new logic when wide or multibyte chars are present. Of course the really harsh route is to count the width of each character in the array using mblen, then somehow mathematically get a result of 20 "printed characters" of uniqueness. Again, IDC really, but since I was looking and thinking about it I figured I'd at least mention it. So, I think the only required thing to fix would be the NULL return if len == -1 without an error message. Beyond that altering the return to be NULL instead of ret and altering to use virReportSystemError are may as well do it since you need to fix the first - with that done: Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John Whether the shortened name displayed "apparent" length rabbit hole needs adjustment - is up to you. I'm fine with it the way it is. I don't believe we ever documented our shortening "rules", but wide and multichar wide charsets really shorten things a lot. Perhaps that's a cross that bridge when someone complains type thing!