Created attachment 150421 [details] [review]
add per-algorithm default values

Hi nobled.
I have review your patch and looks good for me, but let's wait to see what Adam says.

Anyways, I have also seen other key sizes hardcoded in ssh/seahorse-ssh-generate.c and I think we could do the same. Could you also have a look at them? :)

Thanks for the patch.

Alright. I checked, and for SSH it's a bit different: the minimum/maximum for key sizes is hardcoded in ssh/seahorse-ssh-generate.xml#adjustment1, as well as in C. I don't know anything about Gtk's XML format; is there a way to make it use #defines from C code?

Here's my preliminary patch; I'd rather fix the XML file issue before committing though.

Created attachment 153990 [details] [review]
ssh key length logic cleanup

Created attachment 153991 [details] [review]
enforce ssh key length limits

Never mind--checking how it's done for PGP, this new version of the patch should do the same job without touching the XML.

Review of attachment 153991 [details] [review]:

Could you please add a g_message similar to in the first patch indicating the default length is being used?  Also, if the spinner is set correctly now, shouldn't the test be for bits < algo_min instead of bits == 0?

Review of attachment 150421 [details] [review]:

Looks good to commit to me.

Created attachment 153999 [details] [review]
enforce ssh key length limits (v2)

(In reply to comment #6)
> Review of attachment 153991 [details] [review]:
> 
> Could you please add a g_message similar to in the first patch indicating the
> default length is being used?  Also, if the spinner is set correctly now,
> shouldn't the test be for bits < algo_min instead of bits == 0?

There's already a size check and g_message like that in on_response() in the patch--are you talking about the change to seahorse_ssh_operation_generate()?

That function seems to take bits==0 as a special case for "use the default key length" and never did any other checks (the caller checks the length already). So I just made that case work for DSA too.

If there was supposed to be a check for valid lengths in that function all along, it should just throw an error and fail like seahorse_gpgme_key_op_generate does, right? (edited patch)

Review of attachment 150421 [details] [review]:

Won't compile, change "idx" by "sel", and it will be ready.
I would also cleanup max/min/default in the xml since it's not needed anymore (they are overwritten) and can confuse new people.

Review of attachment 153999 [details] [review]:

Looks good for me but fix first a compile warning and whitespaces:

961	    if (type == SSH_ALGO_DSA && bits != DSA_SIZE ||
962	        type == SSH_ALGO_RSA && (bits < MIN_RSA_SIZE || bits > _OPENSSH_MAX_RSA_SIZE)) {
963	        g_warning ("can't generate ssh-%s key: invalid key size %u", algo, bits);
964	        return NULL;
965	    }

seahorse-ssh-operation.c: In function ‘seahorse_ssh_operation_generate’:
seahorse-ssh-operation.c:962: warning: suggest parentheses around ‘&&’ within ‘||’


whitespaces:

attachment.cgi?id=153999:41: trailing whitespace.
        gtk_spin_button_set_range (GTK_SPIN_BUTTON (widget), 
attachment.cgi?id=153999:42: trailing whitespace.
                                   MIN_RSA_SIZE, 
attachment.cgi?id=153999:84: trailing whitespace.
    
attachment.cgi?id=153999:90: trailing whitespace.
    
warning: 4 lines add whitespace errors.

Created attachment 154029 [details] [review]
add per-algorithm default values (v2)

Fixed; I renamed the other variable for consistency's sake.

Created attachment 154032 [details] [review]
enforce ssh key length limits (v3)

Okay, I fixed the compiler warning and the more pointless trailing whitespace. The two new blank lines are still indented though, since they all are.

Is there a site where the XML format is documented? I'm not sure how much of it needs editing for the cleanup; like if bits-entry needs a replacement value for its adjustment property.

Review of attachment 154029 [details] [review]:

In general, I like this.  Please extend it to subkey generation as well.  I just committed a stop gap to set the default key length for subkeys to half of the max or max in the case of DSA (I didn't take into account the max could be 3072).

One thing that needs to be fixed is:

{ N_("DSA (sign only)"), DSA,         DSA_MIN,     DSA_MAX,    LENGTH_DEFAULT  }

Note that LENGTH_DEFAULT = 2048 > DSA_MAX if GPG version <1.4.10 or < 2.0.12.

Review of attachment 154032 [details] [review]:

I don't think the extra checking in on_response is necessary at all (including the original) as the spinners are set correctly.   That should be safe to remove as the only entry point into on_response is when the response signal is fired.

Created attachment 154357 [details] [review]
add per-algorithm default values (v3)

(In reply to comment #13)
> One thing that needs to be fixed is:
> 
> { N_("DSA (sign only)"), DSA,         DSA_MIN,     DSA_MAX,    LENGTH_DEFAULT 
> }
> 
> Note that LENGTH_DEFAULT = 2048 > DSA_MAX if GPG version <1.4.10 or < 2.0.12.

I did take that case into account -- that's why I only updated the /* Set sane default key length */ code instead of deleting it. If the default is greater than the maximum, it's just set to the maximum.

[patch now includes subkeys' defaults]

Review of attachment 154357 [details] [review]:

Looks ready to commit.  Thank you for your work.

Merged with master.