[Spice-devel] spice-xpi

[Peter Hatina]

On 03/15/2012 02:48 PM, Marc-André Lureau wrote:
> Peter,
> 
> To resume the discussion and be a bit more specific:
> 
>> +#if __GNUC__
>> +static inline std::string pretty_func_to_func_name(const std::string& f_name)
>> +{
>> +    std::string name(f_name);
>> +    std::string::size_type end_pos = f_name.find('(');
>> +    if (end_pos == std::string::npos)
>> +        return f_name;
>> +
>> +    std::string::size_type start = f_name.rfind(' ', end_pos);
>> +    if (start == std::string::npos)
>> +        return f_name;
>> +
>> +    end_pos -= ++start;
>> +    return name.substr(start, end_pos);
>> +}
>> +#  define FUNC_NAME pretty_func_to_func_name(__PRETTY_FUNCTION__).c_str()
>> +#else
>> +#  define FUNC_NAME __FUNCTION__
>> +#endif
> 
> why not use only __FUNCTION__?
>

Because, it does not format the string to meet this pattern for classes:
"class::method". What I get is only the method name, which is not enough
in this case.

>>> -        execl("/usr/libexec/spice-xpi-client", "/usr/libexec/spice-xpi-client", NULL);
>>> -        g_message("failed to run spice-xpi-client, running spicec instead");
>>> +        char *spice_xpi_client_args[] = {
>>> +            const_cast<char*>("/usr/libexec/spice-xpi-client"),
>>> +            NULL
>>> +        };
>>> +        std::stringstream ss;
>>> +        int cnt = sizeof(spice_xpi_client_args) / sizeof(spice_xpi_client_args[0]);
>>> +        for (int i = 0; i < cnt; ++i)
>>> +            ss << spice_xpi_client_args[i] << " ";
>>> +        LOG_MESSAGE("Launching " << ss.str());
>>> +        execv(spice_xpi_client_args[0], spice_xpi_client_args);
>>> +        LOG_MESSAGE("failed to run spice-xpi-client, running spicec instead");
> 
> This change is quite intrusive and ugly imho, especially because you
> do it at least twice (here, +more for usb elsewhere).
>

It depends. I wanted to easily dump the parameters, which are passed to
the process - I wanted to avoid copying the same thing twice.

> Can't you define a exec helper instead, using a ostream operator<< for
> logging argv would be a plus, for instance.
> 
>>  /* attribute string Title; */
>>  char *nsPluginInstance::GetTitle() const
>>  {
>> +    std::string title(m_title);
>> +    size_t found = -2;
>> +    while ((found = title.find("%", found + 2)) != std::string::npos)
>> +        title.replace(found, 1, "%%");
>> +    LOG_DEBUG(title);
>>      return stringCopy(m_title);
>>  }
> 
> Hmm, I think I understand why you did this, because we use C
> printf-like g_log functions in the end, but we can't add use
> LOG_DEBUG("%s", title) sadly.
>

Yes, exactly!

> Maybe the g_log functions should be called like g_debug("%s", msg)
> instead, since they are not doing the formating anyway?
> 
> thanks!
> 


-- 
Peter Hatina
EMEA ENG-Desktop Development
Red Hat Czech, Brno