From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1RZG2O-00089A-0d for mharc-grub-devel@gnu.org; Sat, 10 Dec 2011 01:03:56 -0500 Received: from eggs.gnu.org ([140.186.70.92]:38287) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RZG2K-00088o-IT for grub-devel@gnu.org; Sat, 10 Dec 2011 01:03:53 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RZG2I-0007Sl-A8 for grub-devel@gnu.org; Sat, 10 Dec 2011 01:03:51 -0500 Received: from mail-qy0-f169.google.com ([209.85.216.169]:60792) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RZG2I-0007Sf-5x for grub-devel@gnu.org; Sat, 10 Dec 2011 01:03:50 -0500 Received: by qcsd17 with SMTP id d17so3039916qcs.0 for ; Fri, 09 Dec 2011 22:03:49 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=message-id:date:from:user-agent:mime-version:to:subject:references :in-reply-to:content-type; bh=TCKgLdrtrfCMhdYTlKdNYARGnKzj/eGVf2d9Z71jZQw=; b=oVvwd/j2XppUqCA475RXwUtgtYf1rAEaXWAJzfRUdHADbbbWOCpdnHmKw5d71cfn04 18T4Umq+E65eHcIArW1x+LxZ8s/pR5ouiOnV6tbMAkOvVycWimzQ6ViXFbJzVfnNOtJZ ndGb4sbj6hxkDH0aXxsuu369GaKqMWSMoLFmY= Received: by 10.229.24.139 with SMTP id v11mr2547787qcb.175.1323497027039; Fri, 09 Dec 2011 22:03:47 -0800 (PST) Received: from [127.0.0.1] (pool-74-104-23-109.bstnma.east.verizon.net. [74.104.23.109]) by mx.google.com with ESMTPS id ff9sm18813049qab.16.2011.12.09.22.03.45 (version=SSLv3 cipher=OTHER); Fri, 09 Dec 2011 22:03:46 -0800 (PST) Message-ID: <4EE2F646.1050309@gmail.com> Date: Sat, 10 Dec 2011 01:03:50 -0500 From: Peter Lustig User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0) Gecko/20111105 Thunderbird/8.0 MIME-Version: 1.0 To: grub-devel@gnu.org Subject: Re: New command to check NT's hibernation state References: In-Reply-To: Content-Type: multipart/mixed; boundary="------------080105060202020007030109" X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 2) X-Received-From: 209.85.216.169 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 10 Dec 2011 06:03:54 -0000 This is a multi-part message in MIME format. --------------080105060202020007030109 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit On 12/3/2011 11:41 AM, Vladimir 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. >> */ >> >> #include >> #include >> #include >> #include >> #include >> #include >> #include >> #include >> #include >> #include >> >> 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} >> }; >> > No need for empty array. passing 0 instead of this array works. I have tried it both ways. For some reason, passing the empty array works, but passing a null pointer doesn't. That is, when I try the latter, the options '-u' and '-h' are treated as regular arguments to the command. >> static grub_err_t >> grub_cmd_nthibr (grub_extcmd_context_t ctxt __attribute__ ((unused)), >> int argc, char **args) >> { >> grub_err_t status = GRUB_ERR_NONE; >> char *partition_name, *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) >> { >> status = grub_error (GRUB_ERR_BAD_ARGUMENT, >> N_("too few arguments specified")); >> goto exit; >> } >> else if (argc> 1) >> { >> status = grub_error (GRUB_ERR_BAD_ARGUMENT, >> N_("too many arguments specified")); >> goto exit; >> } >> >> partition_name = args[0]; >> name_length = grub_strlen (partition_name); >> >> /* Check if partition specifier 'looks right' */ >> if (partition_name[0] != '(' || partition_name[name_length - 1] != ')') >> { >> status = grub_error (GRUB_ERR_BAD_FILENAME, >> N_("invalid partition specifier")); >> goto exit; >> } >> > I would recommend adding '(' and ')' if none found rather than giving error. >> /* Check if partition actually exists */ >> partition_name[name_length - 1] = '\0'; >> partition = grub_disk_open (partition_name + 1); >> if (!partition) >> { >> status = grub_error (GRUB_ERR_UNKNOWN_DEVICE, >> N_("partition not found")); > No need to run grub_error here. grub_disk_open already sets grub_errno > and grub_errmsg. Just status = grub_errno is enough. >> goto exit; >> } >> else >> { >> grub_disk_close (partition); >> partition_name[name_length - 1] = ')'; >> } >> > No need for 'else' clause (goto already takes care of it) >> /* Build path to 'hiberfil.sys' */ >> hibr_file_path = grub_malloc (grub_strlen (partition_name) >> + grub_strlen ("/hiberfil.sys") + 1); >> if (!hibr_file_path) >> { >> status = grub_error (GRUB_ERR_OUT_OF_MEMORY, >> N_("out of memory")); >> goto exit; >> } >> else >> { >> grub_strcpy (hibr_file_path, partition_name); >> grub_strcat (hibr_file_path, "/hiberfil.sys"); >> } >> > Same here (no need for grub_error and else) >> /* Try to open 'hiberfil.sys' */ >> hibr_file = grub_file_open (hibr_file_path); >> if (!hibr_file) >> { >> status = grub_error (GRUB_ERR_FILE_NOT_FOUND, >> N_("'hiberfil.sys' not found")); > ditto >> 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) >> { >> status = grub_error (GRUB_ERR_BAD_FILE_TYPE, >> N_("'hiberfil.sys' too small")); > you need to conditionalize call to grub_error. if (!grub_errno) ... > since incomplete read may indicate an FS problem in which case you want > the original error message to remain. >> goto exit; >> } >> >> /* Return SUCCESS if magic indicates file is active; else return FAILURE */ >> if (!grub_strncasecmp ("hibr", hibr_file_magic, magic_size)) >> grub_puts (N_("The system is hibernated.")); >> else >> { >> grub_puts (N_("The system is NOT hibernated.")); >> status = GRUB_ERR_TEST_FAILURE; >> } >> > We also do grub_error (GRUB_ERR_TEST_FAILURE, "false"); >> exit: >> /* Ensure unmanaged resources are cleaned up */ >> if (hibr_file_path) >> grub_free (hibr_file_path); > grub_free already checks for not-NULL pointers. >> if (hibr_file) >> grub_file_close (hibr_file); >> >> return status; > You can eliminate status and the need of keeping one by using return > grub_errno; I was unaware 'grub_errno' was used so extensively. Thanks for the tip: it definitely makes the logic cleaner. >> } > I've followed all your recommendations (as far as possible). Here's the code again. ~Peter Lustig --------------080105060202020007030109 Content-Type: text/plain; name="nthibr.c" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="nthibr.c" /* 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 . */ #include #include #include #include #include #include #include #include #include #include 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} }; 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")); /* 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"); /* 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")); 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); 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); } --------------080105060202020007030109--