On 17.12.2011 00:41, Vladimir 'phcoder' Serbinenko wrote:

> /* nthibr.c - tests whether an MS Windows system partition is
> hibernated */
> /*
>    *  GRUB  --  GRand Unified Bootloader
>    *  Copyright (C) 2007,2008,2009,2011  Free Software Foundation, Inc.
>    *
>    *  GRUB is free software: you can redistribute it and/or modify
>    *  it under the terms of the GNU General Public License as
> published by
>    *  the Free Software Foundation, either version 3 of the License, or
>    *  (at your option) any later version.
>    *
>    *  GRUB is distributed in the hope that it will be useful,
>    *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>    *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>    *  GNU General Public License for more details.
>    *
>    *  You should have received a copy of the GNU General Public License
>    *  along with GRUB.  If not, see<http://www.gnu.org/licenses/>.
>    */
>
> #include<grub/types.h>
> #include<grub/mm.h>
> #include<grub/disk.h>
> #include<grub/file.h>
> #include<grub/misc.h>
> #include<grub/dl.h>
> #include<grub/extcmd.h>
> #include<grub/lib/arg.h>
> #include<grub/err.h>
> #include<grub/i18n.h>
>
> GRUB_MOD_LICENSE("GPLv3+");
>
> /* Define this 'empty' array to let the '-h' and '-u' switches be
> processed */
> static const struct grub_arg_option options[] = {
>     {0, 0, 0, 0, 0, 0}
> };
> Please remove this and use grub_command_t
> I understand now.  What threw me off was seeing the 'hello' module use
> 'extcmd.'
>>> static grub_err_t
>>> grub_cmd_nthibr (grub_extcmd_context_t ctxt __attribute__ ((unused)),
>>>                    int argc, char **args)
>>> {
>>>     char *partition_name = 0, *hibr_file_path = 0, hibr_file_magic[5];
>>>     grub_size_t name_length;
>>>     grub_ssize_t magic_size;
>>>     grub_disk_t partition;
>>>     grub_file_t hibr_file = 0;
>>>
>>>     /* Check argument count */
>>>     if (!argc)
>>>       return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("too few arguments"));
>>>     else if (argc>   1)
>>>       return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("too many
>>> arguments"));
>>>
>> Either ignore trailing argument or put both in one check != 1
>>>     /* Make copy of partition specifier, so it can be modified (if
>>> needed) */
>>>     name_length = grub_strlen (args[0]);
>>>     partition_name = grub_zalloc (name_length + 3);
>>>     if (!partition_name)
>>>       goto exit;
>>>     grub_strcpy (partition_name, args[0]);
>>>
>>>     /* Ensure partition specifier start with a '(' */
>>>     if (partition_name[0] != '(')
>>>       {
>>>         grub_memmove (partition_name + 1, partition_name, name_length++);
>>>         partition_name[0] = '(';
>>>       }
>>>
>>>     /* Ensure partition specifier ends with a ')' */
>>>     if (partition_name[name_length - 1] != ')')
>>>       partition_name[(++name_length) - 1] = ')';
>>>
>>>     /* Check if partition actually exists */
>>>     partition_name[name_length - 1] = '\0';
>>>     partition = grub_disk_open (partition_name + 1);
>>>     if (!partition)
>>>       goto exit;
>>>     grub_disk_close (partition);
>>>     partition_name[name_length - 1] = ')';
>>>
>>>     /* Build path to 'hiberfil.sys' */
>>>     hibr_file_path = grub_malloc ((grub_strlen (partition_name)
>>>                                    + grub_strlen ("/hiberfil.sys") + 1));
>>>     if (!hibr_file_path)
>>>       goto exit;
>>>     grub_strcpy (hibr_file_path, partition_name);
>>>     grub_strcat (hibr_file_path, "/hiberfil.sys");
>>>
>> It would be more efficient if you just try to open file and see what
>> error you get. Actually you don't even need to distinguish between
>> error cases here since you return the error you encountered. This will
>> also save one useless alloc.
>>>     /* Try to open 'hiberfil.sys' */
>>>     hibr_file = grub_file_open (hibr_file_path);
>>>     if (!hibr_file)
>>>       {
>>>         if (grub_errno == GRUB_ERR_FILE_NOT_FOUND)
>>>           grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("'hiberfil.sys' not
>>> found"));
>> This overriding is unnecessary. FS code already provides this message
> This was intentional:  the default message ('file not found') is not
> particularly helpful in this case since the
> command's description nowhere mentions what file it's looking for (let
> alone that any file is needed for it to
> function).  I've updated this section to pop the first error off the
> stack to eliminate the duplication (and added
> a descriptive comment).
>>>         goto exit;
>>>       }
>>>
>>>     /* Try to read magic number of 'hiberfil.sys' */
>>>     magic_size = sizeof (hibr_file_magic) - 1;
>>>     grub_memset (hibr_file_magic, 0, sizeof (hibr_file_magic));
>>>     if (grub_file_read (hibr_file, hibr_file_magic, magic_size)<
>>> magic_size)
>>>       {
>>>         if (!grub_errno)
>>>           grub_error (GRUB_ERR_BAD_FILE_TYPE, N_("'hiberfil.sys' too
>>> small"));
>>>         goto exit;
>>>       }
>>>
>>>     /* Return SUCCESS if magic indicates file is active; else return
>>> FAILURE */
>>>     if (!grub_strncasecmp ("hibr", hibr_file_magic, magic_size))
>>>       grub_error (GRUB_ERR_NONE, "true");
>>>     else
>>>       grub_error (GRUB_ERR_TEST_FAILURE, "false");
>>>
>>> exit:
>>>     /* Ensure unmanaged resources are cleaned up */
>>>     if (partition_name)
>>>       grub_free (partition_name);
>>>     if (hibr_file_path)
>>>       grub_free (hibr_file_path);
>> No need to check that argument to free is non-NULL.
>>>     if (hibr_file)
>>>       grub_file_close (hibr_file);
>>>
>>>     return grub_errno;
>>> }
>>>
>>> static grub_extcmd_t cmd;
>>>
>>> GRUB_MOD_INIT (nthibr)
>>> {
>>>     cmd = grub_register_extcmd ("nthibr", grub_cmd_nthibr, 0,
>>>                                 N_("DEVICE"),
>>>                                 N_("Test whether an NT system partition "
>>>                                    "is hibernated."),
>>>                                 options);
>>> }
>>>
>>> GRUB_MOD_FINI (nthibr)
>>> {
>>>     grub_unregister_extcmd (cmd);
>>> }
>>>
> ~Peter Lustig
> -------------- next part --------------
> An HTML attachment was scrubbed...
> URL:<http://lists.gnu.org/archive/html/grub-devel/attachments/20111217/71127c00/attachment.html>
> -------------- next part --------------
> An embedded and charset-unspecified text was scrubbed...
> Name: nthibr.c
> URL:<http://lists.gnu.org/archive/html/grub-devel/attachments/20111217/71127c00/attachment.c>
>
> ------------------------------
>
> Message: 2
> Date: Sat, 17 Dec 2011 12:39:41 +0100
> From: Vladimir '?-coder/phcoder' Serbinenko 	<phcoder@gmail.com>
> To:grub-devel@gnu.org
> Subject: Re: New command to check NT's hibernation state
> Message-ID:<4EEC7F7D.2050900@gmail.com>
> Content-Type: text/plain; charset=UTF-8; format=flowed
>
>
>>>>     /* Try to open 'hiberfil.sys' */
>>>>     hibr_file = grub_file_open (hibr_file_path);
>>>>     if (!hibr_file)
>>>>       {
>>>>         if (grub_errno == GRUB_ERR_FILE_NOT_FOUND)
>>>>           grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("'hiberfil.sys' not
>>>> found"));
>>> This overriding is unnecessary. FS code already provides this message
>> This was intentional:  the default message ('file not found') is not
>> particularly helpful in this case since the
>> command's description nowhere mentions what file it's looking for (let
>> alone that any file is needed for it to
>> function).  I've updated this section to pop the first error off the
>> stack to eliminate the duplication (and added
>> a descriptive comment).
> This means that your copy of GRUB is old. Newer ones do have the
> descriptive message. And even if it wasn't the case it's something to be
> fixed in FS code, not by replacing error message here.
I've been using the latest release (1.99), so this fix must still be on 
the development branch.  But I do appreciate
the idea of fixing something once at the source instead of at multiple 
points of consumption.